* [PATCH v1 0/1] target/arm: Remove access_el3_aa32ns() @ 2020-04-28 16:03 Edgar E. Iglesias 2020-04-28 16:03 ` [PATCH v1 1/1] target/arm: Drop access_el3_aa32ns() Edgar E. Iglesias 0 siblings, 1 reply; 4+ messages in thread From: Edgar E. Iglesias @ 2020-04-28 16:03 UTC (permalink / raw) To: qemu-devel Cc: laurent.desnogues, peter.maydell, qemu-arm, richard.henderson, edgar.iglesias From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> Hi, Laurent reported hitting the assert in access_el3_aa32ns() when accessing 32-bit versions of some of the virtualization regs when EL3 is 64-bit. I think we got this wrong back then and it seems to me like we should remove direct usage of access_el3_aa32ns() and always call access_el3_aa32ns_aa64_any() to handle both the aa32-only cases and the mixed aa32/aa64. Cheers, Edgar Edgar E. Iglesias (1): target/arm: Drop access_el3_aa32ns() target/arm/helper.c | 34 ++++++++++------------------------ 1 file changed, 10 insertions(+), 24 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v1 1/1] target/arm: Drop access_el3_aa32ns() 2020-04-28 16:03 [PATCH v1 0/1] target/arm: Remove access_el3_aa32ns() Edgar E. Iglesias @ 2020-04-28 16:03 ` Edgar E. Iglesias 2020-05-04 11:01 ` Peter Maydell 0 siblings, 1 reply; 4+ messages in thread From: Edgar E. Iglesias @ 2020-04-28 16:03 UTC (permalink / raw) To: qemu-devel Cc: laurent.desnogues, peter.maydell, qemu-arm, richard.henderson, edgar.iglesias From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> Calling access_el3_aa32ns() works for AArch32 only cores but it does not handle 32-bit EL2 on top of 64-bit EL3 for mixed 32/64-bit cores. Fold access_el3_aa32ns() into access_el3_aa32ns_aa64any() and replace all direct uses of the aa32 only version with access_el3_aa32ns_aa64any(). Fixes: 68e9c2fe65 ("target-arm: Add VTCR_EL2") Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> --- target/arm/helper.c | 34 ++++++++++------------------------ 1 file changed, 10 insertions(+), 24 deletions(-) diff --git a/target/arm/helper.c b/target/arm/helper.c index 7e9ea5d20f..888f5f2314 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -504,29 +504,15 @@ void init_cpreg_list(ARMCPU *cpu) /* * Some registers are not accessible if EL3.NS=0 and EL3 is using AArch32 but * they are accessible when EL3 is using AArch64 regardless of EL3.NS. - * - * access_el3_aa32ns: Used to check AArch32 register views. - * access_el3_aa32ns_aa64any: Used to check both AArch32/64 register views. */ -static CPAccessResult access_el3_aa32ns(CPUARMState *env, - const ARMCPRegInfo *ri, - bool isread) -{ - bool secure = arm_is_secure_below_el3(env); - - assert(!arm_el_is_aa64(env, 3)); - if (secure) { - return CP_ACCESS_TRAP_UNCATEGORIZED; - } - return CP_ACCESS_OK; -} - static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env, const ARMCPRegInfo *ri, bool isread) { - if (!arm_el_is_aa64(env, 3)) { - return access_el3_aa32ns(env, ri, isread); + bool secure = arm_is_secure_below_el3(env); + + if (!arm_el_is_aa64(env, 3) && secure) { + return CP_ACCESS_TRAP_UNCATEGORIZED; } return CP_ACCESS_OK; } @@ -5223,7 +5209,7 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = { .type = ARM_CP_CONST, .resetvalue = 0 }, { .name = "VTTBR", .state = ARM_CP_STATE_AA32, .cp = 15, .opc1 = 6, .crm = 2, - .access = PL2_RW, .accessfn = access_el3_aa32ns, + .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any, .type = ARM_CP_CONST | ARM_CP_64BIT, .resetvalue = 0 }, { .name = "VTTBR_EL2", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 4, .crn = 2, .crm = 1, .opc2 = 0, @@ -5556,7 +5542,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = { { .name = "VTCR", .state = ARM_CP_STATE_AA32, .cp = 15, .opc1 = 4, .crn = 2, .crm = 1, .opc2 = 2, .type = ARM_CP_ALIAS, - .access = PL2_RW, .accessfn = access_el3_aa32ns, + .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any, .fieldoffset = offsetof(CPUARMState, cp15.vtcr_el2) }, { .name = "VTCR_EL2", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 4, .crn = 2, .crm = 1, .opc2 = 2, @@ -5568,7 +5554,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = { { .name = "VTTBR", .state = ARM_CP_STATE_AA32, .cp = 15, .opc1 = 6, .crm = 2, .type = ARM_CP_64BIT | ARM_CP_ALIAS, - .access = PL2_RW, .accessfn = access_el3_aa32ns, + .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any, .fieldoffset = offsetof(CPUARMState, cp15.vttbr_el2), .writefn = vttbr_write }, { .name = "VTTBR_EL2", .state = ARM_CP_STATE_AA64, @@ -5708,7 +5694,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = { .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el2), }, { .name = "HPFAR", .state = ARM_CP_STATE_AA32, .cp = 15, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4, - .access = PL2_RW, .accessfn = access_el3_aa32ns, + .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any, .fieldoffset = offsetof(CPUARMState, cp15.hpfar_el2) }, { .name = "HPFAR_EL2", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4, @@ -7565,7 +7551,7 @@ void register_cp_regs_for_features(ARMCPU *cpu) ARMCPRegInfo vpidr_regs[] = { { .name = "VPIDR", .state = ARM_CP_STATE_AA32, .cp = 15, .opc1 = 4, .crn = 0, .crm = 0, .opc2 = 0, - .access = PL2_RW, .accessfn = access_el3_aa32ns, + .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any, .resetvalue = cpu->midr, .type = ARM_CP_ALIAS, .fieldoffset = offsetoflow32(CPUARMState, cp15.vpidr_el2) }, { .name = "VPIDR_EL2", .state = ARM_CP_STATE_AA64, @@ -7574,7 +7560,7 @@ void register_cp_regs_for_features(ARMCPU *cpu) .fieldoffset = offsetof(CPUARMState, cp15.vpidr_el2) }, { .name = "VMPIDR", .state = ARM_CP_STATE_AA32, .cp = 15, .opc1 = 4, .crn = 0, .crm = 0, .opc2 = 5, - .access = PL2_RW, .accessfn = access_el3_aa32ns, + .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any, .resetvalue = vmpidr_def, .type = ARM_CP_ALIAS, .fieldoffset = offsetoflow32(CPUARMState, cp15.vmpidr_el2) }, { .name = "VMPIDR_EL2", .state = ARM_CP_STATE_AA64, -- 2.20.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v1 1/1] target/arm: Drop access_el3_aa32ns() 2020-04-28 16:03 ` [PATCH v1 1/1] target/arm: Drop access_el3_aa32ns() Edgar E. Iglesias @ 2020-05-04 11:01 ` Peter Maydell 2020-05-04 14:18 ` Edgar E. Iglesias 0 siblings, 1 reply; 4+ messages in thread From: Peter Maydell @ 2020-05-04 11:01 UTC (permalink / raw) To: Edgar E. Iglesias Cc: Laurent Desnogues, Edgar Iglesias, qemu-arm, Richard Henderson, QEMU Developers On Tue, 28 Apr 2020 at 17:03, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > Calling access_el3_aa32ns() works for AArch32 only cores > but it does not handle 32-bit EL2 on top of 64-bit EL3 > for mixed 32/64-bit cores. > > Fold access_el3_aa32ns() into access_el3_aa32ns_aa64any() > and replace all direct uses of the aa32 only version with > access_el3_aa32ns_aa64any(). > > Fixes: 68e9c2fe65 ("target-arm: Add VTCR_EL2") > Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> So, this is definitely a bug, but I think we could be clearer about what we're fixing. For all these registers, the way the Arm ARM pseudocode phrases this access check is: * for the AArch64 view of the register, no check * for the AArch32 view of the register: ... elsif PSTATE.EL == EL2 then return VTTBR; elsif PSTATE.EL == EL3 then if SCR.NS == '0' then UNDEFINED; else return VTTBR; (similarly for the write path). We don't implement the HSTR.T2 traps, so for us these registers are all .access = PL2_RW and we just UNDEF for all EL0/EL1 accesses. So what we're really trying to check for is "current EL is EL3 and we are AArch32 and SCR.NS == '0'". Because it's not possible to be in AArch32 Hyp with SCR.NS == 0, the check we make in your function is an equivalent test, but we could improve the comments: > --- > target/arm/helper.c | 34 ++++++++++------------------------ > 1 file changed, 10 insertions(+), 24 deletions(-) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 7e9ea5d20f..888f5f2314 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -504,29 +504,15 @@ void init_cpreg_list(ARMCPU *cpu) > /* > * Some registers are not accessible if EL3.NS=0 and EL3 is using AArch32 but > * they are accessible when EL3 is using AArch64 regardless of EL3.NS. This could be rewritten as: Some registers are not accessible from AArch32 EL3 if SCR.NS == 0. > - * > - * access_el3_aa32ns: Used to check AArch32 register views. > - * access_el3_aa32ns_aa64any: Used to check both AArch32/64 register views. > */ > -static CPAccessResult access_el3_aa32ns(CPUARMState *env, > - const ARMCPRegInfo *ri, > - bool isread) > -{ > - bool secure = arm_is_secure_below_el3(env); > - > - assert(!arm_el_is_aa64(env, 3)); > - if (secure) { > - return CP_ACCESS_TRAP_UNCATEGORIZED; > - } > - return CP_ACCESS_OK; > -} > - > static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env, > const ARMCPRegInfo *ri, > bool isread) > { > - if (!arm_el_is_aa64(env, 3)) { > - return access_el3_aa32ns(env, ri, isread); > + bool secure = arm_is_secure_below_el3(env); > + > + if (!arm_el_is_aa64(env, 3) && secure) { We could either rephrase this as if (!is_a64(env) && arm_current_el(env) == 3 && arm_is_secure_below_el3(env)) { or just have a comment /* * This access function is only used with .access = PL2_RW * registers, so we are in AArch32 EL3 with SCR.NS == 0 * if and only if EL3 is AArch32 and SCR.NS == 0, because * if SCR.NS == 0 we cannot be in EL2. */ depending on how much you proritize a more efficient test over a more clearly correct test :-) > + return CP_ACCESS_TRAP_UNCATEGORIZED; > } > return CP_ACCESS_OK; > } Also, once we don't have a distinction between two different flavours of this access function we should use the simpler "access_el2_aa32ns", rather than ending up using the longer name for the one version of the function we're keeping. thanks -- PMM ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1 1/1] target/arm: Drop access_el3_aa32ns() 2020-05-04 11:01 ` Peter Maydell @ 2020-05-04 14:18 ` Edgar E. Iglesias 0 siblings, 0 replies; 4+ messages in thread From: Edgar E. Iglesias @ 2020-05-04 14:18 UTC (permalink / raw) To: Peter Maydell Cc: Laurent Desnogues, Edgar E. Iglesias, qemu-arm, Richard Henderson, QEMU Developers On Mon, May 04, 2020 at 12:01:07PM +0100, Peter Maydell wrote: > On Tue, 28 Apr 2020 at 17:03, Edgar E. Iglesias > <edgar.iglesias@gmail.com> wrote: > > > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > > Calling access_el3_aa32ns() works for AArch32 only cores > > but it does not handle 32-bit EL2 on top of 64-bit EL3 > > for mixed 32/64-bit cores. > > > > Fold access_el3_aa32ns() into access_el3_aa32ns_aa64any() > > and replace all direct uses of the aa32 only version with > > access_el3_aa32ns_aa64any(). > > > > Fixes: 68e9c2fe65 ("target-arm: Add VTCR_EL2") > > Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com> > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > So, this is definitely a bug, but I think we could be > clearer about what we're fixing. > > For all these registers, the way the Arm ARM pseudocode phrases > this access check is: > * for the AArch64 view of the register, no check > * for the AArch32 view of the register: > ... > elsif PSTATE.EL == EL2 then > return VTTBR; > elsif PSTATE.EL == EL3 then > if SCR.NS == '0' then > UNDEFINED; > else > return VTTBR; > (similarly for the write path). We don't implement the HSTR.T2 > traps, so for us these registers are all .access = PL2_RW and > we just UNDEF for all EL0/EL1 accesses. > > So what we're really trying to check for is "current EL is EL3 > and we are AArch32 and SCR.NS == '0'". Because it's not possible > to be in AArch32 Hyp with SCR.NS == 0, the check we make in > your function is an equivalent test, but we could improve > the comments: > > --- > > target/arm/helper.c | 34 ++++++++++------------------------ > > 1 file changed, 10 insertions(+), 24 deletions(-) > > > > diff --git a/target/arm/helper.c b/target/arm/helper.c > > index 7e9ea5d20f..888f5f2314 100644 > > --- a/target/arm/helper.c > > +++ b/target/arm/helper.c > > @@ -504,29 +504,15 @@ void init_cpreg_list(ARMCPU *cpu) > > /* > > * Some registers are not accessible if EL3.NS=0 and EL3 is using AArch32 but > > * they are accessible when EL3 is using AArch64 regardless of EL3.NS. > > This could be rewritten as: > Some registers are not accessible from AArch32 EL3 if SCR.NS == 0. Done in v2. > > > - * > > - * access_el3_aa32ns: Used to check AArch32 register views. > > - * access_el3_aa32ns_aa64any: Used to check both AArch32/64 register views. > > */ > > -static CPAccessResult access_el3_aa32ns(CPUARMState *env, > > - const ARMCPRegInfo *ri, > > - bool isread) > > -{ > > - bool secure = arm_is_secure_below_el3(env); > > - > > - assert(!arm_el_is_aa64(env, 3)); > > - if (secure) { > > - return CP_ACCESS_TRAP_UNCATEGORIZED; > > - } > > - return CP_ACCESS_OK; > > -} > > - > > static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env, > > const ARMCPRegInfo *ri, > > bool isread) > > { > > - if (!arm_el_is_aa64(env, 3)) { > > - return access_el3_aa32ns(env, ri, isread); > > + bool secure = arm_is_secure_below_el3(env); > > + > > + if (!arm_el_is_aa64(env, 3) && secure) { > > We could either rephrase this as > if (!is_a64(env) && arm_current_el(env) == 3 && > arm_is_secure_below_el3(env)) { Went for this logic in v2. > > or just have a comment > /* > * This access function is only used with .access = PL2_RW > * registers, so we are in AArch32 EL3 with SCR.NS == 0 > * if and only if EL3 is AArch32 and SCR.NS == 0, because > * if SCR.NS == 0 we cannot be in EL2. > */ > > depending on how much you proritize a more efficient test > over a more clearly correct test :-) > > > + return CP_ACCESS_TRAP_UNCATEGORIZED; > > } > > return CP_ACCESS_OK; > > } > > Also, once we don't have a distinction between two different > flavours of this access function we should use the simpler > "access_el2_aa32ns", rather than ending up using the longer > name for the one version of the function we're keeping. Done in v2. Thanks, Edgar ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-05-04 14:24 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-28 16:03 [PATCH v1 0/1] target/arm: Remove access_el3_aa32ns() Edgar E. Iglesias 2020-04-28 16:03 ` [PATCH v1 1/1] target/arm: Drop access_el3_aa32ns() Edgar E. Iglesias 2020-05-04 11:01 ` Peter Maydell 2020-05-04 14:18 ` Edgar E. Iglesias
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.