[PATCH] target/arm: Clean up arm_cpu_vq_map_next_smaller asserts

[PATCH] target/arm: Clean up arm_cpu_vq_map_next_smaller asserts

[Richard Henderson]

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 <richard.henderson@linaro.org>
---
 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,
-- 
2.17.1

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

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> 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 <richard.henderson@linaro.org>

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

> ---
>  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

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 <richard.henderson@linaro.org>
> ---
>  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
> 

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

[Richard Henderson]

On 11/15/19 5:06 PM, Andrew Jones wrote:
>>      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...

Eh?  It's required by the spec that 128-bit vectors are always supported.


> 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() ?

But that makes sense all on its own, so we don't do quite so much +1/-1 faffing
about.


r~

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

[Andrew Jones]

On Fri, Nov 15, 2019 at 06:45:51PM +0100, Richard Henderson wrote:
> On 11/15/19 5:06 PM, Andrew Jones wrote:
> >>      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...
> 
> Eh?  It's required by the spec that 128-bit vectors are always supported.

If some vendor messes things up with SVE in a way that makes it impossible
to configure all should-be-supported lengths, then there's a chance KVM
will simply not advertise the lengths that cannot be configured as a
workaround. This may be quite unlikely, but when KVM is in use, IMO, it
should be the sole authority on what lengths are available. Assuming
lengths are available because the spec says so should work, but then
'hardware' is just another way to spell 'errata'...

> 
> 
> > 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() ?
> 
> But that makes sense all on its own, so we don't do quite so much +1/-1 faffing
> about.
>

Thanks,
drew