[Qemu-devel] [PATCH v2 0/3] Implement basic MSA functions

[Qemu-devel] [PATCH v2 0/3] Implement basic MSA functions

[David Hildenbrand]

Some leftover from "target/s390x: tcg improvments + MSA functions".

Implement all basic MSA (cpacf/crypto) instructions <= z13. Only provide
the query subfunction (to query available subfunctions), no actual
de/encryption yet. Good enough to unlock the STFL bits.

I have written kvm-unit-tests for MSA functions and for SPM/IPM. Will
send them out soon. I use the following cpu model to test with an upstream
kernel compiled for z10:

... -cpu qemu,mvcos=on,stfle=on,ldisp=on,ldisphp=on,\
              eimm=on,stckf=on,csst=on,csst2=on,ginste=on,\
              exrl=on,msa-base=on,msa3-base=on,msa4-base=on,msa5-base=on ...

Based on: https://github.com/cohuck/qemu.git s390-next
Available on: https://github.com/davidhildenbrand/qemu.git s390x-msa

v1 -> v2:
 - "s390x/tcg: implement spm (SET PROGRAM MASK)"
 -- use tcg_gen_extrl_i64_i32 + tcg_gen_extract_i32
 - "s390x/tcg: move wrap_address() to internal.h"
 -- internal.h instead of cpu.h
 - "s390x/tcg: add basic MSA features"
 -- minor style fixes


David Hildenbrand (3):
  s390x/tcg: implement spm (SET PROGRAM MASK)
  s390x/tcg: move wrap_address() to internal.h
  s390x/tcg: add basic MSA features

 target/s390x/Makefile.objs   |  2 +-
 target/s390x/cpu.h           |  2 ++
 target/s390x/cpu_models.c    |  4 +++
 target/s390x/crypto_helper.c | 65 ++++++++++++++++++++++++++++++++++++++++++
 target/s390x/helper.h        |  1 +
 target/s390x/insn-data.def   | 15 ++++++++++
 target/s390x/internal.h      | 14 +++++++++
 target/s390x/mem_helper.c    | 14 ---------
 target/s390x/translate.c     | 67 ++++++++++++++++++++++++++++++++++++++++++++
 9 files changed, 169 insertions(+), 15 deletions(-)
 create mode 100644 target/s390x/crypto_helper.c

-- 
2.13.5

[Qemu-devel] [PATCH v2 1/3] s390x/tcg: implement spm (SET PROGRAM MASK)

[David Hildenbrand]

Missing and is used inside Linux in the context of CPACF.

Reviewed-by: Richard Henderson <rth@twiddle.net>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu.h         |  2 ++
 target/s390x/insn-data.def |  2 ++
 target/s390x/translate.c   | 11 +++++++++++
 3 files changed, 15 insertions(+)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 9b549dc491..4414485089 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -292,6 +292,7 @@ extern const struct VMStateDescription vmstate_s390_cpu;
 #undef PSW_SHIFT_ASC
 #undef PSW_MASK_CC
 #undef PSW_MASK_PM
+#undef PSW_SHIFT_MASK_PM
 #undef PSW_MASK_64
 #undef PSW_MASK_32
 #undef PSW_MASK_ESA_ADDR
@@ -309,6 +310,7 @@ extern const struct VMStateDescription vmstate_s390_cpu;
 #define PSW_SHIFT_ASC           46
 #define PSW_MASK_CC             0x0000300000000000ULL
 #define PSW_MASK_PM             0x00000F0000000000ULL
+#define PSW_SHIFT_MASK_PM       40
 #define PSW_MASK_64             0x0000000100000000ULL
 #define PSW_MASK_32             0x0000000080000000ULL
 #define PSW_MASK_ESA_ADDR       0x000000007fffffffULL
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index ad84c748e3..84233a456d 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -755,6 +755,8 @@
     C(0xb2b8, SRNMB,   S,     FPE, 0, 0, 0, 0, srnm, 0)
 /* SET DFP ROUNDING MODE */
     C(0xb2b9, SRNMT,   S,     DFPR, 0, 0, 0, 0, srnm, 0)
