[Qemu-devel] [PATCH 0/6] target/arm SVE updates

[Qemu-devel] [PATCH 0/6] target/arm SVE updates

[Richard Henderson]

Patch 1 fixes the SIGFPE that Alex found with --test-sve=3.
Patch 2 fixes a problem pointed out by Laurent, presumably
via inspection.
  
The rest begin enabling cpu features for -cpu max.
I'm still working on SVE itself, but these are standalone
and perhaps worth merging before softfreeze. 
  
 
r~
 

Richard Henderson (6):
  target/arm: Fix SVE signed division vs x86 overflow exception
  target/arm: Fix SVE system register access checks
  target/arm: Prune a57 features from max
  target/arm: Prune a15 features from max
  target/arm: Add ID_ISAR6
  target/arm: Set ISAR bits for -cpu max

 target/arm/cpu.h           |  1 +
 target/arm/cpu.c           | 31 +++++++++++++++++--------
 target/arm/cpu64.c         | 47 ++++++++++++++++++++++++--------------
 target/arm/helper.c        | 13 +++++------
 target/arm/sve_helper.c    | 16 +++++++++----
 target/arm/translate-a64.c |  5 ++--
 6 files changed, 71 insertions(+), 42 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/6] target/arm: Fix SVE signed division vs x86 overflow exception

[Richard Henderson]

We already check for the same condition within the normal integer
sdiv and sdiv64 helpers.  Use a slightly different formation that
does not require deducing the expression type.

Fixes: f97cfd596ed
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/sve_helper.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
index 790cbacd14..7d7fc90566 100644
--- a/target/arm/sve_helper.c
+++ b/target/arm/sve_helper.c
@@ -369,7 +369,13 @@ void HELPER(NAME)(void *vd, void *vn, void *vm, void *vg, uint32_t desc) \
 #define DO_MIN(N, M)  ((N) >= (M) ? (M) : (N))
 #define DO_ABD(N, M)  ((N) >= (M) ? (N) - (M) : (M) - (N))
 #define DO_MUL(N, M)  (N * M)
-#define DO_DIV(N, M)  (M ? N / M : 0)
+
+/* The zero divisor case is architectural; the -1 divisor case works
+ * around the x86 INT_MIN / -1 overflow exception without having to
+ * deduce the minimum integer for the type of the expression.
+ */
+#define DO_SDIV(N, M) (unlikely(M == 0) ? 0 : unlikely(M == -1) ? -N : N / M)
+#define DO_UDIV(N, M) (unlikely(M == 0) ? 0 : N / M)
 
 DO_ZPZZ(sve_and_zpzz_b, uint8_t, H1, DO_AND)
 DO_ZPZZ(sve_and_zpzz_h, uint16_t, H1_2, DO_AND)
@@ -477,11 +483,11 @@ DO_ZPZZ(sve_umulh_zpzz_h, uint16_t, H1_2, do_mulh_h)
 DO_ZPZZ(sve_umulh_zpzz_s, uint32_t, H1_4, do_mulh_s)
 DO_ZPZZ_D(sve_umulh_zpzz_d, uint64_t, do_umulh_d)
 
-DO_ZPZZ(sve_sdiv_zpzz_s, int32_t, H1_4, DO_DIV)
-DO_ZPZZ_D(sve_sdiv_zpzz_d, int64_t, DO_DIV)
+DO_ZPZZ(sve_sdiv_zpzz_s, int32_t, H1_4, DO_SDIV)
+DO_ZPZZ_D(sve_sdiv_zpzz_d, int64_t, DO_SDIV)
 
-DO_ZPZZ(sve_udiv_zpzz_s, uint32_t, H1_4, DO_DIV)
-DO_ZPZZ_D(sve_udiv_zpzz_d, uint64_t, DO_DIV)
+DO_ZPZZ(sve_udiv_zpzz_s, uint32_t, H1_4, DO_UDIV)
+DO_ZPZZ_D(sve_udiv_zpzz_d, uint64_t, DO_UDIV)
 
 /* Note that all bits of the shift are significant
    and not modulo the element size.  */
-- 
2.17.1

[Qemu-devel] [PATCH 2/6] target/arm: Fix SVE system register access checks

[Richard Henderson]

Leave ARM_CP_SVE, removing ARM_CP_FPU; the sve_access_check
produced by the flag already includes fp_access_check.  If
we also check ARM_CP_FPU the double fp_access_check asserts.

Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.c        | 8 ++++----
 target/arm/translate-a64.c | 5 ++---
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index b19c7ace78..a855da045b 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -4393,7 +4393,7 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 static const ARMCPRegInfo zcr_el1_reginfo = {
     .name = "ZCR_EL1", .state = ARM_CP_STATE_AA64,
     .opc0 = 3, .opc1 = 0, .crn = 1, .crm = 2, .opc2 = 0,
-    .access = PL1_RW, .type = ARM_CP_SVE | ARM_CP_FPU,
+    .access = PL1_RW, .type = ARM_CP_SVE,
     .fieldoffset = offsetof(CPUARMState, vfp.zcr_el[1]),
     .writefn = zcr_write, .raw_writefn = raw_write
 };
