[Qemu-devel] [PATCH v2 00/10] target/arm: LOR, HPD, AA32HPD

[Qemu-devel] [PATCH v2 00/10] target/arm: LOR, HPD, AA32HPD

[Richard Henderson]

Three relatively simple post-8.0 extensions.

Changes since v1:
  * Add TLOR access checks for LOR registers.
  * Clean up access to HCR_EL2.
  * Clean up setting of SCR_EL3.
  * Other changes as noted within each patch.


r~


Richard Henderson (10):
  target/arm: Move id_aa64mmfr* to ARMISARegisters
  target/arm: Add HCR_EL2 bits up to ARMv8.5
  target/arm: Add SCR_EL3 bits up to ARMv8.5
  target/arm: Fix HCR_EL2.TGE check in arm_phys_excp_target_el
  target/arm: Introduce arm_hcr_el2_eff
  target/arm: Use arm_hcr_el2_eff more places
  target/arm: Tidy scr_write
  target/arm: Implement the ARMv8.1-LOR extension
  target/arm: Implement the ARMv8.1-HPD extension
  target/arm: Implement the ARMv8.2-AA32HPD extension

 target/arm/cpu.h           | 141 ++++++++++++---------
 target/arm/internals.h     |   3 +-
 hw/intc/arm_gicv3_cpuif.c  |  21 ++--
 target/arm/cpu.c           |   4 +
 target/arm/cpu64.c         |  11 +-
 target/arm/helper.c        | 247 +++++++++++++++++++++++++++++++------
 target/arm/kvm64.c         |   4 +
 target/arm/op_helper.c     |  14 +--
 target/arm/translate-a64.c |  12 ++
 9 files changed, 342 insertions(+), 115 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH v2 01/10] target/arm: Move id_aa64mmfr* to ARMISARegisters

[Richard Henderson]

At the same time, define the fields for these registers,
and use those defines in arm_pamax().

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

----
v2: Include the v8.5 fields; init the registers for kvm.
    Upcase all of the field names.
---
 target/arm/cpu.h       | 26 ++++++++++++++++++++++++--
 target/arm/internals.h |  3 ++-
 target/arm/cpu64.c     |  6 +++---
 target/arm/helper.c    |  4 ++--
 target/arm/kvm64.c     |  4 ++++
 5 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 2a73fed9a0..656a96a8f8 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -818,6 +818,8 @@ struct ARMCPU {
         uint64_t id_aa64isar1;
         uint64_t id_aa64pfr0;
         uint64_t id_aa64pfr1;
+        uint64_t id_aa64mmfr0;
+        uint64_t id_aa64mmfr1;
     } isar;
     uint32_t midr;
     uint32_t revidr;
@@ -839,8 +841,6 @@ struct ARMCPU {
     uint64_t id_aa64dfr1;
     uint64_t id_aa64afr0;
     uint64_t id_aa64afr1;
-    uint64_t id_aa64mmfr0;
-    uint64_t id_aa64mmfr1;
     uint32_t dbgdidr;
     uint32_t clidr;
     uint64_t mp_affinity; /* MP ID without feature bits */
@@ -1557,6 +1557,28 @@ FIELD(ID_AA64PFR0, GIC, 24, 4)
 FIELD(ID_AA64PFR0, RAS, 28, 4)
 FIELD(ID_AA64PFR0, SVE, 32, 4)
 
+FIELD(ID_AA64MMFR0, PARANGE, 0, 4)
+FIELD(ID_AA64MMFR0, ASIDBITS, 4, 4)
+FIELD(ID_AA64MMFR0, BIGEND, 8, 4)
+FIELD(ID_AA64MMFR0, SNSMEM, 12, 4)
+FIELD(ID_AA64MMFR0, BIGENDEL0, 16, 4)
+FIELD(ID_AA64MMFR0, TGRAN16, 20, 4)
+FIELD(ID_AA64MMFR0, TGRAN64, 24, 4)
+FIELD(ID_AA64MMFR0, TGRAN4, 28, 4)
+FIELD(ID_AA64MMFR0, TGRAN16_2, 32, 4)
+FIELD(ID_AA64MMFR0, TGRAN64_2, 36, 4)
+FIELD(ID_AA64MMFR0, TGRAN4_2, 40, 4)
+FIELD(ID_AA64MMFR0, EXS, 44, 4)
+
+FIELD(ID_AA64MMFR1, HAFDBS, 0, 4)
+FIELD(ID_AA64MMFR1, VMIDBITS, 4, 4)
+FIELD(ID_AA64MMFR1, VH, 8, 4)
+FIELD(ID_AA64MMFR1, HPDS, 12, 4)
+FIELD(ID_AA64MMFR1, LO, 16, 4)
+FIELD(ID_AA64MMFR1, PAN, 20, 4)
+FIELD(ID_AA64MMFR1, SPECSEI, 24, 4)
+FIELD(ID_AA64MMFR1, XNX, 28, 4)
+
 QEMU_BUILD_BUG_ON(ARRAY_SIZE(((ARMCPU *)0)->ccsidr) <= R_V7M_CSSELR_INDEX_MASK);
 
 /* If adding a feature bit which corresponds to a Linux ELF
diff --git a/target/arm/internals.h b/target/arm/internals.h
index d208b70a64..78e026d6e9 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -229,7 +229,8 @@ static inline unsigned int arm_pamax(ARMCPU *cpu)
         [4] = 44,
         [5] = 48,
     };
-    unsigned int parange = extract32(cpu->id_aa64mmfr0, 0, 4);
+    unsigned int parange =
+        FIELD_EX64(cpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
 
     /* id_aa64mmfr0 is a read-only register so values outside of the
      * supported mappings can be considered an implementation error.  */
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 873f059bf2..0babe483ac 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -141,7 +141,7 @@ static void aarch64_a57_initfn(Object *obj)
     cpu->pmceid0 = 0x00000000;
     cpu->pmceid1 = 0x00000000;
     cpu->isar.id_aa64isar0 = 0x00011120;
-    cpu->id_aa64mmfr0 = 0x00001124;
+    cpu->isar.id_aa64mmfr0 = 0x00001124;
     cpu->dbgdidr = 0x3516d000;
     cpu->clidr = 0x0a200023;
     cpu->ccsidr[0] = 0x701fe00a; /* 32KB L1 dcache */
@@ -195,7 +195,7 @@ static void aarch64_a53_initfn(Object *obj)
     cpu->isar.id_aa64pfr0 = 0x00002222;
     cpu->id_aa64dfr0 = 0x10305106;
     cpu->isar.id_aa64isar0 = 0x00011120;
-    cpu->id_aa64mmfr0 = 0x00001122; /* 40 bit physical addr */
+    cpu->isar.id_aa64mmfr0 = 0x00001122; /* 40 bit physical addr */
     cpu->dbgdidr = 0x3516d000;
     cpu->clidr = 0x0a200023;
     cpu->ccsidr[0] = 0x700fe01a; /* 32KB L1 dcache */
@@ -249,7 +249,7 @@ static void aarch64_a72_initfn(Object *obj)
     cpu->pmceid0 = 0x00000000;
     cpu->pmceid1 = 0x00000000;
     cpu->isar.id_aa64isar0 = 0x00011120;
-    cpu->id_aa64mmfr0 = 0x00001124;
+    cpu->isar.id_aa64mmfr0 = 0x00001124;
     cpu->dbgdidr = 0x3516d000;
     cpu->clidr = 0x0a200023;
     cpu->ccsidr[0] = 0x701fe00a; /* 32KB L1 dcache */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 0da1424f72..04c4a91b04 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5207,11 +5207,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             { .name = "ID_AA64MMFR0_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 0,
               .access = PL1_R, .type = ARM_CP_CONST,
-              .resetvalue = cpu->id_aa64mmfr0 },
+              .resetvalue = cpu->isar.id_aa64mmfr0 },
             { .name = "ID_AA64MMFR1_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 1,
               .access = PL1_R, .type = ARM_CP_CONST,
-              .resetvalue = cpu->id_aa64mmfr1 },
+              .resetvalue = cpu->isar.id_aa64mmfr1 },
             { .name = "ID_AA64MMFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 2,
               .access = PL1_R, .type = ARM_CP_CONST,
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 0a502091e7..ad83e1479c 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -538,6 +538,10 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
                               ARM64_SYS_REG(3, 0, 0, 6, 0));
         err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64isar1,
                               ARM64_SYS_REG(3, 0, 0, 6, 1));
