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

[RESEND PATCH v5 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:
===========
v5: no code changes - correct a mistake in the commit log message.

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

[RESEND PATCH v5 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

[RESEND PATCH v5 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

[RESEND PATCH v5 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

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

[Ani Sinha]

The test attaches a SCSI controller to a non-zero slot and a pcie-to-pci bridge
on slot 0 on the same pcie-root-port. Since a downstream device can be attached
to a pcie-root-port only on slot 0, the above test configuration is not allowed.
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

[RESEND PATCH v5 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: [RESEND PATCH v5 4/5] tests/qtest/hd-geo-test: fix incorrect pcie-root-port usage and simplify test

[Igor Mammedov]

On Mon, 26 Jun 2023 21:42:43 +0530
Ani Sinha <anisinha@redhat.com> wrote:

> The test attaches a SCSI controller to a non-zero slot and a pcie-to-pci bridge
> on slot 0 on the same pcie-root-port. Since a downstream device can be attached
> to a pcie-root-port only on slot 0, the above test configuration is not allowed.

> Additionally using pcie.0 as id for pcie-root-port is incorrect as that id is
                                      ^^^^ shouldn't it be pcie-to-pci ?
> 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);
>  }
>  

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

[Igor Mammedov]

On Mon, 26 Jun 2023 21:42:44 +0530
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.

btw, previously you mentioned ARI.
So if we turn it on, wouldn't this patch actually become regression?

> 
> 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;

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

[Ani Sinha]

> On 27-Jun-2023, at 2:24 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> On Mon, 26 Jun 2023 21:42:43 +0530
> Ani Sinha <anisinha@redhat.com> wrote:
> 
>> The test attaches a SCSI controller to a non-zero slot and a pcie-to-pci bridge
>> on slot 0 on the same pcie-root-port. Since a downstream device can be attached
>> to a pcie-root-port only on slot 0, the above test configuration is not allowed.
> 
>> Additionally using pcie.0 as id for pcie-root-port is incorrect as that id is
>                                      ^^^^ shouldn't it be pcie-to-pci ?

Yes you are right. Pcie-root-port is “br” in the test. Its so confusing!

>> 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);
>> }
>> 
> 

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

[Ani Sinha]

> On 27-Jun-2023, at 2:32 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> On Mon, 26 Jun 2023 21:42:44 +0530
> 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.
> 
> btw, previously you mentioned ARI.
> So if we turn it on, wouldn't this patch actually become regression?

If ARI breaks this, it will break other areas in QEMU too, ex anywhere pci_get_function_0() is used.
Regardless, I think at least the tests are worth fixing, particularly the mess with hd-geo-test.

> 
>> 
>> 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;
> 

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

[Michael S. Tsirkin]

On Tue, Jun 27, 2023 at 03:23:04PM +0530, Ani Sinha wrote:
> 
> 
> > On 27-Jun-2023, at 2:32 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > 
> > On Mon, 26 Jun 2023 21:42:44 +0530
> > 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.
> > 
> > btw, previously you mentioned ARI.
> > So if we turn it on, wouldn't this patch actually become regression?
> 
> If ARI breaks this, it will break other areas in QEMU too, ex anywhere pci_get_function_0() is used.

We will just fix pci_get_function_0.

> Regardless, I think at least the tests are worth fixing, particularly the mess with hd-geo-test.

ok

> > 
> >> 
> >> 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;
> > 

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

[Ani Sinha]

> On 27-Jun-2023, at 3:23 PM, Ani Sinha <anisinha@redhat.com> wrote:
> 
> 
> 
>> On 27-Jun-2023, at 2:32 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>> 
>> On Mon, 26 Jun 2023 21:42:44 +0530
>> 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.
>> 
>> btw, previously you mentioned ARI.
>> So if we turn it on, wouldn't this patch actually become regression?

Looking at https://pcisig.com/sites/default/files/specification_documents/ECN-alt-rid-interpretation-070604.pdf, section 7.23, seems a root port does not support ARI but it can support ARI forwarding capability (section 7.8.5).
Also with ARI enabled, the device cannot have a non-zero device number. Also, shouldn't any code path that uses PCI_SLOT() should probably also check for ARI if it wants to be ARI complaint?

Anyways these are just facts I could find but I am not sure if this would answer your above question.

> 
> If ARI breaks this, it will break other areas in QEMU too, ex anywhere pci_get_function_0() is used.
> Regardless, I think at least the tests are worth fixing, particularly the mess with hd-geo-test.
> 
>> 
>>> 
>>> 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;
>> 
> 

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

[Igor Mammedov]

On Tue, 27 Jun 2023 15:23:04 +0530
Ani Sinha <anisinha@redhat.com> wrote:

> > On 27-Jun-2023, at 2:32 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > 
> > On Mon, 26 Jun 2023 21:42:44 +0530
> > 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.  
> > 
> > btw, previously you mentioned ARI.
> > So if we turn it on, wouldn't this patch actually become regression?  
> 
> If ARI breaks this, it will break other areas in QEMU too, ex anywhere pci_get_function_0() is used.
> Regardless, I think at least the tests are worth fixing, particularly the mess with hd-geo-test.

I'm fine with this patch if you test it with ARI enabled and it won't break
something that has been working before this patch. Just mention what testing
you've done in commit message.

> 
> >   
> >> 
> >> 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;  
> >   
> 

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

[Ani Sinha]

> On 27-Jun-2023, at 5:25 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Tue, Jun 27, 2023 at 03:23:04PM +0530, Ani Sinha wrote:
>> 
>> 
>>> On 27-Jun-2023, at 2:32 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>>> 
>>> On Mon, 26 Jun 2023 21:42:44 +0530
>>> 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.
>>> 
>>> btw, previously you mentioned ARI.
>>> So if we turn it on, wouldn't this patch actually become regression?
>> 
>> If ARI breaks this, it will break other areas in QEMU too, ex anywhere pci_get_function_0() is used.
> 
> We will just fix pci_get_function_0.

Any code with PCI_SLOT() I believe also would need fixing?

> 
>> Regardless, I think at least the tests are worth fixing, particularly the mess with hd-geo-test.
> 
> ok
> 
>>> 
>>>> 
>>>> 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;
>>> 
> 

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

[Michael S. Tsirkin]

On Tue, Jun 27, 2023 at 05:31:37PM +0530, Ani Sinha wrote:
> 
> 
> > On 27-Jun-2023, at 5:25 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 
> > On Tue, Jun 27, 2023 at 03:23:04PM +0530, Ani Sinha wrote:
> >> 
> >> 
> >>> On 27-Jun-2023, at 2:32 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> >>> 
> >>> On Mon, 26 Jun 2023 21:42:44 +0530
> >>> 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.
> >>> 
> >>> btw, previously you mentioned ARI.
> >>> So if we turn it on, wouldn't this patch actually become regression?
> >> 
> >> If ARI breaks this, it will break other areas in QEMU too, ex anywhere pci_get_function_0() is used.
> > 
> > We will just fix pci_get_function_0.
> 
> Any code with PCI_SLOT() I believe also would need fixing?

Oh, absolutely.

> > 
> >> Regardless, I think at least the tests are worth fixing, particularly the mess with hd-geo-test.
> > 
> > ok
> > 
> >>> 
> >>>> 
> >>>> 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;
> >>> 
> > 

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

[Michael S. Tsirkin]

On Tue, Jun 27, 2023 at 01:58:49PM +0200, Igor Mammedov wrote:
> On Tue, 27 Jun 2023 15:23:04 +0530
> Ani Sinha <anisinha@redhat.com> wrote:
> 
> > > On 27-Jun-2023, at 2:32 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > > 
> > > On Mon, 26 Jun 2023 21:42:44 +0530
> > > 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.  
> > > 
> > > btw, previously you mentioned ARI.
> > > So if we turn it on, wouldn't this patch actually become regression?  
> > 
> > If ARI breaks this, it will break other areas in QEMU too, ex anywhere pci_get_function_0() is used.
> > Regardless, I think at least the tests are worth fixing, particularly the mess with hd-geo-test.
> 
> I'm fine with this patch if you test it with ARI enabled and it won't break
> something that has been working before this patch. Just mention what testing
> you've done in commit message.

Oh yes. That's why it was checking !vf originally. It's because the most
common use of ARI is SRIOV, so it works a a kind of hack.

> > 
> > >   
> > >> 
> > >> 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;  
> > >   
> > 

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

[Ani Sinha]

> On 27-Jun-2023, at 5:53 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Tue, Jun 27, 2023 at 01:58:49PM +0200, Igor Mammedov wrote:
>> On Tue, 27 Jun 2023 15:23:04 +0530
>> Ani Sinha <anisinha@redhat.com> wrote:
>> 
>>>> On 27-Jun-2023, at 2:32 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>>>> 
>>>> On Mon, 26 Jun 2023 21:42:44 +0530
>>>> 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.  
>>>> 
>>>> btw, previously you mentioned ARI.
>>>> So if we turn it on, wouldn't this patch actually become regression?  
>>> 
>>> If ARI breaks this, it will break other areas in QEMU too, ex anywhere pci_get_function_0() is used.
>>> Regardless, I think at least the tests are worth fixing, particularly the mess with hd-geo-test.
>> 
>> I'm fine with this patch if you test it with ARI enabled and it won't break
>> something that has been working before this patch. Just mention what testing
>> you've done in commit message.
> 
> Oh yes. That's why it was checking !vf originally. It's because the most
> common use of ARI is SRIOV, so it works a a kind of hack.

Ok so should I put it back?
Also I was thinking of running the qtest and avocado test mentioned in https://www.qemu.org/docs/master/system/devices/igb.html . Not sure if it would be enough to test ARI.

> 
>>> 
>>>> 
>>>>> 
>>>>> 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;  

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

[Igor Mammedov]

On Tue, 27 Jun 2023 08:23:25 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Jun 27, 2023 at 01:58:49PM +0200, Igor Mammedov wrote:
> > On Tue, 27 Jun 2023 15:23:04 +0530
> > Ani Sinha <anisinha@redhat.com> wrote:
> >   
> > > > On 27-Jun-2023, at 2:32 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > > > 
> > > > On Mon, 26 Jun 2023 21:42:44 +0530
> > > > 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.    
> > > > 
> > > > btw, previously you mentioned ARI.
> > > > So if we turn it on, wouldn't this patch actually become regression?    
> > > 
> > > If ARI breaks this, it will break other areas in QEMU too, ex anywhere pci_get_function_0() is used.
> > > Regardless, I think at least the tests are worth fixing, particularly the mess with hd-geo-test.  
> > 
> > I'm fine with this patch if you test it with ARI enabled and it won't break
> > something that has been working before this patch. Just mention what testing
> > you've done in commit message.  
> 
> Oh yes. That's why it was checking !vf originally. It's because the most
> common use of ARI is SRIOV, so it works a a kind of hack.

should we check for ARI cap instead of vf hack?
why we haven't that from the beginning?

> 
> > >   
> > > >     
> > > >> 
> > > >> 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;    
> > > >     
> > >   
> 

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

[Michael S. Tsirkin]

On Tue, Jun 27, 2023 at 05:59:02PM +0530, Ani Sinha wrote:
> 
> 
> > On 27-Jun-2023, at 5:53 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 
> > On Tue, Jun 27, 2023 at 01:58:49PM +0200, Igor Mammedov wrote:
> >> On Tue, 27 Jun 2023 15:23:04 +0530
> >> Ani Sinha <anisinha@redhat.com> wrote:
> >> 
> >>>> On 27-Jun-2023, at 2:32 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> >>>> 
> >>>> On Mon, 26 Jun 2023 21:42:44 +0530
> >>>> 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.  
> >>>> 
> >>>> btw, previously you mentioned ARI.
> >>>> So if we turn it on, wouldn't this patch actually become regression?  
> >>> 
> >>> If ARI breaks this, it will break other areas in QEMU too, ex anywhere pci_get_function_0() is used.
> >>> Regardless, I think at least the tests are worth fixing, particularly the mess with hd-geo-test.
> >> 
> >> I'm fine with this patch if you test it with ARI enabled and it won't break
> >> something that has been working before this patch. Just mention what testing
> >> you've done in commit message.
> > 
> > Oh yes. That's why it was checking !vf originally. It's because the most
> > common use of ARI is SRIOV, so it works a a kind of hack.
> 
> Ok so should I put it back?
> Also I was thinking of running the qtest and avocado test mentioned in https://www.qemu.org/docs/master/system/devices/igb.html . Not sure if it would be enough to test ARI.

Well you need > 8 VFs for that.

> > 
> >>> 
> >>>> 
> >>>>> 
> >>>>> 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;  

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

[Michael S. Tsirkin]

On Tue, Jun 27, 2023 at 02:38:44PM +0200, Igor Mammedov wrote:
> On Tue, 27 Jun 2023 08:23:25 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Jun 27, 2023 at 01:58:49PM +0200, Igor Mammedov wrote:
> > > On Tue, 27 Jun 2023 15:23:04 +0530
> > > Ani Sinha <anisinha@redhat.com> wrote:
> > >   
> > > > > On 27-Jun-2023, at 2:32 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > > > > 
> > > > > On Mon, 26 Jun 2023 21:42:44 +0530
> > > > > 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.    
> > > > > 
> > > > > btw, previously you mentioned ARI.
> > > > > So if we turn it on, wouldn't this patch actually become regression?    
> > > > 
> > > > If ARI breaks this, it will break other areas in QEMU too, ex anywhere pci_get_function_0() is used.
> > > > Regardless, I think at least the tests are worth fixing, particularly the mess with hd-geo-test.  
> > > 
> > > I'm fine with this patch if you test it with ARI enabled and it won't break
> > > something that has been working before this patch. Just mention what testing
> > > you've done in commit message.  
> > 
> > Oh yes. That's why it was checking !vf originally. It's because the most
> > common use of ARI is SRIOV, so it works a a kind of hack.
> 
> should we check for ARI cap instead of vf hack?
> why we haven't that from the beginning?

Maybe. ARI is a capability, driver has to activate it, so it's not 100%
It does not help that our ARI implementation is broken in several
places.


> > 
> > > >   
> > > > >     
> > > > >> 
> > > > >> 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;    
> > > > >     
> > > >   
> > 

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

[Ani Sinha]

> On 27-Jun-2023, at 7:57 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Tue, Jun 27, 2023 at 02:38:44PM +0200, Igor Mammedov wrote:
>> On Tue, 27 Jun 2023 08:23:25 -0400
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>>> On Tue, Jun 27, 2023 at 01:58:49PM +0200, Igor Mammedov wrote:
>>>> On Tue, 27 Jun 2023 15:23:04 +0530
>>>> Ani Sinha <anisinha@redhat.com> wrote:
>>>> 
>>>>>> On 27-Jun-2023, at 2:32 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>>>>>> 
>>>>>> On Mon, 26 Jun 2023 21:42:44 +0530
>>>>>> 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.    
>>>>>> 
>>>>>> btw, previously you mentioned ARI.
>>>>>> So if we turn it on, wouldn't this patch actually become regression?    
>>>>> 
>>>>> If ARI breaks this, it will break other areas in QEMU too, ex anywhere pci_get_function_0() is used.
>>>>> Regardless, I think at least the tests are worth fixing, particularly the mess with hd-geo-test.  
>>>> 
>>>> I'm fine with this patch if you test it with ARI enabled and it won't break
>>>> something that has been working before this patch. Just mention what testing
>>>> you've done in commit message.  
>>> 
>>> Oh yes. That's why it was checking !vf originally. It's because the most
>>> common use of ARI is SRIOV, so it works a a kind of hack.
>> 
>> should we check for ARI cap instead of vf hack?
>> why we haven't that from the beginning?
> 
> Maybe.

Maybe not. I tried doing 

!pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI)

But sadly since the device has not been fully initialised and the config space memory has not been allocated yet, we can’t check for that capability at that point in the code.

Seems checking for vfs is the only best approximation at this point.

> ARI is a capability, driver has to activate it, so it's not 100%
> It does not help that our ARI implementation is broken in several
> places.
> 
> 
>>> 
>>>>> 
>>>>>> 
>>>>>>> 
>>>>>>> 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;    
>>>>>> 
>>>>> 
>>> 
>