[PATCH 0/4] target/arm: Implement ARMv8.2-UAO

[PATCH 0/4] target/arm: Implement ARMv8.2-UAO

[Richard Henderson]

Based-on: <20191203225333.17055-1-richard.henderson@linaro.org>
("target/arm: Implement ARMv8.1-PAN + ARMv8.2-ATS1E1")

This was relatively easy to do, and related to the VHE
and PAN patch sets.  It's also already supported by the
Linux kernel and thus easily tested.


r~


Richard Henderson (4):
  target/arm: Add ID_AA64MMFR2_EL1
  target/arm: Update MSR access to UAO
  target/arm: Implement UAO semantics
  target/arm: Enable ARMv8.2-UAO in -cpu max

 target/arm/cpu.h           | 23 +++++++++++++
 target/arm/cpu64.c         |  4 +++
 target/arm/helper.c        | 66 +++++++++++++++++++++++++-------------
 target/arm/kvm64.c         |  2 ++
 target/arm/translate-a64.c | 14 ++++++++
 5 files changed, 87 insertions(+), 22 deletions(-)

-- 
2.17.1

[PATCH 1/4] target/arm: Add ID_AA64MMFR2_EL1

[Richard Henderson]

Add definitions for all of the fields, up to ARMv8.5.
Convert the existing RESERVED register to a full register.
Query KVM for the value of the register for the host.

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

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index d403dc5947..cdf6caf869 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -864,6 +864,7 @@ struct ARMCPU {
         uint64_t id_aa64pfr1;
         uint64_t id_aa64mmfr0;
         uint64_t id_aa64mmfr1;
+        uint64_t id_aa64mmfr2;
     } isar;
     uint32_t midr;
     uint32_t revidr;
@@ -1778,6 +1779,22 @@ FIELD(ID_AA64MMFR1, PAN, 20, 4)
 FIELD(ID_AA64MMFR1, SPECSEI, 24, 4)
 FIELD(ID_AA64MMFR1, XNX, 28, 4)
 
+FIELD(ID_AA64MMFR2, CNP, 0, 4)
+FIELD(ID_AA64MMFR2, UAO, 4, 4)
+FIELD(ID_AA64MMFR2, LSM, 8, 4)
+FIELD(ID_AA64MMFR2, IESB, 12, 4)
+FIELD(ID_AA64MMFR2, VARANGE, 16, 4)
+FIELD(ID_AA64MMFR2, CCIDX, 20, 4)
+FIELD(ID_AA64MMFR2, NV, 24, 4)
+FIELD(ID_AA64MMFR2, ST, 28, 4)
+FIELD(ID_AA64MMFR2, AT, 32, 4)
+FIELD(ID_AA64MMFR2, IDS, 36, 4)
+FIELD(ID_AA64MMFR2, FWB, 40, 4)
+FIELD(ID_AA64MMFR2, TTL, 48, 4)
+FIELD(ID_AA64MMFR2, BBM, 52, 4)
+FIELD(ID_AA64MMFR2, EVT, 56, 4)
+FIELD(ID_AA64MMFR2, E0PD, 60, 4)
+
 FIELD(ID_DFR0, COPDBG, 0, 4)
 FIELD(ID_DFR0, COPSDBG, 4, 4)
 FIELD(ID_DFR0, MMAPDBG, 8, 4)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index f1eab4fb28..70f2db5447 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6825,11 +6825,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .access = PL1_R, .type = ARM_CP_CONST,
               .accessfn = access_aa64_tid3,
               .resetvalue = cpu->isar.id_aa64mmfr1 },
