[PATCH 0/2] target/m68k: fix two writes to %sr

[PATCH 0/2] target/m68k: fix two writes to %sr

[Richard Henderson]

The second was described by Mark in the lobby of KVM Forum.
The first was found by inspection of other uses of gen_helper_set_sr.

r~

Richard Henderson (2):
  target/m68k: Fix MACSR to CCR
  target/m68k: Perform writback before modifying SR

 target/m68k/translate.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH 1/2] target/m68k: Fix MACSR to CCR

[Richard Henderson]

First, we were writing to the entire SR register, instead
of only the flags portion.  Second, we were not clearing C
as per the documentation (X was cleared via the 0xf mask).

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/m68k/translate.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 5098f7e570..87044382c3 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -5892,8 +5892,10 @@ DISAS_INSN(from_mext)
 DISAS_INSN(macsr_to_ccr)
 {
     TCGv tmp = tcg_temp_new();
-    tcg_gen_andi_i32(tmp, QREG_MACSR, 0xf);
-    gen_helper_set_sr(cpu_env, tmp);
+
+    /* Note that X and C are always cleared. */
+    tcg_gen_andi_i32(tmp, QREG_MACSR, CCF_N | CCF_Z | CCF_V);
+    gen_helper_set_ccr(cpu_env, tmp);
     tcg_temp_free(tmp);
     set_cc_op(s, CC_OP_FLAGS);
 }
-- 
2.34.1

[PATCH 2/2] target/m68k: Perform writback before modifying SR

[Richard Henderson]

Writes to SR may change security state, which may involve
a swap of %ssp with %usp as reflected in %a7.  Finish the
writeback of %sp@+ before swapping stack pointers.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1206
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/m68k/translate.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 87044382c3..8506da0a0b 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -2285,9 +2285,9 @@ static void gen_set_sr_im(DisasContext *s, uint16_t val, int ccr_only)
         tcg_gen_movi_i32(QREG_CC_N, val & CCF_N ? -1 : 0);
         tcg_gen_movi_i32(QREG_CC_X, val & CCF_X ? 1 : 0);
     } else {
-        TCGv sr = tcg_const_i32(val);
-        gen_helper_set_sr(cpu_env, sr);
-        tcg_temp_free(sr);
+        /* Must writeback before changing security state. */
+        do_writebacks(s);
+        gen_helper_set_sr(cpu_env, tcg_constant_i32(val));
     }
     set_cc_op(s, CC_OP_FLAGS);
 }
@@ -2297,6 +2297,8 @@ static void gen_set_sr(DisasContext *s, TCGv val, int ccr_only)
     if (ccr_only) {
         gen_helper_set_ccr(cpu_env, val);
     } else {
+        /* Must writeback before changing security state. */
+        do_writebacks(s);
         gen_helper_set_sr(cpu_env, val);
     }
     set_cc_op(s, CC_OP_FLAGS);
-- 
2.34.1

Re: [PATCH 2/2] target/m68k: Perform writback before modifying SR

[Laurent Vivier]

Le 13/09/2022 à 16:28, Richard Henderson a écrit :
> Writes to SR may change security state, which may involve
> a swap of %ssp with %usp as reflected in %a7.  Finish the
> writeback of %sp@+ before swapping stack pointers.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1206
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/m68k/translate.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 

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

Re: [PATCH 1/2] target/m68k: Fix MACSR to CCR

[Laurent Vivier]

Le 13/09/2022 à 16:28, Richard Henderson a écrit :
> First, we were writing to the entire SR register, instead
> of only the flags portion.  Second, we were not clearing C
> as per the documentation (X was cleared via the 0xf mask).
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/m68k/translate.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index 5098f7e570..87044382c3 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -5892,8 +5892,10 @@ DISAS_INSN(from_mext)
>   DISAS_INSN(macsr_to_ccr)
>   {
>       TCGv tmp = tcg_temp_new();
> -    tcg_gen_andi_i32(tmp, QREG_MACSR, 0xf);
> -    gen_helper_set_sr(cpu_env, tmp);
> +
> +    /* Note that X and C are always cleared. */
> +    tcg_gen_andi_i32(tmp, QREG_MACSR, CCF_N | CCF_Z | CCF_V);
> +    gen_helper_set_ccr(cpu_env, tmp);
>       tcg_temp_free(tmp);
>       set_cc_op(s, CC_OP_FLAGS);
>   }

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

