Re: [Qemu-devel] [PATCH 3/3] m68k: fix usp processing on interrupt entry and exception exit

[Greg Ungerer]

Hi Peter,

On 19/06/15 15:49, Peter Crosthwaite wrote:
> On Mon, Aug 18, 2014 at 10:37 PM,  <address@hidden> wrote:
>> From: Greg Ungerer <address@hidden>
>>
>> The action to potentially switch sp register is not occurring at the correct
>> point in the interrupt entry or exception exit sequences.
>>
>> For the interrupt entry case the sp on entry is used to create the stack
>> exception frame - but this may well be the user stack pointer, since we
>> haven't done the switch yet. Re-order the flow to switch the sp regs then
>> use the current sp to create the exception frame.
>>
> 
> So I see the bug. The existing code is definitely bogus, that in both
> int and ret the, newly switched SP is overwitten by a stale one that
> has just been used. The actual stack push and pop logic happens after
> and before the switch for int and ret respectively so it is pretty
> clear the author intended the stacking to happen on the supervisors
> stack. Is this mandated by the spec one way or the other or is it up
> to implementation which stack these values are stored on? I can see it
> could work both ways.

There is 2 cases to deal with.

1. Early ColdFire parts only had a single stack pointer (a7).
   There is no notion of swapping to a supervisor stack in this case.
   All ColdFire parts power up in this mode, only a single stack pointer.
   This mode works with the existing qemu code.

2. Later model ColdFire's introduced the conventional m68k user and
   supervisor stack pointers. It is enabled via a mode bit in the 
   CPU CACR register. Once the EUSP bit is enabled then on interrupt
   and exception the ColdFire reference manual states that "The processor
   saves the current context by creating an exception stack frame on the
   system stack". So it must be on the supervisor stack, using the
   supervisor stack pointer.

Case 2 is what this patch fixes.


>> For the return from exception case the code is unwinding the sp after
>> switching sp registers. But it should always unwind the supervisor sp
>> first, then carry out any required sp switch.
>>
>> Note that these problems don't effect operation unless the user sp bit is
>> set in the CACR register. Only a single sp is used in the default power up
>> state. Previously Linux only used this single sp mode. But modern versions
>> of Linux use the user sp mode now, so we need correct behavior for Linux
>> to work.
>>
>> Signed-off-by: Greg Ungerer <address@hidden>
>> ---
>>  target-m68k/op_helper.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/target-m68k/op_helper.c b/target-m68k/op_helper.c
>> index 9dd3e74..cd748b8 100644
>> --- a/target-m68k/op_helper.c
>> +++ b/target-m68k/op_helper.c
>> @@ -63,8 +63,8 @@ static void do_rte(CPUM68KState *env)
>>      env->pc = cpu_ldl_kernel(env, sp + 4);
>>      sp |= (fmt >> 28) & 3;
>>      env->sr = fmt & 0xffff;
>> -    m68k_switch_sp(env);
>>      env->aregs[7] = sp + 8;
>> +    m68k_switch_sp(env);
>>  }
>>
>>  static void do_interrupt_all(CPUM68KState *env, int is_hw)
>> @@ -108,10 +108,7 @@ static void do_interrupt_all(CPUM68KState *env, int 
>> is_hw)
>>
>>      vector = cs->exception_index << 2;
>>
>> -    sp = env->aregs[7];
>> -
>>      fmt |= 0x40000000;
>> -    fmt |= (sp & 3) << 28;
>>      fmt |= vector << 16;
>>      fmt |= env->sr;
> 
> You should move them all to keep them together.

We need to get at least env->sr into fmt here, before we modify
it in steps immediately after this. At least if we don't want
to save env->sr locally to use later.


> Otherwise,
> 
> Reviewed-by: Peter Crosthwaite <address@hidden>

Thanks for the review.

Regards
Greg



>> @@ -121,6 +118,8 @@ static void do_interrupt_all(CPUM68KState *env, int 
>> is_hw)
>>          env->sr &= ~SR_M;
>>      }
>>      m68k_switch_sp(env);
>> +    sp = env->aregs[7];
>> +    fmt |= (sp & 3) << 28;
>>
>>      /* ??? This could cause MMU faults.  */
>>      sp &= ~3;
>> --
>> 1.9.1
>>
>>
>