On 2020-11-26 4:46 a.m., Laszlo Ersek wrote:
On 11/26/20 11:24, Ankur Arora wrote:
On 2020-11-24 4:25 a.m., 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 <imammedo@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
+ 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/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;
I might be missing something but I don't see where cdev->fw_remove is being
set.
See just below, in the cpu_hotplug_wr() hunk. When bit#4 is written --
which happens through the ACPI code change --, fw_remove is inverted.
Thanks that makes sense. I was reading the AML building code all wrong.
We do set cdev->is_removing in acpi_cpu_unplug_request_cb() so AFAICS
we would always end up setting this bit:
val |= cdev->is_removing ? 4 : 0;
Also, if cdev->fw_remove and cdev->is_removing are both true, val would be
(4 | 16). I'm guessing that in that case the AML determines which case gets
handled but it might make sense to set just one of these?
"is_removing" is set directly in response to the device_del QMP command.
That QMP command is asynchronous to the execution of the guest OS.
j
"fw_remove" is set (by virtue of inverting) by ACPI CEJ0, which is
executed by the guest OS's ACPI interpreter, after the guest OS has
de-scheduled all processes from the CPU being removed (= basically after
the OS has willfully forgotten about the CPU).
Therefore, considering the bitmask (is_removing, fw_remove), three
variations make sense:
Just annotating these with the corresponding ACPI code to make sure
I have it straight. Please correct if my interpretation is wrong. Also,
a few questions inline:
#1 (is_removing=0, fw_remove=0) -- normal status; no unplug requested
#2 (is_removing=1, fw_remove=0) -- unplug requested via QMP, guest OS
is processing the request
Guest executes the CSCN method and reads rm_evt (bit 2) (thus noticing
the is_removing=1), and then notifies the CPU to be removed via the
CTFY method.
ifctx = aml_if(aml_equal(rm_evt, one));
{
aml_append(ifctx,
aml_call2(CPU_NOTIFY_METHOD, uid, eject_req));
aml_append(ifctx, aml_store(one, rm_evt));
aml_append(ifctx, aml_store(one, has_event));
}
Then it does a store to rm_evt (bit 2). That would result in clearing
of is_removing. (Igor mentions that in a separate mail.)
1. Do we need to clear is_removing at all? AFAICS, it's only useful as
an ack to QEMU and I can't think of why that's useful. OTOH it
doesn't serve any useful purpose once the guest OS has seen the request.