[Qemu-devel] [PATCH for-4.0 0/2] target/s390x: Implement STCK et al for CONFIG_USER_ONLY

[Qemu-devel] [PATCH for-4.0 0/2] target/s390x: Implement STCK et al for CONFIG_USER_ONLY

[Richard Henderson]

This is a non-privileged instruction that was only implemented
for system mode.  However, the stck instruction is used by glibc,
so this was causing SIGILL for programs run under debian stretch.


r~


Richard Henderson (2):
  target/s390x: Move TOD_UNIX_EPOCH to cpu.h
  target/s390x: Implement STCK et al for CONFIG_USER_ONLY

 include/hw/s390x/tod.h     |  3 ---
 target/s390x/cpu.h         |  3 +++
 target/s390x/helper.h      |  2 +-
 target/s390x/misc_helper.c | 13 ++++++++++++-
 target/s390x/translate.c   |  2 ++
 target/s390x/insn-data.def | 11 ++++++-----
 6 files changed, 24 insertions(+), 10 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH for-4.0 1/2] target/s390x: Move TOD_UNIX_EPOCH to cpu.h

[Richard Henderson]

We will need this from CONFIG_USER_ONLY as well,
which cannot access include/hw/.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/hw/s390x/tod.h | 3 ---
 target/s390x/cpu.h     | 3 +++
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/hw/s390x/tod.h b/include/hw/s390x/tod.h
index 413c0d7c02..6ec4fb426c 100644
--- a/include/hw/s390x/tod.h
+++ b/include/hw/s390x/tod.h
@@ -44,9 +44,6 @@ typedef struct S390TODClass {
     void (*set)(S390TODState *td, const S390TOD *tod, Error **errp);
 } S390TODClass;
 
-/* The value of the TOD clock for 1.1.1970. */
-#define TOD_UNIX_EPOCH 0x7d91048bca000000ULL
-
 /* Converts ns to s390's clock format */
 static inline uint64_t time2tod(uint64_t ns)
 {
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 8c2320e882..d8049d181a 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -797,4 +797,7 @@ void s390_init_sigp(void);
 /* outside of target/s390x/ */
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
 
+/* The value of the TOD clock for 1.1.1970. */
+#define TOD_UNIX_EPOCH 0x7d91048bca000000ULL
+
 #endif
-- 
2.17.2

[Qemu-devel] [PATCH for-4.0 2/2] target/s390x: Implement STCK et al for CONFIG_USER_ONLY

[Richard Henderson]

This is a non-privileged instruction that was only implemented
for system mode.  However, the stck instruction is used by glibc,
so this was causing SIGILL for programs run under debian stretch.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/helper.h      |  2 +-
 target/s390x/misc_helper.c | 13 ++++++++++++-
 target/s390x/translate.c   |  2 ++
 target/s390x/insn-data.def | 11 ++++++-----
 4 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 018e9dd414..6260b50496 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -121,13 +121,13 @@ DEF_HELPER_4(cu41, i32, env, i32, i32, i32)
 DEF_HELPER_4(cu42, i32, env, i32, i32, i32)
 DEF_HELPER_5(msa, i32, env, i32, i32, i32, i32)
 DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env)
+DEF_HELPER_FLAGS_1(stck, TCG_CALL_NO_RWG_SE, i64, env)
 
 #ifndef CONFIG_USER_ONLY
 DEF_HELPER_3(servc, i32, env, i64, i64)
 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)
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 3f91579570..c2940afecb 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -76,8 +76,19 @@ uint64_t HELPER(stpt)(CPUS390XState *env)
 #endif
 }
 
-#ifndef CONFIG_USER_ONLY
+#ifdef CONFIG_USER_ONLY
+/* Store Clock */
+uint64_t HELPER(stck)(CPUS390XState *env)
+{
+    struct timespec ts;
+    uint64_t ms;
 
+    clock_gettime(CLOCK_REALTIME, &ts);
+    ms = (ts.tv_nsec / 1000) + (ts.tv_sec * 100000ull);
+
+    return TOD_UNIX_EPOCH + ms;
+}
+#else
 /* SCLP service call */
 uint32_t HELPER(servc)(CPUS390XState *env, uint64_t r1, uint64_t r2)
 {
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index b5bd56b7ee..8bb9c0529a 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -4010,6 +4010,7 @@ static DisasJumpType op_stap(DisasContext *s, DisasOps *o)
     tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, core_id));
     return DISAS_NEXT;
 }
+#endif
 
 static DisasJumpType op_stck(DisasContext *s, DisasOps *o)
 {
@@ -4046,6 +4047,7 @@ static DisasJumpType op_stcke(DisasContext *s, DisasOps *o)
     return DISAS_NEXT;
 }
 
