[Qemu-devel] [PATCH 0/9] target-s390 tcg improvements

[Qemu-devel] [PATCH 0/9] target-s390 tcg improvements

[Richard Henderson]

With this patch set we can boot the fedora 19 kernel, and make
it all the way to /bin/init.  At which point the process either
hangs or crashes; in either case the kernel winds up with no
runnable processes and spends its time in the idle loop.

The choice of z9-109 for the facilities is because that appears
to be what fedora 19 is targeting as the minimum.

That said, a debian install can make it all the way through to
completion, so the fedora crash/hang must be related to something
in the extra z9-109 insns.



r~


Richard Henderson (10):
  target-s390: Move facilities bits to env
  target-s390: Implement STFLE
  target-s390: Add facilities bits and sets
  target-s390: Raise OPERATION exception for disabled insns
  target-s390: Implement SAM31 and SAM64
  target-s390: Implement EPSW
  target-s390: Fix STIDP
  target-s390: Fix STURA
  target-s390: Implement LURA, LURAG, STURG
  target-s390: Implement ECAG

 target-s390x/cpu.c         |  78 ++++++++++++++++++++++
 target-s390x/cpu.h         |  74 ++++++++++++++++++++-
 target-s390x/helper.h      |   4 ++
 target-s390x/insn-data.def |  18 +++--
 target-s390x/mem_helper.c  |  18 ++++-
 target-s390x/misc_helper.c |  13 ++++
 target-s390x/translate.c   | 161 ++++++++++++++++++++++++++++++++++++---------
 7 files changed, 329 insertions(+), 37 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 01/10] target-s390: Move facilities bits to env

[Richard Henderson]

Rather than simply hard-coding them in STFL instruction.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-s390x/cpu.c       |  3 +++
 target-s390x/cpu.h       |  1 +
 target-s390x/translate.c | 10 +++++-----
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 3c89f8a..ff691df 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -181,6 +181,9 @@ static void s390_cpu_initfn(Object *obj)
     env->cpu_num = cpu_num++;
     env->ext_index = -1;
 
