Re: [PATCH v3 1/4] hw/arm/virt: Consider SMP configuration in CPU topology

[Gavin Shan]

Hi Igor,

On 3/30/22 9:18 PM, Igor Mammedov wrote:
> On Wed, 23 Mar 2022 15:24:35 +0800
> Gavin Shan <gshan@redhat.com> wrote:
>
> > Currently, the SMP configuration isn't considered when the CPU
> > topology is populated. In this case, it's impossible to provide
> > the default CPU-to-NUMA mapping or association based on the socket
> > ID of the given CPU.
> >
> > This takes account of SMP configuration when the CPU topology
> > is populated. The die ID for the given CPU isn't assigned since
> > it's not supported on arm/virt machine yet. Besides, the cluster
> > ID for the given CPU is assigned because it has been supported
> > on arm/virt machine.
> >
> > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > ---
> >   hw/arm/virt.c     | 11 +++++++++++
> >   qapi/machine.json |  6 ++++--
> >   2 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index d2e5ecd234..064eac42f7 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -2505,6 +2505,7 @@ static const CPUArchIdList 
> > *virt_possible_cpu_arch_ids(MachineState *ms)
> >       int n;
> >       unsigned int max_cpus = ms->smp.max_cpus;
> >       VirtMachineState *vms = VIRT_MACHINE(ms);
> > +    MachineClass *mc = MACHINE_GET_CLASS(vms);
> >   
> >       if (ms->possible_cpus) {
> >           assert(ms->possible_cpus->len == max_cpus);
> > @@ -2518,6 +2519,16 @@ static const CPUArchIdList 
> > *virt_possible_cpu_arch_ids(MachineState *ms)
> >           ms->possible_cpus->cpus[n].type = ms->cpu_type;
> >           ms->possible_cpus->cpus[n].arch_id =
> >               virt_cpu_mp_affinity(vms, n);
> > +
> > +        assert(!mc->smp_props.dies_supported);
> > +        ms->possible_cpus->cpus[n].props.has_socket_id = true;
> > +        ms->possible_cpus->cpus[n].props.socket_id =
> > +            n / (ms->smp.clusters * ms->smp.cores * ms->smp.threads);
> > +        ms->possible_cpus->cpus[n].props.has_cluster_id = true;
> > +        ms->possible_cpus->cpus[n].props.cluster_id =
> > +            n / (ms->smp.cores * ms->smp.threads);
> > +        ms->possible_cpus->cpus[n].props.has_core_id = true;
> > +        ms->possible_cpus->cpus[n].props.core_id = n / ms->smp.threads;
> >           ms->possible_cpus->cpus[n].props.has_thread_id = true;
> >           ms->possible_cpus->cpus[n].props.thread_id = n;
>
> shouldn't be above values calculated similar to the way they are
> calculated in x86_topo_ids_from_idx()? /note '% foo' part/

I think it's fine not to have '% foo' here. However, I think it'd
better to have the similar '% foo' as x86 does. I will add this part
in v4.

> >       }
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index 42fc68403d..99c945f258 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -868,10 +868,11 @@
> >   # @node-id: NUMA node ID the CPU belongs to
> >   # @socket-id: socket number within node/board the CPU belongs to
> >   # @die-id: die number within socket the CPU belongs to (since 4.1)
> > -# @core-id: core number within die the CPU belongs to
> > +# @cluster-id: cluster number within die the CPU belongs to
> > +# @core-id: core number within cluster the CPU belongs to
> >   # @thread-id: thread number within core the CPU belongs to
> >   #
> > -# Note: currently there are 5 properties that could be present
> > +# Note: currently there are 6 properties that could be present
> >   #       but management should be prepared to pass through other
> >   #       properties with device_add command to allow for future
> >   #       interface extension. This also requires the filed names to be kept in
> > @@ -883,6 +884,7 @@
> >     'data': { '*node-id': 'int',
> >               '*socket-id': 'int',
> >               '*die-id': 'int',
> > +            '*cluster-id': 'int',
> >               '*core-id': 'int',
> >               '*thread-id': 'int'
> >     }

Thanks,
Gavin