| Message ID | 1494926884-10118-1-git-send-email-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2017-05-16 11:28, Thomas Huth wrote: > TEST BLOCK was likely once used to execute basic memory > tests, but nowadays it's just a (slow) way to clear a page. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > target/s390x/helper.h | 1 + > target/s390x/insn-data.def | 2 ++ > target/s390x/mem_helper.c | 12 ++++++++++++ > target/s390x/translate.c | 10 ++++++++++ > 4 files changed, 25 insertions(+) > > diff --git a/target/s390x/helper.h b/target/s390x/helper.h > index 9102071..a5a3705 100644 > --- a/target/s390x/helper.h > +++ b/target/s390x/helper.h > @@ -99,6 +99,7 @@ DEF_HELPER_FLAGS_4(lctl, TCG_CALL_NO_WG, void, env, i32, i64, i32) > DEF_HELPER_FLAGS_4(lctlg, TCG_CALL_NO_WG, void, env, i32, i64, i32) > DEF_HELPER_FLAGS_4(stctl, TCG_CALL_NO_WG, void, env, i32, i64, i32) > DEF_HELPER_FLAGS_4(stctg, TCG_CALL_NO_WG, void, env, i32, i64, i32) > +DEF_HELPER_2(testblock, void, env, i64) > DEF_HELPER_FLAGS_2(tprot, TCG_CALL_NO_RWG, i32, i64, i64) > DEF_HELPER_FLAGS_2(iske, TCG_CALL_NO_RWG_SE, i64, env, i64) > DEF_HELPER_FLAGS_3(sske, TCG_CALL_NO_RWG, void, env, i64, i64) > diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def > index 075ff59..a6aba54 100644 > --- a/target/s390x/insn-data.def > +++ b/target/s390x/insn-data.def > @@ -912,6 +912,8 @@ > /* STORE USING REAL ADDRESS */ > C(0xb246, STURA, RRE, Z, r1_o, r2_o, 0, 0, stura, 0) > C(0xb925, STURG, RRE, Z, r1_o, r2_o, 0, 0, sturg, 0) > +/* TEST BLOCK */ > + C(0xb22c, TB, RRE, Z, 0, r2_o, 0, 0, testblock, 0) > /* TEST PROTECTION */ > C(0xe501, TPROT, SSE, Z, la1, a2, 0, 0, tprot, 0) > > diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c > index 675aba2..0349c10 100644 > --- a/target/s390x/mem_helper.c > +++ b/target/s390x/mem_helper.c > @@ -933,6 +933,18 @@ void HELPER(stctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) > } > } > > +void HELPER(testblock)(CPUS390XState *env, uint64_t addr) > +{ > + CPUState *cs = CPU(s390_env_get_cpu(env)); > + int i; > + > + addr = get_address(env, 0, 0, addr) & ~0xfffULL; I guess you want to use ~TARGET_PAGE_MASK here. > + for (i = 0; i < TARGET_PAGE_SIZE; i += 8) { > + stq_phys(cs->as, addr + i, 0); > + } > + env->cc_op = 0; > +} > + From what I understand the resulting condition code should depends if the block is usable or not. Shouldn't there be a check to see if the address actually targets the RAM?
On 05/16/2017 02:28 AM, Thomas Huth wrote: > +void HELPER(testblock)(CPUS390XState *env, uint64_t addr) > +{ > + CPUState *cs = CPU(s390_env_get_cpu(env)); > + int i; > + > + addr = get_address(env, 0, 0, addr) & ~0xfffULL; > + for (i = 0; i < TARGET_PAGE_SIZE; i += 8) { > + stq_phys(cs->as, addr + i, 0); > + } > + env->cc_op = 0; > +} This needs several changes: check that the physical page does indeed exist, "low address protection", return the cc code. > +DEF_HELPER_2(testblock, void, env, i64) With cc returned, this becomes DEF_HELPER_FLAGS_2(testblock, TCG_CALL_NO_RWG, i32, env, i64) r~
On 16.05.2017 15:42, Aurelien Jarno wrote: > On 2017-05-16 11:28, Thomas Huth wrote: >> TEST BLOCK was likely once used to execute basic memory >> tests, but nowadays it's just a (slow) way to clear a page. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> target/s390x/helper.h | 1 + >> target/s390x/insn-data.def | 2 ++ >> target/s390x/mem_helper.c | 12 ++++++++++++ >> target/s390x/translate.c | 10 ++++++++++ >> 4 files changed, 25 insertions(+) >> >> diff --git a/target/s390x/helper.h b/target/s390x/helper.h >> index 9102071..a5a3705 100644 >> --- a/target/s390x/helper.h >> +++ b/target/s390x/helper.h >> @@ -99,6 +99,7 @@ DEF_HELPER_FLAGS_4(lctl, TCG_CALL_NO_WG, void, env, i32, i64, i32) >> DEF_HELPER_FLAGS_4(lctlg, TCG_CALL_NO_WG, void, env, i32, i64, i32) >> DEF_HELPER_FLAGS_4(stctl, TCG_CALL_NO_WG, void, env, i32, i64, i32) >> DEF_HELPER_FLAGS_4(stctg, TCG_CALL_NO_WG, void, env, i32, i64, i32) >> +DEF_HELPER_2(testblock, void, env, i64) >> DEF_HELPER_FLAGS_2(tprot, TCG_CALL_NO_RWG, i32, i64, i64) >> DEF_HELPER_FLAGS_2(iske, TCG_CALL_NO_RWG_SE, i64, env, i64) >> DEF_HELPER_FLAGS_3(sske, TCG_CALL_NO_RWG, void, env, i64, i64) >> diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def >> index 075ff59..a6aba54 100644 >> --- a/target/s390x/insn-data.def >> +++ b/target/s390x/insn-data.def >> @@ -912,6 +912,8 @@ >> /* STORE USING REAL ADDRESS */ >> C(0xb246, STURA, RRE, Z, r1_o, r2_o, 0, 0, stura, 0) >> C(0xb925, STURG, RRE, Z, r1_o, r2_o, 0, 0, sturg, 0) >> +/* TEST BLOCK */ >> + C(0xb22c, TB, RRE, Z, 0, r2_o, 0, 0, testblock, 0) >> /* TEST PROTECTION */ >> C(0xe501, TPROT, SSE, Z, la1, a2, 0, 0, tprot, 0) >> >> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c >> index 675aba2..0349c10 100644 >> --- a/target/s390x/mem_helper.c >> +++ b/target/s390x/mem_helper.c >> @@ -933,6 +933,18 @@ void HELPER(stctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) >> } >> } >> >> +void HELPER(testblock)(CPUS390XState *env, uint64_t addr) >> +{ >> + CPUState *cs = CPU(s390_env_get_cpu(env)); >> + int i; >> + >> + addr = get_address(env, 0, 0, addr) & ~0xfffULL; > > I guess you want to use ~TARGET_PAGE_MASK here. Yes, that's better, thanks! >> + for (i = 0; i < TARGET_PAGE_SIZE; i += 8) { >> + stq_phys(cs->as, addr + i, 0); >> + } >> + env->cc_op = 0; >> +} >> + > > From what I understand the resulting condition code should depends if > the block is usable or not. Shouldn't there be a check to see if the > address actually targets the RAM? I just tried it under z/VM and KVM, and in case you specify an address that does not point to valid RAM, you get an addressing exception instead. So the CC=1 case was rather meant for memory blocks of valid RAM which contain uncorrectable bit flip errors or something like that. That does not make much sense for emulation, so CC=0 is the only useful code that we can return here. But of course I've also got to add a check for valid RAM and return an addressing exception here now, too... Thomas
On 16.05.2017 21:06, Richard Henderson wrote: > On 05/16/2017 02:28 AM, Thomas Huth wrote: >> +void HELPER(testblock)(CPUS390XState *env, uint64_t addr) >> +{ >> + CPUState *cs = CPU(s390_env_get_cpu(env)); >> + int i; >> + >> + addr = get_address(env, 0, 0, addr) & ~0xfffULL; >> + for (i = 0; i < TARGET_PAGE_SIZE; i += 8) { >> + stq_phys(cs->as, addr + i, 0); >> + } >> + env->cc_op = 0; >> +} > > This needs several changes: check that the physical page does indeed > exist, "low address protection", return the cc code. Ok, ... but if we care about "low address protection", shouldn't we also add that to the other CPU instructions, too? (As far as I can see, it is not supported by the TCG code at all yet) >> +DEF_HELPER_2(testblock, void, env, i64) > > With cc returned, this becomes > > DEF_HELPER_FLAGS_2(testblock, TCG_CALL_NO_RWG, i32, env, i64) Ok, thanks ... I'm still learning the details of writing TCG code ... ;-) Thomas
On 17.05.2017 18:05, Thomas Huth wrote: > On 16.05.2017 21:06, Richard Henderson wrote: >> On 05/16/2017 02:28 AM, Thomas Huth wrote: >>> +void HELPER(testblock)(CPUS390XState *env, uint64_t addr) >>> +{ >>> + CPUState *cs = CPU(s390_env_get_cpu(env)); >>> + int i; >>> + >>> + addr = get_address(env, 0, 0, addr) & ~0xfffULL; >>> + for (i = 0; i < TARGET_PAGE_SIZE; i += 8) { >>> + stq_phys(cs->as, addr + i, 0); >>> + } >>> + env->cc_op = 0; >>> +} >> >> This needs several changes: check that the physical page does indeed >> exist, "low address protection", return the cc code. > > Ok, ... but if we care about "low address protection", shouldn't we also > add that to the other CPU instructions, too? (As far as I can see, it is > not supported by the TCG code at all yet) The same should be true for storage key checks, right? I think there are a lot of such checks missing.
diff --git a/target/s390x/helper.h b/target/s390x/helper.h index 9102071..a5a3705 100644 --- a/target/s390x/helper.h +++ b/target/s390x/helper.h @@ -99,6 +99,7 @@ DEF_HELPER_FLAGS_4(lctl, TCG_CALL_NO_WG, void, env, i32, i64, i32) DEF_HELPER_FLAGS_4(lctlg, TCG_CALL_NO_WG, void, env, i32, i64, i32) DEF_HELPER_FLAGS_4(stctl, TCG_CALL_NO_WG, void, env, i32, i64, i32) DEF_HELPER_FLAGS_4(stctg, TCG_CALL_NO_WG, void, env, i32, i64, i32) +DEF_HELPER_2(testblock, void, env, i64) DEF_HELPER_FLAGS_2(tprot, TCG_CALL_NO_RWG, i32, i64, i64) DEF_HELPER_FLAGS_2(iske, TCG_CALL_NO_RWG_SE, i64, env, i64) DEF_HELPER_FLAGS_3(sske, TCG_CALL_NO_RWG, void, env, i64, i64) diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def index 075ff59..a6aba54 100644 --- a/target/s390x/insn-data.def +++ b/target/s390x/insn-data.def @@ -912,6 +912,8 @@ /* STORE USING REAL ADDRESS */ C(0xb246, STURA, RRE, Z, r1_o, r2_o, 0, 0, stura, 0) C(0xb925, STURG, RRE, Z, r1_o, r2_o, 0, 0, sturg, 0) +/* TEST BLOCK */ + C(0xb22c, TB, RRE, Z, 0, r2_o, 0, 0, testblock, 0) /* TEST PROTECTION */ C(0xe501, TPROT, SSE, Z, la1, a2, 0, 0, tprot, 0) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 675aba2..0349c10 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -933,6 +933,18 @@ void HELPER(stctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3) } } +void HELPER(testblock)(CPUS390XState *env, uint64_t addr) +{ + CPUState *cs = CPU(s390_env_get_cpu(env)); + int i; + + addr = get_address(env, 0, 0, addr) & ~0xfffULL; + for (i = 0; i < TARGET_PAGE_SIZE; i += 8) { + stq_phys(cs->as, addr + i, 0); + } + env->cc_op = 0; +} + uint32_t HELPER(tprot)(uint64_t a1, uint64_t a2) { /* XXX implement */ diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 01c6217..f48c899 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -4016,6 +4016,15 @@ static ExitStatus op_tcxb(DisasContext *s, DisasOps *o) } #ifndef CONFIG_USER_ONLY + +static ExitStatus op_testblock(DisasContext *s, DisasOps *o) +{ + check_privileged(s); + gen_helper_testblock(cpu_env, o->in2); + set_cc_static(s); + return NO_EXIT; +} + static ExitStatus op_tprot(DisasContext *s, DisasOps *o) { potential_page_fault(s); @@ -4023,6 +4032,7 @@ static ExitStatus op_tprot(DisasContext *s, DisasOps *o) set_cc_static(s); return NO_EXIT; } + #endif static ExitStatus op_tr(DisasContext *s, DisasOps *o)
TEST BLOCK was likely once used to execute basic memory tests, but nowadays it's just a (slow) way to clear a page. Signed-off-by: Thomas Huth <thuth@redhat.com> --- target/s390x/helper.h | 1 + target/s390x/insn-data.def | 2 ++ target/s390x/mem_helper.c | 12 ++++++++++++ target/s390x/translate.c | 10 ++++++++++ 4 files changed, 25 insertions(+)