[PULL 00/13] virtio,pc,acpi: features, fixes

[PULL 00/13] virtio,pc,acpi: features, fixes

[Michael S. Tsirkin]

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>

----------------------------------------------------------------
Ani Sinha (1):
      Introduce a new flag for i440fx to disable PCI hotplug on the root bus

Michael S. Tsirkin (5):
      acpi: allow DSDT changes
      i386/acpi: fix inconsistent QEMU/OVMF device paths
      arm/acpi: fix an out of spec _UID for PCI root
      disassemble-aml: -o actually works
      acpi: update expected DSDT files with _UID changes

Stefan Hajnoczi (5):
      virtio-pci: add virtio_pci_optimal_num_queues() helper
      virtio-scsi: introduce a constant for fixed virtqueues
      virtio-scsi-pci: default num_queues to -smp N
      virtio-blk-pci: default num_queues to -smp N
      vhost-user-blk-pci: default num_queues to -smp N

Ying Fang (2):
      hw/smbios: add options for type 4 max-speed and current-speed
      tests/bios-tables-test: add smbios cpu speed test

 hw/virtio/virtio-pci.h             |   9 ++++++++
 include/hw/acpi/pcihp.h            |   2 +-
 include/hw/virtio/vhost-user-blk.h |   2 ++
 include/hw/virtio/virtio-blk.h     |   2 ++
 include/hw/virtio/virtio-scsi.h    |   5 +++++
 hw/acpi/pcihp.c                    |  23 +++++++++++++++++++-
 hw/acpi/piix4.c                    |   5 ++++-
 hw/arm/virt-acpi-build.c           |   2 +-
 hw/block/vhost-user-blk.c          |   6 +++++-
 hw/block/virtio-blk.c              |   6 +++++-
 hw/core/machine.c                  |   8 ++++++-
 hw/i386/acpi-build.c               |   4 ++--
 hw/scsi/vhost-scsi.c               |   3 ++-
 hw/scsi/vhost-user-scsi.c          |   5 +++--
 hw/scsi/virtio-scsi.c              |  13 ++++++++----
 hw/smbios/smbios.c                 |  36 +++++++++++++++++++++++++++----
 hw/virtio/vhost-scsi-pci.c         |   9 ++++++--
 hw/virtio/vhost-user-blk-pci.c     |   4 ++++
 hw/virtio/vhost-user-scsi-pci.c    |   9 ++++++--
 hw/virtio/virtio-blk-pci.c         |   7 ++++++-
 hw/virtio/virtio-pci.c             |  32 ++++++++++++++++++++++++++++
 hw/virtio/virtio-scsi-pci.c        |   9 ++++++--
 tests/qtest/bios-tables-test.c     |  42 +++++++++++++++++++++++++++++++++++++
 qemu-options.hx                    |   2 +-
 tests/data/acpi/disassemle-aml.sh  |  11 +++++++---
 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
 46 files changed, 225 insertions(+), 31 deletions(-)

[PULL 01/13] acpi: allow DSDT changes

[Michael S. Tsirkin]

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

[PULL 02/13] i386/acpi: fix inconsistent QEMU/OVMF device paths

[Michael S. Tsirkin]

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

[PULL 03/13] arm/acpi: fix an out of spec _UID for PCI root

[Michael S. Tsirkin]

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

[PULL 04/13] disassemble-aml: -o actually works

[Michael S. Tsirkin]

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

[PULL 05/13] acpi: update expected DSDT files with _UID changes

[Michael S. Tsirkin]

_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

[PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

[Michael S. Tsirkin]

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

[PULL 07/13] virtio-pci: add virtio_pci_optimal_num_queues() helper

[Michael S. Tsirkin]

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

[PULL 08/13] virtio-scsi: introduce a constant for fixed virtqueues

[Michael S. Tsirkin]

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

[PULL 09/13] virtio-scsi-pci: default num_queues to -smp N

[Michael S. Tsirkin]

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

[PULL 10/13] virtio-blk-pci: default num_queues to -smp N

[Michael S. Tsirkin]

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

[PULL 11/13] vhost-user-blk-pci: default num_queues to -smp N

[Michael S. Tsirkin]

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

[PULL 12/13] hw/smbios: add options for type 4 max-speed and current-speed

[Michael S. Tsirkin]

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

[PULL 13/13] tests/bios-tables-test: add smbios cpu speed test

[Michael S. Tsirkin]

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

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

[Igor Mammedov]

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(),

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

[Ani Sinha]

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(),
>

Re: [PULL 00/13] virtio,pc,acpi: features, fixes

[Peter Maydell]

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

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

[Igor Mammedov]

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(),  
> >  
> 

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

[Ani Sinha]

[-- 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 --]

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

[Julia Suvorova]

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(),
>
>
>
>

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

[Ani Sinha]

[-- 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 --]

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

[Julia Suvorova]

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(),
>
>
>
>
>
>

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

[Michael S. Tsirkin]

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(),

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

[Ani Sinha]

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(),
> > >
> >
>

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

[Michael S. Tsirkin]

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(),
> >

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

[Ani Sinha]

> 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(),
>>> 
> 

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

[Ani Sinha]

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(),
> >
> >
> >
> >
>

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

[Ani Sinha]

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(),
> > >
> > >
> > >
> > >
> >