[v2] hw/arm/sbsa-ref: add "reg" property to DT cpu nodes

[PATCH v2] hw/arm/sbsa-ref: add "reg" property to DT cpu nodes

Posted by Leif Lindholm 3 years, 8 months ago

The sbsa-ref platform uses a minimal device tree to pass amount of memory
as well as number of cpus to the firmware. However, when dumping that
minimal dtb (with -M sbsa-virt,dumpdtb=<file>), the resulting blob
generates a warning when decompiled by dtc due to lack of reg property.

Add a simple reg property per cpu, representing a 64-bit MPIDR_EL1.

This also ends up being cleaner than having the firmware calculating its
own IDs for generating APCI.

Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---

As per Graeme's feedback, properly represent the MPIDR topology info
in the reg property rather than just counting cores (and update the
commit message on why this is useful).
I'm using the local helper function sbsa_ref_cpu_mp_affinity() for this,
and moving it up somewhat rather than adding a forward declaration.

 hw/arm/sbsa-ref.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index f030a416fd..3e65ded9a0 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -138,6 +138,12 @@ static const int sbsa_ref_irqmap[] = {
     [SBSA_EHCI] = 11,
 };
 
+static uint64_t sbsa_ref_cpu_mp_affinity(SBSAMachineState *sms, int idx)
+{
+    uint8_t clustersz = ARM_DEFAULT_CPUS_PER_CLUSTER;
+    return arm_cpu_mp_affinity(idx, clustersz);
+}
+
 /*
  * Firmware on this machine only uses ACPI table to load OS, these limited
  * device tree nodes are just to let firmware know the info which varies from
@@ -183,14 +189,31 @@ static void create_fdt(SBSAMachineState *sms)
         g_free(matrix);
     }
 
+    /*
+     * From Documentation/devicetree/bindings/arm/cpus.yaml
+     *  On ARM v8 64-bit systems this property is required
+     *    and matches the MPIDR_EL1 register affinity bits.
+     *
+     *    * If cpus node's #address-cells property is set to 2
+     *
+     *      The first reg cell bits [7:0] must be set to
+     *      bits [39:32] of MPIDR_EL1.
+     *
+     *      The second reg cell bits [23:0] must be set to
+     *      bits [23:0] of MPIDR_EL1.
+     */
     qemu_fdt_add_subnode(sms->fdt, "/cpus");
+    qemu_fdt_setprop_cell(sms->fdt, "/cpus", "#address-cells", 2);
+    qemu_fdt_setprop_cell(sms->fdt, "/cpus", "#size-cells", 0x0);
 
     for (cpu = sms->smp_cpus - 1; cpu >= 0; cpu--) {
         char *nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
         ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(cpu));
         CPUState *cs = CPU(armcpu);
+        uint64_t mpidr = sbsa_ref_cpu_mp_affinity(sms, cpu);
 
         qemu_fdt_add_subnode(sms->fdt, nodename);
