On Tue, 3 Oct 2023 12:05:11 +0100
Salil Mehta <salil.me...@huawei.com> wrote:
> Hi Jonathan,
>
> > From: Jonathan Cameron <jonathan.came...@huawei.com>
> > Sent: Monday, October 2, 2023 4:53 PM
> > To: Salil Mehta <salil.me...@huawei.com>
> > Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org; m...@kernel.org; jean-
> > phili...@linaro.org; lpieral...@kernel.org; peter.mayd...@linaro.org;
> > richard.hender...@linaro.org; imamm...@redhat.com; andrew.jo...@linux.dev;
> > da...@redhat.com; phi...@linaro.org; eric.au...@redhat.com;
> > oliver.up...@linux.dev; pbonz...@redhat.com; m...@redhat.com;
> > w...@kernel.org; gs...@redhat.com; raf...@kernel.org;
> > alex.ben...@linaro.org; li...@armlinux.org.uk;
> > dar...@os.amperecomputing.com; il...@os.amperecomputing.com;
> > vis...@os.amperecomputing.com; karl.heub...@oracle.com;
> > miguel.l...@oracle.com; salil.me...@opnsrc.net; zhukeqian
> > <zhukeqi...@huawei.com>; wangxiongfeng (C) <wangxiongfe...@huawei.com>;
> > wangyanan (Y) <wangyana...@huawei.com>; jiakern...@gmail.com;
> > maob...@loongson.cn; lixiang...@loongson.cn; Linuxarm <linux...@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.me...@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.me...@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 :)
