[PATCH v3 0/3] target/ppc: vcpu hotplug failure handling fixes

[PATCH v3 0/3] target/ppc: vcpu hotplug failure handling fixes

[Harsh Prateek Bora]

On ppc64, the PowerVM hypervisor runs with limited memory and a VCPU
creation during hotplug may fail during kvm_ioctl for KVM_CREATE_VCPU,
leading to termination of guest since errp is set to &error_fatal while
calling kvm_init_vcpu. This unexpected behaviour can be avoided by
pre-creating and parking vcpu on success or return error otherwise.
This enables graceful error delivery for any vcpu hotplug failures while
the guest can keep running.

This series adds another helper to create and park vcpu (based on below
patch by Salil), exports cpu_get_free_index to be reused later and adds
ppc arch specfic handling for vcpu hotplug failure using kvm accel
helper cpu_target_realize.

Based on api refactoring to create/park vcpus introduced in 1/8 of patch series:
https://lore.kernel.org/qemu-devel/20240522211111.232114-1-salil.mehta@huawei.com/

Changelog:
v3: Addressed review comments from Nick
v2: Addressed review comments from Nick
v1: Initial patch

Harsh Prateek Bora (3):
  accel/kvm: Introduce kvm_create_and_park_vcpu() helper
  cpu-common.c: export cpu_get_free_index to be reused later
  target/ppc: handle vcpu hotplug failure gracefully

 accel/kvm/kvm-cpus.h      |  8 ++++++++
 include/exec/cpu-common.h |  2 ++
 accel/kvm/kvm-all.c       | 12 ++++++++++++
 cpu-common.c              |  7 ++++---
 target/ppc/kvm.c          | 41 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 67 insertions(+), 3 deletions(-)

-- 
2.39.3

[PATCH v3 1/3] accel/kvm: Introduce kvm_create_and_park_vcpu() helper

[Harsh Prateek Bora]

There are distinct helpers for creating and parking a KVM vCPU.
However, there can be cases where a platform needs to create and
immediately park the vCPU during early stages of vcpu init which
can later be reused when vcpu thread gets initialized. This would
help detect failures with kvm_create_vcpu at an early stage.

Based on api refactoring to create/park vcpus introduced in 1/8 of patch series:
https://lore.kernel.org/qemu-devel/20240522211111.232114-1-salil.mehta@huawei.com/

Suggested-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
---
 accel/kvm/kvm-cpus.h |  8 ++++++++
 accel/kvm/kvm-all.c  | 12 ++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/accel/kvm/kvm-cpus.h b/accel/kvm/kvm-cpus.h
index 2e6bb38b5d..00e534b3b9 100644
--- a/accel/kvm/kvm-cpus.h
+++ b/accel/kvm/kvm-cpus.h
@@ -46,4 +46,12 @@ void kvm_park_vcpu(CPUState *cpu);
  * @returns: KVM fd
  */
 int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id);
+
+/**
+ *  * kvm_create_and_park_vcpu - Create and park a KVM vCPU
+ *   * @cpu: QOM CPUState object for which KVM vCPU has to be created and parked.
+ *    *
+ *     * @returns: 0 when success, errno (<0) when failed.
+ *      */
+int kvm_create_and_park_vcpu(CPUState *cpu);
 #endif /* KVM_CPUS_H */
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index fc30e5d5b8..d70ca62ff5 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -398,6 +398,18 @@ int kvm_create_vcpu(CPUState *cpu)
     return 0;
 }
 
+int kvm_create_and_park_vcpu(CPUState *cpu)
+{
+    int ret = 0;
+
+    ret = kvm_create_vcpu(cpu);
+    if (!ret) {
+        kvm_park_vcpu(cpu);
+    }
+
+    return ret;
+}
+
 static int do_kvm_destroy_vcpu(CPUState *cpu)
 {
     KVMState *s = kvm_state;
-- 
2.39.3

Re: [PATCH v3 1/3] accel/kvm: Introduce kvm_create_and_park_vcpu() helper

[Nicholas Piggin]

On Thu May 23, 2024 at 5:26 PM AEST, Harsh Prateek Bora wrote:
> There are distinct helpers for creating and parking a KVM vCPU.
> However, there can be cases where a platform needs to create and
> immediately park the vCPU during early stages of vcpu init which
> can later be reused when vcpu thread gets initialized. This would
> help detect failures with kvm_create_vcpu at an early stage.
>
> Based on api refactoring to create/park vcpus introduced in 1/8 of patch series:
> https://lore.kernel.org/qemu-devel/20240522211111.232114-1-salil.mehta@huawei.com/
>

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> Suggested-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> ---
>  accel/kvm/kvm-cpus.h |  8 ++++++++
>  accel/kvm/kvm-all.c  | 12 ++++++++++++
>  2 files changed, 20 insertions(+)
>
> diff --git a/accel/kvm/kvm-cpus.h b/accel/kvm/kvm-cpus.h
> index 2e6bb38b5d..00e534b3b9 100644
> --- a/accel/kvm/kvm-cpus.h
> +++ b/accel/kvm/kvm-cpus.h
> @@ -46,4 +46,12 @@ void kvm_park_vcpu(CPUState *cpu);
>   * @returns: KVM fd
>   */
>  int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id);
> +
> +/**
> + *  * kvm_create_and_park_vcpu - Create and park a KVM vCPU
> + *   * @cpu: QOM CPUState object for which KVM vCPU has to be created and parked.
> + *    *
> + *     * @returns: 0 when success, errno (<0) when failed.
> + *      */

