RE: [PATCH V3 01/10] accel/kvm: Extract common KVM vCPU {creation,parking} code

[Salil Mehta]

> From: David Hildenbrand <david@redhat.com>
> Sent: Monday, October 9, 2023 3:11 PM
> To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org; qemu-
> arm@nongnu.org
> Cc: maz@kernel.org; jean-philippe@linaro.org; Jonathan Cameron
> <jonathan.cameron@huawei.com>; lpieralisi@kernel.org;
> peter.maydell@linaro.org; richard.henderson@linaro.org;
> imammedo@redhat.com; andrew.jones@linux.dev; philmd@linaro.org;
> eric.auger@redhat.com; oliver.upton@linux.dev; pbonzini@redhat.com;
> mst@redhat.com; will@kernel.org; gshan@redhat.com; rafael@kernel.org;
> alex.bennee@linaro.org; linux@armlinux.org.uk;
> darren@os.amperecomputing.com; ilkka@os.amperecomputing.com;
> vishnu@os.amperecomputing.com; karl.heubaum@oracle.com;
> miguel.luis@oracle.com; salil.mehta@opnsrc.net; zhukeqian
> <zhukeqian1@huawei.com>; wangxiongfeng (C) <wangxiongfeng2@huawei.com>;
> wangyanan (Y) <wangyanan55@huawei.com>; jiakernel2@gmail.com;
> maobibo@loongson.cn; lixianglai@loongson.cn; Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH V3 01/10] accel/kvm: Extract common KVM vCPU
> {creation,parking} code
> 
> On 09.10.23 15:42, Salil Mehta wrote:
> > Hi David,
> > Thanks for the review.
> >
> >> From: David Hildenbrand <david@redhat.com>
> >> Sent: Monday, October 9, 2023 1:21 PM
> >> To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org; 
> >> qemu-arm@nongnu.org
> >> Cc: maz@kernel.org; jean-philippe@linaro.org; Jonathan Cameron
> >> <jonathan.cameron@huawei.com>; lpieralisi@kernel.org;
> >> peter.maydell@linaro.org; richard.henderson@linaro.org;
> >> imammedo@redhat.com; andrew.jones@linux.dev; philmd@linaro.org;
> >> eric.auger@redhat.com; oliver.upton@linux.dev; pbonzini@redhat.com;
> >> mst@redhat.com; will@kernel.org; gshan@redhat.com; rafael@kernel.org;
> >> alex.bennee@linaro.org; linux@armlinux.org.uk;
> >> darren@os.amperecomputing.com; ilkka@os.amperecomputing.com;
> >> vishnu@os.amperecomputing.com; karl.heubaum@oracle.com;
> >> miguel.luis@oracle.com; salil.mehta@opnsrc.net; zhukeqian
> >> <zhukeqian1@huawei.com>; wangxiongfeng (C) <wangxiongfeng2@huawei.com>;
> >> wangyanan (Y) <wangyanan55@huawei.com>; jiakernel2@gmail.com;
> >> maobibo@loongson.cn; lixianglai@loongson.cn; Linuxarm
> <linuxarm@huawei.com>
> >> Subject: Re: [PATCH V3 01/10] accel/kvm: Extract common KVM vCPU
> >> {creation,parking} code
> >>
> >> On 09.10.23 13:28, Salil Mehta wrote:
> >>> KVM vCPU creation is done once during the initialization of the VM when 
> >>> Qemu
> >>> thread is spawned. This is common to all the architectures.
> >>>
> >>> 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 and its
> >>> representative KVM vCPU object/context in Qemu is parked.
> >>>
> >>> Refactor common logic so that some APIs could be reused by vCPU Hotplug 
> >>> code.
> >>>
> >>> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> >>
> >> [...]
> >>
> >>>
> >>>    int kvm_init_vcpu(CPUState *cpu, Error **errp)
> >>> @@ -395,19 +434,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)",
> >>
> >> Unrelated change.
> >
> >
> > It is related. I think you missed kvm_get_vcpu -> kvm_create_vcpu change
> > in the string.
> 
> Indeed, I did :)
> 
> >
> >
> >>>                             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 399aaeb0ec..08e2dc253f 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) "creating KVM 
> >>> cpu: cpu_index: %d arch vcpu-id: %lu"
> >>> +kvm_get_vcpu(unsigned long arch_cpu_id) "unparking KVM vcpu: arch 
> >>> vcpu-id: %lu"
> >>> +kvm_destroy_vcpu(int cpu_index, unsigned long arch_cpu_id) "destroy 
> >>> vcpu: cpu_index: %d arch vcpu-id: %lu"
> >>> +kvm_park_vcpu(int cpu_index, unsigned long arch_cpu_id) "parking KVM 
> >>> vcpu: cpu_index: %d arch vcpu-id: %lu"
> >>
> >> It's a bit confusing that there is now
> >>
> >> 1) create (create new or return parked)
> >> 2) destroy (cleanup + park)
> >> 3) park (park only)
> >>
> >> Why would one use 2) instead of 3) or the other way around? But I
> >> suspect that kvm_destroy_vcpu() is only supposed to be a KVM-internal
> >> helper ...
> >
> > kvm_destroy_vcpu is more than just parking:
> >
> > 1. Arch destroy vcpu
> > 2. Unmap cpu->kvm_run
> > 3. Parking logic
> >
> > To support virtual CPU Hotplug on ARM platforms we pre-create all
> > the KVM vCPUs but their corresponding Qemu threads are not spawned
> > (and hence cpu->kvm_run is not mapped). Unplugged vCPUs remains
> > parked in the list. Hence, only step-3 is required.
> 
> IIUC, your current flow is going to be
> 
> 1) Create
> 2) Park
> 3) Create [which ends up reusing the parked VCPU]
> 4) Destroy [when unplugging the CPU]


In the ARM specific code, Yes.

 
> If that's the case, that API really is suboptimal.
> 
> What speaks against an API that models 1) and 2) in a single step


API is generic and is part of architecture agnostic code.


> 
> kvm_precreate_vcpu

pre-creation is very much specific to ARM right now. I am not sure
if it is right to have an API with this name in the code which is
common to other architectures.


> kvm_create_vcpu
> kvm_destroy_vcpu
> 
> One could even make kvm_create_vcpu() fail on ARM if the VCPU hasn't
> been pre-created.

Right now, we abort the CPU initialization process if this happens. I
am planning to change abort() into 'fatal_error' in RFC V3 though.



> 
> Or did I get it all wrong? :)

I won't say that it is just another point of view which is absolutely
fine. But I would like to stick to current APIs.


Thanks
Salil.