+    env->facilities[0] = 0xc000000000000000ull;
+    env->facilities[1] = 0;
+
     if (tcg_enabled() && !inited) {
         inited = true;
         s390x_translate_init();
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 8be5648..746aec8 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -136,6 +136,7 @@ typedef struct CPUS390XState {
     CPU_COMMON
 
     /* reset does memset(0) up to here */
+    uint64_t facilities[2];
 
     int cpu_num;
     uint8_t *storage_keys;
diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index afe90eb..d4dc8ea 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -3230,12 +3230,12 @@ static ExitStatus op_spt(DisasContext *s, DisasOps *o)
 
 static ExitStatus op_stfl(DisasContext *s, DisasOps *o)
 {
-    TCGv_i64 f, a;
-    /* We really ought to have more complete indication of facilities
-       that we implement.  Address this when STFLE is implemented.  */
+    TCGv_i64 f = tcg_temp_new_i64();
+    TCGv_i64 a = tcg_const_i64(200);
+
     check_privileged(s);
-    f = tcg_const_i64(0xc0000000);
-    a = tcg_const_i64(200);
+    tcg_gen_ld_i64(f, cpu_env, offsetof(CPUS390XState, facilities[0]));
+    tcg_gen_shri_i64(f, f, 32);
     tcg_gen_qemu_st32(f, a, get_mem_index(s));
     tcg_temp_free_i64(f);
     tcg_temp_free_i64(a);
-- 
1.8.1.4

[Qemu-devel] [PATCH 02/10] target-s390: Implement STFLE

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-s390x/helper.h      |  1 +
 target-s390x/insn-data.def |  2 ++
 target-s390x/misc_helper.c | 13 +++++++++++++
 target-s390x/translate.c   |  8 ++++++++
 4 files changed, 24 insertions(+)

diff --git a/target-s390x/helper.h b/target-s390x/helper.h
index 0d80aa0..547b46a 100644
--- a/target-s390x/helper.h
+++ b/target-s390x/helper.h
@@ -85,6 +85,7 @@ DEF_HELPER_FLAGS_5(calc_cc, TCG_CALL_NO_RWG_SE, i32, env, i32, i64, i64, i64)
 DEF_HELPER_FLAGS_2(sfpc, TCG_CALL_NO_RWG, void, env, i64)
 DEF_HELPER_FLAGS_2(sfas, TCG_CALL_NO_WG, void, env, i64)
 DEF_HELPER_FLAGS_1(popcnt, TCG_CALL_NO_RWG_SE, i64, i64)
+DEF_HELPER_2(stfle, i32, env, i64)
 
 #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 b42ebb6..4b462d4 100644
--- a/target-s390x/insn-data.def
+++ b/target-s390x/insn-data.def
@@ -639,6 +639,8 @@
     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 FACILITY LIST EXTENDED */
+    C(0xb2b0, STFLE,   S,  SFLE,   0, a2, 0, 0, stfle, 0)
 /* STORE FPC */
     C(0xb29c, STFPC,   S,     Z,   0, a2, new, m2_32, efpc, 0)
 
diff --git a/target-s390x/misc_helper.c b/target-s390x/misc_helper.c
index 1690907..b6ee429 100644
--- a/target-s390x/misc_helper.c
+++ b/target-s390x/misc_helper.c
@@ -518,3 +518,16 @@ uint32_t HELPER(sigp)(CPUS390XState *env, uint64_t order_code, uint32_t r1,
     return cc;
 }
 #endif
+
+uint32_t HELPER(stfle)(CPUS390XState *env, uint64_t addr)
+{
+    int max_m1 = (env->facilities[1] != 0);
+    int count_m1 = env->regs[0] & 0xff;
+
+    cpu_stq_data(env, addr, env->facilities[0]);
+    if (count_m1 > 0) {
+        cpu_stq_data(env, addr + 8, env->facilities[1]);
+    }
+    env->regs[0] = max_m1;
+    return (count_m1 >= max_m1 ? 0 : 3);
+}
diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index d4dc8ea..690b4a0 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -3314,6 +3314,14 @@ static ExitStatus op_stura(DisasContext *s, DisasOps *o)
 }
 #endif
 
+static ExitStatus op_stfle(DisasContext *s, DisasOps *o)
+{
+    potential_page_fault(s);
+    gen_helper_stfle(cc_op, cpu_env, o->in2);
+    set_cc_static(s);
+    return NO_EXIT;
+}
+
 static ExitStatus op_st8(DisasContext *s, DisasOps *o)
 {
     tcg_gen_qemu_st8(o->in1, o->in2, get_mem_index(s));
-- 
1.8.1.4

[Qemu-devel] [PATCH 03/10] target-s390: Add facilities bits and sets

[Richard Henderson]

Name the facilities bits, collect the set of bits for tcg and the various
real processor revisions.  Update the set of facilities reported for TCG.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-s390x/cpu.c       | 75 +++++++++++++++++++++++++++++++++++++++++++++++-
 target-s390x/cpu.h       | 59 +++++++++++++++++++++++++++++++++++++
 target-s390x/translate.c | 49 ++++++++++++++++---------------
 3 files changed, 157 insertions(+), 26 deletions(-)

diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index ff691df..01ff49b 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -152,6 +152,70 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     scc->parent_realize(dev, errp);
 }
 
+#define FAC0_TCG \
+    ( FAC_BIT(0, FAC_N3) \
+    | FAC_BIT(0, FAC_ZARCH) \
+    | FAC_BIT(0, FAC_ZARCH_ACTIVE) \
+    | FAC_BIT(0, FAC_STFLE) \
+    | FAC_BIT(0, FAC_LONG_DISPLACEMENT) \
+    | FAC_BIT(0, FAC_LONG_DISPLACEMENT_FAST) \
+    | FAC_BIT(0, FAC_EXTENDED_IMMEDIATE) \
+    | FAC_BIT(0, FAC_GENERAL_INSTRUCTIONS_EXT) \
+    | FAC_BIT(0, FAC_FLOATING_POINT_EXT) \
+    | FAC_BIT(0, FAC_FLOATING_POINT_SUPPPORT_ENH))
+    /* ??? We may have most of the collection of facilities at bit 45.  */
+
+/* ??? These lists of facilities gleaned from arch/s390/kernel/head.S.  */
+#define FAC0_Z900 \
+    ( FAC_BIT(0, FAC_N3) \
+    | FAC_BIT(0, FAC_ZARCH) \
+    | FAC_BIT(0, FAC_ZARCH_ACTIVE))
+
+#define FAC0_Z990 \
+    ( FAC0_Z900 \
+    | FAC_BIT(0, FAC_LONG_DISPLACEMENT) \
+    | FAC_BIT(0, FAC_LONG_DISPLACEMENT_FAST))
+
+#define FAC0_Z9_109 \
+    ( FAC0_Z990 \
+    | FAC_BIT(0, FAC_STFLE) \
+    | FAC_BIT(0, FAC_EXTENDED_TRANSLATION_2) \
+    | FAC_BIT(0, FAC_MESSAGE_SECURITY_ASSIST) \
+    | FAC_BIT(0, FAC_LONG_DISPLACEMENT) \
+    | FAC_BIT(0, FAC_LONG_DISPLACEMENT_FAST) \
+    | FAC_BIT(0, FAC_HFP_MADDSUB) \
+    | FAC_BIT(0, FAC_EXTENDED_IMMEDIATE) \
+    | FAC_BIT(0, FAC_EXTENDED_TRANSLATION_3) \
+    | FAC_BIT(0, FAC_HFP_UNNORMALIZED_EXT) \
+    | FAC_BIT(0, FAC_ETF2_ENH) \
+    | FAC_BIT(0, FAC_STORE_CLOCK_FAST) \
+    | FAC_BIT(0, FAC_ETF3_ENH) \
+    | FAC_BIT(0, FAC_EXTRACT_CPU_TIME))
+
+#define FAC0_Z10 \
+    ( FAC0_Z9_109 \
+    | FAC_BIT(0, FAC_COMPARE_AND_SWAP_AND_STORE) \
+    | FAC_BIT(0, FAC_COMPARE_AND_SWAP_AND_STORE_2) \
+    | FAC_BIT(0, FAC_GENERAL_INSTRUCTIONS_EXT) \
+    | FAC_BIT(0, FAC_EXECUTE_EXT) \
+    | FAC_BIT(0, FAC_FLOATING_POINT_SUPPPORT_ENH) \
+    | FAC_BIT(0, FAC_DFP) \
+    | FAC_BIT(0, FAC_PFPO))
+
+#define FAC0_Z196 \
+    ( FAC0_Z10 \
+    | FAC_BIT(0, FAC_FLOATING_POINT_EXT) \
+    | FAC_BIT(0, FAC_MULTI_45))
+
+#define FAC0_ZEC12 \
+    ( FAC0_Z196 \
+    | FAC_BIT(0, FAC_DFP_ZONED_CONVERSION) \
+    | FAC_BIT(0, FAC_MULTI_49) \
+    | FAC_BIT(0, FAC_CONSTRAINT_TRANSACTIONAL_EXE))
+#define FAC1_ZEC12 \
+    FAC_BIT(1, FAC_TRANSACTIONAL_EXE)
+
+
 static void s390_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -181,9 +245,18 @@ static void s390_cpu_initfn(Object *obj)
     env->cpu_num = cpu_num++;
     env->ext_index = -1;
 
-    env->facilities[0] = 0xc000000000000000ull;
+    env->facilities[0] = FAC0_TCG;
     env->facilities[1] = 0;
 
+    /* ??? Current distros are targeting Z9-109 as the minimum.  TCG
+       supports most of the Z9-109 facilities but not all.  Sadly, the
+       kernel checks for facilities it doesn't actually need, minor stuff
+       like hex floating point and translation.  For now, include all
+       that the kernel requires we support.  */
+#ifndef CONFIG_USER_ONLY
+    env->facilities[0] |= FAC0_Z9_109;
+#endif
+
     if (tcg_enabled() && !inited) {
         inited = true;
         s390x_translate_init();
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 746aec8..a0bafef 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -83,6 +83,65 @@ typedef struct MchkQueue {
 #define KVM_S390_RUNTIME_DIRTY_PARTIAL  1
 #define KVM_S390_RUNTIME_DIRTY_FULL     2
 
+typedef enum {
+    FAC_N3                              = 0,
+    FAC_ZARCH                           = 1,
+    FAC_ZARCH_ACTIVE                    = 2,
+    FAC_DAT_ENH                         = 3,
+    FAC_ASN_LX_REUSE                    = 6,
+    FAC_STFLE                           = 7,
+    FAC_ENHANCED_DAT_1                  = 8,
+    FAC_SENSE_RUNNING_STATUS            = 9,
+    FAC_CONDITIONAL_SSKE                = 10,
+    FAC_CONFIGURATION_TOPOLOGY          = 11,
+    FAC_IPTE_RANGE                      = 13,
+    FAC_NONQ_KEY_SETTING                = 14,
+    FAC_EXTENDED_TRANSLATION_2          = 16,
+    FAC_MESSAGE_SECURITY_ASSIST         = 17,
+    FAC_LONG_DISPLACEMENT               = 18,
+    FAC_LONG_DISPLACEMENT_FAST          = 19,
+    FAC_HFP_MADDSUB                     = 20,
+    FAC_EXTENDED_IMMEDIATE              = 21,
+    FAC_EXTENDED_TRANSLATION_3          = 22,
+    FAC_HFP_UNNORMALIZED_EXT            = 23,
+    FAC_ETF2_ENH                        = 24,
+    FAC_STORE_CLOCK_FAST                = 25,
+    FAC_PARSING_ENH                     = 26,
+    FAC_MOVE_WITH_OPTIONAL_SPEC         = 27,
+    FAC_TOD_CLOCK_STEERING              = 28,
+    FAC_ETF3_ENH                        = 30,
+    FAC_EXTRACT_CPU_TIME                = 31,
+    FAC_COMPARE_AND_SWAP_AND_STORE      = 32,
+    FAC_COMPARE_AND_SWAP_AND_STORE_2    = 33,
+    FAC_GENERAL_INSTRUCTIONS_EXT        = 34,
+    FAC_EXECUTE_EXT                     = 35,
+    FAC_ENHANCED_MONITOR                = 36,
+    FAC_FLOATING_POINT_EXT              = 37,
+    FAC_SET_PROGRAM_PARAMETERS          = 40,
+    FAC_FLOATING_POINT_SUPPPORT_ENH     = 41,
+    FAC_DFP                             = 42,
+    FAC_DFP_FAST                        = 43,
+    FAC_PFPO                            = 44,
+    FAC_MULTI_45                        = 45,
+    FAC_CMPSC_ENH                       = 47,
+    FAC_DFP_ZONED_CONVERSION            = 48,
+    FAC_MULTI_49                        = 49,
+    FAC_CONSTRAINT_TRANSACTIONAL_EXE    = 50,
+    FAC_LOCAL_TLB_CLEARING              = 51,
+    FAC_INTERLOCKED_ACCESS_2            = 52,
+    FAC_RESET_REFERENCE_BITS_MULTIPLE   = 66,
+    FAC_CPU_MEASUREMENT_COUNTER         = 67,
+    FAC_CPU_MEASUREMENT_SAMPLING        = 68,
+    FAC_TRANSACTIONAL_EXE               = 73,
+    FAC_ACCESS_EXCEPTION_FS_INDICATION  = 75,
+    FAC_MESSAGE_SECURITY_ASSIST_3       = 76,
+    FAC_MESSAGE_SECURITY_ASSIST_4       = 77,
+    FAC_ENHANCED_DAT_2                  = 78,
+} S390Facility;
+
+#define FAC_BIT(WORD, BIT)  (BIT / 64 == WORD ? 1ull << (63 - BIT % 64) : 0)
+
+
 typedef struct CPUS390XState {
     uint64_t regs[16];     /* GP registers */
     CPU_DoubleU fregs[16]; /* FP registers */
diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 690b4a0..5aac7b0 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -1102,33 +1102,10 @@ typedef enum {
     EXIT_NORETURN,
 } ExitStatus;
 
-typedef enum DisasFacility {
-    FAC_Z,                  /* zarch (default) */
-    FAC_CASS,               /* compare and swap and store */
-    FAC_CASS2,              /* compare and swap and store 2*/
-    FAC_DFP,                /* decimal floating point */
-    FAC_DFPR,               /* decimal floating point rounding */
-    FAC_DO,                 /* distinct operands */
-    FAC_EE,                 /* execute extensions */
-    FAC_EI,                 /* extended immediate */
-    FAC_FPE,                /* floating point extension */
-    FAC_FPSSH,              /* floating point support sign handling */
-    FAC_FPRGR,              /* FPR-GR transfer */
-    FAC_GIE,                /* general instructions extension */
-    FAC_HFP_MA,             /* HFP multiply-and-add/subtract */
-    FAC_HW,                 /* high-word */
-    FAC_IEEEE_SIM,          /* IEEE exception sumilation */
-    FAC_LOC,                /* load/store on condition */
-    FAC_LD,                 /* long displacement */
-    FAC_PC,                 /* population count */
-    FAC_SCF,                /* store clock fast */
-    FAC_SFLE,               /* store facility list extended */
-} DisasFacility;
-
 struct DisasInsn {
     unsigned opc:16;
     DisasFormat fmt:8;
-    DisasFacility fac:8;
+    S390Facility fac:8;
     unsigned spec:8;
 
     const char *name;
@@ -4442,6 +4419,28 @@ static void in2_i2_32u_shl(DisasContext *s, DisasFields *f, DisasOps *o)
 
 /* ====================================================================== */
 
+/* Abbreviations for facilities used in the table.  */
+#define DFAC_Z               FAC_ZARCH
+#define DFAC_CASS            FAC_COMPARE_AND_SWAP_AND_STORE
+#define DFAC_CASS2           FAC_COMPARE_AND_SWAP_AND_STORE_2
+#define DFAC_DFP             FAC_DFP
+#define DFAC_DFPR            FAC_FLOATING_POINT_SUPPPORT_ENH
+#define DFAC_DO              FAC_MULTI_45
+#define DFAC_EE              FAC_EXECUTE_EXT
+#define DFAC_EI              FAC_EXTENDED_IMMEDIATE
+#define DFAC_FPE             FAC_FLOATING_POINT_EXT
+#define DFAC_FPSSH           FAC_FLOATING_POINT_SUPPPORT_ENH
+#define DFAC_FPRGR           FAC_FLOATING_POINT_SUPPPORT_ENH
+#define DFAC_GIE             FAC_GENERAL_INSTRUCTIONS_EXT
+#define DFAC_HFP_MA          FAC_HFP_MADDSUB
+#define DFAC_HW              FAC_MULTI_45
+#define DFAC_IEEEE_SIM       FAC_FLOATING_POINT_SUPPPORT_ENH
+#define DFAC_LOC             FAC_MULTI_45
+#define DFAC_LD              FAC_LONG_DISPLACEMENT
+#define DFAC_PC              FAC_MULTI_45
+#define DFAC_SCF             FAC_STORE_CLOCK_FAST
+#define DFAC_SFLE            FAC_STFLE
+
 /* Find opc within the table of insns.  This is formulated as a switch
    statement so that (1) we get compile-time notice of cut-paste errors
    for duplicated opcodes, and (2) the compiler generates the binary
@@ -4460,7 +4459,7 @@ enum DisasInsnEnum {
 #define D(OPC, NM, FT, FC, I1, I2, P, W, OP, CC, D) {                       \
     .opc = OPC,                                                             \
     .fmt = FMT_##FT,                                                        \
-    .fac = FAC_##FC,                                                        \
+    .fac = DFAC_##FC,                                                        \
     .spec = SPEC_in1_##I1 | SPEC_in2_##I2 | SPEC_prep_##P | SPEC_wout_##W,  \
     .name = #NM,                                                            \
     .help_in1 = in1_##I1,                                                   \
-- 
1.8.1.4

[Qemu-devel] [PATCH 04/10] target-s390: Raise OPERATION exception for disabled insns

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-s390x/translate.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 5aac7b0..c8bbedb 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -53,6 +53,7 @@ struct DisasContext {
     const DisasInsn *insn;
     DisasFields *fields;
     uint64_t pc, next_pc;
+    uint64_t fac0;
     enum cc_op cc_op;
     bool singlestep_enabled;
 };
@@ -4651,6 +4652,16 @@ static ExitStatus translate_one(CPUS390XState *env, DisasContext *s)
         return EXIT_NORETURN;
     }
 
+    /* Check for operation exceptions for insns that have been disabled.
+       Do this by shifting the facilities word0 up by the IBM big-endian
+       bit numbering, leaving the bit to be tested in the sign bit.
+       Note that TCG does not currently support any facilities in word1.  */
+    assert(insn->fac < 64);
+    if ((int64_t)(s->fac0 << insn->fac) >= 0) {
+        gen_program_exception(s, PGM_OPERATION);
+        return EXIT_NORETURN;
+    }
+
     /* Check for insn specification exceptions.  */
     if (insn->spec) {
         int spec = insn->spec, excp = 0, r;
@@ -4769,6 +4780,7 @@ static inline void gen_intermediate_code_internal(S390CPU *cpu,
     dc.tb = tb;
     dc.pc = pc_start;
     dc.cc_op = CC_OP_DYNAMIC;
+    dc.fac0 = env->facilities[0];
     do_debug = dc.singlestep_enabled = cs->singlestep_enabled;
 
     gen_opc_end = tcg_ctx.gen_opc_buf + OPC_MAX_SIZE;
-- 
1.8.1.4

[Qemu-devel] [PATCH 05/10] target-s390: Implement SAM31 and SAM64

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-s390x/insn-data.def |  8 ++++----
 target-s390x/translate.c   | 29 +++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/target-s390x/insn-data.def b/target-s390x/insn-data.def
index 4b462d4..c528eb4 100644
--- a/target-s390x/insn-data.def
+++ b/target-s390x/insn-data.def
@@ -566,6 +566,10 @@
 
 /* SET ACCESS */
     C(0xb24e, SAR,     RRE,   Z,   0, r2_o, 0, 0, sar, 0)
+/* SET ADDRESSING MODE */
+    /* We only do 32 and 64-bit.  Let SAM24 signal illegal instruction.  */
+    C(0x010d, SAM31,   E,     Z,   0, 0, 0, 0, sam31, 0)
+    C(0x010e, SAM64,   E,     Z,   0, 0, 0, 0, sam64, 0)
 /* SET FPC */
     C(0xb384, SFPC,    RRE,   Z,   0, r1_o, 0, 0, sfpc, 0)
 /* SET FPC AND SIGNAL */
@@ -745,10 +749,6 @@
     C(0xb22a, RRBE,    RRE,   Z,   0, r2_o, 0, 0, rrbe, 0)
 /* SERVICE CALL LOGICAL PROCESSOR (PV hypercall) */
     C(0xb220, SERVC,   RRE,   Z,   r1_o, r2_o, 0, 0, servc, 0)
-/* SET ADDRESSING MODE */
-    /* We only do 64-bit, so accept this as a no-op.
-       Let SAM24 and SAM31 signal illegal instruction.  */
-    C(0x010e, SAM64,   E,     Z,   0, 0, 0, 0, 0, 0)
 /* SET ADDRESS SPACE CONTROL FAST */
     C(0xb279, SACF,    S,     Z,   0, a2, 0, 0, sacf, 0)
 /* SET CLOCK */
diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index c8bbedb..6ca8f0b 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2912,6 +2912,35 @@ static ExitStatus op_sacf(DisasContext *s, DisasOps *o)
 }
 #endif
 
+static ExitStatus op_sam31(DisasContext *s, DisasOps *o)
+{
+    /* Bizzare but true, we check the address of the current insn for the
+       specification exception, not the next to be executed.  Thus the PoO
+       documents that Bad Things Happen at 0x7ffffffe.  */
+    if (s->pc & ~0x7ffffff) {
+        gen_program_exception(s, PGM_SPECIFICATION);
+        return EXIT_NORETURN;
+    }
+
+    tcg_gen_andi_i64(psw_mask, psw_mask, ~PSW_MASK_64);
+    tcg_gen_ori_i64(psw_mask, psw_mask, PSW_MASK_32);
+
+    /* Always exit the TB, since we (may have) changed execution mode.  */
+    s->pc = s->next_pc & 0x7fffffff;
+    update_psw_addr(s);
+    return EXIT_PC_UPDATED;
+}
+
+static ExitStatus op_sam64(DisasContext *s, DisasOps *o)
+{
+    tcg_gen_ori_i64(psw_mask, psw_mask, PSW_MASK_32 | PSW_MASK_64);
+
+    /* Always exit the TB, since we (may have) changed execution mode.  */
+    s->pc = s->next_pc;
+    update_psw_addr(s);
+    return EXIT_PC_UPDATED;
+}
+
 static ExitStatus op_sar(DisasContext *s, DisasOps *o)
 {
     int r1 = get_field(s->fields, r1);
-- 
1.8.1.4

[Qemu-devel] [PATCH 06/10] target-s390: Implement EPSW

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-s390x/insn-data.def |  2 ++
 target-s390x/translate.c   | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/target-s390x/insn-data.def b/target-s390x/insn-data.def
index c528eb4..48850ff 100644
--- a/target-s390x/insn-data.def
+++ b/target-s390x/insn-data.def
@@ -287,6 +287,8 @@
     C(0xb24f, EAR,     RRE,   Z,   0, 0, new, r1_32, ear, 0)
 /* EXTRACT FPC */
     C(0xb38c, EFPC,    RRE,   Z,   0, 0, new, r1_32, efpc, 0)
+/* EXTRACT PSW */
+    C(0xb98d, EPSW,    RRE,   Z,   0, 0, 0, 0, epsw, 0)
 
 /* FIND LEFTMOST ONE */
     C(0xb983, FLOGR,   RRE,   EI,  0, r2_o, r1_P, 0, flogr, 0)
diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 6ca8f0b..192d54e 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2036,6 +2036,24 @@ static ExitStatus op_efpc(DisasContext *s, DisasOps *o)
     return NO_EXIT;
 }
 
+static ExitStatus op_epsw(DisasContext *s, DisasOps *o)
+{
+    int r1 = get_field(s->fields, r1);
+    int r2 = get_field(s->fields, r2);
+    TCGv_i64 t = tcg_temp_new_i64();
+
+    /* Note the "subsequently" in the PoO, which implies a defined result
+       if r1 == r2.  Thus we cannot defer these writes to an output hook.  */
+    tcg_gen_shri_i64(t, psw_mask, 32);
+    store_reg32_i64(r1, t);
+    if (r2 != 0) {
+        store_reg32_i64(r2, psw_mask);
+    }
+
+    tcg_temp_free_i64(t);
+    return NO_EXIT;
+}
+
 static ExitStatus op_ex(DisasContext *s, DisasOps *o)
 {
     /* ??? Perhaps a better way to implement EXECUTE is to set a bit in
-- 
1.8.1.4

[Qemu-devel] [PATCH 07/10] target-s390: Fix STIDP

[Richard Henderson]

The implementation had been incomplete, as we did not store the
machine type.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-s390x/cpu.c       |  2 ++
 target-s390x/cpu.h       | 14 +++++++++++++-
 target-s390x/translate.c |  2 +-
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 01ff49b..d003dcf 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -257,6 +257,8 @@ static void s390_cpu_initfn(Object *obj)
     env->facilities[0] |= FAC0_Z9_109;
 #endif
 
+    env->machine_type = 0x20940000;   /* ??? Also Z9-109.  */
+
     if (tcg_enabled() && !inited) {
         inited = true;
         s390x_translate_init();
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index a0bafef..95f9cab 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -197,7 +197,19 @@ typedef struct CPUS390XState {
     /* reset does memset(0) up to here */
     uint64_t facilities[2];
 
-    int cpu_num;
+    union {
+        uint64_t cpuid;
+        struct {
+#ifdef HOST_WORDS_BIGENDIAN
+            uint32_t cpu_num;
+            uint32_t machine_type;
+#else
+            uint32_t machine_type;
+            uint32_t cpu_num;
+#endif
+        };
+    };
+
     uint8_t *storage_keys;
 
     uint64_t tod_offset;
diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 192d54e..25a6537 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -3242,7 +3242,7 @@ static ExitStatus op_stctl(DisasContext *s, DisasOps *o)
 static ExitStatus op_stidp(DisasContext *s, DisasOps *o)
 {
     check_privileged(s);
-    tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, cpu_num));
+    tcg_gen_ld_i64(o->out, cpu_env, offsetof(CPUS390XState, cpuid));
     return NO_EXIT;
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 08/10] target-s390: Fix STURA

[Richard Henderson]

We were storing 16 bits instead of 32.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-s390x/mem_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-s390x/mem_helper.c b/target-s390x/mem_helper.c
index 1422ae9..408836c 100644
--- a/target-s390x/mem_helper.c
+++ b/target-s390x/mem_helper.c
@@ -1041,7 +1041,7 @@ void HELPER(ptlb)(CPUS390XState *env)
 /* store using real address */
 void HELPER(stura)(CPUS390XState *env, uint64_t addr, uint64_t v1)
 {
-    stw_phys(get_address(env, 0, 0, addr), (uint32_t)v1);
+    stl_phys(get_address(env, 0, 0, addr), (uint32_t)v1);
 }
 
 /* load real address */
-- 
1.8.1.4

[Qemu-devel] [PATCH 09/10] target-s390: Implement LURA, LURAG, STURG

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-s390x/helper.h      |  3 +++
 target-s390x/insn-data.def |  4 ++++
 target-s390x/mem_helper.c  | 16 ++++++++++++++++
 target-s390x/translate.c   | 26 ++++++++++++++++++++++++++
 4 files changed, 49 insertions(+)

diff --git a/target-s390x/helper.h b/target-s390x/helper.h
index 547b46a..20047d2 100644
--- a/target-s390x/helper.h
+++ b/target-s390x/helper.h
@@ -114,7 +114,10 @@ DEF_HELPER_FLAGS_2(sacf, TCG_CALL_NO_WG, void, env, i64)
 DEF_HELPER_FLAGS_3(ipte, TCG_CALL_NO_RWG, void, env, i64, i64)
 DEF_HELPER_FLAGS_1(ptlb, TCG_CALL_NO_RWG, void, env)
 DEF_HELPER_2(lra, i64, env, i64)
+DEF_HELPER_FLAGS_2(lura, TCG_CALL_NO_WG, i64, env, i64)
+DEF_HELPER_FLAGS_2(lurag, TCG_CALL_NO_WG, i64, env, i64)
 DEF_HELPER_FLAGS_3(stura, TCG_CALL_NO_WG, void, env, i64, i64)
+DEF_HELPER_FLAGS_3(sturg, TCG_CALL_NO_WG, void, env, i64, i64)
 #endif
 
 #include "exec/def-helper.h"
diff --git a/target-s390x/insn-data.def b/target-s390x/insn-data.def
index 48850ff..a405f64 100644
--- a/target-s390x/insn-data.def
+++ b/target-s390x/insn-data.def
@@ -741,6 +741,9 @@
     C(0xb100, LRA,     RX_a,  Z,   0, a2, r1, 0, lra, 0)
     C(0xe313, LRAY,    RXY_a, LD,  0, a2, r1, 0, lra, 0)
     C(0xe303, LRAG,    RXY_a, Z,   0, a2, r1, 0, lra, 0)
+/* LOAD USING REAL ADDRESS */
+    C(0xb24b, LURA,    RRE,   Z,   0, r2, new, r1_32, lura, 0)
+    C(0xb905, LURAG,   RRE,   Z,   0, r2, r1, 0, lurag, 0)
 /* MOVE TO PRIMARY */
     C(0xda00, MVCP,    SS_d,  Z,   la1, a2, 0, 0, mvcp, 0)
 /* MOVE TO SECONDARY */
@@ -798,6 +801,7 @@
     C(0xad00, STOSM,   SI,    Z,   la1, 0, 0, 0, stnosm, 0)
 /* STORE USING REAL ADDRESS */
     C(0xb246, STURA,   RRE,   Z,   r1_o, r2_o, 0, 0, stura, 0)
+    C(0xb925, STURG,   RRE,   Z,   r1_o, r2_o, 0, 0, sturg, 0)
 /* TEST PROTECTION */
     C(0xe501, TPROT,   SSE,   Z,   la1, a2, 0, 0, tprot, 0)
 
diff --git a/target-s390x/mem_helper.c b/target-s390x/mem_helper.c
index 408836c..9bd5e10 100644
--- a/target-s390x/mem_helper.c
+++ b/target-s390x/mem_helper.c
@@ -1038,12 +1038,28 @@ void HELPER(ptlb)(CPUS390XState *env)
     tlb_flush(env, 1);
 }
 
+/* load using real address */
+uint64_t HELPER(lura)(CPUS390XState *env, uint64_t addr)
+{
+    return (uint32_t)ldl_phys(get_address(env, 0, 0, addr));
+}
+
+uint64_t HELPER(lurag)(CPUS390XState *env, uint64_t addr)
+{
+    return ldq_phys(get_address(env, 0, 0, addr));
+}
+
 /* store using real address */
 void HELPER(stura)(CPUS390XState *env, uint64_t addr, uint64_t v1)
 {
     stl_phys(get_address(env, 0, 0, addr), (uint32_t)v1);
 }
 
+void HELPER(sturg)(CPUS390XState *env, uint64_t addr, uint64_t v1)
+{
+    stq_phys(get_address(env, 0, 0, addr), v1);
+}
+
 /* load real address */
 uint64_t HELPER(lra)(CPUS390XState *env, uint64_t addr)
 {
diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 25a6537..e39305d 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2463,6 +2463,24 @@ static ExitStatus op_lm64(DisasContext *s, DisasOps *o)
     return NO_EXIT;
 }
 
+#ifndef CONFIG_USER_ONLY
+static ExitStatus op_lura(DisasContext *s, DisasOps *o)
+{
+    check_privileged(s);
+    potential_page_fault(s);
+    gen_helper_lura(o->out, cpu_env, o->in2);
+    return NO_EXIT;
+}
+
+static ExitStatus op_lurag(DisasContext *s, DisasOps *o)
+{
+    check_privileged(s);
+    potential_page_fault(s);
+    gen_helper_lurag(o->out, cpu_env, o->in2);
+    return NO_EXIT;
+}
+#endif
+
 static ExitStatus op_mov2(DisasContext *s, DisasOps *o)
 {
     o->out = o->in2;
@@ -3337,6 +3355,14 @@ static ExitStatus op_stura(DisasContext *s, DisasOps *o)
     gen_helper_stura(cpu_env, o->in2, o->in1);
     return NO_EXIT;
 }
+
+static ExitStatus op_sturg(DisasContext *s, DisasOps *o)
+{
+    check_privileged(s);
+    potential_page_fault(s);
+    gen_helper_sturg(cpu_env, o->in2, o->in1);
+    return NO_EXIT;
+}
 #endif
 
 static ExitStatus op_stfle(DisasContext *s, DisasOps *o)
-- 
1.8.1.4

[Qemu-devel] [PATCH 10/10] target-s390: Implement ECAG

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-s390x/insn-data.def | 2 ++
 target-s390x/translate.c   | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/target-s390x/insn-data.def b/target-s390x/insn-data.def
index a405f64..d3bc5b1 100644
--- a/target-s390x/insn-data.def
+++ b/target-s390x/insn-data.def
@@ -285,6 +285,8 @@
 
 /* EXTRACT ACCESS */
     C(0xb24f, EAR,     RRE,   Z,   0, 0, new, r1_32, ear, 0)
+/* EXTRACT CPU ATTRIBUTE */
+    C(0xeb4c, ECAG,    RSY_a, GIE, 0, a2, r1, 0, ecag, 0)
 /* EXTRACT FPC */
     C(0xb38c, EFPC,    RRE,   Z,   0, 0, new, r1_32, efpc, 0)
 /* EXTRACT PSW */
diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index e39305d..119f700 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2030,6 +2030,13 @@ static ExitStatus op_ear(DisasContext *s, DisasOps *o)
     return NO_EXIT;
 }
 
+static ExitStatus op_ecag(DisasContext *s, DisasOps *o)
+{
+    /* No cache information provided.  */
+    tcg_gen_movi_i64(o->out, -1);
+    return NO_EXIT;
+}
+
 static ExitStatus op_efpc(DisasContext *s, DisasOps *o)
 {
     tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, fpc));
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 01/10] target-s390: Move facilities bits to env

[Alexander Graf]

On 09/23/2013 04:04 PM, Richard Henderson wrote:
> Rather than simply hard-coding them in STFL instruction.
>
> Signed-off-by: Richard Henderson<rth@twiddle.net>
> ---
>   target-s390x/cpu.c       |  3 +++
>   target-s390x/cpu.h       |  1 +
>   target-s390x/translate.c | 10 +++++-----
>   3 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 3c89f8a..ff691df 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -181,6 +181,9 @@ static void s390_cpu_initfn(Object *obj)
>       env->cpu_num = cpu_num++;
>       env->ext_index = -1;
>
> +    env->facilities[0] = 0xc000000000000000ull;
> +    env->facilities[1] = 0;

Could we add CPU definitions along the way here? I'm fine with making z9 
the default CPU type, but we should make this explicit :).

> +
>       if (tcg_enabled()&&  !inited) {
>           inited = true;
>           s390x_translate_init();
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 8be5648..746aec8 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -136,6 +136,7 @@ typedef struct CPUS390XState {
>       CPU_COMMON
>
>       /* reset does memset(0) up to here */
> +    uint64_t facilities[2];
>
>       int cpu_num;
>       uint8_t *storage_keys;
> diff --git a/target-s390x/translate.c b/target-s390x/translate.c
> index afe90eb..d4dc8ea 100644
> --- a/target-s390x/translate.c
> +++ b/target-s390x/translate.c
> @@ -3230,12 +3230,12 @@ static ExitStatus op_spt(DisasContext *s, DisasOps *o)
>
>   static ExitStatus op_stfl(DisasContext *s, DisasOps *o)
>   {
> -    TCGv_i64 f, a;
> -    /* We really ought to have more complete indication of facilities
> -       that we implement.  Address this when STFLE is implemented.  */
> +    TCGv_i64 f = tcg_temp_new_i64();
> +    TCGv_i64 a = tcg_const_i64(200);
> +
>       check_privileged(s);
> -    f = tcg_const_i64(0xc0000000);
> -    a = tcg_const_i64(200);
> +    tcg_gen_ld_i64(f, cpu_env, offsetof(CPUS390XState, facilities[0]));
> +    tcg_gen_shri_i64(f, f, 32);

IMHO the facility list should be stored in DisasContext. That way we can 
check whether we're generating code against the correct target.


Alex

Re: [Qemu-devel] [PATCH 07/10] target-s390: Fix STIDP

[Alexander Graf]

On 09/23/2013 04:04 PM, Richard Henderson wrote:
> The implementation had been incomplete, as we did not store the
> machine type.
>
> Signed-off-by: Richard Henderson<rth@twiddle.net>
> ---
>   target-s390x/cpu.c       |  2 ++
>   target-s390x/cpu.h       | 14 +++++++++++++-
>   target-s390x/translate.c |  2 +-
>   3 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 01ff49b..d003dcf 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -257,6 +257,8 @@ static void s390_cpu_initfn(Object *obj)
>       env->facilities[0] |= FAC0_Z9_109;
>   #endif
>
> +    env->machine_type = 0x20940000;   /* ??? Also Z9-109.  */

This really wants to be selected by -cpu.

> +
>       if (tcg_enabled()&&  !inited) {
>           inited = true;
>           s390x_translate_init();
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index a0bafef..95f9cab 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -197,7 +197,19 @@ typedef struct CPUS390XState {
>       /* reset does memset(0) up to here */
>       uint64_t facilities[2];
>
> -    int cpu_num;
> +    union {
> +        uint64_t cpuid;
> +        struct {
> +#ifdef HOST_WORDS_BIGENDIAN
> +            uint32_t cpu_num;
> +            uint32_t machine_type;
> +#else
> +            uint32_t machine_type;
> +            uint32_t cpu_num;
> +#endif

Are we guaranteed that we don't need to pack? Also anonymous 
unions/structs are a gcc extension IIRC. And why do you swap endianness 
here, but not above when defining the machine_type value?


Alex

> +        };
> +    };
> +
>       uint8_t *storage_keys;
>
>       uint64_t tod_offset;
> diff --git a/target-s390x/translate.c b/target-s390x/translate.c
> index 192d54e..25a6537 100644
> --- a/target-s390x/translate.c
> +++ b/target-s390x/translate.c
> @@ -3242,7 +3242,7 @@ static ExitStatus op_stctl(DisasContext *s, DisasOps *o)
>   static ExitStatus op_stidp(DisasContext *s, DisasOps *o)
>   {
>       check_privileged(s);
> -    tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, cpu_num));
> +    tcg_gen_ld_i64(o->out, cpu_env, offsetof(CPUS390XState, cpuid));
>       return NO_EXIT;
>   }
>

Re: [Qemu-devel] [PATCH 0/9] target-s390 tcg improvements

[Alexander Graf]

On 09/23/2013 04:04 PM, Richard Henderson wrote:
> With this patch set we can boot the fedora 19 kernel, and make
> it all the way to /bin/init.  At which point the process either
> hangs or crashes; in either case the kernel winds up with no
> runnable processes and spends its time in the idle loop.
>
> The choice of z9-109 for the facilities is because that appears
> to be what fedora 19 is targeting as the minimum.
>
> That said, a debian install can make it all the way through to
> completion, so the fedora crash/hang must be related to something
> in the extra z9-109 insns.

Reviewed-by: Alexander Graf <agraf@suse.de>

I couldn't spot anything obviously wrong.


Alex

>
>
>
> r~
>
>
> Richard Henderson (10):
>    target-s390: Move facilities bits to env
>    target-s390: Implement STFLE
>    target-s390: Add facilities bits and sets
>    target-s390: Raise OPERATION exception for disabled insns
>    target-s390: Implement SAM31 and SAM64
>    target-s390: Implement EPSW
>    target-s390: Fix STIDP
>    target-s390: Fix STURA
>    target-s390: Implement LURA, LURAG, STURG
>    target-s390: Implement ECAG
>
>   target-s390x/cpu.c         |  78 ++++++++++++++++++++++
>   target-s390x/cpu.h         |  74 ++++++++++++++++++++-
>   target-s390x/helper.h      |   4 ++
>   target-s390x/insn-data.def |  18 +++--
>   target-s390x/mem_helper.c  |  18 ++++-
>   target-s390x/misc_helper.c |  13 ++++
>   target-s390x/translate.c   | 161 ++++++++++++++++++++++++++++++++++++---------
>   7 files changed, 329 insertions(+), 37 deletions(-)
>

Re: [Qemu-devel] [PATCH 01/10] target-s390: Move facilities bits to env

[Richard Henderson]

On 09/30/2013 11:03 AM, Alexander Graf wrote:
> On 09/23/2013 04:04 PM, Richard Henderson wrote:
>> Rather than simply hard-coding them in STFL instruction.
>>
>> Signed-off-by: Richard Henderson<rth@twiddle.net>
>> ---
>>   target-s390x/cpu.c       |  3 +++
>>   target-s390x/cpu.h       |  1 +
>>   target-s390x/translate.c | 10 +++++-----
>>   3 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>> index 3c89f8a..ff691df 100644
>> --- a/target-s390x/cpu.c
>> +++ b/target-s390x/cpu.c
>> @@ -181,6 +181,9 @@ static void s390_cpu_initfn(Object *obj)
>>       env->cpu_num = cpu_num++;
>>       env->ext_index = -1;
>>
>> +    env->facilities[0] = 0xc000000000000000ull;
>> +    env->facilities[1] = 0;
> 
> Could we add CPU definitions along the way here? I'm fine with making z9 the
> default CPU type, but we should make this explicit :).

Certainly that's what we should do.  I just hadn't yet researched the
currently correct way to do that.  I know there's some amount of out-of-date
examples in the current source base.

Pointers?

>>   static ExitStatus op_stfl(DisasContext *s, DisasOps *o)
>>   {
>> -    TCGv_i64 f, a;
>> -    /* We really ought to have more complete indication of facilities
>> -       that we implement.  Address this when STFLE is implemented.  */
>> +    TCGv_i64 f = tcg_temp_new_i64();
>> +    TCGv_i64 a = tcg_const_i64(200);
>> +
>>       check_privileged(s);
>> -    f = tcg_const_i64(0xc0000000);
>> -    a = tcg_const_i64(200);
>> +    tcg_gen_ld_i64(f, cpu_env, offsetof(CPUS390XState, facilities[0]));
>> +    tcg_gen_shri_i64(f, f, 32);
> 
> IMHO the facility list should be stored in DisasContext. That way we can check
> whether we're generating code against the correct target.

See patch 4.

As for the code we generate here, does it really matter if we load the value
from env, or have it encoded as a constant?  It still has to get stored to
memory, so it's not like the TCG optimizer is going to do anything with the
constant.


r~

Re: [Qemu-devel] [PATCH 07/10] target-s390: Fix STIDP

[Richard Henderson]

On 09/30/2013 11:13 AM, Alexander Graf wrote:
>> -    int cpu_num;
>> +    union {
>> +        uint64_t cpuid;
>> +        struct {
>> +#ifdef HOST_WORDS_BIGENDIAN
>> +            uint32_t cpu_num;
>> +            uint32_t machine_type;
>> +#else
>> +            uint32_t machine_type;
>> +            uint32_t cpu_num;
>> +#endif
> 
> Are we guaranteed that we don't need to pack? Also anonymous unions/structs are
> a gcc extension IIRC. And why do you swap endianness here, but not above when
> defining the machine_type value?

(1) I can't imagine that we would; such struct/unions are used all over.
(2) Sure, but we've so many other gcc extensions I figured it didn't matter.
(3) Of course.  I want host endianness, not target endianness.


r~

Re: [Qemu-devel] [PATCH 01/10] target-s390: Move facilities bits to env

[Alexander Graf]

On 09/30/2013 09:15 PM, Richard Henderson wrote:
> On 09/30/2013 11:03 AM, Alexander Graf wrote:
>> On 09/23/2013 04:04 PM, Richard Henderson wrote:
>>> Rather than simply hard-coding them in STFL instruction.
>>>
>>> Signed-off-by: Richard Henderson<rth@twiddle.net>
>>> ---
>>>    target-s390x/cpu.c       |  3 +++
>>>    target-s390x/cpu.h       |  1 +
>>>    target-s390x/translate.c | 10 +++++-----
>>>    3 files changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>>> index 3c89f8a..ff691df 100644
>>> --- a/target-s390x/cpu.c
>>> +++ b/target-s390x/cpu.c
>>> @@ -181,6 +181,9 @@ static void s390_cpu_initfn(Object *obj)
>>>        env->cpu_num = cpu_num++;
>>>        env->ext_index = -1;
>>>
>>> +    env->facilities[0] = 0xc000000000000000ull;
>>> +    env->facilities[1] = 0;
>> Could we add CPU definitions along the way here? I'm fine with making z9 the
>> default CPU type, but we should make this explicit :).
> Certainly that's what we should do.  I just hadn't yet researched the
> currently correct way to do that.  I know there's some amount of out-of-date
> examples in the current source base.

I'll leave the answer to that to Andreas :).

>
> Pointers?
>
>>>    static ExitStatus op_stfl(DisasContext *s, DisasOps *o)
>>>    {
>>> -    TCGv_i64 f, a;
>>> -    /* We really ought to have more complete indication of facilities
>>> -       that we implement.  Address this when STFLE is implemented.  */
>>> +    TCGv_i64 f = tcg_temp_new_i64();
>>> +    TCGv_i64 a = tcg_const_i64(200);
>>> +
>>>        check_privileged(s);
>>> -    f = tcg_const_i64(0xc0000000);
>>> -    a = tcg_const_i64(200);
>>> +    tcg_gen_ld_i64(f, cpu_env, offsetof(CPUS390XState, facilities[0]));
>>> +    tcg_gen_shri_i64(f, f, 32);
>> IMHO the facility list should be stored in DisasContext. That way we can check
>> whether we're generating code against the correct target.
> See patch 4.
>
> As for the code we generate here, does it really matter if we load the value
> from env, or have it encoded as a constant?  It still has to get stored to
> memory, so it's not like the TCG optimizer is going to do anything with the
> constant.

No, it only seemed more straight forward to me from a "single source of 
information" point of view. But it really doesn't matter. Shifting in C 
seems to be easier to read :).


Alex

Re: [Qemu-devel] [PATCH 07/10] target-s390: Fix STIDP

[Alexander Graf]

On 09/30/2013 09:48 PM, Richard Henderson wrote:
> On 09/30/2013 11:13 AM, Alexander Graf wrote:
>>> -    int cpu_num;
>>> +    union {
>>> +        uint64_t cpuid;
>>> +        struct {
>>> +#ifdef HOST_WORDS_BIGENDIAN
>>> +            uint32_t cpu_num;
>>> +            uint32_t machine_type;
>>> +#else
>>> +            uint32_t machine_type;
>>> +            uint32_t cpu_num;
>>> +#endif
>> Are we guaranteed that we don't need to pack? Also anonymous unions/structs are
>> a gcc extension IIRC. And why do you swap endianness here, but not above when
>> defining the machine_type value?
> (1) I can't imagine that we would; such struct/unions are used all over.

*shrug* you're the expert :).

> (2) Sure, but we've so many other gcc extensions I figured it didn't matter.

Avi complained about it to me in Linux patches. Not sure how much we 
care in QEMU.

> (3) Of course.  I want host endianness, not target endianness.

Phew. I think I'm slowly starting to grasp what you're trying to do 
here. Any way you could make this more explicit through shifts and ors 
and other explicit operations? This feels like too much magic to "just 
understand on a glimpse" to me.


Alex

Re: [Qemu-devel] [PATCH 01/10] target-s390: Move facilities bits to env

[Richard Henderson]

On 10/01/2013 08:48 AM, Alexander Graf wrote:
> On 09/30/2013 09:15 PM, Richard Henderson wrote:
>> On 09/30/2013 11:03 AM, Alexander Graf wrote:
>>> On 09/23/2013 04:04 PM, Richard Henderson wrote:
>>>> Rather than simply hard-coding them in STFL instruction.
>>>>
>>>> Signed-off-by: Richard Henderson<rth@twiddle.net>
>>>> ---
>>>>    target-s390x/cpu.c       |  3 +++
>>>>    target-s390x/cpu.h       |  1 +
>>>>    target-s390x/translate.c | 10 +++++-----
>>>>    3 files changed, 9 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>>>> index 3c89f8a..ff691df 100644
>>>> --- a/target-s390x/cpu.c
>>>> +++ b/target-s390x/cpu.c
>>>> @@ -181,6 +181,9 @@ static void s390_cpu_initfn(Object *obj)
>>>>        env->cpu_num = cpu_num++;
>>>>        env->ext_index = -1;
>>>>
>>>> +    env->facilities[0] = 0xc000000000000000ull;
>>>> +    env->facilities[1] = 0;
>>> Could we add CPU definitions along the way here? I'm fine with making z9 the
>>> default CPU type, but we should make this explicit :).
>> Certainly that's what we should do.  I just hadn't yet researched the
>> currently correct way to do that.  I know there's some amount of out-of-date
>> examples in the current source base.
> 
> I'll leave the answer to that to Andreas :).

Can we leave that for a separate patch series then?

>>>> -    TCGv_i64 f, a;
>>>> -    /* We really ought to have more complete indication of facilities
>>>> -       that we implement.  Address this when STFLE is implemented.  */
>>>> +    TCGv_i64 f = tcg_temp_new_i64();
>>>> +    TCGv_i64 a = tcg_const_i64(200);
>>>> +
>>>>        check_privileged(s);
>>>> -    f = tcg_const_i64(0xc0000000);
>>>> -    a = tcg_const_i64(200);
>>>> +    tcg_gen_ld_i64(f, cpu_env, offsetof(CPUS390XState, facilities[0]));
>>>> +    tcg_gen_shri_i64(f, f, 32);
>>> IMHO the facility list should be stored in DisasContext. That way we can check
>>> whether we're generating code against the correct target.
>> See patch 4.
>>
>> As for the code we generate here, does it really matter if we load the value
>> from env, or have it encoded as a constant?  It still has to get stored to
>> memory, so it's not like the TCG optimizer is going to do anything with the
>> constant.
> 
> No, it only seemed more straight forward to me from a "single source of
> information" point of view. But it really doesn't matter. Shifting in C seems
> to be easier to read :).

Fair enough.  I'll rearrange the order of the patches so that we can
update STFL to use the DisasContext data.


r~

Re: [Qemu-devel] [PATCH 07/10] target-s390: Fix STIDP

[Richard Henderson]

On 10/01/2013 08:52 AM, Alexander Graf wrote:
> On 09/30/2013 09:48 PM, Richard Henderson wrote:
>> On 09/30/2013 11:13 AM, Alexander Graf wrote:
>>>> -    int cpu_num;
>>>> +    union {
>>>> +        uint64_t cpuid;
>>>> +        struct {
>>>> +#ifdef HOST_WORDS_BIGENDIAN
>>>> +            uint32_t cpu_num;
>>>> +            uint32_t machine_type;
>>>> +#else
>>>> +            uint32_t machine_type;
>>>> +            uint32_t cpu_num;
>>>> +#endif
>>> Are we guaranteed that we don't need to pack? Also anonymous unions/structs are
>>> a gcc extension IIRC. And why do you swap endianness here, but not above when
>>> defining the machine_type value?
>> (1) I can't imagine that we would; such struct/unions are used all over.
> 
> *shrug* you're the expert :).
> 
>> (2) Sure, but we've so many other gcc extensions I figured it didn't matter.
> 
> Avi complained about it to me in Linux patches. Not sure how much we care in QEMU.
> 
>> (3) Of course.  I want host endianness, not target endianness.
> 
> Phew. I think I'm slowly starting to grasp what you're trying to do here. Any
> way you could make this more explicit through shifts and ors and other explicit
> operations? This feels like too much magic to "just understand on a glimpse" to
> me.

Yes, I could arrange for it to be two loads and assembled at runtime.

It's not like this insn is used on any hot path... or indeed, apparently
more than once in the whole lifetime of the system.


r~

Re: [Qemu-devel] [PATCH 01/10] target-s390: Move facilities bits to env

[Alexander Graf]

On 10/01/2013 05:52 PM, Richard Henderson wrote:
> On 10/01/2013 08:48 AM, Alexander Graf wrote:
>> On 09/30/2013 09:15 PM, Richard Henderson wrote:
>>> On 09/30/2013 11:03 AM, Alexander Graf wrote:
>>>> On 09/23/2013 04:04 PM, Richard Henderson wrote:
>>>>> Rather than simply hard-coding them in STFL instruction.
>>>>>
>>>>> Signed-off-by: Richard Henderson<rth@twiddle.net>
>>>>> ---
>>>>>     target-s390x/cpu.c       |  3 +++
>>>>>     target-s390x/cpu.h       |  1 +
>>>>>     target-s390x/translate.c | 10 +++++-----
>>>>>     3 files changed, 9 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>>>>> index 3c89f8a..ff691df 100644
>>>>> --- a/target-s390x/cpu.c
>>>>> +++ b/target-s390x/cpu.c
>>>>> @@ -181,6 +181,9 @@ static void s390_cpu_initfn(Object *obj)
>>>>>         env->cpu_num = cpu_num++;
>>>>>         env->ext_index = -1;
>>>>>
>>>>> +    env->facilities[0] = 0xc000000000000000ull;
>>>>> +    env->facilities[1] = 0;
>>>> Could we add CPU definitions along the way here? I'm fine with making z9 the
>>>> default CPU type, but we should make this explicit :).
>>> Certainly that's what we should do.  I just hadn't yet researched the
>>> currently correct way to do that.  I know there's some amount of out-of-date
>>> examples in the current source base.
>> I'll leave the answer to that to Andreas :).
> Can we leave that for a separate patch series then?

Just make sure you actually check for feature bits on every instruction 
(which I think you do, but the current code is way too magical to me to 
really understand it anymore) so that we can always implement a z900 cpu 
type later on.

>
>>>>> -    TCGv_i64 f, a;
>>>>> -    /* We really ought to have more complete indication of facilities
>>>>> -       that we implement.  Address this when STFLE is implemented.  */
>>>>> +    TCGv_i64 f = tcg_temp_new_i64();
>>>>> +    TCGv_i64 a = tcg_const_i64(200);
>>>>> +
>>>>>         check_privileged(s);
>>>>> -    f = tcg_const_i64(0xc0000000);
>>>>> -    a = tcg_const_i64(200);
>>>>> +    tcg_gen_ld_i64(f, cpu_env, offsetof(CPUS390XState, facilities[0]));
>>>>> +    tcg_gen_shri_i64(f, f, 32);
>>>> IMHO the facility list should be stored in DisasContext. That way we can check
>>>> whether we're generating code against the correct target.
>>> See patch 4.
>>>
>>> As for the code we generate here, does it really matter if we load the value
>>> from env, or have it encoded as a constant?  It still has to get stored to
>>> memory, so it's not like the TCG optimizer is going to do anything with the
>>> constant.
>> No, it only seemed more straight forward to me from a "single source of
>> information" point of view. But it really doesn't matter. Shifting in C seems
>> to be easier to read :).
> Fair enough.  I'll rearrange the order of the patches so that we can
> update STFL to use the DisasContext data.

Thanks :)


Alex

Re: [Qemu-devel] [PATCH 01/10] target-s390: Move facilities bits to env

[Richard Henderson]

On 10/01/2013 08:54 AM, Alexander Graf wrote:
> Just make sure you actually check for feature bits on every instruction (which
> I think you do, but the current code is way too magical to me to really
> understand it anymore) so that we can always implement a z900 cpu type later on.

Yes, the code to check is in "translate_one".  Which, as you might imagine,
translates one instruction.  ;-)


r~