+#ifndef CONFIG_USER_ONLY
 static DisasJumpType op_sck(DisasContext *s, DisasOps *o)
 {
     tcg_gen_qemu_ld_i64(o->in1, o->addr1, get_mem_index(s), MO_TEQ | MO_ALIGN);
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 54e39df831..922227bada 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -837,6 +837,12 @@
     C(0xe33e, STRV,    RXY_a, Z,   la2, r1_32u, new, m1_32, rev32, 0)
     C(0xe32f, STRVG,   RXY_a, Z,   la2, r1_o, new, m1_64, rev64, 0)
 
+/* STORE CLOCK */
+    C(0xb205, STCK,    S,     Z,   la2, 0, new, m1_64, stck, 0)
+    C(0xb27c, STCKF,   S,     SCF, la2, 0, new, m1_64, stck, 0)
+/* STORE CLOCK EXTENDED */
+    C(0xb278, STCKE,   S,     Z,   0, a2, 0, 0, stcke, 0)
+
 /* STORE FACILITY LIST EXTENDED */
     C(0xb2b0, STFLE,   S,  SFLE,   0, a2, 0, 0, stfle, 0)
 /* STORE FPC */
@@ -1020,11 +1026,6 @@
     F(0x8000, SSM,     S,     Z,   0, m2_8u, 0, 0, ssm, 0, IF_PRIV)
 /* SIGNAL PROCESSOR */
     F(0xae00, SIGP,    RS_a,  Z,   0, a2, 0, 0, sigp, 0, IF_PRIV)
-/* STORE CLOCK */
-    C(0xb205, STCK,    S,     Z,   la2, 0, new, m1_64, stck, 0)
-    C(0xb27c, STCKF,   S,     SCF, la2, 0, new, m1_64, stck, 0)
-/* STORE CLOCK EXTENDED */
-    C(0xb278, STCKE,   S,     Z,   0, a2, 0, 0, stcke, 0)
 /* STORE CLOCK COMPARATOR */
     F(0xb207, STCKC,   S,     Z,   la2, 0, new, m1_64a, stckc, 0, IF_PRIV)
 /* STORE CONTROL */
-- 
2.17.2

Re: [Qemu-devel] [qemu-s390x] [PATCH for-4.0 1/2] target/s390x: Move TOD_UNIX_EPOCH to cpu.h

[Thomas Huth]

On 2018-11-30 20:22, Richard Henderson wrote:
> We will need this from CONFIG_USER_ONLY as well,
> which cannot access include/hw/.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/hw/s390x/tod.h | 3 ---
>  target/s390x/cpu.h     | 3 +++
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/s390x/tod.h b/include/hw/s390x/tod.h
> index 413c0d7c02..6ec4fb426c 100644
> --- a/include/hw/s390x/tod.h
> +++ b/include/hw/s390x/tod.h
> @@ -44,9 +44,6 @@ typedef struct S390TODClass {
>      void (*set)(S390TODState *td, const S390TOD *tod, Error **errp);
>  } S390TODClass;
>  
> -/* The value of the TOD clock for 1.1.1970. */
> -#define TOD_UNIX_EPOCH 0x7d91048bca000000ULL
> -
>  /* Converts ns to s390's clock format */
>  static inline uint64_t time2tod(uint64_t ns)
>  {
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 8c2320e882..d8049d181a 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -797,4 +797,7 @@ void s390_init_sigp(void);
>  /* outside of target/s390x/ */
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
>  
> +/* The value of the TOD clock for 1.1.1970. */
> +#define TOD_UNIX_EPOCH 0x7d91048bca000000ULL
> +
>  #endif
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [qemu-s390x] [PATCH for-4.0 2/2] target/s390x: Implement STCK et al for CONFIG_USER_ONLY

[Thomas Huth]

On 2018-11-30 20:22, Richard Henderson wrote:
> This is a non-privileged instruction that was only implemented
> for system mode.  However, the stck instruction is used by glibc,
> so this was causing SIGILL for programs run under debian stretch.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/s390x/helper.h      |  2 +-
>  target/s390x/misc_helper.c | 13 ++++++++++++-
>  target/s390x/translate.c   |  2 ++
>  target/s390x/insn-data.def | 11 ++++++-----
>  4 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
> index 018e9dd414..6260b50496 100644
> --- a/target/s390x/helper.h
> +++ b/target/s390x/helper.h
> @@ -121,13 +121,13 @@ DEF_HELPER_4(cu41, i32, env, i32, i32, i32)
>  DEF_HELPER_4(cu42, i32, env, i32, i32, i32)
>  DEF_HELPER_5(msa, i32, env, i32, i32, i32, i32)
>  DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env)
> +DEF_HELPER_FLAGS_1(stck, TCG_CALL_NO_RWG_SE, i64, env)
>  
>  #ifndef CONFIG_USER_ONLY
>  DEF_HELPER_3(servc, i32, env, i64, i64)
>  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)
> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> index 3f91579570..c2940afecb 100644
> --- a/target/s390x/misc_helper.c
> +++ b/target/s390x/misc_helper.c
> @@ -76,8 +76,19 @@ uint64_t HELPER(stpt)(CPUS390XState *env)
>  #endif
>  }
>  
> -#ifndef CONFIG_USER_ONLY
> +#ifdef CONFIG_USER_ONLY
> +/* Store Clock */
> +uint64_t HELPER(stck)(CPUS390XState *env)
> +{
> +    struct timespec ts;
> +    uint64_t ms;

May I suggest "us" for microseconds? "ms" makes me thing of
"milliseconds"...

> +    clock_gettime(CLOCK_REALTIME, &ts);
> +    ms = (ts.tv_nsec / 1000) + (ts.tv_sec * 100000ull);
> +
> +    return TOD_UNIX_EPOCH + ms;
> +}

