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

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

[Harsh Prateek Bora]

+ qemu-devel

On 4/23/24 11:40, 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 vcpu and parking it on success or return error otherwise.
> This enables graceful error delivery for any vcpu hotplug failures while
> the guest can keep running.
> 
> Based on api refactoring to create/park vcpus introduced in 1/8 of patch series:
> https://lore.kernel.org/qemu-devel/20240312020000.12992-2-salil.mehta@huawei.com/
> 
> 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>
> ---
> ---
>   target/ppc/kvm.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 42 insertions(+)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 8231feb2d4..c887f6dfa0 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 "sysemu/kvm.h"
> +#include "hw/core/accel-cpu.h"
>   
>   #define PROC_DEVTREE_CPU      "/proc/device-tree/cpus/"
>   
> @@ -2339,6 +2341,43 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
>       }
>   }
>   
> +static int max_cpu_index = 0;
> +
> +static bool kvmppc_cpu_realize(CPUState *cs, Error **errp)
> +{
> +    int ret;
> +
> +    cs->cpu_index = max_cpu_index++;
> +
> +    POWERPC_CPU(cs)->vcpu_id = cs->cpu_index;
> +
> +    if (cs->parent_obj.hotplugged) {
> +        /* create and park to fail gracefully in case vcpu hotplug fails */
> +        ret = kvm_create_vcpu(cs);
> +        if (!ret) {
> +            kvm_park_vcpu(cs);
> +        } else {
> +            max_cpu_index--;
> +            error_setg(errp, "%s: vcpu hotplug failed with %d",
> +                             __func__, ret);
> +            return false;
> +        }
> +    }
> +    return true;
> +}
> +
> +static void kvmppc_cpu_unrealize(CPUState *cpu)
> +{
> +    if (POWERPC_CPU(cpu)->vcpu_id == (max_cpu_index - 1)) {
> +    /* only reclaim vcpuid if its the last one assigned
> +     * as reclaiming random vcpuid for parked vcpus may lead
> +     * to unexpected behaviour due to an existing kernel bug
> +     * when drc_index doesnt get reclaimed as expected.
> +     */
> +        max_cpu_index--;
> +    }
> +}
> +
>   static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
>   {
>       PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> @@ -2963,4 +3002,7 @@ bool kvm_arch_cpu_check_are_resettable(void)
>   
>   void kvm_arch_accel_class_init(ObjectClass *oc)
>   {
> +    AccelClass *ac = ACCEL_CLASS(oc);
> +    ac->cpu_common_realize = kvmppc_cpu_realize;
> +    ac->cpu_common_unrealize = kvmppc_cpu_unrealize;
>   }

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

[Nicholas Piggin]

On Tue Apr 23, 2024 at 4:30 PM AEST, Harsh Prateek Bora wrote:
> + qemu-devel
>
> On 4/23/24 11:40, 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 vcpu and parking it on success or return error otherwise.
> > This enables graceful error delivery for any vcpu hotplug failures while
> > the guest can keep running.

So this puts in on the park list so when kvm_init_vcpu() later runs it
will just take it off the park list instead of issuing another
KVM_CREATE_VCPU ioctl.

And kvm_init_vcpu() runs in the vcpu thread function, which does not
have a good way to indicate failure to the caller.

I'm don't know a lot about this part of qemu but it seems like a good
idea to move fail-able initialisation out of the vcpu thread in that
case. So the general idea seems good to me.

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

So from this series AFAIKS you're just using kvm_create / kvm_park
routines? You could easily pull that patch 1 out ahead of that larger
series if progress is slow on it, it's a decent cleanup by itself by
the looks.

> > 
> > 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>
> > ---
> > ---
> >   target/ppc/kvm.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 42 insertions(+)
> > 
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index 8231feb2d4..c887f6dfa0 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 "sysemu/kvm.h"
> > +#include "hw/core/accel-cpu.h"
> >   
> >   #define PROC_DEVTREE_CPU      "/proc/device-tree/cpus/"
> >   
> > @@ -2339,6 +2341,43 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
> >       }
> >   }
> >   
> > +static int max_cpu_index = 0;
> > +
> > +static bool kvmppc_cpu_realize(CPUState *cs, Error **errp)
> > +{
> > +    int ret;
> > +
> > +    cs->cpu_index = max_cpu_index++;
> > +
> > +    POWERPC_CPU(cs)->vcpu_id = cs->cpu_index;

So you're overriding the cpu_get_free_index() allocator here.
And you need to because vcpu_id needs to be assigned before
the KVM create, I guess.

I guess it works. I would add a comment like s390x has.

> > +
> > +    if (cs->parent_obj.hotplugged) {

Can _all_ kvm cpu creation go via this path? Why just limit it to
hotplugged?

> > +        /* create and park to fail gracefully in case vcpu hotplug fails */
> > +        ret = kvm_create_vcpu(cs);
> > +        if (!ret) {
> > +            kvm_park_vcpu(cs);

Seems like a small thing, but I would add a new core kvm function
that creates and parks the vcpu, so the target code doesn't have
to know about the parking internals, just that it needs to be
called.

Unless I'm missing something, we could get all targets to move their kvm
create to here and remove it removed from kvm_init_vcpu(), that would
just expect it to be on the parked list. But that could be done
incrementally.

> > +        } else {
> > +            max_cpu_index--;
> > +            error_setg(errp, "%s: vcpu hotplug failed with %d",
> > +                             __func__, ret);
> > +            return false;
> > +        }
> > +    }
> > +    return true;
> > +}
> > +
> > +static void kvmppc_cpu_unrealize(CPUState *cpu)
> > +{
> > +    if (POWERPC_CPU(cpu)->vcpu_id == (max_cpu_index - 1)) {
> > +    /* only reclaim vcpuid if its the last one assigned
> > +     * as reclaiming random vcpuid for parked vcpus may lead
> > +     * to unexpected behaviour due to an existing kernel bug
> > +     * when drc_index doesnt get reclaimed as expected.
> > +     */
> > +        max_cpu_index--;
> > +    }

This looks like a fairly lossy allocator. Using cpu_get_free_index()
would be the way to go I think. I would export that and call it here,
and then you don't need this. Just have to take care of the assert,
something like this:

diff --git a/cpu-common.c b/cpu-common.c
index ce78273af5..9f90c8ec9b 100644
--- a/cpu-common.c
+++ b/cpu-common.c
@@ -57,14 +57,11 @@ 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 +80,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 {


> > +}
> > +
> >   static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
> >   {
> >       PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> > @@ -2963,4 +3002,7 @@ bool kvm_arch_cpu_check_are_resettable(void)
> >   
> >   void kvm_arch_accel_class_init(ObjectClass *oc)
> >   {
> > +    AccelClass *ac = ACCEL_CLASS(oc);
> > +    ac->cpu_common_realize = kvmppc_cpu_realize;
> > +    ac->cpu_common_unrealize = kvmppc_cpu_unrealize;
> >   }

Thanks,
Nick

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

[Harsh Prateek Bora]

Hi Nick,

On 5/14/24 08:39, Nicholas Piggin wrote:
> On Tue Apr 23, 2024 at 4:30 PM AEST, Harsh Prateek Bora wrote:
>> + qemu-devel
>>
>> On 4/23/24 11:40, 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 vcpu and parking it on success or return error otherwise.
>>> This enables graceful error delivery for any vcpu hotplug failures while
>>> the guest can keep running.
> 
> So this puts in on the park list so when kvm_init_vcpu() later runs it
> will just take it off the park list instead of issuing another
> KVM_CREATE_VCPU ioctl.
> 
> And kvm_init_vcpu() runs in the vcpu thread function, which does not
> have a good way to indicate failure to the caller.
> 
> I'm don't know a lot about this part of qemu but it seems like a good
> idea to move fail-able initialisation out of the vcpu thread in that
> case. So the general idea seems good to me.
> 

Yeh ..

>>>
>>> Based on api refactoring to create/park vcpus introduced in 1/8 of patch series:
>>> https://lore.kernel.org/qemu-devel/20240312020000.12992-2-salil.mehta@huawei.com/
> 
> So from this series AFAIKS you're just using kvm_create / kvm_park
> routines? You could easily pull that patch 1 out ahead of that larger
> series if progress is slow on it, it's a decent cleanup by itself by
> the looks.
> 

Yeh, patch 1 of that series is only we need but the author mentioned on 
the list that he is about to post next version soon.

>>>
>>> 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>
>>> ---
>>> ---
>>>    target/ppc/kvm.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 42 insertions(+)
>>>
>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>> index 8231feb2d4..c887f6dfa0 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 "sysemu/kvm.h"
>>> +#include "hw/core/accel-cpu.h"
>>>    
>>>    #define PROC_DEVTREE_CPU      "/proc/device-tree/cpus/"
>>>    
>>> @@ -2339,6 +2341,43 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
>>>        }
>>>    }
>>>    
>>> +static int max_cpu_index = 0;
>>> +
>>> +static bool kvmppc_cpu_realize(CPUState *cs, Error **errp)
>>> +{
>>> +    int ret;
>>> +
>>> +    cs->cpu_index = max_cpu_index++;
>>> +
>>> +    POWERPC_CPU(cs)->vcpu_id = cs->cpu_index;
> 
> So you're overriding the cpu_get_free_index() allocator here.
> And you need to because vcpu_id needs to be assigned before
> the KVM create, I guess.
> 

Yes ..

> I guess it works. I would add a comment like s390x has.
> 
Not sure which comment you were referring to but with exporting
cpu_get_free_index as suggested later, not sure if we still need any
comment.

>>> +
>>> +    if (cs->parent_obj.hotplugged) {
> 
> Can _all_ kvm cpu creation go via this path? Why just limit it to
> hotplugged?

For the initial bootup, we actually want to abort if the requested vCPUs
cant be allocated so that user can retry until the requested vCPUs are
allocated. For hotplug failure, bringing down entire guest isn't fair,
hence the fix.

> 
>>> +        /* create and park to fail gracefully in case vcpu hotplug fails */
>>> +        ret = kvm_create_vcpu(cs);
>>> +        if (!ret) {
>>> +            kvm_park_vcpu(cs);
> 
> Seems like a small thing, but I would add a new core kvm function
> that creates and parks the vcpu, so the target code doesn't have
> to know about the parking internals, just that it needs to be
> called.

Make sense, I will add another kvm helper: kvm_create_and_park_vcpu()

> 
> Unless I'm missing something, we could get all targets to move their kvm
> create to here and remove it removed from kvm_init_vcpu(), that would
> just expect it to be on the parked list. But that could be done
> incrementally.

Hmm ..

> 
>>> +        } else {
>>> +            max_cpu_index--;
>>> +            error_setg(errp, "%s: vcpu hotplug failed with %d",
>>> +                             __func__, ret);
>>> +            return false;
>>> +        }
>>> +    }
>>> +    return true;
>>> +}
>>> +
>>> +static void kvmppc_cpu_unrealize(CPUState *cpu)
>>> +{
>>> +    if (POWERPC_CPU(cpu)->vcpu_id == (max_cpu_index - 1)) {
>>> +    /* only reclaim vcpuid if its the last one assigned
>>> +     * as reclaiming random vcpuid for parked vcpus may lead
>>> +     * to unexpected behaviour due to an existing kernel bug
>>> +     * when drc_index doesnt get reclaimed as expected.
>>> +     */
>>> +        max_cpu_index--;
>>> +    }
> 
> This looks like a fairly lossy allocator. Using cpu_get_free_index()
> would be the way to go I think. I would export that and call it here,
> and then you don't need this. Just have to take care of the assert,
> something like this:
> 
> diff --git a/cpu-common.c b/cpu-common.c
> index ce78273af5..9f90c8ec9b 100644
> --- a/cpu-common.c
> +++ b/cpu-common.c
> @@ -57,14 +57,11 @@ 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 +80,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 {
> 

This indeed seems better, will update v2 as suggested.

Thanks
Harsh
> 
>>> +}
>>> +
>>>    static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
>>>    {
>>>        PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
>>> @@ -2963,4 +3002,7 @@ bool kvm_arch_cpu_check_are_resettable(void)
>>>    
>>>    void kvm_arch_accel_class_init(ObjectClass *oc)
>>>    {
>>> +    AccelClass *ac = ACCEL_CLASS(oc);
>>> +    ac->cpu_common_realize = kvmppc_cpu_realize;
>>> +    ac->cpu_common_unrealize = kvmppc_cpu_unrealize;
>>>    }
> 
> Thanks,
> Nick

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

[Nicholas Piggin]

On Thu May 16, 2024 at 2:31 PM AEST, Harsh Prateek Bora wrote:
> Hi Nick,
>
> On 5/14/24 08:39, Nicholas Piggin wrote:
> > On Tue Apr 23, 2024 at 4:30 PM AEST, Harsh Prateek Bora wrote:
> >> + qemu-devel
> >>
> >> On 4/23/24 11:40, 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 vcpu and parking it on success or return error otherwise.
> >>> This enables graceful error delivery for any vcpu hotplug failures while
> >>> the guest can keep running.
> > 
> > So this puts in on the park list so when kvm_init_vcpu() later runs it
> > will just take it off the park list instead of issuing another
> > KVM_CREATE_VCPU ioctl.
> > 
> > And kvm_init_vcpu() runs in the vcpu thread function, which does not
> > have a good way to indicate failure to the caller.
> > 
> > I'm don't know a lot about this part of qemu but it seems like a good
> > idea to move fail-able initialisation out of the vcpu thread in that
> > case. So the general idea seems good to me.
> > 
>
> Yeh ..
>
> >>>
> >>> Based on api refactoring to create/park vcpus introduced in 1/8 of patch series:
> >>> https://lore.kernel.org/qemu-devel/20240312020000.12992-2-salil.mehta@huawei.com/
> > 
> > So from this series AFAIKS you're just using kvm_create / kvm_park
> > routines? You could easily pull that patch 1 out ahead of that larger
> > series if progress is slow on it, it's a decent cleanup by itself by
> > the looks.
> > 
>
> Yeh, patch 1 of that series is only we need but the author mentioned on 
> the list that he is about to post next version soon.
>
> >>>
> >>> 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>
> >>> ---
> >>> ---
> >>>    target/ppc/kvm.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >>>    1 file changed, 42 insertions(+)
> >>>
> >>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> >>> index 8231feb2d4..c887f6dfa0 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 "sysemu/kvm.h"
> >>> +#include "hw/core/accel-cpu.h"
> >>>    
> >>>    #define PROC_DEVTREE_CPU      "/proc/device-tree/cpus/"
> >>>    
> >>> @@ -2339,6 +2341,43 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
> >>>        }
> >>>    }
> >>>    
> >>> +static int max_cpu_index = 0;
> >>> +
> >>> +static bool kvmppc_cpu_realize(CPUState *cs, Error **errp)
> >>> +{
> >>> +    int ret;
> >>> +
> >>> +    cs->cpu_index = max_cpu_index++;
> >>> +
> >>> +    POWERPC_CPU(cs)->vcpu_id = cs->cpu_index;
> > 
> > So you're overriding the cpu_get_free_index() allocator here.
> > And you need to because vcpu_id needs to be assigned before
> > the KVM create, I guess.
> > 
>
> Yes ..
>
> > I guess it works. I would add a comment like s390x has.
> > 
> Not sure which comment you were referring to but with exporting
> cpu_get_free_index as suggested later, not sure if we still need any
> comment.

Yeah that's true.

> >>> +
> >>> +    if (cs->parent_obj.hotplugged) {
> > 
> > Can _all_ kvm cpu creation go via this path? Why just limit it to
> > hotplugged?
>
> For the initial bootup, we actually want to abort if the requested vCPUs
> cant be allocated so that user can retry until the requested vCPUs are
> allocated. For hotplug failure, bringing down entire guest isn't fair,
> hence the fix.

But you could make the error handling depend on hotplugged, no?
Perhaps put that error handling decision in common code so policy
is the same for all targets and back ends.

[...]

> >>> +}
> >>> +
> >>>    static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
> >>>    {
> >>>        PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> >>> @@ -2963,4 +3002,7 @@ bool kvm_arch_cpu_check_are_resettable(void)
> >>>    
> >>>    void kvm_arch_accel_class_init(ObjectClass *oc)
> >>>    {
> >>> +    AccelClass *ac = ACCEL_CLASS(oc);
> >>> +    ac->cpu_common_realize = kvmppc_cpu_realize;
> >>> +    ac->cpu_common_unrealize = kvmppc_cpu_unrealize;
> >>>    }

One other thing I noticed -- cpu_common_realize seems to be for
core code and cpu_target_realize for targets. Should we be
using the latter here? If not, a comment would be warranted and
probably also a comment in accel_cpu_common_realize().

Thanks,
Nick

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

[Harsh Prateek Bora]

On 5/17/24 09:30, Nicholas Piggin wrote:
> On Thu May 16, 2024 at 2:31 PM AEST, Harsh Prateek Bora wrote:
>> Hi Nick,
>>
>> On 5/14/24 08:39, Nicholas Piggin wrote:
>>> On Tue Apr 23, 2024 at 4:30 PM AEST, Harsh Prateek Bora wrote:
>>>> + qemu-devel
>>>>
>>>> On 4/23/24 11:40, 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 vcpu and parking it on success or return error otherwise.
>>>>> This enables graceful error delivery for any vcpu hotplug failures while
>>>>> the guest can keep running.
>>>
>>> So this puts in on the park list so when kvm_init_vcpu() later runs it
>>> will just take it off the park list instead of issuing another
>>> KVM_CREATE_VCPU ioctl.
>>>
>>> And kvm_init_vcpu() runs in the vcpu thread function, which does not
>>> have a good way to indicate failure to the caller.
>>>
>>> I'm don't know a lot about this part of qemu but it seems like a good
>>> idea to move fail-able initialisation out of the vcpu thread in that
>>> case. So the general idea seems good to me.
>>>
>>
>> Yeh ..
>>
>>>>>
>>>>> Based on api refactoring to create/park vcpus introduced in 1/8 of patch series:
>>>>> https://lore.kernel.org/qemu-devel/20240312020000.12992-2-salil.mehta@huawei.com/
>>>
>>> So from this series AFAIKS you're just using kvm_create / kvm_park
>>> routines? You could easily pull that patch 1 out ahead of that larger
>>> series if progress is slow on it, it's a decent cleanup by itself by
>>> the looks.
>>>
>>
>> Yeh, patch 1 of that series is only we need but the author mentioned on
>> the list that he is about to post next version soon.
>>
>>>>>
>>>>> 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>
>>>>> ---
>>>>> ---
>>>>>     target/ppc/kvm.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>>>     1 file changed, 42 insertions(+)
>>>>>
>>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>>> index 8231feb2d4..c887f6dfa0 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 "sysemu/kvm.h"
>>>>> +#include "hw/core/accel-cpu.h"
>>>>>     
>>>>>     #define PROC_DEVTREE_CPU      "/proc/device-tree/cpus/"
>>>>>     
>>>>> @@ -2339,6 +2341,43 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
>>>>>         }
>>>>>     }
>>>>>     
>>>>> +static int max_cpu_index = 0;
>>>>> +
>>>>> +static bool kvmppc_cpu_realize(CPUState *cs, Error **errp)
>>>>> +{
>>>>> +    int ret;
>>>>> +
>>>>> +    cs->cpu_index = max_cpu_index++;
>>>>> +
>>>>> +    POWERPC_CPU(cs)->vcpu_id = cs->cpu_index;
>>>
>>> So you're overriding the cpu_get_free_index() allocator here.
>>> And you need to because vcpu_id needs to be assigned before
>>> the KVM create, I guess.
>>>
>>
>> Yes ..
>>
>>> I guess it works. I would add a comment like s390x has.
>>>
>> Not sure which comment you were referring to but with exporting
>> cpu_get_free_index as suggested later, not sure if we still need any
>> comment.
> 
> Yeah that's true.
> 
>>>>> +
>>>>> +    if (cs->parent_obj.hotplugged) {
>>>
>>> Can _all_ kvm cpu creation go via this path? Why just limit it to
>>> hotplugged?
>>
>> For the initial bootup, we actually want to abort if the requested vCPUs
>> cant be allocated so that user can retry until the requested vCPUs are
>> allocated. For hotplug failure, bringing down entire guest isn't fair,
>> hence the fix.
> 
> But you could make the error handling depend on hotplugged, no?
> Perhaps put that error handling decision in common code so policy
> is the same for all targets and back ends.

Hmm, I think just setting errp appropriately would suffice for both
cases as existing behaviour takes care of the rest of handling.
Something like below:

+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);
>>>>> @@ -2963,4 +3002,7 @@ bool kvm_arch_cpu_check_are_resettable(void)
>>>>>     
>>>>>     void kvm_arch_accel_class_init(ObjectClass *oc)
>>>>>     {
>>>>> +    AccelClass *ac = ACCEL_CLASS(oc);
>>>>> +    ac->cpu_common_realize = kvmppc_cpu_realize;
>>>>> +    ac->cpu_common_unrealize = kvmppc_cpu_unrealize;
>>>>>     }
> 
> One other thing I noticed -- cpu_common_realize seems to be for
> core code and cpu_target_realize for targets. Should we be
> using the latter here? If not, a comment would be warranted and
> probably also a comment in accel_cpu_common_realize().

Well, I looked at that initially but looks like accel_cpu itself (which
contains vector for cpu_target_realize) doesnt get initialized in case
of kvm on ppc for some reason and therefore wouldnt work. I think
this change is generic enough for cpu_common_realize as well (now having
similar behaviour for initial vcpus also), so I guess comment may not 
really be needed, but let me know if still need any comment.

Else, I can post next version with above proposed diff.

regards,
Harsh

> 
> Thanks,
> Nick