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

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

[Igor Mammedov]

v2:
  - AML: clean is_inserted flag only after SMI callback
  - make x-smi-cpu-hotunplug false by default
  - massage error hint on not supported unplug
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 (7):
  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
  acpi: add aml_land() and aml_break() primitives
  tests: acpi: mark to be changed tables in
    bios-tables-test-allowed-diff
  x68: acpi: trigger SMI before sending hotplug Notify event to OSPM
  tests: acpi: update acpi blobs with new AML

 include/hw/acpi/aml-build.h       |   2 +
 include/hw/acpi/cpu.h             |   1 +
 include/hw/i386/ich9.h            |   4 ++
 include/hw/i386/pc.h              |   3 ++
 hw/acpi/aml-build.c               |  16 ++++++
 hw/acpi/cpu.c                     |  80 ++++++++++++++++++++++++++++--
 hw/acpi/ich9.c                    |  24 ++++++++-
 hw/i386/acpi-build.c              |  35 ++++++++++++-
 hw/i386/pc.c                      |  17 ++++++-
 hw/i386/pc_piix.c                 |   1 +
 hw/i386/pc_q35.c                  |   1 +
 hw/isa/lpc_ich9.c                 |  16 ++++++
 tests/data/acpi/pc/DSDT           | Bin 4934 -> 5009 bytes
 tests/data/acpi/pc/DSDT.acpihmat  | Bin 6258 -> 6334 bytes
 tests/data/acpi/pc/DSDT.bridge    | Bin 6793 -> 6868 bytes
 tests/data/acpi/pc/DSDT.cphp      | Bin 5397 -> 5473 bytes
 tests/data/acpi/pc/DSDT.dimmpxm   | Bin 6587 -> 6663 bytes
 tests/data/acpi/pc/DSDT.ipmikcs   | Bin 5006 -> 5081 bytes
 tests/data/acpi/pc/DSDT.memhp     | Bin 6293 -> 6368 bytes
 tests/data/acpi/pc/DSDT.numamem   | Bin 4940 -> 5015 bytes
 tests/data/acpi/q35/DSDT          | Bin 7678 -> 7753 bytes
 tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9078 bytes
 tests/data/acpi/q35/DSDT.bridge   | Bin 7695 -> 7770 bytes
 tests/data/acpi/q35/DSDT.cphp     | Bin 8141 -> 8217 bytes
 tests/data/acpi/q35/DSDT.dimmpxm  | Bin 9331 -> 9407 bytes
 tests/data/acpi/q35/DSDT.ipmibt   | Bin 7753 -> 7828 bytes
 tests/data/acpi/q35/DSDT.memhp    | Bin 9037 -> 9112 bytes
 tests/data/acpi/q35/DSDT.mmio64   | Bin 8808 -> 8883 bytes
 tests/data/acpi/q35/DSDT.numamem  | Bin 7684 -> 7759 bytes
 tests/data/acpi/q35/DSDT.tis      | Bin 8283 -> 8358 bytes
 30 files changed, 192 insertions(+), 8 deletions(-)

-- 
2.26.2

[PATCH v2 1/7] 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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
v2:
  - rebase on top of 5.1 (move compat values to 5.1 machine)
  - make "x-smi-cpu-hotunplug" false by default (Laszlo Ersek <lersek@redhat.com>)
---
 include/hw/i386/ich9.h |  2 ++
 include/hw/i386/pc.h   |  3 +++
 hw/i386/pc.c           |  6 +++++-
 hw/i386/pc_piix.c      |  1 +
 hw/i386/pc_q35.c       |  1 +
 hw/isa/lpc_ich9.c      | 13 +++++++++++++
 6 files changed, 25 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/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 3d7ed3a55e..fe52e165b2 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -193,6 +193,9 @@ void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
                        const CPUArchIdList *apic_ids, GArray *entry);
 
+extern GlobalProperty pc_compat_5_1[];
+extern const size_t pc_compat_5_1_len;
+
 extern GlobalProperty pc_compat_5_0[];
 extern const size_t pc_compat_5_0_len;
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 47c5ca3e34..99c6bdbab4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -97,8 +97,12 @@
 #include "fw_cfg.h"
 #include "trace.h"
 
-GlobalProperty pc_compat_5_0[] = {
+GlobalProperty pc_compat_5_1[] = {
+    { "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
 };
+const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
+
+GlobalProperty pc_compat_5_0[] = { };
 const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
 
 GlobalProperty pc_compat_4_2[] = {
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index b789e83f9a..d56f2e1b96 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -433,6 +433,7 @@ static void pc_i440fx_5_1_machine_options(MachineClass *m)
     m->alias = "pc";
     m->is_default = true;
     pcmc->default_cpu_version = 1;
+    compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
 }
 
 DEFINE_I440FX_MACHINE(v5_1, "pc-i440fx-5.1", NULL,
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index a3e607a544..0ca1146a59 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -359,6 +359,7 @@ static void pc_q35_5_1_machine_options(MachineClass *m)
     pc_q35_machine_options(m);
     m->alias = "q35";
     pcmc->default_cpu_version = 1;
+    compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
 }
 
 DEFINE_Q35_MACHINE(v5_1, "pc-q35-5.1", NULL,
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index cd6e169d47..19f32bed3e 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -373,6 +373,15 @@ 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 +756,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, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.26.2

[PATCH v2 2/7] 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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Laszlo Ersek <lersek@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 99c6bdbab4..f07a8ab6c4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1499,6 +1499,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 v2 3/7] 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 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>
Tested-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
v2:
 - fix typo in commit message
 - drop 5.1 version from hint message (Laszlo)
---
 hw/acpi/ich9.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 0acc9a3107..95cb0f935b 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -460,6 +460,18 @@ 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 a version having "
+                                    "x-smi-cpu-hotunplug=on and firmware that "
+                                    "supports CPU hot-unplug with SMM");
+            return;
+        }
+
         acpi_cpu_unplug_request_cb(hotplug_dev, &lpc->pm.cpuhp_state,
                                    dev, errp);
     } else {
-- 
2.26.2

[PATCH v2 4/7] acpi: add aml_land() and aml_break() primitives

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/acpi/aml-build.h |  2 ++
 hw/acpi/aml-build.c         | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index d27da03d64..e213fc554c 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -291,6 +291,7 @@ Aml *aml_store(Aml *val, Aml *target);
 Aml *aml_and(Aml *arg1, Aml *arg2, Aml *dst);
 Aml *aml_or(Aml *arg1, Aml *arg2, Aml *dst);
 Aml *aml_lor(Aml *arg1, Aml *arg2);
+Aml *aml_land(Aml *arg1, Aml *arg2);
 Aml *aml_shiftleft(Aml *arg1, Aml *count);
 Aml *aml_shiftright(Aml *arg1, Aml *count, Aml *dst);
 Aml *aml_lless(Aml *arg1, Aml *arg2);
@@ -300,6 +301,7 @@ Aml *aml_increment(Aml *arg);
 Aml *aml_decrement(Aml *arg);
 Aml *aml_index(Aml *arg1, Aml *idx);
 Aml *aml_notify(Aml *arg1, Aml *arg2);
+Aml *aml_break(void);
 Aml *aml_call0(const char *method);
 Aml *aml_call1(const char *method, Aml *arg1);
 Aml *aml_call2(const char *method, Aml *arg1, Aml *arg2);
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index f6fbc9b95d..14b41b56f0 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -556,6 +556,15 @@ Aml *aml_or(Aml *arg1, Aml *arg2, Aml *dst)
     return build_opcode_2arg_dst(0x7D /* OrOp */, arg1, arg2, dst);
 }
 
+/* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefLAnd */
+Aml *aml_land(Aml *arg1, Aml *arg2)
+{
+    Aml *var = aml_opcode(0x90 /* LandOp */);
+    aml_append(var, arg1);
+    aml_append(var, arg2);
+    return var;
+}
+
 /* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefLOr */
 Aml *aml_lor(Aml *arg1, Aml *arg2)
 {
@@ -629,6 +638,13 @@ Aml *aml_notify(Aml *arg1, Aml *arg2)
     return var;
 }
 
+/* ACPI 1.0b: 16.2.5.3 Type 1 Opcodes Encoding: DefBreak */
+Aml *aml_break(void)
+{
+    Aml *var = aml_opcode(0xa5 /* BreakOp */);
+    return var;
+}
+
 /* helper to call method without argument */
 Aml *aml_call0(const char *method)
 {
-- 
2.26.2

[PATCH v2 5/7] 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 v2 6/7] x68: acpi: trigger SMI before sending hotplug Notify event to OSPM

[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 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 processed
by the next CPU_SCAN_METHOD, when pending SCI is handled.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
 - clear insert event after firmware has returned
   control from SMI. (Laszlo Ersek <lersek@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          | 80 +++++++++++++++++++++++++++++++++++++++---
 hw/i386/acpi-build.c   | 35 +++++++++++++++++-
 hw/isa/lpc_ich9.c      |  3 ++
 5 files changed, 115 insertions(+), 6 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..4864c3b396 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,21 +474,61 @@ 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 *uid = aml_local(3);
+            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));
+            /* start scan from the 1st CPU */
+            aml_append(method, aml_store(zero, uid));
+            while_ctx = aml_while(aml_land(aml_equal(has_event, one),
+                                  aml_lless(uid, aml_int(arch_ids->len))));
             {
                  /* clear loop exit condition, ins_evt/rm_evt checks
                   * will set it to 1 while next_cpu_cmd returns a CPU
                   * with events */
                  aml_append(while_ctx, aml_store(zero, has_event));
+
+                 aml_append(while_ctx, aml_store(uid, cpu_selector));
                  aml_append(while_ctx, aml_store(next_cpu_cmd, cpu_cmd));
+
+                 /*
+                  * wrap around case, scan is complete, exit loop.
+                  * It happens since events are not cleared in scan loop,
+                  * so next_cpu_cmd continues to find already processed CPUs.
+                  */
+                 ifctx = aml_if(aml_lless(cpu_data, uid));
+                 {
+                     aml_append(ifctx, aml_break());
+                 }
+                 aml_append(while_ctx, ifctx);
+
+                 aml_append(while_ctx, aml_store(cpu_data, uid));
                  ifctx = aml_if(aml_equal(ins_evt, one));
                  {
-                     aml_append(ifctx,
-                         aml_call2(CPU_NOTIFY_METHOD, cpu_data, dev_chk));
-                     aml_append(ifctx, aml_store(one, ins_evt));
+                     /* cache added CPUs to Notify/Wakeup later */
+                     aml_append(ifctx, aml_store(uid,
+                         aml_index(new_cpus, num_added_cpus)));
+                     aml_append(ifctx, aml_increment(num_added_cpus));
                      aml_append(ifctx, aml_store(one, has_event));
                  }
                  aml_append(while_ctx, ifctx);
@@ -493,14 +536,41 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
                  ifctx = aml_if(aml_equal(rm_evt, one));
                  {
                      aml_append(ifctx,
-                         aml_call2(CPU_NOTIFY_METHOD, cpu_data, eject_req));
+                         aml_call2(CPU_NOTIFY_METHOD, uid, eject_req));
                      aml_append(ifctx, aml_store(one, rm_evt));
                      aml_append(ifctx, aml_store(one, has_event));
                  }
                  aml_append(else_ctx, ifctx);
                  aml_append(while_ctx, else_ctx);
+                 aml_append(while_ctx, aml_increment(uid));
+            }
+            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);
+            }
+
+            /* Notify OSPM about new CPUs and clear insert events */
+            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_store(one, ins_evt));
+                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 19f32bed3e..8124d20338 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -647,6 +647,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 v2 7/7] tests: acpi: update acpi blobs with new AML

[Igor Mammedov]

