RE: [PATCH RFC V4 02/33] cpu-common: Add common CPU utility for possible vCPUs

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

HI Miguel,

>  From: Miguel Luis <miguel.luis@oracle.com>
>  Sent: Thursday, October 10, 2024 3:47 PM
>  To: Salil Mehta <salil.mehta@huawei.com>
>  
>  Hi Salil,
>  
>  > On 9 Oct 2024, at 03:17, Salil Mehta <salil.mehta@huawei.com> wrote:
>  >
>  > This patch adds various utility functions that may be required to
>  > fetch or check the state of possible vCPUs. It also introduces the
>  > concept of *disabled* vCPUs, which are part of the *possible* vCPUs
>  > but are not enabled. This state will be used during machine
>  > initialization and later during the plugging or unplugging of vCPUs. We
>  release the QOM CPU objects for all disabled vCPUs.
>  >
>  > 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>
>  > ---
>  > cpu-common.c          | 21 ++++++++++++++++++++
>  > include/hw/core/cpu.h | 46
>  +++++++++++++++++++++++++++++++++++++++++++
>  > 2 files changed, 67 insertions(+)
>  >
>  > diff --git a/cpu-common.c b/cpu-common.c index 6b262233a3..4a446f6f7f
>  > 100644
>  > --- a/cpu-common.c
>  > +++ b/cpu-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;
>  > @@ -108,6 +109,26 @@ 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;
>  
>  I don’t feel qemu_present_cpu should be needed as cpus are implicitly
>  present as an initialization premise and arm/virt being the only user of this
>  now.


Yes, as explained to you earlier there is a history to it. In the earlier protoypes,
I was planning to hide the persistence of the vCPU object behind this function
but then the same function was also being used at other place within the code
like GICv3. Later, I introduced qemu_enabled_cpu() and acpi_persistent_cpu()
realizing that persistence of vCPU is only required at the ACPI level. And hence
got away with most of the qemu_present_cpu() usages.

Gavin also commented on this earlier. Maybe we can deprecate it.


Thanks
Salil.


>  
>  Thanks
>  Miguel
>  
>  > +}
>  > +
>  > +bool qemu_enabled_cpu(CPUState *cpu)
>  > +{
>  > +    return cpu && !cpu->disabled;
>  > +}
>  > +
>  > CPUState *qemu_get_cpu(int index)
>  > {
>  >     CPUState *cpu;
>  > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index
>  > 1c9c775df6..73a4e4cce1 100644
>  > --- a/include/hw/core/cpu.h
>  > +++ b/include/hw/core/cpu.h
>  > @@ -538,6 +538,20 @@ struct CPUState {
>  >     CPUPluginState *plugin_state;
>  > #endif
>  >
>  > +    /*
>  > +     * In the guest kernel, the presence of vCPUs is determined by
>  information
>  > +     * provided by the VMM or firmware via the ACPI MADT at boot time.
>  Some
>  > +     * architectures do not allow modifications to this configuration after
>  > +     * the guest has booted. Therefore, for such architectures,
>  hotpluggable
>  > +     * vCPUs are exposed by the VMM as not 'ACPI Enabled' to the kernel.
>  > +     * Within QEMU, such vCPUs (those that are 'yet-to-be-plugged' or
>  have
>  > +     * been hot-unplugged) may either have a `CPUState` object in a
>  'disabled'
>  > +     * state or may not have a `CPUState` object at all.
>  > +     *
>  > +     * By default, `CPUState` objects are enabled across all architectures.
>  > +     */
>  > +    bool disabled;
>  > +
>  >     /* TODO Move common fields from CPUArchState here. */
>  >     int cpu_index;
>  >     int cluster_index;
>  > @@ -924,6 +938,38 @@ 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);
>  > +
>  > /**
>  >  * cpu_exists:
>  >  * @id: Guest-exposed CPU ID to lookup.
>  > --
>  > 2.34.1
>  >