On Mon, 2 Oct 2023 17:12:43 +0100
Salil Mehta <salil.me...@huawei.com> wrote:
> Hi Gavin,
>
> > From: Gavin Shan <gs...@redhat.com>
> > Sent: Wednesday, September 27, 2023 7:29 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 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 <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: Gavin Shan <gavin.s...@redhat.com>
> > > [GS: pointed the assertion due to wrong range check]
> > > Signed-off-by: Salil Mehta <salil.me...@huawei.com>
> > > ---
> > > 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.
