Re: [RFC PATCH v2 2/6] hw/arm/virt: DT: Add cpu-map

[Andrew Jones]

On Tue, Apr 27, 2021 at 11:47:17AM +0200, Philippe Mathieu-Daudé wrote:
> Hi Yanan, Drew,
> 
> On 4/13/21 10:07 AM, Yanan Wang wrote:
> > From: Andrew Jones <drjones@redhat.com>
> > 
> > Support device tree CPU topology descriptions.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> > ---
> >  hw/arm/virt.c         | 41 ++++++++++++++++++++++++++++++++++++++++-
> >  include/hw/arm/virt.h |  1 +
> >  2 files changed, 41 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 9f01d9041b..f4ae60ded9 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -352,10 +352,11 @@ static void fdt_add_cpu_nodes(const VirtMachineState 
> > *vms)
> >      int cpu;
> >      int addr_cells = 1;
> >      const MachineState *ms = MACHINE(vms);
> > +    const VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> >      int smp_cpus = ms->smp.cpus;
> >  
> >      /*
> > -     * From Documentation/devicetree/bindings/arm/cpus.txt
> > +     *  See Linux Documentation/devicetree/bindings/arm/cpus.yaml
> >       *  On ARM v8 64-bit systems value should be set to 2,
> >       *  that corresponds to the MPIDR_EL1 register size.
> >       *  If MPIDR_EL1[63:32] value is equal to 0 on all CPUs
> > @@ -408,8 +409,45 @@ static void fdt_add_cpu_nodes(const VirtMachineState 
> > *vms)
> >                  ms->possible_cpus->cpus[cs->cpu_index].props.node_id);
> >          }
> >  
> > +        if (ms->smp.cpus > 1 && !vmc->no_cpu_topology) {
> > +            qemu_fdt_setprop_cell(ms->fdt, nodename, "phandle",
> > +                                  qemu_fdt_alloc_phandle(ms->fdt));
> > +        }
> > +
> >          g_free(nodename);
> >      }
> > +
> > +    if (ms->smp.cpus > 1 && !vmc->no_cpu_topology) {
> > +        /*
> > +         * See Linux Documentation/devicetree/bindings/cpu/cpu-topology.txt
> > +         * In a SMP system, the hierarchy of CPUs is defined through four
> > +         * entities that are used to describe the layout of physical CPUs
> > +         * in the system: socket/cluster/core/thread.
> > +         */
> > +        qemu_fdt_add_subnode(ms->fdt, "/cpus/cpu-map");
> > +
> > +        for (cpu = ms->smp.cpus - 1; cpu >= 0; cpu--) {
> > +            char *cpu_path = g_strdup_printf("/cpus/cpu@%d", cpu);
> > +            char *map_path;
> > +
> > +            if (ms->smp.threads > 1) {
> > +                map_path = g_strdup_printf(
> > +                    "/cpus/cpu-map/%s%d/%s%d/%s%d",
> > +                    "socket", cpu / (ms->smp.cores * ms->smp.threads),
> > +                    "core", (cpu / ms->smp.threads) % ms->smp.cores,
> > +                    "thread", cpu % ms->smp.threads);
> > +            } else {
> > +                map_path = g_strdup_printf(
> > +                    "/cpus/cpu-map/%s%d/%s%d",
> > +                    "socket", cpu / ms->smp.cores,
> > +                    "core", cpu % ms->smp.cores);
> > +            }
> > +            qemu_fdt_add_path(ms->fdt, map_path);
> > +            qemu_fdt_setprop_phandle(ms->fdt, map_path, "cpu", cpu_path);
> > +            g_free(map_path);
> > +            g_free(cpu_path);
> > +        }
> > +    }
> >  }
> >  
> >  static void fdt_add_its_gic_node(VirtMachineState *vms)
> > @@ -2769,6 +2807,7 @@ static void virt_machine_5_2_options(MachineClass *mc)
> >      virt_machine_6_0_options(mc);
> >      compat_props_add(mc->compat_props, hw_compat_5_2, hw_compat_5_2_len);
> >      vmc->no_secure_gpio = true;
> > +    vmc->no_cpu_topology = true;
> 
> Bare with me because "machine versioning" is something new to me, I was
> expecting it to be only related to migrated fields.
> Why do we need to care about not adding the FDT node in older machines?
> Shouldn't the guest skip unknown FDT nodes?

It probably should, the question is whether it would. Also, the nodes may
not be unknown, so the guest will read the information and set up its
topology as instructed. That topology may not be the same as what was
getting used by default without the topology description. It's possible
that a user's application has a dependency on the topology and if that
topology gets changed under its feat it'll behave differently.

In short, machine versioning isn't just about vmstate, it's also about
keeping a machine type looking the same to the guest.

Now, it's possible that we're being overly cautious here, but this compat
variable doesn't complicate code too much. So I think I'd prefer to use it
than not.

Thanks,
drew