[PATCH for-8.0] target/s390x/tcg: Fix and improve the SACF instruction

[PATCH for-8.0] target/s390x/tcg: Fix and improve the SACF instruction

[Thomas Huth]

The SET ADDRESS SPACE CONTROL FAST code has a couple of issues:

1) The instruction is not privileged, it can be used from problem space,
too. Just the switching to the home address space is privileged and
should still generate a privilege exception. This bug is e.g. causing
programs like Java that use the "getcpu" vdso kernel function to crash
(see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=990417)

2) If DAT is not enabled, the instruction is supposed to generate
a special operation exception.

3) The switch-case statement in the code hid a weird oddity: It did not
support the secondary address space though that should be working without
problems. But there is a "case 0x100" which means access register mode -
and that is not implemented in QEMU yet. The code used the secondary mode
for the access register mode instead - which seems to sufficient to make
the Linux kernel happy that still temporarily tries to switch to the access
register mode here and there. Anyway, let's get rid of the cumbersome
switch-case statement and add a proper comment for the access register
oddity to make it more clear what is going on here.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/655
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target/s390x/tcg/insn-data.h.inc |  2 +-
 target/s390x/tcg/cc_helper.c     | 36 +++++++++++++++++++-------------
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/target/s390x/tcg/insn-data.h.inc b/target/s390x/tcg/insn-data.h.inc
index 7e952bdfc8..54d4250c9f 100644
--- a/target/s390x/tcg/insn-data.h.inc
+++ b/target/s390x/tcg/insn-data.h.inc
@@ -1365,7 +1365,7 @@
 /* SERVICE CALL LOGICAL PROCESSOR (PV hypercall) */
     F(0xb220, SERVC,   RRE,   Z,   r1_o, r2_o, 0, 0, servc, 0, IF_PRIV | IF_IO)
 /* SET ADDRESS SPACE CONTROL FAST */
-    F(0xb279, SACF,    S,     Z,   0, a2, 0, 0, sacf, 0, IF_PRIV)
+    C(0xb279, SACF,    S,     Z,   0, a2, 0, 0, sacf, 0)
 /* SET CLOCK */
     F(0xb204, SCK,     S,     Z,   0, m2_64a, 0, 0, sck, 0, IF_PRIV | IF_IO)
 /* SET CLOCK COMPARATOR */
diff --git a/target/s390x/tcg/cc_helper.c b/target/s390x/tcg/cc_helper.c
index b2e8d3d9f5..6f1350c4e2 100644
--- a/target/s390x/tcg/cc_helper.c
+++ b/target/s390x/tcg/cc_helper.c
@@ -486,23 +486,31 @@ void HELPER(load_psw)(CPUS390XState *env, uint64_t mask, uint64_t addr)
 void HELPER(sacf)(CPUS390XState *env, uint64_t a1)
 {
     HELPER_LOG("%s: %16" PRIx64 "\n", __func__, a1);
+    uint64_t as = (a1 & 0xf00) >> 8;
 
-    switch (a1 & 0xf00) {
-    case 0x000:
-        env->psw.mask &= ~PSW_MASK_ASC;
-        env->psw.mask |= PSW_ASC_PRIMARY;
-        break;
-    case 0x100:
-        env->psw.mask &= ~PSW_MASK_ASC;
-        env->psw.mask |= PSW_ASC_SECONDARY;
-        break;
-    case 0x300:
-        env->psw.mask &= ~PSW_MASK_ASC;
-        env->psw.mask |= PSW_ASC_HOME;
-        break;
-    default:
+    if (!(env->psw.mask & PSW_MASK_DAT)) {
+        tcg_s390_program_interrupt(env, PGM_SPECIAL_OP, GETPC());
+    }
+
+    if (as == AS_HOME && (env->psw.mask & PSW_MASK_PSTATE)) {
+        tcg_s390_program_interrupt(env, PGM_PRIVILEGED, GETPC());
+    }
+
+    if ((as & 0xc) != 0) {
         HELPER_LOG("unknown sacf mode: %" PRIx64 "\n", a1);
         tcg_s390_program_interrupt(env, PGM_SPECIFICATION, GETPC());
     }
+
+    if (as == AS_ACCREG) {
+        /*
+         * FIXME: Access register mode is not implemented yet, but Linux
+         * still seems to try to temporarily use this. Fortunately, it
+         * seems to be happy if we use the secondary mode instead.
+         */
+        as = AS_SECONDARY;
+    }
+
+    env->psw.mask &= ~PSW_MASK_ASC;
+    env->psw.mask |= as << PSW_SHIFT_ASC;
 }
 #endif
-- 
2.31.1

Re: [PATCH for-8.0] target/s390x/tcg: Fix and improve the SACF instruction

[Thomas Huth]

On 01/12/2022 19.27, Thomas Huth wrote:
> The SET ADDRESS SPACE CONTROL FAST code has a couple of issues:
> 
> 1) The instruction is not privileged, it can be used from problem space,
> too. Just the switching to the home address space is privileged and
> should still generate a privilege exception. This bug is e.g. causing
> programs like Java that use the "getcpu" vdso kernel function to crash
> (see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=990417)
> 
> 2) If DAT is not enabled, the instruction is supposed to generate
> a special operation exception.
> 
> 3) The switch-case statement in the code hid a weird oddity: It did not
> support the secondary address space though that should be working without
> problems. But there is a "case 0x100" which means access register mode -
> and that is not implemented in QEMU yet. The code used the secondary mode
> for the access register mode instead - which seems to sufficient to make
> the Linux kernel happy that still temporarily tries to switch to the access
> register mode here and there. Anyway, let's get rid of the cumbersome
> switch-case statement and add a proper comment for the access register
> oddity to make it more clear what is going on here.

Oooops, never mind, I just missed the part in the Principles of Operations 
where this is explained: The bit ordering is different here compared to the 
bits in the PSW. Ugly. I'll rework my patch...

  Thomas