@@ -4401,7 +4401,7 @@ static const ARMCPRegInfo zcr_el1_reginfo = {
 static const ARMCPRegInfo zcr_el2_reginfo = {
     .name = "ZCR_EL2", .state = ARM_CP_STATE_AA64,
     .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 2, .opc2 = 0,
-    .access = PL2_RW, .type = ARM_CP_SVE | ARM_CP_FPU,
+    .access = PL2_RW, .type = ARM_CP_SVE,
     .fieldoffset = offsetof(CPUARMState, vfp.zcr_el[2]),
     .writefn = zcr_write, .raw_writefn = raw_write
 };
@@ -4409,14 +4409,14 @@ static const ARMCPRegInfo zcr_el2_reginfo = {
 static const ARMCPRegInfo zcr_no_el2_reginfo = {
     .name = "ZCR_EL2", .state = ARM_CP_STATE_AA64,
     .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 2, .opc2 = 0,
-    .access = PL2_RW, .type = ARM_CP_SVE | ARM_CP_FPU,
+    .access = PL2_RW, .type = ARM_CP_SVE,
     .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore
 };
 
 static const ARMCPRegInfo zcr_el3_reginfo = {
     .name = "ZCR_EL3", .state = ARM_CP_STATE_AA64,
     .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 2, .opc2 = 0,
-    .access = PL3_RW, .type = ARM_CP_SVE | ARM_CP_FPU,
+    .access = PL3_RW, .type = ARM_CP_SVE,
     .fieldoffset = offsetof(CPUARMState, vfp.zcr_el[3]),
     .writefn = zcr_write, .raw_writefn = raw_write
 };
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index f986340832..45a6c2a3aa 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1633,11 +1633,10 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
     default:
         break;
     }
-    if ((ri->type & ARM_CP_SVE) && !sve_access_check(s)) {
-        return;
-    }
     if ((ri->type & ARM_CP_FPU) && !fp_access_check(s)) {
         return;
+    } else if ((ri->type & ARM_CP_SVE) && !sve_access_check(s)) {
+        return;
     }
 
     if ((tb_cflags(s->base.tb) & CF_USE_ICOUNT) && (ri->type & ARM_CP_IO)) {
-- 
2.17.1

[Qemu-devel] [PATCH 3/6] target/arm: Prune a57 features from max

[Richard Henderson]

There is no need to re-set these 9 features already
implied by the call to aarch64_a57_initfn.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu64.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 3b4bc73ffa..8040493d5c 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -235,19 +235,10 @@ static void aarch64_max_initfn(Object *obj)
          * whereas the architecture requires them to be present in both if
          * present in either.
          */
-        set_feature(&cpu->env, ARM_FEATURE_V8);
-        set_feature(&cpu->env, ARM_FEATURE_VFP4);
-        set_feature(&cpu->env, ARM_FEATURE_NEON);
-        set_feature(&cpu->env, ARM_FEATURE_AARCH64);
-        set_feature(&cpu->env, ARM_FEATURE_V8_AES);
-        set_feature(&cpu->env, ARM_FEATURE_V8_SHA1);
-        set_feature(&cpu->env, ARM_FEATURE_V8_SHA256);
         set_feature(&cpu->env, ARM_FEATURE_V8_SHA512);
         set_feature(&cpu->env, ARM_FEATURE_V8_SHA3);
         set_feature(&cpu->env, ARM_FEATURE_V8_SM3);
         set_feature(&cpu->env, ARM_FEATURE_V8_SM4);
-        set_feature(&cpu->env, ARM_FEATURE_V8_PMULL);
-        set_feature(&cpu->env, ARM_FEATURE_CRC);
         set_feature(&cpu->env, ARM_FEATURE_V8_ATOMICS);
         set_feature(&cpu->env, ARM_FEATURE_V8_RDM);
         set_feature(&cpu->env, ARM_FEATURE_V8_DOTPROD);
-- 
2.17.1

[Qemu-devel] [PATCH 4/6] target/arm: Prune a15 features from max

[Richard Henderson]

There is no need to re-set these 3 features already
implied by the call to aarch64_a15_initfn.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index aa62315cea..878cc6c7e8 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1796,9 +1796,6 @@ static void arm_max_initfn(Object *obj)
          * since we don't correctly set the ID registers to advertise them,
          */
         set_feature(&cpu->env, ARM_FEATURE_V8);
-        set_feature(&cpu->env, ARM_FEATURE_VFP4);
-        set_feature(&cpu->env, ARM_FEATURE_NEON);
-        set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
         set_feature(&cpu->env, ARM_FEATURE_V8_AES);
         set_feature(&cpu->env, ARM_FEATURE_V8_SHA1);
         set_feature(&cpu->env, ARM_FEATURE_V8_SHA256);
-- 
2.17.1

[Qemu-devel] [PATCH 5/6] target/arm: Add ID_ISAR6

[Richard Henderson]

This register was added to aa32 state by ARMv8.2.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h    | 1 +
 target/arm/cpu.c    | 4 ++++
 target/arm/cpu64.c  | 2 ++
 target/arm/helper.c | 5 ++---
 4 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 6a8441c2dd..1505ac936e 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -813,6 +813,7 @@ struct ARMCPU {
     uint32_t id_isar3;
     uint32_t id_isar4;
     uint32_t id_isar5;
+    uint32_t id_isar6;
     uint64_t id_aa64pfr0;
     uint64_t id_aa64pfr1;
     uint64_t id_aa64dfr0;
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 878cc6c7e8..de1a07a9f1 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1262,6 +1262,7 @@ static void cortex_m3_initfn(Object *obj)
     cpu->id_isar3 = 0x01111110;
     cpu->id_isar4 = 0x01310102;
     cpu->id_isar5 = 0x00000000;
+    cpu->id_isar6 = 0x00000000;
 }
 
 static void cortex_m4_initfn(Object *obj)
@@ -1288,6 +1289,7 @@ static void cortex_m4_initfn(Object *obj)
     cpu->id_isar3 = 0x01111110;
     cpu->id_isar4 = 0x01310102;
     cpu->id_isar5 = 0x00000000;
+    cpu->id_isar6 = 0x00000000;
 }
 
 static void cortex_m33_initfn(Object *obj)
@@ -1316,6 +1318,7 @@ static void cortex_m33_initfn(Object *obj)
     cpu->id_isar3 = 0x01111131;
     cpu->id_isar4 = 0x01310132;
     cpu->id_isar5 = 0x00000000;
+    cpu->id_isar6 = 0x00000000;
     cpu->clidr = 0x00000000;
     cpu->ctr = 0x8000c000;
 }
@@ -1366,6 +1369,7 @@ static void cortex_r5_initfn(Object *obj)
     cpu->id_isar3 = 0x01112131;
     cpu->id_isar4 = 0x0010142;
     cpu->id_isar5 = 0x0;
+    cpu->id_isar6 = 0x0;
     cpu->mp_is_up = true;
     cpu->pmsav7_dregion = 16;
     define_arm_cp_regs(cpu, cortexr5_cp_reginfo);
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 8040493d5c..d0581d59d8 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -139,6 +139,7 @@ static void aarch64_a57_initfn(Object *obj)
     cpu->id_isar3 = 0x01112131;
     cpu->id_isar4 = 0x00011142;
     cpu->id_isar5 = 0x00011121;
+    cpu->id_isar6 = 0;
     cpu->id_aa64pfr0 = 0x00002222;
     cpu->id_aa64dfr0 = 0x10305106;
     cpu->pmceid0 = 0x00000000;
@@ -199,6 +200,7 @@ static void aarch64_a53_initfn(Object *obj)
     cpu->id_isar3 = 0x01112131;
     cpu->id_isar4 = 0x00011142;
     cpu->id_isar5 = 0x00011121;
+    cpu->id_isar6 = 0;
     cpu->id_aa64pfr0 = 0x00002222;
     cpu->id_aa64dfr0 = 0x10305106;
     cpu->id_aa64isar0 = 0x00011120;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index a855da045b..e62f02d4e5 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -4851,11 +4851,10 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 6,
               .access = PL1_R, .type = ARM_CP_CONST,
               .resetvalue = cpu->id_mmfr4 },
-            /* 7 is as yet unallocated and must RAZ */
-            { .name = "ID_ISAR7_RESERVED", .state = ARM_CP_STATE_BOTH,
+            { .name = "ID_ISAR6", .state = ARM_CP_STATE_BOTH,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 7,
               .access = PL1_R, .type = ARM_CP_CONST,
-              .resetvalue = 0 },
+              .resetvalue = cpu->id_isar6 },
             REGINFO_SENTINEL
         };
         define_arm_cp_regs(cpu, v6_idregs);
-- 
2.17.1

[Qemu-devel] [PATCH 6/6] target/arm: Set ISAR bits for -cpu max

[Richard Henderson]

