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

[Gavin Shan]

Hi Yanan,

On 4/21/22 7:50 PM, wangyanan (Y) wrote:
> Hi Gavin,
> Sorry I missed the v6.

No problem at all. thanks for your review again :)

> On 2022/4/20 18:49, 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. Besides, the used SMP
> > configuration in qtest/numa-test/aarch64_numa_cpu() is corrcted
> > to avoid testing failure
> >
> > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > Reviewed-by: Yanan Wang <wangyanan55@huawei.com>
> > ---
> >   hw/arm/virt.c           | 15 ++++++++++++++-
> >   tests/qtest/numa-test.c |  3 ++-
> >   2 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index d2e5ecd234..5443ecae92 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,20 @@ 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));
> nit: so the outermost "()" is unnecessary too.

It was kept by intention so that it has same style as to other
fields like cluster_id. I will remove it in v8 and it doesn't
matter actually.

> > +        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;
> >       }
> >       return ms->possible_cpus;
> >   }
> > diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
> > index 90bf68a5b3..aeda8c774c 100644
> > --- a/tests/qtest/numa-test.c
> > +++ b/tests/qtest/numa-test.c
> > @@ -223,7 +223,8 @@ static void aarch64_numa_cpu(const void *data)
> >       QTestState *qts;
> >       g_autofree char *cli = NULL;
> > -    cli = make_cli(data, "-machine smp.cpus=2 "
> > +    cli = make_cli(data, "-machine "
> > +        "smp.cpus=2,smp.sockets=1,smp.clusters=1,smp.cores=1,smp.threads=2 "
> >           "-numa node,nodeid=0,memdev=ram -numa node,nodeid=1 "
> >           "-numa cpu,node-id=1,thread-id=0 "
> >           "-numa cpu,node-id=0,thread-id=1");

As discussed with Igor, the changes to test/qtest/numa-test.c will
be split into a separate patch in v8, which goes before this one.
I assume your reviewed-by tag is still valid, even for the separate
patch.

Thanks,
Gavin