hw/misc/imx6_src: Don't crash trying to reset missing CPUs

[Qemu-devel] [PATCH for-2.9] hw/misc/imx6_src: Don't crash trying to reset missing CPUs

Posted by Peter Maydell 7 years, 1 month ago

Commit 4881658a4b introduced a call to arm_get_cpu_by_id(),
and Coverity noticed that we weren't checking that it didn't
return NULL (CID 1371652).

Normally this won't happen (because all 4 CPUs are expected
to exist), but it's possible the user requested fewer CPUs
on the command line. Handle this possibility by silently
doing nothing, which is the same behaviour as before commit
4881658a4b and also how we handle the other CPU operations
(since we ignore the INVALID_PARAM returns from arm_set_cpu_on()
and friends).

There is a slight behavioural difference to the pre-4881658a4b
situation: the "reset this core" bit will remain set rather
than not being permitted to be set. The imx6 datasheet is
unclear about the behaviour in this odd corner case, so we
opt for the simpler code rather than complicated logic to
maintain identical behaviour.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I couldn't actually get this to crash even with -smp 1 with
my test image, but we should fix it anyhow.

 hw/misc/imx6_src.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/misc/imx6_src.c b/hw/misc/imx6_src.c
index edbb756..cfb0871 100644
--- a/hw/misc/imx6_src.c
+++ b/hw/misc/imx6_src.c
@@ -143,13 +143,17 @@ static void imx6_defer_clear_reset_bit(int cpuid,
                                        unsigned long reset_shift)
 {
     struct SRCSCRResetInfo *ri;
+    CPUState *cpu = arm_get_cpu_by_id(cpuid);
+
+    if (!cpu) {
+        return;
+    }
 
     ri = g_malloc(sizeof(struct SRCSCRResetInfo));
     ri->s = s;
     ri->reset_bit = reset_shift;
 
-    async_run_on_cpu(arm_get_cpu_by_id(cpuid), imx6_clear_reset_bit,
-                     RUN_ON_CPU_HOST_PTR(ri));
+    async_run_on_cpu(cpu, imx6_clear_reset_bit, RUN_ON_CPU_HOST_PTR(ri));
 }
 
 
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.9] hw/misc/imx6_src: Don't crash trying to reset missing CPUs

Posted by Peter Maydell 7 years, 1 month ago

Ping for review...

thanks
-- PMM

On 3 March 2017 at 12:59, Peter Maydell <peter.maydell@linaro.org> wrote:
> Commit 4881658a4b introduced a call to arm_get_cpu_by_id(),
> and Coverity noticed that we weren't checking that it didn't
> return NULL (CID 1371652).
>
> Normally this won't happen (because all 4 CPUs are expected
> to exist), but it's possible the user requested fewer CPUs
> on the command line. Handle this possibility by silently
> doing nothing, which is the same behaviour as before commit
> 4881658a4b and also how we handle the other CPU operations
> (since we ignore the INVALID_PARAM returns from arm_set_cpu_on()
> and friends).
>
> There is a slight behavioural difference to the pre-4881658a4b
> situation: the "reset this core" bit will remain set rather
> than not being permitted to be set. The imx6 datasheet is
> unclear about the behaviour in this odd corner case, so we
> opt for the simpler code rather than complicated logic to
> maintain identical behaviour.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I couldn't actually get this to crash even with -smp 1 with
> my test image, but we should fix it anyhow.
>
>  hw/misc/imx6_src.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/misc/imx6_src.c b/hw/misc/imx6_src.c
> index edbb756..cfb0871 100644
> --- a/hw/misc/imx6_src.c
> +++ b/hw/misc/imx6_src.c
> @@ -143,13 +143,17 @@ static void imx6_defer_clear_reset_bit(int cpuid,
>                                         unsigned long reset_shift)
>  {
>      struct SRCSCRResetInfo *ri;
> +    CPUState *cpu = arm_get_cpu_by_id(cpuid);
> +
> +    if (!cpu) {
> +        return;
> +    }
>
>      ri = g_malloc(sizeof(struct SRCSCRResetInfo));
>      ri->s = s;
>      ri->reset_bit = reset_shift;
>
> -    async_run_on_cpu(arm_get_cpu_by_id(cpuid), imx6_clear_reset_bit,
> -                     RUN_ON_CPU_HOST_PTR(ri));
> +    async_run_on_cpu(cpu, imx6_clear_reset_bit, RUN_ON_CPU_HOST_PTR(ri));
>  }
>
>
> --
> 2.7.4

Re: [Qemu-devel] [PATCH for-2.9] hw/misc/imx6_src: Don't crash trying to reset missing CPUs