For the supported extensions, fill in the appropriate bits in
ID_ISAR5, ID_ISAR6, ID_AA64ISAR0, ID_AA64ISAR1.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.c   | 24 +++++++++++++++++-------
 target/arm/cpu64.c | 36 ++++++++++++++++++++++++++++--------
 2 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index de1a07a9f1..943c589445 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1795,19 +1795,29 @@ static void arm_max_initfn(Object *obj)
         kvm_arm_set_cpu_features_from_host(cpu);
     } else {
         cortex_a15_initfn(obj);
+
+        set_feature(&cpu->env, ARM_FEATURE_V8_AES);
+        cpu->id_isar5 = deposit32(cpu->id_isar5, 4, 4, 2);
+        set_feature(&cpu->env, ARM_FEATURE_V8_SHA1);
+        cpu->id_isar5 = deposit32(cpu->id_isar5, 8, 4, 1);
+        set_feature(&cpu->env, ARM_FEATURE_V8_SHA256);
+        cpu->id_isar5 = deposit32(cpu->id_isar5, 12, 4, 1);
+        set_feature(&cpu->env, ARM_FEATURE_CRC);
+        cpu->id_isar5 = deposit32(cpu->id_isar5, 16, 4, 1);
+        set_feature(&cpu->env, ARM_FEATURE_V8_RDM);
+        cpu->id_isar5 = deposit32(cpu->id_isar5, 24, 4, 1);
+        set_feature(&cpu->env, ARM_FEATURE_V8_FCMA);
+        cpu->id_isar5 = deposit32(cpu->id_isar5, 28, 4, 1);
+
+        set_feature(&cpu->env, ARM_FEATURE_V8_DOTPROD);
+        cpu->id_isar6 = deposit32(cpu->id_isar6, 4, 4, 1);
+
 #ifdef CONFIG_USER_ONLY
         /* We don't set these in system emulation mode for the moment,
          * since we don't correctly set the ID registers to advertise them,
          */
         set_feature(&cpu->env, ARM_FEATURE_V8);
-        set_feature(&cpu->env, ARM_FEATURE_V8_AES);
-        set_feature(&cpu->env, ARM_FEATURE_V8_SHA1);
-        set_feature(&cpu->env, ARM_FEATURE_V8_SHA256);
         set_feature(&cpu->env, ARM_FEATURE_V8_PMULL);
-        set_feature(&cpu->env, ARM_FEATURE_CRC);
-        set_feature(&cpu->env, ARM_FEATURE_V8_RDM);
-        set_feature(&cpu->env, ARM_FEATURE_V8_DOTPROD);
-        set_feature(&cpu->env, ARM_FEATURE_V8_FCMA);
 #endif
     }
 }
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index d0581d59d8..b24fee45e3 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -230,6 +230,34 @@ static void aarch64_max_initfn(Object *obj)
         kvm_arm_set_cpu_features_from_host(cpu);
     } else {
         aarch64_a57_initfn(obj);
+
+        set_feature(&cpu->env, ARM_FEATURE_V8_SHA512);
+        cpu->id_aa64isar0 = deposit64(cpu->id_aa64isar0, 12, 4, 2);
+
+        set_feature(&cpu->env, ARM_FEATURE_V8_ATOMICS);
+        cpu->id_aa64isar0 = deposit64(cpu->id_aa64isar0, 20, 4, 2);
+
+        set_feature(&cpu->env, ARM_FEATURE_V8_RDM);
+        cpu->id_aa64isar0 = deposit64(cpu->id_aa64isar0, 28, 4, 1);
+        cpu->id_isar5 = deposit32(cpu->id_isar5, 24, 4, 1);
+
+        set_feature(&cpu->env, ARM_FEATURE_V8_SHA3);
+        cpu->id_aa64isar0 = deposit64(cpu->id_aa64isar0, 32, 4, 1);
+
+        set_feature(&cpu->env, ARM_FEATURE_V8_SM3);
+        cpu->id_aa64isar0 = deposit64(cpu->id_aa64isar0, 36, 4, 1);
+
+        set_feature(&cpu->env, ARM_FEATURE_V8_SM4);
+        cpu->id_aa64isar0 = deposit64(cpu->id_aa64isar0, 40, 4, 1);
+
+        set_feature(&cpu->env, ARM_FEATURE_V8_DOTPROD);
+        cpu->id_aa64isar0 = deposit64(cpu->id_aa64isar0, 44, 4, 1);
+        cpu->id_isar6 = deposit32(cpu->id_isar6, 4, 4, 1);
+
+        set_feature(&cpu->env, ARM_FEATURE_V8_FCMA);
+        cpu->id_aa64isar1 = deposit64(cpu->id_aa64isar1, 16, 4, 1);
+        cpu->id_isar5 = deposit32(cpu->id_isar5, 28, 4, 1);
+
 #ifdef CONFIG_USER_ONLY
         /* We don't set these in system emulation mode for the moment,
          * since we don't correctly set the ID registers to advertise them,
@@ -237,15 +265,7 @@ static void aarch64_max_initfn(Object *obj)
          * whereas the architecture requires them to be present in both if
          * present in either.
          */
-        set_feature(&cpu->env, ARM_FEATURE_V8_SHA512);
-        set_feature(&cpu->env, ARM_FEATURE_V8_SHA3);
-        set_feature(&cpu->env, ARM_FEATURE_V8_SM3);
-        set_feature(&cpu->env, ARM_FEATURE_V8_SM4);
-        set_feature(&cpu->env, ARM_FEATURE_V8_ATOMICS);
-        set_feature(&cpu->env, ARM_FEATURE_V8_RDM);
-        set_feature(&cpu->env, ARM_FEATURE_V8_DOTPROD);
         set_feature(&cpu->env, ARM_FEATURE_V8_FP16);
-        set_feature(&cpu->env, ARM_FEATURE_V8_FCMA);
         set_feature(&cpu->env, ARM_FEATURE_SVE);
         /* For usermode -cpu max we can use a larger and more efficient DCZ
          * blocksize since we don't have to follow what the hardware does.
-- 
2.17.1

Re: [Qemu-devel] [PATCH 3/6] target/arm: Prune a57 features from max

[Philippe Mathieu-Daudé]

On 06/28/2018 09:15 PM, Richard Henderson wrote:
> There is no need to re-set these 9 features already
> implied by the call to aarch64_a57_initfn.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/cpu64.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 3b4bc73ffa..8040493d5c 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -235,19 +235,10 @@ static void aarch64_max_initfn(Object *obj)
>           * whereas the architecture requires them to be present in both if
>           * present in either.
>           */
> -        set_feature(&cpu->env, ARM_FEATURE_V8);
> -        set_feature(&cpu->env, ARM_FEATURE_VFP4);
> -        set_feature(&cpu->env, ARM_FEATURE_NEON);
> -        set_feature(&cpu->env, ARM_FEATURE_AARCH64);
> -        set_feature(&cpu->env, ARM_FEATURE_V8_AES);
> -        set_feature(&cpu->env, ARM_FEATURE_V8_SHA1);
> -        set_feature(&cpu->env, ARM_FEATURE_V8_SHA256);
>          set_feature(&cpu->env, ARM_FEATURE_V8_SHA512);
>          set_feature(&cpu->env, ARM_FEATURE_V8_SHA3);
>          set_feature(&cpu->env, ARM_FEATURE_V8_SM3);
>          set_feature(&cpu->env, ARM_FEATURE_V8_SM4);
> -        set_feature(&cpu->env, ARM_FEATURE_V8_PMULL);
> -        set_feature(&cpu->env, ARM_FEATURE_CRC);
>          set_feature(&cpu->env, ARM_FEATURE_V8_ATOMICS);
>          set_feature(&cpu->env, ARM_FEATURE_V8_RDM);
>          set_feature(&cpu->env, ARM_FEATURE_V8_DOTPROD);
> 

Re: [Qemu-devel] [PATCH 4/6] target/arm: Prune a15 features from max

[Philippe Mathieu-Daudé]

On 06/28/2018 09:15 PM, Richard Henderson wrote:
> There is no need to re-set these 3 features already
> implied by the call to aarch64_a15_initfn.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/cpu.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index aa62315cea..878cc6c7e8 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1796,9 +1796,6 @@ static void arm_max_initfn(Object *obj)
>           * since we don't correctly set the ID registers to advertise them,
>           */
>          set_feature(&cpu->env, ARM_FEATURE_V8);
> -        set_feature(&cpu->env, ARM_FEATURE_VFP4);
> -        set_feature(&cpu->env, ARM_FEATURE_NEON);
> -        set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
>          set_feature(&cpu->env, ARM_FEATURE_V8_AES);
>          set_feature(&cpu->env, ARM_FEATURE_V8_SHA1);
>          set_feature(&cpu->env, ARM_FEATURE_V8_SHA256);
> 

Re: [Qemu-devel] [PATCH 1/6] target/arm: Fix SVE signed division vs x86 overflow exception

[Philippe Mathieu-Daudé]

On 06/28/2018 09:15 PM, Richard Henderson wrote:
> We already check for the same condition within the normal integer
> sdiv and sdiv64 helpers.  Use a slightly different formation that
> does not require deducing the expression type.
> 
> Fixes: f97cfd596ed
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/sve_helper.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
> index 790cbacd14..7d7fc90566 100644
> --- a/target/arm/sve_helper.c
> +++ b/target/arm/sve_helper.c
> @@ -369,7 +369,13 @@ void HELPER(NAME)(void *vd, void *vn, void *vm, void *vg, uint32_t desc) \
>  #define DO_MIN(N, M)  ((N) >= (M) ? (M) : (N))
>  #define DO_ABD(N, M)  ((N) >= (M) ? (N) - (M) : (M) - (N))
>  #define DO_MUL(N, M)  (N * M)
> -#define DO_DIV(N, M)  (M ? N / M : 0)
> +
> +/* The zero divisor case is architectural; the -1 divisor case works
> + * around the x86 INT_MIN / -1 overflow exception without having to
> + * deduce the minimum integer for the type of the expression.
> + */
> +#define DO_SDIV(N, M) (unlikely(M == 0) ? 0 : unlikely(M == -1) ? -N : N / M)
> +#define DO_UDIV(N, M) (unlikely(M == 0) ? 0 : N / M)
>  
>  DO_ZPZZ(sve_and_zpzz_b, uint8_t, H1, DO_AND)
>  DO_ZPZZ(sve_and_zpzz_h, uint16_t, H1_2, DO_AND)
> @@ -477,11 +483,11 @@ DO_ZPZZ(sve_umulh_zpzz_h, uint16_t, H1_2, do_mulh_h)
>  DO_ZPZZ(sve_umulh_zpzz_s, uint32_t, H1_4, do_mulh_s)
>  DO_ZPZZ_D(sve_umulh_zpzz_d, uint64_t, do_umulh_d)
>  
> -DO_ZPZZ(sve_sdiv_zpzz_s, int32_t, H1_4, DO_DIV)
> -DO_ZPZZ_D(sve_sdiv_zpzz_d, int64_t, DO_DIV)
> +DO_ZPZZ(sve_sdiv_zpzz_s, int32_t, H1_4, DO_SDIV)
> +DO_ZPZZ_D(sve_sdiv_zpzz_d, int64_t, DO_SDIV)
>  
> -DO_ZPZZ(sve_udiv_zpzz_s, uint32_t, H1_4, DO_DIV)
> -DO_ZPZZ_D(sve_udiv_zpzz_d, uint64_t, DO_DIV)
> +DO_ZPZZ(sve_udiv_zpzz_s, uint32_t, H1_4, DO_UDIV)
> +DO_ZPZZ_D(sve_udiv_zpzz_d, uint64_t, DO_UDIV)
>  
>  /* Note that all bits of the shift are significant
>     and not modulo the element size.  */
> 

Re: [Qemu-devel] [PATCH 2/6] target/arm: Fix SVE system register access checks

[Philippe Mathieu-Daudé]

On 06/28/2018 09:15 PM, Richard Henderson wrote:
> Leave ARM_CP_SVE, removing ARM_CP_FPU; the sve_access_check
> produced by the flag already includes fp_access_check.  If
> we also check ARM_CP_FPU the double fp_access_check asserts.

Maybe we can surround this assert() with #ifdef DEBUG_DISAS...

> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/helper.c        | 8 ++++----
>  target/arm/translate-a64.c | 5 ++---
>  2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index b19c7ace78..a855da045b 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -4393,7 +4393,7 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>  static const ARMCPRegInfo zcr_el1_reginfo = {
>      .name = "ZCR_EL1", .state = ARM_CP_STATE_AA64,
>      .opc0 = 3, .opc1 = 0, .crn = 1, .crm = 2, .opc2 = 0,
> -    .access = PL1_RW, .type = ARM_CP_SVE | ARM_CP_FPU,
> +    .access = PL1_RW, .type = ARM_CP_SVE,
>      .fieldoffset = offsetof(CPUARMState, vfp.zcr_el[1]),
>      .writefn = zcr_write, .raw_writefn = raw_write
>  };
> @@ -4401,7 +4401,7 @@ static const ARMCPRegInfo zcr_el1_reginfo = {
>  static const ARMCPRegInfo zcr_el2_reginfo = {
>      .name = "ZCR_EL2", .state = ARM_CP_STATE_AA64,
>      .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 2, .opc2 = 0,
> -    .access = PL2_RW, .type = ARM_CP_SVE | ARM_CP_FPU,
> +    .access = PL2_RW, .type = ARM_CP_SVE,
>      .fieldoffset = offsetof(CPUARMState, vfp.zcr_el[2]),
>      .writefn = zcr_write, .raw_writefn = raw_write
>  };
> @@ -4409,14 +4409,14 @@ static const ARMCPRegInfo zcr_el2_reginfo = {
>  static const ARMCPRegInfo zcr_no_el2_reginfo = {
>      .name = "ZCR_EL2", .state = ARM_CP_STATE_AA64,
>      .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 2, .opc2 = 0,
> -    .access = PL2_RW, .type = ARM_CP_SVE | ARM_CP_FPU,
> +    .access = PL2_RW, .type = ARM_CP_SVE,
>      .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore
>  };
>  
>  static const ARMCPRegInfo zcr_el3_reginfo = {
>      .name = "ZCR_EL3", .state = ARM_CP_STATE_AA64,
>      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 2, .opc2 = 0,
> -    .access = PL3_RW, .type = ARM_CP_SVE | ARM_CP_FPU,
> +    .access = PL3_RW, .type = ARM_CP_SVE,
>      .fieldoffset = offsetof(CPUARMState, vfp.zcr_el[3]),
>      .writefn = zcr_write, .raw_writefn = raw_write
>  };
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index f986340832..45a6c2a3aa 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -1633,11 +1633,10 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
>      default:
>          break;
>      }
> -    if ((ri->type & ARM_CP_SVE) && !sve_access_check(s)) {
> -        return;
> -    }
>      if ((ri->type & ARM_CP_FPU) && !fp_access_check(s)) {
>          return;
> +    } else if ((ri->type & ARM_CP_SVE) && !sve_access_check(s)) {
> +        return;
>      }
>  
>      if ((tb_cflags(s->base.tb) & CF_USE_ICOUNT) && (ri->type & ARM_CP_IO)) {
> 

Re: [Qemu-devel] [PATCH 5/6] target/arm: Add ID_ISAR6

[Philippe Mathieu-Daudé]

Hi Richard,

On 06/28/2018 09:15 PM, Richard Henderson wrote:
> This register was added to aa32 state by ARMv8.2.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h    | 1 +
>  target/arm/cpu.c    | 4 ++++
>  target/arm/cpu64.c  | 2 ++
>  target/arm/helper.c | 5 ++---
>  4 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 6a8441c2dd..1505ac936e 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -813,6 +813,7 @@ struct ARMCPU {
>      uint32_t id_isar3;
>      uint32_t id_isar4;
>      uint32_t id_isar5;
> +    uint32_t id_isar6;
>      uint64_t id_aa64pfr0;
>      uint64_t id_aa64pfr1;
>      uint64_t id_aa64dfr0;
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 878cc6c7e8..de1a07a9f1 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1262,6 +1262,7 @@ static void cortex_m3_initfn(Object *obj)
>      cpu->id_isar3 = 0x01111110;
>      cpu->id_isar4 = 0x01310102;
>      cpu->id_isar5 = 0x00000000;
> +    cpu->id_isar6 = 0x00000000;
>  }
>  
>  static void cortex_m4_initfn(Object *obj)
> @@ -1288,6 +1289,7 @@ static void cortex_m4_initfn(Object *obj)
>      cpu->id_isar3 = 0x01111110;
>      cpu->id_isar4 = 0x01310102;
>      cpu->id_isar5 = 0x00000000;
> +    cpu->id_isar6 = 0x00000000;
>  }
>  
>  static void cortex_m33_initfn(Object *obj)
> @@ -1316,6 +1318,7 @@ static void cortex_m33_initfn(Object *obj)
>      cpu->id_isar3 = 0x01111131;
>      cpu->id_isar4 = 0x01310132;
>      cpu->id_isar5 = 0x00000000;
> +    cpu->id_isar6 = 0x00000000;
>      cpu->clidr = 0x00000000;
>      cpu->ctr = 0x8000c000;
>  }
> @@ -1366,6 +1369,7 @@ static void cortex_r5_initfn(Object *obj)
>      cpu->id_isar3 = 0x01112131;
>      cpu->id_isar4 = 0x0010142;
>      cpu->id_isar5 = 0x0;
> +    cpu->id_isar6 = 0x0;
>      cpu->mp_is_up = true;
>      cpu->pmsav7_dregion = 16;
>      define_arm_cp_regs(cpu, cortexr5_cp_reginfo);
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 8040493d5c..d0581d59d8 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -139,6 +139,7 @@ static void aarch64_a57_initfn(Object *obj)
>      cpu->id_isar3 = 0x01112131;
>      cpu->id_isar4 = 0x00011142;
>      cpu->id_isar5 = 0x00011121;
> +    cpu->id_isar6 = 0;
>      cpu->id_aa64pfr0 = 0x00002222;
>      cpu->id_aa64dfr0 = 0x10305106;
>      cpu->pmceid0 = 0x00000000;
> @@ -199,6 +200,7 @@ static void aarch64_a53_initfn(Object *obj)
>      cpu->id_isar3 = 0x01112131;
>      cpu->id_isar4 = 0x00011142;
>      cpu->id_isar5 = 0x00011121;
> +    cpu->id_isar6 = 0;
>      cpu->id_aa64pfr0 = 0x00002222;
>      cpu->id_aa64dfr0 = 0x10305106;
>      cpu->id_aa64isar0 = 0x00011120;
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index a855da045b..e62f02d4e5 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -4851,11 +4851,10 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 6,
>                .access = PL1_R, .type = ARM_CP_CONST,
>                .resetvalue = cpu->id_mmfr4 },
> -            /* 7 is as yet unallocated and must RAZ */
> -            { .name = "ID_ISAR7_RESERVED", .state = ARM_CP_STATE_BOTH,

You added this in

    if (arm_feature(env, ARM_FEATURE_V6)) {

Shouldn't this stay same and a new entry added later in

    if (arm_feature(env, ARM_FEATURE_V8)) {

?

> +            { .name = "ID_ISAR6", .state = ARM_CP_STATE_BOTH,

If added under ARM_FEATURE_V8, shouldn't this be ARM_CP_STATE_AA32?

>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 7,
>                .access = PL1_R, .type = ARM_CP_CONST,
> -              .resetvalue = 0 },
> +              .resetvalue = cpu->id_isar6 },
>              REGINFO_SENTINEL
>          };
>          define_arm_cp_regs(cpu, v6_idregs);
> 

Re: [Qemu-devel] [PATCH 6/6] target/arm: Set ISAR bits for -cpu max

[Philippe Mathieu-Daudé]

On 06/28/2018 09:15 PM, Richard Henderson wrote:
> For the supported extensions, fill in the appropriate bits in
> ID_ISAR5, ID_ISAR6, ID_AA64ISAR0, ID_AA64ISAR1.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.c   | 24 +++++++++++++++++-------
>  target/arm/cpu64.c | 36 ++++++++++++++++++++++++++++--------
>  2 files changed, 45 insertions(+), 15 deletions(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index de1a07a9f1..943c589445 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1795,19 +1795,29 @@ static void arm_max_initfn(Object *obj)
>          kvm_arm_set_cpu_features_from_host(cpu);
>      } else {
>          cortex_a15_initfn(obj);
> +
> +        set_feature(&cpu->env, ARM_FEATURE_V8_AES);
> +        cpu->id_isar5 = deposit32(cpu->id_isar5, 4, 4, 2);
> +        set_feature(&cpu->env, ARM_FEATURE_V8_SHA1);
> +        cpu->id_isar5 = deposit32(cpu->id_isar5, 8, 4, 1);
> +        set_feature(&cpu->env, ARM_FEATURE_V8_SHA256);
> +        cpu->id_isar5 = deposit32(cpu->id_isar5, 12, 4, 1);
> +        set_feature(&cpu->env, ARM_FEATURE_CRC);
> +        cpu->id_isar5 = deposit32(cpu->id_isar5, 16, 4, 1);
> +        set_feature(&cpu->env, ARM_FEATURE_V8_RDM);
> +        cpu->id_isar5 = deposit32(cpu->id_isar5, 24, 4, 1);
> +        set_feature(&cpu->env, ARM_FEATURE_V8_FCMA);
> +        cpu->id_isar5 = deposit32(cpu->id_isar5, 28, 4, 1);
> +
> +        set_feature(&cpu->env, ARM_FEATURE_V8_DOTPROD);
> +        cpu->id_isar6 = deposit32(cpu->id_isar6, 4, 4, 1);

Since I don't have ARM_FEATURE_V8_DOTPROD, I now realize this series
goes on top of your "target/arm SVE patches" v6:
http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg07698.html

> +
>  #ifdef CONFIG_USER_ONLY
>          /* We don't set these in system emulation mode for the moment,
>           * since we don't correctly set the ID registers to advertise them,
>           */
>          set_feature(&cpu->env, ARM_FEATURE_V8);
> -        set_feature(&cpu->env, ARM_FEATURE_V8_AES);
> -        set_feature(&cpu->env, ARM_FEATURE_V8_SHA1);
> -        set_feature(&cpu->env, ARM_FEATURE_V8_SHA256);
>          set_feature(&cpu->env, ARM_FEATURE_V8_PMULL);
> -        set_feature(&cpu->env, ARM_FEATURE_CRC);
> -        set_feature(&cpu->env, ARM_FEATURE_V8_RDM);
> -        set_feature(&cpu->env, ARM_FEATURE_V8_DOTPROD);
> -        set_feature(&cpu->env, ARM_FEATURE_V8_FCMA);
>  #endif
>      }
>  }
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index d0581d59d8..b24fee45e3 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -230,6 +230,34 @@ static void aarch64_max_initfn(Object *obj)
>          kvm_arm_set_cpu_features_from_host(cpu);
>      } else {
>          aarch64_a57_initfn(obj);
> +
> +        set_feature(&cpu->env, ARM_FEATURE_V8_SHA512);
> +        cpu->id_aa64isar0 = deposit64(cpu->id_aa64isar0, 12, 4, 2);
> +
> +        set_feature(&cpu->env, ARM_FEATURE_V8_ATOMICS);
> +        cpu->id_aa64isar0 = deposit64(cpu->id_aa64isar0, 20, 4, 2);
> +
> +        set_feature(&cpu->env, ARM_FEATURE_V8_RDM);
> +        cpu->id_aa64isar0 = deposit64(cpu->id_aa64isar0, 28, 4, 1);
> +        cpu->id_isar5 = deposit32(cpu->id_isar5, 24, 4, 1);
> +
> +        set_feature(&cpu->env, ARM_FEATURE_V8_SHA3);
> +        cpu->id_aa64isar0 = deposit64(cpu->id_aa64isar0, 32, 4, 1);
> +
> +        set_feature(&cpu->env, ARM_FEATURE_V8_SM3);
> +        cpu->id_aa64isar0 = deposit64(cpu->id_aa64isar0, 36, 4, 1);
> +
> +        set_feature(&cpu->env, ARM_FEATURE_V8_SM4);
> +        cpu->id_aa64isar0 = deposit64(cpu->id_aa64isar0, 40, 4, 1);
> +
> +        set_feature(&cpu->env, ARM_FEATURE_V8_DOTPROD);
> +        cpu->id_aa64isar0 = deposit64(cpu->id_aa64isar0, 44, 4, 1);
> +        cpu->id_isar6 = deposit32(cpu->id_isar6, 4, 4, 1);
> +
> +        set_feature(&cpu->env, ARM_FEATURE_V8_FCMA);
> +        cpu->id_aa64isar1 = deposit64(cpu->id_aa64isar1, 16, 4, 1);
> +        cpu->id_isar5 = deposit32(cpu->id_isar5, 28, 4, 1);
> +
>  #ifdef CONFIG_USER_ONLY
>          /* We don't set these in system emulation mode for the moment,
>           * since we don't correctly set the ID registers to advertise them,
> @@ -237,15 +265,7 @@ static void aarch64_max_initfn(Object *obj)
>           * whereas the architecture requires them to be present in both if
>           * present in either.
>           */
> -        set_feature(&cpu->env, ARM_FEATURE_V8_SHA512);
> -        set_feature(&cpu->env, ARM_FEATURE_V8_SHA3);
> -        set_feature(&cpu->env, ARM_FEATURE_V8_SM3);
> -        set_feature(&cpu->env, ARM_FEATURE_V8_SM4);
> -        set_feature(&cpu->env, ARM_FEATURE_V8_ATOMICS);
> -        set_feature(&cpu->env, ARM_FEATURE_V8_RDM);
> -        set_feature(&cpu->env, ARM_FEATURE_V8_DOTPROD);
>          set_feature(&cpu->env, ARM_FEATURE_V8_FP16);
> -        set_feature(&cpu->env, ARM_FEATURE_V8_FCMA);
>          set_feature(&cpu->env, ARM_FEATURE_SVE);
>          /* For usermode -cpu max we can use a larger and more efficient DCZ
>           * blocksize since we don't have to follow what the hardware does.
> 

Re: [Qemu-devel] [PATCH 0/6] target/arm SVE updates

[Philippe Mathieu-Daudé]

On 06/28/2018 09:15 PM, Richard Henderson wrote:
> Patch 1 fixes the SIGFPE that Alex found with --test-sve=3.
> Patch 2 fixes a problem pointed out by Laurent, presumably
> via inspection.
>   
> The rest begin enabling cpu features for -cpu max.
> I'm still working on SVE itself, but these are standalone
> and perhaps worth merging before softfreeze.

I realized once reviewed the last patch, that this series is
Based-on: 20180627043328.11531-1-richard.henderson@linaro.org
http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg07698.html

Also probably meaning no-reply@patchew.org is laggy/dead.

> Richard Henderson (6):
>   target/arm: Fix SVE signed division vs x86 overflow exception
>   target/arm: Fix SVE system register access checks
>   target/arm: Prune a57 features from max
>   target/arm: Prune a15 features from max
>   target/arm: Add ID_ISAR6
>   target/arm: Set ISAR bits for -cpu max
> 
>  target/arm/cpu.h           |  1 +
>  target/arm/cpu.c           | 31 +++++++++++++++++--------
>  target/arm/cpu64.c         | 47 ++++++++++++++++++++++++--------------
>  target/arm/helper.c        | 13 +++++------
>  target/arm/sve_helper.c    | 16 +++++++++----
>  target/arm/translate-a64.c |  5 ++--
>  6 files changed, 71 insertions(+), 42 deletions(-)
> 

Re: [Qemu-devel] [PATCH 5/6] target/arm: Add ID_ISAR6

[Philippe Mathieu-Daudé]

On 06/28/2018 09:57 PM, Philippe Mathieu-Daudé wrote:
> Hi Richard,
> 
> On 06/28/2018 09:15 PM, Richard Henderson wrote:
>> This register was added to aa32 state by ARMv8.2.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  target/arm/cpu.h    | 1 +
>>  target/arm/cpu.c    | 4 ++++
>>  target/arm/cpu64.c  | 2 ++
>>  target/arm/helper.c | 5 ++---
>>  4 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index 6a8441c2dd..1505ac936e 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -813,6 +813,7 @@ struct ARMCPU {
>>      uint32_t id_isar3;
>>      uint32_t id_isar4;
>>      uint32_t id_isar5;
>> +    uint32_t id_isar6;
>>      uint64_t id_aa64pfr0;
>>      uint64_t id_aa64pfr1;
>>      uint64_t id_aa64dfr0;
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 878cc6c7e8..de1a07a9f1 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1262,6 +1262,7 @@ static void cortex_m3_initfn(Object *obj)
>>      cpu->id_isar3 = 0x01111110;
>>      cpu->id_isar4 = 0x01310102;
>>      cpu->id_isar5 = 0x00000000;
>> +    cpu->id_isar6 = 0x00000000;
>>  }
>>  
>>  static void cortex_m4_initfn(Object *obj)
>> @@ -1288,6 +1289,7 @@ static void cortex_m4_initfn(Object *obj)
>>      cpu->id_isar3 = 0x01111110;
>>      cpu->id_isar4 = 0x01310102;
>>      cpu->id_isar5 = 0x00000000;
>> +    cpu->id_isar6 = 0x00000000;
>>  }
>>  
>>  static void cortex_m33_initfn(Object *obj)
>> @@ -1316,6 +1318,7 @@ static void cortex_m33_initfn(Object *obj)
>>      cpu->id_isar3 = 0x01111131;
>>      cpu->id_isar4 = 0x01310132;
>>      cpu->id_isar5 = 0x00000000;
>> +    cpu->id_isar6 = 0x00000000;
>>      cpu->clidr = 0x00000000;
>>      cpu->ctr = 0x8000c000;
>>  }
>> @@ -1366,6 +1369,7 @@ static void cortex_r5_initfn(Object *obj)
>>      cpu->id_isar3 = 0x01112131;
>>      cpu->id_isar4 = 0x0010142;
>>      cpu->id_isar5 = 0x0;
>> +    cpu->id_isar6 = 0x0;
>>      cpu->mp_is_up = true;
>>      cpu->pmsav7_dregion = 16;
>>      define_arm_cp_regs(cpu, cortexr5_cp_reginfo);
>> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
>> index 8040493d5c..d0581d59d8 100644
>> --- a/target/arm/cpu64.c
>> +++ b/target/arm/cpu64.c
>> @@ -139,6 +139,7 @@ static void aarch64_a57_initfn(Object *obj)
>>      cpu->id_isar3 = 0x01112131;
>>      cpu->id_isar4 = 0x00011142;
>>      cpu->id_isar5 = 0x00011121;
>> +    cpu->id_isar6 = 0;
>>      cpu->id_aa64pfr0 = 0x00002222;
>>      cpu->id_aa64dfr0 = 0x10305106;
>>      cpu->pmceid0 = 0x00000000;
>> @@ -199,6 +200,7 @@ static void aarch64_a53_initfn(Object *obj)
>>      cpu->id_isar3 = 0x01112131;
>>      cpu->id_isar4 = 0x00011142;
>>      cpu->id_isar5 = 0x00011121;
>> +    cpu->id_isar6 = 0;
>>      cpu->id_aa64pfr0 = 0x00002222;
>>      cpu->id_aa64dfr0 = 0x10305106;
>>      cpu->id_aa64isar0 = 0x00011120;
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index a855da045b..e62f02d4e5 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -4851,11 +4851,10 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 6,
>>                .access = PL1_R, .type = ARM_CP_CONST,
>>                .resetvalue = cpu->id_mmfr4 },
>> -            /* 7 is as yet unallocated and must RAZ */
>> -            { .name = "ID_ISAR7_RESERVED", .state = ARM_CP_STATE_BOTH,
> 
> You added this in
> 
>     if (arm_feature(env, ARM_FEATURE_V6)) {
> 
> Shouldn't this stay same and a new entry added later in
> 
>     if (arm_feature(env, ARM_FEATURE_V8)) {
> 
> ?

I now applied this on top of your "target/arm SVE patches" v6 but still
see this under the ARM_FEATURE_V6 feature check.

> 
>> +            { .name = "ID_ISAR6", .state = ARM_CP_STATE_BOTH,
> 
> If added under ARM_FEATURE_V8, shouldn't this be ARM_CP_STATE_AA32?
> 
>>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 7,
>>                .access = PL1_R, .type = ARM_CP_CONST,
>> -              .resetvalue = 0 },
>> +              .resetvalue = cpu->id_isar6 },
>>              REGINFO_SENTINEL
>>          };
>>          define_arm_cp_regs(cpu, v6_idregs);
>>

Re: [Qemu-devel] [PATCH 5/6] target/arm: Add ID_ISAR6

[Richard Henderson]

On 06/28/2018 05:57 PM, Philippe Mathieu-Daudé wrote:
>> @@ -4851,11 +4851,10 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 6,
>>                .access = PL1_R, .type = ARM_CP_CONST,
>>                .resetvalue = cpu->id_mmfr4 },
>> -            /* 7 is as yet unallocated and must RAZ */
>> -            { .name = "ID_ISAR7_RESERVED", .state = ARM_CP_STATE_BOTH,
> 
> You added this in
> 
>     if (arm_feature(env, ARM_FEATURE_V6)) {
> 
> Shouldn't this stay same and a new entry added later in
> 
>     if (arm_feature(env, ARM_FEATURE_V8)) {
> 
> ?
> 
>> +            { .name = "ID_ISAR6", .state = ARM_CP_STATE_BOTH,
> 
> If added under ARM_FEATURE_V8, shouldn't this be ARM_CP_STATE_AA32?

You'll notice that I'm replacing an entry that
was previously marked "reserved".

I'm not going to second-guess that.


r~

Re: [Qemu-devel] [PATCH 1/6] target/arm: Fix SVE signed division vs x86 overflow exception

[Peter Maydell]

On 29 June 2018 at 01:15, Richard Henderson
<richard.henderson@linaro.org> wrote:
> We already check for the same condition within the normal integer
> sdiv and sdiv64 helpers.  Use a slightly different formation that
> does not require deducing the expression type.
>
> Fixes: f97cfd596ed
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/sve_helper.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
> index 790cbacd14..7d7fc90566 100644
> --- a/target/arm/sve_helper.c
> +++ b/target/arm/sve_helper.c
> @@ -369,7 +369,13 @@ void HELPER(NAME)(void *vd, void *vn, void *vm, void *vg, uint32_t desc) \
>  #define DO_MIN(N, M)  ((N) >= (M) ? (M) : (N))
>  #define DO_ABD(N, M)  ((N) >= (M) ? (N) - (M) : (M) - (N))
>  #define DO_MUL(N, M)  (N * M)
> -#define DO_DIV(N, M)  (M ? N / M : 0)
> +
> +/* The zero divisor case is architectural; the -1 divisor case works
> + * around the x86 INT_MIN / -1 overflow exception without having to
> + * deduce the minimum integer for the type of the expression.
> + */

It works around INT_MIN / -1 being C undefined behaviour: the
need to special-case this is not x86-specific... The required
answer for Arm is just as architectural as the required answer
for division-by-zero (which is also C UB).

> +#define DO_SDIV(N, M) (unlikely(M == 0) ? 0 : unlikely(M == -1) ? -N : N / M)
> +#define DO_UDIV(N, M) (unlikely(M == 0) ? 0 : N / M)
>
>  DO_ZPZZ(sve_and_zpzz_b, uint8_t, H1, DO_AND)
>  DO_ZPZZ(sve_and_zpzz_h, uint16_t, H1_2, DO_AND)
> @@ -477,11 +483,11 @@ DO_ZPZZ(sve_umulh_zpzz_h, uint16_t, H1_2, do_mulh_h)
>  DO_ZPZZ(sve_umulh_zpzz_s, uint32_t, H1_4, do_mulh_s)
>  DO_ZPZZ_D(sve_umulh_zpzz_d, uint64_t, do_umulh_d)
>
> -DO_ZPZZ(sve_sdiv_zpzz_s, int32_t, H1_4, DO_DIV)
> -DO_ZPZZ_D(sve_sdiv_zpzz_d, int64_t, DO_DIV)
> +DO_ZPZZ(sve_sdiv_zpzz_s, int32_t, H1_4, DO_SDIV)
> +DO_ZPZZ_D(sve_sdiv_zpzz_d, int64_t, DO_SDIV)
>
> -DO_ZPZZ(sve_udiv_zpzz_s, uint32_t, H1_4, DO_DIV)
> -DO_ZPZZ_D(sve_udiv_zpzz_d, uint64_t, DO_DIV)
> +DO_ZPZZ(sve_udiv_zpzz_s, uint32_t, H1_4, DO_UDIV)
> +DO_ZPZZ_D(sve_udiv_zpzz_d, uint64_t, DO_UDIV)
>
>  /* Note that all bits of the shift are significant
>     and not modulo the element size.  */

Other than quibbling about the comment,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/6] target/arm: Fix SVE system register access checks

[Peter Maydell]

On 29 June 2018 at 01:15, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Leave ARM_CP_SVE, removing ARM_CP_FPU; the sve_access_check
> produced by the flag already includes fp_access_check.  If
> we also check ARM_CP_FPU the double fp_access_check asserts.
>
> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/6] target/arm: Prune a57 features from max

[Peter Maydell]

On 29 June 2018 at 01:15, Richard Henderson
<richard.henderson@linaro.org> wrote:
> There is no need to re-set these 9 features already
> implied by the call to aarch64_a57_initfn.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu64.c | 9 ---------
>  1 file changed, 9 deletions(-)
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/6] target/arm: Prune a15 features from max

[Peter Maydell]

On 29 June 2018 at 01:15, Richard Henderson
<richard.henderson@linaro.org> wrote:
> There is no need to re-set these 3 features already
> implied by the call to aarch64_a15_initfn.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.c | 3 ---
>  1 file changed, 3 deletions(-)
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/6] target/arm: Add ID_ISAR6

[Peter Maydell]

On 29 June 2018 at 01:15, Richard Henderson
<richard.henderson@linaro.org> wrote:
> This register was added to aa32 state by ARMv8.2.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h    | 1 +
>  target/arm/cpu.c    | 4 ++++
>  target/arm/cpu64.c  | 2 ++
>  target/arm/helper.c | 5 ++---
>  4 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 6a8441c2dd..1505ac936e 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -813,6 +813,7 @@ struct ARMCPU {
>      uint32_t id_isar3;
>      uint32_t id_isar4;
>      uint32_t id_isar5;
> +    uint32_t id_isar6;
>      uint64_t id_aa64pfr0;
>      uint64_t id_aa64pfr1;
>      uint64_t id_aa64dfr0;
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 878cc6c7e8..de1a07a9f1 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1262,6 +1262,7 @@ static void cortex_m3_initfn(Object *obj)
>      cpu->id_isar3 = 0x01111110;
>      cpu->id_isar4 = 0x01310102;
>      cpu->id_isar5 = 0x00000000;
> +    cpu->id_isar6 = 0x00000000;
>  }
>
>  static void cortex_m4_initfn(Object *obj)
> @@ -1288,6 +1289,7 @@ static void cortex_m4_initfn(Object *obj)
>      cpu->id_isar3 = 0x01111110;
>      cpu->id_isar4 = 0x01310102;
>      cpu->id_isar5 = 0x00000000;
> +    cpu->id_isar6 = 0x00000000;
>  }
>
>  static void cortex_m33_initfn(Object *obj)
> @@ -1316,6 +1318,7 @@ static void cortex_m33_initfn(Object *obj)
>      cpu->id_isar3 = 0x01111131;
>      cpu->id_isar4 = 0x01310132;
>      cpu->id_isar5 = 0x00000000;
> +    cpu->id_isar6 = 0x00000000;
>      cpu->clidr = 0x00000000;
>      cpu->ctr = 0x8000c000;
>  }
> @@ -1366,6 +1369,7 @@ static void cortex_r5_initfn(Object *obj)
>      cpu->id_isar3 = 0x01112131;
>      cpu->id_isar4 = 0x0010142;
>      cpu->id_isar5 = 0x0;
> +    cpu->id_isar6 = 0x0;
>      cpu->mp_is_up = true;
>      cpu->pmsav7_dregion = 16;
>      define_arm_cp_regs(cpu, cortexr5_cp_reginfo);
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 8040493d5c..d0581d59d8 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -139,6 +139,7 @@ static void aarch64_a57_initfn(Object *obj)
>      cpu->id_isar3 = 0x01112131;
>      cpu->id_isar4 = 0x00011142;
>      cpu->id_isar5 = 0x00011121;
> +    cpu->id_isar6 = 0;
>      cpu->id_aa64pfr0 = 0x00002222;
>      cpu->id_aa64dfr0 = 0x10305106;
>      cpu->pmceid0 = 0x00000000;
> @@ -199,6 +200,7 @@ static void aarch64_a53_initfn(Object *obj)
>      cpu->id_isar3 = 0x01112131;
>      cpu->id_isar4 = 0x00011142;
>      cpu->id_isar5 = 0x00011121;
> +    cpu->id_isar6 = 0;
>      cpu->id_aa64pfr0 = 0x00002222;
>      cpu->id_aa64dfr0 = 0x10305106;
>      cpu->id_aa64isar0 = 0x00011120;