Don't you need to shift the value around a little bit for getting the
right format of the TOD clock? The microseconds are not starting with
the lowest bit, the TOD clock has a higher resolution. See also the
time2tod() macro in include/hw/s390x/tod.h ...

 Thomas

Re: [Qemu-devel] [PATCH for-4.0 2/2] target/s390x: Implement STCK et al for CONFIG_USER_ONLY

[David Hildenbrand]

On 30.11.18 20:22, Richard Henderson wrote:
> This is a non-privileged instruction that was only implemented
> for system mode.  However, the stck instruction is used by glibc,
> so this was causing SIGILL for programs run under debian stretch.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/s390x/helper.h      |  2 +-
>  target/s390x/misc_helper.c | 13 ++++++++++++-
>  target/s390x/translate.c   |  2 ++
>  target/s390x/insn-data.def | 11 ++++++-----
>  4 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
> index 018e9dd414..6260b50496 100644
> --- a/target/s390x/helper.h
> +++ b/target/s390x/helper.h
> @@ -121,13 +121,13 @@ DEF_HELPER_4(cu41, i32, env, i32, i32, i32)
>  DEF_HELPER_4(cu42, i32, env, i32, i32, i32)
>  DEF_HELPER_5(msa, i32, env, i32, i32, i32, i32)
>  DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env)
> +DEF_HELPER_FLAGS_1(stck, TCG_CALL_NO_RWG_SE, i64, env)
>  
>  #ifndef CONFIG_USER_ONLY
>  DEF_HELPER_3(servc, i32, env, i64, i64)
>  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)
> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> index 3f91579570..c2940afecb 100644
> --- a/target/s390x/misc_helper.c
> +++ b/target/s390x/misc_helper.c
> @@ -76,8 +76,19 @@ uint64_t HELPER(stpt)(CPUS390XState *env)
>  #endif
>  }
>  
> -#ifndef CONFIG_USER_ONLY
> +#ifdef CONFIG_USER_ONLY
> +/* Store Clock */
> +uint64_t HELPER(stck)(CPUS390XState *env)
> +{
> +    struct timespec ts;
> +    uint64_t ms;
>  
> +    clock_gettime(CLOCK_REALTIME, &ts);
> +    ms = (ts.tv_nsec / 1000) + (ts.tv_sec * 100000ull);
> +
> +    return TOD_UNIX_EPOCH + ms;

In theory, the TOD can be completely controlled by the operating system
(e.g. set the TOD to X). So for user space, there isn't really any
guarantee about the values returned via stck.

E.g. in the PoP 4-51:

"4. A program using the clock value as a time-of-day
and calendar indication must be consistent with
the programming support under which the pro-
gram is to be executed. [...]"

Of course, in an ideal world, the system TOD value really corresponds to
the reference TOD. So I assume, in theory, we can simply indicate any
monotonic values here. E.g. not using TOD_UNIX_EPOCH and only forwarding
the result from CLOCK_REALTIME converted to appropriate TOD values.

If I am not wrong, under Linux, user space will only use stck for
measurement, and use the vdso for getting the real time.


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH for-4.0 1/2] target/s390x: Move TOD_UNIX_EPOCH to cpu.h

[David Hildenbrand]

On 30.11.18 20:22, Richard Henderson wrote:
> We will need this from CONFIG_USER_ONLY as well,
> which cannot access include/hw/.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/hw/s390x/tod.h | 3 ---
>  target/s390x/cpu.h     | 3 +++
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/s390x/tod.h b/include/hw/s390x/tod.h
> index 413c0d7c02..6ec4fb426c 100644
> --- a/include/hw/s390x/tod.h
> +++ b/include/hw/s390x/tod.h
> @@ -44,9 +44,6 @@ typedef struct S390TODClass {
>      void (*set)(S390TODState *td, const S390TOD *tod, Error **errp);
>  } S390TODClass;
>  
> -/* The value of the TOD clock for 1.1.1970. */
> -#define TOD_UNIX_EPOCH 0x7d91048bca000000ULL
> -
>  /* Converts ns to s390's clock format */
>  static inline uint64_t time2tod(uint64_t ns)
>  {
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 8c2320e882..d8049d181a 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -797,4 +797,7 @@ void s390_init_sigp(void);
>  /* outside of target/s390x/ */
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
>  
> +/* The value of the TOD clock for 1.1.1970. */
> +#define TOD_UNIX_EPOCH 0x7d91048bca000000ULL
> +
>  #endif
> 

As said in response to patch 2, this might not be needed.

If you're going to stick to it

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

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH for-4.0 2/2] target/s390x: Implement STCK et al for CONFIG_USER_ONLY

