[PATCH 0/9] Add architecture agnostic code to support vCPU Hotplug

[PATCH 0/9] Add architecture agnostic code to support vCPU Hotplug

[Salil Mehta via]

Virtual CPU hotplug support is being added across various architectures [1][3].
This series adds various code bits common across all archtiectures:

1. vCPU creation and Parking code refactor [Patch 1]
2. Update ACPI GED framework to support vCPU Hotplug [Patch 4,6,7]
3. ACPI CPUs AML code change [Patch 5]
3. Helper functions to support unrealization of CPU objects [Patch 8]
4. Misc [Patch 2,3,9]


References:
[1] https://lore.kernel.org/qemu-devel/20230926100436.28284-2-salil.mehta@huawei.com/
[2] https://lore.kernel.org/all/20230913163823.7880-1-james.morse@arm.com/
[3] https://lore.kernel.org/qemu-devel/cover.1695697701.git.lixianglai@loongson.cn/

Jean-Philippe Brucker (1):
  target/arm/kvm: Write CPU state back to KVM on reset

Salil Mehta (8):
  accel/kvm: Extract common KVM vCPU {creation,parking} code
  hw/acpi: Move CPU ctrl-dev MMIO region len macro to common header file
  hw/acpi: Add ACPI CPU hotplug init stub
  hw/acpi: Init GED framework with cpu hotplug events
  hw/acpi: Update CPUs AML with cpu-(ctrl)dev change
  hw/acpi: Update GED _EVT method AML with cpu scan
  hw/acpi: Update ACPI GED framework to support vCPU Hotplug
  physmem,gdbstub: Add helper functions to help *unrealize* vCPU object

 accel/kvm/kvm-all.c                    | 61 ++++++++++++++++++++------
 gdbstub/gdbstub.c                      | 13 ++++++
 hw/acpi/acpi-cpu-hotplug-stub.c        |  6 +++
 hw/acpi/cpu.c                          | 25 +++++++----
 hw/acpi/generic_event_device.c         | 22 ++++++++++
 hw/i386/acpi-build.c                   |  2 +-
 include/exec/cpu-common.h              |  8 ++++
 include/exec/gdbstub.h                 |  1 +
 include/hw/acpi/cpu.h                  |  5 ++-
 include/hw/acpi/cpu_hotplug.h          |  4 ++
 include/hw/acpi/generic_event_device.h |  5 +++
 include/hw/core/cpu.h                  |  1 +
 include/sysemu/kvm.h                   |  2 +
 softmmu/physmem.c                      | 25 +++++++++++
 target/arm/kvm.c                       |  8 +++-
 15 files changed, 162 insertions(+), 26 deletions(-)

-- 
2.34.1

[PATCH 1/9] accel/kvm: Extract common KVM vCPU {creation, parking} code

[Salil Mehta via]

KVM vCPU creation is done once during the initialization of the VM when Qemu
threads are spawned. This is common to all the architectures.

Hot-unplug of vCPU results in destruction of the vCPU objects in QOM but
the KVM vCPU objects in the Host KVM are not destroyed and their representative
KVM vCPU objects/context in Qemu are parked.

Refactor common logic so that some APIs could be reused by vCPU Hotplug code.

Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
 accel/kvm/kvm-all.c  | 61 ++++++++++++++++++++++++++++++++++----------
 include/sysemu/kvm.h |  2 ++
 2 files changed, 49 insertions(+), 14 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index ff1578bb32..57889c5f6c 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -137,6 +137,7 @@ static QemuMutex kml_slots_lock;
 #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
 
 static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
+static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
 
 static inline void kvm_resample_fd_remove(int gsi)
 {
@@ -320,11 +321,51 @@ err:
     return ret;
 }
 
+void kvm_park_vcpu(CPUState *cpu)
+{
+    unsigned long vcpu_id = cpu->cpu_index;
+    struct KVMParkedVcpu *vcpu;
+
+    vcpu = g_malloc0(sizeof(*vcpu));
+    vcpu->vcpu_id = vcpu_id;
+    vcpu->kvm_fd = cpu->kvm_fd;
+    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
+}
+
+int kvm_create_vcpu(CPUState *cpu)
+{
+    unsigned long vcpu_id = cpu->cpu_index;
+    KVMState *s = kvm_state;
+    int ret;
+
+    DPRINTF("kvm_create_vcpu\n");
+
+    /* check if the KVM vCPU already exist but is parked */
+    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
+    if (ret > 0) {
+        goto found;
+    }
+
+    /* create a new KVM vCPU */
+    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
+    if (ret < 0) {
+        return ret;
+    }
+
+found:
+    cpu->vcpu_dirty = true;
+    cpu->kvm_fd = ret;
+    cpu->kvm_state = s;
+    cpu->dirty_pages = 0;
+    cpu->throttle_us_per_full = 0;
+
+    return 0;
+}
+
 static int do_kvm_destroy_vcpu(CPUState *cpu)
 {
     KVMState *s = kvm_state;
     long mmap_size;
-    struct KVMParkedVcpu *vcpu = NULL;
     int ret = 0;
 
     DPRINTF("kvm_destroy_vcpu\n");
@@ -353,10 +394,7 @@ static int do_kvm_destroy_vcpu(CPUState *cpu)
         }
     }
 
-    vcpu = g_malloc0(sizeof(*vcpu));
-    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
-    vcpu->kvm_fd = cpu->kvm_fd;
-    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
+    kvm_park_vcpu(cpu);
 err:
     return ret;
 }
@@ -384,7 +422,7 @@ static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
         }
     }
 
-    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
+    return -1;
 }
 
 int kvm_init_vcpu(CPUState *cpu, Error **errp)
@@ -395,19 +433,14 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
 
     trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
 
-    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
+    ret = kvm_create_vcpu(cpu);
     if (ret < 0) {
-        error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu failed (%lu)",
+        error_setg_errno(errp, -ret,
+                         "kvm_init_vcpu: kvm_create_vcpu failed (%lu)",
                          kvm_arch_vcpu_id(cpu));
         goto err;
     }
 
-    cpu->kvm_fd = ret;
-    cpu->kvm_state = s;
-    cpu->vcpu_dirty = true;
-    cpu->dirty_pages = 0;
-    cpu->throttle_us_per_full = 0;
-
     mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
     if (mmap_size < 0) {
         ret = mmap_size;
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index ee9025f8e9..17919567a8 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -464,6 +464,8 @@ void kvm_set_sigmask_len(KVMState *s, unsigned int sigmask_len);
 
 int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
                                        hwaddr *phys_addr);
+int kvm_create_vcpu(CPUState *cpu);
+void kvm_park_vcpu(CPUState *cpu);
 
 #endif /* NEED_CPU_H */
 
-- 
2.34.1

Re: [PATCH 1/9] accel/kvm: Extract common KVM vCPU {creation,parking} code

[Alex Bennée]

Salil Mehta <salil.mehta@huawei.com> writes:

> KVM vCPU creation is done once during the initialization of the VM when Qemu
> threads are spawned. This is common to all the architectures.
>
> Hot-unplug of vCPU results in destruction of the vCPU objects in QOM but
> the KVM vCPU objects in the Host KVM are not destroyed and their representative
> KVM vCPU objects/context in Qemu are parked.
>
> Refactor common logic so that some APIs could be reused by vCPU Hotplug code.
>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> ---
>  accel/kvm/kvm-all.c  | 61 ++++++++++++++++++++++++++++++++++----------
>  include/sysemu/kvm.h |  2 ++
>  2 files changed, 49 insertions(+), 14 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index ff1578bb32..57889c5f6c 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -137,6 +137,7 @@ static QemuMutex kml_slots_lock;
>  #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
>  
>  static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
> +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);

weird to have a forward static declaration here if you are declare in
the header later on.

>  
>  static inline void kvm_resample_fd_remove(int gsi)
>  {
> @@ -320,11 +321,51 @@ err:
>      return ret;
>  }
>  
> +void kvm_park_vcpu(CPUState *cpu)
> +{
> +    unsigned long vcpu_id = cpu->cpu_index;

cpu_index is a plain int in CPUState:

    int cpu_index;

but is defined as an unsigned long in KVMParkedVcpu:

    unsigned long vcpu_id;

I'm not sure if this is just a historical anomaly but I suspect we
should fixup the discrepancy first so all users of cpu_index use the
same size.

> +    struct KVMParkedVcpu *vcpu;
> +
> +    vcpu = g_malloc0(sizeof(*vcpu));
> +    vcpu->vcpu_id = vcpu_id;
> +    vcpu->kvm_fd = cpu->kvm_fd;
> +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
> +}
> +
> +int kvm_create_vcpu(CPUState *cpu)
> +{

I don't think you get anything other than -1 on failure so at this point
you might as well return a bool.

> +    unsigned long vcpu_id = cpu->cpu_index;

Is this used?

> +    KVMState *s = kvm_state;
> +    int ret;
> +
> +    DPRINTF("kvm_create_vcpu\n");

Please don't add new DPRINTFs - use tracepoints instead. Whether its
worth cleaning up up kvm-all first I leave up to you. 

> +    /* check if the KVM vCPU already exist but is parked */
> +    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
> +    if (ret > 0) {
> +        goto found;
> +    }
> +
> +    /* create a new KVM vCPU */
> +    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +found:
> +    cpu->vcpu_dirty = true;
> +    cpu->kvm_fd = ret;
> +    cpu->kvm_state = s;
> +    cpu->dirty_pages = 0;
> +    cpu->throttle_us_per_full = 0;
> +
> +    return 0;
> +}
> +