+/* SET PROGRAM MASK */
+    C(0x0400, SPM,     RR_a,  Z,   r1, 0, 0, 0, spm, 0)
 
 /* SHIFT LEFT SINGLE */
     D(0x8b00, SLA,     RS_a,  Z,   r1, sh32, new, r1_32, sla, 0, 31)
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 5abd34fb34..59fde44d55 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -3796,6 +3796,17 @@ static ExitStatus op_srnm(DisasContext *s, DisasOps *o)
     return NO_EXIT;
 }
 
+static ExitStatus op_spm(DisasContext *s, DisasOps *o)
+{
+    tcg_gen_extrl_i64_i32(cc_op, o->in1);
+    tcg_gen_extract_i32(cc_op, cc_op, 28, 2);
+    set_cc_static(s);
+
+    tcg_gen_shri_i64(o->in1, o->in1, 24);
+    tcg_gen_deposit_i64(psw_mask, psw_mask, o->in1, PSW_SHIFT_MASK_PM, 4);
+    return NO_EXIT;
+}
+
 #ifndef CONFIG_USER_ONLY
 static ExitStatus op_spka(DisasContext *s, DisasOps *o)
 {
-- 
2.13.5

[Qemu-devel] [PATCH v2 2/3] s390x/tcg: move wrap_address() to internal.h

[David Hildenbrand]

We want to use it in another file.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/internal.h   | 14 ++++++++++++++
 target/s390x/mem_helper.c | 14 --------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/target/s390x/internal.h b/target/s390x/internal.h
index bc8f83129a..70d2b87e55 100644
--- a/target/s390x/internal.h
+++ b/target/s390x/internal.h
@@ -162,6 +162,20 @@ static inline uint8_t get_per_atmid(CPUS390XState *env)
            ((env->psw.mask & PSW_ASC_ACCREG) ?    (1 << 2) : 0);
 }
 
+static inline uint64_t wrap_address(CPUS390XState *env, uint64_t a)
+{
+    if (!(env->psw.mask & PSW_MASK_64)) {
+        if (!(env->psw.mask & PSW_MASK_32)) {
+            /* 24-Bit mode */
+            a &= 0x00ffffff;
+        } else {
+            /* 31-Bit mode */
+            a &= 0x7fffffff;
+        }
+    }
+    return a;
+}
+
 /* CC optimization */
 
 /* Instead of computing the condition codes after each x86 instruction,
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index ec4760e390..a254613dc2 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -122,20 +122,6 @@ static inline void cpu_stsize_data_ra(CPUS390XState *env, uint64_t addr,
     }
 }
 
-static inline uint64_t wrap_address(CPUS390XState *env, uint64_t a)
-{
-    if (!(env->psw.mask & PSW_MASK_64)) {
-        if (!(env->psw.mask & PSW_MASK_32)) {
-            /* 24-Bit mode */
-            a &= 0x00ffffff;
-        } else {
-            /* 31-Bit mode */
-            a &= 0x7fffffff;
-        }
-    }
-    return a;
-}
-
 static void fast_memset(CPUS390XState *env, uint64_t dest, uint8_t byte,
                         uint32_t l, uintptr_t ra)
 {
-- 
2.13.5

[Qemu-devel] [PATCH v2 3/3] s390x/tcg: add basic MSA features

[David Hildenbrand]

The STFLE bits for the MSA (extension) facilities simply indicate that
the respective instructions can be executed. The QUERY subfunction can then
be used to identify which features exactly are available.

Availability of subfunctions can also vary on real hardware. For now, we
simply implement a CPU model without any available subfunctions except
QUERY (which is always around).

As all MSA functions behave quite similarly, we can use one translation
handler for now. Prepare the code for implementation of actual subfunctions.

At least MSA is helpful for now, as older Linux kernels require this
facility when compiled for a z9 model. Allow to enable the facilities
for the qemu cpu model.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/Makefile.objs   |  2 +-
 target/s390x/cpu_models.c    |  4 +++
 target/s390x/crypto_helper.c | 65 ++++++++++++++++++++++++++++++++++++++++++++
 target/s390x/helper.h        |  1 +
 target/s390x/insn-data.def   | 13 +++++++++
 target/s390x/translate.c     | 56 ++++++++++++++++++++++++++++++++++++++
 6 files changed, 140 insertions(+), 1 deletion(-)
 create mode 100644 target/s390x/crypto_helper.c

diff --git a/target/s390x/Makefile.objs b/target/s390x/Makefile.objs
index 9615256d81..c88ac81e84 100644
--- a/target/s390x/Makefile.objs
+++ b/target/s390x/Makefile.objs
@@ -1,6 +1,6 @@
 obj-y += cpu.o cpu_models.o cpu_features.o gdbstub.o interrupt.o helper.o
 obj-$(CONFIG_TCG) += translate.o cc_helper.o excp_helper.o fpu_helper.o
-obj-$(CONFIG_TCG) += int_helper.o mem_helper.o misc_helper.o
+obj-$(CONFIG_TCG) += int_helper.o mem_helper.o misc_helper.o crypto_helper.o
 obj-$(CONFIG_SOFTMMU) += machine.o ioinst.o arch_dump.o mmu_helper.o diag.o
 obj-$(CONFIG_KVM) += kvm.o
 obj-$(call lnot,$(CONFIG_KVM)) += kvm-stub.o
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 5169379db5..1051a16ece 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -825,6 +825,7 @@ static void add_qemu_cpu_model_features(S390FeatBitmap fbm)
         S390_FEAT_STFLE,
         S390_FEAT_EXTENDED_IMMEDIATE,
         S390_FEAT_EXTENDED_TRANSLATION_2,
+        S390_FEAT_MSA,
         S390_FEAT_EXTENDED_TRANSLATION_3,
         S390_FEAT_LONG_DISPLACEMENT,
         S390_FEAT_LONG_DISPLACEMENT_FAST,
@@ -841,6 +842,9 @@ static void add_qemu_cpu_model_features(S390FeatBitmap fbm)
         S390_FEAT_STFLE_49,
         S390_FEAT_LOCAL_TLB_CLEARING,
         S390_FEAT_STFLE_53,
+        S390_FEAT_MSA_EXT_5,
+        S390_FEAT_MSA_EXT_3,
+        S390_FEAT_MSA_EXT_4,
     };
     int i;
 
