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

[PATCH v6 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:
===========
v6: make patch 5 ARI compliant. fix commit message (s/pcie-root-port/pcie-to-pci/)
in patch 4. Rebase patchset to latest master.

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                      |  15 +++++++++++++++
 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, 25 insertions(+), 12 deletions(-)

-- 
2.39.1

[PATCH v6 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 v6 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 v6 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 v6 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-to-pci bridge 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

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

[Thomas Huth]

On 29/06/2023 06.07, Ani Sinha 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-to-pci bridge 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(-)


Acked-by: Thomas Huth <thuth@redhat.com>

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

[Igor Mammedov]

On Thu, 29 Jun 2023 09:37:06 +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-to-pci bridge 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>

Reviewed-by: Igor Mammedov <imammedo@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);
>  }
>  

[PATCH v6 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.

The change has been tested to not break ARI by instantiating seven vfs on an
emulated igb device (the maximum number of vfs the linux igb driver supports).
The vfs are seen to have non-zero device/slot numbers in the conventional
PCI BDF representation.

CC: jusual@redhat.com
CC: imammedo@redhat.com
CC: akihiko.odaki@daynix.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 | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index e2eb4c3b4a..0320ac2bb3 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -65,6 +65,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),
@@ -1190,6 +1191,20 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
                    name);
 
        return NULL;
+    } /*
+       * With SRIOV and ARI, vfs can have non-zero slot in the conventional
+       * PCI interpretation as all five bits reserved for slot addresses are
+       * also used for function bits for the various vfs. Ignore that case.
+       * It is too early here to check for ARI capabilities in the PCI config
+       * space. Hence, we check for a vf device instead.
+       */
+    else if (!pci_is_vf(pci_dev) &&
+             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 v6 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Akihiko Odaki]

On 2023/06/29 13:07, Ani Sinha 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.
> 
> The change has been tested to not break ARI by instantiating seven vfs on an
> emulated igb device (the maximum number of vfs the linux igb driver supports).
> The vfs are seen to have non-zero device/slot numbers in the conventional
> PCI BDF representation.
> 
> CC: jusual@redhat.com
> CC: imammedo@redhat.com
> CC: akihiko.odaki@daynix.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 | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index e2eb4c3b4a..0320ac2bb3 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -65,6 +65,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),
> @@ -1190,6 +1191,20 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>                      name);
>   
>          return NULL;
> +    } /*
> +       * With SRIOV and ARI, vfs can have non-zero slot in the conventional
> +       * PCI interpretation as all five bits reserved for slot addresses are
> +       * also used for function bits for the various vfs. Ignore that case.
> +       * It is too early here to check for ARI capabilities in the PCI config
> +       * space. Hence, we check for a vf device instead.
> +       */

Why don't just perform this check after the capabilities are set?

Regards,
Akihiko Odaki

> +    else if (!pci_is_vf(pci_dev) &&
> +             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: [PATCH v6 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Ani Sinha]

[-- Attachment #1: Type: text/plain, Size: 2642 bytes --]

On Thu, 29 Jun, 2023, 12:17 pm Akihiko Odaki, <akihiko.odaki@daynix.com>
wrote:

> On 2023/06/29 13:07, Ani Sinha 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.
> >
> > The change has been tested to not break ARI by instantiating seven vfs
> on an
> > emulated igb device (the maximum number of vfs the linux igb driver
> supports).
> > The vfs are seen to have non-zero device/slot numbers in the conventional
> > PCI BDF representation.
> >
> > CC: jusual@redhat.com
> > CC: imammedo@redhat.com
> > CC: akihiko.odaki@daynix.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 | 15 +++++++++++++++
> >   1 file changed, 15 insertions(+)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index e2eb4c3b4a..0320ac2bb3 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -65,6 +65,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),
> > @@ -1190,6 +1191,20 @@ static PCIDevice
> *do_pci_register_device(PCIDevice *pci_dev,
> >                      name);
> >
> >          return NULL;
> > +    } /*
> > +       * With SRIOV and ARI, vfs can have non-zero slot in the
> conventional
> > +       * PCI interpretation as all five bits reserved for slot
> addresses are
> > +       * also used for function bits for the various vfs. Ignore that
> case.
> > +       * It is too early here to check for ARI capabilities in the PCI
> config
> > +       * space. Hence, we check for a vf device instead.
> > +       */
>
> Why don't just perform this check after the capabilities are set?
>

We don't want to allocate resources for wrong device parameters. We want to
error out early. Other checks also are performed at the same place .
Show quoted text

>
>
>
> Regards,
> Akihiko Odaki
>
> > +    else if (!pci_is_vf(pci_dev) &&
> > +             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;
>
>

[-- Attachment #2: Type: text/html, Size: 4451 bytes --]

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

[Akihiko Odaki]

On 2023/06/29 17:05, Ani Sinha wrote:
> 
> 
> On Thu, 29 Jun, 2023, 12:17 pm Akihiko Odaki, <akihiko.odaki@daynix.com 
> <mailto:akihiko.odaki@daynix.com>> wrote:
> 
>     On 2023/06/29 13:07, Ani Sinha 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.
>      >
>      > The change has been tested to not break ARI by instantiating
>     seven vfs on an
>      > emulated igb device (the maximum number of vfs the linux igb
>     driver supports).
>      > The vfs are seen to have non-zero device/slot numbers in the
>     conventional
>      > PCI BDF representation.
>      >
>      > CC: jusual@redhat.com <mailto:jusual@redhat.com>
>      > CC: imammedo@redhat.com <mailto:imammedo@redhat.com>
>      > CC: akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>
>      >
>      > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
>     <https://bugzilla.redhat.com/show_bug.cgi?id=2128929>
>      > Signed-off-by: Ani Sinha <anisinha@redhat.com
>     <mailto:anisinha@redhat.com>>
>      > Reviewed-by: Julia Suvorova <jusual@redhat.com
>     <mailto:jusual@redhat.com>>
>      > ---
>      >   hw/pci/pci.c | 15 +++++++++++++++
>      >   1 file changed, 15 insertions(+)
>      >
>      > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>      > index e2eb4c3b4a..0320ac2bb3 100644
>      > --- a/hw/pci/pci.c
>      > +++ b/hw/pci/pci.c
>      > @@ -65,6 +65,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),
>      > @@ -1190,6 +1191,20 @@ static PCIDevice
>     *do_pci_register_device(PCIDevice *pci_dev,
>      >                      name);
>      >
>      >          return NULL;
>      > +    } /*
>      > +       * With SRIOV and ARI, vfs can have non-zero slot in the
>     conventional
>      > +       * PCI interpretation as all five bits reserved for slot
>     addresses are
>      > +       * also used for function bits for the various vfs. Ignore
>     that case.
>      > +       * It is too early here to check for ARI capabilities in
>     the PCI config
>      > +       * space. Hence, we check for a vf device instead.
>      > +       */
> 
>     Why don't just perform this check after the capabilities are set?
> 
> 
> We don't want to allocate resources for wrong device parameters. We want 
> to error out early. Other checks also are performed at the same place .

It is indeed better to raise an error as early as possible so that we 
can avoid allocation and other operations that will be reverted and may 
go wrong due to the invalid condition. That should be the reason why 
other checks for the address are performed here.

However, in this particular case, we cannot confidently perform the 
check here because it is unknown if the ARI capability will be 
advertised until the device realization code runs. This can justify 
delaying the check after the device realization, unlike the other checks.

> Show quoted text
> 
> 
> 
> 
>     Regards,
>     Akihiko Odaki
> 
>      > +    else if (!pci_is_vf(pci_dev) &&
>      > +             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: [PATCH v6 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Ani Sinha]

> On 29-Jun-2023, at 2:19 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> 
> On 2023/06/29 17:05, Ani Sinha wrote:
>> On Thu, 29 Jun, 2023, 12:17 pm Akihiko Odaki, <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>> wrote:
>>    On 2023/06/29 13:07, Ani Sinha 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.
>>     >
>>     > The change has been tested to not break ARI by instantiating
>>    seven vfs on an
>>     > emulated igb device (the maximum number of vfs the linux igb
>>    driver supports).
>>     > The vfs are seen to have non-zero device/slot numbers in the
>>    conventional
>>     > PCI BDF representation.
>>     >
>>     > CC: jusual@redhat.com <mailto:jusual@redhat.com>
>>     > CC: imammedo@redhat.com <mailto:imammedo@redhat.com>
>>     > CC: akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>
>>     >
>>     > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
>>    <https://bugzilla.redhat.com/show_bug.cgi?id=2128929>
>>     > Signed-off-by: Ani Sinha <anisinha@redhat.com
>>    <mailto:anisinha@redhat.com>>
>>     > Reviewed-by: Julia Suvorova <jusual@redhat.com
>>    <mailto:jusual@redhat.com>>
>>     > ---
>>     >   hw/pci/pci.c | 15 +++++++++++++++
>>     >   1 file changed, 15 insertions(+)
>>     >
>>     > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>     > index e2eb4c3b4a..0320ac2bb3 100644
>>     > --- a/hw/pci/pci.c
>>     > +++ b/hw/pci/pci.c
>>     > @@ -65,6 +65,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),
>>     > @@ -1190,6 +1191,20 @@ static PCIDevice
>>    *do_pci_register_device(PCIDevice *pci_dev,
>>     >                      name);
>>     >
>>     >          return NULL;
>>     > +    } /*
>>     > +       * With SRIOV and ARI, vfs can have non-zero slot in the
>>    conventional
>>     > +       * PCI interpretation as all five bits reserved for slot
>>    addresses are
>>     > +       * also used for function bits for the various vfs. Ignore
>>    that case.
>>     > +       * It is too early here to check for ARI capabilities in
>>    the PCI config
>>     > +       * space. Hence, we check for a vf device instead.
>>     > +       */
>>    Why don't just perform this check after the capabilities are set?
>> We don't want to allocate resources for wrong device parameters. We want to error out early. Other checks also are performed at the same place .
> 
> It is indeed better to raise an error as early as possible so that we can avoid allocation and other operations that will be reverted and may go wrong due to the invalid condition. That should be the reason why other checks for the address are performed here.
> 
> However, in this particular case, we cannot confidently perform the check here because it is unknown if the ARI capability will be advertised until the device realization code runs. This can justify delaying the check after the device realization, unlike the other checks.

Ok so are you proposing that the check we have right before (the check for unoccupied function 0) be also moved? It also uses the same vf approximation for seemingly to support ARI.
Also where do you propose we move the check?

> 
>> Show quoted text
>>    Regards,
>>    Akihiko Odaki
>>     > +    else if (!pci_is_vf(pci_dev) &&
>>     > +             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: [PATCH v6 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Akihiko Odaki]

On 2023/06/29 23:18, Ani Sinha wrote:
> 
> 
>> On 29-Jun-2023, at 2:19 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2023/06/29 17:05, Ani Sinha wrote:
>>> On Thu, 29 Jun, 2023, 12:17 pm Akihiko Odaki, <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>> wrote:
>>>     On 2023/06/29 13:07, Ani Sinha 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.
>>>      >
>>>      > The change has been tested to not break ARI by instantiating
>>>     seven vfs on an
>>>      > emulated igb device (the maximum number of vfs the linux igb
>>>     driver supports).
>>>      > The vfs are seen to have non-zero device/slot numbers in the
>>>     conventional
>>>      > PCI BDF representation.
>>>      >
>>>      > CC: jusual@redhat.com <mailto:jusual@redhat.com>
>>>      > CC: imammedo@redhat.com <mailto:imammedo@redhat.com>
>>>      > CC: akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>
>>>      >
>>>      > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
>>>     <https://bugzilla.redhat.com/show_bug.cgi?id=2128929>
>>>      > Signed-off-by: Ani Sinha <anisinha@redhat.com
>>>     <mailto:anisinha@redhat.com>>
>>>      > Reviewed-by: Julia Suvorova <jusual@redhat.com
>>>     <mailto:jusual@redhat.com>>
>>>      > ---
>>>      >   hw/pci/pci.c | 15 +++++++++++++++
>>>      >   1 file changed, 15 insertions(+)
>>>      >
>>>      > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>      > index e2eb4c3b4a..0320ac2bb3 100644
>>>      > --- a/hw/pci/pci.c
>>>      > +++ b/hw/pci/pci.c
>>>      > @@ -65,6 +65,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),
>>>      > @@ -1190,6 +1191,20 @@ static PCIDevice
>>>     *do_pci_register_device(PCIDevice *pci_dev,
>>>      >                      name);
>>>      >
>>>      >          return NULL;
>>>      > +    } /*
>>>      > +       * With SRIOV and ARI, vfs can have non-zero slot in the
>>>     conventional
>>>      > +       * PCI interpretation as all five bits reserved for slot
>>>     addresses are
>>>      > +       * also used for function bits for the various vfs. Ignore
>>>     that case.
>>>      > +       * It is too early here to check for ARI capabilities in
>>>     the PCI config
>>>      > +       * space. Hence, we check for a vf device instead.
>>>      > +       */
>>>     Why don't just perform this check after the capabilities are set?
>>> We don't want to allocate resources for wrong device parameters. We want to error out early. Other checks also are performed at the same place .
>>
>> It is indeed better to raise an error as early as possible so that we can avoid allocation and other operations that will be reverted and may go wrong due to the invalid condition. That should be the reason why other checks for the address are performed here.
>>
>> However, in this particular case, we cannot confidently perform the check here because it is unknown if the ARI capability will be advertised until the device realization code runs. This can justify delaying the check after the device realization, unlike the other checks.
> 
> Ok so are you proposing that the check we have right before (the check for unoccupied function 0) be also moved? It also uses the same vf approximation for seemingly to support ARI.

No, I don't think the check for function 0 is required to be disabled 
because of the change of addressing caused by ARI, but it is required 
because SR-IOV VF can be added and removed while the PF (function 0) 
remains. I think this check should be performed also when SR-IOV is 
disabled and ARI is enabled.