Something went wrong with re-flowing the comment?

Don't worry about resending for that just now, at least until we work
out the dependency with Salil's series and ready to merge.

Thanks,
Nick

> +int kvm_create_and_park_vcpu(CPUState *cpu);
>  #endif /* KVM_CPUS_H */
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index fc30e5d5b8..d70ca62ff5 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -398,6 +398,18 @@ int kvm_create_vcpu(CPUState *cpu)
>      return 0;
>  }
>  
> +int kvm_create_and_park_vcpu(CPUState *cpu)
> +{
> +    int ret = 0;
> +
> +    ret = kvm_create_vcpu(cpu);
> +    if (!ret) {
> +        kvm_park_vcpu(cpu);
> +    }
> +
> +    return ret;
> +}
> +
>  static int do_kvm_destroy_vcpu(CPUState *cpu)
>  {
>      KVMState *s = kvm_state;

[PATCH v3 2/3] cpu-common.c: export cpu_get_free_index to be reused later

[Harsh Prateek Bora]

This helper provides an easy way to identify the next available free cpu
index which can be used for vcpu creation. Until now, this is being
called at a very later stage and there is a need to be able to call it
earlier (for now, with ppc64) hence the need to export.

Suggested-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
---
 include/exec/cpu-common.h | 2 ++
 cpu-common.c              | 7 ++++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 6d5318895a..0386f1ab29 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -29,6 +29,8 @@ void cpu_list_lock(void);
 void cpu_list_unlock(void);
 unsigned int cpu_list_generation_id_get(void);
 
+int cpu_get_free_index(void);
+
 void tcg_iommu_init_notifier_list(CPUState *cpu);
 void tcg_iommu_free_notifier_list(CPUState *cpu);
 
diff --git a/cpu-common.c b/cpu-common.c
index ce78273af5..82bd1b432d 100644
--- a/cpu-common.c
+++ b/cpu-common.c
@@ -57,14 +57,12 @@ void cpu_list_unlock(void)
     qemu_mutex_unlock(&qemu_cpu_list_lock);
 }
 
-static bool cpu_index_auto_assigned;
 
