RE: [PULL 60/65] hw/acpi: Update ACPI `_STA` method with QOM vCPU ACPI Hotplug states

[Salil Mehta]

HI Igor,

Thank for you replying back.

>  From: qemu-devel-bounces+salil.mehta=huawei.com@nongnu.org <qemu-
>  devel-bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of Igor
>  Mammedov
>  Sent: Wednesday, November 6, 2024 9:01 AM
>  To: Salil Mehta <salil.mehta@huawei.com>
>  
>  On Tue, 5 Nov 2024 21:12:00 +0000
>  Salil Mehta <salil.mehta@huawei.com> wrote:
>  
>  > >  From: Igor Mammedov <imammedo@redhat.com>
>  > >  Sent: Tuesday, November 5, 2024 12:50 PM
>  > >  To: Michael S. Tsirkin <mst@redhat.com>
>  > >  Cc: qemu-devel@nongnu.org; Peter Maydell
>  > > <peter.maydell@linaro.org>;  Salil Mehta <salil.mehta@huawei.com>;
>  > > Ani Sinha <anisinha@redhat.com>;  Eduardo Habkost
>  > > <eduardo@habkost.net>; Marcel Apfelbaum
>  > > <marcel.apfelbaum@gmail.com>; Philippe Mathieu-Daudé
>  > > <philmd@linaro.org>; wangyanan (Y) <wangyanan55@huawei.com>;
>  Zhao
>  > > Liu <zhao1.liu@intel.com>
>  > >  Subject: Re: [PULL 60/65] hw/acpi: Update ACPI `_STA` method with
>  > > QOM  vCPU ACPI Hotplug states
>  > >
>  > >  On Mon, 4 Nov 2024 16:09:26 -0500
>  > >  "Michael S. Tsirkin" <mst@redhat.com> wrote:
>  > >
>  > >  > From: Salil Mehta <salil.mehta@huawei.com>  >  > Reflect the QOM
>  > > vCPUs ACPI CPU hotplug states in the `_STA.Present`  > and and
>  > > `_STA.Enabled` bits when the guest kernel evaluates the ACPI  >
>  > > `_STA` method during initialization, as well as when vCPUs are  >
>  > > hot-plugged or hot-unplugged. If the CPU is present then the its  >
>  > > `enabled` status can be fetched using architecture-specific code [1].
>  > >  >
>  > >  > Reference:
>  > >  > [1] Example implementation of architecture-specific hook to fetch
>  CPU
>  > >  >     `enabled status
>  > >  >     Link:
>  > >  > https://github.com/salil-
>  > >  mehta/qemu/commit/c0b416b11e5af6505e558866f0e
>  > >  > b6c9f3709173e
>  > >  >
>  > >  > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>  >
>  > > Message-Id: <20241103102419.202225-4-salil.mehta@huawei.com>
>  > >  > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>  >
>  > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>  > ---  >
>  > > include/hw/core/cpu.h |  1 +
>  > >  >  hw/acpi/cpu.c         | 38 ++++++++++++++++++++++++++++++++++--
>  --
>  > >  >  2 files changed, 35 insertions(+), 4 deletions(-)  >  > diff
>  > > --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index  >
>  > > e7de77dc6d..db8a6fbc6e 100644  > --- a/include/hw/core/cpu.h  > +++
>  > > b/include/hw/core/cpu.h  > @@ -159,6 +159,7 @@ struct CPUClass {
>  > >  >      void (*query_cpu_fast)(CPUState *cpu, CpuInfoFast *value);
>  > >  >      int64_t (*get_arch_id)(CPUState *cpu);
>  > >  >      bool (*cpu_persistent_status)(CPUState *cpu);
>  > >  > +    bool (*cpu_enabled_status)(CPUState *cpu);
>  > >  >      void (*set_pc)(CPUState *cpu, vaddr value);
>  > >  >      vaddr (*get_pc)(CPUState *cpu);
>  > >  >      int (*gdb_read_register)(CPUState *cpu, GByteArray *buf, int
>  > >  > reg); diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index  >
>  > > 9b03b4292e..23443f09a5 100644  > --- a/hw/acpi/cpu.c  > +++
>  > > b/hw/acpi/cpu.c  > @@ -50,6 +50,18 @@ void
>  > > acpi_cpu_ospm_status(CPUHotplugState
>  > >  *cpu_st, ACPIOSTInfoList ***list)
>  > >  >      }
>  > >  >  }
>  > >  >
>  > >  > +static bool check_cpu_enabled_status(DeviceState *dev) {
>  > >  > +    CPUClass *k = dev ? CPU_GET_CLASS(dev) : NULL;
>  > >  > +    CPUState *cpu = CPU(dev);
>  > >  > +
>  > >  > +    if (cpu && (!k->cpu_enabled_status || k-
>  >cpu_enabled_status(cpu)))
>  > >  {
>  > >  > +        return true;
>  > >  > +    }
>  > >  > +
>  > >  > +    return false;
>  > >  > +}
>  > >  > +
>  > >  >  static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr,
>  > > unsigned  > size)  {
>  > >  >      uint64_t val = 0;
>  > >  > @@ -63,10 +75,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 |= check_cpu_enabled_status(DEVICE(cdev->cpu)) ? 1 : 0;
>  > >  >          val |= cdev->is_inserting ? 2 : 0;
>  > >  >          val |= cdev->is_removing  ? 4 : 0;
>  > >  >          val |= cdev->fw_remove  ? 16 : 0;
>  > >  > +        val |= cdev->cpu ? 32 : 0;
>  > >  >          trace_cpuhp_acpi_read_flags(cpu_st->selector, val);
>  > >  >          break;
>  > >  >      case ACPI_CPU_CMD_DATA_OFFSET_RW:
>  > >  > @@ -349,6 +362,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, @@ -409,7 +423,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);
>  > >  >
>  > >  > @@ -439,6 +455,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); @@ -467,13 +484,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));
>  > >
>  > >  very likely this will break hotplug on after migration to older QEMU.
>  >
>  >
>  > The above are local variables and are not being migrated. These are
>  > not the same as the earlier comment you provided here. I've removed
>  > those `is_present,enabled` states to address your earlier concerns:
>  > https://lore.kernel.org/qemu-
>  devel/20241018163118.0ae01a84@imammedo.us
>  > ers.ipa.redhat.com/
>  >
>  > State-1: Possible State of ACPI _STA (without patches)
>  >
>  > _STA.Present = 0
>  > _STA.Enabled = 0
>  >
>  > _STA.Present = 1
>  > _STA.Enabled = 1
>  >
>  > State-2: Possible State of ACPI _STA (with patches)
>  >
>  > _STA.Present = 0
>  > _STA.Enabled = 0
>  >
>  > _STA.Present = 1
>  > _STA.Enabled = 1
>  >
>  > _STA.Present = 1
>  > _STA.Enabled = 0  [New return state which should not affect x86]
>  >
>  >
>  > State-1 is subset of State-2. If vCPU HP feature is off on the newer
>  > Qemu then, State-1 becomes proper subset of State-2.
>  > Later is also the state of the older Qemu?
>  
>  
>  here is AML change from the next patch:
>  
>  -                If ((\_SB.PCI0.PRES.CPEN == One))
>  -                {
>  -                    Local0 = 0x0F
>  +                If ((\_SB.PCI0.PRES.CPRS == One))
>  +                {
>  
>  'enable check' branch is not taken unless presence is set.
>  However on old qemu there is no presence bit in register block and it always
>  0, isn't it?


Thanks. I can see your point now. We can rearrange the check as follows:

                If ((\_SB.PCI0.PRES.CPEN == One))
                {
                      Local0 = 0x0F
                }
+              Else
+              {
+                    If ((\_SB.PCI0.PRES.CPRS == One))
+                    {
+                        Local0 = 0x0D
+                    }
+              }


If you are okay then I'll send above change as a Fix to  [PULL 60/65], [PULL 
61/65]
Please confirm.

Many thanks!


Best regards
Salil.


>  
>  If above is true, the new _STA running on old QEMU after migration will
>  always return 0 and thus break x86 cphp.
>  
>  
>  +                    If ((\_SB.PCI0.PRES.CPEN == One))
>  +                    {
>  +                        Local0 = 0x0F
>  +                    }
>  +                    Else
>  +                    {
>  +                        Local0 = 0x0D
>  +                    }
>                   }
>  
>  >
>  >
>  > >  >              {
>  > >  > -                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));
>  > >
>  >
>