[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast

From: Ankur Arora
Subject: Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled
Date: Fri, 27 Nov 2020 15:48:48 -0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0

On 2020-11-27 7:02 a.m., Laszlo Ersek wrote:
On 11/27/20 12:33, Igor Mammedov wrote:
On Thu, 26 Nov 2020 19:35:30 -0800
Ankur Arora <ankur.a.arora@oracle.com> wrote:

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>
     - 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
    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
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
+              firmware shall clear this event by writing 1 into it
+              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
+            4: if set to 1 OSPM hands over device eject to firmware,
+               Firmware shall issue device eject request as described
+               (bit #3) and OSPM should not touch device eject bit (#3),
+            5-7: reserved, OSPM must clear them before writing to
        [0x5] Command field: (1 byte access)
                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

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.
"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_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.
no firmware doesn't need to care about it, it's consumed by OSPM only
2. Would it make sense to clear it first and then call CPU_NOTIFY_METHOD?
CPU_NOTIFY_METHOD (or _EJ0, COST) don't depend on is_removing but
that might change in the future.

all methods are protected by be same mutex, so if _EJ0 is called while CSCN
in progress it will wait till CSCN is finished.
But clearing bit #2 before Notify should work too.

I'd suggest not reordering existent stuff unless we really have to; the
firmware will have to deal with "is_removing" being the *only* status
flag set anway, as QMP "device_del" command(s) may set that bit for
another CPU (or multiple other CPUs) while the SMI handler is running,
and the "get pending" method will return such CPUs as well.

Yeah I was must making sure my understanding of these related pieces of
code was correct. And anyway, as Igor mentioned, that bit of AML is protected
by the mutex so the ordering doesn't even matter.

I wouldn't complicate the patches just in order to "hide" is_removing --
that's not a goal, so let's just keep as much AML untouched as we can.

(BTW I now understand why "is_removing" is clear when the eject method
runs for the same CPU -- because Notify (and so the eject method) is not
entered synchronously, it's only queued asynchronously. So it's actually
dispatched after the is_removing flag has been cleared.)

The notify would end up in calling acpi_hotplug_schedule() which would be
responsible for queuing work (on CPU0) to detach+unplug the CPU.

Once the OS level detach succeeds, the worker evaluates the "_EJ0" method
which would do the actual CPU_EJECT_METHOD work.

If the detach fails then it evaluates the CPU_OST_METHOD which updates
the status for the event and the status.

At this point the state is back to:

(is_removing=0, fw_remove=0)
if OSPM fails to release CPU for whatever reasons, it's valid
state, we just notify user using OST event that requested unplug wasn't 

#3 (is_removing=1, fw_remove=1) -- guest OS removed all references from
                                     the CPU, firmware is permitted /
                                     required to forget about the CPU as
                                     well, and then unplug the CPU

CPU_EJECT_METHOD will do a store to bit 4, which would invert (and
thus set) fw_remove and then do the SMI.

So, this would be
#3 (is_removing=0, fw_remove=1)

At this point the firmware calls QemuCPUhpCollectApicIds() which
(after changes) notices CPU(s) with fw_remove set.

Collects them and does a store to bit 4, which would clear fw_remove.

I don't think we should clear fw_remove as soon as we collect the CPU,
in the firmware. Status flag modifications should be kept out of

I'd skip this step on firmware side and make QEMU clear it
when CPU is ejected.

Makes sense.

#4 (is_removing=1, fw_remove=0) -- fimware is about to unplug the CPU

#5 (is_removing=0, fw_remove=0) -- firmware performing unplug
Firmware does an unplug and writes to bit 3, thus clearing is_removing.

On return from the firmware the guest evaluates the COST again.
it's optional and depends on OSPM implementation (some do not call it on 

And, eventually goes back to the CSCN where it processes more
hotplug or unplug events.
CSCN in case of unplug finishes first, and only after that EJ0 calls
are processed

The variation (is_removing=0, fw_remove=1) is invalid / unused.


The firmware may be investigating the CPU register block between steps
#2 and #3 -- in other words, the firmware may see a CPU for which
is_remove is set (unplug requested via QMP), but the OS has not vacated
yet (fw_remove=0). In that case, the firmware must just skip the CPU --
once the OS is done, it will set fw_remove too, and raise another SMI.
Yeah, it makes sense for the firmware to only care about a CPU once it
sees fw_remove=1. (And as currently situated, the firmware would never
see is_removing=1 at all.)

The firmware may well see is_removing=1, as the QMP device_del command
may set that bit on some other CPU, asynchronously to the firmware's
execution. The firmware needs to recognize if the "get pending" command
returns a CPU because of this, and just continue scanning.
Right. Processing (for unplug) any pending CPUs which are not OSPM sanctioned
(so do not have fw_remove set) could be catastrophic for the guest and so
the unplug path should ignore them.

It is possible that there are CPUs with bits for both is_inserting and
is_removing. In that case QemuCpuhpCollectApicIds() would put them in the
PluggedApicIds array and the unplug eventually happens in the next
firmware invocation.

If a CPU has both is_inserting and fw_remove set, the firmware processes the
hotplug in that invocation and the unplug happens whenever the OSPM triggers
the firmware next.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]