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

> 
> 
>>> 
>>>>> no?
>>>>> 
>>>>> I am quite worried about all this work going into blocking
>>>>> what we think is disallowed configurations. We should have
>>>>> maybe blocked them originally, but now that we didn't
>>>>> there's a non zero chance of regressions,
>>>> 
>>>> Sigh,
>>> 
>>> There's value in patches 1-4 I think - the last patch helped you find
>>> these. so there's value in this work.
>>> 
>>>> no medals here for being brave :-)
>>> 
>>> Try removing support for a 3.5mm jack next. Oh wait ...
>> 
>> Indeed. Everyone uses bluetooth these days. I for one is happy that the jack 
>> is gone (and they were bold enough to do it while Samsung and others still 
>> carry the useless port ) :-)
>> 
>>> 
>>>>> and the benefit
>>>>> is not guaranteed.
>>>>> 
>>>>> -- 
>>>>> MST