diff --git a/target/s390x/crypto_helper.c b/target/s390x/crypto_helper.c
new file mode 100644
index 0000000000..5b210b4080
--- /dev/null
+++ b/target/s390x/crypto_helper.c
@@ -0,0 +1,65 @@
+/*
+ *  s390x crypto helpers
+ *
+ *  Copyright (c) 2017 Red Hat Inc
+ *
+ *  Authors:
+ *   David Hildenbrand <david@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/main-loop.h"
+#include "internal.h"
+#include "exec/helper-proto.h"
+#include "exec/exec-all.h"
+#include "exec/cpu_ldst.h"
+
+uint32_t HELPER(msa)(CPUS390XState *env, uint32_t r1, uint32_t r2, uint32_t r3,
+                     uint32_t type)
+{
+    const uintptr_t ra = GETPC();
+    const uint8_t mod = env->regs[0] & 0x80ULL;
+    const uint8_t fc = env->regs[0] & 0x7fULL;
+    CPUState *cs = CPU(s390_env_get_cpu(env));
+    uint8_t subfunc[16] = { 0 };
+    uint64_t param_addr;
+    int i;
+
+    switch (type) {
+    case S390_FEAT_TYPE_KMAC:
+    case S390_FEAT_TYPE_KIMD:
+    case S390_FEAT_TYPE_KLMD:
+    case S390_FEAT_TYPE_PCKMO:
+    case S390_FEAT_TYPE_PCC:
+        if (mod) {
+            cpu_restore_state(cs, ra);
+            program_interrupt(env, PGM_SPECIFICATION, 4);
+            return 0;
+        }
+        break;
+    }
+
+    s390_get_feat_block(type, subfunc);
+    if (fc >= 128 || !test_be_bit(fc, subfunc)) {
+        cpu_restore_state(cs, ra);
+        program_interrupt(env, PGM_SPECIFICATION, 4);
+        return 0;
+    }
+
+    switch (fc) {
+    case 0: /* query subfunction */
+        for (i = 0; i < 16; i++) {
+            param_addr = wrap_address(env, env->regs[1] + i);
+            cpu_stb_data_ra(env, param_addr, subfunc[i], ra);
+        }
+        break;
+    default:
+        /* we don't implement any other subfunction yet */
+        g_assert_not_reached();
+    }
+
+    return 0;
+}
diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 4b0290774e..75ba04fc15 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -115,6 +115,7 @@ DEF_HELPER_4(cu21, i32, env, i32, i32, i32)
 DEF_HELPER_4(cu24, i32, env, i32, i32, i32)
 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)
 
 #ifndef CONFIG_USER_ONLY
 DEF_HELPER_3(servc, i32, env, i64, i64)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 84233a456d..d09f2ed538 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -941,6 +941,19 @@
 /* UNPACK UNICODE */
     C(0xe200, UNPKU,   SS_a,  E2,  la1, a2, 0, 0, unpku, 0)
 
