RE: [PATCH V1 3/4] hw/acpi: Reflect ACPI vCPU {present,enabled} states in ACPI _STA.{PRES,ENA} Bits

[Salil Mehta via <qemu-devel@nongnu.org>]

Hi Igor,

>  From: Igor Mammedov <imammedo@redhat.com>
>  Sent: Friday, October 18, 2024 3:25 PM
>  To: Salil Mehta <salil.mehta@huawei.com>
>  Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; mst@redhat.com;
>  maz@kernel.org; jean-philippe@linaro.org; Jonathan Cameron
>  <jonathan.cameron@huawei.com>; lpieralisi@kernel.org;
>  peter.maydell@linaro.org; richard.henderson@linaro.org;
>  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; gshan@redhat.com;
>  rafael@kernel.org; borntraeger@linux.ibm.com; alex.bennee@linaro.org;
>  npiggin@gmail.com; harshpb@linux.ibm.com; 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; shahuang@redhat.com;
>  zhao1.liu@intel.com; Linuxarm <linuxarm@huawei.com>;
>  gustavo.romero@linaro.org
>  Subject: Re: [PATCH V1 3/4] hw/acpi: Reflect ACPI vCPU {present,enabled}
>  states in ACPI _STA.{PRES,ENA} Bits
>  
>  On Mon, 14 Oct 2024 20:22:04 +0100
>  Salil Mehta <salil.mehta@huawei.com> wrote:
>  
>  > Reflect the ACPI CPU hotplug `is_{present, enabled}` states in the
>  > `_STA.PRES`
>  > (presence) and `_STA.ENA` (enabled) bits when the guest kernel
>  > evaluates the ACPI `_STA` method during initialization, as well as
>  > when vCPUs are hot-plugged or hot-unplugged. The presence of
>  unplugged
>  > vCPUs may need to be deliberately
>  > *simulated* at the ACPI level to maintain a *persistent* view of vCPUs
>  > for the guest kernel.
>  
>  given questionable future of is_present/is_enabled fields, it probably
>  premature to review this part.


Sure, let us discuss the pain points. 


>  The only thing, I have to say here is repeating spec/doc update patch
>  describing how it should work should come 1st, so that we could compare
>  this impl. with it.


From the user perspective, there should not be any change in how he
plug/unplugs the ARM vCPUs. This is a requirement, as we have to
assists folks who wants to migrate from x86 world to ARM world
seamlessly (without affecting performance parameters) and this
feature is one small part of that bigger effort.


>  > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>  > ---
>  >  hw/acpi/cpu.c | 26 ++++++++++++++++++++++----
>  >  1 file changed, 22 insertions(+), 4 deletions(-)
>  >
>  > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index
>  > 700aa855e9..23ea2b9c70 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;
>  >      case ACPI_CPU_CMD_DATA_OFFSET_RW:
>  > @@ -376,6 +377,7 @@ const VMStateDescription vmstate_cpu_hotplug =
>  {
>  > #define CPU_REMOVE_EVENT  "CRMV"
>  >  #define CPU_EJECT_EVENT   "CEJ0"
>  >  #define CPU_FW_EJECT_EVENT "CEJF"
>  > +#define CPU_PRESENT       "CPRS"
>  >
>  >  void build_cpus_aml(Aml *table, MachineState *machine,
>  CPUHotplugFeatures opts,
>  >                      build_madt_cpu_fn build_madt_cpu, hwaddr
>  > base_addr, @@ -436,7 +438,9 @@ void build_cpus_aml(Aml *table,
>  MachineState *machine, CPUHotplugFeatures opts,
>  >          aml_append(field, aml_named_field(CPU_EJECT_EVENT, 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(3));
>  > +        /* 1 if present, read only */
>  > +        aml_append(field, aml_named_field(CPU_PRESENT, 1));
>  > +        aml_append(field, aml_reserved_field(2));
>  >          aml_append(field, aml_named_field(CPU_COMMAND, 8));
>  >          aml_append(cpu_ctrl_dev, field);
>  >
>  > @@ -466,6 +470,7 @@ void build_cpus_aml(Aml *table, MachineState
>  *machine, CPUHotplugFeatures opts,
>  >          Aml *ctrl_lock = aml_name("%s.%s", cphp_res_path, CPU_LOCK);
>  >          Aml *cpu_selector = aml_name("%s.%s", cphp_res_path,
>  CPU_SELECTOR);
>  >          Aml *is_enabled = aml_name("%s.%s", cphp_res_path,
>  > CPU_ENABLED);
>  > +        Aml *is_present = aml_name("%s.%s", cphp_res_path,
>  > + CPU_PRESENT);
>  >          Aml *cpu_cmd = aml_name("%s.%s", cphp_res_path,
>  CPU_COMMAND);
>  >          Aml *cpu_data = aml_name("%s.%s", cphp_res_path, CPU_DATA);
>  >          Aml *ins_evt = aml_name("%s.%s", cphp_res_path,
>  > CPU_INSERT_EVENT); @@ -494,13 +499,26 @@ void build_cpus_aml(Aml
>  *table, MachineState *machine, CPUHotplugFeatures opts,
>  >          {
>  >              Aml *idx = aml_arg(0);
>  >              Aml *sta = aml_local(0);
>  > +            Aml *ifctx2;
>  > +            Aml *else_ctx;
>  >
>  >              aml_append(method, aml_acquire(ctrl_lock, 0xFFFF));
>  >              aml_append(method, aml_store(idx, cpu_selector));
>  >              aml_append(method, aml_store(zero, sta));
>  > -            ifctx = aml_if(aml_equal(is_enabled, one));
>  > +            ifctx = aml_if(aml_equal(is_present, one));
>  >              {
>  > -                aml_append(ifctx, aml_store(aml_int(0xF), sta));
>  > +                ifctx2 = aml_if(aml_equal(is_enabled, one));
>  > +                {
>  > +                    /* cpu is present and enabled */
>  > +                    aml_append(ifctx2, aml_store(aml_int(0xF), sta));
>  > +                }
>  > +                aml_append(ifctx, ifctx2);
>  > +                else_ctx = aml_else();
>  > +                {
>  > +                    /* cpu is present but disabled */
>  > +                    aml_append(else_ctx, aml_store(aml_int(0xD), sta));
>  > +                }
>  > +                aml_append(ifctx, else_ctx);
>  >              }
>  >              aml_append(method, ifctx);
>  >              aml_append(method, aml_release(ctrl_lock));
>