On 8/5/24 12:46, Salil Mehta wrote:
Hi Phillipe,
Sorry, I missed this mail earlier.
From: Philippe Mathieu-Daudé <phi...@linaro.org>
Sent: Friday, May 3, 2024 7:23 PM
To: Salil Mehta <salil.me...@huawei.com>; qemu-devel@nongnu.org;
qemu-...@nongnu.org
On 3/5/24 17:57, Salil Mehta wrote:
> Hi Philippe,
>
>> From: Philippe Mathieu-Daudé <phi...@linaro.org>
>> Sent: Friday, May 3, 2024 10:40 AM
>> Subject: Re: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU
>> {creation,parking} code
>>
>> Hi Salil,
>>
>> On 12/3/24 02:59, Salil Mehta wrote:
>> > KVM vCPU creation is done once during the vCPU realization when Qemu
>> > vCPU thread is spawned. This is common to all the architectures as of
now.
>> >
>> > 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
>> > as KVM doesn't support vCPU removal. Therefore, its representative KVM
>> > vCPU object/context in Qemu is parked.
>> >
>> > Refactor architecture common logic so that some APIs could be reused
>> > by vCPU Hotplug code of some architectures likes ARM, Loongson etc.
>> > Update new/old APIs with trace events instead of DPRINTF. No functional
>> change is intended here.
>> >
>> > Signed-off-by: Salil Mehta <salil.me...@huawei.com>
>> > Reviewed-by: Gavin Shan <gs...@redhat.com>
>> > Tested-by: Vishnu Pajjuri <vis...@os.amperecomputing.com>
>> > Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>
>> > Tested-by: Xianglai Li <lixiang...@loongson.cn>
>> > Tested-by: Miguel Luis <miguel.l...@oracle.com>
>> > Reviewed-by: Shaoqin Huang <shahu...@redhat.com>
>> > ---
>> > accel/kvm/kvm-all.c | 64 ++++++++++++++++++++++++++++++++-
-----
>> ----
>> > accel/kvm/trace-events | 5 +++-
>> > include/sysemu/kvm.h | 16 +++++++++++
>> > 3 files changed, 69 insertions(+), 16 deletions(-)
>> >
>> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index
>> > a8cecd040e..3bc3207bda 100644
>> > --- a/accel/kvm/kvm-all.c
>> > +++ b/accel/kvm/kvm-all.c
>> > @@ -126,6 +126,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)
>> > {
>> > @@ -314,14 +315,53 @@ err:
>> > return ret;
>> > }
>> >
>> > +void kvm_park_vcpu(CPUState *cpu)
>> > +{
>> > + struct KVMParkedVcpu *vcpu;
>> > +
>> > + trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(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); }
>> > +
>> > +int kvm_create_vcpu(CPUState *cpu)
>> > +{
>> > + unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
>> > + KVMState *s = kvm_state;
>> > + int kvm_fd;
>> > +
>> > + trace_kvm_create_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>> > +
>> > + /* check if the KVM vCPU already exist but is parked */
>> > + kvm_fd = kvm_get_vcpu(s, vcpu_id);
>> > + if (kvm_fd < 0) {
>> > + /* vCPU not parked: create a new KVM vCPU */
>> > + kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
>> > + if (kvm_fd < 0) {
>> > + error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu",
vcpu_id);
>> > + return kvm_fd;
>> > + }
>> > + }
>> > +
>> > + cpu->kvm_fd = kvm_fd;
>> > + cpu->kvm_state = s;
>> > + cpu->vcpu_dirty = true;
>> > + cpu->dirty_pages = 0;
>> > + cpu->throttle_us_per_full = 0;
>> > +
>> > + return 0;
>> > +}
>>
>> This seems generic enough to be implemented for all accelerators.
>>
>> See AccelOpsClass in include/sysemu/accel-ops.h.
>>
>> That said, can be done later on top.
>
> Let me understand correctly. Are you suggesting to implement above
> even for HVF, TCG, QTEST etc?
Not for you to implement the other non-KVM accelerators, but since you
are introducing this, now is a good time to think about a generic interface.
So far AccelOpsClass::[un]park_vcpu() handlers make sense to me.
Sure, but what is the advantage of defining these 'supporting' functions
as part of the AccelOpsClass? Each of these functions in any case will need
to be defined individually for different Accelerators or unless we are
planning to extract some common accelerator functions in a separate file
and use them across all the accelerators?
kvm_arm_create_host_vcpu() [*] seems generic. Maybe we could do the
same with HVF at least.
[*]
https://lore.kernel.org/qemu-devel/20230926100436.28284-7-salil.me...@huawei.com/
I'm surely missing some key point here.
I started https://etherpad.opendev.org/p/QEMU_vCPU_life to
document the vCPU spagetti code. In that big picture the "park"
method makes sense to me, but we can discuss that later. Again,
certainly not a block for your work, I'm just trying to see
the whole view.
Regards,
Phil.