+/* MSA Instructions */
+    D(0xb91e, KMAC,    RRE,   MSA,  0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMAC)
+    D(0xb928, PCKMO,   RRE,   MSA3, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PCKMO)
+    D(0xb92a, KMF,     RRE,   MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMF)
+    D(0xb92b, KMO,     RRE,   MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMO)
+    D(0xb92c, PCC,     RRE,   MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PCC)
+    D(0xb92d, KMCTR,   RRF_b, MSA4, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMCTR)
+    D(0xb92e, KM,      RRE,   MSA,  0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KM)
+    D(0xb92f, KMC,     RRE,   MSA,  0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KMC)
+    D(0xb93c, PPNO,    RRE,   MSA5, 0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_PPNO)
+    D(0xb93e, KIMD,    RRE,   MSA,  0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KIMD)
+    D(0xb93f, KLMD,    RRE,   MSA,  0, 0, 0, 0, msa, 0, S390_FEAT_TYPE_KLMD)
+
 #ifndef CONFIG_USER_ONLY
 /* COMPARE AND SWAP AND PURGE */
     D(0xb250, CSP,     RRE,   Z,   r1_32u, ra2, r1_P, 0, csp, 0, MO_TEUL)
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 59fde44d55..78ffd8ff24 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -2422,6 +2422,58 @@ static ExitStatus op_iske(DisasContext *s, DisasOps *o)
 }
 #endif
 