Thus the check for unoccupied function 0 needs to use pci_is_vf() 
instead of checking ARI capability, and that can happen in 
do_pci_register_device().

> Also where do you propose we move the check?

In pci_qdev_realize(), somewhere after pc->realize() and before option 
ROM loading. See the check for failover pair as an example. I guess it's 
also placed in this region because it needs capability information.

> 
>>
>>> Show quoted text
>>>     Regards,
>>>     Akihiko Odaki
>>>      > +    else if (!pci_is_vf(pci_dev) &&
>>>      > +             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: [PATCH v6 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Michael S. Tsirkin]

On Fri, Jun 30, 2023 at 11:43:15AM +0900, Akihiko Odaki wrote:
> On 2023/06/29 23:18, Ani Sinha wrote:
> > 
> > 
> > > On 29-Jun-2023, at 2:19 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> > > 
> > > On 2023/06/29 17:05, Ani Sinha wrote:
> > > > On Thu, 29 Jun, 2023, 12:17 pm Akihiko Odaki, <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>> wrote:
> > > >     On 2023/06/29 13:07, Ani Sinha 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.
> > > >      >
> > > >      > The change has been tested to not break ARI by instantiating
> > > >     seven vfs on an
> > > >      > emulated igb device (the maximum number of vfs the linux igb
> > > >     driver supports).
> > > >      > The vfs are seen to have non-zero device/slot numbers in the
> > > >     conventional
> > > >      > PCI BDF representation.
> > > >      >
> > > >      > CC: jusual@redhat.com <mailto:jusual@redhat.com>
> > > >      > CC: imammedo@redhat.com <mailto:imammedo@redhat.com>
> > > >      > CC: akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>
> > > >      >
> > > >      > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
> > > >     <https://bugzilla.redhat.com/show_bug.cgi?id=2128929>
> > > >      > Signed-off-by: Ani Sinha <anisinha@redhat.com
> > > >     <mailto:anisinha@redhat.com>>
> > > >      > Reviewed-by: Julia Suvorova <jusual@redhat.com
> > > >     <mailto:jusual@redhat.com>>
> > > >      > ---
> > > >      >   hw/pci/pci.c | 15 +++++++++++++++
> > > >      >   1 file changed, 15 insertions(+)
> > > >      >
> > > >      > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > >      > index e2eb4c3b4a..0320ac2bb3 100644
> > > >      > --- a/hw/pci/pci.c
> > > >      > +++ b/hw/pci/pci.c
> > > >      > @@ -65,6 +65,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),
> > > >      > @@ -1190,6 +1191,20 @@ static PCIDevice
> > > >     *do_pci_register_device(PCIDevice *pci_dev,
> > > >      >                      name);
> > > >      >
> > > >      >          return NULL;
> > > >      > +    } /*
> > > >      > +       * With SRIOV and ARI, vfs can have non-zero slot in the
> > > >     conventional
> > > >      > +       * PCI interpretation as all five bits reserved for slot
> > > >     addresses are
> > > >      > +       * also used for function bits for the various vfs. Ignore
> > > >     that case.
> > > >      > +       * It is too early here to check for ARI capabilities in
> > > >     the PCI config
> > > >      > +       * space. Hence, we check for a vf device instead.
> > > >      > +       */
> > > >     Why don't just perform this check after the capabilities are set?
> > > > We don't want to allocate resources for wrong device parameters. We want to error out early. Other checks also are performed at the same place .
> > > 
> > > It is indeed better to raise an error as early as possible so that we can avoid allocation and other operations that will be reverted and may go wrong due to the invalid condition. That should be the reason why other checks for the address are performed here.
> > > 
> > > However, in this particular case, we cannot confidently perform the check here because it is unknown if the ARI capability will be advertised until the device realization code runs. This can justify delaying the check after the device realization, unlike the other checks.
> > 
> > Ok so are you proposing that the check we have right before (the check for unoccupied function 0) be also moved? It also uses the same vf approximation for seemingly to support ARI.
> 
> No, I don't think the check for function 0 is required to be disabled
> because of the change of addressing caused by ARI, but it is required
> because SR-IOV VF can be added and removed while the PF (function 0)
> remains. I think this check should be performed also when SR-IOV is disabled
> and ARI is enabled.
> 
> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of
> checking ARI capability, and that can happen in do_pci_register_device().
> 
> > Also where do you propose we move the check?
> 
> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM
> loading. See the check for failover pair as an example. I guess it's also
> placed in this region because it needs capability information.

How about instead of spending so much time on working around
incomplete ARI support we actually complete ARI support?



> > 
> > > 
> > > > Show quoted text
> > > >     Regards,
> > > >     Akihiko Odaki
> > > >      > +    else if (!pci_is_vf(pci_dev) &&
> > > >      > +             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: [PATCH v6 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Ani Sinha]

> On 30-Jun-2023, at 8:13 AM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> 
> On 2023/06/29 23:18, Ani Sinha wrote:
>>> On 29-Jun-2023, at 2:19 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>> 
>>> On 2023/06/29 17:05, Ani Sinha wrote:
>>>> On Thu, 29 Jun, 2023, 12:17 pm Akihiko Odaki, <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>> wrote:
>>>>    On 2023/06/29 13:07, Ani Sinha 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.
>>>>     >
>>>>     > The change has been tested to not break ARI by instantiating
>>>>    seven vfs on an
>>>>     > emulated igb device (the maximum number of vfs the linux igb
>>>>    driver supports).
>>>>     > The vfs are seen to have non-zero device/slot numbers in the
>>>>    conventional
>>>>     > PCI BDF representation.
>>>>     >
>>>>     > CC: jusual@redhat.com <mailto:jusual@redhat.com>
>>>>     > CC: imammedo@redhat.com <mailto:imammedo@redhat.com>
>>>>     > CC: akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com>
>>>>     >
>>>>     > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
>>>>    <https://bugzilla.redhat.com/show_bug.cgi?id=2128929>
>>>>     > Signed-off-by: Ani Sinha <anisinha@redhat.com
>>>>    <mailto:anisinha@redhat.com>>
>>>>     > Reviewed-by: Julia Suvorova <jusual@redhat.com
>>>>    <mailto:jusual@redhat.com>>
>>>>     > ---
>>>>     >   hw/pci/pci.c | 15 +++++++++++++++
>>>>     >   1 file changed, 15 insertions(+)
>>>>     >
>>>>     > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>>     > index e2eb4c3b4a..0320ac2bb3 100644
>>>>     > --- a/hw/pci/pci.c
>>>>     > +++ b/hw/pci/pci.c
>>>>     > @@ -65,6 +65,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),
>>>>     > @@ -1190,6 +1191,20 @@ static PCIDevice
>>>>    *do_pci_register_device(PCIDevice *pci_dev,
>>>>     >                      name);
>>>>     >
>>>>     >          return NULL;
>>>>     > +    } /*
>>>>     > +       * With SRIOV and ARI, vfs can have non-zero slot in the
>>>>    conventional
>>>>     > +       * PCI interpretation as all five bits reserved for slot
>>>>    addresses are
>>>>     > +       * also used for function bits for the various vfs. Ignore
>>>>    that case.
>>>>     > +       * It is too early here to check for ARI capabilities in
>>>>    the PCI config
>>>>     > +       * space. Hence, we check for a vf device instead.
>>>>     > +       */
>>>>    Why don't just perform this check after the capabilities are set?
>>>> We don't want to allocate resources for wrong device parameters. We want to error out early. Other checks also are performed at the same place .
>>> 
>>> It is indeed better to raise an error as early as possible so that we can avoid allocation and other operations that will be reverted and may go wrong due to the invalid condition. That should be the reason why other checks for the address are performed here.
>>> 
>>> However, in this particular case, we cannot confidently perform the check here because it is unknown if the ARI capability will be advertised until the device realization code runs. This can justify delaying the check after the device realization, unlike the other checks.
>> Ok so are you proposing that the check we have right before (the check for unoccupied function 0) be also moved? It also uses the same vf approximation for seemingly to support ARI.
> 
> No, I don't think the check for function 0 is required to be disabled because of the change of addressing caused by ARI, but it is required because SR-IOV VF can be added and removed while the PF (function 0) remains. I think this check should be performed also when SR-IOV is disabled and ARI is enabled.

OK I am a little confused with your explanation here. I understand the non-ARI case - to detect the PF we need to have function 0 available. Looking at the comment in pci_init_multifunction() it seems in ARI case, we can simply have a vf in function 0 (conventional interpretation) as well. Hence we need to ignore vfs both in ARI and non-ARI cases. This is what I understood.

> 
> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
> 
>> Also where do you propose we move the check?
> 
> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.

Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:

-device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0

The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.

> See the check for failover pair as an example. I guess it's also placed in this region because it needs capability information.
> 
>>> 
>>>> Show quoted text
>>>>    Regards,
>>>>    Akihiko Odaki
>>>>     > +    else if (!pci_is_vf(pci_dev) &&
>>>>     > +             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: [PATCH v6 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Michael S. Tsirkin]

On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
> > 
> > Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
> > 
> >> Also where do you propose we move the check?
> > 
> > In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.
> 
> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
> 
> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
> 
> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.

I think it's allowed because it expects you to hotplug function 0 later,
no?

I am quite worried about all this work going into blocking
what we think is disallowed configurations. We should have
maybe blocked them originally, but now that we didn't
there's a non zero chance of regressions, and the benefit
is not guaranteed.

-- 
MST

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

[Ani Sinha]

> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
>>> 
>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
>>> 
>>>> Also where do you propose we move the check?
>>> 
>>> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.
>> 
>> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
>> 
>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
>> 
>> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.
> 
> I think it's allowed because it expects you to hotplug function 0 later,

This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.

> no?
> 
> I am quite worried about all this work going into blocking
> what we think is disallowed configurations. We should have
> maybe blocked them originally, but now that we didn't
> there's a non zero chance of regressions,

Sigh, no medals here for being brave :-)

> and the benefit
> is not guaranteed.
> 
> -- 
> MST
> 

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

[Michael S. Tsirkin]

On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:
> 
> 
> > On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 
> > On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
> >>> 
> >>> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
> >>> 
> >>>> Also where do you propose we move the check?
> >>> 
> >>> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.
> >> 
> >> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
> >> 
> >> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
> >> 
> >> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.
> > 
> > I think it's allowed because it expects you to hotplug function 0 later,
> 
> This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.

yes but if you later add a device with ARI and with next field pointing
slot 2 guest will suddently find both.

> > no?
> > 
> > I am quite worried about all this work going into blocking
> > what we think is disallowed configurations. We should have
> > maybe blocked them originally, but now that we didn't
> > there's a non zero chance of regressions,
> 
> Sigh,

There's value in patches 1-4 I think - the last patch helped you find
these. so there's value in this work.

> no medals here for being brave :-)

Try removing support for a 3.5mm jack next. Oh wait ...

> > and the benefit
> > is not guaranteed.
> > 
> > -- 
> > MST
> > 

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

[Ani Sinha]

> On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:
>> 
>> 
>>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> 
>>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
>>>>> 
>>>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
>>>>> 
>>>>>> Also where do you propose we move the check?
>>>>> 
>>>>> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.
>>>> 
>>>> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
>>>> 
>>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
>>>> 
>>>> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.
>>> 
>>> I think it's allowed because it expects you to hotplug function 0 later,
>> 
>> This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.
> 
> yes but if you later add a device with ARI and with next field pointing
> slot 2 guest will suddently find both.

Hmm, I tried this:

-device pcie-root-port,id=p \
-device igb,bus=p,addr=0x2.0x0 \
-device igb,bus=p,addr=0x0.0x0 \

The guest only found the second igb device not the first. You can try too.

> 
>>> no?
>>> 
>>> I am quite worried about all this work going into blocking
>>> what we think is disallowed configurations. We should have
>>> maybe blocked them originally, but now that we didn't
>>> there's a non zero chance of regressions,
>> 
>> Sigh,
> 
> There's value in patches 1-4 I think - the last patch helped you find
> these. so there's value in this work.
> 
>> no medals here for being brave :-)
> 
> Try removing support for a 3.5mm jack next. Oh wait ...

Indeed. Everyone uses bluetooth these days. I for one is happy that the jack is gone (and they were bold enough to do it while Samsung and others still carry the useless port ) :-)
 
> 
>>> and the benefit
>>> is not guaranteed.
>>> 
>>> -- 
>>> MST

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

[Michael S. Tsirkin]

On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:
> 
> 
> > On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 
> > On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:
> >> 
> >> 
> >>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>> 
> >>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
> >>>>> 
> >>>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
> >>>>> 
> >>>>>> Also where do you propose we move the check?
> >>>>> 
> >>>>> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.
> >>>> 
> >>>> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
> >>>> 
> >>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
> >>>> 
> >>>> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.
> >>> 
> >>> I think it's allowed because it expects you to hotplug function 0 later,
> >> 
> >> This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.
> > 
> > yes but if you later add a device with ARI and with next field pointing
> > slot 2 guest will suddently find both.
> 
> Hmm, I tried this:
> 
> -device pcie-root-port,id=p \
> -device igb,bus=p,addr=0x2.0x0 \
> -device igb,bus=p,addr=0x0.0x0 \
> 
> The guest only found the second igb device not the first. You can try too.

Because next parameter in pcie_ari_init does not match.


> > 
> >>> no?
> >>> 
> >>> I am quite worried about all this work going into blocking
> >>> what we think is disallowed configurations. We should have
> >>> maybe blocked them originally, but now that we didn't
> >>> there's a non zero chance of regressions,
> >> 
> >> Sigh,
> > 
> > There's value in patches 1-4 I think - the last patch helped you find
> > these. so there's value in this work.
> > 
> >> no medals here for being brave :-)
> > 
> > Try removing support for a 3.5mm jack next. Oh wait ...
> 
> Indeed. Everyone uses bluetooth these days. I for one is happy that the jack is gone (and they were bold enough to do it while Samsung and others still carry the useless port ) :-)
>  
> > 
> >>> and the benefit
> >>> is not guaranteed.
> >>> 
> >>> -- 
> >>> MST

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

