Re: [PATCH] target/arm: Clean up arm_cpu_vq_map_next_smaller asserts

[Alex Bennée]

Richard Henderson <address@hidden> writes:

> Coverity reports, in sve_zcr_get_valid_len,
>
> "Subtract operation overflows on operands
> arm_cpu_vq_map_next_smaller(cpu, start_vq + 1U) and 1U"
>
> First, fix the aarch32 stub version to not return 0, but to
> simply assert unreachable.  Because that nonsense return value
> does exactly what Coverity reports.
>
> Second, 1 is the minimum value that can be returned from the
> aarch64 version of arm_cpu_vq_map_next_smaller, but that is
> non-obvious from the set of asserts in the function.  Begin by
> asserting that 2 is the minimum input, and finish by asserting
> that we did in fact find a set bit in the bitmap.  Bit 0 is
> always set, so we must be able to find that.
>
> Reported-by: Coverity (CID 1407217)
> Signed-off-by: Richard Henderson <address@hidden>

Reviewed-by: Alex Bennée <address@hidden>

> ---
>  target/arm/cpu.h   |  4 +++-
>  target/arm/cpu64.c | 11 +++++++++--
>  2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index e1a66a2d1c..d89e727d7b 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -190,7 +190,9 @@ uint32_t arm_cpu_vq_map_next_smaller(ARMCPU *cpu, 
> uint32_t vq);
>  # define ARM_MAX_VQ    1
>  static inline void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp) { }
>  static inline uint32_t arm_cpu_vq_map_next_smaller(ARMCPU *cpu, uint32_t vq)
> -{ return 0; }
> +{
> +    g_assert_not_reached();
> +}
>  #endif
>
>  typedef struct ARMVectorReg {
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 68baf0482f..83ff8c8713 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -466,11 +466,18 @@ uint32_t arm_cpu_vq_map_next_smaller(ARMCPU *cpu, 
> uint32_t vq)
>       * We allow vq == ARM_MAX_VQ + 1 to be input because the caller may want
>       * to find the maximum vq enabled, which may be ARM_MAX_VQ, but this
>       * function always returns the next smaller than the input.
> +     *
> +     * Similarly, vq == 2 is the minimum input because 1 is the minimum
> +     * output that makes sense.
>       */
> -    assert(vq && vq <= ARM_MAX_VQ + 1);
> +    assert(vq >= 2 && vq <= ARM_MAX_VQ + 1);
>
>      bitnum = find_last_bit(cpu->sve_vq_map, vq - 1);
> -    return bitnum == vq - 1 ? 0 : bitnum + 1;
> +
> +    /* We always have vq == 1 present in sve_vq_map.  */
> +    assert(bitnum < vq - 1);
> +
> +    return bitnum + 1;
>  }
>
>  static void cpu_max_get_sve_max_vq(Object *obj, Visitor *v, const char *name,


--
Alex Bennée