+static ExitStatus op_msa(DisasContext *s, DisasOps *o)
+{
+    int r1 = have_field(s->fields, r1) ? get_field(s->fields, r1) : 0;
+    int r2 = have_field(s->fields, r2) ? get_field(s->fields, r2) : 0;
+    int r3 = have_field(s->fields, r3) ? get_field(s->fields, r3) : 0;
+    TCGv_i32 t_r1, t_r2, t_r3, type;
+
+    switch (s->insn->data) {
+    case S390_FEAT_TYPE_KMCTR:
+        if (r3 & 1 || !r3) {
+            gen_program_exception(s, PGM_SPECIFICATION);
+            return EXIT_NORETURN;
+        }
+        /* FALL THROUGH */
+    case S390_FEAT_TYPE_PPNO:
+    case S390_FEAT_TYPE_KMF:
+    case S390_FEAT_TYPE_KMC:
+    case S390_FEAT_TYPE_KMO:
+    case S390_FEAT_TYPE_KM:
+        if (r1 & 1 || !r1) {
+            gen_program_exception(s, PGM_SPECIFICATION);
+            return EXIT_NORETURN;
+        }
+        /* FALL THROUGH */
+    case S390_FEAT_TYPE_KMAC:
+    case S390_FEAT_TYPE_KIMD:
+    case S390_FEAT_TYPE_KLMD:
+        if (r2 & 1 || !r2) {
+            gen_program_exception(s, PGM_SPECIFICATION);
+            return EXIT_NORETURN;
+        }
+        /* FALL THROUGH */
+    case S390_FEAT_TYPE_PCKMO:
+    case S390_FEAT_TYPE_PCC:
+        break;
+    default:
+        g_assert_not_reached();
+    };
+
+    t_r1 = tcg_const_i32(r1);
+    t_r2 = tcg_const_i32(r2);
+    t_r3 = tcg_const_i32(r3);
+    type = tcg_const_i32(s->insn->data);
+    gen_helper_msa(cc_op, cpu_env, t_r1, t_r2, t_r3, type);
+    set_cc_static(s);
+    tcg_temp_free_i32(t_r1);
+    tcg_temp_free_i32(t_r2);
+    tcg_temp_free_i32(t_r3);
+    tcg_temp_free_i32(type);
+    return NO_EXIT;
+}
+
 static ExitStatus op_keb(DisasContext *s, DisasOps *o)
 {
     gen_helper_keb(cc_op, cpu_env, o->in1, o->in2);
@@ -5505,6 +5557,10 @@ enum DisasInsnEnum {
 #define FAC_PPA         S390_FEAT_STFLE_49 /* processor-assist */
 #define FAC_LZRB        S390_FEAT_STFLE_53 /* load-and-zero-rightmost-byte */
 #define FAC_ETF3        S390_FEAT_EXTENDED_TRANSLATION_3
+#define FAC_MSA         S390_FEAT_MSA /* message-security-assist facility */
+#define FAC_MSA3        S390_FEAT_MSA_EXT_3 /* msa-extension-3 facility */
+#define FAC_MSA4        S390_FEAT_MSA_EXT_4 /* msa-extension-4 facility */
+#define FAC_MSA5        S390_FEAT_MSA_EXT_5 /* msa-extension-5 facility */
 
 static const DisasInsn insn_info[] = {
 #include "insn-data.def"
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 2/3] s390x/tcg: move wrap_address() to internal.h

[Philippe Mathieu-Daudé]

On 09/19/2017 11:26 AM, David Hildenbrand wrote:
> We want to use it in another file.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>   target/s390x/internal.h   | 14 ++++++++++++++
>   target/s390x/mem_helper.c | 14 --------------
>   2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/target/s390x/internal.h b/target/s390x/internal.h
> index bc8f83129a..70d2b87e55 100644
> --- a/target/s390x/internal.h
> +++ b/target/s390x/internal.h
> @@ -162,6 +162,20 @@ static inline uint8_t get_per_atmid(CPUS390XState *env)
>              ((env->psw.mask & PSW_ASC_ACCREG) ?    (1 << 2) : 0);
>   }
>   
> +static inline uint64_t wrap_address(CPUS390XState *env, uint64_t a)
> +{
> +    if (!(env->psw.mask & PSW_MASK_64)) {
> +        if (!(env->psw.mask & PSW_MASK_32)) {
> +            /* 24-Bit mode */
> +            a &= 0x00ffffff;
> +        } else {
> +            /* 31-Bit mode */
> +            a &= 0x7fffffff;
> +        }
> +    }
> +    return a;
> +}
> +
>   /* CC optimization */
>   
>   /* Instead of computing the condition codes after each x86 instruction,
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index ec4760e390..a254613dc2 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -122,20 +122,6 @@ static inline void cpu_stsize_data_ra(CPUS390XState *env, uint64_t addr,
>       }
>   }
>   
> -static inline uint64_t wrap_address(CPUS390XState *env, uint64_t a)
> -{
> -    if (!(env->psw.mask & PSW_MASK_64)) {
> -        if (!(env->psw.mask & PSW_MASK_32)) {
> -            /* 24-Bit mode */
> -            a &= 0x00ffffff;
> -        } else {
> -            /* 31-Bit mode */
> -            a &= 0x7fffffff;
> -        }
> -    }
> -    return a;
> -}
> -
>   static void fast_memset(CPUS390XState *env, uint64_t dest, uint8_t byte,
>                           uint32_t l, uintptr_t ra)
>   {
> 

Re: [Qemu-devel] [PATCH v2 2/3] s390x/tcg: move wrap_address() to internal.h

[Richard Henderson]

On 09/19/2017 09:26 AM, David Hildenbrand wrote:
> We want to use it in another file.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/internal.h   | 14 ++++++++++++++
>  target/s390x/mem_helper.c | 14 --------------
>  2 files changed, 14 insertions(+), 14 deletions(-)

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


r~

Re: [Qemu-devel] [PATCH v2 3/3] s390x/tcg: add basic MSA features

[Richard Henderson]

On 09/19/2017 09:26 AM, David Hildenbrand wrote:
> +    const uint8_t fc = env->regs[0] & 0x7fULL;

Don't mask here...

> +    if (fc >= 128 || !test_be_bit(fc, subfunc)) {
> +        cpu_restore_state(cs, ra);
> +        program_interrupt(env, PGM_SPECIFICATION, 4);
> +        return 0;
> +    }

... because you are indeed supposed to test bit 56.  But you've already ensured
that won't be set above.

Do you in fact need to set bit 63 in subfunc, since that itself is query?
Certainly you won't get past this test, regardless...


> +static ExitStatus op_msa(DisasContext *s, DisasOps *o)
> +{
> +    int r1 = have_field(s->fields, r1) ? get_field(s->fields, r1) : 0;
> +    int r2 = have_field(s->fields, r2) ? get_field(s->fields, r2) : 0;
> +    int r3 = have_field(s->fields, r3) ? get_field(s->fields, r3) : 0;
> +    TCGv_i32 t_r1, t_r2, t_r3, type;
> +
> +    switch (s->insn->data) {
> +    case S390_FEAT_TYPE_KMCTR:
> +        if (r3 & 1 || !r3) {
> +            gen_program_exception(s, PGM_SPECIFICATION);
> +            return EXIT_NORETURN;
> +        }
> +        /* FALL THROUGH */
> +    case S390_FEAT_TYPE_PPNO:
> +    case S390_FEAT_TYPE_KMF:
> +    case S390_FEAT_TYPE_KMC:
> +    case S390_FEAT_TYPE_KMO:
> +    case S390_FEAT_TYPE_KM:
> +        if (r1 & 1 || !r1) {
> +            gen_program_exception(s, PGM_SPECIFICATION);
> +            return EXIT_NORETURN;
> +        }
> +        /* FALL THROUGH */
> +    case S390_FEAT_TYPE_KMAC:
> +    case S390_FEAT_TYPE_KIMD:
> +    case S390_FEAT_TYPE_KLMD:
> +        if (r2 & 1 || !r2) {
> +            gen_program_exception(s, PGM_SPECIFICATION);
> +            return EXIT_NORETURN;
> +        }

Even though it would be silly to do so, considering that r0+r1 are implicit
arguments to these insns, I see nothing that insists that r[123] are not 0,
only that they are even.


r~

Re: [Qemu-devel] [PATCH v2 3/3] s390x/tcg: add basic MSA features

[David Hildenbrand]

On 19.09.2017 19:47, Richard Henderson wrote:
> On 09/19/2017 09:26 AM, David Hildenbrand wrote:
>> +    const uint8_t fc = env->regs[0] & 0x7fULL;
> 
> Don't mask here...

Bit 56 is the mod bit (see variable "mod") and is checked inside the
switch(). The function code really is just composed of bit 57 - 63, so
this is correct.

> 
>> +    if (fc >= 128 || !test_be_bit(fc, subfunc)) {

... however the fc >= 128 case can be dropped, as this can in fact never
happen (due to the masking).

>> +        cpu_restore_state(cs, ra);
>> +        program_interrupt(env, PGM_SPECIFICATION, 4);
>> +        return 0;
>> +    }
> 
> ... because you are indeed supposed to test bit 56.  But you've already ensured
> that won't be set above.
> 
> Do you in fact need to set bit 63 in subfunc, since that itself is query?
> Certainly you won't get past this test, regardless...

Yes, it always has to be set and is guaranteed by s390_get_feat_block().
So test_be_bit(fc, subfunc) will always allow bit 0 (query).

Especially due to the statement:

"When a bit is one, the corresponding function is installed; otherwise,
the function is not installed."

Query is always installed.


> 
> 
>> +static ExitStatus op_msa(DisasContext *s, DisasOps *o)
>> +{
>> +    int r1 = have_field(s->fields, r1) ? get_field(s->fields, r1) : 0;
>> +    int r2 = have_field(s->fields, r2) ? get_field(s->fields, r2) : 0;
>> +    int r3 = have_field(s->fields, r3) ? get_field(s->fields, r3) : 0;
>> +    TCGv_i32 t_r1, t_r2, t_r3, type;
>> +
>> +    switch (s->insn->data) {
>> +    case S390_FEAT_TYPE_KMCTR:
>> +        if (r3 & 1 || !r3) {
>> +            gen_program_exception(s, PGM_SPECIFICATION);
>> +            return EXIT_NORETURN;
>> +        }
>> +        /* FALL THROUGH */
>> +    case S390_FEAT_TYPE_PPNO:
>> +    case S390_FEAT_TYPE_KMF:
>> +    case S390_FEAT_TYPE_KMC:
>> +    case S390_FEAT_TYPE_KMO:
>> +    case S390_FEAT_TYPE_KM:
>> +        if (r1 & 1 || !r1) {
>> +            gen_program_exception(s, PGM_SPECIFICATION);
>> +            return EXIT_NORETURN;
>> +        }
>> +        /* FALL THROUGH */
>> +    case S390_FEAT_TYPE_KMAC:
>> +    case S390_FEAT_TYPE_KIMD:
>> +    case S390_FEAT_TYPE_KLMD:
>> +        if (r2 & 1 || !r2) {
>> +            gen_program_exception(s, PGM_SPECIFICATION);
>> +            return EXIT_NORETURN;
>> +        }
> 
> Even though it would be silly to do so, considering that r0+r1 are implicit
> arguments to these insns, I see nothing that insists that r[123] are not 0,
> only that they are even.
> 

E.g. for KMAC (7-187):

"A specification exception is recognized and no other
action is taken if any of the following occurs:
[...]
 The R2 field designates an odd-numbered regis-
ter or general register 0."

Or KMCTR (7-103):

"A specification exception is recognized and no other
action is taken if any of the following occurs:
[...]
 The R1 R2, or R3 field designates an odd-num-
bered register or general register 0."


I implemented kvm-unit-tests that test all of these conditions.
Especially they also pass on KVM, so I assume they are correct (as these
instructions are fully interpreted by HW).

https://www.spinics.net/lists/kvm/msg155690.html

In the test included is
a) checking that the mod bit must not be set for certain instructions
b) r1-3 either odd or 0 for certain instructions
c) that query is always indicated


Thanks for the review!

> 
> r~
> 


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v2 3/3] s390x/tcg: add basic MSA features

[Richard Henderson]

On 09/19/2017 02:36 PM, David Hildenbrand wrote:
> On 19.09.2017 19:47, Richard Henderson wrote:
>> On 09/19/2017 09:26 AM, David Hildenbrand wrote:
>>> +    const uint8_t fc = env->regs[0] & 0x7fULL;
>>
>> Don't mask here...
> 
> Bit 56 is the mod bit (see variable "mod") and is checked inside the
> switch(). The function code really is just composed of bit 57 - 63, so
> this is correct.

"mod bit"?

  # Bit 56 of general register 0 must be zero; oth-
  # erwise, a specification exception is recognized.

I don't suppose this is a change in PoO v11?  I'm still looking at v10.

> E.g. for KMAC (7-187):
> 
> "A specification exception is recognized and no other
> action is taken if any of the following occurs:
> [...]
>  The R2 field designates an odd-numbered regis-
> ter or general register 0."

Hmm, ok.  There's different text earlier on page 7-178 that describes the
even-odd requirement but omits the non-zero requirement.  Irritating.


r~

Re: [Qemu-devel] [PATCH v2 3/3] s390x/tcg: add basic MSA features

[David Hildenbrand]

On 20.09.2017 15:23, Richard Henderson wrote:
> On 09/19/2017 02:36 PM, David Hildenbrand wrote:
>> On 19.09.2017 19:47, Richard Henderson wrote:
>>> On 09/19/2017 09:26 AM, David Hildenbrand wrote:
>>>> +    const uint8_t fc = env->regs[0] & 0x7fULL;
>>>
>>> Don't mask here...
>>
>> Bit 56 is the mod bit (see variable "mod") and is checked inside the
>> switch(). The function code really is just composed of bit 57 - 63, so
>> this is correct.
> 
> "mod bit"?
> 
>   # Bit 56 of general register 0 must be zero; oth-
>   # erwise, a specification exception is recognized.
> 
> I don't suppose this is a change in PoO v11?  I'm still looking at v10.
> 

I'm also looking at v10.

This bit is called "M" bit for all cpacf instructions that allow
en/decryption. M == modifier bit == mod bit == bit 56

e.g. for KM (7-56)

"When the modifier bit in general register 0 is one, a
decipher operation is performed. ...".

For instructions that don't allow switching between de/encryption (e.g.
KLMD), the modifier bit a.k.a. bit 56 must not be set.

