Re: [PATCH v5 03/22] target/arm: Add MTE system registers

2019-12-06 Thread Richard Henderson 
On 12/3/19 3:48 AM, Peter Maydell wrote:
>> +{ .name = "GMID_EL1", .state = ARM_CP_STATE_AA64,
>> +  .opc0 = 3, .opc1 = 1, .crn = 0, .crm = 0, .opc2 = 4,
>> +  .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = GMID_EL1_BS },
> 
> This should trap if HCR_EL2.TID5 is 1 (since we're adding
> support for the TID* ID reg trap bits now).

Done.

> So, aa64_mte_insn_reg here is checking for ID_AA64PFR1_EL1 != 0
> ("instructions accessible at EL0 are implemented")
> and aa64_mte is checking for >= 2 ("full implementation").
> I think a couple of brief comments would clarify:

Done.

> (The other way to arrange this would be to have the 'real'
> TCO regdef in mte_reginfo, and separately have "reginfo
> if we only have the dummy visible-from-EL0-parts-only
> which defines a constant 0 TCO" (and also make the MSR_i
> code implement a RAZ/WI for this case, for consistency).

Done.  I agree this is a better way to treat the EL0-only case...

> An implementation that allows the guest to toggle the PSTATE.TCO
> bit to no visible effect is architecturally valid, though.

... because this could turn out to be slightly confusing.


r~

Re: [PATCH v5 03/22] target/arm: Add MTE system registers

2019-12-03 Thread Peter Maydell 
On Fri, 11 Oct 2019 at 14:48, Richard Henderson
 wrote:
>
> This is TFSRE0_EL1, TFSR_EL1, TFSR_EL2, TFSR_EL3,
> RGSR_EL1, GCR_EL1, GMID_EL1, and PSTATE.TCO.
>
> Signed-off-by: Richard Henderson 
> ---
> v3: Add GMID; add access_mte.
> v4: Define only TCO at mte_insn_reg.
> ---
>  target/arm/cpu.h   |  3 ++
>  target/arm/internals.h |  6 
>  target/arm/helper.c| 73 ++
>  target/arm/translate-a64.c | 11 ++
>  4 files changed, 93 insertions(+)


> +{ .name = "GMID_EL1", .state = ARM_CP_STATE_AA64,
> +  .opc0 = 3, .opc1 = 1, .crn = 0, .crm = 0, .opc2 = 4,
> +  .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = GMID_EL1_BS },

This should trap if HCR_EL2.TID5 is 1 (since we're adding
support for the TID* ID reg trap bits now).

> +REGINFO_SENTINEL
> +};
> +
> +static const ARMCPRegInfo mte_tco_reginfo[] = {
> +{ .name = "TCO", .state = ARM_CP_STATE_AA64,
> +  .opc0 = 3, .opc1 = 3, .crn = 4, .crm = 2, .opc2 = 7,
> +  .type = ARM_CP_NO_RAW,
> +  .access = PL0_RW, .readfn = tco_read, .writefn = tco_write },
> +REGINFO_SENTINEL
> +};
>  #endif
>
>  static CPAccessResult access_predinv(CPUARMState *env, const ARMCPRegInfo 
> *ri,
> @@ -6881,6 +6948,12 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>  if (cpu_isar_feature(aa64_rndr, cpu)) {
>  define_arm_cp_regs(cpu, rndr_reginfo);
>  }

So, aa64_mte_insn_reg here is checking for ID_AA64PFR1_EL1 != 0
("instructions accessible at EL0 are implemented")
and aa64_mte is checking for >= 2 ("full implementation").
I think a couple of brief comments would clarify:

> +if (cpu_isar_feature(aa64_mte_insn_reg, cpu)) {
   /* EL0-visible MTE registers, present even for dummy
implementation */
> +define_arm_cp_regs(cpu, mte_tco_reginfo);
> +}
> +if (cpu_isar_feature(aa64_mte, cpu)) {
   /* MTE registers present for a full implementation */
> +define_arm_cp_regs(cpu, mte_reginfo);
> +}

(The other way to arrange this would be to have the 'real'
TCO regdef in mte_reginfo, and separately have "reginfo
if we only have the dummy visible-from-EL0-parts-only
which defines a constant 0 TCO" (and also make the MSR_i
code implement a RAZ/WI for this case, for consistency).
An implementation that allows the guest to toggle the PSTATE.TCO
bit to no visible effect is architecturally valid, though.)

>  #endif
>
>  /*
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index c85db69db4..62bdf50796 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -1611,6 +1611,17 @@ static void handle_msr_i(DisasContext *s, uint32_t 
> insn,
>  s->base.is_jmp = DISAS_UPDATE;
>  break;
>
> +case 0x1c: /* TCO */
> +if (!dc_isar_feature(aa64_mte_insn_reg, s)) {
> +goto do_unallocated;
> +}
> +if (crm & 1) {
> +set_pstate_bits(PSTATE_TCO);
> +} else {
> +clear_pstate_bits(PSTATE_TCO);
> +}
> +break;
> +
>  default:
>  do_unallocated:
>  unallocated_encoding(s);
> --
> 2.17.1

Otherwise
Reviewed-by: Peter Maydell 


thanks
-- PMM

[PATCH v5 03/22] target/arm: Add MTE system registers

2019-10-11 Thread Richard Henderson 
This is TFSRE0_EL1, TFSR_EL1, TFSR_EL2, TFSR_EL3,
RGSR_EL1, GCR_EL1, GMID_EL1, and PSTATE.TCO.

