[Qemu-devel] [PATCH 0/2] Fix qemu-system-aarch64 crash

[Qemu-devel] [PATCH 0/2] Fix qemu-system-aarch64 crash

[Richard Henderson]

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

[Qemu-devel] [PATCH 1/2] target/arm: Always return ARMASIdx_NS when num_ases == 1

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index e310ffc29d..c26cc43ea8 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2915,7 +2915,7 @@ enum {
 /* Return the address space index to use for a memory access */
 static inline int arm_asidx_from_attrs(CPUState *cs, MemTxAttrs attrs)
 {
-    return attrs.secure ? ARMASIdx_S : ARMASIdx_NS;
+    return cs->num_ases > 1 && attrs.secure ? ARMASIdx_S : ARMASIdx_NS;
 }
 
 /* Return the AddressSpace to use for a memory access
-- 
2.17.1

[Qemu-devel] [PATCH 2/2] cpu: Assert asidx_from_attrs return value in range

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/qom/cpu.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index cce2fd6acc..bd796579ee 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -620,11 +620,13 @@ static inline hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
 static inline int cpu_asidx_from_attrs(CPUState *cpu, MemTxAttrs attrs)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
+    int ret = 0;
 
     if (cc->asidx_from_attrs) {
-        return cc->asidx_from_attrs(cpu, attrs);
+        ret = cc->asidx_from_attrs(cpu, attrs);
+        assert(ret < cpu->num_ases && ret >= 0);
     }
-    return 0;
+    return ret;
 }
 #endif
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH 2/2] cpu: Assert asidx_from_attrs return value in range

[Peter Maydell]

On 30 June 2018 at 01:02, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/qom/cpu.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index cce2fd6acc..bd796579ee 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -620,11 +620,13 @@ static inline hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
>  static inline int cpu_asidx_from_attrs(CPUState *cpu, MemTxAttrs attrs)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> +    int ret = 0;
>
>      if (cc->asidx_from_attrs) {
> -        return cc->asidx_from_attrs(cpu, attrs);
> +        ret = cc->asidx_from_attrs(cpu, attrs);
> +        assert(ret < cpu->num_ases && ret >= 0);
>      }
> -    return 0;
> +    return ret;
>  }
>  #endif
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/2] Fix qemu-system-aarch64 crash

[Peter Maydell]

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