Re: [PATCH v4] hw/pci: enforce use of slot only slot 0 when devices have an upstream PCIE port

[Michael S. Tsirkin]

On Tue, Jun 20, 2023 at 10:59:42AM +0200, Igor Mammedov wrote:
> On Tue, 20 Jun 2023 12:48:05 +0530
> Ani Sinha <anisinha@redhat.com> wrote:
> 
> > When a device has an upstream PCIE port, we can only use slot 0. Non-zero 
> > slots
> > are invalid. 
> > This change ensures that we throw an error if the user
> > tries to hotplug a device with an upstream PCIE port to a non-zero slot.
> 
> Isn't the same true for coldplugged devices?
> Why you limit it only to hotplug?
> 
> > 
> > CC: jusual@redhat.com
> > CC: imammedo@redhat.com
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
> > Signed-off-by: Ani Sinha <anisinha@redhat.com>
> > ---
> >  hw/pci/pci.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > changelog:
> > v2: addressed issue with multifunction pcie root ports. Should allow
> > hotplug on functions other than function 0.
> > v3: improved commit message.
> > v4: improve commit message and code comments further. Some more
> > improvements might come in v5. No claims made here that this is
> > the final one :-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index bf38905b7d..30ce6a78cb 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -64,6 +64,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),
> > @@ -1182,6 +1183,11 @@ static PCIDevice *do_pci_register_device(PCIDevice 
> > *pci_dev,
> >      } else if (dev->hotplugged &&
> >                 !pci_is_vf(pci_dev) &&
> >                 pci_get_function_0(pci_dev)) {
> > +        /*
> > +         * populating function 0 triggers a bus scan from the guest that
> > +         * exposes other non-zero functions. Hence we need to ensure that
> > +         * function 0 wasn't added yet.
> > +         */
> >          error_setg(errp, "PCI: slot %d function 0 already occupied by %s,"
> >                     " new func %s cannot be exposed to guest.",
> >                     PCI_SLOT(pci_get_function_0(pci_dev)->devfn),
> > @@ -1189,6 +1195,18 @@ static PCIDevice *do_pci_register_device(PCIDevice 
> > *pci_dev,
> >                     name);
> >  
> >         return NULL;
> > +    } else if (dev->hotplugged &&
> > +               !pci_is_vf(pci_dev) &&
> > +               pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
> > +        /*
> > +         * If the device has an upstream PCIE port, like a pcie root port,
> > +         * we only support functions on slot 0.
> > +         */
> > +        error_setg(errp, "PCI: slot %d is not valid for %s,"
> > +                   " only functions on slot 0 is supported for devices"
> > +                   " with an upstream PCIE port.",
> 
> upstream port language is confusing here and elsewhere you mention it.
> It would be better to use root-port instead.

No i do not think this is specific to root ports.
it is technically any non-integrated express device but we also plug
pci devices into express ports as a hack.
so checking where device is plugged (this is what pcie_has_upstream_port
does) seems like a reasonable approach.

> > +                   PCI_SLOT(devfn), name);
> > +        return NULL;
> >      }
> >  
> >      pci_dev->devfn = devfn;