The ARMCPU struct fields should all be initially cleared to
zero, so you don't really need to explicitly zero-initialize
isar6 all over the place like this. (Compare isar5, which is
I think only set in CPUs that are new enough that their TRMs
mentioned it.) In particular it's somewhat out of place to
zero it in the m3/m4/m33 initfns, since there is no such
register in the v8M Arm ARM. On the other hand it doesn't hurt.

> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index a855da045b..e62f02d4e5 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -4851,11 +4851,10 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 6,
>                .access = PL1_R, .type = ARM_CP_CONST,
>                .resetvalue = cpu->id_mmfr4 },
> -            /* 7 is as yet unallocated and must RAZ */
> -            { .name = "ID_ISAR7_RESERVED", .state = ARM_CP_STATE_BOTH,
> +            { .name = "ID_ISAR6", .state = ARM_CP_STATE_BOTH,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 7,
>                .access = PL1_R, .type = ARM_CP_CONST,
> -              .resetvalue = 0 },
> +              .resetvalue = cpu->id_isar6 },
>              REGINFO_SENTINEL
>          };
>          define_arm_cp_regs(cpu, v6_idregs);

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 6/6] target/arm: Set ISAR bits for -cpu max

[Peter Maydell]

On 29 June 2018 at 01:15, Richard Henderson
<richard.henderson@linaro.org> wrote:
> For the supported extensions, fill in the appropriate bits in
> ID_ISAR5, ID_ISAR6, ID_AA64ISAR0, ID_AA64ISAR1.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