[Richard Henderson]

On 12/3/18 12:19 AM, Thomas Huth wrote:
> On 2018-11-30 20:22, Richard Henderson wrote:
>> This is a non-privileged instruction that was only implemented
>> for system mode.  However, the stck instruction is used by glibc,
>> so this was causing SIGILL for programs run under debian stretch.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  target/s390x/helper.h      |  2 +-
>>  target/s390x/misc_helper.c | 13 ++++++++++++-
>>  target/s390x/translate.c   |  2 ++
>>  target/s390x/insn-data.def | 11 ++++++-----
>>  4 files changed, 21 insertions(+), 7 deletions(-)
>>
>> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
>> index 018e9dd414..6260b50496 100644
>> --- a/target/s390x/helper.h
>> +++ b/target/s390x/helper.h
>> @@ -121,13 +121,13 @@ DEF_HELPER_4(cu41, i32, env, i32, i32, i32)
>>  DEF_HELPER_4(cu42, i32, env, i32, i32, i32)
>>  DEF_HELPER_5(msa, i32, env, i32, i32, i32, i32)
>>  DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env)
>> +DEF_HELPER_FLAGS_1(stck, TCG_CALL_NO_RWG_SE, i64, env)
>>  
>>  #ifndef CONFIG_USER_ONLY
>>  DEF_HELPER_3(servc, i32, env, i64, i64)
>>  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)
>> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
>> index 3f91579570..c2940afecb 100644
>> --- a/target/s390x/misc_helper.c
>> +++ b/target/s390x/misc_helper.c
>> @@ -76,8 +76,19 @@ uint64_t HELPER(stpt)(CPUS390XState *env)
>>  #endif
>>  }
>>  
>> -#ifndef CONFIG_USER_ONLY
>> +#ifdef CONFIG_USER_ONLY
>> +/* Store Clock */
>> +uint64_t HELPER(stck)(CPUS390XState *env)
>> +{
>> +    struct timespec ts;
>> +    uint64_t ms;
> 
> May I suggest "us" for microseconds? "ms" makes me thing of
> "milliseconds"...

Oops, think-o.

>> +    clock_gettime(CLOCK_REALTIME, &ts);
>> +    ms = (ts.tv_nsec / 1000) + (ts.tv_sec * 100000ull);
>> +
>> +    return TOD_UNIX_EPOCH + ms;
>> +}
> 
> Don't you need to shift the value around a little bit for getting the
> right format of the TOD clock? The microseconds are not starting with
> the lowest bit, the TOD clock has a higher resolution. See also the
> time2tod() macro in include/hw/s390x/tod.h ...

That's for STCKE, which does use this routine and shift the result around.


r~

Re: [Qemu-devel] [PATCH for-4.0 2/2] target/s390x: Implement STCK et al for CONFIG_USER_ONLY

[Richard Henderson]

