[Qemu-devel] [PATCH 0/2] target/arm: Take exceptions on ATS instructions

[Qemu-devel] [PATCH 0/2] target/arm: Take exceptions on ATS instructions

[Peter Maydell]

The translation table walk for an ATS instruction can result in
various faults.  In general these are just reported back via the
PAR_EL1 fault status fields, but in some cases the architecture
requires that the fault is turned into an exception:
 * synchronous stage 2 faults of any kind during AT S1E0* and
   AT S1E1* instructions executed from NS EL1 fault to EL2 or EL3
 * synchronous external aborts are taken as Data Abort exceptions
    
(This is documented in the v8A Arm ARM DDI0487A.e D5.2.11 and G5.13.4.)

I noticed this by code inspection back last year sometime when
I was investigating a guest boot failure that turned out to be
due to an entirely different cause. I got about halfway through
trying to code up a fix before I realised it was irrelevant to
that bug. This patchset is just tidying up and completing that
work so it doesn't get lost.

Use of ATS insns in the cases where they might actually fault
is quite rare (obviously nobody sets up page tables where there's
no memory and they'll take external aborts, and even for the
"take a hyp trap for a stage 2 fault" case you need a setup
with a hypervisor and a guest that uses ATS insns, and Linux as
a guest doesn't use ATS at all. So my testing of this patchset
has been more "check it doesn't break things" rather than
actively finding and testing a use of the throw-an-exception path...

thanks
-- PMM

Peter Maydell (2):
  target/arm: Allow ARMCPRegInfo read/write functions to throw
    exceptions
  target/arm: Take exceptions on ATS instructions when needed

 target/arm/cpu.h           |   6 ++-
 target/arm/helper.c        | 107 +++++++++++++++++++++++++++++++------
 target/arm/translate-a64.c |   6 +++
 target/arm/translate.c     |   7 +++
 4 files changed, 110 insertions(+), 16 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/2] target/arm: Allow ARMCPRegInfo read/write functions to throw exceptions

[Peter Maydell]

Currently the only part of an ARMCPRegInfo which is allowed to cause
a CPU exception is the access function, which returns a value indicating
that some flavour of UNDEF should be generated.

For the ATS system instructions, we would like to conditionally
generate exceptions as part of the writefn, because some faults
during the page table walk (like external aborts) should cause
an exception to be raised rather than returning a value.

There are several ways we could do this:
 * plumb the GETPC() value from the top level set_cp_reg/get_cp_reg
   helper functions through into the readfn and writefn hooks
 * add extra readfn_with_ra/writefn_with_ra hooks that take the GETPC()
   value
 * require the ATS instructions to provide a dummy accessfn,
   which serves no purpose except to cause the code generation
   to emit TCG ops to sync the CPU state
 * add an ARM_CP_ flag to mark the ARMCPRegInfo as possibly
   throwing an exception in its read/write hooks, and make the
   codegen sync the CPU state before calling the hooks if the
   flag is set

This patch opts for the last of these, as it is fairly simple
to implement and doesn't require invasive changes like updating
the readfn/writefn hook function prototype signature.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h           | 6 +++++-
 target/arm/translate-a64.c | 6 ++++++
 target/arm/translate.c     | 7 +++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 94c990cddbd..021b552334b 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2206,6 +2206,9 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t cpregid)
  * IO indicates that this register does I/O and therefore its accesses
  * need to be surrounded by gen_io_start()/gen_io_end(). In particular,
  * registers which implement clocks or timers require this.
+ * RAISES_EXC is for when the read or write hook might raise an exception;
+ * the generated code will synchronize the CPU state before calling the hook
+ * so that it is safe for the hook to call raise_exception().
  */
 #define ARM_CP_SPECIAL           0x0001
 #define ARM_CP_CONST             0x0002
@@ -2224,10 +2227,11 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t cpregid)
 #define ARM_CP_FPU               0x1000
 #define ARM_CP_SVE               0x2000
 #define ARM_CP_NO_GDB            0x4000
+#define ARM_CP_RAISES_EXC        0x8000
 /* Used only as a terminator for ARMCPRegInfo lists */
 #define ARM_CP_SENTINEL          0xffff
 /* Mask of only the flag bits in a type field */
