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

Commit Message

Salil Mehta June 7, 2024, 11:56 a.m. UTC

KVM vCPU creation is done once during the vCPU realization when Qemu vCPU thread
is spawned. This is common to all the architectures as of now.

Hot-unplug of vCPU results in destruction of the vCPU object in QOM but the
corresponding KVM vCPU object in the Host KVM is not destroyed as KVM doesn't
support vCPU removal. Therefore, its representative KVM vCPU object/context in
Qemu is parked.

Refactor architecture common logic so that some APIs could be reused by vCPU
Hotplug code of some architectures likes ARM, Loongson etc. Update new/old APIs
with trace events. No functional change is intended here.

Signed-off-by: Salil Mehta <[e-mail]>
Reviewed-by: Gavin Shan <[e-mail]>
Tested-by: Vishnu Pajjuri <[e-mail]>
Reviewed-by: Jonathan Cameron <[e-mail]>
Tested-by: Xianglai Li <[e-mail]>
Tested-by: Miguel Luis <[e-mail]>
Reviewed-by: Shaoqin Huang <[e-mail]>
Reviewed-by: Vishnu Pajjuri <[e-mail]>
Reviewed-by: Nicholas Piggin <[e-mail]>
Tested-by: Zhao Liu <[e-mail]>
Reviewed-by: Zhao Liu <[e-mail]>
Reviewed-by: Harsh Prateek Bora <[e-mail]>
---
 accel/kvm/kvm-all.c    | 95 ++++++++++++++++++++++++++++--------------
 accel/kvm/kvm-cpus.h   |  1 -
 accel/kvm/trace-events |  5 ++-
 include/sysemu/kvm.h   | 25 +++++++++++
 4 files changed, 92 insertions(+), 34 deletions(-)

Comments

Harsh Prateek Bora June 25, 2024, 5:08 a.m. UTC | #1

+qemu-devel, qemu-ppc

Ping!

On 6/17/24 15:18, Harsh Prateek Bora wrote:
> 
> + MST, Igor - to help with early review/merge. TIA.
> 
> On 6/14/24 16:06, Salil Mehta wrote:
>> Hello
>>
>>>   From: Harsh Prateek Bora <[e-mail]>
>>>   Sent: Friday, June 14, 2024 6:24 AM
>>>   Hi Paolo, Nick,
>>>   Can this patch 1/8 be merged earlier provided we have got 
>>> sufficient R-bys
>>>   for it and the review of entire series may take a longer time?
>>>   We have some ppc64 patches based on it, hence the ask.
>>>   Hi Salil,
>>>   I am hoping we are not expecting anymore changes to this patch, please
>>>   confirm.
>>
>>
>> I do not expect any change. I had requested Michael to merge the complete
>> series as it is stranding other users. He then requested Igor to take 
>> a final look but
>> he has not reverted yet. I'll remind Michael again. BTW, can you reply 
>> to below
>> patch explicitly indicating your interest in the series so that MST 
>> knows who else
>> are the stake holders here
>>
>> https://lore.kernel.org/qemu-devel/[e-mail]/
>>
>>
>> Hi Paolo,
>>
>> A request, would it be possible to skim through this series from KVM 
>> perspective?
>> (although nothing has changed which will affect the KVM and this is 
>> architecture
>> agnostic patch-set)
>>
>> Many thanks!
>>
>> Best
>> Salil.
>>
>>
>>>   regards,
>>>   Harsh
>>>   On 6/7/24 17:26, Salil Mehta wrote:
>>>   > KVM vCPU creation is done once during the vCPU realization when Qemu
>>>   > vCPU thread is spawned. This is common to all the architectures 
>>> as of now.
>>>   >
>>>   > Hot-unplug of vCPU results in destruction of the vCPU object in QOM
>>>   > but the corresponding KVM vCPU object in the Host KVM is not 
>>> destroyed
>>>   > as KVM doesn't support vCPU removal. Therefore, its 
>>> representative KVM
>>>   > vCPU object/context in Qemu is parked.
>>>   >
>>>   > Refactor architecture common logic so that some APIs could be reused
>>>   > by vCPU Hotplug code of some architectures likes ARM, Loongson etc.
>>>   > Update new/old APIs with trace events. No functional change is 
>>> intended
>>>   here.
>>>   >
>>>   > Signed-off-by: Salil Mehta <[e-mail]>
>>>   > Reviewed-by: Gavin Shan <[e-mail]>
>>>   > Tested-by: Vishnu Pajjuri <[e-mail]>
>>>   > Reviewed-by: Jonathan Cameron <[e-mail]>
>>>   > Tested-by: Xianglai Li <[e-mail]>
>>>   > Tested-by: Miguel Luis <[e-mail]>
>>>   > Reviewed-by: Shaoqin Huang <[e-mail]>
>>>   > Reviewed-by: Vishnu Pajjuri <[e-mail]>
>>>   > Reviewed-by: Nicholas Piggin <[e-mail]>
>>>   > Tested-by: Zhao Liu <[e-mail]>
>>>   > Reviewed-by: Zhao Liu <[e-mail]>
>>>   > Reviewed-by: Harsh Prateek Bora <[e-mail]>
>>>   > ---
>>>   >   accel/kvm/kvm-all.c    | 95 
>>> ++++++++++++++++++++++++++++------------
>>>   --
>>>   >   accel/kvm/kvm-cpus.h   |  1 -
>>>   >   accel/kvm/trace-events |  5 ++-
>>>   >   include/sysemu/kvm.h   | 25 +++++++++++
>>>   >   4 files changed, 92 insertions(+), 34 deletions(-)
>>>   >
>>>   > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index
>>>   > c0be9f5eed..8f9128bb92 100644
>>>   > --- a/accel/kvm/kvm-all.c
>>>   > +++ b/accel/kvm/kvm-all.c
>>>   > @@ -340,14 +340,71 @@ err:
>>>   >       return ret;
>>>   >   }
>>>   >
>>>   > +void kvm_park_vcpu(CPUState *cpu)
>>>   > +{
>>>   > +    struct KVMParkedVcpu *vcpu;
>>>   > +
>>>   > +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(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); }
>>>   > +
>>>   > +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id) {
>>>   > +    struct KVMParkedVcpu *cpu;
>>>   > +    int kvm_fd = -ENOENT;
>>>   > +
>>>   > +    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
>>>   > +        if (cpu->vcpu_id == vcpu_id) {
>>>   > +            QLIST_REMOVE(cpu, node);
>>>   > +            kvm_fd = cpu->kvm_fd;
>>>   > +            g_free(cpu);
>>>   > +        }
>>>   > +    }
>>>   > +
>>>   > +    trace_kvm_unpark_vcpu(vcpu_id, kvm_fd > 0 ? "unparked" : "not
>>>   > + found parked");
>>>   > +
>>>   > +    return kvm_fd;
>>>   > +}
>>>   > +
>>>   > +int kvm_create_vcpu(CPUState *cpu)
>>>   > +{
>>>   > +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
>>>   > +    KVMState *s = kvm_state;
>>>   > +    int kvm_fd;
>>>   > +
>>>   > +    /* check if the KVM vCPU already exist but is parked */
>>>   > +    kvm_fd = kvm_unpark_vcpu(s, vcpu_id);
>>>   > +    if (kvm_fd < 0) {
>>>   > +        /* vCPU not parked: create a new KVM vCPU */
>>>   > +        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
>>>   > +        if (kvm_fd < 0) {
>>>   > +            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU 
>>> %lu",
>>>   vcpu_id);
>>>   > +            return kvm_fd;
>>>   > +        }
>>>   > +    }
>>>   > +
>>>   > +    cpu->kvm_fd = kvm_fd;
>>>   > +    cpu->kvm_state = s;
>>>   > +    cpu->vcpu_dirty = true;
>>>   > +    cpu->dirty_pages = 0;
>>>   > +    cpu->throttle_us_per_full = 0;
>>>   > +
>>>   > +    trace_kvm_create_vcpu(cpu->cpu_index, vcpu_id, kvm_fd);
>>>   > +
>>>   > +    return 0;
>>>   > +}
>>>   > +
>>>   >   static int do_kvm_destroy_vcpu(CPUState *cpu)
>>>   >   {
>>>   >       KVMState *s = kvm_state;
>>>   >       long mmap_size;
>>>   > -    struct KVMParkedVcpu *vcpu = NULL;
>>>   >       int ret = 0;
>>>   >
>>>   > -    trace_kvm_destroy_vcpu();
>>>   > +    trace_kvm_destroy_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>>>   >
>>>   >       ret = kvm_arch_destroy_vcpu(cpu);
>>>   >       if (ret < 0) {
>>>   > @@ -373,10 +430,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;
>>>   >   }
>>>   > @@ -389,24 +443,6 @@ void kvm_destroy_vcpu(CPUState *cpu)
>>>   >       }
>>>   >   }
>>>   >
>>>   > -static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id) -{
>>>   > -    struct KVMParkedVcpu *cpu;
>>>   > -
>>>   > -    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
>>>   > -        if (cpu->vcpu_id == vcpu_id) {
>>>   > -            int kvm_fd;
>>>   > -
>>>   > -            QLIST_REMOVE(cpu, node);
>>>   > -            kvm_fd = cpu->kvm_fd;
>>>   > -            g_free(cpu);
>>>   > -            return kvm_fd;
>>>   > -        }
>>>   > -    }
>>>   > -
>>>   > -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
>>>   > -}
>>>   > -
>>>   >   int kvm_init_vcpu(CPUState *cpu, Error **errp)
>>>   >   {
>>>   >       KVMState *s = kvm_state;
>>>   > @@ -415,19 +451,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/accel/kvm/kvm-cpus.h b/accel/kvm/kvm-cpus.h index
>>>   > ca40add32c..171b22fd29 100644
>>>   > --- a/accel/kvm/kvm-cpus.h
>>>   > +++ b/accel/kvm/kvm-cpus.h
>>>   > @@ -22,5 +22,4 @@ bool kvm_supports_guest_debug(void);
>>>   >   int kvm_insert_breakpoint(CPUState *cpu, int type, vaddr addr, 
>>> vaddr
>>>   len);
>>>   >   int kvm_remove_breakpoint(CPUState *cpu, int type, vaddr addr, 
>>> vaddr
>>>   len);
>>>   >   void kvm_remove_all_breakpoints(CPUState *cpu);
>>>   > -
>>>   >   #endif /* KVM_CPUS_H */
>>>   > diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events index
>>>   > 681ccb667d..37626c1ac5 100644
>>>   > --- a/accel/kvm/trace-events
>>>   > +++ b/accel/kvm/trace-events
>>>   > @@ -9,6 +9,10 @@ kvm_device_ioctl(int fd, int type, void *arg) 
>>> "dev fd
>>>   %d, type 0x%x, arg %p"
>>>   >   kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: 
>>> Unable to
>>>   retrieve ONEREG %" PRIu64 " from KVM: %s"
>>>   >   kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: 
>>> Unable to
>>>   set ONEREG %" PRIu64 " to KVM: %s"
>>>   >   kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: 
>>> %d id:
>>>   %lu"
>>>   > +kvm_create_vcpu(int cpu_index, unsigned long arch_cpu_id, int
>>>   kvm_fd) "index: %d, id: %lu, kvm fd: %d"
>>>   > +kvm_destroy_vcpu(int cpu_index, unsigned long arch_cpu_id) "index:
>>>   %d id: %lu"
>>>   > +kvm_park_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d
>>>   id: %lu"
>>>   > +kvm_unpark_vcpu(unsigned long arch_cpu_id, const char *msg) "id: 
>>> %lu
>>>   %s"
>>>   >   kvm_irqchip_commit_routes(void) ""
>>>   >   kvm_irqchip_add_msi_route(char *name, int vector, int virq) 
>>> "dev %s
>>>   vector %d virq %d"
>>>   >   kvm_irqchip_update_msi_route(int virq) "Updating MSI route 
>>> virq=%d"
>>>   > @@ -25,7 +29,6 @@ kvm_dirty_ring_reaper(const char *s) "%s"
>>>   >   kvm_dirty_ring_reap(uint64_t count, int64_t t) "reaped 
>>> %"PRIu64" pages
>>>   (took %"PRIi64" us)"
>>>   >   kvm_dirty_ring_reaper_kick(const char *reason) "%s"
>>>   >   kvm_dirty_ring_flush(int finished) "%d"
>>>   > -kvm_destroy_vcpu(void) ""
>>>   >   kvm_failed_get_vcpu_mmap_size(void) ""
>>>   >   kvm_cpu_exec(void) ""
>>>   >   kvm_interrupt_exit_request(void) ""
>>>   > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index
>>>   > c31d9c7356..c4a914b3d8 100644
>>>   > --- a/include/sysemu/kvm.h
>>>   > +++ b/include/sysemu/kvm.h
>>>   > @@ -313,6 +313,31 @@ int kvm_create_device(KVMState *s, uint64_t
>>>   type, bool test);
>>>   >    */
>>>   >   bool kvm_device_supported(int vmfd, uint64_t type);
>>>   >
>>>   > +/**
>>>   > + * kvm_create_vcpu - Gets a parked KVM vCPU or creates a KVM vCPU
>>>   > + * @cpu: QOM CPUState object for which KVM vCPU has to be
>>>   fetched/created.
>>>   > + *
>>>   > + * @returns: 0 when success, errno (<0) when failed.
>>>   > + */
>>>   > +int kvm_create_vcpu(CPUState *cpu);
>>>   > +
>>>   > +/**
>>>   > + * kvm_park_vcpu - Park QEMU KVM vCPU context
>>>   > + * @cpu: QOM CPUState object for which QEMU KVM vCPU context has
>>>   to be parked.
>>>   > + *
>>>   > + * @returns: none
>>>   > + */
>>>   > +void kvm_park_vcpu(CPUState *cpu);
>>>   > +
>>>   > +/**
>>>   > + * kvm_unpark_vcpu - unpark QEMU KVM vCPU context
>>>   > + * @s: KVM State
>>>   > + * @vcpu_id: Architecture vCPU ID of the parked vCPU
>>>   > + *
>>>   > + * @returns: KVM fd
>>>   > + */
>>>   > +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id);
>>>   > +
>>>   >   /* Arch specific hooks */
>>>   >
>>>   >   extern const KVMCapabilityInfo kvm_arch_required_capabilities[];

