On Thu, 26 Nov 2020 12:17:27 +0100
Laszlo Ersek <ler...@redhat.com> wrote:
> On 11/24/20 13:25, Igor Mammedov wrote:
> > If firmware negotiates ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT feature,
> > OSPM on CPU eject will set bit #4 in CPU hotplug block for to be
> > ejected CPU to mark it for removal by firmware and trigger SMI
> > upcall to let firmware do actual eject.
> >
> > Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> > ---
> > PS:
> > - abuse 5.1 machine type for now to turn off unplug feature
> > (it will be moved to 5.2 machine type once new merge window is open)
> > ---
> > include/hw/acpi/cpu.h | 2 ++
> > docs/specs/acpi_cpu_hotplug.txt | 11 +++++++++--
> > hw/acpi/cpu.c | 18 ++++++++++++++++--
> > hw/i386/acpi-build.c | 5 +++++
> > hw/i386/pc.c | 1 +
> > hw/isa/lpc_ich9.c | 2 +-
> > 6 files changed, 34 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> > index 0eeedaa491..999caaf510 100644
> > --- a/include/hw/acpi/cpu.h
> > +++ b/include/hw/acpi/cpu.h
> > @@ -22,6 +22,7 @@ typedef struct AcpiCpuStatus {
> > uint64_t arch_id;
> > bool is_inserting;
> > bool is_removing;
> > + bool fw_remove;
> > uint32_t ost_event;
> > uint32_t ost_status;
> > } AcpiCpuStatus;
> > @@ -50,6 +51,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
> > typedef struct CPUHotplugFeatures {
> > bool acpi_1_compatible;
> > bool has_legacy_cphp;
> > + bool fw_unplugs_cpu;
> > const char *smi_path;
> > } CPUHotplugFeatures;
> >
> > diff --git a/docs/specs/acpi_cpu_hotplug.txt
> > b/docs/specs/acpi_cpu_hotplug.txt
> > index 9bb22d1270..f68ef6e06c 100644
> > --- a/docs/specs/acpi_cpu_hotplug.txt
> > +++ b/docs/specs/acpi_cpu_hotplug.txt
> > @@ -57,7 +57,11 @@ read access:
> > It's valid only when bit 0 is set.
> > 2: Device remove event, used to distinguish device for which
> > no device eject request to OSPM was issued.
> > - 3-7: reserved and should be ignored by OSPM
> > + 3: reserved and should be ignored by OSPM
> > + 4: if set to 1, OSPM requests firmware to perform device eject,
> > + firmware shall clear this event by writing 1 into it before
>
> (1) s/clear this event/clear this event bit/
>
> > + performing device eject.
>
> (2) move the second and third lines ("firmware shall clear....") over to
> the write documentation, below? In particular:
>
> > + 5-7: reserved and should be ignored by OSPM
> > [0x5-0x7] reserved
> > [0x8] Command data: (DWORD access)
> > contains 0 unless value last stored in 'Command field' is one of:
> > @@ -82,7 +86,10 @@ write access:
> > selected CPU device
> > 3: if set to 1 initiates device eject, set by OSPM when it
> > triggers CPU device removal and calls _EJ0 method
> > - 4-7: reserved, OSPM must clear them before writing to register
> > + 4: if set to 1 OSPM hands over device eject to firmware,
> > + Firmware shall issue device eject request as described above
> > + (bit #3) and OSPM should not touch device eject bit (#3),
>
> (3) it would be clearer if we documented the exact bit writing order
> here:
> - clear bit#4, *then* set bit#3 (two write accesses)
> - versus clear bit#4 *and* set bit#3 (single access)
I was thinking that FW should not bother with clearing bit #4,
and QEMU should clear it when handling write to bit #3.
(it looks like I forgot to actually do that)
>
>
>
> > + 5-7: reserved, OSPM must clear them before writing to register
> > [0x5] Command field: (1 byte access)
> > value:
> > 0: selects a CPU device with inserting/removing events and
> > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> > index f099b50927..09d2f20dae 100644
> > --- a/hw/acpi/cpu.c
> > +++ b/hw/acpi/cpu.c
> > @@ -71,6 +71,7 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr,
> > unsigned size)
> > val |= cdev->cpu ? 1 : 0;
> > val |= cdev->is_inserting ? 2 : 0;
> > val |= cdev->is_removing ? 4 : 0;
> > + val |= cdev->fw_remove ? 16 : 0;
> > trace_cpuhp_acpi_read_flags(cpu_st->selector, val);
> > break;
> > case ACPI_CPU_CMD_DATA_OFFSET_RW:
> > @@ -148,6 +149,8 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr,
> > uint64_t data,
> > hotplug_ctrl = qdev_get_hotplug_handler(dev);
> > hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
> > object_unparent(OBJECT(dev));
> > + } else if (data & 16) {
> > + cdev->fw_remove = !cdev->fw_remove;
>
> hm... so I guess the ACPI code will first write bit#4 to flip
> "fw_remove" from "off" to "on". Then the firmware will write bit#4 to
> flip "fw_remove" back to "off". And finally, the firmware will write
> bit#3 (strictly as a separate access) to unplug the CPU.
sorry for confusion in doc vs impl, FW should only read bit #4, as for bit #3
only write.
> (4) But anyway, taking a step back: what do we need the new bit for?
>
> > }
> > break;
> > case ACPI_CPU_CMD_OFFSET_WR:
> > @@ -332,6 +335,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
> > #define CPU_INSERT_EVENT "CINS"
> > #define CPU_REMOVE_EVENT "CRMV"
> > #define CPU_EJECT_EVENT "CEJ0"
> > +#define CPU_FW_EJECT_EVENT "CEJF"
> >
> > void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures
> > opts,
> > hwaddr io_base,
> > @@ -384,7 +388,10 @@ void build_cpus_aml(Aml *table, MachineState *machine,
> > CPUHotplugFeatures opts,
> > aml_append(field, aml_named_field(CPU_REMOVE_EVENT, 1));
> > /* initiates device eject, write only */
> > aml_append(field, aml_named_field(CPU_EJECT_EVENT, 1));
> > - aml_append(field, aml_reserved_field(4));
> > + aml_append(field, aml_reserved_field(1));
> > + /* tell firmware to do device eject, write only */
> > + aml_append(field, aml_named_field(CPU_FW_EJECT_EVENT, 1));
> > + aml_append(field, aml_reserved_field(2));
> > aml_append(field, aml_named_field(CPU_COMMAND, 8));
> > aml_append(cpu_ctrl_dev, field);
> >
> > @@ -419,6 +426,7 @@ void build_cpus_aml(Aml *table, MachineState *machine,
> > CPUHotplugFeatures opts,
> > Aml *ins_evt = aml_name("%s.%s", cphp_res_path, CPU_INSERT_EVENT);
> > Aml *rm_evt = aml_name("%s.%s", cphp_res_path, CPU_REMOVE_EVENT);
> > Aml *ej_evt = aml_name("%s.%s", cphp_res_path, CPU_EJECT_EVENT);
> > + Aml *fw_ej_evt = aml_name("%s.%s", cphp_res_path,
> > CPU_FW_EJECT_EVENT);
> >
> > aml_append(cpus_dev, aml_name_decl("_HID",
> > aml_string("ACPI0010")));
> > aml_append(cpus_dev, aml_name_decl("_CID", aml_eisaid("PNP0A05")));
> > @@ -461,7 +469,13 @@ void build_cpus_aml(Aml *table, MachineState *machine,
> > CPUHotplugFeatures opts,
> >
> > aml_append(method, aml_acquire(ctrl_lock, 0xFFFF));
> > aml_append(method, aml_store(idx, cpu_selector));
> > - aml_append(method, aml_store(one, ej_evt));
> > + if (opts.fw_unplugs_cpu) {
> > + aml_append(method, aml_store(one, fw_ej_evt));
> > + aml_append(method, aml_store(aml_int(OVMF_CPUHP_SMI_CMD),
> > + aml_name("%s", opts.smi_path)));
> > + } else {
> > + aml_append(method, aml_store(one, ej_evt));
> > + }
> > aml_append(method, aml_release(ctrl_lock));
> > }
> > aml_append(cpus_dev, method);
>
> Hmmm, OK, let me parse this.
>
> Assume there is a big bunch of device_del QMP commands, QEMU marks the
> "remove" event pending on the corresponding set of CPUs, plus also makes
> the ACPI interrupt pending. The ACPI interrupt handler in the OS runs,
> and calls CSCN. CSCN runs a loop, and for each CPU where the remove
> event is pending, notifies the OS one by one. The OS in turn forgets
> about the subject CPU, and calls the _EJ0 method on the affected CPU
> ACPI object. The _EJ0 method on the CPU ACPI object calls CEJ0, passing
> in the affected CPU's identifier.
>
> The above hunk modifies the CEJ0 method.
>
> (5) Question: pre-patch, both the CSCN method and the CEJ0 method
> acquire the CPLK lock, but CEJ0 is actually called within CSCN
> (indirectly, with the OS's cooperation). Is CPLK a recursive lock?
Theoretically scep supports recursive mutexes but I don't think it's the case
here.
Considering it works currently, I think OS implements Notify event as async.
hence no clash wrt mutex. If EJ0 were handled within CSCN context,
EJ0 would mess cpu_selector value that CSCN is also using.
> Anyway, let's see the CEJ0 modification. After the OS is done forgetting
> about the CPU, the CEJ0 method no longer unplugs the CPU, instead it
> sets the new bit#4 in the register block, and raises an SMI.
>
> (6) So that's one SMI per CPU being removed. Is that OK?
I guess it has performance penalty but there is nothing we can do about it,
OSPM does EJ0 calls asynchronously.
> (7) What if there are asynchronous plugs going on, and the firmware
> notices them in the register block? ... Hm, I hope that should be OK,
> because ultimately the CSCN method will learn about those too, and
> inform the OS. On plug, the firmware doesn't modify the register block.
shouldn't be issue (modulo bugs, I haven't tried to hot add and hot remove
the same CPU at the same time)
i.e.
(QEMU) pause
(QEMU) device_add
(QEMU) device_del
(QEMU) cont
> Ah! OK. I think I understand why bit#4 is important. The firmware may
> notice pending remove events, but it must not act upon them -- it must
> simply ignore them -- unless bit#4 is also set. Bit#2 set with bit#4
> clear means the event is pending (QEMU got a device_del), but the OS has
> not forgotten about the CPU yet -- so the firmware must not unplug it
> yet. When the modified CEJ0 method runs, it sets bit#4 in addition to
> the already set bit#2, advertising that the OS has *already* abandoned
> the CPU.
firmware should ignore bit #2, it doesn't mean anything to it, OSPM might
ignore or nonsupport CPU removal. What firmware must care about is bit #4,
which tells it that OSPM is done with CPU and asks for to be removed by
firmware.
>
> This means we'll have to modify the QemuCpuhpCollectApicIds() function
> in OVMF as well -- for collecting a CPU for unplug, just bit#2
> (QEMU_CPUHP_STAT_REMOVE) is insufficient -- on such CPUs, the OS may
> still be executing threads.
>
> OK, this approach sounds plausible to me.
>
> (8) Please extend the description of bit#2 in the "status flags read
> access" section: "firmware must ignore bit#2 unless bit#4 is set".
>
>
>
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 1f5c211245..475e76f514 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -96,6 +96,7 @@ typedef struct AcpiPmInfo {
> > bool s4_disabled;
> > bool pcihp_bridge_en;
> > bool smi_on_cpuhp;
> > + bool smi_on_cpu_unplug;
> > bool pcihp_root_en;
> > uint8_t s4_val;
> > AcpiFadtData fadt;
> > @@ -197,6 +198,7 @@ static void acpi_get_pm_info(MachineState *machine,
> > AcpiPmInfo *pm)
> > pm->pcihp_io_base = 0;
> > pm->pcihp_io_len = 0;
> > pm->smi_on_cpuhp = false;
> > + pm->smi_on_cpu_unplug = false;
> >
> > assert(obj);
> > init_common_fadt_data(machine, obj, &pm->fadt);
> > @@ -220,6 +222,8 @@ static void acpi_get_pm_info(MachineState *machine,
> > AcpiPmInfo *pm)
> > 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));
> > + pm->smi_on_cpu_unplug =
> > + !!(smi_features & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT));
> > }
> >
> > /* The above need not be conditional on machine type because the reset
> > port
> > @@ -1582,6 +1586,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > CPUHotplugFeatures opts = {
> > .acpi_1_compatible = true, .has_legacy_cphp = true,
> > .smi_path = pm->smi_on_cpuhp ? "\\_SB.PCI0.SMI0.SMIC" : NULL,
> > + .fw_unplugs_cpu = pm->smi_on_cpu_unplug,
> > };
> > build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> > "\\_SB.PCI0", "\\_GPE._E02");
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 17b514d1da..2952a00fe6 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -99,6 +99,7 @@
> >
> > GlobalProperty pc_compat_5_1[] = {
> > { "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
> > + { "ICH9-LPC", "x-smi-cpu-hotunplug", "off" },
> > };
> > const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
> >
> > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> > index 087a18d04d..8c667b7166 100644
> > --- a/hw/isa/lpc_ich9.c
> > +++ b/hw/isa/lpc_ich9.c
> > @@ -770,7 +770,7 @@ static Property ich9_lpc_properties[] = {
> > 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),
> > + ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT, true),
> > DEFINE_PROP_END_OF_LIST(),
> > };
> >
> >
>
> (9) You have to extend smi_features_ok_callback() as well -- it is
> invalid for the firmware to negotiate unplug, without negotiating plug.
>
> In fact, as far as I can tell, that would even crash QEMU, given this
> patch. Because, "opts.smi_path" would be set to NULL, but
> "opts.fw_unplugs_cpu" would be set to "true". As a consequence, the
> CPU_EJECT_METHOD change above would call aml_name("%s", NULL).
>
> So something like the following looks necessary:
Thanks for suggestions,
I'll respin v2 with your feedback included.
>
> > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> > index 8c667b7166c7..5bc3f212fe77 100644
> > --- a/hw/isa/lpc_ich9.c
> > +++ b/hw/isa/lpc_ich9.c
> > @@ -366,6 +366,7 @@ static void smi_features_ok_callback(void *opaque)
> > {
> > ICH9LPCState *lpc = opaque;
> > uint64_t guest_features;
> > + uint64_t guest_cpu_hotplug_features;
> >
> > if (lpc->smi_features_ok) {
> > /* negotiation already complete, features locked */
> > @@ -378,9 +379,11 @@ static void smi_features_ok_callback(void *opaque)
> > /* guest requests invalid features, leave @features_ok at zero */
> > return;
> > }
> > + guest_cpu_hotplug_features = guest_features &
> > + (BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT) |
> > +
> > BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT));
> > 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))) {
> > + guest_cpu_hotplug_features) {
> > /*
> > * cpu hot-[un]plug with SMI requires SMI broadcast,
> > * leave @features_ok at zero
> > @@ -388,6 +391,12 @@ static void smi_features_ok_callback(void *opaque)
> > return;
> > }
> >
> > + if (guest_cpu_hotplug_features ==
> > + BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT)) {
> > + /* cpu hot-unplug is unsupported without cpu-hotplug */
> > + return;
> > + }
> > +
> > /* valid feature subset requested, lock it down, report success */
> > lpc->smi_negotiated_features = guest_features;
> > lpc->smi_features_ok = 1;
>
>
> (10) It would be nice to separate this work into multiple patches. I
> propose:
>
> - [PATCH 1/5] x86: ich9: factor out "guest_cpu_hotplug_features"
>
> > hw/isa/lpc_ich9.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> > index 087a18d04de4..c46eefd13fd4 100644
> > --- a/hw/isa/lpc_ich9.c
> > +++ b/hw/isa/lpc_ich9.c
> > @@ -366,6 +366,7 @@ static void smi_features_ok_callback(void *opaque)
> > {
> > ICH9LPCState *lpc = opaque;
> > uint64_t guest_features;
> > + uint64_t guest_cpu_hotplug_features;
> >
> > if (lpc->smi_features_ok) {
> > /* negotiation already complete, features locked */
> > @@ -378,9 +379,11 @@ static void smi_features_ok_callback(void *opaque)
> > /* guest requests invalid features, leave @features_ok at zero */
> > return;
> > }
> > + guest_cpu_hotplug_features = guest_features &
> > + (BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT) |
> > +
> > BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT));
> > 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))) {
> > + guest_cpu_hotplug_features) {
> > /*
> > * cpu hot-[un]plug with SMI requires SMI broadcast,
> > * leave @features_ok at zero
>
>
> - [PATCH 2/5] x86: ich9: let firmware negotiate 'CPU hot-unplug with SMI'
> feature
>
> > hw/i386/pc.c | 1 +
> > hw/isa/lpc_ich9.c | 8 +++++++-
> > 2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 17b514d1da50..2952a00fe694 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -99,6 +99,7 @@
> >
> > GlobalProperty pc_compat_5_1[] = {
> > { "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
> > + { "ICH9-LPC", "x-smi-cpu-hotunplug", "off" },
> > };
> > const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
> >
> > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> > index c46eefd13fd4..5bc3f212fe77 100644
> > --- a/hw/isa/lpc_ich9.c
> > +++ b/hw/isa/lpc_ich9.c
> > @@ -391,6 +391,12 @@ static void smi_features_ok_callback(void *opaque)
> > return;
> > }
> >
> > + if (guest_cpu_hotplug_features ==
> > + BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT)) {
> > + /* cpu hot-unplug is unsupported without cpu-hotplug */
> > + return;
> > + }
> > +
> > /* valid feature subset requested, lock it down, report success */
> > lpc->smi_negotiated_features = guest_features;
> > lpc->smi_features_ok = 1;
> > @@ -773,7 +779,7 @@ static Property ich9_lpc_properties[] = {
> > 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),
> > + ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT, true),
> > DEFINE_PROP_END_OF_LIST(),
> > };
> >
>
>
> - [PATCH 3/5] x86: acpi: introduce AcpiPmInfo::smi_on_cpu_unplug
>
> > hw/i386/acpi-build.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 1f5c2112452a..9036e5594c92 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -96,6 +96,7 @@ typedef struct AcpiPmInfo {
> > bool s4_disabled;
> > bool pcihp_bridge_en;
> > bool smi_on_cpuhp;
> > + bool smi_on_cpu_unplug;
> > bool pcihp_root_en;
> > uint8_t s4_val;
> > AcpiFadtData fadt;
> > @@ -197,6 +198,7 @@ static void acpi_get_pm_info(MachineState *machine,
> > AcpiPmInfo *pm)
> > pm->pcihp_io_base = 0;
> > pm->pcihp_io_len = 0;
> > pm->smi_on_cpuhp = false;
> > + pm->smi_on_cpu_unplug = false;
> >
> > assert(obj);
> > init_common_fadt_data(machine, obj, &pm->fadt);
> > @@ -220,6 +222,8 @@ static void acpi_get_pm_info(MachineState *machine,
> > AcpiPmInfo *pm)
> > 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));
> > + pm->smi_on_cpu_unplug =
> > + !!(smi_features & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT));
> > }
> >
> > /* The above need not be conditional on machine type because the reset
> > port
>
>
> - [PATCH 4/5] acpi: cpuhp: introduce 'firmware performs eject' status/control
> bits
>
> > docs/specs/acpi_cpu_hotplug.txt | 11 +++++++++--
> > include/hw/acpi/cpu.h | 1 +
> > hw/acpi/cpu.c | 3 +++
> > 3 files changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/docs/specs/acpi_cpu_hotplug.txt
> > b/docs/specs/acpi_cpu_hotplug.txt
> > index 9bb22d1270a9..f68ef6e06c7a 100644
> > --- a/docs/specs/acpi_cpu_hotplug.txt
> > +++ b/docs/specs/acpi_cpu_hotplug.txt
> > @@ -57,7 +57,11 @@ read access:
> > It's valid only when bit 0 is set.
> > 2: Device remove event, used to distinguish device for which
> > no device eject request to OSPM was issued.
> > - 3-7: reserved and should be ignored by OSPM
> > + 3: reserved and should be ignored by OSPM
> > + 4: if set to 1, OSPM requests firmware to perform device eject,
> > + firmware shall clear this event by writing 1 into it before
> > + performing device eject.
> > + 5-7: reserved and should be ignored by OSPM
> > [0x5-0x7] reserved
> > [0x8] Command data: (DWORD access)
> > contains 0 unless value last stored in 'Command field' is one of:
> > @@ -82,7 +86,10 @@ write access:
> > selected CPU device
> > 3: if set to 1 initiates device eject, set by OSPM when it
> > triggers CPU device removal and calls _EJ0 method
> > - 4-7: reserved, OSPM must clear them before writing to register
> > + 4: if set to 1 OSPM hands over device eject to firmware,
> > + Firmware shall issue device eject request as described above
> > + (bit #3) and OSPM should not touch device eject bit (#3),
> > + 5-7: reserved, OSPM must clear them before writing to register
> > [0x5] Command field: (1 byte access)
> > value:
> > 0: selects a CPU device with inserting/removing events and
> > diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> > index 0eeedaa491c1..d71edde456f2 100644
> > --- a/include/hw/acpi/cpu.h
> > +++ b/include/hw/acpi/cpu.h
> > @@ -22,6 +22,7 @@ typedef struct AcpiCpuStatus {
> > uint64_t arch_id;
> > bool is_inserting;
> > bool is_removing;
> > + bool fw_remove;
> > uint32_t ost_event;
> > uint32_t ost_status;
> > } AcpiCpuStatus;
> > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> > index f099b5092730..3dc83d73e20b 100644
> > --- a/hw/acpi/cpu.c
> > +++ b/hw/acpi/cpu.c
> > @@ -71,6 +71,7 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr,
> > unsigned size)
> > val |= cdev->cpu ? 1 : 0;
> > val |= cdev->is_inserting ? 2 : 0;
> > val |= cdev->is_removing ? 4 : 0;
> > + val |= cdev->fw_remove ? 16 : 0;
> > trace_cpuhp_acpi_read_flags(cpu_st->selector, val);
> > break;
> > case ACPI_CPU_CMD_DATA_OFFSET_RW:
> > @@ -148,6 +149,8 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr,
> > uint64_t data,
> > hotplug_ctrl = qdev_get_hotplug_handler(dev);
> > hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
> > object_unparent(OBJECT(dev));
> > + } else if (data & 16) {
> > + cdev->fw_remove = !cdev->fw_remove;
> > }
> > break;
> > case ACPI_CPU_CMD_OFFSET_WR:
>
>
> - [PATCH 5/5] x86: acpi: let the firmware handle pending "CPU remove" events
> in SMM
>
> > include/hw/acpi/cpu.h | 1 +
> > hw/acpi/cpu.c | 15 +++++++++++++--
> > hw/i386/acpi-build.c | 1 +
> > 3 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> > index d71edde456f2..999caaf51060 100644
> > --- a/include/hw/acpi/cpu.h
> > +++ b/include/hw/acpi/cpu.h
> > @@ -51,6 +51,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
> > typedef struct CPUHotplugFeatures {
> > bool acpi_1_compatible;
> > bool has_legacy_cphp;
> > + bool fw_unplugs_cpu;
> > const char *smi_path;
> > } CPUHotplugFeatures;
> >
> > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> > index 3dc83d73e20b..09d2f20daec0 100644
> > --- a/hw/acpi/cpu.c
> > +++ b/hw/acpi/cpu.c
> > @@ -335,6 +335,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
> > #define CPU_INSERT_EVENT "CINS"
> > #define CPU_REMOVE_EVENT "CRMV"
> > #define CPU_EJECT_EVENT "CEJ0"
> > +#define CPU_FW_EJECT_EVENT "CEJF"
> >
> > void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures
> > opts,
> > hwaddr io_base,
> > @@ -387,7 +388,10 @@ void build_cpus_aml(Aml *table, MachineState *machine,
> > CPUHotplugFeatures opts,
> > aml_append(field, aml_named_field(CPU_REMOVE_EVENT, 1));
> > /* initiates device eject, write only */
> > aml_append(field, aml_named_field(CPU_EJECT_EVENT, 1));
> > - aml_append(field, aml_reserved_field(4));
> > + aml_append(field, aml_reserved_field(1));
> > + /* tell firmware to do device eject, write only */
> > + aml_append(field, aml_named_field(CPU_FW_EJECT_EVENT, 1));
> > + aml_append(field, aml_reserved_field(2));
> > aml_append(field, aml_named_field(CPU_COMMAND, 8));
> > aml_append(cpu_ctrl_dev, field);
> >
> > @@ -422,6 +426,7 @@ void build_cpus_aml(Aml *table, MachineState *machine,
> > CPUHotplugFeatures opts,
> > Aml *ins_evt = aml_name("%s.%s", cphp_res_path, CPU_INSERT_EVENT);
> > Aml *rm_evt = aml_name("%s.%s", cphp_res_path, CPU_REMOVE_EVENT);
> > Aml *ej_evt = aml_name("%s.%s", cphp_res_path, CPU_EJECT_EVENT);
> > + Aml *fw_ej_evt = aml_name("%s.%s", cphp_res_path,
> > CPU_FW_EJECT_EVENT);
> >
> > aml_append(cpus_dev, aml_name_decl("_HID",
> > aml_string("ACPI0010")));
> > aml_append(cpus_dev, aml_name_decl("_CID", aml_eisaid("PNP0A05")));
> > @@ -464,7 +469,13 @@ void build_cpus_aml(Aml *table, MachineState *machine,
> > CPUHotplugFeatures opts,
> >
> > aml_append(method, aml_acquire(ctrl_lock, 0xFFFF));
> > aml_append(method, aml_store(idx, cpu_selector));
> > - aml_append(method, aml_store(one, ej_evt));
> > + if (opts.fw_unplugs_cpu) {
> > + aml_append(method, aml_store(one, fw_ej_evt));
> > + aml_append(method, aml_store(aml_int(OVMF_CPUHP_SMI_CMD),
> > + aml_name("%s", opts.smi_path)));
> > + } else {
> > + aml_append(method, aml_store(one, ej_evt));
> > + }
> > 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 9036e5594c92..475e76f514ff 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1586,6 +1586,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > CPUHotplugFeatures opts = {
> > .acpi_1_compatible = true, .has_legacy_cphp = true,
> > .smi_path = pm->smi_on_cpuhp ? "\\_SB.PCI0.SMI0.SMIC" : NULL,
> > + .fw_unplugs_cpu = pm->smi_on_cpu_unplug,
> > };
> > build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> > "\\_SB.PCI0", "\\_GPE._E02");
>
> Thanks!
> Laszlo
>
>