[Ani Sinha]

> On 30-Jun-2023, at 3:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:
>> 
>> 
>>> On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> 
>>> On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:
>>>> 
>>>> 
>>>>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>> 
>>>>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
>>>>>>> 
>>>>>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
>>>>>>> 
>>>>>>>> Also where do you propose we move the check?
>>>>>>> 
>>>>>>> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.
>>>>>> 
>>>>>> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
>>>>>> 
>>>>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
>>>>>> 
>>>>>> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.
>>>>> 
>>>>> I think it's allowed because it expects you to hotplug function 0 later,
>>>> 
>>>> This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.
>>> 
>>> yes but if you later add a device with ARI and with next field pointing
>>> slot 2 guest will suddently find both.
>> 
>> Hmm, I tried this:
>> 
>> -device pcie-root-port,id=p \
>> -device igb,bus=p,addr=0x2.0x0 \
>> -device igb,bus=p,addr=0x0.0x0 \
>> 
>> The guest only found the second igb device not the first. You can try too.
> 
> Because next parameter in pcie_ari_init does not match.

OK send me a command line that I can test it with. I can’t come up with a case that actually works in practice.

> 
> 
>>> 
>>>>> no?
>>>>> 
>>>>> I am quite worried about all this work going into blocking
>>>>> what we think is disallowed configurations. We should have
>>>>> maybe blocked them originally, but now that we didn't
>>>>> there's a non zero chance of regressions,
>>>> 
>>>> Sigh,
>>> 
>>> There's value in patches 1-4 I think - the last patch helped you find
>>> these. so there's value in this work.
>>> 
>>>> no medals here for being brave :-)
>>> 
>>> Try removing support for a 3.5mm jack next. Oh wait ...
>> 
>> Indeed. Everyone uses bluetooth these days. I for one is happy that the jack is gone (and they were bold enough to do it while Samsung and others still carry the useless port ) :-)
>> 
>>> 
>>>>> and the benefit
>>>>> is not guaranteed.
>>>>> 
>>>>> -- 
>>>>> MST

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

[Michael S. Tsirkin]

On Fri, Jun 30, 2023 at 04:07:32PM +0530, Ani Sinha wrote:
> 
> 
> > On 30-Jun-2023, at 3:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 
> > On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:
> >> 
> >> 
> >>> On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>> 
> >>> On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:
> >>>> 
> >>>> 
> >>>>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>> 
> >>>>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
> >>>>>>> 
> >>>>>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
> >>>>>>> 
> >>>>>>>> Also where do you propose we move the check?
> >>>>>>> 
> >>>>>>> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.
> >>>>>> 
> >>>>>> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
> >>>>>> 
> >>>>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
> >>>>>> 
> >>>>>> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.
> >>>>> 
> >>>>> I think it's allowed because it expects you to hotplug function 0 later,
> >>>> 
> >>>> This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.
> >>> 
> >>> yes but if you later add a device with ARI and with next field pointing
> >>> slot 2 guest will suddently find both.
> >> 
> >> Hmm, I tried this:
> >> 
> >> -device pcie-root-port,id=p \
> >> -device igb,bus=p,addr=0x2.0x0 \
> >> -device igb,bus=p,addr=0x0.0x0 \
> >> 
> >> The guest only found the second igb device not the first. You can try too.
> > 
> > Because next parameter in pcie_ari_init does not match.
> 
> OK send me a command line that I can test it with. I can’t come up with a case that actually works in practice.


you need to patch igb to pass 2 as next parameter.
maybe add a property to make it easier to play with.

> > 
> > 
> >>> 
> >>>>> no?
> >>>>> 
> >>>>> I am quite worried about all this work going into blocking
> >>>>> what we think is disallowed configurations. We should have
> >>>>> maybe blocked them originally, but now that we didn't
> >>>>> there's a non zero chance of regressions,
> >>>> 
> >>>> Sigh,
> >>> 
> >>> There's value in patches 1-4 I think - the last patch helped you find
> >>> these. so there's value in this work.
> >>> 
> >>>> no medals here for being brave :-)
> >>> 
> >>> Try removing support for a 3.5mm jack next. Oh wait ...
> >> 
> >> Indeed. Everyone uses bluetooth these days. I for one is happy that the jack is gone (and they were bold enough to do it while Samsung and others still carry the useless port ) :-)
> >> 
> >>> 
> >>>>> and the benefit
> >>>>> is not guaranteed.
> >>>>> 
> >>>>> -- 
> >>>>> MST

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

[Ani Sinha]

> On 30-Jun-2023, at 4:10 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Fri, Jun 30, 2023 at 04:07:32PM +0530, Ani Sinha wrote:
>> 
>> 
>>> On 30-Jun-2023, at 3:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> 
>>> On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:
>>>> 
>>>> 
>>>>> On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>> 
>>>>> On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:
>>>>>> 
>>>>>> 
>>>>>>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>> 
>>>>>>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
>>>>>>>>> 
>>>>>>>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
>>>>>>>>> 
>>>>>>>>>> Also where do you propose we move the check?
>>>>>>>>> 
>>>>>>>>> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.
>>>>>>>> 
>>>>>>>> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
>>>>>>>> 
>>>>>>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
>>>>>>>> 
>>>>>>>> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.
>>>>>>> 
>>>>>>> I think it's allowed because it expects you to hotplug function 0 later,
>>>>>> 
>>>>>> This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.
>>>>> 
>>>>> yes but if you later add a device with ARI and with next field pointing
>>>>> slot 2 guest will suddently find both.
>>>> 
>>>> Hmm, I tried this:
>>>> 
>>>> -device pcie-root-port,id=p \
>>>> -device igb,bus=p,addr=0x2.0x0 \
>>>> -device igb,bus=p,addr=0x0.0x0 \
>>>> 
>>>> The guest only found the second igb device not the first. You can try too.
>>> 
>>> Because next parameter in pcie_ari_init does not match.
>> 
>> OK send me a command line that I can test it with. I can’t come up with a case that actually works in practice.
> 
> 
> you need to patch igb to pass 2 as next parameter.
> maybe add a property to make it easier to play with.

Yes but without patching, could I not change the device parameters like

-device pcie-root-port,id=p \
-device igb,bus=p,addr=0x1.0x0 \
-device igb,bus=p,addr=0x0.0x0 \

I tried the above and it did not work.

> 
>>> 
>>> 
>>>>> 
>>>>>>> no?
>>>>>>> 
>>>>>>> I am quite worried about all this work going into blocking
>>>>>>> what we think is disallowed configurations. We should have
>>>>>>> maybe blocked them originally, but now that we didn't
>>>>>>> there's a non zero chance of regressions,
>>>>>> 
>>>>>> Sigh,
>>>>> 
>>>>> There's value in patches 1-4 I think - the last patch helped you find
>>>>> these. so there's value in this work.
>>>>> 
>>>>>> no medals here for being brave :-)
>>>>> 
>>>>> Try removing support for a 3.5mm jack next. Oh wait ...
>>>> 
>>>> Indeed. Everyone uses bluetooth these days. I for one is happy that the jack is gone (and they were bold enough to do it while Samsung and others still carry the useless port ) :-)
>>>> 
>>>>> 
>>>>>>> and the benefit
>>>>>>> is not guaranteed.
>>>>>>> 
>>>>>>> -- 
>>>>>>> MST

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

[Ani Sinha]

> On 30-Jun-2023, at 4:10 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Fri, Jun 30, 2023 at 04:07:32PM +0530, Ani Sinha wrote:
>> 
>> 
>>> On 30-Jun-2023, at 3:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> 
>>> On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:
>>>> 
>>>> 
>>>>> On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>> 
>>>>> On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:
>>>>>> 
>>>>>> 
>>>>>>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>> 
>>>>>>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
>>>>>>>>> 
>>>>>>>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
>>>>>>>>> 
>>>>>>>>>> Also where do you propose we move the check?
>>>>>>>>> 
>>>>>>>>> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.
>>>>>>>> 
>>>>>>>> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
>>>>>>>> 
>>>>>>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
>>>>>>>> 
>>>>>>>> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.
>>>>>>> 
>>>>>>> I think it's allowed because it expects you to hotplug function 0 later,
>>>>>> 
>>>>>> This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.
>>>>> 
>>>>> yes but if you later add a device with ARI and with next field pointing
>>>>> slot 2 guest will suddently find both.
>>>> 
>>>> Hmm, I tried this:
>>>> 
>>>> -device pcie-root-port,id=p \
>>>> -device igb,bus=p,addr=0x2.0x0 \
>>>> -device igb,bus=p,addr=0x0.0x0 \
>>>> 
>>>> The guest only found the second igb device not the first. You can try too.
>>> 
>>> Because next parameter in pcie_ari_init does not match.
>> 
>> OK send me a command line that I can test it with. I can’t come up with a case that actually works in practice.
> 
> 
> you need to patch igb to pass 2 as next parameter.

OK I tried this too along with

-device pcie-root-port,id=p \
-device igb,bus=p,addr=0x2.0x0 \
-device igb,bus=p,addr=0x0.0x0 \

Still same result. The guest only detects the last one.

> maybe add a property to make it easier to play with.
> 
>>> 
>>> 
>>>>> 
>>>>>>> no?
>>>>>>> 
>>>>>>> I am quite worried about all this work going into blocking
>>>>>>> what we think is disallowed configurations. We should have
>>>>>>> maybe blocked them originally, but now that we didn't
>>>>>>> there's a non zero chance of regressions,
>>>>>> 
>>>>>> Sigh,
>>>>> 
>>>>> There's value in patches 1-4 I think - the last patch helped you find
>>>>> these. so there's value in this work.
>>>>> 
>>>>>> no medals here for being brave :-)
>>>>> 
>>>>> Try removing support for a 3.5mm jack next. Oh wait ...
>>>> 
>>>> Indeed. Everyone uses bluetooth these days. I for one is happy that the jack is gone (and they were bold enough to do it while Samsung and others still carry the useless port ) :-)
>>>> 
>>>>> 
>>>>>>> and the benefit
>>>>>>> is not guaranteed.
>>>>>>> 
>>>>>>> -- 
>>>>>>> MST

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

[Akihiko Odaki]

On 2023/06/30 19:37, Ani Sinha wrote:
> 
> 
>> On 30-Jun-2023, at 3:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:
>>>
>>>
>>>> On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>
>>>> On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:
>>>>>
>>>>>
>>>>>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>
>>>>>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
>>>>>>>>
>>>>>>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
>>>>>>>>
>>>>>>>>> Also where do you propose we move the check?
>>>>>>>>
>>>>>>>> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.
>>>>>>>
>>>>>>> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
>>>>>>>
>>>>>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
>>>>>>>
>>>>>>> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.
>>>>>>
>>>>>> I think it's allowed because it expects you to hotplug function 0 later,
>>>>>
>>>>> This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.
>>>>
>>>> yes but if you later add a device with ARI and with next field pointing
>>>> slot 2 guest will suddently find both.
>>>
>>> Hmm, I tried this:
>>>
>>> -device pcie-root-port,id=p \
>>> -device igb,bus=p,addr=0x2.0x0 \
>>> -device igb,bus=p,addr=0x0.0x0 \
>>>
>>> The guest only found the second igb device not the first. You can try too.
>>
>> Because next parameter in pcie_ari_init does not match.
> 
> OK send me a command line that I can test it with. I can’t come up with a case that actually works in practice.

I don't think there is one because the code for PCI multifunction does 
not care ARI. In my opinion, we need yet another check to make 
non-SR-IOV multifunction and ARI capability mutually exclusive; if a 
function has the ARI capability and it is not a VF, an attempt to assign 
non-zero function number for it should fail.

But it should be a distinct check as it will need to check the function 
number bits.

> 
>>
>>
>>>>
>>>>>> no?
>>>>>>
>>>>>> I am quite worried about all this work going into blocking
>>>>>> what we think is disallowed configurations. We should have
>>>>>> maybe blocked them originally, but now that we didn't
>>>>>> there's a non zero chance of regressions,
>>>>>
>>>>> Sigh,
>>>>
>>>> There's value in patches 1-4 I think - the last patch helped you find
>>>> these. so there's value in this work.
>>>>
>>>>> no medals here for being brave :-)
>>>>
>>>> Try removing support for a 3.5mm jack next. Oh wait ...
>>>
>>> Indeed. Everyone uses bluetooth these days. I for one is happy that the jack is gone (and they were bold enough to do it while Samsung and others still carry the useless port ) :-)

Hello from a guy using a shiny M2 Macbook Air carrying the legacy jack 
with a 100-yen earphone. Even people who ported Linux to this machine 
spent efforts to get the jack to work on Linux ;)

>>>
>>>>
>>>>>> and the benefit
>>>>>> is not guaranteed.
>>>>>>
>>>>>> -- 
>>>>>> MST
> 

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

[Ani Sinha]

