[PATCH v2 0/3] target/s390x: Fix missing clock-comparator interrupts

[PATCH v2 0/3] target/s390x: Fix missing clock-comparator interrupts

[Ilya Leoshkevich]

v1: https://lore.kernel.org/qemu-devel/20251014160743.398093-1-iii@linux.ibm.com/
v1 -> v2: Add Thomas' R-b.
          Cc: stable (Michael).
          Improve formatting, commit messages, and test (Ilya).

Hi,

While trying to (so far unsuccessfully) reproduce [1], I found two bugs
in the clock comparator handling. This series fixes both and adds a
test.

[1] https://lore.kernel.org/lkml/ab3131a2-c42a-47ff-bf03-e9f68ac053c0@t-8ch.de/

Best regards,
Ilya

Ilya Leoshkevich (3):
  target/s390x: Fix missing interrupts for small CKC values
  target/s390x: Fix missing clock-comparator interrupts after reset
  tests/tcg/s390x: Test SET CLOCK COMPARATOR

 hw/s390x/s390-virtio-ccw.c              |  6 +--
 target/s390x/cpu.c                      |  8 ++++
 target/s390x/tcg/misc_helper.c          | 12 ++++--
 tests/tcg/s390x/Makefile.softmmu-target |  1 +
 tests/tcg/s390x/sckc.S                  | 55 +++++++++++++++++++++++++
 5 files changed, 75 insertions(+), 7 deletions(-)
 create mode 100644 tests/tcg/s390x/sckc.S

-- 
2.51.0

[PATCH v2 1/3] target/s390x: Fix missing interrupts for small CKC values

[Ilya Leoshkevich]

Suppose TOD clock value is 0x1111111111111111 and clock-comparator
value is 0, in which case clock-comparator interruption should occur
immediately.

With the current code, tod2time(env->ckc - td->base.low) ends up being
a very large number, so this interruption never happens.

Fix by firing the timer immediately if env->ckc < td->base.low.

Cc: qemu-stable@nongnu.org
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 target/s390x/tcg/misc_helper.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/target/s390x/tcg/misc_helper.c b/target/s390x/tcg/misc_helper.c
index 6d9d601d29a..215b5b9d933 100644
--- a/target/s390x/tcg/misc_helper.c
+++ b/target/s390x/tcg/misc_helper.c
@@ -199,11 +199,15 @@ static void update_ckc_timer(CPUS390XState *env)
         return;
     }
 
-    /* difference between origins */
-    time = env->ckc - td->base.low;
+    if (env->ckc < td->base.low) {
+        time = 0;
+    } else {
+        /* difference between origins */
+        time = env->ckc - td->base.low;
 
-    /* nanoseconds */
-    time = tod2time(time);
+        /* nanoseconds */
+        time = tod2time(time);
+    }
 
     timer_mod(env->tod_timer, time);
 }
-- 
2.51.0

[PATCH v2 2/3] target/s390x: Fix missing clock-comparator interrupts after reset

[Ilya Leoshkevich]

After reset, CKC value is set to 0, so if clock-comparator interrupts
are enabled, one should occur very shortly thereafter.

Currently the code does not set tod_timer, so this does not happen.

Fix by adding a tcg_s390_tod_updated() call. Initialize TOD clock
before CPUs in order to ensure that the respective object exists
during the CPU reset.

Cc: qemu-stable@nongnu.org
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 6 +++---
 target/s390x/cpu.c         | 8 ++++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index ad2c48188a8..9f37a96ae5a 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -286,6 +286,9 @@ static void ccw_init(MachineState *machine)
     /* init memory + setup max page size. Required for the CPU model */
     s390_memory_init(machine);
 
+    /* init the TOD clock */
+    s390_init_tod();
+
     /* init CPUs (incl. CPU model) early so s390_has_feature() works */
     s390_init_cpus(machine);
 
@@ -332,9 +335,6 @@ static void ccw_init(MachineState *machine)
         s390_create_sclpconsole(ms->sclp, "sclplmconsole", serial_hd(1));
     }
 
