[PATCH v2 0/5] test and QEMU fixes to ensure proper PCIE device usage

[PATCH v2 0/5] test and QEMU fixes to ensure proper PCIE device usage

[Ani Sinha]

Patches 1-4:
Fix tests so that devices do not use non-zero slots on the pcie root
ports. PCIE ports only have one slot, so PCIE devices can only be
plugged into slot 0 on a PCIE port.

Patch 5:
Enforce only one slot on PCIE port.

The test fixes must be applied before the QEMU change that checks for use
of a single slot in PCIE port.

CC: mst@redhat.com
CC: imammedo@redhat.com
CC: jusual@redhat.com
CC: thuth@redhat.com
CC: lvivier@redhat.com
CC: michael.labiuk@virtuozzo.com

Changelog:
v2: add hd-geo-test fix as well as the actual QEMU code fix to the patchset.
The patches are added in the right order.

Ani Sinha (5):
  tests/acpi: allow changes in DSDT.noacpihp table blob
  tests/acpi/bios-tables-test: use the correct slot on the
    pcie-root-port
  tests/acpi/bios-tables-test: update acpi blob q35/DSDT.noacpihp
  tests/qtest/hd-geo-test: fix test by removing unnecessary
    pcie-root-port
  hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

 hw/pci/pci.c                      |   6 ++++++
 tests/data/acpi/q35/DSDT.noacpihp | Bin 8248 -> 8241 bytes
 tests/qtest/bios-tables-test.c    |   4 ++--
 tests/qtest/hd-geo-test.c         |  18 ++++++++----------
 4 files changed, 16 insertions(+), 12 deletions(-)

-- 
2.39.1

[PATCH v2 1/5] tests/acpi: allow changes in DSDT.noacpihp table blob

[Ani Sinha]

We are going to fix bio-tables-test in the next patch and hence need to
make sure the acpi tests continue to pass.

Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 tests/qtest/bios-tables-test-allowed-diff.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..31df9c6187 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,2 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/q35/DSDT.noacpihp",
-- 
2.39.1

[PATCH v2 2/5] tests/acpi/bios-tables-test: use the correct slot on the pcie-root-port

[Ani Sinha]

PCIE ports only have one slot, slot 0. Hence, non-zero slots are not available
for PCIE devices on PCIE root ports. Fix test_acpi_q35_tcg_no_acpi_hotplug()
so that the test does not use them.

Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 tests/qtest/bios-tables-test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index ed1c69cf01..47ba20b957 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -1020,9 +1020,9 @@ static void test_acpi_q35_tcg_no_acpi_hotplug(void)
         " -device pci-testdev,bus=nohprp,acpi-index=501"
         " -device pcie-root-port,id=nohprpint,port=0x0,chassis=3,hotplug=off,"
                                  "multifunction=on,addr=8.0"
-        " -device pci-testdev,bus=nohprpint,acpi-index=601,addr=8.1"
+        " -device pci-testdev,bus=nohprpint,acpi-index=601,addr=0.1"
         " -device pcie-root-port,id=hprp2,port=0x0,chassis=4,bus=nohprpint,"
-                                 "addr=9.0"
+                                 "addr=0.2"
         " -device pci-testdev,bus=hprp2,acpi-index=602"
         , &data);
     free_test_data(&data);
-- 
2.39.1

[PATCH v2 3/5] tests/acpi/bios-tables-test: update acpi blob q35/DSDT.noacpihp

[Ani Sinha]

Some fixes were committed in bios-tables-test in the previous commit. Update
the acpi blob and clear bios-tables-test-allowed-diff.h so that the test
continues to pass with the changes in the bios-tables-test.

Following is the asl diff between the old and the newly updated blob:

@@ -1,30 +1,30 @@
 /*
  * Intel ACPI Component Architecture
  * AML/ASL+ Disassembler version 20210604 (64-bit version)
  * Copyright (c) 2000 - 2021 Intel Corporation
  *
  * Disassembling to symbolic ASL+ operators
  *
- * Disassembly of tests/data/acpi/q35/DSDT.noacpihp, Wed Jun 21 18:26:52 2023
+ * Disassembly of /tmp/aml-O8SU61, Wed Jun 21 18:26:52 2023
  *
  * Original Table Header:
  *     Signature        "DSDT"
- *     Length           0x00002038 (8248)
+ *     Length           0x00002031 (8241)
  *     Revision         0x01 **** 32-bit table (V1), no 64-bit math support
- *     Checksum         0x4A
+ *     Checksum         0x89
  *     OEM ID           "BOCHS "
  *     OEM Table ID     "BXPC    "
  *     OEM Revision     0x00000001 (1)
  *     Compiler ID      "BXPC"
  *     Compiler Version 0x00000001 (1)
  */
 DefinitionBlock ("", "DSDT", 1, "BOCHS ", "BXPC    ", 0x00000001)
 {
     Scope (\)
     {
         OperationRegion (DBG, SystemIO, 0x0402, One)
         Field (DBG, ByteAcc, NoLock, Preserve)
         {
             DBGB,   8
         }

@@ -3148,48 +3148,48 @@
                 {
                     Name (_ADR, Zero)  // _ADR: Address
                     Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                     {
                         Local0 = Package (0x01)
                             {
                                 0x01F5
                             }
                         Return (EDSM (Arg0, Arg1, Arg2, Arg3, Local0))
                     }
                 }
             }

             Device (S40)
             {
                 Name (_ADR, 0x00080000)  // _ADR: Address
-                Device (S41)
+                Device (S01)
                 {
-                    Name (_ADR, 0x00080001)  // _ADR: Address
+                    Name (_ADR, One)  // _ADR: Address
                     Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                     {
                         Local0 = Package (0x01)
                             {
                                 0x0259
                             }
                         Return (EDSM (Arg0, Arg1, Arg2, Arg3, Local0))
                     }
                 }

-                Device (S48)
+                Device (S02)
                 {
-                    Name (_ADR, 0x00090000)  // _ADR: Address
+                    Name (_ADR, 0x02)  // _ADR: Address
                     Device (S00)
                     {
                         Name (_ADR, Zero)  // _ADR: Address
                     }
                 }
             }

             Device (SF8)
             {
                 Name (_ADR, 0x001F0000)  // _ADR: Address
                 OperationRegion (PIRQ, PCI_Config, 0x60, 0x0C)
                 Scope (\_SB)
                 {
                     Field (PCI0.SF8.PIRQ, ByteAcc, NoLock, Preserve)
                     {
                         PRQA,   8,

Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 tests/data/acpi/q35/DSDT.noacpihp           | Bin 8248 -> 8241 bytes
 tests/qtest/bios-tables-test-allowed-diff.h |   1 -
 2 files changed, 1 deletion(-)

diff --git a/tests/data/acpi/q35/DSDT.noacpihp b/tests/data/acpi/q35/DSDT.noacpihp
index 6ab1f0e52543fcb7f84a7fd1327fe5aa42010565..8cab2f8eb9ae94e0165f3f17857ec7d080fb0e13 100644
GIT binary patch
delta 109
zcmdntu+f3bCD<jzP=SGgv2!Dri!7J3UQB$jQ@nt;?&b(tDMlAZ)?gEZc#e2SmmnSn
z1`dYkCY4|VLx=#Qh(x?gurE)65Gx~hBvZl?S0FDVGb=kGx=AwFzzCv>i)r&-xoSoL
DyqFtK

delta 94
zcmdn!u)~4NCD<jzLV<yS(Q6}@i!7IyUQB$jQ@nta-sT8dDMm#P)?gEZc#e2SmmnSn
k1`dYkCXHYdL#O~FP+)SuoHV~ou!#j+5huguZF1F&02bsG6#xJL

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 31df9c6187..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,2 +1 @@
 /* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/q35/DSDT.noacpihp",
-- 
2.39.1

[PATCH v2 4/5] tests/qtest/hd-geo-test: fix test by removing unnecessary pcie-root-port

[Ani Sinha]

A SCSI controller can be attached to a pcie-to-pci bridge which in turn can be
attached directly to the root bus (peie.0). There is no need to attach a
pcie-root-port on the root bus in order to attach the pcie-ro-pci bridge.
Fix it.

CC: mst@redhat.com
CC: imammedo@redhat.com
CC: Michael Labiuk <michael.labiuk@virtuozzo.com>

Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 tests/qtest/hd-geo-test.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/tests/qtest/hd-geo-test.c b/tests/qtest/hd-geo-test.c
index 5aa258a2b3..d08bffad91 100644
--- a/tests/qtest/hd-geo-test.c
+++ b/tests/qtest/hd-geo-test.c
@@ -784,14 +784,12 @@ static void test_override_scsi(void)
     test_override(args, "pc", expected);
 }
 
-static void setup_pci_bridge(TestArgs *args, const char *id, const char *rootid)
+static void setup_pci_bridge(TestArgs *args, const char *id)
 {
 
-    char *root, *br;
-    root = g_strdup_printf("-device pcie-root-port,id=%s", rootid);
-    br = g_strdup_printf("-device pcie-pci-bridge,bus=%s,id=%s", rootid, id);
+    char *br;
+    br = g_strdup_printf("-device pcie-pci-bridge,bus=pcie.0,id=%s", id);
 
-    args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, root);
     args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, br);
 }
 
@@ -811,8 +809,8 @@ static void test_override_scsi_q35(void)
     add_drive_with_mbr(args, empty_mbr, 1);
     add_drive_with_mbr(args, empty_mbr, 1);
     add_drive_with_mbr(args, empty_mbr, 1);
-    setup_pci_bridge(args, "pcie.0", "br");
-    add_scsi_controller(args, "lsi53c895a", "br", 3);
+    setup_pci_bridge(args, "pcie-pci-br");
+    add_scsi_controller(args, "lsi53c895a", "pcie-pci-br", 3);
     add_scsi_disk(args, 0, 0, 0, 0, 0, 10000, 120, 30);
     add_scsi_disk(args, 1, 0, 0, 1, 0, 9000, 120, 30);
     add_scsi_disk(args, 2, 0, 0, 2, 0, 1, 0, 0);
@@ -868,9 +866,9 @@ static void test_override_virtio_blk_q35(void)
     };
     add_drive_with_mbr(args, empty_mbr, 1);
     add_drive_with_mbr(args, empty_mbr, 1);
-    setup_pci_bridge(args, "pcie.0", "br");
-    add_virtio_disk(args, 0, "br", 3, 10000, 120, 30);
-    add_virtio_disk(args, 1, "br", 4, 9000, 120, 30);
+    setup_pci_bridge(args, "pcie-pci-br");
+    add_virtio_disk(args, 0, "pcie-pci-br", 3, 10000, 120, 30);
+    add_virtio_disk(args, 1, "pcie-pci-br", 4, 9000, 120, 30);
     test_override(args, "q35", expected);
 }
 
-- 
2.39.1

[PATCH v2 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Ani Sinha]

PCI Express ports only have one slot, so PCI Express devices can only be
plugged into slot 0 on a PCIE port. Enforce it.

CC: jusual@redhat.com
CC: imammedo@redhat.com
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 hw/pci/pci.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index bf38905b7d..5f25ab9f5e 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -64,6 +64,7 @@ bool pci_available = true;
 static char *pcibus_get_dev_path(DeviceState *dev);
 static char *pcibus_get_fw_dev_path(DeviceState *dev);
 static void pcibus_reset(BusState *qbus);
+static bool pcie_has_upstream_port(PCIDevice *dev);
 
 static Property pci_props[] = {
     DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
@@ -1189,6 +1190,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
                    name);
 
        return NULL;
+    } else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
+        error_setg(errp, "PCI: slot %d is not valid for %s,"
+                   " PCI express devices can only be plugged into slot 0.",
+                   PCI_SLOT(devfn), name);
+        return NULL;
     }
 
     pci_dev->devfn = devfn;
-- 
2.39.1

Re: [PATCH v2 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Julia Suvorova]

On Thu, Jun 22, 2023 at 12:34 PM Ani Sinha <anisinha@redhat.com> wrote:
>
> PCI Express ports only have one slot, so PCI Express devices can only be
> plugged into slot 0 on a PCIE port. Enforce it.
>
> CC: jusual@redhat.com
> CC: imammedo@redhat.com
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> ---
>  hw/pci/pci.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index bf38905b7d..5f25ab9f5e 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -64,6 +64,7 @@ bool pci_available = true;
>  static char *pcibus_get_dev_path(DeviceState *dev);
>  static char *pcibus_get_fw_dev_path(DeviceState *dev);
>  static void pcibus_reset(BusState *qbus);
> +static bool pcie_has_upstream_port(PCIDevice *dev);
>
>  static Property pci_props[] = {
>      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> @@ -1189,6 +1190,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>                     name);
>
>         return NULL;
> +    } else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
> +        error_setg(errp, "PCI: slot %d is not valid for %s,"
> +                   " PCI express devices can only be plugged into slot 0.",

This is not technically correct, because downstream ports and root
ports are also PCIe devices, and they can have different slots under
upstream ports and RC. But this error will never be shown for them, so
it seems fine.

Reviewed-by: Julia Suvorova <jusual@redhat.com>




> +                   PCI_SLOT(devfn), name);
> +        return NULL;
>      }
>
>      pci_dev->devfn = devfn;
> --
> 2.39.1
>

Re: [PATCH v2 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Michael S. Tsirkin]

On Thu, Jun 22, 2023 at 05:46:40PM +0200, Julia Suvorova wrote:
> On Thu, Jun 22, 2023 at 12:34 PM Ani Sinha <anisinha@redhat.com> wrote:
> >
> > PCI Express ports only have one slot, so PCI Express devices can only be
> > plugged into slot 0 on a PCIE port. Enforce it.
> >
> > CC: jusual@redhat.com
> > CC: imammedo@redhat.com
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
> > Signed-off-by: Ani Sinha <anisinha@redhat.com>
> > ---
> >  hw/pci/pci.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index bf38905b7d..5f25ab9f5e 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -64,6 +64,7 @@ bool pci_available = true;
> >  static char *pcibus_get_dev_path(DeviceState *dev);
> >  static char *pcibus_get_fw_dev_path(DeviceState *dev);
> >  static void pcibus_reset(BusState *qbus);
> > +static bool pcie_has_upstream_port(PCIDevice *dev);
> >
> >  static Property pci_props[] = {
> >      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> > @@ -1189,6 +1190,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
> >                     name);
> >
> >         return NULL;
> > +    } else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
> > +        error_setg(errp, "PCI: slot %d is not valid for %s,"
> > +                   " PCI express devices can only be plugged into slot 0.",
> 
> This is not technically correct, because downstream ports and root
> ports are also PCIe devices, and they can have different slots under
> upstream ports and RC. But this error will never be shown for them, so
> it seems fine.

Hmm. Confusing users is not nice ... I agree this might
make people think they can not use root ports in slot !=0 either.

Would you add "with an upstream port"?
E.g. "PCI Express devices with an upstream port" ?

> 
> Reviewed-by: Julia Suvorova <jusual@redhat.com>
> 
> 
> 
> 
> > +                   PCI_SLOT(devfn), name);
> > +        return NULL;
> >      }
> >
> >      pci_dev->devfn = devfn;
> > --
> > 2.39.1
> >

Re: [PATCH v2 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Julia Suvorova]

On Thu, Jun 22, 2023 at 7:48 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Jun 22, 2023 at 05:46:40PM +0200, Julia Suvorova wrote:
> > On Thu, Jun 22, 2023 at 12:34 PM Ani Sinha <anisinha@redhat.com> wrote:
> > >
> > > PCI Express ports only have one slot, so PCI Express devices can only be
> > > plugged into slot 0 on a PCIE port. Enforce it.
> > >
> > > CC: jusual@redhat.com
> > > CC: imammedo@redhat.com
> > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
> > > Signed-off-by: Ani Sinha <anisinha@redhat.com>
> > > ---
> > >  hw/pci/pci.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index bf38905b7d..5f25ab9f5e 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -64,6 +64,7 @@ bool pci_available = true;
> > >  static char *pcibus_get_dev_path(DeviceState *dev);
> > >  static char *pcibus_get_fw_dev_path(DeviceState *dev);
> > >  static void pcibus_reset(BusState *qbus);
> > > +static bool pcie_has_upstream_port(PCIDevice *dev);
> > >
> > >  static Property pci_props[] = {
> > >      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> > > @@ -1189,6 +1190,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
> > >                     name);
> > >
> > >         return NULL;
> > > +    } else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
> > > +        error_setg(errp, "PCI: slot %d is not valid for %s,"
> > > +                   " PCI express devices can only be plugged into slot 0.",
> >
> > This is not technically correct, because downstream ports and root
> > ports are also PCIe devices, and they can have different slots under
> > upstream ports and RC. But this error will never be shown for them, so
> > it seems fine.
>
> Hmm. Confusing users is not nice ... I agree this might
> make people think they can not use root ports in slot !=0 either.
>
> Would you add "with an upstream port"?
> E.g. "PCI Express devices with an upstream port" ?

This whole upstream port conditioning is quite confusing.
How about "%parent device% only allows plugging into slot 0"?

Best regards, Julia Suvorova.

> >
> > Reviewed-by: Julia Suvorova <jusual@redhat.com>
> >
> >
> >
> >
> > > +                   PCI_SLOT(devfn), name);
> > > +        return NULL;
> > >      }
> > >
> > >      pci_dev->devfn = devfn;
> > > --
> > > 2.39.1
> > >
>

Re: [PATCH v2 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Michael S. Tsirkin]

On Fri, Jun 23, 2023 at 04:57:20PM +0200, Julia Suvorova wrote:
> On Thu, Jun 22, 2023 at 7:48 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Jun 22, 2023 at 05:46:40PM +0200, Julia Suvorova wrote:
> > > On Thu, Jun 22, 2023 at 12:34 PM Ani Sinha <anisinha@redhat.com> wrote:
> > > >
> > > > PCI Express ports only have one slot, so PCI Express devices can only be
> > > > plugged into slot 0 on a PCIE port. Enforce it.
> > > >
> > > > CC: jusual@redhat.com
> > > > CC: imammedo@redhat.com
> > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
> > > > Signed-off-by: Ani Sinha <anisinha@redhat.com>
> > > > ---
> > > >  hw/pci/pci.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > index bf38905b7d..5f25ab9f5e 100644
> > > > --- a/hw/pci/pci.c
> > > > +++ b/hw/pci/pci.c
> > > > @@ -64,6 +64,7 @@ bool pci_available = true;
> > > >  static char *pcibus_get_dev_path(DeviceState *dev);
> > > >  static char *pcibus_get_fw_dev_path(DeviceState *dev);
> > > >  static void pcibus_reset(BusState *qbus);
> > > > +static bool pcie_has_upstream_port(PCIDevice *dev);
> > > >
> > > >  static Property pci_props[] = {
> > > >      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> > > > @@ -1189,6 +1190,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
> > > >                     name);
> > > >
> > > >         return NULL;
> > > > +    } else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
> > > > +        error_setg(errp, "PCI: slot %d is not valid for %s,"
> > > > +                   " PCI express devices can only be plugged into slot 0.",
> > >
> > > This is not technically correct, because downstream ports and root
> > > ports are also PCIe devices, and they can have different slots under
> > > upstream ports and RC. But this error will never be shown for them, so
> > > it seems fine.
> >
> > Hmm. Confusing users is not nice ... I agree this might
> > make people think they can not use root ports in slot !=0 either.
> >
> > Would you add "with an upstream port"?
> > E.g. "PCI Express devices with an upstream port" ?
> 
> This whole upstream port conditioning is quite confusing.
> How about "%parent device% only allows plugging into slot 0"?
> 
> Best regards, Julia Suvorova.

Good idea!

> > >
> > > Reviewed-by: Julia Suvorova <jusual@redhat.com>
> > >
> > >
> > >
> > >
> > > > +                   PCI_SLOT(devfn), name);
> > > > +        return NULL;
> > > >      }
> > > >
> > > >      pci_dev->devfn = devfn;
> > > > --
> > > > 2.39.1
> > > >
> >

Re: [PATCH v2 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Ani Sinha]

> On 26-Jun-2023, at 2:41 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Fri, Jun 23, 2023 at 04:57:20PM +0200, Julia Suvorova wrote:
>> On Thu, Jun 22, 2023 at 7:48 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>> 
>>> On Thu, Jun 22, 2023 at 05:46:40PM +0200, Julia Suvorova wrote:
>>>> On Thu, Jun 22, 2023 at 12:34 PM Ani Sinha <anisinha@redhat.com> wrote:
>>>>> 
>>>>> PCI Express ports only have one slot, so PCI Express devices can only be
>>>>> plugged into slot 0 on a PCIE port. Enforce it.
>>>>> 
>>>>> CC: jusual@redhat.com
>>>>> CC: imammedo@redhat.com
>>>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
>>>>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>>>>> ---
>>>>> hw/pci/pci.c | 6 ++++++
>>>>> 1 file changed, 6 insertions(+)
>>>>> 
>>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>>> index bf38905b7d..5f25ab9f5e 100644
>>>>> --- a/hw/pci/pci.c
>>>>> +++ b/hw/pci/pci.c
>>>>> @@ -64,6 +64,7 @@ bool pci_available = true;
>>>>> static char *pcibus_get_dev_path(DeviceState *dev);
>>>>> static char *pcibus_get_fw_dev_path(DeviceState *dev);
>>>>> static void pcibus_reset(BusState *qbus);
>>>>> +static bool pcie_has_upstream_port(PCIDevice *dev);
>>>>> 
>>>>> static Property pci_props[] = {
>>>>>     DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
>>>>> @@ -1189,6 +1190,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>>>>>                    name);
>>>>> 
>>>>>        return NULL;
>>>>> +    } else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
>>>>> +        error_setg(errp, "PCI: slot %d is not valid for %s,"
>>>>> +                   " PCI express devices can only be plugged into slot 0.",
>>>> 
>>>> This is not technically correct, because downstream ports and root
>>>> ports are also PCIe devices, and they can have different slots under
>>>> upstream ports and RC. But this error will never be shown for them, so
>>>> it seems fine.
>>> 
>>> Hmm. Confusing users is not nice ... I agree this might
>>> make people think they can not use root ports in slot !=0 either.
>>> 
>>> Would you add "with an upstream port"?
>>> E.g. "PCI Express devices with an upstream port" ?
>> 
>> This whole upstream port conditioning is quite confusing.
>> How about "%parent device% only allows plugging into slot 0"?
>> 
>> Best regards, Julia Suvorova.
> 
> Good idea!

OK I will send out another iteration once Igor has a chance to take a look at the test fixes or someone else reviews them. 

> 
>>>> 
>>>> Reviewed-by: Julia Suvorova <jusual@redhat.com>
>>>> 
>>>> 
>>>> 
>>>> 
>>>>> +                   PCI_SLOT(devfn), name);
>>>>> +        return NULL;
>>>>>     }
>>>>> 
>>>>>     pci_dev->devfn = devfn;
>>>>> --
>>>>> 2.39.1
>>>>> 
>>> 
> 

Re: [PATCH v2 4/5] tests/qtest/hd-geo-test: fix test by removing unnecessary pcie-root-port

[Igor Mammedov]

On Thu, 22 Jun 2023 16:02:54 +0530
Ani Sinha <anisinha@redhat.com> wrote:

> A SCSI controller can be attached to a pcie-to-pci bridge which in turn can be
> attached directly to the root bus (peie.0). There is no need to attach a
> pcie-root-port on the root bus in order to attach the pcie-ro-pci bridge.
> Fix it.

bridge can be both on pcie.0 or on root-port and both are valid configs.
So what exactly and why we are fixing here?

> 
> CC: mst@redhat.com
> CC: imammedo@redhat.com
> CC: Michael Labiuk <michael.labiuk@virtuozzo.com>
> 
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> ---
>  tests/qtest/hd-geo-test.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/qtest/hd-geo-test.c b/tests/qtest/hd-geo-test.c
> index 5aa258a2b3..d08bffad91 100644
> --- a/tests/qtest/hd-geo-test.c
> +++ b/tests/qtest/hd-geo-test.c
> @@ -784,14 +784,12 @@ static void test_override_scsi(void)
>      test_override(args, "pc", expected);
>  }
>  
> -static void setup_pci_bridge(TestArgs *args, const char *id, const char *rootid)
> +static void setup_pci_bridge(TestArgs *args, const char *id)
>  {
>  
> -    char *root, *br;
> -    root = g_strdup_printf("-device pcie-root-port,id=%s", rootid);
> -    br = g_strdup_printf("-device pcie-pci-bridge,bus=%s,id=%s", rootid, id);
> +    char *br;
> +    br = g_strdup_printf("-device pcie-pci-bridge,bus=pcie.0,id=%s", id);
>  
> -    args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, root);
>      args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, br);
>  }
>  
> @@ -811,8 +809,8 @@ static void test_override_scsi_q35(void)
>      add_drive_with_mbr(args, empty_mbr, 1);
>      add_drive_with_mbr(args, empty_mbr, 1);
>      add_drive_with_mbr(args, empty_mbr, 1);
> -    setup_pci_bridge(args, "pcie.0", "br");
> -    add_scsi_controller(args, "lsi53c895a", "br", 3);
> +    setup_pci_bridge(args, "pcie-pci-br");
> +    add_scsi_controller(args, "lsi53c895a", "pcie-pci-br", 3);
>      add_scsi_disk(args, 0, 0, 0, 0, 0, 10000, 120, 30);
>      add_scsi_disk(args, 1, 0, 0, 1, 0, 9000, 120, 30);
>      add_scsi_disk(args, 2, 0, 0, 2, 0, 1, 0, 0);
> @@ -868,9 +866,9 @@ static void test_override_virtio_blk_q35(void)
>      };
>      add_drive_with_mbr(args, empty_mbr, 1);
>      add_drive_with_mbr(args, empty_mbr, 1);
> -    setup_pci_bridge(args, "pcie.0", "br");
> -    add_virtio_disk(args, 0, "br", 3, 10000, 120, 30);
> -    add_virtio_disk(args, 1, "br", 4, 9000, 120, 30);
> +    setup_pci_bridge(args, "pcie-pci-br");
> +    add_virtio_disk(args, 0, "pcie-pci-br", 3, 10000, 120, 30);
> +    add_virtio_disk(args, 1, "pcie-pci-br", 4, 9000, 120, 30);
>      test_override(args, "q35", expected);
>  }
>  

Re: [PATCH v2 4/5] tests/qtest/hd-geo-test: fix test by removing unnecessary pcie-root-port

[Ani Sinha]

> On 26-Jun-2023, at 4:45 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> On Thu, 22 Jun 2023 16:02:54 +0530
> Ani Sinha <anisinha@redhat.com> wrote:
> 
>> A SCSI controller can be attached to a pcie-to-pci bridge which in turn can be
>> attached directly to the root bus (peie.0). There is no need to attach a
>> pcie-root-port on the root bus in order to attach the pcie-ro-pci bridge.
>> Fix it.
> 
> bridge can be both on pcie.0 or on root-port and both are valid configs.
> So what exactly and why we are fixing here?

If you look at the functions carefully, “br” is a pcie-root-port and “pcie.0” is a pcie-to-pci bridge.
The bug here is that both the SCSI controller and the pcie-to-pci bridge (pcie.0) were getting attached to the same pcie-root-port. I think the intention of the author was to attach the SCSI controller to pcie-to-pci bridge.
In any case, I do not see the reason to attach a pcie-root-port here. We can attach the pcie-to-pci bridge on the RC and then attach the SCSI controllers on the bridge.

> 
>> 
>> CC: mst@redhat.com
>> CC: imammedo@redhat.com
>> CC: Michael Labiuk <michael.labiuk@virtuozzo.com>
>> 
>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>> ---
>> tests/qtest/hd-geo-test.c | 18 ++++++++----------
>> 1 file changed, 8 insertions(+), 10 deletions(-)
>> 
>> diff --git a/tests/qtest/hd-geo-test.c b/tests/qtest/hd-geo-test.c
>> index 5aa258a2b3..d08bffad91 100644
>> --- a/tests/qtest/hd-geo-test.c
>> +++ b/tests/qtest/hd-geo-test.c
>> @@ -784,14 +784,12 @@ static void test_override_scsi(void)
>>     test_override(args, "pc", expected);
>> }
>> 
>> -static void setup_pci_bridge(TestArgs *args, const char *id, const char *rootid)
>> +static void setup_pci_bridge(TestArgs *args, const char *id)
>> {
>> 
>> -    char *root, *br;
>> -    root = g_strdup_printf("-device pcie-root-port,id=%s", rootid);
>> -    br = g_strdup_printf("-device pcie-pci-bridge,bus=%s,id=%s", rootid, id);
>> +    char *br;
>> +    br = g_strdup_printf("-device pcie-pci-bridge,bus=pcie.0,id=%s", id);
>> 
>> -    args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, root);
>>     args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, br);
>> }
>> 
>> @@ -811,8 +809,8 @@ static void test_override_scsi_q35(void)
>>     add_drive_with_mbr(args, empty_mbr, 1);
>>     add_drive_with_mbr(args, empty_mbr, 1);
>>     add_drive_with_mbr(args, empty_mbr, 1);
>> -    setup_pci_bridge(args, "pcie.0", "br");
>> -    add_scsi_controller(args, "lsi53c895a", "br", 3);
>> +    setup_pci_bridge(args, "pcie-pci-br");
>> +    add_scsi_controller(args, "lsi53c895a", "pcie-pci-br", 3);
>>     add_scsi_disk(args, 0, 0, 0, 0, 0, 10000, 120, 30);
>>     add_scsi_disk(args, 1, 0, 0, 1, 0, 9000, 120, 30);
>>     add_scsi_disk(args, 2, 0, 0, 2, 0, 1, 0, 0);
>> @@ -868,9 +866,9 @@ static void test_override_virtio_blk_q35(void)
>>     };
>>     add_drive_with_mbr(args, empty_mbr, 1);
>>     add_drive_with_mbr(args, empty_mbr, 1);
>> -    setup_pci_bridge(args, "pcie.0", "br");
>> -    add_virtio_disk(args, 0, "br", 3, 10000, 120, 30);
>> -    add_virtio_disk(args, 1, "br", 4, 9000, 120, 30);
>> +    setup_pci_bridge(args, "pcie-pci-br");
>> +    add_virtio_disk(args, 0, "pcie-pci-br", 3, 10000, 120, 30);
>> +    add_virtio_disk(args, 1, "pcie-pci-br", 4, 9000, 120, 30);
>>     test_override(args, "q35", expected);
>> }
>> 
> 

Re: [PATCH v2 4/5] tests/qtest/hd-geo-test: fix test by removing unnecessary pcie-root-port

[Igor Mammedov]

On Mon, 26 Jun 2023 17:01:29 +0530
Ani Sinha <anisinha@redhat.com> wrote:

> > On 26-Jun-2023, at 4:45 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > 
> > On Thu, 22 Jun 2023 16:02:54 +0530
> > Ani Sinha <anisinha@redhat.com> wrote:
> >   
> >> A SCSI controller can be attached to a pcie-to-pci bridge which in turn can be
> >> attached directly to the root bus (peie.0). There is no need to attach a
> >> pcie-root-port on the root bus in order to attach the pcie-ro-pci bridge.
> >> Fix it.  
> > 
> > bridge can be both on pcie.0 or on root-port and both are valid configs.
> > So what exactly and why we are fixing here?  
> 
> If you look at the functions carefully, “br” is a pcie-root-port and “pcie.0” is a pcie-to-pci bridge.
> The bug here is that both the SCSI controller and the pcie-to-pci bridge (pcie.0) were getting attached to the same pcie-root-port. I think the intention of the author was to attach the SCSI controller to pcie-to-pci bridge.
> In any case, I do not see the reason to attach a pcie-root-port here. We can attach the pcie-to-pci bridge on the RC and then attach the SCSI controllers on the bridge.

Description of what's wrong should be in commit message
then you can follow up with a reasoning why you don't think it's worth fixing
and what you do to remedy situation.

> 
> >   
> >> 
> >> CC: mst@redhat.com
> >> CC: imammedo@redhat.com
> >> CC: Michael Labiuk <michael.labiuk@virtuozzo.com>
> >> 
> >> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> >> ---
> >> tests/qtest/hd-geo-test.c | 18 ++++++++----------
> >> 1 file changed, 8 insertions(+), 10 deletions(-)
> >> 
> >> diff --git a/tests/qtest/hd-geo-test.c b/tests/qtest/hd-geo-test.c
> >> index 5aa258a2b3..d08bffad91 100644
> >> --- a/tests/qtest/hd-geo-test.c
> >> +++ b/tests/qtest/hd-geo-test.c
> >> @@ -784,14 +784,12 @@ static void test_override_scsi(void)
> >>     test_override(args, "pc", expected);
> >> }
> >> 
> >> -static void setup_pci_bridge(TestArgs *args, const char *id, const char *rootid)
> >> +static void setup_pci_bridge(TestArgs *args, const char *id)
> >> {
> >> 
> >> -    char *root, *br;
> >> -    root = g_strdup_printf("-device pcie-root-port,id=%s", rootid);
> >> -    br = g_strdup_printf("-device pcie-pci-bridge,bus=%s,id=%s", rootid, id);
> >> +    char *br;
> >> +    br = g_strdup_printf("-device pcie-pci-bridge,bus=pcie.0,id=%s", id);
> >> 
> >> -    args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, root);
> >>     args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, br);
> >> }
> >> 
> >> @@ -811,8 +809,8 @@ static void test_override_scsi_q35(void)
> >>     add_drive_with_mbr(args, empty_mbr, 1);
> >>     add_drive_with_mbr(args, empty_mbr, 1);
> >>     add_drive_with_mbr(args, empty_mbr, 1);
> >> -    setup_pci_bridge(args, "pcie.0", "br");
> >> -    add_scsi_controller(args, "lsi53c895a", "br", 3);
> >> +    setup_pci_bridge(args, "pcie-pci-br");
> >> +    add_scsi_controller(args, "lsi53c895a", "pcie-pci-br", 3);
> >>     add_scsi_disk(args, 0, 0, 0, 0, 0, 10000, 120, 30);
> >>     add_scsi_disk(args, 1, 0, 0, 1, 0, 9000, 120, 30);
> >>     add_scsi_disk(args, 2, 0, 0, 2, 0, 1, 0, 0);
> >> @@ -868,9 +866,9 @@ static void test_override_virtio_blk_q35(void)
> >>     };
> >>     add_drive_with_mbr(args, empty_mbr, 1);
> >>     add_drive_with_mbr(args, empty_mbr, 1);
> >> -    setup_pci_bridge(args, "pcie.0", "br");
> >> -    add_virtio_disk(args, 0, "br", 3, 10000, 120, 30);
> >> -    add_virtio_disk(args, 1, "br", 4, 9000, 120, 30);
> >> +    setup_pci_bridge(args, "pcie-pci-br");
> >> +    add_virtio_disk(args, 0, "pcie-pci-br", 3, 10000, 120, 30);
> >> +    add_virtio_disk(args, 1, "pcie-pci-br", 4, 9000, 120, 30);
> >>     test_override(args, "q35", expected);
> >> }
> >>   
> >   
>