> On 30-Jun-2023, at 5:06 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> 
> On 2023/06/30 19:37, Ani Sinha wrote:
>>> On 30-Jun-2023, at 3:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> 
>>> On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:
>>>> 
>>>> 
>>>>> On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>> 
>>>>> On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:
>>>>>> 
>>>>>> 
>>>>>>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>> 
>>>>>>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
>>>>>>>>> 
>>>>>>>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
>>>>>>>>> 
>>>>>>>>>> Also where do you propose we move the check?
>>>>>>>>> 
>>>>>>>>> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.
>>>>>>>> 
>>>>>>>> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
>>>>>>>> 
>>>>>>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
>>>>>>>> 
>>>>>>>> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.
>>>>>>> 
>>>>>>> I think it's allowed because it expects you to hotplug function 0 later,
>>>>>> 
>>>>>> This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.
>>>>> 
>>>>> yes but if you later add a device with ARI and with next field pointing
>>>>> slot 2 guest will suddently find both.
>>>> 
>>>> Hmm, I tried this:
>>>> 
>>>> -device pcie-root-port,id=p \
>>>> -device igb,bus=p,addr=0x2.0x0 \
>>>> -device igb,bus=p,addr=0x0.0x0 \
>>>> 
>>>> The guest only found the second igb device not the first. You can try too.
>>> 
>>> Because next parameter in pcie_ari_init does not match.
>> OK send me a command line that I can test it with. I can’t come up with a case that actually works in practice.
> 
> I don't think there is one because the code for PCI multifunction does not care ARI. In my opinion, we need yet another check to make non-SR-IOV multifunction and ARI capability mutually exclusive; if a function has the ARI capability and it is not a VF, an attempt to assign non-zero function number for it should fail.
> 
> But it should be a distinct check as it will need to check the function number bits.

I will leave this for mst to comment.

> 
>>> 
>>> 
>>>>> 
>>>>>>> no?
>>>>>>> 
>>>>>>> I am quite worried about all this work going into blocking
>>>>>>> what we think is disallowed configurations. We should have
>>>>>>> maybe blocked them originally, but now that we didn't
>>>>>>> there's a non zero chance of regressions,
>>>>>> 
>>>>>> Sigh,
>>>>> 
>>>>> There's value in patches 1-4 I think - the last patch helped you find
>>>>> these. so there's value in this work.
>>>>> 
>>>>>> no medals here for being brave :-)
>>>>> 
>>>>> Try removing support for a 3.5mm jack next. Oh wait ...
>>>> 
>>>> Indeed. Everyone uses bluetooth these days. I for one is happy that the jack is gone (and they were bold enough to do it while Samsung and others still carry the useless port ) :-)
> 
> Hello from a guy using a shiny M2 Macbook Air carrying the legacy jack with a 100-yen earphone. Even people who ported Linux to this machine spent efforts to get the jack to work on Linux ;)

Yes that is because the jack exists. If they did not make the jack work, it would be a broken device. With bluetooth being so pervasive, its mostly a matter of time before most devices stop shipping with a 3.5mm.

All I am saying is sometimes one needs to be bold to bring changes. Otherwise changes never come. Status quo is not an option IMHO. 

> 
>>>> 
>>>>> 
>>>>>>> and the benefit
>>>>>>> is not guaranteed.
>>>>>>> 
>>>>>>> -- 
>>>>>>> MST

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

[Akihiko Odaki]

On 2023/06/30 20:36, Akihiko Odaki wrote:
> On 2023/06/30 19:37, Ani Sinha wrote:
>>
>>
>>> On 30-Jun-2023, at 3:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>
>>> On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:
>>>>
>>>>
>>>>> On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>
>>>>> On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:
>>>>>>
>>>>>>
>>>>>>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> 
>>>>>>> wrote:
>>>>>>>
>>>>>>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
>>>>>>>>>
>>>>>>>>> Thus the check for unoccupied function 0 needs to use 
>>>>>>>>> pci_is_vf() instead of checking ARI capability, and that can 
>>>>>>>>> happen in do_pci_register_device().
>>>>>>>>>
>>>>>>>>>> Also where do you propose we move the check?
>>>>>>>>>
>>>>>>>>> In pci_qdev_realize(), somewhere after pc->realize() and before 
>>>>>>>>> option ROM loading.
>>>>>>>>
>>>>>>>> Hmm, I tried this. The issue here is something like this would 
>>>>>>>> be now allowed since the PF has ARI capability:
>>>>>>>>
>>>>>>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
>>>>>>>>
>>>>>>>> The above should not be allowed and when used, we do not see the 
>>>>>>>> igb ethernet device from the guest OS.
>>>>>>>
>>>>>>> I think it's allowed because it expects you to hotplug function 0 
>>>>>>> later,
>>>>>>
>>>>>> This is about the igb device being plugged into the non-zero slot 
>>>>>> of the pci-root-port. The guest OS ignores it.
>>>>>
>>>>> yes but if you later add a device with ARI and with next field 
>>>>> pointing
>>>>> slot 2 guest will suddently find both.
>>>>
>>>> Hmm, I tried this:
>>>>
>>>> -device pcie-root-port,id=p \
>>>> -device igb,bus=p,addr=0x2.0x0 \
>>>> -device igb,bus=p,addr=0x0.0x0 \
>>>>
>>>> The guest only found the second igb device not the first. You can 
>>>> try too.
>>>
>>> Because next parameter in pcie_ari_init does not match.
>>
>> OK send me a command line that I can test it with. I can’t come up 
>> with a case that actually works in practice.
> 
> I don't think there is one because the code for PCI multifunction does 
> not care ARI. In my opinion, we need yet another check to make 
> non-SR-IOV multifunction and ARI capability mutually exclusive; if a 
> function has the ARI capability and it is not a VF, an attempt to assign 
> non-zero function number for it should fail.

No, the more straightforward way to fix this problem is to check the 
device function number is less than the next function number advertised 
with ARI.

> 
> But it should be a distinct check as it will need to check the function 
> number bits.

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

[Ani Sinha]

> On 30-Jun-2023, at 5:25 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> 
> On 2023/06/30 20:36, Akihiko Odaki wrote:
>> On 2023/06/30 19:37, Ani Sinha wrote:
>>> 
>>> 
>>>> On 30-Jun-2023, at 3:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> 
>>>> On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:
>>>>> 
>>>>> 
>>>>>> On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>> 
>>>>>> On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:
>>>>>>> 
>>>>>>> 
>>>>>>>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>> 
>>>>>>>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
>>>>>>>>>> 
>>>>>>>>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
>>>>>>>>>> 
>>>>>>>>>>> Also where do you propose we move the check?
>>>>>>>>>> 
>>>>>>>>>> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.
>>>>>>>>> 
>>>>>>>>> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
>>>>>>>>> 
>>>>>>>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
>>>>>>>>> 
>>>>>>>>> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.
>>>>>>>> 
>>>>>>>> I think it's allowed because it expects you to hotplug function 0 later,
>>>>>>> 
>>>>>>> This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.
>>>>>> 
>>>>>> yes but if you later add a device with ARI and with next field pointing
>>>>>> slot 2 guest will suddently find both.
>>>>> 
>>>>> Hmm, I tried this:
>>>>> 
>>>>> -device pcie-root-port,id=p \
>>>>> -device igb,bus=p,addr=0x2.0x0 \
>>>>> -device igb,bus=p,addr=0x0.0x0 \
>>>>> 
>>>>> The guest only found the second igb device not the first. You can try too.
>>>> 
>>>> Because next parameter in pcie_ari_init does not match.
>>> 
>>> OK send me a command line that I can test it with. I can’t come up with a case that actually works in practice.
>> I don't think there is one because the code for PCI multifunction does not care ARI. In my opinion, we need yet another check to make non-SR-IOV multifunction and ARI capability mutually exclusive; if a function has the ARI capability and it is not a VF, an attempt to assign non-zero function number for it should fail.
> 
> No, the more straightforward way to fix this problem is to check the device function number is less than the next function number advertised with ARI.
> 

Personally I would leave the check for ARI capable devices to someone else. I am ok with moving the check to pci_qdev_realize() and I verified that both unit tests and the breaking test case for BZ 2128929 is caught. I have also verified that the change does not break igb vf generation. 
If there is any interest to push this change, I will spin a new version with tags for test fixes added and the rework for this patch with the check moved to the new location as you had suggested.


>> But it should be a distinct check as it will need to check the function number bits.

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

[Akihiko Odaki]

On 2023/06/30 22:56, Ani Sinha wrote:
> 
> 
>> On 30-Jun-2023, at 5:25 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2023/06/30 20:36, Akihiko Odaki wrote:
>>> On 2023/06/30 19:37, Ani Sinha wrote:
>>>>
>>>>
>>>>> On 30-Jun-2023, at 3:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>
>>>>> On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:
>>>>>>
>>>>>>
>>>>>>> On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>
>>>>>>> On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>>>
>>>>>>>>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
>>>>>>>>>>>
>>>>>>>>>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
>>>>>>>>>>>
>>>>>>>>>>>> Also where do you propose we move the check?
>>>>>>>>>>>
>>>>>>>>>>> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.
>>>>>>>>>>
>>>>>>>>>> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
>>>>>>>>>>
>>>>>>>>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
>>>>>>>>>>
>>>>>>>>>> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.
>>>>>>>>>
>>>>>>>>> I think it's allowed because it expects you to hotplug function 0 later,
>>>>>>>>
>>>>>>>> This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.
>>>>>>>
>>>>>>> yes but if you later add a device with ARI and with next field pointing
>>>>>>> slot 2 guest will suddently find both.
>>>>>>
>>>>>> Hmm, I tried this:
>>>>>>
>>>>>> -device pcie-root-port,id=p \
>>>>>> -device igb,bus=p,addr=0x2.0x0 \
>>>>>> -device igb,bus=p,addr=0x0.0x0 \
>>>>>>
>>>>>> The guest only found the second igb device not the first. You can try too.
>>>>>
>>>>> Because next parameter in pcie_ari_init does not match.
>>>>
>>>> OK send me a command line that I can test it with. I can’t come up with a case that actually works in practice.
>>> I don't think there is one because the code for PCI multifunction does not care ARI. In my opinion, we need yet another check to make non-SR-IOV multifunction and ARI capability mutually exclusive; if a function has the ARI capability and it is not a VF, an attempt to assign non-zero function number for it should fail.
>>
>> No, the more straightforward way to fix this problem is to check the device function number is less than the next function number advertised with ARI.
>>
> 
> Personally I would leave the check for ARI capable devices to someone else. I am ok with moving the check to pci_qdev_realize() and I verified that both unit tests and the breaking test case for BZ 2128929 is caught. I have also verified that the change does not break igb vf generation.
> If there is any interest to push this change, I will spin a new version with tags for test fixes added and the rework for this patch with the check moved to the new location as you had suggested.

I sent a patch series to add ARI next function number check:
https://lore.kernel.org/qemu-devel/20230701070133.24877-1-akihiko.odaki@daynix.com/

Yes, I want the slot number restriction to be enforced. If it worries 
you too much for regressions, you may implement it as a warning first 
and then turn it a hard error when the next development phase starts.

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

[Michael S. Tsirkin]

On Sat, Jul 01, 2023 at 04:09:31PM +0900, Akihiko Odaki wrote:
> Yes, I want the slot number restriction to be enforced. If it worries you
> too much for regressions, you may implement it as a warning first and then
> turn it a hard error when the next development phase starts.

That's not a bad idea.

-- 
MST

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

[Ani Sinha]

> On 02-Jul-2023, at 10:29 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Sat, Jul 01, 2023 at 04:09:31PM +0900, Akihiko Odaki wrote:
>> Yes, I want the slot number restriction to be enforced. If it worries you
>> too much for regressions, you may implement it as a warning first and then
>> turn it a hard error when the next development phase starts.
> 
> That's not a bad idea.

If we had not enforced the check strongly, the tests that we fixed would not get noticed.

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

[Akihiko Odaki]

On 2023/07/03 15:08, Ani Sinha wrote:
> 
> 
>> On 02-Jul-2023, at 10:29 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Sat, Jul 01, 2023 at 04:09:31PM +0900, Akihiko Odaki wrote:
>>> Yes, I want the slot number restriction to be enforced. If it worries you
>>> too much for regressions, you may implement it as a warning first and then
>>> turn it a hard error when the next development phase starts.
>>
>> That's not a bad idea.
> 
> If we had not enforced the check strongly, the tests that we fixed would not get noticed.
> 

Perhaps so, but we don't have much time before feature freeze. I rather 
want to see the check implemented as warning in 8.1 instead of delaying 
the initial implementation of the check after 8.1 (though I worry if 
it's already too late for 8.1.)

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

[Ani Sinha]

> On 04-Jul-2023, at 10:31 AM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> 
> On 2023/07/03 15:08, Ani Sinha wrote:
>>> On 02-Jul-2023, at 10:29 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> 
>>> On Sat, Jul 01, 2023 at 04:09:31PM +0900, Akihiko Odaki wrote:
>>>> Yes, I want the slot number restriction to be enforced. If it worries you
>>>> too much for regressions, you may implement it as a warning first and then
>>>> turn it a hard error when the next development phase starts.
>>> 
>>> That's not a bad idea.
>> If we had not enforced the check strongly, the tests that we fixed would not get noticed.
> 
> Perhaps so, but we don't have much time before feature freeze. I rather want to see the check implemented as warning in 8.1 instead of delaying the initial implementation of the check after 8.1 (though I worry if it's already too late for 8.1.)

The feature hard freeze window starts from 12th of next week. So I am still debating whether to keep the hard check or just have a warning. If the hard check causes regressions, we can always revert it to a warning later.

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

[Ani Sinha]

> On 04-Jul-2023, at 11:09 AM, Ani Sinha <anisinha@redhat.com> wrote:
> 
> 
> 
>> On 04-Jul-2023, at 10:31 AM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>> 
>> On 2023/07/03 15:08, Ani Sinha wrote:
>>>> On 02-Jul-2023, at 10:29 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> 
>>>> On Sat, Jul 01, 2023 at 04:09:31PM +0900, Akihiko Odaki wrote:
>>>>> Yes, I want the slot number restriction to be enforced. If it worries you
>>>>> too much for regressions, you may implement it as a warning first and then
>>>>> turn it a hard error when the next development phase starts.
>>>> 
>>>> That's not a bad idea.
>>> If we had not enforced the check strongly, the tests that we fixed would not get noticed.
>> 
>> Perhaps so, but we don't have much time before feature freeze. I rather want to see the check implemented as warning in 8.1 instead of delaying the initial implementation of the check after 8.1 (though I worry if it's already too late for 8.1.)
> 
> The feature hard freeze window starts from 12th of next week. So I am still debating whether to keep the hard check or just have a warning. If the hard check causes regressions, we can always revert it to a warning later.

