* [PATCH v2] target/arm: Merge arm_cpu_vq_map_next_smaller into sole caller
@ 2019-11-16 11:06 Richard Henderson
2019-11-18 7:58 ` Andrew Jones
0 siblings, 1 reply; 3+ messages in thread
From: Richard Henderson @ 2019-11-16 11:06 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, drjones
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] target/arm: Merge arm_cpu_vq_map_next_smaller into sole caller
2019-11-16 11:06 [PATCH v2] target/arm: Merge arm_cpu_vq_map_next_smaller into sole caller Richard Henderson
@ 2019-11-18 7:58 ` Andrew Jones
2019-11-18 8:27 ` Richard Henderson
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Jones @ 2019-11-18 7:58 UTC (permalink / raw)
To: Richard Henderson; +Cc: peter.maydell, qemu-devel
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>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] target/arm: Merge arm_cpu_vq_map_next_smaller into sole caller
2019-11-18 7:58 ` Andrew Jones
@ 2019-11-18 8:27 ` Richard Henderson
0 siblings, 0 replies; 3+ messages in thread
From: Richard Henderson @ 2019-11-18 8:27 UTC (permalink / raw)
To: Andrew Jones; +Cc: peter.maydell, qemu-devel
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>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-11-18 8:28 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-11-16 11:06 [PATCH v2] target/arm: Merge arm_cpu_vq_map_next_smaller into sole caller Richard Henderson
2019-11-18 7:58 ` Andrew Jones
2019-11-18 8:27 ` Richard Henderson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).