Re: [Qemu-devel] [PATCH v3 7/7] target-arm: A64: Create Instruction Syndromes for Data Aborts

2016-05-05 Thread Edgar E. Iglesias 
On Wed, May 04, 2016 at 06:38:49PM +0100, Peter Maydell wrote:
> On 29 April 2016 at 13:08, Edgar E. Iglesias  wrote:
> > From: "Edgar E. Iglesias" 
> >
> > Add support for generating the instruction syndrome for Data Aborts.
> > These syndromes are used by hypervisors for example to trap and emulate
> > memory accesses.
> >
> > We save the decoded data out-of-band with the TBs at translation time.
> > When exceptions hit, the extra data attached to the TB is used to
> > recreate the state needed to encode instruction syndromes.
> > This avoids the need to emit moves with every load/store.
> >
> > Based on a suggestion from Peter Maydell.
> 
> Worth mentioning in the commit message that we don't currently generate
> ISV information for exceptions from AArch32?

Yes, I've changed the first part of the commit message to:

target-arm: A64: Create Instruction Syndromes for Data Aborts

Add support for generating the ISS (Instruction Specific Syndrome) for
Data Abort exceptions taken from AArch64.

...



> 
> > Suggested-by: Peter Maydell 
> > Signed-off-by: Edgar E. Iglesias 
> > ---
> >  target-arm/cpu.h   |  12 -
> >  target-arm/op_helper.c |  49 ++---
> >  target-arm/translate-a64.c | 130 
> > +++--
> >  target-arm/translate.c |   5 +-
> >  target-arm/translate.h |   2 +
> >  5 files changed, 173 insertions(+), 25 deletions(-)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index 066ff67..256fbec 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -94,7 +94,17 @@
> >  struct arm_boot_info;
> >
> >  #define NB_MMU_MODES 7
> > -#define TARGET_INSN_START_EXTRA_WORDS 1
> > +/* ARM-specific extra insn start words:
> > + * 1: Conditional execution bits
> > + * 2: Partial exception syndrome for data aborts
> > + */
> > +#define TARGET_INSN_START_EXTRA_WORDS 2
> > +
> > +/* The 2nd extra word holding syndrome info for data aborts does not use
> > + * the lower 14 bits. We shift it down to help the sleb128 encoder do a
> > + * better job. When restoring the CPU state, we shift it back up.
> > + */
> > +#define ARM_INSN_START_WORD2_SHIFT 14
> 
> We could also not bother putting bits 25..31 (ie the field that always reads
> EC_DATAABORT) in the insn start word.

Yes, good point. I'll mask them out for v4.


> 
> > @@ -720,23 +720,55 @@ static void gen_adc_CC(int sf, TCGv_i64 dest, 
> > TCGv_i64 t0, TCGv_i64 t1)
> >   * Store from GPR register to memory.
> >   */
> >  static void do_gpr_st_memidx(DisasContext *s, TCGv_i64 source,
> > - TCGv_i64 tcg_addr, int size, int memidx)
> > + TCGv_i64 tcg_addr, int size, int memidx,
> > + bool iss_valid,
> > + unsigned int iss_srt,
> > + bool iss_sf, bool iss_ar)
> >  {
> >  g_assert(size <= 3);
> >  tcg_gen_qemu_st_i64(source, tcg_addr, memidx, s->be_data + size);
> > +
> > +if (iss_valid) {
> > +uint32_t isyn32;
> > +
> > +isyn32 = syn_data_abort_with_iss(0,
> > + size,
> > + false,
> > + iss_srt,
> > + iss_sf,
> > + iss_ar,
> > + 0, 0, 0, 0, 0, false);
> > +isyn32 >>= ARM_INSN_START_WORD2_SHIFT;
> > +tcg_set_insn_param(s->insn_start_idx, 2, isyn32);
> 
> Is it worth doing
>   assert(s->insn_start_idx);
>   tcg_set_insn_param(...);
>   s->insn_start_idx = 0;
> 
> as a way to catch accidentally trying to set the syndrome info twice ?

Yes, why not. I've done that in v4.



> 
> > +}
> > +}
> > +
> > +static void do_gpr_st_with_isv(DisasContext *s, TCGv_i64 source,
> > +   TCGv_i64 tcg_addr, int size,
> > +   bool iss_valid,
> > +   unsigned int iss_srt,
> > +   bool iss_sf, bool iss_ar)
> > +{
> > +do_gpr_st_memidx(s, source, tcg_addr, size, get_mem_index(s),
> > + iss_valid, iss_srt, iss_sf, iss_ar);
> >  }
> >
> >  static void do_gpr_st(DisasContext *s, TCGv_i64 source,
> >TCGv_i64 tcg_addr, int size)
> >  {
> > -do_gpr_st_memidx(s, source, tcg_addr, size, get_mem_index(s));
> > +do_gpr_st_with_isv(s, source, tcg_addr, size,
> > +   false, 0, false, false);
> >  }
> 
> There's only two places where we use do_gpr_st() rather than the
> _with_isv() version (both in the load/store pair function), and
> similarly for do_gpr_ld(); so I think it would be better just to
> have do_gpr_ld/st always take the iss arguments and not have a
> separate