Nicholas Piggin July 4, 2024, 7:35 a.m. UTC | #2

Looks like there is a bit of noise around this recently. Do we
think the hotplug patches can get over the line this time?

If not, perhaps we work with Salil to get this patch 1 upstream
at least.

Thanks,
Nick

On Tue Jun 25, 2024 at 3:08 PM AEST, Harsh Prateek Bora wrote:
> +qemu-devel, qemu-ppc
>
> Ping!
>
> On 6/17/24 15:18, Harsh Prateek Bora wrote:
> > 
> > + MST, Igor - to help with early review/merge. TIA.
> > 
> > On 6/14/24 16:06, Salil Mehta wrote:
> >> Hello
> >>
> >>>   From: Harsh Prateek Bora <[e-mail]>
> >>>   Sent: Friday, June 14, 2024 6:24 AM
> >>>   Hi Paolo, Nick,
> >>>   Can this patch 1/8 be merged earlier provided we have got 
> >>> sufficient R-bys
> >>>   for it and the review of entire series may take a longer time?
> >>>   We have some ppc64 patches based on it, hence the ask.
> >>>   Hi Salil,
> >>>   I am hoping we are not expecting anymore changes to this patch, please
> >>>   confirm.
> >>
> >>
> >> I do not expect any change. I had requested Michael to merge the complete
> >> series as it is stranding other users. He then requested Igor to take 
> >> a final look but
> >> he has not reverted yet. I'll remind Michael again. BTW, can you reply 
> >> to below
> >> patch explicitly indicating your interest in the series so that MST 
> >> knows who else
> >> are the stake holders here
> >>
> >> https://lore.kernel.org/qemu-devel/[e-mail]/
> >>
> >>
> >> Hi Paolo,
> >>
> >> A request, would it be possible to skim through this series from KVM 
> >> perspective?
> >> (although nothing has changed which will affect the KVM and this is 
> >> architecture
> >> agnostic patch-set)
> >>
> >> Many thanks!
> >>
> >> Best
> >> Salil.
> >>
> >>
> >>>   regards,
> >>>   Harsh
> >>>   On 6/7/24 17:26, Salil Mehta wrote:
> >>>   > KVM vCPU creation is done once during the vCPU realization when Qemu
> >>>   > vCPU thread is spawned. This is common to all the architectures 
> >>> as of now.
> >>>   >
> >>>   > Hot-unplug of vCPU results in destruction of the vCPU object in QOM
> >>>   > but the corresponding KVM vCPU object in the Host KVM is not 
> >>> destroyed
> >>>   > as KVM doesn't support vCPU removal. Therefore, its 
> >>> representative KVM
> >>>   > vCPU object/context in Qemu is parked.
> >>>   >
> >>>   > Refactor architecture common logic so that some APIs could be reused
> >>>   > by vCPU Hotplug code of some architectures likes ARM, Loongson etc.
> >>>   > Update new/old APIs with trace events. No functional change is 
> >>> intended
> >>>   here.
> >>>   >
> >>>   > Signed-off-by: Salil Mehta <[e-mail]>
> >>>   > Reviewed-by: Gavin Shan <[e-mail]>
> >>>   > Tested-by: Vishnu Pajjuri <[e-mail]>
> >>>   > Reviewed-by: Jonathan Cameron <[e-mail]>
> >>>   > Tested-by: Xianglai Li <[e-mail]>
> >>>   > Tested-by: Miguel Luis <[e-mail]>
> >>>   > Reviewed-by: Shaoqin Huang <[e-mail]>
> >>>   > Reviewed-by: Vishnu Pajjuri <[e-mail]>
> >>>   > Reviewed-by: Nicholas Piggin <[e-mail]>
> >>>   > Tested-by: Zhao Liu <[e-mail]>
> >>>   > Reviewed-by: Zhao Liu <[e-mail]>
> >>>   > Reviewed-by: Harsh Prateek Bora <[e-mail]>
> >>>   > ---
> >>>   >   accel/kvm/kvm-all.c    | 95 
> >>> ++++++++++++++++++++++++++++------------
> >>>   --
> >>>   >   accel/kvm/kvm-cpus.h   |  1 -
> >>>   >   accel/kvm/trace-events |  5 ++-
> >>>   >   include/sysemu/kvm.h   | 25 +++++++++++
> >>>   >   4 files changed, 92 insertions(+), 34 deletions(-)
> >>>   >
> >>>   > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index
> >>>   > c0be9f5eed..8f9128bb92 100644
> >>>   > --- a/accel/kvm/kvm-all.c
> >>>   > +++ b/accel/kvm/kvm-all.c
> >>>   > @@ -340,14 +340,71 @@ err:
> >>>   >       return ret;
> >>>   >   }
> >>>   >
> >>>   > +void kvm_park_vcpu(CPUState *cpu)
> >>>   > +{
> >>>   > +    struct KVMParkedVcpu *vcpu;
> >>>   > +
> >>>   > +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(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); }
> >>>   > +
> >>>   > +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id) {
> >>>   > +    struct KVMParkedVcpu *cpu;
> >>>   > +    int kvm_fd = -ENOENT;
> >>>   > +
> >>>   > +    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
> >>>   > +        if (cpu->vcpu_id == vcpu_id) {
> >>>   > +            QLIST_REMOVE(cpu, node);
> >>>   > +            kvm_fd = cpu->kvm_fd;
> >>>   > +            g_free(cpu);
> >>>   > +        }
> >>>   > +    }
> >>>   > +
> >>>   > +    trace_kvm_unpark_vcpu(vcpu_id, kvm_fd > 0 ? "unparked" : "not
> >>>   > + found parked");
> >>>   > +
> >>>   > +    return kvm_fd;
> >>>   > +}
> >>>   > +
> >>>   > +int kvm_create_vcpu(CPUState *cpu)
> >>>   > +{
> >>>   > +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
> >>>   > +    KVMState *s = kvm_state;
> >>>   > +    int kvm_fd;
> >>>   > +
> >>>   > +    /* check if the KVM vCPU already exist but is parked */
> >>>   > +    kvm_fd = kvm_unpark_vcpu(s, vcpu_id);
> >>>   > +    if (kvm_fd < 0) {
> >>>   > +        /* vCPU not parked: create a new KVM vCPU */
> >>>   > +        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
> >>>   > +        if (kvm_fd < 0) {
> >>>   > +            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU 
> >>> %lu",
> >>>   vcpu_id);
> >>>   > +            return kvm_fd;
> >>>   > +        }
> >>>   > +    }
> >>>   > +
> >>>   > +    cpu->kvm_fd = kvm_fd;
> >>>   > +    cpu->kvm_state = s;
> >>>   > +    cpu->vcpu_dirty = true;
> >>>   > +    cpu->dirty_pages = 0;
> >>>   > +    cpu->throttle_us_per_full = 0;
> >>>   > +
> >>>   > +    trace_kvm_create_vcpu(cpu->cpu_index, vcpu_id, kvm_fd);
> >>>   > +
> >>>   > +    return 0;
> >>>   > +}
> >>>   > +
> >>>   >   static int do_kvm_destroy_vcpu(CPUState *cpu)
> >>>   >   {
> >>>   >       KVMState *s = kvm_state;
> >>>   >       long mmap_size;
> >>>   > -    struct KVMParkedVcpu *vcpu = NULL;
> >>>   >       int ret = 0;
> >>>   >
> >>>   > -    trace_kvm_destroy_vcpu();
> >>>   > +    trace_kvm_destroy_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
> >>>   >
> >>>   >       ret = kvm_arch_destroy_vcpu(cpu);
> >>>   >       if (ret < 0) {
> >>>   > @@ -373,10 +430,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;
> >>>   >   }
> >>>   > @@ -389,24 +443,6 @@ void kvm_destroy_vcpu(CPUState *cpu)
> >>>   >       }
> >>>   >   }
> >>>   >
> >>>   > -static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id) -{
> >>>   > -    struct KVMParkedVcpu *cpu;
> >>>   > -
> >>>   > -    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
> >>>   > -        if (cpu->vcpu_id == vcpu_id) {
> >>>   > -            int kvm_fd;
> >>>   > -
> >>>   > -            QLIST_REMOVE(cpu, node);
> >>>   > -            kvm_fd = cpu->kvm_fd;
> >>>   > -            g_free(cpu);
> >>>   > -            return kvm_fd;
> >>>   > -        }
> >>>   > -    }
> >>>   > -
> >>>   > -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
> >>>   > -}
> >>>   > -
> >>>   >   int kvm_init_vcpu(CPUState *cpu, Error **errp)
> >>>   >   {
> >>>   >       KVMState *s = kvm_state;
> >>>   > @@ -415,19 +451,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/accel/kvm/kvm-cpus.h b/accel/kvm/kvm-cpus.h index
> >>>   > ca40add32c..171b22fd29 100644
> >>>   > --- a/accel/kvm/kvm-cpus.h
> >>>   > +++ b/accel/kvm/kvm-cpus.h
> >>>   > @@ -22,5 +22,4 @@ bool kvm_supports_guest_debug(void);
> >>>   >   int kvm_insert_breakpoint(CPUState *cpu, int type, vaddr addr, 
> >>> vaddr
> >>>   len);
> >>>   >   int kvm_remove_breakpoint(CPUState *cpu, int type, vaddr addr, 
> >>> vaddr
> >>>   len);
> >>>   >   void kvm_remove_all_breakpoints(CPUState *cpu);
> >>>   > -
> >>>   >   #endif /* KVM_CPUS_H */
> >>>   > diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events index
> >>>   > 681ccb667d..37626c1ac5 100644
> >>>   > --- a/accel/kvm/trace-events
> >>>   > +++ b/accel/kvm/trace-events
> >>>   > @@ -9,6 +9,10 @@ kvm_device_ioctl(int fd, int type, void *arg) 
> >>> "dev fd
> >>>   %d, type 0x%x, arg %p"
> >>>   >   kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: 
> >>> Unable to
> >>>   retrieve ONEREG %" PRIu64 " from KVM: %s"
> >>>   >   kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: 
> >>> Unable to
> >>>   set ONEREG %" PRIu64 " to KVM: %s"
> >>>   >   kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: 
> >>> %d id:
> >>>   %lu"
> >>>   > +kvm_create_vcpu(int cpu_index, unsigned long arch_cpu_id, int
> >>>   kvm_fd) "index: %d, id: %lu, kvm fd: %d"
> >>>   > +kvm_destroy_vcpu(int cpu_index, unsigned long arch_cpu_id) "index:
> >>>   %d id: %lu"
> >>>   > +kvm_park_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d
> >>>   id: %lu"
> >>>   > +kvm_unpark_vcpu(unsigned long arch_cpu_id, const char *msg) "id: 
> >>> %lu
> >>>   %s"
> >>>   >   kvm_irqchip_commit_routes(void) ""
> >>>   >   kvm_irqchip_add_msi_route(char *name, int vector, int virq) 
> >>> "dev %s
> >>>   vector %d virq %d"
> >>>   >   kvm_irqchip_update_msi_route(int virq) "Updating MSI route 
> >>> virq=%d"
> >>>   > @@ -25,7 +29,6 @@ kvm_dirty_ring_reaper(const char *s) "%s"
> >>>   >   kvm_dirty_ring_reap(uint64_t count, int64_t t) "reaped 
> >>> %"PRIu64" pages
> >>>   (took %"PRIi64" us)"
> >>>   >   kvm_dirty_ring_reaper_kick(const char *reason) "%s"
> >>>   >   kvm_dirty_ring_flush(int finished) "%d"
> >>>   > -kvm_destroy_vcpu(void) ""
> >>>   >   kvm_failed_get_vcpu_mmap_size(void) ""
> >>>   >   kvm_cpu_exec(void) ""
> >>>   >   kvm_interrupt_exit_request(void) ""
> >>>   > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index
> >>>   > c31d9c7356..c4a914b3d8 100644
> >>>   > --- a/include/sysemu/kvm.h
> >>>   > +++ b/include/sysemu/kvm.h
> >>>   > @@ -313,6 +313,31 @@ int kvm_create_device(KVMState *s, uint64_t
> >>>   type, bool test);
> >>>   >    */
> >>>   >   bool kvm_device_supported(int vmfd, uint64_t type);
> >>>   >
> >>>   > +/**
> >>>   > + * kvm_create_vcpu - Gets a parked KVM vCPU or creates a KVM vCPU
> >>>   > + * @cpu: QOM CPUState object for which KVM vCPU has to be
> >>>   fetched/created.
> >>>   > + *
> >>>   > + * @returns: 0 when success, errno (<0) when failed.
> >>>   > + */
> >>>   > +int kvm_create_vcpu(CPUState *cpu);
> >>>   > +
> >>>   > +/**
> >>>   > + * kvm_park_vcpu - Park QEMU KVM vCPU context
> >>>   > + * @cpu: QOM CPUState object for which QEMU KVM vCPU context has
> >>>   to be parked.
> >>>   > + *
> >>>   > + * @returns: none
> >>>   > + */
> >>>   > +void kvm_park_vcpu(CPUState *cpu);
> >>>   > +
> >>>   > +/**
> >>>   > + * kvm_unpark_vcpu - unpark QEMU KVM vCPU context
> >>>   > + * @s: KVM State
> >>>   > + * @vcpu_id: Architecture vCPU ID of the parked vCPU
> >>>   > + *
> >>>   > + * @returns: KVM fd
> >>>   > + */
> >>>   > +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id);
> >>>   > +
> >>>   >   /* Arch specific hooks */
> >>>   >
> >>>   >   extern const KVMCapabilityInfo kvm_arch_required_capabilities[];