This is trivially nestable to avoid gotos:

  bool kvm_create_vcpu(CPUState *cpu)
  {
      unsigned long vcpu_id = cpu->cpu_index;
      KVMState *s = kvm_state;
      int ret;

      /* check if the KVM vCPU already exist but is parked */
      ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
      if (ret < 0) {
          /* not found, try to create a new KVM vCPU */
          ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
          if (ret < 0) {
              return false;
          }
      }

      cpu->vcpu_dirty = true;
      cpu->kvm_fd = ret;
      cpu->kvm_state = s;
      cpu->dirty_pages = 0;
      cpu->throttle_us_per_full = 0;

      return true;
  }

>  static int do_kvm_destroy_vcpu(CPUState *cpu)
>  {
>      KVMState *s = kvm_state;
>      long mmap_size;
> -    struct KVMParkedVcpu *vcpu = NULL;
>      int ret = 0;
>  
>      DPRINTF("kvm_destroy_vcpu\n");
> @@ -353,10 +394,7 @@ static int do_kvm_destroy_vcpu(CPUState *cpu)
>          }
>      }
>  
> -    vcpu = g_malloc0(sizeof(*vcpu));
> -    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
> -    vcpu->kvm_fd = cpu->kvm_fd;
> -    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
> +    kvm_park_vcpu(cpu);
>  err:
>      return ret;
>  }
> @@ -384,7 +422,7 @@ static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
>          }
>      }
>  
> -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
> +    return -1;
>  }
>  
>  int kvm_init_vcpu(CPUState *cpu, Error **errp)
> @@ -395,19 +433,14 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)

Hmm it looks like no one cares about the return value here and the fact
that callers use &error_fatal should be enough to exit. You can then
just return early and avoid the error ladder.

>  
>      trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  
> -    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
> +    ret = kvm_create_vcpu(cpu);
>      if (ret < 0) {
> -        error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu failed (%lu)",
> +        error_setg_errno(errp, -ret,
> +                         "kvm_init_vcpu: kvm_create_vcpu failed (%lu)",
>                           kvm_arch_vcpu_id(cpu));
>          goto err;
>      }
>  
> -    cpu->kvm_fd = ret;
> -    cpu->kvm_state = s;
> -    cpu->vcpu_dirty = true;
> -    cpu->dirty_pages = 0;
> -    cpu->throttle_us_per_full = 0;
> -
>      mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
>      if (mmap_size < 0) {
>          ret = mmap_size;
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index ee9025f8e9..17919567a8 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -464,6 +464,8 @@ void kvm_set_sigmask_len(KVMState *s, unsigned int sigmask_len);
>  
>  int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
>                                         hwaddr *phys_addr);
> +int kvm_create_vcpu(CPUState *cpu);
> +void kvm_park_vcpu(CPUState *cpu);

Please add kdoc comments for the public API functions describing their
usage and parameters.

>  
>  #endif /* NEED_CPU_H */


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

RE: [PATCH 1/9] accel/kvm: Extract common KVM vCPU {creation, parking} code

[Salil Mehta via]

Hi Alex,
Thanks for taking pains to review it.

> From: Alex Bennée <alex.bennee@linaro.org>
> Sent: Friday, September 29, 2023 2:32 PM
> To: Salil Mehta <salil.mehta@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; 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;
> oliver.upton@linux.dev; pbonzini@redhat.com; mst@redhat.com;
> will@kernel.org; gshan@redhat.com; rafael@kernel.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; Linuxarm
> <linuxarm@huawei.com>
> Subject: Re: [PATCH 1/9] accel/kvm: Extract common KVM vCPU
> {creation,parking} code
> 
> 
> Salil Mehta <salil.mehta@huawei.com> writes:
> 
> > KVM vCPU creation is done once during the initialization of the VM when Qemu
> > threads are spawned. This is common to all the architectures.
> >
> > Hot-unplug of vCPU results in destruction of the vCPU objects in QOM but
> > the KVM vCPU objects in the Host KVM are not destroyed and their representative
> > KVM vCPU objects/context in Qemu are parked.
> >
> > Refactor common logic so that some APIs could be reused by vCPU Hotplug code.
> >
> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> > ---
> >  accel/kvm/kvm-all.c  | 61 ++++++++++++++++++++++++++++++++++----------
> >  include/sysemu/kvm.h |  2 ++
> >  2 files changed, 49 insertions(+), 14 deletions(-)
> >
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index ff1578bb32..57889c5f6c 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -137,6 +137,7 @@ static QemuMutex kml_slots_lock;
> >  #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
> >
> >  static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
> > +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
> 
> weird to have a forward static declaration here if you are declare in
> the header later on.

Yes, it is awkward. Actually, I had to move a function to get
a diff of changes which were more readable. Without moving
existing kvm_get_vcpu() function, change was not getting
highlighted properly in the generated patch. This relocation
of the function meant the order of declaration and its call
got reversed. So I had to use above tactics, even though weird.

Suggestions welcome to solve this?

> >  static inline void kvm_resample_fd_remove(int gsi)
> >  {
> > @@ -320,11 +321,51 @@ err:
> >      return ret;
> >  }
> >
> > +void kvm_park_vcpu(CPUState *cpu)
> > +{
> > +    unsigned long vcpu_id = cpu->cpu_index;
> 
> cpu_index is a plain int in CPUState:
> 
>     int cpu_index;
> 
> but is defined as an unsigned long in KVMParkedVcpu:
> 
>     unsigned long vcpu_id;
> 
> I'm not sure if this is just a historical anomaly but I suspect we
> should fixup the discrepancy first so all users of cpu_index use the
> same size.


Yes, agreed. But it is out of scope of this patch.


> > +    struct KVMParkedVcpu *vcpu;
> > +
> > +    vcpu = g_malloc0(sizeof(*vcpu));
> > +    vcpu->vcpu_id = vcpu_id;
> > +    vcpu->kvm_fd = cpu->kvm_fd;
> > +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
> > +}
> > +
> > +int kvm_create_vcpu(CPUState *cpu)
> > +{
> 
> I don't think you get anything other than -1 on failure so at this point
> you might as well return a bool.

kvm_vm_ioctl() can return other values? The 'ret' value can then be set 
using error_setg_errno() for example, in kvm_init_vcpu().

It is better to keep error handling standard for any future changes
as well.


> 
> > +    unsigned long vcpu_id = cpu->cpu_index;
> 
> Is this used?

It is used but we can get away with this. I think it
is a stray left over from shuffle.

Thanks!

> > +    KVMState *s = kvm_state;
> > +    int ret;
> > +
> > +    DPRINTF("kvm_create_vcpu\n");
> 
> Please don't add new DPRINTFs - use tracepoints instead. Whether its
> worth cleaning up up kvm-all first I leave up to you.

I can definitely change for this code.


> 
> > +    /* check if the KVM vCPU already exist but is parked */
> > +    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
> > +    if (ret > 0) {
> > +        goto found;
> > +    }
> > +
> > +    /* create a new KVM vCPU */
> > +    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> > +
> > +found:
> > +    cpu->vcpu_dirty = true;
> > +    cpu->kvm_fd = ret;
> > +    cpu->kvm_state = s;
> > +    cpu->dirty_pages = 0;
> > +    cpu->throttle_us_per_full = 0;
> > +
> > +    return 0;
> > +}
> > +
> 
> This is trivially nestable to avoid gotos:


No issues. I can remove gotos. I was trying to reduce
indentation.


> 
>   bool kvm_create_vcpu(CPUState *cpu)

It is better to return ioctl value so that error can be
set at the caller handling code.

>   {
>       unsigned long vcpu_id = cpu->cpu_index;
>       KVMState *s = kvm_state;
>       int ret;
> 
>       /* check if the KVM vCPU already exist but is parked */
>       ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
>       if (ret < 0) {
>           /* not found, try to create a new KVM vCPU */
>           ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
>           if (ret < 0) {
>               return false;
>           }
>       }

Yes, can be done this way.

> 
>       cpu->vcpu_dirty = true;
>       cpu->kvm_fd = ret;
>       cpu->kvm_state = s;
>       cpu->dirty_pages = 0;
>       cpu->throttle_us_per_full = 0;
> 
>       return true;

return value is better than Boolean.

>   }
> 
> >  static int do_kvm_destroy_vcpu(CPUState *cpu)
> >  {
> >      KVMState *s = kvm_state;
> >      long mmap_size;
> > -    struct KVMParkedVcpu *vcpu = NULL;
> >      int ret = 0;
> >
> >      DPRINTF("kvm_destroy_vcpu\n");
> > @@ -353,10 +394,7 @@ static int do_kvm_destroy_vcpu(CPUState *cpu)
> >          }
> >      }
> >
> > -    vcpu = g_malloc0(sizeof(*vcpu));
> > -    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
> > -    vcpu->kvm_fd = cpu->kvm_fd;
> > -    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
> > +    kvm_park_vcpu(cpu);
> >  err:
> >      return ret;
> >  }
> > @@ -384,7 +422,7 @@ static int kvm_get_vcpu(KVMState *s, unsigned long
> vcpu_id)
> >          }
> >      }
> >
> > -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
> > +    return -1;
> >  }
> >
> >  int kvm_init_vcpu(CPUState *cpu, Error **errp)
> > @@ -395,19 +433,14 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
> 
> Hmm it looks like no one cares about the return value here and the fact
> that callers use &error_fatal should be enough to exit. You can then
> just return early and avoid the error ladder.