I don't really see the need here to rename mod -> bit_56 here.

Thank!

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v2 0/3] Implement basic MSA functions

[Cornelia Huck]

On Tue, 19 Sep 2017 16:26:51 +0200
David Hildenbrand <david@redhat.com> wrote:

> Some leftover from "target/s390x: tcg improvments + MSA functions".
> 
> Implement all basic MSA (cpacf/crypto) instructions <= z13. Only provide
> the query subfunction (to query available subfunctions), no actual
> de/encryption yet. Good enough to unlock the STFL bits.
> 
> I have written kvm-unit-tests for MSA functions and for SPM/IPM. Will
> send them out soon. I use the following cpu model to test with an upstream
> kernel compiled for z10:
> 
> ... -cpu qemu,mvcos=on,stfle=on,ldisp=on,ldisphp=on,\
>               eimm=on,stckf=on,csst=on,csst2=on,ginste=on,\
>               exrl=on,msa-base=on,msa3-base=on,msa4-base=on,msa5-base=on ...
> 
> Based on: https://github.com/cohuck/qemu.git s390-next
> Available on: https://github.com/davidhildenbrand/qemu.git s390x-msa

Do I understand correctly that you'll send a v3 with changes to patch 3?

If yes, I'll just wait and pick that.

Re: [Qemu-devel] [PATCH v2 0/3] Implement basic MSA functions

