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

[Akihiko Odaki]

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.

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