Re: [Qemu-devel] [[PATCH] 3/7] target-arm: Update interrupt handling to use target EL

[Peter Maydell]

On 16 April 2015 at 22:03, Greg Bellows <address@hidden> wrote:
>
>
> On Thu, Apr 16, 2015 at 12:52 PM, Peter Maydell <address@hidden>
> wrote:
>>
>> On 27 March 2015 at 19:10, Greg Bellows <address@hidden> wrote:
>> > Updated the interrupt handling to utilize and report through the target
>> > EL
>> > exception field.  This includes consolidating and cleaning up code where
>> > needed. Target EL is now calculated once in arm_cpu_exec_interrupt() and
>> > do_interrupt was updated to use the target_el exception field.  The
>> > necessary code from arm_excp_target_el() was merged in where needed and
>> > the
>> > function removed.
>>
>> > --- a/target-arm/helper-a64.c
>> > +++ b/target-arm/helper-a64.c
>> > @@ -463,7 +463,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>> >  {
>> >      ARMCPU *cpu = ARM_CPU(cs);
>> >      CPUARMState *env = &cpu->env;
>> > -    unsigned int new_el = arm_excp_target_el(cs, cs->exception_index);
>> > +    unsigned int new_el = MAX(env->exception.target_el, 1);
>>
>> Surely we should never be able to get here with target_el zero?
>
>
> Ideally that would be true and I wondered that myself so I took out the the
> MAX safety net in arm_excp_target_el() and later hit the assert in
> aarch64_banked_spsr_index() because new_el was 0.  This is why I preserved
> the MAX behavior everywhere because just like the original code, there are
> cases where current_el is 0.

I guessed we might assert. But this is bad, because it suggests there's
somewhere that's not setting target_el at all, which in turn probably
means that after we've taken one exception, the next time around we'll
just go to whichever target_el the first one set up...

There should be a definite rule about who has to set up target_el,
so when we're reviewing or changing code we can tell whether it's
been done correctly. (Compare the rule for exception.syndrome, which
is "must be set up before the exception is raised, unless
excp_is_internal(excp)", which we have some assertions to try to
enforce. There's also a less-documented rule for exception.vaddress,
which is "must be set for EXCP_PREFETCH_ABORT and EXCP_DATA_ABORT".)

-- PMM