[David Hildenbrand]

On 20.09.2017 17:14, Cornelia Huck wrote:
> On Tue, 19 Sep 2017 16:26:51 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Some leftover from "target/s390x: tcg improvments + MSA functions".
>>
>> Implement all basic MSA (cpacf/crypto) instructions <= z13. Only provide
>> the query subfunction (to query available subfunctions), no actual
>> de/encryption yet. Good enough to unlock the STFL bits.
>>
>> I have written kvm-unit-tests for MSA functions and for SPM/IPM. Will
>> send them out soon. I use the following cpu model to test with an upstream
>> kernel compiled for z10:
>>
>> ... -cpu qemu,mvcos=on,stfle=on,ldisp=on,ldisphp=on,\
>>               eimm=on,stckf=on,csst=on,csst2=on,ginste=on,\
>>               exrl=on,msa-base=on,msa3-base=on,msa4-base=on,msa5-base=on ...
>>
>> Based on: https://github.com/cohuck/qemu.git s390-next
>> Available on: https://github.com/davidhildenbrand/qemu.git s390x-msa
> 
> Do I understand correctly that you'll send a v3 with changes to patch 3?
> 
> If yes, I'll just wait and pick that.
> 

It is really just dropping one superfluous check in patch nr 3.

So you can either fix that up or I can resend, whatever you prefer.

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v2 0/3] Implement basic MSA functions