-    /* init the TOD clock */
-    s390_init_tod();
-
     /* init SCLP event Control-Program Identification */
     if (s390mc->use_cpi) {
         s390_create_sclpcpi(ms->sclp);
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index f05ce317da1..cc2de6ce08e 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -40,6 +40,7 @@
 #include "system/reset.h"
 #endif
 #include "hw/s390x/cpu-topology.h"
+#include "tcg/tcg_s390x.h"
 
 #define CR0_RESET       0xE0UL
 #define CR14_RESET      0xC2000000UL;
@@ -215,6 +216,13 @@ static void s390_cpu_reset_hold(Object *obj, ResetType type)
             break;
         }
     }
+
+#ifndef CONFIG_USER_ONLY
+    if (tcg_enabled()) {
+        /* Rearm the CKC timer if necessary */
+        tcg_s390_tod_updated(CPU(cpu), RUN_ON_CPU_NULL);
+    }
+#endif
 }
 
 static void s390_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
-- 
2.51.0

[PATCH v2 3/3] tests/tcg/s390x: Test SET CLOCK COMPARATOR

[Ilya Leoshkevich]

Add a small test to prevent regressions.

Cc: qemu-stable@nongnu.org
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/s390x/Makefile.softmmu-target |  1 +
 tests/tcg/s390x/sckc.S                  | 55 +++++++++++++++++++++++++
 2 files changed, 56 insertions(+)
 create mode 100644 tests/tcg/s390x/sckc.S

diff --git a/tests/tcg/s390x/Makefile.softmmu-target b/tests/tcg/s390x/Makefile.softmmu-target
index 8cd4667c63b..a4425d3184a 100644
--- a/tests/tcg/s390x/Makefile.softmmu-target
+++ b/tests/tcg/s390x/Makefile.softmmu-target
@@ -28,6 +28,7 @@ ASM_TESTS =                                                                    \
     mc                                                                         \
     per                                                                        \
     precise-smc-softmmu                                                        \
+    sckc                                                                       \
     ssm-early                                                                  \
     stosm-early                                                                \
     stpq                                                                       \
diff --git a/tests/tcg/s390x/sckc.S b/tests/tcg/s390x/sckc.S
new file mode 100644
index 00000000000..66e8733f45c
--- /dev/null
+++ b/tests/tcg/s390x/sckc.S
@@ -0,0 +1,55 @@
+/*
+ * Test clock comparator.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+    .org 0x130
+ext_old_psw:
+    .org 0x1b0
+ext_new_psw:
+    .quad 0x180000000, _ext            /* 64-bit mode */
+    .org 0x200                         /* lowcore padding */
+
+    .globl _start
+_start:
+    lpswe start31_psw
+_start31:
+    stctg %c0,%c0,c0
+    oi c0+6,8                          /* set clock-comparator subclass mask */
+    lctlg %c0,%c0,c0
+    jg .
+
+_ext:
+    stg %r0,ext_saved_r0
+
+    lg %r0,ext_counter
+    aghi %r0,1
+    stg %r0,ext_counter
+
+    cgfi %r0,0x1000
+    jnz 0f
+    lpswe success_psw
+0:
+
+    stck clock
+    lg %r0,clock
+    agfi %r0,0x40000                   /* 64us * 0x1000 =~ 0.25s */
+    stg %r0,clock
+    sckc clock
+
+    lg %r0,ext_saved_r0
+    lpswe ext_old_psw
+
+    .align 8
+start31_psw:
+    .quad 0x100000080000000,_start31   /* EX, 31-bit mode */
+success_psw:
+    .quad 0x2000000000000,0xfff        /* see is_special_wait_psw() */
+c0:
+    .skip 8
+clock:
+    .quad 0
+ext_counter:
+    .quad 0
+ext_saved_r0:
+    .skip 8
-- 
2.51.0

Re: [PATCH v2 2/3] target/s390x: Fix missing clock-comparator interrupts after reset

[Thomas Huth]

  Hi Ilya!

On 15/10/2025 16.21, Ilya Leoshkevich wrote:
> After reset, CKC value is set to 0, so if clock-comparator interrupts
> are enabled, one should occur very shortly thereafter.
> 
> Currently the code does not set tod_timer, so this does not happen.
> 
> Fix by adding a tcg_s390_tod_updated() call. Initialize TOD clock
> before CPUs in order to ensure that the respective object exists
> during the CPU reset.

