* [PATCH v3 0/5] test and QEMU fixes to ensure proper PCIE device usage @ 2023-06-26 12:43 Ani Sinha 2023-06-26 12:43 ` [PATCH v3 1/5] tests/acpi: allow changes in DSDT.noacpihp table blob Ani Sinha ` (4 more replies) 0 siblings, 5 replies; 9+ messages in thread From: Ani Sinha @ 2023-06-26 12:43 UTC (permalink / raw) To: qemu-devel Cc: Ani Sinha, mst, imammedo, jusual, thuth, lvivier, michael.labiuk Patches 1-4: Fix tests so that devices do not use non-zero slots on the pcie root ports. PCIE ports only have one slot, so PCIE devices can only be plugged into slot 0 on a PCIE port. Patch 5: Enforce only one slot on PCIE port. The test fixes must be applied before the QEMU change that checks for use of a single slot in PCIE port. CC: mst@redhat.com CC: imammedo@redhat.com CC: jusual@redhat.com CC: thuth@redhat.com CC: lvivier@redhat.com CC: michael.labiuk@virtuozzo.com Changelog: v3: tags added. reword the error description in patch 5. Reword commit log in patch 4. v2: add hd-geo-test fix as well as the actual QEMU code fix to the patchset. The patches are added in the right order. Ani Sinha (5): tests/acpi: allow changes in DSDT.noacpihp table blob tests/acpi/bios-tables-test: use the correct slot on the pcie-root-port tests/acpi/bios-tables-test: update acpi blob q35/DSDT.noacpihp tests/qtest/hd-geo-test: fix test by removing unnecessary pcie-root-port hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port hw/pci/pci.c | 6 ++++++ tests/data/acpi/q35/DSDT.noacpihp | Bin 8248 -> 8241 bytes tests/qtest/bios-tables-test.c | 4 ++-- tests/qtest/hd-geo-test.c | 18 ++++++++---------- 4 files changed, 16 insertions(+), 12 deletions(-) -- 2.39.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 1/5] tests/acpi: allow changes in DSDT.noacpihp table blob 2023-06-26 12:43 [PATCH v3 0/5] test and QEMU fixes to ensure proper PCIE device usage Ani Sinha @ 2023-06-26 12:43 ` Ani Sinha 2023-06-26 12:43 ` [PATCH v3 2/5] tests/acpi/bios-tables-test: use the correct slot on the pcie-root-port Ani Sinha ` (3 subsequent siblings) 4 siblings, 0 replies; 9+ messages in thread From: Ani Sinha @ 2023-06-26 12:43 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] 9+ messages in thread
* [PATCH v3 2/5] tests/acpi/bios-tables-test: use the correct slot on the pcie-root-port 2023-06-26 12:43 [PATCH v3 0/5] test and QEMU fixes to ensure proper PCIE device usage Ani Sinha 2023-06-26 12:43 ` [PATCH v3 1/5] tests/acpi: allow changes in DSDT.noacpihp table blob Ani Sinha @ 2023-06-26 12:43 ` Ani Sinha 2023-06-26 12:43 ` [PATCH v3 3/5] tests/acpi/bios-tables-test: update acpi blob q35/DSDT.noacpihp Ani Sinha ` (2 subsequent siblings) 4 siblings, 0 replies; 9+ messages in thread From: Ani Sinha @ 2023-06-26 12:43 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] 9+ messages in thread
* [PATCH v3 3/5] tests/acpi/bios-tables-test: update acpi blob q35/DSDT.noacpihp 2023-06-26 12:43 [PATCH v3 0/5] test and QEMU fixes to ensure proper PCIE device usage Ani Sinha 2023-06-26 12:43 ` [PATCH v3 1/5] tests/acpi: allow changes in DSDT.noacpihp table blob Ani Sinha 2023-06-26 12:43 ` [PATCH v3 2/5] tests/acpi/bios-tables-test: use the correct slot on the pcie-root-port Ani Sinha @ 2023-06-26 12:43 ` Ani Sinha 2023-06-26 12:43 ` [PATCH v3 4/5] tests/qtest/hd-geo-test: fix test by removing unnecessary pcie-root-port Ani Sinha 2023-06-26 12:43 ` [PATCH v3 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port Ani Sinha 4 siblings, 0 replies; 9+ messages in thread From: Ani Sinha @ 2023-06-26 12:43 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] 9+ messages in thread
* [PATCH v3 4/5] tests/qtest/hd-geo-test: fix test by removing unnecessary pcie-root-port 2023-06-26 12:43 [PATCH v3 0/5] test and QEMU fixes to ensure proper PCIE device usage Ani Sinha ` (2 preceding siblings ...) 2023-06-26 12:43 ` [PATCH v3 3/5] tests/acpi/bios-tables-test: update acpi blob q35/DSDT.noacpihp Ani Sinha @ 2023-06-26 12:43 ` Ani Sinha 2023-06-26 13:30 ` Igor Mammedov 2023-06-26 12:43 ` [PATCH v3 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port Ani Sinha 4 siblings, 1 reply; 9+ messages in thread From: Ani Sinha @ 2023-06-26 12:43 UTC (permalink / raw) To: qemu-devel, Thomas Huth, Laurent Vivier, Paolo Bonzini Cc: Ani Sinha, mst, imammedo, Michael Labiuk The test attaches both a SCSI controller and a pcie-to-pci bridge on the same pcie-root-port. This is incorrect since only one downstream device can be attached to a pcie-root-port. Further, 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 in turn can be directly attached to the root bus (peie.0). Fix the test and simplify it. CC: mst@redhat.com CC: imammedo@redhat.com CC: Michael Labiuk <michael.labiuk@virtuozzo.com> Signed-off-by: Ani Sinha <anisinha@redhat.com> --- tests/qtest/hd-geo-test.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/tests/qtest/hd-geo-test.c b/tests/qtest/hd-geo-test.c index 5aa258a2b3..d08bffad91 100644 --- a/tests/qtest/hd-geo-test.c +++ b/tests/qtest/hd-geo-test.c @@ -784,14 +784,12 @@ static void test_override_scsi(void) test_override(args, "pc", expected); } -static void setup_pci_bridge(TestArgs *args, const char *id, const char *rootid) +static void setup_pci_bridge(TestArgs *args, const char *id) { - char *root, *br; - root = g_strdup_printf("-device pcie-root-port,id=%s", rootid); - br = g_strdup_printf("-device pcie-pci-bridge,bus=%s,id=%s", rootid, id); + char *br; + br = g_strdup_printf("-device pcie-pci-bridge,bus=pcie.0,id=%s", id); - args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, root); args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, br); } @@ -811,8 +809,8 @@ static void test_override_scsi_q35(void) add_drive_with_mbr(args, empty_mbr, 1); add_drive_with_mbr(args, empty_mbr, 1); add_drive_with_mbr(args, empty_mbr, 1); - setup_pci_bridge(args, "pcie.0", "br"); - add_scsi_controller(args, "lsi53c895a", "br", 3); + setup_pci_bridge(args, "pcie-pci-br"); + add_scsi_controller(args, "lsi53c895a", "pcie-pci-br", 3); add_scsi_disk(args, 0, 0, 0, 0, 0, 10000, 120, 30); add_scsi_disk(args, 1, 0, 0, 1, 0, 9000, 120, 30); add_scsi_disk(args, 2, 0, 0, 2, 0, 1, 0, 0); @@ -868,9 +866,9 @@ static void test_override_virtio_blk_q35(void) }; add_drive_with_mbr(args, empty_mbr, 1); add_drive_with_mbr(args, empty_mbr, 1); - setup_pci_bridge(args, "pcie.0", "br"); - add_virtio_disk(args, 0, "br", 3, 10000, 120, 30); - add_virtio_disk(args, 1, "br", 4, 9000, 120, 30); + setup_pci_bridge(args, "pcie-pci-br"); + add_virtio_disk(args, 0, "pcie-pci-br", 3, 10000, 120, 30); + add_virtio_disk(args, 1, "pcie-pci-br", 4, 9000, 120, 30); test_override(args, "q35", expected); } -- 2.39.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 4/5] tests/qtest/hd-geo-test: fix test by removing unnecessary pcie-root-port 2023-06-26 12:43 ` [PATCH v3 4/5] tests/qtest/hd-geo-test: fix test by removing unnecessary pcie-root-port Ani Sinha @ 2023-06-26 13:30 ` Igor Mammedov 2023-06-26 13:53 ` Michael S. Tsirkin 0 siblings, 1 reply; 9+ messages in thread From: Igor Mammedov @ 2023-06-26 13:30 UTC (permalink / raw) To: Ani Sinha Cc: qemu-devel, Thomas Huth, Laurent Vivier, Paolo Bonzini, mst, Michael Labiuk On Mon, 26 Jun 2023 18:13:05 +0530 Ani Sinha <anisinha@redhat.com> wrote: > The test attaches both a SCSI controller and a pcie-to-pci bridge on the same > pcie-root-port. at slot addresses ... > This is incorrect since only one downstream device can be > attached to a pcie-root-port. not true in case of multifunction perhaps s/only one downstream device/a downstream device/ s/can be attached to/can be attached to slot 0/ also point out mess with pcie.0 bus name used for as id for bridge. > Further, 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 in turn can be directly attached to the > root bus (peie.0). Fix the test and simplify it. > > CC: mst@redhat.com > CC: imammedo@redhat.com > CC: Michael Labiuk <michael.labiuk@virtuozzo.com> > > Signed-off-by: Ani Sinha <anisinha@redhat.com> > --- > tests/qtest/hd-geo-test.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) > > diff --git a/tests/qtest/hd-geo-test.c b/tests/qtest/hd-geo-test.c > index 5aa258a2b3..d08bffad91 100644 > --- a/tests/qtest/hd-geo-test.c > +++ b/tests/qtest/hd-geo-test.c > @@ -784,14 +784,12 @@ static void test_override_scsi(void) > test_override(args, "pc", expected); > } > > -static void setup_pci_bridge(TestArgs *args, const char *id, const char *rootid) > +static void setup_pci_bridge(TestArgs *args, const char *id) > { > > - char *root, *br; > - root = g_strdup_printf("-device pcie-root-port,id=%s", rootid); > - br = g_strdup_printf("-device pcie-pci-bridge,bus=%s,id=%s", rootid, id); > + char *br; > + br = g_strdup_printf("-device pcie-pci-bridge,bus=pcie.0,id=%s", id); > > - args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, root); > args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, br); > } > > @@ -811,8 +809,8 @@ static void test_override_scsi_q35(void) > add_drive_with_mbr(args, empty_mbr, 1); > add_drive_with_mbr(args, empty_mbr, 1); > add_drive_with_mbr(args, empty_mbr, 1); > - setup_pci_bridge(args, "pcie.0", "br"); > - add_scsi_controller(args, "lsi53c895a", "br", 3); > + setup_pci_bridge(args, "pcie-pci-br"); > + add_scsi_controller(args, "lsi53c895a", "pcie-pci-br", 3); > add_scsi_disk(args, 0, 0, 0, 0, 0, 10000, 120, 30); > add_scsi_disk(args, 1, 0, 0, 1, 0, 9000, 120, 30); > add_scsi_disk(args, 2, 0, 0, 2, 0, 1, 0, 0); > @@ -868,9 +866,9 @@ static void test_override_virtio_blk_q35(void) > }; > add_drive_with_mbr(args, empty_mbr, 1); > add_drive_with_mbr(args, empty_mbr, 1); > - setup_pci_bridge(args, "pcie.0", "br"); > - add_virtio_disk(args, 0, "br", 3, 10000, 120, 30); > - add_virtio_disk(args, 1, "br", 4, 9000, 120, 30); > + setup_pci_bridge(args, "pcie-pci-br"); > + add_virtio_disk(args, 0, "pcie-pci-br", 3, 10000, 120, 30); > + add_virtio_disk(args, 1, "pcie-pci-br", 4, 9000, 120, 30); > test_override(args, "q35", expected); > } > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 4/5] tests/qtest/hd-geo-test: fix test by removing unnecessary pcie-root-port 2023-06-26 13:30 ` Igor Mammedov @ 2023-06-26 13:53 ` Michael S. Tsirkin 2023-06-26 14:09 ` Igor Mammedov 0 siblings, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2023-06-26 13:53 UTC (permalink / raw) To: Igor Mammedov Cc: Ani Sinha, qemu-devel, Thomas Huth, Laurent Vivier, Paolo Bonzini, Michael Labiuk On Mon, Jun 26, 2023 at 03:30:14PM +0200, Igor Mammedov wrote: > On Mon, 26 Jun 2023 18:13:05 +0530 > Ani Sinha <anisinha@redhat.com> wrote: > > > The test attaches both a SCSI controller and a pcie-to-pci bridge on the same > > pcie-root-port. > > at slot addresses ... > > This is incorrect since only one downstream device can be > > attached to a pcie-root-port. > not true in case of multifunction > perhaps > s/only one downstream device/a downstream device/ > s/can be attached to/can be attached to slot 0/ The limitation is that devices can only be attached to slot 0. BTW once we have ARI all these checks will have to be rewritten. > also point out mess with pcie.0 bus name used for as id for bridge. > > > Further, 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 in turn can be directly attached to the > > root bus (peie.0). Fix the test and simplify it. > > > > CC: mst@redhat.com > > CC: imammedo@redhat.com > > CC: Michael Labiuk <michael.labiuk@virtuozzo.com> > > > > Signed-off-by: Ani Sinha <anisinha@redhat.com> > > --- > > tests/qtest/hd-geo-test.c | 18 ++++++++---------- > > 1 file changed, 8 insertions(+), 10 deletions(-) > > > > diff --git a/tests/qtest/hd-geo-test.c b/tests/qtest/hd-geo-test.c > > index 5aa258a2b3..d08bffad91 100644 > > --- a/tests/qtest/hd-geo-test.c > > +++ b/tests/qtest/hd-geo-test.c > > @@ -784,14 +784,12 @@ static void test_override_scsi(void) > > test_override(args, "pc", expected); > > } > > > > -static void setup_pci_bridge(TestArgs *args, const char *id, const char *rootid) > > +static void setup_pci_bridge(TestArgs *args, const char *id) > > { > > > > - char *root, *br; > > - root = g_strdup_printf("-device pcie-root-port,id=%s", rootid); > > - br = g_strdup_printf("-device pcie-pci-bridge,bus=%s,id=%s", rootid, id); > > + char *br; > > + br = g_strdup_printf("-device pcie-pci-bridge,bus=pcie.0,id=%s", id); > > > > - args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, root); > > args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, br); > > } > > > > @@ -811,8 +809,8 @@ static void test_override_scsi_q35(void) > > add_drive_with_mbr(args, empty_mbr, 1); > > add_drive_with_mbr(args, empty_mbr, 1); > > add_drive_with_mbr(args, empty_mbr, 1); > > - setup_pci_bridge(args, "pcie.0", "br"); > > - add_scsi_controller(args, "lsi53c895a", "br", 3); > > + setup_pci_bridge(args, "pcie-pci-br"); > > + add_scsi_controller(args, "lsi53c895a", "pcie-pci-br", 3); > > add_scsi_disk(args, 0, 0, 0, 0, 0, 10000, 120, 30); > > add_scsi_disk(args, 1, 0, 0, 1, 0, 9000, 120, 30); > > add_scsi_disk(args, 2, 0, 0, 2, 0, 1, 0, 0); > > @@ -868,9 +866,9 @@ static void test_override_virtio_blk_q35(void) > > }; > > add_drive_with_mbr(args, empty_mbr, 1); > > add_drive_with_mbr(args, empty_mbr, 1); > > - setup_pci_bridge(args, "pcie.0", "br"); > > - add_virtio_disk(args, 0, "br", 3, 10000, 120, 30); > > - add_virtio_disk(args, 1, "br", 4, 9000, 120, 30); > > + setup_pci_bridge(args, "pcie-pci-br"); > > + add_virtio_disk(args, 0, "pcie-pci-br", 3, 10000, 120, 30); > > + add_virtio_disk(args, 1, "pcie-pci-br", 4, 9000, 120, 30); > > test_override(args, "q35", expected); > > } > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 4/5] tests/qtest/hd-geo-test: fix test by removing unnecessary pcie-root-port 2023-06-26 13:53 ` Michael S. Tsirkin @ 2023-06-26 14:09 ` Igor Mammedov 0 siblings, 0 replies; 9+ messages in thread From: Igor Mammedov @ 2023-06-26 14:09 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Ani Sinha, qemu-devel, Thomas Huth, Laurent Vivier, Paolo Bonzini, Michael Labiuk On Mon, 26 Jun 2023 09:53:40 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Mon, Jun 26, 2023 at 03:30:14PM +0200, Igor Mammedov wrote: > > On Mon, 26 Jun 2023 18:13:05 +0530 > > Ani Sinha <anisinha@redhat.com> wrote: > > > > > The test attaches both a SCSI controller and a pcie-to-pci bridge on the same > > > pcie-root-port. > > > > at slot addresses ... > > > This is incorrect since only one downstream device can be > > > attached to a pcie-root-port. > > not true in case of multifunction > > perhaps > > s/only one downstream device/a downstream device/ > > s/can be attached to/can be attached to slot 0/ > > The limitation is that devices can only be attached to slot 0. > > BTW once we have ARI all these checks will have to be rewritten. I'm under impression that we already support ARI (pcie_ari_init) > > > > > also point out mess with pcie.0 bus name used for as id for bridge. > > > > > Further, 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 in turn can be directly attached to the > > > root bus (peie.0). Fix the test and simplify it. > > > > > > CC: mst@redhat.com > > > CC: imammedo@redhat.com > > > CC: Michael Labiuk <michael.labiuk@virtuozzo.com> > > > > > > Signed-off-by: Ani Sinha <anisinha@redhat.com> > > > --- > > > tests/qtest/hd-geo-test.c | 18 ++++++++---------- > > > 1 file changed, 8 insertions(+), 10 deletions(-) > > > > > > diff --git a/tests/qtest/hd-geo-test.c b/tests/qtest/hd-geo-test.c > > > index 5aa258a2b3..d08bffad91 100644 > > > --- a/tests/qtest/hd-geo-test.c > > > +++ b/tests/qtest/hd-geo-test.c > > > @@ -784,14 +784,12 @@ static void test_override_scsi(void) > > > test_override(args, "pc", expected); > > > } > > > > > > -static void setup_pci_bridge(TestArgs *args, const char *id, const char *rootid) > > > +static void setup_pci_bridge(TestArgs *args, const char *id) > > > { > > > > > > - char *root, *br; > > > - root = g_strdup_printf("-device pcie-root-port,id=%s", rootid); > > > - br = g_strdup_printf("-device pcie-pci-bridge,bus=%s,id=%s", rootid, id); > > > + char *br; > > > + br = g_strdup_printf("-device pcie-pci-bridge,bus=pcie.0,id=%s", id); > > > > > > - args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, root); > > > args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, br); > > > } > > > > > > @@ -811,8 +809,8 @@ static void test_override_scsi_q35(void) > > > add_drive_with_mbr(args, empty_mbr, 1); > > > add_drive_with_mbr(args, empty_mbr, 1); > > > add_drive_with_mbr(args, empty_mbr, 1); > > > - setup_pci_bridge(args, "pcie.0", "br"); > > > - add_scsi_controller(args, "lsi53c895a", "br", 3); > > > + setup_pci_bridge(args, "pcie-pci-br"); > > > + add_scsi_controller(args, "lsi53c895a", "pcie-pci-br", 3); > > > add_scsi_disk(args, 0, 0, 0, 0, 0, 10000, 120, 30); > > > add_scsi_disk(args, 1, 0, 0, 1, 0, 9000, 120, 30); > > > add_scsi_disk(args, 2, 0, 0, 2, 0, 1, 0, 0); > > > @@ -868,9 +866,9 @@ static void test_override_virtio_blk_q35(void) > > > }; > > > add_drive_with_mbr(args, empty_mbr, 1); > > > add_drive_with_mbr(args, empty_mbr, 1); > > > - setup_pci_bridge(args, "pcie.0", "br"); > > > - add_virtio_disk(args, 0, "br", 3, 10000, 120, 30); > > > - add_virtio_disk(args, 1, "br", 4, 9000, 120, 30); > > > + setup_pci_bridge(args, "pcie-pci-br"); > > > + add_virtio_disk(args, 0, "pcie-pci-br", 3, 10000, 120, 30); > > > + add_virtio_disk(args, 1, "pcie-pci-br", 4, 9000, 120, 30); > > > test_override(args, "q35", expected); > > > } > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port 2023-06-26 12:43 [PATCH v3 0/5] test and QEMU fixes to ensure proper PCIE device usage Ani Sinha ` (3 preceding siblings ...) 2023-06-26 12:43 ` [PATCH v3 4/5] tests/qtest/hd-geo-test: fix test by removing unnecessary pcie-root-port Ani Sinha @ 2023-06-26 12:43 ` Ani Sinha 4 siblings, 0 replies; 9+ messages in thread From: Ani Sinha @ 2023-06-26 12:43 UTC (permalink / raw) To: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum Cc: Ani Sinha, jusual, imammedo PCI Express ports only have one slot, so PCI Express devices can only be plugged into slot 0 on a PCIE port. Enforce it. CC: jusual@redhat.com CC: imammedo@redhat.com Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929 Signed-off-by: Ani Sinha <anisinha@redhat.com> Reviewed-by: Julia Suvorova <jusual@redhat.com> --- hw/pci/pci.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index bf38905b7d..426af133b0 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -64,6 +64,7 @@ bool pci_available = true; static char *pcibus_get_dev_path(DeviceState *dev); static char *pcibus_get_fw_dev_path(DeviceState *dev); static void pcibus_reset(BusState *qbus); +static bool pcie_has_upstream_port(PCIDevice *dev); static Property pci_props[] = { DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), @@ -1189,6 +1190,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, name); return NULL; + } else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) { + error_setg(errp, "PCI: slot %d is not valid for %s," + " parent device only allows plugging into slot 0.", + PCI_SLOT(devfn), name); + return NULL; } pci_dev->devfn = devfn; -- 2.39.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-06-26 14:09 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-06-26 12:43 [PATCH v3 0/5] test and QEMU fixes to ensure proper PCIE device usage Ani Sinha 2023-06-26 12:43 ` [PATCH v3 1/5] tests/acpi: allow changes in DSDT.noacpihp table blob Ani Sinha 2023-06-26 12:43 ` [PATCH v3 2/5] tests/acpi/bios-tables-test: use the correct slot on the pcie-root-port Ani Sinha 2023-06-26 12:43 ` [PATCH v3 3/5] tests/acpi/bios-tables-test: update acpi blob q35/DSDT.noacpihp Ani Sinha 2023-06-26 12:43 ` [PATCH v3 4/5] tests/qtest/hd-geo-test: fix test by removing unnecessary pcie-root-port Ani Sinha 2023-06-26 13:30 ` Igor Mammedov 2023-06-26 13:53 ` Michael S. Tsirkin 2023-06-26 14:09 ` Igor Mammedov 2023-06-26 12:43 ` [PATCH v3 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port Ani Sinha
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).