Update CPU hotplug AML with following changes

             Method (CSCN, 0, Serialized)
             {
                 Acquire (\_SB.PCI0.PRES.CPLK, 0xFFFF)
+                Name (CNEW, Package (0x01){})
+                Local1 = Zero
                 Local0 = One
-                While ((Local0 == One))
+                Local3 = Zero
+                While (((Local0 == One) && (Local3 < One)))
                 {
                     Local0 = Zero
+                    \_SB.PCI0.PRES.CSEL = Local3
                     \_SB.PCI0.PRES.CCMD = Zero
+                    If ((\_SB.PCI0.PRES.CDAT < Local3))
+                    {
+                        Break
+                    }
+
+                    Local3 = \_SB.PCI0.PRES.CDAT
                     If ((\_SB.PCI0.PRES.CINS == One))
                     {
-                        CTFY (\_SB.PCI0.PRES.CDAT, One)
-                        \_SB.PCI0.PRES.CINS = One
+                        CNEW [Local1] = Local3
+                        Local1++
                         Local0 = One
                     }
                     ElseIf ((\_SB.PCI0.PRES.CRMV == One))
                     {
-                        CTFY (\_SB.PCI0.PRES.CDAT, 0x03)
+                        CTFY (Local3, 0x03)
                         \_SB.PCI0.PRES.CRMV = One
                         Local0 = One
                     }
+
+                    Local3++
+                }
+
+                Local2 = Zero
+                While ((Local2 < Local1))
+                {
+                    CTFY (DerefOf (CNEW [Local2]), One)
+                    \_SB.PCI0.PRES.CINS = One
+                    Local2++
                 }

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/qtest/bios-tables-test-allowed-diff.h |  19 -------------------
 tests/data/acpi/pc/DSDT                     | Bin 4934 -> 5009 bytes
 tests/data/acpi/pc/DSDT.acpihmat            | Bin 6258 -> 6334 bytes
 tests/data/acpi/pc/DSDT.bridge              | Bin 6793 -> 6868 bytes
 tests/data/acpi/pc/DSDT.cphp                | Bin 5397 -> 5473 bytes
 tests/data/acpi/pc/DSDT.dimmpxm             | Bin 6587 -> 6663 bytes
 tests/data/acpi/pc/DSDT.ipmikcs             | Bin 5006 -> 5081 bytes
 tests/data/acpi/pc/DSDT.memhp               | Bin 6293 -> 6368 bytes
 tests/data/acpi/pc/DSDT.numamem             | Bin 4940 -> 5015 bytes
 tests/data/acpi/q35/DSDT                    | Bin 7678 -> 7753 bytes
 tests/data/acpi/q35/DSDT.acpihmat           | Bin 9002 -> 9078 bytes
 tests/data/acpi/q35/DSDT.bridge             | Bin 7695 -> 7770 bytes
 tests/data/acpi/q35/DSDT.cphp               | Bin 8141 -> 8217 bytes
 tests/data/acpi/q35/DSDT.dimmpxm            | Bin 9331 -> 9407 bytes
 tests/data/acpi/q35/DSDT.ipmibt             | Bin 7753 -> 7828 bytes
 tests/data/acpi/q35/DSDT.memhp              | Bin 9037 -> 9112 bytes
 tests/data/acpi/q35/DSDT.mmio64             | Bin 8808 -> 8883 bytes
 tests/data/acpi/q35/DSDT.numamem            | Bin 7684 -> 7759 bytes
 tests/data/acpi/q35/DSDT.tis                | Bin 8283 -> 8358 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..716d412a2474e0990136560b0340f808e490b514 100644
GIT binary patch
delta 304
zcmX@6Hc_3+CD<ioqA&vk<I0U(TiBS~ttTH~Tg~KUFgcg~FO#$I<Qxu}dJbnl*Ki>w
z#sY@K0>*>_hU7&~JQF4-FiuSd3MCXI$LO=f2Rp?FIC~ld1i1z~2fO+dFrW)M`?@R;
zpNcN(;uw;=v;dpv0+q>7;d;2io_@iM1<4&C=Oi+ef*iBZe1R!U9b8$EZy2L<h+AYb
t7jpq4y4gTUpuW=N0*0hT8dH-JK?<9}MkX;ZVk!q($yA!O*_M-&3jjiOSquOG

delta 172
zcmbQJeoT$aCD<jzO_+g!asNiHEo@9aW|I%Ft!DDqo}A16m&t=~at?=#N&#cSB1i7Y
z35*2{2?Y!>`YiFmPVoWGo(2IyuEEaEzAg*gStkGI5EXY0af?Kkc5w`uT)-)23N&P)
bH_HMKmdWTUf_%dmap`p6l-k_E$;kx(->We%

diff --git a/tests/data/acpi/pc/DSDT.acpihmat b/tests/data/acpi/pc/DSDT.acpihmat
index 2e5e02400b1bd2842989d395c573fc593f45503b..198e87dfc2d7df261f2f51d685d90db5c6eb8929 100644
GIT binary patch
delta 295
zcmexlu+NaoCD<iop9BK~W7<ZpEo@9~j*}0tt!DBx+nmeJ!N}<%>>TXu#}TbOnUO=-
z?EilbXFu0)Att5*hQtEKgaU@-Mb11ECMPgXP38iMB@`sb=(EHJJH-b$dm01;xduB2
zyZTI?&mmSXJ{4WY#W5s#X#qCT1uB!#g+2X(84Hp-K(;3`l!EMDXuiM{Ls^h-7^8ED
zTVyg9a{(i=8I$cex%5ht3mB3XX-rK@1gU8To0i1Dh^i9gXrOJSNt5F_#5Tuqitqyf
D&!|=a

delta 215
zcmdmI_{o6FCD<jTNP>ZZv3Dca7B(g?o5=^*Rx|k;Zq8-rU}X2;a}IX)<CwgWQ=*<R
zVUZ*E<OIe7hJ*r!7=4!bV5j&1XHSEGAlG1LXJ3~E?ktnhg+2X(8J$DiBGJWN977lj
o7|}(6stXts7J9QR@W3=mI><K+msyj2IHe}XbBJwT#VNuM0G&-dEC2ui

diff --git a/tests/data/acpi/pc/DSDT.bridge b/tests/data/acpi/pc/DSDT.bridge
index 623c4c03585c47d4d28adc611823b7cce8f4a5c7..e105bb152eff5b064b1a02aefa70e4cc3e18ffc5 100644
GIT binary patch
delta 304
zcmeA)y<*Dc66_LkMT&ue@zX}GEo@Bg){_sgt!DBvn4HW0m&sXpat?<~J%_WOYq$^-
zV*x{A0b@b|L-Hago(Yo^7^fx!g%S#qWAs_#gPr07oIMQ!f?R{0gI#?J7|;cseO(ra
zPem7XaSTabT7XS-fy!j4a6Q~$PrqQsg5(a6a}pU!L5^8yzQ7cw4z4W7H;mCa#4R$J
ti@AUi-E5#FP+w_s0YlOvjj2hAAcf6fBa;{yF_i<YWGYSCY|H5*2mlk)S_}XH

delta 172
zcmca&+G)z=66_MvDaF9R$gz=Y3mcP<+2jLktC{?@C+D*NW%A&goWmibQoxw7$dP+;
z0%HL~LIFdJK1+PCQ+$B4r$IoFYp}Dkugd~=mdXD)M8%y$+#=DXT^vIu7jTN10u5Q{
b&9cCQWiq;oAm1=XTsj>%r8akP`UnC5wjePt

diff --git a/tests/data/acpi/pc/DSDT.cphp b/tests/data/acpi/pc/DSDT.cphp
index e0a43ccdadae150c0f39599c85e4e21ed8fff2a4..01c8baf29575aa9c90489891ea14a707fccbf326 100644
GIT binary patch
delta 232
zcmbQL^-zn;CD<h-QIvs!k#i&07B(jLh{*@oRx^3|Zq8-5WMp>{b`Ey-<CyHvA+E^b
z?B^OT#Kcy>kXXQ&P{5G9$eCxt<OIg4$y`9Ogo5PB|Jg++U*Zs95}!JmkyDsCd1(QN
zc1d2KGC4+{B|g|GKET=2ARx#!*xA!Bn6V(a17u_(Ln+9>h2{%PF_Z=QhA}#axJ4#U
rj^~upD@`t7NLr*ZH7OA!)(qC2#K2g<h{g2Mq{$OF#5VVHws8Xh#TG-G

delta 200
zcmaE;HC2ntCD<iIRFr{%(PAUl7B(jD;K>KrRx|l|Y|dr3WMuc?a}IX)<CyHvA(79R
zu*i{nasp!kLqY*Vj6O?zuv2`1v!_8okZZ8Bv#-kncb3WM!k&J?jLso$k?7(sjv<T%
pjOe03)dh?R3%ywuc(6>K+{h^`flKq`T286S6F9^+-{x%N1^{(<H|zia

diff --git a/tests/data/acpi/pc/DSDT.dimmpxm b/tests/data/acpi/pc/DSDT.dimmpxm
index 21eb065a0ee3bd96f1a2e7601aa83fefa833349a..a766128d7a37ba5577699ec6fca1e418f7f2c905 100644
GIT binary patch
delta 306
zcmdmO+-}0<66_MfF2%sW_+=y47B(ga-^mBqRx>#}ZO&y^U}SO;p4`tNQ_tb-=Nc}=
z#8SYJSiqQ2z>vJinP<Y}1jeb!TtKmeg5(%|miS<&_yA{5gMc8{VCP_0p8^JSL1$l=
z1>#fDMO_?2l9v`>6J4M(86rIS8V6r}L2?Jk6^RU`AU7;DUtkKAMc5kT8^-7y;ue|A
x#azIMZX!?;sIN4+fFWs-#?+)lkiurLkx2}Un94o<f=iR4)#0XW&gKjd0093fS%d%p

delta 212
zcmZoS*=@|_66_MPTatl+QDGz37B(gqkI4tvRx`QVZq8*_U}W;(o7~SK!&AVRu*i{n
zasp!kL&D_y9AfqEER$pOS>l78;scyL4FZB(gPlG7f*GAd+#=D%T^vIg3mDNwfvSOq
iF7#$u;DKpWkZ%|+qqvwsW{IF$4O9;_Z1Wn<XaNAP9Xk&I

diff --git a/tests/data/acpi/pc/DSDT.ipmikcs b/tests/data/acpi/pc/DSDT.ipmikcs
index b8f08f266b5735fe6967d4e105ee6b3662dad7e6..e6de2da36e01436be3d5d05a2bebe5666ccfbe37 100644
GIT binary patch
delta 268
zcmeBEzp2jU66_LkQ<#B)(RL%(A2ud;>&YDKtC_qECU0f`TkkCF9PI4J5v?4f&k`T(
z6d&O1X%G<P8tfe4<Ng0XhqIq+xDXR#0YhQ|V?qH#@**dm36m2TrzQi15(<*hH3hr+
z6fmF*I{UgT5TA-J>f#uZytKd;oA3ga$>_qKe!+|d$sHi)Br=qO9JA1Tf$3y>PD#e(
p$=RGzVx`Fi3`vVLrY0pihqy&HgH<LmOjh6&V=7JB+{($x1po~KP!0e9

delta 182
zcmcbq-lxvx66_MvC(OXW7`&0|4;zz@*<=p()lB}{lee<}W%uB74tDnAnEaMg!iF(n
zkt6ry1jYh}gaU>beU|uOr}zM8PlJFU*I;L7UzY{$ER)fNJ^g|iokQFrCmV7K%QF@*
lVv#RkOjzj6vcQ97avrB7NSzcHGp52I-!P!U%@;U1xd2{IGED#g

diff --git a/tests/data/acpi/pc/DSDT.memhp b/tests/data/acpi/pc/DSDT.memhp
index 9a9418f4bde5fb18883c244ea956122e371ff01a..71f34760fe10a7f4face8d9f956b96668d5d2951 100644
GIT binary patch
delta 304
zcmbPg_`s0MCD<k8fdm5sW6wseEo@A_){_sgt!8pCoSe)4m&sXpat?<~J%_WOYq$^-
zV*x{A0b@b|L-Hago(Yo^7^fx!g%S#qWAs_#gPr07oIMQ!f?R{0gI#?J7|;cseO(ra
zPem7XaSTabT7XS-fy!j4a6Q~$PrqQsg5(a6a}pU!L5^8yzQ7cw4z4W7H;mCa#4R$J
ti@AUi-E5#FP+w_s0YlOvjj2hAAcf6fBa;{yF_i<YWGYSCY|9zJ4*<KkS;qhX

delta 172
zcmaE0IMtBLCD<iosssZA<Hn6#TiBSK%qJgUTg~LEGdY+2FOvt~<Qxtel>)|uMULE)
z6Br8^5(*e%^jYGAo#F$WJq-eaT!WpReO(r~vrPWaAu8@1;ueW6?cx|Rxqws56llmo
bZ<YlfER)ey1o?(B;?n8BDYdzSGlCxgB{(ta

diff --git a/tests/data/acpi/pc/DSDT.numamem b/tests/data/acpi/pc/DSDT.numamem
index 6eec385c2ec00544c6eaa7e19d32b2ccd5a51915..88070659ad48a4ed5a6019b8234abfdd29dd4703 100644
GIT binary patch
delta 304
zcmX@3HeH>|CD<iox-bI+qsvCFEo@A_){_sgt!8pCoSe)4m&sXpat?<~J%_WOYq$^-
zV*x{A0b@b|L-Hago(Yo^7^fx!g%S#qWAs_#gPr07oIMQ!f?R{0gI#?J7|;cseO(ra
zPem7XaSTabT7XS-fy!j4a6Q~$PrqQsg5(a6a}pU!L5^8yzQ7cw4z4W7H;mCa#4R$J
ti@AUi-E5#FP+w_s0YlOvjj2hAAcf6fBa;{yF_i<YWGYSCY|F{d1pwj@SiArL

delta 172
zcmbQPenySUCD<jzN0@<uF>@o=7B(g)^T`L;Rx`QkOwMKh%jCf~Ifp|=rGPPEkt6ry
z1jYh}gaU>beU|uOr}zM8PlJFU*I;L7UzY{$ER+9ph>AOhxJ9B%yEukSF5nb11sbx@
bn`MCq%VcyFLB3&(xO6&jN^S1o<mUnatOhXO

diff --git a/tests/data/acpi/q35/DSDT b/tests/data/acpi/q35/DSDT
index e63676d7a63afec714debeb465ee478ea4714337..27515d30b233e0eb4563f30c324e1cb538e5c761 100644
GIT binary patch
delta 230
zcmexoebR=@CD<jzQ;vawamq%n2@*{1)|2N*tY-2unCvO}m)%*|IoR2cWAb}RaRm-%
zKi6;}CdLAW!~({I0*2&8PCOGPCooP;1_~t<Bu}0%DLT1GN`y&#>g0t|!pzA_3qZ6>
z@&c8~G5Rd=!A|i3&YlJVL9W5ho_@iM1<4&C6B8LqLFO$qUto%%EXX&E(K*B|GI{cU
pDJi|u<N}7IMH*9+5<z0kVBJX!j0KEXOfOBEY#=4JSzr1*69AQCMsolF

delta 214
zcmX?U^Us>gCD<k8pDY6d<LZrE6C{{?%qGu~Sk2_GJ=s(8FS`eybFi}?$K*z7iF(F_
zMULE)6Br8^5(*e%^jYGAo#F$WJq-eaT!WpReO(r~vrI-8_Vf#8bPjQgL>G5)3}Gx_
nL>C3BE?`Vp=*_ag1Jfw!Am1=tW=)onmYQrJCAPUr`aBZ=mR>#8

diff --git a/tests/data/acpi/q35/DSDT.acpihmat b/tests/data/acpi/q35/DSDT.acpihmat
index cd97b819824e4140d087e465d179b71775d6a494..1ebd77e15912c54d5d6ff2d6c23475b283dd3555 100644
GIT binary patch
delta 252
zcmZ4G_RWpUCD<jTOqqd!F>oW-1PLZL$H{XfRx^2;ZT6JpU}Sd@b`Ey-<Cr{AQoNqS
z+0Qjxh>59yA+dllp@1QIku%SP$q9^8levIm2?fb9`YiFmPVoWGo(2IyuEEa1u090}
z=z`9^E(^q`qKmpXh9oa7z$UstWimuq0b#JGUoc}qatFvgi43J6*DN$&U^@Aplq6&F
pWG-nbvC`xMhNMLrQ<D;%L);>p!77s&ChwOLV=7JBtRl_E0RVL(NvHq-

delta 183
zcmez7w#tpmCD<iIOPPUzv0@|F1PLZDo5^z|Rx|k;ZuXSqU}X2;a}IX)<Cr{AQo@EY
zVUZ*E<OIe7hJ*r!7=4!bV5j&1XHSEGAlG1LXJ3~E?ktnhg+2X(8J$DiA}3#z5|(Ey
mV8kL{z?iVmn`MCq%Va)jNsu}zE@n)HLB3%?g_{ec**E|&IWjK*

diff --git a/tests/data/acpi/q35/DSDT.bridge b/tests/data/acpi/q35/DSDT.bridge
index 8b0fb497dbbaeba18e9d0e1503de4396f1c230b0..0f5531792789f7874382477c100504d1c5940ed1 100644
GIT binary patch
delta 230
zcmeCTxn;xU66_KZCC9+PXtI%Of&`Ph_2fActC_qECVNW$Wp@^K4tDnAnEYN+T!F*c
z&ox|#iLrnov4An5fFXI26VHUn35-*dfkFud$&=?xicT()5@8abI(ea#Fmv+K0ub$z
zyg+4gj6O?zuv2`1v!_8okZZ8Br(ZB*L2?Jk#6*Ttka-Ku7nouw3-S$PbPjQgOrHE-
pN=mOZxqu;Qk;c@dM37iBSa%WwV*w);(@T>k8%T+5)|Y<31OWeXMc4oU

delta 214
zcmca*({ID&66_MfFUP>Z=(Uk+f&`O~+2lDAtC{?@Cwof%W%uB74tDnAnA|8WQO}sL
z$dP+;0%HL~LIFdJK1+PCQ+$B4r$IoFYp}Dkugd~=mdWVCo_@iM&LM7*=;AJpA&do#
n=%PT?1&j#`y;&A`U>YSI<Qs;|tjRLcQj-m&#5Pw+KVSj?KQBBw

diff --git a/tests/data/acpi/q35/DSDT.cphp b/tests/data/acpi/q35/DSDT.cphp
index d9bb414e9bf15d9b9149f38c9bb5d8b993f88650..e5e7ee11b38e42cb97000ec5fd38b51fcd38d34b 100644
GIT binary patch
delta 270
zcmX?WKhuHBCD<iIQh|Yi@ykZ82@*{15tHXgtY-4^-Rvo8$yo0q>>TXu#}Ta@qt6l_
z>=Ym1>}e1X<QnW8;N$)OKZmoQYq$^-TLD920b@b|L-HbLo(Yo^7^fz40mTvulF@Yq
zyZRI`pbI+tx-1Z%iZ1Hn7?Qlyt^k|x0+q?=!k&J?j0MRZAonCPl!9Ed(0qaEWPWK$
r#^lM?(o$lj$ps8ai!`PtB|3+=MK*&~CNWI@ASK3BnzT7ox`G7&2#ZkB

delta 188
zcmbQ~aMqs7CD<k8tULn)qsm6E2@*`+!IS4mtY-4_*z753$;j!!=N#<p#}TbO*-=K=
z{{R01#)L(V+>;X+3m6g#7-IBU;)9*y1Drh#0)kwFot=GM7PzxaMi=(<3ubf<af_VH
rC?hP-Sip!y9%$}DZ<YlfER*e}B|++>xR@~&2Kj~o6>eTAUBLnX$bvLy

diff --git a/tests/data/acpi/q35/DSDT.dimmpxm b/tests/data/acpi/q35/DSDT.dimmpxm
index 29f19b22a38f9d8e7dc9870f0c1aa3d4470643ff..68859b7c9ff5ed9566650d75924d8ca8868c22fc 100644
GIT binary patch
delta 306
zcmezDvEP%+CD<iozX}5b<DrdQ6C{`%d?(M5Sk2_@wAoWqfsx5YcyhLsOg)FQpKG`f
z6H5U@VgX}90Yma4XPya@6Bwr^a{<K?3X)^=S>l78;scyL4FZB(gPnt2eF_-R1)Y6e
z7Kl$p7j<zANnTojO>}|EWQcG*!eCFoV8(*v4v>2i8A?H}S!lk%6siuPEXX&E(K*B|
wGMS6HfDzqnpd?UVX>tKW(jtwiNr@nZ&0r&w7#J~?d-^d4mnLntkv8W90J=_Di2wiq

delta 173
zcmdn*`PqZZCD<jTScQRs@$E*g2@*^$9+T%ttY&hz-Rvo;z{upmH#u8MMx}r;VUZ*E
z<OIe7hJ*r!7=4!bV5j&1XHSEGAlG1LXJ3~E?kto4ONoj*hqy(eOS?FROfHZXGX)y5
c(3@p}2g_u16+ymXjJR|<NK0+*kT&N805Y91;s5{u

diff --git a/tests/data/acpi/q35/DSDT.ipmibt b/tests/data/acpi/q35/DSDT.ipmibt
index e8dea1ea42af765babcb747af998b0d912abdcbd..a34df87fddbe85a83f713913de58f9dd8c66ffa4 100644
GIT binary patch
delta 250
zcmX?UGsTw6CD<ioiW~z2<B5%2w<Va|ttUU1Sk2^RFnNmPUv_6<=U`_)j>#7##p^ko
z{anL^m>3Hf5(^j;3K)_XIq^)GoWM9W87P!ckQ}4W5+CdoAK>h15D?@V>>TXsQ^0^O
z=<MsVKzu5?sEcDr^3noqq6<_eLxttx27CGiGZrLwfSi-aPzrL)Lh}WtljWo(8Ivb_
nNlS^9CKoUyEz+2pl;|Ad7TF9|nZz*pzmynLY0~Bt>8DHp6X;Bs

delta 228
zcmbPYd(wu>CD<jzQ;vaw@!m$R+Y(GZW|N;wtY-4po;*eJFS`eybFi}?$K;EW67`G;
ziyXNpComQ;Bor{j=(EHJJH-b$dm01;xduBs`?@S}XPJyH?CBTG=p5n}i7xKq7{XY<
rh%O3LUBH;I(3@p}2c}Vz<D@xs(RBdL;bO)#D9ASqXwc^M(odNHC~!hJ

diff --git a/tests/data/acpi/q35/DSDT.memhp b/tests/data/acpi/q35/DSDT.memhp
index dca76db15b943122efd5c200cb54ce3dbc97a55f..8ceeada582a57f7217ebcdc572a6599e8dd798fd 100644
GIT binary patch
delta 230
zcmX@>Hp89ECD<iohB5;KWBEp|2@*`c)|2N*tY&gBoa`z2m)%*|IoR2cWAb}RaRm-%
zKi6;}CdLAW!~({I0*2&8PCOGPCooP;1_~t<Bu}0%DLT1GN`y&#>g0t|!pzA_3qZ6>
z@&c8~G5Rd=!A|i3&YlJVL9W5ho_@iM1<4&C6B8LqLFO$qUto%%EXX&E(K*B|GI{cU
pDJi|u<N}7IMH*9+5<z0kVBJX!j0KEXOfOBEY#=4JSzlU?0|0%*MX&$>

delta 214
zcmbQ?e%6i4CD<jzSDAr<an44r2@*_B=9A}0tY&i6nd~X~m)(QUIoR2cV{)UkL_K4|
zB1i7Y35*2{2?Y!>`YiFmPVoWGo(2IyuEEaEzAg*gStg?kd-?@4I)}JLqKmsYhA<W|
nqKg7m7ceF)^k!M$foYU<kZ%|+vnI<(OHDSA65CuQEyn=>vyD6w

diff --git a/tests/data/acpi/q35/DSDT.mmio64 b/tests/data/acpi/q35/DSDT.mmio64
index 6d8facd9e179140682ad5d4302f3dad14dbec342..318d5de996d97aaf175e232157e48a443ef7bbbc 100644
GIT binary patch
delta 230
zcmaFive}i(CD<iovl0UXqwz+r2@*`c)|2N*tY&gBoa`z2m)%*|IoR2cWAb}RaRm-%
zKi6;}CdLAW!~({I0*2&8PCOGPCooP;1_~t<Bu}0%DLT1GN`y&#>g0t|!pzA_3qZ6>
z@&c8~G5Rd=!A|i3&YlJVL9W5ho_@iM1<4&C6B8LqLFO$qUto%%EXX&E(K*B|GI{cU
pDJi|u<N}7IMH*9+5<z0kVBJX!j0KEXOfOBEY#=4JSzp?c9RP>QMbZEO

delta 214
zcmdn&`oe|FCD<h-Ly3WbF=8Xv1PLZ5^T~50Rx`QkO!k!g%kIJF9PI4JF}YD%qMk8f
zkt6ry1jYh}gaU>beU|uOr}zM8PlJFU*I;L7UzY{$ER)fNJ^g|iokQFr(ZyXHLl_Gf
n(M5r(3m6j?db2F>z%)ua$Ttj^S(9a?r6wCliEXZuwqyqYy%Rh%

diff --git a/tests/data/acpi/q35/DSDT.numamem b/tests/data/acpi/q35/DSDT.numamem
index 737325dc3082fdf06283857811f6f5174e0ff2a9..a964d7c6a4b2a781f8e8791d32e23eadf994b502 100644
GIT binary patch
delta 230
zcmZp%Id8+|66_M<FUP>ZsK1eGf&`PV_2fActC<`OCwof%Wp@^K4tDnAnEYN+T!F*c
z&ox|#iLrnov4An5fFXI26VHUn35-*dfkFud$&=?xicT()5@8abI(ea#Fmv+K0ub$z
zyg+4gj6O?zuv2`1v!_8okZZ8Br(ZB*L2?Jk#6*Ttka-Ku7nouw3-S$PbPjQgOrHE-
pN=mOZxqu;Qk;c@dM37iBSa%WwV*w);(@T>k8%T+5)|bA@1OVH=MWg@#

delta 214
zcmX?a(_+Kr66_MfBFDhM7_yOTf&`P3`Q$kgtC?JNCVNW$W%uB74tDnAnA|8WQO}sL
z$dP+;0%HL~LIFdJK1+PCQ+$B4r$IoFYp}Dkugd~=mdWVCo_@iM&LM7*=;AJpA&do#
n=%PT?1&j#`y;&A`U>YSI<Qs;|tjRLcQj-m&#5Pw+Uu6OS9%VcR