Maybe yes, but is that change really required?

> 
> >
> >      trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
> >
> > -    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
> > +    ret = kvm_create_vcpu(cpu);
> >      if (ret < 0) {
> > -        error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu failed
> (%lu)",
> > +        error_setg_errno(errp, -ret,
> > +                         "kvm_init_vcpu: kvm_create_vcpu failed (%lu)",
> >                           kvm_arch_vcpu_id(cpu));
> >          goto err;
> >      }
> >
> > -    cpu->kvm_fd = ret;
> > -    cpu->kvm_state = s;
> > -    cpu->vcpu_dirty = true;
> > -    cpu->dirty_pages = 0;
> > -    cpu->throttle_us_per_full = 0;
> > -
> >      mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
> >      if (mmap_size < 0) {
> >          ret = mmap_size;
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index ee9025f8e9..17919567a8 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -464,6 +464,8 @@ void kvm_set_sigmask_len(KVMState *s, unsigned int
> sigmask_len);
> >
> >  int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
> >                                         hwaddr *phys_addr);
> > +int kvm_create_vcpu(CPUState *cpu);
> > +void kvm_park_vcpu(CPUState *cpu);
> 
> Please add kdoc comments for the public API functions describing their
> usage and parameters.

Sure.


Thanks
Salil.

Re: [PATCH 1/9] accel/kvm: Extract common KVM vCPU {creation,parking} code

[Alex Bennée]

Salil Mehta <salil.mehta@huawei.com> writes:

> Hi Alex,
> Thanks for taking pains to review it.
>
>> From: Alex Bennée <alex.bennee@linaro.org>
<snip>
>> Salil Mehta <salil.mehta@huawei.com> writes:
>> 
>> > KVM vCPU creation is done once during the initialization of the VM when Qemu
>> > threads are spawned. This is common to all the architectures.
>> >
>> > Hot-unplug of vCPU results in destruction of the vCPU objects in QOM but
>> > the KVM vCPU objects in the Host KVM are not destroyed and their representative
>> > KVM vCPU objects/context in Qemu are parked.
>> >
>> > Refactor common logic so that some APIs could be reused by vCPU Hotplug code.
>> >
>> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>> > ---
>> >  accel/kvm/kvm-all.c  | 61 ++++++++++++++++++++++++++++++++++----------
>> >  include/sysemu/kvm.h |  2 ++
>> >  2 files changed, 49 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> > index ff1578bb32..57889c5f6c 100644
>> > --- a/accel/kvm/kvm-all.c
>> > +++ b/accel/kvm/kvm-all.c
>> > @@ -137,6 +137,7 @@ static QemuMutex kml_slots_lock;
>> >  #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
>> >
>> >  static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
>> > +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
>> 
>> weird to have a forward static declaration here if you are declare in
>> the header later on.
>
> Yes, it is awkward. Actually, I had to move a function to get
> a diff of changes which were more readable. Without moving
> existing kvm_get_vcpu() function, change was not getting
> highlighted properly in the generated patch. This relocation
> of the function meant the order of declaration and its call
> got reversed. So I had to use above tactics, even though weird.
>
> Suggestions welcome to solve this?

You could have a code-motion commit ahead of this one to move things
into more useful places.

>
>> >  static inline void kvm_resample_fd_remove(int gsi)
>> >  {
>> > @@ -320,11 +321,51 @@ err:
>> >      return ret;
>> >  }
>> >
>> > +void kvm_park_vcpu(CPUState *cpu)
>> > +{
>> > +    unsigned long vcpu_id = cpu->cpu_index;
>> 
>> cpu_index is a plain int in CPUState:
>> 
>>     int cpu_index;
>> 
>> but is defined as an unsigned long in KVMParkedVcpu:
>> 
>>     unsigned long vcpu_id;
>> 
>> I'm not sure if this is just a historical anomaly but I suspect we
>> should fixup the discrepancy first so all users of cpu_index use the
>> same size.
>
>
> Yes, agreed. But it is out of scope of this patch.

Not so sure about that. Its adding another potential integer overflow
that would have to be cleared up later rather than fixing the
foundations before doing the refactor.

>> > +    struct KVMParkedVcpu *vcpu;
>> > +
>> > +    vcpu = g_malloc0(sizeof(*vcpu));
>> > +    vcpu->vcpu_id = vcpu_id;
>> > +    vcpu->kvm_fd = cpu->kvm_fd;
>> > +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
>> > +}
>> > +
>> > +int kvm_create_vcpu(CPUState *cpu)
>> > +{
>> 
>> I don't think you get anything other than -1 on failure so at this point
>> you might as well return a bool.
>
> kvm_vm_ioctl() can return other values? The 'ret' value can then be set 
> using error_setg_errno() for example, in kvm_init_vcpu().

Ahh yes - although:

    if (ret == -1) {
        ret = -errno;
    }

So I think you end up inverting again at the error reporting stage.

> It is better to keep error handling standard for any future changes
> as well.

Well the "proper" way is to pass errp down because the -1 from
kvm_get_cpu() isn't the same as -errno from the kvm_vm_ioctl() so
passing that to error_setg_errno() for the former is incorrect as its
just a lookup failure and -1 doesn't mean anything other than fail in
that context.

So yes returning int can be valid but often bool + errp makes for neater
code.

>> 
>> > +    unsigned long vcpu_id = cpu->cpu_index;
>> 
>> Is this used?
>
> It is used but we can get away with this. I think it
> is a stray left over from shuffle.
>
> Thanks!
>
>> > +    KVMState *s = kvm_state;
>> > +    int ret;
>> > +
>> > +    DPRINTF("kvm_create_vcpu\n");
>> 
>> Please don't add new DPRINTFs - use tracepoints instead. Whether its
>> worth cleaning up up kvm-all first I leave up to you.
>
> I can definitely change for this code.
>
>
>> 
>> > +    /* check if the KVM vCPU already exist but is parked */
>> > +    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
>> > +    if (ret > 0) {
>> > +        goto found;
>> > +    }
>> > +
>> > +    /* create a new KVM vCPU */
>> > +    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
>> > +    if (ret < 0) {
>> > +        return ret;
>> > +    }
>> > +
>> > +found:
>> > +    cpu->vcpu_dirty = true;
>> > +    cpu->kvm_fd = ret;
>> > +    cpu->kvm_state = s;
>> > +    cpu->dirty_pages = 0;
>> > +    cpu->throttle_us_per_full = 0;
>> > +
>> > +    return 0;
>> > +}
>> > +
>> 
>> This is trivially nestable to avoid gotos:
>
>
> No issues. I can remove gotos. I was trying to reduce
> indentation.

I think 2 levels is fair enough. We have a lot of goto's in the code for
error clean-up paths although we are slowly cleaning them up now we have
things like g_autoptr and friends. 

>> 
>>   bool kvm_create_vcpu(CPUState *cpu)
>
> It is better to return ioctl value so that error can be
> set at the caller handling code.

Or pass down errp.

>
>>   {
>>       unsigned long vcpu_id = cpu->cpu_index;
>>       KVMState *s = kvm_state;
>>       int ret;
>> 
>>       /* check if the KVM vCPU already exist but is parked */
>>       ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
>>       if (ret < 0) {
>>           /* not found, try to create a new KVM vCPU */
>>           ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
>>           if (ret < 0) {
>>               return false;
>>           }
>>       }
>
> Yes, can be done this way.
>
>> 
>>       cpu->vcpu_dirty = true;
>>       cpu->kvm_fd = ret;
>>       cpu->kvm_state = s;
>>       cpu->dirty_pages = 0;
>>       cpu->throttle_us_per_full = 0;
>> 
>>       return true;
>
> return value is better than Boolean.
>
>>   }
>> 
>> >  static int do_kvm_destroy_vcpu(CPUState *cpu)
>> >  {
>> >      KVMState *s = kvm_state;
>> >      long mmap_size;
>> > -    struct KVMParkedVcpu *vcpu = NULL;
>> >      int ret = 0;
>> >
>> >      DPRINTF("kvm_destroy_vcpu\n");
>> > @@ -353,10 +394,7 @@ static int do_kvm_destroy_vcpu(CPUState *cpu)
>> >          }
>> >      }
>> >
>> > -    vcpu = g_malloc0(sizeof(*vcpu));
>> > -    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>> > -    vcpu->kvm_fd = cpu->kvm_fd;
>> > -    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
>> > +    kvm_park_vcpu(cpu);
>> >  err:
>> >      return ret;
>> >  }
>> > @@ -384,7 +422,7 @@ static int kvm_get_vcpu(KVMState *s, unsigned long
>> vcpu_id)
>> >          }
>> >      }
>> >
>> > -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
>> > +    return -1;
>> >  }
>> >
>> >  int kvm_init_vcpu(CPUState *cpu, Error **errp)
>> > @@ -395,19 +433,14 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
>> 
>> Hmm it looks like no one cares about the return value here and the fact
>> that callers use &error_fatal should be enough to exit. You can then
>> just return early and avoid the error ladder.
>
> Maybe yes, but is that change really required?

Well it would simplify the goto jumps in the function that are only
there to ensure a value that is never looked at is generated. 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 2/9] hw/acpi: Move CPU ctrl-dev MMIO region len macro to common header file

