1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH v1] target/s390x: Add support for the TEST BLOCK instruction

Thomas Huth posted 1 patch 6 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1494926884-10118-1-git-send-email-thuth@redhat.com
Test checkpatch passed
Test docker passed
Test s390x passed
There is a newer version of this series
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(+)
[Qemu-devel] [PATCH v1] target/s390x: Add support for the TEST BLOCK instruction
Posted by Thomas Huth 6 years, 11 months ago 
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
Re: [Qemu-devel] [PATCH v1] target/s390x: Add support for the TEST BLOCK instruction
Posted by Aurelien Jarno 6 years, 11 months ago 
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
Re: [Qemu-devel] [PATCH v1] target/s390x: Add support for the TEST BLOCK instruction
Posted by Thomas Huth 6 years, 11 months ago 
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
Re: [Qemu-devel] [PATCH v1] target/s390x: Add support for the TEST BLOCK instruction
Posted by Richard Henderson 6 years, 11 months ago 
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~
Re: [Qemu-devel] [PATCH v1] target/s390x: Add support for the TEST BLOCK instruction
Posted by Thomas Huth 6 years, 11 months ago 
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
Re: [Qemu-devel] [PATCH v1] target/s390x: Add support for the TEST BLOCK instruction
Posted by David Hildenbrand 6 years, 11 months ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.