-#define ARM_CP_FLAG_MASK         0x70ff
+#define ARM_CP_FLAG_MASK         0xf0ff
 
 /* Valid values for ARMCPRegInfo state field, indicating which of
  * the AArch32 and AArch64 execution states this register is visible in.
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index d3231477a27..908a186bfec 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1729,6 +1729,12 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
         tcg_temp_free_ptr(tmpptr);
         tcg_temp_free_i32(tcg_syn);
         tcg_temp_free_i32(tcg_isread);
+    } else if (ri->type & ARM_CP_RAISES_EXC) {
+        /*
+         * The readfn or writefn might raise an exception;
+         * synchronize the CPU state in case it does.
+         */
+        gen_a64_set_pc_im(s->pc - 4);
     }
 
     /* Handle special cases first */
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 7853462b21b..da38e15be8d 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -7228,6 +7228,13 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
             tcg_temp_free_ptr(tmpptr);
             tcg_temp_free_i32(tcg_syn);
             tcg_temp_free_i32(tcg_isread);
+        } else if (ri->type & ARM_CP_RAISES_EXC) {
+            /*
+             * The readfn or writefn might raise an exception;
+             * synchronize the CPU state in case it does.
+             */
+            gen_set_condexec(s);
+            gen_set_pc_im(s, s->pc - 4);
         }
 
         /* Handle special cases first */
-- 
2.20.1

Re: [Qemu-devel] [PATCH 1/2] target/arm: Allow ARMCPRegInfo read/write functions to throw exceptions

[Richard Henderson]

On 8/16/19 1:58 PM, Peter Maydell wrote:
> @@ -1729,6 +1729,12 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
>          tcg_temp_free_ptr(tmpptr);
>          tcg_temp_free_i32(tcg_syn);
>          tcg_temp_free_i32(tcg_isread);
> +    } else if (ri->type & ARM_CP_RAISES_EXC) {
> +        /*
> +         * The readfn or writefn might raise an exception;
> +         * synchronize the CPU state in case it does.
> +         */
> +        gen_a64_set_pc_im(s->pc - 4);

This will now need an update for master, but otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 1/2] target/arm: Allow ARMCPRegInfo read/write functions to throw exceptions

[Peter Maydell]

On Sun, 18 Aug 2019 at 07:12, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/16/19 1:58 PM, Peter Maydell wrote:
> > @@ -1729,6 +1729,12 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
> >          tcg_temp_free_ptr(tmpptr);
> >          tcg_temp_free_i32(tcg_syn);
> >          tcg_temp_free_i32(tcg_isread);
> > +    } else if (ri->type & ARM_CP_RAISES_EXC) {
> > +        /*
> > +         * The readfn or writefn might raise an exception;
> > +         * synchronize the CPU state in case it does.
> > +         */
> > +        gen_a64_set_pc_im(s->pc - 4);
>
> This will now need an update for master, but otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks; applied series to target-arm.next with this squashed
in to handle the changes in master:

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index b3053d3fb89..4d09ae6f424 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1719,7 +1719,7 @@ static void handle_sys(DisasContext *s, uint32_t
insn, bool isread,
          * The readfn or writefn might raise an exception;
          * synchronize the CPU state in case it does.
          */
-        gen_a64_set_pc_im(s->pc - 4);
+        gen_a64_set_pc_im(s->pc_curr);
     }

     /* Handle special cases first */
diff --git a/target/arm/translate.c b/target/arm/translate.c
index adb97dc6a3d..78d93f63cab 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -7197,7 +7197,7 @@ static int disas_coproc_insn(DisasContext *s,
uint32_t insn)
              * synchronize the CPU state in case it does.
              */
             gen_set_condexec(s);
-            gen_set_pc_im(s, s->pc - 4);
+            gen_set_pc_im(s, s->pc_curr);
         }

         /* Handle special cases first */


-- PMM

[Qemu-devel] [PATCH 2/2] target/arm: Take exceptions on ATS instructions when needed

[Peter Maydell]

The translation table walk for an ATS instruction can result in
various faults.  In general these are just reported back via the
PAR_EL1 fault status fields, but in some cases the architecture
requires that the fault is turned into an exception:
 * synchronous stage 2 faults of any kind during AT S1E0* and
   AT S1E1* instructions executed from NS EL1 fault to EL2 or EL3
 * synchronous external aborts are taken as Data Abort exceptions

