[PATCH for-7.0 0/6] target/arm: Implement LVA, LPA, LPA2 features

[PATCH for-7.0 0/6] target/arm: Implement LVA, LPA, LPA2 features

[Richard Henderson]

These features are all related and relatively small.

Testing so far has been limited to booting a kernel
with 64k pages and VA and PA set to 52 bits, which
excercises LVA and LPA.

There is not yet upstream support for LPA2, probably
because it's an ARMv8.7 addition.


r~


Richard Henderson (6):
  target/arm: Fault on invalid TCR_ELx.TxSZ
  target/arm: Move arm_pamax out of line
  target/arm: Honor TCR_ELx.{I}PS
  target/arm: Implement FEAT_LVA
  target/arm: Implement FEAT_LPA
  target/arm: Implement FEAT_LPA2

 target/arm/cpu-param.h |   4 +-
 target/arm/cpu.h       |  17 ++++
 target/arm/internals.h |  22 +----
 target/arm/cpu64.c     |   5 +-
 target/arm/helper.c    | 211 ++++++++++++++++++++++++++++++++++-------
 5 files changed, 204 insertions(+), 55 deletions(-)

-- 
2.25.1

[PATCH 1/6] target/arm: Fault on invalid TCR_ELx.TxSZ

[Richard Henderson]

Without FEAT_LVA, the behaviour of programming an invalid value
is IMPLEMENTATION DEFINED.  With FEAT_LVA, programming an invalid
minimum value requires a Translation fault.

It is most self-consistent to choose to generate the fault always.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 9b317899a6..575723d62c 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -11129,7 +11129,7 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
 {
     uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
     bool epd, hpd, using16k, using64k;
-    int select, tsz, tbi, max_tsz;
+    int select, tsz, tbi;
 
     if (!regime_has_2_ranges(mmu_idx)) {
         select = 0;
@@ -11165,15 +11165,6 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
         }
     }
 
