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

Thomas Huth posted 1 patch 6 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1495107549-6097-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/cpu.h         |  1 +
target/s390x/helper.h      |  1 +
target/s390x/insn-data.def |  2 ++
target/s390x/mem_helper.c  | 28 ++++++++++++++++++++++++++++
target/s390x/mmu_helper.c  |  2 +-
target/s390x/translate.c   | 10 ++++++++++
6 files changed, 43 insertions(+), 1 deletion(-)
[Qemu-devel] [PATCH v2] target/s390x: Add support for the TEST BLOCK instruction
Posted by Thomas Huth 6 years, 10 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>
---
 v2:
 - Use DEF_HELPER_FLAGS_2 instead for DEF_HELPER_2 for returning CC value
 - Convert real to absolute address
 - Added a check for valid RAM page
 - Added low-address protection check

 target/s390x/cpu.h         |  1 +
 target/s390x/helper.h      |  1 +
 target/s390x/insn-data.def |  2 ++
 target/s390x/mem_helper.c  | 28 ++++++++++++++++++++++++++++
 target/s390x/mmu_helper.c  |  2 +-
 target/s390x/translate.c   | 10 ++++++++++
 6 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 240b8a5..4f38ba0 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -1082,6 +1082,7 @@ struct sysib_322 {
 #define SIGP_ORDER_MASK 0x000000ff
 
 void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr);
+target_ulong mmu_real2abs(CPUS390XState *env, target_ulong raddr);
 int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
                   target_ulong *raddr, int *flags, bool exc);
 int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 0b70770..1fae191 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -102,6 +102,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_FLAGS_2(testblock, TCG_CALL_NO_WG, i32, 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 55a7c52..cac0f51 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -918,6 +918,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 f6e5bce..de0ecd4 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -20,6 +20,7 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
+#include "exec/address-spaces.h"
 #include "exec/helper-proto.h"
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
@@ -973,6 +974,33 @@ void HELPER(stctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
     }
 }
 
+uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr)
+{
+    CPUState *cs = CPU(s390_env_get_cpu(env));
+    uint64_t abs_addr;
+    int i;
+
+    real_addr = fix_address(env, real_addr);
+    abs_addr = mmu_real2abs(env, real_addr) & TARGET_PAGE_MASK;
+    if (!address_space_access_valid(&address_space_memory, abs_addr,
+                                    TARGET_PAGE_SIZE, true)) {
+        program_interrupt(env, PGM_ADDRESSING, 4);
+        return 1;
+    }
+
+    /* Check low-address protection */
+    if ((env->cregs[0] & CR0_LOWPROT) != 0 && real_addr < 0x2000) {
+        program_interrupt(env, PGM_PROTECTION, 4);
+        return 1;
+    }
+
+    for (i = 0; i < TARGET_PAGE_SIZE; i += 8) {
+        stq_phys(cs->as, abs_addr + i, 0);
+    }
+
+    return 0;
+}
+
 uint32_t HELPER(tprot)(uint64_t a1, uint64_t a2)
 {
     /* XXX implement */
diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index b11a027..31eb9ef 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -108,7 +108,7 @@ static void trigger_page_fault(CPUS390XState *env, target_ulong vaddr,
  * Translate real address to absolute (= physical)
  * address by taking care of the prefix mapping.
  */
-static target_ulong mmu_real2abs(CPUS390XState *env, target_ulong raddr)
+target_ulong mmu_real2abs(CPUS390XState *env, target_ulong raddr)
 {
     if (raddr < 0x2000) {
         return raddr + env->psa;    /* Map the lowcore. */
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 4c48c59..61aa2c9 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -4057,6 +4057,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(cc_op, cpu_env, o->in2);
+    set_cc_static(s);
+    return NO_EXIT;
+}
+
 static ExitStatus op_tprot(DisasContext *s, DisasOps *o)
 {
     potential_page_fault(s);
@@ -4064,6 +4073,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 v2] target/s390x: Add support for the TEST BLOCK instruction
Posted by David Hildenbrand 6 years, 10 months ago
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index f6e5bce..de0ecd4 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -20,6 +20,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "cpu.h"
> +#include "exec/address-spaces.h"
>  #include "exec/helper-proto.h"
>  #include "exec/exec-all.h"
>  #include "exec/cpu_ldst.h"
> @@ -973,6 +974,33 @@ void HELPER(stctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
>      }
>  }
>  
> +uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr)
> +{
> +    CPUState *cs = CPU(s390_env_get_cpu(env));
> +    uint64_t abs_addr;
> +    int i;
> +

It is somewhat strange that we set a condition code in case of a program
interrupt (I assume that's the magic of the return value?). But maybe
setting the CC on program interrupts is perfectly valid.

Reading the PoP, I think it is supposed to be like this:

- Memory not addressable? -> PGM_ADDRESSING
- Memory protected? -> PGM_PROTECTION
- Memory not usable ("invalid checking-block-code", using it would
  result in a machine check) -> CC=1
- Memory usable and cleared -> CC=0

So in essence, I think we should only set the CC if successful, as we
are not simulating ram failure. But maybe that's how all these handlers
work, and we can't hinder it from setting the CC.

> +    real_addr = fix_address(env, real_addr);

Could it be that fix_address() misses handling for 24 bit mode? (no idea
if that is really relevant, just wondering).

> +    abs_addr = mmu_real2abs(env, real_addr) & TARGET_PAGE_MASK;
> +    if (!address_space_access_valid(&address_space_memory, abs_addr,
> +                                    TARGET_PAGE_SIZE, true)) {
> +        program_interrupt(env, PGM_ADDRESSING, 4);
> +        return 1;
> +    }
> +
> +    /* Check low-address protection */
> +    if ((env->cregs[0] & CR0_LOWPROT) != 0 && real_addr < 0x2000) {

I would drop the != 0.


> +        program_interrupt(env, PGM_PROTECTION, 4);
> +        return 1;
> +    }
> +
> +    for (i = 0; i < TARGET_PAGE_SIZE; i += 8) {
> +        stq_phys(cs->as, abs_addr + i, 0);
> +    }
> +
> +    return 0;
> +}

Looks good to me!

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v2] target/s390x: Add support for the TEST BLOCK instruction
Posted by Thomas Huth 6 years, 10 months ago
On 18.05.2017 14:23, David Hildenbrand wrote:
> 
>> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
>> index f6e5bce..de0ecd4 100644
>> --- a/target/s390x/mem_helper.c
>> +++ b/target/s390x/mem_helper.c
>> @@ -20,6 +20,7 @@
>>  
>>  #include "qemu/osdep.h"
>>  #include "cpu.h"
>> +#include "exec/address-spaces.h"
>>  #include "exec/helper-proto.h"
>>  #include "exec/exec-all.h"
>>  #include "exec/cpu_ldst.h"
>> @@ -973,6 +974,33 @@ void HELPER(stctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
>>      }
>>  }
>>  
>> +uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr)
>> +{
>> +    CPUState *cs = CPU(s390_env_get_cpu(env));
>> +    uint64_t abs_addr;
>> +    int i;
>> +
> 
> It is somewhat strange that we set a condition code in case of a program
> interrupt (I assume that's the magic of the return value?). But maybe
> setting the CC on program interrupts is perfectly valid.

According to the PoP:

"[...] The operation is ter-
minated on addressing and protection exceptions. If
termination occurs, the condition code and the con-
tents of bit positions 32-63 of general register 0 are
unpredictable in the 24-bit or 31-bit addressing
mode, or the condition code and bits 0-63 of the reg-
ister are unpredictable in the 64-bit addressing mode."

So setting CC=1 seems a valid behavior here ;-)

>> +    real_addr = fix_address(env, real_addr);
> 
> Could it be that fix_address() misses handling for 24 bit mode? (no idea
> if that is really relevant, just wondering).

Yes, 24-bit mode is not emulated by QEMU at all, as far as I know... but
that's another story.

>> +    abs_addr = mmu_real2abs(env, real_addr) & TARGET_PAGE_MASK;
>> +    if (!address_space_access_valid(&address_space_memory, abs_addr,
>> +                                    TARGET_PAGE_SIZE, true)) {
>> +        program_interrupt(env, PGM_ADDRESSING, 4);
>> +        return 1;
>> +    }
>> +
>> +    /* Check low-address protection */
>> +    if ((env->cregs[0] & CR0_LOWPROT) != 0 && real_addr < 0x2000) {
> 
> I would drop the != 0.

Ok, I can do that in case I have to respin the patch.

>> +        program_interrupt(env, PGM_PROTECTION, 4);
>> +        return 1;
>> +    }
>> +
>> +    for (i = 0; i < TARGET_PAGE_SIZE; i += 8) {
>> +        stq_phys(cs->as, abs_addr + i, 0);
>> +    }
>> +
>> +    return 0;
>> +}
> 
> Looks good to me!