[Salil Mehta via]

CPU ctrl-dev MMIO region length could be used in ACPI GED and various other
architecture specific places. Move ACPI_CPU_HOTPLUG_REG_LEN macro to more
appropriate common header file.

Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
 hw/acpi/cpu.c                 | 2 +-
 include/hw/acpi/cpu_hotplug.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 19c154d78f..45defdc0e2 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -1,12 +1,12 @@
 #include "qemu/osdep.h"
 #include "migration/vmstate.h"
 #include "hw/acpi/cpu.h"
+#include "hw/acpi/cpu_hotplug.h"
 #include "qapi/error.h"
 #include "qapi/qapi-events-acpi.h"
 #include "trace.h"
 #include "sysemu/numa.h"
 
-#define ACPI_CPU_HOTPLUG_REG_LEN 12
 #define ACPI_CPU_SELECTOR_OFFSET_WR 0
 #define ACPI_CPU_FLAGS_OFFSET_RW 4
 #define ACPI_CPU_CMD_OFFSET_WR 5
diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
index 3b932abbbb..48b291e45e 100644
--- a/include/hw/acpi/cpu_hotplug.h
+++ b/include/hw/acpi/cpu_hotplug.h
@@ -19,6 +19,8 @@
 #include "hw/hotplug.h"
 #include "hw/acpi/cpu.h"
 
+#define ACPI_CPU_HOTPLUG_REG_LEN 12
+
 typedef struct AcpiCpuHotplug {
     Object *device;
     MemoryRegion io;
-- 
2.34.1

Re: [PATCH 2/9] hw/acpi: Move CPU ctrl-dev MMIO region len macro to common header file

[Alex Bennée]

Salil Mehta <salil.mehta@huawei.com> writes:

> CPU ctrl-dev MMIO region length could be used in ACPI GED and various other
> architecture specific places. Move ACPI_CPU_HOTPLUG_REG_LEN macro to more
> appropriate common header file.
>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

RE: [PATCH 2/9] hw/acpi: Move CPU ctrl-dev MMIO region len macro to common header file

[Salil Mehta via]

> From: Alex Bennée <alex.bennee@linaro.org>
> Sent: Friday, September 29, 2023 3:03 PM
> To: Salil Mehta <salil.mehta@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; 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;
> oliver.upton@linux.dev; pbonzini@redhat.com; mst@redhat.com;
> will@kernel.org; gshan@redhat.com; rafael@kernel.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; Linuxarm
> <linuxarm@huawei.com>
> Subject: Re: [PATCH 2/9] hw/acpi: Move CPU ctrl-dev MMIO region len macro
> to common header file
> 
> 
> Salil Mehta <salil.mehta@huawei.com> writes:
> 
> > CPU ctrl-dev MMIO region length could be used in ACPI GED and various
> other
> > architecture specific places. Move ACPI_CPU_HOTPLUG_REG_LEN macro to more
> > appropriate common header file.
> >
> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Thanks
Salil.

[PATCH 3/9] hw/acpi: Add ACPI CPU hotplug init stub

[Salil Mehta via]

ACPI CPU hotplug related initialization should only happen if ACPI_CPU_HOTPLUG
support has been enabled for particular architecture. Add cpu_hotplug_hw_init()
stub to avoid compilation break.

Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
 hw/acpi/acpi-cpu-hotplug-stub.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c
index 3fc4b14c26..c6c61bb9cd 100644
--- a/hw/acpi/acpi-cpu-hotplug-stub.c
+++ b/hw/acpi/acpi-cpu-hotplug-stub.c
@@ -19,6 +19,12 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
     return;
 }
 
+void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
+                         CPUHotplugState *state, hwaddr base_addr)
+{
+    return;
+}
+
 void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList ***list)
 {
     return;
-- 
2.34.1

Re: [PATCH 3/9] hw/acpi: Add ACPI CPU hotplug init stub

[Alex Bennée]

Salil Mehta <salil.mehta@huawei.com> writes:

> ACPI CPU hotplug related initialization should only happen if ACPI_CPU_HOTPLUG
> support has been enabled for particular architecture. Add cpu_hotplug_hw_init()
> stub to avoid compilation break.

When does the compilation break? It's usually ok to include stubs with
that commit.

>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> ---
>  hw/acpi/acpi-cpu-hotplug-stub.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c
> index 3fc4b14c26..c6c61bb9cd 100644
> --- a/hw/acpi/acpi-cpu-hotplug-stub.c
> +++ b/hw/acpi/acpi-cpu-hotplug-stub.c
> @@ -19,6 +19,12 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
>      return;
>  }
>  
> +void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
> +                         CPUHotplugState *state, hwaddr base_addr)
> +{
> +    return;
> +}
> +
>  void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList ***list)
>  {
>      return;


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

RE: [PATCH 3/9] hw/acpi: Add ACPI CPU hotplug init stub

[Salil Mehta via]

> From: Alex Bennée <alex.bennee@linaro.org>
> Sent: Friday, September 29, 2023 3:27 PM
> To: Salil Mehta <salil.mehta@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; 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;
> oliver.upton@linux.dev; pbonzini@redhat.com; mst@redhat.com;
> will@kernel.org; gshan@redhat.com; rafael@kernel.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; Linuxarm
> <linuxarm@huawei.com>
> Subject: Re: [PATCH 3/9] hw/acpi: Add ACPI CPU hotplug init stub
> 
> 
> Salil Mehta <salil.mehta@huawei.com> writes:
> 
> > ACPI CPU hotplug related initialization should only happen if ACPI_CPU_HOTPLUG
> > support has been enabled for particular architecture. Add cpu_hotplug_hw_init()
> > stub to avoid compilation break.
> 
> When does the compilation break? It's usually ok to include stubs with
> that commit.


To be specific. it is not a compilation break but linking error.

Support of ACPI_CPU_HOTPLUG is optional. This flag is defined in architecture
specific Kconfig. Function cpu_hotplug_hw_init() is part of the hw/acpi/cpu.c
which gets compiled only when a particular architecture defines ACPI_CPU_HOTPLUG
flag. 

ACPI GED framework support for CPU Hotplug is not specific to any architecture.
acpi_ged_initfn() calls cpu_hotplug_hw_init() when GED device gets created.
This functions gets called irrespective of the fact CPU Hotplug is supported or
not. If ACPI_CPU_HOTPLUG is not enabled then presence of cpu_hotplug_hw_init()
will cause linking error without the presence of stub.


Thanks
Salil.

> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> > ---
> >  hw/acpi/acpi-cpu-hotplug-stub.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-
> stub.c
> > index 3fc4b14c26..c6c61bb9cd 100644
> > --- a/hw/acpi/acpi-cpu-hotplug-stub.c
> > +++ b/hw/acpi/acpi-cpu-hotplug-stub.c
> > @@ -19,6 +19,12 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion
> *parent, Object *owner,
> >      return;
> >  }
> >
> > +void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
> > +                         CPUHotplugState *state, hwaddr base_addr)
> > +{
> > +    return;
> > +}
> > +
> >  void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList
> ***list)
> >  {
> >      return;
> 
> 
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro

Re: [PATCH 3/9] hw/acpi: Add ACPI CPU hotplug init stub

[Philippe Mathieu-Daudé]

Hi Salil,

On 29/9/23 17:47, Salil Mehta wrote:
>> From: Alex Bennée <alex.bennee@linaro.org>
>> Sent: Friday, September 29, 2023 3:27 PM
>> To: Salil Mehta <salil.mehta@huawei.com>
>> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; 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;
>> oliver.upton@linux.dev; pbonzini@redhat.com; mst@redhat.com;
>> will@kernel.org; gshan@redhat.com; rafael@kernel.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; Linuxarm
>> <linuxarm@huawei.com>
>> Subject: Re: [PATCH 3/9] hw/acpi: Add ACPI CPU hotplug init stub
>>
>>
>> Salil Mehta <salil.mehta@huawei.com> writes:
>>
>>> ACPI CPU hotplug related initialization should only happen if ACPI_CPU_HOTPLUG
>>> support has been enabled for particular architecture. Add cpu_hotplug_hw_init()
>>> stub to avoid compilation break.
>>
>> When does the compilation break? It's usually ok to include stubs with
>> that commit.
> 
> 
> To be specific. it is not a compilation break but linking error.
> 
> Support of ACPI_CPU_HOTPLUG is optional. This flag is defined in architecture
> specific Kconfig. Function cpu_hotplug_hw_init() is part of the hw/acpi/cpu.c
> which gets compiled only when a particular architecture defines ACPI_CPU_HOTPLUG
> flag.
> 
> ACPI GED framework support for CPU Hotplug is not specific to any architecture.
> acpi_ged_initfn() calls cpu_hotplug_hw_init() when GED device gets created.
> This functions gets called irrespective of the fact CPU Hotplug is supported or
> not. If ACPI_CPU_HOTPLUG is not enabled then presence of cpu_hotplug_hw_init()
> will cause linking error without the presence of stub.

We can extract the common pattern in both
- ich9_pm_set_cpu_hotplug_legacy()
- piix4_set_cpu_hotplug_legacy()
to take (AcpiCpuHotplug, CPUHotplugState) params,
then guard the cpu_hotplug_hw_init() call within
CONFIG_ACPI_CPU_HOTPLUG ifdef'ry. Doing so we can
also remove the acpi_switch_to_modern_cphp() stub.

Regards,

Phil.

>>> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>>> ---
>>>   hw/acpi/acpi-cpu-hotplug-stub.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)

