[Qemu-devel] m68k: More bug fixes for translation code

[Qemu-devel] m68k: More bug fixes for translation code

[John Paul Adrian Glaubitz]

Hi Laurent!

As promised, here are the fixes for the two recently discovered
bugs in the m68k translation code.

The first patch fixes the opcode mask for the fbcc instruction which
is currently incorrect as it masks the 6th bit as constant (0xffc0).
However, according to the ColdFire reference manual, this bit is
used to determine the size of the displacement for the jump, either
16 or 32 bits:

> http://www.nxp.com/files/dsp/doc/ref_manual/CFPRM.pdf (p. 229)

Looking at DISAS_INSN(fbcc), the emulated instruction actually tests
for the 6th bit and sets the offset accordingly. However, since the
current opcode mask ignores this bit, long jumps can never work. In
fact, what we actually see is an illegal instruction: 0xf2e0.

Changing the opcode mask to 0xff80 makes the 6th bit variable and
allows long jumps to work as expected.

The second patch addresses a problem with the thread safety of
register_m68k_insns(). It turns out, that the opcode table is
rebuild for every thread that is started which means that in
a multithreaded environment, one thread can destroy the opcode
table of a concurrent thread which makes this thread crash
with an illegal instruction.

This patch changes register_m68k_insns() such that it returns
without doing anything in case the opcode table has already been
built and re-registering the instructions is therefore not necessary
but rather harmful.

Credits go to Michael Karcher for helping to debug these issues!

Cheers,
Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

[Qemu-devel] [PATCH 1/2] m68k: Fix opcode mask for fbcc instruction

[John Paul Adrian Glaubitz]

Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
---
 target-m68k/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index 342c040..535d7f9 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -2940,7 +2940,7 @@ void register_m68k_insns (CPUM68KState *env)
     INSN(shift_reg, e0a0, f0f0, CF_ISA_A);
     INSN(undef_fpu, f000, f000, CF_ISA_A);
     INSN(fpu,       f200, ffc0, CF_FPU);
-    INSN(fbcc,      f280, ffc0, CF_FPU);
+    INSN(fbcc,      f280, ff80, CF_FPU);
     INSN(frestore,  f340, ffc0, CF_FPU);
     INSN(fsave,     f340, ffc0, CF_FPU);
     INSN(intouch,   f340, ffc0, CF_ISA_A);
-- 
2.7.0

[Qemu-devel] [PATCH 2/2] m68k: Build the opcode table only once to avoid multithreading issues