Thanks!

 Thomas

Re: [Qemu-devel] [PATCH v2] target/s390x: Add support for the TEST BLOCK instruction
Posted by David Hildenbrand 6 years, 10 months ago
On 18.05.2017 14:59, Thomas Huth wrote:
> On 18.05.2017 14:23, David Hildenbrand wrote:
>>
>>> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
>>> index f6e5bce..de0ecd4 100644
>>> --- a/target/s390x/mem_helper.c
>>> +++ b/target/s390x/mem_helper.c
>>> @@ -20,6 +20,7 @@
>>>  
>>>  #include "qemu/osdep.h"
>>>  #include "cpu.h"
>>> +#include "exec/address-spaces.h"
>>>  #include "exec/helper-proto.h"
>>>  #include "exec/exec-all.h"
>>>  #include "exec/cpu_ldst.h"
>>> @@ -973,6 +974,33 @@ void HELPER(stctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
>>>      }
>>>  }
>>>  
>>> +uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr)
>>> +{
>>> +    CPUState *cs = CPU(s390_env_get_cpu(env));
>>> +    uint64_t abs_addr;
>>> +    int i;
>>> +
>>
>> It is somewhat strange that we set a condition code in case of a program
>> interrupt (I assume that's the magic of the return value?). But maybe
>> setting the CC on program interrupts is perfectly valid.
> 
> According to the PoP:
> 
> "[...] The operation is ter-
> minated on addressing and protection exceptions. If
> termination occurs, the condition code and the con-
> tents of bit positions 32-63 of general register 0 are
> unpredictable in the 24-bit or 31-bit addressing
> mode, or the condition code and bits 0-63 of the reg-
> ister are unpredictable in the 64-bit addressing mode."
> 
> So setting CC=1 seems a valid behavior here ;-)

So

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v2] target/s390x: Add support for the TEST BLOCK instruction
Posted by Aurelien Jarno 6 years, 10 months ago
On 2017-05-18 14:59, Thomas Huth wrote:
> On 18.05.2017 14:23, David Hildenbrand wrote:
> > 
> >> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> >> index f6e5bce..de0ecd4 100644
> >> --- a/target/s390x/mem_helper.c
> >> +++ b/target/s390x/mem_helper.c
> >> @@ -20,6 +20,7 @@
> >>  
> >>  #include "qemu/osdep.h"
> >>  #include "cpu.h"
> >> +#include "exec/address-spaces.h"
> >>  #include "exec/helper-proto.h"
> >>  #include "exec/exec-all.h"
> >>  #include "exec/cpu_ldst.h"
> >> @@ -973,6 +974,33 @@ void HELPER(stctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
> >>      }
> >>  }
> >>  
> >> +uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr)
> >> +{
> >> +    CPUState *cs = CPU(s390_env_get_cpu(env));
> >> +    uint64_t abs_addr;
> >> +    int i;
> >> +
> > 
> > It is somewhat strange that we set a condition code in case of a program
> > interrupt (I assume that's the magic of the return value?). But maybe
> > setting the CC on program interrupts is perfectly valid.
> 
> According to the PoP:
> 
> "[...] The operation is ter-
> minated on addressing and protection exceptions. If
> termination occurs, the condition code and the con-
> tents of bit positions 32-63 of general register 0 are
> unpredictable in the 24-bit or 31-bit addressing
> mode, or the condition code and bits 0-63 of the reg-
> ister are unpredictable in the 64-bit addressing mode."
> 
> So setting CC=1 seems a valid behavior here ;-)

Actually program_interrupt will never return, so CC is left unchanged in
case of an exception. It's also matches the unpredictable value
described in the PoP ;-).

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH v2] target/s390x: Add support for the TEST BLOCK instruction
Posted by Thomas Huth 6 years, 10 months ago
On 18.05.2017 15:20, Aurelien Jarno wrote:
> On 2017-05-18 14:59, Thomas Huth wrote:
>> On 18.05.2017 14:23, David Hildenbrand wrote:
>>>
>>>> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
>>>> index f6e5bce..de0ecd4 100644
>>>> --- a/target/s390x/mem_helper.c
>>>> +++ b/target/s390x/mem_helper.c
>>>> @@ -20,6 +20,7 @@
>>>>  
>>>>  #include "qemu/osdep.h"
>>>>  #include "cpu.h"
>>>> +#include "exec/address-spaces.h"
>>>>  #include "exec/helper-proto.h"
>>>>  #include "exec/exec-all.h"
>>>>  #include "exec/cpu_ldst.h"
>>>> @@ -973,6 +974,33 @@ void HELPER(stctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
>>>>      }
>>>>  }
>>>>  
>>>> +uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr)
>>>> +{
>>>> +    CPUState *cs = CPU(s390_env_get_cpu(env));
>>>> +    uint64_t abs_addr;
>>>> +    int i;
>>>> +
>>>
>>> It is somewhat strange that we set a condition code in case of a program
>>> interrupt (I assume that's the magic of the return value?). But maybe
>>> setting the CC on program interrupts is perfectly valid.
>>
>> According to the PoP:
>>
>> "[...] The operation is ter-
>> minated on addressing and protection exceptions. If
>> termination occurs, the condition code and the con-
>> tents of bit positions 32-63 of general register 0 are
>> unpredictable in the 24-bit or 31-bit addressing
>> mode, or the condition code and bits 0-63 of the reg-
>> ister are unpredictable in the 64-bit addressing mode."
>>
>> So setting CC=1 seems a valid behavior here ;-)
> 
> Actually program_interrupt will never return, so CC is left unchanged in
> case of an exception. It's also matches the unpredictable value
> described in the PoP ;-).

