RE: [PATCH RFC V2 02/37] cpus-common: Add common CPU utility for possible vCPUs

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

Hi Gavin,

> From: Gavin Shan <gshan@redhat.com>
> Sent: Wednesday, September 27, 2023 4:54 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 02/37] cpus-common: Add common CPU utility for
> possible vCPUs
> 
> Hi Salil,
> 
> On 9/26/23 20:04, Salil Mehta wrote:
> > Adds various utility functions which might be required to fetch or check
> the
> > state of the possible vCPUs. This also introduces concept of *disabled*
> vCPUs,
> > which are part of the *possible* vCPUs but are not part of the *present*
> vCPU.
> > This state shall be used during machine init time to check the presence
> of
> > vcpus.
>    ^^^^^
> 
>    vCPUs


Yes. Thanks.


> > 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>
> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> > ---
> >   cpus-common.c         | 31 +++++++++++++++++++++++++
> >   include/hw/core/cpu.h | 53 +++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 84 insertions(+)
> >
> > diff --git a/cpus-common.c b/cpus-common.c
> > index 45c745ecf6..24c04199a1 100644
> > --- a/cpus-common.c
> > +++ b/cpus-common.c
> > @@ -24,6 +24,7 @@
> >   #include "sysemu/cpus.h"
> >   #include "qemu/lockable.h"
> >   #include "trace/trace-root.h"
> > +#include "hw/boards.h"
> >
> >   QemuMutex qemu_cpu_list_lock;
> >   static QemuCond exclusive_cond;
> > @@ -107,6 +108,36 @@ void cpu_list_remove(CPUState *cpu)
> >       cpu_list_generation_id++;
> >   }
> >
> > +CPUState *qemu_get_possible_cpu(int index)
> > +{
> > +    MachineState *ms = MACHINE(qdev_get_machine());
> > +    const CPUArchIdList *possible_cpus = ms->possible_cpus;
> > +
> > +    assert((index >= 0) && (index < possible_cpus->len));
> > +
> > +    return CPU(possible_cpus->cpus[index].cpu);
> > +}
> > +
> > +bool qemu_present_cpu(CPUState *cpu)
> > +{
> > +    return cpu;
> > +}
> > +
> > +bool qemu_enabled_cpu(CPUState *cpu)
> > +{
> > +    return cpu && !cpu->disabled;
> > +}
> > +
> 
> I do think it's a good idea to have wrappers to check for CPU's states since
> these CPU states play important role in this series to support vCPU hotplug.
> However, it would be nice to move them around into header file (include/hw/boards.h)
> because all the checks are originated from ms->possible_cpus->cpus[]. It sounds
> functions to a machine (board) instead of global scope. Besides, it would be
> nice to have same input (index) for all functions. How about something like
> below in include/hw/boards.h?

These are operations related to CPUState and hence cpus-common.c seem to be
more appropriate to me. You can see similar functions like qemu_get_cpu()
already exists in the same file.

Yes, some operation do make use of the possible list which is maintained at
board level but eventually what we are returning is the CPUState. 

I am open to move some of above to board level not all like present,
enabled checks should exist in this file only. I would prefer to keep
all of them in this file.


> 
> static inline  bool machine_has_possible_cpu(int index)
> {
>      MachineState *ms = MACHINE(qdev_get_machine());
> 
>      if (!ms || !ms->possible_cpus || index < 0 || index >= ms-
> >possible_cus->len) {
>          return false;
>      }
> 
>      return true;
> }
> 
> static inline bool machine_has_present_cpu(int index)
> {
>      MachineState *ms = MACHINE(qdev_get_machine());
> 
>      if (!machine_is_possible_cpu(index) ||
>          !ms->possible_cpus->cpus[index].cpu) {
>          return false;
>      }
> 
>      return true;
> }
> 
> static inline bool machine_has_enabled_cpu(int index)
> {
>      MachineState *ms = MACHINE(qdev_get_machine());
>      CPUState *cs;
> 
>      if (!machine_is_present_cpu(index)) {
>          return false;
>      }
> 
>      cs = CPU(ms->possible_cpus->cpus[index].cpu);
>      return !cs->disabled
> }
> 
> > +uint64_t qemu_get_cpu_archid(int cpu_index)
> > +{
> > +    MachineState *ms = MACHINE(qdev_get_machine());
> > +    const CPUArchIdList *possible_cpus = ms->possible_cpus;
> > +
> > +    assert((cpu_index >= 0) && (cpu_index < possible_cpus->len));
> > +
> > +    return possible_cpus->cpus[cpu_index].arch_id;
> > +}
> > +
> 
> I think it's unnecessary to keep it since it's called for once by
> hw/arm/virt-acpi-build.c::build_madt. The architectural ID can be
> directly fetched from possible_cpus->cpus[i].arch_id. It's fine
> to drop this function and fold the logic to the following patch.

It is a very useful accessor API. I can see this code is being
replicated everywhere which also means many time its related
variables are repeatedly defined. 

Maybe this is being used once now. But this can be used across
architectures later.

> 
> [PATCH RFC V2 21/37] hw/arm: MADT Tbl change to size the guest with
> possible vCPUs
> 
> 
> >   CPUState *qemu_get_cpu(int index)
> >   {
> >       CPUState *cpu;
> > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > index fdcbe87352..e5af79950c 100644
> > --- a/include/hw/core/cpu.h
> > +++ b/include/hw/core/cpu.h
> > @@ -413,6 +413,17 @@ struct CPUState {
> >       SavedIOTLB saved_iotlb;
> >   #endif
> >
> > +    /*
> > +     * Some architectures do not allow *presence* of vCPUs to be changed
> > +     * after guest has booted using information specified by VMM/firmware
> > +     * via ACPI MADT at the boot time. Thus to enable vCPU hotplug on these
> > +     * architectures possible vCPU can have CPUState object in 'disabled'
> > +     * state or can also not have CPUState object at all. This is possible
> > +     * when vCPU Hotplug is supported and vCPUs are 'yet-to-be-plugged' in
> > +     * the QOM or have been hot-unplugged.
> > +     * By default every CPUState is enabled as of now across all archs.
> > +     */
> > +    bool disabled;
> >       /* TODO Move common fields from CPUArchState here. */
> >       int cpu_index;
> >       int cluster_index;
> 
> I guess the comments can be simplified a bit. How about something like
> below?
>      /*
>       * In order to support vCPU hotplug on architectures like aarch64,
>       * the vCPU states fall into possible, present or enabled. This field
>       * is added to distinguish present and enabled vCPUs. By default, all
>       * vCPUs are present and enabled.
>       */

I can definitely try to simplify it but above is not properly conveying the
reason why we require the disabled state.


> 
> > @@ -770,6 +781,48 @@ static inline bool cpu_in_exclusive_context(const
> CPUState *cpu)
> >    */
> >   CPUState *qemu_get_cpu(int index);
> >
> > +/**
> > + * qemu_get_possible_cpu:
> > + * @index: The CPUState@cpu_index value of the CPU to obtain.
> > + *         Input index MUST be in range [0, Max Possible CPUs)
> > + *
> > + * If CPUState object exists,then it gets a CPU matching
> > + * @index in the possible CPU array.
> > + *
> > + * Returns: The possible CPU or %NULL if CPU does not exist.
> > + */
> > +CPUState *qemu_get_possible_cpu(int index);
> > +
> > +/**
> > + * qemu_present_cpu:
> > + * @cpu: The vCPU to check
> > + *
> > + * Checks if the vCPU is amongst the present possible vcpus.
> > + *
> > + * Returns: True if it is present possible vCPU else false
> > + */
> > +bool qemu_present_cpu(CPUState *cpu);
> > +
> > +/**
> > + * qemu_enabled_cpu:
> > + * @cpu: The vCPU to check
> > + *
> > + * Checks if the vCPU is enabled.
> > + *
> > + * Returns: True if it is 'enabled' else false
> > + */
> > +bool qemu_enabled_cpu(CPUState *cpu);
> > +
> > +/**
> > + * qemu_get_cpu_archid:
> > + * @cpu_index: possible vCPU for which arch-id needs to be retreived
> > + *
> > + * Fetches the vCPU arch-id from the present possible vCPUs.
> > + *
> > + * Returns: arch-id of the possible vCPU
> > + */
> > +uint64_t qemu_get_cpu_archid(int cpu_index);
> > +