From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50835) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VtY1J-0006L6-7P for qemu-devel@nongnu.org; Thu, 19 Dec 2013 02:27:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VtY1B-00023x-Ky for qemu-devel@nongnu.org; Thu, 19 Dec 2013 02:27:45 -0500 Received: from mailout2.w1.samsung.com ([210.118.77.12]:33273) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VtY1B-0001zo-Bk for qemu-devel@nongnu.org; Thu, 19 Dec 2013 02:27:37 -0500 Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout2.w1.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0MY100JBBM1XHGA0@mailout2.w1.samsung.com> for qemu-devel@nongnu.org; Thu, 19 Dec 2013 07:27:33 +0000 (GMT) Message-id: <52B29FE5.40705@samsung.com> Date: Thu, 19 Dec 2013 11:27:33 +0400 From: Fedorov Sergey MIME-version: 1.0 References: <1386060535-15908-1-git-send-email-s.fedorov@samsung.com> <1386060535-15908-19-git-send-email-s.fedorov@samsung.com> In-reply-to: Content-type: text/plain; charset=ISO-8859-1; format=flowed Content-transfer-encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Peter Maydell , a.basov@samsung.com, "qemu-devel@nongnu.org Developers" , Johannes Winter On 12/19/2013 08:37 AM, Peter Crosthwaite wrote: > On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov 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 >> --- >> 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 >> >> -- Best regards, Sergey Fedorov