Re: [PATCH v5 00/22] target/arm: Implement ARMv8.5-MemTag, system mode

[Evgenii Stepanov <eugeni.stepanov@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8451 bytes --]

Hi,

please find attached three random fixes for instruction translation
and one for syscall emulation.

On Fri, Oct 11, 2019 at 6:48 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This is an update of the v4 patch from March.
>
> I believe I've fixed the address space issues that Peter noticed.
> If the board model does not supply tag memory, then I downgrade
> the cpu support to "instructions only" (id_aa64pfr1.mte == 1),
> which does not allow tag memory access to be enabled in the cpu.
>
> I did not update the arm_hcr_el2_eff comment re ARMv8.4, because
> I have not done a complete audit of all of the v8.5 bits.
>
> The hacky kernel testing patch has needed some updates since March.
> The following applies to v5.4-rc2.
>
>
> r~
>
>
> Richard Henderson (22):
>   target/arm: Add MTE_ACTIVE to tb_flags
>   target/arm: Add regime_has_2_ranges
>   target/arm: Add MTE system registers
>   target/arm: Add helper_mte_check{1,2,3}
>   target/arm: Suppress tag check for sp+offset
>   target/arm: Implement the IRG instruction
>   target/arm: Implement ADDG, SUBG instructions
>   target/arm: Implement the GMI instruction
>   target/arm: Implement the SUBP instruction
>   target/arm: Define arm_cpu_do_unaligned_access for CONFIG_USER_ONLY
>   target/arm: Implement LDG, STG, ST2G instructions
>   target/arm: Implement the STGP instruction
>   target/arm: Implement the LDGM and STGM instructions
>   target/arm: Implement the access tag cache flushes
>   target/arm: Clean address for DC ZVA
>   target/arm: Implement data cache set allocation tags
>   target/arm: Set PSTATE.TCO on exception entry
>   target/arm: Enable MTE
>   target/arm: Cache the Tagged bit for a page in MemTxAttrs
>   target/arm: Create tagged ram when MTE is enabled
>   target/arm: Add mmu indexes for tag memory
>   target/arm: Add allocation tag storage for system mode
>
>  target/arm/cpu-param.h     |   2 +-
>  target/arm/cpu.h           |  37 ++-
>  target/arm/helper-a64.h    |  17 ++
>  target/arm/internals.h     |  45 +++
>  target/arm/translate.h     |   2 +
>  hw/arm/virt.c              |  54 ++++
>  target/arm/cpu.c           |  63 +++-
>  target/arm/cpu64.c         |   1 +
>  target/arm/helper.c        | 277 ++++++++++++++---
>  target/arm/mte_helper.c    | 601 +++++++++++++++++++++++++++++++++++++
>  target/arm/tlb_helper.c    |   3 +-
>  target/arm/translate-a64.c | 342 ++++++++++++++++++---
>  target/arm/Makefile.objs   |   1 +
>  13 files changed, 1345 insertions(+), 100 deletions(-)
>  create mode 100644 target/arm/mte_helper.c
>
> --- kernel patch
>
> diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> index f19fe4b9acc4..ee6b7f387a9a 100644
> --- a/arch/arm64/include/asm/cpucaps.h
> +++ b/arch/arm64/include/asm/cpucaps.h
> @@ -52,7 +52,8 @@
>  #define ARM64_HAS_IRQ_PRIO_MASKING             42
>  #define ARM64_HAS_DCPODP                       43
>  #define ARM64_WORKAROUND_1463225               44
> +#define ARM64_HAS_MTE                          45
>
> -#define ARM64_NCAPS                            45
> +#define ARM64_NCAPS                            46
>
>  #endif /* __ASM_CPUCAPS_H */
> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> index ddf9d762ac62..5825130bd8eb 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -12,6 +12,7 @@
>  #include <asm/types.h>
>
>  /* Hyp Configuration Register (HCR) bits */
> +#define HCR_ATA                (UL(1) << 56)
>  #define HCR_FWB                (UL(1) << 46)
>  #define HCR_API                (UL(1) << 41)
>  #define HCR_APK                (UL(1) << 40)
> @@ -78,8 +79,8 @@
>                          HCR_AMO | HCR_SWIO | HCR_TIDCP | HCR_RW | HCR_TLOR | \
>                          HCR_FMO | HCR_IMO)
>  #define HCR_VIRT_EXCP_MASK (HCR_VSE | HCR_VI | HCR_VF)
> -#define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK)
> -#define HCR_HOST_VHE_FLAGS (HCR_RW | HCR_TGE | HCR_E2H)
> +#define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK | HCR_ATA)
> +#define HCR_HOST_VHE_FLAGS (HCR_RW | HCR_TGE | HCR_E2H | HCR_ATA)
>
>  /* TCR_EL2 Registers bits */
>  #define TCR_EL2_RES1           ((1 << 31) | (1 << 23))
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 972d196c7714..2a65831f6e0f 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -482,6 +482,7 @@
>
>  /* Common SCTLR_ELx flags. */
>  #define SCTLR_ELx_DSSBS        (BIT(44))
> +#define SCTLR_ELx_ATA  (BIT(43))
>  #define SCTLR_ELx_ENIA (BIT(31))
>  #define SCTLR_ELx_ENIB (BIT(30))
>  #define SCTLR_ELx_ENDA (BIT(27))
> @@ -510,6 +511,7 @@
>  #endif
>
>  /* SCTLR_EL1 specific flags. */
> +#define SCTLR_EL1_ATA0         (BIT(42))
>  #define SCTLR_EL1_UCI          (BIT(26))
>  #define SCTLR_EL1_E0E          (BIT(24))
>  #define SCTLR_EL1_SPAN         (BIT(23))
> @@ -598,6 +600,7 @@
>  #define ID_AA64PFR0_EL0_32BIT_64BIT    0x2
>
>  /* id_aa64pfr1 */
> +#define ID_AA64PFR1_MTE_SHIFT          8
>  #define ID_AA64PFR1_SSBS_SHIFT         4
>
>  #define ID_AA64PFR1_SSBS_PSTATE_NI     0
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index cabebf1a7976..6a122ed7f76b 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -171,6 +171,7 @@ static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = {
>  };
>
>  static const struct arm64_ftr_bits ftr_id_aa64pfr1[] = {
> +       ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR1_MTE_SHIFT, 4, 0),
>         ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR1_SSBS_SHIFT, 4, ID_AA64PFR1_SSBS_PSTATE_NI),
>         ARM64_FTR_END,
>  };
> @@ -1261,6 +1262,11 @@ static bool can_use_gic_priorities(const struct arm64_cpu_capabilities *entry,
>  }
>  #endif
>
> +static void cpu_enable_mte(struct arm64_cpu_capabilities const *cap)
> +{
> +       sysreg_clear_set(sctlr_el1, 0, SCTLR_ELx_ATA | SCTLR_EL1_ATA0);
> +}
> +
>  static const struct arm64_cpu_capabilities arm64_features[] = {
>         {
>                 .desc = "GIC system register CPU interface",
> @@ -1561,6 +1567,17 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
>                 .min_field_value = 1,
>         },
>  #endif
> +       {
> +               .desc = "Memory Tagging",
> +               .capability = ARM64_HAS_MTE,
> +               .type = ARM64_CPUCAP_SYSTEM_FEATURE,
> +               .matches = has_cpuid_feature,
> +               .sys_reg = SYS_ID_AA64PFR1_EL1,
> +               .field_pos = ID_AA64PFR1_MTE_SHIFT,
> +               .sign = FTR_UNSIGNED,
> +               .min_field_value = 2,
> +               .cpu_enable = cpu_enable_mte,
> +       },
>         {},
>  };
>
> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index a1e0592d1fbc..32cfa35195ae 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -424,14 +424,14 @@ ENTRY(__cpu_setup)
>          *   DEVICE_nGnRE       001     00000100
>          *   DEVICE_GRE         010     00001100
>          *   NORMAL_NC          011     01000100
> -        *   NORMAL             100     11111111
> +        *   NORMAL             100     11110000 (Tag)
>          *   NORMAL_WT          101     10111011
>          */
>         ldr     x5, =MAIR(0x00, MT_DEVICE_nGnRnE) | \
>                      MAIR(0x04, MT_DEVICE_nGnRE) | \
>                      MAIR(0x0c, MT_DEVICE_GRE) | \
>                      MAIR(0x44, MT_NORMAL_NC) | \
> -                    MAIR(0xff, MT_NORMAL) | \
> +                    MAIR(0xf0, MT_NORMAL) | \
>                      MAIR(0xbb, MT_NORMAL_WT)
>         msr     mair_el1, x5
>         /*
>
> --- mte smoke test
>
> /*
>  * Memory tagging, basic pass cases.
>  */
>
> #include <stdio.h>
> #include <assert.h>
> #include <sys/mman.h>
>
> asm(".arch armv8.5-a+memtag");
>
> int data[16 / sizeof(int)] __attribute__((aligned(16)));
>
> int main(int ac, char **av)
> {
>     int *p0 = data;
>     int *p1, *p2;
>     long c;
>
>     if (mlock(data, sizeof(data)) < 0) {
>         perror("mlock");
>         return 1;
>     }
>
>     asm("irg %0,%1,%2" : "=r"(p1) : "r"(p0), "r"(1));
>     assert(p1 != p0);
>     asm("subp %0,%1,%2" : "=r"(c) : "r"(p0), "r"(p1));
>     assert(c == 0);
>
>     asm("stg %0, [%0]" : : "r"(p1));
>     asm("ldg %0, [%1]" : "=r"(p2) : "r"(p0), "0"(p0));
>     assert(p1 == p2);
>
>     return 0;
> }
>
>