-            { .name = "ID_AA64MMFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
+            { .name = "ID_AA64MMFR2_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 2,
               .access = PL1_R, .type = ARM_CP_CONST,
               .accessfn = access_aa64_tid3,
-              .resetvalue = 0 },
+              .resetvalue = cpu->isar.id_aa64mmfr2 },
             { .name = "ID_AA64MMFR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 3,
               .access = PL1_R, .type = ARM_CP_CONST,
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 876184b8fe..482e7fdfbb 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -549,6 +549,8 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
                               ARM64_SYS_REG(3, 0, 0, 7, 0));
         err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64mmfr1,
                               ARM64_SYS_REG(3, 0, 0, 7, 1));
+        err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64mmfr2,
+                              ARM64_SYS_REG(3, 0, 0, 7, 2));
 
         /*
          * Note that if AArch32 support is not present in the host,
-- 
2.17.1

[PATCH 2/4] target/arm: Update MSR access to UAO

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h           |  6 ++++++
 target/arm/helper.c        | 21 +++++++++++++++++++++
 target/arm/translate-a64.c | 14 ++++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index cdf6caf869..dd284ba5c7 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1228,6 +1228,7 @@ void pmu_init(ARMCPU *cpu);
 #define PSTATE_IL (1U << 20)
 #define PSTATE_SS (1U << 21)
 #define PSTATE_PAN (1U << 22)
+#define PSTATE_UAO (1U << 23)
 #define PSTATE_V (1U << 28)
 #define PSTATE_C (1U << 29)
 #define PSTATE_Z (1U << 30)
@@ -3598,6 +3599,11 @@ static inline bool isar_feature_aa64_ats1e1(const ARMISARegisters *id)
     return FIELD_EX64(id->id_aa64mmfr1, ID_AA64MMFR1, PAN) >= 2;
 }
 
+static inline bool isar_feature_aa64_uao(const ARMISARegisters *id)
+{
+    return FIELD_EX64(id->id_aa64mmfr2, ID_AA64MMFR2, UAO) != 0;
+}
+
 static inline bool isar_feature_aa64_bti(const ARMISARegisters *id)
 {
     return FIELD_EX64(id->id_aa64pfr1, ID_AA64PFR1, BT) != 0;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 70f2db5447..8941a6c10f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -4131,6 +4131,17 @@ static void aa64_pan_write(CPUARMState *env, const ARMCPRegInfo *ri,
     env->pstate = (env->pstate & ~PSTATE_PAN) | (value & PSTATE_PAN);
 }
 
+static uint64_t aa64_uao_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    return env->pstate & PSTATE_UAO;
+}
+
+static void aa64_uao_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                           uint64_t value)
+{
+    env->pstate = (env->pstate & ~PSTATE_UAO) | (value & PSTATE_UAO);
+}
+
 static CPAccessResult aa64_cacheop_access(CPUARMState *env,
                                           const ARMCPRegInfo *ri,
                                           bool isread)
@@ -7464,6 +7475,16 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         define_arm_cp_regs(cpu, ats1cp_reginfo);
     }
 #endif
+    if (cpu_isar_feature(aa64_uao, cpu)) {
+        static const ARMCPRegInfo uao_reginfo[] = {
+            { .name = "UAO", .state = ARM_CP_STATE_AA64,
+              .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 2, .opc2 = 4,
+              .type = ARM_CP_NO_RAW, .access = PL1_RW,
+              .readfn = aa64_uao_read, .writefn = aa64_uao_write, },
+            REGINFO_SENTINEL
+        };
+        define_arm_cp_regs(cpu, uao_reginfo);
+    }
 
     if (arm_feature(env, ARM_FEATURE_EL2) && cpu_isar_feature(aa64_vh, cpu)) {
         static const ARMCPRegInfo vhe_reginfo[] = {
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 7f5a68106b..2b6846ef01 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1601,6 +1601,20 @@ static void handle_msr_i(DisasContext *s, uint32_t insn,
         s->base.is_jmp = DISAS_NEXT;
         break;
 
+    case 0x03: /* UAO */
+        if (!dc_isar_feature(aa64_uao, s) || s->current_el == 0) {
+            goto do_unallocated;
+        }
+        if (crm & 1) {
+            set_pstate_bits(PSTATE_UAO);
+        } else {
+            clear_pstate_bits(PSTATE_UAO);
+        }
+        t1 = tcg_const_i32(s->current_el);
+        gen_helper_rebuild_hflags_a64(cpu_env, t1);
+        tcg_temp_free_i32(t1);
+        break;
+
     case 0x04: /* PAN */
         if (!dc_isar_feature(aa64_pan, s) || s->current_el == 0) {
             goto do_unallocated;
-- 
2.17.1

[PATCH 3/4] target/arm: Implement UAO semantics

[Richard Henderson]

We need only override the current condition under which
TBFLAG_A64.UNPRIV is set.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 8941a6c10f..6d7a8349b5 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -12050,28 +12050,29 @@ static uint32_t rebuild_hflags_a64(CPUARMState *env, int el, int fp_el,
     }
 
     /* Compute the condition for using AccType_UNPRIV for LDTR et al. */
-    /* TODO: ARMv8.2-UAO */
-    switch (mmu_idx) {
-    case ARMMMUIdx_EL10_1:
-    case ARMMMUIdx_EL10_1_PAN:
-    case ARMMMUIdx_SE1:
-    case ARMMMUIdx_SE1_PAN:
-        /* TODO: ARMv8.3-NV */
-        flags = FIELD_DP32(flags, TBFLAG_A64, UNPRIV, 1);
-        break;
-    case ARMMMUIdx_EL20_2:
-    case ARMMMUIdx_EL20_2_PAN:
-        /* TODO: ARMv8.4-SecEL2 */
-        /*
-         * Note that EL20_2 is gated by HCR_EL2.E2H == 1, but EL20_0 is
-         * gated by HCR_EL2.<E2H,TGE> == '11', and so is LDTR.
-         */
-        if (env->cp15.hcr_el2 & HCR_TGE) {
+    if (!(env->pstate & PSTATE_UAO)) {
+        switch (mmu_idx) {
+        case ARMMMUIdx_EL10_1:
+        case ARMMMUIdx_EL10_1_PAN:
+        case ARMMMUIdx_SE1:
+        case ARMMMUIdx_SE1_PAN:
+            /* TODO: ARMv8.3-NV */
             flags = FIELD_DP32(flags, TBFLAG_A64, UNPRIV, 1);
+            break;
+        case ARMMMUIdx_EL20_2:
+        case ARMMMUIdx_EL20_2_PAN:
+            /* TODO: ARMv8.4-SecEL2 */
+            /*
+             * Note that EL20_2 is gated by HCR_EL2.E2H == 1, but EL20_0 is
+             * gated by HCR_EL2.<E2H,TGE> == '11', and so is LDTR.
+             */
+            if (env->cp15.hcr_el2 & HCR_TGE) {
+                flags = FIELD_DP32(flags, TBFLAG_A64, UNPRIV, 1);
+            }
+            break;
+        default:
+            break;
         }
-        break;
-    default:
-        break;
     }
 
     return rebuild_hflags_common(env, fp_el, mmu_idx, flags);
-- 
2.17.1

[PATCH 4/4] target/arm: Enable ARMv8.2-UAO in -cpu max

[Richard Henderson]

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

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 9399253b4c..03377084e3 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -674,6 +674,10 @@ static void aarch64_max_initfn(Object *obj)
         t = FIELD_DP64(t, ID_AA64MMFR1, PAN, 2); /* ATS1E1 */
         cpu->isar.id_aa64mmfr1 = t;
 
+        t = cpu->isar.id_aa64mmfr2;
+        t = FIELD_DP64(t, ID_AA64MMFR2, UAO, 1);
+        cpu->isar.id_aa64mmfr2 = t;
+
         /* Replicate the same data to the 32-bit id registers.  */
         u = cpu->isar.id_isar5;
         u = FIELD_DP32(u, ID_ISAR5, AES, 2); /* AES + PMULL */
-- 
2.17.1

Re: [PATCH 1/4] target/arm: Add ID_AA64MMFR2_EL1

[Peter Maydell]

On Tue, 3 Dec 2019 at 23:42, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Add definitions for all of the fields, up to ARMv8.5.
> Convert the existing RESERVED register to a full register.
> Query KVM for the value of the register for the host.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h    | 17 +++++++++++++++++
>  target/arm/helper.c |  4 ++--
>  target/arm/kvm64.c  |  2 ++
>  3 files changed, 21 insertions(+), 2 deletions(-)

> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 876184b8fe..482e7fdfbb 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -549,6 +549,8 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>                                ARM64_SYS_REG(3, 0, 0, 7, 0));
>          err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64mmfr1,
>                                ARM64_SYS_REG(3, 0, 0, 7, 1));
> +        err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64mmfr2,
> +                              ARM64_SYS_REG(3, 0, 0, 7, 2));