Signed-off-by: Richard Henderson 
---
v3: Add GMID; add access_mte.
v4: Define only TCO at mte_insn_reg.
---
 target/arm/cpu.h   |  3 ++
 target/arm/internals.h |  6 
 target/arm/helper.c| 73 ++
 target/arm/translate-a64.c | 11 ++
 4 files changed, 93 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 408d749b7a..d99bb5e956 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -486,6 +486,9 @@ typedef struct CPUARMState {
 uint64_t pmccfiltr_el0; /* Performance Monitor Filter Register */
 uint64_t vpidr_el2; /* Virtualization Processor ID Register */
 uint64_t vmpidr_el2; /* Virtualization Multiprocessor ID Register */
+uint64_t tfsr_el[4]; /* tfsrel0_el1 is index 0.  */
+uint64_t gcr_el1;
+uint64_t rgsr_el1;
 } cp15;
 
 struct {
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 9486680b87..bfa243be06 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1079,4 +1079,10 @@ void arm_log_exception(int idx);
 
 #endif /* !CONFIG_USER_ONLY */
 
+/*
+ * The log2 of the words in the tag block, for GMID_EL1.BS.
+ * The is the maximum, 256 bytes, which manipulates 64-bits of tags.
+ */
+#define GMID_EL1_BS  6
+
 #endif
diff --git a/target/arm/helper.c b/target/arm/helper.c
index f9dee51ede..f435a8d8bd 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5921,6 +5921,73 @@ static const ARMCPRegInfo rndr_reginfo[] = {
   .access = PL0_R, .readfn = rndr_readfn },
 REGINFO_SENTINEL
 };
+
+static CPAccessResult access_mte(CPUARMState *env, const ARMCPRegInfo *ri,
+ bool isread)
+{
+int el = arm_current_el(env);
+
+if (el < 2 &&
+arm_feature(env, ARM_FEATURE_EL2) &&
+!(arm_hcr_el2_eff(env) & HCR_ATA)) {
+return CP_ACCESS_TRAP_EL2;
+}
+if (el < 3 &&
+arm_feature(env, ARM_FEATURE_EL3) &&
+!(env->cp15.scr_el3 & SCR_ATA)) {
+return CP_ACCESS_TRAP_EL3;
+}
+return CP_ACCESS_OK;
+}
+
+static uint64_t tco_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+return env->pstate & PSTATE_TCO;
+}
+
+static void tco_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t val)
+{
+env->pstate = (env->pstate & ~PSTATE_TCO) | (val & PSTATE_TCO);
+}
+
+static const ARMCPRegInfo mte_reginfo[] = {
+{ .name = "TFSRE0_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 0, .crn = 6, .crm = 6, .opc2 = 1,
+  .access = PL1_RW, .accessfn = access_mte,
+  .fieldoffset = offsetof(CPUARMState, cp15.tfsr_el[0]) },
+{ .name = "TFSR_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 0, .crn = 6, .crm = 5, .opc2 = 0,
+  .access = PL1_RW, .accessfn = access_mte,
+  .fieldoffset = offsetof(CPUARMState, cp15.tfsr_el[1]) },
+{ .name = "TFSR_EL2", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 5, .opc2 = 0,
+  .access = PL2_RW, .accessfn = access_mte,
+  .fieldoffset = offsetof(CPUARMState, cp15.tfsr_el[2]) },
+{ .name = "TFSR_EL3", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 6, .crn = 6, .crm = 6, .opc2 = 0,
+  .access = PL3_RW,
+  .fieldoffset = offsetof(CPUARMState, cp15.tfsr_el[3]) },
+{ .name = "RGSR_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 0, .crn = 1, .crm = 0, .opc2 = 5,
+  .access = PL1_RW, .accessfn = access_mte,
+  .fieldoffset = offsetof(CPUARMState, cp15.rgsr_el1) },
+{ .name = "GCR_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 0, .crn = 1, .crm = 0, .opc2 = 6,
+  .access = PL1_RW, .accessfn = access_mte,
+  .fieldoffset = offsetof(CPUARMState, cp15.gcr_el1) },
+{ .name = "GMID_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 1, .crn = 0, .crm = 0, .opc2 = 4,
+  .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = GMID_EL1_BS },
+REGINFO_SENTINEL
+};
+
+static const ARMCPRegInfo mte_tco_reginfo[] = {
+{ .name = "TCO", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 3, .crn = 4, .crm = 2, .opc2 = 7,
+  .type = ARM_CP_NO_RAW,
+  .access = PL0_RW, .readfn = tco_read, .writefn = tco_write },
+REGINFO_SENTINEL
+};
 #endif
 
 static CPAccessResult access_predinv(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -6881,6 +6948,12 @@ void register_cp_regs_for_features(ARMCPU *cpu)
 if (cpu_isar_feature(aa64_rndr, cpu)) {
 define_arm_cp_regs(cpu, rndr_reginfo);
 }
+if (cpu_isar_feature(aa64_mte_insn_reg, cpu)) {
+define_arm_cp_regs(cpu, mte_tco_reginfo);
+}
+if (cpu_isar_feature(aa64_mte, cpu)) {
+define_arm_cp_regs(cpu, mte_reginfo);
+}
 #endif
 
 /*
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index c85db69db4..62bdf50796 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.