* [PATCH v7 1/6] tests/acpi: allow changes in DSDT.noacpihp table blob
2023-07-04 11:25 [PATCH v7 0/6] test and QEMU fixes to ensure proper PCIE device usage Ani Sinha
@ 2023-07-04 11:25 ` Ani Sinha
2023-07-04 11:25 ` [PATCH v7 2/6] tests/acpi/bios-tables-test: use the correct slot on the pcie-root-port Ani Sinha
` (4 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: Ani Sinha @ 2023-07-04 11:25 UTC (permalink / raw)
To: qemu-devel, Michael S. Tsirkin, Igor Mammedov, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v7 2/6] tests/acpi/bios-tables-test: use the correct slot on the pcie-root-port
2023-07-04 11:25 [PATCH v7 0/6] test and QEMU fixes to ensure proper PCIE device usage Ani Sinha
2023-07-04 11:25 ` [PATCH v7 1/6] tests/acpi: allow changes in DSDT.noacpihp table blob Ani Sinha
@ 2023-07-04 11:25 ` Ani Sinha
2023-07-04 11:25 ` [PATCH v7 3/6] tests/acpi/bios-tables-test: update acpi blob q35/DSDT.noacpihp Ani Sinha
` (3 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: Ani Sinha @ 2023-07-04 11:25 UTC (permalink / raw)
To: qemu-devel, Michael S. Tsirkin, Igor Mammedov, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v7 3/6] tests/acpi/bios-tables-test: update acpi blob q35/DSDT.noacpihp
2023-07-04 11:25 [PATCH v7 0/6] test and QEMU fixes to ensure proper PCIE device usage Ani Sinha
2023-07-04 11:25 ` [PATCH v7 1/6] tests/acpi: allow changes in DSDT.noacpihp table blob Ani Sinha
2023-07-04 11:25 ` [PATCH v7 2/6] tests/acpi/bios-tables-test: use the correct slot on the pcie-root-port Ani Sinha
@ 2023-07-04 11:25 ` Ani Sinha
2023-07-04 11:25 ` [PATCH v7 4/6] tests/qtest/hd-geo-test: fix incorrect pcie-root-port usage and simplify test Ani Sinha
` (2 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: Ani Sinha @ 2023-07-04 11:25 UTC (permalink / raw)
To: qemu-devel, Michael S. Tsirkin, Igor Mammedov, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v7 4/6] tests/qtest/hd-geo-test: fix incorrect pcie-root-port usage and simplify test
2023-07-04 11:25 [PATCH v7 0/6] test and QEMU fixes to ensure proper PCIE device usage Ani Sinha
` (2 preceding siblings ...)
2023-07-04 11:25 ` [PATCH v7 3/6] tests/acpi/bios-tables-test: update acpi blob q35/DSDT.noacpihp Ani Sinha
@ 2023-07-04 11:25 ` Ani Sinha
2023-07-04 11:25 ` [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port Ani Sinha
2023-07-04 11:25 ` [PATCH v7 6/6] hw/pci: add comment explaining the reason for checking function 0 in hotplug Ani Sinha
5 siblings, 0 replies; 23+ messages in thread
From: Ani Sinha @ 2023-07-04 11:25 UTC (permalink / raw)
To: qemu-devel, Thomas Huth, Laurent Vivier, Paolo Bonzini
Cc: Ani Sinha, mst, imammedo, Michael Labiuk
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
2023-07-04 11:25 [PATCH v7 0/6] test and QEMU fixes to ensure proper PCIE device usage Ani Sinha
` (3 preceding siblings ...)
2023-07-04 11:25 ` [PATCH v7 4/6] tests/qtest/hd-geo-test: fix incorrect pcie-root-port usage and simplify test Ani Sinha
@ 2023-07-04 11:25 ` Ani Sinha
2023-07-04 11:38 ` Ani Sinha
2023-07-04 11:54 ` Akihiko Odaki
2023-07-04 11:25 ` [PATCH v7 6/6] hw/pci: add comment explaining the reason for checking function 0 in hotplug Ani Sinha
5 siblings, 2 replies; 23+ messages in thread
From: Ani Sinha @ 2023-07-04 11:25 UTC (permalink / raw)
To: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum
Cc: Ani Sinha, jusual, imammedo, akihiko.odaki
PCI Express ports only have one slot, 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 on once
we get more clarity on whether we are introducing a bad regression for users
currenly 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 linux igb driver supports). The vfs 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 | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index e2eb4c3b4a..47517ba3db 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,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
}
}
+ /*
+ * 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.
+ */
+ 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
2023-07-04 11:25 ` [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port Ani Sinha
@ 2023-07-04 11:38 ` Ani Sinha
2023-07-04 11:54 ` Akihiko Odaki
1 sibling, 0 replies; 23+ messages in thread
From: Ani Sinha @ 2023-07-04 11:38 UTC (permalink / raw)
To: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum
Cc: jusual, imammedo, akihiko.odaki
> On 04-Jul-2023, at 4:55 PM, Ani Sinha <anisinha@redhat.com> wrote:
>
> PCI Express ports only have one slot, so PCI Express devices can only be
> plugged into slot 0 on a PCIE port. Add a warning to let users know when the
> invalid configuration is used. We may enforce this more strongly later on once
> we get more clarity on whether we are introducing a bad regression for users
> currenly 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 linux igb driver supports). The vfs instantiated correctly
> and are seen to have non-zero device/slot numbers in the conventional PCI BDF
> representation.
I wil send a v8 with the patch subject line changed to something like
"hw/pci: warn when PCIE devices are plugged into non-zero slot of PCIE port”
which is more appropriate. Meanwhile, I will wait to get more comments/tags on v7.
>
> 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 | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index e2eb4c3b4a..47517ba3db 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,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> }
> }
>
> + /*
> + * 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.
> + */
> + 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
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
2023-07-04 11:25 ` [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port Ani Sinha
2023-07-04 11:38 ` Ani Sinha
@ 2023-07-04 11:54 ` Akihiko Odaki
2023-07-04 11:59 ` Ani Sinha
1 sibling, 1 reply; 23+ messages in thread
From: Akihiko Odaki @ 2023-07-04 11:54 UTC (permalink / raw)
To: Ani Sinha, qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum
Cc: jusual, imammedo
On 2023/07/04 20:25, 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. Add a warning to let users know when the
> invalid configuration is used. We may enforce this more strongly later on once
> we get more clarity on whether we are introducing a bad regression for users
> currenly 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 linux igb driver supports). The vfs 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 | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index e2eb4c3b4a..47517ba3db 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,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> }
> }
>
> + /*
> + * 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.
You don't have to mention SR/IOV; it affects all ARI-capable devices. A
PF can also have non-zero slot number in the conventional interpretation
so you shouldn't call it vf either.
> + */
> + 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 "
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
2023-07-04 11:54 ` Akihiko Odaki
@ 2023-07-04 11:59 ` Ani Sinha
2023-07-04 12:02 ` Akihiko Odaki
0 siblings, 1 reply; 23+ messages in thread
From: Ani Sinha @ 2023-07-04 11:59 UTC (permalink / raw)
To: Akihiko Odaki
Cc: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum, Julia Suvorova,
imammedo
> On 04-Jul-2023, at 5:24 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2023/07/04 20:25, 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. Add a warning to let users know when the
>> invalid configuration is used. We may enforce this more strongly later on once
>> we get more clarity on whether we are introducing a bad regression for users
>> currenly 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 linux igb driver supports). The vfs 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 | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index e2eb4c3b4a..47517ba3db 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,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>> }
>> }
>> + /*
>> + * 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.
>
> You don't have to mention SR/IOV; it affects all ARI-capable devices. A PF can also have non-zero slot number in the conventional interpretation so you shouldn't call it vf either.
Can you please help write a comment that explains this properly for all cases - ARI/non-ARI, PFs and VFs? Once everyone agrees that its clear and correct, I will re-spin.
>
>> + */
>> + 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 "
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
2023-07-04 11:59 ` Ani Sinha
@ 2023-07-04 12:02 ` Akihiko Odaki
2023-07-04 12:08 ` Ani Sinha
2023-07-04 12:48 ` Igor Mammedov
0 siblings, 2 replies; 23+ messages in thread
From: Akihiko Odaki @ 2023-07-04 12:02 UTC (permalink / raw)
To: Ani Sinha
Cc: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum, Julia Suvorova,
imammedo
On 2023/07/04 20:59, Ani Sinha wrote:
>
>
>> On 04-Jul-2023, at 5:24 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2023/07/04 20:25, 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. Add a warning to let users know when the
>>> invalid configuration is used. We may enforce this more strongly later on once
>>> we get more clarity on whether we are introducing a bad regression for users
>>> currenly 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 linux igb driver supports). The vfs 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 | 15 +++++++++++++++
>>> 1 file changed, 15 insertions(+)
>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>> index e2eb4c3b4a..47517ba3db 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,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>>> }
>>> }
>>> + /*
>>> + * 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.
>>
>> You don't have to mention SR/IOV; it affects all ARI-capable devices. A PF can also have non-zero slot number in the conventional interpretation so you shouldn't call it vf either.
>
> Can you please help write a comment that explains this properly for all cases - ARI/non-ARI, PFs and VFs? Once everyone agrees that its clear and correct, I will re-spin.
Simply, you can say:
With ARI, the slot number field in the conventional PCI interpretation
can have a non-zero value as the field bits are reused to extend the
function number bits. Ignore that case.
>
>>
>>> + */
>>> + 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 "
>>
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
2023-07-04 12:02 ` Akihiko Odaki
@ 2023-07-04 12:08 ` Ani Sinha
2023-07-04 12:09 ` Akihiko Odaki
2023-07-04 12:48 ` Igor Mammedov
1 sibling, 1 reply; 23+ messages in thread
From: Ani Sinha @ 2023-07-04 12:08 UTC (permalink / raw)
To: Akihiko Odaki
Cc: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum, Julia Suvorova,
imammedo
> On 04-Jul-2023, at 5:32 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2023/07/04 20:59, Ani Sinha wrote:
>>> On 04-Jul-2023, at 5:24 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>
>>> On 2023/07/04 20:25, 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. Add a warning to let users know when the
>>>> invalid configuration is used. We may enforce this more strongly later on once
>>>> we get more clarity on whether we are introducing a bad regression for users
>>>> currenly 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 linux igb driver supports). The vfs 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 | 15 +++++++++++++++
>>>> 1 file changed, 15 insertions(+)
>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>> index e2eb4c3b4a..47517ba3db 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,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>>>> }
>>>> }
>>>> + /*
>>>> + * 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.
>>>
>>> You don't have to mention SR/IOV; it affects all ARI-capable devices. A PF can also have non-zero slot number in the conventional interpretation so you shouldn't call it vf either.
>> Can you please help write a comment that explains this properly for all cases - ARI/non-ARI, PFs and VFs? Once everyone agrees that its clear and correct, I will re-spin.
>
> Simply, you can say:
> With ARI, the slot number field in the conventional PCI interpretation can have a non-zero value as the field bits are reused to extend the function number bits. Ignore that case.
but we are not checking for ARI capability here in the code. So the comment is confusing.
>
>>>
>>>> + */
>>>> + 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 "
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
2023-07-04 12:08 ` Ani Sinha
@ 2023-07-04 12:09 ` Akihiko Odaki
2023-07-04 12:28 ` Ani Sinha
0 siblings, 1 reply; 23+ messages in thread
From: Akihiko Odaki @ 2023-07-04 12:09 UTC (permalink / raw)
To: Ani Sinha
Cc: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum, Julia Suvorova,
imammedo
On 2023/07/04 21:08, Ani Sinha wrote:
>
>
>> On 04-Jul-2023, at 5:32 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2023/07/04 20:59, Ani Sinha wrote:
>>>> On 04-Jul-2023, at 5:24 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> On 2023/07/04 20:25, 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. Add a warning to let users know when the
>>>>> invalid configuration is used. We may enforce this more strongly later on once
>>>>> we get more clarity on whether we are introducing a bad regression for users
>>>>> currenly 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 linux igb driver supports). The vfs 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 | 15 +++++++++++++++
>>>>> 1 file changed, 15 insertions(+)
>>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>>> index e2eb4c3b4a..47517ba3db 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,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>>>>> }
>>>>> }
>>>>> + /*
>>>>> + * 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.
>>>>
>>>> You don't have to mention SR/IOV; it affects all ARI-capable devices. A PF can also have non-zero slot number in the conventional interpretation so you shouldn't call it vf either.
>>> Can you please help write a comment that explains this properly for all cases - ARI/non-ARI, PFs and VFs? Once everyone agrees that its clear and correct, I will re-spin.
>>
>> Simply, you can say:
>> With ARI, the slot number field in the conventional PCI interpretation can have a non-zero value as the field bits are reused to extend the function number bits. Ignore that case.
>
> but we are not checking for ARI capability here in the code. So the comment is confusing.
Don't we? We check for:
!pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI)
>
>>
>>>>
>>>>> + */
>>>>> + 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 "
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
2023-07-04 12:09 ` Akihiko Odaki
@ 2023-07-04 12:28 ` Ani Sinha
0 siblings, 0 replies; 23+ messages in thread
From: Ani Sinha @ 2023-07-04 12:28 UTC (permalink / raw)
To: Akihiko Odaki
Cc: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum, Julia Suvorova,
Igor Mammedov
[-- Attachment #1: Type: text/plain, Size: 4152 bytes --]
On Tue, 4 Jul, 2023, 5:39 pm Akihiko Odaki, <akihiko.odaki@daynix.com>
wrote:
> On 2023/07/04 21:08, Ani Sinha wrote:
> >
> >
> >> On 04-Jul-2023, at 5:32 PM, Akihiko Odaki <akihiko.odaki@daynix.com>
> wrote:
> >>
> >> On 2023/07/04 20:59, Ani Sinha wrote:
> >>>> On 04-Jul-2023, at 5:24 PM, Akihiko Odaki <akihiko.odaki@daynix.com>
> wrote:
> >>>>
> >>>> On 2023/07/04 20:25, 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. Add a warning to let users know
> when the
> >>>>> invalid configuration is used. We may enforce this more strongly
> later on once
> >>>>> we get more clarity on whether we are introducing a bad regression
> for users
> >>>>> currenly 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 linux igb driver supports). The vfs 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 | 15 +++++++++++++++
> >>>>> 1 file changed, 15 insertions(+)
> >>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >>>>> index e2eb4c3b4a..47517ba3db 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,20 @@ static void pci_qdev_realize(DeviceState
> *qdev, Error **errp)
> >>>>> }
> >>>>> }
> >>>>> + /*
> >>>>> + * 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.
> >>>>
> >>>> You don't have to mention SR/IOV; it affects all ARI-capable devices.
> A PF can also have non-zero slot number in the conventional interpretation
> so you shouldn't call it vf either.
> >>> Can you please help write a comment that explains this properly for
> all cases - ARI/non-ARI, PFs and VFs? Once everyone agrees that its clear
> and correct, I will re-spin.
> >>
> >> Simply, you can say:
> >> With ARI, the slot number field in the conventional PCI interpretation
> can have a non-zero value as the field bits are reused to extend the
> function number bits. Ignore that case.
> >
> > but we are not checking for ARI capability here in the code. So the
> comment is confusing.
>
> Don't we? We check for:
> !pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI)
>
Yes I was thinking of patch 6 in the series which also adds a comment for
ARI.
I'll wait to see what others thought of your suggestion before respinning
patch 5
>
> >>
> >>>>
> >>>>> + */
> >>>>> + 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 "
> >
>
>
[-- Attachment #2: Type: text/html, Size: 6765 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
2023-07-04 12:02 ` Akihiko Odaki
2023-07-04 12:08 ` Ani Sinha
@ 2023-07-04 12:48 ` Igor Mammedov
2023-07-04 13:50 ` Ani Sinha
1 sibling, 1 reply; 23+ messages in thread
From: Igor Mammedov @ 2023-07-04 12:48 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Ani Sinha, qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum,
Julia Suvorova
On Tue, 4 Jul 2023 21:02:09 +0900
Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> On 2023/07/04 20:59, Ani Sinha wrote:
> >
> >
> >> On 04-Jul-2023, at 5:24 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> On 2023/07/04 20:25, 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. Add a warning to let users know when the
> >>> invalid configuration is used. We may enforce this more strongly later on once
> >>> we get more clarity on whether we are introducing a bad regression for users
> >>> currenly 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 linux igb driver supports). The vfs 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 | 15 +++++++++++++++
> >>> 1 file changed, 15 insertions(+)
> >>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >>> index e2eb4c3b4a..47517ba3db 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,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> >>> }
> >>> }
> >>> + /*
> >>> + * 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.
> >>
> >> You don't have to mention SR/IOV; it affects all ARI-capable devices. A PF can also have non-zero slot number in the conventional interpretation so you shouldn't call it vf either.
> >
> > Can you please help write a comment that explains this properly for all cases - ARI/non-ARI, PFs and VFs? Once everyone agrees that its clear and correct, I will re-spin.
>
> Simply, you can say:
> With ARI, the slot number field in the conventional PCI interpretation
> can have a non-zero value as the field bits are reused to extend the
> function number bits. Ignore that case.
mentioning 'conventional PCI interpretation' in comment and then immediately
checking 'pci_is_express(pci_dev)' is confusing. Since comment belongs
only to PCIE branch it would be better to talk in only about PCIe stuff
and referring to relevant portions of spec.
(for example see how it's done in kernel code: only_one_child(...)
PS:
kernel can be forced to scan for !0 device numbers, but that's rather
a hack, so we shouldn't really care about that.
>
> >
> >>
> >>> + */
> >>> + 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 "
> >>
> >
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
2023-07-04 12:48 ` Igor Mammedov
@ 2023-07-04 13:50 ` Ani Sinha
2023-07-04 14:28 ` Igor Mammedov
0 siblings, 1 reply; 23+ messages in thread
From: Ani Sinha @ 2023-07-04 13:50 UTC (permalink / raw)
To: Igor Mammedov
Cc: Akihiko Odaki, qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum,
Julia Suvorova
> On 04-Jul-2023, at 6:18 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Tue, 4 Jul 2023 21:02:09 +0900
> Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
>> On 2023/07/04 20:59, Ani Sinha wrote:
>>>
>>>
>>>> On 04-Jul-2023, at 5:24 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> On 2023/07/04 20:25, 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. Add a warning to let users know when the
>>>>> invalid configuration is used. We may enforce this more strongly later on once
>>>>> we get more clarity on whether we are introducing a bad regression for users
>>>>> currenly 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 linux igb driver supports). The vfs 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 | 15 +++++++++++++++
>>>>> 1 file changed, 15 insertions(+)
>>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>>> index e2eb4c3b4a..47517ba3db 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,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>>>>> }
>>>>> }
>>>>> + /*
>>>>> + * 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.
>>>>
>>>> You don't have to mention SR/IOV; it affects all ARI-capable devices. A PF can also have non-zero slot number in the conventional interpretation so you shouldn't call it vf either.
>>>
>>> Can you please help write a comment that explains this properly for all cases - ARI/non-ARI, PFs and VFs? Once everyone agrees that its clear and correct, I will re-spin.
>>
>> Simply, you can say:
>> With ARI, the slot number field in the conventional PCI interpretation
>> can have a non-zero value as the field bits are reused to extend the
>> function number bits. Ignore that case.
>
> mentioning 'conventional PCI interpretation' in comment and then immediately
> checking 'pci_is_express(pci_dev)' is confusing. Since comment belongs
> only to PCIE branch it would be better to talk in only about PCIe stuff
> and referring to relevant portions of spec.
Ok so how about this?
* With ARI, devices can have non-zero slot in the traditional BDF
* representation as all five bits reserved for slot addresses are
* also used for function bits. Ignore that case.
> (for example see how it's done in kernel code: only_one_child(...)
>
> PS:
> kernel can be forced to scan for !0 device numbers, but that's rather
> a hack, so we shouldn't really care about that.
>
>>
>>>
>>>>
>>>>> + */
>>>>> + 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 "
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
2023-07-04 13:50 ` Ani Sinha
@ 2023-07-04 14:28 ` Igor Mammedov
2023-07-04 15:07 ` Ani Sinha
0 siblings, 1 reply; 23+ messages in thread
From: Igor Mammedov @ 2023-07-04 14:28 UTC (permalink / raw)
To: Ani Sinha
Cc: Akihiko Odaki, qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum,
Julia Suvorova
On Tue, 4 Jul 2023 19:20:00 +0530
Ani Sinha <anisinha@redhat.com> wrote:
> > On 04-Jul-2023, at 6:18 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Tue, 4 Jul 2023 21:02:09 +0900
> > Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >
> >> On 2023/07/04 20:59, Ani Sinha wrote:
> >>>
> >>>
> >>>> On 04-Jul-2023, at 5:24 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>>>
> >>>> On 2023/07/04 20:25, 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. Add a warning to let users know when the
> >>>>> invalid configuration is used. We may enforce this more strongly later on once
> >>>>> we get more clarity on whether we are introducing a bad regression for users
> >>>>> currenly 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 linux igb driver supports). The vfs 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 | 15 +++++++++++++++
> >>>>> 1 file changed, 15 insertions(+)
> >>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >>>>> index e2eb4c3b4a..47517ba3db 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,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> >>>>> }
> >>>>> }
> >>>>> + /*
> >>>>> + * 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.
> >>>>
> >>>> You don't have to mention SR/IOV; it affects all ARI-capable devices. A PF can also have non-zero slot number in the conventional interpretation so you shouldn't call it vf either.
> >>>
> >>> Can you please help write a comment that explains this properly for all cases - ARI/non-ARI, PFs and VFs? Once everyone agrees that its clear and correct, I will re-spin.
> >>
> >> Simply, you can say:
> >> With ARI, the slot number field in the conventional PCI interpretation
> >> can have a non-zero value as the field bits are reused to extend the
> >> function number bits. Ignore that case.
> >
> > mentioning 'conventional PCI interpretation' in comment and then immediately
> > checking 'pci_is_express(pci_dev)' is confusing. Since comment belongs
> > only to PCIE branch it would be better to talk in only about PCIe stuff
> > and referring to relevant portions of spec.
>
> Ok so how about this?
>
> * With ARI, devices can have non-zero slot in the traditional BDF
> * representation as all five bits reserved for slot addresses are
> * also used for function bits. Ignore that case.
you still refer to traditional (which I misread as 'conventional'),
steal the linux comment and argument it with ARI if necessary,
something like this (probably needs some more massaging):
/*
* A PCIe Downstream Port normally leads to a Link with only Device
* 0 on it (PCIe spec r3.1, sec 7.3.1).
However PCI_SLOT() is broken if ARI is enabled, hence work around it
by skipping check if the later cap is present.
*/
>
>
> > (for example see how it's done in kernel code: only_one_child(...)
> >
> > PS:
> > kernel can be forced to scan for !0 device numbers, but that's rather
> > a hack, so we shouldn't really care about that.
> >
> >>
> >>>
> >>>>
> >>>>> + */
> >>>>> + 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 "
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
2023-07-04 14:28 ` Igor Mammedov
@ 2023-07-04 15:07 ` Ani Sinha
2023-07-05 1:39 ` Akihiko Odaki
0 siblings, 1 reply; 23+ messages in thread
From: Ani Sinha @ 2023-07-04 15:07 UTC (permalink / raw)
To: Igor Mammedov
Cc: Akihiko Odaki, qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum,
Julia Suvorova
> On 04-Jul-2023, at 7:58 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Tue, 4 Jul 2023 19:20:00 +0530
> Ani Sinha <anisinha@redhat.com> wrote:
>
>>> On 04-Jul-2023, at 6:18 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>>>
>>> On Tue, 4 Jul 2023 21:02:09 +0900
>>> Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>
>>>> On 2023/07/04 20:59, Ani Sinha wrote:
>>>>>
>>>>>
>>>>>> On 04-Jul-2023, at 5:24 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>>
>>>>>> On 2023/07/04 20:25, 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. Add a warning to let users know when the
>>>>>>> invalid configuration is used. We may enforce this more strongly later on once
>>>>>>> we get more clarity on whether we are introducing a bad regression for users
>>>>>>> currenly 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 linux igb driver supports). The vfs 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 | 15 +++++++++++++++
>>>>>>> 1 file changed, 15 insertions(+)
>>>>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>>>>> index e2eb4c3b4a..47517ba3db 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,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>>>>>>> }
>>>>>>> }
>>>>>>> + /*
>>>>>>> + * 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.
>>>>>>
>>>>>> You don't have to mention SR/IOV; it affects all ARI-capable devices. A PF can also have non-zero slot number in the conventional interpretation so you shouldn't call it vf either.
>>>>>
>>>>> Can you please help write a comment that explains this properly for all cases - ARI/non-ARI, PFs and VFs? Once everyone agrees that its clear and correct, I will re-spin.
>>>>
>>>> Simply, you can say:
>>>> With ARI, the slot number field in the conventional PCI interpretation
>>>> can have a non-zero value as the field bits are reused to extend the
>>>> function number bits. Ignore that case.
>>>
>>> mentioning 'conventional PCI interpretation' in comment and then immediately
>>> checking 'pci_is_express(pci_dev)' is confusing. Since comment belongs
>>> only to PCIE branch it would be better to talk in only about PCIe stuff
>>> and referring to relevant portions of spec.
>>
>> Ok so how about this?
>>
>> * With ARI, devices can have non-zero slot in the traditional BDF
>> * representation as all five bits reserved for slot addresses are
>> * also used for function bits. Ignore that case.
>
> you still refer to traditional (which I misread as 'conventional'),
> steal the linux comment and argument it with ARI if necessary,
> something like this (probably needs some more massaging):
The comment messaging in these patches seems to exceed the value of the patch itself :-)
How about this?
/*
* A PCIe Downstream Port normally leads to a Link with only Device
* 0 on it (PCIe spec r3.1, sec 7.3.1).
* With ARI, PCI_SLOT() can return non-zero value as all five bits
* reserved for slot addresses are also used for function bits.
* Hence, ignore ARI capable devices.
*/
>
>
> /*
> * A PCIe Downstream Port normally leads to a Link with only Device
> * 0 on it (PCIe spec r3.1, sec 7.3.1).
> However PCI_SLOT() is broken if ARI is enabled, hence work around it
> by skipping check if the later cap is present.
> */
>
>>
>>
>>> (for example see how it's done in kernel code: only_one_child(...)
>>>
>>> PS:
>>> kernel can be forced to scan for !0 device numbers, but that's rather
>>> a hack, so we shouldn't really care about that.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>> + */
>>>>>>> + 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 "
>>
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
2023-07-04 15:07 ` Ani Sinha
@ 2023-07-05 1:39 ` Akihiko Odaki
2023-07-05 5:43 ` Ani Sinha
0 siblings, 1 reply; 23+ messages in thread
From: Akihiko Odaki @ 2023-07-05 1:39 UTC (permalink / raw)
To: Ani Sinha, Igor Mammedov
Cc: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum, Julia Suvorova
On 2023/07/05 0:07, Ani Sinha wrote:
>
>
>> On 04-Jul-2023, at 7:58 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>>
>> On Tue, 4 Jul 2023 19:20:00 +0530
>> Ani Sinha <anisinha@redhat.com> wrote:
>>
>>>> On 04-Jul-2023, at 6:18 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>>>>
>>>> On Tue, 4 Jul 2023 21:02:09 +0900
>>>> Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>>> On 2023/07/04 20:59, Ani Sinha wrote:
>>>>>>
>>>>>>
>>>>>>> On 04-Jul-2023, at 5:24 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>>>
>>>>>>> On 2023/07/04 20:25, 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. Add a warning to let users know when the
>>>>>>>> invalid configuration is used. We may enforce this more strongly later on once
>>>>>>>> we get more clarity on whether we are introducing a bad regression for users
>>>>>>>> currenly 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 linux igb driver supports). The vfs 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 | 15 +++++++++++++++
>>>>>>>> 1 file changed, 15 insertions(+)
>>>>>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>>>>>> index e2eb4c3b4a..47517ba3db 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,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>>>>>>>> }
>>>>>>>> }
>>>>>>>> + /*
>>>>>>>> + * 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.
>>>>>>>
>>>>>>> You don't have to mention SR/IOV; it affects all ARI-capable devices. A PF can also have non-zero slot number in the conventional interpretation so you shouldn't call it vf either.
>>>>>>
>>>>>> Can you please help write a comment that explains this properly for all cases - ARI/non-ARI, PFs and VFs? Once everyone agrees that its clear and correct, I will re-spin.
>>>>>
>>>>> Simply, you can say:
>>>>> With ARI, the slot number field in the conventional PCI interpretation
>>>>> can have a non-zero value as the field bits are reused to extend the
>>>>> function number bits. Ignore that case.
>>>>
>>>> mentioning 'conventional PCI interpretation' in comment and then immediately
>>>> checking 'pci_is_express(pci_dev)' is confusing. Since comment belongs
>>>> only to PCIE branch it would be better to talk in only about PCIe stuff
>>>> and referring to relevant portions of spec.
>>>
>>> Ok so how about this?
>>>
>>> * With ARI, devices can have non-zero slot in the traditional BDF
>>> * representation as all five bits reserved for slot addresses are
>>> * also used for function bits. Ignore that case.
>>
>> you still refer to traditional (which I misread as 'conventional'),
>> steal the linux comment and argument it with ARI if necessary,
>> something like this (probably needs some more massaging):
>
> The comment messaging in these patches seems to exceed the value of the patch itself :-)
>
> How about this?
>
> /*
> * A PCIe Downstream Port normally leads to a Link with only Device
> * 0 on it (PCIe spec r3.1, sec 7.3.1).
> * With ARI, PCI_SLOT() can return non-zero value as all five bits
> * reserved for slot addresses are also used for function bits.
> * Hence, ignore ARI capable devices.
> */
Perhaps: s/normally leads to/must lead to/
From the kernel perspective, they may need to deal with a quirky
hardware that does not conform with the specification, but from QEMU
perspective, it is what we *must* conform with.
Otherwise looks good to me.
>
>>
>>
>> /*
>> * A PCIe Downstream Port normally leads to a Link with only Device
>> * 0 on it (PCIe spec r3.1, sec 7.3.1).
>> However PCI_SLOT() is broken if ARI is enabled, hence work around it
>> by skipping check if the later cap is present.
>> */
>>
>>>
>>>
>>>> (for example see how it's done in kernel code: only_one_child(...)
>>>>
>>>> PS:
>>>> kernel can be forced to scan for !0 device numbers, but that's rather
>>>> a hack, so we shouldn't really care about that.
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> + */
>>>>>>>> + 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 "
>>>
>>
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
2023-07-05 1:39 ` Akihiko Odaki
@ 2023-07-05 5:43 ` Ani Sinha
2023-07-05 10:42 ` Akihiko Odaki
0 siblings, 1 reply; 23+ messages in thread
From: Ani Sinha @ 2023-07-05 5:43 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Igor Mammedov, qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum,
Julia Suvorova
> On 05-Jul-2023, at 7:09 AM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
>
>
> On 2023/07/05 0:07, Ani Sinha wrote:
>>> On 04-Jul-2023, at 7:58 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>>>
>>> On Tue, 4 Jul 2023 19:20:00 +0530
>>> Ani Sinha <anisinha@redhat.com> wrote:
>>>
>>>>> On 04-Jul-2023, at 6:18 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>>>>>
>>>>> On Tue, 4 Jul 2023 21:02:09 +0900
>>>>> Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>
>>>>>> On 2023/07/04 20:59, Ani Sinha wrote:
>>>>>>>
>>>>>>>
>>>>>>>> On 04-Jul-2023, at 5:24 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>>>>
>>>>>>>> On 2023/07/04 20:25, 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. Add a warning to let users know when the
>>>>>>>>> invalid configuration is used. We may enforce this more strongly later on once
>>>>>>>>> we get more clarity on whether we are introducing a bad regression for users
>>>>>>>>> currenly 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 linux igb driver supports). The vfs 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 | 15 +++++++++++++++
>>>>>>>>> 1 file changed, 15 insertions(+)
>>>>>>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>>>>>>> index e2eb4c3b4a..47517ba3db 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,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>>>>>>>>> }
>>>>>>>>> }
>>>>>>>>> + /*
>>>>>>>>> + * 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.
>>>>>>>>
>>>>>>>> You don't have to mention SR/IOV; it affects all ARI-capable devices. A PF can also have non-zero slot number in the conventional interpretation so you shouldn't call it vf either.
>>>>>>>
>>>>>>> Can you please help write a comment that explains this properly for all cases - ARI/non-ARI, PFs and VFs? Once everyone agrees that its clear and correct, I will re-spin.
>>>>>>
>>>>>> Simply, you can say:
>>>>>> With ARI, the slot number field in the conventional PCI interpretation
>>>>>> can have a non-zero value as the field bits are reused to extend the
>>>>>> function number bits. Ignore that case.
>>>>>
>>>>> mentioning 'conventional PCI interpretation' in comment and then immediately
>>>>> checking 'pci_is_express(pci_dev)' is confusing. Since comment belongs
>>>>> only to PCIE branch it would be better to talk in only about PCIe stuff
>>>>> and referring to relevant portions of spec.
>>>>
>>>> Ok so how about this?
>>>>
>>>> * With ARI, devices can have non-zero slot in the traditional BDF
>>>> * representation as all five bits reserved for slot addresses are
>>>> * also used for function bits. Ignore that case.
>>>
>>> you still refer to traditional (which I misread as 'conventional'),
>>> steal the linux comment and argument it with ARI if necessary,
>>> something like this (probably needs some more massaging):
>> The comment messaging in these patches seems to exceed the value of the patch itself :-)
>> How about this?
>> /*
>> * A PCIe Downstream Port normally leads to a Link with only Device
>> * 0 on it (PCIe spec r3.1, sec 7.3.1).
>> * With ARI, PCI_SLOT() can return non-zero value as all five bits
>> * reserved for slot addresses are also used for function bits.
>> * Hence, ignore ARI capable devices.
>> */
>
> Perhaps: s/normally leads to/must lead to/
>
> From the kernel perspective, they may need to deal with a quirky hardware that does not conform with the specification, but from QEMU perspective, it is what we *must* conform with.
PCI base spec 4.0, rev 3, section 7.3.1 says:
"
Downstream Ports that do not have ARI Forwarding enabled must associate only Device 0 with the device attached to the Logical Bus representing the Link from the Port. Configuration Requests 15 targeting the Bus Number associated with a Link specifying Device Number 0 are delivered to the device attached to the Link; Configuration Requests specifying all other Device Numbers (1-31) must be terminated by the Switch Downstream Port or the Root Port with an Unsupported Request Completion Status (equivalent to Master Abort in PCI). Non-ARI Devices must not assume that Device Number 0 is associated with their Upstream Port, but must capture their assigned Device Number as discussed in Section 2.2.6.2. Non-ARI Devices must respond to all Type 0 Configuration Read Requests, regardless of the Device Number specified in the Request.
…
With an ARI Device, its Device Number is implied to be 0 rather than specified by a field within an ID. 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. See Section 6.13. Any Type 0 Configuration Request targeting an unimplemented Function in an ARI Device must be handled as an Unsupported Request.
“
So it seems they do indeed use the “must” clause. I prefer to use the line from the spec verbatim as possible. Hence, this is what I am going with and be done with this patchset:
/*
* 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.
*/
>
> Otherwise looks good to me.
>
>>>
>>>
>>> /*
>>> * A PCIe Downstream Port normally leads to a Link with only Device
>>> * 0 on it (PCIe spec r3.1, sec 7.3.1).
>>> However PCI_SLOT() is broken if ARI is enabled, hence work around it
>>> by skipping check if the later cap is present.
>>> */
>>>
>>>>
>>>>
>>>>> (for example see how it's done in kernel code: only_one_child(...)
>>>>>
>>>>> PS:
>>>>> kernel can be forced to scan for !0 device numbers, but that's rather
>>>>> a hack, so we shouldn't really care about that.
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> + */
>>>>>>>>> + 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 "
>>>>
>>>
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port
2023-07-05 5:43 ` Ani Sinha
@ 2023-07-05 10:42 ` Akihiko Odaki
0 siblings, 0 replies; 23+ messages in thread
From: Akihiko Odaki @ 2023-07-05 10:42 UTC (permalink / raw)
To: Ani Sinha
Cc: Igor Mammedov, qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum,
Julia Suvorova
On 2023/07/05 14:43, Ani Sinha wrote:
>
>
>> On 05-Jul-2023, at 7:09 AM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>>
>>
>> On 2023/07/05 0:07, Ani Sinha wrote:
>>>> On 04-Jul-2023, at 7:58 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>>>>
>>>> On Tue, 4 Jul 2023 19:20:00 +0530
>>>> Ani Sinha <anisinha@redhat.com> wrote:
>>>>
>>>>>> On 04-Jul-2023, at 6:18 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>>>>>>
>>>>>> On Tue, 4 Jul 2023 21:02:09 +0900
>>>>>> Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>>
>>>>>>> On 2023/07/04 20:59, Ani Sinha wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>> On 04-Jul-2023, at 5:24 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>>>>>
>>>>>>>>> On 2023/07/04 20:25, 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. Add a warning to let users know when the
>>>>>>>>>> invalid configuration is used. We may enforce this more strongly later on once
>>>>>>>>>> we get more clarity on whether we are introducing a bad regression for users
>>>>>>>>>> currenly 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 linux igb driver supports). The vfs 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 | 15 +++++++++++++++
>>>>>>>>>> 1 file changed, 15 insertions(+)
>>>>>>>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>>>>>>>> index e2eb4c3b4a..47517ba3db 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,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>>>>>>>>>> }
>>>>>>>>>> }
>>>>>>>>>> + /*
>>>>>>>>>> + * 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.
>>>>>>>>>
>>>>>>>>> You don't have to mention SR/IOV; it affects all ARI-capable devices. A PF can also have non-zero slot number in the conventional interpretation so you shouldn't call it vf either.
>>>>>>>>
>>>>>>>> Can you please help write a comment that explains this properly for all cases - ARI/non-ARI, PFs and VFs? Once everyone agrees that its clear and correct, I will re-spin.
>>>>>>>
>>>>>>> Simply, you can say:
>>>>>>> With ARI, the slot number field in the conventional PCI interpretation
>>>>>>> can have a non-zero value as the field bits are reused to extend the
>>>>>>> function number bits. Ignore that case.
>>>>>>
>>>>>> mentioning 'conventional PCI interpretation' in comment and then immediately
>>>>>> checking 'pci_is_express(pci_dev)' is confusing. Since comment belongs
>>>>>> only to PCIE branch it would be better to talk in only about PCIe stuff
>>>>>> and referring to relevant portions of spec.
>>>>>
>>>>> Ok so how about this?
>>>>>
>>>>> * With ARI, devices can have non-zero slot in the traditional BDF
>>>>> * representation as all five bits reserved for slot addresses are
>>>>> * also used for function bits. Ignore that case.
>>>>
>>>> you still refer to traditional (which I misread as 'conventional'),
>>>> steal the linux comment and argument it with ARI if necessary,
>>>> something like this (probably needs some more massaging):
>>> The comment messaging in these patches seems to exceed the value of the patch itself :-)
>>> How about this?
>>> /*
>>> * A PCIe Downstream Port normally leads to a Link with only Device
>>> * 0 on it (PCIe spec r3.1, sec 7.3.1).
>>> * With ARI, PCI_SLOT() can return non-zero value as all five bits
>>> * reserved for slot addresses are also used for function bits.
>>> * Hence, ignore ARI capable devices.
>>> */
>>
>> Perhaps: s/normally leads to/must lead to/
>>
>> From the kernel perspective, they may need to deal with a quirky hardware that does not conform with the specification, but from QEMU perspective, it is what we *must* conform with.
>
> PCI base spec 4.0, rev 3, section 7.3.1 says:
>
> "
> Downstream Ports that do not have ARI Forwarding enabled must associate only Device 0 with the device attached to the Logical Bus representing the Link from the Port. Configuration Requests 15 targeting the Bus Number associated with a Link specifying Device Number 0 are delivered to the device attached to the Link; Configuration Requests specifying all other Device Numbers (1-31) must be terminated by the Switch Downstream Port or the Root Port with an Unsupported Request Completion Status (equivalent to Master Abort in PCI). Non-ARI Devices must not assume that Device Number 0 is associated with their Upstream Port, but must capture their assigned Device Number as discussed in Section 2.2.6.2. Non-ARI Devices must respond to all Type 0 Configuration Read Requests, regardless of the Device Number specified in the Request.
>
> …
>
> With an ARI Device, its Device Number is implied to be 0 rather than specified by a field within an ID. 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. See Section 6.13. Any Type 0 Configuration Request targeting an unimplemented Function in an ARI Device must be handled as an Unsupported Request.
>
> “
>
> So it seems they do indeed use the “must” clause. I prefer to use the line from the spec verbatim as possible. Hence, this is what I am going with and be done with this patchset:
>
> /*
> * 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.
> */
Looks perfect.
>
>
>>
>> Otherwise looks good to me.
>>
>>>>
>>>>
>>>> /*
>>>> * A PCIe Downstream Port normally leads to a Link with only Device
>>>> * 0 on it (PCIe spec r3.1, sec 7.3.1).
>>>> However PCI_SLOT() is broken if ARI is enabled, hence work around it
>>>> by skipping check if the later cap is present.
>>>> */
>>>>
>>>>>
>>>>>
>>>>>> (for example see how it's done in kernel code: only_one_child(...)
>>>>>>
>>>>>> PS:
>>>>>> kernel can be forced to scan for !0 device numbers, but that's rather
>>>>>> a hack, so we shouldn't really care about that.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> + */
>>>>>>>>>> + 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 "
>>>>>
>>>>
>>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v7 6/6] hw/pci: add comment explaining the reason for checking function 0 in hotplug
2023-07-04 11:25 [PATCH v7 0/6] test and QEMU fixes to ensure proper PCIE device usage Ani Sinha
` (4 preceding siblings ...)
2023-07-04 11:25 ` [PATCH v7 5/6] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port Ani Sinha
@ 2023-07-04 11:25 ` Ani Sinha
2023-07-04 12:15 ` Igor Mammedov
5 siblings, 1 reply; 23+ messages in thread
From: Ani Sinha @ 2023-07-04 11:25 UTC (permalink / raw)
To: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum; +Cc: 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 | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 47517ba3db..e3ff3808b6 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1181,9 +1181,15 @@ 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. With SRIOV and with or without ARI
+ * the PF must be hotplugged into function 0 for it to be detected.
+ */
+ 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v7 6/6] hw/pci: add comment explaining the reason for checking function 0 in hotplug
2023-07-04 11:25 ` [PATCH v7 6/6] hw/pci: add comment explaining the reason for checking function 0 in hotplug Ani Sinha
@ 2023-07-04 12:15 ` Igor Mammedov
2023-07-04 12:31 ` Ani Sinha
0 siblings, 1 reply; 23+ messages in thread
From: Igor Mammedov @ 2023-07-04 12:15 UTC (permalink / raw)
To: Ani Sinha; +Cc: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum
On Tue, 4 Jul 2023 16:55:55 +0530
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.
>
> CC: mst@redhat.com
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> ---
> hw/pci/pci.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 47517ba3db..e3ff3808b6 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1181,9 +1181,15 @@ 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.
> With SRIOV and with or without ARI
> + * the PF must be hotplugged into function 0 for it to be detected.
Wouldn't the same apply to non-SR-IOV devices as well?
> + */
> + 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),
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v7 6/6] hw/pci: add comment explaining the reason for checking function 0 in hotplug
2023-07-04 12:15 ` Igor Mammedov
@ 2023-07-04 12:31 ` Ani Sinha
0 siblings, 0 replies; 23+ messages in thread
From: Ani Sinha @ 2023-07-04 12:31 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum
[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]
On Tue, 4 Jul, 2023, 5:45 pm Igor Mammedov, <imammedo@redhat.com> wrote:
> On Tue, 4 Jul 2023 16:55:55 +0530
> 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.
> >
> > CC: mst@redhat.com
> > Signed-off-by: Ani Sinha <anisinha@redhat.com>
> > ---
> > hw/pci/pci.c | 12 +++++++++---
> > 1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 47517ba3db..e3ff3808b6 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1181,9 +1181,15 @@ 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.
>
> > With SRIOV and with or without ARI
> > + * the PF must be hotplugged into function 0 for it to be
> detected.
> Wouldn't the same apply to non-SR-IOV devices as well?
>
I was trying to emphasize PFs and SRIOV. But may be it adds more confusion
and better left out.
>
> > + */
> > + 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),
>
>
[-- Attachment #2: Type: text/html, Size: 3061 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread