[PATCH 0/6] x86: fix cpu hotplug with secure boot

[PATCH 0/6] x86: fix cpu hotplug with secure boot

[Igor Mammedov]

v1:
  - fix typos and some phrases (Laszlo)
  - add unplug check (Laszlo)
  - redo AML scan logic to avoid race when adding multiple CPUs

CPU hotplug with Secure Boot was not really supported and firmware wasn't aware
of hotplugged CPUs (which might lead to guest crashes). During 4.2 we introduced
locked SMI handler RAM arrea to make sure that guest OS wasn't able to inject
its own SMI handler and OVMF added initial CPU hotplug support.

This series is QEMU part of that support which lets QMVF tell QEMU that
CPU hotplug with SMI broadcast enabled is supported so that QEMU would be able
to prevent hotplug in case it's not supported and trigger SMI on hotplug when
it's necessary. 


Igor Mammedov (6):
  x86: lpc9: let firmware negotiate 'CPU hotplug with SMI' features
  x86: cphp: prevent guest crash on CPU hotplug when broadcast SMI is in
    use
  x86: cpuhp: refuse cpu hot-unplug request earlier if not supported
  tests: acpi: mark to be changed tables in
    bios-tables-test-allowed-diff
  x68: acpi: trigger SMI before scanning for hotplugged CPUs
  tests: acpi: update acpi blobs with new AML

 include/hw/acpi/cpu.h             |   1 +
 include/hw/i386/ich9.h            |   4 +++
 hw/acpi/cpu.c                     |  50 ++++++++++++++++++++++++++++--
 hw/acpi/ich9.c                    |  23 +++++++++++++-
 hw/i386/acpi-build.c              |  35 ++++++++++++++++++++-
 hw/i386/pc.c                      |  16 +++++++++-
 hw/isa/lpc_ich9.c                 |  15 +++++++++
 tests/data/acpi/pc/DSDT           | Bin 4934 -> 4973 bytes
 tests/data/acpi/pc/DSDT.acpihmat  | Bin 6258 -> 6297 bytes
 tests/data/acpi/pc/DSDT.bridge    | Bin 6793 -> 6832 bytes
 tests/data/acpi/pc/DSDT.cphp      | Bin 5397 -> 5436 bytes
 tests/data/acpi/pc/DSDT.dimmpxm   | Bin 6587 -> 6626 bytes
 tests/data/acpi/pc/DSDT.ipmikcs   | Bin 5006 -> 5045 bytes
 tests/data/acpi/pc/DSDT.memhp     | Bin 6293 -> 6332 bytes
 tests/data/acpi/pc/DSDT.numamem   | Bin 4940 -> 4979 bytes
 tests/data/acpi/q35/DSDT          | Bin 7678 -> 7717 bytes
 tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9041 bytes
 tests/data/acpi/q35/DSDT.bridge   | Bin 7695 -> 7734 bytes
 tests/data/acpi/q35/DSDT.cphp     | Bin 8141 -> 8180 bytes
 tests/data/acpi/q35/DSDT.dimmpxm  | Bin 9331 -> 9370 bytes
 tests/data/acpi/q35/DSDT.ipmibt   | Bin 7753 -> 7792 bytes
 tests/data/acpi/q35/DSDT.memhp    | Bin 9037 -> 9076 bytes
 tests/data/acpi/q35/DSDT.mmio64   | Bin 8808 -> 8847 bytes
 tests/data/acpi/q35/DSDT.numamem  | Bin 7684 -> 7723 bytes
 tests/data/acpi/q35/DSDT.tis      | Bin 8283 -> 8322 bytes
 25 files changed, 139 insertions(+), 5 deletions(-)

-- 
2.26.2

[PATCH 1/6] x86: lpc9: let firmware negotiate 'CPU hotplug with SMI' features

[Igor Mammedov]

It will allow firmware to notify QEMU that firmware requires SMI
being triggered on CPU hot[un]plug, so that it would be able to account
for hotplugged CPU and relocate it to new SMM base and/or safely remove
CPU on unplug.

Using negotiated features, follow up patches will insert SMI upcall
into AML code, to make sure that firmware processes hotplug before
guest OS would attempt to use new CPU.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/i386/ich9.h |  2 ++
 hw/i386/pc.c           |  5 ++++-
 hw/isa/lpc_ich9.c      | 12 ++++++++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
index a98d10b252..d1bb3f7bf0 100644
--- a/include/hw/i386/ich9.h
+++ b/include/hw/i386/ich9.h
@@ -247,5 +247,7 @@ typedef struct ICH9LPCState {
 
 /* bit positions used in fw_cfg SMI feature negotiation */
 #define ICH9_LPC_SMI_F_BROADCAST_BIT            0
+#define ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT          1
+#define ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT       2
 
 #endif /* HW_ICH9_H */
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3d419d5991..57d50fad6b 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -97,7 +97,10 @@
 #include "fw_cfg.h"
 #include "trace.h"
 
-GlobalProperty pc_compat_5_0[] = {};
+GlobalProperty pc_compat_5_0[] = {
+    { "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
+    { "ICH9-LPC", "x-smi-cpu-hotunplug", "off" },
+};
 const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
 
 GlobalProperty pc_compat_4_2[] = {
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index cd6e169d47..c9305080b5 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -373,6 +373,14 @@ static void smi_features_ok_callback(void *opaque)
         /* guest requests invalid features, leave @features_ok at zero */
         return;
     }
+    if (!(guest_features & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT)) &&
+        guest_features & (BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT) |
+                          BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT))) {
+        /* cpu hot-[un]plug with SMI requires SMI broadcast,
+         * leave @features_ok at zero
+         */
+        return;
+    }
 
     /* valid feature subset requested, lock it down, report success */
     lpc->smi_negotiated_features = guest_features;
@@ -747,6 +755,10 @@ static Property ich9_lpc_properties[] = {
     DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, true),
     DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
                       ICH9_LPC_SMI_F_BROADCAST_BIT, true),
+    DEFINE_PROP_BIT64("x-smi-cpu-hotplug", ICH9LPCState, smi_host_features,
+                      ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT, true),
+    DEFINE_PROP_BIT64("x-smi-cpu-hotunplug", ICH9LPCState, smi_host_features,
+                      ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.26.2

[PATCH 2/6] x86: cphp: prevent guest crash on CPU hotplug when broadcast SMI is in use

[Igor Mammedov]

There were reports of guest crash on CPU hotplug, when using q35 machine
type and OVMF with SMM, due to hotplugged CPU trying to process SMI at
default SMI handler location without it being relocated by firmware first.

Fix it by refusing hotplug if firmware hasn't negotiated CPU hotplug with
SMI support while SMI broadcast is in use.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v1:
   fix typos an use suggested wording in commit and error msg
   s/secure boot/smm/; s/hotplug SMI/hotplug with SMI/
      (Laszlo Ersek <lersek@redhat.com>)
---
 hw/acpi/ich9.c | 12 +++++++++++-
 hw/i386/pc.c   | 11 +++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 6a19070cec..0acc9a3107 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -408,10 +408,20 @@ void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
     ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev);
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
-        !lpc->pm.acpi_memory_hotplug.is_enabled)
+        !lpc->pm.acpi_memory_hotplug.is_enabled) {
         error_setg(errp,
                    "memory hotplug is not enabled: %s.memory-hotplug-support "
                    "is not set", object_get_typename(OBJECT(lpc)));
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        uint64_t negotiated = lpc->smi_negotiated_features;
+
+        if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&
+            !(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT))) {
+            error_setg(errp, "cpu hotplug with SMI wasn't enabled by firmware");
+            error_append_hint(errp, "update machine type to newer than 5.1 "
+                "and firmware that suppors CPU hotplug with SMM");
+        }
+    }
 }
 
 void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 57d50fad6b..3f4b7e3d9a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1497,6 +1497,17 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
         return;
     }
 
+    if (pcms->acpi_dev) {
+        Error *local_err = NULL;
+
+        hotplug_handler_pre_plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev,
+                                 &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+
     init_topo_info(&topo_info, x86ms);
 
     env->nr_dies = x86ms->smp_dies;
-- 
2.26.2

[PATCH 3/6] x86: cpuhp: refuse cpu hot-unplug request earlier if not supported

[Igor Mammedov]

CPU hot-unplug with SMM requires firmware participation to prevent
guest crash (i.e. CPU can be removed only after OS _and_ firmware
were prepared for th action).
Previous patches introduced ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT
feature bit, which is advertised by firmware when it has support
for CPU hot-unplug. Use it to check if guest is able to handle
unplug and make device_del fail gracefully if hot-unplug feature
hasn't been negotiated.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/ich9.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 0acc9a3107..98fc363186 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -460,6 +460,17 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
                                       errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) &&
                !lpc->pm.cpu_hotplug_legacy) {
+        uint64_t negotiated = lpc->smi_negotiated_features;
+
+        if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&
+            !(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT))) {
+            error_setg(errp, "cpu hot-unplug with SMI wasn't enabled "
+                             "by firmware");
+            error_append_hint(errp, "update machine type to newer than 5.1 "
+                "and firmware that suppors CPU hot-unplug with SMM");
+            return;
+        }
+
         acpi_cpu_unplug_request_cb(hotplug_dev, &lpc->pm.cpuhp_state,
                                    dev, errp);
     } else {
-- 
2.26.2

[PATCH 4/6] tests: acpi: mark to be changed tables in bios-tables-test-allowed-diff

[Igor Mammedov]

... to let tests pass until binary blobs are updated with new AML

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/qtest/bios-tables-test-allowed-diff.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..dba32d5613 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,20 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/pc/DSDT",
+"tests/data/acpi/q35/DSDT",
+"tests/data/acpi/q35/DSDT.tis",
+"tests/data/acpi/q35/DSDT.bridge",
+"tests/data/acpi/q35/DSDT.mmio64",
+"tests/data/acpi/q35/DSDT.ipmibt",
+"tests/data/acpi/q35/DSDT.cphp",
+"tests/data/acpi/q35/DSDT.memhp",
+"tests/data/acpi/q35/DSDT.numamem",
+"tests/data/acpi/q35/DSDT.dimmpxm",
+"tests/data/acpi/q35/DSDT.acpihmat",
+"tests/data/acpi/pc/DSDT.bridge",
+"tests/data/acpi/pc/DSDT.ipmikcs",
+"tests/data/acpi/pc/DSDT.cphp",
+"tests/data/acpi/pc/DSDT.memhp",
+"tests/data/acpi/pc/DSDT.numamem",
+"tests/data/acpi/pc/DSDT.dimmpxm",
+"tests/data/acpi/pc/DSDT.acpihmat",
+"tests/data/acpi/pc/DSDT.acpihmat",
-- 
2.26.2

[PATCH 5/6] x68: acpi: trigger SMI before scanning for hotplugged CPUs

[Igor Mammedov]

In case firmware has negotiated CPU hotplug SMI feature, generate
AML to describe SMI IO port region and send SMI to firmware
on each CPU hotplug SCI in case new CPUs were htplugged.

Since new CPUs can be hotplugged while CPU_SCAN_METHOD is running
we can't send SMI before new CPUs are fetched from QEMU as it
could cause sending Notify to a CPU that firmware hasn't seen yet.
So fetch new CPUs into local cache first and then send SMI and
after that sends Notify events to cached CPUs. This should ensure
that Notify is sent only to CPUs which were processed by firmware
first.
Any CPUs that were hotplugged after caching will be process
by the next CPU_SCAN_METHOD, when pending SCI is handled.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v1:
 - make sure that Notify is sent only to CPUs seen by FW

 - (Laszlo Ersek <lersek@redhat.com>)
    - use macro instead of x-smi-negotiated-features
    - style fixes
    - reserve whole SMI block (0xB2, 0xB3)
v0:
 - s/aml_string/aml_eisaid/
   /fixes Windows BSOD, on nonsense value/ (Laszlo Ersek <lersek@redhat.com>)
 - put SMI device under PCI0 like the rest of hotplug IO port
 - do not generate SMI AML if CPU hotplug SMI feature hasn't been negotiated

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/acpi/cpu.h  |  1 +
 include/hw/i386/ich9.h |  2 ++
 hw/acpi/cpu.c          | 50 ++++++++++++++++++++++++++++++++++++++++--
 hw/i386/acpi-build.c   | 35 ++++++++++++++++++++++++++++-
 hw/isa/lpc_ich9.c      |  3 +++
 5 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
index 62f0278ba2..0eeedaa491 100644
--- a/include/hw/acpi/cpu.h
+++ b/include/hw/acpi/cpu.h
@@ -50,6 +50,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
 typedef struct CPUHotplugFeatures {
     bool acpi_1_compatible;
     bool has_legacy_cphp;
+    const char *smi_path;
 } CPUHotplugFeatures;
 
 void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
index d1bb3f7bf0..0f43ef2481 100644
--- a/include/hw/i386/ich9.h
+++ b/include/hw/i386/ich9.h
@@ -245,6 +245,8 @@ typedef struct ICH9LPCState {
 #define ICH9_SMB_HST_D1                         0x06
 #define ICH9_SMB_HOST_BLOCK_DB                  0x07
 
+#define ICH9_LPC_SMI_NEGOTIATED_FEAT_PROP "x-smi-negotiated-features"
+
 /* bit positions used in fw_cfg SMI feature negotiation */
 #define ICH9_LPC_SMI_F_BROADCAST_BIT            0
 #define ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT          1
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 3d6a500fb7..a6dd6e252a 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -14,6 +14,8 @@
 #define ACPI_CPU_CMD_DATA_OFFSET_RW 8
 #define ACPI_CPU_CMD_DATA2_OFFSET_R 0
 
+#define OVMF_CPUHP_SMI_CMD 4
+
 enum {
     CPHP_GET_NEXT_CPU_WITH_EVENT_CMD = 0,
     CPHP_OST_EVENT_CMD = 1,
@@ -321,6 +323,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
 #define CPU_NOTIFY_METHOD "CTFY"
 #define CPU_EJECT_METHOD  "CEJ0"
 #define CPU_OST_METHOD    "COST"
+#define CPU_ADDED_LIST    "CNEW"
 
 #define CPU_ENABLED       "CPEN"
 #define CPU_SELECTOR      "CSEL"
@@ -471,8 +474,27 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
             Aml *dev_chk = aml_int(1);
             Aml *eject_req = aml_int(3);
             Aml *next_cpu_cmd = aml_int(CPHP_GET_NEXT_CPU_WITH_EVENT_CMD);
+            Aml *num_added_cpus = aml_local(1);
+            Aml *cpu_idx = aml_local(2);
+            Aml *new_cpus = aml_name(CPU_ADDED_LIST);
 
             aml_append(method, aml_acquire(ctrl_lock, 0xFFFF));
+
+            /* use named package as old Windows don't support it in local var */
+            if (arch_ids->len <= 256) {
+                /* For compatibility with Windows Server 2008 (max 256 cpus),
+                 * use ACPI1.0 PackageOp which can cache max 255 elements.
+                 * Which is fine for 256 CPUs VM. Max 255 can be hotplugged,
+                 * sice at least one CPU must be already present.
+                */
+                aml_append(method, aml_name_decl(CPU_ADDED_LIST,
+                    aml_package(arch_ids->len)));
+            } else {
+                aml_append(method, aml_name_decl(CPU_ADDED_LIST,
+                    aml_varpackage(arch_ids->len)));
+            }
+
+            aml_append(method, aml_store(zero, num_added_cpus));
             aml_append(method, aml_store(one, has_event));
             while_ctx = aml_while(aml_equal(has_event, one));
             {
@@ -483,8 +505,10 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
                  aml_append(while_ctx, aml_store(next_cpu_cmd, cpu_cmd));
                  ifctx = aml_if(aml_equal(ins_evt, one));
                  {
-                     aml_append(ifctx,
-                         aml_call2(CPU_NOTIFY_METHOD, cpu_data, dev_chk));
+                     /* cache added CPUs to Notify/Wakeup later */
+                     aml_append(ifctx, aml_store(cpu_data,
+                         aml_index(new_cpus, num_added_cpus)));
+                     aml_append(ifctx, aml_increment(num_added_cpus));
                      aml_append(ifctx, aml_store(one, ins_evt));
                      aml_append(ifctx, aml_store(one, has_event));
                  }
@@ -501,6 +525,28 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
                  aml_append(while_ctx, else_ctx);
             }
             aml_append(method, while_ctx);
+
+            /* in case FW negotiated ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT,
+             * make upcall to FW, so it can pull in new CPUs before
+             * OS is notified and wakes them up */
+            if (opts.smi_path) {
+                ifctx = aml_if(aml_lnot(aml_equal(num_added_cpus, zero)));
+                {
+                    aml_append(ifctx, aml_store(aml_int(OVMF_CPUHP_SMI_CMD),
+                        aml_name("%s", opts.smi_path)));
+                }
+                aml_append(method, ifctx);
+            }
+
+            aml_append(method, aml_store(zero, cpu_idx));
+            while_ctx = aml_while(aml_lless(cpu_idx, num_added_cpus));
+            {
+                aml_append(while_ctx, aml_call2(CPU_NOTIFY_METHOD,
+                    aml_derefof(aml_index(new_cpus, cpu_idx)), dev_chk));
+                aml_append(while_ctx, aml_increment(cpu_idx));
+            }
+            aml_append(method, while_ctx);
+
             aml_append(method, aml_release(ctrl_lock));
         }
         aml_append(cpus_dev, method);
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b7bcbbbb2a..2291c050ba 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -95,6 +95,7 @@ typedef struct AcpiPmInfo {
     bool s3_disabled;
     bool s4_disabled;
     bool pcihp_bridge_en;
+    bool smi_on_cpuhp;
     uint8_t s4_val;
     AcpiFadtData fadt;
     uint16_t cpu_hp_io_base;
@@ -194,6 +195,7 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
     pm->cpu_hp_io_base = 0;
     pm->pcihp_io_base = 0;
     pm->pcihp_io_len = 0;
+    pm->smi_on_cpuhp = false;
 
     assert(obj);
     init_common_fadt_data(machine, obj, &pm->fadt);
@@ -207,12 +209,16 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
             object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
     }
     if (lpc) {
+        uint64_t smi_features = object_property_get_uint(lpc,
+            ICH9_LPC_SMI_NEGOTIATED_FEAT_PROP, NULL);
         struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO,
             .bit_width = 8, .address = ICH9_RST_CNT_IOPORT };
         pm->fadt.reset_reg = r;
         pm->fadt.reset_val = 0xf;
         pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP;
         pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
+        pm->smi_on_cpuhp =
+            !!(smi_features & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT));
     }
 
     /* The above need not be conditional on machine type because the reset port
@@ -1515,6 +1521,32 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
         aml_append(dev, aml_name_decl("_UID", aml_int(1)));
         aml_append(dev, build_q35_osc_method());
         aml_append(sb_scope, dev);
+
+        if (pm->smi_on_cpuhp) {
+            /* reserve SMI block resources, IO ports 0xB2, 0xB3 */
+            dev = aml_device("PCI0.SMI0");
+            aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A06")));
+            aml_append(dev, aml_name_decl("_UID", aml_string("SMI resources")));
+            crs = aml_resource_template();
+            aml_append(crs,
+                aml_io(
+                       AML_DECODE16,
+                       ACPI_PORT_SMI_CMD,
+                       ACPI_PORT_SMI_CMD,
+                       1,
+                       2)
+            );
+            aml_append(dev, aml_name_decl("_CRS", crs));
+            aml_append(dev, aml_operation_region("SMIR", AML_SYSTEM_IO,
+                aml_int(ACPI_PORT_SMI_CMD), 2));
+            field = aml_field("SMIR", AML_BYTE_ACC, AML_NOLOCK,
+                              AML_WRITE_AS_ZEROS);
+            aml_append(field, aml_named_field("SMIC", 8));
+            aml_append(field, aml_reserved_field(8));
+            aml_append(dev, field);
+            aml_append(sb_scope, dev);
+        }
+
         aml_append(dsdt, sb_scope);
 
         build_hpet_aml(dsdt);
@@ -1530,7 +1562,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
         build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
     } else {
         CPUHotplugFeatures opts = {
-            .acpi_1_compatible = true, .has_legacy_cphp = true
+            .acpi_1_compatible = true, .has_legacy_cphp = true,
+            .smi_path = pm->smi_on_cpuhp ? "\\_SB.PCI0.SMI0.SMIC" : NULL,
         };
         build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
                        "\\_SB.PCI0", "\\_GPE._E02");
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index c9305080b5..70afc2c5f2 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -646,6 +646,9 @@ static void ich9_lpc_initfn(Object *obj)
                                   &acpi_enable_cmd, OBJ_PROP_FLAG_READ);
     object_property_add_uint8_ptr(OBJECT(lpc), ACPI_PM_PROP_ACPI_DISABLE_CMD,
                                   &acpi_disable_cmd, OBJ_PROP_FLAG_READ);
+    object_property_add_uint64_ptr(obj, ICH9_LPC_SMI_NEGOTIATED_FEAT_PROP,
+                                   &lpc->smi_negotiated_features,
+                                   OBJ_PROP_FLAG_READ);
 
     ich9_pm_add_properties(obj, &lpc->pm);
 }
-- 
2.26.2

[PATCH 6/6] tests: acpi: update acpi blobs with new AML

[Igor Mammedov]

Update CPU hotplug AML with following changes

@@ -2917,6 +2917,8 @@ DefinitionBlock ("", "DSDT", 1, "BOCHS ", "BXPCDSDT", 0x00000001)
             Method (CSCN, 0, Serialized)
             {
                 Acquire (\_SB.PCI0.PRES.CPLK, 0xFFFF)
+                Name (CNEW, Package (0x01){})
+                Local1 = Zero
                 Local0 = One
                 While ((Local0 == One))
                 {
@@ -2924,7 +2926,8 @@ DefinitionBlock ("", "DSDT", 1, "BOCHS ", "BXPCDSDT", 0x00000001)
                     \_SB.PCI0.PRES.CCMD = Zero
                     If ((\_SB.PCI0.PRES.CINS == One))
                     {
-                        CTFY (\_SB.PCI0.PRES.CDAT, One)
+                        CNEW [Local1] = \_SB.PCI0.PRES.CDAT
+                        Local1++
                         \_SB.PCI0.PRES.CINS = One
                         Local0 = One
                     }
@@ -2936,6 +2939,13 @@ DefinitionBlock ("", "DSDT", 1, "BOCHS ", "BXPCDSDT", 0x00000001)
                     }
                 }

+                Local2 = Zero
+                While ((Local2 < Local1))
+                {
+                    CTFY (DerefOf (CNEW [Local2]), One)
+                    Local2++
+                }
+
                 Release (\_SB.PCI0.PRES.CPLK)
             }

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/qtest/bios-tables-test-allowed-diff.h |  19 -------------------
 tests/data/acpi/pc/DSDT                     | Bin 4934 -> 4973 bytes
 tests/data/acpi/pc/DSDT.acpihmat            | Bin 6258 -> 6297 bytes
 tests/data/acpi/pc/DSDT.bridge              | Bin 6793 -> 6832 bytes
 tests/data/acpi/pc/DSDT.cphp                | Bin 5397 -> 5436 bytes
 tests/data/acpi/pc/DSDT.dimmpxm             | Bin 6587 -> 6626 bytes
 tests/data/acpi/pc/DSDT.ipmikcs             | Bin 5006 -> 5045 bytes
 tests/data/acpi/pc/DSDT.memhp               | Bin 6293 -> 6332 bytes
 tests/data/acpi/pc/DSDT.numamem             | Bin 4940 -> 4979 bytes
 tests/data/acpi/q35/DSDT                    | Bin 7678 -> 7717 bytes
 tests/data/acpi/q35/DSDT.acpihmat           | Bin 9002 -> 9041 bytes
 tests/data/acpi/q35/DSDT.bridge             | Bin 7695 -> 7734 bytes
 tests/data/acpi/q35/DSDT.cphp               | Bin 8141 -> 8180 bytes
 tests/data/acpi/q35/DSDT.dimmpxm            | Bin 9331 -> 9370 bytes
 tests/data/acpi/q35/DSDT.ipmibt             | Bin 7753 -> 7792 bytes
 tests/data/acpi/q35/DSDT.memhp              | Bin 9037 -> 9076 bytes
 tests/data/acpi/q35/DSDT.mmio64             | Bin 8808 -> 8847 bytes
 tests/data/acpi/q35/DSDT.numamem            | Bin 7684 -> 7723 bytes
 tests/data/acpi/q35/DSDT.tis                | Bin 8283 -> 8322 bytes
 19 files changed, 19 deletions(-)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dba32d5613..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,20 +1 @@
 /* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/pc/DSDT",
-"tests/data/acpi/q35/DSDT",
-"tests/data/acpi/q35/DSDT.tis",
-"tests/data/acpi/q35/DSDT.bridge",
-"tests/data/acpi/q35/DSDT.mmio64",
-"tests/data/acpi/q35/DSDT.ipmibt",
-"tests/data/acpi/q35/DSDT.cphp",
-"tests/data/acpi/q35/DSDT.memhp",
-"tests/data/acpi/q35/DSDT.numamem",
-"tests/data/acpi/q35/DSDT.dimmpxm",
-"tests/data/acpi/q35/DSDT.acpihmat",
-"tests/data/acpi/pc/DSDT.bridge",
-"tests/data/acpi/pc/DSDT.ipmikcs",
-"tests/data/acpi/pc/DSDT.cphp",
-"tests/data/acpi/pc/DSDT.memhp",
-"tests/data/acpi/pc/DSDT.numamem",
-"tests/data/acpi/pc/DSDT.dimmpxm",
-"tests/data/acpi/pc/DSDT.acpihmat",
-"tests/data/acpi/pc/DSDT.acpihmat",
diff --git a/tests/data/acpi/pc/DSDT b/tests/data/acpi/pc/DSDT
index 6d0aaf729ac7d64cf966621adf276534de5cc555..2ce71cb385e06394d399b1540c8e2d775aa4a9b4 100644
GIT binary patch
delta 147
zcmX@6_EwF{CD<h-SD1l;@zh4HEo@BAmXi;#t!8r5o1Dx3m&spXat?=#0Ee@mYq$^-
zV*x{A0b{}<x5>R6vL-$(lVkK*;)9*y1Drh#0)kwFojv`684J(_T^vI?K>8CIN)spF
h<VX=OU`SddJT)oNIm9ip87!5=z*w5J*_M-&3jk|-D=`27

delta 111
zcmaE>c1(@SCD<jzO_+g!asNiHEo@9aW|I%Ft!DDqo}A16m)(QUIoR2cV{#*>1Pf!r
zBFD)yIArzRStiHmv&08G#RoWh8UzHn20MHD1v5H_xJ9CiyEuk0PJYRevN?s5lM4VS
C4j>@_

diff --git a/tests/data/acpi/pc/DSDT.acpihmat b/tests/data/acpi/pc/DSDT.acpihmat
index 2e5e02400b1bd2842989d395c573fc593f45503b..90ef187d087b436641d9c77806beaf8bc2bf8b63 100644
GIT binary patch
delta 161
zcmexlFw>CBCD<iorUU~6<MfSOTiBQ!?I#~#Tg~KRvN@NXgOSr;z&Y62k0V-nG9!nu
z(EtA&&VH`pLQG5r42cDd35(n&&)|?X@nM-9qt6l_>=Ym1>}e1X<QnYk=@-mcfG+6b
v7}5dKpU6;}IQa=jig*D-(jwuhNr}!OZjsGksU!x*(xl1p9AcZ}I7RpYo@*}X

delta 124
zcmbPf_{o6FCD<jTNP>ZZv3Dca7B(g?o5=^*Rx|k;Zq8-rVC3}Ra}IX)<A_$C%*Y|E
z`u~3cW5Oaw?#T&^1q=xV3^DpF@xe~<0nVNV0YR?8&d$Cr3*09k<Pei^4snY_mvwOr
PVVwMhBV}_wrxrf|$#o<`

diff --git a/tests/data/acpi/pc/DSDT.bridge b/tests/data/acpi/pc/DSDT.bridge
index 623c4c03585c47d4d28adc611823b7cce8f4a5c7..46f651d7a106032cca053b0e59a107a333f3fe7e 100644
GIT binary patch
delta 147
zcmeA)-C)Y)66_MPL5hKaQFtTQ7B(hl%gG1WRx`QjP0nTi%j7RGIfp|=fWz6(HC%{^
zv4A15fH7f_+vHvjSrZ?Y$uasY@xe~<0nVNV0YR?8&Yph3j0Na|E{-7`ApMC9rHPYo
ha-@hCFeEJ!o|=^C9O4$)43<h_U@T4AY|H5*2mk;)DzX3o

delta 111
zcmdmB+G)z=66_MvDaF9R$gz=Y3mcP<+2jLktC{?@C+D*NW%uB74tDnAnB2%I!NQoZ
z$Z_%v4q1J7mdP>tEb+lk@d3`B1_42?!Oot3!Hmu!ZjtEXE{-9LlV5VAY);|y5d;9)
C1RtUR

diff --git a/tests/data/acpi/pc/DSDT.cphp b/tests/data/acpi/pc/DSDT.cphp
index e0a43ccdadae150c0f39599c85e4e21ed8fff2a4..74d132918a096b7548eaa0e13884a04fad73df21 100644
GIT binary patch
delta 143
zcmbQLwMUD~CD<jzMwEep(QYHx7B(iQu*nD5Rx`PJZ_Z`6WMuaja1M6%<CyHvAuhn-
z?B^OT#Kcy>kXXQ&u*hxlNe)>FAC}26`YiFmPVoWGo(2IyuEEZpe!+|dlNmXMIXgf)
r5*bPpCyR2Xh!-#<EfSuZl;|Ad7TFAzN@8FvO`1G`Lu_+DXB#&FuY)J6

delta 136
zcmdm^HC2ntCD<iIRFr{%(PAUl7B(jD;K>KrRx|l|Y|dr3WMuc?a}IX)<CyHvArZis
zu*i{nasp!kLqY*Vj6O?zuv2`1v!_8okZZ8Bv#-kncb3WM!k&J?jLso$k?7(sjv<Vb
NRXI~OFW{WR4FJjPC2Ifx

diff --git a/tests/data/acpi/pc/DSDT.dimmpxm b/tests/data/acpi/pc/DSDT.dimmpxm
index 21eb065a0ee3bd96f1a2e7601aa83fefa833349a..929e50d002d98872ab6100420cd5c275678a134f 100644
GIT binary patch
delta 131
zcmdmO{K%NgCD<k8kt72HBiBZ*Eo@BQUXu^7t!DDG-<->?z{un;Fu9*YMu5ZF&ox|#
ziKT!cv4AmQk=x`A9I}i)lh1L8$ri-uv&08G#RoWh8UzHn20ObrhID|GCo+^KPX5D@
fB3{6dv`BbrQlfK+TVyj>Dv5!yG--1JXS4tSxGE;e

delta 89
zcmV-f0H*)qGrKbiL{mgmyBPog0U)sov<3o1NRz+@s{%(ovuy_;0Rl)4laB}-1#kgi
vqCu0p2ptwj1e07Z1Yc7^Ur<9yFi=uOQ$tBkQ<FXk7y$v33<+ejhY4E{fyWrg

diff --git a/tests/data/acpi/pc/DSDT.ipmikcs b/tests/data/acpi/pc/DSDT.ipmikcs
index b8f08f266b5735fe6967d4e105ee6b3662dad7e6..7a1fc5f0c7b149210cac6098b18dfe243be4fadb 100644
GIT binary patch
delta 194
zcmeBE->S~#66_MPRhWT+F>WK*A2udu%gG$<tC`&NCU0f`TkkL69PI4J5v?4f&k`T(
z6d&O1X%G<P8tfe4<Ng0XhqIq+xDXR#0YhQ|W5Oag?#T&2c0vIIx@u=%mjym7lhK7e
z{el?_&;?x_Lpng_CNh*JPFCbh5iej!S|mI*DbYE^EwULbmBheUnzXr<lamVosm3_<

delta 113
zcmdn0-lxvx66_MvC(OXW7`&0|4;zz@*<=p()lB}{lee<}RrcU>4tDnAh*pl#XNeDX
ziVtx1GzbWC4R#Lj@&5n6fH7f_<Kzb%vW)JN|8a;(IET1JqAPT93}KvX$eFTv4JRiT
E0FWRe4gdfE

diff --git a/tests/data/acpi/pc/DSDT.memhp b/tests/data/acpi/pc/DSDT.memhp
index 9a9418f4bde5fb18883c244ea956122e371ff01a..5b5bf7f7d1dd78f5cb10d71be68656b3b842886f 100644
GIT binary patch
delta 147
zcmbPgxW|yoCD<ioj|2k)<GhVrTiBR9EhisfTg~L7H#wL6FO$E(<Qxte0S;$B*Ki>w
z#sY@K0>*?zZj*aCWKDcnCdcTr#0NXY2RM5g1O&MTJA3*CGZvr=x;Tb(fb=IalqOET
h$&n&nz>u^^cxqCjbBJ4HGgvB#fw44cvn^)?KLA|eD{TM(

delta 111
zcmdmEIMtBLCD<iosssZA<Hn6#TiBSK%qJgUTg~LEGdY+2FS`eybFi}?$K*y%2^PkL
zMUInaaLDSrvrLZBXNeDXiVtx1GzbWC4R-eQ3ubf<af?J3cX141ocxj_WpfH=1U~>B
C?;u?O

diff --git a/tests/data/acpi/pc/DSDT.numamem b/tests/data/acpi/pc/DSDT.numamem
index 6eec385c2ec00544c6eaa7e19d32b2ccd5a51915..57b03c88d75cbfcffc81e17d52970e52fb767dc8 100644
GIT binary patch
delta 147
zcmX@3_F0X~CD<jTSeSu<F>E8(7B(hN%gG1WRx|nNP0nTi%j7RGIfp|=fWz6(HC%{^
zv4A15fH7f_+vHvjSrZ?Y$uasY@xe~<0nVNV0YR?8&Yph3j0Na|E{-7`ApMC9rHPYo
ha-@hCFeEJ!o|=^C9O4$)43<h_U@T4AY|F{d1pq%GD&YVC

delta 111
zcmeyYc1DfMCD<jzN0@<uF>@o=7B(g)^T`L;Rx`QkOwMKh%kIJF9PI4JF}aabf`u_*
zk>lhU9J2cEER$pOS>l78;scyL4FZB(gPlG7f*GAd+#=D%T^vIgC%@!K*_^`3&jkPo
C=O57k

diff --git a/tests/data/acpi/q35/DSDT b/tests/data/acpi/q35/DSDT
index e63676d7a63afec714debeb465ee478ea4714337..26a26e68d166c7e02971d62a48b0e79c38744338 100644
GIT binary patch
delta 142
zcmexoz0`)wCD<iIRgQsyaqC8|2@*`smXqg5tY&i4o9rq1m)&2$IoR2cWAb}RaRClz
zKi6;}CdLAW!~({IMQ)QbrDP?1SSH8lv&08G#RoWh8UzHn20MHD1v3^*UMMBZ*#Xj#
r$WWR%d8<^4cmYGwBH^h?iOwNzk<DPKBnHOPq{#+SVw?4)&oco4kr^r?

delta 135
zcmZ2#^Us>gCD<k8pDY6d<LZrE6C{{?%qGu~Sk2_GJ=s(8FS`eybFi}?$K>~t5&?_}
ziyXNpComQ;Bor{j=(EHJJH-b$dm01;xduBs`?@S}XPJyH?CBTG=p5n}i7xKq7{WOD
Ms8q^k2kECw004a`%>V!Z

diff --git a/tests/data/acpi/q35/DSDT.acpihmat b/tests/data/acpi/q35/DSDT.acpihmat
index cd97b819824e4140d087e465d179b71775d6a494..bf363910b99c8501669041cb6a796e2a5fb82c40 100644
GIT binary patch
delta 177
zcmZ4GcF~Q?CD<h-P?>>&v1udM1PLZb`^j@8Rx`PnZ1$AoU}X0fa1M6%<Cr{AQaq8v
z+0Qjxh>59yA+dllVUZj6<OIe7hJ*r!7=4!bV5j&1XHSEGAlG1LXJ3~EJ}i^bg+2X(
x84J(_T^vI?K;|YglqOC-AeACsz>u^^cxqCjbBJ4HGgvB#fw44cvx+nu2LS#(GAsZ9

delta 112
zcmccUw#tpmCD<iIOPPUzv0@|F1PLZDo5^z|Rx|k;ZuXSqU}X2;a}IX)<Cr{AQi6ps
zVUgqHdMR0bcb3U9`YiFmPVoWGo(2IyuEEZpe!+~+A#RcA;x3LMjFT@&rEK<=X5#<=
D$~zv~

diff --git a/tests/data/acpi/q35/DSDT.bridge b/tests/data/acpi/q35/DSDT.bridge
index 8b0fb497dbbaeba18e9d0e1503de4396f1c230b0..22d0eab1632cdd9e78330a7c1c746120ee0a90b1 100644
GIT binary patch
delta 142
zcmeCT*=ED#66_LUCda_Q7`l;bf&`PZ<>WaMtC`&NCVNW$W%n0w4tDnAnEYN+T!6#b
z&ox|#iLrnov4AmQk=x`<DOm|0mdP>tEb+lk@d3`B1_42?!Oot3!Hfly7fK0pc7Svw
qGL$Ax-YS(MUcivFNO)>eqH~B_WHVSQiGi^+X|jQo*k*m{2TTAqS1E@8

delta 135
zcmdmH({ID&66_MfFUP>Z=(Uk+f&`O~+2lDAtC{?@Cwof%W%uB74tDnAnEYN+B7iYr
zkt6ry1jYh}gaU>beU|uOr}zM8PlJFU*I;L7UzY{$ER)fNJ^g|iokQFr(ZyXHLl`F?
Ml}g#{ApMOA04-uCG5`Po

diff --git a/tests/data/acpi/q35/DSDT.cphp b/tests/data/acpi/q35/DSDT.cphp
index d9bb414e9bf15d9b9149f38c9bb5d8b993f88650..ee2bd28aac83fc9543cafd13d654be6c0b7d47f1 100644
GIT binary patch
delta 195
zcmX?W|HYomCD<k8i#!7Zqt-^Q2@*_BVUy=btY&ic-s~x9$yo0%;2iAi#}Ta@qt6l_
z>=Ym1>}e1X<QnW8;N$)OKZmoQYq$^-TLD920b{}<H}1&^j0Fq{1q|q_oqb&v_^?by
z7xwfEW-LG#ba4#n0GXS}P?|XTqg0A`0YlOv;i*Z9&LM7*&0wh{2FB8)&7sm2EC3uJ
BI&lC1

delta 114
zcmexjf7YJMCD<k8tULn)qsm6E2@*`+!IS4mtY-4_*z753$*AnX=N#<p#}Ta@qt6l_
z>=Ym1>}e1X<QnW8;N$)Oe*t5{BFD)aq+}W0C!doNlW-1ki$quG;uyj>nNd1rbB%Nb
F3jo7{B2NGS

diff --git a/tests/data/acpi/q35/DSDT.dimmpxm b/tests/data/acpi/q35/DSDT.dimmpxm
index 29f19b22a38f9d8e7dc9870f0c1aa3d4470643ff..f58daacaf329c70c0f3007e1b4d162a20e5e7408 100644
GIT binary patch
delta 148
zcmezDG0T(7CD<iomI?y{<MoYP6C{|ty(Z6*Sk2^Tzu8k#fsx5yU~;yUi~xtTpKG`f
z6H5U@VgX~qBDcvsQnDsKER$pOS>l78;scyL4FZB(gPlG7f*A|Y1zj9NIzajp8A=l;
i-;hcXFJMSoBs?`K(K*B|vKcIu#K2gZwAn`5oD%?Qkt<IC

delta 116
zcmbQ``PqZZCD<jTScQRs@$E*g2@*^$9+T%ttY&hz-Rvo;z{u{w=N#<p$1%A<T7rc!
zVUgqH=~A-#?ktmI^jYGAo#F$WJq-eaT!WoG{el^tL);?K#a$dj7$?7wN}0?kBf7al
HT8R??uE`+;

diff --git a/tests/data/acpi/q35/DSDT.ipmibt b/tests/data/acpi/q35/DSDT.ipmibt
index e8dea1ea42af765babcb747af998b0d912abdcbd..4ecf98b8659229fe33ecf1deee5d61d4425cf52e 100644
GIT binary patch
delta 175
zcmX?U^TCG8CD<jTK#qZd@#RLY+Y(I9mXn`LtY&i4n><DGFT1~hbFi}?$K;EW;)xv2
zey-s{OpFB#i3N-ai`=*;Cji+A1q?CzEb+lk@d3`B1_42?!OqUUE(?5ECZh{``UNu<
upbNS<hID|;O=KudoFtthUcivFNO)>eqH~B_WHVSMiGi^+X>*G7QzigB(={9b

delta 111
zcmexhbJB*(CD<jzQ;vaw@!m$R+Y(GZW|N;wtY-4po;*eJFS`eybFi}?$K;EW5-f}f
ziySBKmy*?YXPF$M&k`T(6d&O1X%G<P8tm-p7tH7!;ueW6?&282I9WhCWplUmQzihc
CSRxAm

diff --git a/tests/data/acpi/q35/DSDT.memhp b/tests/data/acpi/q35/DSDT.memhp
index dca76db15b943122efd5c200cb54ce3dbc97a55f..06bc000ff275668d6864b5d16117d48b308c6d61 100644
GIT binary patch
delta 142
zcmX@>_Qj3MCD<jTM45qsv2!EW1PLZj%gJ*jRx|nNP4<-h%kD4W9PI4JG5NiuxB!Q<
zpKG`f6Jr5GVgX~qBDcwzQnC_0ER$pOS>l78;scyL4FZB(gPlG7f*A`YFO(AI>;UOV
qWGGFXyj3biynrETk?_={MCTB<$Y!uq5(8st(qscEvCaC@avT80Z7JRW

delta 135
zcmez3cGiu{CD<jzSDAr<an44r2@*_B=9A}0tY&i6nd~X~m)(QUIoR2cWAb}Ri2%lg
zMULE)6Br8^5(*e%^jYGAo#F$WJq-eaT!WpReO(r~vrI-8_Vf#8bPjQgL>G5)3}Kvn
MR4Qe&gR}_;0Jjb&i2wiq

diff --git a/tests/data/acpi/q35/DSDT.mmio64 b/tests/data/acpi/q35/DSDT.mmio64
index 6d8facd9e179140682ad5d4302f3dad14dbec342..1d34813a26b46e229c5dee7d7b19680aa941b006 100644
GIT binary patch
delta 142
zcmaFi((lUU66_Mvuf)K>=(>?>f&`PN<>WaMtC@WCCVNW$W%n0w4tDnAnEYN+T!6#b
z&ox|#iLrnov4AmQk=x`<DOm|0mdP>tEb+lk@d3`B1_42?!Oot3!Hfly7fK0pc7Svw
qGL$Ax-YS(MUcivFNO)>eqH~B_WHVSQiGi^+X|jQo*k*lcOLhQ=J}H3!

delta 135
zcmeBoec{6866_L^p~S$z7_pISf&`P3`Q$kgtC?JNCVNW$W%uB74tDnAnEYN+B7iYr
zkt6ry1jYh}gaU>beU|uOr}zM8PlJFU*I;L7UzY{$ER)fNJ^g|iokQFr(ZyXHLl`F?
Ml}g#{Annf%0DB@QEC2ui

diff --git a/tests/data/acpi/q35/DSDT.numamem b/tests/data/acpi/q35/DSDT.numamem
index 737325dc3082fdf06283857811f6f5174e0ff2a9..7e1775656cf08224d815e6e2986fa68dbe54d4e1 100644
GIT binary patch
delta 142
zcmZp%S#87R66_M9EyuvX=(v$<f&`PN<>WaMtC@WCCVNW$W%n0w4tDnAnEYN+T!6#b
z&ox|#iLrnov4AmQk=x`<DOm|0mdP>tEb+lk@d3`B1_42?!Oot3!Hfly7fK0pc7Svw
qGL$Ax-YS(MUcivFNO)>eqH~B_WHVSQiGi^+X|jQo*k*m{t4shN`YBTY

delta 135
zcmZ2&(_+Kr66_MfBFDhM7_yOTf&`P3`Q$kgtC?JNCVNW$W%uB74tDnAnEYN+B7iYr
zkt6ry1jYh}gaU>beU|uOr}zM8PlJFU*I;L7UzY{$ER)fNJ^g|iokQFr(ZyXHLl`F?
Ml}g#{ApM#N02x0f{r~^~

diff --git a/tests/data/acpi/q35/DSDT.tis b/tests/data/acpi/q35/DSDT.tis
index 27ee927fc5ac05b89724c154197304c39e653269..d7872b2dc57bfe64d76051158f315816e166002a 100644
GIT binary patch
delta 142
zcmccZ(B#PF66_Mvq`<(y*td~uf&`PZ<>WaMtC`&NCVNW$W%n0w4tDnAnEYN+T!6#b
z&ox|#iLrnov4AmQk=x`<DOm|0mdP>tEb+lk@d3`B1_42?!Oot3!Hfly7fK0pc7Svw
qGL$Ax-YS(MUcivFNO)>eqH~B_WHVSQiGi^+X|jQo*k*lc9aaF0nJIn%

delta 135
zcmZp2yzRi{66_KZt-!#**u0Tzf&`O~+2lDAtC{?@Cwof%W%uB74tDnAnEYN+B7iYr
zkt6ry1jYh}gaU>beU|uOr}zM8PlJFU*I;L7UzY{$ER)fNJ^g|iokQFr(ZyXHLl`F?
Ml}g#{Ann8o0DcZ9DF6Tf

-- 
2.26.2

Re: [PATCH 1/6] x86: lpc9: let firmware negotiate 'CPU hotplug with SMI' features

[Laszlo Ersek]

On 07/20/20 16:16, Igor Mammedov wrote:
> It will allow firmware to notify QEMU that firmware requires SMI
> being triggered on CPU hot[un]plug, so that it would be able to account
> for hotplugged CPU and relocate it to new SMM base and/or safely remove
> CPU on unplug.
> 
> Using negotiated features, follow up patches will insert SMI upcall
> into AML code, to make sure that firmware processes hotplug before
> guest OS would attempt to use new CPU.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/hw/i386/ich9.h |  2 ++
>  hw/i386/pc.c           |  5 ++++-
>  hw/isa/lpc_ich9.c      | 12 ++++++++++++
>  3 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> index a98d10b252..d1bb3f7bf0 100644
> --- a/include/hw/i386/ich9.h
> +++ b/include/hw/i386/ich9.h
> @@ -247,5 +247,7 @@ typedef struct ICH9LPCState {
>  
>  /* bit positions used in fw_cfg SMI feature negotiation */
>  #define ICH9_LPC_SMI_F_BROADCAST_BIT            0
> +#define ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT          1
> +#define ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT       2
>  
>  #endif /* HW_ICH9_H */
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3d419d5991..57d50fad6b 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -97,7 +97,10 @@
>  #include "fw_cfg.h"
>  #include "trace.h"
>  
> -GlobalProperty pc_compat_5_0[] = {};
> +GlobalProperty pc_compat_5_0[] = {
> +    { "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
> +    { "ICH9-LPC", "x-smi-cpu-hotunplug", "off" },
> +};
>  const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
>  
>  GlobalProperty pc_compat_4_2[] = {
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index cd6e169d47..c9305080b5 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -373,6 +373,14 @@ static void smi_features_ok_callback(void *opaque)
>          /* guest requests invalid features, leave @features_ok at zero */
>          return;
>      }
> +    if (!(guest_features & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT)) &&
> +        guest_features & (BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT) |
> +                          BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT))) {
> +        /* cpu hot-[un]plug with SMI requires SMI broadcast,
> +         * leave @features_ok at zero
> +         */
> +        return;
> +    }
>  
>      /* valid feature subset requested, lock it down, report success */
>      lpc->smi_negotiated_features = guest_features;
> @@ -747,6 +755,10 @@ static Property ich9_lpc_properties[] = {
>      DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, true),
>      DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
>                        ICH9_LPC_SMI_F_BROADCAST_BIT, true),
> +    DEFINE_PROP_BIT64("x-smi-cpu-hotplug", ICH9LPCState, smi_host_features,
> +                      ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT, true),
> +    DEFINE_PROP_BIT64("x-smi-cpu-hotunplug", ICH9LPCState, smi_host_features,
> +                      ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> 

(1) I think that, at this time, the default value for
"x-smi-cpu-hotunplug" should be false. Accordingly, the
"x-smi-cpu-hotunplug" compat property should be dropped.

The reason is that in this series, QEMU won't actually learn to handle
CPU hot-unplug with SMI. We shouldn't advertize support for it.

We should only recognize the feature, so that the QMP command can rely
on it for rejecting hot-unplug attempts. If (later) we have a more
advanced OVMF binary with unplug support (so that OVMF would try to
negotiate unplug), but the user runs it on QEMU-5.1 (or more precisely
with an 5.1 machine type), which will support plug, but not unplug, then
QEMU should reject the device_del QMP command.

In brief, we need both properties because we want both device_add and
device_del to fail gracefully, but the default values (and accordingly
the compat properties) should reflect the actual feature support. So
unplug should default to false at this point.

Thanks,
Laszlo

Re: [PATCH 2/6] x86: cphp: prevent guest crash on CPU hotplug when broadcast SMI is in use

[Laszlo Ersek]

On 07/20/20 16:16, Igor Mammedov wrote:
> There were reports of guest crash on CPU hotplug, when using q35 machine
> type and OVMF with SMM, due to hotplugged CPU trying to process SMI at
> default SMI handler location without it being relocated by firmware first.
> 
> Fix it by refusing hotplug if firmware hasn't negotiated CPU hotplug with
> SMI support while SMI broadcast is in use.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v1:
>    fix typos an use suggested wording in commit and error msg
>    s/secure boot/smm/; s/hotplug SMI/hotplug with SMI/
>       (Laszlo Ersek <lersek@redhat.com>)
> ---
>  hw/acpi/ich9.c | 12 +++++++++++-
>  hw/i386/pc.c   | 11 +++++++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 6a19070cec..0acc9a3107 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -408,10 +408,20 @@ void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>      ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev);
>  
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
> -        !lpc->pm.acpi_memory_hotplug.is_enabled)
> +        !lpc->pm.acpi_memory_hotplug.is_enabled) {
>          error_setg(errp,
>                     "memory hotplug is not enabled: %s.memory-hotplug-support "
>                     "is not set", object_get_typename(OBJECT(lpc)));
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> +        uint64_t negotiated = lpc->smi_negotiated_features;
> +
> +        if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&
> +            !(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT))) {
> +            error_setg(errp, "cpu hotplug with SMI wasn't enabled by firmware");
> +            error_append_hint(errp, "update machine type to newer than 5.1 "
> +                "and firmware that suppors CPU hotplug with SMM");
> +        }
> +    }
>  }
>  
>  void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 57d50fad6b..3f4b7e3d9a 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1497,6 +1497,17 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>          return;
>      }
>  
> +    if (pcms->acpi_dev) {
> +        Error *local_err = NULL;
> +
> +        hotplug_handler_pre_plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev,
> +                                 &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    }
> +
>      init_topo_info(&topo_info, x86ms);
>  
>      env->nr_dies = x86ms->smp_dies;
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH 3/6] x86: cpuhp: refuse cpu hot-unplug request earlier if not supported

[Laszlo Ersek]

On 07/20/20 16:16, Igor Mammedov wrote:
> CPU hot-unplug with SMM requires firmware participation to prevent
> guest crash (i.e. CPU can be removed only after OS _and_ firmware
> were prepared for th action).

(1) s/th action/the action/

> Previous patches introduced ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT
> feature bit, which is advertised by firmware when it has support
> for CPU hot-unplug. Use it to check if guest is able to handle
> unplug and make device_del fail gracefully if hot-unplug feature
> hasn't been negotiated.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/acpi/ich9.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 0acc9a3107..98fc363186 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -460,6 +460,17 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>                                        errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) &&
>                 !lpc->pm.cpu_hotplug_legacy) {
> +        uint64_t negotiated = lpc->smi_negotiated_features;
> +
> +        if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&
> +            !(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT))) {
> +            error_setg(errp, "cpu hot-unplug with SMI wasn't enabled "
> +                             "by firmware");
> +            error_append_hint(errp, "update machine type to newer than 5.1 "
> +                "and firmware that suppors CPU hot-unplug with SMM");

(2) I think the "machine type to newer than 5.1" reference should be removed at this point. (Similarly to how "x-smi-cpu-hotunplug" should default to "false" at this point in time, in patch#1.) We don't know when hot-unplug will work; we only know that it doesn't work now.

We could say something like

            error_append_hint(errp, "update machine type to a version having "
                              "x-smi-cpu-hotunplug=on and firmware that "
                              "suppors CPU hot-unplug with SMM");


> +            return;
> +        }
> +
>          acpi_cpu_unplug_request_cb(hotplug_dev, &lpc->pm.cpuhp_state,
>                                     dev, errp);
>      } else {
> 

These superficial comments aside, the patch works for me:

Tested-by: Laszlo Ersek <lersek@redhat.com>

and with (1) and (2) fixed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [PATCH 1/6] x86: lpc9: let firmware negotiate 'CPU hotplug with SMI' features

[Laszlo Ersek]

On 07/22/20 14:03, Laszlo Ersek wrote:
> On 07/20/20 16:16, Igor Mammedov wrote:
>> It will allow firmware to notify QEMU that firmware requires SMI
>> being triggered on CPU hot[un]plug, so that it would be able to account
>> for hotplugged CPU and relocate it to new SMM base and/or safely remove
>> CPU on unplug.
>>
>> Using negotiated features, follow up patches will insert SMI upcall
>> into AML code, to make sure that firmware processes hotplug before
>> guest OS would attempt to use new CPU.
>>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> ---
>>  include/hw/i386/ich9.h |  2 ++
>>  hw/i386/pc.c           |  5 ++++-
>>  hw/isa/lpc_ich9.c      | 12 ++++++++++++
>>  3 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
>> index a98d10b252..d1bb3f7bf0 100644
>> --- a/include/hw/i386/ich9.h
>> +++ b/include/hw/i386/ich9.h
>> @@ -247,5 +247,7 @@ typedef struct ICH9LPCState {
>>  
>>  /* bit positions used in fw_cfg SMI feature negotiation */
>>  #define ICH9_LPC_SMI_F_BROADCAST_BIT            0
>> +#define ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT          1
>> +#define ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT       2
>>  
>>  #endif /* HW_ICH9_H */
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 3d419d5991..57d50fad6b 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -97,7 +97,10 @@
>>  #include "fw_cfg.h"
>>  #include "trace.h"
>>  
>> -GlobalProperty pc_compat_5_0[] = {};
>> +GlobalProperty pc_compat_5_0[] = {
>> +    { "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
>> +    { "ICH9-LPC", "x-smi-cpu-hotunplug", "off" },
>> +};
>>  const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
>>  
>>  GlobalProperty pc_compat_4_2[] = {
>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>> index cd6e169d47..c9305080b5 100644
>> --- a/hw/isa/lpc_ich9.c
>> +++ b/hw/isa/lpc_ich9.c
>> @@ -373,6 +373,14 @@ static void smi_features_ok_callback(void *opaque)
>>          /* guest requests invalid features, leave @features_ok at zero */
>>          return;
>>      }
>> +    if (!(guest_features & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT)) &&
>> +        guest_features & (BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT) |
>> +                          BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT))) {
>> +        /* cpu hot-[un]plug with SMI requires SMI broadcast,
>> +         * leave @features_ok at zero
>> +         */
>> +        return;
>> +    }
>>  
>>      /* valid feature subset requested, lock it down, report success */
>>      lpc->smi_negotiated_features = guest_features;
>> @@ -747,6 +755,10 @@ static Property ich9_lpc_properties[] = {
>>      DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, true),
>>      DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
>>                        ICH9_LPC_SMI_F_BROADCAST_BIT, true),
>> +    DEFINE_PROP_BIT64("x-smi-cpu-hotplug", ICH9LPCState, smi_host_features,
>> +                      ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT, true),
>> +    DEFINE_PROP_BIT64("x-smi-cpu-hotunplug", ICH9LPCState, smi_host_features,
>> +                      ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT, true),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>  
>>
> 
> (1) I think that, at this time, the default value for
> "x-smi-cpu-hotunplug" should be false. Accordingly, the
> "x-smi-cpu-hotunplug" compat property should be dropped.
> 
> The reason is that in this series, QEMU won't actually learn to handle
> CPU hot-unplug with SMI. We shouldn't advertize support for it.
> 
> We should only recognize the feature, so that the QMP command can rely
> on it for rejecting hot-unplug attempts. If (later) we have a more
> advanced OVMF binary with unplug support (so that OVMF would try to
> negotiate unplug), but the user runs it on QEMU-5.1 (or more precisely
> with an 5.1 machine type), which will support plug, but not unplug, then
> QEMU should reject the device_del QMP command.
> 
> In brief, we need both properties because we want both device_add and
> device_del to fail gracefully, but the default values (and accordingly
> the compat properties) should reflect the actual feature support. So
> unplug should default to false at this point.

With (1) updated:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH 5/6] x68: acpi: trigger SMI before scanning for hotplugged CPUs

[Laszlo Ersek]

On 07/20/20 16:16, Igor Mammedov wrote:
> In case firmware has negotiated CPU hotplug SMI feature, generate
> AML to describe SMI IO port region and send SMI to firmware
> on each CPU hotplug SCI in case new CPUs were htplugged.

(1) s/htplugged/hotplugged/

>
> Since new CPUs can be hotplugged while CPU_SCAN_METHOD is running
> we can't send SMI before new CPUs are fetched from QEMU as it
> could cause sending Notify to a CPU that firmware hasn't seen yet.
> So fetch new CPUs into local cache first and then send SMI and
> after that sends Notify events to cached CPUs. This should ensure
> that Notify is sent only to CPUs which were processed by firmware
> first.
> Any CPUs that were hotplugged after caching will be process

(2) s/will be process/will be processed/

> by the next CPU_SCAN_METHOD, when pending SCI is handled.

(3) I think that the subject ("trigger SMI before scanning") may no
longer apply, because we do scan before triggering the SMI.

>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v1:
>  - make sure that Notify is sent only to CPUs seen by FW
>
>  - (Laszlo Ersek <lersek@redhat.com>)
>     - use macro instead of x-smi-negotiated-features
>     - style fixes
>     - reserve whole SMI block (0xB2, 0xB3)
> v0:
>  - s/aml_string/aml_eisaid/
>    /fixes Windows BSOD, on nonsense value/ (Laszlo Ersek <lersek@redhat.com>)
>  - put SMI device under PCI0 like the rest of hotplug IO port
>  - do not generate SMI AML if CPU hotplug SMI feature hasn't been negotiated
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/hw/acpi/cpu.h  |  1 +
>  include/hw/i386/ich9.h |  2 ++
>  hw/acpi/cpu.c          | 50 ++++++++++++++++++++++++++++++++++++++++--
>  hw/i386/acpi-build.c   | 35 ++++++++++++++++++++++++++++-
>  hw/isa/lpc_ich9.c      |  3 +++
>  5 files changed, 88 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> index 62f0278ba2..0eeedaa491 100644
> --- a/include/hw/acpi/cpu.h
> +++ b/include/hw/acpi/cpu.h
> @@ -50,6 +50,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
>  typedef struct CPUHotplugFeatures {
>      bool acpi_1_compatible;
>      bool has_legacy_cphp;
> +    const char *smi_path;
>  } CPUHotplugFeatures;
>
>  void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> index d1bb3f7bf0..0f43ef2481 100644
> --- a/include/hw/i386/ich9.h
> +++ b/include/hw/i386/ich9.h
> @@ -245,6 +245,8 @@ typedef struct ICH9LPCState {
>  #define ICH9_SMB_HST_D1                         0x06
>  #define ICH9_SMB_HOST_BLOCK_DB                  0x07
>
> +#define ICH9_LPC_SMI_NEGOTIATED_FEAT_PROP "x-smi-negotiated-features"
> +
>  /* bit positions used in fw_cfg SMI feature negotiation */
>  #define ICH9_LPC_SMI_F_BROADCAST_BIT            0
>  #define ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT          1
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index 3d6a500fb7..a6dd6e252a 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -14,6 +14,8 @@
>  #define ACPI_CPU_CMD_DATA_OFFSET_RW 8
>  #define ACPI_CPU_CMD_DATA2_OFFSET_R 0
>
> +#define OVMF_CPUHP_SMI_CMD 4
> +
>  enum {
>      CPHP_GET_NEXT_CPU_WITH_EVENT_CMD = 0,
>      CPHP_OST_EVENT_CMD = 1,
> @@ -321,6 +323,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
>  #define CPU_NOTIFY_METHOD "CTFY"
>  #define CPU_EJECT_METHOD  "CEJ0"
>  #define CPU_OST_METHOD    "COST"
> +#define CPU_ADDED_LIST    "CNEW"
>
>  #define CPU_ENABLED       "CPEN"
>  #define CPU_SELECTOR      "CSEL"
> @@ -471,8 +474,27 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>              Aml *dev_chk = aml_int(1);
>              Aml *eject_req = aml_int(3);
>              Aml *next_cpu_cmd = aml_int(CPHP_GET_NEXT_CPU_WITH_EVENT_CMD);
> +            Aml *num_added_cpus = aml_local(1);
> +            Aml *cpu_idx = aml_local(2);
> +            Aml *new_cpus = aml_name(CPU_ADDED_LIST);
>
>              aml_append(method, aml_acquire(ctrl_lock, 0xFFFF));
> +
> +            /* use named package as old Windows don't support it in local var */
> +            if (arch_ids->len <= 256) {
> +                /* For compatibility with Windows Server 2008 (max 256 cpus),
> +                 * use ACPI1.0 PackageOp which can cache max 255 elements.
> +                 * Which is fine for 256 CPUs VM. Max 255 can be hotplugged,
> +                 * sice at least one CPU must be already present.
> +                */
> +                aml_append(method, aml_name_decl(CPU_ADDED_LIST,
> +                    aml_package(arch_ids->len)));
> +            } else {
> +                aml_append(method, aml_name_decl(CPU_ADDED_LIST,
> +                    aml_varpackage(arch_ids->len)));
> +            }
> +
> +            aml_append(method, aml_store(zero, num_added_cpus));
>              aml_append(method, aml_store(one, has_event));
>              while_ctx = aml_while(aml_equal(has_event, one));
>              {
> @@ -483,8 +505,10 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>                   aml_append(while_ctx, aml_store(next_cpu_cmd, cpu_cmd));
>                   ifctx = aml_if(aml_equal(ins_evt, one));
>                   {
> -                     aml_append(ifctx,
> -                         aml_call2(CPU_NOTIFY_METHOD, cpu_data, dev_chk));
> +                     /* cache added CPUs to Notify/Wakeup later */
> +                     aml_append(ifctx, aml_store(cpu_data,
> +                         aml_index(new_cpus, num_added_cpus)));
> +                     aml_append(ifctx, aml_increment(num_added_cpus));
>                       aml_append(ifctx, aml_store(one, ins_evt));
>                       aml_append(ifctx, aml_store(one, has_event));
>                   }
> @@ -501,6 +525,28 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>                   aml_append(while_ctx, else_ctx);
>              }
>              aml_append(method, while_ctx);
> +
> +            /* in case FW negotiated ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT,
> +             * make upcall to FW, so it can pull in new CPUs before
> +             * OS is notified and wakes them up */
> +            if (opts.smi_path) {
> +                ifctx = aml_if(aml_lnot(aml_equal(num_added_cpus, zero)));
> +                {
> +                    aml_append(ifctx, aml_store(aml_int(OVMF_CPUHP_SMI_CMD),
> +                        aml_name("%s", opts.smi_path)));
> +                }
> +                aml_append(method, ifctx);
> +            }
> +
> +            aml_append(method, aml_store(zero, cpu_idx));
> +            while_ctx = aml_while(aml_lless(cpu_idx, num_added_cpus));
> +            {
> +                aml_append(while_ctx, aml_call2(CPU_NOTIFY_METHOD,
> +                    aml_derefof(aml_index(new_cpus, cpu_idx)), dev_chk));
> +                aml_append(while_ctx, aml_increment(cpu_idx));
> +            }
> +            aml_append(method, while_ctx);
> +
>              aml_append(method, aml_release(ctrl_lock));
>          }
>          aml_append(cpus_dev, method);

(4) This version addresses all my requests from the previous version,
but the above method changes unfortunately break the hot-plugging of a
single CPU even.

Here's the decompiled method (using a topology with 4 possible CPUs):

   1  Method (CSCN, 0, Serialized)
   2  {
   3      Acquire (\_SB.PCI0.PRES.CPLK, 0xFFFF)
   4      Name (CNEW, Package (0x04){})
   5      Local1 = Zero
   6      Local0 = One
   7      While ((Local0 == One))
   8      {
   9          Local0 = Zero
  10          \_SB.PCI0.PRES.CCMD = Zero
  11          If ((\_SB.PCI0.PRES.CINS == One))
  12          {
  13              CNEW [Local1] = \_SB.PCI0.PRES.CDAT
  14              Local1++
  15              \_SB.PCI0.PRES.CINS = One
  16              Local0 = One
  17          }
  18          ElseIf ((\_SB.PCI0.PRES.CRMV == One))
  19          {
  20              CTFY (\_SB.PCI0.PRES.CDAT, 0x03)
  21              \_SB.PCI0.PRES.CRMV = One
  22              Local0 = One
  23          }
  24      }
  25
  26      If ((Local1 != Zero))
  27      {
  28          \_SB.PCI0.SMI0.SMIC = 0x04
  29      }
  30
  31      Local2 = Zero
  32      While ((Local2 < Local1))
  33      {
  34          CTFY (DerefOf (CNEW [Local2]), One)
  35          Local2++
  36      }
  37
  38      Release (\_SB.PCI0.PRES.CPLK)
  39  }

The problem is on line 15. When you write 1 to \_SB.PCI0.PRES.CINS, the
following happens (quoting "docs/specs/acpi_cpu_hotplug.txt"):

> write access:
>     offset:
> [...]
>     [0x4] CPU device control fields: (1 byte access)
>         bits:
>             0: [...]
>             1: if set to 1 clears device insert event, set by OSPM
>                after it has emitted device check event for the
>                selected CPU device

Because of this, by the time the SMI is raised on line 28, the firmware
will see no pending events.

The scanning (= collection into the package) has to be implemented such
that the pending events never change.

This is possible to do; the firmware already does it. The idea is to
advance the "get next CPU with event" command by manually incrementing
the CPU selector past the CPU that has a pending event, rather than by
clearing the events for the currently selected CPU. Here's the
pseudo-code:

    bool scan;
    uint32_t current_selector;
    uint32_t pending_selector;
    uint32_t event_count;
    uint32_t plug_count;
    uint32_t event;

    scan = true;
    current_selector = 0;
    event_count = 0;
    plug_count = 0;

    while (scan) {
        scan = false;

        /* the "get next CPU with pending event" command starts scanning
         * at the currently selected CPU, *inclusive*
         */
        CSEL = current_selector;
        CCMD = 0;
        pending_selector = CDAT;

        /* the "get next CPU with pending event" command may cause the
         * current selector to wrap around; in which case we're done
         */
        if (pending_selector >= current_selector) {
            current_selector = pending_selector;

            /* if there's no pending event for the currently selected
             * CPU, we're done
             */
            if (CINS) {
                /* record INSERT event for currently selected CPU, and
                 * continue scanning
                 */
                CNEW[event_count] = current_selector;
                CEVT[event_count] = 1;
                event_count++;
                plug_count++;
                scan = true;
            } else if (CRMV) {
                /* record REMOVE event for currently selected CPU, and
                 * continue scanning
                 */
                CNEW[event_count] = current_selector;
                CEVT[event_count] = 3;
                event_count++;
                scan = true;
            }

            if (scan) {
                scan = false;
                /* advance past this CPU manually (as we must not clear
                 * events pending for this CPU)
                 */
                current_selector++;
                if (current_selector < possible_cpu_count) {
                    scan = true;
                }
            }
        }
    }

    /* raise "hotplug SMI" now if at least one INSERT event has been
     * collected
     */
    if (plug_count > 0) {
        SMIC = 0x04;
    }

    /* notify the OS about all events, and clear pending events (same
     * order as before)
     */
    event = 0;
    while (event < event_count) {
        CTFY(CNEW[event], CEVT[event]);

        CSEL = CNEW[event];
        if (CEVT[event] == 1) {
            CINS = 1;
        } else if (CEVT[event] == 3) {
            CRMV = 1;
        }

        event++;
    }


Thanks
Laszlo