[PATCH 0/2] target/s390x: Implement Early Exception Recognition

[PATCH 0/2] target/s390x: Implement Early Exception Recognition

[Ilya Leoshkevich]

Hi,

Currently loading bad PSW flags does not lead to an exception, which is
not correct. This series fixes this by implementing what PoP calls
"Early Exception Recognition". Since it applies to both loading PSW with
LPSW/LPSWE and to interrupt handling, s390_cpu_set_psw() looks like the
right place for it to be in.

Patch 1 fixes the issue, patch 2 adds a test.

Best regards,
Ilya

Ilya Leoshkevich (2):
  target/s390x: Implement Early Exception Recognition
  tests/tcg/s390x: Add early-exception-recognition.S

 target/s390x/cpu.c                            | 26 +++++++++++++
 target/s390x/cpu.h                            |  1 +
 target/s390x/tcg/excp_helper.c                |  3 +-
 tests/tcg/s390x/Makefile.softmmu-target       |  1 +
 tests/tcg/s390x/early-exception-recognition.S | 38 +++++++++++++++++++
 5 files changed, 68 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/s390x/early-exception-recognition.S

-- 
2.39.2

[PATCH 1/2] target/s390x: Implement Early Exception Recognition

[Ilya Leoshkevich]

Generate specification exception if a reserved bit is set in the PSW
mask or if the PSW address is out of bounds dictated by the addresing
mode.

Reported-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 target/s390x/cpu.c             | 26 ++++++++++++++++++++++++++
 target/s390x/cpu.h             |  1 +
 target/s390x/tcg/excp_helper.c |  3 ++-
 3 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index b10a8541ff8..8e6e46aa3d8 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -41,6 +41,26 @@
 #define CR0_RESET       0xE0UL
 #define CR14_RESET      0xC2000000UL;
 
