Re: [PATCH v6 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Ani Sinha]

> On 30-Jun-2023, at 5:25 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> 
> On 2023/06/30 20:36, Akihiko Odaki wrote:
>> On 2023/06/30 19:37, Ani Sinha wrote:
>>> 
>>> 
>>>> On 30-Jun-2023, at 3:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> 
>>>> On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:
>>>>> 
>>>>> 
>>>>>> On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>> 
>>>>>> On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:
>>>>>>> 
>>>>>>> 
>>>>>>>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>> 
>>>>>>>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
>>>>>>>>>> 
>>>>>>>>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() 
>>>>>>>>>> instead of checking ARI capability, and that can happen in 
>>>>>>>>>> do_pci_register_device().
>>>>>>>>>> 
>>>>>>>>>>> Also where do you propose we move the check?
>>>>>>>>>> 
>>>>>>>>>> In pci_qdev_realize(), somewhere after pc->realize() and before 
>>>>>>>>>> option ROM loading.
>>>>>>>>> 
>>>>>>>>> Hmm, I tried this. The issue here is something like this would be now 
>>>>>>>>> allowed since the PF has ARI capability:
>>>>>>>>> 
>>>>>>>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
>>>>>>>>> 
>>>>>>>>> The above should not be allowed and when used, we do not see the igb 
>>>>>>>>> ethernet device from the guest OS.
>>>>>>>> 
>>>>>>>> I think it's allowed because it expects you to hotplug function 0 
>>>>>>>> later,
>>>>>>> 
>>>>>>> This is about the igb device being plugged into the non-zero slot of 
>>>>>>> the pci-root-port. The guest OS ignores it.
>>>>>> 
>>>>>> yes but if you later add a device with ARI and with next field pointing
>>>>>> slot 2 guest will suddently find both.
>>>>> 
>>>>> Hmm, I tried this:
>>>>> 
>>>>> -device pcie-root-port,id=p \
>>>>> -device igb,bus=p,addr=0x2.0x0 \
>>>>> -device igb,bus=p,addr=0x0.0x0 \
>>>>> 
>>>>> The guest only found the second igb device not the first. You can try too.
>>>> 
>>>> Because next parameter in pcie_ari_init does not match.
>>> 
>>> OK send me a command line that I can test it with. I can’t come up with a 
>>> case that actually works in practice.
>> I don't think there is one because the code for PCI multifunction does not 
>> care ARI. In my opinion, we need yet another check to make non-SR-IOV 
>> multifunction and ARI capability mutually exclusive; if a function has the 
>> ARI capability and it is not a VF, an attempt to assign non-zero function 
>> number for it should fail.
> 
> No, the more straightforward way to fix this problem is to check the device 
> function number is less than the next function number advertised with ARI.
> 

Personally I would leave the check for ARI capable devices to someone else. I 
am ok with moving the check to pci_qdev_realize() and I verified that both unit 
tests and the breaking test case for BZ 2128929 is caught. I have also verified 
that the change does not break igb vf generation. 
If there is any interest to push this change, I will spin a new version with 
tags for test fixes added and the rework for this patch with the check moved to 
the new location as you had suggested.


>> But it should be a distinct check as it will need to check the function 
>> number bits.