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

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

> From: Jonathan Cameron <jonathan.cameron@huawei.com>
> Sent: Tuesday, October 3, 2023 12:51 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 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.

Sure, I meant there is a reason why type was chosen as 'unsigned long'
and not an 'integer'. Perhaps the value can exceed the 'integer' size
limits because of the way CPU index is being created on certain archs?

If we try to put value from a larger container 'unsigned long' to a
smaller container 'integer' things can go wrong.


[...]

> > >
> > > > +    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.

Got it.

I meant 'unsigned long' return type in the kvm_arch_vcpu_id(). On some
Architectures, the required container size for their vcpu-id could
exceed an 'integer'. Sorry, for not making it clear.

Thanks
Salil.