On 12/3/18 4:21 AM, David Hildenbrand wrote:
> On 30.11.18 20:22, Richard Henderson wrote:
>> This is a non-privileged instruction that was only implemented
>> for system mode.  However, the stck instruction is used by glibc,
>> so this was causing SIGILL for programs run under debian stretch.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  target/s390x/helper.h      |  2 +-
>>  target/s390x/misc_helper.c | 13 ++++++++++++-
>>  target/s390x/translate.c   |  2 ++
>>  target/s390x/insn-data.def | 11 ++++++-----
>>  4 files changed, 21 insertions(+), 7 deletions(-)
>>
>> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
>> index 018e9dd414..6260b50496 100644
>> --- a/target/s390x/helper.h
>> +++ b/target/s390x/helper.h
>> @@ -121,13 +121,13 @@ DEF_HELPER_4(cu41, i32, env, i32, i32, i32)
>>  DEF_HELPER_4(cu42, i32, env, i32, i32, i32)
>>  DEF_HELPER_5(msa, i32, env, i32, i32, i32, i32)
>>  DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env)
>> +DEF_HELPER_FLAGS_1(stck, TCG_CALL_NO_RWG_SE, i64, env)
>>  
>>  #ifndef CONFIG_USER_ONLY
>>  DEF_HELPER_3(servc, i32, env, i64, i64)
>>  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)
>> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
>> index 3f91579570..c2940afecb 100644
>> --- a/target/s390x/misc_helper.c
>> +++ b/target/s390x/misc_helper.c
>> @@ -76,8 +76,19 @@ uint64_t HELPER(stpt)(CPUS390XState *env)
>>  #endif
>>  }
>>  
>> -#ifndef CONFIG_USER_ONLY
>> +#ifdef CONFIG_USER_ONLY
>> +/* Store Clock */
>> +uint64_t HELPER(stck)(CPUS390XState *env)
>> +{
>> +    struct timespec ts;
>> +    uint64_t ms;
>>  
>> +    clock_gettime(CLOCK_REALTIME, &ts);
>> +    ms = (ts.tv_nsec / 1000) + (ts.tv_sec * 100000ull);
>> +
>> +    return TOD_UNIX_EPOCH + ms;
> 
> In theory, the TOD can be completely controlled by the operating system
> (e.g. set the TOD to X). So for user space, there isn't really any
> guarantee about the values returned via stck.
> 
> E.g. in the PoP 4-51:
> 
> "4. A program using the clock value as a time-of-day
> and calendar indication must be consistent with
> the programming support under which the pro-
> gram is to be executed. [...]"

Ok, but then there's the matter of the CC result.  We currently hard-code this
as 0, meaning "clock is set", meaning it does have the real TOD value.

We could set CC as 1, meaning "clock is not set", meaning the value is only
good for relative computation.

Is this perhaps a bug in our system implementation as well?

What CC value is provided to userspace on real hardware?


r~

Re: [Qemu-devel] [qemu-s390x] [PATCH for-4.0 2/2] target/s390x: Implement STCK et al for CONFIG_USER_ONLY

[Thomas Huth]

On 2018-12-03 14:27, Richard Henderson wrote:
> On 12/3/18 12:19 AM, Thomas Huth wrote:
>> On 2018-11-30 20:22, Richard Henderson wrote:
>>> This is a non-privileged instruction that was only implemented
>>> for system mode.  However, the stck instruction is used by glibc,
>>> so this was causing SIGILL for programs run under debian stretch.
>>>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>  target/s390x/helper.h      |  2 +-
>>>  target/s390x/misc_helper.c | 13 ++++++++++++-
>>>  target/s390x/translate.c   |  2 ++
>>>  target/s390x/insn-data.def | 11 ++++++-----
>>>  4 files changed, 21 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
>>> index 018e9dd414..6260b50496 100644
>>> --- a/target/s390x/helper.h
>>> +++ b/target/s390x/helper.h
>>> @@ -121,13 +121,13 @@ DEF_HELPER_4(cu41, i32, env, i32, i32, i32)
>>>  DEF_HELPER_4(cu42, i32, env, i32, i32, i32)
>>>  DEF_HELPER_5(msa, i32, env, i32, i32, i32, i32)
>>>  DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env)
>>> +DEF_HELPER_FLAGS_1(stck, TCG_CALL_NO_RWG_SE, i64, env)
>>>  
>>>  #ifndef CONFIG_USER_ONLY
>>>  DEF_HELPER_3(servc, i32, env, i64, i64)
>>>  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)
>>> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
>>> index 3f91579570..c2940afecb 100644
>>> --- a/target/s390x/misc_helper.c
>>> +++ b/target/s390x/misc_helper.c
>>> @@ -76,8 +76,19 @@ uint64_t HELPER(stpt)(CPUS390XState *env)
>>>  #endif
>>>  }
>>>  
>>> -#ifndef CONFIG_USER_ONLY
>>> +#ifdef CONFIG_USER_ONLY
>>> +/* Store Clock */
>>> +uint64_t HELPER(stck)(CPUS390XState *env)
>>> +{
>>> +    struct timespec ts;
>>> +    uint64_t ms;
>>
>> May I suggest "us" for microseconds? "ms" makes me thing of
>> "milliseconds"...
> 
> Oops, think-o.
> 
>>> +    clock_gettime(CLOCK_REALTIME, &ts);
>>> +    ms = (ts.tv_nsec / 1000) + (ts.tv_sec * 100000ull);
>>> +
>>> +    return TOD_UNIX_EPOCH + ms;
>>> +}
>>
>> Don't you need to shift the value around a little bit for getting the
>> right format of the TOD clock? The microseconds are not starting with
>> the lowest bit, the TOD clock has a higher resolution. See also the
>> time2tod() macro in include/hw/s390x/tod.h ...
> 
> That's for STCKE, which does use this routine and shift the result around.

STCKE certainly has even more bits ... but still, according to the PoP:

"The TOD clock nominally is incremented by adding a
one in bit position 51 every microsecond."

So you've got to shift the microseconds by 12 bits to the left?

... but maybe I'm also just reading the PoP or the source code in a
wrong way again...

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH for-4.0 2/2] target/s390x: Implement STCK et al for CONFIG_USER_ONLY

[Richard Henderson]

