Re: [Qemu-devel] [PATCH v2 1/3] tcg/arm: fix TLB access in qemu-ld/st ops

[Aurelien Jarno]

On Wed, Oct 31, 2012 at 02:54:38PM +0100, Laurent Desnogues wrote:
> On Tue, Oct 30, 2012 at 1:18 AM,  <address@hidden> wrote:
> > From: Aurelien Jarno <address@hidden>
> >
> > The TCG arm backend considers likely that the offset to the TLB
> > entries does not exceed 12 bits for mem_index = 0. In practice this is
> > not true for at least the MIPS target.
> >
> > The current patch fixes that by loading the bits 23-12 with a separate
> > instruction, and using loads with address writeback, independently of
> > the value of mem_idx. In total this allow a 24-bit offset, which is a
> > lot more than needed.
> >
> > Cc: Andrzej Zaborowski <address@hidden>
> > Cc: Peter Maydell <address@hidden>
> > Cc: address@hidden
> > Signed-off-by: Aurelien Jarno <address@hidden>
> > ---
> >  tcg/arm/tcg-target.c |   77 
> > +++++++++++++++++++++++++++-----------------------
> >  1 file changed, 41 insertions(+), 36 deletions(-)
> >
> > diff --git a/tcg/arm/tcg-target.c b/tcg/arm/tcg-target.c
> > index e790bf0..03b1576 100644
> > --- a/tcg/arm/tcg-target.c
> > +++ b/tcg/arm/tcg-target.c
> > @@ -639,6 +639,22 @@ static inline void tcg_out_ld32_12(TCGContext *s, int 
> > cond,
> >                          (rn << 16) | (rd << 12) | ((-im) & 0xfff));
> >  }
> >
> > +/* Offset pre-increment with base writeback.  */
> > +static inline void tcg_out_ld32_12wb(TCGContext *s, int cond,
> > +                                     int rd, int rn, tcg_target_long im)
> > +{
> > +    /* ldr with writeback and both register equals is UNPREDICTABLE */
> > +    assert(rd != rn);
> > +
> > +    if (im >= 0) {
> > +        tcg_out32(s, (cond << 28) | 0x05b00000 |
> > +                        (rn << 16) | (rd << 12) | (im & 0xfff));
> > +    } else {
> > +        tcg_out32(s, (cond << 28) | 0x05300000 |
> > +                        (rn << 16) | (rd << 12) | ((-im) & 0xfff));
> > +    }
> > +}
> > +
> >  static inline void tcg_out_st32_12(TCGContext *s, int cond,
> >                  int rd, int rn, tcg_target_long im)
> >  {
> > @@ -1071,7 +1087,7 @@ static inline void tcg_out_qemu_ld(TCGContext *s, 
> > const TCGArg *args, int opc)
> >  {
> >      int addr_reg, data_reg, data_reg2, bswap;
> >  #ifdef CONFIG_SOFTMMU
> > -    int mem_index, s_bits;
> > +    int mem_index, s_bits, tlb_offset;
> >      TCGReg argreg;
> >  # if TARGET_LONG_BITS == 64
> >      int addr_reg2;
> > @@ -1111,19 +1127,15 @@ static inline void tcg_out_qemu_ld(TCGContext *s, 
> > const TCGArg *args, int opc)
> >                      TCG_REG_R0, TCG_REG_R8, CPU_TLB_SIZE - 1);
> >      tcg_out_dat_reg(s, COND_AL, ARITH_ADD, TCG_REG_R0, TCG_AREG0,
> >                      TCG_REG_R0, SHIFT_IMM_LSL(CPU_TLB_ENTRY_BITS));
> > -    /* In the
> > -     *  ldr r1 [r0, #(offsetof(CPUArchState, 
> > tlb_table[mem_index][0].addr_read))]
> > -     * below, the offset is likely to exceed 12 bits if mem_index != 0 and
> > -     * not exceed otherwise, so use an
> > -     *  add r0, r0, #(mem_index * sizeof *CPUArchState.tlb_table)
> > -     * before.
> > -     */
> > -    if (mem_index)
> > +    /* We assume that the offset is contained within 24 bits.  */
> > +    tlb_offset = offsetof(CPUArchState, tlb_table[mem_index][0].addr_read);
> > +    assert(tlb_offset & ~0xffffff == 0);
> > +    if (tlb_offset > 0xfff) {
> >          tcg_out_dat_imm(s, COND_AL, ARITH_ADD, TCG_REG_R0, TCG_REG_R0,
> > -                        (mem_index << (TLB_SHIFT & 1)) |
> > -                        ((16 - (TLB_SHIFT >> 1)) << 8));
> > -    tcg_out_ld32_12(s, COND_AL, TCG_REG_R1, TCG_REG_R0,
> > -                    offsetof(CPUArchState, tlb_table[0][0].addr_read));
> > +                        0xa00 | (tlb_offset >> 12));
> 
> Isn't it 20 bits rather than 24 bits since the immediate is 8-bit 
> right-rotated
> by 20?
> 

You are indeed correct. I'll send a new version of the patch soon.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net