Re: [Qemu-devel] [[PATCH] 1/7] target-arm: Add exception target el infrastructure

[Greg Bellows]

On Thu, Apr 16, 2015 at 12:50 PM, Peter Maydell <address@hidden> wrote:

On 27 March 2015 at 19:10, Greg Bellows <address@hidden> wrote:
> Add a CPU state exception target EL field that will be used for communicating
> the EL to which an exception should be routed.
>
> Add a target EL argument to the generic exception helper for callers to specify
> the EL to which the exception should be routed.  Extended the helper to set
> the newly added CPU state exception target el.  Updated calls to helpers to
> include target EL, minimally the current el, which gets upgraded as needed.
>
> Signed-off-by: Greg Bellows <address@hidden>

This is generally OK, but I don't like the way we directly use
s->current_el as the target EL in many places:

(1) putting the MAX(target_el, 1) in gen_exception() feels
like the wrong place.

​This was simply a safety net as we have to catch the case eventually and since the target_el originates here it seemed like the most logical place. Perhaps we just need to pass 1 here and anything that is not 1 needs to be addressed at this point. 

 

(2) if we're executing with a 32-bit EL3 and we're in Secure
EL0, surely the target EL should be 3, not 1?

​

Maybe a default_target_el() function would help (returning
2 if current EL is EL2, 3 if EL3, and 1 or 3 as appropriate
if current EL is EL0 or EL1) ?

​Hmmm... yes that is true in the case of secure EL0 it should be routed to EL3. Which differs from the case of nonsecure EL0.  Would this function make sense in place of the MAX above?

Also, what's the plan for handling exceptions which can
trap to EL1, 2 or 3 configurably (eg the various floating
point disables)? Do we put all those trap bits into the TB
flags? If we want to be able to combine them into a single
"FP is disabled" bit in the TB flags, then we need to generate
the same code for any kind of FP-disabled case, which means we
can't encode the target-EL in the generated code like this:
we would need to do something like have a new EXCP_FPDIS
which we then sorted out into an (EXCP_UDEF, target_el, syndrome)
at runtime based on the trap register bits.

I was thinking that we could just turn the fpen into an exception level (int instead of bool) that if not 0 meant that an exception should be generated.  We would catch this in fp_access_check().  The value of fpen (or whatever a better name would be) would get passed into the gen_exception_insn() call as the target el.

 

thanks
-- PMM