* [PULL 01/13] acpi: allow DSDT changes
2020-08-27 13:40 [PULL 00/13] virtio,pc,acpi: features, fixes Michael S. Tsirkin
@ 2020-08-27 13:40 ` Michael S. Tsirkin
2020-08-27 13:40 ` [PULL 02/13] i386/acpi: fix inconsistent QEMU/OVMF device paths Michael S. Tsirkin
` (12 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Michael S. Tsirkin @ 2020-08-27 13:40 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Igor Mammedov
We are updating all DSDTs with UID 0 for PCI Root.
Allow changes.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
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",
--
MST
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PULL 02/13] i386/acpi: fix inconsistent QEMU/OVMF device paths
2020-08-27 13:40 [PULL 00/13] virtio,pc,acpi: features, fixes Michael S. Tsirkin
2020-08-27 13:40 ` [PULL 01/13] acpi: allow DSDT changes Michael S. Tsirkin
@ 2020-08-27 13:40 ` Michael S. Tsirkin
2020-08-27 13:40 ` [PULL 03/13] arm/acpi: fix an out of spec _UID for PCI root Michael S. Tsirkin
` (11 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Michael S. Tsirkin @ 2020-08-27 13:40 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Vitaly Cheptsov, qemu-stable, Eduardo Habkost,
Paolo Bonzini, Igor Mammedov, Laszlo Ersek, Richard Henderson
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 <lersek@redhat.com>
Tested-by: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
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);
--
MST
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PULL 03/13] arm/acpi: fix an out of spec _UID for PCI root
2020-08-27 13:40 [PULL 00/13] virtio,pc,acpi: features, fixes Michael S. Tsirkin
2020-08-27 13:40 ` [PULL 01/13] acpi: allow DSDT changes Michael S. Tsirkin
2020-08-27 13:40 ` [PULL 02/13] i386/acpi: fix inconsistent QEMU/OVMF device paths Michael S. Tsirkin
@ 2020-08-27 13:40 ` Michael S. Tsirkin
2020-08-27 13:40 ` [PULL 04/13] disassemble-aml: -o actually works Michael S. Tsirkin
` (10 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Michael S. Tsirkin @ 2020-08-27 13:40 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Vitaly Cheptsov, qemu-stable, Shannon Zhao,
qemu-arm, Igor Mammedov, Laszlo Ersek
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 <vit9696@protonmail.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
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)));
--
MST
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PULL 04/13] disassemble-aml: -o actually works
2020-08-27 13:40 [PULL 00/13] virtio,pc,acpi: features, fixes Michael S. Tsirkin
` (2 preceding siblings ...)
2020-08-27 13:40 ` [PULL 03/13] arm/acpi: fix an out of spec _UID for PCI root Michael S. Tsirkin
@ 2020-08-27 13:40 ` Michael S. Tsirkin
2020-08-27 13:40 ` [PULL 05/13] acpi: update expected DSDT files with _UID changes Michael S. Tsirkin
` (9 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Michael S. Tsirkin @ 2020-08-27 13:40 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Igor Mammedov
Turns out that option was borken due to weird iasl
command line handling. Fix it.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
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
--
MST
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PULL 05/13] acpi: update expected DSDT files with _UID changes
2020-08-27 13:40 [PULL 00/13] virtio,pc,acpi: features, fixes Michael S. Tsirkin
` (3 preceding siblings ...)
2020-08-27 13:40 ` [PULL 04/13] disassemble-aml: -o actually works Michael S. Tsirkin
@ 2020-08-27 13:40 ` Michael S. Tsirkin
2020-08-27 13:40 ` [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus Michael S. Tsirkin
` (8 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Michael S. Tsirkin @ 2020-08-27 13:40 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Igor Mammedov
_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 <mst@redhat.com>
---
tests/qtest/bios-tables-test-allowed-diff.h | 21 --------------------
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
22 files changed, 21 deletions(-)
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",
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(@SCD<jzO_+g!@xVl`Hb#bx6SoQh09#!LR{#J2
delta 24
gcmX@6c1(@SCD<jzO_+g!asNcFHb%yc6SoQh09#lGR{#J2
diff --git a/tests/data/acpi/pc/DSDT.acpihmat b/tests/data/acpi/pc/DSDT.acpihmat
index 2e5e02400b1bd2842989d395c573fc593f45503b..b0dbb943f4cea83a5adde23aefa54f1678c560a1 100644
GIT binary patch
delta 24
fcmexl@X3J7CD<jTNP>ZZv2P+*8zaNUi4VmAW<Uq?
delta 24
fcmexl@X3J7CD<jTNP>ZZv3DX@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_b33dtTlwx3D<ebRW#>lX7;txpxP_YKs
delta 24
fcmeA)?KI_b33dtTlwx3D<e13S#>lvF;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
fcmbQLHC2ntCD<iIRFr{%(Q+bJ8zaNUi7g@kO4bG#
delta 24
fcmbQLHC2ntCD<iIRFr{%(PAQ38zbY!i7g@kO3?-w
diff --git a/tests/data/acpi/pc/DSDT.dimmpxm b/tests/data/acpi/pc/DSDT.dimmpxm
index 21eb065a0ee3bd96f1a2e7601aa83fefa833349a..1649953b6cccb933e4a440dc56507dc9197c4a8a 100644
GIT binary patch
delta 24
fcmdmOyxW+|CD<iow<H4tqvAxaHb#bx6SX7(TZsm<
delta 24
fcmdmOyxW+|CD<iow<H4tqryb4Hb%yc6SX7(TZ9I)
diff --git a/tests/data/acpi/pc/DSDT.ipmikcs b/tests/data/acpi/pc/DSDT.ipmikcs
index b8f08f266b5735fe6967d4e105ee6b3662dad7e6..92748d49dcd418e4a734da47e8d5c0268aedfc29 100644
GIT binary patch
delta 24
fcmeBE?^EY;33dtT6J}sw44KH)#>lX7;$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
gcmbPgIMtBLCD<iosssZA<EDvRZHx>XC$dNY09SGbrT_o{
delta 24
ecmbPgIMtBLCD<iosssZA<Hm_xZHz#YMFId<Z3d<Q
diff --git a/tests/data/acpi/pc/DSDT.numamem b/tests/data/acpi/pc/DSDT.numamem
index 6eec385c2ec00544c6eaa7e19d32b2ccd5a51915..4d9ba337a82954af094e739b8a83467f89a37cc0 100644
GIT binary patch
delta 24
fcmX@3c1DfMCD<jzN0@<uF>4}M8zaNUiMxdWS$hW=
delta 24
fcmX@3c1DfMCD<jzN0@<uF>@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+`qCD<k8pDY6d<C=+FlNcE`&P|jB0CR8%ZU6uP
delta 24
gcmexo{m+`qCD<k8pDY6d<LZfAlNcE{&P|jB0CQ^yZU6uP
diff --git a/tests/data/acpi/q35/DSDT.acpihmat b/tests/data/acpi/q35/DSDT.acpihmat
index cd97b819824e4140d087e465d179b71775d6a494..9cac92418b5fcc2767dc74603d599642b59623fe 100644
GIT binary patch
delta 24
fcmZ4Gw#tpmCD<iIOPPUzv2r5UBu0jfb9<ElSc3-)
delta 24
fcmZ4Gw#tpmCD<iIOPPUzv0@_EBu2)Kb9<ElSbhf#
diff --git a/tests/data/acpi/q35/DSDT.bridge b/tests/data/acpi/q35/DSDT.bridge
index 8b0fb497dbbaeba18e9d0e1503de4396f1c230b0..f08b7245f59aad491fcaa60e2bab1085c369ea1c 100644
GIT binary patch
delta 24
fcmeCT>9^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<k8tULn)qv}MiNsJ5|=NiZZ0AlF}F8}}l
delta 24
gcmX?Wf7YJMCD<k8tULn)qsm0CNsNpe=NiZZ0Al0^F8}}l
diff --git a/tests/data/acpi/q35/DSDT.dimmpxm b/tests/data/acpi/q35/DSDT.dimmpxm
index 29f19b22a38f9d8e7dc9870f0c1aa3d4470643ff..9d5bd5744e2ba2e0f6126c3aba0bb36af865e499 100644
GIT binary patch
delta 24
gcmezD@!5mRCD<jTScQRs@!dqONsJ5|=U!6=0B>^$s{jB1
delta 24
gcmezD@!5mRCD<jTScQRs@$E#eNsNpe=U!6=0B>#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*(CD<jzQ;vaw@%}`vNsJ5|=dO_j0Ad{n&;S4c
delta 24
gcmX?UbJB*(CD<jzQ;vaw@!mwPNsNpe=dO_j0Ad&i&;S4c
diff --git a/tests/data/acpi/q35/DSDT.memhp b/tests/data/acpi/q35/DSDT.memhp
index dca76db15b943122efd5c200cb54ce3dbc97a55f..05a7a73ec43130d5c3018bb462fd84981bfb151c 100644
GIT binary patch
delta 24
gcmX@>cGiu{CD<jzSDAr<aqdK}NsJ5|=Wb8}0Ah#-yZ`_I
delta 24
gcmX@>cGiu{CD<jzSDAr<an3}pNsNpe=Wb8}0Ahm&yZ`_I
diff --git a/tests/data/acpi/q35/DSDT.mmio64 b/tests/data/acpi/q35/DSDT.mmio64
index 6d8facd9e179140682ad5d4302f3dad14dbec342..efd3f1188f2b55da1514212d4be081a61c2a96e9 100644
GIT binary patch
delta 24
gcmaFi^1_A7CD<h-Ly3WbF>)f;Bu0jfb5AP*0A?Ns-T(jq
delta 24
gcmaFi^1_A7CD<h-Ly3WbF=8UuBu2)Kb5AP*0A?8n-T(jq
diff --git a/tests/data/acpi/q35/DSDT.numamem b/tests/data/acpi/q35/DSDT.numamem
index 737325dc3082fdf06283857811f6f5174e0ff2a9..1978b55f1255402bf9bade0b91150b5cb49789a4 100644
GIT binary patch
delta 24
fcmZp%X|dsQ33dr#kz-(B44ud|iIHLB+;mw0OTGp&
delta 24
fcmZp%X|dsQ33dr#kz-(B44KF^iIH*R+;mw0OSuLz
diff --git a/tests/data/acpi/q35/DSDT.tis b/tests/data/acpi/q35/DSDT.tis
index 27ee927fc5ac05b89724c154197304c39e653269..638de3872673d17b1958497d0e62c83653de1602 100644
GIT binary patch
delta 24
gcmccZaNB{)CD<h-T7iLqv1KCHBu0jfbN9&u0AtGs$p8QV
delta 24
gcmccZaNB{)CD<h-T7iLqv3VlbBu2)KbN9&u0At1n$p8QV
diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
index e669508d175f1e3ddf355f8a9b0d419266cac8aa..9b002836f35fd03afeab9e827fdde3134d26ed2e 100644
GIT binary patch
delta 36
scmcbraY2L2CD<h-K!kyT>DNRqX{K(cjp`Ddj82msIE^-!bKc_u0L51dc>n+a
delta 42
ycmcbhaaDuMCD<h-RD^+n>ET2!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<iop(FzXlk`L`X{Mg(8`UK^8J#9Oa2jnc=hPPf0JT*KRR910
delta 42
ycmZ2zyv&%(CD<ionIr=P6VpU4X{N>*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
scmcbraY2L2CD<h-K!kyT>DNRqX{K(cjp`Ddj82msIE^-!bKc_u0L51dc>n+a
delta 42
ycmcbhaaDuMCD<h-RD^+n>ET2!X{H9}jp`DdjP8>iIE`3&1Drh#HWzW;;{pKve+zj4
--
MST
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus
2020-08-27 13:40 [PULL 00/13] virtio,pc,acpi: features, fixes Michael S. Tsirkin
` (4 preceding siblings ...)
2020-08-27 13:40 ` [PULL 05/13] acpi: update expected DSDT files with _UID changes Michael S. Tsirkin
@ 2020-08-27 13:40 ` Michael S. Tsirkin
2020-08-27 17:41 ` Igor Mammedov
2020-08-27 13:40 ` [PULL 07/13] virtio-pci: add virtio_pci_optimal_num_queues() helper Michael S. Tsirkin
` (7 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Michael S. Tsirkin @ 2020-08-27 13:40 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Aleksandar Markovic, Ani Sinha, Igor Mammedov,
Philippe Mathieu-Daudé, Aurelien Jarno
From: Ani Sinha <ani@anisinha.ca>
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 <ani@anisinha.ca>
Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
include/hw/acpi/pcihp.h | 2 +-
hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
hw/acpi/piix4.c | 5 ++++-
3 files changed, 27 insertions(+), 3 deletions(-)
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;
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(),
--
MST
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus
2020-08-27 13:40 ` [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus Michael S. Tsirkin
@ 2020-08-27 17:41 ` Igor Mammedov
2020-08-27 17:59 ` Ani Sinha
2020-08-30 9:56 ` Michael S. Tsirkin
0 siblings, 2 replies; 28+ messages in thread
From: Igor Mammedov @ 2020-08-27 17:41 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Peter Maydell, qemu-devel, Aleksandar Markovic, Ani Sinha,
Philippe Mathieu-Daudé, Aurelien Jarno
On Thu, 27 Aug 2020 09:40:34 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> From: Ani Sinha <ani@anisinha.ca>
>
> 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 <ani@anisinha.ca>
> Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Tested-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
A glitch in scripts?
I didn't review nor tested this (v8) version
> ---
> include/hw/acpi/pcihp.h | 2 +-
> hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
> hw/acpi/piix4.c | 5 ++++-
> 3 files changed, 27 insertions(+), 3 deletions(-)
>
> 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;
>
> 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(),
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus
2020-08-27 17:41 ` Igor Mammedov
@ 2020-08-27 17:59 ` Ani Sinha
2020-08-28 9:49 ` Igor Mammedov
2020-08-30 20:57 ` Michael S. Tsirkin
2020-08-30 9:56 ` Michael S. Tsirkin
1 sibling, 2 replies; 28+ messages in thread
From: Ani Sinha @ 2020-08-27 17:59 UTC (permalink / raw)
To: Igor Mammedov
Cc: Peter Maydell, Michael S. Tsirkin, qemu-devel,
Aleksandar Markovic, Philippe Mathieu-Daudé, Aurelien Jarno
On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Thu, 27 Aug 2020 09:40:34 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > From: Ani Sinha <ani@anisinha.ca>
> >
> > 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 <ani@anisinha.ca>
> > Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
>
> > Tested-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> A glitch in scripts?
> I didn't review nor tested this (v8) version
oops! I am so eager to get this done and dusted :)
>
> > ---
> > include/hw/acpi/pcihp.h | 2 +-
> > hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
> > hw/acpi/piix4.c | 5 ++++-
> > 3 files changed, 27 insertions(+), 3 deletions(-)
> >
> > 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;
> >
> > 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(),
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus
2020-08-27 17:59 ` Ani Sinha
@ 2020-08-28 9:49 ` Igor Mammedov
2020-08-28 9:51 ` Ani Sinha
2020-08-30 10:02 ` Ani Sinha
2020-08-30 20:57 ` Michael S. Tsirkin
1 sibling, 2 replies; 28+ messages in thread
From: Igor Mammedov @ 2020-08-28 9:49 UTC (permalink / raw)
To: Ani Sinha
Cc: Peter Maydell, Michael S. Tsirkin, qemu-devel,
Aleksandar Markovic, Philippe Mathieu-Daudé, Aurelien Jarno
On Thu, 27 Aug 2020 23:29:34 +0530
Ani Sinha <ani@anisinha.ca> wrote:
> On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Thu, 27 Aug 2020 09:40:34 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > From: Ani Sinha <ani@anisinha.ca>
> > >
> > > 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 <ani@anisinha.ca>
> > > Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >
> > > Tested-by: Igor Mammedov <imammedo@redhat.com>
> > > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > A glitch in scripts?
> > I didn't review nor tested this (v8) version
>
> oops! I am so eager to get this done and dusted :)
it's merged now,
can you add a test case for it please?
You can use test_acpi_piix4_tcg_bridge() as model.
See header comment at the top of bios-tables-test.c
for how to prepare and submit testcase.
>
> >
> > > ---
> > > include/hw/acpi/pcihp.h | 2 +-
> > > hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
> > > hw/acpi/piix4.c | 5 ++++-
> > > 3 files changed, 27 insertions(+), 3 deletions(-)
> > >
> > > 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;
> > >
> > > 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(),
> >
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus
2020-08-28 9:49 ` Igor Mammedov
@ 2020-08-28 9:51 ` Ani Sinha
2020-08-28 13:10 ` Julia Suvorova
2020-08-30 10:02 ` Ani Sinha
1 sibling, 1 reply; 28+ messages in thread
From: Ani Sinha @ 2020-08-28 9:51 UTC (permalink / raw)
To: Igor Mammedov
Cc: Peter Maydell, Michael S. Tsirkin, qemu-devel,
Aleksandar Markovic, Philippe Mathieu-Daudé, Aurelien Jarno
[-- Attachment #1: Type: text/plain, Size: 6185 bytes --]
Ani
On Aug 28, 2020, 15:19 +0530, Igor Mammedov <imammedo@redhat.com>, wrote:
> On Thu, 27 Aug 2020 23:29:34 +0530
> Ani Sinha <ani@anisinha.ca> wrote:
>
> > On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > >
> > > On Thu, 27 Aug 2020 09:40:34 -0400
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > From: Ani Sinha <ani@anisinha.ca>
> > > >
> > > > 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 <ani@anisinha.ca>
> > > > Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > >
> > > > Tested-by: Igor Mammedov <imammedo@redhat.com>
> > > > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > > A glitch in scripts?
> > > I didn't review nor tested this (v8) version
> >
> > oops! I am so eager to get this done and dusted :)
> it's merged now,
Wait it merged without your review?
>
> can you add a test case for it please?
>
> You can use test_acpi_piix4_tcg_bridge() as model.
> See header comment at the top of bios-tables-test.c
> for how to prepare and submit testcase.
Will get on it.
>
> >
> > >
> > > > ---
> > > > include/hw/acpi/pcihp.h | 2 +-
> > > > hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
> > > > hw/acpi/piix4.c | 5 ++++-
> > > > 3 files changed, 27 insertions(+), 3 deletions(-)
> > > >
> > > > 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;
> > > >
> > > > 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(),
> > >
> >
>
[-- Attachment #2: Type: text/html, Size: 101973 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus
2020-08-28 9:51 ` Ani Sinha
@ 2020-08-28 13:10 ` Julia Suvorova
2020-08-28 13:15 ` Ani Sinha
2020-09-01 6:27 ` Ani Sinha
0 siblings, 2 replies; 28+ messages in thread
From: Julia Suvorova @ 2020-08-28 13:10 UTC (permalink / raw)
To: Ani Sinha
Cc: Peter Maydell, Michael S. Tsirkin, QEMU Developers,
Aleksandar Markovic, Igor Mammedov, Philippe Mathieu-Daudé,
Aurelien Jarno
On Fri, Aug 28, 2020 at 11:53 AM Ani Sinha <ani@anisinha.ca> wrote:
>
>
> Ani
> On Aug 28, 2020, 15:19 +0530, Igor Mammedov <imammedo@redhat.com>, wrote:
>
> On Thu, 27 Aug 2020 23:29:34 +0530
>
> Ani Sinha <ani@anisinha.ca> wrote:
>
>
> On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
>
> On Thu, 27 Aug 2020 09:40:34 -0400
>
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>
> From: Ani Sinha <ani@anisinha.ca>
>
>
> 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 <ani@anisinha.ca>
>
> Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
>
>
> Tested-by: Igor Mammedov <imammedo@redhat.com>
>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>
> A glitch in scripts?
>
> I didn't review nor tested this (v8) version
>
>
> oops! I am so eager to get this done and dusted :)
>
> it's merged now,
>
>
> Wait it merged without your review?
Yeah, not only added into the pull request, but actually merged.
>
> can you add a test case for it please?
>
>
> You can use test_acpi_piix4_tcg_bridge() as model.
>
> See header comment at the top of bios-tables-test.c
>
> for how to prepare and submit testcase.
>
>
> Will get on it.
Also, while the whole approach seems good to me, it leaves the hotplug
registers initialized (see build_piix4_pci_hotplug()), even if both
root and bridges hotplug are disabled. Which you can exploit by
writing something like this to the io registers:
outl 0xae10 0
outl 0xae08 your_slot
And because of these quite interesting lines the device will be
successfully unplugged:
static PCIBus *acpi_pcihp_find_hotplug_bus(AcpiPciHpState *s, int bsel)
{
...
/* Make bsel 0 eject root bus if bsel property is not set,
* for compatibility with non acpi setups.
* TODO: really needed?
*/
if (!bsel && !find.bus) {
find.bus = s->root;
}
return find.bus;
}
Could you please cover both issues in the follow-up patches?
Best regards, Julia Suvorova.
>
>
>
> ---
>
> include/hw/acpi/pcihp.h | 2 +-
>
> hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
>
> hw/acpi/piix4.c | 5 ++++-
>
> 3 files changed, 27 insertions(+), 3 deletions(-)
>
>
> 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;
>
>
> 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(),
>
>
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus
2020-08-28 13:10 ` Julia Suvorova
@ 2020-08-28 13:15 ` Ani Sinha
2020-08-28 15:45 ` Julia Suvorova
2020-09-01 6:27 ` Ani Sinha
1 sibling, 1 reply; 28+ messages in thread
From: Ani Sinha @ 2020-08-28 13:15 UTC (permalink / raw)
To: Julia Suvorova
Cc: Peter Maydell, Michael S. Tsirkin, QEMU Developers,
Aleksandar Markovic, Igor Mammedov, Philippe Mathieu-Daudé,
Aurelien Jarno
[-- Attachment #1: Type: text/plain, Size: 7717 bytes --]
On Aug 28, 2020, 18:40 +0530, Julia Suvorova <jusual@redhat.com>, wrote:
> On Fri, Aug 28, 2020 at 11:53 AM Ani Sinha <ani@anisinha.ca> wrote:
> >
> >
> > Ani
> > On Aug 28, 2020, 15:19 +0530, Igor Mammedov <imammedo@redhat.com>, wrote:
> >
> > On Thu, 27 Aug 2020 23:29:34 +0530
> >
> > Ani Sinha <ani@anisinha.ca> wrote:
> >
> >
> > On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> >
> > On Thu, 27 Aug 2020 09:40:34 -0400
> >
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> >
> > From: Ani Sinha <ani@anisinha.ca>
> >
> >
> > 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 <ani@anisinha.ca>
> >
> > Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> >
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >
> >
> > Tested-by: Igor Mammedov <imammedo@redhat.com>
> >
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> >
> > A glitch in scripts?
> >
> > I didn't review nor tested this (v8) version
> >
> >
> > oops! I am so eager to get this done and dusted :)
> >
> > it's merged now,
> >
> >
> > Wait it merged without your review?
>
> Yeah, not only added into the pull request, but actually merged.
>
> >
> > can you add a test case for it please?
> >
> >
> > You can use test_acpi_piix4_tcg_bridge() as model.
> >
> > See header comment at the top of bios-tables-test.c
> >
> > for how to prepare and submit testcase.
> >
> >
> > Will get on it.
>
> Also, while the whole approach seems good to me, it leaves the hotplug
> registers initialized (see build_piix4_pci_hotplug()), even if both
> root and bridges hotplug are disabled. Which you can exploit by
> writing something like this to the io registers:
>
> outl 0xae10 0
You’re setting bsel 0 with this line correct?
> outl 0xae08 your_slot
>
> And because of these quite interesting lines the device will be
> successfully unplugged:
>
> static PCIBus *acpi_pcihp_find_hotplug_bus(AcpiPciHpState *s, int bsel)
> {
> ...
> /* Make bsel 0 eject root bus if bsel property is not set,
> * for compatibility with non acpi setups.
> * TODO: really needed?
> */
> if (!bsel && !find.bus) {
> find.bus = s->root;
> }
> return find.bus;
> }
>
> Could you please cover both issues in the follow-up patches?
Can you please explain what do you mean by both issues? The unit test issue and leaving the registers initialized?
>
> Best regards, Julia Suvorova.
>
> >
> >
> >
> > ---
> >
> > include/hw/acpi/pcihp.h | 2 +-
> >
> > hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
> >
> > hw/acpi/piix4.c | 5 ++++-
> >
> > 3 files changed, 27 insertions(+), 3 deletions(-)
> >
> >
> > 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;
> >
> >
> > 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(),
> >
> >
> >
> >
>
[-- Attachment #2: Type: text/html, Size: 117706 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus
2020-08-28 13:15 ` Ani Sinha
@ 2020-08-28 15:45 ` Julia Suvorova
0 siblings, 0 replies; 28+ messages in thread
From: Julia Suvorova @ 2020-08-28 15:45 UTC (permalink / raw)
To: Ani Sinha
Cc: Peter Maydell, Michael S. Tsirkin, QEMU Developers,
Aleksandar Markovic, Igor Mammedov, Philippe Mathieu-Daudé,
Aurelien Jarno
On Fri, Aug 28, 2020 at 3:16 PM Ani Sinha <ani@anisinha.ca> wrote:
>
> On Aug 28, 2020, 18:40 +0530, Julia Suvorova <jusual@redhat.com>, wrote:
>
> On Fri, Aug 28, 2020 at 11:53 AM Ani Sinha <ani@anisinha.ca> wrote:
>
>
>
> Ani
>
> On Aug 28, 2020, 15:19 +0530, Igor Mammedov <imammedo@redhat.com>, wrote:
>
>
> On Thu, 27 Aug 2020 23:29:34 +0530
>
>
> Ani Sinha <ani@anisinha.ca> wrote:
>
>
>
> On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
>
>
> On Thu, 27 Aug 2020 09:40:34 -0400
>
>
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>
>
> From: Ani Sinha <ani@anisinha.ca>
>
>
>
> 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 <ani@anisinha.ca>
>
>
> Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
>
>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
>
>
>
> Tested-by: Igor Mammedov <imammedo@redhat.com>
>
>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>
>
> A glitch in scripts?
>
>
> I didn't review nor tested this (v8) version
>
>
>
> oops! I am so eager to get this done and dusted :)
>
>
> it's merged now,
>
>
>
> Wait it merged without your review?
>
>
> Yeah, not only added into the pull request, but actually merged.
>
>
>
> can you add a test case for it please?
>
>
>
> You can use test_acpi_piix4_tcg_bridge() as model.
>
>
> See header comment at the top of bios-tables-test.c
>
>
> for how to prepare and submit testcase.
>
>
>
> Will get on it.
>
>
> Also, while the whole approach seems good to me, it leaves the hotplug
>
> registers initialized (see build_piix4_pci_hotplug()), even if both
>
> root and bridges hotplug are disabled. Which you can exploit by
>
> writing something like this to the io registers:
>
>
> outl 0xae10 0
>
>
> You’re setting bsel 0 with this line correct?
Yes, it selects bsel.
> outl 0xae08 your_slot
>
>
> And because of these quite interesting lines the device will be
>
> successfully unplugged:
>
>
> static PCIBus *acpi_pcihp_find_hotplug_bus(AcpiPciHpState *s, int bsel)
>
> {
>
> ...
>
> /* Make bsel 0 eject root bus if bsel property is not set,
>
> * for compatibility with non acpi setups.
>
> * TODO: really needed?
>
> */
>
> if (!bsel && !find.bus) {
>
> find.bus = s->root;
>
> }
>
> return find.bus;
>
> }
>
>
> Could you please cover both issues in the follow-up patches?
>
>
> Can you please explain what do you mean by both issues? The unit test issue and leaving the registers initialized?
No, I mean initializing registers and returning root as default in
acpi_pcihp_find_hotplug_bus() in addition to Igor's notes.
Returning NULL if hotplug is disabled for root should be fine, unless
Michael confirms that we can remove this check completely.
> Best regards, Julia Suvorova.
>
>
>
>
>
> ---
>
>
> include/hw/acpi/pcihp.h | 2 +-
>
>
> hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
>
>
> hw/acpi/piix4.c | 5 ++++-
>
>
> 3 files changed, 27 insertions(+), 3 deletions(-)
>
>
>
> 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;
>
>
>
> 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(),
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus
2020-08-28 13:10 ` Julia Suvorova
2020-08-28 13:15 ` Ani Sinha
@ 2020-09-01 6:27 ` Ani Sinha
2020-09-01 12:04 ` Ani Sinha
1 sibling, 1 reply; 28+ messages in thread
From: Ani Sinha @ 2020-09-01 6:27 UTC (permalink / raw)
To: Julia Suvorova
Cc: Peter Maydell, Michael S. Tsirkin, QEMU Developers,
Aleksandar Markovic, Igor Mammedov, Philippe Mathieu-Daudé,
Aurelien Jarno
On Fri, Aug 28, 2020 at 6:40 PM Julia Suvorova <jusual@redhat.com> wrote:
>
> On Fri, Aug 28, 2020 at 11:53 AM Ani Sinha <ani@anisinha.ca> wrote:
> >
> >
> > Ani
> > On Aug 28, 2020, 15:19 +0530, Igor Mammedov <imammedo@redhat.com>, wrote:
> >
> > On Thu, 27 Aug 2020 23:29:34 +0530
> >
> > Ani Sinha <ani@anisinha.ca> wrote:
> >
> >
> > On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> >
> > On Thu, 27 Aug 2020 09:40:34 -0400
> >
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> >
> > From: Ani Sinha <ani@anisinha.ca>
> >
> >
> > 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 <ani@anisinha.ca>
> >
> > Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> >
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >
> >
> > Tested-by: Igor Mammedov <imammedo@redhat.com>
> >
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> >
> > A glitch in scripts?
> >
> > I didn't review nor tested this (v8) version
> >
> >
> > oops! I am so eager to get this done and dusted :)
> >
> > it's merged now,
> >
> >
> > Wait it merged without your review?
>
> Yeah, not only added into the pull request, but actually merged.
>
> >
> > can you add a test case for it please?
> >
> >
> > You can use test_acpi_piix4_tcg_bridge() as model.
> >
> > See header comment at the top of bios-tables-test.c
> >
> > for how to prepare and submit testcase.
> >
> >
> > Will get on it.
>
> Also, while the whole approach seems good to me, it leaves the hotplug
> registers initialized (see build_piix4_pci_hotplug()), even if both
> root and bridges hotplug are disabled. Which you can exploit by
> writing something like this to the io registers:
>
> outl 0xae10 0
> outl 0xae08 your_slot
>
> And because of these quite interesting lines the device will be
> successfully unplugged:
>
> static PCIBus *acpi_pcihp_find_hotplug_bus(AcpiPciHpState *s, int bsel)
> {
> ...
> /* Make bsel 0 eject root bus if bsel property is not set,
> * for compatibility with non acpi setups.
> * TODO: really needed?
> */
> if (!bsel && !find.bus) {
> find.bus = s->root;
> }
> return find.bus;
> }
>
I have sent a patch to address this. Please review.
> Could you please cover both issues in the follow-up patches?
Also julia, looking at the code, should we also set pm->pcihp_io_len
to 0 as well in case where bridge and pci bus 0 hotplugs are both
disabled? This is because I see:
/* reserve PCIHP resources */
if (pm->pcihp_io_len) {
which we do not want to do if PCI hotplug is disabled altogether?
>
> Best regards, Julia Suvorova.
>
> >
> >
> >
> > ---
> >
> > include/hw/acpi/pcihp.h | 2 +-
> >
> > hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
> >
> > hw/acpi/piix4.c | 5 ++++-
> >
> > 3 files changed, 27 insertions(+), 3 deletions(-)
> >
> >
> > 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;
> >
> >
> > 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(),
> >
> >
> >
> >
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus
2020-09-01 6:27 ` Ani Sinha
@ 2020-09-01 12:04 ` Ani Sinha
0 siblings, 0 replies; 28+ messages in thread
From: Ani Sinha @ 2020-09-01 12:04 UTC (permalink / raw)
To: Julia Suvorova
Cc: Peter Maydell, Michael S. Tsirkin, QEMU Developers,
Aleksandar Markovic, Igor Mammedov, Philippe Mathieu-Daudé,
Aurelien Jarno
On Tue, Sep 1, 2020 at 11:57 AM Ani Sinha <ani@anisinha.ca> wrote:
>
> On Fri, Aug 28, 2020 at 6:40 PM Julia Suvorova <jusual@redhat.com> wrote:
> >
> > On Fri, Aug 28, 2020 at 11:53 AM Ani Sinha <ani@anisinha.ca> wrote:
> > >
> > >
> > > Ani
> > > On Aug 28, 2020, 15:19 +0530, Igor Mammedov <imammedo@redhat.com>, wrote:
> > >
> > > On Thu, 27 Aug 2020 23:29:34 +0530
> > >
> > > Ani Sinha <ani@anisinha.ca> wrote:
> > >
> > >
> > > On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > >
> > >
> > > On Thu, 27 Aug 2020 09:40:34 -0400
> > >
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > >
> > > From: Ani Sinha <ani@anisinha.ca>
> > >
> > >
> > > 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 <ani@anisinha.ca>
> > >
> > > Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> > >
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > >
> > >
> > > Tested-by: Igor Mammedov <imammedo@redhat.com>
> > >
> > > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > >
> > > A glitch in scripts?
> > >
> > > I didn't review nor tested this (v8) version
> > >
> > >
> > > oops! I am so eager to get this done and dusted :)
> > >
> > > it's merged now,
> > >
> > >
> > > Wait it merged without your review?
> >
> > Yeah, not only added into the pull request, but actually merged.
> >
> > >
> > > can you add a test case for it please?
> > >
> > >
> > > You can use test_acpi_piix4_tcg_bridge() as model.
> > >
> > > See header comment at the top of bios-tables-test.c
> > >
> > > for how to prepare and submit testcase.
> > >
> > >
> > > Will get on it.
> >
> > Also, while the whole approach seems good to me, it leaves the hotplug
> > registers initialized (see build_piix4_pci_hotplug()), even if both
> > root and bridges hotplug are disabled. Which you can exploit by
> > writing something like this to the io registers:
> >
> > outl 0xae10 0
> > outl 0xae08 your_slot
> >
> > And because of these quite interesting lines the device will be
> > successfully unplugged:
> >
> > static PCIBus *acpi_pcihp_find_hotplug_bus(AcpiPciHpState *s, int bsel)
> > {
> > ...
> > /* Make bsel 0 eject root bus if bsel property is not set,
> > * for compatibility with non acpi setups.
> > * TODO: really needed?
> > */
> > if (!bsel && !find.bus) {
> > find.bus = s->root;
> > }
> > return find.bus;
> > }
> >
>
> I have sent a patch to address this. Please review.
>
> > Could you please cover both issues in the follow-up patches?
>
> Also julia, looking at the code, should we also set pm->pcihp_io_len
> to 0 as well in case where bridge and pci bus 0 hotplugs are both
> disabled?
I have sent a RFC patch where I check if at least one of the two
options is ON in the conditional below. Seems the right thing to do
instead of setting the io_len to 0.
This is because I see:
>
> /* reserve PCIHP resources */
> if (pm->pcihp_io_len) {
>
> which we do not want to do if PCI hotplug is disabled altogether?
>
> >
> > Best regards, Julia Suvorova.
> >
> > >
> > >
> > >
> > > ---
> > >
> > > include/hw/acpi/pcihp.h | 2 +-
> > >
> > > hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
> > >
> > > hw/acpi/piix4.c | 5 ++++-
> > >
> > > 3 files changed, 27 insertions(+), 3 deletions(-)
> > >
> > >
> > > 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;
> > >
> > >
> > > 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(),
> > >
> > >
> > >
> > >
> >
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus
2020-08-28 9:49 ` Igor Mammedov
2020-08-28 9:51 ` Ani Sinha
@ 2020-08-30 10:02 ` Ani Sinha
1 sibling, 0 replies; 28+ messages in thread
From: Ani Sinha @ 2020-08-30 10:02 UTC (permalink / raw)
To: Igor Mammedov
Cc: Peter Maydell, Michael S. Tsirkin, QEMU Developers,
Aleksandar Markovic, Philippe Mathieu-Daudé, Aurelien Jarno
On Fri, Aug 28, 2020 at 3:19 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Thu, 27 Aug 2020 23:29:34 +0530
> Ani Sinha <ani@anisinha.ca> wrote:
>
> > On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > >
> > > On Thu, 27 Aug 2020 09:40:34 -0400
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > From: Ani Sinha <ani@anisinha.ca>
> > > >
> > > > 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 <ani@anisinha.ca>
> > > > Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > >
> > > > Tested-by: Igor Mammedov <imammedo@redhat.com>
> > > > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > > A glitch in scripts?
> > > I didn't review nor tested this (v8) version
> >
> > oops! I am so eager to get this done and dusted :)
> it's merged now,
>
> can you add a test case for it please?
Yes just sent the unit test patch set.
>
> You can use test_acpi_piix4_tcg_bridge() as model.
> See header comment at the top of bios-tables-test.c
> for how to prepare and submit testcase.
>
> >
> > >
> > > > ---
> > > > include/hw/acpi/pcihp.h | 2 +-
> > > > hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
> > > > hw/acpi/piix4.c | 5 ++++-
> > > > 3 files changed, 27 insertions(+), 3 deletions(-)
> > > >
> > > > 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;
> > > >
> > > > 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(),
> > >
> >
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus
2020-08-27 17:59 ` Ani Sinha
2020-08-28 9:49 ` Igor Mammedov
@ 2020-08-30 20:57 ` Michael S. Tsirkin
2020-08-31 12:15 ` Ani Sinha
1 sibling, 1 reply; 28+ messages in thread
From: Michael S. Tsirkin @ 2020-08-30 20:57 UTC (permalink / raw)
To: Ani Sinha
Cc: Peter Maydell, qemu-devel, Aleksandar Markovic, Igor Mammedov,
Philippe Mathieu-Daudé, Aurelien Jarno
On Thu, Aug 27, 2020 at 11:29:34PM +0530, Ani Sinha wrote:
> On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Thu, 27 Aug 2020 09:40:34 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > From: Ani Sinha <ani@anisinha.ca>
> > >
> > > 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 <ani@anisinha.ca>
> > > Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >
> > > Tested-by: Igor Mammedov <imammedo@redhat.com>
> > > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > A glitch in scripts?
> > I didn't review nor tested this (v8) version
>
> oops! I am so eager to get this done and dusted :)
One thing I would like to see is that all of these flags
should also disable the relevant functionality.
> >
> > > ---
> > > include/hw/acpi/pcihp.h | 2 +-
> > > hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
> > > hw/acpi/piix4.c | 5 ++++-
> > > 3 files changed, 27 insertions(+), 3 deletions(-)
> > >
> > > 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;
> > >
> > > 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(),
> >
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus
2020-08-30 20:57 ` Michael S. Tsirkin
@ 2020-08-31 12:15 ` Ani Sinha
0 siblings, 0 replies; 28+ messages in thread
From: Ani Sinha @ 2020-08-31 12:15 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Peter Maydell, qemu-devel, Aleksandar Markovic, Igor Mammedov,
Philippe Mathieu-Daudé, Aurelien Jarno
> On Aug 31, 2020, at 2:27 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Aug 27, 2020 at 11:29:34PM +0530, Ani Sinha wrote:
>>> On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
>>>
>>> On Thu, 27 Aug 2020 09:40:34 -0400
>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>
>>>> From: Ani Sinha <ani@anisinha.ca>
>>>>
>>>> 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 <ani@anisinha.ca>
>>>> Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>
>>>
>>>> Tested-by: Igor Mammedov <imammedo@redhat.com>
>>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>> A glitch in scripts?
>>> I didn't review nor tested this (v8) version
>>
>> oops! I am so eager to get this done and dusted :)
>
>
> One thing I would like to see is that all of these flags
> should also disable the relevant functionality.
The flag that I introduced would with this patch along with the other patch I sent to address Julia's concern over ejecting root bus when bsel is absent.
>
>
>
>>>
>>>> ---
>>>> include/hw/acpi/pcihp.h | 2 +-
>>>> hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
>>>> hw/acpi/piix4.c | 5 ++++-
>>>> 3 files changed, 27 insertions(+), 3 deletions(-)
>>>>
>>>> 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;
>>>>
>>>> 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(),
>>>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus
2020-08-27 17:41 ` Igor Mammedov
2020-08-27 17:59 ` Ani Sinha
@ 2020-08-30 9:56 ` Michael S. Tsirkin
1 sibling, 0 replies; 28+ messages in thread
From: Michael S. Tsirkin @ 2020-08-30 9:56 UTC (permalink / raw)
To: Igor Mammedov
Cc: Peter Maydell, qemu-devel, Aleksandar Markovic, Ani Sinha,
Philippe Mathieu-Daudé, Aurelien Jarno
On Thu, Aug 27, 2020 at 07:41:15PM +0200, Igor Mammedov wrote:
> On Thu, 27 Aug 2020 09:40:34 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > From: Ani Sinha <ani@anisinha.ca>
> >
> > 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 <ani@anisinha.ca>
> > Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
>
> > Tested-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> A glitch in scripts?
> I didn't review nor tested this (v8) version
I guess I picked the tags from the previous version.
Are you ok with this one?
> > ---
> > include/hw/acpi/pcihp.h | 2 +-
> > hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
> > hw/acpi/piix4.c | 5 ++++-
> > 3 files changed, 27 insertions(+), 3 deletions(-)
> >
> > 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;
> >
> > 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(),
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PULL 07/13] virtio-pci: add virtio_pci_optimal_num_queues() helper
2020-08-27 13:40 [PULL 00/13] virtio,pc,acpi: features, fixes Michael S. Tsirkin
` (5 preceding siblings ...)
2020-08-27 13:40 ` [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus Michael S. Tsirkin
@ 2020-08-27 13:40 ` Michael S. Tsirkin
2020-08-27 13:40 ` [PULL 08/13] virtio-scsi: introduce a constant for fixed virtqueues Michael S. Tsirkin
` (6 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Michael S. Tsirkin @ 2020-08-27 13:40 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Cornelia Huck, Stefan Hajnoczi
From: Stefan Hajnoczi <stefanha@redhat.com>
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 <stefanha@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Message-Id: <20200818143348.310613-4-stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/virtio/virtio-pci.h | 9 +++++++++
hw/virtio/virtio-pci.c | 32 ++++++++++++++++++++++++++++++++
2 files changed, 41 insertions(+)
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
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,
--
MST
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PULL 08/13] virtio-scsi: introduce a constant for fixed virtqueues
2020-08-27 13:40 [PULL 00/13] virtio,pc,acpi: features, fixes Michael S. Tsirkin
` (6 preceding siblings ...)
2020-08-27 13:40 ` [PULL 07/13] virtio-pci: add virtio_pci_optimal_num_queues() helper Michael S. Tsirkin
@ 2020-08-27 13:40 ` Michael S. Tsirkin
2020-08-27 13:40 ` [PULL 09/13] virtio-scsi-pci: default num_queues to -smp N Michael S. Tsirkin
` (5 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Michael S. Tsirkin @ 2020-08-27 13:40 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Pankaj Gupta, Cornelia Huck,
Raphael Norwitz, Stefan Hajnoczi, Paolo Bonzini,
Philippe Mathieu-Daudé
From: Stefan Hajnoczi <stefanha@redhat.com>
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 <stefanha@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
Message-Id: <20200818143348.310613-5-stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/virtio/virtio-scsi.h | 3 +++
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 ++-
6 files changed, 14 insertions(+), 7 deletions(-)
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;
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;
}
/*
--
MST
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PULL 09/13] virtio-scsi-pci: default num_queues to -smp N
2020-08-27 13:40 [PULL 00/13] virtio,pc,acpi: features, fixes Michael S. Tsirkin
` (7 preceding siblings ...)
2020-08-27 13:40 ` [PULL 08/13] virtio-scsi: introduce a constant for fixed virtqueues Michael S. Tsirkin
@ 2020-08-27 13:40 ` Michael S. Tsirkin
2020-08-27 13:40 ` [PULL 10/13] virtio-blk-pci: " Michael S. Tsirkin
` (4 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Michael S. Tsirkin @ 2020-08-27 13:40 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Eduardo Habkost, Cornelia Huck,
Stefan Hajnoczi, Raphael Norwitz, Paolo Bonzini
From: Stefan Hajnoczi <stefanha@redhat.com>
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 <stefanha@redhat.com>
Message-Id: <20200818143348.310613-6-stefanha@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/virtio/virtio-scsi.h | 2 ++
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 +++++++---
8 files changed, 37 insertions(+), 13 deletions(-)
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;
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;
}
/*
--
MST
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PULL 10/13] virtio-blk-pci: default num_queues to -smp N
2020-08-27 13:40 [PULL 00/13] virtio,pc,acpi: features, fixes Michael S. Tsirkin
` (8 preceding siblings ...)
2020-08-27 13:40 ` [PULL 09/13] virtio-scsi-pci: default num_queues to -smp N Michael S. Tsirkin
@ 2020-08-27 13:40 ` Michael S. Tsirkin
2020-08-27 13:40 ` [PULL 11/13] vhost-user-blk-pci: " Michael S. Tsirkin
` (3 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Michael S. Tsirkin @ 2020-08-27 13:40 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Peter Maydell, Eduardo Habkost, qemu-block,
Pankaj Gupta, Cornelia Huck, Max Reitz, Stefan Hajnoczi
From: Stefan Hajnoczi <stefanha@redhat.com>
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 <stefanha@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Message-Id: <20200818143348.310613-7-stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/virtio/virtio-blk.h | 2 ++
hw/block/virtio-blk.c | 6 +++++-
hw/core/machine.c | 1 +
hw/virtio/virtio-blk-pci.c | 7 ++++++-
4 files changed, 14 insertions(+), 2 deletions(-)
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;
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);
--
MST
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PULL 11/13] vhost-user-blk-pci: default num_queues to -smp N
2020-08-27 13:40 [PULL 00/13] virtio,pc,acpi: features, fixes Michael S. Tsirkin
` (9 preceding siblings ...)
2020-08-27 13:40 ` [PULL 10/13] virtio-blk-pci: " Michael S. Tsirkin
@ 2020-08-27 13:40 ` Michael S. Tsirkin
2020-08-27 13:40 ` [PULL 12/13] hw/smbios: add options for type 4 max-speed and current-speed Michael S. Tsirkin
` (2 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Michael S. Tsirkin @ 2020-08-27 13:40 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Peter Maydell, Eduardo Habkost, qemu-block,
Cornelia Huck, Max Reitz, Stefan Hajnoczi, Raphael Norwitz
From: Stefan Hajnoczi <stefanha@redhat.com>
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 <stefanha@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
Message-Id: <20200818143348.310613-8-stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/virtio/vhost-user-blk.h | 2 ++
hw/block/vhost-user-blk.c | 6 +++++-
hw/core/machine.c | 1 +
hw/virtio/vhost-user-blk-pci.c | 4 ++++
4 files changed, 12 insertions(+), 1 deletion(-)
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;
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;
}
--
MST
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PULL 12/13] hw/smbios: add options for type 4 max-speed and current-speed
2020-08-27 13:40 [PULL 00/13] virtio,pc,acpi: features, fixes Michael S. Tsirkin
` (10 preceding siblings ...)
2020-08-27 13:40 ` [PULL 11/13] vhost-user-blk-pci: " Michael S. Tsirkin
@ 2020-08-27 13:40 ` Michael S. Tsirkin
2020-08-27 13:41 ` [PULL 13/13] tests/bios-tables-test: add smbios cpu speed test Michael S. Tsirkin
2020-08-27 22:09 ` [PULL 00/13] virtio,pc,acpi: features, fixes Peter Maydell
13 siblings, 0 replies; 28+ messages in thread
From: Michael S. Tsirkin @ 2020-08-27 13:40 UTC (permalink / raw)
To: qemu-devel; +Cc: Ying Fang, Peter Maydell, Heyi Guo, Igor Mammedov
From: Ying Fang <fangying1@huawei.com>
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 <imammedo@redhat.com>
Signed-off-by: Ying Fang <fangying1@huawei.com>
Signed-off-by: Heyi Guo <guoheyi@huawei.com>
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"
--
MST
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PULL 13/13] tests/bios-tables-test: add smbios cpu speed test
2020-08-27 13:40 [PULL 00/13] virtio,pc,acpi: features, fixes Michael S. Tsirkin
` (11 preceding siblings ...)
2020-08-27 13:40 ` [PULL 12/13] hw/smbios: add options for type 4 max-speed and current-speed Michael S. Tsirkin
@ 2020-08-27 13:41 ` Michael S. Tsirkin
2020-08-27 22:09 ` [PULL 00/13] virtio,pc,acpi: features, fixes Peter Maydell
13 siblings, 0 replies; 28+ messages in thread
From: Michael S. Tsirkin @ 2020-08-27 13:41 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Peter Maydell, Thomas Huth, Heyi Guo,
Paolo Bonzini, Ying Fang, Igor Mammedov
From: Ying Fang <fangying1@huawei.com>
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 <fangying1@huawei.com>
Signed-off-by: Heyi Guo <guoheyi@huawei.com>
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[])
--
MST
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PULL 00/13] virtio,pc,acpi: features, fixes
2020-08-27 13:40 [PULL 00/13] virtio,pc,acpi: features, fixes Michael S. Tsirkin
` (12 preceding siblings ...)
2020-08-27 13:41 ` [PULL 13/13] tests/bios-tables-test: add smbios cpu speed test Michael S. Tsirkin
@ 2020-08-27 22:09 ` Peter Maydell
13 siblings, 0 replies; 28+ messages in thread
From: Peter Maydell @ 2020-08-27 22:09 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: QEMU Developers
On Thu, 27 Aug 2020 at 14:40, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> The following changes since commit 8e49197ca5e76fdb8928833b2649ef13fc5aab2f:
>
> Merge remote-tracking branch 'remotes/hdeller/tags/target-hppa-v3-pull-request' into staging (2020-08-26 22:23:53 +0100)
>
> are available in the Git repository at:
>
> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to e1647539b1d04f121b70f1f6f438976477450f10:
>
> tests/bios-tables-test: add smbios cpu speed test (2020-08-27 08:29:13 -0400)
>
> ----------------------------------------------------------------
> virtio,pc,acpi: features, fixes
>
> better number of queues for vhost
> smbios speed options
> acpi fixes
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Applied, thanks.
Please update the changelog at https://wiki.qemu.org/ChangeLog/5.2
for any user-visible changes.
-- PMM
^ permalink raw reply [flat|nested] 28+ messages in thread