Re: [PATCH 2/2] target/m68k: Perform writback before modifying SR

[Mark Cave-Ayland]

On 13/09/2022 15:28, Richard Henderson wrote:

> Writes to SR may change security state, which may involve
> a swap of %ssp with %usp as reflected in %a7.  Finish the
> writeback of %sp@+ before swapping stack pointers.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1206
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/m68k/translate.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index 87044382c3..8506da0a0b 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -2285,9 +2285,9 @@ static void gen_set_sr_im(DisasContext *s, uint16_t val, int ccr_only)
>           tcg_gen_movi_i32(QREG_CC_N, val & CCF_N ? -1 : 0);
>           tcg_gen_movi_i32(QREG_CC_X, val & CCF_X ? 1 : 0);
>       } else {
> -        TCGv sr = tcg_const_i32(val);
> -        gen_helper_set_sr(cpu_env, sr);
> -        tcg_temp_free(sr);
> +        /* Must writeback before changing security state. */
> +        do_writebacks(s);
> +        gen_helper_set_sr(cpu_env, tcg_constant_i32(val));
>       }
>       set_cc_op(s, CC_OP_FLAGS);
>   }
> @@ -2297,6 +2297,8 @@ static void gen_set_sr(DisasContext *s, TCGv val, int ccr_only)
>       if (ccr_only) {
>           gen_helper_set_ccr(cpu_env, val);
>       } else {
> +        /* Must writeback before changing security state. */
> +        do_writebacks(s);
>           gen_helper_set_sr(cpu_env, val);
>       }
>       set_cc_op(s, CC_OP_FLAGS);

Thanks Richard! Subject needs s/writback/writeback/ but anyhow:

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


ATB,

Mark.

Re: [PATCH 0/2] target/m68k: fix two writes to %sr

[Mark Cave-Ayland]

On 13/09/2022 15:28, Richard Henderson wrote:

> The second was described by Mark in the lobby of KVM Forum.
> The first was found by inspection of other uses of gen_helper_set_sr.
> 
> r~
> 
> Richard Henderson (2):
>    target/m68k: Fix MACSR to CCR
>    target/m68k: Perform writback before modifying SR
> 
>   target/m68k/translate.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)

I've applied these on top of my MacOS virtual memory branch at 
https://github.com/mcayland/qemu/commits/q800.upstream2-vm and I can confirm that 
MacOS 8.1 now boots here with virtual memory enabled :)

Possibly it might be worth including a tidied-up version of the "WIP: target/m68k: 
always exit_tb when changing sr with andi/ori/eori" commit from that branch which is 
also related to switching between supervisor and user modes under MacOS. Shall I tidy 
it up and send it to the list?


ATB,

Mark.

Re: [PATCH 0/2] target/m68k: fix two writes to %sr

[Laurent Vivier]

Le 13/09/2022 à 18:29, Mark Cave-Ayland a écrit :
> On 13/09/2022 15:28, Richard Henderson wrote:
> 
>> The second was described by Mark in the lobby of KVM Forum.
>> The first was found by inspection of other uses of gen_helper_set_sr.
>>
>> r~
>>
>> Richard Henderson (2):
>>    target/m68k: Fix MACSR to CCR
>>    target/m68k: Perform writback before modifying SR
>>
>>   target/m68k/translate.c | 14 +++++++++-----
>>   1 file changed, 9 insertions(+), 5 deletions(-)
> 
> I've applied these on top of my MacOS virtual memory branch at 
> https://github.com/mcayland/qemu/commits/q800.upstream2-vm and I can confirm that MacOS 8.1 now 
> boots here with virtual memory enabled :)
> 
> Possibly it might be worth including a tidied-up version of the "WIP: target/m68k: always exit_tb 
> when changing sr with andi/ori/eori" commit from that branch which is also related to switching 
> between supervisor and user modes under MacOS. Shall I tidy it up and send it to the list?

