| Message ID | 20191115131623.322-1-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Clean up arm_cpu_vq_map_next_smaller asserts | expand |
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
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 >
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~
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
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,
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(-) -- 2.17.1