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

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

Posted by Yao Xingtao via 1 year, 6 months 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>

---
V3[3] -> V4:
 - make the error message more readable
 - fix the downstream port check error

V2[2] -> V3:
 - Move this check into pcie_cap_init()

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

[1] https://lore.kernel.org/qemu-devel/20240704033834.3362-1-yaoxt.fnst@fujitsu.com
[2] https://lore.kernel.org/qemu-devel/20240717085621.55315-1-yaoxt.fnst@fujitsu.com
[3] https://lore.kernel.org/qemu-devel/20240725032731.13032-1-yaoxt.fnst@fujitsu.com
---
 hw/pci/pcie.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 4b2f0805c6e0..1e53be1bc7c5 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -192,6 +192,13 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset,
 
     assert(pci_is_express(dev));
 
+    if ((type == PCI_EXP_TYPE_DOWNSTREAM || type == PCI_EXP_TYPE_ROOT_PORT) &&
+        pcie_find_port_by_pn(pci_get_bus(dev), port)) {
+        error_setg(errp, "The port %d is already in use, please select "
+                   "another port", port);
+        return -EBUSY;
+    }
+
     pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
                              PCI_EXP_VER2_SIZEOF, errp);
     if (pos < 0) {
-- 
2.41.0

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

Posted by Michael S. Tsirkin 1 year, 4 months ago

On Thu, Jul 25, 2024 at 05:38:19AM -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>
> 
> ---
> V3[3] -> V4:
>  - make the error message more readable
>  - fix the downstream port check error
> 
> V2[2] -> V3:
>  - Move this check into pcie_cap_init()
> 
> V1[1] -> V2:
>  - Move downstream port check forward
> 
> [1] https://lore.kernel.org/qemu-devel/20240704033834.3362-1-yaoxt.fnst@fujitsu.com
> [2] https://lore.kernel.org/qemu-devel/20240717085621.55315-1-yaoxt.fnst@fujitsu.com
> [3] https://lore.kernel.org/qemu-devel/20240725032731.13032-1-yaoxt.fnst@fujitsu.com
> ---
>  hw/pci/pcie.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 4b2f0805c6e0..1e53be1bc7c5 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -192,6 +192,13 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset,
>  
>      assert(pci_is_express(dev));
>  
> +    if ((type == PCI_EXP_TYPE_DOWNSTREAM || type == PCI_EXP_TYPE_ROOT_PORT) &&
> +        pcie_find_port_by_pn(pci_get_bus(dev), port)) {
> +        error_setg(errp, "The port %d is already in use, please select "
> +                   "another port", port);
> +        return -EBUSY;
> +    }
> +
>      pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
>                               PCI_EXP_VER2_SIZEOF, errp);
>      if (pos < 0) {


But can't there be two functions of a multi-function device,
sharing a port?

> -- 
> 2.41.0

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

Posted by Xingtao Yao (Fujitsu) via 1 year, 4 months ago


> -----Original Message-----
> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, September 10, 2024 11:17 PM
> To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>
> Cc: marcel.apfelbaum@gmail.com; qemu-devel@nongnu.org
> Subject: Re: [PATCH v4] pci-bridge: avoid linking a single downstream port more
> than once
> 
> On Thu, Jul 25, 2024 at 05:38:19AM -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>
> >
> > ---
> > V3[3] -> V4:
> >  - make the error message more readable
> >  - fix the downstream port check error
> >
> > V2[2] -> V3:
> >  - Move this check into pcie_cap_init()
> >
> > V1[1] -> V2:
> >  - Move downstream port check forward
> >
> > [1]
> https://lore.kernel.org/qemu-devel/20240704033834.3362-1-yaoxt.fnst@fujitsu.co
> m
> > [2]
> https://lore.kernel.org/qemu-devel/20240717085621.55315-1-yaoxt.fnst@fujitsu.c
> om
> > [3]
> https://lore.kernel.org/qemu-devel/20240725032731.13032-1-yaoxt.fnst@fujitsu.c
> om
> > ---
> >  hw/pci/pcie.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 4b2f0805c6e0..1e53be1bc7c5 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -192,6 +192,13 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset,
> >
> >      assert(pci_is_express(dev));
> >
> > +    if ((type == PCI_EXP_TYPE_DOWNSTREAM || type ==
> PCI_EXP_TYPE_ROOT_PORT) &&
> > +        pcie_find_port_by_pn(pci_get_bus(dev), port)) {
> > +        error_setg(errp, "The port %d is already in use, please select "
> > +                   "another port", port);
> > +        return -EBUSY;
> > +    }
> > +
> >      pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
> >                               PCI_EXP_VER2_SIZEOF, errp);
> >      if (pos < 0) {
> 
> 
> But can't there be two functions of a multi-function device,
> sharing a port?
Good question. 
But I am not good at PCIe protocol, can anyone give me some advice when using the
mulit-function feature?
> 
> > --
> > 2.41.0

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

Posted by Xingtao Yao (Fujitsu) via 1 year, 5 months ago

ping.

> -----Original Message-----
> From: Yao Xingtao <yaoxt.fnst@fujitsu.com>
> Sent: Thursday, July 25, 2024 5:38 PM
> To: mst@redhat.com; marcel.apfelbaum@gmail.com
> Cc: qemu-devel@nongnu.org; Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>
> Subject: [PATCH v4] pci-bridge: avoid linking a single downstream port more than
> once
> 
> 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>
> 
> ---
> V3[3] -> V4:
>  - make the error message more readable
>  - fix the downstream port check error
> 
> V2[2] -> V3:
>  - Move this check into pcie_cap_init()
> 
> V1[1] -> V2:
>  - Move downstream port check forward
> 
> [1]
> https://lore.kernel.org/qemu-devel/20240704033834.3362-1-yaoxt.fnst@fujitsu.co
> m
> [2]
> https://lore.kernel.org/qemu-devel/20240717085621.55315-1-yaoxt.fnst@fujitsu.c
> om
> [3]
> https://lore.kernel.org/qemu-devel/20240725032731.13032-1-yaoxt.fnst@fujitsu.c
> om
> ---
>  hw/pci/pcie.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 4b2f0805c6e0..1e53be1bc7c5 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -192,6 +192,13 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset,
> 
>      assert(pci_is_express(dev));
> 
> +    if ((type == PCI_EXP_TYPE_DOWNSTREAM || type ==
> PCI_EXP_TYPE_ROOT_PORT) &&
> +        pcie_find_port_by_pn(pci_get_bus(dev), port)) {
> +        error_setg(errp, "The port %d is already in use, please select "
> +                   "another port", port);
> +        return -EBUSY;
> +    }
> +
>      pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
>                               PCI_EXP_VER2_SIZEOF, errp);
>      if (pos < 0) {
> --
> 2.41.0