mst?

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

[Michael S. Tsirkin]

On Tue, Jul 04, 2023 at 04:03:54PM +0530, Ani Sinha wrote:
> 
> 
> > On 04-Jul-2023, at 11:09 AM, Ani Sinha <anisinha@redhat.com> wrote:
> > 
> > 
> > 
> >> On 04-Jul-2023, at 10:31 AM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >> 
> >> On 2023/07/03 15:08, Ani Sinha wrote:
> >>>> On 02-Jul-2023, at 10:29 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>> 
> >>>> On Sat, Jul 01, 2023 at 04:09:31PM +0900, Akihiko Odaki wrote:
> >>>>> Yes, I want the slot number restriction to be enforced. If it worries you
> >>>>> too much for regressions, you may implement it as a warning first and then
> >>>>> turn it a hard error when the next development phase starts.
> >>>> 
> >>>> That's not a bad idea.
> >>> If we had not enforced the check strongly, the tests that we fixed would not get noticed.
> >> 
> >> Perhaps so, but we don't have much time before feature freeze. I rather want to see the check implemented as warning in 8.1 instead of delaying the initial implementation of the check after 8.1 (though I worry if it's already too late for 8.1.)
> > 
> > The feature hard freeze window starts from 12th of next week. So I am still debating whether to keep the hard check or just have a warning. If the hard check causes regressions, we can always revert it to a warning later.
> 
> mst?

I'd go for a warning now. Let's see what triggers for users without
actually breaking things too badly for them.

-- 
MST

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

[Ani Sinha]

> On 04-Jul-2023, at 4:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Tue, Jul 04, 2023 at 04:03:54PM +0530, Ani Sinha wrote:
>> 
>> 
>>> On 04-Jul-2023, at 11:09 AM, Ani Sinha <anisinha@redhat.com> wrote:
>>> 
>>> 
>>> 
>>>> On 04-Jul-2023, at 10:31 AM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>> 
>>>> On 2023/07/03 15:08, Ani Sinha wrote:
>>>>>> On 02-Jul-2023, at 10:29 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>> 
>>>>>> On Sat, Jul 01, 2023 at 04:09:31PM +0900, Akihiko Odaki wrote:
>>>>>>> Yes, I want the slot number restriction to be enforced. If it worries you
>>>>>>> too much for regressions, you may implement it as a warning first and then
>>>>>>> turn it a hard error when the next development phase starts.
>>>>>> 
>>>>>> That's not a bad idea.
>>>>> If we had not enforced the check strongly, the tests that we fixed would not get noticed.
>>>> 
>>>> Perhaps so, but we don't have much time before feature freeze. I rather want to see the check implemented as warning in 8.1 instead of delaying the initial implementation of the check after 8.1 (though I worry if it's already too late for 8.1.)
>>> 
>>> The feature hard freeze window starts from 12th of next week. So I am still debating whether to keep the hard check or just have a warning. If the hard check causes regressions, we can always revert it to a warning later.
>> 
>> mst?
> 
> I'd go for a warning now. Let's see what triggers for users without
> actually breaking things too badly for them.

🤦🏻

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

[Ani Sinha]

> On 02-Jul-2023, at 10:29 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Sat, Jul 01, 2023 at 04:09:31PM +0900, Akihiko Odaki wrote:
>> Yes, I want the slot number restriction to be enforced. If it worries you
>> too much for regressions, you may implement it as a warning first and then
>> turn it a hard error when the next development phase starts.
> 
> That's not a bad idea.

This is with just the warning - the device still gets added:

(qemu) netdev_add socket,id=hostnet1,listen=:1234
(qemu)  device_add e1000e,netdev=hostnet1,mac=00:11:22:33:44:03,id=net1,bus=pci.6,addr=0x2.0x5
warning: PCI: slot 2 is not valid for e1000e, parent device only allows plugging into slot 0.
(qemu) info network
igb.0: index=0,type=nic,model=igb,macaddr=52:54:00:12:34:56
igb.1: index=0,type=nic,model=igb,macaddr=52:54:00:12:34:57
net1: index=0,type=nic,model=e1000e,macaddr=00:11:22:33:44:03
 \ hostnet1: index=0,type=socket,

device_remove won’t be able to remove the nic unless guest cooperates.

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

[Michael S. Tsirkin]

On Fri, Jun 30, 2023 at 08:36:38PM +0900, Akihiko Odaki wrote:
> On 2023/06/30 19:37, Ani Sinha wrote:
> > 
> > 
> > > On 30-Jun-2023, at 3:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > 
> > > On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:
> > > > 
> > > > 
> > > > > On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > 
> > > > > On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:
> > > > > > 
> > > > > > 
> > > > > > > On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > > 
> > > > > > > On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
> > > > > > > > > 
> > > > > > > > > Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
> > > > > > > > > 
> > > > > > > > > > Also where do you propose we move the check?
> > > > > > > > > 
> > > > > > > > > In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.
> > > > > > > > 
> > > > > > > > Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
> > > > > > > > 
> > > > > > > > -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
> > > > > > > > 
> > > > > > > > The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.
> > > > > > > 
> > > > > > > I think it's allowed because it expects you to hotplug function 0 later,
> > > > > > 
> > > > > > This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.
> > > > > 
> > > > > yes but if you later add a device with ARI and with next field pointing
> > > > > slot 2 guest will suddently find both.
> > > > 
> > > > Hmm, I tried this:
> > > > 
> > > > -device pcie-root-port,id=p \
> > > > -device igb,bus=p,addr=0x2.0x0 \
> > > > -device igb,bus=p,addr=0x0.0x0 \
> > > > 
> > > > The guest only found the second igb device not the first. You can try too.
> > > 
> > > Because next parameter in pcie_ari_init does not match.
> > 
> > OK send me a command line that I can test it with. I can’t come up with a case that actually works in practice.
> 
> I don't think there is one because the code for PCI multifunction does not
> care ARI. In my opinion, we need yet another check to make non-SR-IOV
> multifunction and ARI capability mutually exclusive; if a function has the
> ARI capability and it is not a VF, an attempt to assign non-zero function
> number for it should fail.

Why is that? My understanding is that ARI capable devices should also
set the multifunction bit in the header. It's not terribly clear from
the spec though.

> But it should be a distinct check as it will need to check the function
> number bits.
> 
> > 
> > > 
> > > 
> > > > > 
> > > > > > > no?
> > > > > > > 
> > > > > > > I am quite worried about all this work going into blocking
> > > > > > > what we think is disallowed configurations. We should have
> > > > > > > maybe blocked them originally, but now that we didn't
> > > > > > > there's a non zero chance of regressions,
> > > > > > 
> > > > > > Sigh,
> > > > > 
> > > > > There's value in patches 1-4 I think - the last patch helped you find
> > > > > these. so there's value in this work.
> > > > > 
> > > > > > no medals here for being brave :-)
> > > > > 
> > > > > Try removing support for a 3.5mm jack next. Oh wait ...
> > > > 
> > > > Indeed. Everyone uses bluetooth these days. I for one is happy that the jack is gone (and they were bold enough to do it while Samsung and others still carry the useless port ) :-)
> 
> Hello from a guy using a shiny M2 Macbook Air carrying the legacy jack with
> a 100-yen earphone. Even people who ported Linux to this machine spent
> efforts to get the jack to work on Linux ;)
> 
> > > > 
> > > > > 
> > > > > > > and the benefit
> > > > > > > is not guaranteed.
> > > > > > > 
> > > > > > > -- 
> > > > > > > MST
> > 

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

[Akihiko Odaki]

On 2023/07/01 0:29, Michael S. Tsirkin wrote:
> On Fri, Jun 30, 2023 at 08:36:38PM +0900, Akihiko Odaki wrote:
>> On 2023/06/30 19:37, Ani Sinha wrote:
>>>
>>>
>>>> On 30-Jun-2023, at 3:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>
>>>> On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:
>>>>>
>>>>>
>>>>>> On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>
>>>>>> On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:
>>>>>>>
>>>>>>>
>>>>>>>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>>
>>>>>>>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
>>>>>>>>>>
>>>>>>>>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
>>>>>>>>>>
>>>>>>>>>>> Also where do you propose we move the check?
>>>>>>>>>>
>>>>>>>>>> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.
>>>>>>>>>
>>>>>>>>> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
>>>>>>>>>
>>>>>>>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
>>>>>>>>>
>>>>>>>>> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.
>>>>>>>>
>>>>>>>> I think it's allowed because it expects you to hotplug function 0 later,
>>>>>>>
>>>>>>> This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.
>>>>>>
>>>>>> yes but if you later add a device with ARI and with next field pointing
>>>>>> slot 2 guest will suddently find both.
>>>>>
>>>>> Hmm, I tried this:
>>>>>
>>>>> -device pcie-root-port,id=p \
>>>>> -device igb,bus=p,addr=0x2.0x0 \
>>>>> -device igb,bus=p,addr=0x0.0x0 \
>>>>>
>>>>> The guest only found the second igb device not the first. You can try too.
>>>>
>>>> Because next parameter in pcie_ari_init does not match.
>>>
>>> OK send me a command line that I can test it with. I can’t come up with a case that actually works in practice.
>>
>> I don't think there is one because the code for PCI multifunction does not
>> care ARI. In my opinion, we need yet another check to make non-SR-IOV
>> multifunction and ARI capability mutually exclusive; if a function has the
>> ARI capability and it is not a VF, an attempt to assign non-zero function
>> number for it should fail.
> 
> Why is that? My understanding is that ARI capable devices should also
> set the multifunction bit in the header. It's not terribly clear from
> the spec though.

Something like the following will not work properly with ARI-capable 
device (think of a as an ARI-capable device):
-device a,addr=0x1.0x0,multifunction=on -device a,addr=0x1.0x1

This is because the next function numbers advertised with ARI are not 
updated with the multifunction configuration, but they are hardcoded in 
the device implementation. In this sense, the traditional (non-SR/IOV) 
multifunction mechanism QEMU has will not work with ARI-capable devices.

> 
>> But it should be a distinct check as it will need to check the function
>> number bits.
>>
>>>
>>>>
>>>>
>>>>>>
>>>>>>>> no?
>>>>>>>>
>>>>>>>> I am quite worried about all this work going into blocking
>>>>>>>> what we think is disallowed configurations. We should have
>>>>>>>> maybe blocked them originally, but now that we didn't
>>>>>>>> there's a non zero chance of regressions,
>>>>>>>
>>>>>>> Sigh,
>>>>>>
>>>>>> There's value in patches 1-4 I think - the last patch helped you find
>>>>>> these. so there's value in this work.
>>>>>>
>>>>>>> no medals here for being brave :-)
>>>>>>
>>>>>> Try removing support for a 3.5mm jack next. Oh wait ...
>>>>>
>>>>> Indeed. Everyone uses bluetooth these days. I for one is happy that the jack is gone (and they were bold enough to do it while Samsung and others still carry the useless port ) :-)
>>
>> Hello from a guy using a shiny M2 Macbook Air carrying the legacy jack with
>> a 100-yen earphone. Even people who ported Linux to this machine spent
>> efforts to get the jack to work on Linux ;)
>>
>>>>>
>>>>>>
>>>>>>>> and the benefit
>>>>>>>> is not guaranteed.
>>>>>>>>
>>>>>>>> -- 
>>>>>>>> MST
>>>
> 

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

[Igor Mammedov]

On Sat, 1 Jul 2023 16:28:30 +0900
Akihiko Odaki <akihiko.odaki@daynix.com> wrote:

> On 2023/07/01 0:29, Michael S. Tsirkin wrote:
> > On Fri, Jun 30, 2023 at 08:36:38PM +0900, Akihiko Odaki wrote:  
> >> On 2023/06/30 19:37, Ani Sinha wrote:  
> >>>
> >>>  
> >>>> On 30-Jun-2023, at 3:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>
> >>>> On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:  
> >>>>>
> >>>>>  
> >>>>>> On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>>>
> >>>>>> On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:  
> >>>>>>>
> >>>>>>>  
> >>>>>>>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>>>>>
> >>>>>>>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:  
> >>>>>>>>>>
> >>>>>>>>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
> >>>>>>>>>>  
> >>>>>>>>>>> Also where do you propose we move the check?  
> >>>>>>>>>>
> >>>>>>>>>> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.  
> >>>>>>>>>
> >>>>>>>>> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
> >>>>>>>>>
> >>>>>>>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
> >>>>>>>>>
> >>>>>>>>> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.  
> >>>>>>>>
> >>>>>>>> I think it's allowed because it expects you to hotplug function 0 later,  
> >>>>>>>
> >>>>>>> This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.  
> >>>>>>
> >>>>>> yes but if you later add a device with ARI and with next field pointing
> >>>>>> slot 2 guest will suddently find both.  
> >>>>>
> >>>>> Hmm, I tried this:
> >>>>>
> >>>>> -device pcie-root-port,id=p \
> >>>>> -device igb,bus=p,addr=0x2.0x0 \
> >>>>> -device igb,bus=p,addr=0x0.0x0 \
> >>>>>
> >>>>> The guest only found the second igb device not the first. You can try too.  
> >>>>
> >>>> Because next parameter in pcie_ari_init does not match.  
> >>>
> >>> OK send me a command line that I can test it with. I can’t come up with a case that actually works in practice.  
> >>
> >> I don't think there is one because the code for PCI multifunction does not
> >> care ARI. In my opinion, we need yet another check to make non-SR-IOV
> >> multifunction and ARI capability mutually exclusive; if a function has the
> >> ARI capability and it is not a VF, an attempt to assign non-zero function
> >> number for it should fail.  

is it stated somewhere in spec(s) that ARI and !SR-IOV are mutually exclusive?

