* [Qemu-devel] m68k: More bug fixes for translation code
@ 2016-02-03 9:36 John Paul Adrian Glaubitz
2016-02-03 9:37 ` [Qemu-devel] [PATCH 1/2] m68k: Fix opcode mask for fbcc instruction John Paul Adrian Glaubitz
2016-02-03 9:37 ` [Qemu-devel] [PATCH 2/2] m68k: Build the opcode table only once to avoid multithreading issues John Paul Adrian Glaubitz
0 siblings, 2 replies; 12+ messages in thread
From: John Paul Adrian Glaubitz @ 2016-02-03 9:36 UTC (permalink / raw)
To: qemu-devel; +Cc: laurent
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
^ permalink raw reply [flat|nested] 12+ messages in thread* [Qemu-devel] [PATCH 1/2] m68k: Fix opcode mask for fbcc instruction 2016-02-03 9:36 [Qemu-devel] m68k: More bug fixes for translation code John Paul Adrian Glaubitz @ 2016-02-03 9:37 ` John Paul Adrian Glaubitz 2016-02-03 9:38 ` Laurent Vivier 2016-02-03 9:40 ` John Paul Adrian Glaubitz 2016-02-03 9:37 ` [Qemu-devel] [PATCH 2/2] m68k: Build the opcode table only once to avoid multithreading issues John Paul Adrian Glaubitz 1 sibling, 2 replies; 12+ messages in thread From: John Paul Adrian Glaubitz @ 2016-02-03 9:37 UTC (permalink / raw) To: qemu-devel; +Cc: laurent, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] m68k: Fix opcode mask for fbcc instruction 2016-02-03 9:37 ` [Qemu-devel] [PATCH 1/2] m68k: Fix opcode mask for fbcc instruction John Paul Adrian Glaubitz @ 2016-02-03 9:38 ` Laurent Vivier 2016-02-08 23:13 ` John Paul Adrian Glaubitz 2016-02-03 9:40 ` John Paul Adrian Glaubitz 1 sibling, 1 reply; 12+ messages in thread From: Laurent Vivier @ 2016-02-03 9:38 UTC (permalink / raw) To: John Paul Adrian Glaubitz, qemu-devel 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> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] m68k: Fix opcode mask for fbcc instruction 2016-02-03 9:38 ` Laurent Vivier @ 2016-02-08 23:13 ` John Paul Adrian Glaubitz 0 siblings, 0 replies; 12+ messages in thread From: John Paul Adrian Glaubitz @ 2016-02-08 23:13 UTC (permalink / raw) To: Laurent Vivier, qemu-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] m68k: Fix opcode mask for fbcc instruction 2016-02-03 9:37 ` [Qemu-devel] [PATCH 1/2] m68k: Fix opcode mask for fbcc instruction John Paul Adrian Glaubitz 2016-02-03 9:38 ` Laurent Vivier @ 2016-02-03 9:40 ` John Paul Adrian Glaubitz 2016-02-03 9:42 ` Laurent Vivier 1 sibling, 1 reply; 12+ messages in thread From: John Paul Adrian Glaubitz @ 2016-02-03 9:40 UTC (permalink / raw) To: qemu-devel; +Cc: laurent 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] m68k: Fix opcode mask for fbcc instruction 2016-02-03 9:40 ` John Paul Adrian Glaubitz @ 2016-02-03 9:42 ` Laurent Vivier 0 siblings, 0 replies; 12+ messages in thread From: Laurent Vivier @ 2016-02-03 9:42 UTC (permalink / raw) To: John Paul Adrian Glaubitz, qemu-devel 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); >> > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH 2/2] m68k: Build the opcode table only once to avoid multithreading issues 2016-02-03 9:36 [Qemu-devel] m68k: More bug fixes for translation code John Paul Adrian Glaubitz 2016-02-03 9:37 ` [Qemu-devel] [PATCH 1/2] m68k: Fix opcode mask for fbcc instruction John Paul Adrian Glaubitz @ 2016-02-03 9:37 ` John Paul Adrian Glaubitz 2016-02-03 9:39 ` Laurent Vivier 1 sibling, 1 reply; 12+ messages in thread From: John Paul Adrian Glaubitz @ 2016-02-03 9:37 UTC (permalink / raw) To: qemu-devel; +Cc: laurent, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] m68k: Build the opcode table only once to avoid multithreading issues 2016-02-03 9:37 ` [Qemu-devel] [PATCH 2/2] m68k: Build the opcode table only once to avoid multithreading issues John Paul Adrian Glaubitz @ 2016-02-03 9:39 ` Laurent Vivier 2016-02-03 9:57 ` Laurent Vivier 0 siblings, 1 reply; 12+ messages in thread From: Laurent Vivier @ 2016-02-03 9:39 UTC (permalink / raw) To: John Paul Adrian Glaubitz, qemu-devel 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> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] m68k: Build the opcode table only once to avoid multithreading issues 2016-02-03 9:39 ` Laurent Vivier @ 2016-02-03 9:57 ` Laurent Vivier 2016-02-03 10:06 ` John Paul Adrian Glaubitz 0 siblings, 1 reply; 12+ messages in thread From: Laurent Vivier @ 2016-02-03 9:57 UTC (permalink / raw) To: John Paul Adrian Glaubitz, qemu-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] m68k: Build the opcode table only once to avoid multithreading issues 2016-02-03 9:57 ` Laurent Vivier @ 2016-02-03 10:06 ` John Paul Adrian Glaubitz 2016-02-03 10:13 ` Laurent Vivier 0 siblings, 1 reply; 12+ messages in thread From: John Paul Adrian Glaubitz @ 2016-02-03 10:06 UTC (permalink / raw) To: Laurent Vivier, qemu-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] m68k: Build the opcode table only once to avoid multithreading issues 2016-02-03 10:06 ` John Paul Adrian Glaubitz @ 2016-02-03 10:13 ` Laurent Vivier 2016-02-03 10:17 ` John Paul Adrian Glaubitz 0 siblings, 1 reply; 12+ messages in thread From: Laurent Vivier @ 2016-02-03 10:13 UTC (permalink / raw) To: John Paul Adrian Glaubitz, qemu-devel, Peter Maydell 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] m68k: Build the opcode table only once to avoid multithreading issues 2016-02-03 10:13 ` Laurent Vivier @ 2016-02-03 10:17 ` John Paul Adrian Glaubitz 0 siblings, 0 replies; 12+ messages in thread From: John Paul Adrian Glaubitz @ 2016-02-03 10:17 UTC (permalink / raw) To: Laurent Vivier, qemu-devel, Peter Maydell 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-02-08 23:13 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-02-03 9:36 [Qemu-devel] m68k: More bug fixes for translation code John Paul Adrian Glaubitz 2016-02-03 9:37 ` [Qemu-devel] [PATCH 1/2] m68k: Fix opcode mask for fbcc instruction John Paul Adrian Glaubitz 2016-02-03 9:38 ` Laurent Vivier 2016-02-08 23:13 ` John Paul Adrian Glaubitz 2016-02-03 9:40 ` John Paul Adrian Glaubitz 2016-02-03 9:42 ` Laurent Vivier 2016-02-03 9:37 ` [Qemu-devel] [PATCH 2/2] m68k: Build the opcode table only once to avoid multithreading issues John Paul Adrian Glaubitz 2016-02-03 9:39 ` Laurent Vivier 2016-02-03 9:57 ` Laurent Vivier 2016-02-03 10:06 ` John Paul Adrian Glaubitz 2016-02-03 10:13 ` Laurent Vivier 2016-02-03 10:17 ` John Paul Adrian Glaubitz
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.