Hi Gavin,
> From: Gavin Shan <gs...@redhat.com>
> Sent: Wednesday, September 27, 2023 11:05 AM
> To: Salil Mehta <salil.me...@huawei.com>; qemu-devel@nongnu.org; qemu-
> a...@nongnu.org
> Cc: m...@kernel.org; jean-phili...@linaro.org; Jonathan Cameron
> <jonathan.came...@huawei.com>; 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; w...@kernel.org; a...@kernel.org;
> oliver.up...@linux.dev; pbonz...@redhat.com; m...@redhat.com;
> raf...@kernel.org; borntrae...@linux.ibm.com; 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
> Subject: Re: [PATCH RFC V2 06/37] arm/virt,kvm: Pre-create disabled
> possible vCPUs @machine init
>
> Hi Salil,
>
> On 9/26/23 20:04, Salil Mehta wrote:
> > In ARMv8 architecture, GIC needs all the vCPUs to be created and present
> > when
> > it is initialized. This is because:
> > 1. GICC and MPIDR association must be fixed at the VM initialization time.
> > This is represented by register GIC_TYPER(mp_afffinity, proc_num)
> > 2. GICC(cpu interfaces), GICR(redistributors) etc all must be initialized
> > at the boot time as well.
> > 3. Memory regions associated with GICR etc. cannot be changed(add/del/mod)
> > after VM has inited.
> >
> > This patch adds the support to pre-create all such possible vCPUs within the
> > host using the KVM interface as part of the virt machine initialization.
> > These
> > vCPUs could later be attached to QOM/ACPI while they are actually hot
> > plugged
> > and made present.
> >
> > Co-developed-by: Salil Mehta <salil.me...@huawei.com>
> > Signed-off-by: Salil Mehta <salil.me...@huawei.com>
> > Co-developed-by: Keqian Zhu <zhukeqi...@huawei.com>
> > Signed-off-by: Keqian Zhu <zhukeqi...@huawei.com>
> > Reported-by: Vishnu Pajjuri <vis...@os.amperecomputing.com>
> > [VP: Identified CPU stall issue & suggested probable fix]
> > Signed-off-by: Salil Mehta <salil.me...@huawei.com>
> > ---
> > hw/arm/virt.c | 53 +++++++++++++++++++++++++++++++++++++++++--
> > include/hw/core/cpu.h | 1 +
> > target/arm/cpu64.c | 1 +
> > target/arm/kvm.c | 32 ++++++++++++++++++++++++++
> > target/arm/kvm64.c | 9 +++++++-
> > target/arm/kvm_arm.h | 11 +++++++++
> > 6 files changed, 104 insertions(+), 3 deletions(-)
> >
>
> The subject looks a bit misleading. (possible && disabled) == (disabled). So
> it
> can be simplified to something like below:
I will improve it.
> arm/virt,kvm: Pre-create KVM objects for hotpluggable vCPUs
>
> I think the commit log can be improved to something like below:
>
> All possible vCPUs are classified to cold-booting and hotpluggable vCPUs.
> In ARMv8 architecture, GIC needs all the possible vCPUs to be existing
> and present when it is initialized for several factors. After the
> initializaion,
> the CPU instances for those hotpluggable vCPUs aren't needed, but the
> KVM objects like vCPU's file descriptor should be kept as they have been
> shared to host.
>
> 1. GICC and MPIDR association must be fixed at the VM initialization time.
> This is represented by register GIC_TYPER(mp_afffinity, proc_num)
> 2. GICC(cpu interfaces), GICR(redistributors) etc all must be initialized
> at the boot time as well.
> 3. Memory regions associated with GICR etc. cannot be changed(add/del/mod)
> after VM has inited.
>
> This creates and realizes CPU instances for those cold-booting vCPUs. They
> becomes enabled eventually. For these hotpluggable vCPUs, the vCPU
> instances
> are created, but not realized. They become present eventually.
Above is too complex. I'll make it more succinct. Will fix this.
Thanks
Salil.
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 3668ad27ec..6ba131b799 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -2293,8 +2293,10 @@ static void machvirt_init(MachineState *machine)
> > assert(possible_cpus->len == max_cpus);
> > for (n = 0; n < possible_cpus->len; n++) {
> > Object *cpuobj;
> > + CPUState *cs;
> >
> > cpuobj = object_new(possible_cpus->cpus[n].type);
> > + cs = CPU(cpuobj);
> >
> > aarch64 &= object_property_get_bool(cpuobj, "aarch64", NULL);
> > object_property_set_int(cpuobj, "socket-id",
> > @@ -2306,8 +2308,55 @@ static void machvirt_init(MachineState *machine)
> > object_property_set_int(cpuobj, "thread-id",
> > virt_get_thread_id(machine, n), NULL);
> >
> > - qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> > - object_unref(cpuobj);
> > + if (n < smp_cpus) {
> > + qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> > + object_unref(cpuobj);
> > + } else {
> > + CPUArchId *cpu_slot;
> > +
> > + /* handling for vcpus which are yet to be hot-plugged */
> > + cs->cpu_index = n;
> > + cpu_slot = virt_find_cpu_slot(machine, cs->cpu_index);
> > +
> > + /*
> > + * ARM host vCPU features need to be fixed at the boot time.
> > But as
> > + * per current approach this CPU object will be destroyed
> > during
> > + * cpu_post_init(). During hotplug of vCPUs these properties
> > are
> > + * initialized again.
> > + */
> > + virt_cpu_set_properties(cpuobj, cpu_slot, &error_fatal);
> > +
> > + /*
> > + * For KVM, we shall be pre-creating the now
> > disabled/un-plugged
> > + * possbile host vcpus and park them till the time they are
> > + * actually hot plugged. This is required to pre-size thehost
> > + * GICC and GICR with the all possible vcpus for this VM.
> > + */
> > + if (kvm_enabled()) {
> > + kvm_arm_create_host_vcpu(ARM_CPU(cs));
> > + }
>
> /*
> * For KVM, the associated objects like vCPU's file descriptor
> * is reserved so that they can reused when the vCPU is hot
> added.
> * :
> */
I think. Unnecessary.
> > + /*
> > + * Add disabled vCPU to CPU slot during the init phase of the
> > virt
> > + * machine
> > + * 1. We need this ARMCPU object during the GIC init. This
> > object
> > + * will facilitate in pre-realizing the GIC. Any info like
> > + * mp-affinity(required to derive gicr_type) etc. could
> > still be
> > + * fetched while preserving QOM abstraction akin to realized
> > + * vCPUs.
> > + * 2. Now, after initialization of the virt machine is
> > complete we
> > + * could use two approaches to deal with this ARMCPU object:
> > + * (i) re-use this ARMCPU object during hotplug of this
> > vCPU.
> > + * OR
> > + * (ii) defer release this ARMCPU object after gic has been
> > + * initialized or during pre-plug phase when a vCPU is
> > + * hotplugged.
> > + *
> > + * We will use the (ii) approach and release the ARMCPU
> > objects
> > + * after GIC and machine has been fully initialized during
> > + * machine_init_done() phase.
> > + */
> > + cpu_slot->cpu = OBJECT(cs);
> > + }
>
> /*
> * Make the hotpluggable vCPU present because ....
> */
Sorry, did not get?
> > }
> > fdt_add_timer_nodes(vms);
> > fdt_add_cpu_nodes(vms);
> > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > index e5af79950c..b2201a98ee 100644
> > --- a/include/hw/core/cpu.h
> > +++ b/include/hw/core/cpu.h
> > @@ -401,6 +401,7 @@ struct CPUState {
> > uint32_t kvm_fetch_index;
> > uint64_t dirty_pages;
> > int kvm_vcpu_stats_fd;
> > + VMChangeStateEntry *vmcse;
> >
> > /* Use by accel-block: CPU is executing an ioctl() */
> > QemuLockCnt in_ioctl_lock;
> > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> > index a660e3f483..3a38e7ccaf 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -748,6 +748,7 @@ static void aarch64_cpu_initfn(Object *obj)
> > * enabled explicitly
> > */
> > cs->disabled = true;
> > + cs->thread_id = 0;
> > }
> >
> > static void aarch64_cpu_finalizefn(Object *obj)
> > diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> > index b4c7654f49..0e1d0692b1 100644
> > --- a/target/arm/kvm.c
> > +++ b/target/arm/kvm.c
> > @@ -637,6 +637,38 @@ void kvm_arm_reset_vcpu(ARMCPU *cpu)
> > write_list_to_cpustate(cpu);
> > }
> >
> > +void kvm_arm_create_host_vcpu(ARMCPU *cpu)
> > +{
> > + CPUState *cs = CPU(cpu);
> > + unsigned long vcpu_id = cs->cpu_index;
> > + int ret;
> > +
> > + ret = kvm_create_vcpu(cs);
> > + if (ret < 0) {
> > + error_report("Failed to create host vcpu %ld", vcpu_id);
> > + abort();
> > + }
> > +
> > + /*
> > + * Initialize the vCPU in the host. This will reset the sys regs
> > + * for this vCPU and related registers like MPIDR_EL1 etc. also
> > + * gets programmed during this call to host. These are referred
> > + * later while setting device attributes of the GICR during GICv3
> > + * reset
> > + */
> > + ret = kvm_arch_init_vcpu(cs);
> > + if (ret < 0) {
> > + error_report("Failed to initialize host vcpu %ld", vcpu_id);
> > + abort();
> > + }
> > +
> > + /*
> > + * park the created vCPU. shall be used during kvm_get_vcpu() when
> > + * threads are created during realization of ARM vCPUs.
> > + */
> > + kvm_park_vcpu(cs);
> > +}
> > +
> > /*
> > * Update KVM's MP_STATE based on what QEMU thinks it is
> > */
> > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > index 94bbd9661f..364cc21f81 100644
> > --- a/target/arm/kvm64.c
> > +++ b/target/arm/kvm64.c
> > @@ -566,7 +566,14 @@ int kvm_arch_init_vcpu(CPUState *cs)
> > return -EINVAL;
> > }
> >
> > - qemu_add_vm_change_state_handler(kvm_arm_vm_state_change, cs);
> > + /*
> > + * Install VM change handler only when vCPU thread has been spawned
> > + * i.e. vCPU is being realized
> > + */
> > + if (cs->thread_id) {
> > + cs->vmcse =
> qemu_add_vm_change_state_handler(kvm_arm_vm_state_change,
> > + cs);
> > + }
> >
> > /* Determine init features for this CPU */
> > memset(cpu->kvm_init_features, 0, sizeof(cpu->kvm_init_features));
> > diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> > index 051a0da41c..31408499b3 100644
> > --- a/target/arm/kvm_arm.h
> > +++ b/target/arm/kvm_arm.h
> > @@ -163,6 +163,17 @@ void kvm_arm_cpu_post_load(ARMCPU *cpu);
> > */
> > void kvm_arm_reset_vcpu(ARMCPU *cpu);
> >
> > +/**
> > + * kvm_arm_create_host_vcpu:
> > + * @cpu: ARMCPU
> > + *
> > + * Called at to pre create all possible kvm vCPUs within the the host at
> the
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> to create instances for the hotpluggable vCPUs
No. hot-plugging in ARM is a higher level operation which is being
done at QOM not at KVM level. Later is totally agnostic of
what Qemu is doing as part of hot(un)plug operations happening at
QOM. I would not want to associate hot-plugging with KVM as it
gives a wrong impression.
Thanks
Salil.
