On Mon, 2 Oct 2023 17:12:43 +0100 Salil Mehta <[email protected]> wrote:
> Hi Gavin, > > > From: Gavin Shan <[email protected]> > > Sent: Wednesday, September 27, 2023 7:29 AM > > To: Salil Mehta <[email protected]>; [email protected]; qemu- > > [email protected] > > Cc: [email protected]; [email protected]; Jonathan Cameron > > <[email protected]>; [email protected]; > > [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; > > [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > zhukeqian <[email protected]>; wangxiongfeng (C) > > <[email protected]>; wangyanan (Y) <[email protected]>; > > [email protected]; [email protected]; [email protected] > > Subject: Re: [PATCH RFC V2 04/37] arm/virt,target/arm: Machine init time > > change common to vCPU {cold|hot}-plug > > > > Hi Salil, > > > > On 9/26/23 20:04, Salil Mehta wrote: > > > Refactor and introduce the common logic required during the > > initialization of > > > both cold and hot plugged vCPUs. Also initialize the *disabled* state of > > > the > > > vCPUs which shall be used further during init phases of various other > > > components > > > like GIC, PMU, ACPI etc as part of the virt machine initialization. > > > > > > KVM vCPUs corresponding to unplugged/yet-to-be-plugged QOM CPUs are kept > > > in > > > powered-off state in the KVM Host and do not run the guest code. Plugged > > > vCPUs > > > are also kept in powered-off state but vCPU threads exist and is kept > > > sleeping. > > > > > > TBD: > > > For the cold booted vCPUs, this change also exists in the > > > arm_load_kernel() > > > in boot.c but for the hotplugged CPUs this change should still remain > > > part of > > > the pre-plug phase. We are duplicating the powering-off of the cold > > > booted CPUs. > > > Shall we remove the duplicate change from boot.c? > > > > > > Co-developed-by: Salil Mehta <[email protected]> > > > Signed-off-by: Salil Mehta <[email protected]> > > > Co-developed-by: Keqian Zhu <[email protected]> > > > Signed-off-by: Keqian Zhu <[email protected]> > > > Reported-by: Gavin Shan <[email protected]> > > > [GS: pointed the assertion due to wrong range check] > > > Signed-off-by: Salil Mehta <[email protected]> > > > --- > > > hw/arm/virt.c | 149 ++++++++++++++++++++++++++++++++++++++++----- > > > target/arm/cpu.c | 7 +++ > > > target/arm/cpu64.c | 14 +++++ > > > 3 files changed, 156 insertions(+), 14 deletions(-) > > > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > > index 0eb6bf5a18..3668ad27ec 100644 > > > --- a/hw/arm/virt.c > > > +++ b/hw/arm/virt.c > > > @@ -221,6 +221,7 @@ static const char *valid_cpus[] = { > > > ARM_CPU_TYPE_NAME("max"), > > > }; > > > > > > +static CPUArchId *virt_find_cpu_slot(MachineState *ms, int vcpuid); > > > static int virt_get_socket_id(const MachineState *ms, int cpu_index); > > > static int virt_get_cluster_id(const MachineState *ms, int cpu_index); > > > static int virt_get_core_id(const MachineState *ms, int cpu_index); > > > @@ -2154,6 +2155,14 @@ static void machvirt_init(MachineState *machine) > > > exit(1); > > > } > > > > > > + finalize_gic_version(vms); > > > + if (tcg_enabled() || hvf_enabled() || qtest_enabled() || > > > + (vms->gic_version < VIRT_GIC_VERSION_3)) { > > > + machine->smp.max_cpus = smp_cpus; > > > + mc->has_hotpluggable_cpus = false; > > > + warn_report("cpu hotplug feature has been disabled"); > > > + } > > > + > > > > Comments needed here to explain why @mc->has_hotpluggable_cpus is set to > > false. > > I guess it's something related to TODO list, mentioned in the cover letter. > > > > > I can put a comment explaining the checks as to why feature has been disabled. > BTW, isn't code self-explanatory here? Would be good to gate that warn_report() on whether any attempt to enable CPU hotplug has been made if (max_cpus > smp for example). Right now it's noise on a lot of pre existing configurations.
