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

[Andrew Jones]

On Fri, Nov 15, 2019 at 02:16:23PM +0100, Richard Henderson wrote:
> 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>
> ---
>  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();
> +}

This is indeed a better way to implement a stub.

>  #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);

This makes sense since nobody should request the next-smaller than
the smallest.

>  
>      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.  */

This is true with TCG and 99.9999% likely to be true with KVM, but we
take pains to not assume it's true in all SVE paths shared with KVM. This
function isn't currently used by KVM, but nothing about it looks TCG
specific. Maybe we should just remove this function and put the
find_last_bit() call and all input/output validation directly in
sve_zcr_get_valid_len() ?

Thanks,
drew

> +    assert(bitnum < vq - 1);
> +
> +    return bitnum + 1;
>  }
>  
>  static void cpu_max_get_sve_max_vq(Object *obj, Visitor *v, const char *name,
> -- 
> 2.17.1
>