+#ifndef CONFIG_USER_ONLY
+static bool is_early_exception_recognized(uint64_t mask, uint64_t addr)
+{
+    if (mask & PSW_MASK_RESERVED) {
+        return true;
+    }
+
+    switch (mask & (PSW_MASK_32 | PSW_MASK_64)) {
+    case 0:
+        return addr & ~0xffffffULL;
+    case PSW_MASK_32:
+        return addr & ~0x7fffffffULL;
+    case PSW_MASK_32 | PSW_MASK_64:
+        return false;
+    default: /* PSW_MASK_64 */
+        return true;
+    }
+}
+#endif
+
 void s390_cpu_set_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
 {
 #ifndef CONFIG_USER_ONLY
@@ -57,6 +77,12 @@ void s390_cpu_set_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
     env->cc_op = (mask >> 44) & 3;
 
 #ifndef CONFIG_USER_ONLY
+    if (is_early_exception_recognized(mask, addr)) {
+        env->int_pgm_ilen = 0;
+        trigger_pgm_exception(env, PGM_SPECIFICATION);
+        return;
+    }
+
     if ((old_mask ^ mask) & PSW_MASK_PER) {
         s390_cpu_recompute_watchpoints(env_cpu(env));
     }
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 7d6d01325b2..b4de6945936 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -292,6 +292,7 @@ extern const VMStateDescription vmstate_s390_cpu;
 #define PSW_MASK_32             0x0000000080000000ULL
 #define PSW_MASK_SHORT_ADDR     0x000000007fffffffULL
 #define PSW_MASK_SHORT_CTRL     0xffffffff80000000ULL
+#define PSW_MASK_RESERVED       0xb800007e7fffffffULL
 
 #undef PSW_ASC_PRIMARY
 #undef PSW_ASC_ACCREG
diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c
index bc767f04438..a7829b1e494 100644
--- a/target/s390x/tcg/excp_helper.c
+++ b/target/s390x/tcg/excp_helper.c
@@ -212,7 +212,8 @@ static void do_program_interrupt(CPUS390XState *env)
     LowCore *lowcore;
     int ilen = env->int_pgm_ilen;
 
-    assert(ilen == 2 || ilen == 4 || ilen == 6);
+    assert((env->int_pgm_code == PGM_SPECIFICATION && ilen == 0) ||
+           ilen == 2 || ilen == 4 || ilen == 6);
 
     switch (env->int_pgm_code) {
     case PGM_PER:
-- 
2.39.2

[PATCH 2/2] tests/tcg/s390x: Add early-exception-recognition.S

[Ilya Leoshkevich]

Add a small test that checks whether early exceptions are recognized
and whether the correct ILC and old PSW are stored when they happen.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/s390x/Makefile.softmmu-target       |  1 +
 tests/tcg/s390x/early-exception-recognition.S | 38 +++++++++++++++++++
 2 files changed, 39 insertions(+)
 create mode 100644 tests/tcg/s390x/early-exception-recognition.S

diff --git a/tests/tcg/s390x/Makefile.softmmu-target b/tests/tcg/s390x/Makefile.softmmu-target
index 725b6c598db..9d3180bbcba 100644
--- a/tests/tcg/s390x/Makefile.softmmu-target
+++ b/tests/tcg/s390x/Makefile.softmmu-target
@@ -9,3 +9,4 @@ QEMU_OPTS=-action panic=exit-failure -kernel
 TESTS += unaligned-lowcore
 TESTS += bal
 TESTS += sam
+TESTS += early-exception-recognition
diff --git a/tests/tcg/s390x/early-exception-recognition.S b/tests/tcg/s390x/early-exception-recognition.S
new file mode 100644
index 00000000000..4e9677fea36
--- /dev/null
+++ b/tests/tcg/s390x/early-exception-recognition.S
@@ -0,0 +1,38 @@
+/*
+ * Test early exception recognition.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+    .org 0x8d
+ilc:
+    .org 0x8e
+program_interruption_code:
+    .org 0x150
+program_old_psw:
+    .org 0x1D0                         /* program new PSW */
+    .quad 0,pgm
+    .org 0x200                         /* lowcore padding */
+
+    .globl _start
+_start:
+    lpswe bad_psw
+    j failure
+
+pgm:
+    chhsi program_interruption_code,0x6          /* specification exception? */
+    jne failure
+    cli ilc,0                                    /* ilc zero? */
+    jne failure
+    clc program_old_psw(16),bad_psw              /* correct old PSW? */
+    jne failure
+    lpswe success_psw
+failure:
+    lpswe failure_psw
+
+    .align 8
+bad_psw:
+    .quad 0x8000000000000000,0xfedcba9876543210  /* bit 0 set */
+success_psw:
+    .quad 0x2000000000000,0xfff        /* see is_special_wait_psw() */
+failure_psw:
+    .quad 0x2000000000000,0            /* disabled wait */
-- 
2.39.2

Re: [PATCH 1/2] target/s390x: Implement Early Exception Recognition

[Richard Henderson]

On 3/14/23 04:00, Ilya Leoshkevich wrote:
> Generate specification exception if a reserved bit is set in the PSW
> mask or if the PSW address is out of bounds dictated by the addresing
> mode.
> 
> Reported-by: Nina Schoetterl-Glausch<nsg@linux.ibm.com>
> Signed-off-by: Ilya Leoshkevich<iii@linux.ibm.com>
> ---
>   target/s390x/cpu.c             | 26 ++++++++++++++++++++++++++
>   target/s390x/cpu.h             |  1 +
>   target/s390x/tcg/excp_helper.c |  3 ++-
>   3 files changed, 29 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/2] tests/tcg/s390x: Add early-exception-recognition.S

[Richard Henderson]

On 3/14/23 04:00, Ilya Leoshkevich wrote:
> Add a small test that checks whether early exceptions are recognized
> and whether the correct ILC and old PSW are stored when they happen.
> 
> Signed-off-by: Ilya Leoshkevich<iii@linux.ibm.com>
> ---
>   tests/tcg/s390x/Makefile.softmmu-target       |  1 +
>   tests/tcg/s390x/early-exception-recognition.S | 38 +++++++++++++++++++
>   2 files changed, 39 insertions(+)
>   create mode 100644 tests/tcg/s390x/early-exception-recognition.S

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 1/2] target/s390x: Implement Early Exception Recognition

[David Hildenbrand]

On 14.03.23 12:00, Ilya Leoshkevich wrote:
> Generate specification exception if a reserved bit is set in the PSW
> mask or if the PSW address is out of bounds dictated by the addresing
> mode.
> 
> Reported-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

Unofficially known to be broken (and ignored) for a long time :D

> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   target/s390x/cpu.c             | 26 ++++++++++++++++++++++++++
>   target/s390x/cpu.h             |  1 +
>   target/s390x/tcg/excp_helper.c |  3 ++-
>   3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index b10a8541ff8..8e6e46aa3d8 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -41,6 +41,26 @@
>   #define CR0_RESET       0xE0UL
>   #define CR14_RESET      0xC2000000UL;
>   
> +#ifndef CONFIG_USER_ONLY
> +static bool is_early_exception_recognized(uint64_t mask, uint64_t addr)

Nit: I'd call this is_early_exception_psw() or sth like that.

Thanks!

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

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/2] target/s390x: Implement Early Exception Recognition

[Nina Schoetterl-Glausch]

On Tue, 2023-03-14 at 12:00 +0100, Ilya Leoshkevich wrote:
> Generate specification exception if a reserved bit is set in the PSW

Generate a ...
> mask or if the PSW address is out of bounds dictated by the addresing

