From: YASUOKA Masahiko <yasuoka@openbsd.org>
Subject: Re: iavf: panic on QEMU with i440fx chipset emulation
To: naito.yuichiro@gmail.com
Cc: kettenis@openbsd.org, tech@openbsd.org
Date: Mon, 07 Oct 2024 12:02:29 +0900

On Mon, 07 Oct 2024 11:25:00 +0900 (JST)
Yuichiro NAITO <naito.yuichiro@gmail.com> wrote:
> From: YASUOKA Masahiko <yasuoka@openbsd.org>
> Subject: Re: iavf: panic on QEMU with i440fx chipset emulation
> Date: Mon, 07 Oct 2024 10:53:56 +0900 (JST)
> 
>> On Mon, 07 Oct 2024 10:22:00 +0900 (JST)
>> Yuichiro NAITO <naito.yuichiro@gmail.com> wrote:
>>> From: YASUOKA Masahiko <yasuoka@openbsd.org>
>>> Subject: Re: iavf: panic on QEMU with i440fx chipset emulation
>>> Date: Fri, 04 Oct 2024 17:19:28 +0900 (JST)
>>> 
>>>> On Wed, 25 Sep 2024 11:42:02 +0900 (JST)
>>>> Yuichiro NAITO <naito.yuichiro@gmail.com> wrote:
>>>>> Hi, While I'm testing iavf(4) on Linux QEMU with i440fx chipset emulation,
>>>>> I had the following panic message on my OpenBSD current build.
>>>>> I440fx is the default chipset emulation of QEMU. I know q35 chipset
>>>>> emulation is required for msix interrupts. But if users forget to select it,
>>>>> I tested what happens on the default chipset emulation. 
>>>> 
>>>> I think the root problem is this i440fx situation.
>>> 
>>> No, it's a separate issue.
>>> 
>>> I mean the iavf(4) panic occurs if an error happens between
>>> 'iavf_get_vf_resources' and 'if_atatch' in the 'iavf_attach' function.
>>> Looking at the 'iavf_attach' function, there are at least 3 functions
>>> that may cause an error, 'iavf_config_irq_map', 'pci_intr_map_msix',
>>> and 'pci_intr_establish' functions.
>>> The 'pci_intr_map_msix' case is 100% reproducible on the QEMU i440fx
>>> chipset emulation. The other cases are very rare but should be fixed for
>>> stability.
>> 
>> Yes, it might be true if there is a broken hardware which has the same
>> problem of qemu.  We are discussing to have a quirk for qemu.  For the
>> other hardwares than qemu, I think
>> 
>> diff --git a/sys/dev/pci/if_iavf.c b/sys/dev/pci/if_iavf.c
>> index d573d6725f4..e3eadf23904 100644
>> --- a/sys/dev/pci/if_iavf.c
>> +++ b/sys/dev/pci/if_iavf.c
>> @@ -766,6 +766,11 @@ iavf_attach(struct device *parent, struct device *self, void *aux)
>>  	pcireg_t memtype;
>>  	int tries;
>>  
>> +	if (pci_intr_msix_count(pa) <= 0) {
>> +		printf(": no msix interrupt\n");
>> +		return;
>> +	}
>> +
>>  	rw_init(&sc->sc_cfg_lock, "iavfcfg");
>>  
>>  	sc->sc_pc = pa->pa_pc;
>> 
>> this kind of workaround is enough.
> 
> Why don't you imagine another kind of PCI bus error?

Ah, I'm sorry I overlooked what you wanted to do.

I understand your original diff is to fix for general error cases.