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

[PATCH v4 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:
v4: reword commit log for patch 4.
v3: tags added. reword the error description in patch 5. Reword commit log in patch 4. 
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 incorrect pcie-root-port usage and
    simplify test
  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 v4 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>
Acked-by: Igor Mammedov <imammedo@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 v4 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>
Reviewed-by: Igor Mammedov <imammedo@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 v4 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>
Acked-by: Igor Mammedov <imammedo@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 v4 4/5] tests/qtest/hd-geo-test: fix incorrect pcie-root-port usage and simplify test

[Ani Sinha]

The test attaches both a SCSI controller and a pcie-to-pci bridge on the same
pcie-root-port on slot 0. This is incorrect since a single downstream device
can be attached to a non-multifunction pcie-root-port on slot 0. Additionally
using pcie.0 as id for pcie-root-port is incorrect as that id is reserved
only for the root bus.

In the test scenario, there is no need to attach a pcie-root-port to the
root complex. A SCSI controller can be attached to a pcie-to-pci bridge
which can then be directly attached to the root bus (pcie.0).

Fix the test and simplify 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 v4 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>
Reviewed-by: Julia Suvorova <jusual@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..426af133b0 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,"
+                   " parent device only allows plugging into slot 0.",
+                   PCI_SLOT(devfn), name);
+        return NULL;
     }
 
     pci_dev->devfn = devfn;
-- 
2.39.1

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

[Michael S. Tsirkin]

Ani, two versions a day is too much, pls don't unless first one
has been sent by mistake or something. then explain what
it was pls.

On Mon, Jun 26, 2023 at 07:23:19PM +0530, Ani Sinha wrote:
> 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:
> v4: reword commit log for patch 4.
> v3: tags added. reword the error description in patch 5. Reword commit log in patch 4. 
> 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 incorrect pcie-root-port usage and
>     simplify test
>   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

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

[Ani Sinha]

> On 26-Jun-2023, at 7:24 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> Ani, two versions a day is too much,

So you suggesting I slow down my CPU freq? :-) 

> pls don't unless first one
> has been sent by mistake or something. then explain what
> it was pls.

I did say 

v4: reword commit log for patch 4

> 
> On Mon, Jun 26, 2023 at 07:23:19PM +0530, Ani Sinha wrote:
>> 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:
>> v4: reword commit log for patch 4.
>> v3: tags added. reword the error description in patch 5. Reword commit log in patch 4. 
>> 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 incorrect pcie-root-port usage and
>>    simplify test
>>  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
> 

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

[Ani Sinha]

> On 26-Jun-2023, at 7:26 PM, Ani Sinha <anisinha@redhat.com> wrote:
> 
> 
> 
>> On 26-Jun-2023, at 7:24 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> 
>> Ani, two versions a day is too much,
> 
> So you suggesting I slow down my CPU freq? :-) 

Btw, the libvirt guys does this better in this respect I think. The maintainer takes the patchset but if small rework in commit logs or comments etc is needed, the maintainer does this himself and pushes the patch. That way there is less churn.

> 
>> pls don't unless first one
>> has been sent by mistake or something. then explain what
>> it was pls.
> 
> I did say 
> 
> v4: reword commit log for patch 4
> 
>> 
>> On Mon, Jun 26, 2023 at 07:23:19PM +0530, Ani Sinha wrote:
>>> 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:
>>> v4: reword commit log for patch 4.
>>> v3: tags added. reword the error description in patch 5. Reword commit log in patch 4. 
>>> 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 incorrect pcie-root-port usage and
>>>   simplify test
>>> 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
>> 
> 

Re: [PATCH v4 4/5] tests/qtest/hd-geo-test: fix incorrect pcie-root-port usage and simplify test

[Igor Mammedov]

On Mon, 26 Jun 2023 19:23:23 +0530
Ani Sinha <anisinha@redhat.com> wrote:

> The test attaches both a SCSI controller and a pcie-to-pci bridge on the same
> pcie-root-port on slot 0.
statement is right only for bridge, while it's still incorrect for storage part
(see add_scsi_controller/add_virtio_disk)
they try to use slot !0

> This is incorrect since a single downstream device
> can be attached to a non-multifunction pcie-root-port on slot 0.

>Additionally
> using pcie.0 as id for pcie-root-port is incorrect as that id is reserved
> only for the root bus.
 ack on this part
 
> In the test scenario, there is no need to attach a pcie-root-port to the
> root complex. A SCSI controller can be attached to a pcie-to-pci bridge
> which can then be directly attached to the root bus (pcie.0).
> 
> Fix the test and simplify 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);
>  }
>  

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

[Michael S. Tsirkin]

On Mon, Jun 26, 2023 at 07:29:26PM +0530, Ani Sinha wrote:
> 
> 
> > On 26-Jun-2023, at 7:26 PM, Ani Sinha <anisinha@redhat.com> wrote:
> > 
> > 
> > 
> >> On 26-Jun-2023, at 7:24 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> 
> >> Ani, two versions a day is too much,
> > 
> > So you suggesting I slow down my CPU freq? :-) 
> 
> Btw, the libvirt guys does this better in this respect I think. The maintainer takes the patchset but if small rework in commit logs or comments etc is needed, the maintainer does this himself and pushes the patch. That way there is less churn.

I can do that too sometimes. I'm less of a dictator, prefer to get
acks from others :)

> > 
> >> pls don't unless first one
> >> has been sent by mistake or something. then explain what
> >> it was pls.
> > 
> > I did say 
> > 
> > v4: reword commit log for patch 4

Oh, missed that. it's ok then, just make this a bit more prominent:
note: no changes to patches, just commit log tweaks.

> >> 
> >> On Mon, Jun 26, 2023 at 07:23:19PM +0530, Ani Sinha wrote:
> >>> 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:
> >>> v4: reword commit log for patch 4.
> >>> v3: tags added. reword the error description in patch 5. Reword commit log in patch 4. 
> >>> 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 incorrect pcie-root-port usage and
> >>>   simplify test
> >>> 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
> >> 
> > 

Re: [PATCH v4 4/5] tests/qtest/hd-geo-test: fix incorrect pcie-root-port usage and simplify test

[Ani Sinha]

> On 26-Jun-2023, at 7:52 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> On Mon, 26 Jun 2023 19:23:23 +0530
> Ani Sinha <anisinha@redhat.com> wrote:
> 
>> The test attaches both a SCSI controller and a pcie-to-pci bridge on the same
>> pcie-root-port on slot 0.
> statement is right only for bridge, while it's still incorrect for storage part
> (see add_scsi_controller/add_virtio_disk)
> they try to use slot !0

Grr! Sorry about this, not sure what I was thinking.
Juggling too many things.

Patchset V5 sent.

> 
>> This is incorrect since a single downstream device
>> can be attached to a non-multifunction pcie-root-port on slot 0.
> 
>> Additionally
>> using pcie.0 as id for pcie-root-port is incorrect as that id is reserved
>> only for the root bus.
> ack on this part
> 
>> In the test scenario, there is no need to attach a pcie-root-port to the
>> root complex. A SCSI controller can be attached to a pcie-to-pci bridge
>> which can then be directly attached to the root bus (pcie.0).
>> 
>> Fix the test and simplify 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);
>> }
>> 
>