[PATCH v3 0/2] target/arm: Misc cleanups surrounding TBI

[PATCH v3 0/2] target/arm: Misc cleanups surrounding TBI

[Richard Henderson]

Changes for v3:
  * All but 2 patches are now merged.
  * Use regime_el to determine aa64-ness of physical memory addressing.


r~


Richard Henderson (2):
  target/arm: Check addresses for disabled regimes
  target/arm: Disable clean_data_tbi for system mode

 target/arm/helper.c        | 35 ++++++++++++++++++++++++++++++++++-
 target/arm/translate-a64.c | 11 +++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)

-- 
2.20.1

[PATCH v3 1/2] target/arm: Check addresses for disabled regimes

[Richard Henderson]

We fail to validate the upper bits of a virtual address on a
translation disabled regime, as per AArch64.TranslateAddressS1Off.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index f91e5d5345..8f81ca4f54 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -11780,7 +11780,40 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
     /* Definitely a real MMU, not an MPU */
 
     if (regime_translation_disabled(env, mmu_idx)) {
-        /* MMU disabled. */
+        /*
+         * MMU disabled.  S1 addresses within aa64 translation regimes are
+         * still checked for bounds -- see AArch64.TranslateAddressS1Off.
+         */
+        if (mmu_idx != ARMMMUIdx_Stage2) {
+            int r_el = regime_el(env, mmu_idx);
+            if (arm_el_is_aa64(env, r_el)) {
+                int pamax = arm_pamax(env_archcpu(env));
+                uint64_t tcr = env->cp15.tcr_el[r_el].raw_tcr;
+                int addrtop, tbi;
+
+                tbi = aa64_va_parameter_tbi(tcr, mmu_idx);
+                if (access_type == MMU_INST_FETCH) {
+                    tbi &= ~aa64_va_parameter_tbid(tcr, mmu_idx);
+                }
+                tbi = (tbi >> extract64(address, 55, 1)) & 1;
+                addrtop = (tbi ? 55 : 63);
+
+                if (extract64(address, pamax, addrtop - pamax + 1) != 0) {
+                    fi->type = ARMFault_AddressSize;
+                    fi->level = 0;
+                    fi->stage2 = false;
+                    return 1;
+                }
+
+                /*
+                 * When TBI is disabled, we've just validated that all of the
+                 * bits above PAMax are zero, so logically we only need to
+                 * clear the top byte for TBI.  But it's clearer to follow
+                 * the pseudocode set of addrdesc.paddress.
+                 */
+                address = extract64(address, 0, 52);
+            }
+        }
         *phys_ptr = address;
         *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
         *page_size = TARGET_PAGE_SIZE;
-- 
2.20.1

[PATCH v3 2/2] target/arm: Disable clean_data_tbi for system mode

[Richard Henderson]

We must include the tag in the FAR_ELx register when raising
an addressing exception.  Which means that we should not clear
out the tag during translation.

We cannot at present comply with this for user mode, so we
retain the clean_data_tbi function for the moment, though it
no longer does what it says on the tin for system mode.  This
function is to be replaced with MTE, so don't worry about the
slight misnaming.

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

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index fefe8af7f5..8fffb52203 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -228,7 +228,18 @@ static void gen_a64_set_pc(DisasContext *s, TCGv_i64 src)
 static TCGv_i64 clean_data_tbi(DisasContext *s, TCGv_i64 addr)
 {
     TCGv_i64 clean = new_tmp_a64(s);
+    /*
+     * In order to get the correct value in the FAR_ELx register,
+     * we must present the memory subsystem with the "dirty" address
+     * including the TBI.  In system mode we can make this work via
+     * the TLB, dropping the TBI during translation.  But for user-only
+     * mode we don't have that option, and must remove the top byte now.
+     */
+#ifdef CONFIG_USER_ONLY
     gen_top_byte_ignore(s, clean, addr, s->tbid);
+#else
+    tcg_gen_mov_i64(clean, addr);
+#endif
     return clean;
 }
 
-- 
2.20.1

Re: [PATCH v3 0/2] target/arm: Misc cleanups surrounding TBI

[Peter Maydell]

On Sun, 8 Mar 2020 at 01:29, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Changes for v3:
>   * All but 2 patches are now merged.
>   * Use regime_el to determine aa64-ness of physical memory addressing.
>



Applied to target-arm.next, thanks.

-- PMM

Re: [PATCH v3 0/2] target/arm: Misc cleanups surrounding TBI

[Richard Henderson]

On 3/12/20 5:02 AM, Peter Maydell wrote:
> On Sun, 8 Mar 2020 at 01:29, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Changes for v3:
>>   * All but 2 patches are now merged.
>>   * Use regime_el to determine aa64-ness of physical memory addressing.
>>
> 
> 
> 
> Applied to target-arm.next, thanks.

Amusingly a bug report came in yesterday about this.
If you like, you can add

Buglink: https://bugs.launchpad.net/qemu/+bug/1867072

to the final patch.


r~