[Qemu-devel] [PATCH for-4.0 0/4] target/arm: LOR, HPD, AA32HPD extensions

[Qemu-devel] [PATCH for-4.0 0/4] target/arm: LOR, HPD, AA32HPD extensions

[Richard Henderson]

Three relatively simple post-8.0 extensions.


r~


Richard Henderson (4):
  target/arm: Move id_aa64mmfr* to ARMISARegisters
  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           | 35 ++++++++++++++++-
 target/arm/internals.h     |  3 +-
 target/arm/cpu.c           |  4 ++
 target/arm/cpu64.c         | 11 ++++--
 target/arm/helper.c        | 80 +++++++++++++++++++++++++++++++++-----
 target/arm/translate-a64.c | 12 ++++++
 6 files changed, 129 insertions(+), 16 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH for-4.0 1/4] 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>
---
 target/arm/cpu.h       | 22 ++++++++++++++++++++--
 target/arm/internals.h |  3 ++-
 target/arm/cpu64.c     |  6 +++---
 target/arm/helper.c    |  4 ++--
 4 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 8e6779936e..2ce5e80dfc 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -815,6 +815,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;
@@ -836,8 +838,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 */
@@ -1554,6 +1554,24 @@ 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_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 6c2bb2deeb..bf844abc47 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -213,7 +213,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 0ea95b0815..70376764cb 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5228,11 +5228,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,
-- 
2.17.2

Re: [Qemu-devel] [Qemu-arm] [PATCH for-4.0 1/4] target/arm: Move id_aa64mmfr* to ARMISARegisters

[Peter Maydell]

On 2 November 2018 at 13:41, 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>

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

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH for-4.0 1/4] target/arm: Move id_aa64mmfr* to ARMISARegisters

[Peter Maydell]

On 15 November 2018 at 16:49, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 November 2018 at 13:41, 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>
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

...though since you'll need a v2 anyway, maybe add the v8.5
fields in ID_AA64MMFR0 ?

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)

thanks
-- PMM

[Qemu-devel] [PATCH for-4.0 2/4] 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>
---
 target/arm/cpu.h           |  5 +++++
 target/arm/cpu64.c         |  4 ++++
 target/arm/helper.c        | 26 ++++++++++++++++++++++++++
 target/arm/translate-a64.c | 12 ++++++++++++
 4 files changed, 47 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 2ce5e80dfc..f12a6afddc 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3278,6 +3278,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 70376764cb..758ddac5e9 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5714,6 +5714,32 @@ 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, .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, .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, .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, .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_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+        };
+        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 88195ab949..2307a18d5a 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 for-4.0 2/4] target/arm: Implement the ARMv8.1-LOR extension

[Richard Henderson]

On 11/2/18 1:41 PM, Richard Henderson wrote:
> +            { .name = "LORID_EL1", .state = ARM_CP_STATE_AA64,
> +              .opc0 = 3, .opc1 = 0, .crn = 10, .crm = 4, .opc2 = 7,
> +              .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },

For v2 this will be correctly marked RO.


r~

Re: [Qemu-devel] [Qemu-arm] [PATCH for-4.0 2/4] target/arm: Implement the ARMv8.1-LOR extension

[Peter Maydell]

On 2 November 2018 at 13:41, 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>
> ---
>  target/arm/cpu.h           |  5 +++++
>  target/arm/cpu64.c         |  4 ++++
>  target/arm/helper.c        | 26 ++++++++++++++++++++++++++
>  target/arm/translate-a64.c | 12 ++++++++++++
>  4 files changed, 47 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 2ce5e80dfc..f12a6afddc 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3278,6 +3278,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 70376764cb..758ddac5e9 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5714,6 +5714,32 @@ 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, .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, .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, .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, .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_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> +        };

There are access checks needed for these registers:
HCR_EL2.TLOR controls trapping to EL2
SCR_EL3.TLOR controls trapping to EL3.
All except LORID_EL1 are also inaccessible from Secure state.

> +        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 88195ab949..2307a18d5a 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) {
> --

We should check the SBO bits in these encodings:
the instructions have "(1)"s in the fields for Rs and Rt2, which means
that the behaviour if not set is CONSTRAINED UNPREDICTABLE
(see DDI0487D.a C2.2.2). "Executes as if the value of the bit is 1"
is a permitted choice, so not checking the fields isn't wrong, but
it does make it a bit harder to tell when some future guest makes
use of some new encoding that borrows some of the space, so it's
nicer to take the "make the instruction UNDEFINED" option.
It looks like we missed this with some of the other encodings in
this space too.

thanks
-- PMM

[Qemu-devel] [PATCH for-4.0 3/4] 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.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu64.c  |  1 +
 target/arm/helper.c | 29 +++++++++++++++++++++--------
 2 files changed, 22 insertions(+), 8 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 758ddac5e9..312d3e6f02 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9682,6 +9682,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.
@@ -9796,6 +9797,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);
@@ -9811,6 +9819,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:
@@ -9904,7 +9915,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).
@@ -9928,14 +9939,16 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
             /* Stage 2 table descriptors do not include any attribute fields */
             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] */
