1. Patchew
  2. QEMU
  3. View series

[PATCH v2] pci-bridge: avoid linking a single downstream port more than once

Yao Xingtao via posted 1 patch 1 month, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240717085621.55315-1-yaoxt.fnst@fujitsu.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
There is a newer version of this series
hw/pci-bridge/cxl_downstream.c     | 5 +++++
hw/pci-bridge/pcie_root_port.c     | 5 +++++
hw/pci-bridge/xio3130_downstream.c | 5 +++++
3 files changed, 15 insertions(+)
[PATCH v2] pci-bridge: avoid linking a single downstream port more than once
Posted by Yao Xingtao via 1 month, 3 weeks ago 
Since the downstream port is not checked, two slots can be linked to
a single port. However, this can prevent the driver from detecting the
device properly.

It is necessary to ensure that a downstream port is not linked more than
once.

Links: https://lore.kernel.org/qemu-devel/OSZPR01MB6453BC61D2FF4035F18084EF8DDC2@OSZPR01MB6453.jpnprd01.prod.outlook.com
Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>

---
V1[1] -> V2:
 - Move downstream port check forward

[1] https://lore.kernel.org/qemu-devel/20240704033834.3362-1-yaoxt.fnst@fujitsu.com
---
 hw/pci-bridge/cxl_downstream.c     | 5 +++++
 hw/pci-bridge/pcie_root_port.c     | 5 +++++
 hw/pci-bridge/xio3130_downstream.c | 5 +++++
 3 files changed, 15 insertions(+)

diff --git a/hw/pci-bridge/cxl_downstream.c b/hw/pci-bridge/cxl_downstream.c
index 742da07a015a..af81ddfeec13 100644
--- a/hw/pci-bridge/cxl_downstream.c
+++ b/hw/pci-bridge/cxl_downstream.c
@@ -142,6 +142,11 @@ static void cxl_dsp_realize(PCIDevice *d, Error **errp)
     MemoryRegion *component_bar = &cregs->component_registers;
     int rc;
 
+    if (pcie_find_port_by_pn(pci_get_bus(d), p->port) != NULL) {
+        error_setg(errp, "Can't link port, error %d", -EBUSY);
+        return;
+    }
+
     pci_bridge_initfn(d, TYPE_PCIE_BUS);
     pcie_port_init_reg(d);
 
diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index 09a34786bc62..a540204bda27 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -67,6 +67,11 @@ static void rp_realize(PCIDevice *d, Error **errp)
     PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d);
     int rc;
 