This makes sense, but I'd rather have a bit of time to think
about how exactly we want to handle feature bits vs ID
register values (the current codebase is not entirely
coherent on the topic), so I'd rather not put this in
for softfreeze unless there's a strong reason we should...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/6] target/arm: Fix SVE signed division vs x86 overflow exception

[Peter Maydell]

On 29 June 2018 at 09:29, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 29 June 2018 at 01:15, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>> We already check for the same condition within the normal integer
>> sdiv and sdiv64 helpers.  Use a slightly different formation that
>> does not require deducing the expression type.
>>
>> Fixes: f97cfd596ed
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  target/arm/sve_helper.c | 16 +++++++++++-----
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
>> index 790cbacd14..7d7fc90566 100644
>> --- a/target/arm/sve_helper.c
>> +++ b/target/arm/sve_helper.c
>> @@ -369,7 +369,13 @@ void HELPER(NAME)(void *vd, void *vn, void *vm, void *vg, uint32_t desc) \
>>  #define DO_MIN(N, M)  ((N) >= (M) ? (M) : (N))
>>  #define DO_ABD(N, M)  ((N) >= (M) ? (N) - (M) : (M) - (N))
>>  #define DO_MUL(N, M)  (N * M)
>> -#define DO_DIV(N, M)  (M ? N / M : 0)
>> +
>> +/* The zero divisor case is architectural; the -1 divisor case works
>> + * around the x86 INT_MIN / -1 overflow exception without having to
>> + * deduce the minimum integer for the type of the expression.
>> + */
>
> It works around INT_MIN / -1 being C undefined behaviour: the
> need to special-case this is not x86-specific... The required
> answer for Arm is just as architectural as the required answer
> for division-by-zero (which is also C UB).