[-- Attachment #2: 0004-Fix-pre-post-index-confusion-in-disas_ldst_tag.patch --]
[-- Type: text/x-patch, Size: 1111 bytes --]

From 7dfe3f53bc606d2c5bb81e5828e6cf32225f6b72 Mon Sep 17 00:00:00 2001
From: Evgenii Stepanov <eugenis@google.com>
Date: Tue, 27 Aug 2019 16:14:37 -0700
Subject: [PATCH 4/4] Fix pre/post-index confusion in disas_ldst_tag.

---
 target/arm/translate-a64.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index a85bae1f27..5728b68ccc 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -3679,7 +3679,7 @@ static void disas_ldst_tag(DisasContext *s, uint32_t insn)
     }
 
     dirty_addr = read_cpu_reg_sp(s, rn, true);
-    if (index <= 0) {
+    if (index >= 0) {
         /* pre-index or signed offset */
         tcg_gen_addi_i64(dirty_addr, dirty_addr, offset);
     }
@@ -3726,7 +3726,7 @@ static void disas_ldst_tag(DisasContext *s, uint32_t insn)
 
     if (index != 0) {
         /* pre-index or post-index */
-        if (index > 0) {
+        if (index < 0) {
             /* post-index */
             tcg_gen_addi_i64(dirty_addr, dirty_addr, offset);
         }
-- 
2.23.0.700.g56cf767bdb-goog


[-- Attachment #3: 0003-Fix-wrong-field-size-in-disas_ldst_tag.patch --]
[-- Type: text/x-patch, Size: 944 bytes --]

From 1ecf4f7baedfe2de80a97c408fa2cc64ccd99dbe Mon Sep 17 00:00:00 2001
From: Evgenii Stepanov <eugenis@google.com>
Date: Tue, 27 Aug 2019 16:13:26 -0700
Subject: [PATCH 3/4] Fix wrong field size in disas_ldst_tag.

---
 target/arm/translate-a64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 0a168506d2..a85bae1f27 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -3611,7 +3611,7 @@ static void disas_ldst_tag(DisasContext *s, uint32_t insn)
     int rt = extract32(insn, 0, 5);
     int rn = extract32(insn, 5, 5);
     uint64_t offset = sextract64(insn, 12, 9) << LOG2_TAG_GRANULE;
-    int op2 = extract32(insn, 10, 3);
+    int op2 = extract32(insn, 10, 2);
     int op1 = extract32(insn, 22, 2);
     bool is_load = false, is_pair = false, is_zero = false, is_mult = false;
     int index = 0;
-- 
2.23.0.700.g56cf767bdb-goog


[-- Attachment #4: 0001-Fix-STGP-offset-scale.patch --]
[-- Type: text/x-patch, Size: 748 bytes --]

From a8a024202d1ba80142eacc09dab10c7780874582 Mon Sep 17 00:00:00 2001
From: Evgenii Stepanov <eugenis@google.com>
Date: Thu, 22 Aug 2019 18:05:34 -0700
Subject: [PATCH 1/4] Fix STGP offset scale.

---
 target/arm/translate-a64.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 9a554856e9..0a168506d2 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -2763,7 +2763,8 @@ static void disas_ldst_pair(DisasContext *s, uint32_t insn)
         return;
     }
 
-    offset <<= size;
+    // STGP offset is 16-scaled.
+    offset <<= (size + set_tag);
 
     if (rn == 31) {
         gen_check_sp_alignment(s);
-- 
2.23.0.700.g56cf767bdb-goog


[-- Attachment #5: 0002-Untag-userspace-addresses-in-syscall-emulation.patch --]
[-- Type: text/x-patch, Size: 2133 bytes --]

From bde3007cbe33ccbbba4648c7ee093534be08ccd0 Mon Sep 17 00:00:00 2001
From: Evgenii Stepanov <eugenis@google.com>
Date: Tue, 27 Aug 2019 16:12:38 -0700
Subject: [PATCH 2/4] Untag userspace addresses in syscall emulation.

---
 linux-user/qemu.h | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index aac0334627..a8f0a8eee9 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -456,8 +456,16 @@ extern unsigned long guest_stack_size;
 #define VERIFY_READ 0
 #define VERIFY_WRITE 1 /* implies read access */
 
+static inline abi_ulong untagged_addr(abi_ulong addr) {
+#if TARGET_ABI_BITS == 64
+    addr &= (((abi_ulong)-1) >> 8);
+#endif
+    return addr;
+}
+
 static inline int access_ok(int type, abi_ulong addr, abi_ulong size)
 {
+    addr = untagged_addr(addr);
     return guest_addr_valid(addr) &&
            (size == 0 || guest_addr_valid(addr + size - 1)) &&
            page_check_range((target_ulong)addr, size,
@@ -601,6 +609,7 @@ static inline void *lock_user(int type, abi_ulong guest_addr, long len, int copy
 {
     if (!access_ok(type, guest_addr, len))
         return NULL;
+    guest_addr = untagged_addr(guest_addr);
 #ifdef DEBUG_REMAP
     {
         void *addr;
@@ -642,7 +651,7 @@ abi_long target_strlen(abi_ulong gaddr);
 static inline void *lock_user_string(abi_ulong guest_addr)
 {
     abi_long len;
-    len = target_strlen(guest_addr);
+    len = target_strlen(untagged_addr(guest_addr));
     if (len < 0)
         return NULL;
     return lock_user(VERIFY_READ, guest_addr, (long)(len + 1), 1);
@@ -650,7 +659,7 @@ static inline void *lock_user_string(abi_ulong guest_addr)
 
 /* Helper macros for locking/unlocking a target struct.  */
 #define lock_user_struct(type, host_ptr, guest_addr, copy)	\
-    (host_ptr = lock_user(type, guest_addr, sizeof(*host_ptr), copy))
+    (host_ptr = lock_user(type, untagged_addr(guest_addr), sizeof(*host_ptr), copy))
 #define unlock_user_struct(host_ptr, guest_addr, copy)		\
     unlock_user(host_ptr, guest_addr, (copy) ? sizeof(*host_ptr) : 0)
 
-- 
2.23.0.700.g56cf767bdb-goog