From 42a62c20925e02aef0d849f92a0e9540888e79ae Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Tue, 4 Aug 2020 10:40:23 -0400 Subject: [PATCH 01/13] acpi: allow DSDT changes We are updating all DSDTs with UID 0 for PCI Root. Allow changes. Signed-off-by: Michael S. Tsirkin --- tests/qtest/bios-tables-test-allowed-diff.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index dfb8523c8b..ea46c3399e 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1 +1,22 @@ /* List of comma-separated changed AML files to ignore */ +"tests/data/acpi/q35/DSDT.mmio64", +"tests/data/acpi/q35/DSDT.memhp", +"tests/data/acpi/q35/DSDT.tis", +"tests/data/acpi/q35/DSDT.acpihmat", +"tests/data/acpi/q35/DSDT.bridge", +"tests/data/acpi/q35/DSDT.ipmibt", +"tests/data/acpi/q35/DSDT.numamem", +"tests/data/acpi/q35/DSDT.dimmpxm", +"tests/data/acpi/q35/DSDT.cphp", +"tests/data/acpi/q35/DSDT", +"tests/data/acpi/pc/DSDT.memhp", +"tests/data/acpi/pc/DSDT.acpihmat", +"tests/data/acpi/pc/DSDT.bridge", +"tests/data/acpi/pc/DSDT.numamem", +"tests/data/acpi/pc/DSDT.ipmikcs", +"tests/data/acpi/pc/DSDT.dimmpxm", +"tests/data/acpi/pc/DSDT.cphp", +"tests/data/acpi/pc/DSDT", +"tests/data/acpi/virt/DSDT.memhp", +"tests/data/acpi/virt/DSDT.numamem", +"tests/data/acpi/virt/DSDT", From af1b80ae56c9495999e8ccf7b70ef894378de642 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 30 Jul 2020 11:44:06 -0400 Subject: [PATCH 02/13] i386/acpi: fix inconsistent QEMU/OVMF device paths macOS uses ACPI UIDs to build the DevicePath for NVRAM boot options, while OVMF firmware gets them via an internal channel through QEMU. Due to a bug in QEMU ACPI currently UEFI firmware and ACPI have different values, and this makes the underlying operating system unable to report its boot option. The particular node in question is the primary PciRoot (PCI0 in ACPI), which for some reason gets assigned 1 in ACPI UID and 0 in the DevicePath. This is due to the _UID assigned to it by build_dsdt in hw/i386/acpi-build.c Which does not correspond to the primary PCI identifier given by pcibus_num in hw/pci/pci.c Reference with the device paths, OVMF startup logs, and ACPI table dumps (SysReport): https://github.com/acidanthera/bugtracker/issues/1050 In UEFI v2.8, section "10.4.2 Rules with ACPI _HID and _UID" ends with the paragraph, Root PCI bridges will use the plug and play ID of PNP0A03, This will be stored in the ACPI Device Path _HID field, or in the Expanded ACPI Device Path _CID field to match the ACPI name space. The _UID in the ACPI Device Path structure must match the _UID in the ACPI name space. (See especially the last sentence.) Considering *extra* root bridges / root buses (with bus number > 0), QEMU's ACPI generator actually does the right thing; since QEMU commit c96d9286a6d7 ("i386/acpi-build: more traditional _UID and _HID for PXB root buses", 2015-06-11). However, the _UID values for root bridge zero (on both i440fx and q35) have always been "wrong" (from UEFI perspective), going back in QEMU to commit 74523b850189 ("i386: add ACPI table files from seabios", 2013-10-14). Even in SeaBIOS, these _UID values have always been 1; see commit a4d357638c57 ("Port rombios32 code from bochs-bios.", 2008-03-08) for i440fx, and commit ecbe3fd61511 ("seabios: q35: add dsdt", 2012-12-01) for q35. Cc: qemu-stable@nongnu.org Suggested-by: Laszlo Ersek Tested-by: Vitaly Cheptsov Signed-off-by: Michael S. Tsirkin Reviewed-by: Laszlo Ersek --- hw/i386/acpi-build.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index b7bcbbbb2a..7a5a8b3521 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -1497,7 +1497,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, dev = aml_device("PCI0"); aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03"))); aml_append(dev, aml_name_decl("_ADR", aml_int(0))); - aml_append(dev, aml_name_decl("_UID", aml_int(1))); + aml_append(dev, aml_name_decl("_UID", aml_int(0))); aml_append(sb_scope, dev); aml_append(dsdt, sb_scope); @@ -1512,7 +1512,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08"))); aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03"))); aml_append(dev, aml_name_decl("_ADR", aml_int(0))); - aml_append(dev, aml_name_decl("_UID", aml_int(1))); + aml_append(dev, aml_name_decl("_UID", aml_int(0))); aml_append(dev, build_q35_osc_method()); aml_append(sb_scope, dev); aml_append(dsdt, sb_scope); From 9b897b399eb1f6b71add88ed0bfaeabf63609115 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 30 Jul 2020 11:44:06 -0400 Subject: [PATCH 03/13] arm/acpi: fix an out of spec _UID for PCI root On ARM/virt machine type QEMU currently reports an incorrect _UID in ACPI. The particular node in question is the primary PciRoot (PCI0 in ACPI), which gets assigned PCI0 in ACPI UID and 0 in the DevicePath. This is due to the _UID assigned to it by build_dsdt in hw/arm/virt-acpi-build.c Which does not correspond to the primary PCI identifier given by pcibus_num in hw/pci/pci.c In UEFI v2.8, section "10.4.2 Rules with ACPI _HID and _UID" ends with the paragraph, Root PCI bridges will use the plug and play ID of PNP0A03, This will be stored in the ACPI Device Path _HID field, or in the Expanded ACPI Device Path _CID field to match the ACPI name space. The _UID in the ACPI Device Path structure must match the _UID in the ACPI name space. (See especially the last sentence.) A similar bug has been reported on i386, on that architecture it has been reported to confuse at least macOS which uses ACPI UIDs to build the DevicePath for NVRAM boot options, while OVMF firmware gets them via an internal channel through QEMU. When UEFI firmware and ACPI have different values, this makes the underlying operating system unable to report its boot option. Cc: qemu-stable@nongnu.org Reported-by: Vitaly Cheptsov Signed-off-by: Michael S. Tsirkin Reviewed-by: Laszlo Ersek --- hw/arm/virt-acpi-build.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index 91f0df7b13..0a482ff6f7 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -170,7 +170,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, aml_append(dev, aml_name_decl("_CID", aml_string("PNP0A03"))); aml_append(dev, aml_name_decl("_SEG", aml_int(0))); aml_append(dev, aml_name_decl("_BBN", aml_int(0))); - aml_append(dev, aml_name_decl("_UID", aml_string("PCI0"))); + aml_append(dev, aml_name_decl("_UID", aml_int(0))); aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0 Device"))); aml_append(dev, aml_name_decl("_CCA", aml_int(1))); From c27c1cc3ca6b6177520dff066c0fb6bb6a0297d4 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Tue, 4 Aug 2020 11:21:05 -0400 Subject: [PATCH 04/13] disassemble-aml: -o actually works Turns out that option was borken due to weird iasl command line handling. Fix it. Signed-off-by: Michael S. Tsirkin --- tests/data/acpi/disassemle-aml.sh | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tests/data/acpi/disassemle-aml.sh b/tests/data/acpi/disassemle-aml.sh index 1d8a4d0301..253b7620a0 100755 --- a/tests/data/acpi/disassemle-aml.sh +++ b/tests/data/acpi/disassemle-aml.sh @@ -42,11 +42,16 @@ do else extra="" fi - asl=${aml}.dsl if [[ "${outdir}" ]]; then - asl="${outdir}"/${machine}/${asl} + # iasl strips an extension from prefix if there. + # since we have some files with . in the name, the + # last component gets interpreted as an extension: + # add another extension to work around that. + prefix="-p ${outdir}/${aml}.dsl" + else + prefix="" fi - iasl -d -p ${asl} ${extra} ${aml} + iasl ${extra} ${prefix} -d ${aml} done done From af1dfe1ec0864e6700237a43cc36018176f9eba9 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Tue, 4 Aug 2020 17:43:44 -0400 Subject: [PATCH 05/13] acpi: update expected DSDT files with _UID changes _UID of the PCI root has been changed to 0. Update expected files accordingly, and re-enable their testing. Full diff of changed files disassembly: diff -ru /tmp/old/tests/data/acpi/pc/DSDT.acpihmat.dsl /tmp/new/tests/data/acpi/pc/DSDT.acpihmat.dsl --- /tmp/old/tests/data/acpi/pc/DSDT.acpihmat.dsl 2020-08-04 17:37:55.727798633 -0400 +++ /tmp/new/tests/data/acpi/pc/DSDT.acpihmat.dsl 2020-08-04 17:42:57.258859861 -0400 @@ -50,7 +50,7 @@ { Name (_HID, EisaId ("PNP0A03") /* PCI Bus */) // _HID: Hardware ID Name (_ADR, Zero) // _ADR: Address - Name (_UID, One) // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID } } diff -ru /tmp/old/tests/data/acpi/pc/DSDT.bridge.dsl /tmp/new/tests/data/acpi/pc/DSDT.bridge.dsl --- /tmp/old/tests/data/acpi/pc/DSDT.bridge.dsl 2020-08-04 17:37:55.737798601 -0400 +++ /tmp/new/tests/data/acpi/pc/DSDT.bridge.dsl 2020-08-04 17:42:57.262859849 -0400 @@ -50,7 +50,7 @@ { Name (_HID, EisaId ("PNP0A03") /* PCI Bus */) // _HID: Hardware ID Name (_ADR, Zero) // _ADR: Address - Name (_UID, One) // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID } } diff -ru /tmp/old/tests/data/acpi/pc/DSDT.cphp.dsl /tmp/new/tests/data/acpi/pc/DSDT.cphp.dsl --- /tmp/old/tests/data/acpi/pc/DSDT.cphp.dsl 2020-08-04 17:37:55.745798576 -0400 +++ /tmp/new/tests/data/acpi/pc/DSDT.cphp.dsl 2020-08-04 17:42:57.265859839 -0400 @@ -50,7 +50,7 @@ { Name (_HID, EisaId ("PNP0A03") /* PCI Bus */) // _HID: Hardware ID Name (_ADR, Zero) // _ADR: Address - Name (_UID, One) // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID } } diff -ru /tmp/old/tests/data/acpi/pc/DSDT.dimmpxm.dsl /tmp/new/tests/data/acpi/pc/DSDT.dimmpxm.dsl --- /tmp/old/tests/data/acpi/pc/DSDT.dimmpxm.dsl 2020-08-04 17:37:55.759798533 -0400 +++ /tmp/new/tests/data/acpi/pc/DSDT.dimmpxm.dsl 2020-08-04 17:42:57.268859830 -0400 @@ -52,7 +52,7 @@ { Name (_HID, EisaId ("PNP0A03") /* PCI Bus */) // _HID: Hardware ID Name (_ADR, Zero) // _ADR: Address - Name (_UID, One) // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID } } diff -ru /tmp/old/tests/data/acpi/pc/DSDT.dsl /tmp/new/tests/data/acpi/pc/DSDT.dsl --- /tmp/old/tests/data/acpi/pc/DSDT.dsl 2020-08-04 17:37:55.713798676 -0400 +++ /tmp/new/tests/data/acpi/pc/DSDT.dsl 2020-08-04 17:42:57.256859867 -0400 @@ -50,7 +50,7 @@ { Name (_HID, EisaId ("PNP0A03") /* PCI Bus */) // _HID: Hardware ID Name (_ADR, Zero) // _ADR: Address - Name (_UID, One) // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID } } diff -ru /tmp/old/tests/data/acpi/pc/DSDT.ipmikcs.dsl /tmp/new/tests/data/acpi/pc/DSDT.ipmikcs.dsl --- /tmp/old/tests/data/acpi/pc/DSDT.ipmikcs.dsl 2020-08-04 17:37:55.765798514 -0400 +++ /tmp/new/tests/data/acpi/pc/DSDT.ipmikcs.dsl 2020-08-04 17:42:57.270859824 -0400 @@ -50,7 +50,7 @@ { Name (_HID, EisaId ("PNP0A03") /* PCI Bus */) // _HID: Hardware ID Name (_ADR, Zero) // _ADR: Address - Name (_UID, One) // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID } } diff -ru /tmp/old/tests/data/acpi/pc/DSDT.memhp.dsl /tmp/new/tests/data/acpi/pc/DSDT.memhp.dsl --- /tmp/old/tests/data/acpi/pc/DSDT.memhp.dsl 2020-08-04 17:37:55.773798489 -0400 +++ /tmp/new/tests/data/acpi/pc/DSDT.memhp.dsl 2020-08-04 17:42:57.273859814 -0400 @@ -50,7 +50,7 @@ { Name (_HID, EisaId ("PNP0A03") /* PCI Bus */) // _HID: Hardware ID Name (_ADR, Zero) // _ADR: Address - Name (_UID, One) // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID } } diff -ru /tmp/old/tests/data/acpi/pc/DSDT.numamem.dsl /tmp/new/tests/data/acpi/pc/DSDT.numamem.dsl --- /tmp/old/tests/data/acpi/pc/DSDT.numamem.dsl 2020-08-04 17:37:55.782798461 -0400 +++ /tmp/new/tests/data/acpi/pc/DSDT.numamem.dsl 2020-08-04 17:42:57.276859805 -0400 @@ -50,7 +50,7 @@ { Name (_HID, EisaId ("PNP0A03") /* PCI Bus */) // _HID: Hardware ID Name (_ADR, Zero) // _ADR: Address - Name (_UID, One) // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID } } diff -ru /tmp/old/tests/data/acpi/q35/DSDT.acpihmat.dsl /tmp/new/tests/data/acpi/q35/DSDT.acpihmat.dsl --- /tmp/old/tests/data/acpi/q35/DSDT.acpihmat.dsl 2020-08-04 17:37:55.911798060 -0400 +++ /tmp/new/tests/data/acpi/q35/DSDT.acpihmat.dsl 2020-08-04 17:42:57.327859646 -0400 @@ -51,7 +51,7 @@ Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */) // _HID: Hardware ID Name (_CID, EisaId ("PNP0A03") /* PCI Bus */) // _CID: Compatible ID Name (_ADR, Zero) // _ADR: Address - Name (_UID, One) // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID Method (_OSC, 4, NotSerialized) // _OSC: Operating System Capabilities { CreateDWordField (Arg3, Zero, CDW1) diff -ru /tmp/old/tests/data/acpi/q35/DSDT.bridge.dsl /tmp/new/tests/data/acpi/q35/DSDT.bridge.dsl --- /tmp/old/tests/data/acpi/q35/DSDT.bridge.dsl 2020-08-04 17:37:55.920798032 -0400 +++ /tmp/new/tests/data/acpi/q35/DSDT.bridge.dsl 2020-08-04 17:42:57.331859634 -0400 @@ -51,7 +51,7 @@ Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */) // _HID: Hardware ID Name (_CID, EisaId ("PNP0A03") /* PCI Bus */) // _CID: Compatible ID Name (_ADR, Zero) // _ADR: Address - Name (_UID, One) // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID Method (_OSC, 4, NotSerialized) // _OSC: Operating System Capabilities { CreateDWordField (Arg3, Zero, CDW1) diff -ru /tmp/old/tests/data/acpi/q35/DSDT.cphp.dsl /tmp/new/tests/data/acpi/q35/DSDT.cphp.dsl --- /tmp/old/tests/data/acpi/q35/DSDT.cphp.dsl 2020-08-04 17:37:55.930798001 -0400 +++ /tmp/new/tests/data/acpi/q35/DSDT.cphp.dsl 2020-08-04 17:42:57.336859618 -0400 @@ -51,7 +51,7 @@ Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */) // _HID: Hardware ID Name (_CID, EisaId ("PNP0A03") /* PCI Bus */) // _CID: Compatible ID Name (_ADR, Zero) // _ADR: Address - Name (_UID, One) // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID Method (_OSC, 4, NotSerialized) // _OSC: Operating System Capabilities { CreateDWordField (Arg3, Zero, CDW1) diff -ru /tmp/old/tests/data/acpi/q35/DSDT.dimmpxm.dsl /tmp/new/tests/data/acpi/q35/DSDT.dimmpxm.dsl --- /tmp/old/tests/data/acpi/q35/DSDT.dimmpxm.dsl 2020-08-04 17:37:55.942797963 -0400 +++ /tmp/new/tests/data/acpi/q35/DSDT.dimmpxm.dsl 2020-08-04 17:42:57.340859606 -0400 @@ -53,7 +53,7 @@ Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */) // _HID: Hardware ID Name (_CID, EisaId ("PNP0A03") /* PCI Bus */) // _CID: Compatible ID Name (_ADR, Zero) // _ADR: Address - Name (_UID, One) // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID Method (_OSC, 4, NotSerialized) // _OSC: Operating System Capabilities { CreateDWordField (Arg3, Zero, CDW1) diff -ru /tmp/old/tests/data/acpi/q35/DSDT.dsl /tmp/new/tests/data/acpi/q35/DSDT.dsl --- /tmp/old/tests/data/acpi/q35/DSDT.dsl 2020-08-04 17:37:55.898798100 -0400 +++ /tmp/new/tests/data/acpi/q35/DSDT.dsl 2020-08-04 17:42:57.323859659 -0400 @@ -51,7 +51,7 @@ Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */) // _HID: Hardware ID Name (_CID, EisaId ("PNP0A03") /* PCI Bus */) // _CID: Compatible ID Name (_ADR, Zero) // _ADR: Address - Name (_UID, One) // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID Method (_OSC, 4, NotSerialized) // _OSC: Operating System Capabilities { CreateDWordField (Arg3, Zero, CDW1) diff -ru /tmp/old/tests/data/acpi/q35/DSDT.ipmibt.dsl /tmp/new/tests/data/acpi/q35/DSDT.ipmibt.dsl --- /tmp/old/tests/data/acpi/q35/DSDT.ipmibt.dsl 2020-08-04 17:37:55.952797932 -0400 +++ /tmp/new/tests/data/acpi/q35/DSDT.ipmibt.dsl 2020-08-04 17:42:57.344859593 -0400 @@ -51,7 +51,7 @@ Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */) // _HID: Hardware ID Name (_CID, EisaId ("PNP0A03") /* PCI Bus */) // _CID: Compatible ID Name (_ADR, Zero) // _ADR: Address - Name (_UID, One) // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID Method (_OSC, 4, NotSerialized) // _OSC: Operating System Capabilities { CreateDWordField (Arg3, Zero, CDW1) diff -ru /tmp/old/tests/data/acpi/q35/DSDT.memhp.dsl /tmp/new/tests/data/acpi/q35/DSDT.memhp.dsl --- /tmp/old/tests/data/acpi/q35/DSDT.memhp.dsl 2020-08-04 17:37:55.962797901 -0400 +++ /tmp/new/tests/data/acpi/q35/DSDT.memhp.dsl 2020-08-04 17:42:57.348859581 -0400 @@ -51,7 +51,7 @@ Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */) // _HID: Hardware ID Name (_CID, EisaId ("PNP0A03") /* PCI Bus */) // _CID: Compatible ID Name (_ADR, Zero) // _ADR: Address - Name (_UID, One) // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID Method (_OSC, 4, NotSerialized) // _OSC: Operating System Capabilities { CreateDWordField (Arg3, Zero, CDW1) diff -ru /tmp/old/tests/data/acpi/q35/DSDT.mmio64.dsl /tmp/new/tests/data/acpi/q35/DSDT.mmio64.dsl --- /tmp/old/tests/data/acpi/q35/DSDT.mmio64.dsl 2020-08-04 17:37:55.972797870 -0400 +++ /tmp/new/tests/data/acpi/q35/DSDT.mmio64.dsl 2020-08-04 17:42:57.351859572 -0400 @@ -51,7 +51,7 @@ Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */) // _HID: Hardware ID Name (_CID, EisaId ("PNP0A03") /* PCI Bus */) // _CID: Compatible ID Name (_ADR, Zero) // _ADR: Address - Name (_UID, One) // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID Method (_OSC, 4, NotSerialized) // _OSC: Operating System Capabilities { CreateDWordField (Arg3, Zero, CDW1) diff -ru /tmp/old/tests/data/acpi/q35/DSDT.numamem.dsl /tmp/new/tests/data/acpi/q35/DSDT.numamem.dsl --- /tmp/old/tests/data/acpi/q35/DSDT.numamem.dsl 2020-08-04 17:37:55.983797836 -0400 +++ /tmp/new/tests/data/acpi/q35/DSDT.numamem.dsl 2020-08-04 17:42:57.354859562 -0400 @@ -51,7 +51,7 @@ Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */) // _HID: Hardware ID Name (_CID, EisaId ("PNP0A03") /* PCI Bus */) // _CID: Compatible ID Name (_ADR, Zero) // _ADR: Address - Name (_UID, One) // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID Method (_OSC, 4, NotSerialized) // _OSC: Operating System Capabilities { CreateDWordField (Arg3, Zero, CDW1) diff -ru /tmp/old/tests/data/acpi/q35/DSDT.tis.dsl /tmp/new/tests/data/acpi/q35/DSDT.tis.dsl --- /tmp/old/tests/data/acpi/q35/DSDT.tis.dsl 2020-08-04 17:37:55.993797804 -0400 +++ /tmp/new/tests/data/acpi/q35/DSDT.tis.dsl 2020-08-04 17:42:57.358859550 -0400 @@ -51,7 +51,7 @@ Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */) // _HID: Hardware ID Name (_CID, EisaId ("PNP0A03") /* PCI Bus */) // _CID: Compatible ID Name (_ADR, Zero) // _ADR: Address - Name (_UID, One) // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID Method (_OSC, 4, NotSerialized) // _OSC: Operating System Capabilities { CreateDWordField (Arg3, Zero, CDW1) diff -ru /tmp/old/tests/data/acpi/virt/DSDT.dsl /tmp/new/tests/data/acpi/virt/DSDT.dsl --- /tmp/old/tests/data/acpi/virt/DSDT.dsl 2020-08-04 17:37:56.121797406 -0400 +++ /tmp/new/tests/data/acpi/virt/DSDT.dsl 2020-08-04 17:42:57.408859394 -0400 @@ -641,7 +641,7 @@ Name (_CID, "PNP0A03" /* PCI Bus */) // _CID: Compatible ID Name (_SEG, Zero) // _SEG: PCI Segment Name (_BBN, Zero) // _BBN: BIOS Bus Number - Name (_UID, "PCI0") // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID Name (_STR, Unicode ("PCIe 0 Device")) // _STR: Description String Name (_CCA, One) // _CCA: Cache Coherency Attribute Name (_PRT, Package (0x80) // _PRT: PCI Routing Table diff -ru /tmp/old/tests/data/acpi/virt/DSDT.memhp.dsl /tmp/new/tests/data/acpi/virt/DSDT.memhp.dsl --- /tmp/old/tests/data/acpi/virt/DSDT.memhp.dsl 2020-08-04 17:37:56.129797381 -0400 +++ /tmp/new/tests/data/acpi/virt/DSDT.memhp.dsl 2020-08-04 17:42:57.411859385 -0400 @@ -643,7 +643,7 @@ Name (_CID, "PNP0A03" /* PCI Bus */) // _CID: Compatible ID Name (_SEG, Zero) // _SEG: PCI Segment Name (_BBN, Zero) // _BBN: BIOS Bus Number - Name (_UID, "PCI0") // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID Name (_STR, Unicode ("PCIe 0 Device")) // _STR: Description String Name (_CCA, One) // _CCA: Cache Coherency Attribute Name (_PRT, Package (0x80) // _PRT: PCI Routing Table diff -ru /tmp/old/tests/data/acpi/virt/DSDT.numamem.dsl /tmp/new/tests/data/acpi/virt/DSDT.numamem.dsl --- /tmp/old/tests/data/acpi/virt/DSDT.numamem.dsl 2020-08-04 17:37:56.141797343 -0400 +++ /tmp/new/tests/data/acpi/virt/DSDT.numamem.dsl 2020-08-04 17:42:57.413859379 -0400 @@ -641,7 +641,7 @@ Name (_CID, "PNP0A03" /* PCI Bus */) // _CID: Compatible ID Name (_SEG, Zero) // _SEG: PCI Segment Name (_BBN, Zero) // _BBN: BIOS Bus Number - Name (_UID, "PCI0") // _UID: Unique ID + Name (_UID, Zero) // _UID: Unique ID Name (_STR, Unicode ("PCIe 0 Device")) // _STR: Description String Name (_CCA, One) // _CCA: Cache Coherency Attribute Name (_PRT, Package (0x80) // _PRT: PCI Routing Table Signed-off-by: Michael S. Tsirkin --- tests/data/acpi/pc/DSDT | Bin 4934 -> 4934 bytes tests/data/acpi/pc/DSDT.acpihmat | Bin 6258 -> 6258 bytes tests/data/acpi/pc/DSDT.bridge | Bin 6793 -> 6793 bytes tests/data/acpi/pc/DSDT.cphp | Bin 5397 -> 5397 bytes tests/data/acpi/pc/DSDT.dimmpxm | Bin 6587 -> 6587 bytes tests/data/acpi/pc/DSDT.ipmikcs | Bin 5006 -> 5006 bytes tests/data/acpi/pc/DSDT.memhp | Bin 6293 -> 6293 bytes tests/data/acpi/pc/DSDT.numamem | Bin 4940 -> 4940 bytes tests/data/acpi/q35/DSDT | Bin 7678 -> 7678 bytes tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9002 bytes tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 7695 bytes tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8141 bytes tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9331 bytes tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 7753 bytes tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9037 bytes tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 8808 bytes tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7684 bytes tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8283 bytes tests/data/acpi/virt/DSDT | Bin 5205 -> 5200 bytes tests/data/acpi/virt/DSDT.memhp | Bin 6566 -> 6561 bytes tests/data/acpi/virt/DSDT.numamem | Bin 5205 -> 5200 bytes tests/qtest/bios-tables-test-allowed-diff.h | 21 -------------------- 22 files changed, 21 deletions(-) diff --git a/tests/data/acpi/pc/DSDT b/tests/data/acpi/pc/DSDT index 6d0aaf729ac7d64cf966621adf276534de5cc555..b121bb5bc124be522e233516efb17cdc94de5a75 100644 GIT binary patch delta 24 gcmX@6c1(@SCDZZv2P+*8zaNUi4VmAWZZv3DX@8zbY!i4VmAW;+M- diff --git a/tests/data/acpi/pc/DSDT.bridge b/tests/data/acpi/pc/DSDT.bridge index 623c4c03585c47d4d28adc611823b7cce8f4a5c7..7b6c7a47875fc73b03fbe88807890f3867ddba1a 100644 GIT binary patch delta 24 fcmeA)?KI_b33dtTlwx3DlX7;txpxP_YKs delta 24 fcmeA)?KI_b33dtTlwx3DlvF;txpxP^<>n diff --git a/tests/data/acpi/pc/DSDT.cphp b/tests/data/acpi/pc/DSDT.cphp index e0a43ccdadae150c0f39599c85e4e21ed8fff2a4..c0e8aa5b32d84f39e5d6c9a5024505f818707c12 100644 GIT binary patch delta 24 fcmbQLHC2ntCDlX7;$I;EQV9nQ delta 24 fcmeBE?^EY;33dtT6J}sw44%l<#>lvF;$I;EQUnJL diff --git a/tests/data/acpi/pc/DSDT.memhp b/tests/data/acpi/pc/DSDT.memhp index 9a9418f4bde5fb18883c244ea956122e371ff01a..4026772906e910af514beb76de6e4cca0bc2171b 100644 GIT binary patch delta 24 gcmbPgIMtBLCDXC$dNY09SGbrT_o{ delta 24 ecmbPgIMtBLCD4}M8zaNUiMxdWS$hW= delta 24 fcmX@3c1DfMCD@kU8zbY!iMxdWS#}2* diff --git a/tests/data/acpi/q35/DSDT b/tests/data/acpi/q35/DSDT index e63676d7a63afec714debeb465ee478ea4714337..bba8884073a27427b88ac0d733c9c87330a59366 100644 GIT binary patch delta 24 gcmexo{m+`qCD9^r>33dtLmt$aH^q$ByiIHLB+#*>3P7elD delta 24 fcmeCT>9^r>33dtLmt$aH^qR;uiIH*R+#*>3P6`H8 diff --git a/tests/data/acpi/q35/DSDT.cphp b/tests/data/acpi/q35/DSDT.cphp index d9bb414e9bf15d9b9149f38c9bb5d8b993f88650..57d859cef9fa16a8f125c4b338611c8472699f38 100644 GIT binary patch delta 24 gcmX?Wf7YJMCD^$s{jB1 delta 24 gcmezD@!5mRCD#xs{jB1 diff --git a/tests/data/acpi/q35/DSDT.ipmibt b/tests/data/acpi/q35/DSDT.ipmibt index e8dea1ea42af765babcb747af998b0d912abdcbd..5cd11de6a8fe47324e5f922823a22746882f19f5 100644 GIT binary patch delta 24 gcmX?UbJB*(CDcGiu{CDcGiu{CD)f;Bu0jfb5AP*0A?Ns-T(jq delta 24 gcmaFi^1_A7CDDNRqX{K(cjp`Ddj82msIE^-!bKc_u0L51dc>n+a delta 42 ycmcbhaaDuMCDET2!X{H9}jp`DdjP8>iIE`3&1Drh#HWzW;;{pKve+zj4 diff --git a/tests/data/acpi/virt/DSDT.memhp b/tests/data/acpi/virt/DSDT.memhp index 4cb81f692d73526542493a0c4da9c9793cc8366e..545a18c3657781d350a006adfa5e58fa63e63922 100644 GIT binary patch delta 36 scmZ2xywI4-CD*8`UK^8Qmv4a2m1l1~_{fY%b!|7XSe2n+jC` diff --git a/tests/data/acpi/virt/DSDT.numamem b/tests/data/acpi/virt/DSDT.numamem index e669508d175f1e3ddf355f8a9b0d419266cac8aa..9b002836f35fd03afeab9e827fdde3134d26ed2e 100644 GIT binary patch delta 36 scmcbraY2L2CDDNRqX{K(cjp`Ddj82msIE^-!bKc_u0L51dc>n+a delta 42 ycmcbhaaDuMCDET2!X{H9}jp`DdjP8>iIE`3&1Drh#HWzW;;{pKve+zj4 diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index ea46c3399e..dfb8523c8b 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1,22 +1 @@ /* List of comma-separated changed AML files to ignore */ -"tests/data/acpi/q35/DSDT.mmio64", -"tests/data/acpi/q35/DSDT.memhp", -"tests/data/acpi/q35/DSDT.tis", -"tests/data/acpi/q35/DSDT.acpihmat", -"tests/data/acpi/q35/DSDT.bridge", -"tests/data/acpi/q35/DSDT.ipmibt", -"tests/data/acpi/q35/DSDT.numamem", -"tests/data/acpi/q35/DSDT.dimmpxm", -"tests/data/acpi/q35/DSDT.cphp", -"tests/data/acpi/q35/DSDT", -"tests/data/acpi/pc/DSDT.memhp", -"tests/data/acpi/pc/DSDT.acpihmat", -"tests/data/acpi/pc/DSDT.bridge", -"tests/data/acpi/pc/DSDT.numamem", -"tests/data/acpi/pc/DSDT.ipmikcs", -"tests/data/acpi/pc/DSDT.dimmpxm", -"tests/data/acpi/pc/DSDT.cphp", -"tests/data/acpi/pc/DSDT", -"tests/data/acpi/virt/DSDT.memhp", -"tests/data/acpi/virt/DSDT.numamem", -"tests/data/acpi/virt/DSDT", From 3d7e78aa7777f052d72ec6070e611767f0db35ce Mon Sep 17 00:00:00 2001 From: Ani Sinha Date: Fri, 21 Aug 2020 22:24:03 +0530 Subject: [PATCH 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus We introduce a new global flag 'acpi-root-pci-hotplug' for i440fx with which we can turn on or off PCI device hotplug on the root bus. This flag can be used to prevent all PCI devices from getting hotplugged or unplugged from the root PCI bus. This feature is targetted mostly towards Windows VMs. It is useful in cases where some hypervisor admins want to deploy guest VMs in a way so that the users of the guest OSes are not able to hot-eject certain PCI devices from the Windows system tray. Laine has explained the use case here in detail: https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html Julia has resolved this issue for PCIE buses with the following commit: 530a0963184e57e71a5b538 ("pcie_root_port: Add hotplug disabling option") This commit attempts to introduce similar behavior for PCI root buses used in i440fx machine types (although in this case, we do not have a per-slot capability to turn hotplug on or off). Usage: -global PIIX4_PM.acpi-root-pci-hotplug=off By default, this option is enabled which means that hotplug is turned on for the PCI root bus. The previously existing flag 'acpi-pci-hotplug-with-bridge-support' for PCI-PCI bridges remain as is and can be used along with this new flag to control PCI hotplug on PCI bridges. This change has been tested using a Windows 2012R2 server guest image and also with a Windows 2019 server guest image on a Ubuntu 18.04 host using the latest master qemu from upstream. Signed-off-by: Ani Sinha Message-Id: <20200821165403.26589-1-ani@anisinha.ca> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Tested-by: Igor Mammedov Reviewed-by: Igor Mammedov --- hw/acpi/pcihp.c | 23 ++++++++++++++++++++++- hw/acpi/piix4.c | 5 ++++- include/hw/acpi/pcihp.h | 2 +- 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c index 9e31ab2da4..39b1f74442 100644 --- a/hw/acpi/pcihp.c +++ b/hw/acpi/pcihp.c @@ -104,6 +104,24 @@ static void acpi_set_pci_info(void) } } +static void acpi_pcihp_disable_root_bus(void) +{ + static bool root_hp_disabled; + PCIBus *bus; + + if (root_hp_disabled) { + return; + } + + bus = find_i440fx(); + if (bus) { + /* setting the hotplug handler to NULL makes the bus non-hotpluggable */ + qbus_set_hotplug_handler(BUS(bus), NULL); + } + root_hp_disabled = true; + return; +} + static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque) { AcpiPciHpFind *find = opaque; @@ -209,8 +227,11 @@ static void acpi_pcihp_update(AcpiPciHpState *s) } } -void acpi_pcihp_reset(AcpiPciHpState *s) +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off) { + if (acpihp_root_off) { + acpi_pcihp_disable_root_bus(); + } acpi_set_pci_info(); acpi_pcihp_update(s); } diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c index 26bac4f16c..e6163bb6ce 100644 --- a/hw/acpi/piix4.c +++ b/hw/acpi/piix4.c @@ -78,6 +78,7 @@ typedef struct PIIX4PMState { AcpiPciHpState acpi_pci_hotplug; bool use_acpi_hotplug_bridge; + bool use_acpi_root_pci_hotplug; uint8_t disable_s3; uint8_t disable_s4; @@ -324,7 +325,7 @@ static void piix4_pm_reset(DeviceState *dev) pci_conf[0x5B] = 0x02; } pm_io_space_update(s); - acpi_pcihp_reset(&s->acpi_pci_hotplug); + acpi_pcihp_reset(&s->acpi_pci_hotplug, !s->use_acpi_root_pci_hotplug); } static void piix4_pm_powerdown_req(Notifier *n, void *opaque) @@ -635,6 +636,8 @@ static Property piix4_pm_properties[] = { DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2), DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState, use_acpi_hotplug_bridge, true), + DEFINE_PROP_BOOL("acpi-root-pci-hotplug", PIIX4PMState, + use_acpi_root_pci_hotplug, true), DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState, acpi_memory_hotplug.is_enabled, true), DEFINE_PROP_END_OF_LIST(), diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h index 8bc4a4c01d..02f4665767 100644 --- a/include/hw/acpi/pcihp.h +++ b/include/hw/acpi/pcihp.h @@ -67,7 +67,7 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev, Error **errp); /* Called on reset */ -void acpi_pcihp_reset(AcpiPciHpState *s); +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off); extern const VMStateDescription vmstate_acpi_pcihp_pci_status; From 1436f32a84c3fda61d0d80302e24d641d3f3f839 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 18 Aug 2020 15:33:44 +0100 Subject: [PATCH 07/13] virtio-pci: add virtio_pci_optimal_num_queues() helper Multi-queue devices achieve the best performance when each vCPU has a dedicated queue. This ensures that virtqueue used notifications are handled on the same vCPU that submitted virtqueue buffers. When another vCPU handles the the notification an IPI will be necessary to wake the submission vCPU and this incurs a performance overhead. Provide a helper function that virtio-pci devices will use in later patches to automatically select the optimal number of queues. The function handles guests with large numbers of CPUs by limiting the number of queues to fit within the following constraints: 1. The maximum number of MSI-X vectors. 2. The maximum number of virtqueues. Signed-off-by: Stefan Hajnoczi Reviewed-by: Cornelia Huck Message-Id: <20200818143348.310613-4-stefanha@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/virtio/virtio-pci.c | 32 ++++++++++++++++++++++++++++++++ hw/virtio/virtio-pci.h | 9 +++++++++ 2 files changed, 41 insertions(+) diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index ccdf54e81c..fc69570dcc 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -19,6 +19,7 @@ #include "exec/memop.h" #include "standard-headers/linux/virtio_pci.h" +#include "hw/boards.h" #include "hw/virtio/virtio.h" #include "migration/qemu-file-types.h" #include "hw/pci/pci.h" @@ -2058,6 +2059,37 @@ void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t) g_free(base_name); } +unsigned virtio_pci_optimal_num_queues(unsigned fixed_queues) +{ + /* + * 1:1 vq to vCPU mapping is ideal because the same vCPU that submitted + * virtqueue buffers can handle their completion. When a different vCPU + * handles completion it may need to IPI the vCPU that submitted the + * request and this adds overhead. + * + * Virtqueues consume guest RAM and MSI-X vectors. This is wasteful in + * guests with very many vCPUs and a device that is only used by a few + * vCPUs. Unfortunately optimizing that case requires manual pinning inside + * the guest, so those users might as well manually set the number of + * queues. There is no upper limit that can be applied automatically and + * doing so arbitrarily would result in a sudden performance drop once the + * threshold number of vCPUs is exceeded. + */ + unsigned num_queues = current_machine->smp.cpus; + + /* + * The maximum number of MSI-X vectors is PCI_MSIX_FLAGS_QSIZE + 1, but the + * config change interrupt and the fixed virtqueues must be taken into + * account too. + */ + num_queues = MIN(num_queues, PCI_MSIX_FLAGS_QSIZE - fixed_queues); + + /* + * There is a limit to how many virtqueues a device can have. + */ + return MIN(num_queues, VIRTIO_QUEUE_MAX - fixed_queues); +} + /* virtio-pci-bus */ static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size, diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h index e2eaaa9182..91096f0291 100644 --- a/hw/virtio/virtio-pci.h +++ b/hw/virtio/virtio-pci.h @@ -243,4 +243,13 @@ typedef struct VirtioPCIDeviceTypeInfo { /* Register virtio-pci type(s). @t must be static. */ void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t); +/** + * virtio_pci_optimal_num_queues: + * @fixed_queues: number of queues that are always present + * + * Returns: The optimal number of queues for a multi-queue device, excluding + * @fixed_queues. + */ +unsigned virtio_pci_optimal_num_queues(unsigned fixed_queues); + #endif From 4e5163bd844429711b4d60d51030ab83048d80cd Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 18 Aug 2020 15:33:45 +0100 Subject: [PATCH 08/13] virtio-scsi: introduce a constant for fixed virtqueues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The event and control virtqueues are always present, regardless of the multi-queue configuration. Define a constant so that virtqueue number calculations are easier to read. Signed-off-by: Stefan Hajnoczi Reviewed-by: Cornelia Huck Reviewed-by: Pankaj Gupta Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Raphael Norwitz Message-Id: <20200818143348.310613-5-stefanha@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/scsi/vhost-user-scsi.c | 2 +- hw/scsi/virtio-scsi.c | 7 ++++--- hw/virtio/vhost-scsi-pci.c | 3 ++- hw/virtio/vhost-user-scsi-pci.c | 3 ++- hw/virtio/virtio-scsi-pci.c | 3 ++- include/hw/virtio/virtio-scsi.h | 3 +++ 6 files changed, 14 insertions(+), 7 deletions(-) diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c index f2e524438a..a8b821466f 100644 --- a/hw/scsi/vhost-user-scsi.c +++ b/hw/scsi/vhost-user-scsi.c @@ -114,7 +114,7 @@ static void vhost_user_scsi_realize(DeviceState *dev, Error **errp) goto free_virtio; } - vsc->dev.nvqs = 2 + vs->conf.num_queues; + vsc->dev.nvqs = VIRTIO_SCSI_VQ_NUM_FIXED + vs->conf.num_queues; vsc->dev.vqs = g_new0(struct vhost_virtqueue, vsc->dev.nvqs); vsc->dev.vq_index = 0; vsc->dev.backend_features = 0; diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index b49775269e..eecdd05af5 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -191,7 +191,7 @@ static void virtio_scsi_save_request(QEMUFile *f, SCSIRequest *sreq) VirtIOSCSIReq *req = sreq->hba_private; VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(req->dev); VirtIODevice *vdev = VIRTIO_DEVICE(req->dev); - uint32_t n = virtio_get_queue_index(req->vq) - 2; + uint32_t n = virtio_get_queue_index(req->vq) - VIRTIO_SCSI_VQ_NUM_FIXED; assert(n < vs->conf.num_queues); qemu_put_be32s(f, &n); @@ -892,10 +892,11 @@ void virtio_scsi_common_realize(DeviceState *dev, sizeof(VirtIOSCSIConfig)); if (s->conf.num_queues == 0 || - s->conf.num_queues > VIRTIO_QUEUE_MAX - 2) { + s->conf.num_queues > VIRTIO_QUEUE_MAX - VIRTIO_SCSI_VQ_NUM_FIXED) { error_setg(errp, "Invalid number of queues (= %" PRIu32 "), " "must be a positive integer less than %d.", - s->conf.num_queues, VIRTIO_QUEUE_MAX - 2); + s->conf.num_queues, + VIRTIO_QUEUE_MAX - VIRTIO_SCSI_VQ_NUM_FIXED); virtio_cleanup(vdev); return; } diff --git a/hw/virtio/vhost-scsi-pci.c b/hw/virtio/vhost-scsi-pci.c index 095af23f3f..06e814d30e 100644 --- a/hw/virtio/vhost-scsi-pci.c +++ b/hw/virtio/vhost-scsi-pci.c @@ -50,7 +50,8 @@ static void vhost_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev); if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { - vpci_dev->nvectors = vs->conf.num_queues + 3; + vpci_dev->nvectors = vs->conf.num_queues + + VIRTIO_SCSI_VQ_NUM_FIXED + 1; } qdev_realize(vdev, BUS(&vpci_dev->bus), errp); diff --git a/hw/virtio/vhost-user-scsi-pci.c b/hw/virtio/vhost-user-scsi-pci.c index 4705cd54e8..ab6dfb71a9 100644 --- a/hw/virtio/vhost-user-scsi-pci.c +++ b/hw/virtio/vhost-user-scsi-pci.c @@ -56,7 +56,8 @@ static void vhost_user_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev); if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { - vpci_dev->nvectors = vs->conf.num_queues + 3; + vpci_dev->nvectors = vs->conf.num_queues + + VIRTIO_SCSI_VQ_NUM_FIXED + 1; } qdev_realize(vdev, BUS(&vpci_dev->bus), errp); diff --git a/hw/virtio/virtio-scsi-pci.c b/hw/virtio/virtio-scsi-pci.c index c23a134202..3ff9eb7ef6 100644 --- a/hw/virtio/virtio-scsi-pci.c +++ b/hw/virtio/virtio-scsi-pci.c @@ -51,7 +51,8 @@ static void virtio_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) char *bus_name; if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { - vpci_dev->nvectors = vs->conf.num_queues + 3; + vpci_dev->nvectors = vs->conf.num_queues + + VIRTIO_SCSI_VQ_NUM_FIXED + 1; } /* diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h index 24e768909d..9f293bcb80 100644 --- a/include/hw/virtio/virtio-scsi.h +++ b/include/hw/virtio/virtio-scsi.h @@ -36,6 +36,9 @@ #define VIRTIO_SCSI_MAX_TARGET 255 #define VIRTIO_SCSI_MAX_LUN 16383 +/* Number of virtqueues that are always present */ +#define VIRTIO_SCSI_VQ_NUM_FIXED 2 + typedef struct virtio_scsi_cmd_req VirtIOSCSICmdReq; typedef struct virtio_scsi_cmd_resp VirtIOSCSICmdResp; typedef struct virtio_scsi_ctrl_tmf_req VirtIOSCSICtrlTMFReq; From 6a558822849fab604a1fcb71e7c4e528cafa21d9 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 18 Aug 2020 15:33:46 +0100 Subject: [PATCH 09/13] virtio-scsi-pci: default num_queues to -smp N Automatically size the number of virtio-scsi-pci, vhost-scsi-pci, and vhost-user-scsi-pci request virtqueues to match the number of vCPUs. Other transports continue to default to 1 request virtqueue. A 1:1 virtqueue:vCPU mapping ensures that completion interrupts are handled on the same vCPU that submitted the request. No IPI is necessary to complete an I/O request and performance is improved. The maximum number of MSI-X vectors and virtqueues limit are respected. Signed-off-by: Stefan Hajnoczi Message-Id: <20200818143348.310613-6-stefanha@redhat.com> Reviewed-by: Cornelia Huck Reviewed-by: Raphael Norwitz Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/core/machine.c | 6 +++++- hw/scsi/vhost-scsi.c | 3 ++- hw/scsi/vhost-user-scsi.c | 3 ++- hw/scsi/virtio-scsi.c | 6 +++++- hw/virtio/vhost-scsi-pci.c | 10 +++++++--- hw/virtio/vhost-user-scsi-pci.c | 10 +++++++--- hw/virtio/virtio-scsi-pci.c | 10 +++++++--- include/hw/virtio/virtio-scsi.h | 2 ++ 8 files changed, 37 insertions(+), 13 deletions(-) diff --git a/hw/core/machine.c b/hw/core/machine.c index cf5f2dfaeb..9ee2aa0f7b 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -28,7 +28,11 @@ #include "hw/mem/nvdimm.h" #include "migration/vmstate.h" -GlobalProperty hw_compat_5_1[] = {}; +GlobalProperty hw_compat_5_1[] = { + { "vhost-scsi", "num_queues", "1"}, + { "vhost-user-scsi", "num_queues", "1"}, + { "virtio-scsi-device", "num_queues", "1"}, +}; const size_t hw_compat_5_1_len = G_N_ELEMENTS(hw_compat_5_1); GlobalProperty hw_compat_5_0[] = { diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c index 13b05af29b..a83ffeefc8 100644 --- a/hw/scsi/vhost-scsi.c +++ b/hw/scsi/vhost-scsi.c @@ -270,7 +270,8 @@ static Property vhost_scsi_properties[] = { DEFINE_PROP_STRING("vhostfd", VirtIOSCSICommon, conf.vhostfd), DEFINE_PROP_STRING("wwpn", VirtIOSCSICommon, conf.wwpn), DEFINE_PROP_UINT32("boot_tpgt", VirtIOSCSICommon, conf.boot_tpgt, 0), - DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues, 1), + DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues, + VIRTIO_SCSI_AUTO_NUM_QUEUES), DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSICommon, conf.virtqueue_size, 128), DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSICommon, conf.seg_max_adjust, diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c index a8b821466f..7c0631656c 100644 --- a/hw/scsi/vhost-user-scsi.c +++ b/hw/scsi/vhost-user-scsi.c @@ -162,7 +162,8 @@ static void vhost_user_scsi_unrealize(DeviceState *dev) static Property vhost_user_scsi_properties[] = { DEFINE_PROP_CHR("chardev", VirtIOSCSICommon, conf.chardev), DEFINE_PROP_UINT32("boot_tpgt", VirtIOSCSICommon, conf.boot_tpgt, 0), - DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues, 1), + DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues, + VIRTIO_SCSI_AUTO_NUM_QUEUES), DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSICommon, conf.virtqueue_size, 128), DEFINE_PROP_UINT32("max_sectors", VirtIOSCSICommon, conf.max_sectors, diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index eecdd05af5..3a71ea7097 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -891,6 +891,9 @@ void virtio_scsi_common_realize(DeviceState *dev, virtio_init(vdev, "virtio-scsi", VIRTIO_ID_SCSI, sizeof(VirtIOSCSIConfig)); + if (s->conf.num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) { + s->conf.num_queues = 1; + } if (s->conf.num_queues == 0 || s->conf.num_queues > VIRTIO_QUEUE_MAX - VIRTIO_SCSI_VQ_NUM_FIXED) { error_setg(errp, "Invalid number of queues (= %" PRIu32 "), " @@ -964,7 +967,8 @@ static void virtio_scsi_device_unrealize(DeviceState *dev) } static Property virtio_scsi_properties[] = { - DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues, 1), + DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues, + VIRTIO_SCSI_AUTO_NUM_QUEUES), DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI, parent_obj.conf.virtqueue_size, 256), DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSI, diff --git a/hw/virtio/vhost-scsi-pci.c b/hw/virtio/vhost-scsi-pci.c index 06e814d30e..a6bb0dc60d 100644 --- a/hw/virtio/vhost-scsi-pci.c +++ b/hw/virtio/vhost-scsi-pci.c @@ -47,11 +47,15 @@ static void vhost_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) { VHostSCSIPCI *dev = VHOST_SCSI_PCI(vpci_dev); DeviceState *vdev = DEVICE(&dev->vdev); - VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev); + VirtIOSCSIConf *conf = &dev->vdev.parent_obj.parent_obj.conf; + + if (conf->num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) { + conf->num_queues = + virtio_pci_optimal_num_queues(VIRTIO_SCSI_VQ_NUM_FIXED); + } if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { - vpci_dev->nvectors = vs->conf.num_queues + - VIRTIO_SCSI_VQ_NUM_FIXED + 1; + vpci_dev->nvectors = conf->num_queues + VIRTIO_SCSI_VQ_NUM_FIXED + 1; } qdev_realize(vdev, BUS(&vpci_dev->bus), errp); diff --git a/hw/virtio/vhost-user-scsi-pci.c b/hw/virtio/vhost-user-scsi-pci.c index ab6dfb71a9..25e97ca54e 100644 --- a/hw/virtio/vhost-user-scsi-pci.c +++ b/hw/virtio/vhost-user-scsi-pci.c @@ -53,11 +53,15 @@ static void vhost_user_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) { VHostUserSCSIPCI *dev = VHOST_USER_SCSI_PCI(vpci_dev); DeviceState *vdev = DEVICE(&dev->vdev); - VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev); + VirtIOSCSIConf *conf = &dev->vdev.parent_obj.parent_obj.conf; + + if (conf->num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) { + conf->num_queues = + virtio_pci_optimal_num_queues(VIRTIO_SCSI_VQ_NUM_FIXED); + } if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { - vpci_dev->nvectors = vs->conf.num_queues + - VIRTIO_SCSI_VQ_NUM_FIXED + 1; + vpci_dev->nvectors = conf->num_queues + VIRTIO_SCSI_VQ_NUM_FIXED + 1; } qdev_realize(vdev, BUS(&vpci_dev->bus), errp); diff --git a/hw/virtio/virtio-scsi-pci.c b/hw/virtio/virtio-scsi-pci.c index 3ff9eb7ef6..fa4b3bfb50 100644 --- a/hw/virtio/virtio-scsi-pci.c +++ b/hw/virtio/virtio-scsi-pci.c @@ -46,13 +46,17 @@ static void virtio_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) { VirtIOSCSIPCI *dev = VIRTIO_SCSI_PCI(vpci_dev); DeviceState *vdev = DEVICE(&dev->vdev); - VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev); DeviceState *proxy = DEVICE(vpci_dev); + VirtIOSCSIConf *conf = &dev->vdev.parent_obj.conf; char *bus_name; + if (conf->num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) { + conf->num_queues = + virtio_pci_optimal_num_queues(VIRTIO_SCSI_VQ_NUM_FIXED); + } + if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { - vpci_dev->nvectors = vs->conf.num_queues + - VIRTIO_SCSI_VQ_NUM_FIXED + 1; + vpci_dev->nvectors = conf->num_queues + VIRTIO_SCSI_VQ_NUM_FIXED + 1; } /* diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h index 9f293bcb80..c0b8e4dd7e 100644 --- a/include/hw/virtio/virtio-scsi.h +++ b/include/hw/virtio/virtio-scsi.h @@ -39,6 +39,8 @@ /* Number of virtqueues that are always present */ #define VIRTIO_SCSI_VQ_NUM_FIXED 2 +#define VIRTIO_SCSI_AUTO_NUM_QUEUES UINT32_MAX + typedef struct virtio_scsi_cmd_req VirtIOSCSICmdReq; typedef struct virtio_scsi_cmd_resp VirtIOSCSICmdResp; typedef struct virtio_scsi_ctrl_tmf_req VirtIOSCSICtrlTMFReq; From 9445e1e15e66c19e42bea942ba810db28052cd05 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 18 Aug 2020 15:33:47 +0100 Subject: [PATCH 10/13] virtio-blk-pci: default num_queues to -smp N Automatically size the number of virtio-blk-pci request virtqueues to match the number of vCPUs. Other transports continue to default to 1 request virtqueue. A 1:1 virtqueue:vCPU mapping ensures that completion interrupts are handled on the same vCPU that submitted the request. No IPI is necessary to complete an I/O request and performance is improved. The maximum number of MSI-X vectors and virtqueues limit are respected. Performance improves from 78k to 104k IOPS on a 32 vCPU guest with 101 virtio-blk-pci devices (ioengine=libaio, iodepth=1, bs=4k, rw=randread with NVMe storage). Signed-off-by: Stefan Hajnoczi Reviewed-by: Cornelia Huck Reviewed-by: Pankaj Gupta Message-Id: <20200818143348.310613-7-stefanha@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/block/virtio-blk.c | 6 +++++- hw/core/machine.c | 1 + hw/virtio/virtio-blk-pci.c | 7 ++++++- include/hw/virtio/virtio-blk.h | 2 ++ 4 files changed, 14 insertions(+), 2 deletions(-) diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 413783693c..2204ba149e 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -1147,6 +1147,9 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) error_setg(errp, "Device needs media, but drive is empty"); return; } + if (conf->num_queues == VIRTIO_BLK_AUTO_NUM_QUEUES) { + conf->num_queues = 1; + } if (!conf->num_queues) { error_setg(errp, "num-queues property must be larger than 0"); return; @@ -1281,7 +1284,8 @@ static Property virtio_blk_properties[] = { #endif DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0, true), - DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1), + DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, + VIRTIO_BLK_AUTO_NUM_QUEUES), DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 256), DEFINE_PROP_BOOL("seg-max-adjust", VirtIOBlock, conf.seg_max_adjust, true), DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, diff --git a/hw/core/machine.c b/hw/core/machine.c index 9ee2aa0f7b..7f65fa8743 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -31,6 +31,7 @@ GlobalProperty hw_compat_5_1[] = { { "vhost-scsi", "num_queues", "1"}, { "vhost-user-scsi", "num_queues", "1"}, + { "virtio-blk-device", "num-queues", "1"}, { "virtio-scsi-device", "num_queues", "1"}, }; const size_t hw_compat_5_1_len = G_N_ELEMENTS(hw_compat_5_1); diff --git a/hw/virtio/virtio-blk-pci.c b/hw/virtio/virtio-blk-pci.c index 849cc7dfd8..37c6e0aeb4 100644 --- a/hw/virtio/virtio-blk-pci.c +++ b/hw/virtio/virtio-blk-pci.c @@ -50,9 +50,14 @@ static void virtio_blk_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) { VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev); DeviceState *vdev = DEVICE(&dev->vdev); + VirtIOBlkConf *conf = &dev->vdev.conf; + + if (conf->num_queues == VIRTIO_BLK_AUTO_NUM_QUEUES) { + conf->num_queues = virtio_pci_optimal_num_queues(0); + } if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { - vpci_dev->nvectors = dev->vdev.conf.num_queues + 1; + vpci_dev->nvectors = conf->num_queues + 1; } qdev_realize(vdev, BUS(&vpci_dev->bus), errp); diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h index b1334c3904..7539c2b848 100644 --- a/include/hw/virtio/virtio-blk.h +++ b/include/hw/virtio/virtio-blk.h @@ -30,6 +30,8 @@ struct virtio_blk_inhdr unsigned char status; }; +#define VIRTIO_BLK_AUTO_NUM_QUEUES UINT16_MAX + struct VirtIOBlkConf { BlockConf conf; From a4eef0711b2cf7a7476c3e2c202a414b68a1baa0 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 18 Aug 2020 15:33:48 +0100 Subject: [PATCH 11/13] vhost-user-blk-pci: default num_queues to -smp N Automatically size the number of request virtqueues to match the number of vCPUs. This ensures that completion interrupts are handled on the same vCPU that submitted the request. No IPI is necessary to complete an I/O request and performance is improved. The maximum number of MSI-X vectors and virtqueues limit are respected. Signed-off-by: Stefan Hajnoczi Reviewed-by: Cornelia Huck Reviewed-by: Raphael Norwitz Message-Id: <20200818143348.310613-8-stefanha@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/block/vhost-user-blk.c | 6 +++++- hw/core/machine.c | 1 + hw/virtio/vhost-user-blk-pci.c | 4 ++++ include/hw/virtio/vhost-user-blk.h | 2 ++ 4 files changed, 12 insertions(+), 1 deletion(-) diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index a00b854736..39aec42dae 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -420,6 +420,9 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) return; } + if (s->num_queues == VHOST_USER_BLK_AUTO_NUM_QUEUES) { + s->num_queues = 1; + } if (!s->num_queues || s->num_queues > VIRTIO_QUEUE_MAX) { error_setg(errp, "vhost-user-blk: invalid number of IO queues"); return; @@ -531,7 +534,8 @@ static const VMStateDescription vmstate_vhost_user_blk = { static Property vhost_user_blk_properties[] = { DEFINE_PROP_CHR("chardev", VHostUserBlk, chardev), - DEFINE_PROP_UINT16("num-queues", VHostUserBlk, num_queues, 1), + DEFINE_PROP_UINT16("num-queues", VHostUserBlk, num_queues, + VHOST_USER_BLK_AUTO_NUM_QUEUES), DEFINE_PROP_UINT32("queue-size", VHostUserBlk, queue_size, 128), DEFINE_PROP_BIT("config-wce", VHostUserBlk, config_wce, 0, true), DEFINE_PROP_END_OF_LIST(), diff --git a/hw/core/machine.c b/hw/core/machine.c index 7f65fa8743..ea26d61237 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -30,6 +30,7 @@ GlobalProperty hw_compat_5_1[] = { { "vhost-scsi", "num_queues", "1"}, + { "vhost-user-blk", "num-queues", "1"}, { "vhost-user-scsi", "num_queues", "1"}, { "virtio-blk-device", "num-queues", "1"}, { "virtio-scsi-device", "num_queues", "1"}, diff --git a/hw/virtio/vhost-user-blk-pci.c b/hw/virtio/vhost-user-blk-pci.c index 4f5d5cbf44..a62a71e067 100644 --- a/hw/virtio/vhost-user-blk-pci.c +++ b/hw/virtio/vhost-user-blk-pci.c @@ -54,6 +54,10 @@ static void vhost_user_blk_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) VHostUserBlkPCI *dev = VHOST_USER_BLK_PCI(vpci_dev); DeviceState *vdev = DEVICE(&dev->vdev); + if (dev->vdev.num_queues == VHOST_USER_BLK_AUTO_NUM_QUEUES) { + dev->vdev.num_queues = virtio_pci_optimal_num_queues(0); + } + if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { vpci_dev->nvectors = dev->vdev.num_queues + 1; } diff --git a/include/hw/virtio/vhost-user-blk.h b/include/hw/virtio/vhost-user-blk.h index 34ad6f0c0e..292d17147c 100644 --- a/include/hw/virtio/vhost-user-blk.h +++ b/include/hw/virtio/vhost-user-blk.h @@ -25,6 +25,8 @@ #define VHOST_USER_BLK(obj) \ OBJECT_CHECK(VHostUserBlk, (obj), TYPE_VHOST_USER_BLK) +#define VHOST_USER_BLK_AUTO_NUM_QUEUES UINT16_MAX + typedef struct VHostUserBlk { VirtIODevice parent_obj; CharBackend chardev; From c906e03909ec3eacbaa95367c1cb78a329a6c62f Mon Sep 17 00:00:00 2001 From: Ying Fang Date: Thu, 6 Aug 2020 11:56:33 +0800 Subject: [PATCH 12/13] hw/smbios: add options for type 4 max-speed and current-speed Common VM users sometimes care about CPU speed, so we add two new options to allow VM vendors to present CPU speed to their users. Normally these information can be fetched from host smbios. Strictly speaking, the "max speed" and "current speed" in type 4 are not really for the max speed and current speed of processor, for "max speed" identifies a capability of the system, and "current speed" identifies the processor's speed at boot (see smbios spec), but some applications do not tell the differences. Reviewed-by: Igor Mammedov Signed-off-by: Ying Fang Signed-off-by: Heyi Guo Message-Id: <20200806035634.376-2-fangying1@huawei.com> --- hw/smbios/smbios.c | 36 ++++++++++++++++++++++++++++++++---- qemu-options.hx | 2 +- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c index f560826904..7cc950b41c 100644 --- a/hw/smbios/smbios.c +++ b/hw/smbios/smbios.c @@ -92,9 +92,21 @@ static struct { const char *manufacturer, *version, *serial, *asset, *sku; } type3; +/* + * SVVP requires max_speed and current_speed to be set and not being + * 0 which counts as unknown (SMBIOS 3.1.0/Table 21). Set the + * default value to 2000MHz as we did before. + */ +#define DEFAULT_CPU_SPEED 2000 + static struct { const char *sock_pfx, *manufacturer, *version, *serial, *asset, *part; -} type4; + uint64_t max_speed; + uint64_t current_speed; +} type4 = { + .max_speed = DEFAULT_CPU_SPEED, + .current_speed = DEFAULT_CPU_SPEED +}; static struct { size_t nvalues; @@ -272,6 +284,14 @@ static const QemuOptDesc qemu_smbios_type4_opts[] = { .name = "version", .type = QEMU_OPT_STRING, .help = "version number", + },{ + .name = "max-speed", + .type = QEMU_OPT_NUMBER, + .help = "max speed in MHz", + },{ + .name = "current-speed", + .type = QEMU_OPT_NUMBER, + .help = "speed at system boot in MHz", },{ .name = "serial", .type = QEMU_OPT_STRING, @@ -586,9 +606,8 @@ static void smbios_build_type_4_table(MachineState *ms, unsigned instance) SMBIOS_TABLE_SET_STR(4, processor_version_str, type4.version); t->voltage = 0; t->external_clock = cpu_to_le16(0); /* Unknown */ - /* SVVP requires max_speed and current_speed to not be unknown. */ - t->max_speed = cpu_to_le16(2000); /* 2000 MHz */ - t->current_speed = cpu_to_le16(2000); /* 2000 MHz */ + t->max_speed = cpu_to_le16(type4.max_speed); + t->current_speed = cpu_to_le16(type4.current_speed); t->status = 0x41; /* Socket populated, CPU enabled */ t->processor_upgrade = 0x01; /* Other */ t->l1_cache_handle = cpu_to_le16(0xFFFF); /* N/A */ @@ -1116,6 +1135,15 @@ void smbios_entry_add(QemuOpts *opts, Error **errp) save_opt(&type4.serial, opts, "serial"); save_opt(&type4.asset, opts, "asset"); save_opt(&type4.part, opts, "part"); + type4.max_speed = qemu_opt_get_number(opts, "max-speed", + DEFAULT_CPU_SPEED); + type4.current_speed = qemu_opt_get_number(opts, "current-speed", + DEFAULT_CPU_SPEED); + if (type4.max_speed > UINT16_MAX || + type4.current_speed > UINT16_MAX) { + error_setg(errp, "SMBIOS CPU speed is too large (> %d)", + UINT16_MAX); + } return; case 11: if (!qemu_opts_validate(opts, qemu_smbios_type11_opts, errp)) { diff --git a/qemu-options.hx b/qemu-options.hx index 708583b4ce..30019c4eca 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -2294,7 +2294,7 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios, " [,sku=str]\n" " specify SMBIOS type 3 fields\n" "-smbios type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str]\n" - " [,asset=str][,part=str]\n" + " [,asset=str][,part=str][,max-speed=%d][,current-speed=%d]\n" " specify SMBIOS type 4 fields\n" "-smbios type=17[,loc_pfx=str][,bank=str][,manufacturer=str][,serial=str]\n" " [,asset=str][,part=str][,speed=%d]\n" From e1647539b1d04f121b70f1f6f438976477450f10 Mon Sep 17 00:00:00 2001 From: Ying Fang Date: Thu, 6 Aug 2020 11:56:34 +0800 Subject: [PATCH 13/13] tests/bios-tables-test: add smbios cpu speed test Add smbios type 4 CPU speed check for we added new options to set smbios type 4 "max speed" and "current speed". The default value should be 2000 when no option is specified, just as the old version did. We add the test case to one machine of each architecture, though it doesn't really run on aarch64 platform for smbios test can't run on uefi only platform yet. Signed-off-by: Ying Fang Signed-off-by: Heyi Guo Message-Id: <20200806035634.376-3-fangying1@huawei.com> --- tests/qtest/bios-tables-test.c | 42 ++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index d25ff35492..504b810af5 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -80,6 +80,8 @@ typedef struct { GArray *tables; uint32_t smbios_ep_addr; struct smbios_21_entry_point smbios_ep_table; + uint16_t smbios_cpu_max_speed; + uint16_t smbios_cpu_curr_speed; uint8_t *required_struct_types; int required_struct_types_len; QTestState *qts; @@ -563,6 +565,31 @@ static inline bool smbios_single_instance(uint8_t type) } } +static bool smbios_cpu_test(test_data *data, uint32_t addr) +{ + uint16_t expect_speed[2]; + uint16_t real; + int offset[2]; + int i; + + /* Check CPU speed for backward compatibility */ + offset[0] = offsetof(struct smbios_type_4, max_speed); + offset[1] = offsetof(struct smbios_type_4, current_speed); + expect_speed[0] = data->smbios_cpu_max_speed ? : 2000; + expect_speed[1] = data->smbios_cpu_curr_speed ? : 2000; + + for (i = 0; i < 2; i++) { + real = qtest_readw(data->qts, addr + offset[i]); + if (real != expect_speed[i]) { + fprintf(stderr, "Unexpected SMBIOS CPU speed: real %u expect %u\n", + real, expect_speed[i]); + return false; + } + } + + return true; +} + static void test_smbios_structs(test_data *data) { DECLARE_BITMAP(struct_bitmap, SMBIOS_MAX_TYPE+1) = { 0 }; @@ -585,6 +612,10 @@ static void test_smbios_structs(test_data *data) } set_bit(type, struct_bitmap); + if (type == 4) { + g_assert(smbios_cpu_test(data, addr)); + } + /* seek to end of unformatted string area of this struct ("\0\0") */ prv = crt = 1; while (prv || crt) { @@ -719,6 +750,11 @@ static void test_acpi_q35_tcg(void) data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types); test_acpi_one(NULL, &data); free_test_data(&data); + + data.smbios_cpu_max_speed = 3000; + data.smbios_cpu_curr_speed = 2600; + test_acpi_one("-smbios type=4,max-speed=3000,current-speed=2600", &data); + free_test_data(&data); } static void test_acpi_q35_tcg_bridge(void) @@ -1084,6 +1120,12 @@ static void test_acpi_virt_tcg(void) test_acpi_one("-cpu cortex-a57", &data); free_test_data(&data); + + data.smbios_cpu_max_speed = 2900; + data.smbios_cpu_curr_speed = 2700; + test_acpi_one("-cpu cortex-a57 " + "-smbios type=4,max-speed=2900,current-speed=2700", &data); + free_test_data(&data); } int main(int argc, char *argv[])