| Message ID | 20230926100436.28284-6-salil.mehta@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support of Virtual CPU Hotplug for ARMv8 Arch | expand |
Hi Salil, On 9/26/23 20:04, Salil Mehta wrote: > KVM vCPU creation is done once during the initialization of the VM when Qemu > threads are spawned. This is common to all the architectures. If the architecture > supports vCPU hot-{un}plug then this KVM vCPU creation could be deferred to > later point as well. Some architectures might in any case create KVM vCPUs for > the yet-to-be plugged vCPUs (i.e. QoM Object & thread does not exists) during VM > init time and park them. > > 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 in Qemu are parked. > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > --- > accel/kvm/kvm-all.c | 61 ++++++++++++++++++++++++++++++++++---------- > include/sysemu/kvm.h | 2 ++ > 2 files changed, 49 insertions(+), 14 deletions(-) > The most important point seems missed in the commit log: The KVM vCPU objects, including those hotpluggable objects, need to be in place before in-host GICv3 is initialized. So we need expose kvm_create_vcpu() to make those KVM vCPU objects in place, even for those non-present vCPUs. > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index 7b3da8dc3a..86e9c9ea60 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); > > 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; > + struct KVMParkedVcpu *vcpu; > + > + vcpu = g_malloc0(sizeof(*vcpu)); > + vcpu->vcpu_id = vcpu_id; vcpu->vcpu_id = cpu->cpu_index; @vcpu_id can be dropped. > + 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 = cpu->cpu_index; > + KVMState *s = kvm_state; > + int ret; > + > + DPRINTF("kvm_create_vcpu\n"); > + > + /* 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; > +} > + The found tag can be dropped. @cpu can be initialized if vCPU fd is found and then bail early. /* The KVM vCPU may have been existing */ ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu)); if (ret > 0) { cpu->vcpu_dirty = true; : : return 0; } /* Create a new KVM vCPU */ > 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) > > 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 115f0cca79..2c34889b01 100644 > --- a/include/sysemu/kvm.h > +++ b/include/sysemu/kvm.h > @@ -473,6 +473,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); > > #endif /* NEED_CPU_H */ > Thanks, Gavin
Hi Gavin, > From: Gavin Shan <gshan@redhat.com> > Sent: Wednesday, September 27, 2023 7:52 AM > 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; david@redhat.com; > philmd@linaro.org; eric.auger@redhat.com; will@kernel.org; ardb@kernel.org; > oliver.upton@linux.dev; pbonzini@redhat.com; mst@redhat.com; > rafael@kernel.org; borntraeger@linux.ibm.com; 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 > Subject: Re: [PATCH RFC V2 05/37] accel/kvm: Extract common KVM vCPU > {creation,parking} code > > Hi Salil, > > On 9/26/23 20:04, Salil Mehta wrote: > > KVM vCPU creation is done once during the initialization of the VM when Qemu > > threads are spawned. This is common to all the architectures. If the architecture > > supports vCPU hot-{un}plug then this KVM vCPU creation could be deferred to > > later point as well. Some architectures might in any case create KVM vCPUs for > > the yet-to-be plugged vCPUs (i.e. QoM Object & thread does not exists) during VM > > init time and park them. > > > > 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 in Qemu are parked. > > > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > > --- > > accel/kvm/kvm-all.c | 61 ++++++++++++++++++++++++++++++++++---------- > > include/sysemu/kvm.h | 2 ++ > > 2 files changed, 49 insertions(+), 14 deletions(-) > > > > The most important point seems missed in the commit log: The KVM vCPU objects, > including those hotpluggable objects, need to be in place before in-host GICv3 > is initialized. So we need expose kvm_create_vcpu() to make those KVM vCPU > objects in place, even for those non-present vCPUs. This is a patch common to all architectures. The point you are making is specific to the ARM architecture. This patch is now part of the common patch-set. Here, https://lore.kernel.org/qemu-devel/20230930001933.2660-1-salil.mehta@huawei.com/ > > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > > index 7b3da8dc3a..86e9c9ea60 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); > > > > 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; > > + struct KVMParkedVcpu *vcpu; > > + > > + vcpu = g_malloc0(sizeof(*vcpu)); > > + vcpu->vcpu_id = vcpu_id; > > vcpu->vcpu_id = cpu->cpu_index; > > @vcpu_id can be dropped. Yes, agreed. Thanks Salil. > > > + 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 = cpu->cpu_index; > > + KVMState *s = kvm_state; > > + int ret; > > + > > + DPRINTF("kvm_create_vcpu\n"); > > + > > + /* 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; > > +} > > + > > The found tag can be dropped. @cpu can be initialized if vCPU fd is found > and then bail early. Yes, This patch has been refactored and found has been dropped. https://lore.kernel.org/qemu-devel/20230930001933.2660-1-salil.mehta@huawei.com/ Thanks Salil.
Hi Salil, On 10/3/23 02:20, Salil Mehta wrote: >> From: Gavin Shan <gshan@redhat.com> >> Sent: Wednesday, September 27, 2023 7:52 AM >> 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; david@redhat.com; >> philmd@linaro.org; eric.auger@redhat.com; will@kernel.org; ardb@kernel.org; >> oliver.upton@linux.dev; pbonzini@redhat.com; mst@redhat.com; >> rafael@kernel.org; borntraeger@linux.ibm.com; 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 >> Subject: Re: [PATCH RFC V2 05/37] accel/kvm: Extract common KVM vCPU >> {creation,parking} code >> >> Hi Salil, >> >> On 9/26/23 20:04, Salil Mehta wrote: >>> KVM vCPU creation is done once during the initialization of the VM when Qemu >>> threads are spawned. This is common to all the architectures. If the architecture >>> supports vCPU hot-{un}plug then this KVM vCPU creation could be deferred to >>> later point as well. Some architectures might in any case create KVM vCPUs for >>> the yet-to-be plugged vCPUs (i.e. QoM Object & thread does not exists) during VM >>> init time and park them. >>> >>> 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 in Qemu are parked. >>> >>> Signed-off-by: Salil Mehta <salil.mehta@huawei.com> >>> --- >>> accel/kvm/kvm-all.c | 61 ++++++++++++++++++++++++++++++++++---------- >>> include/sysemu/kvm.h | 2 ++ >>> 2 files changed, 49 insertions(+), 14 deletions(-) >>> >> >> The most important point seems missed in the commit log: The KVM vCPU objects, >> including those hotpluggable objects, need to be in place before in-host GICv3 >> is initialized. So we need expose kvm_create_vcpu() to make those KVM vCPU >> objects in place, even for those non-present vCPUs. > > > This is a patch common to all architectures. The point you are making is specific > to the ARM architecture. This patch is now part of the common patch-set. Here, > > https://lore.kernel.org/qemu-devel/20230930001933.2660-1-salil.mehta@huawei.com/ > > Yes, reviewed it again. Lets have more discussions over there. Thanks, Gavin
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 7b3da8dc3a..86e9c9ea60 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); 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; + 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) +{ + unsigned long vcpu_id = cpu->cpu_index; + KVMState *s = kvm_state; + int ret; + + DPRINTF("kvm_create_vcpu\n"); + + /* 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; +} + 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) 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 115f0cca79..2c34889b01 100644 --- a/include/sysemu/kvm.h +++ b/include/sysemu/kvm.h @@ -473,6 +473,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); #endif /* NEED_CPU_H */
KVM vCPU creation is done once during the initialization of the VM when Qemu threads are spawned. This is common to all the architectures. If the architecture supports vCPU hot-{un}plug then this KVM vCPU creation could be deferred to later point as well. Some architectures might in any case create KVM vCPUs for the yet-to-be plugged vCPUs (i.e. QoM Object & thread does not exists) during VM init time and park them. 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 in Qemu are parked. Signed-off-by: Salil Mehta <salil.mehta@huawei.com> --- accel/kvm/kvm-all.c | 61 ++++++++++++++++++++++++++++++++++---------- include/sysemu/kvm.h | 2 ++ 2 files changed, 49 insertions(+), 14 deletions(-)