[PATCH 4/9] hw/acpi: Init GED framework with cpu hotplug events

[Salil Mehta via]

ACPI GED(as described in the ACPI 6.2 spec) can be used to generate ACPI events
when OSPM/guest receives an interrupt listed in the _CRS object of GED. OSPM
then maps or demultiplexes the event by evaluating _EVT method.

This change adds the support of cpu hotplug event initialization in the
existing GED framework.

Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
 hw/acpi/generic_event_device.c         | 8 ++++++++
 include/hw/acpi/generic_event_device.h | 5 +++++
 2 files changed, 13 insertions(+)

diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index a3d31631fe..d2fa1d0e4a 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -25,6 +25,7 @@ static const uint32_t ged_supported_events[] = {
     ACPI_GED_MEM_HOTPLUG_EVT,
     ACPI_GED_PWR_DOWN_EVT,
     ACPI_GED_NVDIMM_HOTPLUG_EVT,
+    ACPI_GED_CPU_HOTPLUG_EVT,
 };
 
 /*
@@ -400,6 +401,13 @@ static void acpi_ged_initfn(Object *obj)
     memory_region_init_io(&ged_st->regs, obj, &ged_regs_ops, ged_st,
                           TYPE_ACPI_GED "-regs", ACPI_GED_REG_COUNT);
     sysbus_init_mmio(sbd, &ged_st->regs);
+
+    s->cpuhp.device = OBJECT(s);
+    memory_region_init(&s->container_cpuhp, OBJECT(dev), "cpuhp container",
+                       ACPI_CPU_HOTPLUG_REG_LEN);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container_cpuhp);
+    cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
+                        &s->cpuhp_state, 0);
 }
 
 static void acpi_ged_class_init(ObjectClass *class, void *data)
diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
index d831bbd889..d0a5a43abf 100644
--- a/include/hw/acpi/generic_event_device.h
+++ b/include/hw/acpi/generic_event_device.h
@@ -60,6 +60,7 @@
 #define HW_ACPI_GENERIC_EVENT_DEVICE_H
 
 #include "hw/sysbus.h"
+#include "hw/acpi/cpu_hotplug.h"
 #include "hw/acpi/memory_hotplug.h"
 #include "hw/acpi/ghes.h"
 #include "qom/object.h"
@@ -97,6 +98,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
 #define ACPI_GED_MEM_HOTPLUG_EVT   0x1
 #define ACPI_GED_PWR_DOWN_EVT      0x2
 #define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
+#define ACPI_GED_CPU_HOTPLUG_EVT    0x8
 
 typedef struct GEDState {
     MemoryRegion evt;
@@ -108,6 +110,9 @@ struct AcpiGedState {
     SysBusDevice parent_obj;
     MemHotplugState memhp_state;
     MemoryRegion container_memhp;
+    CPUHotplugState cpuhp_state;
+    MemoryRegion container_cpuhp;
+    AcpiCpuHotplug cpuhp;
     GEDState ged_state;
     uint32_t ged_event_bitmap;
     qemu_irq irq;
-- 
2.34.1

Re: [PATCH 4/9] hw/acpi: Init GED framework with cpu hotplug events

[Jonathan Cameron via]

On Fri, 29 Sep 2023 13:42:59 +0100
Salil Mehta <salil.mehta@huawei.com> wrote:

> ACPI GED(as described in the ACPI 6.2 spec) can be used to generate ACPI events
> when OSPM/guest receives an interrupt listed in the _CRS object of GED. OSPM
> then maps or demultiplexes the event by evaluating _EVT method.
> 
> This change adds the support of cpu hotplug event initialization in the
> existing GED framework.
> 
> Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
FWIW this looks good to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  hw/acpi/generic_event_device.c         | 8 ++++++++
>  include/hw/acpi/generic_event_device.h | 5 +++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index a3d31631fe..d2fa1d0e4a 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -25,6 +25,7 @@ static const uint32_t ged_supported_events[] = {
>      ACPI_GED_MEM_HOTPLUG_EVT,
>      ACPI_GED_PWR_DOWN_EVT,
>      ACPI_GED_NVDIMM_HOTPLUG_EVT,
> +    ACPI_GED_CPU_HOTPLUG_EVT,
>  };
>  
>  /*
> @@ -400,6 +401,13 @@ static void acpi_ged_initfn(Object *obj)
>      memory_region_init_io(&ged_st->regs, obj, &ged_regs_ops, ged_st,
>                            TYPE_ACPI_GED "-regs", ACPI_GED_REG_COUNT);
>      sysbus_init_mmio(sbd, &ged_st->regs);
> +
> +    s->cpuhp.device = OBJECT(s);
> +    memory_region_init(&s->container_cpuhp, OBJECT(dev), "cpuhp container",
> +                       ACPI_CPU_HOTPLUG_REG_LEN);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container_cpuhp);
> +    cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
> +                        &s->cpuhp_state, 0);
>  }
>  
>  static void acpi_ged_class_init(ObjectClass *class, void *data)
> diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> index d831bbd889..d0a5a43abf 100644
> --- a/include/hw/acpi/generic_event_device.h
> +++ b/include/hw/acpi/generic_event_device.h
> @@ -60,6 +60,7 @@
>  #define HW_ACPI_GENERIC_EVENT_DEVICE_H
>  
>  #include "hw/sysbus.h"
> +#include "hw/acpi/cpu_hotplug.h"
>  #include "hw/acpi/memory_hotplug.h"
>  #include "hw/acpi/ghes.h"
>  #include "qom/object.h"
> @@ -97,6 +98,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
>  #define ACPI_GED_MEM_HOTPLUG_EVT   0x1
>  #define ACPI_GED_PWR_DOWN_EVT      0x2
>  #define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
> +#define ACPI_GED_CPU_HOTPLUG_EVT    0x8
>  
>  typedef struct GEDState {
>      MemoryRegion evt;
> @@ -108,6 +110,9 @@ struct AcpiGedState {
>      SysBusDevice parent_obj;
>      MemHotplugState memhp_state;
>      MemoryRegion container_memhp;
> +    CPUHotplugState cpuhp_state;
> +    MemoryRegion container_cpuhp;
> +    AcpiCpuHotplug cpuhp;
>      GEDState ged_state;
>      uint32_t ged_event_bitmap;
>      qemu_irq irq;

Re: [PATCH 4/9] hw/acpi: Init GED framework with cpu hotplug events

[Jonathan Cameron]

On Fri, 29 Sep 2023 13:42:59 +0100
Salil Mehta <salil.mehta@huawei.com> wrote:

> ACPI GED(as described in the ACPI 6.2 spec) can be used to generate ACPI events
> when OSPM/guest receives an interrupt listed in the _CRS object of GED. OSPM
> then maps or demultiplexes the event by evaluating _EVT method.
> 
> This change adds the support of cpu hotplug event initialization in the
> existing GED framework.
> 
> Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
FWIW this looks good to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  hw/acpi/generic_event_device.c         | 8 ++++++++
>  include/hw/acpi/generic_event_device.h | 5 +++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index a3d31631fe..d2fa1d0e4a 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -25,6 +25,7 @@ static const uint32_t ged_supported_events[] = {
>      ACPI_GED_MEM_HOTPLUG_EVT,
>      ACPI_GED_PWR_DOWN_EVT,
>      ACPI_GED_NVDIMM_HOTPLUG_EVT,
> +    ACPI_GED_CPU_HOTPLUG_EVT,
>  };
>  
>  /*
> @@ -400,6 +401,13 @@ static void acpi_ged_initfn(Object *obj)
>      memory_region_init_io(&ged_st->regs, obj, &ged_regs_ops, ged_st,
>                            TYPE_ACPI_GED "-regs", ACPI_GED_REG_COUNT);
>      sysbus_init_mmio(sbd, &ged_st->regs);
> +
> +    s->cpuhp.device = OBJECT(s);
> +    memory_region_init(&s->container_cpuhp, OBJECT(dev), "cpuhp container",
> +                       ACPI_CPU_HOTPLUG_REG_LEN);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container_cpuhp);
> +    cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
> +                        &s->cpuhp_state, 0);
>  }
>  
>  static void acpi_ged_class_init(ObjectClass *class, void *data)
> diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> index d831bbd889..d0a5a43abf 100644
> --- a/include/hw/acpi/generic_event_device.h
> +++ b/include/hw/acpi/generic_event_device.h
> @@ -60,6 +60,7 @@
>  #define HW_ACPI_GENERIC_EVENT_DEVICE_H
>  
>  #include "hw/sysbus.h"
> +#include "hw/acpi/cpu_hotplug.h"
>  #include "hw/acpi/memory_hotplug.h"
>  #include "hw/acpi/ghes.h"
>  #include "qom/object.h"
> @@ -97,6 +98,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
>  #define ACPI_GED_MEM_HOTPLUG_EVT   0x1
>  #define ACPI_GED_PWR_DOWN_EVT      0x2
>  #define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
> +#define ACPI_GED_CPU_HOTPLUG_EVT    0x8
>  
>  typedef struct GEDState {
>      MemoryRegion evt;
> @@ -108,6 +110,9 @@ struct AcpiGedState {
>      SysBusDevice parent_obj;
>      MemHotplugState memhp_state;
>      MemoryRegion container_memhp;
> +    CPUHotplugState cpuhp_state;
> +    MemoryRegion container_cpuhp;
> +    AcpiCpuHotplug cpuhp;
>      GEDState ged_state;
>      uint32_t ged_event_bitmap;
>      qemu_irq irq;

RE: [PATCH 4/9] hw/acpi: Init GED framework with cpu hotplug events

[Salil Mehta via]

Hi Jonathan,
Thanks for looking at it.

> From: Jonathan Cameron <jonathan.cameron@huawei.com>
> Sent: Monday, October 2, 2023 5:06 PM
> To: Salil Mehta <salil.mehta@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; maz@kernel.org; jean-
> philippe@linaro.org; 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;
> oliver.upton@linux.dev; pbonzini@redhat.com; mst@redhat.com;
> will@kernel.org; gshan@redhat.com; rafael@kernel.org;
> 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; Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH 4/9] hw/acpi: Init GED framework with cpu hotplug
> events
> 
> On Fri, 29 Sep 2023 13:42:59 +0100
> Salil Mehta <salil.mehta@huawei.com> wrote:
> 
> > ACPI GED(as described in the ACPI 6.2 spec) can be used to generate ACPI
> events
> > when OSPM/guest receives an interrupt listed in the _CRS object of GED.
> OSPM
> > then maps or demultiplexes the event by evaluating _EVT method.
> >
> > This change adds the support of cpu hotplug event initialization in the
> > existing GED framework.
> >
> > Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
> > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> FWIW this looks good to me.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thank you!
Salil.

RE: [PATCH 4/9] hw/acpi: Init GED framework with cpu hotplug events

[Salil Mehta]

Hi Jonathan,
Thanks for looking at it.

> From: Jonathan Cameron <jonathan.cameron@huawei.com>
> Sent: Monday, October 2, 2023 5:06 PM
> To: Salil Mehta <salil.mehta@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; maz@kernel.org; jean-
> philippe@linaro.org; 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;
> oliver.upton@linux.dev; pbonzini@redhat.com; mst@redhat.com;
> will@kernel.org; gshan@redhat.com; rafael@kernel.org;
> 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; Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH 4/9] hw/acpi: Init GED framework with cpu hotplug
> events
> 
> On Fri, 29 Sep 2023 13:42:59 +0100
> Salil Mehta <salil.mehta@huawei.com> wrote:
> 
> > ACPI GED(as described in the ACPI 6.2 spec) can be used to generate ACPI
> events
> > when OSPM/guest receives an interrupt listed in the _CRS object of GED.
> OSPM
> > then maps or demultiplexes the event by evaluating _EVT method.
> >
> > This change adds the support of cpu hotplug event initialization in the
> > existing GED framework.
> >
> > Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
> > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> FWIW this looks good to me.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thank you!
Salil.

[PATCH 5/9] hw/acpi: Update CPUs AML with cpu-(ctrl)dev change

[Salil Mehta via]

CPUs Control device(\\_SB.PCI0) register interface for the x86 arch is based on
PCI and is IO port based and hence existing cpus AML code assumes _CRS objects
would evaluate to a system resource which describes IO Port address. But on ARM
arch CPUs control device(\\_SB.PRES) register interface is memory-mapped hence
_CRS object should evaluate to system resource which describes memory-mapped
base address.

This cpus AML code change updates the existing inerface of the build cpus AML
function to accept both IO/MEMORY type regions and update the _CRS object
correspondingly.

Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
 hw/acpi/cpu.c         | 23 ++++++++++++++++-------
 hw/i386/acpi-build.c  |  2 +-
 include/hw/acpi/cpu.h |  5 +++--
 3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 45defdc0e2..66a71660ec 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -338,9 +338,10 @@ const VMStateDescription vmstate_cpu_hotplug = {
 #define CPU_FW_EJECT_EVENT "CEJF"
 
 void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
-                    hwaddr io_base,
+                    hwaddr base_addr,
                     const char *res_root,
-                    const char *event_handler_method)
+                    const char *event_handler_method,
+                    AmlRegionSpace rs)
 {
     Aml *ifctx;
     Aml *field;
@@ -367,13 +368,19 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
         aml_append(cpu_ctrl_dev, aml_mutex(CPU_LOCK, 0));
 
         crs = aml_resource_template();
-        aml_append(crs, aml_io(AML_DECODE16, io_base, io_base, 1,
+        if (rs == AML_SYSTEM_IO) {
+            aml_append(crs, aml_io(AML_DECODE16, base_addr, base_addr, 1,
                                ACPI_CPU_HOTPLUG_REG_LEN));
+        } else {
+            aml_append(crs, aml_memory32_fixed(base_addr,
+                               ACPI_CPU_HOTPLUG_REG_LEN, AML_READ_WRITE));
+        }
+
         aml_append(cpu_ctrl_dev, aml_name_decl("_CRS", crs));
 
         /* declare CPU hotplug MMIO region with related access fields */
         aml_append(cpu_ctrl_dev,
-            aml_operation_region("PRST", AML_SYSTEM_IO, aml_int(io_base),
+            aml_operation_region("PRST", rs, aml_int(base_addr),
                                  ACPI_CPU_HOTPLUG_REG_LEN));
 
         field = aml_field("PRST", AML_BYTE_ACC, AML_NOLOCK,
@@ -699,9 +706,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
     aml_append(sb_scope, cpus_dev);
     aml_append(table, sb_scope);
 
-    method = aml_method(event_handler_method, 0, AML_NOTSERIALIZED);
-    aml_append(method, aml_call0("\\_SB.CPUS." CPU_SCAN_METHOD));
-    aml_append(table, method);
+    if (event_handler_method) {
+        method = aml_method(event_handler_method, 0, AML_NOTSERIALIZED);
+        aml_append(method, aml_call0("\\_SB.CPUS." CPU_SCAN_METHOD));
+        aml_append(table, method);
+    }
 
     g_free(cphp_res_path);
 }
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4d2d40bab5..611d3d044d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1550,7 +1550,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
             .fw_unplugs_cpu = pm->smi_on_cpu_unplug,
         };
         build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