diff --git a/tests/data/acpi/q35/DSDT.tis b/tests/data/acpi/q35/DSDT.tis
index 27ee927fc5ac05b89724c154197304c39e653269..e644f94958f61b174abf6bc05cafc4f4340f70eb 100644
GIT binary patch
delta 230
zcmccZu*{LmCD<ionF0d?W9~+-2@*{1)|2N*tY-2unCvO}m)%*|IoR2cWAb}RaRm-%
zKi6;}CdLAW!~({I0*2&8PCOGPCooP;1_~t<Bu}0%DLT1GN`y&#>g0t|!pzA_3qZ6>
z@&c8~G5Rd=!A|i3&YlJVL9W5ho_@iM1<4&C6B8LqLFO$qUto%%EXX&E(K*B|GI{cU
pDJi|u<N}7IMH*9+5<z0kVBJX!j0KEXOfOBEY#=4JSzlU*6#$QeMb7{L

delta 214
zcmZ4Hc-w)?CD<h-T7iLqv3Voc1PLY|v&nNLRx|l)Pxh4j%kIJF9PI4JF}YD%qMk8f
zkt6ry1jYh}gaU>beU|uOr}zM8PlJFU*I;L7UzY{$ER)fNJ^g|iokQFr(ZyXHLl_Gf
n(M5r(3m6j?db2F>z%)ua$Ttj^S(9a?r6wCliEXZu)?ozz!?Qdw

-- 
2.26.2

Re: [PATCH v2 0/7] x86: fix cpu hotplug with secure boot

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200818122208.1243901-1-imammedo@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  TEST    check-unit: tests/test-char
Unexpected error in object_property_try_add() at /tmp/qemu-test/src/qom/object.c:1181:
attempt to add duplicate property 'serial-id' to object (type 'container')
ERROR test-char - too few tests run (expected 38, got 9)
make: *** [check-unit] Error 1
make: *** Waiting for unfinished jobs....
  TEST    iotest-qcow2: 024
  TEST    iotest-qcow2: 025
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=f2b52e779ea044a6af8347ae49d4785e', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-e651x4a8/src/docker-src.2020-08-18-08.43.04.23321:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=f2b52e779ea044a6af8347ae49d4785e
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-e651x4a8/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    13m7.140s
user    0m9.328s


The full log is available at
http://patchew.org/logs/20200818122208.1243901-1-imammedo@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2 0/7] x86: fix cpu hotplug with secure boot

[Philippe Mathieu-Daudé]

On 8/18/20 2:56 PM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200818122208.1243901-1-imammedo@redhat.com/
> 
> 
> 
> Hi,
> 
> This series failed the docker-quick@centos7 build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> make docker-image-centos7 V=1 NETWORK=1
> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
> === TEST SCRIPT END ===
> 
>   TEST    check-unit: tests/test-char
> Unexpected error in object_property_try_add() at /tmp/qemu-test/src/qom/object.c:1181:
> attempt to add duplicate property 'serial-id' to object (type 'container')

Unrelated error.

> ERROR test-char - too few tests run (expected 38, got 9)
> make: *** [check-unit] Error 1
> make: *** Waiting for unfinished jobs....
>   TEST    iotest-qcow2: 024
>   TEST    iotest-qcow2: 025
> ---
>     raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=f2b52e779ea044a6af8347ae49d4785e', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-e651x4a8/src/docker-src.2020-08-18-08.43.04.23321:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
> filter=--filter=label=com.qemu.instance.uuid=f2b52e779ea044a6af8347ae49d4785e
> make[1]: *** [docker-run] Error 1
> make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-e651x4a8/src'
> make: *** [docker-run-test-quick@centos7] Error 2
> 
> real    13m7.140s
> user    0m9.328s
> 
> 
> The full log is available at
> http://patchew.org/logs/20200818122208.1243901-1-imammedo@redhat.com/testing.docker-quick@centos7/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com
> 

Re: [PATCH v2 4/7] acpi: add aml_land() and aml_break() primitives

[Philippe Mathieu-Daudé]

On 8/18/20 2:22 PM, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  include/hw/acpi/aml-build.h |  2 ++
>  hw/acpi/aml-build.c         | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index d27da03d64..e213fc554c 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -291,6 +291,7 @@ Aml *aml_store(Aml *val, Aml *target);
>  Aml *aml_and(Aml *arg1, Aml *arg2, Aml *dst);
>  Aml *aml_or(Aml *arg1, Aml *arg2, Aml *dst);
>  Aml *aml_lor(Aml *arg1, Aml *arg2);
> +Aml *aml_land(Aml *arg1, Aml *arg2);
>  Aml *aml_shiftleft(Aml *arg1, Aml *count);
>  Aml *aml_shiftright(Aml *arg1, Aml *count, Aml *dst);
>  Aml *aml_lless(Aml *arg1, Aml *arg2);
> @@ -300,6 +301,7 @@ Aml *aml_increment(Aml *arg);
>  Aml *aml_decrement(Aml *arg);
>  Aml *aml_index(Aml *arg1, Aml *idx);
>  Aml *aml_notify(Aml *arg1, Aml *arg2);
> +Aml *aml_break(void);
>  Aml *aml_call0(const char *method);
>  Aml *aml_call1(const char *method, Aml *arg1);
>  Aml *aml_call2(const char *method, Aml *arg1, Aml *arg2);
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index f6fbc9b95d..14b41b56f0 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -556,6 +556,15 @@ Aml *aml_or(Aml *arg1, Aml *arg2, Aml *dst)
>      return build_opcode_2arg_dst(0x7D /* OrOp */, arg1, arg2, dst);
>  }
>  
> +/* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefLAnd */
> +Aml *aml_land(Aml *arg1, Aml *arg2)
> +{
> +    Aml *var = aml_opcode(0x90 /* LandOp */);
> +    aml_append(var, arg1);
> +    aml_append(var, arg2);
> +    return var;
> +}
> +
>  /* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefLOr */
>  Aml *aml_lor(Aml *arg1, Aml *arg2)
>  {
> @@ -629,6 +638,13 @@ Aml *aml_notify(Aml *arg1, Aml *arg2)
>      return var;
>  }
>  
> +/* ACPI 1.0b: 16.2.5.3 Type 1 Opcodes Encoding: DefBreak */
> +Aml *aml_break(void)
> +{
> +    Aml *var = aml_opcode(0xa5 /* BreakOp */);
> +    return var;
> +}
> +
>  /* helper to call method without argument */
>  Aml *aml_call0(const char *method)
>  {
> 

Re: [PATCH v2 0/7] x86: fix cpu hotplug with secure boot

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200818122208.1243901-1-imammedo@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  TEST    check-unit: tests/test-char
Unexpected error in object_property_try_add() at /tmp/qemu-test/src/qom/object.c:1181:
attempt to add duplicate property 'serial-id' to object (type 'container')
ERROR test-char - too few tests run (expected 38, got 9)
make: *** [check-unit] Error 1
make: *** Waiting for unfinished jobs....
  TEST    check-qtest-x86_64: tests/qtest/hd-geo-test
qemu-system-aarch64: -accel kvm: invalid accelerator kvm
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=0c3122d22095423b9f3e2db213296b3e', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-4deda8a6/src/docker-src.2020-08-18-09.26.35.1749:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=0c3122d22095423b9f3e2db213296b3e
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-4deda8a6/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    13m18.695s
user    0m8.887s


The full log is available at
http://patchew.org/logs/20200818122208.1243901-1-imammedo@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2 0/7] x86: fix cpu hotplug with secure boot

[Philippe Mathieu-Daudé]

On 8/18/20 3:39 PM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200818122208.1243901-1-imammedo@redhat.com/
> 
> 
> 
> Hi,
> 
> This series failed the docker-quick@centos7 build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> make docker-image-centos7 V=1 NETWORK=1
> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
> === TEST SCRIPT END ===
> 
>   TEST    check-unit: tests/test-char
> Unexpected error in object_property_try_add() at /tmp/qemu-test/src/qom/object.c:1181:
> attempt to add duplicate property 'serial-id' to object (type 'container')

Unrelated issue.

> ERROR test-char - too few tests run (expected 38, got 9)
> make: *** [check-unit] Error 1
> make: *** Waiting for unfinished jobs....
>   TEST    check-qtest-x86_64: tests/qtest/hd-geo-test
> qemu-system-aarch64: -accel kvm: invalid accelerator kvm
> ---
>     raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=0c3122d22095423b9f3e2db213296b3e', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-4deda8a6/src/docker-src.2020-08-18-09.26.35.1749:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
> filter=--filter=label=com.qemu.instance.uuid=0c3122d22095423b9f3e2db213296b3e
> make[1]: *** [docker-run] Error 1
> make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-4deda8a6/src'
> make: *** [docker-run-test-quick@centos7] Error 2
> 
> real    13m18.695s
> user    0m8.887s
> 
> 
> The full log is available at
> http://patchew.org/logs/20200818122208.1243901-1-imammedo@redhat.com/testing.docker-quick@centos7/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com
> 

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

[Laszlo Ersek]

Hi Igor,

(CC'ing Daniel, Cornelia, David, Peter)

On 08/18/20 14:22, 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>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
> v2:
>   - rebase on top of 5.1 (move compat values to 5.1 machine)
>   - make "x-smi-cpu-hotunplug" false by default (Laszlo Ersek <lersek@redhat.com>)
> ---
>  include/hw/i386/ich9.h |  2 ++
>  include/hw/i386/pc.h   |  3 +++
>  hw/i386/pc.c           |  6 +++++-
>  hw/i386/pc_piix.c      |  1 +
>  hw/i386/pc_q35.c       |  1 +
>  hw/isa/lpc_ich9.c      | 13 +++++++++++++
>  6 files changed, 25 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/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 3d7ed3a55e..fe52e165b2 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -193,6 +193,9 @@ void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
>  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
>                         const CPUArchIdList *apic_ids, GArray *entry);
>
> +extern GlobalProperty pc_compat_5_1[];
> +extern const size_t pc_compat_5_1_len;
> +
>  extern GlobalProperty pc_compat_5_0[];
>  extern const size_t pc_compat_5_0_len;
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 47c5ca3e34..99c6bdbab4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -97,8 +97,12 @@
>  #include "fw_cfg.h"
>  #include "trace.h"
>
> -GlobalProperty pc_compat_5_0[] = {
> +GlobalProperty pc_compat_5_1[] = {
> +    { "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
>  };
> +const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
> +
> +GlobalProperty pc_compat_5_0[] = { };
>  const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
>
>  GlobalProperty pc_compat_4_2[] = {
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index b789e83f9a..d56f2e1b96 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -433,6 +433,7 @@ static void pc_i440fx_5_1_machine_options(MachineClass *m)
>      m->alias = "pc";
>      m->is_default = true;
>      pcmc->default_cpu_version = 1;
> +    compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
>  }
>
>  DEFINE_I440FX_MACHINE(v5_1, "pc-i440fx-5.1", NULL,
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index a3e607a544..0ca1146a59 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -359,6 +359,7 @@ static void pc_q35_5_1_machine_options(MachineClass *m)
>      pc_q35_machine_options(m);
>      m->alias = "q35";
>      pcmc->default_cpu_version = 1;
> +    compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
>  }
>
>  DEFINE_Q35_MACHINE(v5_1, "pc-q35-5.1", NULL,
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index cd6e169d47..19f32bed3e 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -373,6 +373,15 @@ 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 +756,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, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
>

this patch does the right thing for the 5.1 PC machine types, but it
does not introduce any 5.2 machine types, so I can't enable the hotplug
feature bit without messing with the x-smi-cpu-hotplug property
manually.

Now... looking at the mailing list, I can see the following patch
pending review (posted by Daniel ~5 days ago):

  [PATCH 01/10] hw: add compat machines for 5.2
  http://mid.mail-archive.com/20200814205424.543857-2-danielhb413@gmail.com

That patch introduces the 5.2 PC machine types.

However, I can't just apply your series on top of that one patch,
because they conflict at least on the "pc_compat_5_1" array.

Given that Daniel's patch was posted before yours, and also that
Daniel's patch introduces 5.2 machine types for arm, ppc and s390x too
(and the "hw_compat_5_1" array in addition to the "pc_compat_5_1"
array), I'd like to request:

- that we please review and/or merge Daniel's patch in isolation (just
  the first patch in the containing series, not the entire series!),

- and that you please rebase your series on top of Daniel's patch.

If Daniel's patch is approved as-is on the list, then I'm OK applying it
locally in isolation from the rest of the containing series, as a basis
for locally applying your v3.

(I could resolve the conflicts myself at once I guess, and proceed with
the review / testing -- but that's not really useful if I want to give a
formal Tested-by. I wouldn't be testing the patches as they were
posted.)

Thanks!
Laszlo

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

[Laszlo Ersek]

correcting myself a bit, with Drew and Thomas CC'd as well:

On 08/19/20 10:39, Laszlo Ersek wrote:
> Hi Igor,
> 
> (CC'ing Daniel, Cornelia, David, Peter)
> 
> On 08/18/20 14:22, 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>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>> v2:
>>   - rebase on top of 5.1 (move compat values to 5.1 machine)
>>   - make "x-smi-cpu-hotunplug" false by default (Laszlo Ersek <lersek@redhat.com>)
>> ---
>>  include/hw/i386/ich9.h |  2 ++
>>  include/hw/i386/pc.h   |  3 +++
>>  hw/i386/pc.c           |  6 +++++-
>>  hw/i386/pc_piix.c      |  1 +
>>  hw/i386/pc_q35.c       |  1 +
>>  hw/isa/lpc_ich9.c      | 13 +++++++++++++
>>  6 files changed, 25 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/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 3d7ed3a55e..fe52e165b2 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -193,6 +193,9 @@ void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
>>  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
>>                         const CPUArchIdList *apic_ids, GArray *entry);
>>
>> +extern GlobalProperty pc_compat_5_1[];
>> +extern const size_t pc_compat_5_1_len;
>> +
>>  extern GlobalProperty pc_compat_5_0[];
>>  extern const size_t pc_compat_5_0_len;
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 47c5ca3e34..99c6bdbab4 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -97,8 +97,12 @@
>>  #include "fw_cfg.h"
>>  #include "trace.h"
>>
>> -GlobalProperty pc_compat_5_0[] = {
>> +GlobalProperty pc_compat_5_1[] = {
>> +    { "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
>>  };
>> +const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
>> +
>> +GlobalProperty pc_compat_5_0[] = { };
>>  const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
>>
>>  GlobalProperty pc_compat_4_2[] = {
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index b789e83f9a..d56f2e1b96 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -433,6 +433,7 @@ static void pc_i440fx_5_1_machine_options(MachineClass *m)
>>      m->alias = "pc";
>>      m->is_default = true;
>>      pcmc->default_cpu_version = 1;
>> +    compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
>>  }
>>
>>  DEFINE_I440FX_MACHINE(v5_1, "pc-i440fx-5.1", NULL,
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index a3e607a544..0ca1146a59 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -359,6 +359,7 @@ static void pc_q35_5_1_machine_options(MachineClass *m)
>>      pc_q35_machine_options(m);
>>      m->alias = "q35";
>>      pcmc->default_cpu_version = 1;
>> +    compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
>>  }
>>
>>  DEFINE_Q35_MACHINE(v5_1, "pc-q35-5.1", NULL,
>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>> index cd6e169d47..19f32bed3e 100644
>> --- a/hw/isa/lpc_ich9.c
>> +++ b/hw/isa/lpc_ich9.c
>> @@ -373,6 +373,15 @@ 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 +756,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, false),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>
>>
> 
> this patch does the right thing for the 5.1 PC machine types, but it
> does not introduce any 5.2 machine types, so I can't enable the hotplug
> feature bit without messing with the x-smi-cpu-hotplug property
> manually.
> 
> Now... looking at the mailing list, I can see the following patch
> pending review (posted by Daniel ~5 days ago):
> 
>   [PATCH 01/10] hw: add compat machines for 5.2
>   http://mid.mail-archive.com/20200814205424.543857-2-danielhb413@gmail.com
> 
> That patch introduces the 5.2 PC machine types.
> 
> However, I can't just apply your series on top of that one patch,
> because they conflict at least on the "pc_compat_5_1" array.
> 
> Given that Daniel's patch was posted before yours, and also that
> Daniel's patch introduces 5.2 machine types for arm, ppc and s390x too
> (and the "hw_compat_5_1" array in addition to the "pc_compat_5_1"
> array), I'd like to request:
> 
> - that we please review and/or merge Daniel's patch in isolation (just
>   the first patch in the containing series, not the entire series!),
> 
> - and that you please rebase your series on top of Daniel's patch.
> 
> If Daniel's patch is approved as-is on the list, then I'm OK applying it
> locally in isolation from the rest of the containing series, as a basis
> for locally applying your v3.
> 
> (I could resolve the conflicts myself at once I guess, and proceed with
> the review / testing -- but that's not really useful if I want to give a
> formal Tested-by. I wouldn't be testing the patches as they were
> posted.)

Sorry, I've just realized: the patch in question was originally written
by Cornelia. And it's been on the list for a good time now -- it's been
multiply reviewed, and also picked up in other series several times:

Original posting, with reviews:

http://mid.mail-archive.com/20200728094645.272149-1-cohuck@redhat.com

Picked into another series (different from Daniel's):

http://mid.mail-archive.com/20200805091640.11134-2-drjones@redhat.com

Peter, would you please consider pushing that one patch directly? It
seems to be a choke point.

Thanks!
Laszlo

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

[Cornelia Huck]

On Wed, 19 Aug 2020 10:39:04 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> Hi Igor,
> 
> (CC'ing Daniel, Cornelia, David, Peter)
> 
> On 08/18/20 14:22, 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>
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > ---
> > v2:
> >   - rebase on top of 5.1 (move compat values to 5.1 machine)
> >   - make "x-smi-cpu-hotunplug" false by default (Laszlo Ersek <lersek@redhat.com>)
> > ---
> >  include/hw/i386/ich9.h |  2 ++
> >  include/hw/i386/pc.h   |  3 +++
> >  hw/i386/pc.c           |  6 +++++-
> >  hw/i386/pc_piix.c      |  1 +
> >  hw/i386/pc_q35.c       |  1 +
> >  hw/isa/lpc_ich9.c      | 13 +++++++++++++
> >  6 files changed, 25 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/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 3d7ed3a55e..fe52e165b2 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -193,6 +193,9 @@ void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
> >  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> >                         const CPUArchIdList *apic_ids, GArray *entry);
> >
> > +extern GlobalProperty pc_compat_5_1[];
> > +extern const size_t pc_compat_5_1_len;
> > +
> >  extern GlobalProperty pc_compat_5_0[];
> >  extern const size_t pc_compat_5_0_len;
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 47c5ca3e34..99c6bdbab4 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -97,8 +97,12 @@
> >  #include "fw_cfg.h"
> >  #include "trace.h"
> >
> > -GlobalProperty pc_compat_5_0[] = {
> > +GlobalProperty pc_compat_5_1[] = {
> > +    { "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
> >  };
> > +const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
> > +
> > +GlobalProperty pc_compat_5_0[] = { };
> >  const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
> >
> >  GlobalProperty pc_compat_4_2[] = {
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index b789e83f9a..d56f2e1b96 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -433,6 +433,7 @@ static void pc_i440fx_5_1_machine_options(MachineClass *m)
> >      m->alias = "pc";
> >      m->is_default = true;
> >      pcmc->default_cpu_version = 1;
> > +    compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
> >  }
> >
> >  DEFINE_I440FX_MACHINE(v5_1, "pc-i440fx-5.1", NULL,
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index a3e607a544..0ca1146a59 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -359,6 +359,7 @@ static void pc_q35_5_1_machine_options(MachineClass *m)
> >      pc_q35_machine_options(m);
> >      m->alias = "q35";
> >      pcmc->default_cpu_version = 1;
> > +    compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
> >  }
> >
> >  DEFINE_Q35_MACHINE(v5_1, "pc-q35-5.1", NULL,
> > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> > index cd6e169d47..19f32bed3e 100644
> > --- a/hw/isa/lpc_ich9.c
> > +++ b/hw/isa/lpc_ich9.c
> > @@ -373,6 +373,15 @@ 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 +756,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, false),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> >  
> 
> this patch does the right thing for the 5.1 PC machine types, but it
> does not introduce any 5.2 machine types, so I can't enable the hotplug
> feature bit without messing with the x-smi-cpu-hotplug property
> manually.
> 
> Now... looking at the mailing list, I can see the following patch
> pending review (posted by Daniel ~5 days ago):
> 
>   [PATCH 01/10] hw: add compat machines for 5.2
>   http://mid.mail-archive.com/20200814205424.543857-2-danielhb413@gmail.com
> 
> That patch introduces the 5.2 PC machine types.
> 
> However, I can't just apply your series on top of that one patch,
> because they conflict at least on the "pc_compat_5_1" array.

I consider any patch that adds compat options without adding all compat
machines first to be buggy. We had that in the past, and it had been
painful to sort it out again later. That's why I usually post a compat
machines patch during hardfreeze, to be picked up by anyone who needs
it.

(See
https://lore.kernel.org/qemu-devel/20200728094645.272149-1-cohuck@redhat.com/
-- this is the patch that Daniel re-posted.)

> 
> Given that Daniel's patch was posted before yours, and also that
> Daniel's patch introduces 5.2 machine types for arm, ppc and s390x too
> (and the "hw_compat_5_1" array in addition to the "pc_compat_5_1"
> array), I'd like to request:
> 
> - that we please review and/or merge Daniel's patch in isolation (just
>   the first patch in the containing series, not the entire series!),
> 
> - and that you please rebase your series on top of Daniel's patch.

Ack. Any series introducing compat options should have the machines
patch as the first patch; depending on merge order, it can then be
dropped again. (Usually, one of the architecture pull requests merges
it; we just don't have anything merged yet.)

> 
> If Daniel's patch is approved as-is on the list, then I'm OK applying it
> locally in isolation from the rest of the containing series, as a basis
> for locally applying your v3.

Just pick my original patch, it has a bunch of acks already.

> 
> (I could resolve the conflicts myself at once I guess, and proceed with
> the review / testing -- but that's not really useful if I want to give a
> formal Tested-by. I wouldn't be testing the patches as they were
> posted.)
> 
> Thanks!
> Laszlo

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

[Laszlo Ersek]

On 08/19/20 10:58, Cornelia Huck wrote:

> I consider any patch that adds compat options without adding all compat
> machines first to be buggy. We had that in the past, and it had been
> painful to sort it out again later. That's why I usually post a compat
> machines patch during hardfreeze, to be picked up by anyone who needs
> it.
> 
> (See
> https://lore.kernel.org/qemu-devel/20200728094645.272149-1-cohuck@redhat.com/
> -- this is the patch that Daniel re-posted.)

Indeed, thank you -- in the end, I did notice that Daniel's patch
started with "From: Cornelia" :) and then I checked my qemu-devel
folders for more emails with the same subject.

> Just pick my original patch, it has a bunch of acks already.

True, but Igor's series wouldn't apply without manual conflict
resolution, and I can't do that if I want to respond with a Tested-by.

(Of course, picking up your patch and including it in v3 is feasible for
Igor.)

Thanks!
Laszlo

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

[Igor Mammedov]

On Wed, 19 Aug 2020 11:14:45 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 08/19/20 10:58, Cornelia Huck wrote:
> 
> > I consider any patch that adds compat options without adding all compat
> > machines first to be buggy. We had that in the past, and it had been
> > painful to sort it out again later. That's why I usually post a compat
> > machines patch during hardfreeze, to be picked up by anyone who needs
> > it.
> > 
> > (See
> > https://lore.kernel.org/qemu-devel/20200728094645.272149-1-cohuck@redhat.com/
> > -- this is the patch that Daniel re-posted.)  
> 
> Indeed, thank you -- in the end, I did notice that Daniel's patch
> started with "From: Cornelia" :) and then I checked my qemu-devel
> folders for more emails with the same subject.
> 
> > Just pick my original patch, it has a bunch of acks already.  
> 
> True, but Igor's series wouldn't apply without manual conflict
> resolution, and I can't do that if I want to respond with a Tested-by.
> 
> (Of course, picking up your patch and including it in v3 is feasible for
> Igor.)

I'll rebase this path on top 5.2 machine patch and post it here.

> 
> Thanks!
> Laszlo

[PATCH v3 1/7] 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>
---
v3:
  - rebase on top of "[PATCH v2] hw: add compat machines for 5.2"
    so apply that before this patch
v2:
  - rebase on top of 5.1 (move compat values to 5.1 machine)
  - make "x-smi-cpu-hotunplug" false by default (Laszlo Ersek <lersek@redhat.com>)
---
 include/hw/i386/ich9.h |  2 ++
 hw/i386/pc.c           |  4 +++-
 hw/i386/pc_piix.c      |  1 +
 hw/i386/pc_q35.c       |  1 +
 hw/isa/lpc_ich9.c      | 13 +++++++++++++
 5 files changed, 20 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 9aa813949c..583db11d28 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -97,7 +97,9 @@
 #include "fw_cfg.h"
 #include "trace.h"
 
-GlobalProperty pc_compat_5_1[] = {};
+GlobalProperty pc_compat_5_1[] = {
+    { "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
+};
 const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
 
 GlobalProperty pc_compat_5_0[] = {
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index c5ba70ca17..68f8ba1bf9 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -433,6 +433,7 @@ static void pc_i440fx_5_2_machine_options(MachineClass *m)
     m->alias = "pc";
     m->is_default = true;
     pcmc->default_cpu_version = 1;
+    compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
 }
 
 DEFINE_I440FX_MACHINE(v5_2, "pc-i440fx-5.2", NULL,
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 0cb9c18cd4..b729cf9a58 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -359,6 +359,7 @@ static void pc_q35_5_2_machine_options(MachineClass *m)
     pc_q35_machine_options(m);
     m->alias = "q35";
     pcmc->default_cpu_version = 1;
+    compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
 }
 
 DEFINE_Q35_MACHINE(v5_2, "pc-q35-5.2", NULL,
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index cd6e169d47..19f32bed3e 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -373,6 +373,15 @@ 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 +756,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, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.26.2

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

[Laszlo Ersek]

Hi Igor,

On 08/20/20 16:56, 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>
> ---
> v3:
>   - rebase on top of "[PATCH v2] hw: add compat machines for 5.2"
>     so apply that before this patch
> v2:
>   - rebase on top of 5.1 (move compat values to 5.1 machine)
>   - make "x-smi-cpu-hotunplug" false by default (Laszlo Ersek <lersek@redhat.com>)
> ---
>  include/hw/i386/ich9.h |  2 ++
>  hw/i386/pc.c           |  4 +++-
>  hw/i386/pc_piix.c      |  1 +
>  hw/i386/pc_q35.c       |  1 +
>  hw/isa/lpc_ich9.c      | 13 +++++++++++++
>  5 files changed, 20 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 9aa813949c..583db11d28 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -97,7 +97,9 @@
>  #include "fw_cfg.h"
>  #include "trace.h"
>  
> -GlobalProperty pc_compat_5_1[] = {};
> +GlobalProperty pc_compat_5_1[] = {
> +    { "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
> +};
>  const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
>  
>  GlobalProperty pc_compat_5_0[] = {
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index c5ba70ca17..68f8ba1bf9 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -433,6 +433,7 @@ static void pc_i440fx_5_2_machine_options(MachineClass *m)
>      m->alias = "pc";
>      m->is_default = true;
>      pcmc->default_cpu_version = 1;
> +    compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
>  }
>  
>  DEFINE_I440FX_MACHINE(v5_2, "pc-i440fx-5.2", NULL,
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 0cb9c18cd4..b729cf9a58 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -359,6 +359,7 @@ static void pc_q35_5_2_machine_options(MachineClass *m)
>      pc_q35_machine_options(m);
>      m->alias = "q35";
>      pcmc->default_cpu_version = 1;
> +    compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
>  }
>  
>  DEFINE_Q35_MACHINE(v5_2, "pc-q35-5.2", NULL,

Sorry about the late response, I was away yesterday (public holiday in
my country).

The above two hunks should be dropped.

Cornelia's patch already handles both of:
- hw_compat_5_1,
- pc_compat_5_1.
in both of:
- pc_i440fx_5_1_machine_options(),
- pc_q35_5_1_machine_options().

So you don't need to modify those functions any longer.

Furthermore, the machine-options functions for 5.2 machine types should
not be modified *at all*. The effect of the above two hunks is that the
5.2 machine types will inherit the 5.1 compat props (such as

  ICH9-LPC.x-smi-cpu-hotplug=off

) and we do not want that.

So please drop the above two hunks.

If you could post a v4 of this patch today, that would be awesome; I
would like to play with this series extensively still today. "tonight"
is definitely an option, so if you can post the patch in the evening
(CEST), that's still great from my POV!

Thanks!
Laszlo

> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index cd6e169d47..19f32bed3e 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -373,6 +373,15 @@ 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 +756,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, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> 

[PATCH v4 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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
v4:
  - fix 5.2 machine types so they won't apply pc_compat_5_1
     (Laszlo Ersek <lersek@redhat.com>)
v3:
  - rebase on top of "[PATCH v2] hw: add compat machines for 5.2"
    so apply that before this patch
v2:
  - rebase on top of 5.1 (move compat values to 5.1 machine)
  - make "x-smi-cpu-hotunplug" false by default (Laszlo Ersek <lersek@redhat.com>)

fixup
---
 include/hw/i386/ich9.h |  2 ++
 hw/i386/pc.c           |  4 +++-
 hw/isa/lpc_ich9.c      | 13 +++++++++++++
 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 9aa813949c..583db11d28 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -97,7 +97,9 @@
 #include "fw_cfg.h"
 #include "trace.h"
 
-GlobalProperty pc_compat_5_1[] = {};
+GlobalProperty pc_compat_5_1[] = {
+    { "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
+};
 const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
 
 GlobalProperty pc_compat_5_0[] = {
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index cd6e169d47..19f32bed3e 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -373,6 +373,15 @@ 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 +756,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, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.26.2

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

[Laszlo Ersek]

On 08/24/20 13:00, 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>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
> v4:
>   - fix 5.2 machine types so they won't apply pc_compat_5_1
>      (Laszlo Ersek <lersek@redhat.com>)
> v3:
>   - rebase on top of "[PATCH v2] hw: add compat machines for 5.2"
>     so apply that before this patch
> v2:
>   - rebase on top of 5.1 (move compat values to 5.1 machine)
>   - make "x-smi-cpu-hotunplug" false by default (Laszlo Ersek <lersek@redhat.com>)
>
> fixup
> ---
>  include/hw/i386/ich9.h |  2 ++
>  hw/i386/pc.c           |  4 +++-
>  hw/isa/lpc_ich9.c      | 13 +++++++++++++
>  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 9aa813949c..583db11d28 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -97,7 +97,9 @@
>  #include "fw_cfg.h"
>  #include "trace.h"
>
> -GlobalProperty pc_compat_5_1[] = {};
> +GlobalProperty pc_compat_5_1[] = {
> +    { "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
> +};
>  const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
>
>  GlobalProperty pc_compat_5_0[] = {
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index cd6e169d47..19f32bed3e 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -373,6 +373,15 @@ 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 +756,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, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
>

This patch doesn't apply with git-am, as of commit 44423107e7b5 ("Merge
remote-tracking branch 'remotes/xtensa/tags/20200821-xtensa' into
staging", 2020-08-24).

The reason is that commit 2becc36a3e53 ("meson: infrastructure for
building emulators", 2020-08-21) introduced

#include CONFIG_DEVICES

to "hw/i386/pc.c", just above the (then) "pc_compat_5_0" array.

Then Cornelia's commit 3ff3c5d31740 ("hw: add compat machines for 5.2",
2020-08-19), which introduced "pc_compat_5_1" independently of the Meson
conversion, needed explicit resolution against CONFIG_DEVICES for
merging into master. That was covered in merge commit ca489cd037e4
("Merge remote-tracking branch
'remotes/ehabkost/tags/machine-next-pull-request' into staging",
2020-08-22).

So the patch applies on top of 3ff3c5d31740, but not on the merge
(ca489cd037e4) that brought 3ff3c5d31740 into master.

Not a problem though: I can apply the patch on 3ff3c5d31740, and then
cleanly (automatically) rebase to current HEAD (44423107e7b5) from
there. This is the range-diff across the rebase:

> 1:  9066fa4ccb49 ! 1:  05188fffe6aa x86: lpc9: let firmware negotiate 'CPU hotplug with SMI' features
>     @@ -31,8 +31,8 @@
>      --- a/hw/i386/pc.c
>      +++ b/hw/i386/pc.c
>      @@
>     - #include "fw_cfg.h"
>       #include "trace.h"
>     + #include CONFIG_DEVICES
>
>      -GlobalProperty pc_compat_5_1[] = {};
>      +GlobalProperty pc_compat_5_1[] = {


So it's indeed just a context update.

Having applied this series to QEMU, my test results are:

  OVMF has 5ba203b54e59  machine type  plug gate  unplug gate  gating result
  ---------------------  ------------  ---------  -----------  -------------
  no                     pc-q35-5.1    reject     reject       pass
  no                     pc-q35-5.2    reject     reject       pass
  yes                    pc-q35-5.1    reject     reject       pass
  yes                    pc-q35-5.2    accept     reject       pass

The results are the same after S3 suspend/resume. (This is relevant
because the features are re-negotiated during S3 resume.)

Thus, for this one patch, so far:

- (just to confirm:)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>

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

Thanks!
Laszlo

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

[Laszlo Ersek]

On 08/18/20 14:22, 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 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>
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
> v2:
>  - fix typo in commit message
>  - drop 5.1 version from hint message (Laszlo)
> ---
>  hw/acpi/ich9.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 0acc9a3107..95cb0f935b 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -460,6 +460,18 @@ 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 a version having "
> +                                    "x-smi-cpu-hotunplug=on and firmware that "
> +                                    "supports CPU hot-unplug with SMM");
> +            return;
> +        }
> +
>          acpi_cpu_unplug_request_cb(hotplug_dev, &lpc->pm.cpuhp_state,
>                                     dev, errp);
>      } else {
> 

A trivial comment:

Patch#2 says "x86: cphp: " in the subject line, but patch#3 says "x86:
cpuhp: " (note the extra "u").

I'm fine with either "cphp" or "cpuhp", but the subjects should be
consistent -- both patches should use the same word.

Preserve my T-b and R-b on both patches #2 and #3, after fixing up one
of the subjects. (Up to you which one.)

Thanks!
Laszlo

Re: [PATCH v2 4/7] acpi: add aml_land() and aml_break() primitives

[Laszlo Ersek]

On 08/18/20 14:22, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/hw/acpi/aml-build.h |  2 ++
>  hw/acpi/aml-build.c         | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index d27da03d64..e213fc554c 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -291,6 +291,7 @@ Aml *aml_store(Aml *val, Aml *target);
>  Aml *aml_and(Aml *arg1, Aml *arg2, Aml *dst);
>  Aml *aml_or(Aml *arg1, Aml *arg2, Aml *dst);
>  Aml *aml_lor(Aml *arg1, Aml *arg2);
> +Aml *aml_land(Aml *arg1, Aml *arg2);
>  Aml *aml_shiftleft(Aml *arg1, Aml *count);
>  Aml *aml_shiftright(Aml *arg1, Aml *count, Aml *dst);
>  Aml *aml_lless(Aml *arg1, Aml *arg2);
> @@ -300,6 +301,7 @@ Aml *aml_increment(Aml *arg);
>  Aml *aml_decrement(Aml *arg);
>  Aml *aml_index(Aml *arg1, Aml *idx);
>  Aml *aml_notify(Aml *arg1, Aml *arg2);
> +Aml *aml_break(void);
>  Aml *aml_call0(const char *method);
>  Aml *aml_call1(const char *method, Aml *arg1);
>  Aml *aml_call2(const char *method, Aml *arg1, Aml *arg2);
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index f6fbc9b95d..14b41b56f0 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -556,6 +556,15 @@ Aml *aml_or(Aml *arg1, Aml *arg2, Aml *dst)
>      return build_opcode_2arg_dst(0x7D /* OrOp */, arg1, arg2, dst);
>  }
>  
> +/* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefLAnd */
> +Aml *aml_land(Aml *arg1, Aml *arg2)
> +{
> +    Aml *var = aml_opcode(0x90 /* LandOp */);

(1) Small typo in the comment: in the spec, it is spelled "LAndOp", not
"LandOp" (consistently with "LOrOp" just below -- although "LOrOp" is
not visible in the context, in this email).

> +    aml_append(var, arg1);
> +    aml_append(var, arg2);
> +    return var;
> +}
> +
>  /* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefLOr */
>  Aml *aml_lor(Aml *arg1, Aml *arg2)
>  {

(2) In the header file, the declaration order is aml_lor(), then
aml_land(). But in the C file, the definitions are in the opposite
order: aml_land(), aml_lor().

Not sure which one is right, but we should be consistent, if we can.

> @@ -629,6 +638,13 @@ Aml *aml_notify(Aml *arg1, Aml *arg2)
>      return var;
>  }
>  
> +/* ACPI 1.0b: 16.2.5.3 Type 1 Opcodes Encoding: DefBreak */
> +Aml *aml_break(void)
> +{
> +    Aml *var = aml_opcode(0xa5 /* BreakOp */);
> +    return var;
> +}
> +
>  /* helper to call method without argument */
>  Aml *aml_call0(const char *method)
>  {
> 

With my trivial comments (1) and (2) fixed:

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

Thanks,
Laszlo

Re: [PATCH v2 6/7] x68: acpi: trigger SMI before sending hotplug Notify event to OSPM

[Laszlo Ersek]

On 08/18/20 14:22, 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 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 processed
> by the next CPU_SCAN_METHOD, when pending SCI is handled.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>  - clear insert event after firmware has returned
>    control from SMI. (Laszlo Ersek <lersek@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          | 80 +++++++++++++++++++++++++++++++++++++++---
>  hw/i386/acpi-build.c   | 35 +++++++++++++++++-
>  hw/isa/lpc_ich9.c      |  3 ++
>  5 files changed, 115 insertions(+), 6 deletions(-)

For easing review, this patch could be split in several patches, I
think. I'm not saying that's a "requirement", just that it would be
possible, and could help review.


* Patch "a":

[subject:]

  x86: ich9: expose "smi_negotiated_features" as a QOM property

[commit message:]

  Expose the "smi_negotiated_features" field of ICH9LPCState as a QOM
  property.

[code extracted from current patch follows]

> 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/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 19f32bed3e..8124d20338 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -647,6 +647,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);
>  }
>

For sub-patch "a":
Reviewed-by: Laszlo Ersek <lersek@redhat.com>


* Patch "b":

[subject:]

  x86: acpi: introduce "AcpiPmInfo.smi_on_cpuhp"

[commit message:]

  Translate the "CPU hotplug with SMI" feature bit, from the property
  added in the last patch, to a dedicated boolean in AcpiPmInfo.

[code extracted from current patch follows]

> 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

For sub-patch "b":
Reviewed-by: Laszlo Ersek <lersek@redhat.com>


* Patch "c":

[subject:]

  x86: acpi: introduce the "PCI0.SMI0" ACPI device

[commit message:]

  When CPU hotplug with SMI has been negotiated, describe the SMI
  register block in the DSDT. Pass the ACPI name of the SMI control
  register to build_cpus_aml(), so that CPU_SCAN_METHOD can access the
  register in the next patch.

[code extracted from current patch follows]

> 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/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
> @@ -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");

For sub-patch "c":
Reviewed-by: Laszlo Ersek <lersek@redhat.com>


* Patch "d":

[subject: copy from current (large) patch]

[commit message: copy from current (large) patch]

[code extracted from current patch follows, with my comments]

> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index 3d6a500fb7..4864c3b396 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,21 +474,61 @@ 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 *uid = aml_local(3);
> +            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)));
> +            }

(1) Both package sizes are off-by-one. We only need room for the CPUs
that have events pending, and that excludes CPU#0.

In particular, in the compatibility case, when "arch_ids->len" is
exactly 256, then we pass 256 to aml_package(). However, aml_package()
-- correctly! -- takes:

  uint8_t num_elements

So with 256 VCPUs, we'd pass a zero "num_elements".

The comparison (<= 256) is correct, we just need to pass

  arch_ids->len - 1

to both aml_package() and aml_varpackage().


> +
> +            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));
> +            /* start scan from the 1st CPU */
> +            aml_append(method, aml_store(zero, uid));
> +            while_ctx = aml_while(aml_land(aml_equal(has_event, one),
> +                                  aml_lless(uid, aml_int(arch_ids->len))));
>              {
>                   /* clear loop exit condition, ins_evt/rm_evt checks
>                    * will set it to 1 while next_cpu_cmd returns a CPU
>                    * with events */
>                   aml_append(while_ctx, aml_store(zero, has_event));
> +
> +                 aml_append(while_ctx, aml_store(uid, cpu_selector));
>                   aml_append(while_ctx, aml_store(next_cpu_cmd, cpu_cmd));
> +
> +                 /*
> +                  * wrap around case, scan is complete, exit loop.
> +                  * It happens since events are not cleared in scan loop,
> +                  * so next_cpu_cmd continues to find already processed CPUs.
> +                  */
> +                 ifctx = aml_if(aml_lless(cpu_data, uid));
> +                 {
> +                     aml_append(ifctx, aml_break());
> +                 }
> +                 aml_append(while_ctx, ifctx);
> +
> +                 aml_append(while_ctx, aml_store(cpu_data, uid));
>                   ifctx = aml_if(aml_equal(ins_evt, one));
>                   {
> -                     aml_append(ifctx,
> -                         aml_call2(CPU_NOTIFY_METHOD, cpu_data, dev_chk));
> -                     aml_append(ifctx, aml_store(one, ins_evt));
> +                     /* cache added CPUs to Notify/Wakeup later */
> +                     aml_append(ifctx, aml_store(uid,
> +                         aml_index(new_cpus, num_added_cpus)));
> +                     aml_append(ifctx, aml_increment(num_added_cpus));
>                       aml_append(ifctx, aml_store(one, has_event));
>                   }
>                   aml_append(while_ctx, ifctx);
> @@ -493,14 +536,41 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>                   ifctx = aml_if(aml_equal(rm_evt, one));
>                   {
>                       aml_append(ifctx,
> -                         aml_call2(CPU_NOTIFY_METHOD, cpu_data, eject_req));
> +                         aml_call2(CPU_NOTIFY_METHOD, uid, eject_req));
>                       aml_append(ifctx, aml_store(one, rm_evt));
>                       aml_append(ifctx, aml_store(one, has_event));
>                   }
>                   aml_append(else_ctx, ifctx);
>                   aml_append(while_ctx, else_ctx);
> +                 aml_append(while_ctx, aml_increment(uid));
> +            }
> +            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)));

(2) The composed "aml_lnot(aml_equal())" is not wrong, but I think
aml_lgreater() would be simpler (for the QEMU source code anyway).

Feel free to ignore.

> +                {
> +                    aml_append(ifctx, aml_store(aml_int(OVMF_CPUHP_SMI_CMD),
> +                        aml_name("%s", opts.smi_path)));
> +                }
> +                aml_append(method, ifctx);
> +            }
> +
> +            /* Notify OSPM about new CPUs and clear insert events */
> +            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_store(one, ins_evt));

(3) Before writing 1 to CINS, you need to set CSEL as well.

This bug is masked if the loop finds *at most* one added CPU. To see
why, here's the decompiled method, with a possible CPUs count of 4, and
the local variable names substituted:

   1  Method (CSCN, 0, Serialized)
   2  {
   3      Acquire (\_SB.PCI0.PRES.CPLK, 0xFFFF)
   4      Name (CNEW, Package (0x04){})
   5
   6      Local_NumAddedCpus = Zero
   7      Local_HasEvent = One
   8      Local_Uid = Zero
   9
  10      While (((Local_HasEvent == One) && (Local_Uid < 0x04)))
  11      {
  12          Local_HasEvent = Zero
  13          \_SB.PCI0.PRES.CSEL = Local_Uid
  14          \_SB.PCI0.PRES.CCMD = Zero
  15          If ((\_SB.PCI0.PRES.CDAT < Local_Uid))
  16          {
  17              Break
  18          }
  19
  20          Local_Uid = \_SB.PCI0.PRES.CDAT
  21          If ((\_SB.PCI0.PRES.CINS == One))
  22          {
  23              CNEW [Local_NumAddedCpus] = Local_Uid
  24              Local_NumAddedCpus++
  25              Local_HasEvent = One
  26          }
  27          ElseIf ((\_SB.PCI0.PRES.CRMV == One))
  28          {
  29              CTFY (Local_Uid, 0x03)
  30              \_SB.PCI0.PRES.CRMV = One
  31              Local_HasEvent = One
  32          }
  33
  34          Local_Uid++
  35      }
  36
  37      If ((Local_NumAddedCpus != Zero))
  38      {
  39          \_SB.PCI0.SMI0.SMIC = 0x04
  40      }
  41
  42      Local_CpuIdx = Zero
  43      While ((Local_CpuIdx < Local_NumAddedCpus))
  44      {
  45          CTFY (DerefOf (CNEW [Local_CpuIdx]), One)
  46          \_SB.PCI0.PRES.CINS = One
  47          Local_CpuIdx++
  48      }
  49
  50      Release (\_SB.PCI0.PRES.CPLK)
  51  }

If the loop has found exactly one VCPU with a pending event, then we
must have exited the loop in one of two ways:

- The UID of the VCPU with the pending event was maximal, and so the
  Local_Uid limit in the While fired on the next iteration (line 10).

- Otherwise, the search wrapped on the next iteration (line 17).

In either case, the loop leaves the "right" CPU selector in place:

- In the first case, when we exit on line 10, we never write to CSEL
  again (line 13).

- In the second case, we wrap back to the *one and only* VCPU with
  pending events, on line 14, before we exit on line 17.

And so when we reach the *one and only* write to CINS on line 46, the
CPU selector happens to be correct.

Because of this, I happened to test this patch successfully with e.g.

  virsh setvcpu DOMAIN 3 --enable --live

(note: "setvcpu", singular).

However, the bug can be triggered when plugging multiple VCPUs in quick
succession, with e.g.

  virsh setvcpus DOMAIN 8 --live

(note "setvcpus", plural).

So I would suggest fetching the CNEW array element back into "uid"
first, then using "uid" for both the NOTIFY call, and the (currently
missing) restoration of CSEL. Then we can write 1 to CINS.

Expressed as a patch on top of yours:

> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index 4864c3b39694..2bea6144fd5e 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -564,8 +564,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>              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_store(aml_derefof(aml_index(new_cpus, cpu_idx)), uid));
> +                aml_append(while_ctx,
> +                    aml_call2(CPU_NOTIFY_METHOD, uid, dev_chk));
> +                aml_append(while_ctx, aml_store(uid, cpu_selector));
>                  aml_append(while_ctx, aml_store(one, ins_evt));
>                  aml_append(while_ctx, aml_increment(cpu_idx));
>              }

This effects the following change, in the decompiled method:

> @@ -37,15 +37,17 @@
>      If ((Local_NumAddedCpus != Zero))
>      {
>          \_SB.PCI0.SMI0.SMIC = 0x04
>      }
>
>      Local_CpuIdx = Zero
>      While ((Local_CpuIdx < Local_NumAddedCpus))
>      {
> -        CTFY (DerefOf (CNEW [Local_CpuIdx]), One)
> +        Local_Uid = DerefOf (CNEW [Local_CpuIdx])
> +        CTFY (Local_Uid, One)
> +        \_SB.PCI0.PRES.CSEL = Local_Uid
>          \_SB.PCI0.PRES.CINS = One
>          Local_CpuIdx++
>      }
>
>      Release (\_SB.PCI0.PRES.CPLK)
>  }

With this change, the

  virsh setvcpus DOMAIN 8 --live

command works for me. The topology in my test domain has CPU#0 and CPU#2
cold-plugged, so the command adds 6 VCPUs. Viewed from the firmware
side, the 6 "device_add" commands, issued in close succession by
libvirtd, coalesce into 4 "batches". (And of course the firmware sees
the 4 batches back-to-back.)

Note the "PluggedCount" message, which reports the size of each batch:

- Batch#1 adds CPU#1:

> QemuCpuhpCollectApicIds: CurrentSelector=1: insert
> QemuCpuhpCollectApicIds: ApicId=0x00000001
> QemuCpuhpCollectApicIds: CurrentSelector=2 PendingSelector=1: wrap-around
> QemuCpuhpCollectApicIds: PluggedCount=1 ToUnplugCount=0
> CpuHotplugMmi: hot-added APIC ID 0x00000001, SMBASE 0x7FF7A000, EFI_SMM_CPU_SERVICE_PROTOCOL assigned number 2

- Batch#2 adds CPU#3:

> QemuCpuhpCollectApicIds: CurrentSelector=3: insert
> QemuCpuhpCollectApicIds: ApicId=0x00000003
> QemuCpuhpCollectApicIds: CurrentSelector=4 PendingSelector=3: wrap-around
> QemuCpuhpCollectApicIds: PluggedCount=1 ToUnplugCount=0
> CpuHotplugMmi: hot-added APIC ID 0x00000003, SMBASE 0x7FF7C000, EFI_SMM_CPU_SERVICE_PROTOCOL assigned number 3

- Batch#3 adds CPU#4, CPU#5, CPU#6:

> QemuCpuhpCollectApicIds: CurrentSelector=4: insert
> QemuCpuhpCollectApicIds: ApicId=0x00000004
> QemuCpuhpCollectApicIds: CurrentSelector=5: insert
> QemuCpuhpCollectApicIds: ApicId=0x00000005
> QemuCpuhpCollectApicIds: CurrentSelector=6: insert
> QemuCpuhpCollectApicIds: ApicId=0x00000006
> QemuCpuhpCollectApicIds: CurrentSelector=7 PendingSelector=4: wrap-around
> QemuCpuhpCollectApicIds: PluggedCount=3 ToUnplugCount=0
> CpuHotplugMmi: hot-added APIC ID 0x00000004, SMBASE 0x7FF7E000, EFI_SMM_CPU_SERVICE_PROTOCOL assigned number 4
> CpuHotplugMmi: hot-added APIC ID 0x00000005, SMBASE 0x7FF80000, EFI_SMM_CPU_SERVICE_PROTOCOL assigned number 5
> CpuHotplugMmi: hot-added APIC ID 0x00000006, SMBASE 0x7FF82000, EFI_SMM_CPU_SERVICE_PROTOCOL assigned number 6

- Batch#4 adds CPU#7:

> QemuCpuhpCollectApicIds: CurrentSelector=7: insert
> QemuCpuhpCollectApicIds: ApicId=0x00000007
> QemuCpuhpCollectApicIds: PluggedCount=1 ToUnplugCount=0
> CpuHotplugMmi: hot-added APIC ID 0x00000007, SMBASE 0x7FF84000, EFI_SMM_CPU_SERVICE_PROTOCOL assigned number 7

Thanks,
Laszlo

On 08/18/20 14:22, Igor Mammedov wrote:
> +                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);

Re: [PATCH v2 6/7] x68: acpi: trigger SMI before sending hotplug Notify event to OSPM

[Igor Mammedov]

On Tue, 25 Aug 2020 19:25:48 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 08/18/20 14:22, 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 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 processed
> > by the next CPU_SCAN_METHOD, when pending SCI is handled.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v2:
> >  - clear insert event after firmware has returned
> >    control from SMI. (Laszlo Ersek <lersek@redhat.com>)

Laszlo,
Thanks a lot for such detailed review
I'll respin series with your feed back incorporated.

[...]

Re: [PATCH v2 6/7] x68: acpi: trigger SMI before sending hotplug Notify event to OSPM

[Laszlo Ersek]

Hi Igor,

On 08/25/20 19:25, Laszlo Ersek wrote:

> So I would suggest fetching the CNEW array element back into "uid"
> first, then using "uid" for both the NOTIFY call, and the (currently
> missing) restoration of CSEL. Then we can write 1 to CINS.
>
> Expressed as a patch on top of yours:
>
>> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
>> index 4864c3b39694..2bea6144fd5e 100644
>> --- a/hw/acpi/cpu.c
>> +++ b/hw/acpi/cpu.c
>> @@ -564,8 +564,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>>              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_store(aml_derefof(aml_index(new_cpus, cpu_idx)), uid));
>> +                aml_append(while_ctx,
>> +                    aml_call2(CPU_NOTIFY_METHOD, uid, dev_chk));
>> +                aml_append(while_ctx, aml_store(uid, cpu_selector));
>>                  aml_append(while_ctx, aml_store(one, ins_evt));
>>                  aml_append(while_ctx, aml_increment(cpu_idx));
>>              }
>
> This effects the following change, in the decompiled method:
>
>> @@ -37,15 +37,17 @@
>>      If ((Local_NumAddedCpus != Zero))
>>      {
>>          \_SB.PCI0.SMI0.SMIC = 0x04
>>      }
>>
>>      Local_CpuIdx = Zero
>>      While ((Local_CpuIdx < Local_NumAddedCpus))
>>      {
>> -        CTFY (DerefOf (CNEW [Local_CpuIdx]), One)
>> +        Local_Uid = DerefOf (CNEW [Local_CpuIdx])
>> +        CTFY (Local_Uid, One)
>> +        \_SB.PCI0.PRES.CSEL = Local_Uid
>>          \_SB.PCI0.PRES.CINS = One
>>          Local_CpuIdx++
>>      }
>>
>>      Release (\_SB.PCI0.PRES.CPLK)
>>  }
>
> With this change, the
>
>   virsh setvcpus DOMAIN 8 --live
>
> command works for me. The topology in my test domain has CPU#0 and
> CPU#2 cold-plugged, so the command adds 6 VCPUs. Viewed from the
> firmware side, the 6 "device_add" commands, issued in close succession
> by libvirtd, coalesce into 4 "batches". (And of course the firmware
> sees the 4 batches back-to-back.)

unfortunately, with more testing, I have run into two more races:

(1) When a "device_add" occurs after the ACPI loop collects the CPUS
    from the register block, but before the SMI.

    Here, the "stray CPU" is processed fine by the firmware. However,
    the CTFY loop in ACPI does not know about the CPU, so it doesn't
    clear the pending insert event for it. And when the firmware is
    entered with an SMI for the *next* time, the firmware sees the same
    CPU *again* as pending, and tries to relocate it again. Bad things
    happen.

(2) When a "device_add" occurs after the SMI, but before the firmware
    collects the pending CPUs from the register block.

    Here, the firmware collects the "stray CPU". However, the "broadcast
    SMI", with which we entered the firmware, did *not* cover the stray
    CPU -- the CPU_FOREACH() loop in ich9_apm_ctrl_changed() could not
    make the SMI pending for the new CPU, because at that time, the CPU
    had not been added yet. As a result, when the firmware sends an
    INIT-SIPI-SIPI to the new CPU, expecting it to boot right into SMM,
    the new CPU instead boots straight into the post-RSM (normal mode)
    "pen", skipping its initial SMI handler. Meaning that the CPU halts
    nicely, but its SMBASE is never relocated, and the SMRAM message
    exchange with the BSP falls apart.

Possible mitigations I can think of:

For problem (1):

  (1a) Change the firmware so it notices that it has relocated the
       "stray" CPU before -- such CPUs should be simply skipped in the
       firmware. The next time the CTFY loop runs in ACPI, it will clear
       the pending event.

  (1b) Alternatively, stop consuming the hotplug register block in the
       firmware altogether, and work out general message passing, from
       ACPI to firmware. See the outline here:

         http://mid.mail-archive.com/cf887d74-f65d-602a-9629-3d25cef93a69@redhat.com

For problem (2):

  (2a) Change the firmware so that it sends a directed SMI as well to
       each CPU, just before sending an INIT-SIPI-SIPI. This should be
       idempotent -- if the broadcast SMI *has* covered the the CPU,
       then sending a directed SMI should make no difference.

  (2b) Alternatively, change the "device_add" command in QEMU so that,
       if "CPU hotplug with SMI" has been negotiated, the new CPU is
       added with the SMI made pending for it at once. (That is, no
       hot-plugged CPU would exist with the directed SMI *not* pending
       for it.)

  (2c) Alternatively, approach (1b) would fix problem (2) as well -- the
       firmware would only relocate such CPUs that ACPI collected before
       injecting the SMI. So all those CPUs would have the SMI pending.


I can experiment with (1a) and (2a), but note that for unplug, we're
likely going to need the array passing method (1b) anyway. I'm concerned
that (1a) and (2a) together may not fix all possible races (i.e. we
might see more kinds).

What's important is that, *IF* we need to change anything in the
firmware for fixing these races, then we either need to do so urgently,
*or* I have to revert OVMF commit 5ba203b54e59 ("OvmfPkg/SmmControl2Dxe:
negotiate ICH9_LPC_SMI_F_CPU_HOTPLUG", 2020-08-24).

That's because we're in feature freeze for edk2-stable202008

  https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning#edk2-stable202008-tag-planning

and we shouldn't make a release with ICH9_LPC_SMI_F_CPU_HOTPLUG
negotiated, knowing that there are races.


Please advise!

Thanks,
Laszlo

Re: [PATCH v2 6/7] x68: acpi: trigger SMI before sending hotplug Notify event to OSPM

[Igor Mammedov]

On Wed, 26 Aug 2020 11:24:14 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> Hi Igor,
> 
> On 08/25/20 19:25, Laszlo Ersek wrote:
> 
> > So I would suggest fetching the CNEW array element back into "uid"
> > first, then using "uid" for both the NOTIFY call, and the (currently
> > missing) restoration of CSEL. Then we can write 1 to CINS.
> >
> > Expressed as a patch on top of yours:
> >  
> >> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> >> index 4864c3b39694..2bea6144fd5e 100644
> >> --- a/hw/acpi/cpu.c
> >> +++ b/hw/acpi/cpu.c
> >> @@ -564,8 +564,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
> >>              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_store(aml_derefof(aml_index(new_cpus, cpu_idx)), uid));
> >> +                aml_append(while_ctx,
> >> +                    aml_call2(CPU_NOTIFY_METHOD, uid, dev_chk));
> >> +                aml_append(while_ctx, aml_store(uid, cpu_selector));
> >>                  aml_append(while_ctx, aml_store(one, ins_evt));
> >>                  aml_append(while_ctx, aml_increment(cpu_idx));
> >>              }  
> >
> > This effects the following change, in the decompiled method:
> >  
> >> @@ -37,15 +37,17 @@
> >>      If ((Local_NumAddedCpus != Zero))
> >>      {
> >>          \_SB.PCI0.SMI0.SMIC = 0x04
> >>      }
> >>
> >>      Local_CpuIdx = Zero
> >>      While ((Local_CpuIdx < Local_NumAddedCpus))
> >>      {
> >> -        CTFY (DerefOf (CNEW [Local_CpuIdx]), One)
> >> +        Local_Uid = DerefOf (CNEW [Local_CpuIdx])
> >> +        CTFY (Local_Uid, One)
> >> +        \_SB.PCI0.PRES.CSEL = Local_Uid
> >>          \_SB.PCI0.PRES.CINS = One
> >>          Local_CpuIdx++
> >>      }
> >>
> >>      Release (\_SB.PCI0.PRES.CPLK)
> >>  }  
> >
> > With this change, the
> >
> >   virsh setvcpus DOMAIN 8 --live
> >
> > command works for me. The topology in my test domain has CPU#0 and
> > CPU#2 cold-plugged, so the command adds 6 VCPUs. Viewed from the
> > firmware side, the 6 "device_add" commands, issued in close succession
> > by libvirtd, coalesce into 4 "batches". (And of course the firmware
> > sees the 4 batches back-to-back.)  
> 
> unfortunately, with more testing, I have run into two more races:
> 
> (1) When a "device_add" occurs after the ACPI loop collects the CPUS
>     from the register block, but before the SMI.
> 
>     Here, the "stray CPU" is processed fine by the firmware. However,
>     the CTFY loop in ACPI does not know about the CPU, so it doesn't
>     clear the pending insert event for it. And when the firmware is
>     entered with an SMI for the *next* time, the firmware sees the same
>     CPU *again* as pending, and tries to relocate it again. Bad things
>     happen.
>
> (2) When a "device_add" occurs after the SMI, but before the firmware
>     collects the pending CPUs from the register block.
> 
>     Here, the firmware collects the "stray CPU". However, the "broadcast
>     SMI", with which we entered the firmware, did *not* cover the stray
>     CPU -- the CPU_FOREACH() loop in ich9_apm_ctrl_changed() could not
>     make the SMI pending for the new CPU, because at that time, the CPU
>     had not been added yet. As a result, when the firmware sends an
>     INIT-SIPI-SIPI to the new CPU, expecting it to boot right into SMM,
>     the new CPU instead boots straight into the post-RSM (normal mode)
>     "pen", skipping its initial SMI handler. Meaning that the CPU halts
>     nicely, but its SMBASE is never relocated, and the SMRAM message
>     exchange with the BSP falls apart.
> 
> Possible mitigations I can think of:
> 
> For problem (1):
> 
>   (1a) Change the firmware so it notices that it has relocated the
>        "stray" CPU before -- such CPUs should be simply skipped in the
>        firmware. The next time the CTFY loop runs in ACPI, it will clear
>        the pending event.
> 
>   (1b) Alternatively, stop consuming the hotplug register block in the
>        firmware altogether, and work out general message passing, from
>        ACPI to firmware. See the outline here:
> 
>          http://mid.mail-archive.com/cf887d74-f65d-602a-9629-3d25cef93a69@redhat.com
> 
> For problem (2):
> 
>   (2a) Change the firmware so that it sends a directed SMI as well to
>        each CPU, just before sending an INIT-SIPI-SIPI. This should be
>        idempotent -- if the broadcast SMI *has* covered the the CPU,
>        then sending a directed SMI should make no difference.
may be still racy, as new cpus can arrive diring/after direct broadcast.
better would be ignore stray CPUs that didn't pass through SMM
relocation.

> 
>   (2b) Alternatively, change the "device_add" command in QEMU so that,
>        if "CPU hotplug with SMI" has been negotiated, the new CPU is
>        added with the SMI made pending for it at once. (That is, no
>        hot-plugged CPU would exist with the directed SMI *not* pending
>        for it.)
> 
>   (2c) Alternatively, approach (1b) would fix problem (2) as well -- the
>        firmware would only relocate such CPUs that ACPI collected before
>        injecting the SMI. So all those CPUs would have the SMI pending.
> 
> 
> I can experiment with (1a) and (2a), but note that for unplug, we're
> likely going to need the array passing method (1b) anyway. I'm concerned
> that (1a) and (2a) together may not fix all possible races (i.e. we
> might see more kinds).

Given async nature of device_add, (1a) and (2a) are expected
(I've thought it were already handled by fw),
so I'd prefer to continue with current approach if possible.
(if fw is not able to deal with 2a without QEMU help,
maybe we could add an extra flag in register block to help,
though my preference is to avoid it if it's possible)

(2b) if twisting CPU state in QEMU were acceptable we would have
had a lot of helping quirks that would put CPU in some desired
for a a particular guest code state, and why stop there just
discard firmware and configure all devices right from QEMU
to and start executing OS code strait away. (it would be
faster than going through what real hw does) (I'm afraid it's
not going to fly).

(1c) it's still very unclear what direction to take there,
 * one as you mentioned, would be introducing new protocol for
   message passing between acpi and fw.
   (so far I've was thinking that payload should be UID of CPU)
 * another approach, I was considering was adding an additional
   flag to register block. So CPU._EJ0 would mark CPU as pending
   for removal and do SMI upcall to fw to act on it.
 
> What's important is that, *IF* we need to change anything in the
> firmware for fixing these races, then we either need to do so urgently,
> *or* I have to revert OVMF commit 5ba203b54e59 ("OvmfPkg/SmmControl2Dxe:
> negotiate ICH9_LPC_SMI_F_CPU_HOTPLUG", 2020-08-24).
> 
> That's because we're in feature freeze for edk2-stable202008
> 
>   https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning#edk2-stable202008-tag-planning
> 
> and we shouldn't make a release with ICH9_LPC_SMI_F_CPU_HOTPLUG
> negotiated, knowing that there are races.
> 
> 
> Please advise!
> 
> Thanks,
> Laszlo

Re: [PATCH v2 6/7] x68: acpi: trigger SMI before sending hotplug Notify event to OSPM

[Laszlo Ersek]

On 08/26/20 11:24, Laszlo Ersek wrote:
> Hi Igor,
> 
> On 08/25/20 19:25, Laszlo Ersek wrote:
> 
>> So I would suggest fetching the CNEW array element back into "uid"
>> first, then using "uid" for both the NOTIFY call, and the (currently
>> missing) restoration of CSEL. Then we can write 1 to CINS.
>>
>> Expressed as a patch on top of yours:
>>
>>> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
>>> index 4864c3b39694..2bea6144fd5e 100644
>>> --- a/hw/acpi/cpu.c
>>> +++ b/hw/acpi/cpu.c
>>> @@ -564,8 +564,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>>>              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_store(aml_derefof(aml_index(new_cpus, cpu_idx)), uid));
>>> +                aml_append(while_ctx,
>>> +                    aml_call2(CPU_NOTIFY_METHOD, uid, dev_chk));
>>> +                aml_append(while_ctx, aml_store(uid, cpu_selector));
>>>                  aml_append(while_ctx, aml_store(one, ins_evt));
>>>                  aml_append(while_ctx, aml_increment(cpu_idx));
>>>              }
>>
>> This effects the following change, in the decompiled method:
>>
>>> @@ -37,15 +37,17 @@
>>>      If ((Local_NumAddedCpus != Zero))
>>>      {
>>>          \_SB.PCI0.SMI0.SMIC = 0x04
>>>      }
>>>
>>>      Local_CpuIdx = Zero
>>>      While ((Local_CpuIdx < Local_NumAddedCpus))
>>>      {
>>> -        CTFY (DerefOf (CNEW [Local_CpuIdx]), One)
>>> +        Local_Uid = DerefOf (CNEW [Local_CpuIdx])
>>> +        CTFY (Local_Uid, One)
>>> +        \_SB.PCI0.PRES.CSEL = Local_Uid
>>>          \_SB.PCI0.PRES.CINS = One
>>>          Local_CpuIdx++
>>>      }
>>>
>>>      Release (\_SB.PCI0.PRES.CPLK)
>>>  }
>>
>> With this change, the
>>
>>   virsh setvcpus DOMAIN 8 --live
>>
>> command works for me. The topology in my test domain has CPU#0 and
>> CPU#2 cold-plugged, so the command adds 6 VCPUs. Viewed from the
>> firmware side, the 6 "device_add" commands, issued in close succession
>> by libvirtd, coalesce into 4 "batches". (And of course the firmware
>> sees the 4 batches back-to-back.)
> 
> unfortunately, with more testing, I have run into two more races:
> 
> (1) When a "device_add" occurs after the ACPI loop collects the CPUS
>     from the register block, but before the SMI.
> 
>     Here, the "stray CPU" is processed fine by the firmware. However,
>     the CTFY loop in ACPI does not know about the CPU, so it doesn't
>     clear the pending insert event for it. And when the firmware is
>     entered with an SMI for the *next* time, the firmware sees the same
>     CPU *again* as pending, and tries to relocate it again. Bad things
>     happen.
> 
> (2) When a "device_add" occurs after the SMI, but before the firmware
>     collects the pending CPUs from the register block.
> 
>     Here, the firmware collects the "stray CPU". However, the "broadcast
>     SMI", with which we entered the firmware, did *not* cover the stray
>     CPU -- the CPU_FOREACH() loop in ich9_apm_ctrl_changed() could not
>     make the SMI pending for the new CPU, because at that time, the CPU
>     had not been added yet. As a result, when the firmware sends an
>     INIT-SIPI-SIPI to the new CPU, expecting it to boot right into SMM,
>     the new CPU instead boots straight into the post-RSM (normal mode)
>     "pen", skipping its initial SMI handler. Meaning that the CPU halts
>     nicely, but its SMBASE is never relocated, and the SMRAM message
>     exchange with the BSP falls apart.
> 
> Possible mitigations I can think of:
> 
> For problem (1):
> 
>   (1a) Change the firmware so it notices that it has relocated the
>        "stray" CPU before -- such CPUs should be simply skipped in the
>        firmware. The next time the CTFY loop runs in ACPI, it will clear
>        the pending event.
> 
>   (1b) Alternatively, stop consuming the hotplug register block in the
>        firmware altogether, and work out general message passing, from
>        ACPI to firmware. See the outline here:
> 
>          http://mid.mail-archive.com/cf887d74-f65d-602a-9629-3d25cef93a69@redhat.com
> 
> For problem (2):
> 
>   (2a) Change the firmware so that it sends a directed SMI as well to
>        each CPU, just before sending an INIT-SIPI-SIPI. This should be
>        idempotent -- if the broadcast SMI *has* covered the the CPU,
>        then sending a directed SMI should make no difference.
> 
>   (2b) Alternatively, change the "device_add" command in QEMU so that,
>        if "CPU hotplug with SMI" has been negotiated, the new CPU is
>        added with the SMI made pending for it at once. (That is, no
>        hot-plugged CPU would exist with the directed SMI *not* pending
>        for it.)
> 
>   (2c) Alternatively, approach (1b) would fix problem (2) as well -- the
>        firmware would only relocate such CPUs that ACPI collected before
>        injecting the SMI. So all those CPUs would have the SMI pending.
> 
> 
> I can experiment with (1a) and (2a),

My patches for (1a) and (1b) seem to work -- my workstation has 10
PCPUs, and I'm using a guest with 20 possible VCPUs and 2 cold-plugged
VCPUs on it, for testing. The patches survive the hot-plugging of 18
VCPUs in one go, or two batches like 9+9. I can see the fixes being
exercised.

Unless you strongly disagree (or I find issues in further testing), I
propose that I post these fixes to edk2-devel (they should still be in
scope for the upcoming release), and that we stick with your current
patch series for QEMU (v3 -- upcoming, or maybe already posted).

Thanks!
Laszlo

Re: [PATCH v2 6/7] x68: acpi: trigger SMI before sending hotplug Notify event to OSPM

[Laszlo Ersek]

On 08/26/20 15:32, Laszlo Ersek wrote:
> On 08/26/20 11:24, Laszlo Ersek wrote:
>> Hi Igor,
>>
>> On 08/25/20 19:25, Laszlo Ersek wrote:
>>
>>> So I would suggest fetching the CNEW array element back into "uid"
>>> first, then using "uid" for both the NOTIFY call, and the (currently
>>> missing) restoration of CSEL. Then we can write 1 to CINS.
>>>
>>> Expressed as a patch on top of yours:
>>>
>>>> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
>>>> index 4864c3b39694..2bea6144fd5e 100644
>>>> --- a/hw/acpi/cpu.c
>>>> +++ b/hw/acpi/cpu.c
>>>> @@ -564,8 +564,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>>>>              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_store(aml_derefof(aml_index(new_cpus, cpu_idx)), uid));
>>>> +                aml_append(while_ctx,
>>>> +                    aml_call2(CPU_NOTIFY_METHOD, uid, dev_chk));
>>>> +                aml_append(while_ctx, aml_store(uid, cpu_selector));
>>>>                  aml_append(while_ctx, aml_store(one, ins_evt));
>>>>                  aml_append(while_ctx, aml_increment(cpu_idx));
>>>>              }
>>>
>>> This effects the following change, in the decompiled method:
>>>
>>>> @@ -37,15 +37,17 @@
>>>>      If ((Local_NumAddedCpus != Zero))
>>>>      {
>>>>          \_SB.PCI0.SMI0.SMIC = 0x04
>>>>      }
>>>>
>>>>      Local_CpuIdx = Zero
>>>>      While ((Local_CpuIdx < Local_NumAddedCpus))
>>>>      {
>>>> -        CTFY (DerefOf (CNEW [Local_CpuIdx]), One)
>>>> +        Local_Uid = DerefOf (CNEW [Local_CpuIdx])
>>>> +        CTFY (Local_Uid, One)
>>>> +        \_SB.PCI0.PRES.CSEL = Local_Uid
>>>>          \_SB.PCI0.PRES.CINS = One
>>>>          Local_CpuIdx++
>>>>      }
>>>>
>>>>      Release (\_SB.PCI0.PRES.CPLK)
>>>>  }
>>>
>>> With this change, the
>>>
>>>   virsh setvcpus DOMAIN 8 --live
>>>
>>> command works for me. The topology in my test domain has CPU#0 and
>>> CPU#2 cold-plugged, so the command adds 6 VCPUs. Viewed from the
>>> firmware side, the 6 "device_add" commands, issued in close succession
>>> by libvirtd, coalesce into 4 "batches". (And of course the firmware
>>> sees the 4 batches back-to-back.)
>>
>> unfortunately, with more testing, I have run into two more races:
>>
>> (1) When a "device_add" occurs after the ACPI loop collects the CPUS
>>     from the register block, but before the SMI.
>>
>>     Here, the "stray CPU" is processed fine by the firmware. However,
>>     the CTFY loop in ACPI does not know about the CPU, so it doesn't
>>     clear the pending insert event for it. And when the firmware is
>>     entered with an SMI for the *next* time, the firmware sees the same
>>     CPU *again* as pending, and tries to relocate it again. Bad things
>>     happen.
>>
>> (2) When a "device_add" occurs after the SMI, but before the firmware
>>     collects the pending CPUs from the register block.
>>
>>     Here, the firmware collects the "stray CPU". However, the "broadcast
>>     SMI", with which we entered the firmware, did *not* cover the stray
>>     CPU -- the CPU_FOREACH() loop in ich9_apm_ctrl_changed() could not
>>     make the SMI pending for the new CPU, because at that time, the CPU
>>     had not been added yet. As a result, when the firmware sends an
>>     INIT-SIPI-SIPI to the new CPU, expecting it to boot right into SMM,
>>     the new CPU instead boots straight into the post-RSM (normal mode)
>>     "pen", skipping its initial SMI handler. Meaning that the CPU halts
>>     nicely, but its SMBASE is never relocated, and the SMRAM message
>>     exchange with the BSP falls apart.
>>
>> Possible mitigations I can think of:
>>
>> For problem (1):
>>
>>   (1a) Change the firmware so it notices that it has relocated the
>>        "stray" CPU before -- such CPUs should be simply skipped in the
>>        firmware. The next time the CTFY loop runs in ACPI, it will clear
>>        the pending event.
>>
>>   (1b) Alternatively, stop consuming the hotplug register block in the
>>        firmware altogether, and work out general message passing, from
>>        ACPI to firmware. See the outline here:
>>
>>          http://mid.mail-archive.com/cf887d74-f65d-602a-9629-3d25cef93a69@redhat.com
>>
>> For problem (2):
>>
>>   (2a) Change the firmware so that it sends a directed SMI as well to
>>        each CPU, just before sending an INIT-SIPI-SIPI. This should be
>>        idempotent -- if the broadcast SMI *has* covered the the CPU,
>>        then sending a directed SMI should make no difference.
>>
>>   (2b) Alternatively, change the "device_add" command in QEMU so that,
>>        if "CPU hotplug with SMI" has been negotiated, the new CPU is
>>        added with the SMI made pending for it at once. (That is, no
>>        hot-plugged CPU would exist with the directed SMI *not* pending
>>        for it.)
>>
>>   (2c) Alternatively, approach (1b) would fix problem (2) as well -- the
>>        firmware would only relocate such CPUs that ACPI collected before
>>        injecting the SMI. So all those CPUs would have the SMI pending.
>>
>>
>> I can experiment with (1a) and (2a),
> 
> My patches for (1a) and (1b) seem to work -- my workstation has 10

aargh, I meant my patches for (1a) and (2a).

sorry
Laszlo

> PCPUs, and I'm using a guest with 20 possible VCPUs and 2 cold-plugged
> VCPUs on it, for testing. The patches survive the hot-plugging of 18
> VCPUs in one go, or two batches like 9+9. I can see the fixes being
> exercised.
> 
> Unless you strongly disagree (or I find issues in further testing), I
> propose that I post these fixes to edk2-devel (they should still be in
> scope for the upcoming release), and that we stick with your current
> patch series for QEMU (v3 -- upcoming, or maybe already posted).
> 
> Thanks!
> Laszlo
> 

Re: [PATCH v2 6/7] x68: acpi: trigger SMI before sending hotplug Notify event to OSPM

[Igor Mammedov]

On Wed, 26 Aug 2020 15:32:07 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 08/26/20 11:24, Laszlo Ersek wrote:
> > Hi Igor,
> > 
> > On 08/25/20 19:25, Laszlo Ersek wrote:
> >   
> >> So I would suggest fetching the CNEW array element back into "uid"
> >> first, then using "uid" for both the NOTIFY call, and the (currently
> >> missing) restoration of CSEL. Then we can write 1 to CINS.
> >>
> >> Expressed as a patch on top of yours:
> >>  
> >>> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> >>> index 4864c3b39694..2bea6144fd5e 100644
> >>> --- a/hw/acpi/cpu.c
> >>> +++ b/hw/acpi/cpu.c
> >>> @@ -564,8 +564,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
> >>>              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_store(aml_derefof(aml_index(new_cpus, cpu_idx)), uid));
> >>> +                aml_append(while_ctx,
> >>> +                    aml_call2(CPU_NOTIFY_METHOD, uid, dev_chk));
> >>> +                aml_append(while_ctx, aml_store(uid, cpu_selector));
> >>>                  aml_append(while_ctx, aml_store(one, ins_evt));
> >>>                  aml_append(while_ctx, aml_increment(cpu_idx));
> >>>              }  
> >>
> >> This effects the following change, in the decompiled method:
> >>  
> >>> @@ -37,15 +37,17 @@
> >>>      If ((Local_NumAddedCpus != Zero))
> >>>      {
> >>>          \_SB.PCI0.SMI0.SMIC = 0x04
> >>>      }
> >>>
> >>>      Local_CpuIdx = Zero
> >>>      While ((Local_CpuIdx < Local_NumAddedCpus))
> >>>      {
> >>> -        CTFY (DerefOf (CNEW [Local_CpuIdx]), One)
> >>> +        Local_Uid = DerefOf (CNEW [Local_CpuIdx])
> >>> +        CTFY (Local_Uid, One)
> >>> +        \_SB.PCI0.PRES.CSEL = Local_Uid
> >>>          \_SB.PCI0.PRES.CINS = One
> >>>          Local_CpuIdx++
> >>>      }
> >>>
> >>>      Release (\_SB.PCI0.PRES.CPLK)
> >>>  }  
> >>
> >> With this change, the
> >>
> >>   virsh setvcpus DOMAIN 8 --live
> >>
> >> command works for me. The topology in my test domain has CPU#0 and
> >> CPU#2 cold-plugged, so the command adds 6 VCPUs. Viewed from the
> >> firmware side, the 6 "device_add" commands, issued in close succession
> >> by libvirtd, coalesce into 4 "batches". (And of course the firmware
> >> sees the 4 batches back-to-back.)  
> > 
> > unfortunately, with more testing, I have run into two more races:
> > 
> > (1) When a "device_add" occurs after the ACPI loop collects the CPUS
> >     from the register block, but before the SMI.
> > 
> >     Here, the "stray CPU" is processed fine by the firmware. However,
> >     the CTFY loop in ACPI does not know about the CPU, so it doesn't
> >     clear the pending insert event for it. And when the firmware is
> >     entered with an SMI for the *next* time, the firmware sees the same
> >     CPU *again* as pending, and tries to relocate it again. Bad things
> >     happen.
> > 
> > (2) When a "device_add" occurs after the SMI, but before the firmware
> >     collects the pending CPUs from the register block.
> > 
> >     Here, the firmware collects the "stray CPU". However, the "broadcast
> >     SMI", with which we entered the firmware, did *not* cover the stray
> >     CPU -- the CPU_FOREACH() loop in ich9_apm_ctrl_changed() could not
> >     make the SMI pending for the new CPU, because at that time, the CPU
> >     had not been added yet. As a result, when the firmware sends an
> >     INIT-SIPI-SIPI to the new CPU, expecting it to boot right into SMM,
> >     the new CPU instead boots straight into the post-RSM (normal mode)
> >     "pen", skipping its initial SMI handler. Meaning that the CPU halts
> >     nicely, but its SMBASE is never relocated, and the SMRAM message
> >     exchange with the BSP falls apart.
> > 
> > Possible mitigations I can think of:
> > 
> > For problem (1):
> > 
> >   (1a) Change the firmware so it notices that it has relocated the
> >        "stray" CPU before -- such CPUs should be simply skipped in the
> >        firmware. The next time the CTFY loop runs in ACPI, it will clear
> >        the pending event.
> > 
> >   (1b) Alternatively, stop consuming the hotplug register block in the
> >        firmware altogether, and work out general message passing, from
> >        ACPI to firmware. See the outline here:
> > 
> >          http://mid.mail-archive.com/cf887d74-f65d-602a-9629-3d25cef93a69@redhat.com
> > 
> > For problem (2):
> > 
> >   (2a) Change the firmware so that it sends a directed SMI as well to
> >        each CPU, just before sending an INIT-SIPI-SIPI. This should be
> >        idempotent -- if the broadcast SMI *has* covered the the CPU,
> >        then sending a directed SMI should make no difference.
> > 
> >   (2b) Alternatively, change the "device_add" command in QEMU so that,
> >        if "CPU hotplug with SMI" has been negotiated, the new CPU is
> >        added with the SMI made pending for it at once. (That is, no
> >        hot-plugged CPU would exist with the directed SMI *not* pending
> >        for it.)
> > 
> >   (2c) Alternatively, approach (1b) would fix problem (2) as well -- the
> >        firmware would only relocate such CPUs that ACPI collected before
> >        injecting the SMI. So all those CPUs would have the SMI pending.
> > 
> > 
> > I can experiment with (1a) and (2a),  
> 
> My patches for (1a) and (1b) seem to work -- my workstation has 10
> PCPUs, and I'm using a guest with 20 possible VCPUs and 2 cold-plugged
> VCPUs on it, for testing. The patches survive the hot-plugging of 18
> VCPUs in one go, or two batches like 9+9. I can see the fixes being
> exercised.
> 
> Unless you strongly disagree (or I find issues in further testing), I
> propose that I post these fixes to edk2-devel (they should still be in
> scope for the upcoming release), and that we stick with your current
> patch series for QEMU (v3 -- upcoming, or maybe already posted).

Lets do as you suggest.
As for QEMU side, I'll try to post next revision next week.

> 
> Thanks!
> Laszlo
> 
> 

Re: [PATCH v2 6/7] x68: acpi: trigger SMI before sending hotplug Notify event to OSPM

[Laszlo Ersek]

On 08/26/20 13:55, Igor Mammedov wrote:
> On Wed, 26 Aug 2020 11:24:14 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:

>>   (2a) Change the firmware so that it sends a directed SMI as well to
>>        each CPU, just before sending an INIT-SIPI-SIPI. This should be
>>        idempotent -- if the broadcast SMI *has* covered the the CPU,
>>        then sending a directed SMI should make no difference.
> may be still racy, as new cpus can arrive diring/after direct broadcast.

(I think you meant "direct SMI")

That's not a problem -- the point is that we must never send
INIT-SIPI-SIPI to a hot-added CPU without making an SMI pending for it.

The above condition can be satisfied by not sending INIT-SIPI-SIPI to a
VCPU at all.

The firmware collects pending CPUs into an array, and then does the
directed SMI + INIT-SIPI-SIPI dance for each, in a separate loop.

So if a new VCPU is hot-added while we are sending the interrupts to the
already collected ones, that's fine -- we're not going to send *either*
SMI *or* INIT-SIPI-SIPI to that VCPU, until the next time we collect VCPUS.

It's basically the same idea as in your ACPI patch for QEMU.

I'll send the OVMF patches soon.

Thanks!
Laszlo