> > 
> > Why is that? My understanding is that ARI capable devices should also
> > set the multifunction bit in the header. It's not terribly clear from
> > the spec though.  
> 
> Something like the following will not work properly with ARI-capable 
> device (think of a as an ARI-capable device):
> -device a,addr=0x1.0x0,multifunction=on -device a,addr=0x1.0x1
(I had a crazy idea, to use it like that so we could put more devices
on port without resorting to adding extra bridges)

Can you elaborate some more why it won't work?

> This is because the next function numbers advertised with ARI are not 
> updated with the multifunction configuration, but they are hardcoded in 
> the device implementation. In this sense, the traditional (non-SR/IOV) 
> multifunction mechanism QEMU has will not work with ARI-capable devices.
> 
> >   
> >> But it should be a distinct check as it will need to check the function
> >> number bits.
> >>  
> >>>  
> >>>>
> >>>>  
> >>>>>>  
> >>>>>>>> no?
> >>>>>>>>
> >>>>>>>> I am quite worried about all this work going into blocking
> >>>>>>>> what we think is disallowed configurations. We should have
> >>>>>>>> maybe blocked them originally, but now that we didn't
> >>>>>>>> there's a non zero chance of regressions,  
> >>>>>>>
> >>>>>>> Sigh,  
> >>>>>>
> >>>>>> There's value in patches 1-4 I think - the last patch helped you find
> >>>>>> these. so there's value in this work.
> >>>>>>  
> >>>>>>> no medals here for being brave :-)  
> >>>>>>
> >>>>>> Try removing support for a 3.5mm jack next. Oh wait ...  
> >>>>>
> >>>>> Indeed. Everyone uses bluetooth these days. I for one is happy that the jack is gone (and they were bold enough to do it while Samsung and others still carry the useless port ) :-)  
> >>
> >> Hello from a guy using a shiny M2 Macbook Air carrying the legacy jack with
> >> a 100-yen earphone. Even people who ported Linux to this machine spent
> >> efforts to get the jack to work on Linux ;)
> >>  
> >>>>>  
> >>>>>>  
> >>>>>>>> and the benefit
> >>>>>>>> is not guaranteed.
> >>>>>>>>
> >>>>>>>> -- 
> >>>>>>>> MST  
> >>>  
> >   
> 

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

[Akihiko Odaki]

On 2023/07/04 20:38, Igor Mammedov wrote:
> On Sat, 1 Jul 2023 16:28:30 +0900
> Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> 
>> On 2023/07/01 0:29, Michael S. Tsirkin wrote:
>>> On Fri, Jun 30, 2023 at 08:36:38PM +0900, Akihiko Odaki wrote:
>>>> On 2023/06/30 19:37, Ani Sinha wrote:
>>>>>
>>>>>   
>>>>>> On 30-Jun-2023, at 3:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>
>>>>>> On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:
>>>>>>>
>>>>>>>   
>>>>>>>> On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>>
>>>>>>>> On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:
>>>>>>>>>
>>>>>>>>>   
>>>>>>>>>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
>>>>>>>>>>>>   
>>>>>>>>>>>>> Also where do you propose we move the check?
>>>>>>>>>>>>
>>>>>>>>>>>> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.
>>>>>>>>>>>
>>>>>>>>>>> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
>>>>>>>>>>>
>>>>>>>>>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
>>>>>>>>>>>
>>>>>>>>>>> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.
>>>>>>>>>>
>>>>>>>>>> I think it's allowed because it expects you to hotplug function 0 later,
>>>>>>>>>
>>>>>>>>> This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.
>>>>>>>>
>>>>>>>> yes but if you later add a device with ARI and with next field pointing
>>>>>>>> slot 2 guest will suddently find both.
>>>>>>>
>>>>>>> Hmm, I tried this:
>>>>>>>
>>>>>>> -device pcie-root-port,id=p \
>>>>>>> -device igb,bus=p,addr=0x2.0x0 \
>>>>>>> -device igb,bus=p,addr=0x0.0x0 \
>>>>>>>
>>>>>>> The guest only found the second igb device not the first. You can try too.
>>>>>>
>>>>>> Because next parameter in pcie_ari_init does not match.
>>>>>
>>>>> OK send me a command line that I can test it with. I can’t come up with a case that actually works in practice.
>>>>
>>>> I don't think there is one because the code for PCI multifunction does not
>>>> care ARI. In my opinion, we need yet another check to make non-SR-IOV
>>>> multifunction and ARI capability mutually exclusive; if a function has the
>>>> ARI capability and it is not a VF, an attempt to assign non-zero function
>>>> number for it should fail.
> 
> is it stated somewhere in spec(s) that ARI and !SR-IOV are mutually exclusive?
> 
>>>
>>> Why is that? My understanding is that ARI capable devices should also
>>> set the multifunction bit in the header. It's not terribly clear from
>>> the spec though.
>>
>> Something like the following will not work properly with ARI-capable
>> device (think of a as an ARI-capable device):
>> -device a,addr=0x1.0x0,multifunction=on -device a,addr=0x1.0x1
> (I had a crazy idea, to use it like that so we could put more devices
> on port without resorting to adding extra bridges)
> 
> Can you elaborate some more why it won't work?

It won't work because the ARI next function number field is fixed. In 
this case, the field of the Function at 0x1.0x0 should point to 0x1.0x1, 
but it doesn't. As the result, the Function at 0x1.0x1 won't be recognized.

It's more problematic if some of the Functions are ARI-capable but 
others are not. In my understanding, all Functions in a ARI-capable 
device need to have ARI capability, but that's not enforced.

> 
>> This is because the next function numbers advertised with ARI are not
>> updated with the multifunction configuration, but they are hardcoded in
>> the device implementation. In this sense, the traditional (non-SR/IOV)
>> multifunction mechanism QEMU has will not work with ARI-capable devices.
>>
>>>    
>>>> But it should be a distinct check as it will need to check the function
>>>> number bits.
>>>>   
>>>>>   
>>>>>>
>>>>>>   
>>>>>>>>   
>>>>>>>>>> no?
>>>>>>>>>>
>>>>>>>>>> I am quite worried about all this work going into blocking
>>>>>>>>>> what we think is disallowed configurations. We should have
>>>>>>>>>> maybe blocked them originally, but now that we didn't
>>>>>>>>>> there's a non zero chance of regressions,
>>>>>>>>>
>>>>>>>>> Sigh,
>>>>>>>>
>>>>>>>> There's value in patches 1-4 I think - the last patch helped you find
>>>>>>>> these. so there's value in this work.
>>>>>>>>   
>>>>>>>>> no medals here for being brave :-)
>>>>>>>>
>>>>>>>> Try removing support for a 3.5mm jack next. Oh wait ...
>>>>>>>
>>>>>>> Indeed. Everyone uses bluetooth these days. I for one is happy that the jack is gone (and they were bold enough to do it while Samsung and others still carry the useless port ) :-)
>>>>
>>>> Hello from a guy using a shiny M2 Macbook Air carrying the legacy jack with
>>>> a 100-yen earphone. Even people who ported Linux to this machine spent
>>>> efforts to get the jack to work on Linux ;)
>>>>   
>>>>>>>   
>>>>>>>>   
>>>>>>>>>> and the benefit
>>>>>>>>>> is not guaranteed.
>>>>>>>>>>
>>>>>>>>>> -- 
>>>>>>>>>> MST
>>>>>   
>>>    
>>
> 

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

[Igor Mammedov]

On Tue, 4 Jul 2023 20:50:49 +0900
Akihiko Odaki <akihiko.odaki@daynix.com> wrote:

> On 2023/07/04 20:38, Igor Mammedov wrote:
> > On Sat, 1 Jul 2023 16:28:30 +0900
> > Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >   
> >> On 2023/07/01 0:29, Michael S. Tsirkin wrote:  
> >>> On Fri, Jun 30, 2023 at 08:36:38PM +0900, Akihiko Odaki wrote:  
> >>>> On 2023/06/30 19:37, Ani Sinha wrote:  
> >>>>>
> >>>>>     
> >>>>>> On 30-Jun-2023, at 3:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>>>
> >>>>>> On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:  
> >>>>>>>
> >>>>>>>     
> >>>>>>>> On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>>>>>
> >>>>>>>> On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:  
> >>>>>>>>>
> >>>>>>>>>     
> >>>>>>>>>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:  
> >>>>>>>>>>>>
> >>>>>>>>>>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
> >>>>>>>>>>>>     
> >>>>>>>>>>>>> Also where do you propose we move the check?  
> >>>>>>>>>>>>
> >>>>>>>>>>>> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.  
> >>>>>>>>>>>
> >>>>>>>>>>> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
> >>>>>>>>>>>
> >>>>>>>>>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
> >>>>>>>>>>>
> >>>>>>>>>>> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.  
> >>>>>>>>>>
> >>>>>>>>>> I think it's allowed because it expects you to hotplug function 0 later,  
> >>>>>>>>>
> >>>>>>>>> This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.  
> >>>>>>>>
> >>>>>>>> yes but if you later add a device with ARI and with next field pointing
> >>>>>>>> slot 2 guest will suddently find both.  
> >>>>>>>
> >>>>>>> Hmm, I tried this:
> >>>>>>>
> >>>>>>> -device pcie-root-port,id=p \
> >>>>>>> -device igb,bus=p,addr=0x2.0x0 \
> >>>>>>> -device igb,bus=p,addr=0x0.0x0 \
> >>>>>>>
> >>>>>>> The guest only found the second igb device not the first. You can try too.  
> >>>>>>
> >>>>>> Because next parameter in pcie_ari_init does not match.  
> >>>>>
> >>>>> OK send me a command line that I can test it with. I can’t come up with a case that actually works in practice.  
> >>>>
> >>>> I don't think there is one because the code for PCI multifunction does not
> >>>> care ARI. In my opinion, we need yet another check to make non-SR-IOV
> >>>> multifunction and ARI capability mutually exclusive; if a function has the
> >>>> ARI capability and it is not a VF, an attempt to assign non-zero function
> >>>> number for it should fail.  
> > 
> > is it stated somewhere in spec(s) that ARI and !SR-IOV are mutually exclusive?
> >   
> >>>
> >>> Why is that? My understanding is that ARI capable devices should also
> >>> set the multifunction bit in the header. It's not terribly clear from
> >>> the spec though.  
> >>
> >> Something like the following will not work properly with ARI-capable
> >> device (think of a as an ARI-capable device):
> >> -device a,addr=0x1.0x0,multifunction=on -device a,addr=0x1.0x1  
> > (I had a crazy idea, to use it like that so we could put more devices
> > on port without resorting to adding extra bridges)
> > 
> > Can you elaborate some more why it won't work?  
> 
> It won't work because the ARI next function number field is fixed. In 
> this case, the field of the Function at 0x1.0x0 should point to 0x1.0x1, 
> but it doesn't. As the result, the Function at 0x1.0x1 won't be recognized.
> 
> It's more problematic if some of the Functions are ARI-capable but 
> others are not. In my understanding, all Functions in a ARI-capable 
> device need to have ARI capability, but that's not enforced.

that doesn't look to me as an real issue but rather as
a QEMU problem that we could fix and handle it properly.

> >> This is because the next function numbers advertised with ARI are not
> >> updated with the multifunction configuration, but they are hardcoded in
> >> the device implementation. In this sense, the traditional (non-SR/IOV)
> >> multifunction mechanism QEMU has will not work with ARI-capable devices.
> >>  
> >>>      
> >>>> But it should be a distinct check as it will need to check the function
> >>>> number bits.
> >>>>     
> >>>>>     
> >>>>>>
> >>>>>>     
> >>>>>>>>     
> >>>>>>>>>> no?
> >>>>>>>>>>
> >>>>>>>>>> I am quite worried about all this work going into blocking
> >>>>>>>>>> what we think is disallowed configurations. We should have
> >>>>>>>>>> maybe blocked them originally, but now that we didn't
> >>>>>>>>>> there's a non zero chance of regressions,  
> >>>>>>>>>
> >>>>>>>>> Sigh,  
> >>>>>>>>
> >>>>>>>> There's value in patches 1-4 I think - the last patch helped you find
> >>>>>>>> these. so there's value in this work.
> >>>>>>>>     
> >>>>>>>>> no medals here for being brave :-)  
> >>>>>>>>
> >>>>>>>> Try removing support for a 3.5mm jack next. Oh wait ...  
> >>>>>>>
> >>>>>>> Indeed. Everyone uses bluetooth these days. I for one is happy that the jack is gone (and they were bold enough to do it while Samsung and others still carry the useless port ) :-)  
> >>>>
> >>>> Hello from a guy using a shiny M2 Macbook Air carrying the legacy jack with
> >>>> a 100-yen earphone. Even people who ported Linux to this machine spent
> >>>> efforts to get the jack to work on Linux ;)
> >>>>     
> >>>>>>>     
> >>>>>>>>     
> >>>>>>>>>> and the benefit
> >>>>>>>>>> is not guaranteed.
> >>>>>>>>>>
> >>>>>>>>>> -- 
> >>>>>>>>>> MST  
> >>>>>     
> >>>      
> >>  
> >   
> 

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

[Akihiko Odaki]

