[Qemu-devel] [PATCH 0/3] m68k: fix coldfire linux problems

[Qemu-devel] [PATCH 0/3] m68k: fix coldfire linux problems

[gerg]

Some small issues are causing problems with running modern versions of
Linux on the m68k-system ColdFire targets. These 3 patches fix those
problems.

This is a repost of these patches rebased onto the current git tree:

  http://lists.nongnu.org/archive/html/qemu-devel/2012-09/msg01954.html
  http://lists.nongnu.org/archive/html/qemu-devel/2012-09/msg02099.html
  http://lists.nongnu.org/archive/html/qemu-devel/2012-09/msg01956.html

Regards
Greg

--

 hw/m68k/mcf_intc.c      |   14 ++++++++++++++
 target-m68k/op_helper.c |    7 +++----
 target-m68k/translate.c |    8 ++++----
 3 files changed, 21 insertions(+), 8 deletions(-)

[Qemu-devel] [PATCH 1/3] m68k: implmenent more ColdFire 5208 interrupt controller functionality

[gerg]

From: Greg Ungerer <gerg@uclinux.org>

Implement the SIMR and CIMR registers of the 5208 interrupt controller.
These are used by modern versions of Linux running on ColdFire (not sure
of the exact version they were introduced, but they have been in for quite
a while now).

Without this change when attempting to run a linux-3.5 kernel you will
see:

  qemu: hardware error: mcf_intc_write: Bad write offset 28

and execution will stop and dump out.

Signed-off-by: Greg Ungerer <gerg@uclinux.org>
---
 hw/m68k/mcf_intc.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/m68k/mcf_intc.c b/hw/m68k/mcf_intc.c
index 621423c..37a9de0 100644
--- a/hw/m68k/mcf_intc.c
+++ b/hw/m68k/mcf_intc.c
@@ -102,6 +102,20 @@ static void mcf_intc_write(void *opaque, hwaddr addr,
     case 0x0c:
         s->imr = (s->imr & 0xffffffff00000000ull) | (uint32_t)val;
         break;
+    case 0x1c:
+        if (val & 0x40) {
+            s->imr = 0xffffffffffffffffull;
+        } else {
+            s->imr |= (0x1ull << (val & 0x3f));
+        }
+        break;
+    case 0x1d:
+        if (val & 0x40) {
+            s->imr = 0ull;
+        } else {
+            s->imr &= ~(0x1ull << (val & 0x3f));
+        }
+        break;
     default:
         hw_error("mcf_intc_write: Bad write offset %d\n", offset);
         break;
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/3] m68k: implmenent more ColdFire 5208 interrupt controller functionality

[Peter Crosthwaite]

