[v1,04/18] target-arm: Route timer access traps to EL1

On 13 May 2015 at 07:52, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@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

         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.

thanks
-- PMM

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