Posted by Alex Bennée 7 years, 1 month ago

Peter Maydell <peter.maydell@linaro.org> writes:

> Commit 4881658a4b introduced a call to arm_get_cpu_by_id(),
> and Coverity noticed that we weren't checking that it didn't
> return NULL (CID 1371652).
>
> Normally this won't happen (because all 4 CPUs are expected
> to exist), but it's possible the user requested fewer CPUs
> on the command line. Handle this possibility by silently
> doing nothing, which is the same behaviour as before commit
> 4881658a4b and also how we handle the other CPU operations
> (since we ignore the INVALID_PARAM returns from arm_set_cpu_on()
> and friends).
>
> There is a slight behavioural difference to the pre-4881658a4b
> situation: the "reset this core" bit will remain set rather
> than not being permitted to be set. The imx6 datasheet is
> unclear about the behaviour in this odd corner case, so we
> opt for the simpler code rather than complicated logic to
> maintain identical behaviour.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I couldn't actually get this to crash even with -smp 1 with
> my test image, but we should fix it anyhow.
>
>  hw/misc/imx6_src.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/misc/imx6_src.c b/hw/misc/imx6_src.c
> index edbb756..cfb0871 100644
> --- a/hw/misc/imx6_src.c
> +++ b/hw/misc/imx6_src.c
> @@ -143,13 +143,17 @@ static void imx6_defer_clear_reset_bit(int cpuid,
>                                         unsigned long reset_shift)
>  {
>      struct SRCSCRResetInfo *ri;
> +    CPUState *cpu = arm_get_cpu_by_id(cpuid);
> +
> +    if (!cpu) {
> +        return;
> +    }
>
>      ri = g_malloc(sizeof(struct SRCSCRResetInfo));
>      ri->s = s;
>      ri->reset_bit = reset_shift;
>
> -    async_run_on_cpu(arm_get_cpu_by_id(cpuid), imx6_clear_reset_bit,
> -                     RUN_ON_CPU_HOST_PTR(ri));
> +    async_run_on_cpu(cpu, imx6_clear_reset_bit, RUN_ON_CPU_HOST_PTR(ri));
>  }

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


--
Alex Bennée

Re: [Qemu-devel] [PATCH for-2.9] hw/misc/imx6_src: Don't crash trying to reset missing CPUs

Posted by Peter Maydell 7 years, 1 month ago

On 14 March 2017 at 17:12, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> Commit 4881658a4b introduced a call to arm_get_cpu_by_id(),
>> and Coverity noticed that we weren't checking that it didn't
>> return NULL (CID 1371652).
>>
>> Normally this won't happen (because all 4 CPUs are expected
>> to exist), but it's possible the user requested fewer CPUs
>> on the command line. Handle this possibility by silently
>> doing nothing, which is the same behaviour as before commit
>> 4881658a4b and also how we handle the other CPU operations
>> (since we ignore the INVALID_PARAM returns from arm_set_cpu_on()
>> and friends).
>>
>> There is a slight behavioural difference to the pre-4881658a4b
>> situation: the "reset this core" bit will remain set rather
>> than not being permitted to be set. The imx6 datasheet is
>> unclear about the behaviour in this odd corner case, so we
>> opt for the simpler code rather than complicated logic to
>> maintain identical behaviour.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> I couldn't actually get this to crash even with -smp 1 with
>> my test image, but we should fix it anyhow.
>>
>>  hw/misc/imx6_src.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/misc/imx6_src.c b/hw/misc/imx6_src.c
>> index edbb756..cfb0871 100644
>> --- a/hw/misc/imx6_src.c
>> +++ b/hw/misc/imx6_src.c
>> @@ -143,13 +143,17 @@ static void imx6_defer_clear_reset_bit(int cpuid,
>>                                         unsigned long reset_shift)
>>  {
>>      struct SRCSCRResetInfo *ri;
>> +    CPUState *cpu = arm_get_cpu_by_id(cpuid);
>> +
>> +    if (!cpu) {
>> +        return;
>> +    }
>>
>>      ri = g_malloc(sizeof(struct SRCSCRResetInfo));
>>      ri->s = s;
>>      ri->reset_bit = reset_shift;
>>
>> -    async_run_on_cpu(arm_get_cpu_by_id(cpuid), imx6_clear_reset_bit,
>> -                     RUN_ON_CPU_HOST_PTR(ri));
>> +    async_run_on_cpu(cpu, imx6_clear_reset_bit, RUN_ON_CPU_HOST_PTR(ri));
>>  }
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Thanks; applied to master.

-- PMM