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

[Salil Mehta]

Hi Vishnu,

>  From: Vishnu Pajjuri <vishnu@amperemail.onmicrosoft.com> 
>  Sent: Thursday, April 4, 2024 3:00 PM
>  Subject: Re: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU 
> {creation,parking} code
>  
>  Hi Salil,
>>  On 12-03-2024 07:29, 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 instead of DPRINTF. No functional change is intended here.
>>  
>>  Signed-off-by: Salil Mehta mailto:salil.mehta@huawei.com
>>  Reviewed-by: Gavin Shan mailto:gshan@redhat.com
>>  Tested-by: Vishnu Pajjuri mailto:vishnu@os.amperecomputing.com
>>  Reviewed-by: Jonathan Cameron mailto:Jonathan.Cameron@huawei.com
>>  Tested-by: Xianglai Li mailto:lixianglai@loongson.cn
>>  Tested-by: Miguel Luis mailto:miguel.luis@oracle.com
>>  Reviewed-by: Shaoqin Huang mailto:shahuang@redhat.com
>>  ---
>>   accel/kvm/kvm-all.c    | 64 ++++++++++++++++++++++++++++++++----------
>>   accel/kvm/trace-events |  5 +++-
>>   include/sysemu/kvm.h   | 16 +++++++++++
>>   3 files changed, 69 insertions(+), 16 deletions(-)
>>  
>>  diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>  index a8cecd040e..3bc3207bda 100644
>>  --- a/accel/kvm/kvm-all.c
>>  +++ b/accel/kvm/kvm-all.c
>>  @@ -126,6 +126,7 @@ static QemuMutex kml_slots_lock;
>>   #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
>>   
>>   static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
>>  +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
>>   
>>   static inline void kvm_resample_fd_remove(int gsi)
>>   {
>>  @@ -314,14 +315,53 @@ err:
>>  return ret;
>>   }
>>   
>>  +void kvm_park_vcpu(CPUState *cpu)
>>  +{
>>  +    struct KVMParkedVcpu *vcpu;
>>  +
>>  +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  It's good if we add kvm_fd to trace.
>  It will be useful to cross verify kvm_get_vcpu()'s kvm_fd with parked vcpu.


Agreed. But this is currently called in context to create and destroy vCPU
where the trace already exists with the info you are seeking. Having
trace here might duplicate the info and end up increasing the noise.

Let me know if you think otherwise or have something else to add.

Thanks

 
>>  +
>>  +    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_create_vcpu(CPUState *cpu)
>>  +{
>>  +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
>>  +    KVMState *s = kvm_state;
>>  +    int kvm_fd;
>>  +
>>  +    trace_kvm_create_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  vcpu_id can be used instead of kvm_arch_vcpu_id(cpu).


KVM arch VCPU Id ensures that ID being traced is meaningful for that
architecture. The way CPU ID gets calculated in on different architectures
could be different. Hence, its value might be quite different.

  
>>  +
>>  +    /* check if the KVM vCPU already exist but is parked */
>>  +    kvm_fd = kvm_get_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;
>>  +
>>  +    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) {
>>  @@ -347,10 +387,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;
>>   }
>>  @@ -371,6 +408,8 @@ static int kvm_get_vcpu(KVMState *s, unsigned long 
>> vcpu_id)
>>  >    if (cpu->vcpu_id == vcpu_id) {
>>  >        int kvm_fd;
>>   
>>  +>       trace_kvm_get_vcpu(vcpu_id);
>  It's good if we add kvm_fd to trace.
>  It will be useful to cross verify kvm_get_vcpu's kvm_fd with parked vcpu.


I can but I'm wondering why you've raised this? Perhaps, I'm not aware of the
interface you are using to configure the VMs and how traces across diferent
VMs get reflected. Please help in my understanding.


>>  +
>>  >        QLIST_REMOVE(cpu, node);
>>  >        kvm_fd = cpu->kvm_fd;
>>  >        g_free(cpu);
>>  @@ -378,7 +417,7 @@ static int kvm_get_vcpu(KVMState *s, unsigned long 
>> vcpu_id)
>>  >    }
>>       }
>>   
>>  -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
>>  +    return -ENOENT;
>>   }
>>   
>>   int kvm_init_vcpu(CPUState *cpu, Error **errp)
>>  @@ -389,19 +428,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/trace-events b/accel/kvm/trace-events
>>  index a25902597b..5558cff0dc 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) "index: %d id: 
>> %lu"
>>  +kvm_get_vcpu(unsigned long arch_cpu_id) "id: %lu"
>>  +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_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 fad9a7e8ff..2ed928aa71 100644
>>  --- a/include/sysemu/kvm.h
>>  +++ b/include/sysemu/kvm.h
>>  @@ -435,6 +435,22 @@ void kvm_set_sigmask_len(KVMState *s, unsigned int 
>> sigmask_len);
>>   int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
>>  >  >  >  >  >       hwaddr *phys_addr);
>>   
>>  +/**
>>  + * 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);
>>  +
>>   #endif /* NEED_CPU_H */
>>   
>>   void kvm_cpu_synchronize_state(CPUState *cpu);
>  Otherwise, Looks good to me.  Feel free to add
>  Reviewed-by: "Vishnu Pajjuri" mailto:vishnu@os.amperecomputing.com
>  Thanks,

Thanks.
Salil



>  -Vishnu