Re: [PULL 04/11] target/arm/cpu64: max cpu: Introduce sve properties

2019-11-15 Thread Richard Henderson
On 11/13/19 10:30 PM, Peter Maydell wrote:
> Coverity may also be looking at the case where
> TARGET_AARCH64 is not defined. The fallback definition
> of arm_cpu_vq_map_next_smaller() for that situation
> always returns 0.

Yeah, that makes more sense.
I think we can make the fallback g_assert_not_reached().

Testing a patch for all this.


r~



Re: [PULL 04/11] target/arm/cpu64: max cpu: Introduce sve properties

2019-11-13 Thread Peter Maydell
On Wed, 13 Nov 2019 at 20:17, Richard Henderson
 wrote:
>
> On 11/12/19 11:23 AM, Peter Maydell wrote:
> >> +static uint32_t sve_zcr_get_valid_len(ARMCPU *cpu, uint32_t start_len)
> >> +{
> >> +uint32_t start_vq = (start_len & 0xf) + 1;
> >> +
> >> +return arm_cpu_vq_map_next_smaller(cpu, start_vq + 1) - 1;
> >
> > "Subtract operation overflows on operands
> > arm_cpu_vq_map_next_smaller(cpu, start_vq + 1U) and 1U"
> >
> > Certainly it looks as if arm_cpu_vq_map_next_smaller() can
> > return 0, and claiming the valid length to be UINT_MAX
> > seems a bit odd in that case.
>
> The lsb is always set in the map, the minimum number we send to next_smaller 
> is
> 2 -> so the minimum number returned from next_smaller is 1.
>
> We should never return UINT_MAX.
>
> > return bitnum == vq - 1 ? 0 : bitnum + 1;
>
> But yes, this computation doesn't seem right.
>
> The beginning assert should probably be (vq >= 2 ...)
> and here we should assert bitnum != vq - 1.

Coverity may also be looking at the case where
TARGET_AARCH64 is not defined. The fallback definition
of arm_cpu_vq_map_next_smaller() for that situation
always returns 0.

thanks
-- PMM



Re: [PULL 04/11] target/arm/cpu64: max cpu: Introduce sve properties

2019-11-13 Thread Richard Henderson
On 11/12/19 11:23 AM, Peter Maydell wrote:
>> +static uint32_t sve_zcr_get_valid_len(ARMCPU *cpu, uint32_t start_len)
>> +{
>> +uint32_t start_vq = (start_len & 0xf) + 1;
>> +
>> +return arm_cpu_vq_map_next_smaller(cpu, start_vq + 1) - 1;
> 
> "Subtract operation overflows on operands
> arm_cpu_vq_map_next_smaller(cpu, start_vq + 1U) and 1U"
> 
> Certainly it looks as if arm_cpu_vq_map_next_smaller() can
> return 0, and claiming the valid length to be UINT_MAX
> seems a bit odd in that case.

The lsb is always set in the map, the minimum number we send to next_smaller is
2 -> so the minimum number returned from next_smaller is 1.

We should never return UINT_MAX.

> return bitnum == vq - 1 ? 0 : bitnum + 1;

But yes, this computation doesn't seem right.

The beginning assert should probably be (vq >= 2 ...)
and here we should assert bitnum != vq - 1.


r~



Re: [PULL 04/11] target/arm/cpu64: max cpu: Introduce sve properties

2019-11-12 Thread Peter Maydell
On Fri, 1 Nov 2019 at 08:51, Peter Maydell  wrote:
>
> From: Andrew Jones 
>
> Introduce cpu properties to give fine control over SVE vector lengths.
> We introduce a property for each valid length up to the current
> maximum supported, which is 2048-bits. The properties are named, e.g.
> sve128, sve256, sve384, sve512, ..., where the number is the number of
> bits. See the updates to docs/arm-cpu-features.rst for a description
> of the semantics and for example uses.
>

Hi; Coverity complains (CID 1407217) about an "overflowed return value":


> +static uint32_t sve_zcr_get_valid_len(ARMCPU *cpu, uint32_t start_len)
> +{
> +uint32_t start_vq = (start_len & 0xf) + 1;
> +
> +return arm_cpu_vq_map_next_smaller(cpu, start_vq + 1) - 1;

"Subtract operation overflows on operands
arm_cpu_vq_map_next_smaller(cpu, start_vq + 1U) and 1U"

Certainly it looks as if arm_cpu_vq_map_next_smaller() can
return 0, and claiming the valid length to be UINT_MAX
seems a bit odd in that case.

thanks
-- PMM



[PULL 04/11] target/arm/cpu64: max cpu: Introduce sve properties

2019-11-01 Thread Peter Maydell
From: Andrew Jones 

Introduce cpu properties to give fine control over SVE vector lengths.
We introduce a property for each valid length up to the current
maximum supported, which is 2048-bits. The properties are named, e.g.
sve128, sve256, sve384, sve512, ..., where the number is the number of
bits. See the updates to docs/arm-cpu-features.rst for a description
of the semantics and for example uses.

Note, as sve-max-vq is still present and we'd like to be able to
support qmp_query_cpu_model_expansion with guests launched with e.g.
-cpu max,sve-max-vq=8 on their command lines, then we do allow
sve-max-vq and sve properties to be provided at the same time, but
this is not recommended, and is why sve-max-vq is not mentioned in the
document.  If sve-max-vq is provided then it enables all lengths smaller
than and including the max and disables all lengths larger. It also has
the side-effect that no larger lengths may be enabled and that the max
itself cannot be disabled. Smaller non-power-of-two lengths may,
however, be disabled, e.g. -cpu max,sve-max-vq=4,sve384=off provides a
guest the vector lengths 128, 256, and 512 bits.

This patch has been co-authored with Richard Henderson, who reworked
the target/arm/cpu64.c changes in order to push all the validation and
auto-enabling/disabling steps into the finalizer, resulting in a nice
LOC reduction.

Signed-off-by: Andrew Jones 
Reviewed-by: Richard Henderson 
Reviewed-by: Eric Auger 
Tested-by: Masayoshi Mizuma 
Reviewed-by: Beata Michalska 
Message-id: 20191031142734.8590-5-drjo...@redhat.com
Signed-off-by: Peter Maydell 
---
 include/qemu/bitops.h |   1 +
 target/arm/cpu.h  |  19 
 target/arm/cpu.c  |  19 
 target/arm/cpu64.c| 192 -
 target/arm/helper.c   |  10 +-
 target/arm/monitor.c  |  12 +++
 tests/arm-cpu-features.c  | 194 ++
 docs/arm-cpu-features.rst | 168 +++--
 8 files changed, 606 insertions(+), 9 deletions(-)

diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
index 3f0926cf40c..ee76552c062 100644
--- a/include/qemu/bitops.h
+++ b/include/qemu/bitops.h
@@ -20,6 +20,7 @@
 #define BITS_PER_LONG   (sizeof (unsigned long) * BITS_PER_BYTE)
 
 #define BIT(nr) (1UL << (nr))
+#define BIT_ULL(nr) (1ULL << (nr))
 #define BIT_MASK(nr)(1UL << ((nr) % BITS_PER_LONG))
 #define BIT_WORD(nr)((nr) / BITS_PER_LONG)
 #define BITS_TO_LONGS(nr)   DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index d844ea21d8d..a044d6028b6 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -184,8 +184,13 @@ typedef struct {
 
 #ifdef TARGET_AARCH64
 # define ARM_MAX_VQ16
+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_VQ1
+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 {
@@ -918,6 +923,18 @@ struct ARMCPU {
 
 /* Used to set the maximum vector length the cpu will support.  */
 uint32_t sve_max_vq;
+
+/*
+ * In sve_vq_map each set bit is a supported vector length of
+ * (bit-number + 1) * 16 bytes, i.e. each bit number + 1 is the vector
+ * length in quadwords.
+ *
+ * While processing properties during initialization, corresponding
+ * sve_vq_init bits are set for bits in sve_vq_map that have been
+ * set by properties.
+ */
+DECLARE_BITMAP(sve_vq_map, ARM_MAX_VQ);
+DECLARE_BITMAP(sve_vq_init, ARM_MAX_VQ);
 };
 
 void arm_cpu_post_init(Object *obj);
@@ -1837,6 +1854,8 @@ static inline int arm_feature(CPUARMState *env, int 
feature)
 return (env->features & (1ULL << feature)) != 0;
 }
 
+void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp);
+
 #if !defined(CONFIG_USER_ONLY)
 /* Return true if exception levels below EL3 are in secure state,
  * or would be following an exception return to that level.
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 72a27ec4b0e..17d1f2b2894 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1198,6 +1198,19 @@ static void arm_cpu_finalizefn(Object *obj)
 #endif
 }
 
+void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
+{
+Error *local_err = NULL;
+
+if (arm_feature(>env, ARM_FEATURE_AARCH64)) {
+arm_cpu_sve_finalize(cpu, _err);
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+return;
+}
+}
+}
+
 static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
 {
 CPUState *cs = CPU(dev);
@@ -1254,6 +1267,12 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
**errp)
 return;
 }
 
+arm_cpu_finalize_features(cpu, _err);
+if (local_err != NULL) {
+