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:06 PM, Akihiko Odaki <akihiko.odaki@daynix.com> 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.
> 
> But it should be a distinct check as it will need to check the function 
> number bits.

I will leave this for mst to comment.

> 
>>> 
>>> 
>>>>> 
>>>>>>> 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 ) :-)
> 
> Hello from a guy using a shiny M2 Macbook Air carrying the legacy jack with a 
> 100-yen earphone. Even people who ported Linux to this machine spent efforts 
> to get the jack to work on Linux ;)

Yes that is because the jack exists. If they did not make the jack work, it 
would be a broken device. With bluetooth being so pervasive, its mostly a 
matter of time before most devices stop shipping with a 3.5mm.

All I am saying is sometimes one needs to be bold to bring changes. Otherwise 
changes never come. Status quo is not an option IMHO. 

> 
>>>> 
>>>>> 
>>>>>>> and the benefit
>>>>>>> is not guaranteed.
>>>>>>> 
>>>>>>> -- 
>>>>>>> MST