(This is documented in the v8A Arm ARM DDI0487A.e D5.2.11 and
G5.13.4.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 107 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 92 insertions(+), 15 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index b74c23a9bc0..7d82195c960 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2944,6 +2944,73 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
     ret = get_phys_addr(env, value, access_type, mmu_idx, &phys_addr, &attrs,
                         &prot, &page_size, &fi, &cacheattrs);
 
+    if (ret) {
+        /*
+         * Some kinds of translation fault must cause exceptions rather
+         * than being reported in the PAR.
+         */
+        int current_el = arm_current_el(env);
+        int target_el;
+        uint32_t syn, fsr, fsc;
+        bool take_exc = false;
+
+        if (fi.s1ptw && current_el == 1 && !arm_is_secure(env)
+            && (mmu_idx == ARMMMUIdx_S1NSE1 || mmu_idx == ARMMMUIdx_S1NSE0)) {
+            /*
+             * Synchronous stage 2 fault on an access made as part of the
+             * translation table walk for AT S1E0* or AT S1E1* insn
+             * executed from NS EL1. If this is a synchronous external abort
+             * and SCR_EL3.EA == 1, then we take a synchronous external abort
+             * to EL3. Otherwise the fault is taken as an exception to EL2,
+             * and HPFAR_EL2 holds the faulting IPA.
+             */
+            if (fi.type == ARMFault_SyncExternalOnWalk &&
+                (env->cp15.scr_el3 & SCR_EA)) {
+                target_el = 3;
+            } else {
+                env->cp15.hpfar_el2 = extract64(fi.s2addr, 12, 47) << 4;
+                target_el = 2;
+            }
+            take_exc = true;
+        } else if (fi.type == ARMFault_SyncExternalOnWalk) {
+            /*
+             * Synchronous external aborts during a translation table walk
+             * are taken as Data Abort exceptions.
+             */
+            if (fi.stage2) {
+                if (current_el == 3) {
+                    target_el = 3;
+                } else {
+                    target_el = 2;
+                }
+            } else {
+                target_el = exception_target_el(env);
+            }
+            take_exc = true;
+        }
+
+        if (take_exc) {
+            /* Construct FSR and FSC using same logic as arm_deliver_fault() */
+            if (target_el == 2 || arm_el_is_aa64(env, target_el) ||
+                arm_s1_regime_using_lpae_format(env, mmu_idx)) {
+                fsr = arm_fi_to_lfsc(&fi);
+                fsc = extract32(fsr, 0, 6);
+            } else {
+                fsr = arm_fi_to_sfsc(&fi);
+                fsc = 0x3f;
+            }
+            /*
+             * Report exception with ESR indicating a fault due to a
+             * translation table walk for a cache maintenance instruction.
+             */
+            syn = syn_data_abort_no_iss(current_el == target_el,
+                                        fi.ea, 1, fi.s1ptw, 1, fsc);
+            env->exception.vaddress = value;
+            env->exception.fsr = fsr;
+            raise_exception(env, EXCP_DATA_ABORT, syn, target_el);
+        }
+    }
+
     if (is_a64(env)) {
         format64 = true;
     } else if (arm_feature(env, ARM_FEATURE_LPAE)) {
@@ -3148,7 +3215,7 @@ static const ARMCPRegInfo vapa_cp_reginfo[] = {
     /* This underdecoding is safe because the reginfo is NO_RAW. */
     { .name = "ATS", .cp = 15, .crn = 7, .crm = 8, .opc1 = 0, .opc2 = CP_ANY,
       .access = PL1_W, .accessfn = ats_access,
-      .writefn = ats_write, .type = ARM_CP_NO_RAW },
+      .writefn = ats_write, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC },
 #endif
     REGINFO_SENTINEL
 };
@@ -4281,35 +4348,45 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
     /* 64 bit address translation operations */
     { .name = "AT_S1E1R", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 0,
-      .access = PL1_W, .type = ARM_CP_NO_RAW, .writefn = ats_write64 },
+      .access = PL1_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
+      .writefn = ats_write64 },
     { .name = "AT_S1E1W", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 1,
-      .access = PL1_W, .type = ARM_CP_NO_RAW, .writefn = ats_write64 },
+      .access = PL1_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
+      .writefn = ats_write64 },
     { .name = "AT_S1E0R", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 2,
-      .access = PL1_W, .type = ARM_CP_NO_RAW, .writefn = ats_write64 },
+      .access = PL1_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
+      .writefn = ats_write64 },
     { .name = "AT_S1E0W", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 8, .opc2 = 3,
-      .access = PL1_W, .type = ARM_CP_NO_RAW, .writefn = ats_write64 },
+      .access = PL1_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
+      .writefn = ats_write64 },
     { .name = "AT_S12E1R", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 4, .crn = 7, .crm = 8, .opc2 = 4,
-      .access = PL2_W, .type = ARM_CP_NO_RAW, .writefn = ats_write64 },
+      .access = PL2_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
+      .writefn = ats_write64 },
     { .name = "AT_S12E1W", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 4, .crn = 7, .crm = 8, .opc2 = 5,
-      .access = PL2_W, .type = ARM_CP_NO_RAW, .writefn = ats_write64 },
+      .access = PL2_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
+      .writefn = ats_write64 },
     { .name = "AT_S12E0R", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 4, .crn = 7, .crm = 8, .opc2 = 6,
-      .access = PL2_W, .type = ARM_CP_NO_RAW, .writefn = ats_write64 },
+      .access = PL2_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
+      .writefn = ats_write64 },
     { .name = "AT_S12E0W", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 4, .crn = 7, .crm = 8, .opc2 = 7,
-      .access = PL2_W, .type = ARM_CP_NO_RAW, .writefn = ats_write64 },
+      .access = PL2_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
+      .writefn = ats_write64 },
     /* AT S1E2* are elsewhere as they UNDEF from EL3 if EL2 is not present */
     { .name = "AT_S1E3R", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 6, .crn = 7, .crm = 8, .opc2 = 0,
-      .access = PL3_W, .type = ARM_CP_NO_RAW, .writefn = ats_write64 },
+      .access = PL3_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
+      .writefn = ats_write64 },
     { .name = "AT_S1E3W", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 6, .crn = 7, .crm = 8, .opc2 = 1,
-      .access = PL3_W, .type = ARM_CP_NO_RAW, .writefn = ats_write64 },
+      .access = PL3_W, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC,
+      .writefn = ats_write64 },
     { .name = "PAR_EL1", .state = ARM_CP_STATE_AA64,
       .type = ARM_CP_ALIAS,
       .opc0 = 3, .opc1 = 0, .crn = 7, .crm = 4, .opc2 = 0,
@@ -4891,11 +4968,11 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
     { .name = "AT_S1E2R", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 4, .crn = 7, .crm = 8, .opc2 = 0,
       .access = PL2_W, .accessfn = at_s1e2_access,
-      .type = ARM_CP_NO_RAW, .writefn = ats_write64 },
+      .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC, .writefn = ats_write64 },
     { .name = "AT_S1E2W", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 4, .crn = 7, .crm = 8, .opc2 = 1,
       .access = PL2_W, .accessfn = at_s1e2_access,
-      .type = ARM_CP_NO_RAW, .writefn = ats_write64 },
+      .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC, .writefn = ats_write64 },
     /* The AArch32 ATS1H* operations are CONSTRAINED UNPREDICTABLE
      * if EL2 is not implemented; we choose to UNDEF. Behaviour at EL3
      * with SCR.NS == 0 outside Monitor mode is UNPREDICTABLE; we choose
@@ -4903,10 +4980,10 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
      */
     { .name = "ATS1HR", .cp = 15, .opc1 = 4, .crn = 7, .crm = 8, .opc2 = 0,
       .access = PL2_W,
-      .writefn = ats1h_write, .type = ARM_CP_NO_RAW },
+      .writefn = ats1h_write, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC },
     { .name = "ATS1HW", .cp = 15, .opc1 = 4, .crn = 7, .crm = 8, .opc2 = 1,
       .access = PL2_W,
-      .writefn = ats1h_write, .type = ARM_CP_NO_RAW },
+      .writefn = ats1h_write, .type = ARM_CP_NO_RAW | ARM_CP_RAISES_EXC },
     { .name = "CNTHCTL_EL2", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 1, .opc2 = 0,
       /* ARMv7 requires bit 0 and 1 to reset to 1. ARMv8 defines the
-- 
2.20.1

Re: [Qemu-devel] [PATCH 2/2] target/arm: Take exceptions on ATS instructions when needed

[Richard Henderson]

On 8/16/19 1:58 PM, Peter Maydell wrote:
> The translation table walk for an ATS instruction can result in
> various faults.  In general these are just reported back via the
> PAR_EL1 fault status fields, but in some cases the architecture
> requires that the fault is turned into an exception:
>  * synchronous stage 2 faults of any kind during AT S1E0* and
>    AT S1E1* instructions executed from NS EL1 fault to EL2 or EL3
>  * synchronous external aborts are taken as Data Abort exceptions
> 
> (This is documented in the v8A Arm ARM DDI0487A.e D5.2.11 and
> G5.13.4.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 107 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 92 insertions(+), 15 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 0/2] target/arm: Take exceptions on ATS instructions

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190816125802.25877-1-peter.maydell@linaro.org/



Hi,

This series failed build test on s390x host. Please find the details below.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e

echo
echo "=== ENV ==="
env

echo
echo "=== PACKAGES ==="
rpm -qa

echo
echo "=== UNAME ==="
uname -a

CC=$HOME/bin/cc
INSTALL=$PWD/install
BUILD=$PWD/build
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --cc=$CC --prefix=$INSTALL
make -j4
# XXX: we need reliable clean up
# make check -j4 V=1
make install
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/20190816125802.25877-1-peter.maydell@linaro.org/testing.s390x/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH 0/2] target/arm: Take exceptions on ATS instructions

[Peter Maydell]

On Fri, 16 Aug 2019 at 13:58, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> The translation table walk for an ATS instruction can result in
> various faults.  In general these are just reported back via the
> PAR_EL1 fault status fields, but in some cases the architecture
> requires that the fault is turned into an exception:
>  * synchronous stage 2 faults of any kind during AT S1E0* and
>    AT S1E1* instructions executed from NS EL1 fault to EL2 or EL3
>  * synchronous external aborts are taken as Data Abort exceptions
>
> (This is documented in the v8A Arm ARM DDI0487A.e D5.2.11 and G5.13.4.)
>
> I noticed this by code inspection back last year sometime when
> I was investigating a guest boot failure that turned out to be
> due to an entirely different cause. I got about halfway through
> trying to code up a fix before I realised it was irrelevant to
> that bug. This patchset is just tidying up and completing that
> work so it doesn't get lost.
>
> Use of ATS insns in the cases where they might actually fault
> is quite rare (obviously nobody sets up page tables where there's
> no memory and they'll take external aborts, and even for the
> "take a hyp trap for a stage 2 fault" case you need a setup
> with a hypervisor and a guest that uses ATS insns, and Linux as
> a guest doesn't use ATS at all. So my testing of this patchset
> has been more "check it doesn't break things" rather than
> actively finding and testing a use of the throw-an-exception path...

I'm told that Xen for Arm makes more active use of ATS
instructions, so I've cc'd a few Xen people -- do any
of you have handy testing setups to try running Xen in
emulation under QEMU? Configs where the guest (EL1) actually
uses ATS instructions are the particularly interesting point
for this patchset.

(if there's a good set of instructions for creating a test
image I could probably add it to the ad-hoc set of things
I sometimes test with.)

> Peter Maydell (2):
>   target/arm: Allow ARMCPRegInfo read/write functions to throw
>     exceptions
>   target/arm: Take exceptions on ATS instructions when needed
>
>  target/arm/cpu.h           |   6 ++-
>  target/arm/helper.c        | 107 +++++++++++++++++++++++++++++++------
>  target/arm/translate-a64.c |   6 +++
>  target/arm/translate.c     |   7 +++
>  4 files changed, 110 insertions(+), 16 deletions(-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/2] target/arm: Take exceptions on ATS instructions

[Edgar E. Iglesias]

On Mon, Aug 19, 2019 at 01:44:37PM +0100, Peter Maydell wrote:
> On Fri, 16 Aug 2019 at 13:58, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > The translation table walk for an ATS instruction can result in
> > various faults.  In general these are just reported back via the
> > PAR_EL1 fault status fields, but in some cases the architecture
> > requires that the fault is turned into an exception:
> >  * synchronous stage 2 faults of any kind during AT S1E0* and
> >    AT S1E1* instructions executed from NS EL1 fault to EL2 or EL3
> >  * synchronous external aborts are taken as Data Abort exceptions
> >
> > (This is documented in the v8A Arm ARM DDI0487A.e D5.2.11 and G5.13.4.)
> >
> > I noticed this by code inspection back last year sometime when
> > I was investigating a guest boot failure that turned out to be
> > due to an entirely different cause. I got about halfway through
> > trying to code up a fix before I realised it was irrelevant to
> > that bug. This patchset is just tidying up and completing that
> > work so it doesn't get lost.
> >
> > Use of ATS insns in the cases where they might actually fault
> > is quite rare (obviously nobody sets up page tables where there's
> > no memory and they'll take external aborts, and even for the
> > "take a hyp trap for a stage 2 fault" case you need a setup
> > with a hypervisor and a guest that uses ATS insns, and Linux as
> > a guest doesn't use ATS at all. So my testing of this patchset
> > has been more "check it doesn't break things" rather than
> > actively finding and testing a use of the throw-an-exception path...
> 
> I'm told that Xen for Arm makes more active use of ATS
> instructions, so I've cc'd a few Xen people -- do any
> of you have handy testing setups to try running Xen in
> emulation under QEMU? Configs where the guest (EL1) actually
> uses ATS instructions are the particularly interesting point
> for this patchset.
> 
> (if there's a good set of instructions for creating a test
> image I could probably add it to the ad-hoc set of things
> I sometimes test with.)
> 

Yes, I'm not very up to date with the Xen code but it used to
be the case that Xen used ATS a fair bit. We have some other
tests that use it but they don't rely on exceptions IIRC.

I'll take your patches and test them with our internal testsuites, including Xen images.

I may also be able to find a Xen image I can share that works with upstream QEMU.
Stefano may have something in his pocket already?

Cheers,
Edgar

Re: [Qemu-devel] [PATCH 0/2] target/arm: Take exceptions on ATS instructions

[Edgar E. Iglesias]

On Mon, Aug 19, 2019 at 01:44:37PM +0100, Peter Maydell wrote:
> On Fri, 16 Aug 2019 at 13:58, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > The translation table walk for an ATS instruction can result in
> > various faults.  In general these are just reported back via the
> > PAR_EL1 fault status fields, but in some cases the architecture
> > requires that the fault is turned into an exception:
> >  * synchronous stage 2 faults of any kind during AT S1E0* and
> >    AT S1E1* instructions executed from NS EL1 fault to EL2 or EL3
> >  * synchronous external aborts are taken as Data Abort exceptions
> >
> > (This is documented in the v8A Arm ARM DDI0487A.e D5.2.11 and G5.13.4.)
> >
> > I noticed this by code inspection back last year sometime when
> > I was investigating a guest boot failure that turned out to be
> > due to an entirely different cause. I got about halfway through
> > trying to code up a fix before I realised it was irrelevant to
> > that bug. This patchset is just tidying up and completing that
> > work so it doesn't get lost.
> >
> > Use of ATS insns in the cases where they might actually fault
> > is quite rare (obviously nobody sets up page tables where there's
> > no memory and they'll take external aborts, and even for the
> > "take a hyp trap for a stage 2 fault" case you need a setup
> > with a hypervisor and a guest that uses ATS insns, and Linux as
> > a guest doesn't use ATS at all. So my testing of this patchset
> > has been more "check it doesn't break things" rather than
> > actively finding and testing a use of the throw-an-exception path...
> 
> I'm told that Xen for Arm makes more active use of ATS
> instructions, so I've cc'd a few Xen people -- do any
> of you have handy testing setups to try running Xen in
> emulation under QEMU? Configs where the guest (EL1) actually
> uses ATS instructions are the particularly interesting point
> for this patchset.
> 
> (if there's a good set of instructions for creating a test
> image I could probably add it to the ad-hoc set of things
> I sometimes test with.)


Hi,

All tests passed.

Tested-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Cheers,
Edgar