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

[Gavin Shan]

Hi Daniel,

On 4/4/22 4:39 PM, Daniel P. Berrangé wrote:
> On Sun, Apr 03, 2022 at 10:59:51PM +0800, Gavin Shan 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.
> >
> > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > ---
> >   hw/arm/virt.c | 16 +++++++++++++++-
> >   1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index d2e5ecd234..3174526730 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,8 +2519,21 @@ 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->smp.sockets;
> > +        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->smp.clusters;
> > +        ms->possible_cpus->cpus[n].props.has_core_id = true;
> > +        ms->possible_cpus->cpus[n].props.core_id =
> > +            (n / ms->smp.threads) % ms->smp.cores;
> >           ms->possible_cpus->cpus[n].props.has_thread_id = true;
> > -        ms->possible_cpus->cpus[n].props.thread_id = n;
> > +        ms->possible_cpus->cpus[n].props.thread_id =
> > +            n % ms->smp.threads;
>
> Does this need to be conditionalized d behind a machine property, so that
> we don't change behaviour of existing machine type versions ?

I think we probably needn't to do that because the default NUMA node
for the given CPU is returned based on the socket ID in next patch.
The socket ID is calculated in this patch. Otherwise, we will see
CPU topology broken warnings in Linux guest. I think we need to fix
this issue for all machine type versions.

Thanks,
Gavin