Yes, send it to the list. I plan to do a PR for m68k soon.

Thanks,
Laurent

Re: [PATCH 0/2] target/m68k: fix two writes to %sr

[Richard Henderson]

On 9/13/22 17:29, Mark Cave-Ayland wrote:
> Possibly it might be worth including a tidied-up version of the "WIP: target/m68k: always 
> exit_tb when changing sr with andi/ori/eori" commit from that branch which is also related 
> to switching between supervisor and user modes under MacOS. Shall I tidy it up and send it 
> to the list?

I peeked at the patch in your tree, and it looks good.
By inspection, strldsr needs to exit the TB as well.


r~

Re: [PATCH 0/2] target/m68k: fix two writes to %sr

[Howard Spoelstra]

[-- Attachment #1: Type: text/plain, Size: 1390 bytes --]

On Tue, Sep 13, 2022 at 6:29 PM Mark Cave-Ayland <
mark.cave-ayland@ilande.co.uk> wrote:

> On 13/09/2022 15:28, Richard Henderson wrote:
>
> > The second was described by Mark in the lobby of KVM Forum.
> > The first was found by inspection of other uses of gen_helper_set_sr.
> >
> > r~
> >
> > Richard Henderson (2):
> >    target/m68k: Fix MACSR to CCR
> >    target/m68k: Perform writback before modifying SR
> >
> >   target/m68k/translate.c | 14 +++++++++-----
> >   1 file changed, 9 insertions(+), 5 deletions(-)
>
> I've applied these on top of my MacOS virtual memory branch at
> https://github.com/mcayland/qemu/commits/q800.upstream2-vm and I can
> confirm that
> MacOS 8.1 now boots here with virtual memory enabled :)
>
> Possibly it might be worth including a tidied-up version of the "WIP:
> target/m68k:
> always exit_tb when changing sr with andi/ori/eori" commit from that
> branch which is
> also related to switching between supervisor and user modes under MacOS.
> Shall I tidy
> it up and send it to the list?
>
>
> ATB,
>
> Mark.
>
>
I've compiled the branch mentioned above with a fully updated MSYS2 on
windows. The executable hangs when running Mac OS 8 with Virtual Memory
enabled. On a fast machine I see Error 7 as before, on a slower machine,
the boot screen just hangs with no error shown. A Linux build does work,
also on the slower machine.

Best,
Howard

