| Message ID | 20240717085621.55315-1-yaoxt.fnst@fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] pci-bridge: avoid linking a single downstream port more than once | expand |
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);
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
> -----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);
> -----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?
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);
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(+)