Igor Mammedov July 6, 2024, 1:11 p.m. UTC | #3

On Fri, 7 Jun 2024 12:56:42 +0100
Salil Mehta <[e-mail]> wrote:

> KVM vCPU creation is done once during the vCPU realization when Qemu vCPU thread
> is spawned. This is common to all the architectures as of now.
> 
> Hot-unplug of vCPU results in destruction of the vCPU object in QOM but the
> corresponding KVM vCPU object in the Host KVM is not destroyed as KVM doesn't
> support vCPU removal. Therefore, its representative KVM vCPU object/context in
> Qemu is parked.
> 
> Refactor architecture common logic so that some APIs could be reused by vCPU
> Hotplug code of some architectures likes ARM, Loongson etc. Update new/old APIs
> with trace events. No functional change is intended here.
> 
> Signed-off-by: Salil Mehta <[e-mail]>
> Reviewed-by: Gavin Shan <[e-mail]>
> Tested-by: Vishnu Pajjuri <[e-mail]>
> Reviewed-by: Jonathan Cameron <[e-mail]>
> Tested-by: Xianglai Li <[e-mail]>
> Tested-by: Miguel Luis <[e-mail]>
> Reviewed-by: Shaoqin Huang <[e-mail]>
> Reviewed-by: Vishnu Pajjuri <[e-mail]>
> Reviewed-by: Nicholas Piggin <[e-mail]>
> Tested-by: Zhao Liu <[e-mail]>
> Reviewed-by: Zhao Liu <[e-mail]>
> Reviewed-by: Harsh Prateek Bora <[e-mail]>
> ---
>  accel/kvm/kvm-all.c    | 95 ++++++++++++++++++++++++++++--------------
>  accel/kvm/kvm-cpus.h   |  1 -
>  accel/kvm/trace-events |  5 ++-
>  include/sysemu/kvm.h   | 25 +++++++++++
>  4 files changed, 92 insertions(+), 34 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index c0be9f5eed..8f9128bb92 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -340,14 +340,71 @@ err:
>      return ret;
>  }
>  
> +void kvm_park_vcpu(CPUState *cpu)
> +{
> +    struct KVMParkedVcpu *vcpu;
> +
> +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(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);
> +}
> +
> +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id)
> +{
> +    struct KVMParkedVcpu *cpu;
> +    int kvm_fd = -ENOENT;
> +
> +    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
> +        if (cpu->vcpu_id == vcpu_id) {
> +            QLIST_REMOVE(cpu, node);
> +            kvm_fd = cpu->kvm_fd;
> +            g_free(cpu);
> +        }
> +    }
> +
> +    trace_kvm_unpark_vcpu(vcpu_id, kvm_fd > 0 ? "unparked" : "not found parked");
> +
> +    return kvm_fd;
> +}
> +
> +int kvm_create_vcpu(CPUState *cpu)
> +{
> +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
> +    KVMState *s = kvm_state;
> +    int kvm_fd;
> +
> +    /* check if the KVM vCPU already exist but is parked */
> +    kvm_fd = kvm_unpark_vcpu(s, vcpu_id);
> +    if (kvm_fd < 0) {
> +        /* vCPU not parked: create a new KVM vCPU */
> +        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
> +        if (kvm_fd < 0) {
> +            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu", vcpu_id);
> +            return kvm_fd;
> +        }
> +    }
> +
> +    cpu->kvm_fd = kvm_fd;
> +    cpu->kvm_state = s;
> +    cpu->vcpu_dirty = true;
> +    cpu->dirty_pages = 0;
> +    cpu->throttle_us_per_full = 0;
> +
> +    trace_kvm_create_vcpu(cpu->cpu_index, vcpu_id, kvm_fd);
> +
> +    return 0;
> +}

Is there any reason why you are embedding/hiding kvm_state in new API
instead of passing it as argument (all callers have it defined, so why not reuse that)?

otherwise patch lgtm 

> +
>  static int do_kvm_destroy_vcpu(CPUState *cpu)
>  {
>      KVMState *s = kvm_state;
>      long mmap_size;
> -    struct KVMParkedVcpu *vcpu = NULL;
>      int ret = 0;
>  
> -    trace_kvm_destroy_vcpu();
> +    trace_kvm_destroy_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  
>      ret = kvm_arch_destroy_vcpu(cpu);
>      if (ret < 0) {
> @@ -373,10 +430,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;
>  }
> @@ -389,24 +443,6 @@ void kvm_destroy_vcpu(CPUState *cpu)
>      }
>  }
>  
> -static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
> -{
> -    struct KVMParkedVcpu *cpu;
> -
> -    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
> -        if (cpu->vcpu_id == vcpu_id) {
> -            int kvm_fd;
> -
> -            QLIST_REMOVE(cpu, node);
> -            kvm_fd = cpu->kvm_fd;
> -            g_free(cpu);
> -            return kvm_fd;
> -        }
> -    }
> -
> -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
> -}
> -
>  int kvm_init_vcpu(CPUState *cpu, Error **errp)
>  {
>      KVMState *s = kvm_state;
> @@ -415,19 +451,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/accel/kvm/kvm-cpus.h b/accel/kvm/kvm-cpus.h
> index ca40add32c..171b22fd29 100644
> --- a/accel/kvm/kvm-cpus.h
> +++ b/accel/kvm/kvm-cpus.h
> @@ -22,5 +22,4 @@ bool kvm_supports_guest_debug(void);
>  int kvm_insert_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len);
>  int kvm_remove_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len);
>  void kvm_remove_all_breakpoints(CPUState *cpu);
> -
>  #endif /* KVM_CPUS_H */
> diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
> index 681ccb667d..37626c1ac5 100644
> --- a/accel/kvm/trace-events
> +++ b/accel/kvm/trace-events
> @@ -9,6 +9,10 @@ kvm_device_ioctl(int fd, int type, void *arg) "dev fd %d, type 0x%x, arg %p"
>  kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to retrieve ONEREG %" PRIu64 " from KVM: %s"
>  kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to set ONEREG %" PRIu64 " to KVM: %s"
>  kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
> +kvm_create_vcpu(int cpu_index, unsigned long arch_cpu_id, int kvm_fd) "index: %d, id: %lu, kvm fd: %d"
> +kvm_destroy_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
> +kvm_park_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
> +kvm_unpark_vcpu(unsigned long arch_cpu_id, const char *msg) "id: %lu %s"
>  kvm_irqchip_commit_routes(void) ""
>  kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s vector %d virq %d"
>  kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d"
> @@ -25,7 +29,6 @@ kvm_dirty_ring_reaper(const char *s) "%s"
>  kvm_dirty_ring_reap(uint64_t count, int64_t t) "reaped %"PRIu64" pages (took %"PRIi64" us)"
>  kvm_dirty_ring_reaper_kick(const char *reason) "%s"
>  kvm_dirty_ring_flush(int finished) "%d"
> -kvm_destroy_vcpu(void) ""
>  kvm_failed_get_vcpu_mmap_size(void) ""
>  kvm_cpu_exec(void) ""
>  kvm_interrupt_exit_request(void) ""
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index c31d9c7356..c4a914b3d8 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -313,6 +313,31 @@ int kvm_create_device(KVMState *s, uint64_t type, bool test);
>   */
>  bool kvm_device_supported(int vmfd, uint64_t type);
>  
> +/**
> + * kvm_create_vcpu - Gets a parked KVM vCPU or creates a KVM vCPU
> + * @cpu: QOM CPUState object for which KVM vCPU has to be fetched/created.
> + *
> + * @returns: 0 when success, errno (<0) when failed.
> + */
> +int kvm_create_vcpu(CPUState *cpu);
> +
> +/**
> + * kvm_park_vcpu - Park QEMU KVM vCPU context
> + * @cpu: QOM CPUState object for which QEMU KVM vCPU context has to be parked.
> + *
> + * @returns: none
> + */
> +void kvm_park_vcpu(CPUState *cpu);
> +
> +/**
> + * kvm_unpark_vcpu - unpark QEMU KVM vCPU context
> + * @s: KVM State
> + * @vcpu_id: Architecture vCPU ID of the parked vCPU
> + *
> + * @returns: KVM fd
> + */
> +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id);
> +
>  /* Arch specific hooks */
>  
>  extern const KVMCapabilityInfo kvm_arch_required_capabilities[];

Salil Mehta July 6, 2024, 3:43 p.m. UTC | #4

Hi Igor,
Thanks for taking out time to review.

On Sat, Jul 6, 2024 at 1:12 PM Igor Mammedov <[e-mail]> wrote:

> On Fri, 7 Jun 2024 12:56:42 +0100
> Salil Mehta <[e-mail]> wrote:
>
> > KVM vCPU creation is done once during the vCPU realization when Qemu
> vCPU thread
> > is spawned. This is common to all the architectures as of now.
> >
> > Hot-unplug of vCPU results in destruction of the vCPU object in QOM but
> the
> > corresponding KVM vCPU object in the Host KVM is not destroyed as KVM
> doesn't
> > support vCPU removal. Therefore, its representative KVM vCPU
> object/context in
> > Qemu is parked.
> >
> > Refactor architecture common logic so that some APIs could be reused by
> vCPU
> > Hotplug code of some architectures likes ARM, Loongson etc. Update
> new/old APIs
> > with trace events. No functional change is intended here.
> >
> > Signed-off-by: Salil Mehta <[e-mail]>
> > Reviewed-by: Gavin Shan <[e-mail]>
> > Tested-by: Vishnu Pajjuri <[e-mail]>
> > Reviewed-by: Jonathan Cameron <[e-mail]>
> > Tested-by: Xianglai Li <[e-mail]>
> > Tested-by: Miguel Luis <[e-mail]>
> > Reviewed-by: Shaoqin Huang <[e-mail]>
> > Reviewed-by: Vishnu Pajjuri <[e-mail]>
> > Reviewed-by: Nicholas Piggin <[e-mail]>
> > Tested-by: Zhao Liu <[e-mail]>
> > Reviewed-by: Zhao Liu <[e-mail]>
> > Reviewed-by: Harsh Prateek Bora <[e-mail]>
> > ---
> >  accel/kvm/kvm-all.c    | 95 ++++++++++++++++++++++++++++--------------
> >  accel/kvm/kvm-cpus.h   |  1 -
> >  accel/kvm/trace-events |  5 ++-
> >  include/sysemu/kvm.h   | 25 +++++++++++
> >  4 files changed, 92 insertions(+), 34 deletions(-)
> >
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index c0be9f5eed..8f9128bb92 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -340,14 +340,71 @@ err:
> >      return ret;
> >  }
> >
> > +void kvm_park_vcpu(CPUState *cpu)
> > +{
> > +    struct KVMParkedVcpu *vcpu;
> > +
> > +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(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);
> > +}
> > +
> > +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id)
> > +{
> > +    struct KVMParkedVcpu *cpu;
> > +    int kvm_fd = -ENOENT;
> > +
> > +    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
> > +        if (cpu->vcpu_id == vcpu_id) {
> > +            QLIST_REMOVE(cpu, node);
> > +            kvm_fd = cpu->kvm_fd;
> > +            g_free(cpu);
> > +        }
> > +    }
> > +
> > +    trace_kvm_unpark_vcpu(vcpu_id, kvm_fd > 0 ? "unparked" : "not found
> parked");
> > +
> > +    return kvm_fd;
> > +}
> > +
> > +int kvm_create_vcpu(CPUState *cpu)
> > +{
> > +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
> > +    KVMState *s = kvm_state;
> > +    int kvm_fd;
> > +
> > +    /* check if the KVM vCPU already exist but is parked */
> > +    kvm_fd = kvm_unpark_vcpu(s, vcpu_id);
> > +    if (kvm_fd < 0) {
> > +        /* vCPU not parked: create a new KVM vCPU */
> > +        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
> > +        if (kvm_fd < 0) {
> > +            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu",
> vcpu_id);
> > +            return kvm_fd;
> > +        }
> > +    }
> > +
> > +    cpu->kvm_fd = kvm_fd;
> > +    cpu->kvm_state = s;
> > +    cpu->vcpu_dirty = true;
> > +    cpu->dirty_pages = 0;
> > +    cpu->throttle_us_per_full = 0;
> > +
> > +    trace_kvm_create_vcpu(cpu->cpu_index, vcpu_id, kvm_fd);
> > +
> > +    return 0;
> > +}
>
> Is there any reason why you are embedding/hiding kvm_state in new API
> instead of passing it as argument (all callers have it defined, so why not
> reuse that)?
>

It is a global variable and I don't think it is a usual practice to specify
the global variable
as an input parameter.



>
> otherwise patch lgtm


May I request your Reviewed-by for this patch?

Thanks
Salil.


>
>
> > +
> >  static int do_kvm_destroy_vcpu(CPUState *cpu)
> >  {
> >      KVMState *s = kvm_state;
> >      long mmap_size;
> > -    struct KVMParkedVcpu *vcpu = NULL;
> >      int ret = 0;
> >
> > -    trace_kvm_destroy_vcpu();
> > +    trace_kvm_destroy_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
> >
> >      ret = kvm_arch_destroy_vcpu(cpu);
> >      if (ret < 0) {
> > @@ -373,10 +430,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;
> >  }
> > @@ -389,24 +443,6 @@ void kvm_destroy_vcpu(CPUState *cpu)
> >      }
> >  }
> >
> > -static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
> > -{
> > -    struct KVMParkedVcpu *cpu;
> > -
> > -    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
> > -        if (cpu->vcpu_id == vcpu_id) {
> > -            int kvm_fd;
> > -
> > -            QLIST_REMOVE(cpu, node);
> > -            kvm_fd = cpu->kvm_fd;
> > -            g_free(cpu);
> > -            return kvm_fd;
> > -        }
> > -    }
> > -
> > -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
> > -}
> > -
> >  int kvm_init_vcpu(CPUState *cpu, Error **errp)
> >  {
> >      KVMState *s = kvm_state;
> > @@ -415,19 +451,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/accel/kvm/kvm-cpus.h b/accel/kvm/kvm-cpus.h
> > index ca40add32c..171b22fd29 100644
> > --- a/accel/kvm/kvm-cpus.h
> > +++ b/accel/kvm/kvm-cpus.h
> > @@ -22,5 +22,4 @@ bool kvm_supports_guest_debug(void);
> >  int kvm_insert_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr
> len);
> >  int kvm_remove_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr
> len);
> >  void kvm_remove_all_breakpoints(CPUState *cpu);
> > -
> >  #endif /* KVM_CPUS_H */
> > diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
> > index 681ccb667d..37626c1ac5 100644
> > --- a/accel/kvm/trace-events
> > +++ b/accel/kvm/trace-events
> > @@ -9,6 +9,10 @@ kvm_device_ioctl(int fd, int type, void *arg) "dev fd
> %d, type 0x%x, arg %p"
> >  kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to
> retrieve ONEREG %" PRIu64 " from KVM: %s"
> >  kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to
> set ONEREG %" PRIu64 " to KVM: %s"
> >  kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id:
> %lu"
> > +kvm_create_vcpu(int cpu_index, unsigned long arch_cpu_id, int kvm_fd)
> "index: %d, id: %lu, kvm fd: %d"
> > +kvm_destroy_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d
> id: %lu"
> > +kvm_park_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id:
> %lu"
> > +kvm_unpark_vcpu(unsigned long arch_cpu_id, const char *msg) "id: %lu %s"
> >  kvm_irqchip_commit_routes(void) ""
> >  kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s
> vector %d virq %d"
> >  kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d"
> > @@ -25,7 +29,6 @@ kvm_dirty_ring_reaper(const char *s) "%s"
> >  kvm_dirty_ring_reap(uint64_t count, int64_t t) "reaped %"PRIu64" pages
> (took %"PRIi64" us)"
> >  kvm_dirty_ring_reaper_kick(const char *reason) "%s"
> >  kvm_dirty_ring_flush(int finished) "%d"
> > -kvm_destroy_vcpu(void) ""
> >  kvm_failed_get_vcpu_mmap_size(void) ""
> >  kvm_cpu_exec(void) ""
> >  kvm_interrupt_exit_request(void) ""
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index c31d9c7356..c4a914b3d8 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -313,6 +313,31 @@ int kvm_create_device(KVMState *s, uint64_t type,
> bool test);
> >   */
> >  bool kvm_device_supported(int vmfd, uint64_t type);
> >
> > +/**
> > + * kvm_create_vcpu - Gets a parked KVM vCPU or creates a KVM vCPU
> > + * @cpu: QOM CPUState object for which KVM vCPU has to be
> fetched/created.
> > + *
> > + * @returns: 0 when success, errno (<0) when failed.
> > + */
> > +int kvm_create_vcpu(CPUState *cpu);
> > +
> > +/**
> > + * kvm_park_vcpu - Park QEMU KVM vCPU context
> > + * @cpu: QOM CPUState object for which QEMU KVM vCPU context has to be
> parked.
> > + *
> > + * @returns: none
> > + */
> > +void kvm_park_vcpu(CPUState *cpu);
> > +
> > +/**
> > + * kvm_unpark_vcpu - unpark QEMU KVM vCPU context
> > + * @s: KVM State
> > + * @vcpu_id: Architecture vCPU ID of the parked vCPU
> > + *
> > + * @returns: KVM fd
> > + */
> > +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id);
> > +
> >  /* Arch specific hooks */
> >
> >  extern const KVMCapabilityInfo kvm_arch_required_capabilities[];
>
>

Igor Mammedov July 8, 2024, 1:32 p.m. UTC | #5

On Sat, 6 Jul 2024 15:43:01 +0000
Salil Mehta <[e-mail]> wrote:

> Hi Igor,
> Thanks for taking out time to review.
> 
> On Sat, Jul 6, 2024 at 1:12 PM Igor Mammedov <[e-mail]> wrote:
> 
> > On Fri, 7 Jun 2024 12:56:42 +0100
> > Salil Mehta <[e-mail]> wrote:
> >  
> > > KVM vCPU creation is done once during the vCPU realization when Qemu  
> > vCPU thread  
> > > is spawned. This is common to all the architectures as of now.
> > >
> > > Hot-unplug of vCPU results in destruction of the vCPU object in QOM but  
> > the  
> > > corresponding KVM vCPU object in the Host KVM is not destroyed as KVM  
> > doesn't  
> > > support vCPU removal. Therefore, its representative KVM vCPU  
> > object/context in  
> > > Qemu is parked.
> > >
> > > Refactor architecture common logic so that some APIs could be reused by  
> > vCPU  
> > > Hotplug code of some architectures likes ARM, Loongson etc. Update  
> > new/old APIs  
> > > with trace events. No functional change is intended here.
> > >
> > > Signed-off-by: Salil Mehta <[e-mail]>
> > > Reviewed-by: Gavin Shan <[e-mail]>
> > > Tested-by: Vishnu Pajjuri <[e-mail]>
> > > Reviewed-by: Jonathan Cameron <[e-mail]>
> > > Tested-by: Xianglai Li <[e-mail]>
> > > Tested-by: Miguel Luis <[e-mail]>
> > > Reviewed-by: Shaoqin Huang <[e-mail]>
> > > Reviewed-by: Vishnu Pajjuri <[e-mail]>
> > > Reviewed-by: Nicholas Piggin <[e-mail]>
> > > Tested-by: Zhao Liu <[e-mail]>
> > > Reviewed-by: Zhao Liu <[e-mail]>
> > > Reviewed-by: Harsh Prateek Bora <[e-mail]>
> > > ---
> > >  accel/kvm/kvm-all.c    | 95 ++++++++++++++++++++++++++++--------------
> > >  accel/kvm/kvm-cpus.h   |  1 -
> > >  accel/kvm/trace-events |  5 ++-
> > >  include/sysemu/kvm.h   | 25 +++++++++++
> > >  4 files changed, 92 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > > index c0be9f5eed..8f9128bb92 100644
> > > --- a/accel/kvm/kvm-all.c
> > > +++ b/accel/kvm/kvm-all.c
> > > @@ -340,14 +340,71 @@ err:
> > >      return ret;
> > >  }
> > >
> > > +void kvm_park_vcpu(CPUState *cpu)
> > > +{
> > > +    struct KVMParkedVcpu *vcpu;
> > > +
> > > +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(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);
> > > +}
> > > +
> > > +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id)
> > > +{
> > > +    struct KVMParkedVcpu *cpu;
> > > +    int kvm_fd = -ENOENT;
> > > +
> > > +    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
> > > +        if (cpu->vcpu_id == vcpu_id) {
> > > +            QLIST_REMOVE(cpu, node);
> > > +            kvm_fd = cpu->kvm_fd;
> > > +            g_free(cpu);
> > > +        }
> > > +    }
> > > +
> > > +    trace_kvm_unpark_vcpu(vcpu_id, kvm_fd > 0 ? "unparked" : "not found  
> > parked");  
> > > +
> > > +    return kvm_fd;
> > > +}
> > > +
> > > +int kvm_create_vcpu(CPUState *cpu)
> > > +{
> > > +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
> > > +    KVMState *s = kvm_state;
> > > +    int kvm_fd;
> > > +
> > > +    /* check if the KVM vCPU already exist but is parked */
> > > +    kvm_fd = kvm_unpark_vcpu(s, vcpu_id);
> > > +    if (kvm_fd < 0) {
> > > +        /* vCPU not parked: create a new KVM vCPU */
> > > +        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
> > > +        if (kvm_fd < 0) {
> > > +            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu",  
> > vcpu_id);  
> > > +            return kvm_fd;
> > > +        }
> > > +    }
> > > +
> > > +    cpu->kvm_fd = kvm_fd;
> > > +    cpu->kvm_state = s;
> > > +    cpu->vcpu_dirty = true;
> > > +    cpu->dirty_pages = 0;
> > > +    cpu->throttle_us_per_full = 0;
> > > +
> > > +    trace_kvm_create_vcpu(cpu->cpu_index, vcpu_id, kvm_fd);
> > > +
> > > +    return 0;
> > > +}  
> >
> > Is there any reason why you are embedding/hiding kvm_state in new API
> > instead of passing it as argument (all callers have it defined, so why not
> > reuse that)?
> >  
> 
> It is a global variable and I don't think it is a usual practice to specify
> the global variable
> as an input parameter.

Ideally, global would be accessed once at API boundary entry
and the passed as an argument to functions it calls.
It makes it easier to follow as opposed to mixed access we have now,
which is harder to review since one has to check both
flavors (argument passed or directly accessed).

in this patch  kvm_init_vcpu() calls new kvm_create_vcpu()
and the former caches these global into 's' local variable,
so I'd reuse that local variable like kvm_get_vcpu() you are removing here did.

