[Qemu-devel] [PATCH] target/arm: Add assertion about FSC format for syndrome registers

Peter Maydell posted 1 patch 6 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1491486152-24304-1-git-send-email-peter.maydell@linaro.org
Test checkpatch passed
Test docker passed
Test s390x passed
target/arm/op_helper.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
[Qemu-devel] [PATCH] target/arm: Add assertion about FSC format for syndrome registers
Posted by Peter Maydell 6 years, 11 months ago
In tlb_fill() we construct a syndrome register value from a
fault status register value which is filled in by arm_tlb_fill().
arm_tlb_fill() returns FSR values which might be in the format
used with short-format page descriptors, or the format used
with long-format (LPAE) descriptors. The syndrome register
always uses LPAE-format FSR status codes.

It isn't actually possible to end up delivering a syndrome
register value to the guest for a fault which is reported
with a short-format FSR (that kind of stage 1 fault will only
happen for an AArch32 translation regime which doesn't have
a syndrome register, and can never be redirected to an AArch64
or Hyp exception level). Add an assertion which checks this,
and adjust the code so that we construct a syndrome with
an invalid status code, rather than allowing set bits in
the FSR input to randomly corrupt other fields in the syndrome.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
It took me a little while to convince myself that you'd
never take a short-format FSR to a using-syndrome EL :-)
---
 target/arm/op_helper.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index d64c867..156b825 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -130,7 +130,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
     if (unlikely(ret)) {
         ARMCPU *cpu = ARM_CPU(cs);
         CPUARMState *env = &cpu->env;
-        uint32_t syn, exc;
+        uint32_t syn, exc, fsc;
         unsigned int target_el;
         bool same_el;
 
@@ -145,19 +145,32 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
             env->cp15.hpfar_el2 = extract64(fi.s2addr, 12, 47) << 4;
         }
         same_el = arm_current_el(env) == target_el;
-        /* AArch64 syndrome does not have an LPAE bit */
-        syn = fsr & ~(1 << 9);
+
+        if (fsr & (1 << 9)) {
+            /* LPAE format fault status register : bottom 6 bits are
+             * status code in the same form as needed for syndrome
+             */
+            fsc = extract32(fsr, 0, 6);
+        } else {
+            /* Short format FSR : this fault will never actually be reported
+             * to an EL that uses a syndrome register. Check that here,
+             * and use a (currently) reserved FSR code in case the constructed
+             * syndrome does leak into the guest somehow.
+             */
+            assert(target_el != 2 && !arm_el_is_aa64(env, target_el));
+            fsc = 0x3f;
+        }
 
         /* For insn and data aborts we assume there is no instruction syndrome
          * information; this is always true for exceptions reported to EL1.
          */
         if (access_type == MMU_INST_FETCH) {
-            syn = syn_insn_abort(same_el, 0, fi.s1ptw, syn);
+            syn = syn_insn_abort(same_el, 0, fi.s1ptw, fsc);
             exc = EXCP_PREFETCH_ABORT;
         } else {
             syn = merge_syn_data_abort(env->exception.syndrome, target_el,
                                        same_el, fi.s1ptw,
-                                       access_type == MMU_DATA_STORE, syn);
+                                       access_type == MMU_DATA_STORE, fsc);
             if (access_type == MMU_DATA_STORE
                 && arm_feature(env, ARM_FEATURE_V6)) {
                 fsr |= (1 << 11);
-- 
2.7.4


Re: [Qemu-devel] [PATCH] target/arm: Add assertion about FSC format for syndrome registers
Posted by Edgar E. Iglesias 6 years, 11 months ago
On Thu, Apr 06, 2017 at 02:42:32PM +0100, Peter Maydell wrote:
> In tlb_fill() we construct a syndrome register value from a
> fault status register value which is filled in by arm_tlb_fill().
> arm_tlb_fill() returns FSR values which might be in the format
> used with short-format page descriptors, or the format used
> with long-format (LPAE) descriptors. The syndrome register
> always uses LPAE-format FSR status codes.
> 
> It isn't actually possible to end up delivering a syndrome
> register value to the guest for a fault which is reported
> with a short-format FSR (that kind of stage 1 fault will only
> happen for an AArch32 translation regime which doesn't have
> a syndrome register, and can never be redirected to an AArch64
> or Hyp exception level). Add an assertion which checks this,
> and adjust the code so that we construct a syndrome with
> an invalid status code, rather than allowing set bits in
> the FSR input to randomly corrupt other fields in the syndrome.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Tricky but it looks correct as far as I can follow the specs...

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




> ---
> It took me a little while to convince myself that you'd
> never take a short-format FSR to a using-syndrome EL :-)
> ---
>  target/arm/op_helper.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index d64c867..156b825 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -130,7 +130,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
>      if (unlikely(ret)) {
>          ARMCPU *cpu = ARM_CPU(cs);
>          CPUARMState *env = &cpu->env;
> -        uint32_t syn, exc;
> +        uint32_t syn, exc, fsc;
>          unsigned int target_el;
>          bool same_el;
>  
> @@ -145,19 +145,32 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
>              env->cp15.hpfar_el2 = extract64(fi.s2addr, 12, 47) << 4;
>          }
>          same_el = arm_current_el(env) == target_el;
> -        /* AArch64 syndrome does not have an LPAE bit */
> -        syn = fsr & ~(1 << 9);
> +
> +        if (fsr & (1 << 9)) {
> +            /* LPAE format fault status register : bottom 6 bits are
> +             * status code in the same form as needed for syndrome
> +             */
> +            fsc = extract32(fsr, 0, 6);
> +        } else {
> +            /* Short format FSR : this fault will never actually be reported
> +             * to an EL that uses a syndrome register. Check that here,
> +             * and use a (currently) reserved FSR code in case the constructed
> +             * syndrome does leak into the guest somehow.
> +             */
> +            assert(target_el != 2 && !arm_el_is_aa64(env, target_el));
> +            fsc = 0x3f;
> +        }
>  
>          /* For insn and data aborts we assume there is no instruction syndrome
>           * information; this is always true for exceptions reported to EL1.
>           */
>          if (access_type == MMU_INST_FETCH) {
> -            syn = syn_insn_abort(same_el, 0, fi.s1ptw, syn);
> +            syn = syn_insn_abort(same_el, 0, fi.s1ptw, fsc);
>              exc = EXCP_PREFETCH_ABORT;
>          } else {
>              syn = merge_syn_data_abort(env->exception.syndrome, target_el,
>                                         same_el, fi.s1ptw,
> -                                       access_type == MMU_DATA_STORE, syn);
> +                                       access_type == MMU_DATA_STORE, fsc);
>              if (access_type == MMU_DATA_STORE
>                  && arm_feature(env, ARM_FEATURE_V6)) {
>                  fsr |= (1 << 11);
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH] target/arm: Add assertion about FSC format for syndrome registers
Posted by Edgar E. Iglesias 6 years, 11 months ago
On Thu, Apr 06, 2017 at 02:42:32PM +0100, Peter Maydell wrote:
> In tlb_fill() we construct a syndrome register value from a
> fault status register value which is filled in by arm_tlb_fill().
> arm_tlb_fill() returns FSR values which might be in the format
> used with short-format page descriptors, or the format used
> with long-format (LPAE) descriptors. The syndrome register
> always uses LPAE-format FSR status codes.
> 
> It isn't actually possible to end up delivering a syndrome
> register value to the guest for a fault which is reported
> with a short-format FSR (that kind of stage 1 fault will only
> happen for an AArch32 translation regime which doesn't have
> a syndrome register, and can never be redirected to an AArch64
> or Hyp exception level). Add an assertion which checks this,
> and adjust the code so that we construct a syndrome with
> an invalid status code, rather than allowing set bits in
> the FSR input to randomly corrupt other fields in the syndrome.


