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

[Laszlo Ersek]

On 11/26/20 21:38, Igor Mammedov wrote:
> On Thu, 26 Nov 2020 12:17:27 +0100
> Laszlo Ersek <lersek@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