[PATCH v8 0/6] test and QEMU fixes to ensure proper PCIE device usage

[PATCH v8 0/6] 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.

Patch 6: add a cosmetic comment addition for better clarity of the code.

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:
===========
v8: more comment messaging. rebased to latest master. small changes in patch
description and title.

v7: added tags, rebased to latest master.
For patch 5, converted a hard error to a warning.
Added patch 6.

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 (6):
  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: warn when PCIe device is plugged into non-zero slot of
    downstream port
  hw/pci: add comment to explain checking for available function 0 in
    pci hotplug

 hw/pci/pci.c                      |  31 +++++++++++++++++++++++++++---
 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, 38 insertions(+), 15 deletions(-)

-- 
2.39.1

[PATCH v8 1/6] 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 v8 2/6] 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 v8 3/6] 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 v8 4/6] 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>
Acked-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 tests/qtest/hd-geo-test.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

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

[PATCH v8 5/6] hw/pci: warn when PCIe device is plugged into non-zero slot of downstream port

[Ani Sinha]

PCIe downstream ports only have a single device 0, so PCI Express devices can
only be plugged into slot 0 on a PCIe port. Add a warning to let users know
when the invalid configuration is used. We may enforce this more strongly later
once we get more clarity on whether we are introducing a bad regression for
users currently using the wrong configuration.

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

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

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index e2eb4c3b4a..62b393dfb7 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),
@@ -2121,6 +2122,25 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
         }
     }
 
