Re: [Qemu-arm] [Qemu-devel] [PATCH] xlnx-zynqmp: Don't create rpu-cluster if there are no RPUs

[Philippe Mathieu-Daudé]

Hi Peter,

On 1/21/19 7:43 PM, Peter Maydell wrote:
> If we aren't going to create any RPUs, then don't create the
> rpu-cluster unit. This allows us to add an assertion to the
> cluster object that it contains at least one CPU, which helps
> to avoid bugs in creating clusters and putting CPUs in them.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> This is a preparatory patch that is necessary for the series
> "[PATCH v3 0/4] tcg: support heterogenous CPU clusters"
> (address@hidden)
> in order to avoid the xlnx-zcu102 board asserting if started with
> fewer than 5 CPUs.
> 
>  hw/arm/xlnx-zynqmp.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 370b0e44a38..16cba433cb7 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -178,6 +178,11 @@ static void xlnx_zynqmp_create_rpu(XlnxZynqMPState *s, 
> const char *boot_cpu,
>      int i;
>      int num_rpus = MIN(smp_cpus - XLNX_ZYNQMP_NUM_APU_CPUS, 
> XLNX_ZYNQMP_NUM_RPU_CPUS);

Not related to this patch, but this check seems dangerous, i.e. using
"-smp 2" we get num_rpus=-2 which luckyly doesn't enter the for() loop.

>  
> +    if (num_rpus == 0) {

With the current codebase, you'd have to check for "num_rpus <= 0", ...

> +        /* Don't create rpu-cluster object if there's nothing to put in it */
> +        return;
> +    }
> +
>      object_initialize_child(OBJECT(s), "rpu-cluster", &s->rpu_cluster,
>                              sizeof(s->rpu_cluster), TYPE_CPU_CLUSTER,
>                              &error_abort, NULL);
> 

... because the rpu cluster is still created:

aarch64-softmmu/qemu-system-aarch64 -M xlnx-zcu102 -smp 2 -S -monitor stdio

(qemu) info qom-tree
/machine (xlnx-zcu102-machine)
  /peripheral (container)
  /soc (xlnx,zynqmp)
    /rpu-cluster (cpu-cluster)
    /apu-cluster (cpu-cluster)
      /apu-cpu[1] (cortex-a53-arm-cpu)
        /unnamed-gpio-in[1] (irq)
        /unnamed-gpio-in[3] (irq)
        /unnamed-gpio-in[2] (irq)
        /unnamed-gpio-in[0] (irq)
      /apu-cpu[0] (cortex-a53-arm-cpu)
        /unnamed-gpio-in[1] (irq)
        /unnamed-gpio-in[3] (irq)
        /unnamed-gpio-in[2] (irq)
        /unnamed-gpio-in[0] (irq)


What about this instead?

-- >8 --
@@ -451,10 +451,12 @@ static void xlnx_zynqmp_realize(DeviceState *dev,
Error **errp)
                     "RPUs just use -smp 6.");
     }

-    xlnx_zynqmp_create_rpu(s, boot_cpu, &err);
-    if (err) {
-        error_propagate(errp, err);
-        return;
+    if (smp_cpus > XLNX_ZYNQMP_NUM_APU_CPUS) {
+        xlnx_zynqmp_create_rpu(s, boot_cpu, &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
     }

     if (!s->boot_cpu_ptr) {
---

Regards,

Phil.