* [PATCH 0/4] target/arm: Implement ARMv8.2-UAO
@ 2019-12-03 23:42 Richard Henderson
2019-12-03 23:42 ` [PATCH 1/4] target/arm: Add ID_AA64MMFR2_EL1 Richard Henderson
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Richard Henderson @ 2019-12-03 23:42 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell
Based-on: <20191203225333.17055-1-richard.henderson@linaro.org>
("target/arm: Implement ARMv8.1-PAN + ARMv8.2-ATS1E1")
This was relatively easy to do, and related to the VHE
and PAN patch sets. It's also already supported by the
Linux kernel and thus easily tested.
r~
Richard Henderson (4):
target/arm: Add ID_AA64MMFR2_EL1
target/arm: Update MSR access to UAO
target/arm: Implement UAO semantics
target/arm: Enable ARMv8.2-UAO in -cpu max
target/arm/cpu.h | 23 +++++++++++++
target/arm/cpu64.c | 4 +++
target/arm/helper.c | 66 +++++++++++++++++++++++++-------------
target/arm/kvm64.c | 2 ++
target/arm/translate-a64.c | 14 ++++++++
5 files changed, 87 insertions(+), 22 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/4] target/arm: Add ID_AA64MMFR2_EL1 2019-12-03 23:42 [PATCH 0/4] target/arm: Implement ARMv8.2-UAO Richard Henderson @ 2019-12-03 23:42 ` Richard Henderson 2019-12-06 18:19 ` Peter Maydell 2019-12-03 23:42 ` [PATCH 2/4] target/arm: Update MSR access to UAO Richard Henderson ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: Richard Henderson @ 2019-12-03 23:42 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell Add definitions for all of the fields, up to ARMv8.5. Convert the existing RESERVED register to a full register. Query KVM for the value of the register for the host. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/cpu.h | 17 +++++++++++++++++ target/arm/helper.c | 4 ++-- target/arm/kvm64.c | 2 ++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index d403dc5947..cdf6caf869 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -864,6 +864,7 @@ struct ARMCPU { uint64_t id_aa64pfr1; uint64_t id_aa64mmfr0; uint64_t id_aa64mmfr1; + uint64_t id_aa64mmfr2; } isar; uint32_t midr; uint32_t revidr; @@ -1778,6 +1779,22 @@ FIELD(ID_AA64MMFR1, PAN, 20, 4) FIELD(ID_AA64MMFR1, SPECSEI, 24, 4) FIELD(ID_AA64MMFR1, XNX, 28, 4) +FIELD(ID_AA64MMFR2, CNP, 0, 4) +FIELD(ID_AA64MMFR2, UAO, 4, 4) +FIELD(ID_AA64MMFR2, LSM, 8, 4) +FIELD(ID_AA64MMFR2, IESB, 12, 4) +FIELD(ID_AA64MMFR2, VARANGE, 16, 4) +FIELD(ID_AA64MMFR2, CCIDX, 20, 4) +FIELD(ID_AA64MMFR2, NV, 24, 4) +FIELD(ID_AA64MMFR2, ST, 28, 4) +FIELD(ID_AA64MMFR2, AT, 32, 4) +FIELD(ID_AA64MMFR2, IDS, 36, 4) +FIELD(ID_AA64MMFR2, FWB, 40, 4) +FIELD(ID_AA64MMFR2, TTL, 48, 4) +FIELD(ID_AA64MMFR2, BBM, 52, 4) +FIELD(ID_AA64MMFR2, EVT, 56, 4) +FIELD(ID_AA64MMFR2, E0PD, 60, 4) + FIELD(ID_DFR0, COPDBG, 0, 4) FIELD(ID_DFR0, COPSDBG, 4, 4) FIELD(ID_DFR0, MMAPDBG, 8, 4) diff --git a/target/arm/helper.c b/target/arm/helper.c index f1eab4fb28..70f2db5447 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -6825,11 +6825,11 @@ void register_cp_regs_for_features(ARMCPU *cpu) .access = PL1_R, .type = ARM_CP_CONST, .accessfn = access_aa64_tid3, .resetvalue = cpu->isar.id_aa64mmfr1 }, - { .name = "ID_AA64MMFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64, + { .name = "ID_AA64MMFR2_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 2, .access = PL1_R, .type = ARM_CP_CONST, .accessfn = access_aa64_tid3, - .resetvalue = 0 }, + .resetvalue = cpu->isar.id_aa64mmfr2 }, { .name = "ID_AA64MMFR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 3, .access = PL1_R, .type = ARM_CP_CONST, diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c index 876184b8fe..482e7fdfbb 100644 --- a/target/arm/kvm64.c +++ b/target/arm/kvm64.c @@ -549,6 +549,8 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) ARM64_SYS_REG(3, 0, 0, 7, 0)); err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64mmfr1, ARM64_SYS_REG(3, 0, 0, 7, 1)); + err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64mmfr2, + ARM64_SYS_REG(3, 0, 0, 7, 2)); /* * Note that if AArch32 support is not present in the host, -- 2.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] target/arm: Add ID_AA64MMFR2_EL1 2019-12-03 23:42 ` [PATCH 1/4] target/arm: Add ID_AA64MMFR2_EL1 Richard Henderson @ 2019-12-06 18:19 ` Peter Maydell 2020-02-02 0:54 ` Richard Henderson 0 siblings, 1 reply; 14+ messages in thread From: Peter Maydell @ 2019-12-06 18:19 UTC (permalink / raw) To: Richard Henderson; +Cc: QEMU Developers On Tue, 3 Dec 2019 at 23:42, Richard Henderson <richard.henderson@linaro.org> wrote: > > Add definitions for all of the fields, up to ARMv8.5. > Convert the existing RESERVED register to a full register. > Query KVM for the value of the register for the host. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/cpu.h | 17 +++++++++++++++++ > target/arm/helper.c | 4 ++-- > target/arm/kvm64.c | 2 ++ > 3 files changed, 21 insertions(+), 2 deletions(-) > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c > index 876184b8fe..482e7fdfbb 100644 > --- a/target/arm/kvm64.c > +++ b/target/arm/kvm64.c > @@ -549,6 +549,8 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) > ARM64_SYS_REG(3, 0, 0, 7, 0)); > err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64mmfr1, > ARM64_SYS_REG(3, 0, 0, 7, 1)); > + err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64mmfr2, > + ARM64_SYS_REG(3, 0, 0, 7, 2)); Do current KVM kernels definitely handle the request for this new register (ie don't return an error)? Otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] target/arm: Add ID_AA64MMFR2_EL1 2019-12-06 18:19 ` Peter Maydell @ 2020-02-02 0:54 ` Richard Henderson 0 siblings, 0 replies; 14+ messages in thread From: Richard Henderson @ 2020-02-02 0:54 UTC (permalink / raw) To: Peter Maydell; +Cc: QEMU Developers On 12/6/19 10:19 AM, Peter Maydell wrote: >> @@ -549,6 +549,8 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) >> ARM64_SYS_REG(3, 0, 0, 7, 0)); >> err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64mmfr1, >> ARM64_SYS_REG(3, 0, 0, 7, 1)); >> + err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64mmfr2, >> + ARM64_SYS_REG(3, 0, 0, 7, 2)); > > Do current KVM kernels definitely handle the request for this > new register (ie don't return an error)? Yes, ID_AA64MMFR2 was added to the sys_regs table in the same commit (93390c0a1b20b) as all of the others here. r~ ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/4] target/arm: Update MSR access to UAO 2019-12-03 23:42 [PATCH 0/4] target/arm: Implement ARMv8.2-UAO Richard Henderson 2019-12-03 23:42 ` [PATCH 1/4] target/arm: Add ID_AA64MMFR2_EL1 Richard Henderson @ 2019-12-03 23:42 ` Richard Henderson 2019-12-06 18:30 ` Peter Maydell 2019-12-03 23:42 ` [PATCH 3/4] target/arm: Implement UAO semantics Richard Henderson 2019-12-03 23:42 ` [PATCH 4/4] target/arm: Enable ARMv8.2-UAO in -cpu max Richard Henderson 3 siblings, 1 reply; 14+ messages in thread From: Richard Henderson @ 2019-12-03 23:42 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/cpu.h | 6 ++++++ target/arm/helper.c | 21 +++++++++++++++++++++ target/arm/translate-a64.c | 14 ++++++++++++++ 3 files changed, 41 insertions(+) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index cdf6caf869..dd284ba5c7 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -1228,6 +1228,7 @@ void pmu_init(ARMCPU *cpu); #define PSTATE_IL (1U << 20) #define PSTATE_SS (1U << 21) #define PSTATE_PAN (1U << 22) +#define PSTATE_UAO (1U << 23) #define PSTATE_V (1U << 28) #define PSTATE_C (1U << 29) #define PSTATE_Z (1U << 30) @@ -3598,6 +3599,11 @@ static inline bool isar_feature_aa64_ats1e1(const ARMISARegisters *id) return FIELD_EX64(id->id_aa64mmfr1, ID_AA64MMFR1, PAN) >= 2; } +static inline bool isar_feature_aa64_uao(const ARMISARegisters *id) +{ + return FIELD_EX64(id->id_aa64mmfr2, ID_AA64MMFR2, UAO) != 0; +} + static inline bool isar_feature_aa64_bti(const ARMISARegisters *id) { return FIELD_EX64(id->id_aa64pfr1, ID_AA64PFR1, BT) != 0; diff --git a/target/arm/helper.c b/target/arm/helper.c index 70f2db5447..8941a6c10f 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -4131,6 +4131,17 @@ static void aa64_pan_write(CPUARMState *env, const ARMCPRegInfo *ri, env->pstate = (env->pstate & ~PSTATE_PAN) | (value & PSTATE_PAN); } +static uint64_t aa64_uao_read(CPUARMState *env, const ARMCPRegInfo *ri) +{ + return env->pstate & PSTATE_UAO; +} + +static void aa64_uao_write(CPUARMState *env, const ARMCPRegInfo *ri, + uint64_t value) +{ + env->pstate = (env->pstate & ~PSTATE_UAO) | (value & PSTATE_UAO); +} + static CPAccessResult aa64_cacheop_access(CPUARMState *env, const ARMCPRegInfo *ri, bool isread) @@ -7464,6 +7475,16 @@ void register_cp_regs_for_features(ARMCPU *cpu) define_arm_cp_regs(cpu, ats1cp_reginfo); } #endif + if (cpu_isar_feature(aa64_uao, cpu)) { + static const ARMCPRegInfo uao_reginfo[] = { + { .name = "UAO", .state = ARM_CP_STATE_AA64, + .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 2, .opc2 = 4, + .type = ARM_CP_NO_RAW, .access = PL1_RW, + .readfn = aa64_uao_read, .writefn = aa64_uao_write, }, + REGINFO_SENTINEL + }; + define_arm_cp_regs(cpu, uao_reginfo); + } if (arm_feature(env, ARM_FEATURE_EL2) && cpu_isar_feature(aa64_vh, cpu)) { static const ARMCPRegInfo vhe_reginfo[] = { diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c index 7f5a68106b..2b6846ef01 100644 --- a/target/arm/translate-a64.c +++ b/target/arm/translate-a64.c @@ -1601,6 +1601,20 @@ static void handle_msr_i(DisasContext *s, uint32_t insn, s->base.is_jmp = DISAS_NEXT; break; + case 0x03: /* UAO */ + if (!dc_isar_feature(aa64_uao, s) || s->current_el == 0) { + goto do_unallocated; + } + if (crm & 1) { + set_pstate_bits(PSTATE_UAO); + } else { + clear_pstate_bits(PSTATE_UAO); + } + t1 = tcg_const_i32(s->current_el); + gen_helper_rebuild_hflags_a64(cpu_env, t1); + tcg_temp_free_i32(t1); + break; + case 0x04: /* PAN */ if (!dc_isar_feature(aa64_pan, s) || s->current_el == 0) { goto do_unallocated; -- 2.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] target/arm: Update MSR access to UAO 2019-12-03 23:42 ` [PATCH 2/4] target/arm: Update MSR access to UAO Richard Henderson @ 2019-12-06 18:30 ` Peter Maydell 2019-12-06 19:00 ` Richard Henderson 2020-02-02 1:00 ` Richard Henderson 0 siblings, 2 replies; 14+ messages in thread From: Peter Maydell @ 2019-12-06 18:30 UTC (permalink / raw) To: Richard Henderson; +Cc: QEMU Developers On Tue, 3 Dec 2019 at 23:42, Richard Henderson <richard.henderson@linaro.org> wrote: > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/cpu.h | 6 ++++++ > target/arm/helper.c | 21 +++++++++++++++++++++ > target/arm/translate-a64.c | 14 ++++++++++++++ > 3 files changed, 41 insertions(+) > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index cdf6caf869..dd284ba5c7 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -1228,6 +1228,7 @@ void pmu_init(ARMCPU *cpu); > #define PSTATE_IL (1U << 20) > #define PSTATE_SS (1U << 21) > #define PSTATE_PAN (1U << 22) > +#define PSTATE_UAO (1U << 23) > #define PSTATE_V (1U << 28) > #define PSTATE_C (1U << 29) > #define PSTATE_Z (1U << 30) > @@ -3598,6 +3599,11 @@ static inline bool isar_feature_aa64_ats1e1(const ARMISARegisters *id) > return FIELD_EX64(id->id_aa64mmfr1, ID_AA64MMFR1, PAN) >= 2; > } > > +static inline bool isar_feature_aa64_uao(const ARMISARegisters *id) > +{ > + return FIELD_EX64(id->id_aa64mmfr2, ID_AA64MMFR2, UAO) != 0; > +} > + > static inline bool isar_feature_aa64_bti(const ARMISARegisters *id) > { > return FIELD_EX64(id->id_aa64pfr1, ID_AA64PFR1, BT) != 0; > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 70f2db5447..8941a6c10f 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -4131,6 +4131,17 @@ static void aa64_pan_write(CPUARMState *env, const ARMCPRegInfo *ri, > env->pstate = (env->pstate & ~PSTATE_PAN) | (value & PSTATE_PAN); > } > > +static uint64_t aa64_uao_read(CPUARMState *env, const ARMCPRegInfo *ri) > +{ > + return env->pstate & PSTATE_UAO; > +} > + > +static void aa64_uao_write(CPUARMState *env, const ARMCPRegInfo *ri, > + uint64_t value) > +{ > + env->pstate = (env->pstate & ~PSTATE_UAO) | (value & PSTATE_UAO); > +} > + > static CPAccessResult aa64_cacheop_access(CPUARMState *env, > const ARMCPRegInfo *ri, > bool isread) > @@ -7464,6 +7475,16 @@ void register_cp_regs_for_features(ARMCPU *cpu) > define_arm_cp_regs(cpu, ats1cp_reginfo); > } > #endif > + if (cpu_isar_feature(aa64_uao, cpu)) { > + static const ARMCPRegInfo uao_reginfo[] = { > + { .name = "UAO", .state = ARM_CP_STATE_AA64, > + .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 2, .opc2 = 4, > + .type = ARM_CP_NO_RAW, .access = PL1_RW, > + .readfn = aa64_uao_read, .writefn = aa64_uao_write, }, > + REGINFO_SENTINEL > + }; This could just be a file-scope global, right? Also, you can just use define_one_arm_cp_reg() rather than having a list with one entry. (cf zcr_el1_reginfo). > + define_arm_cp_regs(cpu, uao_reginfo); > + } > > if (arm_feature(env, ARM_FEATURE_EL2) && cpu_isar_feature(aa64_vh, cpu)) { > static const ARMCPRegInfo vhe_reginfo[] = { > diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c > index 7f5a68106b..2b6846ef01 100644 > --- a/target/arm/translate-a64.c > +++ b/target/arm/translate-a64.c > @@ -1601,6 +1601,20 @@ static void handle_msr_i(DisasContext *s, uint32_t insn, > s->base.is_jmp = DISAS_NEXT; > break; > > + case 0x03: /* UAO */ > + if (!dc_isar_feature(aa64_uao, s) || s->current_el == 0) { > + goto do_unallocated; > + } > + if (crm & 1) { > + set_pstate_bits(PSTATE_UAO); > + } else { > + clear_pstate_bits(PSTATE_UAO); > + } > + t1 = tcg_const_i32(s->current_el); > + gen_helper_rebuild_hflags_a64(cpu_env, t1); > + tcg_temp_free_i32(t1); > + break; Do we also need to end the TB since we've messed with the hflags, or is some bit of code not in the patch context handling that? > + > case 0x04: /* PAN */ > if (!dc_isar_feature(aa64_pan, s) || s->current_el == 0) { > goto do_unallocated; > -- > 2.17.1 Does the "on exception entry PSTATE.UAO is zeroed" behaviour fall out automatically for us? How about "on exception entry from aarch32 to aarch64 SPSR_ELx.UAO is set to zero" ? I think we may also want a minor code change so that an exception return from aarch64 to aarch32 doesn't copy a bogus SPSR UAO==1 into the pstate/cpsr. thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] target/arm: Update MSR access to UAO 2019-12-06 18:30 ` Peter Maydell @ 2019-12-06 19:00 ` Richard Henderson 2020-02-02 1:00 ` Richard Henderson 1 sibling, 0 replies; 14+ messages in thread From: Richard Henderson @ 2019-12-06 19:00 UTC (permalink / raw) To: Peter Maydell; +Cc: QEMU Developers On 12/6/19 10:30 AM, Peter Maydell wrote: >> + if (cpu_isar_feature(aa64_uao, cpu)) { >> + static const ARMCPRegInfo uao_reginfo[] = { >> + { .name = "UAO", .state = ARM_CP_STATE_AA64, >> + .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 2, .opc2 = 4, >> + .type = ARM_CP_NO_RAW, .access = PL1_RW, >> + .readfn = aa64_uao_read, .writefn = aa64_uao_write, }, >> + REGINFO_SENTINEL >> + }; > > This could just be a file-scope global, right? > Also, you can just use define_one_arm_cp_reg() rather > than having a list with one entry. (cf zcr_el1_reginfo). Can do. >> + case 0x03: /* UAO */ >> + if (!dc_isar_feature(aa64_uao, s) || s->current_el == 0) { >> + goto do_unallocated; >> + } >> + if (crm & 1) { >> + set_pstate_bits(PSTATE_UAO); >> + } else { >> + clear_pstate_bits(PSTATE_UAO); >> + } >> + t1 = tcg_const_i32(s->current_el); >> + gen_helper_rebuild_hflags_a64(cpu_env, t1); >> + tcg_temp_free_i32(t1); >> + break; > > Do we also need to end the TB since we've messed with > the hflags, or is some bit of code not in the patch > context handling that? Outside the patch context, at the start of the function, we default to ending the TB. > Does the "on exception entry PSTATE.UAO is zeroed" behaviour > fall out automatically for us? How about "on exception entry > from aarch32 to aarch64 SPSR_ELx.UAO is set to zero" ? Yes to both -- new_mode (perhaps better renamed as new_pstate) is initialized as 0 + stuff required to be non-zero. Thus PAN required special handling but UAO does not. > I think we may also want a minor code change so that an exception > return from aarch64 to aarch32 doesn't copy a bogus SPSR UAO==1 > into the pstate/cpsr. Ah, good catch. r~ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] target/arm: Update MSR access to UAO 2019-12-06 18:30 ` Peter Maydell 2019-12-06 19:00 ` Richard Henderson @ 2020-02-02 1:00 ` Richard Henderson 2020-02-02 13:29 ` Peter Maydell 1 sibling, 1 reply; 14+ messages in thread From: Richard Henderson @ 2020-02-02 1:00 UTC (permalink / raw) To: Peter Maydell; +Cc: QEMU Developers On 12/6/19 10:30 AM, Peter Maydell wrote: >> + if (cpu_isar_feature(aa64_uao, cpu)) { >> + static const ARMCPRegInfo uao_reginfo[] = { >> + { .name = "UAO", .state = ARM_CP_STATE_AA64, >> + .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 2, .opc2 = 4, >> + .type = ARM_CP_NO_RAW, .access = PL1_RW, >> + .readfn = aa64_uao_read, .writefn = aa64_uao_write, }, >> + REGINFO_SENTINEL >> + }; > > This could just be a file-scope global, right? > Also, you can just use define_one_arm_cp_reg() rather > than having a list with one entry. (cf zcr_el1_reginfo). Done. >> + case 0x03: /* UAO */ >> + if (!dc_isar_feature(aa64_uao, s) || s->current_el == 0) { >> + goto do_unallocated; >> + } >> + if (crm & 1) { >> + set_pstate_bits(PSTATE_UAO); >> + } else { >> + clear_pstate_bits(PSTATE_UAO); >> + } >> + t1 = tcg_const_i32(s->current_el); >> + gen_helper_rebuild_hflags_a64(cpu_env, t1); >> + tcg_temp_free_i32(t1); >> + break; > > Do we also need to end the TB since we've messed with > the hflags, or is some bit of code not in the patch > context handling that? This is done by default. We would have to do something special to avoid ending the TB here. > Does the "on exception entry PSTATE.UAO is zeroed" behaviour > fall out automatically for us? Yes, aarch64_pstate_mode() returns a clean PSTATE. > How about "on exception entry > from aarch32 to aarch64 SPSR_ELx.UAO is set to zero" ? This follows the same path as above, as far as I can see. > I think we may also want a minor code change so that an exception > return from aarch64 to aarch32 doesn't copy a bogus SPSR UAO==1 > into the pstate/cpsr. Well, there is no CPSR UAO bit, so there's no aarch32 bit to clear. But I did add a clearing of PSTATE UAO on the exception return to aarch64 path, to prevent the guest from playing games with SPSR. r~ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] target/arm: Update MSR access to UAO 2020-02-02 1:00 ` Richard Henderson @ 2020-02-02 13:29 ` Peter Maydell 2020-02-03 7:46 ` Richard Henderson 0 siblings, 1 reply; 14+ messages in thread From: Peter Maydell @ 2020-02-02 13:29 UTC (permalink / raw) To: Richard Henderson; +Cc: QEMU Developers On Sun, 2 Feb 2020 at 01:00, Richard Henderson <richard.henderson@linaro.org> wrote: > > Does the "on exception entry PSTATE.UAO is zeroed" behaviour > > fall out automatically for us? > > Yes, aarch64_pstate_mode() returns a clean PSTATE. > > > How about "on exception entry > > from aarch32 to aarch64 SPSR_ELx.UAO is set to zero" ? > > This follows the same path as above, as far as I can see. Yes, but SPSR_ELx isn't started with a clean zero and built up the way the new PSTATE is, it gets copied from the AArch32 CPSR via cpsr_read(). I forget how carefully we keep the guest from setting CPSR bits that aren't really valid for the CPU... > > I think we may also want a minor code change so that an exception > > return from aarch64 to aarch32 doesn't copy a bogus SPSR UAO==1 > > into the pstate/cpsr. > > Well, there is no CPSR UAO bit, so there's no aarch32 bit to clear. But I did > add a clearing of PSTATE UAO on the exception return to aarch64 path, to > prevent the guest from playing games with SPSR. ...for instance on the aarch64->aarch32 exception return path, I don't think we sanitize the SPSR bits, so the guest could use a 64->32 exception return to set a bogus CPSR.UAO bit and then enter from 32 to 64 and see SPSR_ELx.UAO set when it should not be, unless we sanitize either in all places where we let the guest set CPSR bits (including 64->32 return), or we sanitize on 32->64 entry. thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] target/arm: Update MSR access to UAO 2020-02-02 13:29 ` Peter Maydell @ 2020-02-03 7:46 ` Richard Henderson 0 siblings, 0 replies; 14+ messages in thread From: Richard Henderson @ 2020-02-03 7:46 UTC (permalink / raw) To: Peter Maydell; +Cc: QEMU Developers On 2/2/20 1:29 PM, Peter Maydell wrote: > Yes, but SPSR_ELx isn't started with a clean zero and built up > the way the new PSTATE is, it gets copied from the AArch32 CPSR > via cpsr_read(). I forget how carefully we keep the guest from setting > CPSR bits that aren't really valid for the CPU... We do an ok job, except... >> Well, there is no CPSR UAO bit, so there's no aarch32 bit to clear. But I did >> add a clearing of PSTATE UAO on the exception return to aarch64 path, to >> prevent the guest from playing games with SPSR. > > ...for instance on the aarch64->aarch32 exception return path, ... here. > I don't think we sanitize the SPSR bits, so the guest could use > a 64->32 exception return to set a bogus CPSR.UAO bit and > then enter from 32 to 64 and see SPSR_ELx.UAO set when > it should not be, unless we sanitize either in all places where > we let the guest set CPSR bits (including 64->32 return), or > we sanitize on 32->64 entry. There is no CPSR.UAO bit, so this chain of logic doesn't hold for that specific instance. But plausibly so for CPSR.PAN. We do sanitize all of the places where CPSR/PSTATE is explicitly set. I think we've covered all but one of the exception return paths, sanitizing the SPSR_ELx values. We could move some of this logic to internals.h so that it could be shared between CPSR and exception return. I'll think about that for v3. r~ ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/4] target/arm: Implement UAO semantics 2019-12-03 23:42 [PATCH 0/4] target/arm: Implement ARMv8.2-UAO Richard Henderson 2019-12-03 23:42 ` [PATCH 1/4] target/arm: Add ID_AA64MMFR2_EL1 Richard Henderson 2019-12-03 23:42 ` [PATCH 2/4] target/arm: Update MSR access to UAO Richard Henderson @ 2019-12-03 23:42 ` Richard Henderson 2019-12-06 18:31 ` Peter Maydell 2019-12-03 23:42 ` [PATCH 4/4] target/arm: Enable ARMv8.2-UAO in -cpu max Richard Henderson 3 siblings, 1 reply; 14+ messages in thread From: Richard Henderson @ 2019-12-03 23:42 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell We need only override the current condition under which TBFLAG_A64.UNPRIV is set. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/helper.c | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/target/arm/helper.c b/target/arm/helper.c index 8941a6c10f..6d7a8349b5 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -12050,28 +12050,29 @@ static uint32_t rebuild_hflags_a64(CPUARMState *env, int el, int fp_el, } /* Compute the condition for using AccType_UNPRIV for LDTR et al. */ - /* TODO: ARMv8.2-UAO */ - switch (mmu_idx) { - case ARMMMUIdx_EL10_1: - case ARMMMUIdx_EL10_1_PAN: - case ARMMMUIdx_SE1: - case ARMMMUIdx_SE1_PAN: - /* TODO: ARMv8.3-NV */ - flags = FIELD_DP32(flags, TBFLAG_A64, UNPRIV, 1); - break; - case ARMMMUIdx_EL20_2: - case ARMMMUIdx_EL20_2_PAN: - /* TODO: ARMv8.4-SecEL2 */ - /* - * Note that EL20_2 is gated by HCR_EL2.E2H == 1, but EL20_0 is - * gated by HCR_EL2.<E2H,TGE> == '11', and so is LDTR. - */ - if (env->cp15.hcr_el2 & HCR_TGE) { + if (!(env->pstate & PSTATE_UAO)) { + switch (mmu_idx) { + case ARMMMUIdx_EL10_1: + case ARMMMUIdx_EL10_1_PAN: + case ARMMMUIdx_SE1: + case ARMMMUIdx_SE1_PAN: + /* TODO: ARMv8.3-NV */ flags = FIELD_DP32(flags, TBFLAG_A64, UNPRIV, 1); + break; + case ARMMMUIdx_EL20_2: + case ARMMMUIdx_EL20_2_PAN: + /* TODO: ARMv8.4-SecEL2 */ + /* + * Note that EL20_2 is gated by HCR_EL2.E2H == 1, but EL20_0 is + * gated by HCR_EL2.<E2H,TGE> == '11', and so is LDTR. + */ + if (env->cp15.hcr_el2 & HCR_TGE) { + flags = FIELD_DP32(flags, TBFLAG_A64, UNPRIV, 1); + } + break; + default: + break; } - break; - default: - break; } return rebuild_hflags_common(env, fp_el, mmu_idx, flags); -- 2.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] target/arm: Implement UAO semantics 2019-12-03 23:42 ` [PATCH 3/4] target/arm: Implement UAO semantics Richard Henderson @ 2019-12-06 18:31 ` Peter Maydell 0 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2019-12-06 18:31 UTC (permalink / raw) To: Richard Henderson; +Cc: QEMU Developers On Tue, 3 Dec 2019 at 23:42, Richard Henderson <richard.henderson@linaro.org> wrote: > > We need only override the current condition under which > TBFLAG_A64.UNPRIV is set. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/helper.c | 41 +++++++++++++++++++++-------------------- > 1 file changed, 21 insertions(+), 20 deletions(-) Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/4] target/arm: Enable ARMv8.2-UAO in -cpu max 2019-12-03 23:42 [PATCH 0/4] target/arm: Implement ARMv8.2-UAO Richard Henderson ` (2 preceding siblings ...) 2019-12-03 23:42 ` [PATCH 3/4] target/arm: Implement UAO semantics Richard Henderson @ 2019-12-03 23:42 ` Richard Henderson 2019-12-06 18:31 ` Peter Maydell 3 siblings, 1 reply; 14+ messages in thread From: Richard Henderson @ 2019-12-03 23:42 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/cpu64.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c index 9399253b4c..03377084e3 100644 --- a/target/arm/cpu64.c +++ b/target/arm/cpu64.c @@ -674,6 +674,10 @@ static void aarch64_max_initfn(Object *obj) t = FIELD_DP64(t, ID_AA64MMFR1, PAN, 2); /* ATS1E1 */ cpu->isar.id_aa64mmfr1 = t; + t = cpu->isar.id_aa64mmfr2; + t = FIELD_DP64(t, ID_AA64MMFR2, UAO, 1); + cpu->isar.id_aa64mmfr2 = t; + /* Replicate the same data to the 32-bit id registers. */ u = cpu->isar.id_isar5; u = FIELD_DP32(u, ID_ISAR5, AES, 2); /* AES + PMULL */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] target/arm: Enable ARMv8.2-UAO in -cpu max 2019-12-03 23:42 ` [PATCH 4/4] target/arm: Enable ARMv8.2-UAO in -cpu max Richard Henderson @ 2019-12-06 18:31 ` Peter Maydell 0 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2019-12-06 18:31 UTC (permalink / raw) To: Richard Henderson; +Cc: QEMU Developers On Tue, 3 Dec 2019 at 23:42, Richard Henderson <richard.henderson@linaro.org> wrote: > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/cpu64.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c > index 9399253b4c..03377084e3 100644 > --- a/target/arm/cpu64.c > +++ b/target/arm/cpu64.c > @@ -674,6 +674,10 @@ static void aarch64_max_initfn(Object *obj) > t = FIELD_DP64(t, ID_AA64MMFR1, PAN, 2); /* ATS1E1 */ > cpu->isar.id_aa64mmfr1 = t; > > + t = cpu->isar.id_aa64mmfr2; > + t = FIELD_DP64(t, ID_AA64MMFR2, UAO, 1); > + cpu->isar.id_aa64mmfr2 = t; > + > /* Replicate the same data to the 32-bit id registers. */ > u = cpu->isar.id_isar5; > u = FIELD_DP32(u, ID_ISAR5, AES, 2); /* AES + PMULL */ > -- > 2.17.1 Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-02-03 7:47 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-03 23:42 [PATCH 0/4] target/arm: Implement ARMv8.2-UAO Richard Henderson 2019-12-03 23:42 ` [PATCH 1/4] target/arm: Add ID_AA64MMFR2_EL1 Richard Henderson 2019-12-06 18:19 ` Peter Maydell 2020-02-02 0:54 ` Richard Henderson 2019-12-03 23:42 ` [PATCH 2/4] target/arm: Update MSR access to UAO Richard Henderson 2019-12-06 18:30 ` Peter Maydell 2019-12-06 19:00 ` Richard Henderson 2020-02-02 1:00 ` Richard Henderson 2020-02-02 13:29 ` Peter Maydell 2020-02-03 7:46 ` Richard Henderson 2019-12-03 23:42 ` [PATCH 3/4] target/arm: Implement UAO semantics Richard Henderson 2019-12-06 18:31 ` Peter Maydell 2019-12-03 23:42 ` [PATCH 4/4] target/arm: Enable ARMv8.2-UAO in -cpu max Richard Henderson 2019-12-06 18:31 ` Peter Maydell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.