On Mon, May 18, 2015 at 07:41:29PM +0100, Peter Maydell wrote:
> On 13 May 2015 at 07:52, Edgar E. Iglesias <edgar.igles...@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.igles...@xilinx.com>
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.igles...@xilinx.com>
> > ---
> > 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
