| Message ID | 1513006964-3371-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [for-2.11?] target/arm: Generate UNDEF for 32-bit Thumb2 insns | expand |
On 12/11/2017 07:42 AM, Peter Maydell wrote: > The refactoring of commit 296e5a0a6c3935 has a nasty bug: > it accidentally dropped the generation of code to raise > the UNDEF exception when disas_thumb2_insn() returns nonzero. > This means that 32-bit Thumb2 instruction patterns that > ought to UNDEF just act like nops instead. This is likely > to break any number of things, including the kernel's "disable > the FPU and use the UNDEF exception to identify when to turn > it back on again" trick. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > This is the smallest possible fix that will correct the > bug, for possible inclusion in 2.11; for 2.12 we should > fix the asymmetry where disas_thumb() generates its own > exception-raising code but disas_thumb2() wants the caller > to do it. (This asymmetry is why we didn't notice the > problem in code review.) > > I'm not sure whether this should go into 2.11 or not -- > this time last week it would have been an easy "yes". Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 11 December 2017 at 17:00, Richard Henderson <rth@twiddle.net> wrote: > On 12/11/2017 07:42 AM, Peter Maydell wrote: >> The refactoring of commit 296e5a0a6c3935 has a nasty bug: >> it accidentally dropped the generation of code to raise >> the UNDEF exception when disas_thumb2_insn() returns nonzero. >> This means that 32-bit Thumb2 instruction patterns that >> ought to UNDEF just act like nops instead. This is likely >> to break any number of things, including the kernel's "disable >> the FPU and use the UNDEF exception to identify when to turn >> it back on again" trick. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> This is the smallest possible fix that will correct the >> bug, for possible inclusion in 2.11; for 2.12 we should >> fix the asymmetry where disas_thumb() generates its own >> exception-raising code but disas_thumb2() wants the caller >> to do it. (This asymmetry is why we didn't notice the >> problem in code review.) >> >> I'm not sure whether this should go into 2.11 or not -- >> this time last week it would have been an easy "yes". > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Thanks. I think I have come down on the side of putting this into 2.11, so rolling an rc5 today, and delaying the final release a day to Wednesday. thanks -- PMM
On Mon, Dec 11, 2017 at 17:32:48 +0000, Peter Maydell wrote: > Thanks. I think I have come down on the side of putting this into > 2.11, so rolling an rc5 today, and delaying the final release > a day to Wednesday. Glad to see it's in -rc5 -- thanks for fixing this so quickly! Again, apologies for not having caught this earlier ;-( Emilio
On 11 December 2017 at 19:42, Emilio G. Cota <cota@braap.org> wrote: > On Mon, Dec 11, 2017 at 17:32:48 +0000, Peter Maydell wrote: >> Thanks. I think I have come down on the side of putting this into >> 2.11, so rolling an rc5 today, and delaying the final release >> a day to Wednesday. > > Glad to see it's in -rc5 -- thanks for fixing this so quickly! > > Again, apologies for not having caught this earlier ;-( It's my own fault really -- my extremely ad-hoc approach to testing for Arm guests was bound to come back and bite me sooner or later. thanks -- PMM
On Mon, Dec 11, 2017 at 08:55:56PM +0000, Peter Maydell wrote: > On 11 December 2017 at 19:42, Emilio G. Cota <cota@braap.org> wrote: > > On Mon, Dec 11, 2017 at 17:32:48 +0000, Peter Maydell wrote: > >> Thanks. I think I have come down on the side of putting this into > >> 2.11, so rolling an rc5 today, and delaying the final release > >> a day to Wednesday. > > > > Glad to see it's in -rc5 -- thanks for fixing this so quickly! > > > > Again, apologies for not having caught this earlier ;-( > > It's my own fault really -- my extremely ad-hoc approach > to testing for Arm guests was bound to come back and > bite me sooner or later. Should we include the vfio fix in too for rc5? http://patchwork.ozlabs.org/patch/844940/ I see that the tag is there already, not sure whether it means it missed the chance again... Thanks,
On 12 December 2017 at 02:48, Peter Xu <peterx@redhat.com> wrote: > On Mon, Dec 11, 2017 at 08:55:56PM +0000, Peter Maydell wrote: >> On 11 December 2017 at 19:42, Emilio G. Cota <cota@braap.org> wrote: >> > On Mon, Dec 11, 2017 at 17:32:48 +0000, Peter Maydell wrote: >> >> Thanks. I think I have come down on the side of putting this into >> >> 2.11, so rolling an rc5 today, and delaying the final release >> >> a day to Wednesday. >> > >> > Glad to see it's in -rc5 -- thanks for fixing this so quickly! >> > >> > Again, apologies for not having caught this earlier ;-( >> >> It's my own fault really -- my extremely ad-hoc approach >> to testing for Arm guests was bound to come back and >> bite me sooner or later. > > Should we include the vfio fix in too for rc5? > > http://patchwork.ozlabs.org/patch/844940/ > > I see that the tag is there already, not sure whether it means it > missed the chance again... Thanks, It did miss the chance, but in any case that vfio fix is not a regression from 2.10, so it doesn't meet the criteria to go in now. thanks -- PMM
diff --git a/target/arm/translate.c b/target/arm/translate.c index 4afb0c8..f120932 100644 --- a/target/arm/translate.c +++ b/target/arm/translate.c @@ -12245,7 +12245,10 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu) if (is_16bit) { disas_thumb_insn(dc, insn); } else { - disas_thumb2_insn(dc, insn); + if (disas_thumb2_insn(dc, insn)) { + gen_exception_insn(dc, 4, EXCP_UDEF, syn_uncategorized(), + default_exception_el(dc)); + } } /* Advance the Thumb condexec condition. */
The refactoring of commit 296e5a0a6c3935 has a nasty bug: it accidentally dropped the generation of code to raise the UNDEF exception when disas_thumb2_insn() returns nonzero. This means that 32-bit Thumb2 instruction patterns that ought to UNDEF just act like nops instead. This is likely to break any number of things, including the kernel's "disable the FPU and use the UNDEF exception to identify when to turn it back on again" trick. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- This is the smallest possible fix that will correct the bug, for possible inclusion in 2.11; for 2.12 we should fix the asymmetry where disas_thumb() generates its own exception-raising code but disas_thumb2() wants the caller to do it. (This asymmetry is why we didn't notice the problem in code review.) I'm not sure whether this should go into 2.11 or not -- this time last week it would have been an easy "yes". --- target/arm/translate.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)