-        /* 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.
+        /*
+         * Merge in attributes from table descriptors, if the
+         * Hierarchical Permission Disable bit is not set.
          */
-        if (extract32(tableattrs, 2, 1)) {
-            attrs &= ~(1 << 4);
+        if (!hpd) {
+            attrs |= extract32(tableattrs, 0, 2) << 11; /* XN, PXN */
+            /* !APTable[0] => AP[1].  */
+            attrs &= ~(extract32(tableattrs, 2, 1) << 4);
+            /* APTable[1] => AP[2] */
+            attrs |= extract32(tableattrs, 3, 1) << 5;
         }
         attrs |= nstable << 3; /* NS */
         break;
-- 
2.17.2

Re: [Qemu-devel] [PATCH for-4.0 3/4] target/arm: Implement the ARMv8.1-HPD extension

[Peter Maydell]

On 2 November 2018 at 13:41, Richard Henderson
<richard.henderson@linaro.org> wrote:
> 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.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

Another way to write the tail end of the loop would be
to move the nstable handling up, so:

        if (mmu_idx == ARMMMUIdx_S2NS) {
            /* Stage 2 table descriptors do not include any attribute fields */
            break;
        }
        /* Merge in attributes from table descriptors */
        attrs |= nstable << 3; /* NS */
        if (!hpd) {
            /* HPD disables all the table attributes except NSTable */
            break;
        }
        attrs |= extract32(tableattrs, 0, 2) << 11; /* XN, PXN */
        attrs |= extract32(tableattrs, 3, 1) << 5; /* APTable[1] => AP[2] */
        /* 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 */
        break;

which would then make the hpd handling use the same
approach we already have for "don't merge attributes if
this is a stage 2 table". But don't bother changing your
patch unless you think that's actually better.

thanks
-- PMM

[Qemu-devel] [PATCH for-4.0 4/4] 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.

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

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index f12a6afddc..a253cdebde 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1517,6 +1517,14 @@ 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_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 8f16e96b6c..3fd85f21c5 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1856,6 +1856,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 312d3e6f02..85d3f4ad89 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2722,6 +2722,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
@@ -2729,6 +2730,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);
 }
 
@@ -2831,6 +2834,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, .crn = 2, .crm = 0, .opc1 = 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)
 {
@@ -5454,6 +5467,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);
@@ -9797,12 +9814,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 */
@@ -9819,8 +9838,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 for-4.0 4/4] target/arm: Implement the ARMv8.2-AA32HPD extension

[Peter Maydell]

On 2 November 2018 at 13:41, Richard Henderson
<richard.henderson@linaro.org> wrote:
> 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.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h    |  8 ++++++++
>  target/arm/cpu.c    |  4 ++++
>  target/arm/helper.c | 37 +++++++++++++++++++++++++++++--------
>  3 files changed, 41 insertions(+), 8 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index f12a6afddc..a253cdebde 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1517,6 +1517,14 @@ 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)

Maybe add the v8.5 field too?
FIELD(ID_MMFR4, EVT, 28, 4)

> +/* 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, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 3,

Our usual order for these fields is cp, opc1, crn, crm, opc2.

> +    .access = PL1_RW, .type = ARM_CP_ALIAS,
> +    .bank_fieldoffsets = { offsetofhigh32(CPUARMState, cp15.tcr_el[3]),
> +                           offsetofhigh32(CPUARMState, cp15.tcr_el[1]) },
> +};

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-4.0 4/4] target/arm: Implement the ARMv8.2-AA32HPD extension

[Peter Maydell]

On 2 November 2018 at 13:41, Richard Henderson
<richard.henderson@linaro.org> wrote:
> 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.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h    |  8 ++++++++
>  target/arm/cpu.c    |  4 ++++
>  target/arm/helper.c | 37 +++++++++++++++++++++++++++++--------
>  3 files changed, 41 insertions(+), 8 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index f12a6afddc..a253cdebde 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1517,6 +1517,14 @@ 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)

Why all caps for SpecSEI in this register, but honouring the
capitalization from the spec in the equivalent ID register
added in patch 1? ("FIELD(ID_AA64MMFR1, SpecSEI, 24, 4)")
I don't mind which, but we should pick a convention and stick
to it.

thanks
-- PMM