On 12/3/18 8:02 AM, Thomas Huth wrote:
> On 2018-12-03 14:27, Richard Henderson wrote:
>> On 12/3/18 12:19 AM, Thomas Huth wrote:
>>> On 2018-11-30 20:22, Richard Henderson wrote:
>>>> This is a non-privileged instruction that was only implemented
>>>> for system mode.  However, the stck instruction is used by glibc,
>>>> so this was causing SIGILL for programs run under debian stretch.
>>>>
>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>>> ---
>>>>  target/s390x/helper.h      |  2 +-
>>>>  target/s390x/misc_helper.c | 13 ++++++++++++-
>>>>  target/s390x/translate.c   |  2 ++
>>>>  target/s390x/insn-data.def | 11 ++++++-----
>>>>  4 files changed, 21 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
>>>> index 018e9dd414..6260b50496 100644
>>>> --- a/target/s390x/helper.h
>>>> +++ b/target/s390x/helper.h
>>>> @@ -121,13 +121,13 @@ DEF_HELPER_4(cu41, i32, env, i32, i32, i32)
>>>>  DEF_HELPER_4(cu42, i32, env, i32, i32, i32)
>>>>  DEF_HELPER_5(msa, i32, env, i32, i32, i32, i32)
>>>>  DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env)
>>>> +DEF_HELPER_FLAGS_1(stck, TCG_CALL_NO_RWG_SE, i64, env)
>>>>  
>>>>  #ifndef CONFIG_USER_ONLY
>>>>  DEF_HELPER_3(servc, i32, env, i64, i64)
>>>>  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)
>>>> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
>>>> index 3f91579570..c2940afecb 100644
>>>> --- a/target/s390x/misc_helper.c
>>>> +++ b/target/s390x/misc_helper.c
>>>> @@ -76,8 +76,19 @@ uint64_t HELPER(stpt)(CPUS390XState *env)
>>>>  #endif
>>>>  }
>>>>  
>>>> -#ifndef CONFIG_USER_ONLY
>>>> +#ifdef CONFIG_USER_ONLY
>>>> +/* Store Clock */
>>>> +uint64_t HELPER(stck)(CPUS390XState *env)
>>>> +{
>>>> +    struct timespec ts;
>>>> +    uint64_t ms;
>>>
>>> May I suggest "us" for microseconds? "ms" makes me thing of
>>> "milliseconds"...
>>
>> Oops, think-o.
>>
>>>> +    clock_gettime(CLOCK_REALTIME, &ts);
>>>> +    ms = (ts.tv_nsec / 1000) + (ts.tv_sec * 100000ull);
>>>> +
>>>> +    return TOD_UNIX_EPOCH + ms;
>>>> +}
>>>
>>> Don't you need to shift the value around a little bit for getting the
>>> right format of the TOD clock? The microseconds are not starting with
>>> the lowest bit, the TOD clock has a higher resolution. See also the
>>> time2tod() macro in include/hw/s390x/tod.h ...
>>
>> That's for STCKE, which does use this routine and shift the result around.
> 
> STCKE certainly has even more bits ... but still, according to the PoP:
> 
> "The TOD clock nominally is incremented by adding a
> one in bit position 51 every microsecond."

I don't see that language within the description of STCK.  Perhaps it's
elsewhere in the PoO.  However you are right because STCK says "bit position 31
of the clock is incremented every 1.048576 seconds", and that amounts to
exactly the same thing.

Probably I should clean up STCKE at the same time, since it only wants a left
shift of 4 bits instead of 12...


r~

Re: [Qemu-devel] [qemu-s390x] [PATCH for-4.0 2/2] target/s390x: Implement STCK et al for CONFIG_USER_ONLY

[Thomas Huth]

