| Message ID | 20170525210508.4910-26-aurelien@aurel32.net |
|---|---|
| State | New |
| Headers | show |
On 05/25/2017 02:05 PM, Aurelien Jarno wrote: > +uint32_t HELPER(trXX)(CPUS390XState *env, uint32_t r1, uint32_t r2, > + uint32_t sizes) > +{ > + uintptr_t ra = GETPC(); > + int dsize = (sizes & 1) ? 1 : 2; > + int ssize = (sizes & 2) ? 1 : 2; > + uint16_t tst = env->regs[0] & ((1 << (8 * dsize)) - 1); I think you should pass in tst as an argument. That way you can pass in an out-of-band value when we implement ETF2 and test field M3 bit 3. r~
On 2017-05-26 10:10, Richard Henderson wrote: > On 05/25/2017 02:05 PM, Aurelien Jarno wrote: > > +uint32_t HELPER(trXX)(CPUS390XState *env, uint32_t r1, uint32_t r2, > > + uint32_t sizes) > > +{ > > + uintptr_t ra = GETPC(); > > + int dsize = (sizes & 1) ? 1 : 2; > > + int ssize = (sizes & 2) ? 1 : 2; > > + uint16_t tst = env->regs[0] & ((1 << (8 * dsize)) - 1); > > I think you should pass in tst as an argument. That way you can pass in an > out-of-band value when we implement ETF2 and test field M3 bit 3. I don't mind passing r0 as an argument. That said if we want to pass tst or bundle the M3 field, it means we need to use TCG instructions to do so. I am not sure it brings a lot compare to doing so in the helper side.
On 05/29/2017 04:17 AM, Aurelien Jarno wrote: > On 2017-05-26 10:10, Richard Henderson wrote: >> On 05/25/2017 02:05 PM, Aurelien Jarno wrote: >>> +uint32_t HELPER(trXX)(CPUS390XState *env, uint32_t r1, uint32_t r2, >>> + uint32_t sizes) >>> +{ >>> + uintptr_t ra = GETPC(); >>> + int dsize = (sizes & 1) ? 1 : 2; >>> + int ssize = (sizes & 2) ? 1 : 2; >>> + uint16_t tst = env->regs[0] & ((1 << (8 * dsize)) - 1); >> >> I think you should pass in tst as an argument. That way you can pass in an >> out-of-band value when we implement ETF2 and test field M3 bit 3. > > I don't mind passing r0 as an argument. That said if we want to pass tst > or bundle the M3 field, it means we need to use TCG instructions to do > so. I am not sure it brings a lot compare to doing so in the helper > side. Not at all -- the M3 bit test would be a translation-time check. r~
On 2017-05-30 09:45, Richard Henderson wrote: > On 05/29/2017 04:17 AM, Aurelien Jarno wrote: > > On 2017-05-26 10:10, Richard Henderson wrote: > > > On 05/25/2017 02:05 PM, Aurelien Jarno wrote: > > > > +uint32_t HELPER(trXX)(CPUS390XState *env, uint32_t r1, uint32_t r2, > > > > + uint32_t sizes) > > > > +{ > > > > + uintptr_t ra = GETPC(); > > > > + int dsize = (sizes & 1) ? 1 : 2; > > > > + int ssize = (sizes & 2) ? 1 : 2; > > > > + uint16_t tst = env->regs[0] & ((1 << (8 * dsize)) - 1); > > > > > > I think you should pass in tst as an argument. That way you can pass in an > > > out-of-band value when we implement ETF2 and test field M3 bit 3. > > > > I don't mind passing r0 as an argument. That said if we want to pass tst > > or bundle the M3 field, it means we need to use TCG instructions to do > > so. I am not sure it brings a lot compare to doing so in the helper > > side. > > Not at all -- the M3 bit test would be a translation-time check. I still don't really see the point. On the TCG side it means we need something like that: if (m3 & 1) { tcg_gen_movi_tl(r0, -1); } else if (dsize == 1) { tcg_gen_ext8u(r0, regs[0]); } else if (dsize == 2) tcg_gen_ext16u(r0, regs[0]); } On the helper side we then need to check if the value passed equals -1 or not to get m3 instead of directly accessing the value in register 0. If we want to bundle m3 with something else, it might be better to bundle it with the "sizes" argument. If what worries you is the cast of tst to uint8_t / uint16_t as function of dsize in the helper, we can rename trXX into an inline function do_trXX and use one helper per instruction instead of a common helper for the 3 instructions.
On 05/30/2017 12:25 PM, Aurelien Jarno wrote: > On 2017-05-30 09:45, Richard Henderson wrote: >> On 05/29/2017 04:17 AM, Aurelien Jarno wrote: >>> On 2017-05-26 10:10, Richard Henderson wrote: >>>> On 05/25/2017 02:05 PM, Aurelien Jarno wrote: >>>>> +uint32_t HELPER(trXX)(CPUS390XState *env, uint32_t r1, uint32_t r2, >>>>> + uint32_t sizes) >>>>> +{ >>>>> + uintptr_t ra = GETPC(); >>>>> + int dsize = (sizes & 1) ? 1 : 2; >>>>> + int ssize = (sizes & 2) ? 1 : 2; >>>>> + uint16_t tst = env->regs[0] & ((1 << (8 * dsize)) - 1); >>>> >>>> I think you should pass in tst as an argument. That way you can pass in an >>>> out-of-band value when we implement ETF2 and test field M3 bit 3. >>> >>> I don't mind passing r0 as an argument. That said if we want to pass tst >>> or bundle the M3 field, it means we need to use TCG instructions to do >>> so. I am not sure it brings a lot compare to doing so in the helper >>> side. >> >> Not at all -- the M3 bit test would be a translation-time check. > > I still don't really see the point. On the TCG side it means we need > something like that: > > if (m3 & 1) { > tcg_gen_movi_tl(r0, -1); > } else if (dsize == 1) { > tcg_gen_ext8u(r0, regs[0]); > } else if (dsize == 2) > tcg_gen_ext16u(r0, regs[0]); > } Yes, exactly. > On the helper side we then need to check if the value passed equals -1 > or not to get m3 instead of directly accessing the value in register 0. How's that? Why would you need to check for -1 at all? The existing helper test works fine, with -1 not matching any value loaded. r~
On 2017-05-30 12:42, Richard Henderson wrote: > On 05/30/2017 12:25 PM, Aurelien Jarno wrote: > > On 2017-05-30 09:45, Richard Henderson wrote: > > > On 05/29/2017 04:17 AM, Aurelien Jarno wrote: > > > > On 2017-05-26 10:10, Richard Henderson wrote: > > > > > On 05/25/2017 02:05 PM, Aurelien Jarno wrote: > > > > > > +uint32_t HELPER(trXX)(CPUS390XState *env, uint32_t r1, uint32_t r2, > > > > > > + uint32_t sizes) > > > > > > +{ > > > > > > + uintptr_t ra = GETPC(); > > > > > > + int dsize = (sizes & 1) ? 1 : 2; > > > > > > + int ssize = (sizes & 2) ? 1 : 2; > > > > > > + uint16_t tst = env->regs[0] & ((1 << (8 * dsize)) - 1); > > > > > > > > > > I think you should pass in tst as an argument. That way you can pass in an > > > > > out-of-band value when we implement ETF2 and test field M3 bit 3. > > > > > > > > I don't mind passing r0 as an argument. That said if we want to pass tst > > > > or bundle the M3 field, it means we need to use TCG instructions to do > > > > so. I am not sure it brings a lot compare to doing so in the helper > > > > side. > > > > > > Not at all -- the M3 bit test would be a translation-time check. > > > > I still don't really see the point. On the TCG side it means we need > > something like that: > > > > if (m3 & 1) { > > tcg_gen_movi_tl(r0, -1); > > } else if (dsize == 1) { > > tcg_gen_ext8u(r0, regs[0]); > > } else if (dsize == 2) > > tcg_gen_ext16u(r0, regs[0]); > > } > > Yes, exactly. > > > On the helper side we then need to check if the value passed equals -1 > > or not to get m3 instead of directly accessing the value in register 0. > > How's that? Why would you need to check for -1 at all? The existing helper > test works fine, with -1 not matching any value loaded. Ok, I get your point now, i'll implement that in the next version.
diff --git a/target/s390x/helper.h b/target/s390x/helper.h index 7bfb0223ce..e469011196 100644 --- a/target/s390x/helper.h +++ b/target/s390x/helper.h @@ -92,6 +92,7 @@ DEF_HELPER_FLAGS_3(tp, TCG_CALL_NO_WG, i32, env, i64, i32) DEF_HELPER_FLAGS_4(tr, TCG_CALL_NO_WG, void, env, i32, i64, i64) DEF_HELPER_4(tre, i64, env, i64, i64, i64) DEF_HELPER_4(trt, i32, env, i32, i64, i64) +DEF_HELPER_4(trXX, i32, env, i32, i32, i32) DEF_HELPER_4(cksm, i64, env, i64, i64, i64) DEF_HELPER_FLAGS_5(calc_cc, TCG_CALL_NO_RWG_SE, i32, env, i32, i64, i64, i64) DEF_HELPER_FLAGS_2(sfpc, TCG_CALL_NO_RWG, void, env, i64) diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def index c0fe04e45c..68364dfec3 100644 --- a/target/s390x/insn-data.def +++ b/target/s390x/insn-data.def @@ -868,6 +868,15 @@ /* TRANSLATE EXTENDED */ C(0xb2a5, TRE, RRE, Z, 0, r2, r1_P, 0, tre, 0) +/* TRANSLATE ONE TO ONE */ + C(0xb993, TROO, RRF_c, E2, 0, 0, 0, 0, trXX, 0) +/* TRANSLATE ONE TO TWO */ + C(0xb992, TROT, RRF_c, E2, 0, 0, 0, 0, trXX, 0) +/* TRANSLATE TWO TO ONE */ + C(0xb991, TRTO, RRF_c, E2, 0, 0, 0, 0, trXX, 0) +/* TRANSLATE TWO TO TWO */ + C(0xb990, TRTT, RRF_c, E2, 0, 0, 0, 0, trXX, 0) + /* UNPACK */ /* Really format SS_b, but we pack both lengths into one argument for the helper call, so we might as well leave one 8-bit field. */ diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index b6e083c7e1..47e772bb12 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -1240,6 +1240,52 @@ uint32_t HELPER(trt)(CPUS390XState *env, uint32_t len, uint64_t array, return do_helper_trt(env, len, array, trans, GETPC()); } +/* Translate one/two to one/two */ +uint32_t HELPER(trXX)(CPUS390XState *env, uint32_t r1, uint32_t r2, + uint32_t sizes) +{ + uintptr_t ra = GETPC(); + int dsize = (sizes & 1) ? 1 : 2; + int ssize = (sizes & 2) ? 1 : 2; + uint16_t tst = env->regs[0] & ((1 << (8 * dsize)) - 1); + uint64_t tbl = get_address(env, 1) & ~7; + uint64_t dst = get_address(env, r1); + uint64_t len = get_length(env, r1 + 1); + uint64_t src = get_address(env, r2); + uint32_t cc = 3; + int i; + + check_alignment(env, len, ssize, ra); + + /* Lest we fail to service interrupts in a timely manner, */ + /* limit the amount of work we're willing to do. */ + for (i = 0; i < 0x2000; i++) { + uint16_t sval = cpu_ldusize_data_ra(env, src, ssize, ra); + uint64_t tble = tbl + (sval * dsize); + uint16_t dval = cpu_ldusize_data_ra(env, tble, dsize, ra); + if (dval == tst) { + cc = 1; + break; + } + cpu_stsize_data_ra(env, dst, dval, dsize, ra); + + len -= ssize; + src += ssize; + dst += dsize; + + if (len == 0) { + cc = 0; + break; + } + } + + set_address(env, r1, dst); + set_length(env, r1 + 1, len); + set_address(env, r2, src); + + return cc; +} + void HELPER(cdsg)(CPUS390XState *env, uint64_t addr, uint32_t r1, uint32_t r3) { diff --git a/target/s390x/translate.c b/target/s390x/translate.c index b51342583e..38c4bf1812 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -4286,6 +4286,19 @@ static ExitStatus op_trt(DisasContext *s, DisasOps *o) return NO_EXIT; } +static ExitStatus op_trXX(DisasContext *s, DisasOps *o) +{ + TCGv_i32 r1 = tcg_const_i32(get_field(s->fields, r1)); + TCGv_i32 r2 = tcg_const_i32(get_field(s->fields, r2)); + TCGv_i32 sizes = tcg_const_i32(s->insn->opc & 3); + gen_helper_trXX(cc_op, cpu_env, r1, r2, sizes); + tcg_temp_free_i32(r1); + tcg_temp_free_i32(r2); + tcg_temp_free_i32(sizes); + set_cc_static(s); + return NO_EXIT; +} + static ExitStatus op_ts(DisasContext *s, DisasOps *o) { TCGv_i32 t1 = tcg_const_i32(0xff);
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> --- target/s390x/helper.h | 1 + target/s390x/insn-data.def | 9 +++++++++ target/s390x/mem_helper.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ target/s390x/translate.c | 13 +++++++++++++ 4 files changed, 69 insertions(+)