Hi Alex, Thanks for taking pains to review it. > From: Alex Bennée <alex.ben...@linaro.org> > Sent: Friday, September 29, 2023 2:32 PM > To: Salil Mehta <salil.me...@huawei.com> > Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org; 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; > da...@redhat.com; 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; > 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 1/9] accel/kvm: Extract common KVM vCPU > {creation,parking} code > > > Salil Mehta <salil.me...@huawei.com> writes: > > > KVM vCPU creation is done once during the initialization of the VM when Qemu > > threads are spawned. This is common to all the architectures. > > > > Hot-unplug of vCPU results in destruction of the vCPU objects in QOM but > > the KVM vCPU objects in the Host KVM are not destroyed and their > > representative > > KVM vCPU objects/context in Qemu are parked. > > > > Refactor common logic so that some APIs could be reused by vCPU Hotplug > > code. > > > > Signed-off-by: Salil Mehta <salil.me...@huawei.com> > > --- > > accel/kvm/kvm-all.c | 61 ++++++++++++++++++++++++++++++++++---------- > > include/sysemu/kvm.h | 2 ++ > > 2 files changed, 49 insertions(+), 14 deletions(-) > > > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > > index ff1578bb32..57889c5f6c 100644 > > --- a/accel/kvm/kvm-all.c > > +++ b/accel/kvm/kvm-all.c > > @@ -137,6 +137,7 @@ static QemuMutex kml_slots_lock; > > #define kvm_slots_unlock() qemu_mutex_unlock(&kml_slots_lock) > > > > static void kvm_slot_init_dirty_bitmap(KVMSlot *mem); > > +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id); > > weird to have a forward static declaration here if you are declare in > the header later on.
Yes, it is awkward. Actually, I had to move a function to get a diff of changes which were more readable. Without moving existing kvm_get_vcpu() function, change was not getting highlighted properly in the generated patch. This relocation of the function meant the order of declaration and its call got reversed. So I had to use above tactics, even though weird. Suggestions welcome to solve this? > > static inline void kvm_resample_fd_remove(int gsi) > > { > > @@ -320,11 +321,51 @@ err: > > return ret; > > } > > > > +void kvm_park_vcpu(CPUState *cpu) > > +{ > > + unsigned long vcpu_id = cpu->cpu_index; > > cpu_index is a plain int in CPUState: > > int cpu_index; > > but is defined as an unsigned long in KVMParkedVcpu: > > unsigned long vcpu_id; > > I'm not sure if this is just a historical anomaly but I suspect we > should fixup the discrepancy first so all users of cpu_index use the > same size. Yes, agreed. But it is out of scope of this patch. > > + struct KVMParkedVcpu *vcpu; > > + > > + vcpu = g_malloc0(sizeof(*vcpu)); > > + vcpu->vcpu_id = vcpu_id; > > + vcpu->kvm_fd = cpu->kvm_fd; > > + QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node); > > +} > > + > > +int kvm_create_vcpu(CPUState *cpu) > > +{ > > I don't think you get anything other than -1 on failure so at this point > you might as well return a bool. kvm_vm_ioctl() can return other values? The 'ret' value can then be set using error_setg_errno() for example, in kvm_init_vcpu(). It is better to keep error handling standard for any future changes as well. > > > + unsigned long vcpu_id = cpu->cpu_index; > > Is this used? It is used but we can get away with this. I think it is a stray left over from shuffle. Thanks! > > + KVMState *s = kvm_state; > > + int ret; > > + > > + DPRINTF("kvm_create_vcpu\n"); > > Please don't add new DPRINTFs - use tracepoints instead. Whether its > worth cleaning up up kvm-all first I leave up to you. I can definitely change for this code. > > > + /* check if the KVM vCPU already exist but is parked */ > > + ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu)); > > + if (ret > 0) { > > + goto found; > > + } > > + > > + /* create a new KVM vCPU */ > > + ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id); > > + if (ret < 0) { > > + return ret; > > + } > > + > > +found: > > + cpu->vcpu_dirty = true; > > + cpu->kvm_fd = ret; > > + cpu->kvm_state = s; > > + cpu->dirty_pages = 0; > > + cpu->throttle_us_per_full = 0; > > + > > + return 0; > > +} > > + > > This is trivially nestable to avoid gotos: No issues. I can remove gotos. I was trying to reduce indentation. > > bool kvm_create_vcpu(CPUState *cpu) It is better to return ioctl value so that error can be set at the caller handling code. > { > unsigned long vcpu_id = cpu->cpu_index; > KVMState *s = kvm_state; > int ret; > > /* check if the KVM vCPU already exist but is parked */ > ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu)); > if (ret < 0) { > /* not found, try to create a new KVM vCPU */ > ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id); > if (ret < 0) { > return false; > } > } Yes, can be done this way. > > cpu->vcpu_dirty = true; > cpu->kvm_fd = ret; > cpu->kvm_state = s; > cpu->dirty_pages = 0; > cpu->throttle_us_per_full = 0; > > return true; return value is better than Boolean. > } > > > static int do_kvm_destroy_vcpu(CPUState *cpu) > > { > > KVMState *s = kvm_state; > > long mmap_size; > > - struct KVMParkedVcpu *vcpu = NULL; > > int ret = 0; > > > > DPRINTF("kvm_destroy_vcpu\n"); > > @@ -353,10 +394,7 @@ static int do_kvm_destroy_vcpu(CPUState *cpu) > > } > > } > > > > - vcpu = g_malloc0(sizeof(*vcpu)); > > - vcpu->vcpu_id = kvm_arch_vcpu_id(cpu); > > - vcpu->kvm_fd = cpu->kvm_fd; > > - QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node); > > + kvm_park_vcpu(cpu); > > err: > > return ret; > > } > > @@ -384,7 +422,7 @@ static int kvm_get_vcpu(KVMState *s, unsigned long > vcpu_id) > > } > > } > > > > - return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id); > > + return -1; > > } > > > > int kvm_init_vcpu(CPUState *cpu, Error **errp) > > @@ -395,19 +433,14 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp) > > Hmm it looks like no one cares about the return value here and the fact > that callers use &error_fatal should be enough to exit. You can then > just return early and avoid the error ladder. Maybe yes, but is that change really required? > > > > > trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu)); > > > > - ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu)); > > + ret = kvm_create_vcpu(cpu); > > if (ret < 0) { > > - error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu failed > (%lu)", > > + error_setg_errno(errp, -ret, > > + "kvm_init_vcpu: kvm_create_vcpu failed (%lu)", > > kvm_arch_vcpu_id(cpu)); > > goto err; > > } > > > > - cpu->kvm_fd = ret; > > - cpu->kvm_state = s; > > - cpu->vcpu_dirty = true; > > - cpu->dirty_pages = 0; > > - cpu->throttle_us_per_full = 0; > > - > > mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0); > > if (mmap_size < 0) { > > ret = mmap_size; > > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h > > index ee9025f8e9..17919567a8 100644 > > --- a/include/sysemu/kvm.h > > +++ b/include/sysemu/kvm.h > > @@ -464,6 +464,8 @@ void kvm_set_sigmask_len(KVMState *s, unsigned int > sigmask_len); > > > > int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr, > > hwaddr *phys_addr); > > +int kvm_create_vcpu(CPUState *cpu); > > +void kvm_park_vcpu(CPUState *cpu); > > Please add kdoc comments for the public API functions describing their > usage and parameters. Sure. Thanks Salil.