From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39090) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YuUVT-0004Rg-0D for qemu-devel@nongnu.org; Mon, 18 May 2015 19:31:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YuUVN-00088W-Vi for qemu-devel@nongnu.org; Mon, 18 May 2015 19:31:34 -0400 Received: from mail-wi0-x235.google.com ([2a00:1450:400c:c05::235]:35488) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YuUVN-00088G-KY for qemu-devel@nongnu.org; Mon, 18 May 2015 19:31:29 -0400 Received: by wicmx19 with SMTP id mx19so97192452wic.0 for ; Mon, 18 May 2015 16:31:28 -0700 (PDT) Date: Tue, 19 May 2015 09:27:24 +1000 From: "Edgar E. Iglesias" Message-ID: <20150518232724.GA10142@toto> References: <1431499963-1019-1-git-send-email-edgar.iglesias@gmail.com> <1431499963-1019-5-git-send-email-edgar.iglesias@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v1 04/18] target-arm: Route timer access traps to EL1 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Edgar Iglesias , Alexander Graf , QEMU Developers , Greg Bellows , Sergey Fedorov , Alex =?iso-8859-1?Q?Benn=E9e?= On Mon, May 18, 2015 at 07:41:29PM +0100, Peter Maydell wrote: > On 13 May 2015 at 07:52, Edgar E. Iglesias wrote: > > From: "Edgar E. Iglesias" > > > > Signed-off-by: Edgar E. Iglesias > > --- > > target-arm/helper.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/target-arm/helper.c b/target-arm/helper.c > > index a4bab78..d849b30 100644 > > --- a/target-arm/helper.c > > +++ b/target-arm/helper.c > > @@ -1147,6 +1147,7 @@ static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri) > > { > > /* CNTFRQ: not visible from PL0 if both PL0PCTEN and PL0VCTEN are zero */ > > if (arm_current_el(env) == 0 && !extract32(env->cp15.c14_cntkctl, 0, 2)) { > > + env->exception.target_el = 1; > > return CP_ACCESS_TRAP; > > } > > return CP_ACCESS_OK; > > @@ -1157,6 +1158,7 @@ static CPAccessResult gt_counter_access(CPUARMState *env, int timeridx) > > /* CNT[PV]CT: not visible from PL0 if ELO[PV]CTEN is zero */ > > if (arm_current_el(env) == 0 && > > !extract32(env->cp15.c14_cntkctl, timeridx, 1)) { > > + env->exception.target_el = 1; > > return CP_ACCESS_TRAP; > > } > > return CP_ACCESS_OK; > > @@ -1169,6 +1171,7 @@ static CPAccessResult gt_timer_access(CPUARMState *env, int timeridx) > > */ > > if (arm_current_el(env) == 0 && > > !extract32(env->cp15.c14_cntkctl, 9 - timeridx, 1)) { > > + env->exception.target_el = 1; > > return CP_ACCESS_TRAP; > > } > > return CP_ACCESS_OK; > > If EL3 is 32-bit and we're in Secure EL0 then the correct > target_el is 3, not 1, so what you actually want here is > exception_target_el(). > > More generally, this seems to be a really easy mistake to make > with access functions. At the moment we come pretty close to > being able to say "always set both exception.target_el and > exception.syndrome in the same place in the code". So I think > the correct fix is > > --- a/target-arm/op_helper.c > +++ b/target-arm/op_helper.c > @@ -333,9 +333,11 @@ void HELPER(access_check_cp_reg)(CPUARMState > *env, void *rip, uint32_t syndrome) > return; > case CP_ACCESS_TRAP: > env->exception.syndrome = syndrome; > + env->target_el = exception_target_el(env); > break; > case CP_ACCESS_TRAP_UNCATEGORIZED: > env->exception.syndrome = syn_uncategorized(); > + env->target_el = exception_target_el(env); > break; > default: > g_assert_not_reached(); > > in the "Extend helpers to route exceptions" patch. If we > get any registers where the correct target EL is something > other than that, we should have new CP_ACCESS_* enums for > them. > > Then the only place where we don't set both syndrome > and target_el at the same time are: > * msr_i_pstate is failing to set a syndrome > * arm_debug_excp_handler() needs to set the target_el > to the debug target el > * arm_cpu_handle_mmu_fault should set the target_el > * the FIQ/IRQ/VIRQ/VFIQ paths in arm_cpu_exec_interrupt > don't set syndrome, because they're interrupts and > there's no syndrome info > > Note that the first three of these are all bugs, which is > a nice demonstration of the utility of the rule. I think > I'd also like to make the FIQ&c code set exception.syndrome > to an invalid value, because then we can probably write > some assertions for exception entry (and also because then > we're consistent about things.) > > That seems like more than I really feel I can justify > just fixing in target-arm.next, so I think I'll drop > Greg's patches 1..3 from target-arm.next and send them > out as part of a series which does the above changes. > Sounds good, thanks! Cheers, Edgar