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

[Thomas Huth]

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 <address@hidden>
>> ---
>>  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