Re: [PATCH RFC V2 04/37] arm/virt,target/arm: Machine init time change common to vCPU {cold|hot}-plug

[Jonathan Cameron]

On Mon, 2 Oct 2023 17:12:43 +0100
Salil Mehta <salil.mehta@huawei.com> wrote:

> Hi Gavin,
> 
> > From: Gavin Shan <gshan@redhat.com>
> > Sent: Wednesday, September 27, 2023 7:29 AM
> > To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org; qemu-
> > arm@nongnu.org
> > Cc: maz@kernel.org; jean-philippe@linaro.org; Jonathan Cameron
> > <jonathan.cameron@huawei.com>; 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; will@kernel.org; ardb@kernel.org;
> > oliver.upton@linux.dev; pbonzini@redhat.com; mst@redhat.com;
> > rafael@kernel.org; borntraeger@linux.ibm.com; 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
> > 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.mehta@huawei.com>
> > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> > > Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
> > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> > > Reported-by: Gavin Shan <gavin.shan@redhat.com>
> > > [GS: pointed the assertion due to wrong range check]
> > > Signed-off-by: Salil Mehta <salil.mehta@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.