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

[Peter Crosthwaite]

On Thu, Jun 18, 2015 at 11:49 PM, Greg Ungerer <address@hidden> wrote:
> 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.
>

Ahh yes I see. No change needed then.

Regards,
Peter

>
>> 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
>>>
>>>
>>
>
>