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 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:
20230701070133.24877-1-akihiko.odaki@daynix.com/">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.