-    if (cpu_isar_feature(aa64_st, env_archcpu(env))) {
-        max_tsz = 48 - using64k;
-    } else {
-        max_tsz = 39;
-    }
-
-    tsz = MIN(tsz, max_tsz);
-    tsz = MAX(tsz, 16);  /* TODO: ARMv8.2-LVA  */
-
     /* Present TBI as a composite with TBID.  */
     tbi = aa64_va_parameter_tbi(tcr, mmu_idx);
     if (!data) {
@@ -11309,9 +11300,30 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
 
     /* TODO: This code does not support shareability levels. */
     if (aarch64) {
+        int min_tsz = 16, max_tsz = 39;  /* TODO: ARMv8.2-LVA  */
+
         param = aa64_va_parameters(env, address, mmu_idx,
                                    access_type != MMU_INST_FETCH);
         level = 0;
+
+        if (cpu_isar_feature(aa64_st, env_archcpu(env))) {
+            max_tsz = 48 - param.using64k;
+        }
+
+        /*
+         * If TxSZ is programmed to a value larger than the maximum,
+         * or smaller than the effective minimum, it is IMPLEMENTATION
+         * DEFINED whether we behave as if the field were programmed
+         * within bounds, or if a level 0 Translation fault is generated.
+         *
+         * With FEAT_LVA, fault on less than minimum becomes required,
+         * so our choice is to always raise the fault.
+         */
+        if (param.tsz < min_tsz || param.tsz > max_tsz) {
+            fault_type = ARMFault_Translation;
+            goto do_fault;
+        }
+
         addrsize = 64 - 8 * param.tbi;
         inputsize = 64 - param.tsz;
     } else {
-- 
2.25.1

Re: [PATCH 1/6] target/arm: Fault on invalid TCR_ELx.TxSZ

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Without FEAT_LVA, the behaviour of programming an invalid value
> is IMPLEMENTATION DEFINED.  With FEAT_LVA, programming an invalid
> minimum value requires a Translation fault.
>
> It is most self-consistent to choose to generate the fault always.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH 1/6] target/arm: Fault on invalid TCR_ELx.TxSZ

[Peter Maydell]

On Wed, 8 Dec 2021 at 23:16, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Without FEAT_LVA, the behaviour of programming an invalid value
> is IMPLEMENTATION DEFINED.  With FEAT_LVA, programming an invalid
> minimum value requires a Translation fault.
>
> It is most self-consistent to choose to generate the fault always.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/helper.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 9b317899a6..575723d62c 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -11129,7 +11129,7 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
>  {
>      uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
>      bool epd, hpd, using16k, using64k;
> -    int select, tsz, tbi, max_tsz;
> +    int select, tsz, tbi;
>
>      if (!regime_has_2_ranges(mmu_idx)) {
>          select = 0;
> @@ -11165,15 +11165,6 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
>          }
>      }
>
> -    if (cpu_isar_feature(aa64_st, env_archcpu(env))) {
> -        max_tsz = 48 - using64k;
> -    } else {
> -        max_tsz = 39;
> -    }
> -
> -    tsz = MIN(tsz, max_tsz);
> -    tsz = MAX(tsz, 16);  /* TODO: ARMv8.2-LVA  */
> -

These changes are OK in themselves, but we also use the
aa64_va_parameters() calculated tsz value in the
pointer-auth code to work out the bottom bit of the
pointer auth field:

    bot_bit = 64 - param.tsz;
    top_bit = 64 - 8 * param.tbi;

Without the clamping of param.tsz to the valid range,
the guest can now program it to a value that will cause
us to have bot_bit > top_bit (eg tsz = 0). We don't
guard against that and as a result code like
extract64(test, bot_bit, top_bit - bot_bit)
will assert on the bogus length value.

(Section D5.1.5 says what the pauth code is allowed to do
if the TnSZ field is out-of-limits: it can use the value as-is,
or it can clamp it to the limit.)

-- PMM

Re: [PATCH 1/6] target/arm: Fault on invalid TCR_ELx.TxSZ

[Peter Maydell]

On Thu, 6 Jan 2022 at 18:27, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 8 Dec 2021 at 23:16, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > Without FEAT_LVA, the behaviour of programming an invalid value
> > is IMPLEMENTATION DEFINED.  With FEAT_LVA, programming an invalid
> > minimum value requires a Translation fault.
> >
> > It is most self-consistent to choose to generate the fault always.


> > -    if (cpu_isar_feature(aa64_st, env_archcpu(env))) {
> > -        max_tsz = 48 - using64k;
> > -    } else {
> > -        max_tsz = 39;
> > -    }
> > -
> > -    tsz = MIN(tsz, max_tsz);
> > -    tsz = MAX(tsz, 16);  /* TODO: ARMv8.2-LVA  */
> > -
>
> These changes are OK in themselves, but we also use the
> aa64_va_parameters() calculated tsz value in the
> pointer-auth code to work out the bottom bit of the
> pointer auth field:
>
>     bot_bit = 64 - param.tsz;
>     top_bit = 64 - 8 * param.tbi;

...and in particular, for linux-user mode as far as I can
tell we aren't initializing TCR_EL1 to anything particularly
sensible (we set TBI0 and leave the rest to 0) so we are
effectively relying on the clamping there at the moment.
We should probably set TCR_EL1 properly. (cf the user
report in qemu-discuss just now.)

-- PMM

[PATCH 2/6] target/arm: Move arm_pamax out of line

[Richard Henderson]

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

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 89f7610ebc..27d2fcd26c 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -243,24 +243,7 @@ static inline void update_spsel(CPUARMState *env, uint32_t imm)
  * Returns the implementation defined bit-width of physical addresses.
  * The ARMv8 reference manuals refer to this as PAMax().
  */
-static inline unsigned int arm_pamax(ARMCPU *cpu)
-{
-    static const unsigned int pamax_map[] = {
-        [0] = 32,
-        [1] = 36,
-        [2] = 40,
-        [3] = 42,
-        [4] = 44,
-        [5] = 48,
-    };
-    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.  */
-    assert(parange < ARRAY_SIZE(pamax_map));
-    return pamax_map[parange];
-}
+unsigned int arm_pamax(ARMCPU *cpu);
 
 /* Return true if extended addresses are enabled.
  * This is always the case if our translation regime is 64 bit,
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 575723d62c..fab9ee70d8 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -11090,6 +11090,28 @@ static uint8_t convert_stage2_attrs(CPUARMState *env, uint8_t s2attrs)
 }
 #endif /* !CONFIG_USER_ONLY */
 
+/* The cpu-specific constant value of PAMax; also used by hw/arm/virt. */
+unsigned int arm_pamax(ARMCPU *cpu)
+{
+    static const unsigned int pamax_map[] = {
+        [0] = 32,
+        [1] = 36,
+        [2] = 40,
+        [3] = 42,
+        [4] = 44,
+        [5] = 48,
+    };
+    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.
+     */
+    assert(parange < ARRAY_SIZE(pamax_map));
+    return pamax_map[parange];
+}
+
 static int aa64_va_parameter_tbi(uint64_t tcr, ARMMMUIdx mmu_idx)
 {
     if (regime_has_2_ranges(mmu_idx)) {
-- 
2.25.1

Re: [PATCH 2/6] target/arm: Move arm_pamax out of line

[Philippe Mathieu-Daudé]

On 12/9/21 00:11, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/internals.h | 19 +------------------
>  target/arm/helper.c    | 22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+), 18 deletions(-)

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

Re: [PATCH 2/6] target/arm: Move arm_pamax out of line

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

A little follow on comment in the commit message would be useful:

  We will shortly need to access PAMax outside of the internals of the translator.

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

Either way:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

[PATCH 3/6] target/arm: Honor TCR_ELx.{I}PS

[Richard Henderson]

This field controls the output (intermediate) physical address size
of the translation process.  V8 requires to raise an AddressSize
fault if the page tables are programmed incorrectly, such that any
intermediate descriptor address, or the final translated address,
is out of range.

Add an outputsize field to ARMVAParameters, and fill it in during
aa64_va_parameters.  Pass the value to check_s2_mmu_setup to use
instead of the raw PAMax value.  Test the descaddr as extracted
from TTBR and from page table entries.

Restrict descaddrmask so that we won't raise the fault for v7.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h |  1 +
 target/arm/helper.c    | 92 +++++++++++++++++++++++++++++-------------
 2 files changed, 65 insertions(+), 28 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 27d2fcd26c..3e801833b4 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1032,6 +1032,7 @@ static inline uint32_t aarch64_pstate_valid_mask(const ARMISARegisters *id)
  */
 typedef struct ARMVAParameters {
     unsigned tsz    : 8;
+    unsigned ps     : 3;
     unsigned select : 1;
     bool tbi        : 1;
     bool epd        : 1;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index fab9ee70d8..568914bd42 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -11003,7 +11003,7 @@ do_fault:
  * false otherwise.
  */
 static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
-                               int inputsize, int stride)
+                               int inputsize, int stride, int outputsize)
 {
     const int grainsize = stride + 3;
     int startsizecheck;
@@ -11019,22 +11019,19 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
     }
 
     if (is_aa64) {
-        CPUARMState *env = &cpu->env;
-        unsigned int pamax = arm_pamax(cpu);
-
         switch (stride) {
         case 13: /* 64KB Pages.  */
-            if (level == 0 || (level == 1 && pamax <= 42)) {
+            if (level == 0 || (level == 1 && outputsize <= 42)) {
                 return false;
             }
             break;
         case 11: /* 16KB Pages.  */
-            if (level == 0 || (level == 1 && pamax <= 40)) {
+            if (level == 0 || (level == 1 && outputsize <= 40)) {
                 return false;
             }
             break;
         case 9: /* 4KB Pages.  */
-            if (level == 0 && pamax <= 42) {
+            if (level == 0 && outputsize <= 42) {
                 return false;
             }
             break;
@@ -11043,8 +11040,8 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
         }
 
         /* Inputsize checks.  */
-        if (inputsize > pamax &&
-            (arm_el_is_aa64(env, 1) || inputsize > 40)) {
+        if (inputsize > outputsize &&
+            (arm_el_is_aa64(&cpu->env, 1) || inputsize > 40)) {
             /* This is CONSTRAINED UNPREDICTABLE and we choose to fault.  */
             return false;
         }
@@ -11090,17 +11087,19 @@ static uint8_t convert_stage2_attrs(CPUARMState *env, uint8_t s2attrs)
 }
 #endif /* !CONFIG_USER_ONLY */
 
+/* This mapping is common between ID_AA64MMFR0.PARANGE and TCR_ELx.{I}PS. */
+static const uint8_t pamax_map[] = {
+    [0] = 32,
+    [1] = 36,
+    [2] = 40,
+    [3] = 42,
+    [4] = 44,
+    [5] = 48,
+};
+
 /* The cpu-specific constant value of PAMax; also used by hw/arm/virt. */
 unsigned int arm_pamax(ARMCPU *cpu)
 {
-    static const unsigned int pamax_map[] = {
-        [0] = 32,
-        [1] = 36,
-        [2] = 40,
-        [3] = 42,
-        [4] = 44,
-        [5] = 48,
-    };
     unsigned int parange =
         FIELD_EX64(cpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
 
@@ -11151,7 +11150,7 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
 {
     uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
     bool epd, hpd, using16k, using64k;
-    int select, tsz, tbi;
+    int select, tsz, tbi, ps;
 
     if (!regime_has_2_ranges(mmu_idx)) {
         select = 0;
@@ -11165,6 +11164,7 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
             hpd = extract32(tcr, 24, 1);
         }
         epd = false;
+        ps = extract64(tcr, 16, 3);
     } else {
         /*
          * Bit 55 is always between the two regions, and is canonical for
@@ -11185,6 +11185,7 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
             epd = extract32(tcr, 23, 1);
             hpd = extract64(tcr, 42, 1);
         }
+        ps = extract64(tcr, 32, 3);
     }
 
     /* Present TBI as a composite with TBID.  */
@@ -11196,6 +11197,7 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
 
     return (ARMVAParameters) {
         .tsz = tsz,
+        .ps = ps,
         .select = select,
         .tbi = tbi,
         .epd = epd,
@@ -11312,7 +11314,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
     target_ulong page_size;
     uint32_t attrs;
     int32_t stride;
-    int addrsize, inputsize;
+    int addrsize, inputsize, outputsize;
     TCR *tcr = regime_tcr(env, mmu_idx);
     int ap, ns, xn, pxn;
     uint32_t el = regime_el(env, mmu_idx);
@@ -11323,6 +11325,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
     /* TODO: This code does not support shareability levels. */
     if (aarch64) {
         int min_tsz = 16, max_tsz = 39;  /* TODO: ARMv8.2-LVA  */
+        int parange;
 
         param = aa64_va_parameters(env, address, mmu_idx,
                                    access_type != MMU_INST_FETCH);
@@ -11348,11 +11351,22 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
 
         addrsize = 64 - 8 * param.tbi;
         inputsize = 64 - param.tsz;
+
+        /*
+         * Bound PS by PARANGE to find the effective output address size.
+         * ID_AA64MMFR0 is a read-only register so values outside of the
+         * supported mappings can be considered an implementation error.
+         */
+        parange = FIELD_EX64(cpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
+        parange = MIN(parange, param.ps);
+        assert(parange < ARRAY_SIZE(pamax_map));
+        outputsize = pamax_map[parange];
     } else {
         param = aa32_va_parameters(env, address, mmu_idx);
         level = 1;
         addrsize = (mmu_idx == ARMMMUIdx_Stage2 ? 40 : 32);
         inputsize = addrsize - param.tsz;
+        outputsize = 40;
     }
 
     /*
@@ -11437,7 +11451,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
 
         /* Check that the starting level is valid. */
         ok = check_s2_mmu_setup(cpu, aarch64, startlevel,
-                                inputsize, stride);
+                                inputsize, stride, outputsize);
         if (!ok) {
             fault_type = ARMFault_Translation;
             goto do_fault;
@@ -11445,24 +11459,41 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
         level = startlevel;
     }
 
-    indexmask_grainsize = (1ULL << (stride + 3)) - 1;
-    indexmask = (1ULL << (inputsize - (stride * (4 - level)))) - 1;
+    indexmask_grainsize = MAKE_64BIT_MASK(0, stride + 3);
+    indexmask = MAKE_64BIT_MASK(0, inputsize - (stride * (4 - level)));
 
     /* Now we can extract the actual base address from the TTBR */
     descaddr = extract64(ttbr, 0, 48);
+
+    /*
+     * If the base address is out of range, raise AddressSizeFault.
+     * In the pseudocode, this is !IsZero(baseregister<47:outputsize>),
+     * but we've just cleared the bits above 47, so simplify the test.
+     */
+    if (descaddr >> outputsize) {
+        level = 0;
+        fault_type = ARMFault_AddressSize;
+        goto do_fault;
+    }
+
     /*
      * We rely on this masking to clear the RES0 bits at the bottom of the TTBR
      * and also to mask out CnP (bit 0) which could validly be non-zero.
      */
     descaddr &= ~indexmask;
 
-    /* The address field in the descriptor goes up to bit 39 for ARMv7
-     * but up to bit 47 for ARMv8, but we use the descaddrmask
-     * up to bit 39 for AArch32, because we don't need other bits in that case
-     * to construct next descriptor address (anyway they should be all zeroes).
+    /*
+     * The address field in the descriptor goes up to bit 39 for ARMv7
+     * but up to bit 47 for ARMv8.  In ARMv7, those middle bits are SBZP,
+     * but in ARMv8 they are checked for zero and an AddressSize fault
+     * is raised if they are not.
      */
-    descaddrmask = ((1ull << (aarch64 ? 48 : 40)) - 1) &
-                   ~indexmask_grainsize;
+    if (aarch64 || arm_feature(env, ARM_FEATURE_V8)) {
+        descaddrmask = MAKE_64BIT_MASK(0, 48);
+    } else {
+        descaddrmask = MAKE_64BIT_MASK(0, 40);
+    }
+    descaddrmask &= ~indexmask_grainsize;
 
     /* Secure accesses start with the page table in secure memory and
      * can be downgraded to non-secure at any step. Non-secure accesses
@@ -11487,7 +11518,12 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
             /* Invalid, or the Reserved level 3 encoding */
             goto do_fault;
         }
+
         descaddr = descriptor & descaddrmask;
+        if (descaddr >> outputsize) {
+            fault_type = ARMFault_AddressSize;
+            goto do_fault;
+        }
 
         if ((descriptor & 2) && (level < 3)) {
             /* Table entry. The top five bits are attributes which may
-- 
2.25.1

Re: [PATCH 3/6] target/arm: Honor TCR_ELx.{I}PS

[Philippe Mathieu-Daudé]

Hi Richard,

On 12/9/21 00:11, Richard Henderson wrote:
> This field controls the output (intermediate) physical address size
> of the translation process.  V8 requires to raise an AddressSize
> fault if the page tables are programmed incorrectly, such that any
> intermediate descriptor address, or the final translated address,
> is out of range.
> 

I'd split this patch as:

> Add an outputsize field to ARMVAParameters,
^ 1

> and fill it in during
> aa64_va_parameters.
^2

> Pass the value to check_s2_mmu_setup to use
> instead of the raw PAMax value.
^1

> Test the descaddr as extracted
> from TTBR and from page table entries.
^2

> Restrict descaddrmask so that we won't raise the fault for v7.
^ could be in 1 (simpler) or 2 if you think it makes sense.

This way #1 is a preliminary refactor/cleanup,
and #2 is only the ps field and V8 addition.

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/internals.h |  1 +
>  target/arm/helper.c    | 92 +++++++++++++++++++++++++++++-------------
>  2 files changed, 65 insertions(+), 28 deletions(-)

Re: [PATCH 3/6] target/arm: Honor TCR_ELx.{I}PS

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> This field controls the output (intermediate) physical address size
> of the translation process.  V8 requires to raise an AddressSize
> fault if the page tables are programmed incorrectly, such that any
> intermediate descriptor address, or the final translated address,
> is out of range.
>
> Add an outputsize field to ARMVAParameters, and fill it in during
> aa64_va_parameters.  Pass the value to check_s2_mmu_setup to use
> instead of the raw PAMax value.  Test the descaddr as extracted
> from TTBR and from page table entries.
>
> Restrict descaddrmask so that we won't raise the fault for v7.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/internals.h |  1 +
>  target/arm/helper.c    | 92 +++++++++++++++++++++++++++++-------------
>  2 files changed, 65 insertions(+), 28 deletions(-)
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 27d2fcd26c..3e801833b4 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1032,6 +1032,7 @@ static inline uint32_t aarch64_pstate_valid_mask(const ARMISARegisters *id)
>   */
>  typedef struct ARMVAParameters {
>      unsigned tsz    : 8;
> +    unsigned ps     : 3;
>      unsigned select : 1;
>      bool tbi        : 1;
>      bool epd        : 1;
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index fab9ee70d8..568914bd42 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -11003,7 +11003,7 @@ do_fault:
>   * false otherwise.
>   */
>  static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
> -                               int inputsize, int stride)
> +                               int inputsize, int stride, int outputsize)
>  {
>      const int grainsize = stride + 3;
>      int startsizecheck;
> @@ -11019,22 +11019,19 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
>      }
>  
>      if (is_aa64) {
> -        CPUARMState *env = &cpu->env;
> -        unsigned int pamax = arm_pamax(cpu);
> -
>          switch (stride) {
>          case 13: /* 64KB Pages.  */
> -            if (level == 0 || (level == 1 && pamax <= 42)) {
> +            if (level == 0 || (level == 1 && outputsize <= 42)) {
>                  return false;
>              }
>              break;
>          case 11: /* 16KB Pages.  */
> -            if (level == 0 || (level == 1 && pamax <= 40)) {
> +            if (level == 0 || (level == 1 && outputsize <= 40)) {
>                  return false;
>              }
>              break;
>          case 9: /* 4KB Pages.  */
> -            if (level == 0 && pamax <= 42) {
> +            if (level == 0 && outputsize <= 42) {
>                  return false;
>              }
>              break;
> @@ -11043,8 +11040,8 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
>          }
>  
>          /* Inputsize checks.  */
> -        if (inputsize > pamax &&
> -            (arm_el_is_aa64(env, 1) || inputsize > 40)) {
> +        if (inputsize > outputsize &&
> +            (arm_el_is_aa64(&cpu->env, 1) || inputsize > 40)) {
>              /* This is CONSTRAINED UNPREDICTABLE and we choose to fault.  */
>              return false;
>          }
> @@ -11090,17 +11087,19 @@ static uint8_t convert_stage2_attrs(CPUARMState *env, uint8_t s2attrs)
>  }
>  #endif /* !CONFIG_USER_ONLY */
>  
> +/* This mapping is common between ID_AA64MMFR0.PARANGE and TCR_ELx.{I}PS. */
> +static const uint8_t pamax_map[] = {
> +    [0] = 32,
> +    [1] = 36,
> +    [2] = 40,
> +    [3] = 42,
> +    [4] = 44,
> +    [5] = 48,
> +};

I note that the IPS encoding includes b110 (6) for 52 bits or 4PB
addressing. I guess that gets introduced later.

Anyway:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH 3/6] target/arm: Honor TCR_ELx.{I}PS

[Peter Maydell]

On Wed, 8 Dec 2021 at 23:18, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This field controls the output (intermediate) physical address size
> of the translation process.  V8 requires to raise an AddressSize
> fault if the page tables are programmed incorrectly, such that any
> intermediate descriptor address, or the final translated address,
> is out of range.
>
> Add an outputsize field to ARMVAParameters, and fill it in during
> aa64_va_parameters.  Pass the value to check_s2_mmu_setup to use
> instead of the raw PAMax value.  Test the descaddr as extracted
> from TTBR and from page table entries.
>
> Restrict descaddrmask so that we won't raise the fault for v7.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/internals.h |  1 +
>  target/arm/helper.c    | 92 +++++++++++++++++++++++++++++-------------
>  2 files changed, 65 insertions(+), 28 deletions(-)
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 27d2fcd26c..3e801833b4 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1032,6 +1032,7 @@ static inline uint32_t aarch64_pstate_valid_mask(const ARMISARegisters *id)
>   */
>  typedef struct ARMVAParameters {
>      unsigned tsz    : 8;
> +    unsigned ps     : 3;
>      unsigned select : 1;
>      bool tbi        : 1;
>      bool epd        : 1;
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index fab9ee70d8..568914bd42 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -11003,7 +11003,7 @@ do_fault:
>   * false otherwise.
>   */
>  static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
> -                               int inputsize, int stride)
> +                               int inputsize, int stride, int outputsize)
>  {
>      const int grainsize = stride + 3;
>      int startsizecheck;
> @@ -11019,22 +11019,19 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
>      }
>
>      if (is_aa64) {
> -        CPUARMState *env = &cpu->env;
> -        unsigned int pamax = arm_pamax(cpu);
> -
>          switch (stride) {
>          case 13: /* 64KB Pages.  */
> -            if (level == 0 || (level == 1 && pamax <= 42)) {
> +            if (level == 0 || (level == 1 && outputsize <= 42)) {
>                  return false;
>              }
>              break;
>          case 11: /* 16KB Pages.  */
> -            if (level == 0 || (level == 1 && pamax <= 40)) {
> +            if (level == 0 || (level == 1 && outputsize <= 40)) {
>                  return false;
>              }
>              break;
>          case 9: /* 4KB Pages.  */
> -            if (level == 0 && pamax <= 42) {
> +            if (level == 0 && outputsize <= 42) {
>                  return false;
>              }
>              break;
> @@ -11043,8 +11040,8 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
>          }
>
>          /* Inputsize checks.  */
> -        if (inputsize > pamax &&
> -            (arm_el_is_aa64(env, 1) || inputsize > 40)) {
> +        if (inputsize > outputsize &&
> +            (arm_el_is_aa64(&cpu->env, 1) || inputsize > 40)) {
>              /* This is CONSTRAINED UNPREDICTABLE and we choose to fault.  */
>              return false;
>          }
> @@ -11090,17 +11087,19 @@ static uint8_t convert_stage2_attrs(CPUARMState *env, uint8_t s2attrs)
>  }
>  #endif /* !CONFIG_USER_ONLY */
>
> +/* This mapping is common between ID_AA64MMFR0.PARANGE and TCR_ELx.{I}PS. */
> +static const uint8_t pamax_map[] = {
> +    [0] = 32,
> +    [1] = 36,
> +    [2] = 40,
> +    [3] = 42,
> +    [4] = 44,
> +    [5] = 48,
> +};
> +
>  /* The cpu-specific constant value of PAMax; also used by hw/arm/virt. */
>  unsigned int arm_pamax(ARMCPU *cpu)
>  {
> -    static const unsigned int pamax_map[] = {
> -        [0] = 32,
> -        [1] = 36,
> -        [2] = 40,
> -        [3] = 42,
> -        [4] = 44,
> -        [5] = 48,
> -    };
>      unsigned int parange =
>          FIELD_EX64(cpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
>
> @@ -11151,7 +11150,7 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
>  {
>      uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
>      bool epd, hpd, using16k, using64k;
> -    int select, tsz, tbi;
> +    int select, tsz, tbi, ps;
>
>      if (!regime_has_2_ranges(mmu_idx)) {
>          select = 0;
> @@ -11165,6 +11164,7 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
>              hpd = extract32(tcr, 24, 1);
>          }
>          epd = false;
> +        ps = extract64(tcr, 16, 3);
>      } else {
>          /*
>           * Bit 55 is always between the two regions, and is canonical for
> @@ -11185,6 +11185,7 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
>              epd = extract32(tcr, 23, 1);
>              hpd = extract64(tcr, 42, 1);
>          }
> +        ps = extract64(tcr, 32, 3);
>      }
>
>      /* Present TBI as a composite with TBID.  */
> @@ -11196,6 +11197,7 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
>
>      return (ARMVAParameters) {
>          .tsz = tsz,
> +        .ps = ps,
>          .select = select,
>          .tbi = tbi,
>          .epd = epd,
> @@ -11312,7 +11314,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
>      target_ulong page_size;
>      uint32_t attrs;
>      int32_t stride;
> -    int addrsize, inputsize;
> +    int addrsize, inputsize, outputsize;
>      TCR *tcr = regime_tcr(env, mmu_idx);
>      int ap, ns, xn, pxn;
>      uint32_t el = regime_el(env, mmu_idx);
> @@ -11323,6 +11325,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
>      /* TODO: This code does not support shareability levels. */
>      if (aarch64) {
>          int min_tsz = 16, max_tsz = 39;  /* TODO: ARMv8.2-LVA  */
> +        int parange;
>
>          param = aa64_va_parameters(env, address, mmu_idx,
>                                     access_type != MMU_INST_FETCH);
> @@ -11348,11 +11351,22 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
>
>          addrsize = 64 - 8 * param.tbi;
>          inputsize = 64 - param.tsz;
> +
> +        /*
> +         * Bound PS by PARANGE to find the effective output address size.
> +         * ID_AA64MMFR0 is a read-only register so values outside of the
> +         * supported mappings can be considered an implementation error.
> +         */
> +        parange = FIELD_EX64(cpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
> +        parange = MIN(parange, param.ps);
> +        assert(parange < ARRAY_SIZE(pamax_map));
> +        outputsize = pamax_map[parange];
>      } else {
>          param = aa32_va_parameters(env, address, mmu_idx);
>          level = 1;
>          addrsize = (mmu_idx == ARMMMUIdx_Stage2 ? 40 : 32);
>          inputsize = addrsize - param.tsz;
> +        outputsize = 40;
>      }
>
>      /*
> @@ -11437,7 +11451,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
>
>          /* Check that the starting level is valid. */
>          ok = check_s2_mmu_setup(cpu, aarch64, startlevel,
> -                                inputsize, stride);
> +                                inputsize, stride, outputsize);
>          if (!ok) {
>              fault_type = ARMFault_Translation;
>              goto do_fault;
> @@ -11445,24 +11459,41 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
>          level = startlevel;
>      }
>
> -    indexmask_grainsize = (1ULL << (stride + 3)) - 1;
> -    indexmask = (1ULL << (inputsize - (stride * (4 - level)))) - 1;
> +    indexmask_grainsize = MAKE_64BIT_MASK(0, stride + 3);
> +    indexmask = MAKE_64BIT_MASK(0, inputsize - (stride * (4 - level)));

This is just a refactoring to use MAKE_64BIT_MASK, right? Could
you keep that kind of thing in its own patch, please?

>
>      /* Now we can extract the actual base address from the TTBR */
>      descaddr = extract64(ttbr, 0, 48);
> +
> +    /*
> +     * If the base address is out of range, raise AddressSizeFault.
> +     * In the pseudocode, this is !IsZero(baseregister<47:outputsize>),
> +     * but we've just cleared the bits above 47, so simplify the test.
> +     */
> +    if (descaddr >> outputsize) {
> +        level = 0;
> +        fault_type = ARMFault_AddressSize;
> +        goto do_fault;
> +    }
> +
>      /*
>       * We rely on this masking to clear the RES0 bits at the bottom of the TTBR
>       * and also to mask out CnP (bit 0) which could validly be non-zero.
>       */
>      descaddr &= ~indexmask;
>
> -    /* The address field in the descriptor goes up to bit 39 for ARMv7
> -     * but up to bit 47 for ARMv8, but we use the descaddrmask
> -     * up to bit 39 for AArch32, because we don't need other bits in that case
> -     * to construct next descriptor address (anyway they should be all zeroes).
> +    /*
> +     * The address field in the descriptor goes up to bit 39 for ARMv7
> +     * but up to bit 47 for ARMv8.  In ARMv7, those middle bits are SBZP,
> +     * but in ARMv8 they are checked for zero and an AddressSize fault
> +     * is raised if they are not.

This text seems a bit confused about whether it wants to talk about
v7 vs v8 or AArch64 vs AArch32. For both v7 and v8 the table address
goes only up to 39 for AArch32; the difference is that in v8 the
SBZ (not SBZP) bits [47:40] must be 0 to avoid an Address size fault.
Maybe:

 /*
  * For AArch32, the address field in the descriptor goes up to bit 39
  * in both v7 and v8. However, for v8 the SBZ bits [47:40] must be 0
  * or an AddressSize fault is raised. So for v8 we extract those SBZ
  * bits as part of the address too.
  * For AArch64 the address field always goes up to bit 47 (ignoring
  * the case of FEAT_LPA's 52-bit output addresses, which we don't
  * yet implement). AArch64 always implies v8.
  */

(feel free to tweak that last bit, I haven't read the second half
of this patchset yet.)

>       */
> -    descaddrmask = ((1ull << (aarch64 ? 48 : 40)) - 1) &
> -                   ~indexmask_grainsize;
> +    if (aarch64 || arm_feature(env, ARM_FEATURE_V8)) {

Why are we testing both of these ? If aarch64 is true then we
must have ARM_FEATURE_V8 set, so we could just test that.
(This then matches what the comment text says we're checking.)

> +        descaddrmask = MAKE_64BIT_MASK(0, 48);
> +    } else {
> +        descaddrmask = MAKE_64BIT_MASK(0, 40);
> +    }
> +    descaddrmask &= ~indexmask_grainsize;
>
>      /* Secure accesses start with the page table in secure memory and
>       * can be downgraded to non-secure at any step. Non-secure accesses
> @@ -11487,7 +11518,12 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
>              /* Invalid, or the Reserved level 3 encoding */
>              goto do_fault;
>          }
> +
>          descaddr = descriptor & descaddrmask;
> +        if (descaddr >> outputsize) {
> +            fault_type = ARMFault_AddressSize;
> +            goto do_fault;
> +        }
>
>          if ((descriptor & 2) && (level < 3)) {
>              /* Table entry. The top five bits are attributes which may
> --
> 2.25.1

thanks
-- PMM

[PATCH 4/6] target/arm: Implement FEAT_LVA

[Richard Henderson]

This feature is relatively small, as it applies only to
64k pages and thus requires no additional changes to the
table descriptor walking algorithm, only a change to the
minimum TSZ (which is the inverse of the maximum virtual
address space size).

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

diff --git a/target/arm/cpu-param.h b/target/arm/cpu-param.h
index 7f38d33b8e..5f9c288b1a 100644
--- a/target/arm/cpu-param.h
+++ b/target/arm/cpu-param.h
@@ -11,7 +11,7 @@
 #ifdef TARGET_AARCH64
 # define TARGET_LONG_BITS             64
 # define TARGET_PHYS_ADDR_SPACE_BITS  48
-# define TARGET_VIRT_ADDR_SPACE_BITS  48
+# define TARGET_VIRT_ADDR_SPACE_BITS  52
 #else
 # define TARGET_LONG_BITS             32
 # define TARGET_PHYS_ADDR_SPACE_BITS  40
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index e33f37b70a..3149000004 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -4288,6 +4288,11 @@ static inline bool isar_feature_aa64_ccidx(const ARMISARegisters *id)
     return FIELD_EX64(id->id_aa64mmfr2, ID_AA64MMFR2, CCIDX) != 0;
 }
 
+static inline bool isar_feature_aa64_lva(const ARMISARegisters *id)
+{
+    return FIELD_EX64(id->id_aa64mmfr2, ID_AA64MMFR2, VARANGE) != 0;
+}
+
 static inline bool isar_feature_aa64_tts2uxn(const ARMISARegisters *id)
 {
     return FIELD_EX64(id->id_aa64mmfr1, ID_AA64MMFR1, XNX) != 0;
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 15245a60a8..f44ee643ef 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -755,6 +755,7 @@ static void aarch64_max_initfn(Object *obj)
         t = FIELD_DP64(t, ID_AA64MMFR2, UAO, 1);
         t = FIELD_DP64(t, ID_AA64MMFR2, CNP, 1); /* TTCNP */
         t = FIELD_DP64(t, ID_AA64MMFR2, ST, 1); /* TTST */
+        t = FIELD_DP64(t, ID_AA64MMFR2, VARANGE, 1); /* FEAT_LPA */
         cpu->isar.id_aa64mmfr2 = t;
 
         t = cpu->isar.id_aa64zfr0;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 568914bd42..6a59975028 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -11324,7 +11324,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
 
     /* TODO: This code does not support shareability levels. */
     if (aarch64) {
-        int min_tsz = 16, max_tsz = 39;  /* TODO: ARMv8.2-LVA  */
+        int min_tsz = 16, max_tsz = 39;
         int parange;
 
         param = aa64_va_parameters(env, address, mmu_idx,
@@ -11334,6 +11334,12 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
         if (cpu_isar_feature(aa64_st, env_archcpu(env))) {
             max_tsz = 48 - param.using64k;
         }
+        if (param.using64k) {
+            if (cpu_isar_feature(aa64_lva, env_archcpu(env))) {
+                min_tsz = 12;
+            }
+        }
+        /* TODO: FEAT_LPA2 */
 
         /*
          * If TxSZ is programmed to a value larger than the maximum,
-- 
2.25.1

Re: [PATCH 4/6] target/arm: Implement FEAT_LVA

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> This feature is relatively small, as it applies only to
> 64k pages and thus requires no additional changes to the
> table descriptor walking algorithm, only a change to the
> minimum TSZ (which is the inverse of the maximum virtual
> address space size).
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH 4/6] target/arm: Implement FEAT_LVA

[Peter Maydell]

On Wed, 8 Dec 2021 at 23:16, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This feature is relatively small, as it applies only to
> 64k pages and thus requires no additional changes to the
> table descriptor walking algorithm, only a change to the
> minimum TSZ (which is the inverse of the maximum virtual
> address space size).
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

FEAT_LVA also expands the size of the VA field in
DBGBVR<n>_EL1. We currently hardcode the size of that
in hw_breakpoint_update() where we do:
        addr = sextract64(bvr, 0, 49) & ~3ULL;

This is also true of DBGWVR<n>_EL1, except that there
we seem to have chosen to take advantage of the spec
defining the high bits of the register as RESS (ie
sign-extended) and we always use all of the address bits
regardless. Maybe we could do something similar with DBGBVR.

(Similarly we use all the bits in the VBAR_ELx so that
code needs no changes.)

Otherwise looks good.

-- PMM

Re: [PATCH 4/6] target/arm: Implement FEAT_LVA

[Richard Henderson]

On 1/7/22 07:23, Peter Maydell wrote:
> On Wed, 8 Dec 2021 at 23:16, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> This feature is relatively small, as it applies only to
>> 64k pages and thus requires no additional changes to the
>> table descriptor walking algorithm, only a change to the
>> minimum TSZ (which is the inverse of the maximum virtual
>> address space size).
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> 
> FEAT_LVA also expands the size of the VA field in
> DBGBVR<n>_EL1. We currently hardcode the size of that
> in hw_breakpoint_update() where we do:
>          addr = sextract64(bvr, 0, 49) & ~3ULL;
> 
> This is also true of DBGWVR<n>_EL1, except that there
> we seem to have chosen to take advantage of the spec
> defining the high bits of the register as RESS (ie
> sign-extended) and we always use all of the address bits
> regardless. Maybe we could do something similar with DBGBVR.

We treat DBGBVR and DBGWVR similarly, with the exception that DVGBVR is context dependent, 
so we must wait until we interpret it together with DBGBCR.

However, I think the combination of IMPLEMENTATION DEFINED for storing the value as 
written and CONSTRAINED UNPREDICTABLE for comparing the RESS bits means that we're allowed 
to rely on Software to perform the appropriate extension and store and compare the entire 
register.

I'll fix this in a separate patch.


r~

[PATCH 5/6] target/arm: Implement FEAT_LPA

[Richard Henderson]

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

diff --git a/target/arm/cpu-param.h b/target/arm/cpu-param.h
index 5f9c288b1a..b59d505761 100644
--- a/target/arm/cpu-param.h
+++ b/target/arm/cpu-param.h
@@ -10,7 +10,7 @@
 
 #ifdef TARGET_AARCH64
 # define TARGET_LONG_BITS             64
-# define TARGET_PHYS_ADDR_SPACE_BITS  48
+# define TARGET_PHYS_ADDR_SPACE_BITS  52
 # define TARGET_VIRT_ADDR_SPACE_BITS  52
 #else
 # define TARGET_LONG_BITS             32
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index f44ee643ef..3bb79ca744 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -739,7 +739,7 @@ static void aarch64_max_initfn(Object *obj)
         cpu->isar.id_aa64pfr1 = t;
 
         t = cpu->isar.id_aa64mmfr0;
-        t = FIELD_DP64(t, ID_AA64MMFR0, PARANGE, 5); /* PARange: 48 bits */
+        t = FIELD_DP64(t, ID_AA64MMFR0, PARANGE, 6); /* FEAT_LPA: 52 bits */
         cpu->isar.id_aa64mmfr0 = t;
 
         t = cpu->isar.id_aa64mmfr1;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 6a59975028..e39c1f5b3a 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -11095,6 +11095,7 @@ static const uint8_t pamax_map[] = {
     [3] = 42,
     [4] = 44,
     [5] = 48,
+    [6] = 52,
 };
 
 /* The cpu-specific constant value of PAMax; also used by hw/arm/virt. */
@@ -11472,11 +11473,15 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
     descaddr = extract64(ttbr, 0, 48);
 
     /*
-     * If the base address is out of range, raise AddressSizeFault.
+     * For FEAT_LPA and PS=6, bits [51:48] of descaddr are in [5:2] of TTBR.
+     *
+     * Otherwise, if the base address is out of range, raise AddressSizeFault.
      * In the pseudocode, this is !IsZero(baseregister<47:outputsize>),
      * but we've just cleared the bits above 47, so simplify the test.
      */
-    if (descaddr >> outputsize) {
+    if (outputsize > 48) {
+        descaddr |= extract64(ttbr, 2, 4) << 48;
+    } else if (descaddr >> outputsize) {
         level = 0;
         fault_type = ARMFault_AddressSize;
         goto do_fault;
@@ -11526,7 +11531,15 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
         }
 
         descaddr = descriptor & descaddrmask;
-        if (descaddr >> outputsize) {
+
+        /*
+         * For FEAT_LPA and PS=6, bits [51:48] of descaddr are in [15:12]
+         * of descriptor.  Otherwise, if descaddr is out of range, raise
+         * AddressSizeFault.
+         */
+        if (outputsize > 48) {
+            descaddr |= extract64(descriptor, 12, 4) << 48;
+        } else if (descaddr >> outputsize) {
             fault_type = ARMFault_AddressSize;
             goto do_fault;
         }
-- 
2.25.1

Re: [PATCH 5/6] target/arm: Implement FEAT_LPA

[Peter Maydell]

On Wed, 8 Dec 2021 at 23:15, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Could you put some content in the commit message, please?
A brief description of what FEAT_LPA is would be helpful,
for instance.

While I was rereading the Arm ARM to review this I noticed
an related check that I think we're missing. In section
D5.2.7 the text describes which levels of translation
tables support Block descriptors. (This varies depending on
whether FEAT_LPA/FEAT_LPA2 are supported, which is why I
ran into it.) e.g for 64KB granule size Block descriptors
are OK in level 2, and if (FEAT_LPA && 52 bit PA size supported)
also in level 1. Finding a block descriptor at a level it
should not be is supposed to cause a level 1 Translation fault.
As far as I can tell we don't check for this, unless it
falls out in the wash in some super-subtle way...

That seems like an unrelated bug to be fixed in a
separate patch (though it does entangle with the LPA stuff
because that will change the conditions for the check).

Side note that might be worth mentioning in the
commit message: FEAT_LPA also extends the nominal field
size of addresses held in the fault registers HPFAR_EL2
and PAR_EL1. Our implementation doesn't explicitly mask
out the high bits of addresses when filling out those
fields, so it doesn't need changes to handle them
expanding to hold bits [51:48] of PAs and IPAs.

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

thanks
-- PMM

[PATCH 6/6] target/arm: Implement FEAT_LPA2

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h       | 12 +++++++
 target/arm/internals.h |  2 ++
 target/arm/cpu64.c     |  2 ++
 target/arm/helper.c    | 80 +++++++++++++++++++++++++++++++++++-------
 4 files changed, 83 insertions(+), 13 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 3149000004..379585352b 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -4283,6 +4283,18 @@ static inline bool isar_feature_aa64_i8mm(const ARMISARegisters *id)
     return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, I8MM) != 0;
 }
 
+static inline bool isar_feature_aa64_tgran4_lpa2(const ARMISARegisters *id)
+{
+    return sextract64(id->id_aa64mmfr0,
+                      R_ID_AA64MMFR0_TGRAN4_SHIFT,
+                      R_ID_AA64MMFR0_TGRAN4_LENGTH) >= 1;
+}
+
+static inline bool isar_feature_aa64_tgran16_lpa2(const ARMISARegisters *id)
+{
+    return FIELD_EX64(id->id_aa64mmfr0, ID_AA64MMFR0, TGRAN16) >= 2;
+}
+
 static inline bool isar_feature_aa64_ccidx(const ARMISARegisters *id)
 {
     return FIELD_EX64(id->id_aa64mmfr2, ID_AA64MMFR2, CCIDX) != 0;
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 3e801833b4..868cae2a55 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1033,12 +1033,14 @@ static inline uint32_t aarch64_pstate_valid_mask(const ARMISARegisters *id)
 typedef struct ARMVAParameters {
     unsigned tsz    : 8;
     unsigned ps     : 3;
+    unsigned sh     : 2;
     unsigned select : 1;
     bool tbi        : 1;
     bool epd        : 1;
     bool hpd        : 1;
     bool using16k   : 1;
     bool using64k   : 1;
+    bool ds         : 1;
 } ARMVAParameters;
 
 ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 3bb79ca744..5a1940aa94 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -740,6 +740,8 @@ static void aarch64_max_initfn(Object *obj)
 
         t = cpu->isar.id_aa64mmfr0;
         t = FIELD_DP64(t, ID_AA64MMFR0, PARANGE, 6); /* FEAT_LPA: 52 bits */
+        t = FIELD_DP64(t, ID_AA64MMFR0, TGRAN16, 2); /* FEAT_LPA2: 52 bits */
+        t = FIELD_DP64(t, ID_AA64MMFR0, TGRAN4, 1);  /* FEAT_LPA2: 52 bits */
         cpu->isar.id_aa64mmfr0 = t;
 
         t = cpu->isar.id_aa64mmfr1;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index e39c1f5b3a..f4a8b37f98 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -11008,8 +11008,13 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
     const int grainsize = stride + 3;
     int startsizecheck;
 
-    /* Negative levels are never allowed.  */
-    if (level < 0) {
+    /*
+     * Negative levels are usually not allowed...
+     * Except for FEAT_LPA2, 4k page table, 52-bit address space, which
+     * begins with level -1.  Note that previous feature tests will have
+     * eliminated this combination if it is not enabled.
+     */
+    if (level < (inputsize == 52 && stride == 9 ? -1 : 0)) {
         return false;
     }
 
@@ -11150,8 +11155,8 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
                                    ARMMMUIdx mmu_idx, bool data)
 {
     uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
-    bool epd, hpd, using16k, using64k;
-    int select, tsz, tbi, ps;
+    bool epd, hpd, using16k, using64k, ds;
+    int select, tsz, tbi, ps, sh;
 
     if (!regime_has_2_ranges(mmu_idx)) {
         select = 0;
@@ -11165,7 +11170,9 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
             hpd = extract32(tcr, 24, 1);
         }
         epd = false;
+        sh = extract64(tcr, 12, 2);
         ps = extract64(tcr, 16, 3);
+        ds = extract64(tcr, 32, 1);
     } else {
         /*
          * Bit 55 is always between the two regions, and is canonical for
@@ -11175,6 +11182,7 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
         if (!select) {
             tsz = extract32(tcr, 0, 6);
             epd = extract32(tcr, 7, 1);
+            sh = extract32(tcr, 12, 2);
             using64k = extract32(tcr, 14, 1);
             using16k = extract32(tcr, 15, 1);
             hpd = extract64(tcr, 41, 1);
@@ -11184,9 +11192,11 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
             using64k = tg == 3;
             tsz = extract32(tcr, 16, 6);
             epd = extract32(tcr, 23, 1);
+            sh = extract32(tcr, 28, 2);
             hpd = extract64(tcr, 42, 1);
         }
         ps = extract64(tcr, 32, 3);
+        ds = extract64(tcr, 59, 1);
     }
 
     /* Present TBI as a composite with TBID.  */
@@ -11199,12 +11209,14 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
     return (ARMVAParameters) {
         .tsz = tsz,
         .ps = ps,
+        .sh = sh,
         .select = select,
         .tbi = tbi,
         .epd = epd,
         .hpd = hpd,
         .using16k = using16k,
         .using64k = using64k,
+        .ds = ds,
     };
 }
 
@@ -11332,15 +11344,31 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
                                    access_type != MMU_INST_FETCH);
         level = 0;
 
-        if (cpu_isar_feature(aa64_st, env_archcpu(env))) {
+        /* Find the minimum allowed input address size. */
+        if (cpu_isar_feature(aa64_st, cpu)) {
             max_tsz = 48 - param.using64k;
         }
+
+        /*
+         * Find the maximum allowed input address size.
+         * DS is RES0 unless FEAT_LPA2 is supported for the given page size;
+         * adjust param.ds to the effective value of DS, as documented.
+         */
         if (param.using64k) {
-            if (cpu_isar_feature(aa64_lva, env_archcpu(env))) {
+            if (cpu_isar_feature(aa64_lva, cpu)) {
                 min_tsz = 12;
             }
+            param.ds = false;
+        } else if (param.ds) {
+            /* ??? Assume tgran{4,16}_2 == 0, i.e. match tgran{4,16}. */
+            if (param.using16k
+                ? cpu_isar_feature(aa64_tgran16_lpa2, cpu)
+                : cpu_isar_feature(aa64_tgran4_lpa2, cpu)) {
+                min_tsz = 12;
+            } else {
+                param.ds = false;
+            }
         }
-        /* TODO: FEAT_LPA2 */
 
         /*
          * If TxSZ is programmed to a value larger than the maximum,
@@ -11441,10 +11469,19 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
          * VTCR_EL2.SL0 field (whose interpretation depends on the page size)
          */
         uint32_t sl0 = extract32(tcr->raw_tcr, 6, 2);
+        uint32_t sl2 = extract64(tcr->raw_tcr, 33, 1);
         uint32_t startlevel;
         bool ok;
 
-        if (!aarch64 || stride == 9) {
+        /* SL2 is RES0 unless DS=1 & 4kb granule. */
+        if (param.ds && stride == 9 && sl2) {
+            if (sl0 != 0) {
+                level = 0;
+                fault_type = ARMFault_Translation;
+                goto do_fault;
+            }
+            startlevel = -1;
+        } else if (!aarch64 || stride == 9) {
             /* AArch32 or 4KB pages */
             startlevel = 2 - sl0;
 
@@ -11499,7 +11536,9 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
      * but in ARMv8 they are checked for zero and an AddressSize fault
      * is raised if they are not.
      */
-    if (aarch64 || arm_feature(env, ARM_FEATURE_V8)) {
+    if (param.ds) {
+        descaddrmask = MAKE_64BIT_MASK(0, 50);
+    } else if (aarch64 || arm_feature(env, ARM_FEATURE_V8)) {
         descaddrmask = MAKE_64BIT_MASK(0, 48);
     } else {
         descaddrmask = MAKE_64BIT_MASK(0, 40);
@@ -11534,11 +11573,16 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
 
         /*
          * For FEAT_LPA and PS=6, bits [51:48] of descaddr are in [15:12]
-         * of descriptor.  Otherwise, if descaddr is out of range, raise
-         * AddressSizeFault.
+         * of descriptor.  For FEAT_LPA2 and effective DS, bits [51:50] of
+         * descaddr are in [9:8].  Otherwise, if descaddr is out of range,
+         * raise AddressSizeFault.
          */
         if (outputsize > 48) {
-            descaddr |= extract64(descriptor, 12, 4) << 48;
+            if (param.ds) {
+                descaddr |= extract64(descriptor, 8, 2) << 50;
+            } else {
+                descaddr |= extract64(descriptor, 12, 4) << 48;
+            }
         } else if (descaddr >> outputsize) {
             fault_type = ARMFault_AddressSize;
             goto do_fault;
@@ -11632,7 +11676,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
         assert(attrindx <= 7);
         cacheattrs->attrs = extract64(mair, attrindx * 8, 8);
     }
-    cacheattrs->shareability = extract32(attrs, 6, 2);
+
+    /*
+     * For FEAT_LPA2 and effective DS, the SH field in the attributes
+     * was re-purposed for output address bits.  The SH attribute in
+     * that case comes from TCR_ELx, which we extracted earlier.
+     */
+    if (param.ds) {
+        cacheattrs->shareability = param.sh;
+    } else {
+        cacheattrs->shareability = extract32(attrs, 6, 2);
+    }
 
     *phys_ptr = descaddr;
     *page_size_ptr = page_size;
-- 
2.25.1

Re: [PATCH 6/6] target/arm: Implement FEAT_LPA2

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h       | 12 +++++++
>  target/arm/internals.h |  2 ++
>  target/arm/cpu64.c     |  2 ++
>  target/arm/helper.c    | 80 +++++++++++++++++++++++++++++++++++-------
>  4 files changed, 83 insertions(+), 13 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 3149000004..379585352b 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -4283,6 +4283,18 @@ static inline bool isar_feature_aa64_i8mm(const ARMISARegisters *id)
>      return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, I8MM) != 0;
>  }
>  
> +static inline bool isar_feature_aa64_tgran4_lpa2(const ARMISARegisters *id)
> +{
> +    return sextract64(id->id_aa64mmfr0,
> +                      R_ID_AA64MMFR0_TGRAN4_SHIFT,
> +                      R_ID_AA64MMFR0_TGRAN4_LENGTH) >= 1;

Is this correct - it shows:

  0b1111 4KB granule not supported.

which I don't think implies FEAT_LPA2 because 1 indicates 4kb granule
supports 52 bit addressing. The other values are also reserved. Maybe it
would be safer just of == 1?

(a little more reading later)

  The ID_AA64MMFR0_EL1.TGran4_2, ID_AA64MMFR0_EL1.TGran16_2 and
  ID_AA64MMFR0_EL1.TGran64_2 fields that identify the memory translation stage 2 granule size, do not follow
  the standard ID scheme. Software must treat these fields as follows:
  • The value 0x0 indicates that support is identified by another field.
  • If the field value is not 0x0, the value indicates the level of support provided.
  This means that software should use a test of the form:
  if (field !=0 and field > value) {
  // do something that relies on the value of the feature
  }

That's just confusing. It implies that you could see any of the TGran
fields set to zero but still support LPA2 if the other fields are set. I
think this at least warrants mentioning in an accompanying comments for
the function. 

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH 6/6] target/arm: Implement FEAT_LPA2

[Richard Henderson]

On 12/14/21 6:57 AM, Alex Bennée wrote:
>> +static inline bool isar_feature_aa64_tgran4_lpa2(const ARMISARegisters *id)
>> +{
>> +    return sextract64(id->id_aa64mmfr0,
>> +                      R_ID_AA64MMFR0_TGRAN4_SHIFT,
>> +                      R_ID_AA64MMFR0_TGRAN4_LENGTH) >= 1;
> 
> Is this correct - it shows:
> 
>    0b1111 4KB granule not supported.

Yes, that's why the signed extract, so not supported comes out as -1.
See D13.1.3 "Principles of the ID scheme for fields in ID registers".


> (a little more reading later)
> 
>    The ID_AA64MMFR0_EL1.TGran4_2, ID_AA64MMFR0_EL1.TGran16_2 and
>    ID_AA64MMFR0_EL1.TGran64_2 fields that identify the memory translation stage 2 granule size, do not follow
>    the standard ID scheme. Software must treat these fields as follows:

Note that we're not testing the *_2 fields, which are *stage2* support, not stage1.  I did 
add a comment about assuming stage2 encodes the same value as stage1 (which is true for 
all supported cpus).



r~

Re: [PATCH 6/6] target/arm: Implement FEAT_LPA2

[Peter Maydell]

On Wed, 8 Dec 2021 at 23:21, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Again, a commit message would be nice.

> ---
>  target/arm/cpu.h       | 12 +++++++
>  target/arm/internals.h |  2 ++
>  target/arm/cpu64.c     |  2 ++
>  target/arm/helper.c    | 80 +++++++++++++++++++++++++++++++++++-------
>  4 files changed, 83 insertions(+), 13 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 3149000004..379585352b 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -4283,6 +4283,18 @@ static inline bool isar_feature_aa64_i8mm(const ARMISARegisters *id)
>      return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, I8MM) != 0;
>  }
>
> +static inline bool isar_feature_aa64_tgran4_lpa2(const ARMISARegisters *id)
> +{
> +    return sextract64(id->id_aa64mmfr0,
> +                      R_ID_AA64MMFR0_TGRAN4_SHIFT,
> +                      R_ID_AA64MMFR0_TGRAN4_LENGTH) >= 1;
> +}
> +
> +static inline bool isar_feature_aa64_tgran16_lpa2(const ARMISARegisters *id)
> +{
> +    return FIELD_EX64(id->id_aa64mmfr0, ID_AA64MMFR0, TGRAN16) >= 2;
> +}

(I wonder if we should have a FIELD_SEX64 etc ?)

> +
>  static inline bool isar_feature_aa64_ccidx(const ARMISARegisters *id)
>  {
>      return FIELD_EX64(id->id_aa64mmfr2, ID_AA64MMFR2, CCIDX) != 0;
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 3e801833b4..868cae2a55 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1033,12 +1033,14 @@ static inline uint32_t aarch64_pstate_valid_mask(const ARMISARegisters *id)
>  typedef struct ARMVAParameters {
>      unsigned tsz    : 8;
>      unsigned ps     : 3;
> +    unsigned sh     : 2;
>      unsigned select : 1;
>      bool tbi        : 1;
>      bool epd        : 1;
>      bool hpd        : 1;
>      bool using16k   : 1;
>      bool using64k   : 1;
> +    bool ds         : 1;
>  } ARMVAParameters;
>
>  ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 3bb79ca744..5a1940aa94 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -740,6 +740,8 @@ static void aarch64_max_initfn(Object *obj)
>
>          t = cpu->isar.id_aa64mmfr0;
>          t = FIELD_DP64(t, ID_AA64MMFR0, PARANGE, 6); /* FEAT_LPA: 52 bits */
> +        t = FIELD_DP64(t, ID_AA64MMFR0, TGRAN16, 2); /* FEAT_LPA2: 52 bits */
> +        t = FIELD_DP64(t, ID_AA64MMFR0, TGRAN4, 1);  /* FEAT_LPA2: 52 bits */

Shouldn't we also set the TGRAN4_2 and TGRAN16_2 fields?

>          cpu->isar.id_aa64mmfr0 = t;
>
>          t = cpu->isar.id_aa64mmfr1;
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index e39c1f5b3a..f4a8b37f98 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -11008,8 +11008,13 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
>      const int grainsize = stride + 3;
>      int startsizecheck;
>
> -    /* Negative levels are never allowed.  */
> -    if (level < 0) {
> +    /*
> +     * Negative levels are usually not allowed...
> +     * Except for FEAT_LPA2, 4k page table, 52-bit address space, which
> +     * begins with level -1.  Note that previous feature tests will have
> +     * eliminated this combination if it is not enabled.
> +     */
> +    if (level < (inputsize == 52 && stride == 9 ? -1 : 0)) {
>          return false;
>      }

The checks on validity of 'level' are getting quite complicated:
 * we do this check here (which now involves 'stride')
 * some values of 'level' will cause the startsizecheck to fail
   (calculation of startsizecheck also involves 'stride')
 * we then switch on 'stride' and check for 'level' validity
   differently in the various cases

Can we simplify this at all ? Or are we just following the logic
the pseudocode uses (I haven't checked) ?

> @@ -11150,8 +11155,8 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
>                                     ARMMMUIdx mmu_idx, bool data)
>  {
>      uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
> -    bool epd, hpd, using16k, using64k;
> -    int select, tsz, tbi, ps;
> +    bool epd, hpd, using16k, using64k, ds;
> +    int select, tsz, tbi, ps, sh;
>
>      if (!regime_has_2_ranges(mmu_idx)) {
>          select = 0;
> @@ -11165,7 +11170,9 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
>              hpd = extract32(tcr, 24, 1);
>          }
>          epd = false;
> +        sh = extract64(tcr, 12, 2);
>          ps = extract64(tcr, 16, 3);
> +        ds = extract64(tcr, 32, 1);
>      } else {
>          /*
>           * Bit 55 is always between the two regions, and is canonical for
> @@ -11175,6 +11182,7 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
>          if (!select) {
>              tsz = extract32(tcr, 0, 6);
>              epd = extract32(tcr, 7, 1);
> +            sh = extract32(tcr, 12, 2);
>              using64k = extract32(tcr, 14, 1);
>              using16k = extract32(tcr, 15, 1);
>              hpd = extract64(tcr, 41, 1);
> @@ -11184,9 +11192,11 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
>              using64k = tg == 3;
>              tsz = extract32(tcr, 16, 6);
>              epd = extract32(tcr, 23, 1);
> +            sh = extract32(tcr, 28, 2);
>              hpd = extract64(tcr, 42, 1);
>          }
>          ps = extract64(tcr, 32, 3);
> +        ds = extract64(tcr, 59, 1);
>      }
>
>      /* Present TBI as a composite with TBID.  */
> @@ -11199,12 +11209,14 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
>      return (ARMVAParameters) {
>          .tsz = tsz,
>          .ps = ps,
> +        .sh = sh,
>          .select = select,
>          .tbi = tbi,
>          .epd = epd,
>          .hpd = hpd,
>          .using16k = using16k,
>          .using64k = using64k,
> +        .ds = ds,
>      };
>  }
>
> @@ -11332,15 +11344,31 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
>                                     access_type != MMU_INST_FETCH);
>          level = 0;
>
> -        if (cpu_isar_feature(aa64_st, env_archcpu(env))) {
> +        /* Find the minimum allowed input address size. */
> +        if (cpu_isar_feature(aa64_st, cpu)) {
>              max_tsz = 48 - param.using64k;
>          }
> +
> +        /*
> +         * Find the maximum allowed input address size.
> +         * DS is RES0 unless FEAT_LPA2 is supported for the given page size;
> +         * adjust param.ds to the effective value of DS, as documented.
> +         */
>          if (param.using64k) {
> -            if (cpu_isar_feature(aa64_lva, env_archcpu(env))) {
> +            if (cpu_isar_feature(aa64_lva, cpu)) {
>                  min_tsz = 12;
>              }
> +            param.ds = false;
> +        } else if (param.ds) {
> +            /* ??? Assume tgran{4,16}_2 == 0, i.e. match tgran{4,16}. */

How painful would it be to fix this '???' ? Setting those fields to 0 is
deprecated, so it would be preferable not to start out depending on that.
(We don't currently use the tgran fields at all, I guess because we allow
all page sizes regardless of the ID register values. Maybe we should
correct that too.)

> +            if (param.using16k
> +                ? cpu_isar_feature(aa64_tgran16_lpa2, cpu)
> +                : cpu_isar_feature(aa64_tgran4_lpa2, cpu)) {
> +                min_tsz = 12;
> +            } else {
> +                param.ds = false;
> +            }
>          }
> -        /* TODO: FEAT_LPA2 */
>
>          /*
>           * If TxSZ is programmed to a value larger than the maximum,
> @@ -11441,10 +11469,19 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
>           * VTCR_EL2.SL0 field (whose interpretation depends on the page size)
>           */
>          uint32_t sl0 = extract32(tcr->raw_tcr, 6, 2);
> +        uint32_t sl2 = extract64(tcr->raw_tcr, 33, 1);
>          uint32_t startlevel;
>          bool ok;
>
> -        if (!aarch64 || stride == 9) {
> +        /* SL2 is RES0 unless DS=1 & 4kb granule. */
> +        if (param.ds && stride == 9 && sl2) {
> +            if (sl0 != 0) {
> +                level = 0;
> +                fault_type = ARMFault_Translation;
> +                goto do_fault;
> +            }
> +            startlevel = -1;
> +        } else if (!aarch64 || stride == 9) {
>              /* AArch32 or 4KB pages */
>              startlevel = 2 - sl0;
>
> @@ -11499,7 +11536,9 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
>       * but in ARMv8 they are checked for zero and an AddressSize fault
>       * is raised if they are not.
>       */
> -    if (aarch64 || arm_feature(env, ARM_FEATURE_V8)) {
> +    if (param.ds) {
> +        descaddrmask = MAKE_64BIT_MASK(0, 50);
> +    } else if (aarch64 || arm_feature(env, ARM_FEATURE_V8)) {
>          descaddrmask = MAKE_64BIT_MASK(0, 48);
>      } else {
>          descaddrmask = MAKE_64BIT_MASK(0, 40);
> @@ -11534,11 +11573,16 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
>
>          /*
>           * For FEAT_LPA and PS=6, bits [51:48] of descaddr are in [15:12]
> -         * of descriptor.  Otherwise, if descaddr is out of range, raise
> -         * AddressSizeFault.
> +         * of descriptor.  For FEAT_LPA2 and effective DS, bits [51:50] of
> +         * descaddr are in [9:8].  Otherwise, if descaddr is out of range,
> +         * raise AddressSizeFault.
>           */
>          if (outputsize > 48) {
> -            descaddr |= extract64(descriptor, 12, 4) << 48;
> +            if (param.ds) {
> +                descaddr |= extract64(descriptor, 8, 2) << 50;
> +            } else {
> +                descaddr |= extract64(descriptor, 12, 4) << 48;
> +            }
>          } else if (descaddr >> outputsize) {
>              fault_type = ARMFault_AddressSize;
>              goto do_fault;
> @@ -11632,7 +11676,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
>          assert(attrindx <= 7);
>          cacheattrs->attrs = extract64(mair, attrindx * 8, 8);
>      }
> -    cacheattrs->shareability = extract32(attrs, 6, 2);
> +
> +    /*
> +     * For FEAT_LPA2 and effective DS, the SH field in the attributes
> +     * was re-purposed for output address bits.  The SH attribute in
> +     * that case comes from TCR_ELx, which we extracted earlier.
> +     */
> +    if (param.ds) {
> +        cacheattrs->shareability = param.sh;
> +    } else {
> +        cacheattrs->shareability = extract32(attrs, 6, 2);
> +    }
>
>      *phys_ptr = descaddr;
>      *page_size_ptr = page_size;

The code above looks correct to me. There are some missing pieces
elsewhere, though:

(1) The handling of the BaseADDR field for TLB range
invalidates needs updating (there's a TODO to this effect in
tlbi_aa64_range_get_base()).

Side note: in that function, we shift the field by TARGET_PAGE_BITS,
but the docs say that the shift should depend on the configured
translation granule. Is that a bug?

(2) There are some new long-form fault status codes with FEAT_LPA2,
corresponding to various fault types that can now occur at level -1.
arm_fi_to_lfsc() needs updating to handle fi->level being -1.
(You could do this bit as a preceding patch; it doesn't need to
be squashed into this one.)

thanks
-- PMM

Re: [PATCH 6/6] target/arm: Implement FEAT_LPA2

[Richard Henderson]

On 1/8/22 01:39, Peter Maydell wrote:
> (1) The handling of the BaseADDR field for TLB range
> invalidates needs updating (there's a TODO to this effect in
> tlbi_aa64_range_get_base()).
> 
> Side note: in that function, we shift the field by TARGET_PAGE_BITS,
> but the docs say that the shift should depend on the configured
> translation granule. Is that a bug?

Yes.

> (2) There are some new long-form fault status codes with FEAT_LPA2,
> corresponding to various fault types that can now occur at level -1.
> arm_fi_to_lfsc() needs updating to handle fi->level being -1.
> (You could do this bit as a preceding patch; it doesn't need to
> be squashed into this one.)

Yep, thanks.


r~

Re: [PATCH for-7.0 0/6] target/arm: Implement LVA, LPA, LPA2 features

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> These features are all related and relatively small.
>
> Testing so far has been limited to booting a kernel
> with 64k pages and VA and PA set to 52 bits, which
> excercises LVA and LPA.

Do any distros ship with 64k pages that we could use for an avocado
test?

> There is not yet upstream support for LPA2, probably
> because it's an ARMv8.7 addition.

I guess we can defer adding tests for this until better upstream
support. Are there any WIP branches to test with? I've CC'd the kernel
guys.

>
>
> r~
>
>
> Richard Henderson (6):
>   target/arm: Fault on invalid TCR_ELx.TxSZ
>   target/arm: Move arm_pamax out of line
>   target/arm: Honor TCR_ELx.{I}PS
>   target/arm: Implement FEAT_LVA
>   target/arm: Implement FEAT_LPA
>   target/arm: Implement FEAT_LPA2
>
>  target/arm/cpu-param.h |   4 +-
>  target/arm/cpu.h       |  17 ++++
>  target/arm/internals.h |  22 +----
>  target/arm/cpu64.c     |   5 +-
>  target/arm/helper.c    | 211 ++++++++++++++++++++++++++++++++++-------
>  5 files changed, 204 insertions(+), 55 deletions(-)


-- 
Alex Bennée

Re: [PATCH for-7.0 0/6] target/arm: Implement LVA, LPA, LPA2 features

[Richard Henderson]

On 12/14/21 8:37 AM, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> These features are all related and relatively small.
>>
>> Testing so far has been limited to booting a kernel
>> with 64k pages and VA and PA set to 52 bits, which
>> excercises LVA and LPA.
> 
> Do any distros ship with 64k pages that we could use for an avocado
> test?

Well, RHEL 8 has 64k pages but with a 48-bit address space.  There are separate kernel 
configuration options for 52-bits.


r~

Re: [PATCH for-7.0 0/6] target/arm: Implement LVA, LPA, LPA2 features

[Peter Maydell]

On Wed, 8 Dec 2021 at 23:14, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> These features are all related and relatively small.
>
> Testing so far has been limited to booting a kernel
> with 64k pages and VA and PA set to 52 bits, which
> excercises LVA and LPA.
>
> There is not yet upstream support for LPA2, probably
> because it's an ARMv8.7 addition.



>  target/arm/cpu-param.h |   4 +-
>  target/arm/cpu.h       |  17 ++++
>  target/arm/internals.h |  22 +----
>  target/arm/cpu64.c     |   5 +-
>  target/arm/helper.c    | 211 ++++++++++++++++++++++++++++++++++-------
>  5 files changed, 204 insertions(+), 55 deletions(-)

I'd forgotten about this document too until a conversation today
brought it to mind, but when adding new feature support please
also update the list of supported emulated features in
docs/system/arm/emulation.rst

thanks
-- PMM