Re: [PATCH 2/8] acpi: cpuhp: introduce 'firmware performs eject' status/control bits

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 7 Dec 2020 00:47:13 -0800
Ankur Arora <ankur.a.arora@oracle.com> wrote:

> On 2020-12-06 10:31 p.m., Ankur Arora wrote:
> > On 2020-12-04 9:09 a.m., Igor Mammedov wrote:  
> >> Adds bit #4 to status/control field of CPU hotplug MMIO interface.
> >> New bit will be used OSPM to mark CPUs as pending for removal by firmware,
> >> when it calls _EJ0 method on CPU device node. Later on, when firmware
> >> sees this bit set, it will perform CPU eject which will clear bit #4
> >> as well.
> >>
> >> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >> ---
> >> v1:
> >>    - rearrange status/control bits description (Laszlo)
> >>    - add clear bit #4 on eject
> >>    - drop toggling logic from bit #4, it can be only set by guest
> >>      and clear as part of cpu eject
> >>    - exclude boot CPU from remove request
> >>    - add trace events for new bit
> >> ---
> >>   include/hw/acpi/cpu.h           |  1 +
> >>   docs/specs/acpi_cpu_hotplug.txt | 19 ++++++++++++++-----
> >>   hw/acpi/cpu.c                   |  9 +++++++++
> >>   hw/acpi/trace-events            |  2 ++
> >>   4 files changed, 26 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> >> index 0eeedaa491..d71edde456 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;
> >> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> >> index 9bb22d1270..9bd59ae0da 100644
> >> --- a/docs/specs/acpi_cpu_hotplug.txt
> >> +++ b/docs/specs/acpi_cpu_hotplug.txt
> >> @@ -56,8 +56,11 @@ read access:
> >>                 no device check event to OSPM was issued.
> >>                 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
> >> +              no device eject request to OSPM was issued. Firmware must
> >> +              ignore this bit.
> >> +           3: reserved and should be ignored by OSPM
> >> +           4: if set to 1, OSPM requests firmware to perform 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:
> >> @@ -79,10 +82,16 @@ write access:
> >>                  selected CPU device
> >>               2: if set to 1 clears device remove event, set by OSPM
> >>                  after it has emitted device eject request for the
> >> -               selected CPU device
> >> +               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
> >> +               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.  
> 
> So I spent some time testing my OVMF series alongside this.
> Just sent out the code on the EDK2 list.
> 
> To do the eject, I'm setting bit#3 after queuing up the processor
> for removal (via RemoveProcessor()).
> 
> This means, however, that the unplug in QEMU would happen before the
> real firmware unplug (which'll happen in SmmCpuUpdate()).
> 
> Clearly, the right place for eject would be either in the appropriate
> APHandler() or in the BSPHandler() after all the APs have been waited
> for but from my reading of related code I don't see any infrastructure
> for doing this (admittedly, I don't know the EDK2 source well enough
> so it's likely I missed something.)
> 
> The other possibility might be to do it in the _EJ0 method itself
> after we return from the SMI:
> 
> @@ -479,9 +515,8 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>                   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_store(one, ej_evt));
>               aml_append(method, aml_release(ctrl_lock));
> 
> This seems to work but it is possible I'm missing something here.

theoretically this leaves unaccounted CPU on fw side,
what if SMM is entered again before CPU is ejected or OS doesn't eject it on purpose?

I'd prefer firmware to remove CPU before returning from SMM.


> 
> 
> Opinions?
> 
> Ankur
> 
> >> +            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) in case
> >> +               it's asked firmware to perform CPU device eject.
> >> +            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..811218f673 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;
> >>           trace_cpuhp_acpi_read_flags(cpu_st->selector, val);
> >>           break;
> >>       case ACPI_CPU_CMD_DATA_OFFSET_RW:
> >> @@ -148,6 +149,14 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, uint64_t data,
> >>               hotplug_ctrl = qdev_get_hotplug_handler(dev);
> >>               hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
> >>               object_unparent(OBJECT(dev));
> >> +            cdev->fw_remove = false;
> >> +        } else if (data & 16) {
> >> +            if (!cdev->cpu || cdev->cpu == first_cpu) {
> >> +                trace_cpuhp_acpi_fw_remove_invalid_cpu(cpu_st->selector);
> >> +                break;
> >> +            }
> >> +            trace_cpuhp_acpi_fw_remove_cpu(cpu_st->selector);
> >> +            cdev->fw_remove = true;
> >>           }
> >>           break;  
> > 
> > By the time the firmware gets the MMI, cdev->is_removing == 0. So we probably
> > need the cdev->fw_remove clause as well.
> > 
> > @@ -168,7 +193,7 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, uint64_t data,
> > 
> >                   do {
> >                       cdev = &cpu_st->devs[iter];
> > -                    if (cdev->is_inserting || cdev->is_removing) {
> > +                    if (cdev->is_inserting || cdev->is_removing || cdev->fw_remove) {
> >                           cpu_st->selector = iter;
> >                           trace_cpuhp_acpi_cpu_has_events(cpu_st->selector,
> >                               cdev->is_inserting, cdev->is_removing);
> > 
> > 
> > Ankur
> >   
> >>       case ACPI_CPU_CMD_OFFSET_WR:
> >> diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events
> >> index afbc77de1c..f91ced477d 100644
> >> --- a/hw/acpi/trace-events
> >> +++ b/hw/acpi/trace-events
> >> @@ -29,6 +29,8 @@ cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]"
> >>   cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]"
> >>   cpuhp_acpi_ejecting_invalid_cpu(uint32_t idx) "0x%"PRIx32
> >>   cpuhp_acpi_ejecting_cpu(uint32_t idx) "0x%"PRIx32
> >> +cpuhp_acpi_fw_remove_invalid_cpu(uint32_t idx) "0x%"PRIx32
> >> +cpuhp_acpi_fw_remove_cpu(uint32_t idx) "0x%"PRIx32
> >>   cpuhp_acpi_write_ost_ev(uint32_t slot, uint32_t ev) "idx[0x%"PRIx32"] OST EVENT: 0x%"PRIx32
> >>   cpuhp_acpi_write_ost_status(uint32_t slot, uint32_t st) "idx[0x%"PRIx32"] OST STATUS: 0x%"PRIx32
> >>  
>