From: Stefan Fritsch <sf@openbsd.org>
Subject: Re: PCI amd64: Enables MSI/MSI-X interrupts on QEMU i440fx chipset emulation
To: Jonathan Gray <jsg@jsg.id.au>
Cc: YASUOKA Masahiko <yasuoka@openbsd.org>,  Mark Kettenis <mark.kettenis@xs4all.nl>, naito.yuichiro@gmail.com,  kettenis@openbsd.org, tech@openbsd.org
Date: Tue, 8 Oct 2024 09:22:49 +0200

On Tue, 8 Oct 2024, Jonathan Gray wrote:
> > > sf proposed checking the subsystem vendor id
> > > https://marc.info/?l=openbsd-tech&m=172812592731721&w=2
> > > your diff checks the wrong register, should be PCI_SUBSYS_ID_REG
> > 
> > I am ok with your diff. But freebsd and netbsd have two intel device ids 
> > with that quirk. The other is for vmware, which likely does not have the 
> > qumranet subsystem vendor id. I would also be ok with a diff that does it 
> > in the same way as the other bsds.
> > 
> > I have checked that with your diff, I could remove the quirk in virtio_pci 
> > on qemu and msix still works. However, we disable MSI if the host bridge 
> > is not Intel/AMD/Nvidia. There are many other hypervisors than qemu and I 
> > think this could be a problem if we removed the virtio_pci quirk. Maybe we 
> > should simply trust the acpi tables on hypervisors, see diff below (in 
> > addition to your diff)? Or trust them on all acpi >= 2 systems?
> 
> alternatively add PCI_VENDOR_* tests for vmware and others
> or perhaps invert it
> 
> 	if (PCI_CLASS(class) == PCI_CLASS_BRIDGE &&
> 	    PCI_SUBCLASS(class) != PCI_SUBCLASS_BRIDGE_HOST) {
> 		switch (PCI_VENDOR(id)) {
> 		case PCI_VENDOR_VIATECH:
> 		case PCI_VENDOR_SIS:
> 			pba.pba_flags &= ~PCI_FLAGS_MSI_ENABLED;
> 		}
> 	}

Adding all vendors used by hypervisors will always be incomplete. I am 
fine with inverting the logic, though.

> 	
> > 
> > Cheers,
> > Stefan
> > 
> > diff --git a/sys/arch/amd64/pci/acpipci.c b/sys/arch/amd64/pci/acpipci.c
> > index ecfb0921d46..d845b0e15bc 100644
> > --- a/sys/arch/amd64/pci/acpipci.c
> > +++ b/sys/arch/amd64/pci/acpipci.c
> > @@ -202,16 +202,18 @@ acpipci_attach_bus(struct device *parent, struct acpipci_softc *sc)
> >  		pba.pba_flags |= PCI_FLAGS_MSI_ENABLED;
> >  
> >  	/*
> >  	 * Don't enable MSI on chipsets from low-end manufacturers
> >  	 * like VIA and SiS.  We do this by looking at the host
> >  	 * bridge, which should be device 0 function 0.
> > +	 * Except on hypervisors, there we trust the acpi info.
> >  	 */
> >  	id = pci_conf_read(pba.pba_pc, tag, PCI_ID_REG);
> >  	class = pci_conf_read(pba.pba_pc, tag, PCI_CLASS_REG);
> > -	if (PCI_CLASS(class) == PCI_CLASS_BRIDGE &&
> > +	if ((cpu_ecxfeature & CPUIDECX_HV) == 0 &&
> > +	    PCI_CLASS(class) == PCI_CLASS_BRIDGE &&
> >  	    PCI_SUBCLASS(class) != PCI_SUBCLASS_BRIDGE_HOST &&
> >  	    PCI_VENDOR(id) != PCI_VENDOR_AMD &&
> >  	    PCI_VENDOR(id) != PCI_VENDOR_NVIDIA &&
> >  	    PCI_VENDOR(id) != PCI_VENDOR_INTEL)
> >  		pba.pba_flags &= ~PCI_FLAGS_MSI_ENABLED;
> >  
> > 
> >