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

[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 <address@hidden> wrote:
> > From: "Edgar E. Iglesias" <address@hidden>
> >
> > 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 <address@hidden>
> > Signed-off-by: Edgar E. Iglesias <address@hidden>
> > ---
> >  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