addresSing

> mode.

Does this approach also work with SET SYSTEM MASK and STORE THEN OR SYSTEM
MASK, i.e. do these call s390_cpu_set_psw via some code path I'm not seeing?

In any case, you don't seem to implement their special requirements with regards
to the ilen, so you should at least mention that.

> 
> Reported-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  target/s390x/cpu.c             | 26 ++++++++++++++++++++++++++
>  target/s390x/cpu.h             |  1 +
>  target/s390x/tcg/excp_helper.c |  3 ++-
>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index b10a8541ff8..8e6e46aa3d8 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -41,6 +41,26 @@
>  #define CR0_RESET       0xE0UL
>  #define CR14_RESET      0xC2000000UL;
>  
> +#ifndef CONFIG_USER_ONLY
> +static bool is_early_exception_recognized(uint64_t mask, uint64_t addr)
> +{
> +    if (mask & PSW_MASK_RESERVED) {
> +        return true;
> +    }
> +
> +    switch (mask & (PSW_MASK_32 | PSW_MASK_64)) {
> +    case 0:
> +        return addr & ~0xffffffULL;
> +    case PSW_MASK_32:
> +        return addr & ~0x7fffffffULL;
> +    case PSW_MASK_32 | PSW_MASK_64:
> +        return false;
> +    default: /* PSW_MASK_64 */
> +        return true;
> +    }
> +}
> +#endif
> +
>  void s390_cpu_set_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
>  {
>  #ifndef CONFIG_USER_ONLY
> @@ -57,6 +77,12 @@ void s390_cpu_set_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
>      env->cc_op = (mask >> 44) & 3;
>  
>  #ifndef CONFIG_USER_ONLY
> +    if (is_early_exception_recognized(mask, addr)) {
> +        env->int_pgm_ilen = 0;
> +        trigger_pgm_exception(env, PGM_SPECIFICATION);
> +        return;
> +    }
> +
>      if ((old_mask ^ mask) & PSW_MASK_PER) {
>          s390_cpu_recompute_watchpoints(env_cpu(env));
>      }
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 7d6d01325b2..b4de6945936 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -292,6 +292,7 @@ extern const VMStateDescription vmstate_s390_cpu;
>  #define PSW_MASK_32             0x0000000080000000ULL
>  #define PSW_MASK_SHORT_ADDR     0x000000007fffffffULL
>  #define PSW_MASK_SHORT_CTRL     0xffffffff80000000ULL
> +#define PSW_MASK_RESERVED       0xb800007e7fffffffULL

What about bit 12?

With bit 12 added:
I haven't checked, but I don't think that would be sufficient for short PSWs loaded
with lpsw. I'm not seeing op_lpsw flipping the 12th bit.
op_lpsw looks pretty broken to me overall, putting the BA bit into the address.
I think it should look something like this (didn't actually try it):

static DisasJumpType op_lpsw(DisasContext *s, DisasOps *o)
{
    TCGv_i64 t1, t2;

    per_breaking_event(s);

    t1 = tcg_temp_new_i64();
    t2 = tcg_temp_new_i64();
    tcg_gen_qemu_ld_i64(t1, o->in2, get_mem_index(s),
                        MO_TEUQ | MO_ALIGN);		//load 8byte instead of 4
    tcg_gen_addi_i64(o->in2, o->in2, 4);
    tcg_gen_qemu_ld32u(t2, o->in2, get_mem_index(s));
    tcg_gen_andi_i64(t2, t2, PSW_MASK_SHORT_ADDR);
    /* Convert the 32-bit PSW_MASK into the 64-bit PSW_MASK.  */
    tcg_gen_andi_i64(t1, t1, PSW_MASK_SHORT_CTRL);
    tcg_gen_xori_i64(t1, t1, PSW_MASK_SHORTPSW);
    gen_helper_load_psw(cpu_env, t1, t2);
    return DISAS_NORETURN;
}
>  
>  #undef PSW_ASC_PRIMARY
>  #undef PSW_ASC_ACCREG
> diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c
> index bc767f04438..a7829b1e494 100644
> --- a/target/s390x/tcg/excp_helper.c
> +++ b/target/s390x/tcg/excp_helper.c
> @@ -212,7 +212,8 @@ static void do_program_interrupt(CPUS390XState *env)
>      LowCore *lowcore;
>      int ilen = env->int_pgm_ilen;
>  
> -    assert(ilen == 2 || ilen == 4 || ilen == 6);
> +    assert((env->int_pgm_code == PGM_SPECIFICATION && ilen == 0) ||
> +           ilen == 2 || ilen == 4 || ilen == 6);
>  
>      switch (env->int_pgm_code) {
>      case PGM_PER: