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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2023/06/30 22:56, Ani Sinha wrote:
> 
> 
>> 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.

I sent a patch series to add ARI next function number check:
https://lore.kernel.org/qemu-devel/20230701070133.24877-1-akihiko.odaki@daynix.com/

Yes, I want the slot number restriction to be enforced. If it worries 
you too much for regressions, you may implement it as a warning first 
and then turn it a hard error when the next development phase starts.