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

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 4 Jul 2023 20:50:49 +0900
Akihiko Odaki <akihiko.odaki@daynix.com> wrote:

> On 2023/07/04 20:38, Igor Mammedov wrote:
> > On Sat, 1 Jul 2023 16:28:30 +0900
> > Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >   
> >> On 2023/07/01 0:29, Michael S. Tsirkin wrote:  
> >>> On Fri, Jun 30, 2023 at 08:36:38PM +0900, 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.  
> > 
> > is it stated somewhere in spec(s) that ARI and !SR-IOV are mutually exclusive?
> >   
> >>>
> >>> Why is that? My understanding is that ARI capable devices should also
> >>> set the multifunction bit in the header. It's not terribly clear from
> >>> the spec though.  
> >>
> >> Something like the following will not work properly with ARI-capable
> >> device (think of a as an ARI-capable device):
> >> -device a,addr=0x1.0x0,multifunction=on -device a,addr=0x1.0x1  
> > (I had a crazy idea, to use it like that so we could put more devices
> > on port without resorting to adding extra bridges)
> > 
> > Can you elaborate some more why it won't work?  
> 
> It won't work because the ARI next function number field is fixed. In 
> this case, the field of the Function at 0x1.0x0 should point to 0x1.0x1, 
> but it doesn't. As the result, the Function at 0x1.0x1 won't be recognized.
> 
> It's more problematic if some of the Functions are ARI-capable but 
> others are not. In my understanding, all Functions in a ARI-capable 
> device need to have ARI capability, but that's not enforced.

that doesn't look to me as an real issue but rather as
a QEMU problem that we could fix and handle it properly.

> >> This is because the next function numbers advertised with ARI are not
> >> updated with the multifunction configuration, but they are hardcoded in
> >> the device implementation. In this sense, the traditional (non-SR/IOV)
> >> multifunction mechanism QEMU has will not work with ARI-capable devices.
> >>  
> >>>      
> >>>> But it should be a distinct check as it will need to check the function
> >>>> number bits.
> >>>>     
> >>>>>     
> >>>>>>
> >>>>>>     
> >>>>>>>>     
> >>>>>>>>>> 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 ;)
> >>>>     
> >>>>>>>     
> >>>>>>>>     
> >>>>>>>>>> and the benefit
> >>>>>>>>>> is not guaranteed.
> >>>>>>>>>>
> >>>>>>>>>> -- 
> >>>>>>>>>> MST  
> >>>>>     
> >>>      
> >>  
> >   
>