[-- Attachment #2: Type: text/html, Size: 2025 bytes --]

Re: [PATCH 0/2] target/m68k: fix two writes to %sr

[Howard Spoelstra]

[-- Attachment #1: Type: text/plain, Size: 1616 bytes --]

On Wed, Sep 14, 2022 at 6:48 PM Howard Spoelstra <hsp.cat7@gmail.com> wrote:

>
>
> On Tue, Sep 13, 2022 at 6:29 PM Mark Cave-Ayland <
> mark.cave-ayland@ilande.co.uk> wrote:
>
>> On 13/09/2022 15:28, Richard Henderson wrote:
>>
>> > The second was described by Mark in the lobby of KVM Forum.
>> > The first was found by inspection of other uses of gen_helper_set_sr.
>> >
>> > r~
>> >
>> > Richard Henderson (2):
>> >    target/m68k: Fix MACSR to CCR
>> >    target/m68k: Perform writback before modifying SR
>> >
>> >   target/m68k/translate.c | 14 +++++++++-----
>> >   1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> I've applied these on top of my MacOS virtual memory branch at
>> https://github.com/mcayland/qemu/commits/q800.upstream2-vm and I can
>> confirm that
>> MacOS 8.1 now boots here with virtual memory enabled :)
>>
>> Possibly it might be worth including a tidied-up version of the "WIP:
>> target/m68k:
>> always exit_tb when changing sr with andi/ori/eori" commit from that
>> branch which is
>> also related to switching between supervisor and user modes under MacOS.
>> Shall I tidy
>> it up and send it to the list?
>>
>>
>> ATB,
>>
>> Mark.
>>
>>
> I've compiled the branch mentioned above with a fully updated MSYS2 on
> windows. The executable hangs when running Mac OS 8 with Virtual Memory
> enabled. On a fast machine I see Error 7 as before, on a slower machine,
> the boot screen just hangs with no error shown. A Linux build does work,
> also on the slower machine.
>
> Best,
> Howard
>

ps: a debug enabled non-stripped build will run with Virtual Memory enabled
on Windows.

[-- Attachment #2: Type: text/html, Size: 2540 bytes --]

Re: [PATCH 1/2] target/m68k: Fix MACSR to CCR

[Laurent Vivier]

Le 13/09/2022 à 16:28, Richard Henderson a écrit :
> First, we were writing to the entire SR register, instead
> of only the flags portion.  Second, we were not clearing C
> as per the documentation (X was cleared via the 0xf mask).
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/m68k/translate.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index 5098f7e570..87044382c3 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -5892,8 +5892,10 @@ DISAS_INSN(from_mext)
>   DISAS_INSN(macsr_to_ccr)
>   {
>       TCGv tmp = tcg_temp_new();
> -    tcg_gen_andi_i32(tmp, QREG_MACSR, 0xf);
> -    gen_helper_set_sr(cpu_env, tmp);
> +
> +    /* Note that X and C are always cleared. */
> +    tcg_gen_andi_i32(tmp, QREG_MACSR, CCF_N | CCF_Z | CCF_V);
> +    gen_helper_set_ccr(cpu_env, tmp);
>       tcg_temp_free(tmp);
>       set_cc_op(s, CC_OP_FLAGS);
>   }

Applied to my m68k-for-7.2 branch

Thanks,
Laurent

Re: [PATCH 2/2] target/m68k: Perform writback before modifying SR

[Laurent Vivier]

Le 13/09/2022 à 16:28, Richard Henderson a écrit :
> Writes to SR may change security state, which may involve
> a swap of %ssp with %usp as reflected in %a7.  Finish the
> writeback of %sp@+ before swapping stack pointers.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1206
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/m68k/translate.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index 87044382c3..8506da0a0b 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -2285,9 +2285,9 @@ static void gen_set_sr_im(DisasContext *s, uint16_t val, int ccr_only)
>           tcg_gen_movi_i32(QREG_CC_N, val & CCF_N ? -1 : 0);
>           tcg_gen_movi_i32(QREG_CC_X, val & CCF_X ? 1 : 0);
>       } else {
> -        TCGv sr = tcg_const_i32(val);
> -        gen_helper_set_sr(cpu_env, sr);
> -        tcg_temp_free(sr);
> +        /* Must writeback before changing security state. */
> +        do_writebacks(s);
> +        gen_helper_set_sr(cpu_env, tcg_constant_i32(val));
>       }
>       set_cc_op(s, CC_OP_FLAGS);
>   }
> @@ -2297,6 +2297,8 @@ static void gen_set_sr(DisasContext *s, TCGv val, int ccr_only)
>       if (ccr_only) {
>           gen_helper_set_ccr(cpu_env, val);
>       } else {
> +        /* Must writeback before changing security state. */
> +        do_writebacks(s);
>           gen_helper_set_sr(cpu_env, val);
>       }
>       set_cc_op(s, CC_OP_FLAGS);

Applied to my m68k-for-7.2 branch

Thanks,
Laurent