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

[Jonathan Cameron]

On Tue, 3 Oct 2023 12:05:11 +0100
Salil Mehta <salil.mehta@huawei.com> wrote:

> 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?

I wasn't concerned with the type, but rather that the
value comes from other places than cpu->cpu_index
on some architectures.
> 
> 
> > >  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?
> 
It wasn't the type that bothered, me but rather that the source of
the data isn't always cpu->cpu_index so I have no idea if the values
are consistent.

> 
> 
> > >      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
> 
ah. Eyes jumped over that :)