On 11/26/20 21:38, Igor Mammedov wrote:
> On Thu, 26 Nov 2020 12:17:27 +0100
> Laszlo Ersek <ler...@redhat.com> wrote:
>
>> On 11/24/20 13:25, Igor Mammedov wrote:
>>> 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)
That should work fine too, as long as it's clearly documented.
>>> @@ -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.
Ah indeed. Yes, making Notify pending at first, and then delivering it
inside the kernel only after the current AML call stack returns -- that
seems to make sense. Otherwise we could get unbounded recursion (the
notify handler calls another AML method, which could contain another
notify ...)
>> 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.
OK. Hot-unplug is not a frequent operation.
>
>> (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.
Makes sense, especially in combination with the idea that clearing the
fw_remove bit should clear is_removing too.
The firmware logic needs to be aware of is_removing though, at least
understand the existence of this bit, as the "get pending" command will
report such CPUs too that only have is_removing set. Shouldn't be a
problem, we just have to recognize it.
[...]
Thanks!
Laszlo
