On Mon, 7 Oct 2024 20:42:13 +1100
Jonathan Gray <jsg@jsg.id.au> 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 <mark.kettenis@xs4all.nl> wrote:
>> >> Date: Mon, 07 Oct 2024 10:54:14 +0900 (JST)
>> >> From: Yuichiro NAITO <naito.yuichiro@gmail.com>
>> >> 
>> >> 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.
>> 
>> That was by sf@.
>> I added the check to jsg's diff.  I tested the diff fixes the problem.
>> 
>> 
>> Index: sys/arch/amd64/pci/acpipci.c
>> ===================================================================
>> RCS file: /disk/cvs/openbsd/src/sys/arch/amd64/pci/acpipci.c,v
>> diff -u -p -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 07:51:39 -0000

>> @@ -176,6 +176,7 @@ acpipci_attach_bus(struct device *parent
>>  	struct pcibus_attach_args pba;
>>  	pcitag_t tag;
>>  	pcireg_t id, class;
>> +	int device, maxdevs;
>>  
>>  	memset(&pba, 0, sizeof(pba));
>>  	pba.pba_busname = "pci";
>> @@ -193,6 +194,22 @@ acpipci_attach_bus(struct device *parent
>>  	if (sc->sc_acpi->sc_fadt->hdr.revision >= 2 &&
>>  	    (sc->sc_acpi->sc_fadt->iapc_boot_arch & FADT_NO_MSI) == 0)
>>  		pba.pba_flags |= PCI_FLAGS_MSI_ENABLED;
>> +
>> +	/* For hypervisors claiming ACPI 1.0. */
>> +	if (sc->sc_acpi->sc_fadt->hdr.revision == 1 &&
>> +	    cpu_ecxfeature & CPUIDECX_HV) {
>> +		/* Enable MSI if it's QEMU */
>> +		maxdevs = pci_bus_maxdevs(pba.pba_pc, sc->sc_bus);
>> +		for (device = 0; device < maxdevs; device++) {
>> +			tag = pci_make_tag(pba.pba_pc, sc->sc_bus, device,
>> +			    0);
>> +			id = pci_conf_read(pba.pba_pc, tag, PCI_ID_REG);
>> +			if (PCI_VENDOR(id) == PCI_VENDOR_QUMRANET) {
>> +				pba.pba_flags |= PCI_FLAGS_MSI_ENABLED;
>> +				break;
>> +			}
>> +		}
>> +	}
> 
> 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 didn't understand the message properly..

I tested the diff fixes the problem on amd64.  On i386, since msi
interrupt doesn't seem to be enabled yet, but I verified msi is
enabled properly on the pcibus on i440fx.

ok yasuoka

> 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
>