On 2023/07/04 21:36, Igor Mammedov wrote:
> On Tue, 4 Jul 2023 20:50:49 +0900
> Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> 
>> On 2023/07/04 20:38, Igor Mammedov wrote:
>>> On Sat, 1 Jul 2023 16:28:30 +0900
>>> Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>    
>>>> On 2023/07/01 0:29, Michael S. Tsirkin wrote:
>>>>> On Fri, Jun 30, 2023 at 08:36:38PM +0900, Akihiko Odaki wrote:
>>>>>> On 2023/06/30 19:37, Ani Sinha wrote:
>>>>>>>
>>>>>>>      
>>>>>>>> On 30-Jun-2023, at 3:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>>
>>>>>>>> On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:
>>>>>>>>>
>>>>>>>>>      
>>>>>>>>>> On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:
>>>>>>>>>>>
>>>>>>>>>>>      
>>>>>>>>>>>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
>>>>>>>>>>>>>>      
>>>>>>>>>>>>>>> Also where do you propose we move the check?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
>>>>>>>>>>>>>
>>>>>>>>>>>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
>>>>>>>>>>>>>
>>>>>>>>>>>>> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.
>>>>>>>>>>>>
>>>>>>>>>>>> I think it's allowed because it expects you to hotplug function 0 later,
>>>>>>>>>>>
>>>>>>>>>>> This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.
>>>>>>>>>>
>>>>>>>>>> yes but if you later add a device with ARI and with next field pointing
>>>>>>>>>> slot 2 guest will suddently find both.
>>>>>>>>>
>>>>>>>>> Hmm, I tried this:
>>>>>>>>>
>>>>>>>>> -device pcie-root-port,id=p \
>>>>>>>>> -device igb,bus=p,addr=0x2.0x0 \
>>>>>>>>> -device igb,bus=p,addr=0x0.0x0 \
>>>>>>>>>
>>>>>>>>> The guest only found the second igb device not the first. You can try too.
>>>>>>>>
>>>>>>>> Because next parameter in pcie_ari_init does not match.
>>>>>>>
>>>>>>> OK send me a command line that I can test it with. I can’t come up with a case that actually works in practice.
>>>>>>
>>>>>> I don't think there is one because the code for PCI multifunction does not
>>>>>> care ARI. In my opinion, we need yet another check to make non-SR-IOV
>>>>>> multifunction and ARI capability mutually exclusive; if a function has the
>>>>>> ARI capability and it is not a VF, an attempt to assign non-zero function
>>>>>> number for it should fail.
>>>
>>> is it stated somewhere in spec(s) that ARI and !SR-IOV are mutually exclusive?
>>>    
>>>>>
>>>>> Why is that? My understanding is that ARI capable devices should also
>>>>> set the multifunction bit in the header. It's not terribly clear from
>>>>> the spec though.
>>>>
>>>> Something like the following will not work properly with ARI-capable
>>>> device (think of a as an ARI-capable device):
>>>> -device a,addr=0x1.0x0,multifunction=on -device a,addr=0x1.0x1
>>> (I had a crazy idea, to use it like that so we could put more devices
>>> on port without resorting to adding extra bridges)
>>>
>>> Can you elaborate some more why it won't work?
>>
>> It won't work because the ARI next function number field is fixed. In
>> this case, the field of the Function at 0x1.0x0 should point to 0x1.0x1,
>> but it doesn't. As the result, the Function at 0x1.0x1 won't be recognized.
>>
>> It's more problematic if some of the Functions are ARI-capable but
>> others are not. In my understanding, all Functions in a ARI-capable
>> device need to have ARI capability, but that's not enforced.
> 
> that doesn't look to me as an real issue but rather as
> a QEMU problem that we could fix and handle it properly.

Yes, it's certainly possible to fix QEMU so that non-SR/IOV 
multifunction works with ARI-capable Functions.

> 
>>>> This is because the next function numbers advertised with ARI are not
>>>> updated with the multifunction configuration, but they are hardcoded in
>>>> the device implementation. In this sense, the traditional (non-SR/IOV)
>>>> multifunction mechanism QEMU has will not work with ARI-capable devices.
>>>>   
>>>>>       
>>>>>> But it should be a distinct check as it will need to check the function
>>>>>> number bits.
>>>>>>      
>>>>>>>      
>>>>>>>>
>>>>>>>>      
>>>>>>>>>>      
>>>>>>>>>>>> no?
>>>>>>>>>>>>
>>>>>>>>>>>> I am quite worried about all this work going into blocking
>>>>>>>>>>>> what we think is disallowed configurations. We should have
>>>>>>>>>>>> maybe blocked them originally, but now that we didn't
>>>>>>>>>>>> there's a non zero chance of regressions,
>>>>>>>>>>>
>>>>>>>>>>> Sigh,
>>>>>>>>>>
>>>>>>>>>> There's value in patches 1-4 I think - the last patch helped you find
>>>>>>>>>> these. so there's value in this work.
>>>>>>>>>>      
>>>>>>>>>>> no medals here for being brave :-)
>>>>>>>>>>
>>>>>>>>>> Try removing support for a 3.5mm jack next. Oh wait ...
>>>>>>>>>
>>>>>>>>> Indeed. Everyone uses bluetooth these days. I for one is happy that the jack is gone (and they were bold enough to do it while Samsung and others still carry the useless port ) :-)
>>>>>>
>>>>>> Hello from a guy using a shiny M2 Macbook Air carrying the legacy jack with
>>>>>> a 100-yen earphone. Even people who ported Linux to this machine spent
>>>>>> efforts to get the jack to work on Linux ;)
>>>>>>      
>>>>>>>>>      
>>>>>>>>>>      
>>>>>>>>>>>> and the benefit
>>>>>>>>>>>> is not guaranteed.
>>>>>>>>>>>>
>>>>>>>>>>>> -- 
>>>>>>>>>>>> MST
>>>>>>>      
>>>>>       
>>>>   
>>>    
>>
> 

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

[Ani Sinha]

> On 04-Jul-2023, at 5:20 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> 
> On 2023/07/04 20:38, Igor Mammedov wrote:
>> On Sat, 1 Jul 2023 16:28:30 +0900
>> Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>> On 2023/07/01 0:29, Michael S. Tsirkin wrote:
>>>> On Fri, Jun 30, 2023 at 08:36:38PM +0900, Akihiko Odaki wrote:
>>>>> On 2023/06/30 19:37, Ani Sinha wrote:
>>>>>> 
>>>>>>  
>>>>>>> On 30-Jun-2023, at 3:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>> 
>>>>>>> On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:
>>>>>>>> 
>>>>>>>>  
>>>>>>>>> On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>>> 
>>>>>>>>> On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:
>>>>>>>>>> 
>>>>>>>>>>  
>>>>>>>>>>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>>>>> 
>>>>>>>>>>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
>>>>>>>>>>>>>  
>>>>>>>>>>>>>> Also where do you propose we move the check?
>>>>>>>>>>>>> 
>>>>>>>>>>>>> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.
>>>>>>>>>>>> 
>>>>>>>>>>>> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
>>>>>>>>>>>> 
>>>>>>>>>>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
>>>>>>>>>>>> 
>>>>>>>>>>>> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.
>>>>>>>>>>> 
>>>>>>>>>>> I think it's allowed because it expects you to hotplug function 0 later,
>>>>>>>>>> 
>>>>>>>>>> This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.
>>>>>>>>> 
>>>>>>>>> yes but if you later add a device with ARI and with next field pointing
>>>>>>>>> slot 2 guest will suddently find both.
>>>>>>>> 
>>>>>>>> Hmm, I tried this:
>>>>>>>> 
>>>>>>>> -device pcie-root-port,id=p \
>>>>>>>> -device igb,bus=p,addr=0x2.0x0 \
>>>>>>>> -device igb,bus=p,addr=0x0.0x0 \
>>>>>>>> 
>>>>>>>> The guest only found the second igb device not the first. You can try too.
>>>>>>> 
>>>>>>> Because next parameter in pcie_ari_init does not match.
>>>>>> 
>>>>>> OK send me a command line that I can test it with. I can’t come up with a case that actually works in practice.
>>>>> 
>>>>> I don't think there is one because the code for PCI multifunction does not
>>>>> care ARI. In my opinion, we need yet another check to make non-SR-IOV
>>>>> multifunction and ARI capability mutually exclusive; if a function has the
>>>>> ARI capability and it is not a VF, an attempt to assign non-zero function
>>>>> number for it should fail.
>> is it stated somewhere in spec(s) that ARI and !SR-IOV are mutually exclusive?
>>>> 
>>>> Why is that? My understanding is that ARI capable devices should also
>>>> set the multifunction bit in the header. It's not terribly clear from
>>>> the spec though.
>>> 
>>> Something like the following will not work properly with ARI-capable
>>> device (think of a as an ARI-capable device):
>>> -device a,addr=0x1.0x0,multifunction=on -device a,addr=0x1.0x1
>> (I had a crazy idea, to use it like that so we could put more devices
>> on port without resorting to adding extra bridges)
>> Can you elaborate some more why it won't work?
> 
> It won't work because the ARI next function number field is fixed. In this case, the field of the Function at 0x1.0x0 should point to 0x1.0x1, but it doesn’t.

Where does it point to in this case then? 0x1.0x2 becasue of the stride?

> As the result, the Function at 0x1.0x1 won't be recognized.
> 
> It's more problematic if some of the Functions are ARI-capable but others are not. In my understanding, all Functions in a ARI-capable device need to have ARI capability, but that's not enforced.
> 
>>> This is because the next function numbers advertised with ARI are not
>>> updated with the multifunction configuration, but they are hardcoded in
>>> the device implementation. In this sense, the traditional (non-SR/IOV)
>>> multifunction mechanism QEMU has will not work with ARI-capable devices.
>>> 
>>>>   
>>>>> But it should be a distinct check as it will need to check the function
>>>>> number bits.
>>>>>  
>>>>>>  
>>>>>>> 
>>>>>>>  
>>>>>>>>>  
>>>>>>>>>>> no?
>>>>>>>>>>> 
>>>>>>>>>>> I am quite worried about all this work going into blocking
>>>>>>>>>>> what we think is disallowed configurations. We should have
>>>>>>>>>>> maybe blocked them originally, but now that we didn't
>>>>>>>>>>> there's a non zero chance of regressions,
>>>>>>>>>> 
>>>>>>>>>> Sigh,
>>>>>>>>> 
>>>>>>>>> There's value in patches 1-4 I think - the last patch helped you find
>>>>>>>>> these. so there's value in this work.
>>>>>>>>>  
>>>>>>>>>> no medals here for being brave :-)
>>>>>>>>> 
>>>>>>>>> Try removing support for a 3.5mm jack next. Oh wait ...
>>>>>>>> 
>>>>>>>> Indeed. Everyone uses bluetooth these days. I for one is happy that the jack is gone (and they were bold enough to do it while Samsung and others still carry the useless port ) :-)
>>>>> 
>>>>> Hello from a guy using a shiny M2 Macbook Air carrying the legacy jack with
>>>>> a 100-yen earphone. Even people who ported Linux to this machine spent
>>>>> efforts to get the jack to work on Linux ;)
>>>>>  
>>>>>>>>  
>>>>>>>>>  
>>>>>>>>>>> and the benefit
>>>>>>>>>>> is not guaranteed.
>>>>>>>>>>> 
>>>>>>>>>>> -- 
>>>>>>>>>>> MST

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

[Akihiko Odaki]

On 2023/07/04 23:03, Ani Sinha wrote:
> 
> 
>> On 04-Jul-2023, at 5:20 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2023/07/04 20:38, Igor Mammedov wrote:
>>> On Sat, 1 Jul 2023 16:28:30 +0900
>>> Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>> On 2023/07/01 0:29, Michael S. Tsirkin wrote:
>>>>> On Fri, Jun 30, 2023 at 08:36:38PM +0900, Akihiko Odaki wrote:
>>>>>> On 2023/06/30 19:37, Ani Sinha wrote:
>>>>>>>
>>>>>>>   
>>>>>>>> On 30-Jun-2023, at 3:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>>
>>>>>>>> On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:
>>>>>>>>>
>>>>>>>>>   
>>>>>>>>>> On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote:
>>>>>>>>>>>
>>>>>>>>>>>   
>>>>>>>>>>>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() instead of checking ARI capability, and that can happen in do_pci_register_device().
>>>>>>>>>>>>>>   
>>>>>>>>>>>>>>> Also where do you propose we move the check?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> In pci_qdev_realize(), somewhere after pc->realize() and before option ROM loading.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hmm, I tried this. The issue here is something like this would be now allowed since the PF has ARI capability:
>>>>>>>>>>>>>
>>>>>>>>>>>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0
>>>>>>>>>>>>>
>>>>>>>>>>>>> The above should not be allowed and when used, we do not see the igb ethernet device from the guest OS.
>>>>>>>>>>>>
>>>>>>>>>>>> I think it's allowed because it expects you to hotplug function 0 later,
>>>>>>>>>>>
>>>>>>>>>>> This is about the igb device being plugged into the non-zero slot of the pci-root-port. The guest OS ignores it.
>>>>>>>>>>
>>>>>>>>>> yes but if you later add a device with ARI and with next field pointing
>>>>>>>>>> slot 2 guest will suddently find both.
>>>>>>>>>
>>>>>>>>> Hmm, I tried this:
>>>>>>>>>
>>>>>>>>> -device pcie-root-port,id=p \
>>>>>>>>> -device igb,bus=p,addr=0x2.0x0 \
>>>>>>>>> -device igb,bus=p,addr=0x0.0x0 \
>>>>>>>>>
>>>>>>>>> The guest only found the second igb device not the first. You can try too.
>>>>>>>>
>>>>>>>> Because next parameter in pcie_ari_init does not match.
>>>>>>>
>>>>>>> OK send me a command line that I can test it with. I can’t come up with a case that actually works in practice.
>>>>>>
>>>>>> I don't think there is one because the code for PCI multifunction does not
>>>>>> care ARI. In my opinion, we need yet another check to make non-SR-IOV
>>>>>> multifunction and ARI capability mutually exclusive; if a function has the
>>>>>> ARI capability and it is not a VF, an attempt to assign non-zero function
>>>>>> number for it should fail.
>>> is it stated somewhere in spec(s) that ARI and !SR-IOV are mutually exclusive?
>>>>>
>>>>> Why is that? My understanding is that ARI capable devices should also
>>>>> set the multifunction bit in the header. It's not terribly clear from
>>>>> the spec though.
>>>>
>>>> Something like the following will not work properly with ARI-capable
>>>> device (think of a as an ARI-capable device):
>>>> -device a,addr=0x1.0x0,multifunction=on -device a,addr=0x1.0x1
>>> (I had a crazy idea, to use it like that so we could put more devices
>>> on port without resorting to adding extra bridges)
>>> Can you elaborate some more why it won't work?
>>
>> It won't work because the ARI next function number field is fixed. In this case, the field of the Function at 0x1.0x0 should point to 0x1.0x1, but it doesn’t.
> 
> Where does it point to in this case then? 0x1.0x2 becasue of the stride?

