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

[Peter Maydell]

On Mon, 21 Jan 2019 at 20:12, Philippe Mathieu-Daudé <address@hidden> 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 <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", ...

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