> 
> 
> 
> >
> > otherwise patch lgtm  
> 
> 
> May I request your Reviewed-by for this patch?
> 
> Thanks
> Salil.
> 
> 
> >
> >  
> > > +
> > >  static int do_kvm_destroy_vcpu(CPUState *cpu)
> > >  {
> > >      KVMState *s = kvm_state;
> > >      long mmap_size;
> > > -    struct KVMParkedVcpu *vcpu = NULL;
> > >      int ret = 0;
> > >
> > > -    trace_kvm_destroy_vcpu();
> > > +    trace_kvm_destroy_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
> > >
> > >      ret = kvm_arch_destroy_vcpu(cpu);
> > >      if (ret < 0) {
> > > @@ -373,10 +430,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;
> > >  }
> > > @@ -389,24 +443,6 @@ void kvm_destroy_vcpu(CPUState *cpu)
> > >      }
> > >  }
> > >
> > > -static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
> > > -{
> > > -    struct KVMParkedVcpu *cpu;
> > > -
> > > -    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
> > > -        if (cpu->vcpu_id == vcpu_id) {
> > > -            int kvm_fd;
> > > -
> > > -            QLIST_REMOVE(cpu, node);
> > > -            kvm_fd = cpu->kvm_fd;
> > > -            g_free(cpu);
> > > -            return kvm_fd;
> > > -        }
> > > -    }
> > > -
> > > -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
> > > -}
> > > -
> > >  int kvm_init_vcpu(CPUState *cpu, Error **errp)
> > >  {
> > >      KVMState *s = kvm_state;
> > > @@ -415,19 +451,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/accel/kvm/kvm-cpus.h b/accel/kvm/kvm-cpus.h
> > > index ca40add32c..171b22fd29 100644
> > > --- a/accel/kvm/kvm-cpus.h
> > > +++ b/accel/kvm/kvm-cpus.h
> > > @@ -22,5 +22,4 @@ bool kvm_supports_guest_debug(void);
> > >  int kvm_insert_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr  
> > len);  
> > >  int kvm_remove_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr  
> > len);  
> > >  void kvm_remove_all_breakpoints(CPUState *cpu);
> > > -
> > >  #endif /* KVM_CPUS_H */
> > > diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
> > > index 681ccb667d..37626c1ac5 100644
> > > --- a/accel/kvm/trace-events
> > > +++ b/accel/kvm/trace-events
> > > @@ -9,6 +9,10 @@ kvm_device_ioctl(int fd, int type, void *arg) "dev fd  
> > %d, type 0x%x, arg %p"  
> > >  kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to  
> > retrieve ONEREG %" PRIu64 " from KVM: %s"  
> > >  kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to  
> > set ONEREG %" PRIu64 " to KVM: %s"  
> > >  kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id:  
> > %lu"  
> > > +kvm_create_vcpu(int cpu_index, unsigned long arch_cpu_id, int kvm_fd)  
> > "index: %d, id: %lu, kvm fd: %d"  
> > > +kvm_destroy_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d  
> > id: %lu"  
> > > +kvm_park_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id:  
> > %lu"  
> > > +kvm_unpark_vcpu(unsigned long arch_cpu_id, const char *msg) "id: %lu %s"
> > >  kvm_irqchip_commit_routes(void) ""
> > >  kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s  
> > vector %d virq %d"  
> > >  kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d"
> > > @@ -25,7 +29,6 @@ kvm_dirty_ring_reaper(const char *s) "%s"
> > >  kvm_dirty_ring_reap(uint64_t count, int64_t t) "reaped %"PRIu64" pages  
> > (took %"PRIi64" us)"  
> > >  kvm_dirty_ring_reaper_kick(const char *reason) "%s"
> > >  kvm_dirty_ring_flush(int finished) "%d"
> > > -kvm_destroy_vcpu(void) ""
> > >  kvm_failed_get_vcpu_mmap_size(void) ""
> > >  kvm_cpu_exec(void) ""
> > >  kvm_interrupt_exit_request(void) ""
> > > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > > index c31d9c7356..c4a914b3d8 100644
> > > --- a/include/sysemu/kvm.h
> > > +++ b/include/sysemu/kvm.h
> > > @@ -313,6 +313,31 @@ int kvm_create_device(KVMState *s, uint64_t type,  
> > bool test);  
> > >   */
> > >  bool kvm_device_supported(int vmfd, uint64_t type);
> > >
> > > +/**
> > > + * kvm_create_vcpu - Gets a parked KVM vCPU or creates a KVM vCPU
> > > + * @cpu: QOM CPUState object for which KVM vCPU has to be  
> > fetched/created.  
> > > + *
> > > + * @returns: 0 when success, errno (<0) when failed.
> > > + */
> > > +int kvm_create_vcpu(CPUState *cpu);
> > > +
> > > +/**
> > > + * kvm_park_vcpu - Park QEMU KVM vCPU context
> > > + * @cpu: QOM CPUState object for which QEMU KVM vCPU context has to be  
> > parked.  
> > > + *
> > > + * @returns: none
> > > + */
> > > +void kvm_park_vcpu(CPUState *cpu);
> > > +
> > > +/**
> > > + * kvm_unpark_vcpu - unpark QEMU KVM vCPU context
> > > + * @s: KVM State
> > > + * @vcpu_id: Architecture vCPU ID of the parked vCPU
> > > + *
> > > + * @returns: KVM fd
> > > + */
> > > +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id);
> > > +
> > >  /* Arch specific hooks */
> > >
> > >  extern const KVMCapabilityInfo kvm_arch_required_capabilities[];  
> >
> >

Salil Mehta July 8, 2024, 11:30 p.m. UTC | #6

Hi Igor,

On 08/07/2024 13:32, Igor Mammedov wrote:
> On Sat, 6 Jul 2024 15:43:01 +0000
> Salil Mehta <[e-mail]> wrote:
>
>> Hi Igor,
>> Thanks for taking out time to review.
>>
>> On Sat, Jul 6, 2024 at 1:12 PM Igor Mammedov <[e-mail]> wrote:
>>
>>> On Fri, 7 Jun 2024 12:56:42 +0100
>>> Salil Mehta <[e-mail]> wrote:
>>>   
>>>> KVM vCPU creation is done once during the vCPU realization when Qemu
>>> vCPU thread
>>>> is spawned. This is common to all the architectures as of now.
>>>>
>>>> Hot-unplug of vCPU results in destruction of the vCPU object in QOM but
>>> the
>>>> corresponding KVM vCPU object in the Host KVM is not destroyed as KVM
>>> doesn't
>>>> support vCPU removal. Therefore, its representative KVM vCPU
>>> object/context in
>>>> Qemu is parked.
>>>>
>>>> Refactor architecture common logic so that some APIs could be reused by
>>> vCPU
>>>> Hotplug code of some architectures likes ARM, Loongson etc. Update
>>> new/old APIs
>>>> with trace events. No functional change is intended here.
>>>>
>>>> Signed-off-by: Salil Mehta <[e-mail]>
>>>> Reviewed-by: Gavin Shan <[e-mail]>
>>>> Tested-by: Vishnu Pajjuri <[e-mail]>
>>>> Reviewed-by: Jonathan Cameron <[e-mail]>
>>>> Tested-by: Xianglai Li <[e-mail]>
>>>> Tested-by: Miguel Luis <[e-mail]>
>>>> Reviewed-by: Shaoqin Huang <[e-mail]>
>>>> Reviewed-by: Vishnu Pajjuri <[e-mail]>
>>>> Reviewed-by: Nicholas Piggin <[e-mail]>
>>>> Tested-by: Zhao Liu <[e-mail]>
>>>> Reviewed-by: Zhao Liu <[e-mail]>
>>>> Reviewed-by: Harsh Prateek Bora <[e-mail]>
>>>> ---
>>>>   accel/kvm/kvm-all.c    | 95 ++++++++++++++++++++++++++++--------------
>>>>   accel/kvm/kvm-cpus.h   |  1 -
>>>>   accel/kvm/trace-events |  5 ++-
>>>>   include/sysemu/kvm.h   | 25 +++++++++++
>>>>   4 files changed, 92 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>>> index c0be9f5eed..8f9128bb92 100644
>>>> --- a/accel/kvm/kvm-all.c
>>>> +++ b/accel/kvm/kvm-all.c
>>>> @@ -340,14 +340,71 @@ err:
>>>>       return ret;
>>>>   }
>>>>
>>>> +void kvm_park_vcpu(CPUState *cpu)
>>>> +{
>>>> +    struct KVMParkedVcpu *vcpu;
>>>> +
>>>> +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(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);
>>>> +}
>>>> +
>>>> +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id)
>>>> +{
>>>> +    struct KVMParkedVcpu *cpu;
>>>> +    int kvm_fd = -ENOENT;
>>>> +
>>>> +    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
>>>> +        if (cpu->vcpu_id == vcpu_id) {
>>>> +            QLIST_REMOVE(cpu, node);
>>>> +            kvm_fd = cpu->kvm_fd;
>>>> +            g_free(cpu);
>>>> +        }
>>>> +    }
>>>> +
>>>> +    trace_kvm_unpark_vcpu(vcpu_id, kvm_fd > 0 ? "unparked" : "not found
>>> parked");
>>>> +
>>>> +    return kvm_fd;
>>>> +}
>>>> +
>>>> +int kvm_create_vcpu(CPUState *cpu)
>>>> +{
>>>> +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
>>>> +    KVMState *s = kvm_state;
>>>> +    int kvm_fd;
>>>> +
>>>> +    /* check if the KVM vCPU already exist but is parked */
>>>> +    kvm_fd = kvm_unpark_vcpu(s, vcpu_id);
>>>> +    if (kvm_fd < 0) {
>>>> +        /* vCPU not parked: create a new KVM vCPU */
>>>> +        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
>>>> +        if (kvm_fd < 0) {
>>>> +            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu",
>>> vcpu_id);
>>>> +            return kvm_fd;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    cpu->kvm_fd = kvm_fd;
>>>> +    cpu->kvm_state = s;
>>>> +    cpu->vcpu_dirty = true;
>>>> +    cpu->dirty_pages = 0;
>>>> +    cpu->throttle_us_per_full = 0;
>>>> +
>>>> +    trace_kvm_create_vcpu(cpu->cpu_index, vcpu_id, kvm_fd);
>>>> +
>>>> +    return 0;
>>>> +}
>>> Is there any reason why you are embedding/hiding kvm_state in new API
>>> instead of passing it as argument (all callers have it defined, so why not
>>> reuse that)?
>>>   
>> It is a global variable and I don't think it is a usual practice to specify
>> the global variable
>> as an input parameter.
> Ideally, global would be accessed once at API boundary entry
> and the passed as an argument to functions it calls.
> It makes it easier to follow as opposed to mixed access we have now,
> which is harder to review since one has to check both
> flavors (argument passed or directly accessed).
>
> in this patch  kvm_init_vcpu() calls new kvm_create_vcpu()
> and the former caches these global into 's' local variable,
> so I'd reuse that local variable like kvm_get_vcpu() you are removing here did.
That is one perspective, but:
1. kvm_create_vcpu() will also be called externally from other contexts.
    It would be awkward to pass this variable from those non-local places
    where it would seem unnecessary.
2. If you look at other symmetrical functions like kvm_destroy_vcpu(),
    they also have a similar prototype.

I think it is about doing a slight trade-off. If you really believe this
change is necessary for us to proceed, I will make the adjustment.
However, please note that it will affect the IBM team as well.

Hi Nick,

I hope you are okay with this suggested change?

Thanks,
Salil
>>> otherwise patch lgtm
>>
>> May I request your Reviewed-by for this patch?
>>
>> Thanks
>> Salil.
>>
>>
>>>   
>>>> +
>>>>   static int do_kvm_destroy_vcpu(CPUState *cpu)
>>>>   {
>>>>       KVMState *s = kvm_state;
>>>>       long mmap_size;
>>>> -    struct KVMParkedVcpu *vcpu = NULL;
>>>>       int ret = 0;
>>>>
>>>> -    trace_kvm_destroy_vcpu();
>>>> +    trace_kvm_destroy_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>>>>
>>>>       ret = kvm_arch_destroy_vcpu(cpu);
>>>>       if (ret < 0) {
>>>> @@ -373,10 +430,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;
>>>>   }
>>>> @@ -389,24 +443,6 @@ void kvm_destroy_vcpu(CPUState *cpu)
>>>>       }
>>>>   }
>>>>
>>>> -static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
>>>> -{
>>>> -    struct KVMParkedVcpu *cpu;
>>>> -
>>>> -    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
>>>> -        if (cpu->vcpu_id == vcpu_id) {
>>>> -            int kvm_fd;
>>>> -
>>>> -            QLIST_REMOVE(cpu, node);
>>>> -            kvm_fd = cpu->kvm_fd;
>>>> -            g_free(cpu);
>>>> -            return kvm_fd;
>>>> -        }
>>>> -    }
>>>> -
>>>> -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
>>>> -}
>>>> -
>>>>   int kvm_init_vcpu(CPUState *cpu, Error **errp)
>>>>   {
>>>>       KVMState *s = kvm_state;
>>>> @@ -415,19 +451,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/accel/kvm/kvm-cpus.h b/accel/kvm/kvm-cpus.h
>>>> index ca40add32c..171b22fd29 100644
>>>> --- a/accel/kvm/kvm-cpus.h
>>>> +++ b/accel/kvm/kvm-cpus.h
>>>> @@ -22,5 +22,4 @@ bool kvm_supports_guest_debug(void);
>>>>   int kvm_insert_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr
>>> len);
>>>>   int kvm_remove_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr
>>> len);
>>>>   void kvm_remove_all_breakpoints(CPUState *cpu);
>>>> -
>>>>   #endif /* KVM_CPUS_H */
>>>> diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
>>>> index 681ccb667d..37626c1ac5 100644
>>>> --- a/accel/kvm/trace-events
>>>> +++ b/accel/kvm/trace-events
>>>> @@ -9,6 +9,10 @@ kvm_device_ioctl(int fd, int type, void *arg) "dev fd
>>> %d, type 0x%x, arg %p"
>>>>   kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to
>>> retrieve ONEREG %" PRIu64 " from KVM: %s"
>>>>   kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to
>>> set ONEREG %" PRIu64 " to KVM: %s"
>>>>   kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id:
>>> %lu"
>>>> +kvm_create_vcpu(int cpu_index, unsigned long arch_cpu_id, int kvm_fd)
>>> "index: %d, id: %lu, kvm fd: %d"
>>>> +kvm_destroy_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d
>>> id: %lu"
>>>> +kvm_park_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id:
>>> %lu"
>>>> +kvm_unpark_vcpu(unsigned long arch_cpu_id, const char *msg) "id: %lu %s"
>>>>   kvm_irqchip_commit_routes(void) ""
>>>>   kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s
>>> vector %d virq %d"
>>>>   kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d"
>>>> @@ -25,7 +29,6 @@ kvm_dirty_ring_reaper(const char *s) "%s"
>>>>   kvm_dirty_ring_reap(uint64_t count, int64_t t) "reaped %"PRIu64" pages
>>> (took %"PRIi64" us)"
>>>>   kvm_dirty_ring_reaper_kick(const char *reason) "%s"
>>>>   kvm_dirty_ring_flush(int finished) "%d"
>>>> -kvm_destroy_vcpu(void) ""
>>>>   kvm_failed_get_vcpu_mmap_size(void) ""
>>>>   kvm_cpu_exec(void) ""
>>>>   kvm_interrupt_exit_request(void) ""
>>>> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
>>>> index c31d9c7356..c4a914b3d8 100644
>>>> --- a/include/sysemu/kvm.h
>>>> +++ b/include/sysemu/kvm.h
>>>> @@ -313,6 +313,31 @@ int kvm_create_device(KVMState *s, uint64_t type,
>>> bool test);
>>>>    */
>>>>   bool kvm_device_supported(int vmfd, uint64_t type);
>>>>
>>>> +/**
>>>> + * kvm_create_vcpu - Gets a parked KVM vCPU or creates a KVM vCPU
>>>> + * @cpu: QOM CPUState object for which KVM vCPU has to be
>>> fetched/created.
>>>> + *
>>>> + * @returns: 0 when success, errno (<0) when failed.
>>>> + */
>>>> +int kvm_create_vcpu(CPUState *cpu);
>>>> +
>>>> +/**
>>>> + * kvm_park_vcpu - Park QEMU KVM vCPU context
>>>> + * @cpu: QOM CPUState object for which QEMU KVM vCPU context has to be
>>> parked.
>>>> + *
>>>> + * @returns: none
>>>> + */
>>>> +void kvm_park_vcpu(CPUState *cpu);
>>>> +
>>>> +/**
>>>> + * kvm_unpark_vcpu - unpark QEMU KVM vCPU context
>>>> + * @s: KVM State
>>>> + * @vcpu_id: Architecture vCPU ID of the parked vCPU
>>>> + *
>>>> + * @returns: KVM fd
>>>> + */
>>>> +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id);
>>>> +
>>>>   /* Arch specific hooks */
>>>>
>>>>   extern const KVMCapabilityInfo kvm_arch_required_capabilities[];
>>>

