|
From: | David Hildenbrand |
Subject: | Re: [PATCH V3 01/10] accel/kvm: Extract common KVM vCPU {creation,parking} code |
Date: | Mon, 9 Oct 2023 16:11:06 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 |
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] If that's the case, that API really is suboptimal. What speaks against an API that models 1) and 2) in a single step kvm_precreate_vcpu kvm_create_vcpu kvm_destroy_vcpuOne could even make kvm_create_vcpu() fail on ARM if the VCPU hasn't been pre-created.
Or did I get it all wrong? :) -- Cheers, David / dhildenb
[Prev in Thread] | Current Thread | [Next in Thread] |