Ah, right, program_interrupt() calls cpu_loop_exit() which in turn does
the longjump ... makes more sense to me now, thanks for the hint!

 Thomas

Re: [Qemu-devel] [PATCH v2] target/s390x: Add support for the TEST BLOCK instruction
Posted by Aurelien Jarno 6 years, 10 months ago
On 2017-05-18 13:39, 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>
> ---
>  v2:
>  - Use DEF_HELPER_FLAGS_2 instead for DEF_HELPER_2 for returning CC value
>  - Convert real to absolute address
>  - Added a check for valid RAM page
>  - Added low-address protection check
> 
>  target/s390x/cpu.h         |  1 +
>  target/s390x/helper.h      |  1 +
>  target/s390x/insn-data.def |  2 ++
>  target/s390x/mem_helper.c  | 28 ++++++++++++++++++++++++++++
>  target/s390x/mmu_helper.c  |  2 +-
>  target/s390x/translate.c   | 10 ++++++++++
>  6 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 240b8a5..4f38ba0 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -1082,6 +1082,7 @@ struct sysib_322 {
>  #define SIGP_ORDER_MASK 0x000000ff
>  
>  void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr);
> +target_ulong mmu_real2abs(CPUS390XState *env, target_ulong raddr);
>  int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
>                    target_ulong *raddr, int *flags, bool exc);
>  int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
> index 0b70770..1fae191 100644
> --- a/target/s390x/helper.h
> +++ b/target/s390x/helper.h
> @@ -102,6 +102,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_FLAGS_2(testblock, TCG_CALL_NO_WG, i32, env, i64)

As the helper does not read any values from the global, you can even use
TCG_CALL_NO_RWG.