On 2018-12-03 15:29, Richard Henderson wrote:
> On 12/3/18 8:02 AM, Thomas Huth wrote:
>> On 2018-12-03 14:27, Richard Henderson wrote:
>>> On 12/3/18 12:19 AM, Thomas Huth wrote:
>>>> On 2018-11-30 20:22, Richard Henderson wrote:
>>>>> This is a non-privileged instruction that was only implemented
>>>>> for system mode.  However, the stck instruction is used by glibc,
>>>>> so this was causing SIGILL for programs run under debian stretch.
>>>>>
>>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>>>> ---
>>>>>  target/s390x/helper.h      |  2 +-
>>>>>  target/s390x/misc_helper.c | 13 ++++++++++++-
>>>>>  target/s390x/translate.c   |  2 ++
>>>>>  target/s390x/insn-data.def | 11 ++++++-----
>>>>>  4 files changed, 21 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
>>>>> index 018e9dd414..6260b50496 100644
>>>>> --- a/target/s390x/helper.h
>>>>> +++ b/target/s390x/helper.h
>>>>> @@ -121,13 +121,13 @@ DEF_HELPER_4(cu41, i32, env, i32, i32, i32)
>>>>>  DEF_HELPER_4(cu42, i32, env, i32, i32, i32)
>>>>>  DEF_HELPER_5(msa, i32, env, i32, i32, i32, i32)
>>>>>  DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env)
>>>>> +DEF_HELPER_FLAGS_1(stck, TCG_CALL_NO_RWG_SE, i64, env)
>>>>>  
>>>>>  #ifndef CONFIG_USER_ONLY
>>>>>  DEF_HELPER_3(servc, i32, env, i64, i64)
>>>>>  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)
>>>>> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
>>>>> index 3f91579570..c2940afecb 100644
>>>>> --- a/target/s390x/misc_helper.c
>>>>> +++ b/target/s390x/misc_helper.c
>>>>> @@ -76,8 +76,19 @@ uint64_t HELPER(stpt)(CPUS390XState *env)
>>>>>  #endif
>>>>>  }
>>>>>  
>>>>> -#ifndef CONFIG_USER_ONLY
>>>>> +#ifdef CONFIG_USER_ONLY
>>>>> +/* Store Clock */
>>>>> +uint64_t HELPER(stck)(CPUS390XState *env)
>>>>> +{
>>>>> +    struct timespec ts;
>>>>> +    uint64_t ms;
>>>>
>>>> May I suggest "us" for microseconds? "ms" makes me thing of
>>>> "milliseconds"...
>>>
>>> Oops, think-o.
>>>
>>>>> +    clock_gettime(CLOCK_REALTIME, &ts);
>>>>> +    ms = (ts.tv_nsec / 1000) + (ts.tv_sec * 100000ull);
>>>>> +
>>>>> +    return TOD_UNIX_EPOCH + ms;
>>>>> +}
>>>>
>>>> Don't you need to shift the value around a little bit for getting the
>>>> right format of the TOD clock? The microseconds are not starting with
>>>> the lowest bit, the TOD clock has a higher resolution. See also the
>>>> time2tod() macro in include/hw/s390x/tod.h ...
>>>
>>> That's for STCKE, which does use this routine and shift the result around.
>>
>> STCKE certainly has even more bits ... but still, according to the PoP:
>>
>> "The TOD clock nominally is incremented by adding a
>> one in bit position 51 every microsecond."
> 
> I don't see that language within the description of STCK.

It's in chapter 4 instead, the sub-section about "Timing".

 Thomas

Re: [Qemu-devel] [PATCH for-4.0 2/2] target/s390x: Implement STCK et al for CONFIG_USER_ONLY

[David Hildenbrand]

On 03.12.18 14:30, Richard Henderson wrote:
> On 12/3/18 4:21 AM, David Hildenbrand wrote:
>> On 30.11.18 20:22, Richard Henderson wrote:
>>> This is a non-privileged instruction that was only implemented
>>> for system mode.  However, the stck instruction is used by glibc,
>>> so this was causing SIGILL for programs run under debian stretch.
>>>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>  target/s390x/helper.h      |  2 +-
>>>  target/s390x/misc_helper.c | 13 ++++++++++++-
>>>  target/s390x/translate.c   |  2 ++
>>>  target/s390x/insn-data.def | 11 ++++++-----
>>>  4 files changed, 21 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
>>> index 018e9dd414..6260b50496 100644
>>> --- a/target/s390x/helper.h
>>> +++ b/target/s390x/helper.h
>>> @@ -121,13 +121,13 @@ DEF_HELPER_4(cu41, i32, env, i32, i32, i32)
>>>  DEF_HELPER_4(cu42, i32, env, i32, i32, i32)
>>>  DEF_HELPER_5(msa, i32, env, i32, i32, i32, i32)
>>>  DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env)
>>> +DEF_HELPER_FLAGS_1(stck, TCG_CALL_NO_RWG_SE, i64, env)
>>>  
>>>  #ifndef CONFIG_USER_ONLY
>>>  DEF_HELPER_3(servc, i32, env, i64, i64)
>>>  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)
>>> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
>>> index 3f91579570..c2940afecb 100644
>>> --- a/target/s390x/misc_helper.c
>>> +++ b/target/s390x/misc_helper.c
>>> @@ -76,8 +76,19 @@ uint64_t HELPER(stpt)(CPUS390XState *env)
>>>  #endif
>>>  }
>>>  
>>> -#ifndef CONFIG_USER_ONLY
>>> +#ifdef CONFIG_USER_ONLY
>>> +/* Store Clock */
>>> +uint64_t HELPER(stck)(CPUS390XState *env)
>>> +{
>>> +    struct timespec ts;
>>> +    uint64_t ms;
>>>  
>>> +    clock_gettime(CLOCK_REALTIME, &ts);
>>> +    ms = (ts.tv_nsec / 1000) + (ts.tv_sec * 100000ull);
>>> +
>>> +    return TOD_UNIX_EPOCH + ms;
>>
>> In theory, the TOD can be completely controlled by the operating system
>> (e.g. set the TOD to X). So for user space, there isn't really any
>> guarantee about the values returned via stck.
>>
>> E.g. in the PoP 4-51:
>>
>> "4. A program using the clock value as a time-of-day
>> and calendar indication must be consistent with
>> the programming support under which the pro-
>> gram is to be executed. [...]"
> 
> Ok, but then there's the matter of the CC result.  We currently hard-code this
> as 0, meaning "clock is set", meaning it does have the real TOD value.

Don't worry about cc/clock state handling. This is handling of old
machine generations and will not happen on real machines nowadays (as
far as I am aware). Clocks are always operating. CC is always 0.

"Not-Set State: When the power for the clock is
turned on, the clock is set to zero, and the clock
enters the not-set state. The clock is incremented
when in the not-set state. When the TOD-clock-steer-
ing facility is installed, the TOD clock is never
reported to be in the not-set state, as the TOD clock
is placed in the set state as part of the initial-
machine-loading (IML) process."

So this is really only handling for operating systems that want to get
the real time from the TOD (because with CC=1, the TOD would not match
an actual date). Not for user space! And not for recent machine generations.

When the OS changes the tod (set clock), the TOD will still be in "set
state". User space has no guarantees here.

> 
> We could set CC as 1, meaning "clock is not set", meaning the value is only
> good for relative computation.
> 
> Is this perhaps a bug in our system implementation as well?

Don't think so, returning always 0 should be fine.

> 
> What CC value is provided to userspace on real hardware?

It will always see CC=0 as far as I know.

E.g. in KVM, after migration the guest TOD might differ to the actual
system TOD. But we will still get CC=0.

CC != 0 is a relict from the past.

I guess the important part here is not to confuse "CC==0" with "the TOD
is synced to time source X". It is just internal state handling for
bringing up the clock source + error handling.

> 
> 
> r~
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH for-4.0 2/2] target/s390x: Implement STCK et al for CONFIG_USER_ONLY

