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

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 4 Apr 2022 18:48:00 +0800
Gavin Shan <gshan@redhat.com> wrote:

> 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.

Agreed.
Also guest-wise it's ACPI only change, which is 'firmware' part of QEMU,
and by default we don't to version those changes unless we a pressed
into it (i.e the same policy that goes for bundled firmware)

> Thanks,
> Gavin
>