[v2] target/arm: Merge arm_cpu_vq_map_next_smaller into sole caller

[PATCH v2] target/arm: Merge arm_cpu_vq_map_next_smaller into sole caller

Posted by Richard Henderson 4 years, 5 months ago

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, the aarch32 stub version of arm_cpu_vq_map_next_smaller,
returning 0, does exactly what Coverity reports.  Remove it.

Second, the aarch64 version of arm_cpu_vq_map_next_smaller has
a set of asserts, but they don't cover the case in question.
Further, there is a fair amount of extra arithmetic needed to
convert from the 0-based zcr register, to the 1-base vq form,
to the 0-based bitmap, and back again.  This can be simplified
by leaving the value in the 0-based form.

Finally, use test_bit to simplify the common case, where the
length in the zcr registers is in fact a supported length.

Reported-by: Coverity (CID 1407217)
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---

v2: Merge arm_cpu_vq_map_next_smaller into sve_zcr_get_valid_len,
    as suggested by Andrew Jones.

    Use test_bit to make the code even more obvious; the
    current_length + 1 thing to let us find current_length in the
    bitmap seemed unnecessarily clever.

---
 target/arm/cpu.h    |  3 ---
 target/arm/cpu64.c  | 15 ---------------
 target/arm/helper.c |  8 ++++++--
 3 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index e1a66a2d1c..47d24a5375 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -185,12 +185,9 @@ typedef struct {
 #ifdef TARGET_AARCH64
 # define ARM_MAX_VQ    16
 void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
-uint32_t arm_cpu_vq_map_next_smaller(ARMCPU *cpu, uint32_t vq);
 #else
 # 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; }
 #endif
 
 typedef struct ARMVectorReg {
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 68baf0482f..a39d6fcea3 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -458,21 +458,6 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
     cpu->sve_max_vq = max_vq;
 }
 
-uint32_t arm_cpu_vq_map_next_smaller(ARMCPU *cpu, uint32_t vq)
-{
-    uint32_t bitnum;
-
-    /*
-     * 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.
-     */
-    assert(vq && vq <= ARM_MAX_VQ + 1);
-
-    bitnum = find_last_bit(cpu->sve_vq_map, vq - 1);
-    return bitnum == vq - 1 ? 0 : bitnum + 1;
-}
-
 static void cpu_max_get_sve_max_vq(Object *obj, Visitor *v, const char *name,
                                    void *opaque, Error **errp)
 {
diff --git a/target/arm/helper.c b/target/arm/helper.c
index be67e2c66d..b5ee35a174 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5363,9 +5363,13 @@ int sve_exception_el(CPUARMState *env, int el)
 
 static uint32_t sve_zcr_get_valid_len(ARMCPU *cpu, uint32_t start_len)
 {
-    uint32_t start_vq = (start_len & 0xf) + 1;
+    uint32_t end_len;
 
-    return arm_cpu_vq_map_next_smaller(cpu, start_vq + 1) - 1;
+    start_len &= 0xf;
+    end_len = find_last_bit(cpu->sve_vq_map, start_len + 1);
+
+    assert(end_len <= start_len);
+    return end_len;
 }
 
 /*
-- 
2.17.1

Re: [PATCH v2] target/arm: Merge arm_cpu_vq_map_next_smaller into sole caller

Posted by Andrew Jones 4 years, 5 months ago

On Sat, Nov 16, 2019 at 12:06:42PM +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, the aarch32 stub version of arm_cpu_vq_map_next_smaller,
> returning 0, does exactly what Coverity reports.  Remove it.
> 
> Second, the aarch64 version of arm_cpu_vq_map_next_smaller has
> a set of asserts, but they don't cover the case in question.
> Further, there is a fair amount of extra arithmetic needed to
> convert from the 0-based zcr register, to the 1-base vq form,
> to the 0-based bitmap, and back again.  This can be simplified
> by leaving the value in the 0-based form.
> 
> Finally, use test_bit to simplify the common case, where the
> length in the zcr registers is in fact a supported length.

I don't see test_bit() getting used in the changes below.

> 
> Reported-by: Coverity (CID 1407217)
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> 
> v2: Merge arm_cpu_vq_map_next_smaller into sve_zcr_get_valid_len,
>     as suggested by Andrew Jones.
> 
>     Use test_bit to make the code even more obvious; the
>     current_length + 1 thing to let us find current_length in the
>     bitmap seemed unnecessarily clever.
> 
> ---
>  target/arm/cpu.h    |  3 ---
>  target/arm/cpu64.c  | 15 ---------------
>  target/arm/helper.c |  8 ++++++--
>  3 files changed, 6 insertions(+), 20 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index e1a66a2d1c..47d24a5375 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -185,12 +185,9 @@ typedef struct {
>  #ifdef TARGET_AARCH64
>  # define ARM_MAX_VQ    16
>  void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
> -uint32_t arm_cpu_vq_map_next_smaller(ARMCPU *cpu, uint32_t vq);
>  #else
>  # 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; }
>  #endif
>  
>  typedef struct ARMVectorReg {
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 68baf0482f..a39d6fcea3 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -458,21 +458,6 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
>      cpu->sve_max_vq = max_vq;
>  }
>  
> -uint32_t arm_cpu_vq_map_next_smaller(ARMCPU *cpu, uint32_t vq)
> -{
> -    uint32_t bitnum;
> -
> -    /*
> -     * 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.
> -     */
> -    assert(vq && vq <= ARM_MAX_VQ + 1);
> -
> -    bitnum = find_last_bit(cpu->sve_vq_map, vq - 1);
> -    return bitnum == vq - 1 ? 0 : bitnum + 1;
> -}
> -
>  static void cpu_max_get_sve_max_vq(Object *obj, Visitor *v, const char *name,
>                                     void *opaque, Error **errp)
>  {
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index be67e2c66d..b5ee35a174 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5363,9 +5363,13 @@ int sve_exception_el(CPUARMState *env, int el)
>  
>  static uint32_t sve_zcr_get_valid_len(ARMCPU *cpu, uint32_t start_len)
>  {
> -    uint32_t start_vq = (start_len & 0xf) + 1;
> +    uint32_t end_len;
>  
> -    return arm_cpu_vq_map_next_smaller(cpu, start_vq + 1) - 1;
> +    start_len &= 0xf;
> +    end_len = find_last_bit(cpu->sve_vq_map, start_len + 1);
> +
> +    assert(end_len <= start_len);
> +    return end_len;
>  }
>  
>  /*
> -- 
> 2.17.1
> 
>

Besides the commit message referencing test_bit, but no use of it, this
looks good to me

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH v2] target/arm: Merge arm_cpu_vq_map_next_smaller into sole caller

Posted by Richard Henderson 4 years, 5 months ago

On 11/18/19 8:58 AM, Andrew Jones wrote:
> On Sat, Nov 16, 2019 at 12:06:42PM +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, the aarch32 stub version of arm_cpu_vq_map_next_smaller,
>> returning 0, does exactly what Coverity reports.  Remove it.
>>
>> Second, the aarch64 version of arm_cpu_vq_map_next_smaller has
>> a set of asserts, but they don't cover the case in question.
>> Further, there is a fair amount of extra arithmetic needed to
>> convert from the 0-based zcr register, to the 1-base vq form,
>> to the 0-based bitmap, and back again.  This can be simplified
>> by leaving the value in the 0-based form.
>>
>> Finally, use test_bit to simplify the common case, where the
>> length in the zcr registers is in fact a supported length.
> 
> I don't see test_bit() getting used in the changes below.

Argh!  It's still uncommitted here in my tree.
I guess I forgot the -a on the git commit --append?

V3 coming up...


r~

> 
>>
>> Reported-by: Coverity (CID 1407217)
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>
>> v2: Merge arm_cpu_vq_map_next_smaller into sve_zcr_get_valid_len,
>>     as suggested by Andrew Jones.
>>
>>     Use test_bit to make the code even more obvious; the
>>     current_length + 1 thing to let us find current_length in the
>>     bitmap seemed unnecessarily clever.
>>
>> ---
>>  target/arm/cpu.h    |  3 ---
>>  target/arm/cpu64.c  | 15 ---------------
>>  target/arm/helper.c |  8 ++++++--
>>  3 files changed, 6 insertions(+), 20 deletions(-)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index e1a66a2d1c..47d24a5375 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -185,12 +185,9 @@ typedef struct {
>>  #ifdef TARGET_AARCH64
>>  # define ARM_MAX_VQ    16
>>  void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
>> -uint32_t arm_cpu_vq_map_next_smaller(ARMCPU *cpu, uint32_t vq);
>>  #else
>>  # 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; }
>>  #endif
>>  
>>  typedef struct ARMVectorReg {
>> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
>> index 68baf0482f..a39d6fcea3 100644
>> --- a/target/arm/cpu64.c
>> +++ b/target/arm/cpu64.c
>> @@ -458,21 +458,6 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
>>      cpu->sve_max_vq = max_vq;
>>  }
>>  
>> -uint32_t arm_cpu_vq_map_next_smaller(ARMCPU *cpu, uint32_t vq)
>> -{
>> -    uint32_t bitnum;
>> -
>> -    /*
>> -     * 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.
>> -     */
>> -    assert(vq && vq <= ARM_MAX_VQ + 1);
>> -
>> -    bitnum = find_last_bit(cpu->sve_vq_map, vq - 1);
>> -    return bitnum == vq - 1 ? 0 : bitnum + 1;
>> -}
>> -
>>  static void cpu_max_get_sve_max_vq(Object *obj, Visitor *v, const char *name,
>>                                     void *opaque, Error **errp)
>>  {
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index be67e2c66d..b5ee35a174 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -5363,9 +5363,13 @@ int sve_exception_el(CPUARMState *env, int el)
>>  
>>  static uint32_t sve_zcr_get_valid_len(ARMCPU *cpu, uint32_t start_len)
>>  {
>> -    uint32_t start_vq = (start_len & 0xf) + 1;
>> +    uint32_t end_len;
>>  
>> -    return arm_cpu_vq_map_next_smaller(cpu, start_vq + 1) - 1;
>> +    start_len &= 0xf;
>> +    end_len = find_last_bit(cpu->sve_vq_map, start_len + 1);
>> +
>> +    assert(end_len <= start_len);
>> +    return end_len;
>>  }
>>  
>>  /*
>> -- 
>> 2.17.1
>>
>>
> 
> Besides the commit message referencing test_bit, but no use of it, this
> looks good to me
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>
>