Igor Mammedov July 9, 2024, 8:06 a.m. UTC | #7

On Mon, 8 Jul 2024 23:30:01 +0000
Salil Mehta <[e-mail]> wrote:

> Hi Igor,
> 
> On 08/07/2024 13:32, Igor Mammedov wrote:
> > On Sat, 6 Jul 2024 15:43:01 +0000
> > Salil Mehta <[e-mail]> wrote:
> >  
> >> Hi Igor,
> >> Thanks for taking out time to review.
> >>
> >> On Sat, Jul 6, 2024 at 1:12 PM Igor Mammedov <[e-mail]> wrote:
> >>  
> >>> On Fri, 7 Jun 2024 12:56:42 +0100
> >>> Salil Mehta <[e-mail]> wrote:
> >>>     
> >>>> KVM vCPU creation is done once during the vCPU realization when Qemu  
> >>> vCPU thread  
> >>>> is spawned. This is common to all the architectures as of now.
> >>>>
> >>>> Hot-unplug of vCPU results in destruction of the vCPU object in QOM but  
> >>> the  
> >>>> corresponding KVM vCPU object in the Host KVM is not destroyed as KVM  
> >>> doesn't  
> >>>> support vCPU removal. Therefore, its representative KVM vCPU  
> >>> object/context in  
> >>>> Qemu is parked.
> >>>>
> >>>> Refactor architecture common logic so that some APIs could be reused by  
> >>> vCPU  
> >>>> Hotplug code of some architectures likes ARM, Loongson etc. Update  
> >>> new/old APIs  
> >>>> with trace events. No functional change is intended here.
> >>>>
> >>>> Signed-off-by: Salil Mehta <[e-mail]>
> >>>> Reviewed-by: Gavin Shan <[e-mail]>
> >>>> Tested-by: Vishnu Pajjuri <[e-mail]>
> >>>> Reviewed-by: Jonathan Cameron <[e-mail]>
> >>>> Tested-by: Xianglai Li <[e-mail]>
> >>>> Tested-by: Miguel Luis <[e-mail]>
> >>>> Reviewed-by: Shaoqin Huang <[e-mail]>
> >>>> Reviewed-by: Vishnu Pajjuri <[e-mail]>
> >>>> Reviewed-by: Nicholas Piggin <[e-mail]>
> >>>> Tested-by: Zhao Liu <[e-mail]>
> >>>> Reviewed-by: Zhao Liu <[e-mail]>
> >>>> Reviewed-by: Harsh Prateek Bora <[e-mail]>
> >>>> ---
> >>>>   accel/kvm/kvm-all.c    | 95 ++++++++++++++++++++++++++++--------------
> >>>>   accel/kvm/kvm-cpus.h   |  1 -
> >>>>   accel/kvm/trace-events |  5 ++-
> >>>>   include/sysemu/kvm.h   | 25 +++++++++++
> >>>>   4 files changed, 92 insertions(+), 34 deletions(-)
> >>>>
> >>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> >>>> index c0be9f5eed..8f9128bb92 100644
> >>>> --- a/accel/kvm/kvm-all.c
> >>>> +++ b/accel/kvm/kvm-all.c
> >>>> @@ -340,14 +340,71 @@ err:
> >>>>       return ret;
> >>>>   }
> >>>>
> >>>> +void kvm_park_vcpu(CPUState *cpu)
> >>>> +{
> >>>> +    struct KVMParkedVcpu *vcpu;
> >>>> +
> >>>> +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(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);
> >>>> +}
> >>>> +
> >>>> +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id)
> >>>> +{
> >>>> +    struct KVMParkedVcpu *cpu;
> >>>> +    int kvm_fd = -ENOENT;
> >>>> +
> >>>> +    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
> >>>> +        if (cpu->vcpu_id == vcpu_id) {
> >>>> +            QLIST_REMOVE(cpu, node);
> >>>> +            kvm_fd = cpu->kvm_fd;
> >>>> +            g_free(cpu);
> >>>> +        }
> >>>> +    }
> >>>> +
> >>>> +    trace_kvm_unpark_vcpu(vcpu_id, kvm_fd > 0 ? "unparked" : "not found  
> >>> parked");  
> >>>> +
> >>>> +    return kvm_fd;
> >>>> +}
> >>>> +
> >>>> +int kvm_create_vcpu(CPUState *cpu)
> >>>> +{
> >>>> +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
> >>>> +    KVMState *s = kvm_state;
> >>>> +    int kvm_fd;
> >>>> +
> >>>> +    /* check if the KVM vCPU already exist but is parked */
> >>>> +    kvm_fd = kvm_unpark_vcpu(s, vcpu_id);
> >>>> +    if (kvm_fd < 0) {
> >>>> +        /* vCPU not parked: create a new KVM vCPU */
> >>>> +        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
> >>>> +        if (kvm_fd < 0) {
> >>>> +            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu",  
> >>> vcpu_id);  
> >>>> +            return kvm_fd;
> >>>> +        }
> >>>> +    }
> >>>> +
> >>>> +    cpu->kvm_fd = kvm_fd;
> >>>> +    cpu->kvm_state = s;
> >>>> +    cpu->vcpu_dirty = true;
> >>>> +    cpu->dirty_pages = 0;
> >>>> +    cpu->throttle_us_per_full = 0;
> >>>> +
> >>>> +    trace_kvm_create_vcpu(cpu->cpu_index, vcpu_id, kvm_fd);
> >>>> +
> >>>> +    return 0;
> >>>> +}  
> >>> Is there any reason why you are embedding/hiding kvm_state in new API
> >>> instead of passing it as argument (all callers have it defined, so why not
> >>> reuse that)?
> >>>     
> >> It is a global variable and I don't think it is a usual practice to specify
> >> the global variable
> >> as an input parameter.  
> > Ideally, global would be accessed once at API boundary entry
> > and the passed as an argument to functions it calls.
> > It makes it easier to follow as opposed to mixed access we have now,
> > which is harder to review since one has to check both
> > flavors (argument passed or directly accessed).
> >
> > in this patch  kvm_init_vcpu() calls new kvm_create_vcpu()
> > and the former caches these global into 's' local variable,
> > so I'd reuse that local variable like kvm_get_vcpu() you are removing here did.  
> That is one perspective, but:
> 1. kvm_create_vcpu() will also be called externally from other contexts.

I'm fine with this as is, it would be better to mention in commit message
that this (others) new API will be called externally. Otherwise it's not
clear why KVMState is hidden inside.

