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

[Gavin Shan <gshan@redhat.com>]

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

> 
> 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?

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.

[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.
      */

> @@ -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);
> +

All these descriptive stuff isn't needed after the functions are moved to
include/hw/boards.h, and qemu_get_cpu_archid() is dropped.

>   /**
>    * cpu_exists:
>    * @id: Guest-exposed CPU ID to lookup.

Thanks,
Gavin