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

[Gavin Shan <gshan@redhat.com>]

Hi Salil,

On 10/2/23 20:21, Salil Mehta wrote:
>> 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.
> 

There are two lists (arrays): ms->possible_cpus->cpus[] and cpus-common.c::cpus.
The former one is a board's property and the later is a global property. In our
implementation, the vCPU state depends on ms->possible_cpus->cpus[], for example:

- The possible vCPU is determined by checking its index falls in the range of
   [0, ms->possible_cpus->len]
- The present vCPU is determined by checking ms->possible_cpus->cpus[index].cpu

However, other two states have to be determined by checking CPUState

- CPUState::acpi_persistent, for always-present vCPUs
- CPUState::disabled, for enabled vCPU

As suggested in other replies, we may manage the vCPU states from board level
due the fact: the vCPU state changes on Creation, hot-add or hot-remove, which
are all driven by a board. Besides, the hotplug handler is managed by board.
Lastly, scatting the information in different places (ms->possible_cpus->cpus[]
and CPUState), which helps to determine vCPU states, seems not a good idea.

In order to maintain all the information in board level, 'struct CPUArchId'
need some adaption like below. With it, the vCPU states can be determined
from ms->possible_cpus.

#define CPU_ARCH_ID_FLAG_ALWAYS_PRESENT		(1UL << 0)
#define CPU_ARCH_ID_FLAG_ENABLED		(1UL << 1)
typedef struct CPUArchId {
     unsigned long flags
        :
};

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

Ok, then please make it inline at least.

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

Ok, I think the association between this field and MDAT still need to be
mentioned.
  
>>
>>> @@ -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);
>>> +

Thanks,
Gavin