+    if (pcie_find_port_by_pn(pci_get_bus(d), p->port) != NULL) {
+        error_setg(errp, "Can't link port, error %d", -EBUSY);
+        return;
+    }
+
     pci_config_set_interrupt_pin(d->config, 1);
     if (d->cap_present & QEMU_PCIE_CAP_CXL) {
         pci_bridge_initfn(d, TYPE_CXL_BUS);
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index 907d5105b019..63f6baa615fd 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -69,6 +69,11 @@ static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
     PCIESlot *s = PCIE_SLOT(d);
     int rc;
 
+    if (pcie_find_port_by_pn(pci_get_bus(d), p->port) != NULL) {
+        error_setg(errp, "Can't link port, error %d", -EBUSY);
+        return;
+    }
+
     pci_bridge_initfn(d, TYPE_PCIE_BUS);
     pcie_port_init_reg(d);
 
-- 
2.37.3
Re: [PATCH v2] pci-bridge: avoid linking a single downstream port more than once
Posted by Michael S. Tsirkin 1 month, 3 weeks ago 
On Wed, Jul 17, 2024 at 04:56:21AM -0400, Yao Xingtao wrote:
> Since the downstream port is not checked, two slots can be linked to
> a single port. However, this can prevent the driver from detecting the
> device properly.
> 
> It is necessary to ensure that a downstream port is not linked more than
> once.
> 
> Links: https://lore.kernel.org/qemu-devel/OSZPR01MB6453BC61D2FF4035F18084EF8DDC2@OSZPR01MB6453.jpnprd01.prod.outlook.com
> Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>

You also need to take ARI into account.
That can look like slot != 0.

> ---
> V1[1] -> V2:
>  - Move downstream port check forward
> 
> [1] https://lore.kernel.org/qemu-devel/20240704033834.3362-1-yaoxt.fnst@fujitsu.com
> ---
>  hw/pci-bridge/cxl_downstream.c     | 5 +++++
>  hw/pci-bridge/pcie_root_port.c     | 5 +++++
>  hw/pci-bridge/xio3130_downstream.c | 5 +++++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/hw/pci-bridge/cxl_downstream.c b/hw/pci-bridge/cxl_downstream.c
> index 742da07a015a..af81ddfeec13 100644
> --- a/hw/pci-bridge/cxl_downstream.c
> +++ b/hw/pci-bridge/cxl_downstream.c
> @@ -142,6 +142,11 @@ static void cxl_dsp_realize(PCIDevice *d, Error **errp)
>      MemoryRegion *component_bar = &cregs->component_registers;
>      int rc;
>  
> +    if (pcie_find_port_by_pn(pci_get_bus(d), p->port) != NULL) {
> +        error_setg(errp, "Can't link port, error %d", -EBUSY);
> +        return;
> +    }
> +
>      pci_bridge_initfn(d, TYPE_PCIE_BUS);
>      pcie_port_init_reg(d);
>  
> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> index 09a34786bc62..a540204bda27 100644
> --- a/hw/pci-bridge/pcie_root_port.c
> +++ b/hw/pci-bridge/pcie_root_port.c
> @@ -67,6 +67,11 @@ static void rp_realize(PCIDevice *d, Error **errp)
>      PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d);
>      int rc;
>  
> +    if (pcie_find_port_by_pn(pci_get_bus(d), p->port) != NULL) {
> +        error_setg(errp, "Can't link port, error %d", -EBUSY);
> +        return;
> +    }
> +
>      pci_config_set_interrupt_pin(d->config, 1);
>      if (d->cap_present & QEMU_PCIE_CAP_CXL) {
>          pci_bridge_initfn(d, TYPE_CXL_BUS);
> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> index 907d5105b019..63f6baa615fd 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -69,6 +69,11 @@ static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
>      PCIESlot *s = PCIE_SLOT(d);
>      int rc;
>  
> +    if (pcie_find_port_by_pn(pci_get_bus(d), p->port) != NULL) {
> +        error_setg(errp, "Can't link port, error %d", -EBUSY);
> +        return;
> +    }
> +
>      pci_bridge_initfn(d, TYPE_PCIE_BUS);
>      pcie_port_init_reg(d);
>  
> -- 
> 2.37.3
RE: [PATCH v2] pci-bridge: avoid linking a single downstream port more than once
Posted by Xingtao Yao (Fujitsu) via 1 month, 3 weeks ago 

> -----Original Message-----
> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, July 17, 2024 8:04 PM
> To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>
> Cc: marcel.apfelbaum@gmail.com; qemu-devel@nongnu.org;
> jonathan.cameron@huawei.com
> Subject: Re: [PATCH v2] pci-bridge: avoid linking a single downstream port more
> than once
> 
> On Wed, Jul 17, 2024 at 04:56:21AM -0400, Yao Xingtao wrote:
> > Since the downstream port is not checked, two slots can be linked to
> > a single port. However, this can prevent the driver from detecting the
> > device properly.
> >
> > It is necessary to ensure that a downstream port is not linked more than
> > once.
> >
> > Links:
> https://lore.kernel.org/qemu-devel/OSZPR01MB6453BC61D2FF4035F18084EF8D
> DC2@OSZPR01MB6453.jpnprd01.prod.outlook.com
> > Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
> 
> You also need to take ARI into account.
> That can look like slot != 0.
Sorry, I'm not familiar with the PCIe protocol, could you explain it in detail?
Re: [PATCH v2] pci-bridge: avoid linking a single downstream port more than once
Posted by Philippe Mathieu-Daudé 1 month, 3 weeks ago 
Hi Yao,

On 17/7/24 10:56, Yao Xingtao via wrote:
> Since the downstream port is not checked, two slots can be linked to
> a single port. However, this can prevent the driver from detecting the
> device properly.
> 
> It is necessary to ensure that a downstream port is not linked more than
> once.
> 
> Links: https://lore.kernel.org/qemu-devel/OSZPR01MB6453BC61D2FF4035F18084EF8DDC2@OSZPR01MB6453.jpnprd01.prod.outlook.com
> Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
> 
> ---
> V1[1] -> V2:
>   - Move downstream port check forward
> 
> [1] https://lore.kernel.org/qemu-devel/20240704033834.3362-1-yaoxt.fnst@fujitsu.com
> ---
>   hw/pci-bridge/cxl_downstream.c     | 5 +++++
>   hw/pci-bridge/pcie_root_port.c     | 5 +++++
>   hw/pci-bridge/xio3130_downstream.c | 5 +++++
>   3 files changed, 15 insertions(+)
> 
> diff --git a/hw/pci-bridge/cxl_downstream.c b/hw/pci-bridge/cxl_downstream.c
> index 742da07a015a..af81ddfeec13 100644
> --- a/hw/pci-bridge/cxl_downstream.c
> +++ b/hw/pci-bridge/cxl_downstream.c
> @@ -142,6 +142,11 @@ static void cxl_dsp_realize(PCIDevice *d, Error **errp)
>       MemoryRegion *component_bar = &cregs->component_registers;
>       int rc;
>   
> +    if (pcie_find_port_by_pn(pci_get_bus(d), p->port) != NULL) {
> +        error_setg(errp, "Can't link port, error %d", -EBUSY);
> +        return;

Could pcie_cap_slot_init() be a good place to check for that?

Otherwise IMHO we should add a helper in "hw/pci/pcie.h" and
call it here, not duplicate this code in each model.

> +    }
> +
>       pci_bridge_initfn(d, TYPE_PCIE_BUS);
>       pcie_port_init_reg(d);
RE: [PATCH v2] pci-bridge: avoid linking a single downstream port more than once
Posted by Xingtao Yao (Fujitsu) via 1 month, 3 weeks ago 

> -----Original Message-----
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Sent: Wednesday, July 17, 2024 6:18 PM
> To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>; mst@redhat.com;
> marcel.apfelbaum@gmail.com
> Cc: qemu-devel@nongnu.org; jonathan.cameron@huawei.com
> Subject: Re: [PATCH v2] pci-bridge: avoid linking a single downstream port more
> than once
> 
> Hi Yao,
> 
> On 17/7/24 10:56, Yao Xingtao via wrote:
> > Since the downstream port is not checked, two slots can be linked to
> > a single port. However, this can prevent the driver from detecting the
> > device properly.
> >
> > It is necessary to ensure that a downstream port is not linked more than
> > once.
> >
> > Links:
> https://lore.kernel.org/qemu-devel/OSZPR01MB6453BC61D2FF4035F18084EF8D
> DC2@OSZPR01MB6453.jpnprd01.prod.outlook.com
> > Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
> >
> > ---
> > V1[1] -> V2:
> >   - Move downstream port check forward
> >
> > [1]
> https://lore.kernel.org/qemu-devel/20240704033834.3362-1-yaoxt.fnst@fujitsu.co
> m
> > ---
> >   hw/pci-bridge/cxl_downstream.c     | 5 +++++
> >   hw/pci-bridge/pcie_root_port.c     | 5 +++++
> >   hw/pci-bridge/xio3130_downstream.c | 5 +++++
> >   3 files changed, 15 insertions(+)
> >
> > diff --git a/hw/pci-bridge/cxl_downstream.c
> b/hw/pci-bridge/cxl_downstream.c
> > index 742da07a015a..af81ddfeec13 100644
> > --- a/hw/pci-bridge/cxl_downstream.c
> > +++ b/hw/pci-bridge/cxl_downstream.c
> > @@ -142,6 +142,11 @@ static void cxl_dsp_realize(PCIDevice *d, Error **errp)
> >       MemoryRegion *component_bar = &cregs->component_registers;
> >       int rc;
> >
> > +    if (pcie_find_port_by_pn(pci_get_bus(d), p->port) != NULL) {
> > +        error_setg(errp, "Can't link port, error %d", -EBUSY);
> > +        return;
> 
> Could pcie_cap_slot_init() be a good place to check for that?
> 
> Otherwise IMHO we should add a helper in "hw/pci/pcie.h" and
> call it here, not duplicate this code in each model.
Yes, thanks for your comment.
I think pcie_cap_init() may be better.
> 
> > +    }
> > +
> >       pci_bridge_initfn(d, TYPE_PCIE_BUS);
> >       pcie_port_init_reg(d);

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.