BTW,

Have you seen something suspicous when running code or was
this a theoretical issue?

Cheers,
Edgar

> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> It took me a little while to convince myself that you'd
> never take a short-format FSR to a using-syndrome EL :-)
> ---
>  target/arm/op_helper.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index d64c867..156b825 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -130,7 +130,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
>      if (unlikely(ret)) {
>          ARMCPU *cpu = ARM_CPU(cs);
>          CPUARMState *env = &cpu->env;
> -        uint32_t syn, exc;
> +        uint32_t syn, exc, fsc;
>          unsigned int target_el;
>          bool same_el;
>  
> @@ -145,19 +145,32 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
>              env->cp15.hpfar_el2 = extract64(fi.s2addr, 12, 47) << 4;
>          }
>          same_el = arm_current_el(env) == target_el;
> -        /* AArch64 syndrome does not have an LPAE bit */
> -        syn = fsr & ~(1 << 9);
> +
> +        if (fsr & (1 << 9)) {
> +            /* LPAE format fault status register : bottom 6 bits are
> +             * status code in the same form as needed for syndrome
> +             */
> +            fsc = extract32(fsr, 0, 6);
> +        } else {
> +            /* Short format FSR : this fault will never actually be reported
> +             * to an EL that uses a syndrome register. Check that here,
> +             * and use a (currently) reserved FSR code in case the constructed
> +             * syndrome does leak into the guest somehow.
> +             */
> +            assert(target_el != 2 && !arm_el_is_aa64(env, target_el));
> +            fsc = 0x3f;
> +        }
>  
>          /* For insn and data aborts we assume there is no instruction syndrome
>           * information; this is always true for exceptions reported to EL1.
>           */
>          if (access_type == MMU_INST_FETCH) {
> -            syn = syn_insn_abort(same_el, 0, fi.s1ptw, syn);
> +            syn = syn_insn_abort(same_el, 0, fi.s1ptw, fsc);
>              exc = EXCP_PREFETCH_ABORT;
>          } else {
>              syn = merge_syn_data_abort(env->exception.syndrome, target_el,
>                                         same_el, fi.s1ptw,
> -                                       access_type == MMU_DATA_STORE, syn);
> +                                       access_type == MMU_DATA_STORE, fsc);
>              if (access_type == MMU_DATA_STORE
>                  && arm_feature(env, ARM_FEATURE_V6)) {
>                  fsr |= (1 << 11);
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH] target/arm: Add assertion about FSC format for syndrome registers
Posted by Peter Maydell 6 years, 11 months ago
On 10 April 2017 at 09:44, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> Have you seen something suspicous when running code or was
> this a theoretical issue?

No, I was looking at the code for a different reason and
was surprised that the syndrome-generation code wasn't doing
anything to handle short-format FSRs. I started out thinking
that this was just a corner case we'd forgotten since it
only affects AArch32 guests (and even wrote some code to do
short-to-long status code conversion), but after some thought
I realised that it's not possible to get to a point where
we would need the syndrome after such a fault. It's a
tricky enough point that it seems worth having an assert
and some comments to assure future readers that it isn't
just something we forgot to handle...

thanks
-- PMM