RE: [PATCH V2 01/10] accel/kvm: Extract common KVM vCPU {creation,parking} code

[Salil Mehta via <qemu-devel@nongnu.org>]

Hi Jonathan,

> From: Jonathan Cameron <jonathan.cameron@huawei.com>
> Sent: Monday, October 2, 2023 4:53 PM
> To: Salil Mehta <salil.mehta@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; maz@kernel.org; jean-
> philippe@linaro.org; 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;
> 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 V2 01/10] accel/kvm: Extract common KVM vCPU
> {creation,parking} code
> 
> On Sat, 30 Sep 2023 01:19:24 +0100
> Salil Mehta <salil.mehta@huawei.com> 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.
> >
> > 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.mehta@huawei.com>
> 
> Hi Salil,
> 
> A few trivial things inline, plus a question about why
> cpu->cpu_index can now be used but kvm_arch_vcpu_id(cpu);
> was previously needed.

Good point. I used the API because it was returning
'unsigned long' and it was being used across the archs.
I thought maybe the size of the index could vary across
archs. For example, for PowerPC above API returns vcpu_id
which presumably could have different data type than
an 'integer'.

But after Alex's comment, I was made to believe that this
assumption might not be correct and CPU index is an
'integer' across archs and perhaps semantics of above
API is not correct.

But perhaps original code was functionally correct?


> >  accel/kvm/kvm-all.c  | 63 +++++++++++++++++++++++++++++++++-----------
> >  include/sysemu/kvm.h | 14 ++++++++++
> >  2 files changed, 61 insertions(+), 16 deletions(-)
> >
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index ff1578bb32..b8c36ba50a 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -80,7 +80,7 @@
> >  #endif
> >
> >  struct KVMParkedVcpu {
> > -    unsigned long vcpu_id;
> > +    int vcpu_id;
> >      int kvm_fd;
> >      QLIST_ENTRY(KVMParkedVcpu) node;
> >  };
> > @@ -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, int vcpu_id);
> >
> >  static inline void kvm_resample_fd_remove(int gsi)
> >  {
> > @@ -320,11 +321,49 @@ err:
> >      return ret;
> >  }
> >
> > +void kvm_park_vcpu(CPUState *cpu)
> > +{
> > +    int vcpu_id = cpu->cpu_index;
> > +    struct KVMParkedVcpu *vcpu;
> > +
> > +    vcpu = g_malloc0(sizeof(*vcpu));
> > +    vcpu->vcpu_id = vcpu_id;
> 
> As vcpu_id is only used here why have the local variable?
> Maybe that changes in later patches, in which case ignore this.
> 
>     vcpu->vcpu_id = cpu->cpu_index;


Yes, thanks.


> 
> Why is kvm_arch_vcpu_id() not necessary here any more but was
> before?


Because I have now changed the type of vcpu_id from 'unsigned long'
to an 'integer'.

> 
> > +    vcpu->kvm_fd = cpu->kvm_fd;
> > +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
> > +}
> > +
> > +int kvm_create_vcpu(CPUState *cpu)
> > +{
> > +    int vcpu_id = cpu->cpu_index;
> 
> See below. I'm not sure why it's safe not to use kvm_arch_vcpu_id()
> Seems a few architectures have less than trivial implementations of
> that function currently.

I doubt this as well. Other architectures like PowerPC are returning
different type?


> > +    KVMState *s = kvm_state;
> > +    int kvm_fd;
> > +
> > +    DPRINTF("kvm_create_vcpu\n");
> > +
> > +    /* 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 %d", vcpu_id);
> > +            return kvm_fd;
> > +        }
> > +    }
> > +
> > +    cpu->vcpu_dirty = true;
> > +    cpu->kvm_fd = kvm_fd;
> > +    cpu->kvm_state = s;
> > +    cpu->dirty_pages = 0;
> > +    cpu->throttle_us_per_full = 0;
> 
> Trivial but I would have maintained the order wrt to the code removed
> below just to avoid a reviewer having to check the two bits of code
> do the same thing after the reorder.


I can do that. No problem.


[...]

> >  int kvm_init_vcpu(CPUState *cpu, Error **errp)
> > @@ -395,19 +431,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);
> 
> The switch from kvm_arch_vcpu_id(cpu) to using
> int vcpu_id = cpu->cpu_index;
> 
> Seems like a functional change on some arch.


Yes, but then we need to revert to original change inside the
new kvm_create_vcpu() API.


> >      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)",
> 
> The rewrap of the lines above seems like an unrelated change.

Function has changed from kvm_get_vcpu to kvm_create_vcpu

[...]

> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index ee9025f8e9..785f3ed083 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -464,6 +464,20 @@ 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);
> > +/**
> > + * kvm_create_vcpu - Gets a parked KVM vCPU or creates a KVM vCPU
> > + * @cpu:  QOM CPUState object for which KVM vCPU has to be
> created/fetched.
> 
> Extra space before QOM (same below)

:)

> 
> > + *
> > + * @returns: 0 when success, errno (<0) when failed.
> > + */
> > +int kvm_create_vcpu(CPUState *cpu);
> 
> Blank line here perhaps.

Ok.


> 
> > +/**
> > + * kvm_park_vcpu - Gets a parked KVM vCPU if it exists
> > + * @cpu:  QOM CPUState object for which parked KVM vCPU has to be
> fetched.
> 
> We aren't returning anything, so why fetch?

copy-paste comment error, I think. Thanks!

cheers
Salil.