Re: [Qemu-devel] [PATCH v3 7/7] target-arm: A64: Create Instruction Syndromes for Data Aborts

2016-05-04 Thread Peter Maydell 
On 29 April 2016 at 13:08, Edgar E. Iglesias  wrote:
> From: "Edgar E. Iglesias" 
>
> Add support for generating the instruction syndrome for Data Aborts.
> These syndromes are used by hypervisors for example to trap and emulate
> memory accesses.
>
> We save the decoded data out-of-band with the TBs at translation time.
> When exceptions hit, the extra data attached to the TB is used to
> recreate the state needed to encode instruction syndromes.
> This avoids the need to emit moves with every load/store.
>
> Based on a suggestion from Peter Maydell.

Worth mentioning in the commit message that we don't currently generate
ISV information for exceptions from AArch32?

> Suggested-by: Peter Maydell 
> Signed-off-by: Edgar E. Iglesias 
> ---
>  target-arm/cpu.h   |  12 -
>  target-arm/op_helper.c |  49 ++---
>  target-arm/translate-a64.c | 130 
> +++--
>  target-arm/translate.c |   5 +-
>  target-arm/translate.h |   2 +
>  5 files changed, 173 insertions(+), 25 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 066ff67..256fbec 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -94,7 +94,17 @@
>  struct arm_boot_info;
>
>  #define NB_MMU_MODES 7
> -#define TARGET_INSN_START_EXTRA_WORDS 1
> +/* ARM-specific extra insn start words:
> + * 1: Conditional execution bits
> + * 2: Partial exception syndrome for data aborts
> + */
> +#define TARGET_INSN_START_EXTRA_WORDS 2
> +
> +/* The 2nd extra word holding syndrome info for data aborts does not use
> + * the lower 14 bits. We shift it down to help the sleb128 encoder do a
> + * better job. When restoring the CPU state, we shift it back up.
> + */
> +#define ARM_INSN_START_WORD2_SHIFT 14

We could also not bother putting bits 25..31 (ie the field that always reads
EC_DATAABORT) in the insn start word.

> @@ -720,23 +720,55 @@ static void gen_adc_CC(int sf, TCGv_i64 dest, TCGv_i64 
> t0, TCGv_i64 t1)
>   * Store from GPR register to memory.
>   */
>  static void do_gpr_st_memidx(DisasContext *s, TCGv_i64 source,
> - TCGv_i64 tcg_addr, int size, int memidx)
> + TCGv_i64 tcg_addr, int size, int memidx,
> + bool iss_valid,
> + unsigned int iss_srt,
> + bool iss_sf, bool iss_ar)
>  {
>  g_assert(size <= 3);
>  tcg_gen_qemu_st_i64(source, tcg_addr, memidx, s->be_data + size);
> +
> +if (iss_valid) {
> +uint32_t isyn32;
> +
> +isyn32 = syn_data_abort_with_iss(0,
> + size,
> + false,
> + iss_srt,
> + iss_sf,
> + iss_ar,
> + 0, 0, 0, 0, 0, false);
> +isyn32 >>= ARM_INSN_START_WORD2_SHIFT;
> +tcg_set_insn_param(s->insn_start_idx, 2, isyn32);

Is it worth doing
  assert(s->insn_start_idx);
  tcg_set_insn_param(...);
  s->insn_start_idx = 0;

as a way to catch accidentally trying to set the syndrome info twice ?

> +}
> +}
> +
> +static void do_gpr_st_with_isv(DisasContext *s, TCGv_i64 source,
> +   TCGv_i64 tcg_addr, int size,
> +   bool iss_valid,
> +   unsigned int iss_srt,
> +   bool iss_sf, bool iss_ar)
> +{
> +do_gpr_st_memidx(s, source, tcg_addr, size, get_mem_index(s),
> + iss_valid, iss_srt, iss_sf, iss_ar);
>  }
>
>  static void do_gpr_st(DisasContext *s, TCGv_i64 source,
>TCGv_i64 tcg_addr, int size)
>  {
> -do_gpr_st_memidx(s, source, tcg_addr, size, get_mem_index(s));
> +do_gpr_st_with_isv(s, source, tcg_addr, size,
> +   false, 0, false, false);
>  }

There's only two places where we use do_gpr_st() rather than the
_with_isv() version (both in the load/store pair function), and
similarly for do_gpr_ld(); so I think it would be better just to
have do_gpr_ld/st always take the iss arguments and not have a
separate do_gpr_st_with_isv(). (This also makes it harder to make
the mistake of using the plain do_gpr_st()  in future code and
forgetting that you need to think about whether to set syndrome info.)


Not in this patch series but something we need to fix:

(1) currently in arm_cpu_do_interrupt_aarch64() there is some
code which does
if (!env->thumb) {
env->cp15.esr_el[new_el] |= 1 << 25;
}
if we take an exception from 32 bit to 64 bit. This is completely
bogus because that's not what the IL bit in the syndrome is for.
This code should just be deleted, and rely on exception.syndrome
having a correct IL bit

[Qemu-devel] [PATCH v3 7/7] target-arm: A64: Create Instruction Syndromes for Data Aborts

2016-04-29 Thread Edgar E. Iglesias 
From: "Edgar E. Iglesias" 

Add support for generating the instruction syndrome for Data Aborts.
These syndromes are used by hypervisors for example to trap and emulate
memory accesses.

We save the decoded data out-of-band with the TBs at translation time.
When exceptions hit, the extra data attached to the TB is used to
recreate the state needed to encode instruction syndromes.
This avoids the need to emit moves with every load/store.

Based on a suggestion from Peter Maydell.

Suggested-by: Peter Maydell 
Signed-off-by: Edgar E. Iglesias 
---
 target-arm/cpu.h   |  12 -
 target-arm/op_helper.c |  49 ++---
 target-arm/translate-a64.c | 130 +++--
 target-arm/translate.c |   5 +-
 target-arm/translate.h |   2 +
 5 files changed, 173 insertions(+), 25 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 066ff67..256fbec 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -94,7 +94,17 @@
 struct arm_boot_info;
 
 #define NB_MMU_MODES 7
-#define TARGET_INSN_START_EXTRA_WORDS 1
+/* ARM-specific extra insn start words:
+ * 1: Conditional execution bits
+ * 2: Partial exception syndrome for data aborts
+ */
+#define TARGET_INSN_START_EXTRA_WORDS 2
+
+/* The 2nd extra word holding syndrome info for data aborts does not use
+ * the lower 14 bits. We shift it down to help the sleb128 encoder do a
+ * better job. When restoring the CPU state, we shift it back up.
+ */
+#define ARM_INSN_START_WORD2_SHIFT 14
 
 /* We currently assume float and double are IEEE single and double
precision respectively.
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index c7fba85..26aa4af 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -75,6 +75,43 @@ uint32_t HELPER(neon_tbl)(CPUARMState *env, uint32_t ireg, 
uint32_t def,
 
 #if !defined(CONFIG_USER_ONLY)
 
+static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
+unsigned int target_el,
+bool same_el,
+bool s1ptw, int is_write,
+int fsc)
+{
+uint32_t syn;
+
+/* ISV is only set for data aborts routed to EL2 and
+ * never for stage-1 page table walks faulting on stage 2.
+ *
+ * Furthermore, ISV is only set for certain kinds of load/stores.
+ * If the template syndrome does not have ISV set, we should leave
+ * it cleared.
+ *
+ * See ARMv8 specs, D7-1974:
+ * ISS encoding for an exception from a Data Abort, the
+ * ISV field.
+ */
+if (!(template_syn & ARM_EL_ISV) || target_el != 2 || s1ptw) {
+syn = syn_data_abort_no_iss(same_el,
+0, 0, s1ptw, is_write == 1, fsc);
+} else {
+/* Fields: IL, ISV, SAS, SSE, SRT, SF and AR come from the template
+ * syndrome created at translation time.
+ * Now we create the runtime syndrome with the remaining fields.
+ */
+syn = syn_data_abort_with_iss(same_el,
+  0, 0, 0, 0, 0,
+  0, 0, s1ptw, is_write == 1, fsc,
+  false);
+/* Merge the runtime syndrome with the template syndrome.  */
+syn |= template_syn;
+}
+return syn;
+}
+
 /* try to fill the TLB and return an exception if error. If retaddr is
  * NULL, it means that the function was called in C code (i.e. not
  * from generated code or from helper.c)
@@ -115,8 +152,8 @@ void tlb_fill(CPUState *cs, target_ulong addr, int 
is_write, int mmu_idx,
 syn = syn_insn_abort(same_el, 0, fi.s1ptw, syn);
 exc = EXCP_PREFETCH_ABORT;
 } else {
-syn = syn_data_abort_no_iss(same_el,
-0, 0, fi.s1ptw, is_write == 1, syn);
+syn = merge_syn_data_abort(env->exception.syndrome, target_el,
+   same_el, fi.s1ptw, is_write, syn);
 if (is_write == 1 && arm_feature(env, ARM_FEATURE_V6)) {
 fsr |= (1 << 11);
 }
@@ -137,6 +174,7 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, 
int is_write,
 CPUARMState *env = >env;
 int target_el;
 bool same_el;
+uint32_t syn;
 
 if (retaddr) {
 /* now we have a real cpu fault */
@@ -161,10 +199,9 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr 
vaddr, int is_write,
 env->exception.fsr |= (1 << 11);
 }
 
-raise_exception(env, EXCP_DATA_ABORT,
-syn_data_abort_no_iss(same_el,
-  0, 0, 0, is_write == 1, 0x21),
-target_el);
+syn = merge_syn_data_abort(env->exception.syndrome, target_el,
+