xlnx-zynqmp: Don't create rpu-cluster if there are no RPUs

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

Posted by Peter Maydell 6 years, 9 months ago

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 <peter.maydell@linaro.org>
---
This is a preparatory patch that is necessary for the series
"[PATCH v3 0/4] tcg: support heterogenous CPU clusters"
(20190121152218.9592-1-peter.maydell@linaro.org)
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);
 
+    if (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);
-- 
2.20.1

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

Posted by Edgar E. Iglesias 6 years, 9 months ago

On Mon, Jan 21, 2019 at 06:43:14PM +0000, 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.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This is a preparatory patch that is necessary for the series
> "[PATCH v3 0/4] tcg: support heterogenous CPU clusters"
> (20190121152218.9592-1-peter.maydell@linaro.org)
> 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);
>  
> +    if (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);
> -- 
> 2.20.1
>

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

Posted by Philippe Mathieu-Daudé 6 years, 9 months ago

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 <peter.maydell@linaro.org>
> ---
> This is a preparatory patch that is necessary for the series
> "[PATCH v3 0/4] tcg: support heterogenous CPU clusters"
> (20190121152218.9592-1-peter.maydell@linaro.org)
> 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.

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

Posted by Peter Maydell 6 years, 9 months ago

On Mon, 21 Jan 2019 at 20:12, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> 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 <peter.maydell@linaro.org>
> > ---
> > This is a preparatory patch that is necessary for the series
> > "[PATCH v3 0/4] tcg: support heterogenous CPU clusters"
> > (20190121152218.9592-1-peter.maydell@linaro.org)
> > 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", ...

Oops, nice catch.

> 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;
> +        }
>      }

Yeah, that would work too. I think I would just go for
using "if (num_rpus <= 0)" in the function, though.

thanks
-- PMM

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

Posted by Philippe Mathieu-Daudé 6 years, 9 months ago

On 1/22/19 10:28 AM, Peter Maydell wrote:
> On Mon, 21 Jan 2019 at 20:12, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> 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 <peter.maydell@linaro.org>
>>> ---
>>> This is a preparatory patch that is necessary for the series
>>> "[PATCH v3 0/4] tcg: support heterogenous CPU clusters"
>>> (20190121152218.9592-1-peter.maydell@linaro.org)
>>> 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", ...
> 
> Oops, nice catch.
> 
>> 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;
>> +        }
>>      }
> 
> Yeah, that would work too. I think I would just go for
> using "if (num_rpus <= 0)" in the function, though.

OK, whichever patch you prefer, you can add:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Regards,

Phil.

> 
> thanks
> -- PMM
>

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

Posted by Peter Maydell 6 years, 9 months ago

On Mon, 21 Jan 2019 at 18:43, Peter Maydell <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org>
> ---
> This is a preparatory patch that is necessary for the series
> "[PATCH v3 0/4] tcg: support heterogenous CPU clusters"
> (20190121152218.9592-1-peter.maydell@linaro.org)
> 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);
>
> +    if (num_rpus == 0) {
> +        /* Don't create rpu-cluster object if there's nothing to put in it */
> +        return;
> +    }

Applied to target-arm.next with a fixup to test for "<= 0".

thanks
-- PMM