On Thu, Jun 18, 2015 at 11:49 PM, Greg Ungerer <g...@uclinux.org> wrote: > Hi Peter, > > On 19/06/15 15:49, Peter Crosthwaite wrote: >> On Mon, Aug 18, 2014 at 10:37 PM, <g...@uclinux.org> wrote: >>> From: Greg Ungerer <g...@uclinux.org> >>> >>> 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 <g...@uclinux.org> >>> --- >>> 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 <peter.crosthwa...@xilinx.com> > > 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 >>> >>> >> > >