>     It would be awkward to pass this variable from those non-local places
>     where it would seem unnecessary.
> 2. If you look at other symmetrical functions like kvm_destroy_vcpu(),
>     they also have a similar prototype.
> 
> I think it is about doing a slight trade-off. If you really believe this
> change is necessary for us to proceed, I will make the adjustment.
> However, please note that it will affect the IBM team as well.
> 
> Hi Nick,
> 
> I hope you are okay with this suggested change?
> 
> Thanks,
> Salil
> >>> otherwise patch lgtm  
> >>
> >> May I request your Reviewed-by for this patch?
> >>
> >> Thanks
> >> Salil.
> >>
> >>  
> >>>     
> >>>> +
> >>>>   static int do_kvm_destroy_vcpu(CPUState *cpu)
> >>>>   {
> >>>>       KVMState *s = kvm_state;
> >>>>       long mmap_size;
> >>>> -    struct KVMParkedVcpu *vcpu = NULL;
> >>>>       int ret = 0;
> >>>>
> >>>> -    trace_kvm_destroy_vcpu();
> >>>> +    trace_kvm_destroy_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
> >>>>
> >>>>       ret = kvm_arch_destroy_vcpu(cpu);
> >>>>       if (ret < 0) {
> >>>> @@ -373,10 +430,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;
> >>>>   }
> >>>> @@ -389,24 +443,6 @@ void kvm_destroy_vcpu(CPUState *cpu)
> >>>>       }
> >>>>   }
> >>>>
> >>>> -static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
> >>>> -{
> >>>> -    struct KVMParkedVcpu *cpu;
> >>>> -
> >>>> -    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
> >>>> -        if (cpu->vcpu_id == vcpu_id) {
> >>>> -            int kvm_fd;
> >>>> -
> >>>> -            QLIST_REMOVE(cpu, node);
> >>>> -            kvm_fd = cpu->kvm_fd;
> >>>> -            g_free(cpu);
> >>>> -            return kvm_fd;
> >>>> -        }
> >>>> -    }
> >>>> -
> >>>> -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
> >>>> -}
> >>>> -
> >>>>   int kvm_init_vcpu(CPUState *cpu, Error **errp)
> >>>>   {
> >>>>       KVMState *s = kvm_state;
> >>>> @@ -415,19 +451,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/accel/kvm/kvm-cpus.h b/accel/kvm/kvm-cpus.h
> >>>> index ca40add32c..171b22fd29 100644
> >>>> --- a/accel/kvm/kvm-cpus.h
> >>>> +++ b/accel/kvm/kvm-cpus.h
> >>>> @@ -22,5 +22,4 @@ bool kvm_supports_guest_debug(void);
> >>>>   int kvm_insert_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr  
> >>> len);  
> >>>>   int kvm_remove_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr  
> >>> len);  
> >>>>   void kvm_remove_all_breakpoints(CPUState *cpu);
> >>>> -
> >>>>   #endif /* KVM_CPUS_H */
> >>>> diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
> >>>> index 681ccb667d..37626c1ac5 100644
> >>>> --- a/accel/kvm/trace-events
> >>>> +++ b/accel/kvm/trace-events
> >>>> @@ -9,6 +9,10 @@ kvm_device_ioctl(int fd, int type, void *arg) "dev fd  
> >>> %d, type 0x%x, arg %p"  
> >>>>   kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to  
> >>> retrieve ONEREG %" PRIu64 " from KVM: %s"  
> >>>>   kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to  
> >>> set ONEREG %" PRIu64 " to KVM: %s"  
> >>>>   kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id:  
> >>> %lu"  
> >>>> +kvm_create_vcpu(int cpu_index, unsigned long arch_cpu_id, int kvm_fd)  
> >>> "index: %d, id: %lu, kvm fd: %d"  
> >>>> +kvm_destroy_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d  
> >>> id: %lu"  
> >>>> +kvm_park_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id:  
> >>> %lu"  
> >>>> +kvm_unpark_vcpu(unsigned long arch_cpu_id, const char *msg) "id: %lu %s"
> >>>>   kvm_irqchip_commit_routes(void) ""
> >>>>   kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s  
> >>> vector %d virq %d"  
> >>>>   kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d"
> >>>> @@ -25,7 +29,6 @@ kvm_dirty_ring_reaper(const char *s) "%s"
> >>>>   kvm_dirty_ring_reap(uint64_t count, int64_t t) "reaped %"PRIu64" pages  
> >>> (took %"PRIi64" us)"  
> >>>>   kvm_dirty_ring_reaper_kick(const char *reason) "%s"
> >>>>   kvm_dirty_ring_flush(int finished) "%d"
> >>>> -kvm_destroy_vcpu(void) ""
> >>>>   kvm_failed_get_vcpu_mmap_size(void) ""
> >>>>   kvm_cpu_exec(void) ""
> >>>>   kvm_interrupt_exit_request(void) ""
> >>>> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> >>>> index c31d9c7356..c4a914b3d8 100644
> >>>> --- a/include/sysemu/kvm.h
> >>>> +++ b/include/sysemu/kvm.h
> >>>> @@ -313,6 +313,31 @@ int kvm_create_device(KVMState *s, uint64_t type,  
> >>> bool test);  
> >>>>    */
> >>>>   bool kvm_device_supported(int vmfd, uint64_t type);
> >>>>
> >>>> +/**
> >>>> + * kvm_create_vcpu - Gets a parked KVM vCPU or creates a KVM vCPU
> >>>> + * @cpu: QOM CPUState object for which KVM vCPU has to be  
> >>> fetched/created.  
> >>>> + *
> >>>> + * @returns: 0 when success, errno (<0) when failed.
> >>>> + */
> >>>> +int kvm_create_vcpu(CPUState *cpu);
> >>>> +
> >>>> +/**
> >>>> + * kvm_park_vcpu - Park QEMU KVM vCPU context
> >>>> + * @cpu: QOM CPUState object for which QEMU KVM vCPU context has to be  
> >>> parked.  
> >>>> + *
> >>>> + * @returns: none
> >>>> + */
> >>>> +void kvm_park_vcpu(CPUState *cpu);
> >>>> +
> >>>> +/**
> >>>> + * kvm_unpark_vcpu - unpark QEMU KVM vCPU context
> >>>> + * @s: KVM State
> >>>> + * @vcpu_id: Architecture vCPU ID of the parked vCPU
> >>>> + *
> >>>> + * @returns: KVM fd
> >>>> + */
> >>>> +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id);
> >>>> +
> >>>>   /* Arch specific hooks */
> >>>>
> >>>>   extern const KVMCapabilityInfo kvm_arch_required_capabilities[];  
> >>>     
>

Salil Mehta July 9, 2024, 10:18 a.m. UTC | #8

>  From: Igor Mammedov <[e-mail]>
>  Sent: Tuesday, July 9, 2024 9:06 AM
>  To: Salil Mehta <[e-mail]>
>  
>  On Mon, 8 Jul 2024 23:30:01 +0000
>  Salil Mehta <[e-mail]> wrote:
>  
>  > Hi Igor,
>  >
>  > On 08/07/2024 13:32, Igor Mammedov wrote:
>  > > On Sat, 6 Jul 2024 15:43:01 +0000
>  > > Salil Mehta <[e-mail]> wrote:
>  > >
>  > >> Hi Igor,
>  > >> Thanks for taking out time to review.
>  > >>
>  > >> On Sat, Jul 6, 2024 at 1:12 PM Igor Mammedov
>  <[e-mail]> wrote:
>  > >>
>  > >>> On Fri, 7 Jun 2024 12:56:42 +0100
>  > >>> Salil Mehta <[e-mail]> wrote:
>  > >>>
>  > >>>> KVM vCPU creation is done once during the vCPU realization when
>  > >>>> Qemu
>  > >>> vCPU thread
>  > >>>> is spawned. This is common to all the architectures as of now.
>  > >>>>
>  > >>>> Hot-unplug of vCPU results in destruction of the vCPU object in
>  > >>>> QOM but
>  > >>> the
>  > >>>> corresponding KVM vCPU object in the Host KVM is not destroyed as
>  > >>>> KVM
>  > >>> doesn't
>  > >>>> support vCPU removal. Therefore, its representative KVM vCPU
>  > >>> object/context in
>  > >>>> Qemu is parked.
>  > >>>>
>  > >>>> Refactor architecture common logic so that some APIs could be
>  > >>>> reused by
>  > >>> vCPU
>  > >>>> Hotplug code of some architectures likes ARM, Loongson etc.
>  > >>>> Update
>  > >>> new/old APIs
>  > >>>> with trace events. No functional change is intended here.
>  > >>>>
>  > >>>> Signed-off-by: Salil Mehta <[e-mail]>
>  > >>>> Reviewed-by: Gavin Shan <[e-mail]>
>  > >>>> Tested-by: Vishnu Pajjuri <[e-mail]>
>  > >>>> Reviewed-by: Jonathan Cameron <[e-mail]>
>  > >>>> Tested-by: Xianglai Li <[e-mail]>
>  > >>>> Tested-by: Miguel Luis <[e-mail]>
>  > >>>> Reviewed-by: Shaoqin Huang <[e-mail]>
>  > >>>> Reviewed-by: Vishnu Pajjuri <[e-mail]>
>  > >>>> Reviewed-by: Nicholas Piggin <[e-mail]>
>  > >>>> Tested-by: Zhao Liu <[e-mail]>
>  > >>>> Reviewed-by: Zhao Liu <[e-mail]>
>  > >>>> Reviewed-by: Harsh Prateek Bora <[e-mail]>
>  > >>>> ---
>  > >>>>   accel/kvm/kvm-all.c    | 95 ++++++++++++++++++++++++++++-----
>  ---------
>  > >>>>   accel/kvm/kvm-cpus.h   |  1 -
>  > >>>>   accel/kvm/trace-events |  5 ++-
>  > >>>>   include/sysemu/kvm.h   | 25 +++++++++++
>  > >>>>   4 files changed, 92 insertions(+), 34 deletions(-)
>  > >>>>
>  > >>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index
>  > >>>> c0be9f5eed..8f9128bb92 100644
>  > >>>> --- a/accel/kvm/kvm-all.c
>  > >>>> +++ b/accel/kvm/kvm-all.c
>  > >>>> @@ -340,14 +340,71 @@ err:
>  > >>>>       return ret;
>  > >>>>   }
>  > >>>>
>  > >>>> +void kvm_park_vcpu(CPUState *cpu) {
>  > >>>> +    struct KVMParkedVcpu *vcpu;
>  > >>>> +
>  > >>>> +    trace_kvm_park_vcpu(cpu->cpu_index,
>  kvm_arch_vcpu_id(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);
>  > >>>> +}
>  > >>>> +
>  > >>>> +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id) {
>  > >>>> +    struct KVMParkedVcpu *cpu;
>  > >>>> +    int kvm_fd = -ENOENT;
>  > >>>> +
>  > >>>> +    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
>  > >>>> +        if (cpu->vcpu_id == vcpu_id) {
>  > >>>> +            QLIST_REMOVE(cpu, node);
>  > >>>> +            kvm_fd = cpu->kvm_fd;
>  > >>>> +            g_free(cpu);
>  > >>>> +        }
>  > >>>> +    }
>  > >>>> +
>  > >>>> +    trace_kvm_unpark_vcpu(vcpu_id, kvm_fd > 0 ? "unparked" :
>  > >>>> + "not found
>  > >>> parked");
>  > >>>> +
>  > >>>> +    return kvm_fd;
>  > >>>> +}
>  > >>>> +
>  > >>>> +int kvm_create_vcpu(CPUState *cpu) {
>  > >>>> +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
>  > >>>> +    KVMState *s = kvm_state;
>  > >>>> +    int kvm_fd;
>  > >>>> +
>  > >>>> +    /* check if the KVM vCPU already exist but is parked */
>  > >>>> +    kvm_fd = kvm_unpark_vcpu(s, vcpu_id);
>  > >>>> +    if (kvm_fd < 0) {
>  > >>>> +        /* vCPU not parked: create a new KVM vCPU */
>  > >>>> +        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
>  > >>>> +        if (kvm_fd < 0) {
>  > >>>> +            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU
>  > >>>> + %lu",
>  > >>> vcpu_id);
>  > >>>> +            return kvm_fd;
>  > >>>> +        }
>  > >>>> +    }
>  > >>>> +
>  > >>>> +    cpu->kvm_fd = kvm_fd;
>  > >>>> +    cpu->kvm_state = s;
>  > >>>> +    cpu->vcpu_dirty = true;
>  > >>>> +    cpu->dirty_pages = 0;
>  > >>>> +    cpu->throttle_us_per_full = 0;
>  > >>>> +
>  > >>>> +    trace_kvm_create_vcpu(cpu->cpu_index, vcpu_id, kvm_fd);
>  > >>>> +
>  > >>>> +    return 0;
>  > >>>> +}
>  > >>> Is there any reason why you are embedding/hiding kvm_state in new
>  > >>> API instead of passing it as argument (all callers have it
>  > >>> defined, so why not reuse that)?
>  > >>>
>  > >> It is a global variable and I don't think it is a usual practice to
>  > >> specify the global variable as an input parameter.
>  > > Ideally, global would be accessed once at API boundary entry and the
>  > > passed as an argument to functions it calls.
>  > > It makes it easier to follow as opposed to mixed access we have now,
>  > > which is harder to review since one has to check both flavors
>  > > (argument passed or directly accessed).
>  > >
>  > > in this patch  kvm_init_vcpu() calls new kvm_create_vcpu() and the
>  > > former caches these global into 's' local variable, so I'd reuse
>  > > that local variable like kvm_get_vcpu() you are removing here did.
>  > That is one perspective, but:
>  > 1. kvm_create_vcpu() will also be called externally from other contexts.
>  
>  I'm fine with this as is, it would be better to mention in commit message
>  that this (others) new API will be called externally. Otherwise it's not clear
>  why KVMState is hidden inside.

ok, thanks.

Best regards
Salil.

