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

[Xingtao Yao (Fujitsu)]

> -----Original Message-----
> From: Igor Mammedov <imammedo@redhat.com>
> Sent: Thursday, July 25, 2024 4:36 PM
> To: Yao Xingtao via <qemu-devel@nongnu.org>
> Cc: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>; mst@redhat.com;
> marcel.apfelbaum@gmail.com
> Subject: Re: [PATCH v3] pci-bridge: avoid linking a single downstream port 
> more
> than once
> 
> On Wed, 24 Jul 2024 23:27:31 -0400
> Yao Xingtao via <qemu-devel@nongnu.org> 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>
> >
> > ---
> > 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
> > ---
> >  hw/pci/pcie.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 4b2f0805c6e0..aa154ec4b054 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 (pci_is_express_downstream_port(dev) &&
sorry, this check will not be valid since the dev->exp.exp_cap was not set.
I will fix this bug in next revision.

> > +        pcie_find_port_by_pn(pci_get_bus(dev), port)) {
> > +        pos = -EBUSY;
> > +        error_setg(errp, "Can't link port, error %d", pos);
> perhaps, also mention what's wrong and suggest user how to fix 
> mis-configuration
Thanks, will append in next revision.
> 
> > +        return pos;
> > +    }
> > +
> >      pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
> >                               PCI_EXP_VER2_SIZEOF, errp);
> >      if (pos < 0) {