Re: [Qemu-devel] [PATCH v2 5/7] s390x/tcg: implement SET CLOCK

[David Hildenbrand <david@redhat.com>]

On 21.06.2018 15:14, Thomas Huth wrote:
> On 20.06.2018 12:08, David Hildenbrand wrote:
>> This allows a guest to change its TOD. We already take care of updating
>> all CKC timers from within S390TODClass.
>>
>> Use MO_ALIGN to load the operand manually - this will properly trigger a
>> SPECIFICATION exception.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  target/s390x/helper.h      |  1 +
>>  target/s390x/insn-data.def |  3 +--
>>  target/s390x/misc_helper.c | 19 +++++++++++++++++++
>>  target/s390x/translate.c   |  9 +++++++++
>>  4 files changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
>> index 59cba86a27..97c60ca7bc 100644
>> --- a/target/s390x/helper.h
>> +++ b/target/s390x/helper.h
>> @@ -127,6 +127,7 @@ DEF_HELPER_4(diag, void, env, i32, i32, i32)
>>  DEF_HELPER_3(load_psw, noreturn, env, i64, i64)
>>  DEF_HELPER_FLAGS_2(spx, TCG_CALL_NO_RWG, void, env, i64)
>>  DEF_HELPER_FLAGS_1(stck, TCG_CALL_NO_RWG_SE, i64, env)
>> +DEF_HELPER_FLAGS_2(sck, TCG_CALL_NO_RWG, i32, env, i64)
>>  DEF_HELPER_FLAGS_2(sckc, TCG_CALL_NO_RWG, void, env, i64)
>>  DEF_HELPER_FLAGS_2(sckpf, TCG_CALL_NO_RWG, void, env, i64)
>>  DEF_HELPER_FLAGS_1(stckc, TCG_CALL_NO_RWG, i64, env)
>> diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
>> index 157619403d..5c6f33ed9c 100644
>> --- a/target/s390x/insn-data.def
>> +++ b/target/s390x/insn-data.def
>> @@ -997,8 +997,7 @@
>>  /* SET ADDRESS SPACE CONTROL FAST */
>>      C(0xb279, SACF,    S,     Z,   0, a2, 0, 0, sacf, 0)
>>  /* SET CLOCK */
>> -    /* ??? Not implemented - is it necessary? */
>> -    C(0xb204, SCK,     S,     Z,   0, 0, 0, 0, 0, 0)
>> +    C(0xb204, SCK,     S,     Z,   la2, 0, 0, 0, sck, 0)
>>  /* SET CLOCK COMPARATOR */
>>      C(0xb206, SCKC,    S,     Z,   0, m2_64a, 0, 0, sckc, 0)
>>  /* SET CLOCK PROGRAMMABLE FIELD */
>> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
>> index d5f9f5e1d3..c9712b9476 100644
>> --- a/target/s390x/misc_helper.c
>> +++ b/target/s390x/misc_helper.c
>> @@ -191,6 +191,25 @@ void tcg_s390_tod_updated(CPUState *cs, run_on_cpu_data opaque)
>>      helper_sckc(env, env->ckc);
>>  }
>>  
>> +/* Set Clock */
>> +uint32_t HELPER(sck)(CPUS390XState *env, uint64_t tod_low)
>> +{
>> +    S390TODState *td = s390_get_tod();
>> +    S390TODClass *tdc = S390_TOD_GET_CLASS(td);
>> +    S390TOD tod = {
>> +        .high = 0,
>> +        .low = tod_low,
>> +    };
>> +    Error *err = NULL;
>> +
>> +    qemu_mutex_lock_iothread();
>> +    tdc->set(td, &tod, &err);
>> +    qemu_mutex_unlock_iothread();
>> +    g_assert(!err);
> 
> I know it currently can't happen, but still, I think it would be nicer
> to use CC3 to tell the guest that something went wrong with the clock,
> instead of abort QEMU here.

Hmm, I thing I should either use error_abort here or do what you suggest.

However, CC=3 means "Clock in not-operational state".

And this implies that also STORE CLOCK and friends will have to fail and
that we have to present a machine check. Especially, once we would
implement the TOD-clock steering facility, CC=3 would not apply anymore.

So instead of faking something that is not architecturally correct, I
think we really should just quit QEMU, as we expect this to never fail.


> 
>  Thomas
> 


-- 

Thanks,

David / dhildenb