* [Qemu-devel] [PATCH v1] target/s390x: Add support for the TEST BLOCK instruction
@ 2017-05-16 9:28 Thomas Huth
2017-05-16 13:42 ` Aurelien Jarno
2017-05-16 19:06 ` Richard Henderson
0 siblings, 2 replies; 6+ messages in thread
From: Thomas Huth @ 2017-05-16 9:28 UTC (permalink / raw)
To: qemu-devel, Richard Henderson, Alexander Graf
Cc: David Hildenbrand, mmarek, mbenes
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;
+ 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)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v1] target/s390x: Add support for the TEST BLOCK instruction
2017-05-16 9:28 [Qemu-devel] [PATCH v1] target/s390x: Add support for the TEST BLOCK instruction Thomas Huth
@ 2017-05-16 13:42 ` Aurelien Jarno
2017-05-17 7:18 ` Thomas Huth
2017-05-16 19:06 ` Richard Henderson
1 sibling, 1 reply; 6+ messages in thread
From: Aurelien Jarno @ 2017-05-16 13:42 UTC (permalink / raw)
To: Thomas Huth
Cc: qemu-devel, Richard Henderson, Alexander Graf, mmarek, mbenes,
David Hildenbrand
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?
--
Aurelien Jarno GPG: 4096R/1DDD8C9B
aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v1] target/s390x: Add support for the TEST BLOCK instruction
2017-05-16 13:42 ` Aurelien Jarno
@ 2017-05-17 7:18 ` Thomas Huth
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Huth @ 2017-05-17 7:18 UTC (permalink / raw)
To: Aurelien Jarno
Cc: qemu-devel, Richard Henderson, Alexander Graf, mmarek, mbenes,
David Hildenbrand
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v1] target/s390x: Add support for the TEST BLOCK instruction
2017-05-16 9:28 [Qemu-devel] [PATCH v1] target/s390x: Add support for the TEST BLOCK instruction Thomas Huth
2017-05-16 13:42 ` Aurelien Jarno
@ 2017-05-16 19:06 ` Richard Henderson
2017-05-17 16:05 ` Thomas Huth
1 sibling, 1 reply; 6+ messages in thread
From: Richard Henderson @ 2017-05-16 19:06 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, Alexander Graf; +Cc: David Hildenbrand, mmarek, mbenes
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~
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v1] target/s390x: Add support for the TEST BLOCK instruction
2017-05-16 19:06 ` Richard Henderson
@ 2017-05-17 16:05 ` Thomas Huth
2017-05-17 17:03 ` David Hildenbrand
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Huth @ 2017-05-17 16:05 UTC (permalink / raw)
To: Richard Henderson, qemu-devel, Alexander Graf
Cc: David Hildenbrand, mmarek, mbenes
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v1] target/s390x: Add support for the TEST BLOCK instruction
2017-05-17 16:05 ` Thomas Huth
@ 2017-05-17 17:03 ` David Hildenbrand
0 siblings, 0 replies; 6+ messages in thread
From: David Hildenbrand @ 2017-05-17 17:03 UTC (permalink / raw)
To: Thomas Huth, Richard Henderson, qemu-devel, Alexander Graf; +Cc: mmarek, mbenes
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.
--
Thanks,
David
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-17 17:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-16 9:28 [Qemu-devel] [PATCH v1] target/s390x: Add support for the TEST BLOCK instruction Thomas Huth
2017-05-16 13:42 ` Aurelien Jarno
2017-05-17 7:18 ` Thomas Huth
2017-05-16 19:06 ` Richard Henderson
2017-05-17 16:05 ` Thomas Huth
2017-05-17 17:03 ` David Hildenbrand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).