RE: [PATCH V1 2/4] hw/acpi: Update ACPI CPU Status `is_{present, enabled}` during vCPU hot(un)plug

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

Hi Igor,

>  From: Igor Mammedov <imammedo@redhat.com>
>  Sent: Friday, October 18, 2024 3:18 PM
>  To: Salil Mehta <salil.mehta@huawei.com>
>  
>  On Mon, 14 Oct 2024 20:22:03 +0100
>  Salil Mehta <salil.mehta@huawei.com> wrote:
>  
>  > Update the `AcpiCpuStatus` for `is_enabled` and `is_present`
>  > accordingly when vCPUs are hot-plugged or hot-unplugged, taking into
>  > account the *persistence* of the vCPUs.
>  >
>  > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>  > ---
>  >  hw/acpi/cpu.c | 7 +++++++
>  >  1 file changed, 7 insertions(+)
>  >
>  > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index
>  > 083c4010c2..700aa855e9 100644
>  > --- a/hw/acpi/cpu.c
>  > +++ b/hw/acpi/cpu.c
>  > @@ -291,6 +291,8 @@ void acpi_cpu_plug_cb(HotplugHandler
>  *hotplug_dev,
>  >      }
>  >
>  >      cdev->cpu = CPU(dev);
>  > +    cdev->is_present = true;
>  > +    cdev->is_enabled = true;
>  
>  hmm, if cpu is always present, then these fields are redundant since
>    (!cdev->cpu) == present
>  and
>    then is_enabled could be fetched from cdev->cpu directly


I believe you are assuming, that all the QOM possible vCPUs objects will
exists (and hence realized) for the entire life time of the VM? If yes, then
we need to first debate on that as we cannot let presence of all the possible
vCPUs affect  the boot time. This is one of the key requirement from this
feature.


>  
>  >      if (dev->hotplugged) {
>  >          cdev->is_inserting = true;
>  >          acpi_send_event(DEVICE(hotplug_dev),
>  > ACPI_CPU_HOTPLUG_STATUS); @@ -322,6 +324,11 @@ void
>  acpi_cpu_unplug_cb(CPUHotplugState *cpu_st,
>  >          return;
>  >      }
>  >
>  > +    cdev->is_enabled = false;
>  > +    if (!acpi_persistent_cpu(CPU(dev))) {
>  > +        cdev->is_present = false;
>  > +    }
>  
>  and other way around works as well.
>  
>  then we don't have to carry around extra state and making sure that it's in
>  sync/migrated.
>  
>  > +
>  >      cdev->cpu = NULL;
>  >  }
>  >
>