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

[Igor Mammedov]

On Sun, 6 Dec 2020 22:31:50 -0800
Ankur Arora <ankur.a.arora@oracle.com> 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.
> > +            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.

indeed, I'll fix in in v2

> 
> @@ -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
> >   
> >   
>