[PATCH] hw/pci/pci_bridge: ensure PCIe slots have only one slot

[PATCH] hw/pci/pci_bridge: ensure PCIe slots have only one slot

[Roman Kagan]

It's possible to create non-working configurations by attaching a device
to a derivative of PCIe slot (pcie-root-port, ioh3420, etc) and
specifying a slot number other that zero, e.g.:

    -device pcie-root-port,id=s0,... \
    -device virtio-blk-pci,bus=s0,addr=4,...

Make QEMU reject such configurations and only allow addr=0 on the
secondary bus of a PCIe slot.

Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
---
 hw/pci/pci_bridge.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index da34c8ebcd..8b38d5ad3d 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -33,6 +33,7 @@
 #include "qemu/units.h"
 #include "hw/pci/pci_bridge.h"
 #include "hw/pci/pci_bus.h"
+#include "hw/pci/pcie_port.h"
 #include "qemu/module.h"
 #include "qemu/range.h"
 #include "qapi/error.h"
@@ -386,6 +387,10 @@ void pci_bridge_initfn(PCIDevice *dev, const char *typename)
     br->windows = pci_bridge_region_init(br);
     QLIST_INIT(&sec_bus->child);
     QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
+
+    if (PCIE_SLOT(dev)) {
+        sec_bus->slot_reserved_mask = ~1u;
+    }
 }
 
 /* default qdev clean up function for PCI-to-PCI bridge */
-- 
2.36.1

Re: [PATCH] hw/pci/pci_bridge: ensure PCIe slots have only one slot

[Vladimir Sementsov-Ogievskiy]

On 7/4/22 13:25, Roman Kagan wrote:
> It's possible to create non-working configurations by attaching a device
> to a derivative of PCIe slot (pcie-root-port, ioh3420, etc) and
> specifying a slot number other that zero, e.g.:
> 
>      -device pcie-root-port,id=s0,... \
>      -device virtio-blk-pci,bus=s0,addr=4,...
> 
> Make QEMU reject such configurations and only allow addr=0 on the
> secondary bus of a PCIe slot.
> 
> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> ---
>   hw/pci/pci_bridge.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index da34c8ebcd..8b38d5ad3d 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -33,6 +33,7 @@
>   #include "qemu/units.h"
>   #include "hw/pci/pci_bridge.h"
>   #include "hw/pci/pci_bus.h"
> +#include "hw/pci/pcie_port.h"
>   #include "qemu/module.h"
>   #include "qemu/range.h"
>   #include "qapi/error.h"
> @@ -386,6 +387,10 @@ void pci_bridge_initfn(PCIDevice *dev, const char *typename)
>       br->windows = pci_bridge_region_init(br);
>       QLIST_INIT(&sec_bus->child);
>       QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
> +
> +    if (PCIE_SLOT(dev)) {

Hmm, wouldn't PCIE_SLOT just crash if dev is not pcie slot? As I understand, PCIE_SLOT is finally an OBJECT_CHECK(), which say:

  * If an invalid object is passed to this function, a run time assert will be
  * generated.


> +        sec_bus->slot_reserved_mask = ~1u;
> +    }
>   }
>   
>   /* default qdev clean up function for PCI-to-PCI bridge */


-- 
Best regards,
Vladimir

Re: [PATCH] hw/pci/pci_bridge: ensure PCIe slots have only one slot

[Roman Kagan]

