Re: [RFC PATCH 02/18] hw/cpu/cpus: introduce _cpus_ device

[Philippe Mathieu-Daudé]

On 30/3/22 14:56, Damien Hedde wrote:
> This object will be a _cpu-cluster_ generalization and
> is meant to allow create cpus of the same type.
>
> The main goal is that this object, on contrary to _cpu-cluster-_,
> can be used to dynamically create cpus: it does not rely on
> external code to populate the object with cpus.
>
> Allowing the user to create a cpu cluster and each _cpu_
> separately would be hard because of the following reasons:
> + cpu reset need to be handled
> + instantiation and realize of cpu-cluster and the cpus
>    are interleaved
> + cpu cluster must contains only identical cpus and it seems
>    difficult to check that at runtime.
> Therefore we add a new type solving all this constraints.
>
> _cpu-cluster_ will be updated to inherit from this class
> in following commits.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>   include/hw/cpu/cpus.h |  71 +++++++++++++++++++++++
>   hw/cpu/cpus.c         | 127 ++++++++++++++++++++++++++++++++++++++++++
>   hw/cpu/meson.build    |   2 +-
>   3 files changed, 199 insertions(+), 1 deletion(-)
>   create mode 100644 include/hw/cpu/cpus.h
>   create mode 100644 hw/cpu/cpus.c
>
> diff --git a/include/hw/cpu/cpus.h b/include/hw/cpu/cpus.h
> new file mode 100644
> index 0000000000..c65f568ef8
> --- /dev/null
> +++ b/include/hw/cpu/cpus.h
> @@ -0,0 +1,71 @@
> +/*
> + * QEMU CPUs type
> + *
> + * Copyright (c) 2022 GreenSocs
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef HW_CPU_CPUS_H
> +#define HW_CPU_CPUS_H
> +
> +#include "qemu/typedefs.h"
> +#include "hw/qdev-core.h"
> +#include "qom/object.h"
> +
> +/*
> + * This object represent several CPUs which are all identical.

Typo "represents".

> + *
> + * If CPUs are not identical (for example, Cortex-A53 and Cortex-A57 CPUs in an
> + * Arm big.LITTLE system) they should be in different groups. If the CPUs do
> + * not have the same view of memory (for example the main CPU and a management
> + * controller processor) they should be in different groups.

This description calls for a clearer CpusGroupState name instead
of CpusState (which confuses me with CPUState). Alternatively
CpusArrayState.

> + *
> + * This is an abstract class, subclasses are supposed to be created on
> + * per-architecture basis to handle the specifics of the cpu architecture.
> + * Subclasses are meant to be user-creatable (for cold-plug).
> + */
> +
> +#define TYPE_CPUS "cpus"
> +OBJECT_DECLARE_TYPE(CpusState, CpusClass, CPUS)
> +
> +/**
> + * CpusState:
> + * @cpu_type: The type of cpu.
> + * @topology.cpus: The number of cpus in this group.
> + *      Explicity put this field into a topology structure in
> + *      order to eventually update this smoothly with a full
> + *      CpuTopology structure in the future.
> + * @cpus: Array of pointer to cpu objects.
> + */
> +struct CpusState {
> +    /*< private >*/
> +    DeviceState parent_obj;
> +
> +    /*< public >*/
> +    char *cpu_type;
> +    struct {
> +        uint16_t cpus;
> +    } topology;
> +    CPUState **cpus;
> +};