[John Paul Adrian Glaubitz]

Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
---
 target-m68k/translate.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index 535d7f9..f508d1e 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -2828,6 +2828,10 @@ register_opcode (disas_proc proc, uint16_t opcode, uint16_t mask)
    Later insn override earlier ones.  */
 void register_m68k_insns (CPUM68KState *env)
 {
+    /* Build the opcode table only once to avoid
+       issues with multithreading. */
+    if(opcode_table[0] != NULL)
+       return;
 #define INSN(name, opcode, mask, feature) do { \
     if (m68k_feature(env, M68K_FEATURE_##feature)) \
         register_opcode(disas_##name, 0x##opcode, 0x##mask); \
-- 
2.7.0

Re: [Qemu-devel] [PATCH 1/2] m68k: Fix opcode mask for fbcc instruction

[Laurent Vivier]

Le 03/02/2016 10:37, John Paul Adrian Glaubitz a écrit :
> Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> ---
>  target-m68k/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-m68k/translate.c b/target-m68k/translate.c
> index 342c040..535d7f9 100644
> --- a/target-m68k/translate.c
> +++ b/target-m68k/translate.c
> @@ -2940,7 +2940,7 @@ void register_m68k_insns (CPUM68KState *env)
>      INSN(shift_reg, e0a0, f0f0, CF_ISA_A);
>      INSN(undef_fpu, f000, f000, CF_ISA_A);
>      INSN(fpu,       f200, ffc0, CF_FPU);
> -    INSN(fbcc,      f280, ffc0, CF_FPU);
> +    INSN(fbcc,      f280, ff80, CF_FPU);
>      INSN(frestore,  f340, ffc0, CF_FPU);
>      INSN(fsave,     f340, ffc0, CF_FPU);
>      INSN(intouch,   f340, ffc0, CF_ISA_A);
> 

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

Re: [Qemu-devel] [PATCH 2/2] m68k: Build the opcode table only once to avoid multithreading issues

[Laurent Vivier]

Le 03/02/2016 10:37, John Paul Adrian Glaubitz a écrit :
> Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> ---
>  target-m68k/translate.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/target-m68k/translate.c b/target-m68k/translate.c
> index 535d7f9..f508d1e 100644
> --- a/target-m68k/translate.c
> +++ b/target-m68k/translate.c
> @@ -2828,6 +2828,10 @@ register_opcode (disas_proc proc, uint16_t opcode, uint16_t mask)
>     Later insn override earlier ones.  */
>  void register_m68k_insns (CPUM68KState *env)
>  {
> +    /* Build the opcode table only once to avoid
> +       issues with multithreading. */
> +    if(opcode_table[0] != NULL)
> +       return;
>  #define INSN(name, opcode, mask, feature) do { \
>      if (m68k_feature(env, M68K_FEATURE_##feature)) \
>          register_opcode(disas_##name, 0x##opcode, 0x##mask); \
> 
Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [Qemu-devel] [PATCH 1/2] m68k: Fix opcode mask for fbcc instruction

[John Paul Adrian Glaubitz]

Strange. There should be a cover letter coming along as well which
explains my changes. Did you get it?

On 02/03/2016 10:37 AM, John Paul Adrian Glaubitz wrote:
> Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> ---
>  target-m68k/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-m68k/translate.c b/target-m68k/translate.c
> index 342c040..535d7f9 100644
> --- a/target-m68k/translate.c
> +++ b/target-m68k/translate.c
> @@ -2940,7 +2940,7 @@ void register_m68k_insns (CPUM68KState *env)
>      INSN(shift_reg, e0a0, f0f0, CF_ISA_A);
>      INSN(undef_fpu, f000, f000, CF_ISA_A);
>      INSN(fpu,       f200, ffc0, CF_FPU);
> -    INSN(fbcc,      f280, ffc0, CF_FPU);
> +    INSN(fbcc,      f280, ff80, CF_FPU);
>      INSN(frestore,  f340, ffc0, CF_FPU);
>      INSN(fsave,     f340, ffc0, CF_FPU);
>      INSN(intouch,   f340, ffc0, CF_ISA_A);
> 


-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [Qemu-devel] [PATCH 1/2] m68k: Fix opcode mask for fbcc instruction

[Laurent Vivier]

Le 03/02/2016 10:40, John Paul Adrian Glaubitz a écrit :
> Strange. There should be a cover letter coming along as well which
> explains my changes. Did you get it?

We have the cover letter, but it is never sent to the sender :)

Laurent
> 
> On 02/03/2016 10:37 AM, John Paul Adrian Glaubitz wrote:
>> Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
>> ---
>>  target-m68k/translate.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target-m68k/translate.c b/target-m68k/translate.c
>> index 342c040..535d7f9 100644
>> --- a/target-m68k/translate.c
>> +++ b/target-m68k/translate.c
>> @@ -2940,7 +2940,7 @@ void register_m68k_insns (CPUM68KState *env)
>>      INSN(shift_reg, e0a0, f0f0, CF_ISA_A);
>>      INSN(undef_fpu, f000, f000, CF_ISA_A);
>>      INSN(fpu,       f200, ffc0, CF_FPU);
>> -    INSN(fbcc,      f280, ffc0, CF_FPU);
>> +    INSN(fbcc,      f280, ff80, CF_FPU);
>>      INSN(frestore,  f340, ffc0, CF_FPU);
>>      INSN(fsave,     f340, ffc0, CF_FPU);
>>      INSN(intouch,   f340, ffc0, CF_ISA_A);
>>
> 
> 

Re: [Qemu-devel] [PATCH 2/2] m68k: Build the opcode table only once to avoid multithreading issues

[Laurent Vivier]

Le 03/02/2016 10:39, Laurent Vivier a écrit :
> 
> 
> Le 03/02/2016 10:37, John Paul Adrian Glaubitz a écrit :
>> Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
>> ---
>>  target-m68k/translate.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/target-m68k/translate.c b/target-m68k/translate.c
>> index 535d7f9..f508d1e 100644
>> --- a/target-m68k/translate.c
>> +++ b/target-m68k/translate.c
>> @@ -2828,6 +2828,10 @@ register_opcode (disas_proc proc, uint16_t opcode, uint16_t mask)
>>     Later insn override earlier ones.  */
>>  void register_m68k_insns (CPUM68KState *env)
>>  {
>> +    /* Build the opcode table only once to avoid
>> +       issues with multithreading. */
>> +    if(opcode_table[0] != NULL)
>> +       return;
>>  #define INSN(name, opcode, mask, feature) do { \
>>      if (m68k_feature(env, M68K_FEATURE_##feature)) \
>>          register_opcode(disas_##name, 0x##opcode, 0x##mask); \
>>
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>

In fact, the line should be:

+    if (opcode_table[0] != NULL)
+        return;

thanks to scripts/checkpatch.pl.

Laurent

Re: [Qemu-devel] [PATCH 2/2] m68k: Build the opcode table only once to avoid multithreading issues

[John Paul Adrian Glaubitz]

On 02/03/2016 10:57 AM, Laurent Vivier wrote:
> In fact, the line should be:
> 
> +    if (opcode_table[0] != NULL)
> +        return;
> 
> thanks to scripts/checkpatch.pl.

Want me to re-send it?

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [Qemu-devel] [PATCH 2/2] m68k: Build the opcode table only once to avoid multithreading issues

[Laurent Vivier]

Le 03/02/2016 11:06, John Paul Adrian Glaubitz a écrit :
> On 02/03/2016 10:57 AM, Laurent Vivier wrote:
>> In fact, the line should be:
>>
>> +    if (opcode_table[0] != NULL)
>> +        return;
>>
>> thanks to scripts/checkpatch.pl.
> 
> Want me to re-send it?
> 

I don't know if the one who will commit this to the tree will want to
update this.

BTW, Peter, perhaps it's the time to add me as m68k maintainer ?
(so I will manage that :) )

Laurent

Re: [Qemu-devel] [PATCH 2/2] m68k: Build the opcode table only once to avoid multithreading issues

[John Paul Adrian Glaubitz]

On 02/03/2016 11:13 AM, Laurent Vivier wrote:
> I don't know if the one who will commit this to the tree will want to
> update this.

Just sent an updated one, just in case :).

> BTW, Peter, perhaps it's the time to add me as m68k maintainer ?
> (so I will manage that :) )

Yes, please. I'm all for that! I'm really motivated to get m68k
support into best shape. qemu-m68k is fun to hack on!

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [Qemu-devel] [PATCH 1/2] m68k: Fix opcode mask for fbcc instruction

[John Paul Adrian Glaubitz]

On 02/03/2016 10:38 AM, Laurent Vivier wrote:
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Any chance to get this merged quickly?

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913