With "[PATCH v5 0/2] pcie: Fix ARI next function numbers"*, it will 
point to 0, which ends the linked list formed with the ARI next number 
fields and make it only contain Function 0.

* 
https://lore.kernel.org/qemu-devel/20230705022421.13115-1-akihiko.odaki@daynix.com/

> 
>> As the result, the Function at 0x1.0x1 won't be recognized.
>>
>> It's more problematic if some of the Functions are ARI-capable but others are not. In my understanding, all Functions in a ARI-capable device need to have ARI capability, but that's not enforced.
>>
>>>> This is because the next function numbers advertised with ARI are not
>>>> updated with the multifunction configuration, but they are hardcoded in
>>>> the device implementation. In this sense, the traditional (non-SR/IOV)
>>>> multifunction mechanism QEMU has will not work with ARI-capable devices.
>>>>
>>>>>    
>>>>>> But it should be a distinct check as it will need to check the function
>>>>>> number bits.
>>>>>>   
>>>>>>>   
>>>>>>>>
>>>>>>>>   
>>>>>>>>>>   
>>>>>>>>>>>> no?
>>>>>>>>>>>>
>>>>>>>>>>>> I am quite worried about all this work going into blocking
>>>>>>>>>>>> what we think is disallowed configurations. We should have
>>>>>>>>>>>> maybe blocked them originally, but now that we didn't
>>>>>>>>>>>> there's a non zero chance of regressions,
>>>>>>>>>>>
>>>>>>>>>>> Sigh,
>>>>>>>>>>
>>>>>>>>>> There's value in patches 1-4 I think - the last patch helped you find
>>>>>>>>>> these. so there's value in this work.
>>>>>>>>>>   
>>>>>>>>>>> no medals here for being brave :-)
>>>>>>>>>>
>>>>>>>>>> Try removing support for a 3.5mm jack next. Oh wait ...
>>>>>>>>>
>>>>>>>>> Indeed. Everyone uses bluetooth these days. I for one is happy that the jack is gone (and they were bold enough to do it while Samsung and others still carry the useless port ) :-)
>>>>>>
>>>>>> Hello from a guy using a shiny M2 Macbook Air carrying the legacy jack with
>>>>>> a 100-yen earphone. Even people who ported Linux to this machine spent
>>>>>> efforts to get the jack to work on Linux ;)
>>>>>>   
>>>>>>>>>   
>>>>>>>>>>   
>>>>>>>>>>>> and the benefit
>>>>>>>>>>>> is not guaranteed.
>>>>>>>>>>>>
>>>>>>>>>>>> -- 
>>>>>>>>>>>> MST
> 

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

[Michael S. Tsirkin]

On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote:
> Indeed. Everyone uses bluetooth these days. I for one is happy that
> the jack is gone (and they were bold enough to do it while Samsung and
> others still carry the useless port ) :-)

🤦🏻

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

[Michael S. Tsirkin]

On Thu, Jun 29, 2023 at 09:37:07AM +0530, Ani Sinha 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.
> 
> The change has been tested to not break ARI by instantiating seven vfs on an
> emulated igb device (the maximum number of vfs the linux igb driver supports).

I guess we need to test with some other device then? 7 VFs is same
slot so hardly a good test.

> The vfs are seen to have non-zero device/slot numbers in the conventional
> PCI BDF representation.
> 
> CC: jusual@redhat.com
> CC: imammedo@redhat.com
> CC: akihiko.odaki@daynix.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 | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index e2eb4c3b4a..0320ac2bb3 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -65,6 +65,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),
> @@ -1190,6 +1191,20 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>                     name);
>  
>         return NULL;
> +    } /*
> +       * With SRIOV and ARI, vfs can have non-zero slot in the conventional
> +       * PCI interpretation as all five bits reserved for slot addresses are
> +       * also used for function bits for the various vfs. Ignore that case.
> +       * It is too early here to check for ARI capabilities in the PCI config
> +       * space. Hence, we check for a vf device instead.
> +       */
> +    else if (!pci_is_vf(pci_dev) &&
> +             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 v6 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Ani Sinha]

> On 29-Jun-2023, at 7:54 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Thu, Jun 29, 2023 at 09:37:07AM +0530, Ani Sinha 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.
>> 
>> The change has been tested to not break ARI by instantiating seven vfs on an
>> emulated igb device (the maximum number of vfs the linux igb driver supports).
> 
> I guess we need to test with some other device then? 7 VFs is same
> slot so hardly a good test.

No its not the same slot. Its using different slots/device numbers. I checked that.
The same patch was failing without the vf check.

> 
>> The vfs are seen to have non-zero device/slot numbers in the conventional
>> PCI BDF representation.
>> 
>> CC: jusual@redhat.com
>> CC: imammedo@redhat.com
>> CC: akihiko.odaki@daynix.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 | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>> 
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index e2eb4c3b4a..0320ac2bb3 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -65,6 +65,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),
>> @@ -1190,6 +1191,20 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>>                    name);
>> 
>>        return NULL;
>> +    } /*
>> +       * With SRIOV and ARI, vfs can have non-zero slot in the conventional
>> +       * PCI interpretation as all five bits reserved for slot addresses are
>> +       * also used for function bits for the various vfs. Ignore that case.
>> +       * It is too early here to check for ARI capabilities in the PCI config
>> +       * space. Hence, we check for a vf device instead.
>> +       */
>> +    else if (!pci_is_vf(pci_dev) &&
>> +             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 v6 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Michael S. Tsirkin]

On Thu, Jun 29, 2023 at 08:07:57PM +0530, Ani Sinha wrote:
> 
> 
> > On 29-Jun-2023, at 7:54 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 
> > On Thu, Jun 29, 2023 at 09:37:07AM +0530, Ani Sinha 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.
> >> 
> >> The change has been tested to not break ARI by instantiating seven vfs on an
> >> emulated igb device (the maximum number of vfs the linux igb driver supports).
> > 
> > I guess we need to test with some other device then? 7 VFs is same
> > slot so hardly a good test.
> 
> No its not the same slot. Its using different slots/device numbers. I checked that.
> The same patch was failing without the vf check.

Ah, playing with VF stride? Could you show the command line please?

> > 
> >> The vfs are seen to have non-zero device/slot numbers in the conventional
> >> PCI BDF representation.
> >> 
> >> CC: jusual@redhat.com
> >> CC: imammedo@redhat.com
> >> CC: akihiko.odaki@daynix.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 | 15 +++++++++++++++
> >> 1 file changed, 15 insertions(+)
> >> 
> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >> index e2eb4c3b4a..0320ac2bb3 100644
> >> --- a/hw/pci/pci.c
> >> +++ b/hw/pci/pci.c
> >> @@ -65,6 +65,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),
> >> @@ -1190,6 +1191,20 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
> >>                    name);
> >> 
> >>        return NULL;
> >> +    } /*
> >> +       * With SRIOV and ARI, vfs can have non-zero slot in the conventional
> >> +       * PCI interpretation as all five bits reserved for slot addresses are
> >> +       * also used for function bits for the various vfs. Ignore that case.
> >> +       * It is too early here to check for ARI capabilities in the PCI config
> >> +       * space. Hence, we check for a vf device instead.
> >> +       */
> >> +    else if (!pci_is_vf(pci_dev) &&
> >> +             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 v6 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Ani Sinha]

> On 29-Jun-2023, at 9:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Thu, Jun 29, 2023 at 08:07:57PM +0530, Ani Sinha wrote:
>> 
>> 
>>> On 29-Jun-2023, at 7:54 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> 
>>> On Thu, Jun 29, 2023 at 09:37:07AM +0530, Ani Sinha 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.
>>>> 
>>>> The change has been tested to not break ARI by instantiating seven vfs on an
>>>> emulated igb device (the maximum number of vfs the linux igb driver supports).
>>> 
>>> I guess we need to test with some other device then? 7 VFs is same
>>> slot so hardly a good test.
>> 
>> No its not the same slot. Its using different slots/device numbers. I checked that.
>> The same patch was failing without the vf check.
> 
> Ah, playing with VF stride? Could you show the command line please?

Akhido mentioned this in the other thread. Basically For QEMU:

-device pcie-root-port,id=p -device igb,bus=p

Then from within the guest (in my case RHEL 9.2):

$ echo 7 > /sys/bus/pci/devices/0000\:01\:00.0/sriov_numvfs

You’ll find that if you use something more than 7 there will be ERANGE from the guest kernel because the driver can create maximum 7 vfs.
This above command line will fail if we do not check for !vfs in the patch with the following error from QEMU:

(qemu) qemu-system-x86_64: PCI: slot 16 is not valid for igbvf, parent device only allows plugging into slot 0.

and an IO error on the write from the guest kernel.

In the current version of the patch with the vf check, you will find the vfs created with the addresses:

01:10.{2,4,6,8} and 01.11.{2,4,6} , that is bus 1 for the root port, devices 10 and 11, functions 2,4,6,8 etc.

There would be no error from QEMU.

> 
>>> 
>>>> The vfs are seen to have non-zero device/slot numbers in the conventional
>>>> PCI BDF representation.
>>>> 
>>>> CC: jusual@redhat.com
>>>> CC: imammedo@redhat.com
>>>> CC: akihiko.odaki@daynix.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 | 15 +++++++++++++++
>>>> 1 file changed, 15 insertions(+)
>>>> 
>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>> index e2eb4c3b4a..0320ac2bb3 100644
>>>> --- a/hw/pci/pci.c
>>>> +++ b/hw/pci/pci.c
>>>> @@ -65,6 +65,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),
>>>> @@ -1190,6 +1191,20 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>>>>                   name);
>>>> 
>>>>       return NULL;
>>>> +    } /*
>>>> +       * With SRIOV and ARI, vfs can have non-zero slot in the conventional
>>>> +       * PCI interpretation as all five bits reserved for slot addresses are
>>>> +       * also used for function bits for the various vfs. Ignore that case.
>>>> +       * It is too early here to check for ARI capabilities in the PCI config
>>>> +       * space. Hence, we check for a vf device instead.
>>>> +       */
>>>> +    else if (!pci_is_vf(pci_dev) &&
>>>> +             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 v6 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Ani Sinha]

> On 29-Jun-2023, at 9:27 PM, Ani Sinha <anisinha@redhat.com> wrote:
> 
> 
> 
>> On 29-Jun-2023, at 9:02 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> 
>> On Thu, Jun 29, 2023 at 08:07:57PM +0530, Ani Sinha wrote:
>>> 
>>> 
>>>> On 29-Jun-2023, at 7:54 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> 
>>>> On Thu, Jun 29, 2023 at 09:37:07AM +0530, Ani Sinha 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.
>>>>> 
>>>>> The change has been tested to not break ARI by instantiating seven vfs on an
>>>>> emulated igb device (the maximum number of vfs the linux igb driver supports).
>>>> 
>>>> I guess we need to test with some other device then? 7 VFs is same
>>>> slot so hardly a good test.
>>> 
>>> No its not the same slot. Its using different slots/device numbers. I checked that.
>>> The same patch was failing without the vf check.
>> 
>> Ah, playing with VF stride?

Indeed. You’ll see IGB_VF_STRIDE is 2. pcie_sriov_pf_init() uses this to initialise the PCIE config space attributes. register_vfs() uses this to increment the devfn values :-) 


>> Could you show the command line please?
> 
> Akhido mentioned this in the other thread. Basically For QEMU:
> 
> -device pcie-root-port,id=p -device igb,bus=p
> 
> Then from within the guest (in my case RHEL 9.2):
> 
> $ echo 7 > /sys/bus/pci/devices/0000\:01\:00.0/sriov_numvfs
> 
> You’ll find that if you use something more than 7 there will be ERANGE from the guest kernel because the driver can create maximum 7 vfs.
> This above command line will fail if we do not check for !vfs in the patch with the following error from QEMU:
> 
> (qemu) qemu-system-x86_64: PCI: slot 16 is not valid for igbvf, parent device only allows plugging into slot 0.
> 
> and an IO error on the write from the guest kernel.
> 
> In the current version of the patch with the vf check, you will find the vfs created with the addresses:
> 
> 01:10.{2,4,6,8} and 01.11.{2,4,6} , that is bus 1 for the root port, devices 10 and 11, functions 2,4,6,8 etc.
> 
> There would be no error from QEMU.
> 
>> 
>>>> 
>>>>> The vfs are seen to have non-zero device/slot numbers in the conventional
>>>>> PCI BDF representation.
>>>>> 
>>>>> CC: jusual@redhat.com
>>>>> CC: imammedo@redhat.com
>>>>> CC: akihiko.odaki@daynix.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 | 15 +++++++++++++++
>>>>> 1 file changed, 15 insertions(+)
>>>>> 
>>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>>> index e2eb4c3b4a..0320ac2bb3 100644
>>>>> --- a/hw/pci/pci.c
>>>>> +++ b/hw/pci/pci.c
>>>>> @@ -65,6 +65,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),
>>>>> @@ -1190,6 +1191,20 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>>>>>                  name);
>>>>> 
>>>>>      return NULL;
>>>>> +    } /*
>>>>> +       * With SRIOV and ARI, vfs can have non-zero slot in the conventional
>>>>> +       * PCI interpretation as all five bits reserved for slot addresses are
>>>>> +       * also used for function bits for the various vfs. Ignore that case.
>>>>> +       * It is too early here to check for ARI capabilities in the PCI config
>>>>> +       * space. Hence, we check for a vf device instead.
>>>>> +       */
>>>>> +    else if (!pci_is_vf(pci_dev) &&
>>>>> +             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