+    /*
+     * A PCIe Downstream Port that do not have ARI Forwarding enabled must
+     * associate only Device 0 with the device attached to the bus
+     * representing the Link from the Port (PCIe base spec rev 4.0 ver 0.3,
+     * sec 7.3.1).
+     * With ARI, PCI_SLOT() can return non-zero value as the traditional
+     * 5-bit Device Number and 3-bit Function Number fields in its associated
+     * Routing IDs, Requester IDs and Completer IDs are interpreted as a
+     * single 8-bit Function Number. Hence, ignore ARI capable devices.
+     */
+    if (pci_is_express(pci_dev) &&
+        !pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI) &&
+        pcie_has_upstream_port(pci_dev) &&
+        PCI_SLOT(pci_dev->devfn)) {
+        warn_report("PCI: slot %d is not valid for %s,"
+                    " parent device only allows plugging into slot 0.",
+                    PCI_SLOT(pci_dev->devfn), pci_dev->name);
+    }
+
     if (pci_dev->failover_pair_id) {
         if (!pci_bus_is_express(pci_get_bus(pci_dev))) {
             error_setg(errp, "failover primary device must be on "
-- 
2.39.1

[PATCH v8 6/6] hw/pci: add comment explaining the reason for checking function 0 in hotplug

[Ani Sinha]

This change is cosmetic. A comment is added explaining why we need to check for
the availability of function 0 when we hotplug a device.

CC: mst@redhat.com
Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 hw/pci/pci.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 62b393dfb7..7aee3a7f12 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1181,9 +1181,14 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
                    PCI_SLOT(devfn), PCI_FUNC(devfn), name,
                    bus->devices[devfn]->name, bus->devices[devfn]->qdev.id);
         return NULL;
-    } else if (dev->hotplugged &&
-               !pci_is_vf(pci_dev) &&
-               pci_get_function_0(pci_dev)) {
+    } /*
+       * Populating function 0 triggers a scan from the guest that
+       * exposes other non-zero functions. Hence we need to ensure that
+       * function 0 wasn't added yet.
+       */
+    else if (dev->hotplugged &&
+             !pci_is_vf(pci_dev) &&
+             pci_get_function_0(pci_dev)) {
         error_setg(errp, "PCI: slot %d function 0 already occupied by %s,"
                    " new func %s cannot be exposed to guest.",
                    PCI_SLOT(pci_get_function_0(pci_dev)->devfn),
-- 
2.39.1

[PATCH v8 6/6] hw/pci: add comment to explain checking for available function 0 in pci hotplug

[Ani Sinha]

This change is cosmetic. A comment is added explaining why we need to check for
the availability of function 0 when we hotplug a device.

CC: mst@redhat.com
Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 hw/pci/pci.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 62b393dfb7..7aee3a7f12 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1181,9 +1181,14 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
                    PCI_SLOT(devfn), PCI_FUNC(devfn), name,
                    bus->devices[devfn]->name, bus->devices[devfn]->qdev.id);
         return NULL;
-    } else if (dev->hotplugged &&
-               !pci_is_vf(pci_dev) &&
-               pci_get_function_0(pci_dev)) {
+    } /*
+       * Populating function 0 triggers a scan from the guest that
+       * exposes other non-zero functions. Hence we need to ensure that
+       * function 0 wasn't added yet.
+       */
+    else if (dev->hotplugged &&
+             !pci_is_vf(pci_dev) &&
+             pci_get_function_0(pci_dev)) {
         error_setg(errp, "PCI: slot %d function 0 already occupied by %s,"
                    " new func %s cannot be exposed to guest.",
                    PCI_SLOT(pci_get_function_0(pci_dev)->devfn),
-- 
2.39.1

Re: [PATCH v8 6/6] hw/pci: add comment explaining the reason for checking function 0 in hotplug

[Ani Sinha]

> On 05-Jul-2023, at 5:29 PM, Ani Sinha <anisinha@redhat.com> wrote:
> 
> This change is cosmetic. A comment is added explaining why we need to check for
> the availability of function 0 when we hotplug a device.

Please ignore this patch. Its a duplicate of one already sent with an updated patch summary.

> 
> CC: mst@redhat.com
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> ---
> hw/pci/pci.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 62b393dfb7..7aee3a7f12 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1181,9 +1181,14 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>                    PCI_SLOT(devfn), PCI_FUNC(devfn), name,
>                    bus->devices[devfn]->name, bus->devices[devfn]->qdev.id);
>         return NULL;
> -    } else if (dev->hotplugged &&
> -               !pci_is_vf(pci_dev) &&
> -               pci_get_function_0(pci_dev)) {
> +    } /*
> +       * Populating function 0 triggers a scan from the guest that
> +       * exposes other non-zero functions. Hence we need to ensure that
> +       * function 0 wasn't added yet.
> +       */
> +    else if (dev->hotplugged &&
> +             !pci_is_vf(pci_dev) &&
> +             pci_get_function_0(pci_dev)) {
>         error_setg(errp, "PCI: slot %d function 0 already occupied by %s,"
>                    " new func %s cannot be exposed to guest.",
>                    PCI_SLOT(pci_get_function_0(pci_dev)->devfn),
> -- 
> 2.39.1
> 

Re: [PATCH v8 5/6] hw/pci: warn when PCIe device is plugged into non-zero slot of downstream port

[Akihiko Odaki]

On 2023/07/05 20:59, Ani Sinha wrote:
> PCIe downstream ports only have a single device 0, so PCI Express devices can
> only be plugged into slot 0 on a PCIe port. Add a warning to let users know
> when the invalid configuration is used. We may enforce this more strongly later
> once we get more clarity on whether we are introducing a bad regression for
> users currently using the wrong configuration.
> 
> The change has been tested to not break or alter behaviors of ARI capable
> devices by instantiating seven vfs on an emulated igb device (the maximum
> number of vfs the igb device supports). The vfs are instantiated correctly
> and are seen to have non-zero device/slot numbers in the conventional PCI BDF
> representation.
> 
> CC: jusual@redhat.com
> CC: imammedo@redhat.com
> CC: mst@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>

Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>

Re: [PATCH v8 6/6] hw/pci: add comment explaining the reason for checking function 0 in hotplug

[Michael S. Tsirkin]

On Wed, Jul 05, 2023 at 05:33:31PM +0530, Ani Sinha wrote:
> 
> 
> > On 05-Jul-2023, at 5:29 PM, Ani Sinha <anisinha@redhat.com> wrote:
> > 
> > This change is cosmetic. A comment is added explaining why we need to check for
> > the availability of function 0 when we hotplug a device.
> 
> Please ignore this patch. Its a duplicate of one already sent with an updated patch summary.

I'm not sure which one it is, sorry. Dropped this for now, repost later
pls.

> > 
> > CC: mst@redhat.com
> > Signed-off-by: Ani Sinha <anisinha@redhat.com>
> > ---
> > hw/pci/pci.c | 11 ++++++++---
> > 1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 62b393dfb7..7aee3a7f12 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1181,9 +1181,14 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
> >                    PCI_SLOT(devfn), PCI_FUNC(devfn), name,
> >                    bus->devices[devfn]->name, bus->devices[devfn]->qdev.id);
> >         return NULL;
> > -    } else if (dev->hotplugged &&
> > -               !pci_is_vf(pci_dev) &&
> > -               pci_get_function_0(pci_dev)) {
> > +    } /*
> > +       * Populating function 0 triggers a scan from the guest that
> > +       * exposes other non-zero functions. Hence we need to ensure that
> > +       * function 0 wasn't added yet.
> > +       */
> > +    else if (dev->hotplugged &&
> > +             !pci_is_vf(pci_dev) &&
> > +             pci_get_function_0(pci_dev)) {
> >         error_setg(errp, "PCI: slot %d function 0 already occupied by %s,"
> >                    " new func %s cannot be exposed to guest.",
> >                    PCI_SLOT(pci_get_function_0(pci_dev)->devfn),
> > -- 
> > 2.39.1
> > 

Re: [PATCH v8 6/6] hw/pci: add comment explaining the reason for checking function 0 in hotplug

[Ani Sinha]

> On 11-Jul-2023, at 1:13 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Wed, Jul 05, 2023 at 05:33:31PM +0530, Ani Sinha wrote:
>> 
>> 
>>> On 05-Jul-2023, at 5:29 PM, Ani Sinha <anisinha@redhat.com> wrote:
>>> 
>>> This change is cosmetic. A comment is added explaining why we need to check for
>>> the availability of function 0 when we hotplug a device.
>> 
>> Please ignore this patch. Its a duplicate of one already sent with an updated patch summary.
> 
> I'm not sure which one it is, sorry. Dropped this for now, repost later

Sure. Since this is only a comment addition, should I also CC qemu-trivial?

> pls.
> 
>>> 
>>> CC: mst@redhat.com
>>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>>> ---
>>> hw/pci/pci.c | 11 ++++++++---
>>> 1 file changed, 8 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>> index 62b393dfb7..7aee3a7f12 100644
>>> --- a/hw/pci/pci.c
>>> +++ b/hw/pci/pci.c
>>> @@ -1181,9 +1181,14 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>>>                   PCI_SLOT(devfn), PCI_FUNC(devfn), name,
>>>                   bus->devices[devfn]->name, bus->devices[devfn]->qdev.id);
>>>        return NULL;
>>> -    } else if (dev->hotplugged &&
>>> -               !pci_is_vf(pci_dev) &&
>>> -               pci_get_function_0(pci_dev)) {
>>> +    } /*
>>> +       * Populating function 0 triggers a scan from the guest that
>>> +       * exposes other non-zero functions. Hence we need to ensure that
>>> +       * function 0 wasn't added yet.
>>> +       */
>>> +    else if (dev->hotplugged &&
>>> +             !pci_is_vf(pci_dev) &&
>>> +             pci_get_function_0(pci_dev)) {
>>>        error_setg(errp, "PCI: slot %d function 0 already occupied by %s,"
>>>                   " new func %s cannot be exposed to guest.",
>>>                   PCI_SLOT(pci_get_function_0(pci_dev)->devfn),
>>> -- 
>>> 2.39.1
>>> 
> 

Re: [PATCH v8 6/6] hw/pci: add comment explaining the reason for checking function 0 in hotplug

[Michael Tokarev]

11.07.2023 06:46, Ani Sinha wrote:

> Sure. Since this is only a comment addition, should I also CC qemu-trivial?

A comment change does not mean the change is trivial.  It's a trivial in a sense
the code changes are trivial (actually not-existent), but the meaning of the comment
is not trivial at all. I for one know nothing about this.

So no, this particular change isn't for -trivial, please :)

/mjt

Re: [PATCH v8 6/6] hw/pci: add comment explaining the reason for checking function 0 in hotplug

[Ani Sinha]

> On 11-Jul-2023, at 9:21 AM, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 
> 11.07.2023 06:46, Ani Sinha wrote:
> 
>> Sure. Since this is only a comment addition, should I also CC qemu-trivial?
> 
> A comment change does not mean the change is trivial.  It's a trivial in a sense
> the code changes are trivial (actually not-existent), but the meaning of the comment
> is not trivial at all. I for one know nothing about this.

This comment was already disucussed in qemu-devel between me, mst and Igor. Perhaps you missed it.

https://patchwork.kernel.org/project/qemu-devel/patch/20230621024824.3779-1-anisinha@redhat.com/

Re: [PATCH v8 6/6] hw/pci: add comment explaining the reason for checking function 0 in hotplug

[Michael Tokarev]

11.07.2023 07:03, Ani Sinha wrote:
> 
> 
>> On 11-Jul-2023, at 9:21 AM, Michael Tokarev <mjt@tls.msk.ru> wrote:
>>
>> 11.07.2023 06:46, Ani Sinha wrote:
>>
>>> Sure. Since this is only a comment addition, should I also CC qemu-trivial?
>>
>> A comment change does not mean the change is trivial.  It's a trivial in a sense
>> the code changes are trivial (actually not-existent), but the meaning of the comment
>> is not trivial at all. I for one know nothing about this.
> 
> This comment was already disucussed in qemu-devel between me, mst and Igor. Perhaps you missed it.

It doesn't matter really. The thing is that it needs explanation, hence it is not "trivial",
that's what I tried to say. It is trivial for sure for someone who knows this particular
subsystem well enough, - I'm not one of them ;)

/mjt

Re: [PATCH v8 6/6] hw/pci: add comment explaining the reason for checking function 0 in hotplug

[Ani Sinha]

> On 11-Jul-2023, at 9:43 AM, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 
> 11.07.2023 07:03, Ani Sinha wrote:
>>> On 11-Jul-2023, at 9:21 AM, Michael Tokarev <mjt@tls.msk.ru> wrote:
>>> 
>>> 11.07.2023 06:46, Ani Sinha wrote:
>>> 
>>>> Sure. Since this is only a comment addition, should I also CC qemu-trivial?
>>> 
>>> A comment change does not mean the change is trivial.  It's a trivial in a sense
>>> the code changes are trivial (actually not-existent), but the meaning of the comment
>>> is not trivial at all. I for one know nothing about this.
>> This comment was already discussed in qemu-devel between me, mst and Igor. Perhaps you missed it.
> 
> It doesn't matter really. The thing is that it needs explanation, hence it is not "trivial",
> that's what I tried to say. It is trivial for sure for someone who knows this particular
> subsystem well enough, - I'm not one of them ;)

That’s ok. I was commenting on your statement " I for one know nothing about this.”  I wanted to make sure that people know that patches (at least those I post) are going though the qemu-devel list in a transparent way. It's up to those who are interested or have expertise in a certain area to monitor the list and review/comment on patches. If they want to be explicitly CC’d they can add themselves to MAINTAINERS. The tooling would ensure that they are added in the CC when certain files are updated or modified in the patches.