Suggested revised comment text:

/* We must avoid the C undefined behaviour cases: division by
 * zero and signed division of INT_MIN by -1. Both of these
 * have architecturally defined required results for Arm.
 * We special case all signed divisions by -1 to avoid having
 * to deduce the minimum integer for the type involved.
 */

?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/6] target/arm: Fix SVE system register access checks

[Laurent Desnogues]

Hello,

On Fri, Jun 29, 2018 at 2:15 AM, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Leave ARM_CP_SVE, removing ARM_CP_FPU; the sve_access_check
> produced by the flag already includes fp_access_check.  If
> we also check ARM_CP_FPU the double fp_access_check asserts.
>
> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Laurent Desnogues <laurent.desnogues@gmail.com>

Thanks for taking care of it,

Laurent

> ---
>  target/arm/helper.c        | 8 ++++----
>  target/arm/translate-a64.c | 5 ++---
>  2 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index b19c7ace78..a855da045b 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -4393,7 +4393,7 @@ static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>  static const ARMCPRegInfo zcr_el1_reginfo = {
>      .name = "ZCR_EL1", .state = ARM_CP_STATE_AA64,
>      .opc0 = 3, .opc1 = 0, .crn = 1, .crm = 2, .opc2 = 0,
> -    .access = PL1_RW, .type = ARM_CP_SVE | ARM_CP_FPU,
> +    .access = PL1_RW, .type = ARM_CP_SVE,
>      .fieldoffset = offsetof(CPUARMState, vfp.zcr_el[1]),
>      .writefn = zcr_write, .raw_writefn = raw_write
>  };
> @@ -4401,7 +4401,7 @@ static const ARMCPRegInfo zcr_el1_reginfo = {
>  static const ARMCPRegInfo zcr_el2_reginfo = {
>      .name = "ZCR_EL2", .state = ARM_CP_STATE_AA64,
>      .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 2, .opc2 = 0,
> -    .access = PL2_RW, .type = ARM_CP_SVE | ARM_CP_FPU,
> +    .access = PL2_RW, .type = ARM_CP_SVE,
>      .fieldoffset = offsetof(CPUARMState, vfp.zcr_el[2]),
>      .writefn = zcr_write, .raw_writefn = raw_write
>  };
> @@ -4409,14 +4409,14 @@ static const ARMCPRegInfo zcr_el2_reginfo = {
>  static const ARMCPRegInfo zcr_no_el2_reginfo = {
>      .name = "ZCR_EL2", .state = ARM_CP_STATE_AA64,
>      .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 2, .opc2 = 0,
> -    .access = PL2_RW, .type = ARM_CP_SVE | ARM_CP_FPU,
> +    .access = PL2_RW, .type = ARM_CP_SVE,
>      .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore
>  };
>
>  static const ARMCPRegInfo zcr_el3_reginfo = {
>      .name = "ZCR_EL3", .state = ARM_CP_STATE_AA64,
>      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 2, .opc2 = 0,
> -    .access = PL3_RW, .type = ARM_CP_SVE | ARM_CP_FPU,
> +    .access = PL3_RW, .type = ARM_CP_SVE,
>      .fieldoffset = offsetof(CPUARMState, vfp.zcr_el[3]),
>      .writefn = zcr_write, .raw_writefn = raw_write
>  };
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index f986340832..45a6c2a3aa 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -1633,11 +1633,10 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
>      default:
>          break;
>      }
> -    if ((ri->type & ARM_CP_SVE) && !sve_access_check(s)) {
> -        return;
> -    }
>      if ((ri->type & ARM_CP_FPU) && !fp_access_check(s)) {
>          return;
> +    } else if ((ri->type & ARM_CP_SVE) && !sve_access_check(s)) {
> +        return;
>      }
>
>      if ((tb_cflags(s->base.tb) & CF_USE_ICOUNT) && (ri->type & ARM_CP_IO)) {
> --
> 2.17.1
>
>

Re: [Qemu-devel] [PATCH 1/6] target/arm: Fix SVE signed division vs x86 overflow exception

[Richard Henderson]

On 06/29/2018 02:10 AM, Peter Maydell wrote:
> Suggested revised comment text:
> 
> /* We must avoid the C undefined behaviour cases: division by
>  * zero and signed division of INT_MIN by -1. Both of these
>  * have architecturally defined required results for Arm.
>  * We special case all signed divisions by -1 to avoid having
>  * to deduce the minimum integer for the type involved.
>  */
> 
> ?

I'm happy with that.


r~

Re: [Qemu-devel] [PATCH 5/6] target/arm: Add ID_ISAR6

[Richard Henderson]

On 06/29/2018 01:40 AM, Peter Maydell wrote:
>>      cpu->id_isar5 = 0x00000000;
>> +    cpu->id_isar6 = 0x00000000;
...
>>      cpu->id_isar5 = 0x00000000;
>> +    cpu->id_isar6 = 0x00000000;
...
>>      cpu->id_isar5 = 0x00000000;
>> +    cpu->id_isar6 = 0x00000000;
...
>>      cpu->id_isar5 = 0x0;
>> +    cpu->id_isar6 = 0x0;
...
> The ARMCPU struct fields should all be initially cleared to
> zero, so you don't really need to explicitly zero-initialize
> isar6 all over the place like this. (Compare isar5, which is
> I think only set in CPUs that are new enough that their TRMs
> mentioned it.)

Actually, the fact that isar5 is explicitly set to 0
in many places is the reason that I initialize isar6.


r~

Re: [Qemu-devel] [PATCH 6/6] target/arm: Set ISAR bits for -cpu max

[Richard Henderson]

On 06/29/2018 01:42 AM, Peter Maydell wrote:
> On 29 June 2018 at 01:15, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>> For the supported extensions, fill in the appropriate bits in
>> ID_ISAR5, ID_ISAR6, ID_AA64ISAR0, ID_AA64ISAR1.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
> 
> This makes sense, but I'd rather have a bit of time to think
> about how exactly we want to handle feature bits vs ID
> register values (the current codebase is not entirely
> coherent on the topic), so I'd rather not put this in
> for softfreeze unless there's a strong reason we should...

Fair.

I was wondering if we'd post-process the feature bits to initialize the id
registers, so that we don't have different places with the same knowledge.

The clearing of EL3 bits from id_pfr1 is an example of that already.


r~

Re: [Qemu-devel] [PATCH 6/6] target/arm: Set ISAR bits for -cpu max

[Peter Maydell]

On 29 June 2018 at 15:54, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 06/29/2018 01:42 AM, Peter Maydell wrote:
>> On 29 June 2018 at 01:15, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>> For the supported extensions, fill in the appropriate bits in
>>> ID_ISAR5, ID_ISAR6, ID_AA64ISAR0, ID_AA64ISAR1.

>> This makes sense, but I'd rather have a bit of time to think
>> about how exactly we want to handle feature bits vs ID
>> register values (the current codebase is not entirely
>> coherent on the topic), so I'd rather not put this in
>> for softfreeze unless there's a strong reason we should...
>
> Fair.
>
> I was wondering if we'd post-process the feature bits to initialize the id
> registers, so that we don't have different places with the same knowledge.
>
> The clearing of EL3 bits from id_pfr1 is an example of that already.

Yes, exactly. We have a few bits that are odd like that,
where we've kind of ad-hoc tweaked stuff to make things work.
I'd rather we decided on a coherent design and then moved
the code to do that. (Another example is Aaron's PMUv3
patchset, which wants to tweak the PMU related ID registers
to match what the emulation code implements.)

The original QEMU design I think was more or less "we report
fixed ID regs which don't necessarily match our actual
capabilities" (which is also how a KVM vCPU will behave).
"Feature bits are the primary source of truth" is also a
coherent design, just not the one we started with...

thanks
-- PMM