+        qemu_fdt_setprop_u64(sms->fdt, nodename, "reg", mpidr);
 
         if (ms->possible_cpus->cpus[cs->cpu_index].props.has_node_id) {
             qemu_fdt_setprop_cell(sms->fdt, nodename, "numa-node-id",
@@ -717,12 +740,6 @@ static void sbsa_ref_init(MachineState *machine)
     arm_load_kernel(ARM_CPU(first_cpu), machine, &sms->bootinfo);
 }
 
-static uint64_t sbsa_ref_cpu_mp_affinity(SBSAMachineState *sms, int idx)
-{
-    uint8_t clustersz = ARM_DEFAULT_CPUS_PER_CLUSTER;
-    return arm_cpu_mp_affinity(idx, clustersz);
-}
-
 static const CPUArchIdList *sbsa_ref_possible_cpu_arch_ids(MachineState *ms)
 {
     unsigned int max_cpus = ms->smp.max_cpus;
-- 
2.20.1

Re: [PATCH v2] hw/arm/sbsa-ref: add "reg" property to DT cpu nodes

Posted by Philippe Mathieu-Daudé 3 years, 8 months ago

Le jeu. 27 août 2020 14:45, Leif Lindholm <leif@nuviainc.com> a écrit :

> The sbsa-ref platform uses a minimal device tree to pass amount of memory
> as well as number of cpus to the firmware. However, when dumping that
> minimal dtb (with -M sbsa-virt,dumpdtb=<file>), the resulting blob
> generates a warning when decompiled by dtc due to lack of reg property.
>
> Add a simple reg property per cpu, representing a 64-bit MPIDR_EL1.
>
> This also ends up being cleaner than having the firmware calculating its
> own IDs for generating APCI.
>
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

---
>
> As per Graeme's feedback, properly represent the MPIDR topology info
> in the reg property rather than just counting cores (and update the
> commit message on why this is useful).
> I'm using the local helper function sbsa_ref_cpu_mp_affinity() for this,
> and moving it up somewhat rather than adding a forward declaration.
>
>  hw/arm/sbsa-ref.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index f030a416fd..3e65ded9a0 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -138,6 +138,12 @@ static const int sbsa_ref_irqmap[] = {
>      [SBSA_EHCI] = 11,
>  };
>
> +static uint64_t sbsa_ref_cpu_mp_affinity(SBSAMachineState *sms, int idx)
> +{
> +    uint8_t clustersz = ARM_DEFAULT_CPUS_PER_CLUSTER;
> +    return arm_cpu_mp_affinity(idx, clustersz);
> +}
> +
>  /*
>   * Firmware on this machine only uses ACPI table to load OS, these limited
>   * device tree nodes are just to let firmware know the info which varies
> from
> @@ -183,14 +189,31 @@ static void create_fdt(SBSAMachineState *sms)
>          g_free(matrix);
>      }
>
> +    /*
> +     * From Documentation/devicetree/bindings/arm/cpus.yaml
> +     *  On ARM v8 64-bit systems this property is required
> +     *    and matches the MPIDR_EL1 register affinity bits.
> +     *
> +     *    * If cpus node's #address-cells property is set to 2
> +     *
> +     *      The first reg cell bits [7:0] must be set to
> +     *      bits [39:32] of MPIDR_EL1.
> +     *
> +     *      The second reg cell bits [23:0] must be set to
> +     *      bits [23:0] of MPIDR_EL1.
> +     */
>      qemu_fdt_add_subnode(sms->fdt, "/cpus");
> +    qemu_fdt_setprop_cell(sms->fdt, "/cpus", "#address-cells", 2);
> +    qemu_fdt_setprop_cell(sms->fdt, "/cpus", "#size-cells", 0x0);
>
>      for (cpu = sms->smp_cpus - 1; cpu >= 0; cpu--) {
>          char *nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
>          ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(cpu));
>          CPUState *cs = CPU(armcpu);
> +        uint64_t mpidr = sbsa_ref_cpu_mp_affinity(sms, cpu);
>
>          qemu_fdt_add_subnode(sms->fdt, nodename);
> +        qemu_fdt_setprop_u64(sms->fdt, nodename, "reg", mpidr);
>
>          if (ms->possible_cpus->cpus[cs->cpu_index].props.has_node_id) {
>              qemu_fdt_setprop_cell(sms->fdt, nodename, "numa-node-id",
> @@ -717,12 +740,6 @@ static void sbsa_ref_init(MachineState *machine)
>      arm_load_kernel(ARM_CPU(first_cpu), machine, &sms->bootinfo);
>  }
>
> -static uint64_t sbsa_ref_cpu_mp_affinity(SBSAMachineState *sms, int idx)
> -{
> -    uint8_t clustersz = ARM_DEFAULT_CPUS_PER_CLUSTER;
> -    return arm_cpu_mp_affinity(idx, clustersz);
> -}
> -
>  static const CPUArchIdList *sbsa_ref_possible_cpu_arch_ids(MachineState
> *ms)
>  {
>      unsigned int max_cpus = ms->smp.max_cpus;
> --
> 2.20.1
>
>
>

Re: [PATCH v2] hw/arm/sbsa-ref: add "reg" property to DT cpu nodes

Posted by Peter Maydell 3 years, 8 months ago

On Thu, 27 Aug 2020 at 13:43, Leif Lindholm <leif@nuviainc.com> wrote:
>
> The sbsa-ref platform uses a minimal device tree to pass amount of memory
> as well as number of cpus to the firmware. However, when dumping that
> minimal dtb (with -M sbsa-virt,dumpdtb=<file>), the resulting blob
> generates a warning when decompiled by dtc due to lack of reg property.
>
> Add a simple reg property per cpu, representing a 64-bit MPIDR_EL1.
>
> This also ends up being cleaner than having the firmware calculating its
> own IDs for generating APCI.
>
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---



Applied to target-arm.next, thanks.

-- PMM