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