>  >     It would be awkward to pass this variable from those non-local
>  > places
>  >     where it would seem unnecessary.
>  > 2. If you look at other symmetrical functions like kvm_destroy_vcpu(),
>  >     they also have a similar prototype.
>  >
>  > I think it is about doing a slight trade-off. If you really believe
>  > this change is necessary for us to proceed, I will make the adjustment.
>  > However, please note that it will affect the IBM team as well.
>  >
>  > Hi Nick,
>  >
>  > I hope you are okay with this suggested change?
>  >
>  > Thanks,
>  > Salil
>  > >>> otherwise patch lgtm
>  > >>
>  > >> May I request your Reviewed-by for this patch?
>  > >>
>  > >> Thanks
>  > >> Salil.
>  > >>
>  > >>
>  > >>>
>  > >>>> +
>  > >>>>   static int do_kvm_destroy_vcpu(CPUState *cpu)
>  > >>>>   {
>  > >>>>       KVMState *s = kvm_state;
>  > >>>>       long mmap_size;
>  > >>>> -    struct KVMParkedVcpu *vcpu = NULL;
>  > >>>>       int ret = 0;
>  > >>>>
>  > >>>> -    trace_kvm_destroy_vcpu();
>  > >>>> +    trace_kvm_destroy_vcpu(cpu->cpu_index,
>  > >>>> + kvm_arch_vcpu_id(cpu));
>  > >>>>
>  > >>>>       ret = kvm_arch_destroy_vcpu(cpu);
>  > >>>>       if (ret < 0) {
>  > >>>> @@ -373,10 +430,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;
>  > >>>>   }
>  > >>>> @@ -389,24 +443,6 @@ void kvm_destroy_vcpu(CPUState *cpu)
>  > >>>>       }
>  > >>>>   }
>  > >>>>
>  > >>>> -static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id) -{
>  > >>>> -    struct KVMParkedVcpu *cpu;
>  > >>>> -
>  > >>>> -    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
>  > >>>> -        if (cpu->vcpu_id == vcpu_id) {
>  > >>>> -            int kvm_fd;
>  > >>>> -
>  > >>>> -            QLIST_REMOVE(cpu, node);
>  > >>>> -            kvm_fd = cpu->kvm_fd;
>  > >>>> -            g_free(cpu);
>  > >>>> -            return kvm_fd;
>  > >>>> -        }
>  > >>>> -    }
>  > >>>> -
>  > >>>> -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
>  > >>>> -}
>  > >>>> -
>  > >>>>   int kvm_init_vcpu(CPUState *cpu, Error **errp)
>  > >>>>   {
>  > >>>>       KVMState *s = kvm_state;
>  > >>>> @@ -415,19 +451,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/accel/kvm/kvm-cpus.h b/accel/kvm/kvm-cpus.h index
>  > >>>> ca40add32c..171b22fd29 100644
>  > >>>> --- a/accel/kvm/kvm-cpus.h
>  > >>>> +++ b/accel/kvm/kvm-cpus.h
>  > >>>> @@ -22,5 +22,4 @@ bool kvm_supports_guest_debug(void);
>  > >>>>   int kvm_insert_breakpoint(CPUState *cpu, int type, vaddr addr,
>  > >>>> vaddr
>  > >>> len);
>  > >>>>   int kvm_remove_breakpoint(CPUState *cpu, int type, vaddr addr,
>  > >>>> vaddr
>  > >>> len);
>  > >>>>   void kvm_remove_all_breakpoints(CPUState *cpu);
>  > >>>> -
>  > >>>>   #endif /* KVM_CPUS_H */
>  > >>>> diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
>  > >>>> index 681ccb667d..37626c1ac5 100644
>  > >>>> --- a/accel/kvm/trace-events
>  > >>>> +++ b/accel/kvm/trace-events
>  > >>>> @@ -9,6 +9,10 @@ kvm_device_ioctl(int fd, int type, void *arg)
>  > >>>> "dev fd
>  > >>> %d, type 0x%x, arg %p"
>  > >>>>   kvm_failed_reg_get(uint64_t id, const char *msg) "Warning:
>  > >>>> Unable to
>  > >>> retrieve ONEREG %" PRIu64 " from KVM: %s"
>  > >>>>   kvm_failed_reg_set(uint64_t id, const char *msg) "Warning:
>  > >>>> Unable to
>  > >>> set ONEREG %" PRIu64 " to KVM: %s"
>  > >>>>   kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index:
>  %d id:
>  > >>> %lu"
>  > >>>> +kvm_create_vcpu(int cpu_index, unsigned long arch_cpu_id, int
>  > >>>> +kvm_fd)
>  > >>> "index: %d, id: %lu, kvm fd: %d"
>  > >>>> +kvm_destroy_vcpu(int cpu_index, unsigned long arch_cpu_id)
>  > >>>> +"index: %d
>  > >>> id: %lu"
>  > >>>> +kvm_park_vcpu(int cpu_index, unsigned long arch_cpu_id) "index:
>  %d id:
>  > >>> %lu"
>  > >>>> +kvm_unpark_vcpu(unsigned long arch_cpu_id, const char *msg)
>  "id: %lu %s"
>  > >>>>   kvm_irqchip_commit_routes(void) ""
>  > >>>>   kvm_irqchip_add_msi_route(char *name, int vector, int virq)
>  > >>>> "dev %s
>  > >>> vector %d virq %d"
>  > >>>>   kvm_irqchip_update_msi_route(int virq) "Updating MSI route
>  virq=%d"
>  > >>>> @@ -25,7 +29,6 @@ kvm_dirty_ring_reaper(const char *s) "%s"
>  > >>>>   kvm_dirty_ring_reap(uint64_t count, int64_t t) "reaped
>  > >>>> %"PRIu64" pages
>  > >>> (took %"PRIi64" us)"
>  > >>>>   kvm_dirty_ring_reaper_kick(const char *reason) "%s"
>  > >>>>   kvm_dirty_ring_flush(int finished) "%d"
>  > >>>> -kvm_destroy_vcpu(void) ""
>  > >>>>   kvm_failed_get_vcpu_mmap_size(void) ""
>  > >>>>   kvm_cpu_exec(void) ""
>  > >>>>   kvm_interrupt_exit_request(void) ""
>  > >>>> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index
>  > >>>> c31d9c7356..c4a914b3d8 100644
>  > >>>> --- a/include/sysemu/kvm.h
>  > >>>> +++ b/include/sysemu/kvm.h
>  > >>>> @@ -313,6 +313,31 @@ int kvm_create_device(KVMState *s,
>  uint64_t
>  > >>>> type,
>  > >>> bool test);
>  > >>>>    */
>  > >>>>   bool kvm_device_supported(int vmfd, uint64_t type);
>  > >>>>
>  > >>>> +/**
>  > >>>> + * kvm_create_vcpu - Gets a parked KVM vCPU or creates a KVM
>  > >>>> +vCPU
>  > >>>> + * @cpu: QOM CPUState object for which KVM vCPU has to be
>  > >>> fetched/created.
>  > >>>> + *
>  > >>>> + * @returns: 0 when success, errno (<0) when failed.
>  > >>>> + */
>  > >>>> +int kvm_create_vcpu(CPUState *cpu);
>  > >>>> +
>  > >>>> +/**
>  > >>>> + * kvm_park_vcpu - Park QEMU KVM vCPU context
>  > >>>> + * @cpu: QOM CPUState object for which QEMU KVM vCPU
>  context has
>  > >>>> +to be
>  > >>> parked.
>  > >>>> + *
>  > >>>> + * @returns: none
>  > >>>> + */
>  > >>>> +void kvm_park_vcpu(CPUState *cpu);
>  > >>>> +
>  > >>>> +/**
>  > >>>> + * kvm_unpark_vcpu - unpark QEMU KVM vCPU context
>  > >>>> + * @s: KVM State
>  > >>>> + * @vcpu_id: Architecture vCPU ID of the parked vCPU
>  > >>>> + *
>  > >>>> + * @returns: KVM fd
>  > >>>> + */
>  > >>>> +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id);
>  > >>>> +
>  > >>>>   /* Arch specific hooks */
>  > >>>>
>  > >>>>   extern const KVMCapabilityInfo
>  > >>>> kvm_arch_required_capabilities[];
>  > >>>
>  >
>

Patch

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index c0be9f5eed..8f9128bb92 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -340,14 +340,71 @@  err:
     return ret;
 }
 
+void kvm_park_vcpu(CPUState *cpu)
+{
+    struct KVMParkedVcpu *vcpu;
+
+    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(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);
+}
+
+int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id)
+{
+    struct KVMParkedVcpu *cpu;
+    int kvm_fd = -ENOENT;
+
+    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
+        if (cpu->vcpu_id == vcpu_id) {
+            QLIST_REMOVE(cpu, node);
+            kvm_fd = cpu->kvm_fd;
+            g_free(cpu);
+        }
+    }
+
+    trace_kvm_unpark_vcpu(vcpu_id, kvm_fd > 0 ? "unparked" : "not found parked");
+
+    return kvm_fd;
+}
+
+int kvm_create_vcpu(CPUState *cpu)
+{
+    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
+    KVMState *s = kvm_state;
+    int kvm_fd;
+
+    /* check if the KVM vCPU already exist but is parked */
+    kvm_fd = kvm_unpark_vcpu(s, vcpu_id);
+    if (kvm_fd < 0) {
+        /* vCPU not parked: create a new KVM vCPU */
+        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
+        if (kvm_fd < 0) {
+            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu", vcpu_id);
+            return kvm_fd;
+        }
+    }
+
+    cpu->kvm_fd = kvm_fd;
+    cpu->kvm_state = s;
+    cpu->vcpu_dirty = true;
+    cpu->dirty_pages = 0;
+    cpu->throttle_us_per_full = 0;
+
+    trace_kvm_create_vcpu(cpu->cpu_index, vcpu_id, kvm_fd);
+
+    return 0;
+}
+
 static int do_kvm_destroy_vcpu(CPUState *cpu)
 {
     KVMState *s = kvm_state;
     long mmap_size;
-    struct KVMParkedVcpu *vcpu = NULL;
     int ret = 0;
 
-    trace_kvm_destroy_vcpu();
+    trace_kvm_destroy_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
 
     ret = kvm_arch_destroy_vcpu(cpu);
     if (ret < 0) {
@@ -373,10 +430,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;
 }
@@ -389,24 +443,6 @@  void kvm_destroy_vcpu(CPUState *cpu)
     }
 }
 
-static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
-{
-    struct KVMParkedVcpu *cpu;
-
-    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
-        if (cpu->vcpu_id == vcpu_id) {
-            int kvm_fd;
-
-            QLIST_REMOVE(cpu, node);
-            kvm_fd = cpu->kvm_fd;
-            g_free(cpu);
-            return kvm_fd;
-        }
-    }
-
-    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
-}
-
 int kvm_init_vcpu(CPUState *cpu, Error **errp)
 {
     KVMState *s = kvm_state;
@@ -415,19 +451,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/accel/kvm/kvm-cpus.h b/accel/kvm/kvm-cpus.h
index ca40add32c..171b22fd29 100644
--- a/accel/kvm/kvm-cpus.h
+++ b/accel/kvm/kvm-cpus.h
@@ -22,5 +22,4 @@  bool kvm_supports_guest_debug(void);
 int kvm_insert_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len);
 int kvm_remove_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len);
 void kvm_remove_all_breakpoints(CPUState *cpu);
-
 #endif /* KVM_CPUS_H */
diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
index 681ccb667d..37626c1ac5 100644
--- a/accel/kvm/trace-events
+++ b/accel/kvm/trace-events
@@ -9,6 +9,10 @@  kvm_device_ioctl(int fd, int type, void *arg) "dev fd %d, type 0x%x, arg %p"
 kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to retrieve ONEREG %" PRIu64 " from KVM: %s"
 kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to set ONEREG %" PRIu64 " to KVM: %s"
 kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
+kvm_create_vcpu(int cpu_index, unsigned long arch_cpu_id, int kvm_fd) "index: %d, id: %lu, kvm fd: %d"
+kvm_destroy_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
+kvm_park_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
+kvm_unpark_vcpu(unsigned long arch_cpu_id, const char *msg) "id: %lu %s"
 kvm_irqchip_commit_routes(void) ""
 kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s vector %d virq %d"
 kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d"
@@ -25,7 +29,6 @@  kvm_dirty_ring_reaper(const char *s) "%s"
 kvm_dirty_ring_reap(uint64_t count, int64_t t) "reaped %"PRIu64" pages (took %"PRIi64" us)"
 kvm_dirty_ring_reaper_kick(const char *reason) "%s"
 kvm_dirty_ring_flush(int finished) "%d"
-kvm_destroy_vcpu(void) ""
 kvm_failed_get_vcpu_mmap_size(void) ""
 kvm_cpu_exec(void) ""
 kvm_interrupt_exit_request(void) ""
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index c31d9c7356..c4a914b3d8 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -313,6 +313,31 @@  int kvm_create_device(KVMState *s, uint64_t type, bool test);
  */
 bool kvm_device_supported(int vmfd, uint64_t type);
 
+/**
+ * kvm_create_vcpu - Gets a parked KVM vCPU or creates a KVM vCPU
+ * @cpu: QOM CPUState object for which KVM vCPU has to be fetched/created.
+ *
+ * @returns: 0 when success, errno (<0) when failed.
+ */
+int kvm_create_vcpu(CPUState *cpu);
+
+/**
+ * kvm_park_vcpu - Park QEMU KVM vCPU context
+ * @cpu: QOM CPUState object for which QEMU KVM vCPU context has to be parked.
+ *
+ * @returns: none
+ */
+void kvm_park_vcpu(CPUState *cpu);
+
+/**
+ * kvm_unpark_vcpu - unpark QEMU KVM vCPU context
+ * @s: KVM State
+ * @vcpu_id: Architecture vCPU ID of the parked vCPU
+ *
+ * @returns: KVM fd
+ */
+int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id);
+
 /* Arch specific hooks */
 
 extern const KVMCapabilityInfo kvm_arch_required_capabilities[];