On Wed, May 04, 2016 at 06:38:49PM +0100, Peter Maydell wrote: > On 29 April 2016 at 13:08, Edgar E. Iglesias <edgar.igles...@gmail.com> wrote: > > From: "Edgar E. Iglesias" <edgar.igles...@xilinx.com> > > > > 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 <peter.mayd...@linaro.org> > > Signed-off-by: Edgar E. Iglesias <edgar.igles...@xilinx.com> > > --- > > 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 do_gpr_st_with_isv(). (This also makes it harder to make > the mistake of using the plain do_gpr_st() &c in future code and > forgetting that you need to think about whether to set syndrome info.) I've removed the _with_isv versions for v4. > 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 (which it didn't for data aborts before > your series but will afterwards). > > (2) syn_insn_abort(), syn_swstep(), syn_watchpoint() > should all create syndromes with the IL bit set, but don't. Sounds good! Thanks, Edgar