From: Jonathan Gray Subject: Re: PCI amd64: Enables MSI/MSI-X interrupts on QEMU i440fx chipset emulation To: Stefan Fritsch Cc: YASUOKA Masahiko , mark.kettenis@xs4all.nl, naito.yuichiro@gmail.com, kettenis@openbsd.org, tech@openbsd.org Date: Tue, 8 Oct 2024 09:45:14 +1100 On Mon, Oct 07, 2024 at 10:58:35PM +0200, Stefan Fritsch wrote: > On Mon, 7 Oct 2024, Jonathan Gray wrote: > > > On Mon, Oct 07, 2024 at 05:04:26PM +0900, YASUOKA Masahiko wrote: > > > On Mon, 07 Oct 2024 09:06:37 +0200 > > > Mark Kettenis wrote: > > > >> Date: Mon, 07 Oct 2024 10:54:14 +0900 (JST) > > > >> From: Yuichiro NAITO > > > >> > > > >> Hi, this patch sets 'PCI_FLAGS_MSI_ENABLED' if the kernel runs on a QEMU > > > >> virtual machine with i440fx chipset emulation. The i440fx is the default > > > >> chipset on the QEMU so users will use it first. > > > >> And then ixv(4) and iavf(4) will fail because MSI-X interrupts are not > > > >> supported by the kernel. > > > >> > > > >> The i440fx is quite old and the ACPI table is revision 1. We cannot know > > > >> if it has MSI capability from the ACPI table. So I add the flag as a quirk. > > > >> > > > >> The same quirk is implemented in NetBSD. > > > >> > > > >> https://github.com/NetBSD/src/blob/affb7619d18b17a184eede63a4823f629a2893fc/sys/arch/x86/pci/pci_machdep.c#L273 > > > >> > > > >> FreeBSD seems to have the quirk but is used as the exception for blacklist > > > >> devices. Probably they see the PCI device capability header of MSI/MSI-X. > > > >> Until we see the same capability, my patch will be useful for the VF driver > > > >> users. > > > >> > > > >> OK? > > > > > > > > I think this should be done using the subsystem vendir/id stuff like > > > > was suggested by someone else in the thread. > > > > > > > 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; } } > > 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; > > > > > > > Index: sys/arch/amd64/pci/acpipci.c > > =================================================================== > > RCS file: /cvs/src/sys/arch/amd64/pci/acpipci.c,v > > diff -u -p -r1.8 acpipci.c > > --- sys/arch/amd64/pci/acpipci.c 13 May 2024 01:15:50 -0000 1.8 > > +++ sys/arch/amd64/pci/acpipci.c 7 Oct 2024 09:27:42 -0000 > > @@ -194,12 +194,18 @@ acpipci_attach_bus(struct device *parent > > (sc->sc_acpi->sc_fadt->iapc_boot_arch & FADT_NO_MSI) == 0) > > pba.pba_flags |= PCI_FLAGS_MSI_ENABLED; > > > > + /* Enable MSI for QEMU claiming ACPI 1.0 */ > > + tag = pci_make_tag(pba.pba_pc, sc->sc_bus, 0, 0); > > + id = pci_conf_read(pba.pba_pc, tag, PCI_SUBSYS_ID_REG); > > + if (sc->sc_acpi->sc_fadt->hdr.revision == 1 && > > + PCI_VENDOR(id) == PCI_VENDOR_QUMRANET) > > + 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. > > */ > > - tag = pci_make_tag(pba.pba_pc, sc->sc_bus, 0, 0); > > 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 && > > Index: sys/arch/i386/pci/pci_machdep.c > > =================================================================== > > RCS file: /cvs/src/sys/arch/i386/pci/pci_machdep.c,v > > diff -u -p -r1.87 pci_machdep.c > > --- sys/arch/i386/pci/pci_machdep.c 11 Mar 2021 11:16:57 -0000 1.87 > > +++ sys/arch/i386/pci/pci_machdep.c 7 Oct 2024 09:26:40 -0000 > > @@ -324,6 +324,11 @@ pci_attach_hook(struct device *parent, s > > break; > > } > > > > + /* Enable MSI for QEMU */ > > + id = pci_conf_read(pc, tag, PCI_SUBSYS_ID_REG); > > + if (PCI_VENDOR(id) == PCI_VENDOR_QUMRANET) > > + pba->pba_flags |= PCI_FLAGS_MSI_ENABLED; > > + > > /* > > * Don't enable MSI on a HyperTransport bus. In order to > > * determine that bus 0 is a HyperTransport bus, we look at > > >