target/arm/cpu.h | 3 --- target/arm/cpu64.c | 15 --------------- target/arm/helper.c | 9 +++++++-- 3 files changed, 7 insertions(+), 20 deletions(-)
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.
v3: 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. (For real this time).
---
target/arm/cpu.h | 3 ---
target/arm/cpu64.c | 15 ---------------
target/arm/helper.c | 9 +++++++--
3 files changed, 7 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..a089fb5a69 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5363,9 +5363,14 @@ 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;
+ end_len = start_len &= 0xf;
+ if (!test_bit(start_len, cpu->sve_vq_map)) {
+ end_len = find_last_bit(cpu->sve_vq_map, start_len);
+ assert(end_len < start_len);
+ }
+ return end_len;
}
/*
--
2.17.1
On Mon, Nov 18, 2019 at 10:14:14AM +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. > > 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. > > v3: 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. (For real this time). > > --- > target/arm/cpu.h | 3 --- > target/arm/cpu64.c | 15 --------------- > target/arm/helper.c | 9 +++++++-- > 3 files changed, 7 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..a089fb5a69 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -5363,9 +5363,14 @@ 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; > + end_len = start_len &= 0xf; > + if (!test_bit(start_len, cpu->sve_vq_map)) { > + end_len = find_last_bit(cpu->sve_vq_map, start_len); > + assert(end_len < start_len); > + } > + return end_len; > } > > /* > -- > 2.17.1 > Reviewed-by: Andrew Jones <drjones@redhat.com>
On Mon, 18 Nov 2019 at 09:31, Andrew Jones <drjones@redhat.com> wrote: > > On Mon, Nov 18, 2019 at 10:14:14AM +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. > > > > 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. > > > > v3: 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. (For real this time). > Reviewed-by: Andrew Jones <drjones@redhat.com> Applied to target-arm.next, thanks. -- PMM
© 2016 - 2024 Red Hat, Inc.