Can this really happen? Looking at CPUS390XState in target/s390x/cpu.h, the 
ckc is next to the cregs[] in the start_initial_reset_fields section, so if 
ckc gets cleared, the cregs get cleared, too. I.e. if ckc gets set to 0 
here, there is no way that the clock comparator interrupt could trigger 
immediately without the guest writing to the control registers first. So I 
think this patch is not really necessary. Or do I miss something?

  Thomas



> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   hw/s390x/s390-virtio-ccw.c | 6 +++---
>   target/s390x/cpu.c         | 8 ++++++++
>   2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index ad2c48188a8..9f37a96ae5a 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -286,6 +286,9 @@ static void ccw_init(MachineState *machine)
>       /* init memory + setup max page size. Required for the CPU model */
>       s390_memory_init(machine);
>   
> +    /* init the TOD clock */
> +    s390_init_tod();
> +
>       /* init CPUs (incl. CPU model) early so s390_has_feature() works */
>       s390_init_cpus(machine);
>   
> @@ -332,9 +335,6 @@ static void ccw_init(MachineState *machine)
>           s390_create_sclpconsole(ms->sclp, "sclplmconsole", serial_hd(1));
>       }
>   
> -    /* init the TOD clock */
> -    s390_init_tod();
> -
>       /* init SCLP event Control-Program Identification */
>       if (s390mc->use_cpi) {
>           s390_create_sclpcpi(ms->sclp);
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index f05ce317da1..cc2de6ce08e 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -40,6 +40,7 @@
>   #include "system/reset.h"
>   #endif
>   #include "hw/s390x/cpu-topology.h"
> +#include "tcg/tcg_s390x.h"
>   
>   #define CR0_RESET       0xE0UL
>   #define CR14_RESET      0xC2000000UL;
> @@ -215,6 +216,13 @@ static void s390_cpu_reset_hold(Object *obj, ResetType type)
>               break;
>           }
>       }
> +
> +#ifndef CONFIG_USER_ONLY
> +    if (tcg_enabled()) {
> +        /* Rearm the CKC timer if necessary */
> +        tcg_s390_tod_updated(CPU(cpu), RUN_ON_CPU_NULL);
> +    }
> +#endif
>   }
>   
>   static void s390_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)

Re: [PATCH v2 3/3] tests/tcg/s390x: Test SET CLOCK COMPARATOR

[Thomas Huth]

On 15/10/2025 16.21, Ilya Leoshkevich wrote:
> Add a small test to prevent regressions.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   tests/tcg/s390x/Makefile.softmmu-target |  1 +
>   tests/tcg/s390x/sckc.S                  | 55 +++++++++++++++++++++++++
>   2 files changed, 56 insertions(+)
>   create mode 100644 tests/tcg/s390x/sckc.S

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

Re: [PATCH v2 2/3] target/s390x: Fix missing clock-comparator interrupts after reset

[Thomas Huth]

On 16/10/2025 11.53, Thomas Huth wrote:
>   Hi Ilya!
> 
> On 15/10/2025 16.21, Ilya Leoshkevich wrote:
>> After reset, CKC value is set to 0, so if clock-comparator interrupts
>> are enabled, one should occur very shortly thereafter.
>>
>> Currently the code does not set tod_timer, so this does not happen.
>>
>> Fix by adding a tcg_s390_tod_updated() call. Initialize TOD clock
>> before CPUs in order to ensure that the respective object exists
>> during the CPU reset.
> 
> Can this really happen? Looking at CPUS390XState in target/s390x/cpu.h, the 
> ckc is next to the cregs[] in the start_initial_reset_fields section, so if 
> ckc gets cleared, the cregs get cleared, too. I.e. if ckc gets set to 0 
> here, there is no way that the clock comparator interrupt could trigger 
> immediately without the guest writing to the control registers first. So I 
> think this patch is not really necessary. Or do I miss something?

... but it looks like your TCG test is failing without this second patch.

Ok, I think I understood it now: We don't re-arm the CKC timer in case the 
guest writes to the CR0. So we need to have the timer started during reset 
already to make the interrupt pending, so that it can fire when the guest 
changes CR0.

So I guess this patch is fine. Or alternatively you could maybe re-arm the 
CKC during stctg, would that be better? What do you think?

  Thomas