-static int cpu_get_free_index(void)
+int cpu_get_free_index(void)
 {
     CPUState *some_cpu;
     int max_cpu_index = 0;
 
-    cpu_index_auto_assigned = true;
     CPU_FOREACH(some_cpu) {
         if (some_cpu->cpu_index >= max_cpu_index) {
             max_cpu_index = some_cpu->cpu_index + 1;
@@ -83,8 +81,11 @@ unsigned int cpu_list_generation_id_get(void)
 
 void cpu_list_add(CPUState *cpu)
 {
+    static bool cpu_index_auto_assigned;
+
     QEMU_LOCK_GUARD(&qemu_cpu_list_lock);
     if (cpu->cpu_index == UNASSIGNED_CPU_INDEX) {
+        cpu_index_auto_assigned = true;
         cpu->cpu_index = cpu_get_free_index();
         assert(cpu->cpu_index != UNASSIGNED_CPU_INDEX);
     } else {
-- 
2.39.3

Re: [PATCH v3 2/3] cpu-common.c: export cpu_get_free_index to be reused later

[Nicholas Piggin]

On Thu May 23, 2024 at 5:26 PM AEST, Harsh Prateek Bora wrote:
> This helper provides an easy way to identify the next available free cpu
> index which can be used for vcpu creation. Until now, this is being
> called at a very later stage and there is a need to be able to call it
> earlier (for now, with ppc64) hence the need to export.
>

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> Suggested-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> ---
>  include/exec/cpu-common.h | 2 ++
>  cpu-common.c              | 7 ++++---
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index 6d5318895a..0386f1ab29 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -29,6 +29,8 @@ void cpu_list_lock(void);
>  void cpu_list_unlock(void);
>  unsigned int cpu_list_generation_id_get(void);
>  
> +int cpu_get_free_index(void);
> +
>  void tcg_iommu_init_notifier_list(CPUState *cpu);
>  void tcg_iommu_free_notifier_list(CPUState *cpu);
>  
> diff --git a/cpu-common.c b/cpu-common.c
> index ce78273af5..82bd1b432d 100644
> --- a/cpu-common.c
> +++ b/cpu-common.c
> @@ -57,14 +57,12 @@ void cpu_list_unlock(void)
>      qemu_mutex_unlock(&qemu_cpu_list_lock);
>  }
>  
> -static bool cpu_index_auto_assigned;
>  
> -static int cpu_get_free_index(void)
> +int cpu_get_free_index(void)
>  {
>      CPUState *some_cpu;
>      int max_cpu_index = 0;
>  
> -    cpu_index_auto_assigned = true;
>      CPU_FOREACH(some_cpu) {
>          if (some_cpu->cpu_index >= max_cpu_index) {
>              max_cpu_index = some_cpu->cpu_index + 1;
> @@ -83,8 +81,11 @@ unsigned int cpu_list_generation_id_get(void)
>  
>  void cpu_list_add(CPUState *cpu)
>  {
> +    static bool cpu_index_auto_assigned;
> +
>      QEMU_LOCK_GUARD(&qemu_cpu_list_lock);
>      if (cpu->cpu_index == UNASSIGNED_CPU_INDEX) {
> +        cpu_index_auto_assigned = true;
>          cpu->cpu_index = cpu_get_free_index();
>          assert(cpu->cpu_index != UNASSIGNED_CPU_INDEX);
>      } else {

[PATCH v3 3/3] target/ppc: handle vcpu hotplug failure gracefully

[Harsh Prateek Bora]

On ppc64, the PowerVM hypervisor runs with limited memory and a VCPU
creation during hotplug may fail during kvm_ioctl for KVM_CREATE_VCPU,
leading to termination of guest since errp is set to &error_fatal while
calling kvm_init_vcpu. This unexpected behaviour can be avoided by
pre-creating and parking vcpu on success or return error otherwise.
This enables graceful error delivery for any vcpu hotplug failures while
the guest can keep running.

Also introducing KVM AccelCPUClass to init cpu_target_realize for kvm.

Tested OK by repeatedly doing a hotplug/unplug of vcpus as below:

 #virsh setvcpus hotplug 40
 #virsh setvcpus hotplug 70
error: internal error: unable to execute QEMU command 'device_add':
kvmppc_cpu_realize: vcpu hotplug failed with -12

Reported-by: Anushree Mathur <anushree.mathur@linux.vnet.ibm.com>
Suggested-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
Suggested-by: Vaibhav Jain <vaibhav@linux.ibm.com>
Signed-off by: Harsh Prateek Bora <harshpb@linux.ibm.com>
Tested-by: Anushree Mathur <anushree.mathur@linux.vnet.ibm.com>
---
 target/ppc/kvm.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 63930d4a77..8e5a7c3d2d 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -48,6 +48,8 @@
 #include "qemu/mmap-alloc.h"
 #include "elf.h"
 #include "sysemu/kvm_int.h"
+#include "accel/kvm/kvm-cpus.h"
+#include "hw/core/accel-cpu.h"
 
 #define PROC_DEVTREE_CPU      "/proc/device-tree/cpus/"
 
@@ -2339,6 +2341,25 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
     }
 }
 
+static bool kvmppc_cpu_realize(CPUState *cs, Error **errp)
+{
+    int ret;
+    const char *vcpu_str = (cs->parent_obj.hotplugged == true) ?
+                           "hotplug" : "create";
+    cs->cpu_index = cpu_get_free_index();
+
+    POWERPC_CPU(cs)->vcpu_id = cs->cpu_index;
+
+    /* create and park to fail gracefully in case vcpu hotplug fails */
+    ret = kvm_create_and_park_vcpu(cs);
+    if (ret) {
+        error_setg(errp, "%s: vcpu %s failed with %d",
+                         __func__, vcpu_str, ret);
+        return false;
+    }
+    return true;
+}
+
 static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
 {
     PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
@@ -2959,3 +2980,23 @@ void kvmppc_set_reg_tb_offset(PowerPCCPU *cpu, int64_t tb_offset)
 void kvm_arch_accel_class_init(ObjectClass *oc)
 {
 }
+
+static void kvm_cpu_accel_class_init(ObjectClass *oc, void *data)
+{
+    AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
+
+    acc->cpu_target_realize = kvmppc_cpu_realize;
+}
+
+static const TypeInfo kvm_cpu_accel_type_info = {
+    .name = ACCEL_CPU_NAME("kvm"),
+
+    .parent = TYPE_ACCEL_CPU,
+    .class_init = kvm_cpu_accel_class_init,
+    .abstract = true,
+};
+static void kvm_cpu_accel_register_types(void)
+{
+    type_register_static(&kvm_cpu_accel_type_info);
+}
+type_init(kvm_cpu_accel_register_types);
-- 
2.39.3

Re: [PATCH v3 3/3] target/ppc: handle vcpu hotplug failure gracefully

[Nicholas Piggin]

On Thu May 23, 2024 at 5:26 PM AEST, Harsh Prateek Bora wrote:
> On ppc64, the PowerVM hypervisor runs with limited memory and a VCPU
> creation during hotplug may fail during kvm_ioctl for KVM_CREATE_VCPU,
> leading to termination of guest since errp is set to &error_fatal while
> calling kvm_init_vcpu. This unexpected behaviour can be avoided by
> pre-creating and parking vcpu on success or return error otherwise.
> This enables graceful error delivery for any vcpu hotplug failures while
> the guest can keep running.
>
> Also introducing KVM AccelCPUClass to init cpu_target_realize for kvm.
>
> Tested OK by repeatedly doing a hotplug/unplug of vcpus as below:
>
>  #virsh setvcpus hotplug 40
>  #virsh setvcpus hotplug 70
> error: internal error: unable to execute QEMU command 'device_add':
> kvmppc_cpu_realize: vcpu hotplug failed with -12
>
> Reported-by: Anushree Mathur <anushree.mathur@linux.vnet.ibm.com>
> Suggested-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> Suggested-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> Signed-off by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> Tested-by: Anushree Mathur <anushree.mathur@linux.vnet.ibm.com>
> ---
>  target/ppc/kvm.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 63930d4a77..8e5a7c3d2d 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -48,6 +48,8 @@
>  #include "qemu/mmap-alloc.h"
>  #include "elf.h"
>  #include "sysemu/kvm_int.h"
> +#include "accel/kvm/kvm-cpus.h"
> +#include "hw/core/accel-cpu.h"
>  
>  #define PROC_DEVTREE_CPU      "/proc/device-tree/cpus/"
>  
> @@ -2339,6 +2341,25 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
>      }
>  }
>  
> +static bool kvmppc_cpu_realize(CPUState *cs, Error **errp)
> +{
> +    int ret;
> +    const char *vcpu_str = (cs->parent_obj.hotplugged == true) ?
> +                           "hotplug" : "create";
> +    cs->cpu_index = cpu_get_free_index();
> +
> +    POWERPC_CPU(cs)->vcpu_id = cs->cpu_index;
> +
> +    /* create and park to fail gracefully in case vcpu hotplug fails */

The only thing that wasn't immediately clear to me is that in the
machine init path, this results in qemu termination, and in the
hotplug path it results in graceful hotplug failure. That is the
behaviour we want, maybe just expand the comment slightly to be
more explicit about it. E.g.,

> +    ret = kvm_create_and_park_vcpu(cs);
> +    if (ret) {
           /*
	    * This causes QEMU to terminate if initial CPU creation
	    * fails, and CPU hotplug failure if the error happens
	    * there.
	    */
> +        error_setg(errp, "%s: vcpu %s failed with %d",
> +                         __func__, vcpu_str, ret);
> +        return false;
> +    }
> +    return true;
> +}
> +

Otherwise looks good.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>  static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> @@ -2959,3 +2980,23 @@ void kvmppc_set_reg_tb_offset(PowerPCCPU *cpu, int64_t tb_offset)
>  void kvm_arch_accel_class_init(ObjectClass *oc)
>  {
>  }
> +
> +static void kvm_cpu_accel_class_init(ObjectClass *oc, void *data)
> +{
> +    AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
> +
> +    acc->cpu_target_realize = kvmppc_cpu_realize;
> +}
> +
> +static const TypeInfo kvm_cpu_accel_type_info = {
> +    .name = ACCEL_CPU_NAME("kvm"),
> +
> +    .parent = TYPE_ACCEL_CPU,
> +    .class_init = kvm_cpu_accel_class_init,
> +    .abstract = true,
> +};
> +static void kvm_cpu_accel_register_types(void)
> +{
> +    type_register_static(&kvm_cpu_accel_type_info);
> +}
> +type_init(kvm_cpu_accel_register_types);