* Re: [Qemu-devel] [PATCH] xlnx-zynqmp: Don't create rpu-cluster if there are no RPUs
2019-01-21 18:43 [Qemu-devel] [PATCH] xlnx-zynqmp: Don't create rpu-cluster if there are no RPUs Peter Maydell
@ 2019-01-21 19:32 ` Edgar E. Iglesias
2019-01-21 20:12 ` Philippe Mathieu-Daudé
2019-01-24 14:12 ` Peter Maydell
2 siblings, 0 replies; 6+ messages in thread
From: Edgar E. Iglesias @ 2019-01-21 19:32 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, qemu-devel, patches, Luc Michel, Alistair Francis
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] xlnx-zynqmp: Don't create rpu-cluster if there are no RPUs
2019-01-21 18:43 [Qemu-devel] [PATCH] xlnx-zynqmp: Don't create rpu-cluster if there are no RPUs Peter Maydell
2019-01-21 19:32 ` Edgar E. Iglesias
@ 2019-01-21 20:12 ` Philippe Mathieu-Daudé
2019-01-22 9:28 ` Peter Maydell
2019-01-24 14:12 ` Peter Maydell
2 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-01-21 20:12 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, Edgar E. Iglesias, Alistair Francis
Cc: qemu-devel, Luc Michel, patches
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] xlnx-zynqmp: Don't create rpu-cluster if there are no RPUs
2019-01-21 20:12 ` Philippe Mathieu-Daudé
@ 2019-01-22 9:28 ` Peter Maydell
2019-01-22 9:43 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2019-01-22 9:28 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-arm, Edgar E. Iglesias, Alistair Francis, QEMU Developers,
Luc Michel, patches@linaro.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] xlnx-zynqmp: Don't create rpu-cluster if there are no RPUs
2019-01-22 9:28 ` Peter Maydell
@ 2019-01-22 9:43 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-01-22 9:43 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-arm, Edgar E. Iglesias, Alistair Francis, QEMU Developers,
Luc Michel, patches@linaro.org
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] xlnx-zynqmp: Don't create rpu-cluster if there are no RPUs
2019-01-21 18:43 [Qemu-devel] [PATCH] xlnx-zynqmp: Don't create rpu-cluster if there are no RPUs Peter Maydell
2019-01-21 19:32 ` Edgar E. Iglesias
2019-01-21 20:12 ` Philippe Mathieu-Daudé
@ 2019-01-24 14:12 ` Peter Maydell
2 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2019-01-24 14:12 UTC (permalink / raw)
To: qemu-arm, QEMU Developers
Cc: patches@linaro.org, Luc Michel, Alistair Francis,
Edgar E. Iglesias
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
^ permalink raw reply [flat|nested] 6+ messages in thread