[Qemu-devel] [PATCH for-2.1] target-arm: Fix bit test in sp_el0_access

[Qemu-devel] [PATCH for-2.1] target-arm: Fix bit test in sp_el0_access

[Stefan Weil]

Static code analyzers complain about a dubious & operation used for a
boolean value. The code does not test the PSTATE_SP bit as it should.

Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Stefan Weil <sw@weilnetz.de>
---

Hello Peter,

I'm not sure whether the "!" is correct at all, because code and comment
don't seem to match. But I am not an ARM expert, so please review.

Thanks,
Stefan

 target-arm/helper.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index d343856..6ecaa61 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1853,7 +1853,7 @@ static uint64_t aa64_dczid_read(CPUARMState *env, const ARMCPRegInfo *ri)
 
 static CPAccessResult sp_el0_access(CPUARMState *env, const ARMCPRegInfo *ri)
 {
-    if (!env->pstate & PSTATE_SP) {
+    if (!(env->pstate & PSTATE_SP)) {
         /* Access to SP_EL0 is undefined if it's being used as
          * the stack pointer.
          */
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH for-2.1] target-arm: Fix bit test in sp_el0_access

[Peter Maydell]

On 26 July 2014 08:26, Stefan Weil <sw@weilnetz.de> wrote:
> Static code analyzers complain about a dubious & operation used for a
> boolean value. The code does not test the PSTATE_SP bit as it should.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>
> Hello Peter,
>
> I'm not sure whether the "!" is correct at all, because code and comment
> don't seem to match. But I am not an ARM expert, so please review.

Code and comment are both correct (apart from the precedence
error). The PSTATE_SP bit is 0 if the CPU should use SP_EL0
regardless of the current exception level, and 1 if the CPU uses
the SP for the current exception level. So we wish to trap if the
bit is zero.

The bug is comparatively speaking fairly harmless, because
the effect is just that we won't ever trap if a badly behaved
guest tries to modify its own SP this way. (The definition of
pstate plus the fact the register has .access = PL1_RW
means env->pstate can't ever be zero here, so the condition
is always false.) So I'm tempted to let this slide into 2.2.

> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index d343856..6ecaa61 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1853,7 +1853,7 @@ static uint64_t aa64_dczid_read(CPUARMState *env, const ARMCPRegInfo *ri)
>
>  static CPAccessResult sp_el0_access(CPUARMState *env, const ARMCPRegInfo *ri)
>  {
> -    if (!env->pstate & PSTATE_SP) {
> +    if (!(env->pstate & PSTATE_SP)) {
>          /* Access to SP_EL0 is undefined if it's being used as
>           * the stack pointer.
>           */

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.1] target-arm: Fix bit test in sp_el0_access

[Peter Maydell]

On 26 July 2014 08:26, Stefan Weil <sw@weilnetz.de> wrote:
> Static code analyzers complain about a dubious & operation used for a
> boolean value. The code does not test the PSTATE_SP bit as it should.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>
> Hello Peter,
>
> I'm not sure whether the "!" is correct at all, because code and comment
> don't seem to match. But I am not an ARM expert, so please review.
>
> Thanks,
> Stefan
>
>  target-arm/helper.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index d343856..6ecaa61 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1853,7 +1853,7 @@ static uint64_t aa64_dczid_read(CPUARMState *env, const ARMCPRegInfo *ri)
>
>  static CPAccessResult sp_el0_access(CPUARMState *env, const ARMCPRegInfo *ri)
>  {
> -    if (!env->pstate & PSTATE_SP) {
> +    if (!(env->pstate & PSTATE_SP)) {
>          /* Access to SP_EL0 is undefined if it's being used as
>           * the stack pointer.
>           */
> --

Applied to target-arm.next.

thanks
-- PMM