mbox series

[0/2] Fix qemu-system-aarch64 crash

Message ID 20180630000242.29594-1-richard.henderson@linaro.org
Headers show
Series Fix qemu-system-aarch64 crash | expand

Message

Richard Henderson June 30, 2018, 12:02 a.m. UTC 
The sequence of events was
  (1) Kernel executed a disabled sve insn,
  (2) Undefined Instruction trap went to EL3,
  (3) Lookup of the exception handler saw el3 and returned asidx 1,
  (4) Which hadn't been set up.

So there's definitely a bug with SVE exception routing.
That said...

With just the first patch, the kernel goes into a silly exception loop
which is understandable.  With just the second patch, qemu gets SIGABRT
instead of SIGSEGV, which is definitely easier to debug.

I think I'm in favor of both patches, but you might say we shouldn't
have to have the first one and just apply the second.


r~


Richard Henderson (2):
  target/arm: Always return ARMASIdx_NS when num_ases == 1
  cpu: Assert asidx_from_attrs return value in range

 include/qom/cpu.h | 6 ++++--
 target/arm/cpu.h  | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

-- 
2.17.1

Comments

Peter Maydell July 2, 2018, 10:46 a.m. UTC | #1 
On 30 June 2018 at 01:02, Richard Henderson
<richard.henderson@linaro.org> wrote:
> The sequence of events was

>   (1) Kernel executed a disabled sve insn,

>   (2) Undefined Instruction trap went to EL3,

>   (3) Lookup of the exception handler saw el3 and returned asidx 1,

>   (4) Which hadn't been set up.

>

> So there's definitely a bug with SVE exception routing.

> That said...

>

> With just the first patch, the kernel goes into a silly exception loop

> which is understandable.  With just the second patch, qemu gets SIGABRT

> instead of SIGSEGV, which is definitely easier to debug.

>

> I think I'm in favor of both patches, but you might say we shouldn't

> have to have the first one and just apply the second.


I think my vote is for just the second -- a CPU without the
security extensions should never be emitting transactions
with attrs.secure true, so that's a bug we want to track down.
Suitably placed assert()s do a better job of that than sweeping
the problem under the carpet by squashing the attributes
in arm_asidx_from_attrs().

thanks
-- PMM