| Message ID | 1386060535-15908-19-git-send-email-s.fedorov@samsung.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote: > Banked coprocessor registers are switched on two cases: > 1) Entering or leaving CPU monitor mode with SCR.NS bit set; > 2) Setting SCR.NS bit not from CPU monitor mode > > Coprocessor register banking is done similar to CPU core register > banking. Some of SCTRL bits are common for secure and non-secure state. > Translation table base masks are updated on register switch instead > of TTBCR write. > So I was rather confused as to your banking scheme until I got to this patch. Now I see the implementation. You are mass-hot-swapping in the active state on critical CPU state changing events. .... > Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com> > --- > target-arm/helper.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 76 insertions(+), 1 deletion(-) > > diff --git a/target-arm/helper.c b/target-arm/helper.c > index e1e9762..7bfadb0 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -11,6 +11,7 @@ static inline int get_phys_addr(CPUARMState *env, uint32_t address, > int access_type, int is_user, > hwaddr *phys_ptr, int *prot, > target_ulong *page_size); > +static void switch_cp15_regs(CPUARMState *env, int secure); > #endif > > static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg) > @@ -1553,6 +1554,17 @@ static int sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) > } > > #ifndef CONFIG_USER_ONLY > +static int scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) > +{ > + if ((value & 1/*NS*/) && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_MON) { > + /* We are going to Non-secure state; switch banked system control registers */ > + switch_cp15_regs(env, 0); > + } > + > + env->cp15.c1_scr = value; > + return 0; > +} > + > static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri, > uint64_t value) > { > @@ -1572,7 +1584,7 @@ static const ARMCPRegInfo tz_cp_reginfo[] = { > #ifndef CONFIG_USER_ONLY > { .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0, > .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr), > - .resetvalue = 0 }, > + .writefn = scr_write, .resetvalue = 0 }, > { .name = "VBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0, > .access = PL1_RW, .type = ARM_CP_BANKED, .writefn = vbar_write, > .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar), > @@ -2284,6 +2296,69 @@ void switch_mode(CPUARMState *env, int mode) > env->regs[13] = env->banked_r13[i]; > env->regs[14] = env->banked_r14[i]; > env->spsr = env->banked_spsr[i]; > + > + if ((mode == ARM_CPU_MODE_MON || old_mode == ARM_CPU_MODE_MON) && > + (env->cp15.c1_scr & 1/*NS*/)) { > + /* We are going to change Security state; switch banked system control registers */ > + switch_cp15_regs(env, (mode == ARM_CPU_MODE_MON)); > + } > +} > + > +void switch_cp15_regs(CPUARMState *env, int secure) > +{ > + int i; > + > + /* Save current Security state registers */ > + i = arm_is_secure(env) ? 0 : 1; > + env->cp15.banked_c0_cssel[i] = env->cp15.c0_cssel; > + env->cp15.banked_c1_sys[i] = env->cp15.c1_sys; > + env->cp15.banked_c2_base0[i] = env->cp15.c2_base0; > + env->cp15.banked_c2_base0_hi[i] = env->cp15.c2_base0_hi; > + env->cp15.banked_c2_base1[i] = env->cp15.c2_base1; > + env->cp15.banked_c2_base1_hi[i] = env->cp15.c2_base1_hi; > + env->cp15.banked_c2_control[i] = env->cp15.c2_control; > + env->cp15.banked_c3[i] = env->cp15.c3; > + env->cp15.banked_c5_data[i] = env->cp15.c5_data; > + env->cp15.banked_c5_insn[i] = env->cp15.c5_insn; > + env->cp15.banked_c6_data[i] = env->cp15.c6_data; > + env->cp15.banked_c6_insn[i] = env->cp15.c6_insn; > + env->cp15.banked_c7_par[i] = env->cp15.c7_par; > + env->cp15.banked_c7_par_hi[i] = env->cp15.c7_par_hi; > + env->cp15.banked_c13_context[i] = env->cp15.c13_context; > + env->cp15.banked_c13_fcse[i] = env->cp15.c13_fcse; > + env->cp15.banked_c13_tls1[i] = env->cp15.c13_tls1; > + env->cp15.banked_c13_tls2[i] = env->cp15.c13_tls2; > + env->cp15.banked_c13_tls3[i] = env->cp15.c13_tls3; > + > + /* Restore new Security state registers */ > + i = secure ? 0 : 1; > + env->cp15.c0_cssel = env->cp15.banked_c0_cssel[i]; > + /* Maintain the value of common bits */ > + env->cp15.c1_sys &= 0x8204000; > + env->cp15.c1_sys |= env->cp15.banked_c1_sys[i] & ~0x8204000; > + env->cp15.c2_base0 = env->cp15.banked_c2_base0[i]; > + env->cp15.c2_base0_hi = env->cp15.banked_c2_base0_hi[i]; > + env->cp15.c2_base1 = env->cp15.banked_c2_base1[i]; > + env->cp15.c2_base1_hi = env->cp15.banked_c2_base1_hi[i]; > + { > + int maskshift; > + env->cp15.c2_control = env->cp15.banked_c2_control[i]; > + maskshift = extract32(env->cp15.c2_control, 0, 3); > + env->cp15.c2_mask = ~(((uint32_t)0xffffffffu) >> maskshift); > + env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> maskshift); > + } > + env->cp15.c3 = env->cp15.banked_c3[i]; > + env->cp15.c5_data = env->cp15.banked_c5_data[i]; > + env->cp15.c5_insn = env->cp15.banked_c5_insn[i]; > + env->cp15.c6_data = env->cp15.banked_c6_data[i]; > + env->cp15.c6_insn = env->cp15.banked_c6_insn[i]; > + env->cp15.c7_par = env->cp15.banked_c7_par[i]; > + env->cp15.c7_par_hi = env->cp15.banked_c7_par_hi[i]; > + env->cp15.c13_context = env->cp15.banked_c13_context[i]; > + env->cp15.c13_fcse = env->cp15.banked_c13_fcse[i]; > + env->cp15.c13_tls1 = env->cp15.banked_c13_tls1[i]; > + env->cp15.c13_tls2 = env->cp15.banked_c13_tls2[i]; > + env->cp15.c13_tls3 = env->cp15.banked_c13_tls3[i]; > } And I think this is awkward. Can't we just teach TCG to get the right value out of the banked array and do away with these active copies completely? This greatly complicates the change pattern for adding a new banked CP. Regards, Peter > > static void v7m_push(CPUARMState *env, uint32_t val) > -- > 1.7.9.5 > >
On 12/19/2013 08:37 AM, Peter Crosthwaite wrote: > On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote: >> Banked coprocessor registers are switched on two cases: >> 1) Entering or leaving CPU monitor mode with SCR.NS bit set; >> 2) Setting SCR.NS bit not from CPU monitor mode >> >> Coprocessor register banking is done similar to CPU core register >> banking. Some of SCTRL bits are common for secure and non-secure state. >> Translation table base masks are updated on register switch instead >> of TTBCR write. >> > So I was rather confused as to your banking scheme until I got to this > patch. Now I see the implementation. You are mass-hot-swapping in the > active state on critical CPU state changing events. .... > >> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com> >> --- >> target-arm/helper.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 76 insertions(+), 1 deletion(-) >> >> diff --git a/target-arm/helper.c b/target-arm/helper.c >> index e1e9762..7bfadb0 100644 >> --- a/target-arm/helper.c >> +++ b/target-arm/helper.c >> @@ -11,6 +11,7 @@ static inline int get_phys_addr(CPUARMState *env, uint32_t address, >> int access_type, int is_user, >> hwaddr *phys_ptr, int *prot, >> target_ulong *page_size); >> +static void switch_cp15_regs(CPUARMState *env, int secure); >> #endif >> >> static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg) >> @@ -1553,6 +1554,17 @@ static int sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) >> } >> >> #ifndef CONFIG_USER_ONLY >> +static int scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) >> +{ >> + if ((value & 1/*NS*/) && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_MON) { >> + /* We are going to Non-secure state; switch banked system control registers */ >> + switch_cp15_regs(env, 0); >> + } >> + >> + env->cp15.c1_scr = value; >> + return 0; >> +} >> + >> static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri, >> uint64_t value) >> { >> @@ -1572,7 +1584,7 @@ static const ARMCPRegInfo tz_cp_reginfo[] = { >> #ifndef CONFIG_USER_ONLY >> { .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0, >> .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr), >> - .resetvalue = 0 }, >> + .writefn = scr_write, .resetvalue = 0 }, >> { .name = "VBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0, >> .access = PL1_RW, .type = ARM_CP_BANKED, .writefn = vbar_write, >> .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar), >> @@ -2284,6 +2296,69 @@ void switch_mode(CPUARMState *env, int mode) >> env->regs[13] = env->banked_r13[i]; >> env->regs[14] = env->banked_r14[i]; >> env->spsr = env->banked_spsr[i]; >> + >> + if ((mode == ARM_CPU_MODE_MON || old_mode == ARM_CPU_MODE_MON) && >> + (env->cp15.c1_scr & 1/*NS*/)) { >> + /* We are going to change Security state; switch banked system control registers */ >> + switch_cp15_regs(env, (mode == ARM_CPU_MODE_MON)); >> + } >> +} >> + >> +void switch_cp15_regs(CPUARMState *env, int secure) >> +{ >> + int i; >> + >> + /* Save current Security state registers */ >> + i = arm_is_secure(env) ? 0 : 1; >> + env->cp15.banked_c0_cssel[i] = env->cp15.c0_cssel; >> + env->cp15.banked_c1_sys[i] = env->cp15.c1_sys; >> + env->cp15.banked_c2_base0[i] = env->cp15.c2_base0; >> + env->cp15.banked_c2_base0_hi[i] = env->cp15.c2_base0_hi; >> + env->cp15.banked_c2_base1[i] = env->cp15.c2_base1; >> + env->cp15.banked_c2_base1_hi[i] = env->cp15.c2_base1_hi; >> + env->cp15.banked_c2_control[i] = env->cp15.c2_control; >> + env->cp15.banked_c3[i] = env->cp15.c3; >> + env->cp15.banked_c5_data[i] = env->cp15.c5_data; >> + env->cp15.banked_c5_insn[i] = env->cp15.c5_insn; >> + env->cp15.banked_c6_data[i] = env->cp15.c6_data; >> + env->cp15.banked_c6_insn[i] = env->cp15.c6_insn; >> + env->cp15.banked_c7_par[i] = env->cp15.c7_par; >> + env->cp15.banked_c7_par_hi[i] = env->cp15.c7_par_hi; >> + env->cp15.banked_c13_context[i] = env->cp15.c13_context; >> + env->cp15.banked_c13_fcse[i] = env->cp15.c13_fcse; >> + env->cp15.banked_c13_tls1[i] = env->cp15.c13_tls1; >> + env->cp15.banked_c13_tls2[i] = env->cp15.c13_tls2; >> + env->cp15.banked_c13_tls3[i] = env->cp15.c13_tls3; >> + >> + /* Restore new Security state registers */ >> + i = secure ? 0 : 1; >> + env->cp15.c0_cssel = env->cp15.banked_c0_cssel[i]; >> + /* Maintain the value of common bits */ >> + env->cp15.c1_sys &= 0x8204000; >> + env->cp15.c1_sys |= env->cp15.banked_c1_sys[i] & ~0x8204000; >> + env->cp15.c2_base0 = env->cp15.banked_c2_base0[i]; >> + env->cp15.c2_base0_hi = env->cp15.banked_c2_base0_hi[i]; >> + env->cp15.c2_base1 = env->cp15.banked_c2_base1[i]; >> + env->cp15.c2_base1_hi = env->cp15.banked_c2_base1_hi[i]; >> + { >> + int maskshift; >> + env->cp15.c2_control = env->cp15.banked_c2_control[i]; >> + maskshift = extract32(env->cp15.c2_control, 0, 3); >> + env->cp15.c2_mask = ~(((uint32_t)0xffffffffu) >> maskshift); >> + env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> maskshift); >> + } >> + env->cp15.c3 = env->cp15.banked_c3[i]; >> + env->cp15.c5_data = env->cp15.banked_c5_data[i]; >> + env->cp15.c5_insn = env->cp15.banked_c5_insn[i]; >> + env->cp15.c6_data = env->cp15.banked_c6_data[i]; >> + env->cp15.c6_insn = env->cp15.banked_c6_insn[i]; >> + env->cp15.c7_par = env->cp15.banked_c7_par[i]; >> + env->cp15.c7_par_hi = env->cp15.banked_c7_par_hi[i]; >> + env->cp15.c13_context = env->cp15.banked_c13_context[i]; >> + env->cp15.c13_fcse = env->cp15.banked_c13_fcse[i]; >> + env->cp15.c13_tls1 = env->cp15.banked_c13_tls1[i]; >> + env->cp15.c13_tls2 = env->cp15.banked_c13_tls2[i]; >> + env->cp15.c13_tls3 = env->cp15.banked_c13_tls3[i]; >> } > And I think this is awkward. Can't we just teach TCG to get the right > value out of the banked array and do away with these active copies > completely? This greatly complicates the change pattern for adding a > new banked CP. > > Regards, > Peter Yes, this banking scheme makes state changing events quite heavy. But maintaining the active copies allows to keep translation table walking code untouched. I think there is a trade-off between state changing and translation table walking overheads. I think the CP banking is the most fragile thing in this patch series and this should become much better after review :) Thanks! > >> static void v7m_push(CPUARMState *env, uint32_t val) >> -- >> 1.7.9.5 >> >>
On 19 December 2013 07:27, Fedorov Sergey <s.fedorov@samsung.com> wrote: > Yes, this banking scheme makes state changing events quite heavy. But > maintaining the active copies allows to keep translation table walking code > untouched. I think there is a trade-off between state changing and > translation table walking overheads. We shouldn't be doing tlb walks that often that it makes a difference whether we do env->ttbr0 or env->ttbr0[env->ns] ... > I think the CP banking is the most fragile thing in this patch series and > this should become much better after review :) It would probably be a good idea to look at the v8 ARM ARM and figure out how banked-for-NS/S registers should fit in with the AArch64 vs AArch32 split. [if you don't have a copy, it's on the ARM website: http://infocenter.arm.com/help/topic/com.arm.doc.ddi0487a.a_errata2/index.html you'll need to register an account on the website if you don't already have one but it's a fairly simple "fill in the form" automated process] Note in particular that: * many of the current uint32_t fields in our CPU state struct are likely to widen to uint64_t, so the AArch64 representation is canonical, and the AArch32 register accessors access a part of that state (typically the lower 32 bits) * registers which are banked S/NS in AArch32 are not necessarily banked in AArch64 AArch64 support is likely to land before your TrustZone stuff does so we need to make the two features work together cleanly. thanks -- PMM
On Thu, Dec 19, 2013 at 9:38 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 19 December 2013 07:27, Fedorov Sergey <s.fedorov@samsung.com> wrote: >> Yes, this banking scheme makes state changing events quite heavy. But >> maintaining the active copies allows to keep translation table walking code >> untouched. I think there is a trade-off between state changing and >> translation table walking overheads. > > We shouldn't be doing tlb walks that often that it makes a > difference whether we do env->ttbr0 or env->ttbr0[env->ns] ... > >> I think the CP banking is the most fragile thing in this patch series and >> this should become much better after review :) > > It would probably be a good idea to look at the v8 ARM ARM and > figure out how banked-for-NS/S registers should fit in with the > AArch64 vs AArch32 split. > [if you don't have a copy, it's on the ARM website: > http://infocenter.arm.com/help/topic/com.arm.doc.ddi0487a.a_errata2/index.html > you'll need to register an account on the website if you don't already > have one but it's a fairly simple "fill in the form" automated process] > > Note in particular that: > * many of the current uint32_t fields in our CPU state struct are > likely to widen to uint64_t, so the AArch64 representation is > canonical, and the AArch32 register accessors access a part > of that state (typically the lower 32 bits) > * registers which are banked S/NS in AArch32 are not necessarily > banked in AArch64 > Adding to that, are there any other reasons to bank a register other than sec-extensions? It seems like what you have implemented here is too sec specific for simply calling it "banked" (without further clarification of what you are banking for). Regards, Peter > AArch64 support is likely to land before your TrustZone stuff > does so we need to make the two features work together cleanly. > > thanks > -- PMM >
On 12/19/2013 04:44 PM, Peter Crosthwaite wrote: > On Thu, Dec 19, 2013 at 9:38 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 19 December 2013 07:27, Fedorov Sergey <s.fedorov@samsung.com> wrote: >>> Yes, this banking scheme makes state changing events quite heavy. But >>> maintaining the active copies allows to keep translation table walking code >>> untouched. I think there is a trade-off between state changing and >>> translation table walking overheads. >> We shouldn't be doing tlb walks that often that it makes a >> difference whether we do env->ttbr0 or env->ttbr0[env->ns] ... >> >>> I think the CP banking is the most fragile thing in this patch series and >>> this should become much better after review :) >> It would probably be a good idea to look at the v8 ARM ARM and >> figure out how banked-for-NS/S registers should fit in with the >> AArch64 vs AArch32 split. >> [if you don't have a copy, it's on the ARM website: >> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0487a.a_errata2/index.html >> you'll need to register an account on the website if you don't already >> have one but it's a fairly simple "fill in the form" automated process] >> >> Note in particular that: >> * many of the current uint32_t fields in our CPU state struct are >> likely to widen to uint64_t, so the AArch64 representation is >> canonical, and the AArch32 register accessors access a part >> of that state (typically the lower 32 bits) >> * registers which are banked S/NS in AArch32 are not necessarily >> banked in AArch64 >> > Adding to that, are there any other reasons to bank a register other > than sec-extensions? It seems like what you have implemented here > is too sec specific for simply calling it "banked" (without further > clarification of what you are banking for). > > Regards, > Peter I'm not sure that I understand your question correctly but I try to answer. From ARMv7 ARM document section "B3.15.3 Classification of system control registers": "Banked system control registers have two copies, one Secure and one Non-secure." I don't know any use of term "CP15 banked registers" other that related to Security Extensions. Best regards, Sergey Fedorov > >> AArch64 support is likely to land before your TrustZone stuff >> does so we need to make the two features work together cleanly. >> >> thanks >> -- PMM >>
On Thu, Dec 19, 2013 at 11:39 PM, Fedorov Sergey <s.fedorov@samsung.com> wrote: > > On 12/19/2013 04:44 PM, Peter Crosthwaite wrote: >> >> On Thu, Dec 19, 2013 at 9:38 PM, Peter Maydell <peter.maydell@linaro.org> >> wrote: >>> >>> On 19 December 2013 07:27, Fedorov Sergey <s.fedorov@samsung.com> wrote: >>>> >>>> Yes, this banking scheme makes state changing events quite heavy. But >>>> maintaining the active copies allows to keep translation table walking >>>> code >>>> untouched. I think there is a trade-off between state changing and >>>> translation table walking overheads. >>> >>> We shouldn't be doing tlb walks that often that it makes a >>> difference whether we do env->ttbr0 or env->ttbr0[env->ns] ... >>> >>>> I think the CP banking is the most fragile thing in this patch series >>>> and >>>> this should become much better after review :) >>> >>> It would probably be a good idea to look at the v8 ARM ARM and >>> figure out how banked-for-NS/S registers should fit in with the >>> AArch64 vs AArch32 split. >>> [if you don't have a copy, it's on the ARM website: >>> >>> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0487a.a_errata2/index.html >>> you'll need to register an account on the website if you don't already >>> have one but it's a fairly simple "fill in the form" automated process] >>> >>> Note in particular that: >>> * many of the current uint32_t fields in our CPU state struct are >>> likely to widen to uint64_t, so the AArch64 representation is >>> canonical, and the AArch32 register accessors access a part >>> of that state (typically the lower 32 bits) >>> * registers which are banked S/NS in AArch32 are not necessarily >>> banked in AArch64 >>> >> Adding to that, are there any other reasons to bank a register other >> than sec-extensions? It seems like what you have implemented here >> is too sec specific for simply calling it "banked" (without further >> clarification of what you are banking for). >> >> Regards, >> Peter > > > I'm not sure that I understand your question correctly but I try to answer. > From ARMv7 ARM document section "B3.15.3 Classification of system control > registers": > > "Banked system control registers have two copies, one Secure and one > Non-secure." > Ok fair enough. I will wager though that sooner or later ARM will find a reason to bank a reg other than sec-ext. But let's stick to their terminology which by that quote its quite clear that "banked" means "banked for security" (No change needed). Regards, Peter > I don't know any use of term "CP15 banked registers" other that related to > Security Extensions. > > Best regards, > Sergey Fedorov > > >> >>> AArch64 support is likely to land before your TrustZone stuff >>> does so we need to make the two features work together cleanly. >>> >>> thanks >>> -- PMM >>> > >
On 19 December 2013 14:01, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Thu, Dec 19, 2013 at 11:39 PM, Fedorov Sergey <s.fedorov@samsung.com> wrote: >> "Banked system control registers have two copies, one Secure and one >> Non-secure." >> > > Ok fair enough. I will wager though that sooner or later ARM will find > a reason to bank a reg other than sec-ext. But let's stick to their > terminology which by that quote its quite clear that "banked" means > "banked for security" (No change needed). Well, the other register banking is the gp regs which get banks for user/svc/fiq/etc in AArch32. But I think in the context of system registers "banked register" is fairly clear. thanks -- PMM
On 12/19/2013 03:38 PM, Peter Maydell wrote: > On 19 December 2013 07:27, Fedorov Sergey <s.fedorov@samsung.com> wrote: >> Yes, this banking scheme makes state changing events quite heavy. But >> maintaining the active copies allows to keep translation table walking code >> untouched. I think there is a trade-off between state changing and >> translation table walking overheads. > We shouldn't be doing tlb walks that often that it makes a > difference whether we do env->ttbr0 or env->ttbr0[env->ns] ... > >> I think the CP banking is the most fragile thing in this patch series and >> this should become much better after review :) > It would probably be a good idea to look at the v8 ARM ARM and > figure out how banked-for-NS/S registers should fit in with the > AArch64 vs AArch32 split. > [if you don't have a copy, it's on the ARM website: > http://infocenter.arm.com/help/topic/com.arm.doc.ddi0487a.a_errata2/index.html > you'll need to register an account on the website if you don't already > have one but it's a fairly simple "fill in the form" automated process] > > Note in particular that: > * many of the current uint32_t fields in our CPU state struct are > likely to widen to uint64_t, so the AArch64 representation is > canonical, and the AArch32 register accessors access a part > of that state (typically the lower 32 bits) > * registers which are banked S/NS in AArch32 are not necessarily > banked in AArch64 > > AArch64 support is likely to land before your TrustZone stuff > does so we need to make the two features work together cleanly. > > thanks > -- PMM > I've briefly looked at the v8 ARM ARM. As I can see there is no banked system control registers in AArch64. Seems the concept is changed to provide separate registers for each meaningful execution level. Please, correct me if I am wrong. So I think there shouldn't be "active" and "banked" fields for banked AArch32 CP15 registers as in my patch. Seems it is worth to use AArch64 view of system control registers as a basis. That means there would be separate S and NS register fields in CPU state structure that will me mapped to separate AArch64 registers. ARMCPRegInfo structure would have additional field holding NS register state filed offset for AArch32 banked registers. Which branch in https://git.linaro.org/people/peter.maydell/qemu-arm.git repository holds the most actual A64 support? Thanks! Best regards, Sergey Fedorov
On 20 December 2013 14:12, Fedorov Sergey <s.fedorov@samsung.com> wrote: > I've briefly looked at the v8 ARM ARM. As I can see there is no banked > system control registers in AArch64. Seems the concept is changed to provide > separate registers for each meaningful execution level. Please, correct me > if I am wrong. Yes, I think this is generally correct. > So I think there shouldn't be "active" and "banked" fields for banked > AArch32 CP15 registers as in my patch. Seems it is worth to use AArch64 view > of system control registers as a basis. That means there would be separate S > and NS register fields in CPU state structure that will me mapped to > separate AArch64 registers. ARMCPRegInfo structure would have additional > field holding NS register state filed offset for AArch32 banked registers. This sounds like it could work, though there are some wrinkles for registers with readfns/writefns -- do we have extra s vs ns read/write functions, or just one set of functions which has to look in env->ns to figure out whether to use the S or NS version? > Which branch in https://git.linaro.org/people/peter.maydell/qemu-arm.git > repository holds the most actual A64 support? It's still a work in progress so it depends what you want. a64-third-fourth-set is the last set of patches that went out for review, and should generally work for integer instructions. a64-working is my work-in-progress branch so it will have the most recent versions of everything, but it rebases frequently and is liable to occasionally be broken... thanks -- PMM
On 12/20/2013 06:33 PM, Peter Maydell wrote: > On 20 December 2013 14:12, Fedorov Sergey <s.fedorov@samsung.com> wrote: >> I've briefly looked at the v8 ARM ARM. As I can see there is no banked >> system control registers in AArch64. Seems the concept is changed to provide >> separate registers for each meaningful execution level. Please, correct me >> if I am wrong. > Yes, I think this is generally correct. > >> So I think there shouldn't be "active" and "banked" fields for banked >> AArch32 CP15 registers as in my patch. Seems it is worth to use AArch64 view >> of system control registers as a basis. That means there would be separate S >> and NS register fields in CPU state structure that will me mapped to >> separate AArch64 registers. ARMCPRegInfo structure would have additional >> field holding NS register state filed offset for AArch32 banked registers. > This sounds like it could work, though there are some wrinkles for > registers with readfns/writefns -- do we have extra s vs ns read/write > functions, or just one set of functions which has to look in env->ns to > figure out whether to use the S or NS version? I think if most read/write functions do the same work for both S/NS versions then this code should not be duplicated. > >> Which branch in https://git.linaro.org/people/peter.maydell/qemu-arm.git >> repository holds the most actual A64 support? > It's still a work in progress so it depends what you want. > a64-third-fourth-set is the last set of patches that went out for > review, and should generally work for integer instructions. > a64-working is my work-in-progress branch so it will have the > most recent versions of everything, but it rebases frequently > and is liable to occasionally be broken... Thanks. > > thanks > -- PMM >
On 12/20/2013 06:38 PM, Fedorov Sergey wrote: > On 12/20/2013 06:33 PM, Peter Maydell wrote: >> On 20 December 2013 14:12, Fedorov Sergey <s.fedorov@samsung.com> wrote: >>> I've briefly looked at the v8 ARM ARM. As I can see there is no banked >>> system control registers in AArch64. Seems the concept is changed to provide >>> separate registers for each meaningful execution level. Please, correct me >>> if I am wrong. >> Yes, I think this is generally correct. >> >>> So I think there shouldn't be "active" and "banked" fields for banked >>> AArch32 CP15 registers as in my patch. Seems it is worth to use AArch64 view >>> of system control registers as a basis. That means there would be separate S >>> and NS register fields in CPU state structure that will me mapped to >>> separate AArch64 registers. ARMCPRegInfo structure would have additional >>> field holding NS register state filed offset for AArch32 banked registers. >> This sounds like it could work, though there are some wrinkles for >> registers with readfns/writefns -- do we have extra s vs ns read/write >> functions, or just one set of functions which has to look in env->ns to >> figure out whether to use the S or NS version? > I think if most read/write functions do the same work for both S/NS > versions then this code should not be duplicated. But on the other hand, separate S/NS read/write functions could be reused for AArch64 register descriptions that is separate for each EL... > >>> Which branch in https://git.linaro.org/people/peter.maydell/qemu-arm.git >>> repository holds the most actual A64 support? >> It's still a work in progress so it depends what you want. >> a64-third-fourth-set is the last set of patches that went out for >> review, and should generally work for integer instructions. >> a64-working is my work-in-progress branch so it will have the >> most recent versions of everything, but it rebases frequently >> and is liable to occasionally be broken... > Thanks. > >> thanks >> -- PMM >>
On Sat, Dec 21, 2013 at 12:33 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 20 December 2013 14:12, Fedorov Sergey <s.fedorov@samsung.com> wrote: >> I've briefly looked at the v8 ARM ARM. As I can see there is no banked >> system control registers in AArch64. Seems the concept is changed to provide >> separate registers for each meaningful execution level. Please, correct me >> if I am wrong. Isn't that just another definition of banking? > > Yes, I think this is generally correct. > >> So I think there shouldn't be "active" and "banked" fields for banked >> AArch32 CP15 registers as in my patch. I don't see how this extra scheme warrants the active-set+mass-hotswapping impl. Why can the accessors just be aware of the two different banking schemes at access time? Seems it is worth to use AArch64 view >> of system control registers as a basis. That means there would be separate S >> and NS register fields in CPU state structure that will me mapped to >> separate AArch64 registers. ARMCPRegInfo structure would have additional >> field holding NS register state filed offset for AArch32 banked registers. > > This sounds like it could work, I largely agree, except for the need for an active set. > though there are some wrinkles for > registers with readfns/writefns -- do we have extra s vs ns read/write > functions, or just one set of functions which has to look in env->ns to > figure out whether to use the S or NS version? One set sounds better to me - if your resorting to open coding your situation is probably complicated enough such that there is little you can do in a data driven way. That said, it could be useful to define both r.w handlers and fieldoffsets, and then the custom handlers access do actual register read/write through a generic helper fn (passed the CPRegInfo) that uses ->fieldoffset and is banking aware. This handlers the common cases where helper functions are doing: 1: Normal access + some side effects 2: Manipulation of the read/written value on the way in/out. without the need for all handlers having to open code banking functionality. Regards, Peter > >> Which branch in https://git.linaro.org/people/peter.maydell/qemu-arm.git >> repository holds the most actual A64 support? > > It's still a work in progress so it depends what you want. > a64-third-fourth-set is the last set of patches that went out for > review, and should generally work for integer instructions. > a64-working is my work-in-progress branch so it will have the > most recent versions of everything, but it rebases frequently > and is liable to occasionally be broken... > > thanks > -- PMM >
On 22 December 2013 01:08, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Sat, Dec 21, 2013 at 12:33 AM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> On 20 December 2013 14:12, Fedorov Sergey <s.fedorov@samsung.com> wrote: >>> I've briefly looked at the v8 ARM ARM. As I can see there is no banked >>> system control registers in AArch64. Seems the concept is changed to provide >>> separate registers for each meaningful execution level. Please, correct me >>> if I am wrong. > > Isn't that just another definition of banking? No: the crn/crm/op values differ for eg ESR_EL1, ESR_EL2 and ESR_EL3, and if you're in EL3 you can access all three of them (the access permissions are what stop EL1 from messing with EL3's ESR). Banked registers are where the same access instruction reads or writes different state depending on the value of something else (whether we're secure/nonsecure, in this case). thanks -- PMM
On 12/22/2013 05:08 AM, Peter Crosthwaite wrote: > On Sat, Dec 21, 2013 at 12:33 AM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> On 20 December 2013 14:12, Fedorov Sergey <s.fedorov@samsung.com> wrote: >>> I've briefly looked at the v8 ARM ARM. As I can see there is no banked >>> system control registers in AArch64. Seems the concept is changed to provide >>> separate registers for each meaningful execution level. Please, correct me >>> if I am wrong. > Isn't that just another definition of banking? > >> Yes, I think this is generally correct. >> >>> So I think there shouldn't be "active" and "banked" fields for banked >>> AArch32 CP15 registers as in my patch. > I don't see how this extra scheme warrants the > active-set+mass-hotswapping impl. Why can the accessors just be aware > of the two different banking schemes at access time? > > Seems it is worth to use AArch64 view >>> of system control registers as a basis. That means there would be separate S >>> and NS register fields in CPU state structure that will me mapped to >>> separate AArch64 registers. ARMCPRegInfo structure would have additional >>> field holding NS register state filed offset for AArch32 banked registers. >> This sounds like it could work, > I largely agree, except for the need for an active set. > >> though there are some wrinkles for >> registers with readfns/writefns -- do we have extra s vs ns read/write >> functions, or just one set of functions which has to look in env->ns to >> figure out whether to use the S or NS version? > One set sounds better to me - if your resorting to open coding your > situation is probably complicated enough such that there is little you > can do in a data driven way. That said, it could be useful to define > both r.w handlers and fieldoffsets, and then the custom handlers > access do actual register read/write through a generic helper fn > (passed the CPRegInfo) that uses ->fieldoffset and is banking aware. > This handlers the common cases where helper functions are doing: > > 1: Normal access + some side effects > 2: Manipulation of the read/written value on the way in/out. > > without the need for all handlers having to open code banking functionality. > > Regards, > Peter > >>> Which branch in https://git.linaro.org/people/peter.maydell/qemu-arm.git >>> repository holds the most actual A64 support? >> It's still a work in progress so it depends what you want. >> a64-third-fourth-set is the last set of patches that went out for >> review, and should generally work for integer instructions. >> a64-working is my work-in-progress branch so it will have the >> most recent versions of everything, but it rebases frequently >> and is liable to occasionally be broken... >> >> thanks >> -- PMM >> Seems I agree with you. Best regards, Sergey Fedorov
On 12/20/2013 06:33 PM, Peter Maydell wrote: > This sounds like it could work, though there are some wrinkles for > registers with readfns/writefns -- do we have extra s vs ns read/write > functions, or just one set of functions which has to look in env->ns to > figure out whether to use the S or NS version? What about defining a separate ARMCPRegInfo for each banked AArch32 system register? I think it would be close to AArch64 concept. It would allow to use separate read/write handlers if necessary or reuse the same handlers otherwise. When the handlers is not used, the translation code would simply lookup the ARMCPRegInfo for corresponding secure state and use the field offset. Best regards, Sergey Fedorov
On 23 December 2013 07:43, Fedorov Sergey <s.fedorov@samsung.com> wrote: > > On 12/20/2013 06:33 PM, Peter Maydell wrote: >> This sounds like it could work, though there are some wrinkles for >> registers with readfns/writefns -- do we have extra s vs ns read/write >> functions, or just one set of functions which has to look in env->ns to >> figure out whether to use the S or NS version? > > What about defining a separate ARMCPRegInfo for each banked AArch32 > system register? I think it would be close to AArch64 concept. It would > allow to use separate read/write handlers if necessary or reuse the same > handlers otherwise. When the handlers is not used, the translation code > would simply lookup the ARMCPRegInfo for corresponding secure state and > use the field offset. Yes, I was thinking about that the other day -- add the S/NS to the set of things in the hash table key, and have the 32 bit MCR/MRC code pass in the right value to get the correct banked register out. (We'd have to make all the non-banked registers go into the hashtable twice, though, but that's not a big deal I think.) thanks -- PMM
diff --git a/target-arm/helper.c b/target-arm/helper.c index e1e9762..7bfadb0 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -11,6 +11,7 @@ static inline int get_phys_addr(CPUARMState *env, uint32_t address, int access_type, int is_user, hwaddr *phys_ptr, int *prot, target_ulong *page_size); +static void switch_cp15_regs(CPUARMState *env, int secure); #endif static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg) @@ -1553,6 +1554,17 @@ static int sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) } #ifndef CONFIG_USER_ONLY +static int scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) +{ + if ((value & 1/*NS*/) && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_MON) { + /* We are going to Non-secure state; switch banked system control registers */ + switch_cp15_regs(env, 0); + } + + env->cp15.c1_scr = value; + return 0; +} + static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) { @@ -1572,7 +1584,7 @@ static const ARMCPRegInfo tz_cp_reginfo[] = { #ifndef CONFIG_USER_ONLY { .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0, .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr), - .resetvalue = 0 }, + .writefn = scr_write, .resetvalue = 0 }, { .name = "VBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0, .access = PL1_RW, .type = ARM_CP_BANKED, .writefn = vbar_write, .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar), @@ -2284,6 +2296,69 @@ void switch_mode(CPUARMState *env, int mode) env->regs[13] = env->banked_r13[i]; env->regs[14] = env->banked_r14[i]; env->spsr = env->banked_spsr[i]; + + if ((mode == ARM_CPU_MODE_MON || old_mode == ARM_CPU_MODE_MON) && + (env->cp15.c1_scr & 1/*NS*/)) { + /* We are going to change Security state; switch banked system control registers */ + switch_cp15_regs(env, (mode == ARM_CPU_MODE_MON)); + } +} + +void switch_cp15_regs(CPUARMState *env, int secure) +{ + int i; + + /* Save current Security state registers */ + i = arm_is_secure(env) ? 0 : 1; + env->cp15.banked_c0_cssel[i] = env->cp15.c0_cssel; + env->cp15.banked_c1_sys[i] = env->cp15.c1_sys; + env->cp15.banked_c2_base0[i] = env->cp15.c2_base0; + env->cp15.banked_c2_base0_hi[i] = env->cp15.c2_base0_hi; + env->cp15.banked_c2_base1[i] = env->cp15.c2_base1; + env->cp15.banked_c2_base1_hi[i] = env->cp15.c2_base1_hi; + env->cp15.banked_c2_control[i] = env->cp15.c2_control; + env->cp15.banked_c3[i] = env->cp15.c3; + env->cp15.banked_c5_data[i] = env->cp15.c5_data; + env->cp15.banked_c5_insn[i] = env->cp15.c5_insn; + env->cp15.banked_c6_data[i] = env->cp15.c6_data; + env->cp15.banked_c6_insn[i] = env->cp15.c6_insn; + env->cp15.banked_c7_par[i] = env->cp15.c7_par; + env->cp15.banked_c7_par_hi[i] = env->cp15.c7_par_hi; + env->cp15.banked_c13_context[i] = env->cp15.c13_context; + env->cp15.banked_c13_fcse[i] = env->cp15.c13_fcse; + env->cp15.banked_c13_tls1[i] = env->cp15.c13_tls1; + env->cp15.banked_c13_tls2[i] = env->cp15.c13_tls2; + env->cp15.banked_c13_tls3[i] = env->cp15.c13_tls3; + + /* Restore new Security state registers */ + i = secure ? 0 : 1; + env->cp15.c0_cssel = env->cp15.banked_c0_cssel[i]; + /* Maintain the value of common bits */ + env->cp15.c1_sys &= 0x8204000; + env->cp15.c1_sys |= env->cp15.banked_c1_sys[i] & ~0x8204000; + env->cp15.c2_base0 = env->cp15.banked_c2_base0[i]; + env->cp15.c2_base0_hi = env->cp15.banked_c2_base0_hi[i]; + env->cp15.c2_base1 = env->cp15.banked_c2_base1[i]; + env->cp15.c2_base1_hi = env->cp15.banked_c2_base1_hi[i]; + { + int maskshift; + env->cp15.c2_control = env->cp15.banked_c2_control[i]; + maskshift = extract32(env->cp15.c2_control, 0, 3); + env->cp15.c2_mask = ~(((uint32_t)0xffffffffu) >> maskshift); + env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> maskshift); + } + env->cp15.c3 = env->cp15.banked_c3[i]; + env->cp15.c5_data = env->cp15.banked_c5_data[i]; + env->cp15.c5_insn = env->cp15.banked_c5_insn[i]; + env->cp15.c6_data = env->cp15.banked_c6_data[i]; + env->cp15.c6_insn = env->cp15.banked_c6_insn[i]; + env->cp15.c7_par = env->cp15.banked_c7_par[i]; + env->cp15.c7_par_hi = env->cp15.banked_c7_par_hi[i]; + env->cp15.c13_context = env->cp15.banked_c13_context[i]; + env->cp15.c13_fcse = env->cp15.banked_c13_fcse[i]; + env->cp15.c13_tls1 = env->cp15.banked_c13_tls1[i]; + env->cp15.c13_tls2 = env->cp15.banked_c13_tls2[i]; + env->cp15.c13_tls3 = env->cp15.banked_c13_tls3[i]; } static void v7m_push(CPUARMState *env, uint32_t val)
Banked coprocessor registers are switched on two cases: 1) Entering or leaving CPU monitor mode with SCR.NS bit set; 2) Setting SCR.NS bit not from CPU monitor mode Coprocessor register banking is done similar to CPU core register banking. Some of SCTRL bits are common for secure and non-secure state. Translation table base masks are updated on register switch instead of TTBCR write. Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com> --- target-arm/helper.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 76 insertions(+), 1 deletion(-)