| Message ID | 20191116110642.12454-1-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] target/arm: Merge arm_cpu_vq_map_next_smaller into sole caller | expand |
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>
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> >
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; } /*
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(-) -- 2.17.1