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

Commit Message

Salil Mehta Sept. 29, 2023, 12:42 p.m. UTC

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 <[e-mail]>
---
 accel/kvm/kvm-all.c  | 61 ++++++++++++++++++++++++++++++++++----------
 include/sysemu/kvm.h |  2 ++
 2 files changed, 49 insertions(+), 14 deletions(-)

Comments

Alex Bennée Sept. 29, 2023, 1:32 p.m. UTC | #1

Salil Mehta <[e-mail]> 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 <[e-mail]>
> ---
>  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 */

Salil Mehta Sept. 29, 2023, 3:22 p.m. UTC | #2

Hi Alex,
Thanks for taking pains to review it.

> From: Alex Bennée <[e-mail]>
> Sent: Friday, September 29, 2023 2:32 PM
> To: Salil Mehta <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail]; jean-
> [e-mail]; Jonathan Cameron <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> zhukeqian <[e-mail]>; wangxiongfeng (C)
> <[e-mail]>; wangyanan (Y) <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail]; Linuxarm
> <[e-mail]>
> Subject: Re: [PATCH 1/9] accel/kvm: Extract common KVM vCPU
> {creation,parking} code
> 
> 
> Salil Mehta <[e-mail]> 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 <[e-mail]>
> > ---
> >  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.

Alex Bennée Sept. 29, 2023, 4:45 p.m. UTC | #3

Salil Mehta <[e-mail]> writes:

> Hi Alex,
> Thanks for taking pains to review it.
>
>> From: Alex Bennée <[e-mail]>
<snip>
>> Salil Mehta <[e-mail]> 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 <[e-mail]>
>> > ---
>> >  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.

Patch

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 */