Re: [Qemu-devel] [PATCH 2/4] qom/cpu: Add cluster_index to CPUState

[Luc Michel]

On 1/8/19 5:30 PM, Peter Maydell wrote:
> For TCG we want to distinguish which cluster a CPU is in, and
> we need to do it quickly. Cache the cluster index in the CPUState
> struct, by having the cluster object set cpu->cluster_index for
> each CPU child when it is realized.
> 
> This means that board/SoC code must add all CPUs to the cluster
> before realizing the cluster object. Regrettably QOM provides no
> way to prevent adding children to a realized object and no way for
> the parent to be notified when a new child is added to it, so
> we don't have any way to enforce/assert this constraint; all
> we can do is document it in a comment.
> 
> The restriction on how many clusters can exist in the system
> is imposed by TCG code which will be added in a subsequent commit,
> but the check to enforce it in cluster.c fits better in this one.
> 
> Signed-off-by: Peter Maydell <address@hidden>
Reviewed-by: Luc Michel <address@hidden>
> ---
>  include/hw/cpu/cluster.h | 19 +++++++++++++++++++
>  include/qom/cpu.h        |  7 +++++++
>  hw/cpu/cluster.c         | 33 +++++++++++++++++++++++++++++++++
>  qom/cpu.c                |  1 +
>  4 files changed, 60 insertions(+)
> 
> diff --git a/include/hw/cpu/cluster.h b/include/hw/cpu/cluster.h
> index 73818232437..d1bef315d10 100644
> --- a/include/hw/cpu/cluster.h
> +++ b/include/hw/cpu/cluster.h
> @@ -34,12 +34,31 @@
>   * Arm big.LITTLE system) they should be in different clusters. 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 clusters.
> + *
> + * A cluster is created by creating an object of TYPE_CPU_CLUSTER, and then
> + * adding the CPUs to it as QOM child objects (e.g. using the
> + * object_initialize_child() or object_property_add_child() functions).
> + * All CPUs must be added as children before the cluster is realized.
> + * (Regrettably QOM provides no way to prevent adding children to a realized
> + * object and no way for the parent to be notified when a new child is added
> + * to it, so this restriction is not checked for, but the system will not
> + * behave correctly if it is not adhered to.)
> + *
> + * A CPU which is not put into any cluster will be considered implicitly
> + * to be in a cluster with all the other "loose" CPUs, so all CPUs that are
> + * not assigned to clusters must be identical.
>   */
>  
>  #define TYPE_CPU_CLUSTER "cpu-cluster"
>  #define CPU_CLUSTER(obj) \
>      OBJECT_CHECK(CPUClusterState, (obj), TYPE_CPU_CLUSTER)
>  
> +/*
> + * This limit is imposed by TCG, which puts the cluster ID into an
> + * 8 bit field (and uses all-1s for the default "not in any cluster").
> + */
> +#define MAX_CLUSTERS 255
> +
>  /**
>   * CPUClusterState:
>   * @cluster_id: The cluster ID. This value is for internal use only and 
> should
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 1396f53e5b5..844becbcedc 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -279,6 +279,11 @@ struct qemu_work_item;
>  /**
>   * CPUState:
>   * @cpu_index: CPU index (informative).
> + * @cluster_index: Identifies which cluster this CPU is in.
> + *   For boards which don't define clusters or for "loose" CPUs not assigned
> + *   to a cluster this will be UNASSIGNED_CLUSTER_INDEX; otherwise it will
> + *   be the same as the cluster-id property of the CPU object's 
> TYPE_CPU_CLUSTER
> + *   QOM parent.
>   * @nr_cores: Number of cores within this CPU package.
>   * @nr_threads: Number of threads within this CPU.
>   * @running: #true if CPU is currently running (lockless).
> @@ -404,6 +409,7 @@ struct CPUState {
>  
>      /* TODO Move common fields from CPUArchState here. */
>      int cpu_index;
> +    int cluster_index;
>      uint32_t halted;
>      uint32_t can_do_io;
>      int32_t exception_index;
> @@ -1109,5 +1115,6 @@ extern const struct VMStateDescription 
> vmstate_cpu_common;
>  #endif /* NEED_CPU_H */
>  
>  #define UNASSIGNED_CPU_INDEX -1
> +#define UNASSIGNED_CLUSTER_INDEX -1
>  
>  #endif
> diff --git a/hw/cpu/cluster.c b/hw/cpu/cluster.c
> index 9d50a235d5c..d672f54a620 100644
> --- a/hw/cpu/cluster.c
> +++ b/hw/cpu/cluster.c
> @@ -20,19 +20,52 @@
>  
>  #include "qemu/osdep.h"
>  #include "hw/cpu/cluster.h"
> +#include "qom/cpu.h"
>  #include "qapi/error.h"
>  #include "qemu/module.h"
> +#include "qemu/cutils.h"
>  
>  static Property cpu_cluster_properties[] = {
>      DEFINE_PROP_UINT32("cluster-id", CPUClusterState, cluster_id, 0),
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> +static void cpu_cluster_realize(DeviceState *dev, Error **errp)
> +{
> +    /* Iterate through all our CPU children and set their cluster_index */
> +    CPUClusterState *cluster = CPU_CLUSTER(dev);
> +    ObjectPropertyIterator iter;
> +    ObjectProperty *prop;
> +    Object *cluster_obj = OBJECT(dev);
> +
> +    if (cluster->cluster_id >= MAX_CLUSTERS) {
> +        error_setg(errp, "cluster-id must be less than %d", MAX_CLUSTERS);
> +        return;
> +    }
> +
> +    object_property_iter_init(&iter, cluster_obj);
> +    while ((prop = object_property_iter_next(&iter))) {
> +        Object *cpu_obj;
> +        CPUState *cpu;
> +
> +        if (!strstart(prop->type, "child<", NULL)) {
> +            continue;
> +        }
> +        cpu_obj = object_property_get_link(cluster_obj, prop->name, NULL);
> +        cpu = (CPUState *)object_dynamic_cast(cpu_obj, TYPE_CPU);
> +        if (!cpu) {
> +            continue;
> +        }
> +        cpu->cluster_index = cluster->cluster_id;
> +    }
> +}
> +
>  static void cpu_cluster_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->props = cpu_cluster_properties;
> +    dc->realize = cpu_cluster_realize;
>  }
>  
>  static const TypeInfo cpu_cluster_type_info = {
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 5442a7323be..f5579b1cd50 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -364,6 +364,7 @@ static void cpu_common_initfn(Object *obj)
>      CPUClass *cc = CPU_GET_CLASS(obj);
>  
>      cpu->cpu_index = UNASSIGNED_CPU_INDEX;
> +    cpu->cluster_index = UNASSIGNED_CLUSTER_INDEX;
>      cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
>      /* *-user doesn't have configurable SMP topology */
>      /* the default value is changed by qemu_init_vcpu() for softmmu */
>