1. Patchew
  2. QEMU
  3. [PATCH v2 0/5] test and QEMU fixes to ensure proper PCIE device usage
  4. View patch

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

Ani Sinha posted 5 patches 2 years, 6 months ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <anisinha@redhat.com>, Thomas Huth <thuth@redhat.com>, Laurent Vivier <lvivier@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
There is a newer version of this series
[PATCH v2 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
Posted by Ani Sinha 2 years, 6 months ago 
PCI Express ports only have one slot, so PCI Express devices can only be
plugged into slot 0 on a PCIE port. Enforce it.

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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index bf38905b7d..5f25ab9f5e 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),
@@ -1189,6 +1190,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
                    name);
 
        return NULL;
+    } else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
+        error_setg(errp, "PCI: slot %d is not valid for %s,"
+                   " PCI express devices can only be plugged into slot 0.",
+                   PCI_SLOT(devfn), name);
+        return NULL;
     }
 
     pci_dev->devfn = devfn;
-- 
2.39.1
Re: [PATCH v2 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
Posted by Julia Suvorova 2 years, 6 months ago 
On Thu, Jun 22, 2023 at 12:34 PM Ani Sinha <anisinha@redhat.com> 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.
>
> 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 | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index bf38905b7d..5f25ab9f5e 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),
> @@ -1189,6 +1190,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>                     name);
>
>         return NULL;
> +    } else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
> +        error_setg(errp, "PCI: slot %d is not valid for %s,"
> +                   " PCI express devices can only be plugged into slot 0.",

This is not technically correct, because downstream ports and root
ports are also PCIe devices, and they can have different slots under
upstream ports and RC. But this error will never be shown for them, so
it seems fine.

Reviewed-by: Julia Suvorova <jusual@redhat.com>




> +                   PCI_SLOT(devfn), name);
> +        return NULL;
>      }
>
>      pci_dev->devfn = devfn;
> --
> 2.39.1
>
Re: [PATCH v2 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
Posted by Michael S. Tsirkin 2 years, 5 months ago 
On Thu, Jun 22, 2023 at 05:46:40PM +0200, Julia Suvorova wrote:
> On Thu, Jun 22, 2023 at 12:34 PM Ani Sinha <anisinha@redhat.com> 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.
> >
> > 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 | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index bf38905b7d..5f25ab9f5e 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),
> > @@ -1189,6 +1190,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
> >                     name);
> >
> >         return NULL;
> > +    } else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
> > +        error_setg(errp, "PCI: slot %d is not valid for %s,"
> > +                   " PCI express devices can only be plugged into slot 0.",
> 
> This is not technically correct, because downstream ports and root
> ports are also PCIe devices, and they can have different slots under
> upstream ports and RC. But this error will never be shown for them, so
> it seems fine.

Hmm. Confusing users is not nice ... I agree this might
make people think they can not use root ports in slot !=0 either.

Would you add "with an upstream port"?
E.g. "PCI Express devices with an upstream port" ?

> 
> Reviewed-by: Julia Suvorova <jusual@redhat.com>
> 
> 
> 
> 
> > +                   PCI_SLOT(devfn), name);
> > +        return NULL;
> >      }
> >
> >      pci_dev->devfn = devfn;
> > --
> > 2.39.1
> >
Re: [PATCH v2 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
Posted by Julia Suvorova 2 years, 5 months ago 
On Thu, Jun 22, 2023 at 7:48 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Jun 22, 2023 at 05:46:40PM +0200, Julia Suvorova wrote:
> > On Thu, Jun 22, 2023 at 12:34 PM Ani Sinha <anisinha@redhat.com> 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.
> > >
> > > 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 | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index bf38905b7d..5f25ab9f5e 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),
> > > @@ -1189,6 +1190,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
> > >                     name);
> > >
> > >         return NULL;
> > > +    } else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
> > > +        error_setg(errp, "PCI: slot %d is not valid for %s,"
> > > +                   " PCI express devices can only be plugged into slot 0.",
> >
> > This is not technically correct, because downstream ports and root
> > ports are also PCIe devices, and they can have different slots under
> > upstream ports and RC. But this error will never be shown for them, so
> > it seems fine.
>
> Hmm. Confusing users is not nice ... I agree this might
> make people think they can not use root ports in slot !=0 either.
>
> Would you add "with an upstream port"?
> E.g. "PCI Express devices with an upstream port" ?

This whole upstream port conditioning is quite confusing.
How about "%parent device% only allows plugging into slot 0"?

Best regards, Julia Suvorova.

> >
> > Reviewed-by: Julia Suvorova <jusual@redhat.com>
> >
> >
> >
> >
> > > +                   PCI_SLOT(devfn), name);
> > > +        return NULL;
> > >      }
> > >
> > >      pci_dev->devfn = devfn;
> > > --
> > > 2.39.1
> > >
>
Re: [PATCH v2 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
Posted by Michael S. Tsirkin 2 years, 5 months ago 
On Fri, Jun 23, 2023 at 04:57:20PM +0200, Julia Suvorova wrote:
> On Thu, Jun 22, 2023 at 7:48 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Jun 22, 2023 at 05:46:40PM +0200, Julia Suvorova wrote:
> > > On Thu, Jun 22, 2023 at 12:34 PM Ani Sinha <anisinha@redhat.com> 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.
> > > >
> > > > 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 | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > index bf38905b7d..5f25ab9f5e 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),
> > > > @@ -1189,6 +1190,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
> > > >                     name);
> > > >
> > > >         return NULL;
> > > > +    } else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
> > > > +        error_setg(errp, "PCI: slot %d is not valid for %s,"
> > > > +                   " PCI express devices can only be plugged into slot 0.",
> > >
> > > This is not technically correct, because downstream ports and root
> > > ports are also PCIe devices, and they can have different slots under
> > > upstream ports and RC. But this error will never be shown for them, so
> > > it seems fine.
> >
> > Hmm. Confusing users is not nice ... I agree this might
> > make people think they can not use root ports in slot !=0 either.
> >
> > Would you add "with an upstream port"?
> > E.g. "PCI Express devices with an upstream port" ?
> 
> This whole upstream port conditioning is quite confusing.
> How about "%parent device% only allows plugging into slot 0"?
> 
> Best regards, Julia Suvorova.

Good idea!

> > >
> > > Reviewed-by: Julia Suvorova <jusual@redhat.com>
> > >
> > >
> > >
> > >
> > > > +                   PCI_SLOT(devfn), name);
> > > > +        return NULL;
> > > >      }
> > > >
> > > >      pci_dev->devfn = devfn;
> > > > --
> > > > 2.39.1
> > > >
> >
Re: [PATCH v2 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
Posted by Ani Sinha 2 years, 5 months ago 

> On 26-Jun-2023, at 2:41 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Fri, Jun 23, 2023 at 04:57:20PM +0200, Julia Suvorova wrote:
>> On Thu, Jun 22, 2023 at 7:48 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>> 
>>> On Thu, Jun 22, 2023 at 05:46:40PM +0200, Julia Suvorova wrote:
>>>> On Thu, Jun 22, 2023 at 12:34 PM Ani Sinha <anisinha@redhat.com> 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.
>>>>> 
>>>>> 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 | 6 ++++++
>>>>> 1 file changed, 6 insertions(+)
>>>>> 
>>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>>> index bf38905b7d..5f25ab9f5e 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),
>>>>> @@ -1189,6 +1190,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>>>>>                    name);
>>>>> 
>>>>>        return NULL;
>>>>> +    } else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
>>>>> +        error_setg(errp, "PCI: slot %d is not valid for %s,"
>>>>> +                   " PCI express devices can only be plugged into slot 0.",
>>>> 
>>>> This is not technically correct, because downstream ports and root
>>>> ports are also PCIe devices, and they can have different slots under
>>>> upstream ports and RC. But this error will never be shown for them, so
>>>> it seems fine.
>>> 
>>> Hmm. Confusing users is not nice ... I agree this might
>>> make people think they can not use root ports in slot !=0 either.
>>> 
>>> Would you add "with an upstream port"?
>>> E.g. "PCI Express devices with an upstream port" ?
>> 
>> This whole upstream port conditioning is quite confusing.
>> How about "%parent device% only allows plugging into slot 0"?
>> 
>> Best regards, Julia Suvorova.
> 
> Good idea!

OK I will send out another iteration once Igor has a chance to take a look at the test fixes or someone else reviews them. 

> 
>>>> 
>>>> Reviewed-by: Julia Suvorova <jusual@redhat.com>
>>>> 
>>>> 
>>>> 
>>>> 
>>>>> +                   PCI_SLOT(devfn), name);
>>>>> +        return NULL;
>>>>>     }
>>>>> 
>>>>>     pci_dev->devfn = devfn;
>>>>> --
>>>>> 2.39.1
>>>>> 
>>> 
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.