On Mon, Aug 18, 2014 at 10:37 PM,  <gerg@uclinux.org> wrote:
> From: Greg Ungerer <gerg@uclinux.org>
>
> Implement the SIMR and CIMR registers of the 5208 interrupt controller.
> These are used by modern versions of Linux running on ColdFire (not sure
> of the exact version they were introduced, but they have been in for quite
> a while now).
>
> Without this change when attempting to run a linux-3.5 kernel you will
> see:
>
>   qemu: hardware error: mcf_intc_write: Bad write offset 28
>
> and execution will stop and dump out.
>
> Signed-off-by: Greg Ungerer <gerg@uclinux.org>
> ---
>  hw/m68k/mcf_intc.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/hw/m68k/mcf_intc.c b/hw/m68k/mcf_intc.c
> index 621423c..37a9de0 100644
> --- a/hw/m68k/mcf_intc.c
> +++ b/hw/m68k/mcf_intc.c
> @@ -102,6 +102,20 @@ static void mcf_intc_write(void *opaque, hwaddr addr,
>      case 0x0c:
>          s->imr = (s->imr & 0xffffffff00000000ull) | (uint32_t)val;
>          break;
> +    case 0x1c:
> +        if (val & 0x40) {
> +            s->imr = 0xffffffffffffffffull;

~0ull.

Otherwise,

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

This introduces magic numbers which is generally discouraged, by this
device has no macrofication at all so I guess it should be cleaned up
at some stage.

Regards,
Peter

> +        } else {
> +            s->imr |= (0x1ull << (val & 0x3f));
> +        }
> +        break;
> +    case 0x1d:
> +        if (val & 0x40) {
> +            s->imr = 0ull;
> +        } else {
> +            s->imr &= ~(0x1ull << (val & 0x3f));
> +        }
> +        break;
>      default:
>          hw_error("mcf_intc_write: Bad write offset %d\n", offset);
>          break;
> --
> 1.9.1
>
>

Re: [Qemu-devel] [PATCH 1/3] m68k: implmenent more ColdFire 5208 interrupt controller functionality

[Greg Ungerer]

Hi Peter,

On 19/06/15 15:24, Peter Crosthwaite wrote:
> On Mon, Aug 18, 2014 at 10:37 PM,  <gerg@uclinux.org> wrote:
>> From: Greg Ungerer <gerg@uclinux.org>
>>
>> Implement the SIMR and CIMR registers of the 5208 interrupt controller.
>> These are used by modern versions of Linux running on ColdFire (not sure
>> of the exact version they were introduced, but they have been in for quite
>> a while now).
>>
>> Without this change when attempting to run a linux-3.5 kernel you will
>> see:
>>
>>   qemu: hardware error: mcf_intc_write: Bad write offset 28
>>
>> and execution will stop and dump out.
>>
>> Signed-off-by: Greg Ungerer <gerg@uclinux.org>
>> ---
>>  hw/m68k/mcf_intc.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/hw/m68k/mcf_intc.c b/hw/m68k/mcf_intc.c
>> index 621423c..37a9de0 100644
>> --- a/hw/m68k/mcf_intc.c
>> +++ b/hw/m68k/mcf_intc.c
>> @@ -102,6 +102,20 @@ static void mcf_intc_write(void *opaque, hwaddr addr,
>>      case 0x0c:
>>          s->imr = (s->imr & 0xffffffff00000000ull) | (uint32_t)val;
>>          break;
>> +    case 0x1c:
>> +        if (val & 0x40) {
>> +            s->imr = 0xffffffffffffffffull;
> 
> ~0ull.
>
> Otherwise,
> 
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Thanks, I'll change that and add the reviewed-by.


> This introduces magic numbers which is generally discouraged, by this
> device has no macrofication at all so I guess it should be cleaned up
> at some stage.

Agreed. I stuck to the existing style in this case.

Regards
Greg


>> +        } else {
>> +            s->imr |= (0x1ull << (val & 0x3f));
>> +        }
>> +        break;
>> +    case 0x1d:
>> +        if (val & 0x40) {
>> +            s->imr = 0ull;
>> +        } else {
>> +            s->imr &= ~(0x1ull << (val & 0x3f));
>> +        }
>> +        break;
>>      default:
>>          hw_error("mcf_intc_write: Bad write offset %d\n", offset);
>>          break;
>> --
>> 1.9.1
>>
>>
> 

[Qemu-devel] [PATCH 2/3] m68k: implement move to/from usp register instruction

[gerg]

From: Greg Ungerer <gerg@uclinux.org>

Fill out the code support for the move to/from usp instructions. They are
being decoded, but there is no code to support there actions. So add it.

Current versions of Linux running on the ColdFire 5208 use these instructions.

Signed-off-by: Greg Ungerer <gerg@uclinux.org>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 target-m68k/translate.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index efd4cfc..5a6666a 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -1995,8 +1995,8 @@ DISAS_INSN(move_from_usp)
         gen_exception(s, s->pc - 2, EXCP_PRIVILEGE);
         return;
     }
-    /* TODO: Implement USP.  */
-    gen_exception(s, s->pc - 2, EXCP_ILLEGAL);
+    tcg_gen_ld_i32(AREG(insn, 0), cpu_env,
+                   offsetof(CPUM68KState, sp[M68K_USP]));
 }
 
 DISAS_INSN(move_to_usp)
@@ -2005,8 +2005,8 @@ DISAS_INSN(move_to_usp)
         gen_exception(s, s->pc - 2, EXCP_PRIVILEGE);
         return;
     }
-    /* TODO: Implement USP.  */
-    gen_exception(s, s->pc - 2, EXCP_ILLEGAL);
+    tcg_gen_st_i32(AREG(insn, 0), cpu_env,
+                   offsetof(CPUM68KState, sp[M68K_USP]));
 }
 
 DISAS_INSN(halt)
-- 
1.9.1

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

[gerg]

From: Greg Ungerer <gerg@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.

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

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,  <gerg@uclinux.org> wrote:
> From: Greg Ungerer <gerg@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.

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

Otherwise,

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

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

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,  <gerg@uclinux.org> wrote:
>> From: Greg Ungerer <gerg@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 <gerg@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.


> Otherwise,
> 
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@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
>>
>>
> 

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 <gerg@uclinux.org> wrote:
> Hi Peter,
>
> On 19/06/15 15:49, Peter Crosthwaite wrote:
>> On Mon, Aug 18, 2014 at 10:37 PM,  <gerg@uclinux.org> wrote:
>>> From: Greg Ungerer <gerg@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 <gerg@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.crosthwaite@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
>>>
>>>
>>
>
>