-                       "\\_SB.PCI0", "\\_GPE._E02");
+                       "\\_SB.PCI0", "\\_GPE._E02", AML_SYSTEM_IO);
     }
 
     if (pcms->memhp_io_base && nr_mem) {
diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
index 999caaf510..b87ebfdf4b 100644
--- a/include/hw/acpi/cpu.h
+++ b/include/hw/acpi/cpu.h
@@ -56,9 +56,10 @@ typedef struct CPUHotplugFeatures {
 } CPUHotplugFeatures;
 
 void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
-                    hwaddr io_base,
+                    hwaddr base_addr,
                     const char *res_root,
-                    const char *event_handler_method);
+                    const char *event_handler_method,
+                    AmlRegionSpace rs);
 
 void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList ***list);
 
-- 
2.34.1

[PATCH 6/9] hw/acpi: Update GED _EVT method AML with cpu scan

[Salil Mehta via]

OSPM evaluates _EVT method to map the event. The cpu hotplug event eventually
results in start of the cpu scan. Scan figures out the cpu and the kind of
event(plug/unplug) and notifies it back to the guest.

The change in this patch updates the GED AML _EVT method with the call to
\\_SB.CPUS.CSCN which will do above.

Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
 hw/acpi/generic_event_device.c | 4 ++++
 include/hw/acpi/cpu_hotplug.h  | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index d2fa1d0e4a..62d504d231 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -108,6 +108,10 @@ void build_ged_aml(Aml *table, const char *name, HotplugHandler *hotplug_dev,
                 aml_append(if_ctx, aml_call0(MEMORY_DEVICES_CONTAINER "."
                                              MEMORY_SLOT_SCAN_METHOD));
                 break;
+            case ACPI_GED_CPU_HOTPLUG_EVT:
+                aml_append(if_ctx, aml_call0(ACPI_CPU_CONTAINER "."
+                                             ACPI_CPU_SCAN_METHOD));
+                break;
             case ACPI_GED_PWR_DOWN_EVT:
                 aml_append(if_ctx,
                            aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
index 48b291e45e..ef631750b4 100644
--- a/include/hw/acpi/cpu_hotplug.h
+++ b/include/hw/acpi/cpu_hotplug.h
@@ -20,6 +20,8 @@
 #include "hw/acpi/cpu.h"
 
 #define ACPI_CPU_HOTPLUG_REG_LEN 12
+#define ACPI_CPU_SCAN_METHOD "CSCN"
+#define ACPI_CPU_CONTAINER "\\_SB.CPUS"
 
 typedef struct AcpiCpuHotplug {
     Object *device;
-- 
2.34.1

[PATCH 7/9] hw/acpi: Update ACPI GED framework to support vCPU Hotplug

[Salil Mehta via]

ACPI GED shall be used to convey to the guest kernel about any CPU hot-(un)plug
events. Therefore, existing ACPI GED framework inside QEMU needs to be enhanced
to support CPU hotplug state and events.

Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
 hw/acpi/generic_event_device.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 62d504d231..0d5f0140e5 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -12,6 +12,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "hw/acpi/acpi.h"
+#include "hw/acpi/cpu.h"
 #include "hw/acpi/generic_event_device.h"
 #include "hw/irq.h"
 #include "hw/mem/pc-dimm.h"
@@ -239,6 +240,8 @@ static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
         } else {
             acpi_memory_plug_cb(hotplug_dev, &s->memhp_state, dev, errp);
         }
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        acpi_cpu_plug_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
     } else {
         error_setg(errp, "virt: device plug request for unsupported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
@@ -253,6 +256,8 @@ static void acpi_ged_unplug_request_cb(HotplugHandler *hotplug_dev,
     if ((object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
                        !(object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)))) {
         acpi_memory_unplug_request_cb(hotplug_dev, &s->memhp_state, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        acpi_cpu_unplug_request_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
     } else {
         error_setg(errp, "acpi: device unplug request for unsupported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
@@ -266,6 +271,8 @@ static void acpi_ged_unplug_cb(HotplugHandler *hotplug_dev,
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         acpi_memory_unplug_cb(&s->memhp_state, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        acpi_cpu_unplug_cb(&s->cpuhp_state, dev, errp);
     } else {
         error_setg(errp, "acpi: device unplug for unsupported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
@@ -277,6 +284,7 @@ static void acpi_ged_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list)
     AcpiGedState *s = ACPI_GED(adev);
 
     acpi_memory_ospm_status(&s->memhp_state, list);
+    acpi_cpu_ospm_status(&s->cpuhp_state, list);
 }
 
 static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
@@ -291,6 +299,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
         sel = ACPI_GED_PWR_DOWN_EVT;
     } else if (ev & ACPI_NVDIMM_HOTPLUG_STATUS) {
         sel = ACPI_GED_NVDIMM_HOTPLUG_EVT;
+    } else if (ev & ACPI_CPU_HOTPLUG_STATUS) {
+        sel = ACPI_GED_CPU_HOTPLUG_EVT;
     } else {
         /* Unknown event. Return without generating interrupt. */
         warn_report("GED: Unsupported event %d. No irq injected", ev);
-- 
2.34.1

[PATCH 8/9] physmem, gdbstub: Add helper functions to help *unrealize* vCPU object

[Salil Mehta via]

vCPU Hot-unplug requires some helper functions to unroll what has happened
during realization of a CPU object.

Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
 gdbstub/gdbstub.c         | 13 +++++++++++++
 include/exec/cpu-common.h |  8 ++++++++
 include/exec/gdbstub.h    |  1 +
 include/hw/core/cpu.h     |  1 +
 softmmu/physmem.c         | 25 +++++++++++++++++++++++++
 5 files changed, 48 insertions(+)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 349d348c7b..a06ea3adad 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -491,6 +491,19 @@ void gdb_register_coprocessor(CPUState *cpu,
     }
 }
 
+void gdb_unregister_coprocessor_all(CPUState *cpu)
+{
+    GDBRegisterState *s, *p;
+
+    p = cpu->gdb_regs;
+    while (p) {
+        s = p;
+        p = p->next;
+        g_free(s);
+    }
+    cpu->gdb_regs = NULL;
+}
+
 static void gdb_process_breakpoint_remove_all(GDBProcess *p)
 {
     CPUState *cpu = gdb_get_first_cpu_in_process(p);
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 41788c0bdd..eb56a228a2 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -120,6 +120,14 @@ size_t qemu_ram_pagesize_largest(void);
  */
 void cpu_address_space_init(CPUState *cpu, int asidx,
                             const char *prefix, MemoryRegion *mr);
+/**
+ * cpu_address_space_destroy:
+ * @cpu: CPU for which address space needs to be destroyed
+ * @asidx: integer index of this address space
+ *
+ * Note that with KVM only one address space is supported.
+ */
+void cpu_address_space_destroy(CPUState *cpu, int asidx);
 
 void cpu_physical_memory_rw(hwaddr addr, void *buf,
                             hwaddr len, bool is_write);
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 16a139043f..986d8d2fa5 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -27,6 +27,7 @@ typedef int (*gdb_set_reg_cb)(CPUArchState *env, uint8_t *buf, int reg);
 void gdb_register_coprocessor(CPUState *cpu,
                               gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
                               int num_regs, const char *xml, int g_pos);
+void gdb_unregister_coprocessor_all(CPUState *cpu);
 
 /**
  * gdbserver_start: start the gdb server
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 648b5b3586..65d2ae4581 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -355,6 +355,7 @@ struct CPUState {
     QSIMPLEQ_HEAD(, qemu_work_item) work_list;
 
     CPUAddressSpace *cpu_ases;
+    int cpu_ases_count;
     int num_ases;
     AddressSpace *as;
     MemoryRegion *memory;
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 4f6ca653b3..4dfa0ca66f 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -761,6 +761,7 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
 
     if (!cpu->cpu_ases) {
         cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
+        cpu->cpu_ases_count = cpu->num_ases;
     }
 
     newas = &cpu->cpu_ases[asidx];
@@ -774,6 +775,30 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
     }
 }
 
+void cpu_address_space_destroy(CPUState *cpu, int asidx)
+{
+    CPUAddressSpace *cpuas;
+
+    assert(asidx < cpu->num_ases);
+    assert(asidx == 0 || !kvm_enabled());
+    assert(cpu->cpu_ases);
+
+    cpuas = &cpu->cpu_ases[asidx];
+    if (tcg_enabled()) {
+        memory_listener_unregister(&cpuas->tcg_as_listener);
+    }
+
+    address_space_destroy(cpuas->as);
+    g_free_rcu(cpuas->as, rcu);
+
+    if (cpu->cpu_ases_count == 1) {
+        g_free(cpu->cpu_ases);
+        cpu->cpu_ases = NULL;
+    }
+
+    cpu->cpu_ases_count--;
+}
+
 AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
 {
     /* Return the AddressSpace corresponding to the specified index */
-- 
2.34.1

Re: [PATCH 8/9] physmem,gdbstub: Add helper functions to help *unrealize* vCPU object

[Alex Bennée]

Salil Mehta <salil.mehta@huawei.com> writes:

> vCPU Hot-unplug requires some helper functions to unroll what has happened
> during realization of a CPU object.
>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> ---
>  gdbstub/gdbstub.c         | 13 +++++++++++++
>  include/exec/cpu-common.h |  8 ++++++++
>  include/exec/gdbstub.h    |  1 +
>  include/hw/core/cpu.h     |  1 +
>  softmmu/physmem.c         | 25 +++++++++++++++++++++++++
>  5 files changed, 48 insertions(+)
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 349d348c7b..a06ea3adad 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -491,6 +491,19 @@ void gdb_register_coprocessor(CPUState *cpu,
>      }
>  }
>  
> +void gdb_unregister_coprocessor_all(CPUState *cpu)
> +{
> +    GDBRegisterState *s, *p;
> +
> +    p = cpu->gdb_regs;
> +    while (p) {
> +        s = p;
> +        p = p->next;

Maybe add:

      /* s->xml is static const char so isn't freed */

> +        g_free(s);
> +    }
> +    cpu->gdb_regs = NULL;
> +}
> +
>  static void gdb_process_breakpoint_remove_all(GDBProcess *p)
>  {
>      CPUState *cpu = gdb_get_first_cpu_in_process(p);
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index 41788c0bdd..eb56a228a2 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -120,6 +120,14 @@ size_t qemu_ram_pagesize_largest(void);
>   */
>  void cpu_address_space_init(CPUState *cpu, int asidx,
>                              const char *prefix, MemoryRegion *mr);
> +/**
> + * cpu_address_space_destroy:
> + * @cpu: CPU for which address space needs to be destroyed
> + * @asidx: integer index of this address space
> + *
> + * Note that with KVM only one address space is supported.
> + */
> +void cpu_address_space_destroy(CPUState *cpu, int asidx);
>  
>  void cpu_physical_memory_rw(hwaddr addr, void *buf,
>                              hwaddr len, bool is_write);
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index 16a139043f..986d8d2fa5 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -27,6 +27,7 @@ typedef int (*gdb_set_reg_cb)(CPUArchState *env, uint8_t *buf, int reg);
>  void gdb_register_coprocessor(CPUState *cpu,
>                                gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
>                                int num_regs, const char *xml, int
>  g_pos);

Can we have a kdoc comment here describing the function.

I suspect this should be split into two patches, one for gdbstub and one
for physmem.

> +void gdb_unregister_coprocessor_all(CPUState *cpu);
>  
>  /**
>   * gdbserver_start: start the gdb server
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 648b5b3586..65d2ae4581 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -355,6 +355,7 @@ struct CPUState {
>      QSIMPLEQ_HEAD(, qemu_work_item) work_list;
>  
>      CPUAddressSpace *cpu_ases;
> +    int cpu_ases_count;
>      int num_ases;
>      AddressSpace *as;
>      MemoryRegion *memory;
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 4f6ca653b3..4dfa0ca66f 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -761,6 +761,7 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
>  
>      if (!cpu->cpu_ases) {
>          cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
> +        cpu->cpu_ases_count = cpu->num_ases;
>      }
>  
>      newas = &cpu->cpu_ases[asidx];
> @@ -774,6 +775,30 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
>      }
>  }
>  
> +void cpu_address_space_destroy(CPUState *cpu, int asidx)
> +{
> +    CPUAddressSpace *cpuas;
> +
> +    assert(asidx < cpu->num_ases);
> +    assert(asidx == 0 || !kvm_enabled());
> +    assert(cpu->cpu_ases);
> +
> +    cpuas = &cpu->cpu_ases[asidx];
> +    if (tcg_enabled()) {
> +        memory_listener_unregister(&cpuas->tcg_as_listener);
> +    }
> +
> +    address_space_destroy(cpuas->as);
> +    g_free_rcu(cpuas->as, rcu);
> +
> +    if (cpu->cpu_ases_count == 1) {
> +        g_free(cpu->cpu_ases);
> +        cpu->cpu_ases = NULL;
> +    }
> +
> +    cpu->cpu_ases_count--;
> +}
> +
>  AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
>  {
>      /* Return the AddressSpace corresponding to the specified index */


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

RE: [PATCH 8/9] physmem,gdbstub: Add helper functions to help *unrealize* vCPU object

[Salil Mehta via]

> From: Alex Bennée <alex.bennee@linaro.org>
> Sent: Friday, September 29, 2023 3:35 PM
> To: Salil Mehta <salil.mehta@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; 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;
> oliver.upton@linux.dev; pbonzini@redhat.com; mst@redhat.com;
> will@kernel.org; gshan@redhat.com; rafael@kernel.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; Linuxarm
> <linuxarm@huawei.com>
> Subject: Re: [PATCH 8/9] physmem,gdbstub: Add helper functions to help
> *unrealize* vCPU object
> 
> 
> Salil Mehta <salil.mehta@huawei.com> writes:
> 
> > vCPU Hot-unplug requires some helper functions to unroll what has happened
> > during realization of a CPU object.
> >
> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> > ---
> >  gdbstub/gdbstub.c         | 13 +++++++++++++
> >  include/exec/cpu-common.h |  8 ++++++++
> >  include/exec/gdbstub.h    |  1 +
> >  include/hw/core/cpu.h     |  1 +
> >  softmmu/physmem.c         | 25 +++++++++++++++++++++++++
> >  5 files changed, 48 insertions(+)
> >
> > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> > index 349d348c7b..a06ea3adad 100644
> > --- a/gdbstub/gdbstub.c
> > +++ b/gdbstub/gdbstub.c
> > @@ -491,6 +491,19 @@ void gdb_register_coprocessor(CPUState *cpu,
> >      }
> >  }
> >
> > +void gdb_unregister_coprocessor_all(CPUState *cpu)
> > +{
> > +    GDBRegisterState *s, *p;
> > +
> > +    p = cpu->gdb_regs;
> > +    while (p) {
> > +        s = p;
> > +        p = p->next;
> 
> Maybe add:
> 
>       /* s->xml is static const char so isn't freed */


Sure.

> > +        g_free(s);
> > +    }
> > +    cpu->gdb_regs = NULL;
> > +}
> > +
> >  static void gdb_process_breakpoint_remove_all(GDBProcess *p)
> >  {
> >      CPUState *cpu = gdb_get_first_cpu_in_process(p);
> > diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> > index 41788c0bdd..eb56a228a2 100644
> > --- a/include/exec/cpu-common.h
> > +++ b/include/exec/cpu-common.h
> > @@ -120,6 +120,14 @@ size_t qemu_ram_pagesize_largest(void);
> >   */
> >  void cpu_address_space_init(CPUState *cpu, int asidx,
> >                              const char *prefix, MemoryRegion *mr);
> > +/**
> > + * cpu_address_space_destroy:
> > + * @cpu: CPU for which address space needs to be destroyed
> > + * @asidx: integer index of this address space
> > + *
> > + * Note that with KVM only one address space is supported.
> > + */
> > +void cpu_address_space_destroy(CPUState *cpu, int asidx);
> >
> >  void cpu_physical_memory_rw(hwaddr addr, void *buf,
> >                              hwaddr len, bool is_write);
> > diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> > index 16a139043f..986d8d2fa5 100644
> > --- a/include/exec/gdbstub.h
> > +++ b/include/exec/gdbstub.h
> > @@ -27,6 +27,7 @@ typedef int (*gdb_set_reg_cb)(CPUArchState *env, uint8_t *buf, int reg);
> >  void gdb_register_coprocessor(CPUState *cpu,
> >                                gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
> >                                int num_regs, const char *xml, int 
> >  g_pos);
> 
> Can we have a kdoc comment here describing the function.

Ok.

> 
> I suspect this should be split into two patches, one for gdbstub and one
> for physmem.


No issues. Can do that.


> > +void gdb_unregister_coprocessor_all(CPUState *cpu);
> >
> >  /**
> >   * gdbserver_start: start the gdb server
> > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > index 648b5b3586..65d2ae4581 100644
> > --- a/include/hw/core/cpu.h
> > +++ b/include/hw/core/cpu.h
> > @@ -355,6 +355,7 @@ struct CPUState {
> >      QSIMPLEQ_HEAD(, qemu_work_item) work_list;
> >
> >      CPUAddressSpace *cpu_ases;
> > +    int cpu_ases_count;
> >      int num_ases;
> >      AddressSpace *as;
> >      MemoryRegion *memory;
> > diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> > index 4f6ca653b3..4dfa0ca66f 100644
> > --- a/softmmu/physmem.c
> > +++ b/softmmu/physmem.c
> > @@ -761,6 +761,7 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
> >
> >      if (!cpu->cpu_ases) {
> >          cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
> > +        cpu->cpu_ases_count = cpu->num_ases;
> >      }
> >
> >      newas = &cpu->cpu_ases[asidx];
> > @@ -774,6 +775,30 @@ void cpu_address_space_init(CPUState *cpu, int
> asidx,
> >      }
> >  }
> >
> > +void cpu_address_space_destroy(CPUState *cpu, int asidx)
> > +{
> > +    CPUAddressSpace *cpuas;
> > +
> > +    assert(asidx < cpu->num_ases);
> > +    assert(asidx == 0 || !kvm_enabled());
> > +    assert(cpu->cpu_ases);
> > +
> > +    cpuas = &cpu->cpu_ases[asidx];
> > +    if (tcg_enabled()) {
> > +        memory_listener_unregister(&cpuas->tcg_as_listener);
> > +    }
> > +
> > +    address_space_destroy(cpuas->as);
> > +    g_free_rcu(cpuas->as, rcu);
> > +
> > +    if (cpu->cpu_ases_count == 1) {
> > +        g_free(cpu->cpu_ases);
> > +        cpu->cpu_ases = NULL;
> > +    }
> > +
> > +    cpu->cpu_ases_count--;
> > +}
> > +
> >  AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
> >  {
> >      /* Return the AddressSpace corresponding to the specified index */
> 
> 
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro

[PATCH 9/9] target/arm/kvm: Write CPU state back to KVM on reset

[Salil Mehta via]

From: Jean-Philippe Brucker <jean-philippe@linaro.org>

When a KVM vCPU is reset following a PSCI CPU_ON call, its power state
is not synchronized with KVM at the moment. Because the vCPU is not
marked dirty, we miss the call to kvm_arch_put_registers() that writes
to KVM's MP_STATE. Force mp_state synchronization.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
 target/arm/kvm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index b66b936a95..8cb70b9e7c 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -642,11 +642,12 @@ void kvm_arm_cpu_post_load(ARMCPU *cpu)
 void kvm_arm_reset_vcpu(ARMCPU *cpu)
 {
     int ret;
+    CPUState *cs = CPU(cpu);
 
     /* Re-init VCPU so that all registers are set to
      * their respective reset values.
      */
-    ret = kvm_arm_vcpu_init(CPU(cpu));
+    ret = kvm_arm_vcpu_init(cs);
     if (ret < 0) {
         fprintf(stderr, "kvm_arm_vcpu_init failed: %s\n", strerror(-ret));
         abort();
@@ -663,6 +664,11 @@ void kvm_arm_reset_vcpu(ARMCPU *cpu)
      * for the same reason we do so in kvm_arch_get_registers().
      */
     write_list_to_cpustate(cpu);
+    /*
+     * Ensure we call kvm_arch_put_registers(). The vCPU isn't marked dirty if
+     * it was parked in KVM and is now booting from a PSCI CPU_ON call.
+     */
+    cs->vcpu_dirty = true;
 }
 
 /*
-- 
2.34.1

Re: [PATCH 9/9] target/arm/kvm: Write CPU state back to KVM on reset

[Alex Bennée]

Salil Mehta <salil.mehta@huawei.com> writes:

> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
>
> When a KVM vCPU is reset following a PSCI CPU_ON call, its power state
> is not synchronized with KVM at the moment. Because the vCPU is not
> marked dirty, we miss the call to kvm_arch_put_registers() that writes
> to KVM's MP_STATE. Force mp_state synchronization.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>

Seems reasonable:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

RE: [PATCH 9/9] target/arm/kvm: Write CPU state back to KVM on reset

[Salil Mehta via]

> From: Alex Bennée <alex.bennee@linaro.org>
> Sent: Friday, September 29, 2023 3:40 PM
> To: Salil Mehta <salil.mehta@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; 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;
> oliver.upton@linux.dev; pbonzini@redhat.com; mst@redhat.com;
> will@kernel.org; gshan@redhat.com; rafael@kernel.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; Linuxarm
> <linuxarm@huawei.com>
> Subject: Re: [PATCH 9/9] target/arm/kvm: Write CPU state back to KVM on
> reset
> 
> 
> Salil Mehta <salil.mehta@huawei.com> writes:
> 
> > From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> >
> > When a KVM vCPU is reset following a PSCI CPU_ON call, its power state
> > is not synchronized with KVM at the moment. Because the vCPU is not
> > marked dirty, we miss the call to kvm_arch_put_registers() that writes
> > to KVM's MP_STATE. Force mp_state synchronization.
> >
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> 
> Seems reasonable:
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Thanks
Salil.