[David Hildenbrand]

On 03.12.18 15:02, Thomas Huth wrote:
> On 2018-12-03 14:27, Richard Henderson wrote:
>> On 12/3/18 12:19 AM, Thomas Huth wrote:
>>> On 2018-11-30 20:22, Richard Henderson wrote:
>>>> This is a non-privileged instruction that was only implemented
>>>> for system mode.  However, the stck instruction is used by glibc,
>>>> so this was causing SIGILL for programs run under debian stretch.
>>>>
>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>>> ---
>>>>  target/s390x/helper.h      |  2 +-
>>>>  target/s390x/misc_helper.c | 13 ++++++++++++-
>>>>  target/s390x/translate.c   |  2 ++
>>>>  target/s390x/insn-data.def | 11 ++++++-----
>>>>  4 files changed, 21 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
>>>> index 018e9dd414..6260b50496 100644
>>>> --- a/target/s390x/helper.h
>>>> +++ b/target/s390x/helper.h
>>>> @@ -121,13 +121,13 @@ DEF_HELPER_4(cu41, i32, env, i32, i32, i32)
>>>>  DEF_HELPER_4(cu42, i32, env, i32, i32, i32)
>>>>  DEF_HELPER_5(msa, i32, env, i32, i32, i32, i32)
>>>>  DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env)
>>>> +DEF_HELPER_FLAGS_1(stck, TCG_CALL_NO_RWG_SE, i64, env)
>>>>  
>>>>  #ifndef CONFIG_USER_ONLY
>>>>  DEF_HELPER_3(servc, i32, env, i64, i64)
>>>>  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)
>>>> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
>>>> index 3f91579570..c2940afecb 100644
>>>> --- a/target/s390x/misc_helper.c
>>>> +++ b/target/s390x/misc_helper.c
>>>> @@ -76,8 +76,19 @@ uint64_t HELPER(stpt)(CPUS390XState *env)
>>>>  #endif
>>>>  }
>>>>  
>>>> -#ifndef CONFIG_USER_ONLY
>>>> +#ifdef CONFIG_USER_ONLY
>>>> +/* Store Clock */
>>>> +uint64_t HELPER(stck)(CPUS390XState *env)
>>>> +{
>>>> +    struct timespec ts;
>>>> +    uint64_t ms;
>>>
>>> May I suggest "us" for microseconds? "ms" makes me thing of
>>> "milliseconds"...
>>
>> Oops, think-o.
>>
>>>> +    clock_gettime(CLOCK_REALTIME, &ts);
>>>> +    ms = (ts.tv_nsec / 1000) + (ts.tv_sec * 100000ull);
>>>> +
>>>> +    return TOD_UNIX_EPOCH + ms;
>>>> +}
>>>
>>> Don't you need to shift the value around a little bit for getting the
>>> right format of the TOD clock? The microseconds are not starting with
>>> the lowest bit, the TOD clock has a higher resolution. See also the
>>> time2tod() macro in include/hw/s390x/tod.h ...

I agree, this needs time2tod(). By definition, the TOD is stepped like
this. STCK/STCKE just provide different means to that value. But the
stepping remains the same.

> 
>  Thomas
> 


-- 

Thanks,

David / dhildenb