RE: [PATCH RFC V2 19/37] hw/acpi: ACPI/AML Changes to reflect the correct _STA.{PRES,ENA} Bits to Guest

[Salil Mehta]

Hi Gavin,

> From: Gavin Shan <gshan@redhat.com>
> Sent: Friday, September 29, 2023 12:34 AM
> To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org; 
> qemu-arm@nongnu.org
> Cc: maz@kernel.org; jean-philippe@linaro.org; Jonathan Cameron
> <jonathan.cameron@huawei.com>; lpieralisi@kernel.org;
> peter.maydell@linaro.org; richard.henderson@linaro.org;
> imammedo@redhat.com; andrew.jones@linux.dev; david@redhat.com;
> philmd@linaro.org; eric.auger@redhat.com; will@kernel.org; ardb@kernel.org;
> oliver.upton@linux.dev; pbonzini@redhat.com; mst@redhat.com;
> rafael@kernel.org; borntraeger@linux.ibm.com; alex.bennee@linaro.org;
> linux@armlinux.org.uk; darren@os.amperecomputing.com;
> ilkka@os.amperecomputing.com; vishnu@os.amperecomputing.com;
> karl.heubaum@oracle.com; miguel.luis@oracle.com; salil.mehta@opnsrc.net;
> zhukeqian <zhukeqian1@huawei.com>; wangxiongfeng (C)
> <wangxiongfeng2@huawei.com>; wangyanan (Y) <wangyanan55@huawei.com>;
> jiakernel2@gmail.com; maobibo@loongson.cn; lixianglai@loongson.cn
> Subject: Re: [PATCH RFC V2 19/37] hw/acpi: ACPI/AML Changes to reflect the
> correct _STA.{PRES,ENA} Bits to Guest
> 
> Hi Salil,
> 
> On 9/26/23 20:04, Salil Mehta wrote:
> > ACPI AML changes to properly reflect the _STA.PRES and _STA.ENA Bits to
> the
> > guest during initialzation, when CPUs are hotplugged and after CPUs are
> > hot-unplugged.
> >
> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> > ---
> >   hw/acpi/cpu.c                  | 49 +++++++++++++++++++++++++++++++---
> >   hw/acpi/generic_event_device.c | 11 ++++++++
> >   include/hw/acpi/cpu.h          |  2 ++
> >   3 files changed, 58 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> > index 232720992d..e1299696d3 100644
> > --- a/hw/acpi/cpu.c
> > +++ b/hw/acpi/cpu.c
> > @@ -63,10 +63,11 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr
> addr, unsigned size)
> >       cdev = &cpu_st->devs[cpu_st->selector];
> >       switch (addr) {
> >       case ACPI_CPU_FLAGS_OFFSET_RW: /* pack and return is_* fields */
> > -        val |= cdev->cpu ? 1 : 0;
> > +        val |= cdev->is_enabled ? 1 : 0;
> >           val |= cdev->is_inserting ? 2 : 0;
> >           val |= cdev->is_removing  ? 4 : 0;
> >           val |= cdev->fw_remove  ? 16 : 0;
> > +        val |= cdev->is_present ? 32 : 0;
> >           trace_cpuhp_acpi_read_flags(cpu_st->selector, val);
> >           break;
> 
> The vCPU states are synchronized to what we had. It means we're maintaining 
> two set
> vCPU states, one for board level and another set for vCPU hotplug here. They 
> look
> duplicate to each other. However, it will need too much code changes to 
> combine
> them.

We need to distinguish between ACPI CPU Hotplug states and QOM CPU states.
ACPI exposes the QOM CPU state to Guest OS. Till now ACPI CPU Hotplug
states and QOM CPU states were consistent to each other. ACPI inferred
the presence of QOM CPUState object as _STA.PRESENT=1 and _STA.ENABLED=1.

But with the ARM CPU hot(un)plug changes, this assumption is no longer true. 
QOM CPUState object might still be present or NULL but ACPI CPU Hotplug
State will always expose un-plugged CPU i.e. with NULL CPUState object
as _STA.PRESENT=1 and _STA.ENABLED=0. This is a key change because of
which we are able to enable hot(un)plug mechanism on ARM.


> 
> >       case ACPI_CPU_CMD_DATA_OFFSET_RW:
> > @@ -228,7 +229,21 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object
> *owner,
> >           struct CPUState *cpu = CPU(id_list->cpus[i].cpu);
> >           if (qemu_present_cpu(cpu)) {
> >               state->devs[i].cpu = cpu;
> > +            state->devs[i].is_present = true;
> > +        } else {
> > +            if (qemu_persistent_cpu(cpu)) {
> > +                state->devs[i].is_present = true;
> > +            } else {
> > +                state->devs[i].is_present = false;
> > +            }
> >           }
> 
> state->devs[i].is_present = qemu_persistent_cpu(cpu);


Sure. Thanks for pointing. :)


> > +
> > +        if (qemu_enabled_cpu(cpu)) {
> > +            state->devs[i].is_enabled = true;
> > +        } else {
> > +            state->devs[i].is_enabled = false;
> > +        }
> > +
> 
> state->dev[i].is_enabled = qemu_enabled_cpu(cpu);


Sure. Yes.


Thanks
Salil.