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

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

[Ani Sinha]

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.

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.",
+                   PCI_SLOT(devfn), name);
+        return NULL;
     }
 
     pci_dev->devfn = devfn;
-- 
2.39.1

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

[Igor Mammedov]

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.
 
> +                   PCI_SLOT(devfn), name);
> +        return NULL;
>      }
>  
>      pci_dev->devfn = devfn;

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 12:48:05PM +0530, Ani Sinha wrote:
> When a device has an upstream PCIE port, we can only use slot 0.

Actually, it's when device is plugged into a PCIE port.
So maybe:

	PCI Express ports only have one slot, so
	PCI Express devices can only be plugged into
	slot 0 on a PCIE port

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

it also adds a comment explaining why function 0 must not exist
when function != 0 is added. or maybe split that part out.

> 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.
> +         */

Pls capitalize populating. Also, comments like this should come
before the logic they document, not after. By the way it doesn't
have to be a bus scan - I'd just say "a scan" - with ACPI
guest knows what was added and can just probe the device functions.

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

why hotplugged? Doesn't the same rule apply to all devices?

> +               !pci_is_vf(pci_dev) &&

Hmm. I think you copied it from here:
    } else if (dev->hotplugged &&
               !pci_is_vf(pci_dev) &&
               pci_get_function_0(pci_dev)) {

it makes sense there because VFs are added later
after PF exists.

But here it makes no sense that I can see.


> +               pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
> +        /*
> +         * If the device has an upstream PCIE port, like a pcie root port,

no, a root port can not be an upstream 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.",


something like:

        error_setg(errp, "PCI: slot %d is not valid for %s:"
                   " PCI Express devices can only be plugged into slot 0")

and then you don't really need a comment.


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

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

[Michael S. Tsirkin]

just noticed a repeated "slot" in the subject btw.

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;

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

[Ani Sinha]

> On 20-Jun-2023, at 4:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Tue, Jun 20, 2023 at 12:48:05PM +0530, Ani Sinha wrote:
>> When a device has an upstream PCIE port, we can only use slot 0.
> 
> Actually, it's when device is plugged into a PCIE port.
> So maybe:
> 
> 	PCI Express ports only have one slot, so
> 	PCI Express devices can only be plugged into
> 	slot 0 on a PCIE port
> 
>> 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.
> 
> it also adds a comment explaining why function 0 must not exist
> when function != 0 is added. or maybe split that part out.
> 
>> 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.
>> +         */
> 
> Pls capitalize populating. Also, comments like this should come
> before the logic they document, not after. By the way it doesn't
> have to be a bus scan - I'd just say "a scan" - with ACPI
> guest knows what was added and can just probe the device functions.
> 
>>         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 &&
> 
> why hotplugged? Doesn't the same rule apply to all devices?
> 
>> +               !pci_is_vf(pci_dev) &&
> 
> Hmm. I think you copied it from here:
>    } else if (dev->hotplugged &&
>               !pci_is_vf(pci_dev) &&
>               pci_get_function_0(pci_dev)) {
> 
> it makes sense there because VFs are added later
> after PF exists.

I thought PFs are handled only in the host OS and only VFs are passthrough into the guest? I thought this check was because VFs have a different domain address separate from other non-vf devices in the guest PCI tree. 

> 
> But here it makes no sense that I can see.
> 
> 
>> +               pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
>> +        /*
>> +         * If the device has an upstream PCIE port, like a pcie root port,
> 
> no, a root port can not be an upstream 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.",
> 
> 
> something like:
> 
>        error_setg(errp, "PCI: slot %d is not valid for %s:"
>                   " PCI Express devices can only be plugged into slot 0")
> 
> and then you don't really need a comment.
> 
> 
>> +                   PCI_SLOT(devfn), name);
>> +        return NULL;
>>     }
>> 
>>     pci_dev->devfn = devfn;
>> -- 
>> 2.39.1

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

[Michael S. Tsirkin]

On Wed, Jun 21, 2023 at 08:09:55AM +0530, Ani Sinha wrote:
> 
> 
> > On 20-Jun-2023, at 4:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 
> > On Tue, Jun 20, 2023 at 12:48:05PM +0530, Ani Sinha wrote:
> >> When a device has an upstream PCIE port, we can only use slot 0.
> > 
> > Actually, it's when device is plugged into a PCIE port.
> > So maybe:
> > 
> > 	PCI Express ports only have one slot, so
> > 	PCI Express devices can only be plugged into
> > 	slot 0 on a PCIE port
> > 
> >> 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.
> > 
> > it also adds a comment explaining why function 0 must not exist
> > when function != 0 is added. or maybe split that part out.
> > 
> >> 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.
> >> +         */
> > 
> > Pls capitalize populating. Also, comments like this should come
> > before the logic they document, not after. By the way it doesn't
> > have to be a bus scan - I'd just say "a scan" - with ACPI
> > guest knows what was added and can just probe the device functions.
> > 
> >>         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 &&
> > 
> > why hotplugged? Doesn't the same rule apply to all devices?
> > 
> >> +               !pci_is_vf(pci_dev) &&
> > 
> > Hmm. I think you copied it from here:
> >    } else if (dev->hotplugged &&
> >               !pci_is_vf(pci_dev) &&
> >               pci_get_function_0(pci_dev)) {
> > 
> > it makes sense there because VFs are added later
> > after PF exists.
> 
> I thought PFs are handled only in the host OS and only VFs are
> passthrough into the guest?

This is emulated SRIOV. host and guest would be nested L2.

> I thought this check was because VFs have
> a different domain address separate from other non-vf devices in the
> guest PCI tree. 

Maybe take a look at the SRIOV spec then.

> > 
> > But here it makes no sense that I can see.
> > 
> > 
> >> +               pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
> >> +        /*
> >> +         * If the device has an upstream PCIE port, like a pcie root port,
> > 
> > no, a root port can not be an upstream 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.",
> > 
> > 
> > something like:
> > 
> >        error_setg(errp, "PCI: slot %d is not valid for %s:"
> >                   " PCI Express devices can only be plugged into slot 0")
> > 
> > and then you don't really need a comment.
> > 
> > 
> >> +                   PCI_SLOT(devfn), name);
> >> +        return NULL;
> >>     }
> >> 
> >>     pci_dev->devfn = devfn;
> >> -- 
> >> 2.39.1

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

[Ani Sinha]

> On 20-Jun-2023, at 4:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Tue, Jun 20, 2023 at 12:48:05PM +0530, Ani Sinha wrote:
>> When a device has an upstream PCIE port, we can only use slot 0.
> 
> Actually, it's when device is plugged into a PCIE port.
> So maybe:
> 
> 	PCI Express ports only have one slot, so
> 	PCI Express devices can only be plugged into
> 	slot 0 on a PCIE port
> 
>> 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.
> 
> it also adds a comment explaining why function 0 must not exist
> when function != 0 is added. or maybe split that part out.
> 
>> 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.
>> +         */
> 
> Pls capitalize populating. Also, comments like this should come
> before the logic they document, not after. By the way it doesn't
> have to be a bus scan - I'd just say "a scan" - with ACPI
> guest knows what was added and can just probe the device functions.
> 
>>         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 &&
> 
> why hotplugged? Doesn't the same rule apply to all devices?
> 
>> +               !pci_is_vf(pci_dev) &&
> 
> Hmm. I think you copied it from here:
>    } else if (dev->hotplugged &&
>               !pci_is_vf(pci_dev) &&
>               pci_get_function_0(pci_dev)) {
> 
> it makes sense there because VFs are added later
> after PF exists.
> 
> But here it makes no sense that I can see.

This patch with these changes causes failures in bios-tables-test which can be fixed easily. It also breaks hd-geo-test and I do not know enough of this test to fix them.

I think I will drop this patch for now.

> 
> 
>> +               pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
>> +        /*
>> +         * If the device has an upstream PCIE port, like a pcie root port,
> 
> no, a root port can not be an upstream 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.",
> 
> 
> something like:
> 
>        error_setg(errp, "PCI: slot %d is not valid for %s:"
>                   " PCI Express devices can only be plugged into slot 0")
> 
> and then you don't really need a comment.
> 
> 
>> +                   PCI_SLOT(devfn), name);
>> +        return NULL;
>>     }
>> 
>>     pci_dev->devfn = devfn;
>> -- 
>> 2.39.1

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

[Ani Sinha]

> On 21-Jun-2023, at 4:36 PM, Ani Sinha <anisinha@redhat.com> wrote:
> 
> 
> 
>> On 20-Jun-2023, at 4:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> 
>> On Tue, Jun 20, 2023 at 12:48:05PM +0530, Ani Sinha wrote:
>>> When a device has an upstream PCIE port, we can only use slot 0.
>> 
>> Actually, it's when device is plugged into a PCIE port.
>> So maybe:
>> 
>> 	PCI Express ports only have one slot, so
>> 	PCI Express devices can only be plugged into
>> 	slot 0 on a PCIE port
>> 
>>> 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.
>> 
>> it also adds a comment explaining why function 0 must not exist
>> when function != 0 is added. or maybe split that part out.
>> 
>>> 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.
>>> +         */
>> 
>> Pls capitalize populating. Also, comments like this should come
>> before the logic they document, not after. By the way it doesn't
>> have to be a bus scan - I'd just say "a scan" - with ACPI
>> guest knows what was added and can just probe the device functions.
>> 
>>>        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 &&
>> 
>> why hotplugged? Doesn't the same rule apply to all devices?
>> 
>>> +               !pci_is_vf(pci_dev) &&
>> 
>> Hmm. I think you copied it from here:
>>   } else if (dev->hotplugged &&
>>              !pci_is_vf(pci_dev) &&
>>              pci_get_function_0(pci_dev)) {
>> 
>> it makes sense there because VFs are added later
>> after PF exists.
>> 
>> But here it makes no sense that I can see.
> 
> This patch with these changes causes failures in bios-tables-test which can be fixed easily. It also breaks hd-geo-test and I do not know enough of this test to fix them.

Specifically it breaks test_override_scsi_q35() and test_override_virtio_blk_q35(). 
I think these tests were wrong to begin with since they attach a pcie-to-pci bridge on a pcie root port and then attach a scsi controller not on the pcie-to-pci bridge but on the root port (which is not possible because we can only attach one device on a non-multifunction pcie root port). Even if I fix them, its failing somewhere else.

I have added Thomas and Laurent, maybe they can help fix these in this test.
I have pushed my patch here: https://gitlab.com/anisinha/qemu/-/commit/24b3eddb968a0739bff222bdf781f722365cc9b2


> 
> I think I will drop this patch for now.
> 
>> 
>> 
>>> +               pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
>>> +        /*
>>> +         * If the device has an upstream PCIE port, like a pcie root port,
>> 
>> no, a root port can not be an upstream 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.",
>> 
>> 
>> something like:
>> 
>>       error_setg(errp, "PCI: slot %d is not valid for %s:"
>>                  " PCI Express devices can only be plugged into slot 0")
>> 
>> and then you don't really need a comment.
>> 
>> 
>>> +                   PCI_SLOT(devfn), name);
>>> +        return NULL;
>>>    }
>>> 
>>>    pci_dev->devfn = devfn;
>>> -- 
>>> 2.39.1

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

[Ani Sinha]

> On 21-Jun-2023, at 4:55 PM, Ani Sinha <anisinha@redhat.com> wrote:
> 
> 
> 
>> On 21-Jun-2023, at 4:36 PM, Ani Sinha <anisinha@redhat.com> wrote:
>> 
>> 
>> 
>>> On 20-Jun-2023, at 4:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> 
>>> On Tue, Jun 20, 2023 at 12:48:05PM +0530, Ani Sinha wrote:
>>>> When a device has an upstream PCIE port, we can only use slot 0.
>>> 
>>> Actually, it's when device is plugged into a PCIE port.
>>> So maybe:
>>> 
>>> 	PCI Express ports only have one slot, so
>>> 	PCI Express devices can only be plugged into
>>> 	slot 0 on a PCIE port
>>> 
>>>> 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.
>>> 
>>> it also adds a comment explaining why function 0 must not exist
>>> when function != 0 is added. or maybe split that part out.
>>> 
>>>> 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.
>>>> +         */
>>> 
>>> Pls capitalize populating. Also, comments like this should come
>>> before the logic they document, not after. By the way it doesn't
>>> have to be a bus scan - I'd just say "a scan" - with ACPI
>>> guest knows what was added and can just probe the device functions.

This commit essentially needs fixing:

commit c46b126088b5616d8b7cd3ff83aaf5d097c36633
Author: Michael Labiuk <michael.labiuk@virtuozzo.com>
Date:   Fri Sep 30 01:35:42 2022 +0300

    tests/x86: Add 'q35' machine type to override-tests in hd-geo-test
    
    Signed-off-by: Michael Labiuk <michael.labiuk@virtuozzo.com>
    Message-Id: <20220929223547.1429580-5-michael.labiuk@virtuozzo.com>
    Signed-off-by: Thomas Huth <thuth@redhat.com>

>>> 
>>>>       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 &&
>>> 
>>> why hotplugged? Doesn't the same rule apply to all devices?
>>> 
>>>> +               !pci_is_vf(pci_dev) &&
>>> 
>>> Hmm. I think you copied it from here:
>>>  } else if (dev->hotplugged &&
>>>             !pci_is_vf(pci_dev) &&
>>>             pci_get_function_0(pci_dev)) {
>>> 
>>> it makes sense there because VFs are added later
>>> after PF exists.
>>> 
>>> But here it makes no sense that I can see.
>> 
>> This patch with these changes causes failures in bios-tables-test which can be fixed easily. It also breaks hd-geo-test and I do not know enough of this test to fix them.
> 
> Specifically it breaks test_override_scsi_q35() and test_override_virtio_blk_q35(). 
> I think these tests were wrong to begin with since they attach a pcie-to-pci bridge on a pcie root port and then attach a scsi controller not on the pcie-to-pci bridge but on the root port (which is not possible because we can only attach one device on a non-multifunction pcie root port). Even if I fix them, its failing somewhere else.
> 
> I have added Thomas and Laurent, maybe they can help fix these in this test.
> I have pushed my patch here: https://gitlab.com/anisinha/qemu/-/commit/24b3eddb968a0739bff222bdf781f722365cc9b2
> 
> 
>> 
>> I think I will drop this patch for now.
>> 
>>> 
>>> 
>>>> +               pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
>>>> +        /*
>>>> +         * If the device has an upstream PCIE port, like a pcie root port,
>>> 
>>> no, a root port can not be an upstream 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.",
>>> 
>>> 
>>> something like:
>>> 
>>>      error_setg(errp, "PCI: slot %d is not valid for %s:"
>>>                 " PCI Express devices can only be plugged into slot 0")
>>> 
>>> and then you don't really need a comment.
>>> 
>>> 
>>>> +                   PCI_SLOT(devfn), name);
>>>> +        return NULL;
>>>>   }
>>>> 
>>>>   pci_dev->devfn = devfn;
>>>> -- 
>>>> 2.39.1

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

[Ani Sinha]

> On 21-Jun-2023, at 5:20 PM, Ani Sinha <anisinha@redhat.com> wrote:
> 
> 
> 
>> On 21-Jun-2023, at 4:55 PM, Ani Sinha <anisinha@redhat.com> wrote:
>> 
>> 
>> 
>>> On 21-Jun-2023, at 4:36 PM, Ani Sinha <anisinha@redhat.com> wrote:
>>> 
>>> 
>>> 
>>>> On 20-Jun-2023, at 4:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> 
>>>> On Tue, Jun 20, 2023 at 12:48:05PM +0530, Ani Sinha wrote:
>>>>> When a device has an upstream PCIE port, we can only use slot 0.
>>>> 
>>>> Actually, it's when device is plugged into a PCIE port.
>>>> So maybe:
>>>> 
>>>> 	PCI Express ports only have one slot, so
>>>> 	PCI Express devices can only be plugged into
>>>> 	slot 0 on a PCIE port
>>>> 
>>>>> 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.
>>>> 
>>>> it also adds a comment explaining why function 0 must not exist
>>>> when function != 0 is added. or maybe split that part out.
>>>> 
>>>>> 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.
>>>>> +         */
>>>> 
>>>> Pls capitalize populating. Also, comments like this should come
>>>> before the logic they document, not after. By the way it doesn't
>>>> have to be a bus scan - I'd just say "a scan" - with ACPI
>>>> guest knows what was added and can just probe the device functions.
> 
> This commit essentially needs fixing:
> 
> commit c46b126088b5616d8b7cd3ff83aaf5d097c36633
> Author: Michael Labiuk <michael.labiuk@virtuozzo.com>
> Date:   Fri Sep 30 01:35:42 2022 +0300
> 
>    tests/x86: Add 'q35' machine type to override-tests in hd-geo-test
> 
>    Signed-off-by: Michael Labiuk <michael.labiuk@virtuozzo.com>
>    Message-Id: <20220929223547.1429580-5-michael.labiuk@virtuozzo.com>
>    Signed-off-by: Thomas Huth <thuth@redhat.com>

I have sent a patch series with a proposed fix for this test (hd-geo-test) as well as fixes for bios-tables-test. The series also includes my QEMU patch that enforces proper PCIE slot usage.
The patches are part of one series and should be applied in the same sequence - test fixes first then the QEMU fix so that the QEMU fix does not break the unit tests.
Please review.

> 
>>>> 
>>>>>      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 &&
>>>> 
>>>> why hotplugged? Doesn't the same rule apply to all devices?
>>>> 
>>>>> +               !pci_is_vf(pci_dev) &&
>>>> 
>>>> Hmm. I think you copied it from here:
>>>> } else if (dev->hotplugged &&
>>>>            !pci_is_vf(pci_dev) &&
>>>>            pci_get_function_0(pci_dev)) {
>>>> 
>>>> it makes sense there because VFs are added later
>>>> after PF exists.
>>>> 
>>>> But here it makes no sense that I can see.
>>> 
>>> This patch with these changes causes failures in bios-tables-test which can be fixed easily. It also breaks hd-geo-test and I do not know enough of this test to fix them.
>> 
>> Specifically it breaks test_override_scsi_q35() and test_override_virtio_blk_q35(). 
>> I think these tests were wrong to begin with since they attach a pcie-to-pci bridge on a pcie root port and then attach a scsi controller not on the pcie-to-pci bridge but on the root port (which is not possible because we can only attach one device on a non-multifunction pcie root port). Even if I fix them, its failing somewhere else.
>> 
>> I have added Thomas and Laurent, maybe they can help fix these in this test.
>> I have pushed my patch here: https://gitlab.com/anisinha/qemu/-/commit/24b3eddb968a0739bff222bdf781f722365cc9b2
>> 
>> 
>>> 
>>> I think I will drop this patch for now.
>>> 
>>>> 
>>>> 
>>>>> +               pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
>>>>> +        /*
>>>>> +         * If the device has an upstream PCIE port, like a pcie root port,
>>>> 
>>>> no, a root port can not be an upstream 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.",
>>>> 
>>>> 
>>>> something like:
>>>> 
>>>>     error_setg(errp, "PCI: slot %d is not valid for %s:"
>>>>                " PCI Express devices can only be plugged into slot 0")
>>>> 
>>>> and then you don't really need a comment.
>>>> 
>>>> 
>>>>> +                   PCI_SLOT(devfn), name);
>>>>> +        return NULL;
>>>>>  }
>>>>> 
>>>>>  pci_dev->devfn = devfn;
>>>>> -- 
>>>>> 2.39.1