[Cornelia Huck]

On Wed, 20 Sep 2017 17:17:33 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 20.09.2017 17:14, Cornelia Huck wrote:
> > On Tue, 19 Sep 2017 16:26:51 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> Some leftover from "target/s390x: tcg improvments + MSA functions".
> >>
> >> Implement all basic MSA (cpacf/crypto) instructions <= z13. Only provide
> >> the query subfunction (to query available subfunctions), no actual
> >> de/encryption yet. Good enough to unlock the STFL bits.
> >>
> >> I have written kvm-unit-tests for MSA functions and for SPM/IPM. Will
> >> send them out soon. I use the following cpu model to test with an upstream
> >> kernel compiled for z10:
> >>
> >> ... -cpu qemu,mvcos=on,stfle=on,ldisp=on,ldisphp=on,\
> >>               eimm=on,stckf=on,csst=on,csst2=on,ginste=on,\
> >>               exrl=on,msa-base=on,msa3-base=on,msa4-base=on,msa5-base=on ...
> >>
> >> Based on: https://github.com/cohuck/qemu.git s390-next
> >> Available on: https://github.com/davidhildenbrand/qemu.git s390x-msa  
> > 
> > Do I understand correctly that you'll send a v3 with changes to patch 3?
> > 
> > If yes, I'll just wait and pick that.
> >   
> 
> It is really just dropping one superfluous check in patch nr 3.
> 
> So you can either fix that up or I can resend, whatever you prefer.
> 

Please just resend before I start mucking around in patches again.