> From: David Hildenbrand <da...@redhat.com>
> Sent: Monday, October 9, 2023 3:11 PM
> To: Salil Mehta <salil.me...@huawei.com>; qemu-devel@nongnu.org; qemu-
> a...@nongnu.org
> Cc: m...@kernel.org; jean-phili...@linaro.org; Jonathan Cameron
> <jonathan.came...@huawei.com>; lpieral...@kernel.org;
> peter.mayd...@linaro.org; richard.hender...@linaro.org;
> imamm...@redhat.com; andrew.jo...@linux.dev; phi...@linaro.org;
> eric.au...@redhat.com; oliver.up...@linux.dev; pbonz...@redhat.com;
> m...@redhat.com; w...@kernel.org; gs...@redhat.com; raf...@kernel.org;
> alex.ben...@linaro.org; li...@armlinux.org.uk;
> dar...@os.amperecomputing.com; il...@os.amperecomputing.com;
> vis...@os.amperecomputing.com; karl.heub...@oracle.com;
> miguel.l...@oracle.com; salil.me...@opnsrc.net; zhukeqian
> <zhukeqi...@huawei.com>; wangxiongfeng (C) <wangxiongfe...@huawei.com>;
> wangyanan (Y) <wangyana...@huawei.com>; jiakern...@gmail.com;
> maob...@loongson.cn; lixiang...@loongson.cn; Linuxarm <linux...@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 <da...@redhat.com>
> >> Sent: Monday, October 9, 2023 1:21 PM
> >> To: Salil Mehta <salil.me...@huawei.com>; qemu-devel@nongnu.org;
> >> qemu-...@nongnu.org
> >> Cc: m...@kernel.org; jean-phili...@linaro.org; Jonathan Cameron
> >> <jonathan.came...@huawei.com>; lpieral...@kernel.org;
> >> peter.mayd...@linaro.org; richard.hender...@linaro.org;
> >> imamm...@redhat.com; andrew.jo...@linux.dev; phi...@linaro.org;
> >> eric.au...@redhat.com; oliver.up...@linux.dev; pbonz...@redhat.com;
> >> m...@redhat.com; w...@kernel.org; gs...@redhat.com; raf...@kernel.org;
> >> alex.ben...@linaro.org; li...@armlinux.org.uk;
> >> dar...@os.amperecomputing.com; il...@os.amperecomputing.com;
> >> vis...@os.amperecomputing.com; karl.heub...@oracle.com;
> >> miguel.l...@oracle.com; salil.me...@opnsrc.net; zhukeqian
> >> <zhukeqi...@huawei.com>; wangxiongfeng (C) <wangxiongfe...@huawei.com>;
> >> wangyanan (Y) <wangyana...@huawei.com>; jiakern...@gmail.com;
> >> maob...@loongson.cn; lixiang...@loongson.cn; Linuxarm
> <linux...@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.me...@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.
