[PATCH 0/2] target/m68k: MacOS related fixes

[PATCH 0/2] target/m68k: MacOS related fixes

[Mark Cave-Ayland]

Here are a couple of extra target/m68k patches taken from my attempts to try
and boot MacOS.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Mark Cave-Ayland (2):
  target/m68k: don't set SSW ATC bit for physical bus errors
  target/m68k: add M68K_FEATURE_NO_DALIGN feature

 target/m68k/cpu.c       |  1 +
 target/m68k/cpu.h       |  1 +
 target/m68k/op_helper.c | 17 +++++++++++++++--
 3 files changed, 17 insertions(+), 2 deletions(-)

-- 
2.20.1

[PATCH 1/2] target/m68k: don't set SSW ATC bit for physical bus errors

[Mark Cave-Ayland]

If a NuBus slot doesn't contain a card, the Quadra hardware generates a physical
bus error if the CPU attempts to access the slot address space. Both Linux and
MacOS use a separate bus error handler during NuBus accesses in order to detect
and recover when addressing empty slots.

According to the MC68040 users manual the ATC bit of the SSW is used to
distinguish between ATC faults and physical bus errors. MacOS specifically checks
the stack frame generated by a NuBus error and panics if the SSW ATC bit is set.

Update m68k_cpu_transaction_failed() so that the SSW ATC bit is not set if the
memory API returns MEMTX_DECODE_ERROR which will be used to indicate that an
access to an empty NuBus slot occurred.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target/m68k/op_helper.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
index 202498deb5..59a6448296 100644
--- a/target/m68k/op_helper.c
+++ b/target/m68k/op_helper.c
@@ -468,7 +468,17 @@ void m68k_cpu_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr,
 
     if (m68k_feature(env, M68K_FEATURE_M68040)) {
         env->mmu.mmusr = 0;
-        env->mmu.ssw |= M68K_ATC_040;
+
+        /*
+         * According to the MC68040 users manual the ATC bit of the SSW is
+         * used to distinguish between ATC faults and physical bus errors.
+         * In the case of a bus error e.g. during nubus read from an empty
+         * slot this bit should not be set
+         */
+        if (response != MEMTX_DECODE_ERROR) {
+            env->mmu.ssw |= M68K_ATC_040;
+        }
+
         /* FIXME: manage MMU table access error */
         env->mmu.ssw &= ~M68K_TM_040;
         if (env->sr & SR_S) { /* SUPERVISOR */
-- 
2.20.1

Re: [PATCH 1/2] target/m68k: don't set SSW ATC bit for physical bus errors

[Laurent Vivier]

Le 07/03/2021 à 21:26, Mark Cave-Ayland a écrit :
> If a NuBus slot doesn't contain a card, the Quadra hardware generates a physical
> bus error if the CPU attempts to access the slot address space. Both Linux and
> MacOS use a separate bus error handler during NuBus accesses in order to detect
> and recover when addressing empty slots.
> 
> According to the MC68040 users manual the ATC bit of the SSW is used to
> distinguish between ATC faults and physical bus errors. MacOS specifically checks
> the stack frame generated by a NuBus error and panics if the SSW ATC bit is set.
> 
> Update m68k_cpu_transaction_failed() so that the SSW ATC bit is not set if the
> memory API returns MEMTX_DECODE_ERROR which will be used to indicate that an
> access to an empty NuBus slot occurred.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  target/m68k/op_helper.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
> index 202498deb5..59a6448296 100644
> --- a/target/m68k/op_helper.c
> +++ b/target/m68k/op_helper.c
> @@ -468,7 +468,17 @@ void m68k_cpu_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr,
>  
>      if (m68k_feature(env, M68K_FEATURE_M68040)) {
>          env->mmu.mmusr = 0;
> -        env->mmu.ssw |= M68K_ATC_040;
> +
> +        /*
> +         * According to the MC68040 users manual the ATC bit of the SSW is
> +         * used to distinguish between ATC faults and physical bus errors.
> +         * In the case of a bus error e.g. during nubus read from an empty
> +         * slot this bit should not be set
> +         */
> +        if (response != MEMTX_DECODE_ERROR) {
> +            env->mmu.ssw |= M68K_ATC_040;
> +        }
> +
>          /* FIXME: manage MMU table access error */
>          env->mmu.ssw &= ~M68K_TM_040;
>          if (env->sr & SR_S) { /* SUPERVISOR */
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

[PATCH 2/2] target/m68k: add M68K_FEATURE_NO_DALIGN feature

[Mark Cave-Ayland]

According to the M68040UM Appendix D the requirement for data accesses to be
word aligned is only for the 68000, 68008 and 68010 CPUs. Later CPUs from the
68020 onwards will allow unaligned data accesses but at the cost of being less
efficient.

Add a new M68K_FEATURE_NO_DALIGN feature to specify that data accesses are not
required to be word aligned, and don't perform the alignment on the stack
pointer when taking an exception if this feature is not selected.

This is required because the MacOS DAFB driver attempts to call an A-trap
with a byte-aligned stack pointer during initialisation and without this the
stack pointer is off by one when the A-trap returns.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target/m68k/cpu.c       | 1 +
 target/m68k/cpu.h       | 1 +
 target/m68k/op_helper.c | 5 ++++-
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 37d2ed9dc7..ea51753eb0 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -161,6 +161,7 @@ static void m68020_cpu_initfn(Object *obj)
     m68k_set_feature(env, M68K_FEATURE_CAS);
     m68k_set_feature(env, M68K_FEATURE_CHK2);
     m68k_set_feature(env, M68K_FEATURE_MSP);
+    m68k_set_feature(env, M68K_FEATURE_NO_DALIGN);
 }
 
 /*
diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index 7c3feeaf8a..1e0876bba8 100644
--- a/target/m68k/cpu.h
+++ b/target/m68k/cpu.h
@@ -505,6 +505,7 @@ enum m68k_features {
     M68K_FEATURE_CHK2,  /* CHK2 insn. (680[2346]0, and CPU32) */
     M68K_FEATURE_MOVEP, /* MOVEP insn. (680[01234]0, and CPU32) */
     M68K_FEATURE_MOVEC, /* MOVEC insn. (from 68010) */
+    M68K_FEATURE_NO_DALIGN, /* Unaligned data accesses (680[2346]0) */
 };
 
 static inline int m68k_feature(CPUM68KState *env, int feature)
diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
index 59a6448296..71b3df0910 100644
--- a/target/m68k/op_helper.c
+++ b/target/m68k/op_helper.c
@@ -348,7 +348,10 @@ static void m68k_interrupt_all(CPUM68KState *env, int is_hw)
     cpu_m68k_set_sr(env, sr);
     sp = env->aregs[7];
 
-    sp &= ~1;
+    if (!m68k_feature(env, M68K_FEATURE_NO_DALIGN)) {
+        sp &= ~1;
+    }
+
     if (cs->exception_index == EXCP_ACCESS) {
         if (env->mmu.fault) {
             cpu_abort(cs, "DOUBLE MMU FAULT\n");
-- 
2.20.1

Re: [PATCH 2/2] target/m68k: add M68K_FEATURE_NO_DALIGN feature

[Laurent Vivier]

Le 07/03/2021 à 21:26, Mark Cave-Ayland a écrit :
> According to the M68040UM Appendix D the requirement for data accesses to be
> word aligned is only for the 68000, 68008 and 68010 CPUs. Later CPUs from the
> 68020 onwards will allow unaligned data accesses but at the cost of being less
> efficient.
> 
> Add a new M68K_FEATURE_NO_DALIGN feature to specify that data accesses are not
> required to be word aligned, and don't perform the alignment on the stack
> pointer when taking an exception if this feature is not selected.
> 
> This is required because the MacOS DAFB driver attempts to call an A-trap
> with a byte-aligned stack pointer during initialisation and without this the
> stack pointer is off by one when the A-trap returns.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  target/m68k/cpu.c       | 1 +
>  target/m68k/cpu.h       | 1 +
>  target/m68k/op_helper.c | 5 ++++-
>  3 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index 37d2ed9dc7..ea51753eb0 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -161,6 +161,7 @@ static void m68020_cpu_initfn(Object *obj)
>      m68k_set_feature(env, M68K_FEATURE_CAS);
>      m68k_set_feature(env, M68K_FEATURE_CHK2);
>      m68k_set_feature(env, M68K_FEATURE_MSP);
> +    m68k_set_feature(env, M68K_FEATURE_NO_DALIGN);
>  }
>  
>  /*
> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
> index 7c3feeaf8a..1e0876bba8 100644
> --- a/target/m68k/cpu.h
> +++ b/target/m68k/cpu.h
> @@ -505,6 +505,7 @@ enum m68k_features {
>      M68K_FEATURE_CHK2,  /* CHK2 insn. (680[2346]0, and CPU32) */
>      M68K_FEATURE_MOVEP, /* MOVEP insn. (680[01234]0, and CPU32) */
>      M68K_FEATURE_MOVEC, /* MOVEC insn. (from 68010) */
> +    M68K_FEATURE_NO_DALIGN, /* Unaligned data accesses (680[2346]0) */
>  };
>  
>  static inline int m68k_feature(CPUM68KState *env, int feature)
> diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
> index 59a6448296..71b3df0910 100644
> --- a/target/m68k/op_helper.c
> +++ b/target/m68k/op_helper.c
> @@ -348,7 +348,10 @@ static void m68k_interrupt_all(CPUM68KState *env, int is_hw)
>      cpu_m68k_set_sr(env, sr);
>      sp = env->aregs[7];
>  
> -    sp &= ~1;
> +    if (!m68k_feature(env, M68K_FEATURE_NO_DALIGN)) {
> +        sp &= ~1;
> +    }
> +
>      if (cs->exception_index == EXCP_ACCESS) {
>          if (env->mmu.fault) {
>              cpu_abort(cs, "DOUBLE MMU FAULT\n");
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 2/2] target/m68k: add M68K_FEATURE_NO_DALIGN feature

[Richard Henderson]

On 3/7/21 12:26 PM, Mark Cave-Ayland wrote:
> According to the M68040UM Appendix D the requirement for data accesses to be
> word aligned is only for the 68000, 68008 and 68010 CPUs. Later CPUs from the
> 68020 onwards will allow unaligned data accesses but at the cost of being less
> efficient.
> 
> Add a new M68K_FEATURE_NO_DALIGN feature to specify that data accesses are not
> required to be word aligned, and don't perform the alignment on the stack
> pointer when taking an exception if this feature is not selected.

How about a positive, rather than negative name, like M68K_FEATURE_UNALIGNED_DATA?

This points out that we should be raising Address Error without this feature. 
This requires a moderate amount of cleanup in translate, manipulating 
MO_ALIGN{,_2} as part of the MemOp parameter to tcg_gen_qemu_{ld,st}_i32.


r~

Re: [PATCH 2/2] target/m68k: add M68K_FEATURE_NO_DALIGN feature

[Mark Cave-Ayland]

On 08/03/2021 01:03, Richard Henderson wrote:

> On 3/7/21 12:26 PM, Mark Cave-Ayland wrote:
>> According to the M68040UM Appendix D the requirement for data accesses to be
>> word aligned is only for the 68000, 68008 and 68010 CPUs. Later CPUs from the
>> 68020 onwards will allow unaligned data accesses but at the cost of being less
>> efficient.
>>
>> Add a new M68K_FEATURE_NO_DALIGN feature to specify that data accesses are not
>> required to be word aligned, and don't perform the alignment on the stack
>> pointer when taking an exception if this feature is not selected.
> 
> How about a positive, rather than negative name, like M68K_FEATURE_UNALIGNED_DATA?

Sure - I'll update that, and send out a v2.

> This points out that we should be raising Address Error without this feature. This 
> requires a moderate amount of cleanup in translate, manipulating MO_ALIGN{,_2} as 
> part of the MemOp parameter to tcg_gen_qemu_{ld,st}_i32.

That's probably true, although all the images I have here for testing are for 040 
CPUs only. There is another set of m68k fixes I'd like to get in before freeze which 
also affect Linux, so those are next on my priority list.

I'll send out v2 tentatively with Laurent's R-B tags added, and then leave it to 
others to decide whether detecting alignment errors can be done as a follow-up later 
(certainly with the last patch there should be no regression).


ATB,

Mark.