Re: [Qemu-devel] [PATCH v1 04/18] target-arm: Route timer access traps to EL1

[Edgar E. Iglesias]

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