Re: [PATCH] target/xtensa: Make use of 'segment' in pptlb helper less confusing

[Max Filippov]

On Tue, Jul 23, 2024 at 8:14 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Coverity gets confused about the use of the 'segment' variable in the
> pptlb helper function: it thinks that we can take a code path where
> we first initialize it:
>   unsigned segment = XTENSA_MPU_PROBE_B;  // 0x40000000
> and then use that value as a shift count:
>   } else if (nhits == 1 && (env->sregs[MPUENB] & (1u << segment))) {
>
> In fact this isn't possible, beacuse xtensa_mpu_lookup() is passed
> '&segment', and it uses that as an output value, which it will always
> set if it returns nonzero.  But the way the code is currently written
> is confusing to a human reader as well as to Coverity.
>
> Instead of initializing 'segment' at the top of the function with a
> value that's only used in the "nhits == 0" code path, use the
> constant value directly in that code path, and don't initialize
> segment.  This matches the way we use xtensa_mpu_lookup() in its
> other callsites in get_physical_addr_mpu().
>
> Resolves: Coverity CID 1547589
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/xtensa/mmu_helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/xtensa/mmu_helper.c b/target/xtensa/mmu_helper.c
> index 997b21d3890..29b84d5dbf6 100644
> --- a/target/xtensa/mmu_helper.c
> +++ b/target/xtensa/mmu_helper.c
> @@ -991,7 +991,7 @@ uint32_t HELPER(rptlb1)(CPUXtensaState *env, uint32_t s)
>  uint32_t HELPER(pptlb)(CPUXtensaState *env, uint32_t v)
>  {
>      unsigned nhits;
> -    unsigned segment = XTENSA_MPU_PROBE_B;
> +    unsigned segment;

The change suggests that coverity is ok with potentially using
uninitialized value in the shift, but not with the value that would
definitely make the shift illegal, which is a bit odd.

Acked-by: Max Filippov <jcmvbkbc@gmail.com>

>      unsigned bg_segment;
>
>      nhits = xtensa_mpu_lookup(env->mpu_fg, env->config->n_mpu_fg_segments,
> @@ -1005,7 +1005,7 @@ uint32_t HELPER(pptlb)(CPUXtensaState *env, uint32_t v)
>          xtensa_mpu_lookup(env->config->mpu_bg,
>                            env->config->n_mpu_bg_segments,
>                            v, &bg_segment);
> -        return env->config->mpu_bg[bg_segment].attr | segment;
> +        return env->config->mpu_bg[bg_segment].attr | XTENSA_MPU_PROBE_B;
>      }
>  }
>
> --
> 2.34.1
>


-- 
Thanks.
-- Max