Re: [PATCH 5/8] x86: acpi: let the firmware handle pending "CPU remove" events in SMM

[Ankur Arora <ankur.a.arora@oracle.com>]

On 2020-12-06 10:20 p.m., Ankur Arora wrote:
> On 2020-12-04 9:09 a.m., Igor Mammedov wrote:
>> if firmware and QEMU negotiated CPU hotunplug support, generate
>> _EJ0 method so that it will mark CPU for removal by firmware and
>> pass control to it by triggering SMI.
>>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> ---
>>   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 d71edde456..999caaf510 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 811218f673..bded2a837f 100644
>> --- a/hw/acpi/cpu.c
>> +++ b/hw/acpi/cpu.c
>> @@ -341,6 +341,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,
>> @@ -393,7 +394,10 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>>           aml_append(field, aml_named_field(CPU_REMOVE_EVENT, 1));
> 
>    Bit 2: Device remove event, used to distinguish device for which
>          no device eject request to OSPM was issued. Firmware must
>          ignore this bit.
> 
>>           /* initiates device eject, write only */
>>           aml_append(field, aml_named_field(CPU_EJECT_EVENT, 1));
> 
>    Bit 3: reserved and should be ignored by OSPM

In write-access mode:
Bit  3: if set to 1 initiates device eject, set by OSPM when it
        triggers CPU device removal and calls _EJ0 method or by firmware
        when bit #4 is set. In case bit #4 were set, it's cleared as
        part of device eject.

(and so should not be a reserved_field() below)

> 
>> -        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));
> 
> Shouldn't this be instead:
> 
>> -        aml_append(field, aml_reserved_field(4));
>> +        /* 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(3));
> 
>    Bit 4: if set to 1, OSPM requests firmware to perform device eject.
>    Bit 5-7: reserved and should be ignored by OSPM
> 
> Otherwise AFAICS CPU_FW_EJECT_EVENT would correspond to bit 5.
> 
> 
> Ankur
> 
>>           aml_append(field, aml_named_field(CPU_COMMAND, 8));
>>           aml_append(cpu_ctrl_dev, field);
>> @@ -428,6 +432,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")));
>> @@ -470,7 +475,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 9036e5594c..475e76f514 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");
>>