+        err |= read_sys_reg64(fdarray[2], &achf->isar.id_aa64mmfr0,
+                              ARM64_SYS_REG(3, 0, 0, 7, 0));
+        err |= read_sys_reg64(fdarray[2], &achf->isar.id_aa64mmfr1,
+                              ARM64_SYS_REG(3, 0, 0, 7, 1));
 
         /*
          * Note that if AArch32 support is not present in the host,
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 01/10] target/arm: Move id_aa64mmfr* to ARMISARegisters

[Peter Maydell]

On Mon, 3 Dec 2018 at 20:38, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> At the same time, define the fields for these registers,
> and use those defines in arm_pamax().
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>
> ----
> v2: Include the v8.5 fields; init the registers for kvm.
>     Upcase all of the field names.

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

thanks
-- PMM

[Qemu-devel] [PATCH v2 02/10] target/arm: Add HCR_EL2 bits up to ARMv8.5

[Richard Henderson]

Post v8.3 bits taken from SysReg_v85_xml-00bet8.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 656a96a8f8..79d58978f7 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1249,7 +1249,7 @@ static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
 #define HCR_TIDCP     (1ULL << 20)
 #define HCR_TACR      (1ULL << 21)
 #define HCR_TSW       (1ULL << 22)
-#define HCR_TPC       (1ULL << 23)
+#define HCR_TPCP      (1ULL << 23)
 #define HCR_TPU       (1ULL << 24)
 #define HCR_TTLB      (1ULL << 25)
 #define HCR_TVM       (1ULL << 26)
@@ -1261,6 +1261,26 @@ static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
 #define HCR_CD        (1ULL << 32)
 #define HCR_ID        (1ULL << 33)
 #define HCR_E2H       (1ULL << 34)
+#define HCR_TLOR      (1ULL << 35)
+#define HCR_TERR      (1ULL << 36)
+#define HCR_TEA       (1ULL << 37)
+#define HCR_MIOCNCE   (1ULL << 38)
+#define HCR_APK       (1ULL << 40)
+#define HCR_API       (1ULL << 41)
+#define HCR_NV        (1ULL << 42)
+#define HCR_NV1       (1ULL << 43)
+#define HCR_AT        (1ULL << 44)
+#define HCR_NV2       (1ULL << 45)
+#define HCR_FWB       (1ULL << 46)
+#define HCR_FIEN      (1ULL << 47)
+#define HCR_TID4      (1ULL << 49)
+#define HCR_TICAB     (1ULL << 50)
+#define HCR_TOCU      (1ULL << 52)
+#define HCR_TTLBIS    (1ULL << 54)
+#define HCR_TTLBOS    (1ULL << 55)
+#define HCR_ATA       (1ULL << 56)
+#define HCR_DCT       (1ULL << 57)
+
 /*
  * When we actually implement ARMv8.1-VHE we should add HCR_E2H to
  * HCR_MASK and then clear it again if the feature bit is not set in
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 02/10] target/arm: Add HCR_EL2 bits up to ARMv8.5

[Peter Maydell]

On Mon, 3 Dec 2018 at 20:38, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Post v8.3 bits taken from SysReg_v85_xml-00bet8.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 656a96a8f8..79d58978f7 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1249,7 +1249,7 @@ static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
>  #define HCR_TIDCP     (1ULL << 20)
>  #define HCR_TACR      (1ULL << 21)
>  #define HCR_TSW       (1ULL << 22)
> -#define HCR_TPC       (1ULL << 23)
> +#define HCR_TPCP      (1ULL << 23)

We were using "TPC" here because that's what the 32-bit HCR
register names the bit; but standardizing on the 64-bit HCR_EL2
names makes sense.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 02/10] target/arm: Add HCR_EL2 bits up to ARMv8.5

[Peter Maydell]

On Thu, 6 Dec 2018 at 11:15, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 3 Dec 2018 at 20:38, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > Post v8.3 bits taken from SysReg_v85_xml-00bet8.
> >
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >  target/arm/cpu.h | 22 +++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 656a96a8f8..79d58978f7 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -1249,7 +1249,7 @@ static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
> >  #define HCR_TIDCP     (1ULL << 20)
> >  #define HCR_TACR      (1ULL << 21)
> >  #define HCR_TSW       (1ULL << 22)
> > -#define HCR_TPC       (1ULL << 23)
> > +#define HCR_TPCP      (1ULL << 23)
>
> We were using "TPC" here because that's what the 32-bit HCR
> register names the bit; but standardizing on the 64-bit HCR_EL2
> names makes sense.

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

thanks
-- PMM

[Qemu-devel] [PATCH v2 03/10] target/arm: Add SCR_EL3 bits up to ARMv8.5

[Richard Henderson]

Post v8.4 bits taken from SysReg_v85_xml-00bet8.

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

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 79d58978f7..20d97b66de 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1302,6 +1302,16 @@ static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
 #define SCR_ST                (1U << 11)
 #define SCR_TWI               (1U << 12)
 #define SCR_TWE               (1U << 13)
+#define SCR_TLOR              (1U << 14)
+#define SCR_TERR              (1U << 15)
+#define SCR_APK               (1U << 16)
+#define SCR_API               (1U << 17)
+#define SCR_EEL2              (1U << 18)
+#define SCR_EASE              (1U << 19)
+#define SCR_NMEA              (1U << 20)
+#define SCR_FIEN              (1U << 21)
+#define SCR_ENSCXT            (1U << 25)
+#define SCR_ATA               (1U << 26)
 #define SCR_AARCH32_MASK      (0x3fff & ~(SCR_RW | SCR_ST))
 #define SCR_AARCH64_MASK      (0x3fff & ~SCR_NET)
 
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 03/10] target/arm: Add SCR_EL3 bits up to ARMv8.5

[Peter Maydell]

On Mon, 3 Dec 2018 at 20:38, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Post v8.4 bits taken from SysReg_v85_xml-00bet8.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>

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

thanks
-- PMM

[Qemu-devel] [PATCH v2 04/10] target/arm: Fix HCR_EL2.TGE check in arm_phys_excp_target_el

[Richard Henderson]

The enable for TGE has already occurred within arm_hcr_el2_amo
and friends.  Moreover, when E2H is also set, the sense is
supposed to be reversed, which has also already occurred within
the helpers.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 04c4a91b04..bf020364e1 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6537,9 +6537,6 @@ uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
         break;
     };
 
-    /* If HCR.TGE is set then HCR is treated as being 1 */
-    hcr |= ((env->cp15.hcr_el2 & HCR_TGE) == HCR_TGE);
-
     /* Perform a table-lookup for the target EL given the current state */
     target_el = target_el_table[is64][scr][rw][hcr][secure][cur_el];
 
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 04/10] target/arm: Fix HCR_EL2.TGE check in arm_phys_excp_target_el

[Peter Maydell]

On Mon, 3 Dec 2018 at 20:38, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The enable for TGE has already occurred within arm_hcr_el2_amo
> and friends.  Moreover, when E2H is also set, the sense is
> supposed to be reversed, which has also already occurred within
> the helpers.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/helper.c | 3 ---
>  1 file changed, 3 deletions(-)

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

thanks
-- PMM

[Qemu-devel] [PATCH v2 05/10] target/arm: Introduce arm_hcr_el2_eff

[Richard Henderson]

Replace arm_hcr_el2_{fmo,imo,amo} with a more general routine
that also takes SCR_EL3.NS (aka arm_is_secure_below_el3) into
account, as documented for the plethora of bits in HCR_EL2.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h          | 67 +++++++++------------------------------
 hw/intc/arm_gicv3_cpuif.c | 21 ++++++------
 target/arm/helper.c       | 65 +++++++++++++++++++++++++++++++------
 3 files changed, 82 insertions(+), 71 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 20d97b66de..e871b946c8 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1722,6 +1722,14 @@ static inline bool arm_is_secure(CPUARMState *env)
 }
 #endif
 
+/**
+ * arm_hcr_el2_eff(): Return the effective value of HCR_EL2.
+ * E.g. when in secure state, fields in HCR_EL2 are suppressed,
+ * "for all purposes other than a direct read or write access of HCR_EL2."
+ * Not included here are RW, VI, VF.
+ */
+uint64_t arm_hcr_el2_eff(CPUARMState *env);
+
 /* Return true if the specified exception level is running in AArch64 state. */
 static inline bool arm_el_is_aa64(CPUARMState *env, int el)
 {
@@ -2407,54 +2415,6 @@ bool write_cpustate_to_list(ARMCPU *cpu);
 #  define TARGET_VIRT_ADDR_SPACE_BITS 32
 #endif
 
-/**
- * arm_hcr_el2_imo(): Return the effective value of HCR_EL2.IMO.
- * Depending on the values of HCR_EL2.E2H and TGE, this may be
- * "behaves as 1 for all purposes other than direct read/write" or
- * "behaves as 0 for all purposes other than direct read/write"
- */
-static inline bool arm_hcr_el2_imo(CPUARMState *env)
-{
-    switch (env->cp15.hcr_el2 & (HCR_TGE | HCR_E2H)) {
-    case HCR_TGE:
-        return true;
-    case HCR_TGE | HCR_E2H:
-        return false;
-    default:
-        return env->cp15.hcr_el2 & HCR_IMO;
-    }
-}
-
-/**
- * arm_hcr_el2_fmo(): Return the effective value of HCR_EL2.FMO.
- */
-static inline bool arm_hcr_el2_fmo(CPUARMState *env)
-{
-    switch (env->cp15.hcr_el2 & (HCR_TGE | HCR_E2H)) {
-    case HCR_TGE:
-        return true;
-    case HCR_TGE | HCR_E2H:
-        return false;
-    default:
-        return env->cp15.hcr_el2 & HCR_FMO;
-    }
-}
-
-/**
- * arm_hcr_el2_amo(): Return the effective value of HCR_EL2.AMO.
- */
-static inline bool arm_hcr_el2_amo(CPUARMState *env)
-{
-    switch (env->cp15.hcr_el2 & (HCR_TGE | HCR_E2H)) {
-    case HCR_TGE:
-        return true;
-    case HCR_TGE | HCR_E2H:
-        return false;
-    default:
-        return env->cp15.hcr_el2 & HCR_AMO;
-    }
-}
-
 static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
                                      unsigned int target_el)
 {
@@ -2463,6 +2423,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
     bool secure = arm_is_secure(env);
     bool pstate_unmasked;
     int8_t unmasked = 0;
+    uint64_t hcr_el2;
 
     /* Don't take exceptions if they target a lower EL.
      * This check should catch any exceptions that would not be taken but left
@@ -2472,6 +2433,8 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
         return false;
     }
 
+    hcr_el2 = arm_hcr_el2_eff(env);
+
     switch (excp_idx) {
     case EXCP_FIQ:
         pstate_unmasked = !(env->daif & PSTATE_F);
@@ -2482,13 +2445,13 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
         break;
 
     case EXCP_VFIQ:
-        if (secure || !arm_hcr_el2_fmo(env) || (env->cp15.hcr_el2 & HCR_TGE)) {
+        if (secure || !(hcr_el2 & HCR_FMO) || (hcr_el2 & HCR_TGE)) {
             /* VFIQs are only taken when hypervized and non-secure.  */
             return false;
         }
         return !(env->daif & PSTATE_F);
     case EXCP_VIRQ:
-        if (secure || !arm_hcr_el2_imo(env) || (env->cp15.hcr_el2 & HCR_TGE)) {
+        if (secure || !(hcr_el2 & HCR_IMO) || (hcr_el2 & HCR_TGE)) {
             /* VIRQs are only taken when hypervized and non-secure.  */
             return false;
         }
@@ -2527,7 +2490,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
                  * to the CPSR.F setting otherwise we further assess the state
                  * below.
                  */
-                hcr = arm_hcr_el2_fmo(env);
+                hcr = hcr_el2 & HCR_FMO;
                 scr = (env->cp15.scr_el3 & SCR_FIQ);
 
                 /* When EL3 is 32-bit, the SCR.FW bit controls whether the
@@ -2544,7 +2507,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
                  * when setting the target EL, so it does not have a further
                  * affect here.
                  */
-                hcr = arm_hcr_el2_imo(env);
+                hcr = hcr_el2 & HCR_IMO;
                 scr = false;
                 break;
             default:
diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 068a8e8e9b..cbad6037f1 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -85,8 +85,8 @@ static bool icv_access(CPUARMState *env, int hcr_flags)
      *  * access if NS EL1 and either IMO or FMO == 1:
      *    CTLR, DIR, PMR, RPR
      */
-    bool flagmatch = ((hcr_flags & HCR_IMO) && arm_hcr_el2_imo(env)) ||
-        ((hcr_flags & HCR_FMO) && arm_hcr_el2_fmo(env));
+    uint64_t hcr_el2 = arm_hcr_el2_eff(env);
+    bool flagmatch = hcr_el2 & hcr_flags & (HCR_IMO | HCR_FMO);
 
     return flagmatch && arm_current_el(env) == 1
         && !arm_is_secure_below_el3(env);
@@ -1552,8 +1552,9 @@ static void icc_dir_write(CPUARMState *env, const ARMCPRegInfo *ri,
     /* No need to include !IsSecure in route_*_to_el2 as it's only
      * tested in cases where we know !IsSecure is true.
      */
-    route_fiq_to_el2 = arm_hcr_el2_fmo(env);
-    route_irq_to_el2 = arm_hcr_el2_imo(env);
+    uint64_t hcr_el2 = arm_hcr_el2_eff(env);
+    route_fiq_to_el2 = hcr_el2 & HCR_FMO;
+    route_irq_to_el2 = hcr_el2 & HCR_IMO;
 
     switch (arm_current_el(env)) {
     case 3:
@@ -1895,8 +1896,8 @@ static CPAccessResult gicv3_irqfiq_access(CPUARMState *env,
     if ((env->cp15.scr_el3 & (SCR_FIQ | SCR_IRQ)) == (SCR_FIQ | SCR_IRQ)) {
         switch (el) {
         case 1:
-            if (arm_is_secure_below_el3(env) ||
-                (arm_hcr_el2_imo(env) == 0 && arm_hcr_el2_fmo(env) == 0)) {
+            /* Note that arm_hcr_el2_eff takes secure state into account.  */
+            if ((arm_hcr_el2_eff(env) & (HCR_IMO | HCR_FMO)) == 0) {
                 r = CP_ACCESS_TRAP_EL3;
             }
             break;
@@ -1936,8 +1937,8 @@ static CPAccessResult gicv3_dir_access(CPUARMState *env,
 static CPAccessResult gicv3_sgi_access(CPUARMState *env,
                                        const ARMCPRegInfo *ri, bool isread)
 {
-    if ((arm_hcr_el2_imo(env) || arm_hcr_el2_fmo(env)) &&
-        arm_current_el(env) == 1 && !arm_is_secure_below_el3(env)) {
+    if (arm_current_el(env) == 1 &&
+        (arm_hcr_el2_eff(env) & (HCR_IMO | HCR_FMO)) != 0) {
         /* Takes priority over a possible EL3 trap */
         return CP_ACCESS_TRAP_EL2;
     }
@@ -1961,7 +1962,7 @@ static CPAccessResult gicv3_fiq_access(CPUARMState *env,
     if (env->cp15.scr_el3 & SCR_FIQ) {
         switch (el) {
         case 1:
-            if (arm_is_secure_below_el3(env) || !arm_hcr_el2_fmo(env)) {
+            if ((arm_hcr_el2_eff(env) & HCR_FMO) == 0) {
                 r = CP_ACCESS_TRAP_EL3;
             }
             break;
@@ -2000,7 +2001,7 @@ static CPAccessResult gicv3_irq_access(CPUARMState *env,
     if (env->cp15.scr_el3 & SCR_IRQ) {
         switch (el) {
         case 1:
-            if (arm_is_secure_below_el3(env) || !arm_hcr_el2_imo(env)) {
+            if ((arm_hcr_el2_eff(env) & HCR_IMO) == 0) {
                 r = CP_ACCESS_TRAP_EL3;
             }
             break;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index bf020364e1..5874c5a73f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1327,9 +1327,10 @@ static void csselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 static uint64_t isr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
     CPUState *cs = ENV_GET_CPU(env);
+    uint64_t hcr_el2 = arm_hcr_el2_eff(env);
     uint64_t ret = 0;
 
-    if (arm_hcr_el2_imo(env)) {
+    if (hcr_el2 & HCR_IMO) {
         if (cs->interrupt_request & CPU_INTERRUPT_VIRQ) {
             ret |= CPSR_I;
         }
@@ -1339,7 +1340,7 @@ static uint64_t isr_read(CPUARMState *env, const ARMCPRegInfo *ri)
         }
     }
 
-    if (arm_hcr_el2_fmo(env)) {
+    if (hcr_el2 & HCR_FMO) {
         if (cs->interrupt_request & CPU_INTERRUPT_VFIQ) {
             ret |= CPSR_F;
         }
@@ -3991,6 +3992,50 @@ static void hcr_writelow(CPUARMState *env, const ARMCPRegInfo *ri,
     hcr_write(env, NULL, value);
 }
 
+/*
+ * Return the effective value of HCR_EL2.
+ * Bits that are not included here:
+ * RW       (read from SCR_EL3.RW as needed)
+ */
+uint64_t arm_hcr_el2_eff(CPUARMState *env)
+{
+    uint64_t ret = env->cp15.hcr_el2;
+
+    if (arm_is_secure_below_el3(env)) {
+        /*
+         * "This register has no effect if EL2 is not enabled in the
+         * current Security state".  This is ARMv8.4-SecEL2 speak for
+         * !(SCR_EL3.NS==1 || SCR_EL3.EEL2==1).
+         *
+         * Prior to that, the language was "In an implementation that
+         * includes EL3, when the value of SCR_EL3.NS is 0 the PE behaves
+         * as if this field is 0 for all purposes other than a direct
+         * read or write access of HCR_EL2".  With lots of enumeration
+         * on a per-field basis.  In current QEMU, this is condition
+         * is arm_is_secure_below_el3.
+         *
+         * Since the v8.4 language applies to the entire register, and
+         * appears to be backward compatible, use that.
+         */
+        ret = 0;
+    } else if (ret & HCR_TGE) {
+        if (ret & HCR_E2H) {
+            ret &= ~(HCR_TTLBOS | HCR_TTLBIS | HCR_TOCU | HCR_TICAB |
+                     HCR_TID4 | HCR_ID | HCR_CD | HCR_TDZ | HCR_TPU |
+                     HCR_TPCP | HCR_TID2 | HCR_TID1 | HCR_TID0 | HCR_TWE |
+                     HCR_TWI | HCR_DC | HCR_BSU_MASK | HCR_FB | HCR_AMO |
+                     HCR_IMO | HCR_FMO | HCR_VM);
+        } else {
+            ret &= ~(HCR_PTW | HCR_FB | HCR_TID1 | HCR_TID3 | HCR_TSC |
+                     HCR_TACR | HCR_TSW | HCR_TTLB | HCR_TVM | HCR_HCD |
+                     HCR_TRVM | HCR_TLOR);
+            ret |= HCR_FMO | HCR_IMO | HCR_AMO;
+        }
+    }
+
+    return ret;
+}
+
 static const ARMCPRegInfo el2_cp_reginfo[] = {
     { .name = "HCR_EL2", .state = ARM_CP_STATE_AA64,
       .type = ARM_CP_IO,
@@ -6505,12 +6550,13 @@ uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
                                  uint32_t cur_el, bool secure)
 {
     CPUARMState *env = cs->env_ptr;
-    int rw;
-    int scr;
-    int hcr;
+    bool rw;
+    bool scr;
+    bool hcr;
     int target_el;
     /* Is the highest EL AArch64? */
-    int is64 = arm_feature(env, ARM_FEATURE_AARCH64);
+    bool is64 = arm_feature(env, ARM_FEATURE_AARCH64);
+    uint64_t hcr_el2;
 
     if (arm_feature(env, ARM_FEATURE_EL3)) {
         rw = ((env->cp15.scr_el3 & SCR_RW) == SCR_RW);
@@ -6522,18 +6568,19 @@ uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
         rw = is64;
     }
 
+    hcr_el2 = arm_hcr_el2_eff(env);
     switch (excp_idx) {
     case EXCP_IRQ:
         scr = ((env->cp15.scr_el3 & SCR_IRQ) == SCR_IRQ);
-        hcr = arm_hcr_el2_imo(env);
+        hcr = hcr_el2 & HCR_IMO;
         break;
     case EXCP_FIQ:
         scr = ((env->cp15.scr_el3 & SCR_FIQ) == SCR_FIQ);
-        hcr = arm_hcr_el2_fmo(env);
+        hcr = hcr_el2 & HCR_FMO;
         break;
     default:
         scr = ((env->cp15.scr_el3 & SCR_EA) == SCR_EA);
-        hcr = arm_hcr_el2_amo(env);
+        hcr = hcr_el2 & HCR_AMO;
         break;
     };
 
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 05/10] target/arm: Introduce arm_hcr_el2_eff

[Peter Maydell]

On Mon, 3 Dec 2018 at 20:38, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Replace arm_hcr_el2_{fmo,imo,amo} with a more general routine
> that also takes SCR_EL3.NS (aka arm_is_secure_below_el3) into
> account, as documented for the plethora of bits in HCR_EL2.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h          | 67 +++++++++------------------------------
>  hw/intc/arm_gicv3_cpuif.c | 21 ++++++------
>  target/arm/helper.c       | 65 +++++++++++++++++++++++++++++++------
>  3 files changed, 82 insertions(+), 71 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 20d97b66de..e871b946c8 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1722,6 +1722,14 @@ static inline bool arm_is_secure(CPUARMState *env)
>  }
>  #endif
>
> +/**
> + * arm_hcr_el2_eff(): Return the effective value of HCR_EL2.
> + * E.g. when in secure state, fields in HCR_EL2 are suppressed,
> + * "for all purposes other than a direct read or write access of HCR_EL2."
> + * Not included here are RW, VI, VF.
> + */
> +uint64_t arm_hcr_el2_eff(CPUARMState *env);

This comment says the not-included bits are RW, VI, VF...

> +/*
> + * Return the effective value of HCR_EL2.
> + * Bits that are not included here:
> + * RW       (read from SCR_EL3.RW as needed)
> + */

...but this comment only lists RW. Which is correct ?

Also, if VI, VF are in the list shouldn't VSE be too ?

> +uint64_t arm_hcr_el2_eff(CPUARMState *env)
> +{
> +    uint64_t ret = env->cp15.hcr_el2;
> +
> +    if (arm_is_secure_below_el3(env)) {
> +        /*
> +         * "This register has no effect if EL2 is not enabled in the
> +         * current Security state".  This is ARMv8.4-SecEL2 speak for
> +         * !(SCR_EL3.NS==1 || SCR_EL3.EEL2==1).
> +         *
> +         * Prior to that, the language was "In an implementation that
> +         * includes EL3, when the value of SCR_EL3.NS is 0 the PE behaves
> +         * as if this field is 0 for all purposes other than a direct
> +         * read or write access of HCR_EL2".  With lots of enumeration
> +         * on a per-field basis.  In current QEMU, this is condition
> +         * is arm_is_secure_below_el3.
> +         *
> +         * Since the v8.4 language applies to the entire register, and
> +         * appears to be backward compatible, use that.
> +         */
> +        ret = 0;
> +    } else if (ret & HCR_TGE) {
> +        if (ret & HCR_E2H) {
> +            ret &= ~(HCR_TTLBOS | HCR_TTLBIS | HCR_TOCU | HCR_TICAB |
> +                     HCR_TID4 | HCR_ID | HCR_CD | HCR_TDZ | HCR_TPU |
> +                     HCR_TPCP | HCR_TID2 | HCR_TID1 | HCR_TID0 | HCR_TWE |
> +                     HCR_TWI | HCR_DC | HCR_BSU_MASK | HCR_FB | HCR_AMO |
> +                     HCR_IMO | HCR_FMO | HCR_VM);


This list should also in theory include HCR_MIOCNCE, but the
QEMU implementation of that bit is going to be RAZ/WI anyway.

HCR_TID1 is in the "ignore if TGE==1" group, not the "ignore if
{E2H, TGE} == {1,1}" group.

Maybe we should be also setting HCR_ATA here ? We could perhaps
leave that til we implement ARMv8.5-MemTag and understand it better.

> +        } else {
> +            ret &= ~(HCR_PTW | HCR_FB | HCR_TID1 | HCR_TID3 | HCR_TSC |
> +                     HCR_TACR | HCR_TSW | HCR_TTLB | HCR_TVM | HCR_HCD |
> +                     HCR_TRVM | HCR_TLOR);

Shouldn't these bits be being cleared regardless of E2H, rather
than only in the TGE==1 E2H==0 case ?

Missing HCR_SWIO ?

The list of bits cleared if TGE && E2H is highest-first, but this
list is lowest-first...

> +            ret |= HCR_FMO | HCR_IMO | HCR_AMO;
> +        }
> +    }
> +
> +    return ret;
> +}

Patch looks good otherwise.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 05/10] target/arm: Introduce arm_hcr_el2_eff

[Richard Henderson]

On 12/6/18 6:09 AM, Peter Maydell wrote:
> On Mon, 3 Dec 2018 at 20:38, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Replace arm_hcr_el2_{fmo,imo,amo} with a more general routine
>> that also takes SCR_EL3.NS (aka arm_is_secure_below_el3) into
>> account, as documented for the plethora of bits in HCR_EL2.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  target/arm/cpu.h          | 67 +++++++++------------------------------
>>  hw/intc/arm_gicv3_cpuif.c | 21 ++++++------
>>  target/arm/helper.c       | 65 +++++++++++++++++++++++++++++++------
>>  3 files changed, 82 insertions(+), 71 deletions(-)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index 20d97b66de..e871b946c8 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -1722,6 +1722,14 @@ static inline bool arm_is_secure(CPUARMState *env)
>>  }
>>  #endif
>>
>> +/**
>> + * arm_hcr_el2_eff(): Return the effective value of HCR_EL2.
>> + * E.g. when in secure state, fields in HCR_EL2 are suppressed,
>> + * "for all purposes other than a direct read or write access of HCR_EL2."
>> + * Not included here are RW, VI, VF.
>> + */
>> +uint64_t arm_hcr_el2_eff(CPUARMState *env);
> 
> This comment says the not-included bits are RW, VI, VF...
> 
>> +/*
>> + * Return the effective value of HCR_EL2.
>> + * Bits that are not included here:
>> + * RW       (read from SCR_EL3.RW as needed)
>> + */
> 
> ...but this comment only lists RW. Which is correct ?
> 
> Also, if VI, VF are in the list shouldn't VSE be too ?

VI and VF were on the list when I first wrote the patch, then one of the
target-arm.next patches rearranged how those bits are handled.

So correct is that they are *not* excluded.

>> +    } else if (ret & HCR_TGE) {
>> +        if (ret & HCR_E2H) {
>> +            ret &= ~(HCR_TTLBOS | HCR_TTLBIS | HCR_TOCU | HCR_TICAB |
>> +                     HCR_TID4 | HCR_ID | HCR_CD | HCR_TDZ | HCR_TPU |
>> +                     HCR_TPCP | HCR_TID2 | HCR_TID1 | HCR_TID0 | HCR_TWE |
>> +                     HCR_TWI | HCR_DC | HCR_BSU_MASK | HCR_FB | HCR_AMO |
>> +                     HCR_IMO | HCR_FMO | HCR_VM);
> 
> 
> This list should also in theory include HCR_MIOCNCE, but the
> QEMU implementation of that bit is going to be RAZ/WI anyway.

Even if we do RAZ/WI, it's probably better to match the docs and exclude it
here.  Fixed.

> 
> HCR_TID1 is in the "ignore if TGE==1" group, not the "ignore if
> {E2H, TGE} == {1,1}" group.

Fixed.

> 
> Maybe we should be also setting HCR_ATA here ? We could perhaps
> leave that til we implement ARMv8.5-MemTag and understand it better.

This list was created with the ARMv8.4 document.
Perhaps a comment to that effect while v8.5 is still in flight?

> 
>> +        } else {
>> +            ret &= ~(HCR_PTW | HCR_FB | HCR_TID1 | HCR_TID3 | HCR_TSC |
>> +                     HCR_TACR | HCR_TSW | HCR_TTLB | HCR_TVM | HCR_HCD |
>> +                     HCR_TRVM | HCR_TLOR);
> 
> Shouldn't these bits be being cleared regardless of E2H, rather
> than only in the TGE==1 E2H==0 case ?

Fixed.

> Missing HCR_SWIO ?

Fixed.

> The list of bits cleared if TGE && E2H is highest-first, but this
> list is lowest-first...

Both lists now lowest first.


r~

[Qemu-devel] [PATCH v2 06/10] target/arm: Use arm_hcr_el2_eff more places

[Richard Henderson]

Since arm_hcr_el2_eff includes a check against
arm_is_secure_below_el3, we can often remove a
nearby check against secure state.

In some cases, sort the call to arm_hcr_el2_eff
to the end of a short-circuit logical sequence.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.c    | 22 ++++++++++++----------
 target/arm/op_helper.c | 14 ++++++--------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 5874c5a73f..b248dfcd39 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -448,7 +448,7 @@ static CPAccessResult access_tdosa(CPUARMState *env, const ARMCPRegInfo *ri,
     int el = arm_current_el(env);
     bool mdcr_el2_tdosa = (env->cp15.mdcr_el2 & MDCR_TDOSA) ||
         (env->cp15.mdcr_el2 & MDCR_TDE) ||
-        (env->cp15.hcr_el2 & HCR_TGE);
+        (arm_hcr_el2_eff(env) & HCR_TGE);
 
     if (el < 2 && mdcr_el2_tdosa && !arm_is_secure_below_el3(env)) {
         return CP_ACCESS_TRAP_EL2;
@@ -468,7 +468,7 @@ static CPAccessResult access_tdra(CPUARMState *env, const ARMCPRegInfo *ri,
     int el = arm_current_el(env);
     bool mdcr_el2_tdra = (env->cp15.mdcr_el2 & MDCR_TDRA) ||
         (env->cp15.mdcr_el2 & MDCR_TDE) ||
-        (env->cp15.hcr_el2 & HCR_TGE);
+        (arm_hcr_el2_eff(env) & HCR_TGE);
 
     if (el < 2 && mdcr_el2_tdra && !arm_is_secure_below_el3(env)) {
         return CP_ACCESS_TRAP_EL2;
@@ -488,7 +488,7 @@ static CPAccessResult access_tda(CPUARMState *env, const ARMCPRegInfo *ri,
     int el = arm_current_el(env);
     bool mdcr_el2_tda = (env->cp15.mdcr_el2 & MDCR_TDA) ||
         (env->cp15.mdcr_el2 & MDCR_TDE) ||
-        (env->cp15.hcr_el2 & HCR_TGE);
+        (arm_hcr_el2_eff(env) & HCR_TGE);
 
     if (el < 2 && mdcr_el2_tda && !arm_is_secure_below_el3(env)) {
         return CP_ACCESS_TRAP_EL2;
@@ -4548,8 +4548,7 @@ int sve_exception_el(CPUARMState *env, int el)
         if (disabled) {
             /* route_to_el2 */
             return (arm_feature(env, ARM_FEATURE_EL2)
-                    && !arm_is_secure(env)
-                    && (env->cp15.hcr_el2 & HCR_TGE) ? 2 : 1);
+                    && (arm_hcr_el2_eff(env) & HCR_TGE) ? 2 : 1);
         }
 
         /* Check CPACR.FPEN.  */
@@ -6194,9 +6193,8 @@ static int bad_mode_switch(CPUARMState *env, int mode, CPSRWriteType write_type)
          * and CPS are treated as illegal mode changes.
          */
         if (write_type == CPSRWriteByInstr &&
-            (env->cp15.hcr_el2 & HCR_TGE) &&
             (env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_MON &&
-            !arm_is_secure_below_el3(env)) {
+            (arm_hcr_el2_eff(env) & HCR_TGE)) {
             return 1;
         }
         return 0;
@@ -8797,6 +8795,8 @@ static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx mmu_idx)
 static inline bool regime_translation_disabled(CPUARMState *env,
                                                ARMMMUIdx mmu_idx)
 {
+    uint64_t hcr_el2;
+
     if (arm_feature(env, ARM_FEATURE_M)) {
         switch (env->v7m.mpu_ctrl[regime_is_secure(env, mmu_idx)] &
                 (R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK)) {
@@ -8815,19 +8815,21 @@ static inline bool regime_translation_disabled(CPUARMState *env,
         }
     }
 
+    hcr_el2 = arm_hcr_el2_eff(env);
+
     if (mmu_idx == ARMMMUIdx_S2NS) {
         /* HCR.DC means HCR.VM behaves as 1 */
-        return (env->cp15.hcr_el2 & (HCR_DC | HCR_VM)) == 0;
+        return (hcr_el2 & (HCR_DC | HCR_VM)) == 0;
     }
 
-    if (env->cp15.hcr_el2 & HCR_TGE) {
+    if (hcr_el2 & HCR_TGE) {
         /* TGE means that NS EL0/1 act as if SCTLR_EL1.M is zero */
         if (!regime_is_secure(env, mmu_idx) && regime_el(env, mmu_idx) == 1) {
             return true;
         }
     }
 
-    if ((env->cp15.hcr_el2 & HCR_DC) &&
+    if ((hcr_el2 & HCR_DC) &&
         (mmu_idx == ARMMMUIdx_S1NSE0 || mmu_idx == ARMMMUIdx_S1NSE1)) {
         /* HCR.DC means SCTLR_EL1.M behaves as 0 */
         return true;
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 0d6e89e474..780caf387b 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -33,8 +33,7 @@ void raise_exception(CPUARMState *env, uint32_t excp,
 {
     CPUState *cs = CPU(arm_env_get_cpu(env));
 
-    if ((env->cp15.hcr_el2 & HCR_TGE) &&
-        target_el == 1 && !arm_is_secure(env)) {
+    if (target_el == 1 && (arm_hcr_el2_eff(env) & HCR_TGE)) {
         /*
          * Redirect NS EL1 exceptions to NS EL2. These are reported with
          * their original syndrome register value, with the exception of
@@ -428,9 +427,9 @@ static inline int check_wfx_trap(CPUARMState *env, bool is_wfe)
      * No need for ARM_FEATURE check as if HCR_EL2 doesn't exist the
      * bits will be zero indicating no trap.
      */
-    if (cur_el < 2 && !arm_is_secure(env)) {
-        mask = (is_wfe) ? HCR_TWE : HCR_TWI;
-        if (env->cp15.hcr_el2 & mask) {
+    if (cur_el < 2) {
+        mask = is_wfe ? HCR_TWE : HCR_TWI;
+        if (arm_hcr_el2_eff(env) & mask) {
             return 2;
         }
     }
@@ -995,7 +994,7 @@ void HELPER(pre_smc)(CPUARMState *env, uint32_t syndrome)
                         exception_target_el(env));
     }
 
-    if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) {
+    if (!secure && cur_el == 1 && (arm_hcr_el2_eff(env) & HCR_TSC)) {
         /* In NS EL1, HCR controlled routing to EL2 has priority over SMD.
          * We also want an EL2 guest to be able to forbid its EL1 from
          * making PSCI calls into QEMU's "firmware" via HCR.TSC.
@@ -1098,8 +1097,7 @@ void HELPER(exception_return)(CPUARMState *env)
         goto illegal_return;
     }
 
-    if (new_el == 1 && (env->cp15.hcr_el2 & HCR_TGE)
-        && !arm_is_secure_below_el3(env)) {
+    if (new_el == 1 && (arm_hcr_el2_eff(env) & HCR_TGE)) {
         goto illegal_return;
     }
 
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 06/10] target/arm: Use arm_hcr_el2_eff more places

[Peter Maydell]

On Mon, 3 Dec 2018 at 20:38, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Since arm_hcr_el2_eff includes a check against
> arm_is_secure_below_el3, we can often remove a
> nearby check against secure state.
>
> In some cases, sort the call to arm_hcr_el2_eff
> to the end of a short-circuit logical sequence.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---


> @@ -8797,6 +8795,8 @@ static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx mmu_idx)
>  static inline bool regime_translation_disabled(CPUARMState *env,
>                                                 ARMMMUIdx mmu_idx)
>  {
> +    uint64_t hcr_el2;
> +
>      if (arm_feature(env, ARM_FEATURE_M)) {
>          switch (env->v7m.mpu_ctrl[regime_is_secure(env, mmu_idx)] &
>                  (R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK)) {
> @@ -8815,19 +8815,21 @@ static inline bool regime_translation_disabled(CPUARMState *env,
>          }
>      }
>
> +    hcr_el2 = arm_hcr_el2_eff(env);

Changing this in this function makes me nervous, because
arm_hcr_el2_eff() looks at the current CPU state via
arm_is_secure_below_el3() rather than at the security state
of the translation regime we're asking about. I think
it probably doesn't change behaviour, but I'm finding
it hard to reason about...

> +
>      if (mmu_idx == ARMMMUIdx_S2NS) {
>          /* HCR.DC means HCR.VM behaves as 1 */
> -        return (env->cp15.hcr_el2 & (HCR_DC | HCR_VM)) == 0;
> +        return (hcr_el2 & (HCR_DC | HCR_VM)) == 0;
>      }
>
> -    if (env->cp15.hcr_el2 & HCR_TGE) {
> +    if (hcr_el2 & HCR_TGE) {
>          /* TGE means that NS EL0/1 act as if SCTLR_EL1.M is zero */
>          if (!regime_is_secure(env, mmu_idx) && regime_el(env, mmu_idx) == 1) {
>              return true;
>          }
>      }
>
> -    if ((env->cp15.hcr_el2 & HCR_DC) &&
> +    if ((hcr_el2 & HCR_DC) &&
>          (mmu_idx == ARMMMUIdx_S1NSE0 || mmu_idx == ARMMMUIdx_S1NSE1)) {
>          /* HCR.DC means SCTLR_EL1.M behaves as 0 */
>          return true;


> @@ -995,7 +994,7 @@ void HELPER(pre_smc)(CPUARMState *env, uint32_t syndrome)
>                          exception_target_el(env));
>      }
>
> -    if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) {
> +    if (!secure && cur_el == 1 && (arm_hcr_el2_eff(env) & HCR_TSC)) {

Why can't we drop the !secure check here?

>          /* In NS EL1, HCR controlled routing to EL2 has priority over SMD.
>           * We also want an EL2 guest to be able to forbid its EL1 from
>           * making PSCI calls into QEMU's "firmware" via HCR.TSC.
> @@ -1098,8 +1097,7 @@ void HELPER(exception_return)(CPUARMState *env)
>          goto illegal_return;
>      }
>
> -    if (new_el == 1 && (env->cp15.hcr_el2 & HCR_TGE)
> -        && !arm_is_secure_below_el3(env)) {
> +    if (new_el == 1 && (arm_hcr_el2_eff(env) & HCR_TGE)) {
>          goto illegal_return;
>      }

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 06/10] target/arm: Use arm_hcr_el2_eff more places

[Richard Henderson]

On 12/6/18 7:06 AM, Peter Maydell wrote:
> On Mon, 3 Dec 2018 at 20:38, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Since arm_hcr_el2_eff includes a check against
>> arm_is_secure_below_el3, we can often remove a
>> nearby check against secure state.
>>
>> In some cases, sort the call to arm_hcr_el2_eff
>> to the end of a short-circuit logical sequence.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
> 
> 
>> @@ -8797,6 +8795,8 @@ static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx mmu_idx)
>>  static inline bool regime_translation_disabled(CPUARMState *env,
>>                                                 ARMMMUIdx mmu_idx)
>>  {
>> +    uint64_t hcr_el2;
>> +
>>      if (arm_feature(env, ARM_FEATURE_M)) {
>>          switch (env->v7m.mpu_ctrl[regime_is_secure(env, mmu_idx)] &
>>                  (R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK)) {
>> @@ -8815,19 +8815,21 @@ static inline bool regime_translation_disabled(CPUARMState *env,
>>          }
>>      }
>>
>> +    hcr_el2 = arm_hcr_el2_eff(env);
> 
> Changing this in this function makes me nervous, because
> arm_hcr_el2_eff() looks at the current CPU state via
> arm_is_secure_below_el3() rather than at the security state
> of the translation regime we're asking about. I think
> it probably doesn't change behaviour, but I'm finding
> it hard to reason about...

Oops, I missed that we weren't talking about "current" here.

I can either just revert this function for now, or introduce a new
arm_hcr_el2_eff_ns that does not take secure into effect, but does adjust all
of the other flags within HCR relative to its own contents.

>> -    if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) {
>> +    if (!secure && cur_el == 1 && (arm_hcr_el2_eff(env) & HCR_TSC)) {
> 
> Why can't we drop the !secure check here?

Either I missed it, or it was premature optimization of the form "well, it's
already computed into a local variable..."


r~

Re: [Qemu-devel] [PATCH v2 06/10] target/arm: Use arm_hcr_el2_eff more places

[Peter Maydell]

On Thu, 6 Dec 2018 at 13:32, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 12/6/18 7:06 AM, Peter Maydell wrote:
> > Changing this in this function makes me nervous, because
> > arm_hcr_el2_eff() looks at the current CPU state via
> > arm_is_secure_below_el3() rather than at the security state
> > of the translation regime we're asking about. I think
> > it probably doesn't change behaviour, but I'm finding
> > it hard to reason about...
>
> Oops, I missed that we weren't talking about "current" here.
>
> I can either just revert this function for now, or introduce a new
> arm_hcr_el2_eff_ns that does not take secure into effect, but does adjust all
> of the other flags within HCR relative to its own contents.

I think I'd just not change this function.

thanks
-- PMM

[Qemu-devel] [PATCH v2 07/10] target/arm: Tidy scr_write

[Richard Henderson]

Because EL3 has a fixed execution mode, we can properly decide
which of the bits are RES{0,1}.

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

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index e871b946c8..a84101efa9 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1312,8 +1312,6 @@ static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
 #define SCR_FIEN              (1U << 21)
 #define SCR_ENSCXT            (1U << 25)
 #define SCR_ATA               (1U << 26)
-#define SCR_AARCH32_MASK      (0x3fff & ~(SCR_RW | SCR_ST))
-#define SCR_AARCH64_MASK      (0x3fff & ~SCR_NET)
 
 /* Return the current FPSCR value.  */
 uint32_t vfp_get_fpscr(CPUARMState *env);
diff --git a/target/arm/helper.c b/target/arm/helper.c
index b248dfcd39..faf7f922bf 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1279,11 +1279,15 @@ static void vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
 
 static void scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
-    /* We only mask off bits that are RES0 both for AArch64 and AArch32.
-     * For bits that vary between AArch32/64, code needs to check the
-     * current execution mode before directly using the feature bit.
-     */
-    uint32_t valid_mask = SCR_AARCH64_MASK | SCR_AARCH32_MASK;
+    /* Begin with base v8.0 state.  */
+    uint32_t valid_mask = 0x3fff;
+
+    if (arm_el_is_aa64(env, 3)) {
+        value |= SCR_FW | SCR_AW;   /* these two bits are RES1.  */
+        valid_mask &= ~SCR_NET;
+    } else {
+        valid_mask &= ~(SCR_RW | SCR_ST);
+    }
 
     if (!arm_feature(env, ARM_FEATURE_EL2)) {
         valid_mask &= ~SCR_HCE;
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 07/10] target/arm: Tidy scr_write

[Peter Maydell]

On Mon, 3 Dec 2018 at 20:38, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Because EL3 has a fixed execution mode, we can properly decide
> which of the bits are RES{0,1}.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h    |  2 --
>  target/arm/helper.c | 14 +++++++++-----
>  2 files changed, 9 insertions(+), 7 deletions(-)

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

thanks
-- PMM

[Qemu-devel] [PATCH v2 08/10] target/arm: Implement the ARMv8.1-LOR extension

[Richard Henderson]

Provide a trivial implementation with zero limited ordering regions,
which causes the LDLAR and STLLR instructions to devolve into the
LDAR and STLR instructions from the base ARMv8.0 instruction set.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
v2: Mark LORID_EL1 read-only.
    Add TLOR access checks.
    Conditionally unmask TLOR in hcr/scr_write.
---
 target/arm/cpu.h           |  5 +++
 target/arm/cpu64.c         |  4 ++
 target/arm/helper.c        | 91 ++++++++++++++++++++++++++++++++++++++
 target/arm/translate-a64.c | 12 +++++
 4 files changed, 112 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index a84101efa9..ba0c368292 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3285,6 +3285,11 @@ static inline bool isar_feature_aa64_atomics(const ARMISARegisters *id)
     return FIELD_EX64(id->id_aa64isar0, ID_AA64ISAR0, ATOMIC) != 0;
 }
 
+static inline bool isar_feature_aa64_lor(const ARMISARegisters *id)
+{
+    return FIELD_EX64(id->id_aa64mmfr0, ID_AA64MMFR1, LO) != 0;
+}
+
 static inline bool isar_feature_aa64_rdm(const ARMISARegisters *id)
 {
     return FIELD_EX64(id->id_aa64isar0, ID_AA64ISAR0, RDM) != 0;
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 0babe483ac..aac6283018 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -324,6 +324,10 @@ static void aarch64_max_initfn(Object *obj)
         t = FIELD_DP64(t, ID_AA64PFR0, ADVSIMD, 1);
         cpu->isar.id_aa64pfr0 = t;
 
+        t = cpu->isar.id_aa64mmfr1;
+        t = FIELD_DP64(t, ID_AA64MMFR1, LO, 1);
+        cpu->isar.id_aa64mmfr1 = 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 */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index faf7f922bf..a0ee1fbc5a 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1281,6 +1281,7 @@ static void scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
     /* Begin with base v8.0 state.  */
     uint32_t valid_mask = 0x3fff;
+    ARMCPU *cpu = arm_env_get_cpu(env);
 
     if (arm_el_is_aa64(env, 3)) {
         value |= SCR_FW | SCR_AW;   /* these two bits are RES1.  */
@@ -1303,6 +1304,9 @@ static void scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
             valid_mask &= ~SCR_SMD;
         }
     }
+    if (cpu_isar_feature(aa64_lor, cpu)) {
+        valid_mask |= SCR_TLOR;
+    }
 
     /* Clear all-context RES0 bits.  */
     value &= valid_mask;
@@ -3950,6 +3954,9 @@ static void hcr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
          */
         valid_mask &= ~HCR_TSC;
     }
+    if (cpu_isar_feature(aa64_lor, cpu)) {
+        valid_mask |= HCR_TLOR;
+    }
 
     /* Clear RES0 bits.  */
     value &= valid_mask;
@@ -5004,6 +5011,58 @@ static uint64_t id_aa64pfr0_read(CPUARMState *env, const ARMCPRegInfo *ri)
     return pfr0;
 }
 
+static CPAccessResult access_lorid(CPUARMState *env, const ARMCPRegInfo *ri,
+                                   bool isread)
+{
+    int el = arm_current_el(env);
+
+    if (arm_is_secure_below_el3(env)) {
+        /* Access ok in secure mode.  */
+        return CP_ACCESS_OK;
+    }
+    if (el < 2 && arm_el_is_aa64(env, 2)) {
+        uint64_t hcr = arm_hcr_el2_eff(env);
+        if (hcr & HCR_E2H) {
+            hcr &= HCR_TLOR;
+        } else {
+            hcr &= HCR_TGE | HCR_TLOR;
+        }
+        if (hcr == HCR_TLOR) {
+            return CP_ACCESS_TRAP_EL2;
+        }
+    }
+    if (el < 3 && (env->cp15.scr_el3 & SCR_TLOR)) {
+        return CP_ACCESS_TRAP_EL3;
+    }
+    return CP_ACCESS_OK;
+}
+
+static CPAccessResult access_lor_other(CPUARMState *env,
+                                       const ARMCPRegInfo *ri, bool isread)
+{
+    int el = arm_current_el(env);
+
+    if (arm_is_secure_below_el3(env)) {
+        /* Access denied in secure mode.  */
+        return CP_ACCESS_TRAP;
+    }
+    if (el < 2 && arm_el_is_aa64(env, 2)) {
+        uint64_t hcr = arm_hcr_el2_eff(env);
+        if (hcr & HCR_E2H) {
+            hcr &= HCR_TLOR;
+        } else {
+            hcr &= HCR_TGE | HCR_TLOR;
+        }
+        if (hcr == HCR_TLOR) {
+            return CP_ACCESS_TRAP_EL2;
+        }
+    }
+    if (el < 3 && (env->cp15.scr_el3 & SCR_TLOR)) {
+        return CP_ACCESS_TRAP_EL3;
+    }
+    return CP_ACCESS_OK;
+}
+
 void register_cp_regs_for_features(ARMCPU *cpu)
 {
     /* Register all the coprocessor registers based on feature bits */
@@ -5741,6 +5800,38 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         define_one_arm_cp_reg(cpu, &sctlr);
     }
 
+    if (cpu_isar_feature(aa64_lor, cpu)) {
+        /*
+         * A trivial implementation of ARMv8.1-LOR leaves all of these
+         * registers fixed at 0, which indicates that there are zero
+         * supported Limited Ordering regions.
+         */
+        static const ARMCPRegInfo lor_reginfo[] = {
+            { .name = "LORSA_EL1", .state = ARM_CP_STATE_AA64,
+              .opc0 = 3, .opc1 = 0, .crn = 10, .crm = 4, .opc2 = 0,
+              .access = PL1_RW, .accessfn = access_lor_other,
+              .type = ARM_CP_CONST, .resetvalue = 0 },
+            { .name = "LOREA_EL1", .state = ARM_CP_STATE_AA64,
+              .opc0 = 3, .opc1 = 0, .crn = 10, .crm = 4, .opc2 = 1,
+              .access = PL1_RW, .accessfn = access_lor_other,
+              .type = ARM_CP_CONST, .resetvalue = 0 },
+            { .name = "LORN_EL1", .state = ARM_CP_STATE_AA64,
+              .opc0 = 3, .opc1 = 0, .crn = 10, .crm = 4, .opc2 = 2,
+              .access = PL1_RW, .accessfn = access_lor_other,
+              .type = ARM_CP_CONST, .resetvalue = 0 },
+            { .name = "LORC_EL1", .state = ARM_CP_STATE_AA64,
+              .opc0 = 3, .opc1 = 0, .crn = 10, .crm = 4, .opc2 = 3,
+              .access = PL1_RW, .accessfn = access_lor_other,
+              .type = ARM_CP_CONST, .resetvalue = 0 },
+            { .name = "LORID_EL1", .state = ARM_CP_STATE_AA64,
+              .opc0 = 3, .opc1 = 0, .crn = 10, .crm = 4, .opc2 = 7,
+              .access = PL1_R, .accessfn = access_lorid,
+              .type = ARM_CP_CONST, .resetvalue = 0 },
+            REGINFO_SENTINEL
+        };
+        define_arm_cp_regs(cpu, lor_reginfo);
+    }
+
     if (cpu_isar_feature(aa64_sve, cpu)) {
         define_one_arm_cp_reg(cpu, &zcr_el1_reginfo);
         if (arm_feature(env, ARM_FEATURE_EL2)) {
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index fd36425f1a..5952a9d1cc 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -2290,6 +2290,12 @@ static void disas_ldst_excl(DisasContext *s, uint32_t insn)
         }
         return;
 
+    case 0x8: /* STLLR */
+        if (!dc_isar_feature(aa64_lor, s)) {
+            break;
+        }
+        /* StoreLORelease is the same as Store-Release for QEMU.  */
+        /* fallthru */
     case 0x9: /* STLR */
         /* Generate ISS for non-exclusive accesses including LASR.  */
         if (rn == 31) {
@@ -2301,6 +2307,12 @@ static void disas_ldst_excl(DisasContext *s, uint32_t insn)
                   disas_ldst_compute_iss_sf(size, false, 0), is_lasr);
         return;
 
+    case 0xc: /* LDLAR */
+        if (!dc_isar_feature(aa64_lor, s)) {
+            break;
+        }
+        /* LoadLOAcquire is the same as Load-Acquire for QEMU.  */
+        /* fallthru */
     case 0xd: /* LDAR */
         /* Generate ISS for non-exclusive accesses including LASR.  */
         if (rn == 31) {
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 08/10] target/arm: Implement the ARMv8.1-LOR extension

[Peter Maydell]

On Mon, 3 Dec 2018 at 20:38, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Provide a trivial implementation with zero limited ordering regions,
> which causes the LDLAR and STLLR instructions to devolve into the
> LDAR and STLR instructions from the base ARMv8.0 instruction set.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>
> ---
> v2: Mark LORID_EL1 read-only.
>     Add TLOR access checks.
>     Conditionally unmask TLOR in hcr/scr_write.
> ---
>  target/arm/cpu.h           |  5 +++
>  target/arm/cpu64.c         |  4 ++
>  target/arm/helper.c        | 91 ++++++++++++++++++++++++++++++++++++++
>  target/arm/translate-a64.c | 12 +++++
>  4 files changed, 112 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index a84101efa9..ba0c368292 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3285,6 +3285,11 @@ static inline bool isar_feature_aa64_atomics(const ARMISARegisters *id)
>      return FIELD_EX64(id->id_aa64isar0, ID_AA64ISAR0, ATOMIC) != 0;
>  }
>
> +static inline bool isar_feature_aa64_lor(const ARMISARegisters *id)
> +{
> +    return FIELD_EX64(id->id_aa64mmfr0, ID_AA64MMFR1, LO) != 0;

You're testing the wrong ID register here...

> +}
> +

> +static CPAccessResult access_lor_other(CPUARMState *env,
> +                                       const ARMCPRegInfo *ri, bool isread)
> +{
> +    int el = arm_current_el(env);
> +
> +    if (arm_is_secure_below_el3(env)) {
> +        /* Access denied in secure mode.  */
> +        return CP_ACCESS_TRAP;
> +    }
> +    if (el < 2 && arm_el_is_aa64(env, 2)) {

You don't need to explicitly check if EL2 is AArch64 --
these registers are AArch64 only so if we accessed them
from a lower EL then that EL is AArch64 and so all the
ELs above it must be too.

> +        uint64_t hcr = arm_hcr_el2_eff(env);
> +        if (hcr & HCR_E2H) {
> +            hcr &= HCR_TLOR;
> +        } else {
> +            hcr &= HCR_TGE | HCR_TLOR;

This doesn't make sense to me: I think TGE can't be 1 here.
We know (.access flag) that we're not in EL0. We know from
the el < 2 that we're in EL1, and we've already ruled out
Secure EL1. So this is NS EL1. If E2H == 0 then TGE can't
be set, because E2H,TGE={0,1} means the CPU can never be
in NS EL1.

(these remarks apply also to the access_lorid function)

> +        }
> +        if (hcr == HCR_TLOR) {
> +            return CP_ACCESS_TRAP_EL2;
> +        }
> +    }
> +    if (el < 3 && (env->cp15.scr_el3 & SCR_TLOR)) {
> +        return CP_ACCESS_TRAP_EL3;
> +    }
> +    return CP_ACCESS_OK;
> +}
> +
>  void register_cp_regs_for_features(ARMCPU *cpu)
>  {
>      /* Register all the coprocessor registers based on feature bits */
> @@ -5741,6 +5800,38 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>          define_one_arm_cp_reg(cpu, &sctlr);
>      }
>
> +    if (cpu_isar_feature(aa64_lor, cpu)) {
> +        /*
> +         * A trivial implementation of ARMv8.1-LOR leaves all of these
> +         * registers fixed at 0, which indicates that there are zero
> +         * supported Limited Ordering regions.
> +         */
> +        static const ARMCPRegInfo lor_reginfo[] = {
> +            { .name = "LORSA_EL1", .state = ARM_CP_STATE_AA64,
> +              .opc0 = 3, .opc1 = 0, .crn = 10, .crm = 4, .opc2 = 0,
> +              .access = PL1_RW, .accessfn = access_lor_other,
> +              .type = ARM_CP_CONST, .resetvalue = 0 },
> +            { .name = "LOREA_EL1", .state = ARM_CP_STATE_AA64,
> +              .opc0 = 3, .opc1 = 0, .crn = 10, .crm = 4, .opc2 = 1,
> +              .access = PL1_RW, .accessfn = access_lor_other,
> +              .type = ARM_CP_CONST, .resetvalue = 0 },
> +            { .name = "LORN_EL1", .state = ARM_CP_STATE_AA64,
> +              .opc0 = 3, .opc1 = 0, .crn = 10, .crm = 4, .opc2 = 2,
> +              .access = PL1_RW, .accessfn = access_lor_other,
> +              .type = ARM_CP_CONST, .resetvalue = 0 },
> +            { .name = "LORC_EL1", .state = ARM_CP_STATE_AA64,
> +              .opc0 = 3, .opc1 = 0, .crn = 10, .crm = 4, .opc2 = 3,
> +              .access = PL1_RW, .accessfn = access_lor_other,
> +              .type = ARM_CP_CONST, .resetvalue = 0 },
> +            { .name = "LORID_EL1", .state = ARM_CP_STATE_AA64,
> +              .opc0 = 3, .opc1 = 0, .crn = 10, .crm = 4, .opc2 = 7,
> +              .access = PL1_R, .accessfn = access_lorid,
> +              .type = ARM_CP_CONST, .resetvalue = 0 },
> +            REGINFO_SENTINEL
> +        };
> +        define_arm_cp_regs(cpu, lor_reginfo);
> +    }
> +
>      if (cpu_isar_feature(aa64_sve, cpu)) {
>          define_one_arm_cp_reg(cpu, &zcr_el1_reginfo);
>          if (arm_feature(env, ARM_FEATURE_EL2)) {
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index fd36425f1a..5952a9d1cc 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -2290,6 +2290,12 @@ static void disas_ldst_excl(DisasContext *s, uint32_t insn)
>          }
>          return;
>
> +    case 0x8: /* STLLR */
> +        if (!dc_isar_feature(aa64_lor, s)) {
> +            break;
> +        }
> +        /* StoreLORelease is the same as Store-Release for QEMU.  */
> +        /* fallthru */

We are as usual a bit inconsistent, but we seem to use the
spelling "fall through" for this linter-hint more often than
any of the other variations.

>      case 0x9: /* STLR */
>          /* Generate ISS for non-exclusive accesses including LASR.  */
>          if (rn == 31) {
> @@ -2301,6 +2307,12 @@ static void disas_ldst_excl(DisasContext *s, uint32_t insn)
>                    disas_ldst_compute_iss_sf(size, false, 0), is_lasr);
>          return;
>
> +    case 0xc: /* LDLAR */
> +        if (!dc_isar_feature(aa64_lor, s)) {
> +            break;
> +        }
> +        /* LoadLOAcquire is the same as Load-Acquire for QEMU.  */
> +        /* fallthru */
>      case 0xd: /* LDAR */
>          /* Generate ISS for non-exclusive accesses including LASR.  */
>          if (rn == 31) {

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 08/10] target/arm: Implement the ARMv8.1-LOR extension

[Richard Henderson]

On 12/6/18 7:49 AM, Peter Maydell wrote:
>> +static inline bool isar_feature_aa64_lor(const ARMISARegisters *id)
>> +{
>> +    return FIELD_EX64(id->id_aa64mmfr0, ID_AA64MMFR1, LO) != 0;
> 
> You're testing the wrong ID register here...

Oops, I thought I fixed that...

>> +static CPAccessResult access_lor_other(CPUARMState *env,
>> +                                       const ARMCPRegInfo *ri, bool isread)
>> +{
>> +    int el = arm_current_el(env);
>> +
>> +    if (arm_is_secure_below_el3(env)) {
>> +        /* Access denied in secure mode.  */
>> +        return CP_ACCESS_TRAP;
>> +    }
>> +    if (el < 2 && arm_el_is_aa64(env, 2)) {
> 
> You don't need to explicitly check if EL2 is AArch64 --
> these registers are AArch64 only so if we accessed them
> from a lower EL then that EL is AArch64 and so all the
> ELs above it must be too.

Ok.

> 
>> +        uint64_t hcr = arm_hcr_el2_eff(env);
>> +        if (hcr & HCR_E2H) {
>> +            hcr &= HCR_TLOR;
>> +        } else {
>> +            hcr &= HCR_TGE | HCR_TLOR;
> 
> This doesn't make sense to me: I think TGE can't be 1 here.
> We know (.access flag) that we're not in EL0. We know from
> the el < 2 that we're in EL1, and we've already ruled out
> Secure EL1. So this is NS EL1. If E2H == 0 then TGE can't
> be set, because E2H,TGE={0,1} means the CPU can never be
> in NS EL1.

Ok.  Perhaps I was too blindly following the access cases listed in the ARM and
not applying any extra thought.

> (these remarks apply also to the access_lorid function)

I think I will split out a shared subroutine for these.

>> +    case 0x8: /* STLLR */
>> +        if (!dc_isar_feature(aa64_lor, s)) {
>> +            break;
>> +        }
>> +        /* StoreLORelease is the same as Store-Release for QEMU.  */
>> +        /* fallthru */
> 
> We are as usual a bit inconsistent, but we seem to use the
> spelling "fall through" for this linter-hint more often than
> any of the other variations.

I was going to say something about old habits, but I do note that I've somehow
migrated from the witheringly old FALLTHRU.  So perhaps I can learn eventually...


r~

Re: [Qemu-devel] [PATCH v2 08/10] target/arm: Implement the ARMv8.1-LOR extension

[Richard Henderson]

On 12/6/18 7:49 AM, Peter Maydell wrote:
>> +        uint64_t hcr = arm_hcr_el2_eff(env);
>> +        if (hcr & HCR_E2H) {
>> +            hcr &= HCR_TLOR;
>> +        } else {
>> +            hcr &= HCR_TGE | HCR_TLOR;
> This doesn't make sense to me

The logic is backward.  What I was after was

  if (hcr & HCR_E2H) {
      hcr &= HCR_TGE | HCR_TLOR;
  } else {
      hcr &= HCR_TLOR;
  }
  if (hcr == HCR_TLOR) {
      trap to el2.
  }

I.e. swap the then and else condition.  This takes care of the two rules

 -- If (SCR_EL3.NS == 1 || SCR_EL3.EEL2 == 1) && IsUsingAArch64(EL2)
    && HCR_EL2.E2H == 0 && HCR_EL2.TLOR == 1, then
    accesses at EL1 are trapped to EL2.
 -- If (SCR_EL3.NS == 1 || SCR_EL3.EEL2 == 1) && IsUsingAArch64(EL2)
    && HCR_EL2.E2H == 1 && HCR_EL2.TGE == 0 && HCR_EL2.TLOR == 1,
    then accesses at EL1 are trapped to EL2.


r~

[Qemu-devel] [PATCH v2 09/10] target/arm: Implement the ARMv8.1-HPD extension

[Richard Henderson]

Since the TCR_*.HPD bits were RES0 in ARMv8.0, we can simply
interpret the bits as if ARMv8.1-HPD is present without checking.
We will need a slightly different check for hpd for aarch32.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu64.c  |  1 +
 target/arm/helper.c | 27 ++++++++++++++++++++-------
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index aac6283018..1d57be0c91 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -325,6 +325,7 @@ static void aarch64_max_initfn(Object *obj)
         cpu->isar.id_aa64pfr0 = t;
 
         t = cpu->isar.id_aa64mmfr1;
+        t = FIELD_DP64(t, ID_AA64MMFR1, HPDS, 1); /* HPD */
         t = FIELD_DP64(t, ID_AA64MMFR1, LO, 1);
         cpu->isar.id_aa64mmfr1 = t;
 
diff --git a/target/arm/helper.c b/target/arm/helper.c
index a0ee1fbc5a..9bb3e364d4 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9776,6 +9776,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
     bool ttbr1_valid = true;
     uint64_t descaddrmask;
     bool aarch64 = arm_el_is_aa64(env, el);
+    bool hpd = false;
 
     /* TODO:
      * This code does not handle the different format TCR for VTCR_EL2.
@@ -9890,6 +9891,13 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         if (tg == 2) { /* 16KB pages */
             stride = 11;
         }
+        if (aarch64) {
+            if (el > 1) {
+                hpd = extract64(tcr->raw_tcr, 24, 1);
+            } else {
+                hpd = extract64(tcr->raw_tcr, 41, 1);
+            }
+        }
     } else {
         /* We should only be here if TTBR1 is valid */
         assert(ttbr1_valid);
@@ -9905,6 +9913,9 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         if (tg == 1) { /* 16KB pages */
             stride = 11;
         }
+        if (aarch64) {
+            hpd = extract64(tcr->raw_tcr, 42, 1);
+        }
     }
 
     /* Here we should have set up all the parameters for the translation:
@@ -9998,7 +10009,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         descaddr = descriptor & descaddrmask;
 
         if ((descriptor & 2) && (level < 3)) {
-            /* Table entry. The top five bits are attributes which  may
+            /* Table entry. The top five bits are attributes which may
              * propagate down through lower levels of the table (and
              * which are all arranged so that 0 means "no effect", so
              * we can gather them up by ORing in the bits at each level).
@@ -10023,15 +10034,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
             break;
         }
         /* Merge in attributes from table descriptors */
-        attrs |= extract32(tableattrs, 0, 2) << 11; /* XN, PXN */
-        attrs |= extract32(tableattrs, 3, 1) << 5; /* APTable[1] => AP[2] */
+        attrs |= nstable << 3; /* NS */
+        if (hpd) {
+            /* HPD disables all the table attributes except NSTable.  */
+            break;
+        }
+        attrs |= extract32(tableattrs, 0, 2) << 11;     /* XN, PXN */
         /* The sense of AP[1] vs APTable[0] is reversed, as APTable[0] == 1
          * means "force PL1 access only", which means forcing AP[1] to 0.
          */
-        if (extract32(tableattrs, 2, 1)) {
-            attrs &= ~(1 << 4);
-        }
-        attrs |= nstable << 3; /* NS */
+        attrs &= ~(extract32(tableattrs, 2, 1) << 4);   /* !APT[0] => AP[1] */
+        attrs |= extract32(tableattrs, 3, 1) << 5;      /* APT[1] => AP[2] */
         break;
     }
     /* Here descaddr is the final physical address, and attributes
-- 
2.17.2

[Qemu-devel] [PATCH v2 10/10] target/arm: Implement the ARMv8.2-AA32HPD extension

[Richard Henderson]

The bulk of the work here, beyond base HPD, is defining the
TTBCR2 register.  In addition we must check TTBCR.T2E, which
is not present (RES0) for AArch64.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h    |  9 +++++++++
 target/arm/cpu.c    |  4 ++++
 target/arm/helper.c | 37 +++++++++++++++++++++++++++++--------
 3 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index ba0c368292..15daa2c050 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1548,6 +1548,15 @@ FIELD(ID_ISAR6, FHM, 8, 4)
 FIELD(ID_ISAR6, SB, 12, 4)
 FIELD(ID_ISAR6, SPECRES, 16, 4)
 
+FIELD(ID_MMFR4, SPECSEI, 0, 4)
+FIELD(ID_MMFR4, AC2, 4, 4)
+FIELD(ID_MMFR4, XNX, 8, 4)
+FIELD(ID_MMFR4, CNP, 12, 4)
+FIELD(ID_MMFR4, HPDS, 16, 4)
+FIELD(ID_MMFR4, LSM, 20, 4)
+FIELD(ID_MMFR4, CCIDX, 24, 4)
+FIELD(ID_MMFR4, EVT, 28, 4)
+
 FIELD(ID_AA64ISAR0, AES, 4, 4)
 FIELD(ID_AA64ISAR0, SHA1, 8, 4)
 FIELD(ID_AA64ISAR0, SHA2, 12, 4)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 60411f6bfe..0b185f8d30 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1932,6 +1932,10 @@ static void arm_max_initfn(Object *obj)
             t = cpu->isar.id_isar6;
             t = FIELD_DP32(t, ID_ISAR6, DP, 1);
             cpu->isar.id_isar6 = t;
+
+            t = cpu->id_mmfr4;
+            t = FIELD_DP32(t, ID_MMFR4, HPDS, 1); /* AA32HPD */
+            cpu->id_mmfr4 = t;
         }
 #endif
     }
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 9bb3e364d4..5df7a9e637 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2733,6 +2733,7 @@ static void vmsa_ttbcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                              uint64_t value)
 {
     ARMCPU *cpu = arm_env_get_cpu(env);
+    TCR *tcr = raw_ptr(env, ri);
 
     if (arm_feature(env, ARM_FEATURE_LPAE)) {
         /* With LPAE the TTBCR could result in a change of ASID
@@ -2740,6 +2741,8 @@ static void vmsa_ttbcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
          */
         tlb_flush(CPU(cpu));
     }
+    /* Preserve the high half of TCR_EL1, set via TTBCR2.  */
+    value = deposit64(tcr->raw_tcr, 0, 32, value);
     vmsa_ttbcr_raw_write(env, ri, value);
 }
 
@@ -2842,6 +2845,16 @@ static const ARMCPRegInfo vmsa_cp_reginfo[] = {
     REGINFO_SENTINEL
 };
 
+/* Note that unlike TTBCR, writing to TTBCR2 does not require flushing
+ * qemu tlbs nor adjusting cached masks.
+ */
+static const ARMCPRegInfo ttbcr2_reginfo = {
+    .name = "TTBCR2", .cp = 15, .opc1 = 0, .crn = 2, .crm = 0, .opc2 = 3,
+    .access = PL1_RW, .type = ARM_CP_ALIAS,
+    .bank_fieldoffsets = { offsetofhigh32(CPUARMState, cp15.tcr_el[3]),
+                           offsetofhigh32(CPUARMState, cp15.tcr_el[1]) },
+};
+
 static void omap_ticonfig_write(CPUARMState *env, const ARMCPRegInfo *ri,
                                 uint64_t value)
 {
@@ -5540,6 +5553,10 @@ void register_cp_regs_for_features(ARMCPU *cpu)
     } else {
         define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo);
         define_arm_cp_regs(cpu, vmsa_cp_reginfo);
+        /* TTCBR2 is introduced with ARMv8.2-A32HPD.  */
+        if (FIELD_EX32(cpu->id_mmfr4, ID_MMFR4, HPDS) != 0) {
+            define_one_arm_cp_reg(cpu, &ttbcr2_reginfo);
+        }
     }
     if (arm_feature(env, ARM_FEATURE_THUMB2EE)) {
         define_arm_cp_regs(cpu, t2ee_cp_reginfo);
@@ -9891,12 +9908,14 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         if (tg == 2) { /* 16KB pages */
             stride = 11;
         }
-        if (aarch64) {
-            if (el > 1) {
-                hpd = extract64(tcr->raw_tcr, 24, 1);
-            } else {
-                hpd = extract64(tcr->raw_tcr, 41, 1);
-            }
+        if (aarch64 && el > 1) {
+            hpd = extract64(tcr->raw_tcr, 24, 1);
+        } else {
+            hpd = extract64(tcr->raw_tcr, 41, 1);
+        }
+        if (!aarch64) {
+            /* For aarch32, hpd0 is not enabled without t2e as well.  */
+            hpd &= extract64(tcr->raw_tcr, 6, 1);
         }
     } else {
         /* We should only be here if TTBR1 is valid */
@@ -9913,8 +9932,10 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         if (tg == 1) { /* 16KB pages */
             stride = 11;
         }
-        if (aarch64) {
-            hpd = extract64(tcr->raw_tcr, 42, 1);
+        hpd = extract64(tcr->raw_tcr, 42, 1);
+        if (!aarch64) {
+            /* For aarch32, hpd1 is not enabled without t2e as well.  */
+            hpd &= extract64(tcr->raw_tcr, 6, 1);
         }
     }
 
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 00/10] target/arm: LOR, HPD, AA32HPD

[Peter Maydell]

On Mon, 3 Dec 2018 at 20:38, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Three relatively simple post-8.0 extensions.
>
> Changes since v1:
>   * Add TLOR access checks for LOR registers.
>   * Clean up access to HCR_EL2.
>   * Clean up setting of SCR_EL3.
>   * Other changes as noted within each patch.

I've taken patches 1,2,3,4,7,9,10 into target-arm.next:

>   target/arm: Move id_aa64mmfr* to ARMISARegisters
>   target/arm: Add HCR_EL2 bits up to ARMv8.5
>   target/arm: Add SCR_EL3 bits up to ARMv8.5
>   target/arm: Fix HCR_EL2.TGE check in arm_phys_excp_target_el
>   target/arm: Tidy scr_write
>   target/arm: Implement the ARMv8.1-HPD extension
>   target/arm: Implement the ARMv8.2-AA32HPD extension

leaving 5,6,8 as needing rework:
>   target/arm: Introduce arm_hcr_el2_eff
>   target/arm: Use arm_hcr_el2_eff more places
>   target/arm: Implement the ARMv8.1-LOR extension

thanks
-- PMM