| Message ID | 20220227020413.11741-7-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tcg: support 32-bit guest addresses as signed | expand |
On Sun, 27 Feb 2022 at 02:10, Richard Henderson <richard.henderson@linaro.org> wrote: > > AArch64 has both sign and zero-extending addressing modes, which > means that either treatment of guest addresses is equally efficient. > Enabling this for AArch64 gives us testing of the feature in CI. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > tcg/aarch64/tcg-target-sa32.h | 8 +++- > tcg/aarch64/tcg-target.c.inc | 69 +++++++++++++++++++++++------------ > 2 files changed, 52 insertions(+), 25 deletions(-) > > diff --git a/tcg/aarch64/tcg-target-sa32.h b/tcg/aarch64/tcg-target-sa32.h > index cb185b1526..c99e502e4c 100644 > --- a/tcg/aarch64/tcg-target-sa32.h > +++ b/tcg/aarch64/tcg-target-sa32.h > @@ -1 +1,7 @@ > -#define TCG_TARGET_SIGNED_ADDR32 0 > +/* > + * AArch64 has both SXTW and UXTW addressing modes, which means that > + * it is agnostic to how guest addresses should be represented. > + * Because aarch64 is more common than the other hosts that will > + * want to use this feature, enable it for continuous testing. > + */ > +#define TCG_TARGET_SIGNED_ADDR32 1 > diff --git a/tcg/aarch64/tcg-target.c.inc b/tcg/aarch64/tcg-target.c.inc > index 077fc51401..65cab73ea0 100644 > --- a/tcg/aarch64/tcg-target.c.inc > +++ b/tcg/aarch64/tcg-target.c.inc > static void tcg_out_qemu_ld(TCGContext *s, TCGReg data_reg, TCGReg addr_reg, > MemOpIdx oi, TCGType ext) > { > MemOp memop = get_memop(oi); > - const TCGType otype = TARGET_LONG_BITS == 64 ? TCG_TYPE_I64 : TCG_TYPE_I32; > + int option = ldst_ext_option(); > > /* Byte swapping is left to middle-end expansion. */ > tcg_debug_assert((memop & MO_BSWAP) == 0); > @@ -1833,7 +1854,7 @@ static void tcg_out_qemu_ld(TCGContext *s, TCGReg data_reg, TCGReg addr_reg, > > tcg_out_tlb_read(s, addr_reg, memop, &label_ptr, mem_index, 1); > tcg_out_qemu_ld_direct(s, memop, ext, data_reg, > - TCG_REG_X1, otype, addr_reg); > + TCG_REG_X1, option, addr_reg); > add_qemu_ldst_label(s, true, oi, ext, data_reg, addr_reg, > s->code_ptr, label_ptr); > #else /* !CONFIG_SOFTMMU */ > @@ -1843,10 +1864,10 @@ static void tcg_out_qemu_ld(TCGContext *s, TCGReg data_reg, TCGReg addr_reg, > } > if (USE_GUEST_BASE) { > tcg_out_qemu_ld_direct(s, memop, ext, data_reg, > - TCG_REG_GUEST_BASE, otype, addr_reg); > + TCG_REG_GUEST_BASE, option, addr_reg); > } else { > tcg_out_qemu_ld_direct(s, memop, ext, data_reg, > - addr_reg, TCG_TYPE_I64, TCG_REG_XZR); > + addr_reg, option, TCG_REG_XZR); This doesn't look right. 'option' specifies how we extend the offset register, but here that is XZR, which is 0 no matter how we choose to extend it, whereas we aren't going to be extending the base register 'addr_reg' which is what we do need to either zero or sign extend. Unfortunately we can't just flip addr_reg and XZR around, because XZR isn't valid as the base reg. Is this a pre-existing bug? If addr_reg needs zero extending we won't be doing that. > } > #endif /* CONFIG_SOFTMMU */ > } > @@ -1855,7 +1876,7 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg data_reg, TCGReg addr_reg, > MemOpIdx oi) > { > MemOp memop = get_memop(oi); > - const TCGType otype = TARGET_LONG_BITS == 64 ? TCG_TYPE_I64 : TCG_TYPE_I32; > + int option = ldst_ext_option(); > > /* Byte swapping is left to middle-end expansion. */ > tcg_debug_assert((memop & MO_BSWAP) == 0); > @@ -1866,7 +1887,7 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg data_reg, TCGReg addr_reg, > > tcg_out_tlb_read(s, addr_reg, memop, &label_ptr, mem_index, 0); > tcg_out_qemu_st_direct(s, memop, data_reg, > - TCG_REG_X1, otype, addr_reg); > + TCG_REG_X1, option, addr_reg); > add_qemu_ldst_label(s, false, oi, (memop & MO_SIZE)== MO_64, > data_reg, addr_reg, s->code_ptr, label_ptr); > #else /* !CONFIG_SOFTMMU */ > @@ -1876,10 +1897,10 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg data_reg, TCGReg addr_reg, > } > if (USE_GUEST_BASE) { > tcg_out_qemu_st_direct(s, memop, data_reg, > - TCG_REG_GUEST_BASE, otype, addr_reg); > + TCG_REG_GUEST_BASE, option, addr_reg); > } else { > tcg_out_qemu_st_direct(s, memop, data_reg, > - addr_reg, TCG_TYPE_I64, TCG_REG_XZR); > + addr_reg, option, TCG_REG_XZR); > Similarly here. thanks -- PMM
On 3/3/22 05:04, Peter Maydell wrote: >> if (USE_GUEST_BASE) { >> tcg_out_qemu_ld_direct(s, memop, ext, data_reg, >> - TCG_REG_GUEST_BASE, otype, addr_reg); >> + TCG_REG_GUEST_BASE, option, addr_reg); >> } else { >> tcg_out_qemu_ld_direct(s, memop, ext, data_reg, >> - addr_reg, TCG_TYPE_I64, TCG_REG_XZR); >> + addr_reg, option, TCG_REG_XZR); > > This doesn't look right. 'option' specifies how we extend the offset > register, but here that is XZR, which is 0 no matter how we choose > to extend it, whereas we aren't going to be extending the base > register 'addr_reg' which is what we do need to either zero or > sign extend. Unfortunately we can't just flip addr_reg and XZR > around, because XZR isn't valid as the base reg. > > Is this a pre-existing bug? If addr_reg needs zero extending > we won't be doing that. It's just confusing, because stuff is hidden in macros: #define USE_GUEST_BASE (guest_base != 0 || TARGET_LONG_BITS == 32) We *always* use TCG_REG_GUEST_BASE when we require an extension, so the else case you point out will always have option == 3 /* LSL #0 */. Previously I had a named constant I could use here, but I didn't create names for the full 'option' field being filled, so I thought it clearer to just pass along the variable. Would it be clearer as 3 /* LSL #0 */ or with some LDST_OPTION_LSL0? r~
On Thu, 3 Mar 2022 at 15:43, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 3/3/22 05:04, Peter Maydell wrote: > >> if (USE_GUEST_BASE) { > >> tcg_out_qemu_ld_direct(s, memop, ext, data_reg, > >> - TCG_REG_GUEST_BASE, otype, addr_reg); > >> + TCG_REG_GUEST_BASE, option, addr_reg); > >> } else { > >> tcg_out_qemu_ld_direct(s, memop, ext, data_reg, > >> - addr_reg, TCG_TYPE_I64, TCG_REG_XZR); > >> + addr_reg, option, TCG_REG_XZR); > > > > This doesn't look right. 'option' specifies how we extend the offset > > register, but here that is XZR, which is 0 no matter how we choose > > to extend it, whereas we aren't going to be extending the base > > register 'addr_reg' which is what we do need to either zero or > > sign extend. Unfortunately we can't just flip addr_reg and XZR > > around, because XZR isn't valid as the base reg. > > > > Is this a pre-existing bug? If addr_reg needs zero extending > > we won't be doing that. > > It's just confusing, because stuff is hidden in macros: > > #define USE_GUEST_BASE (guest_base != 0 || TARGET_LONG_BITS == 32) > > We *always* use TCG_REG_GUEST_BASE when we require an extension, so the else case you > point out will always have option == 3 /* LSL #0 */. > > Previously I had a named constant I could use here, but I didn't create names for the full > 'option' field being filled, so I thought it clearer to just pass along the variable. > Would it be clearer as > > 3 /* LSL #0 */ > > or with some LDST_OPTION_LSL0? I think that using something that says it's LSL 0 (either comment as done elsewhere in the patch, or maybe better with some symbolic constant) would help, yes. Plus an assert or a comment that we know we don't need to extend addr_reg in this half of the if(). thanks -- PMM
diff --git a/tcg/aarch64/tcg-target-sa32.h b/tcg/aarch64/tcg-target-sa32.h index cb185b1526..c99e502e4c 100644 --- a/tcg/aarch64/tcg-target-sa32.h +++ b/tcg/aarch64/tcg-target-sa32.h @@ -1 +1,7 @@ -#define TCG_TARGET_SIGNED_ADDR32 0 +/* + * AArch64 has both SXTW and UXTW addressing modes, which means that + * it is agnostic to how guest addresses should be represented. + * Because aarch64 is more common than the other hosts that will + * want to use this feature, enable it for continuous testing. + */ +#define TCG_TARGET_SIGNED_ADDR32 1 diff --git a/tcg/aarch64/tcg-target.c.inc b/tcg/aarch64/tcg-target.c.inc index 077fc51401..65cab73ea0 100644 --- a/tcg/aarch64/tcg-target.c.inc +++ b/tcg/aarch64/tcg-target.c.inc @@ -806,12 +806,12 @@ static void tcg_out_insn_3617(TCGContext *s, AArch64Insn insn, bool q, } static void tcg_out_insn_3310(TCGContext *s, AArch64Insn insn, - TCGReg rd, TCGReg base, TCGType ext, + TCGReg rd, TCGReg base, int option, TCGReg regoff) { /* Note the AArch64Insn constants above are for C3.3.12. Adjust. */ tcg_out32(s, insn | I3312_TO_I3310 | regoff << 16 | - 0x4000 | ext << 13 | base << 5 | (rd & 0x1f)); + option << 13 | base << 5 | (rd & 0x1f)); } static void tcg_out_insn_3312(TCGContext *s, AArch64Insn insn, @@ -1126,7 +1126,7 @@ static void tcg_out_ldst(TCGContext *s, AArch64Insn insn, TCGReg rd, /* Worst-case scenario, move offset to temp register, use reg offset. */ tcg_out_movi(s, TCG_TYPE_I64, TCG_REG_TMP, offset); - tcg_out_ldst_r(s, insn, rd, rn, TCG_TYPE_I64, TCG_REG_TMP); + tcg_out_ldst_r(s, insn, rd, rn, 3 /* LSL #0 */, TCG_REG_TMP); } static bool tcg_out_mov(TCGContext *s, TCGType type, TCGReg ret, TCGReg arg) @@ -1765,31 +1765,31 @@ static bool tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *l) static void tcg_out_qemu_ld_direct(TCGContext *s, MemOp memop, TCGType ext, TCGReg data_r, TCGReg addr_r, - TCGType otype, TCGReg off_r) + int option, TCGReg off_r) { switch (memop & MO_SSIZE) { case MO_UB: - tcg_out_ldst_r(s, I3312_LDRB, data_r, addr_r, otype, off_r); + tcg_out_ldst_r(s, I3312_LDRB, data_r, addr_r, option, off_r); break; case MO_SB: tcg_out_ldst_r(s, ext ? I3312_LDRSBX : I3312_LDRSBW, - data_r, addr_r, otype, off_r); + data_r, addr_r, option, off_r); break; case MO_UW: - tcg_out_ldst_r(s, I3312_LDRH, data_r, addr_r, otype, off_r); + tcg_out_ldst_r(s, I3312_LDRH, data_r, addr_r, option, off_r); break; case MO_SW: tcg_out_ldst_r(s, (ext ? I3312_LDRSHX : I3312_LDRSHW), - data_r, addr_r, otype, off_r); + data_r, addr_r, option, off_r); break; case MO_UL: - tcg_out_ldst_r(s, I3312_LDRW, data_r, addr_r, otype, off_r); + tcg_out_ldst_r(s, I3312_LDRW, data_r, addr_r, option, off_r); break; case MO_SL: - tcg_out_ldst_r(s, I3312_LDRSWX, data_r, addr_r, otype, off_r); + tcg_out_ldst_r(s, I3312_LDRSWX, data_r, addr_r, option, off_r); break; case MO_UQ: - tcg_out_ldst_r(s, I3312_LDRX, data_r, addr_r, otype, off_r); + tcg_out_ldst_r(s, I3312_LDRX, data_r, addr_r, option, off_r); break; default: tcg_abort(); @@ -1798,31 +1798,52 @@ static void tcg_out_qemu_ld_direct(TCGContext *s, MemOp memop, TCGType ext, static void tcg_out_qemu_st_direct(TCGContext *s, MemOp memop, TCGReg data_r, TCGReg addr_r, - TCGType otype, TCGReg off_r) + int option, TCGReg off_r) { switch (memop & MO_SIZE) { case MO_8: - tcg_out_ldst_r(s, I3312_STRB, data_r, addr_r, otype, off_r); + tcg_out_ldst_r(s, I3312_STRB, data_r, addr_r, option, off_r); break; case MO_16: - tcg_out_ldst_r(s, I3312_STRH, data_r, addr_r, otype, off_r); + tcg_out_ldst_r(s, I3312_STRH, data_r, addr_r, option, off_r); break; case MO_32: - tcg_out_ldst_r(s, I3312_STRW, data_r, addr_r, otype, off_r); + tcg_out_ldst_r(s, I3312_STRW, data_r, addr_r, option, off_r); break; case MO_64: - tcg_out_ldst_r(s, I3312_STRX, data_r, addr_r, otype, off_r); + tcg_out_ldst_r(s, I3312_STRX, data_r, addr_r, option, off_r); break; default: tcg_abort(); } } +/* + * Bits for the option field of LDR/STR (register), + * for application to a guest address. + */ +static int ldst_ext_option(void) +{ +#ifdef CONFIG_USER_ONLY + bool signed_addr32 = guest_base_signed_addr32; +#else + bool signed_addr32 = TCG_TARGET_SIGNED_ADDR32; +#endif + + if (TARGET_LONG_BITS == 64) { + return 3; /* LSL #0 */ + } else if (signed_addr32) { + return 6; /* SXTW */ + } else { + return 2; /* UXTW */ + } +} + static void tcg_out_qemu_ld(TCGContext *s, TCGReg data_reg, TCGReg addr_reg, MemOpIdx oi, TCGType ext) { MemOp memop = get_memop(oi); - const TCGType otype = TARGET_LONG_BITS == 64 ? TCG_TYPE_I64 : TCG_TYPE_I32; + int option = ldst_ext_option(); /* Byte swapping is left to middle-end expansion. */ tcg_debug_assert((memop & MO_BSWAP) == 0); @@ -1833,7 +1854,7 @@ static void tcg_out_qemu_ld(TCGContext *s, TCGReg data_reg, TCGReg addr_reg, tcg_out_tlb_read(s, addr_reg, memop, &label_ptr, mem_index, 1); tcg_out_qemu_ld_direct(s, memop, ext, data_reg, - TCG_REG_X1, otype, addr_reg); + TCG_REG_X1, option, addr_reg); add_qemu_ldst_label(s, true, oi, ext, data_reg, addr_reg, s->code_ptr, label_ptr); #else /* !CONFIG_SOFTMMU */ @@ -1843,10 +1864,10 @@ static void tcg_out_qemu_ld(TCGContext *s, TCGReg data_reg, TCGReg addr_reg, } if (USE_GUEST_BASE) { tcg_out_qemu_ld_direct(s, memop, ext, data_reg, - TCG_REG_GUEST_BASE, otype, addr_reg); + TCG_REG_GUEST_BASE, option, addr_reg); } else { tcg_out_qemu_ld_direct(s, memop, ext, data_reg, - addr_reg, TCG_TYPE_I64, TCG_REG_XZR); + addr_reg, option, TCG_REG_XZR); } #endif /* CONFIG_SOFTMMU */ } @@ -1855,7 +1876,7 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg data_reg, TCGReg addr_reg, MemOpIdx oi) { MemOp memop = get_memop(oi); - const TCGType otype = TARGET_LONG_BITS == 64 ? TCG_TYPE_I64 : TCG_TYPE_I32; + int option = ldst_ext_option(); /* Byte swapping is left to middle-end expansion. */ tcg_debug_assert((memop & MO_BSWAP) == 0); @@ -1866,7 +1887,7 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg data_reg, TCGReg addr_reg, tcg_out_tlb_read(s, addr_reg, memop, &label_ptr, mem_index, 0); tcg_out_qemu_st_direct(s, memop, data_reg, - TCG_REG_X1, otype, addr_reg); + TCG_REG_X1, option, addr_reg); add_qemu_ldst_label(s, false, oi, (memop & MO_SIZE)== MO_64, data_reg, addr_reg, s->code_ptr, label_ptr); #else /* !CONFIG_SOFTMMU */ @@ -1876,10 +1897,10 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg data_reg, TCGReg addr_reg, } if (USE_GUEST_BASE) { tcg_out_qemu_st_direct(s, memop, data_reg, - TCG_REG_GUEST_BASE, otype, addr_reg); + TCG_REG_GUEST_BASE, option, addr_reg); } else { tcg_out_qemu_st_direct(s, memop, data_reg, - addr_reg, TCG_TYPE_I64, TCG_REG_XZR); + addr_reg, option, TCG_REG_XZR); } #endif /* CONFIG_SOFTMMU */ }
AArch64 has both sign and zero-extending addressing modes, which means that either treatment of guest addresses is equally efficient. Enabling this for AArch64 gives us testing of the feature in CI. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- tcg/aarch64/tcg-target-sa32.h | 8 +++- tcg/aarch64/tcg-target.c.inc | 69 +++++++++++++++++++++++------------ 2 files changed, 52 insertions(+), 25 deletions(-)