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
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
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~
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~
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
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~
© 2016 - 2026 Red Hat, Inc.