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

[Akihiko Odaki]

On 2023/06/29 17:05, Ani Sinha wrote:
> On Thu, 29 Jun, 2023, 12:17 pm Akihiko Odaki, <akihiko.odaki@daynix.com 
> <mailto:akihiko.odaki@daynix.com>> wrote:
>
>     On 2023/06/29 13:07, Ani Sinha wrote:
>      > PCI Express ports only have one slot, so PCI Express devices can
>     only be
>      > plugged into slot 0 on a PCIE port. Enforce it.
>      >
>      > The change has been tested to not break ARI by instantiating
>     seven vfs on an
>      > emulated igb device (the maximum number of vfs the linux igb
>     driver supports).
>      > The vfs are seen to have non-zero device/slot numbers in the
>     conventional
>      > PCI BDF representation.
>      >
>      > CC: jusual@redhat.com <mailto:jusual@redhat.com>
>      > CC: imammedo@redhat.com <mailto:imammedo@redhat.com>
>      > CC: akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>
>      >
>      > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
>     <https://bugzilla.redhat.com/show_bug.cgi?id=2128929>
>      > Signed-off-by: Ani Sinha <anisinha@redhat.com
>     <mailto:anisinha@redhat.com>>
>      > Reviewed-by: Julia Suvorova <jusual@redhat.com
>     <mailto:jusual@redhat.com>>
>      > ---
>      >   hw/pci/pci.c | 15 +++++++++++++++
>      >   1 file changed, 15 insertions(+)
>      >
>      > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>      > index e2eb4c3b4a..0320ac2bb3 100644
>      > --- a/hw/pci/pci.c
>      > +++ b/hw/pci/pci.c
>      > @@ -65,6 +65,7 @@ bool pci_available = true;
>      >   static char *pcibus_get_dev_path(DeviceState *dev);
>      >   static char *pcibus_get_fw_dev_path(DeviceState *dev);
>      >   static void pcibus_reset(BusState *qbus);
>      > +static bool pcie_has_upstream_port(PCIDevice *dev);
>      >
>      >   static Property pci_props[] = {
>      >       DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
>      > @@ -1190,6 +1191,20 @@ static PCIDevice
>     *do_pci_register_device(PCIDevice *pci_dev,
>      >                      name);
>      >
>      >          return NULL;
>      > +    } /*
>      > +       * With SRIOV and ARI, vfs can have non-zero slot in the
>     conventional
>      > +       * PCI interpretation as all five bits reserved for slot
>     addresses are
>      > +       * also used for function bits for the various vfs. Ignore
>     that case.
>      > +       * It is too early here to check for ARI capabilities in
>     the PCI config
>      > +       * space. Hence, we check for a vf device instead.
>      > +       */
>
>     Why don't just perform this check after the capabilities are set?
>
>
> We don't want to allocate resources for wrong device parameters. We want 
> to error out early. Other checks also are performed at the same place .

It is indeed better to raise an error as early as possible so that we 
can avoid allocation and other operations that will be reverted and may 
go wrong due to the invalid condition. That should be the reason why 
other checks for the address are performed here.

However, in this particular case, we cannot confidently perform the 
check here because it is unknown if the ARI capability will be 
advertised until the device realization code runs. This can justify 
delaying the check after the device realization, unlike the other checks.

> Show quoted text
>
>
>
>
>     Regards,
>     Akihiko Odaki
>
>      > +    else if (!pci_is_vf(pci_dev) &&
>      > +             pcie_has_upstream_port(pci_dev) &&
>      > +             PCI_SLOT(devfn)) {
>      > +        error_setg(errp, "PCI: slot %d is not valid for %s,"
>      > +                   " parent device only allows plugging into
>     slot 0.",
>      > +                   PCI_SLOT(devfn), name);
>      > +        return NULL;
>      >       }
>      >
>      >       pci_dev->devfn = devfn;