On Wed, Jul 06, 2022 at 09:38:39PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 7/4/22 13:25, Roman Kagan wrote:
> > It's possible to create non-working configurations by attaching a device
> > to a derivative of PCIe slot (pcie-root-port, ioh3420, etc) and
> > specifying a slot number other that zero, e.g.:
> > 
> >      -device pcie-root-port,id=s0,... \
> >      -device virtio-blk-pci,bus=s0,addr=4,...
> > 
> > Make QEMU reject such configurations and only allow addr=0 on the
> > secondary bus of a PCIe slot.
> > 
> > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> > ---
> >   hw/pci/pci_bridge.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> > index da34c8ebcd..8b38d5ad3d 100644
> > --- a/hw/pci/pci_bridge.c
> > +++ b/hw/pci/pci_bridge.c
> > @@ -33,6 +33,7 @@
> >   #include "qemu/units.h"
> >   #include "hw/pci/pci_bridge.h"
> >   #include "hw/pci/pci_bus.h"
> > +#include "hw/pci/pcie_port.h"
> >   #include "qemu/module.h"
> >   #include "qemu/range.h"
> >   #include "qapi/error.h"
> > @@ -386,6 +387,10 @@ void pci_bridge_initfn(PCIDevice *dev, const char *typename)
> >       br->windows = pci_bridge_region_init(br);
> >       QLIST_INIT(&sec_bus->child);
> >       QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
> > +
> > +    if (PCIE_SLOT(dev)) {
> 
> Hmm, wouldn't PCIE_SLOT just crash if dev is not pcie slot? As I understand, PCIE_SLOT is finally an OBJECT_CHECK(), which say:
> 
>  * If an invalid object is passed to this function, a run time assert will be
>  * generated.

Well, the assertion is there only if configured with
--enable-qom-cast-debug which is off by default, that's why it even
passed make check.  As it stands, it's just a typecast which is a no-op
here, and basically it makes every bridge have only a single slot, which
is wrong of course.

Will rework, thanks!
Roman.

Re: [PATCH] hw/pci/pci_bridge: ensure PCIe slots have only one slot

[Michael S. Tsirkin]

On Wed, Jul 06, 2022 at 10:43:12PM +0300, Roman Kagan wrote:
> On Wed, Jul 06, 2022 at 09:38:39PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > On 7/4/22 13:25, Roman Kagan wrote:
> > > It's possible to create non-working configurations by attaching a device
> > > to a derivative of PCIe slot (pcie-root-port, ioh3420, etc) and
> > > specifying a slot number other that zero, e.g.:
> > > 
> > >      -device pcie-root-port,id=s0,... \
> > >      -device virtio-blk-pci,bus=s0,addr=4,...
> > > 
> > > Make QEMU reject such configurations and only allow addr=0 on the
> > > secondary bus of a PCIe slot.
> > > 
> > > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> > > ---
> > >   hw/pci/pci_bridge.c | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> > > index da34c8ebcd..8b38d5ad3d 100644
> > > --- a/hw/pci/pci_bridge.c
> > > +++ b/hw/pci/pci_bridge.c
> > > @@ -33,6 +33,7 @@
> > >   #include "qemu/units.h"
> > >   #include "hw/pci/pci_bridge.h"
> > >   #include "hw/pci/pci_bus.h"
> > > +#include "hw/pci/pcie_port.h"
> > >   #include "qemu/module.h"
> > >   #include "qemu/range.h"
> > >   #include "qapi/error.h"
> > > @@ -386,6 +387,10 @@ void pci_bridge_initfn(PCIDevice *dev, const char *typename)
> > >       br->windows = pci_bridge_region_init(br);
> > >       QLIST_INIT(&sec_bus->child);
> > >       QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
> > > +
> > > +    if (PCIE_SLOT(dev)) {
> > 
> > Hmm, wouldn't PCIE_SLOT just crash if dev is not pcie slot? As I understand, PCIE_SLOT is finally an OBJECT_CHECK(), which say:
> > 
> >  * If an invalid object is passed to this function, a run time assert will be
> >  * generated.
> 
> Well, the assertion is there only if configured with
> --enable-qom-cast-debug which is off by default, that's why it even
> passed make check.  As it stands, it's just a typecast which is a no-op
> here, and basically it makes every bridge have only a single slot, which
> is wrong of course.
> 
> Will rework, thanks!
> Roman.

Which probably means it was not actually tested that the patch
rejects the invalid configuration, was it?

-- 
MST

Re: [PATCH] hw/pci/pci_bridge: ensure PCIe slots have only one slot

[Michael S. Tsirkin]

On Mon, Jul 04, 2022 at 01:25:14PM +0300, Roman Kagan wrote:
> It's possible to create non-working configurations by attaching a device
> to a derivative of PCIe slot (pcie-root-port, ioh3420, etc) and
> specifying a slot number other that zero, e.g.:
> 
>     -device pcie-root-port,id=s0,... \
>     -device virtio-blk-pci,bus=s0,addr=4,...
> 
> Make QEMU reject such configurations and only allow addr=0 on the
> secondary bus of a PCIe slot.
> 
> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> ---
>  hw/pci/pci_bridge.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index da34c8ebcd..8b38d5ad3d 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -33,6 +33,7 @@
>  #include "qemu/units.h"
>  #include "hw/pci/pci_bridge.h"
>  #include "hw/pci/pci_bus.h"
> +#include "hw/pci/pcie_port.h"
>  #include "qemu/module.h"
>  #include "qemu/range.h"
>  #include "qapi/error.h"
> @@ -386,6 +387,10 @@ void pci_bridge_initfn(PCIDevice *dev, const char *typename)
>      br->windows = pci_bridge_region_init(br);
>      QLIST_INIT(&sec_bus->child);
>      QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
> +
> +    if (PCIE_SLOT(dev)) {
> +        sec_bus->slot_reserved_mask = ~1u;
> +    }


Please add comments explaining what's going on.

>  }
>  
>  /* default qdev clean up function for PCI-to-PCI bridge */
> -- 
> 2.36.1

Re: [PATCH] hw/pci/pci_bridge: ensure PCIe slots have only one slot

[Roman Kagan]

On Thu, Jul 07, 2022 at 01:19:18AM -0400, Michael S. Tsirkin wrote:
> On Wed, Jul 06, 2022 at 10:43:12PM +0300, Roman Kagan wrote:
> > On Wed, Jul 06, 2022 at 09:38:39PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > On 7/4/22 13:25, Roman Kagan wrote:
> > > > It's possible to create non-working configurations by attaching a device
> > > > to a derivative of PCIe slot (pcie-root-port, ioh3420, etc) and
> > > > specifying a slot number other that zero, e.g.:
> > > > 
> > > >      -device pcie-root-port,id=s0,... \
> > > >      -device virtio-blk-pci,bus=s0,addr=4,...
> > > > 
> > > > Make QEMU reject such configurations and only allow addr=0 on the
> > > > secondary bus of a PCIe slot.
> > > > 
> > > > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> > > > ---
> > > >   hw/pci/pci_bridge.c | 5 +++++
> > > >   1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> > > > index da34c8ebcd..8b38d5ad3d 100644
> > > > --- a/hw/pci/pci_bridge.c
> > > > +++ b/hw/pci/pci_bridge.c
> > > > @@ -33,6 +33,7 @@
> > > >   #include "qemu/units.h"
> > > >   #include "hw/pci/pci_bridge.h"
> > > >   #include "hw/pci/pci_bus.h"
> > > > +#include "hw/pci/pcie_port.h"
> > > >   #include "qemu/module.h"
> > > >   #include "qemu/range.h"
> > > >   #include "qapi/error.h"
> > > > @@ -386,6 +387,10 @@ void pci_bridge_initfn(PCIDevice *dev, const char *typename)
> > > >       br->windows = pci_bridge_region_init(br);
> > > >       QLIST_INIT(&sec_bus->child);
> > > >       QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
> > > > +
> > > > +    if (PCIE_SLOT(dev)) {
> > > 
> > > Hmm, wouldn't PCIE_SLOT just crash if dev is not pcie slot? As I understand, PCIE_SLOT is finally an OBJECT_CHECK(), which say:
> > > 
> > >  * If an invalid object is passed to this function, a run time assert will be
> > >  * generated.
> > 
> > Well, the assertion is there only if configured with
> > --enable-qom-cast-debug which is off by default, that's why it even
> > passed make check.  As it stands, it's just a typecast which is a no-op
> > here, and basically it makes every bridge have only a single slot, which
> > is wrong of course.
> > 
> > Will rework, thanks!
> > Roman.
> 
> Which probably means it was not actually tested that the patch
> rejects the invalid configuration, was it?

Yes it was.  What wasn't tested was that other PCI bridges remained
unaffected.  In the default configuration (--enable-qom-cast-debug=no)
the patch turns every bridge using pci_bridge_initfn into single-slot
bridges.  This renders e.g. switches like x3130 useless, but the
testsuite doesn't trigger that path.

I'll try and add a test for this in the next iteration.

Thanks,
Roman.