| Message ID | 20190121184314.14311-1-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xlnx-zynqmp: Don't create rpu-cluster if there are no RPUs | expand |
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 >
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.
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
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 >
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
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);
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(+) -- 2.20.1