>  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 55a7c52..cac0f51 100644
> --- a/target/s390x/insn-data.def
> +++ b/target/s390x/insn-data.def
> @@ -918,6 +918,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 f6e5bce..de0ecd4 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -20,6 +20,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "cpu.h"
> +#include "exec/address-spaces.h"
>  #include "exec/helper-proto.h"
>  #include "exec/exec-all.h"
>  #include "exec/cpu_ldst.h"
> @@ -973,6 +974,33 @@ void HELPER(stctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
>      }
>  }
>  
> +uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr)
> +{
> +    CPUState *cs = CPU(s390_env_get_cpu(env));
> +    uint64_t abs_addr;
> +    int i;
> +
> +    real_addr = fix_address(env, real_addr);
> +    abs_addr = mmu_real2abs(env, real_addr) & TARGET_PAGE_MASK;
> +    if (!address_space_access_valid(&address_space_memory, abs_addr,
> +                                    TARGET_PAGE_SIZE, true)) {
> +        program_interrupt(env, PGM_ADDRESSING, 4);
> +        return 1;
> +    }
> +
> +    /* Check low-address protection */
> +    if ((env->cregs[0] & CR0_LOWPROT) != 0 && real_addr < 0x2000) {
> +        program_interrupt(env, PGM_PROTECTION, 4);
> +        return 1;
> +    }
> +
> +    for (i = 0; i < TARGET_PAGE_SIZE; i += 8) {
> +        stq_phys(cs->as, abs_addr + i, 0);
> +    }
> +
> +    return 0;
> +}
> +
>  uint32_t HELPER(tprot)(uint64_t a1, uint64_t a2)
>  {
>      /* XXX implement */
> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index b11a027..31eb9ef 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -108,7 +108,7 @@ static void trigger_page_fault(CPUS390XState *env, target_ulong vaddr,
>   * Translate real address to absolute (= physical)
>   * address by taking care of the prefix mapping.
>   */
> -static target_ulong mmu_real2abs(CPUS390XState *env, target_ulong raddr)
> +target_ulong mmu_real2abs(CPUS390XState *env, target_ulong raddr)
>  {
>      if (raddr < 0x2000) {
>          return raddr + env->psa;    /* Map the lowcore. */
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index 4c48c59..61aa2c9 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -4057,6 +4057,15 @@ static ExitStatus op_tcxb(DisasContext *s, DisasOps *o)
>  }
>  
>  #ifndef CONFIG_USER_ONLY
> +
> +static ExitStatus op_testblock(DisasContext *s, DisasOps *o)
> +{
> +    check_privileged(s);

You should also call potential_page_fault as the helper can trigger an
exception.

> +    gen_helper_testblock(cc_op, cpu_env, o->in2);
> +    set_cc_static(s);
> +    return NO_EXIT;
> +}
> +
>  static ExitStatus op_tprot(DisasContext *s, DisasOps *o)
>  {
>      potential_page_fault(s);
> @@ -4064,6 +4073,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)

Besides the two minor things above, it looks all good to me. Thanks for
the new version.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH v2] target/s390x: Add support for the TEST BLOCK instruction
Posted by Thomas Huth 6 years, 10 months ago
On 18.05.2017 15:20, Aurelien Jarno wrote:
> On 2017-05-18 13:39, 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>
>> ---
>>  v2:
>>  - Use DEF_HELPER_FLAGS_2 instead for DEF_HELPER_2 for returning CC value
>>  - Convert real to absolute address
>>  - Added a check for valid RAM page
>>  - Added low-address protection check
>>
>>  target/s390x/cpu.h         |  1 +
>>  target/s390x/helper.h      |  1 +
>>  target/s390x/insn-data.def |  2 ++
>>  target/s390x/mem_helper.c  | 28 ++++++++++++++++++++++++++++
>>  target/s390x/mmu_helper.c  |  2 +-
>>  target/s390x/translate.c   | 10 ++++++++++
>>  6 files changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 240b8a5..4f38ba0 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -1082,6 +1082,7 @@ struct sysib_322 {
>>  #define SIGP_ORDER_MASK 0x000000ff
>>  
>>  void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr);
>> +target_ulong mmu_real2abs(CPUS390XState *env, target_ulong raddr);
>>  int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
>>                    target_ulong *raddr, int *flags, bool exc);
>>  int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
>> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
>> index 0b70770..1fae191 100644
>> --- a/target/s390x/helper.h
>> +++ b/target/s390x/helper.h
>> @@ -102,6 +102,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_FLAGS_2(testblock, TCG_CALL_NO_WG, i32, env, i64)
> 
> As the helper does not read any values from the global, you can even use
> TCG_CALL_NO_RWG.

Ok.

>>  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 55a7c52..cac0f51 100644
>> --- a/target/s390x/insn-data.def
>> +++ b/target/s390x/insn-data.def
>> @@ -918,6 +918,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 f6e5bce..de0ecd4 100644
>> --- a/target/s390x/mem_helper.c
>> +++ b/target/s390x/mem_helper.c
>> @@ -20,6 +20,7 @@
>>  
>>  #include "qemu/osdep.h"
>>  #include "cpu.h"
>> +#include "exec/address-spaces.h"
>>  #include "exec/helper-proto.h"
>>  #include "exec/exec-all.h"
>>  #include "exec/cpu_ldst.h"
>> @@ -973,6 +974,33 @@ void HELPER(stctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
>>      }
>>  }
>>  
>> +uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr)
>> +{
>> +    CPUState *cs = CPU(s390_env_get_cpu(env));
>> +    uint64_t abs_addr;
>> +    int i;
>> +
>> +    real_addr = fix_address(env, real_addr);
>> +    abs_addr = mmu_real2abs(env, real_addr) & TARGET_PAGE_MASK;
>> +    if (!address_space_access_valid(&address_space_memory, abs_addr,
>> +                                    TARGET_PAGE_SIZE, true)) {
>> +        program_interrupt(env, PGM_ADDRESSING, 4);
>> +        return 1;
>> +    }
>> +
>> +    /* Check low-address protection */
>> +    if ((env->cregs[0] & CR0_LOWPROT) != 0 && real_addr < 0x2000) {
>> +        program_interrupt(env, PGM_PROTECTION, 4);
>> +        return 1;
>> +    }
>> +
>> +    for (i = 0; i < TARGET_PAGE_SIZE; i += 8) {
>> +        stq_phys(cs->as, abs_addr + i, 0);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  uint32_t HELPER(tprot)(uint64_t a1, uint64_t a2)
>>  {
>>      /* XXX implement */
>> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
>> index b11a027..31eb9ef 100644
>> --- a/target/s390x/mmu_helper.c
>> +++ b/target/s390x/mmu_helper.c
>> @@ -108,7 +108,7 @@ static void trigger_page_fault(CPUS390XState *env, target_ulong vaddr,
>>   * Translate real address to absolute (= physical)
>>   * address by taking care of the prefix mapping.
>>   */
>> -static target_ulong mmu_real2abs(CPUS390XState *env, target_ulong raddr)
>> +target_ulong mmu_real2abs(CPUS390XState *env, target_ulong raddr)
>>  {
>>      if (raddr < 0x2000) {
>>          return raddr + env->psa;    /* Map the lowcore. */
>> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
>> index 4c48c59..61aa2c9 100644
>> --- a/target/s390x/translate.c
>> +++ b/target/s390x/translate.c
>> @@ -4057,6 +4057,15 @@ static ExitStatus op_tcxb(DisasContext *s, DisasOps *o)
>>  }
>>  
>>  #ifndef CONFIG_USER_ONLY
>> +
>> +static ExitStatus op_testblock(DisasContext *s, DisasOps *o)
>> +{
>> +    check_privileged(s);
> 
> You should also call potential_page_fault as the helper can trigger an
> exception.

Right, that makes sense now, too.

I'll send a v3 ...

Thanks!

 Thomas

Re: [Qemu-devel] [PATCH v2] target/s390x: Add support for the TEST BLOCK instruction
Posted by Richard Henderson 6 years, 10 months ago
On 05/18/2017 06:20 AM, Aurelien Jarno wrote:
>> +DEF_HELPER_FLAGS_2(testblock, TCG_CALL_NO_WG, i32, env, i64)
> As the helper does not read any values from the global, you can even use
> TCG_CALL_NO_RWG.
> 

By throwing an exception, we imply a read of all values along the exception path.


r~

Re: [Qemu-devel] [PATCH v2] target/s390x: Add support for the TEST BLOCK instruction
Posted by Aurelien Jarno 6 years, 10 months ago
On 2017-05-18 08:42, Richard Henderson wrote:
> On 05/18/2017 06:20 AM, Aurelien Jarno wrote:
> > > +DEF_HELPER_FLAGS_2(testblock, TCG_CALL_NO_WG, i32, env, i64)
> > As the helper does not read any values from the global, you can even use
> > TCG_CALL_NO_RWG.
> > 
> 
> By throwing an exception, we imply a read of all values along the exception path.
> 

You are indeed correct, sorry about the wrong comment.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net