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

[Peter Crosthwaite]

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.

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

Otherwise,

Reviewed-by: Peter Crosthwaite <address@hidden>

Regards,
Peter

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