Do current KVM kernels definitely handle the request for this
new register (ie don't return an error)?

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

thanks
-- PMM

Re: [PATCH 2/4] target/arm: Update MSR access to UAO

[Peter Maydell]

On Tue, 3 Dec 2019 at 23:42, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h           |  6 ++++++
>  target/arm/helper.c        | 21 +++++++++++++++++++++
>  target/arm/translate-a64.c | 14 ++++++++++++++
>  3 files changed, 41 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index cdf6caf869..dd284ba5c7 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1228,6 +1228,7 @@ void pmu_init(ARMCPU *cpu);
>  #define PSTATE_IL (1U << 20)
>  #define PSTATE_SS (1U << 21)
>  #define PSTATE_PAN (1U << 22)
> +#define PSTATE_UAO (1U << 23)
>  #define PSTATE_V (1U << 28)
>  #define PSTATE_C (1U << 29)
>  #define PSTATE_Z (1U << 30)
> @@ -3598,6 +3599,11 @@ static inline bool isar_feature_aa64_ats1e1(const ARMISARegisters *id)
>      return FIELD_EX64(id->id_aa64mmfr1, ID_AA64MMFR1, PAN) >= 2;
>  }
>
> +static inline bool isar_feature_aa64_uao(const ARMISARegisters *id)
> +{
> +    return FIELD_EX64(id->id_aa64mmfr2, ID_AA64MMFR2, UAO) != 0;
> +}
> +
>  static inline bool isar_feature_aa64_bti(const ARMISARegisters *id)
>  {
>      return FIELD_EX64(id->id_aa64pfr1, ID_AA64PFR1, BT) != 0;
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 70f2db5447..8941a6c10f 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -4131,6 +4131,17 @@ static void aa64_pan_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      env->pstate = (env->pstate & ~PSTATE_PAN) | (value & PSTATE_PAN);
>  }
>
> +static uint64_t aa64_uao_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    return env->pstate & PSTATE_UAO;
> +}
> +
> +static void aa64_uao_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                           uint64_t value)
> +{
> +    env->pstate = (env->pstate & ~PSTATE_UAO) | (value & PSTATE_UAO);
> +}
> +
>  static CPAccessResult aa64_cacheop_access(CPUARMState *env,
>                                            const ARMCPRegInfo *ri,
>                                            bool isread)
> @@ -7464,6 +7475,16 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>          define_arm_cp_regs(cpu, ats1cp_reginfo);
>      }
>  #endif
> +    if (cpu_isar_feature(aa64_uao, cpu)) {
> +        static const ARMCPRegInfo uao_reginfo[] = {
> +            { .name = "UAO", .state = ARM_CP_STATE_AA64,
> +              .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 2, .opc2 = 4,
> +              .type = ARM_CP_NO_RAW, .access = PL1_RW,
> +              .readfn = aa64_uao_read, .writefn = aa64_uao_write, },
> +            REGINFO_SENTINEL
> +        };

This could just be a file-scope global, right?
Also, you can just use define_one_arm_cp_reg() rather
than having a list with one entry. (cf zcr_el1_reginfo).

> +        define_arm_cp_regs(cpu, uao_reginfo);
> +    }
>
>      if (arm_feature(env, ARM_FEATURE_EL2) && cpu_isar_feature(aa64_vh, cpu)) {
>          static const ARMCPRegInfo vhe_reginfo[] = {
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 7f5a68106b..2b6846ef01 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -1601,6 +1601,20 @@ static void handle_msr_i(DisasContext *s, uint32_t insn,
>          s->base.is_jmp = DISAS_NEXT;
>          break;
>
> +    case 0x03: /* UAO */
> +        if (!dc_isar_feature(aa64_uao, s) || s->current_el == 0) {
> +            goto do_unallocated;
> +        }
> +        if (crm & 1) {
> +            set_pstate_bits(PSTATE_UAO);
> +        } else {
> +            clear_pstate_bits(PSTATE_UAO);
> +        }
> +        t1 = tcg_const_i32(s->current_el);
> +        gen_helper_rebuild_hflags_a64(cpu_env, t1);
> +        tcg_temp_free_i32(t1);
> +        break;

Do we also need to end the TB since we've messed with
the hflags, or is some bit of code not in the patch
context handling that?

> +
>      case 0x04: /* PAN */
>          if (!dc_isar_feature(aa64_pan, s) || s->current_el == 0) {
>              goto do_unallocated;
> --
> 2.17.1

Does the "on exception entry PSTATE.UAO is zeroed" behaviour
fall out automatically for us? How about "on exception entry
from aarch32 to aarch64 SPSR_ELx.UAO is set to zero" ?

I think we may also want a minor code change so that an exception
return from aarch64 to aarch32 doesn't copy a bogus SPSR UAO==1
into the pstate/cpsr.

thanks
-- PMM

Re: [PATCH 3/4] target/arm: Implement UAO semantics

[Peter Maydell]

On Tue, 3 Dec 2019 at 23:42, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> We need only override the current condition under which
> TBFLAG_A64.UNPRIV is set.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/helper.c | 41 +++++++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 20 deletions(-)

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

thanks
-- PMM

Re: [PATCH 4/4] target/arm: Enable ARMv8.2-UAO in -cpu max

[Peter Maydell]

On Tue, 3 Dec 2019 at 23:42, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu64.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 9399253b4c..03377084e3 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -674,6 +674,10 @@ static void aarch64_max_initfn(Object *obj)
>          t = FIELD_DP64(t, ID_AA64MMFR1, PAN, 2); /* ATS1E1 */
>          cpu->isar.id_aa64mmfr1 = t;
>
> +        t = cpu->isar.id_aa64mmfr2;
> +        t = FIELD_DP64(t, ID_AA64MMFR2, UAO, 1);
> +        cpu->isar.id_aa64mmfr2 = t;
> +
>          /* Replicate the same data to the 32-bit id registers.  */
>          u = cpu->isar.id_isar5;
>          u = FIELD_DP32(u, ID_ISAR5, AES, 2); /* AES + PMULL */
> --
> 2.17.1

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

thanks
-- PMM

Re: [PATCH 2/4] target/arm: Update MSR access to UAO

[Richard Henderson]

On 12/6/19 10:30 AM, Peter Maydell wrote:
>> +    if (cpu_isar_feature(aa64_uao, cpu)) {
>> +        static const ARMCPRegInfo uao_reginfo[] = {
>> +            { .name = "UAO", .state = ARM_CP_STATE_AA64,
>> +              .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 2, .opc2 = 4,
>> +              .type = ARM_CP_NO_RAW, .access = PL1_RW,
>> +              .readfn = aa64_uao_read, .writefn = aa64_uao_write, },
>> +            REGINFO_SENTINEL
>> +        };
> 
> This could just be a file-scope global, right?
> Also, you can just use define_one_arm_cp_reg() rather
> than having a list with one entry. (cf zcr_el1_reginfo).

Can do.

>> +    case 0x03: /* UAO */
>> +        if (!dc_isar_feature(aa64_uao, s) || s->current_el == 0) {
>> +            goto do_unallocated;
>> +        }
>> +        if (crm & 1) {
>> +            set_pstate_bits(PSTATE_UAO);
>> +        } else {
>> +            clear_pstate_bits(PSTATE_UAO);
>> +        }
>> +        t1 = tcg_const_i32(s->current_el);
>> +        gen_helper_rebuild_hflags_a64(cpu_env, t1);
>> +        tcg_temp_free_i32(t1);
>> +        break;
> 
> Do we also need to end the TB since we've messed with
> the hflags, or is some bit of code not in the patch
> context handling that?

Outside the patch context, at the start of the function, we default to ending
the TB.

> Does the "on exception entry PSTATE.UAO is zeroed" behaviour
> fall out automatically for us? How about "on exception entry
> from aarch32 to aarch64 SPSR_ELx.UAO is set to zero" ?

Yes to both -- new_mode (perhaps better renamed as new_pstate) is initialized
as 0 + stuff required to be non-zero.  Thus PAN required special handling but
UAO does not.

> I think we may also want a minor code change so that an exception
> return from aarch64 to aarch32 doesn't copy a bogus SPSR UAO==1
> into the pstate/cpsr.

Ah, good catch.


r~

Re: [PATCH 1/4] target/arm: Add ID_AA64MMFR2_EL1

[Richard Henderson]

On 12/6/19 10:19 AM, Peter Maydell wrote:
>> @@ -549,6 +549,8 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>>                                ARM64_SYS_REG(3, 0, 0, 7, 0));
>>          err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64mmfr1,
>>                                ARM64_SYS_REG(3, 0, 0, 7, 1));
>> +        err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64mmfr2,
>> +                              ARM64_SYS_REG(3, 0, 0, 7, 2));
> 
> Do current KVM kernels definitely handle the request for this
> new register (ie don't return an error)?

Yes, ID_AA64MMFR2 was added to the sys_regs table in the same commit
(93390c0a1b20b) as all of the others here.


r~

Re: [PATCH 2/4] target/arm: Update MSR access to UAO

[Richard Henderson]

On 12/6/19 10:30 AM, Peter Maydell wrote:
>> +    if (cpu_isar_feature(aa64_uao, cpu)) {
>> +        static const ARMCPRegInfo uao_reginfo[] = {
>> +            { .name = "UAO", .state = ARM_CP_STATE_AA64,
>> +              .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 2, .opc2 = 4,
>> +              .type = ARM_CP_NO_RAW, .access = PL1_RW,
>> +              .readfn = aa64_uao_read, .writefn = aa64_uao_write, },
>> +            REGINFO_SENTINEL
>> +        };
> 
> This could just be a file-scope global, right?
> Also, you can just use define_one_arm_cp_reg() rather
> than having a list with one entry. (cf zcr_el1_reginfo).

Done.

>> +    case 0x03: /* UAO */
>> +        if (!dc_isar_feature(aa64_uao, s) || s->current_el == 0) {
>> +            goto do_unallocated;
>> +        }
>> +        if (crm & 1) {
>> +            set_pstate_bits(PSTATE_UAO);
>> +        } else {
>> +            clear_pstate_bits(PSTATE_UAO);
>> +        }
>> +        t1 = tcg_const_i32(s->current_el);
>> +        gen_helper_rebuild_hflags_a64(cpu_env, t1);
>> +        tcg_temp_free_i32(t1);
>> +        break;
> 
> Do we also need to end the TB since we've messed with
> the hflags, or is some bit of code not in the patch
> context handling that?

This is done by default.  We would have to do something special to avoid ending
the TB here.

> Does the "on exception entry PSTATE.UAO is zeroed" behaviour
> fall out automatically for us?

Yes, aarch64_pstate_mode() returns a clean PSTATE.

> How about "on exception entry
> from aarch32 to aarch64 SPSR_ELx.UAO is set to zero" ?

This follows the same path as above, as far as I can see.

> I think we may also want a minor code change so that an exception
> return from aarch64 to aarch32 doesn't copy a bogus SPSR UAO==1
> into the pstate/cpsr.

Well, there is no CPSR UAO bit, so there's no aarch32 bit to clear.  But I did
add a clearing of PSTATE UAO on the exception return to aarch64 path, to
prevent the guest from playing games with SPSR.


r~

Re: [PATCH 2/4] target/arm: Update MSR access to UAO

[Peter Maydell]

On Sun, 2 Feb 2020 at 01:00, Richard Henderson
<richard.henderson@linaro.org> wrote:
> > Does the "on exception entry PSTATE.UAO is zeroed" behaviour
> > fall out automatically for us?
>
> Yes, aarch64_pstate_mode() returns a clean PSTATE.
>
> > How about "on exception entry
> > from aarch32 to aarch64 SPSR_ELx.UAO is set to zero" ?
>
> This follows the same path as above, as far as I can see.

Yes, but SPSR_ELx isn't started with a clean zero and built up
the way the new PSTATE is, it gets copied from the AArch32 CPSR
via cpsr_read(). I forget how carefully we keep the guest from setting
CPSR bits that aren't really valid for the CPU...

> > I think we may also want a minor code change so that an exception
> > return from aarch64 to aarch32 doesn't copy a bogus SPSR UAO==1
> > into the pstate/cpsr.
>
> Well, there is no CPSR UAO bit, so there's no aarch32 bit to clear.  But I did
> add a clearing of PSTATE UAO on the exception return to aarch64 path, to
> prevent the guest from playing games with SPSR.

...for instance on the aarch64->aarch32 exception return path,
I don't think we sanitize the SPSR bits, so the guest could use
a 64->32 exception return to set a bogus CPSR.UAO bit and
then enter from 32 to 64 and see SPSR_ELx.UAO set when
it should not be, unless we sanitize either in all places where
we let the guest set CPSR bits (including 64->32 return), or
we sanitize on 32->64 entry.

thanks
-- PMM

Re: [PATCH 2/4] target/arm: Update MSR access to UAO

[Richard Henderson]

On 2/2/20 1:29 PM, Peter Maydell wrote:
> Yes, but SPSR_ELx isn't started with a clean zero and built up
> the way the new PSTATE is, it gets copied from the AArch32 CPSR
> via cpsr_read(). I forget how carefully we keep the guest from setting
> CPSR bits that aren't really valid for the CPU...

We do an ok job, except...

>> Well, there is no CPSR UAO bit, so there's no aarch32 bit to clear.  But I did
>> add a clearing of PSTATE UAO on the exception return to aarch64 path, to
>> prevent the guest from playing games with SPSR.
> 
> ...for instance on the aarch64->aarch32 exception return path,

... here.

> I don't think we sanitize the SPSR bits, so the guest could use
> a 64->32 exception return to set a bogus CPSR.UAO bit and
> then enter from 32 to 64 and see SPSR_ELx.UAO set when
> it should not be, unless we sanitize either in all places where
> we let the guest set CPSR bits (including 64->32 return), or
> we sanitize on 32->64 entry.

There is no CPSR.UAO bit, so this chain of logic doesn't hold for that specific
instance.  But plausibly so for CPSR.PAN.

We do sanitize all of the places where CPSR/PSTATE is explicitly set.  I think
we've covered all but one of the exception return paths, sanitizing the
SPSR_ELx values.

We could move some of this logic to internals.h so that it could be shared
between CPSR and exception return.  I'll think about that for v3.


r~