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(-)

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.  */
-- 
2.7.4

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,

-- 
Peter Xu

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