[Qemu-devel] [PATCH v2 for-2.12 0/5] s390x/tcg: CCW hotplug support

[Qemu-devel] [PATCH v2 for-2.12 0/5] s390x/tcg: CCW hotplug support

[David Hildenbrand]

Hotplugging a ccw device currently fails due to different reasons.

1. The stored machine check information is partially wrong.
2. The TOD programmable field cannot be restored.
3. STCRW cannot get executed.

With these patches, I am able to sucessfully hotplug e.g. virtio-rng by
issuing "device_add virtio-rng-ccw,id=rng0" to a Linux guest. Unplug
also seems to work.

v1 -> v2:
- avoid accessing unmapped lowcore
- minor style issues, more/better comments and description fixes

David Hildenbrand (5):
  s390x/kvm: factor out build_channel_report_mcic() into cpu.h
  s390x/tcg: fix and cleanup mcck injection
  s390x/tcg: implement SET CLOCK PROGRAMMABLE FIELD
  s390x/tcg: indicate value of TODPR in STCKE
  s390x/tcg: wire up STORE CHANNEL REPORT WORD

 target/s390x/cpu.h         | 22 ++++++++++++++++++++++
 target/s390x/excp_helper.c | 12 ++++++------
 target/s390x/helper.h      |  2 ++
 target/s390x/insn-data.def |  3 +++
 target/s390x/internal.h    |  6 +++---
 target/s390x/kvm.c         | 25 ++-----------------------
 target/s390x/misc_helper.c | 20 ++++++++++++++++++++
 target/s390x/translate.c   | 20 ++++++++++++++++++++
 8 files changed, 78 insertions(+), 32 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v2 for-2.12 1/5] s390x/kvm: factor out build_channel_report_mcic() into cpu.h

[David Hildenbrand]

We'll need it later on in two places. Refactor it to just indicate the
validity bits. While at it, introduce a define for the used CR14 bit (we'll
also need later on).

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu.h | 22 ++++++++++++++++++++++
 target/s390x/kvm.c | 25 ++-----------------------
 2 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 9cfbbbac04..f29f4ccd8b 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -351,6 +351,9 @@ extern const struct VMStateDescription vmstate_s390_cpu;
 #define CR0_CPU_TIMER_SC        0x0000000000000400ULL
 #define CR0_SERVICE_SC          0x0000000000000200ULL
 
+/* Control register 14 bits */
+#define CR14_CHANNEL_REPORT_SC  0x0000000010000000ULL
+
 /* MMU */
 #define MMU_PRIMARY_IDX         0
 #define MMU_SECONDARY_IDX       1
@@ -674,6 +677,25 @@ struct sysib_322 {
 #define MCIC_VB_CT 0x0000000000020000ULL
 #define MCIC_VB_CC 0x0000000000010000ULL
 
+static inline uint64_t s390_build_validity_mcic(void)
+{
+    uint64_t mcic;
+
+    /* Indicate all validity bits (no damage) only. Other bits have to be
+     * added by the caller. (storage errors, subclasses and subclass modifiers)
+     */
+    mcic = MCIC_VB_WP | MCIC_VB_MS | MCIC_VB_PM | MCIC_VB_IA | MCIC_VB_FP |
+           MCIC_VB_GR | MCIC_VB_CR | MCIC_VB_ST | MCIC_VB_AR | MCIC_VB_PR |
+           MCIC_VB_FC | MCIC_VB_CT | MCIC_VB_CC;
+    if (s390_has_feat(S390_FEAT_VECTOR)) {
+        mcic |= MCIC_VB_VR;
+    }
+    if (s390_has_feat(S390_FEAT_GUARDED_STORAGE)) {
+        mcic |= MCIC_VB_GS;
+    }
+    return mcic;
+}
+
 
 /* cpu.c */
 int s390_get_clock(uint8_t *tod_high, uint64_t *tod_low);
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 97c45d5537..9b8b59f2a2 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1852,33 +1852,12 @@ void kvm_s390_io_interrupt(uint16_t subchannel_id,
     kvm_s390_floating_interrupt(&irq);
 }
 
-static uint64_t build_channel_report_mcic(void)
-{
-    uint64_t mcic;
-
-    /* subclass: indicate channel report pending */
-    mcic = MCIC_SC_CP |
-    /* subclass modifiers: none */
-    /* storage errors: none */
-    /* validity bits: no damage */
-        MCIC_VB_WP | MCIC_VB_MS | MCIC_VB_PM | MCIC_VB_IA | MCIC_VB_FP |
-        MCIC_VB_GR | MCIC_VB_CR | MCIC_VB_ST | MCIC_VB_AR | MCIC_VB_PR |
-        MCIC_VB_FC | MCIC_VB_CT | MCIC_VB_CC;
-    if (s390_has_feat(S390_FEAT_VECTOR)) {
-        mcic |= MCIC_VB_VR;
-    }
-    if (s390_has_feat(S390_FEAT_GUARDED_STORAGE)) {
-        mcic |= MCIC_VB_GS;
-    }
-    return mcic;
-}
-
 void kvm_s390_crw_mchk(void)
 {
     struct kvm_s390_irq irq = {
         .type = KVM_S390_MCHK,
-        .u.mchk.cr14 = 1 << 28,
-        .u.mchk.mcic = build_channel_report_mcic(),
+        .u.mchk.cr14 = CR14_CHANNEL_REPORT_SC,
+        .u.mchk.mcic = s390_build_validity_mcic() | MCIC_SC_CP,
     };
     kvm_s390_floating_interrupt(&irq);
 }
-- 
2.14.3

[Qemu-devel] [PATCH v2 for-2.12 2/5] s390x/tcg: fix and cleanup mcck injection

[David Hildenbrand]

The architecture mode indication wasn't stored. The split of certain
64bit fields was unnecessary. Also, the complete clock comparator, not
just bit 0-55 (starting at byte 1) was stored.

We now generate a proper MCIC via the same helper we use for KVM.

There is be more to clean up, but we will change the other parts later on
either way.

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

diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index d831537544..f4697a884d 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -395,6 +395,9 @@ static void do_mchk_interrupt(CPUS390XState *env)
 
     lowcore = cpu_map_lowcore(env);
 
+    /* we are always in z/Architecture mode */
+    lowcore->ar_access_id = 1;
+
     for (i = 0; i < 16; i++) {
         lowcore->floating_pt_save_area[i] = cpu_to_be64(get_freg(env, i)->ll);
         lowcore->gpregs_save_area[i] = cpu_to_be64(env->regs[i]);
@@ -404,13 +407,10 @@ static void do_mchk_interrupt(CPUS390XState *env)
     lowcore->prefixreg_save_area = cpu_to_be32(env->psa);
     lowcore->fpt_creg_save_area = cpu_to_be32(env->fpc);
     lowcore->tod_progreg_save_area = cpu_to_be32(env->todpr);
-    lowcore->cpu_timer_save_area[0] = cpu_to_be32(env->cputm >> 32);
-    lowcore->cpu_timer_save_area[1] = cpu_to_be32((uint32_t)env->cputm);
-    lowcore->clock_comp_save_area[0] = cpu_to_be32(env->ckc >> 32);
-    lowcore->clock_comp_save_area[1] = cpu_to_be32((uint32_t)env->ckc);
+    lowcore->cpu_timer_save_area = cpu_to_be64(env->cputm);
+    lowcore->clock_comp_save_area = cpu_to_be64(env->ckc >> 8);
 
-    lowcore->mcck_interruption_code[0] = cpu_to_be32(0x00400f1d);
-    lowcore->mcck_interruption_code[1] = cpu_to_be32(0x40330000);
+    lowcore->mcic = cpu_to_be64(s390_build_validity_mcic() | MCIC_SC_CP);
     lowcore->mcck_old_psw.mask = cpu_to_be64(get_psw_mask(env));
     lowcore->mcck_old_psw.addr = cpu_to_be64(env->psw.addr);
     mask = be64_to_cpu(lowcore->mcck_new_psw.mask);
diff --git a/target/s390x/internal.h b/target/s390x/internal.h
index 6817b2c432..1a88e4beb4 100644
--- a/target/s390x/internal.h
+++ b/target/s390x/internal.h
@@ -43,7 +43,7 @@ typedef struct LowCore {
     uint8_t         pad3[0xc8 - 0xc4];        /* 0x0c4 */
     uint32_t        stfl_fac_list;            /* 0x0c8 */
     uint8_t         pad4[0xe8 - 0xcc];        /* 0x0cc */
-    uint32_t        mcck_interruption_code[2]; /* 0x0e8 */
+    uint64_t        mcic;                     /* 0x0e8 */
     uint8_t         pad5[0xf4 - 0xf0];        /* 0x0f0 */
     uint32_t        external_damage_code;     /* 0x0f4 */
     uint64_t        failing_storage_address;  /* 0x0f8 */
@@ -118,8 +118,8 @@ typedef struct LowCore {
     uint32_t        fpt_creg_save_area;        /* 0x131c */
     uint8_t         pad16[0x1324 - 0x1320];    /* 0x1320 */
     uint32_t        tod_progreg_save_area;     /* 0x1324 */
-    uint32_t        cpu_timer_save_area[2];    /* 0x1328 */
-    uint32_t        clock_comp_save_area[2];   /* 0x1330 */
+    uint64_t        cpu_timer_save_area;       /* 0x1328 */
+    uint64_t        clock_comp_save_area;      /* 0x1330 */
     uint8_t         pad17[0x1340 - 0x1338];    /* 0x1338 */
     uint32_t        access_regs_save_area[16]; /* 0x1340 */
     uint64_t        cregs_save_area[16];       /* 0x1380 */
-- 
2.14.3

[Qemu-devel] [PATCH v2 for-2.12 3/5] s390x/tcg: implement SET CLOCK PROGRAMMABLE FIELD

[David Hildenbrand]

Needed for machine check handling inside Linux (when restoring registers).

Except for SIGP and machine checks, we don't make use of the register
yet. Sufficient for now.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/helper.h      |  1 +
 target/s390x/insn-data.def |  2 ++
 target/s390x/misc_helper.c | 11 +++++++++++
 target/s390x/translate.c   |  7 +++++++
 4 files changed, 21 insertions(+)

diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 9459b73c73..0281c286b8 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -127,6 +127,7 @@ DEF_HELPER_3(load_psw, noreturn, env, i64, i64)
 DEF_HELPER_FLAGS_2(spx, TCG_CALL_NO_RWG, void, env, i64)
 DEF_HELPER_FLAGS_1(stck, TCG_CALL_NO_RWG_SE, i64, env)
 DEF_HELPER_FLAGS_2(sckc, TCG_CALL_NO_RWG, void, env, i64)
+DEF_HELPER_FLAGS_1(sckpf, TCG_CALL_NO_RWG, void, env)
 DEF_HELPER_FLAGS_1(stckc, TCG_CALL_NO_RWG, i64, env)
 DEF_HELPER_FLAGS_2(spt, TCG_CALL_NO_RWG, void, env, i64)
 DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 16e27c8a35..8c2541f545 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -999,6 +999,8 @@
     C(0xb204, SCK,     S,     Z,   0, 0, 0, 0, 0, 0)
 /* SET CLOCK COMPARATOR */
     C(0xb206, SCKC,    S,     Z,   0, m2_64, 0, 0, sckc, 0)
+/* SET CLOCK PROGRAMMABLE FIELD */
+    C(0x0107, SCKPF,   E,     Z,   0, 0, 0, 0, sckpf, 0)
 /* SET CPU TIMER */
     C(0xb208, SPT,     S,     Z,   0, m2_64, 0, 0, spt, 0)
 /* SET PREFIX */
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 6d766ce1e7..25b71f08ee 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -146,6 +146,17 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time)
     timer_mod(env->tod_timer, env->tod_basetime + time);
 }
 
+/* Set Tod Programmable Field */
+void HELPER(sckpf)(CPUS390XState *env)
+{
+    uint32_t val = env->regs[0];
+
+    if (val & 0xffff0000) {
+        s390_program_interrupt(env, PGM_SPECIFICATION, 2, GETPC());
+    }
+    env->todpr = val;
+}
+
 /* Store Clock Comparator */
 uint64_t HELPER(stckc)(CPUS390XState *env)
 {
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 26cf993405..48b031894a 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -3922,6 +3922,13 @@ static ExitStatus op_sckc(DisasContext *s, DisasOps *o)
     return NO_EXIT;
 }
 
+static ExitStatus op_sckpf(DisasContext *s, DisasOps *o)
+{
+    check_privileged(s);
+    gen_helper_sckpf(cpu_env);
+    return NO_EXIT;
+}
+
 static ExitStatus op_stckc(DisasContext *s, DisasOps *o)
 {
     check_privileged(s);
-- 
2.14.3

[Qemu-devel] [PATCH v2 for-2.12 4/5] s390x/tcg: indicate value of TODPR in STCKE

[David Hildenbrand]

We were not yet using the value of the TOD Programmable Register.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/translate.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 48b031894a..8da8610839 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -3897,7 +3897,10 @@ static ExitStatus op_stcke(DisasContext *s, DisasOps *o)
 {
     TCGv_i64 c1 = tcg_temp_new_i64();
     TCGv_i64 c2 = tcg_temp_new_i64();
+    TCGv_i64 todpr = tcg_temp_new_i64();
     gen_helper_stck(c1, cpu_env);
+    /* 16 bit value store in an uint32_t (only valid bits set) */
+    tcg_gen_ld32u_i64(todpr, cpu_env, offsetof(CPUS390XState, todpr));
     /* Shift the 64-bit value into its place as a zero-extended
        104-bit value.  Note that "bit positions 64-103 are always
        non-zero so that they compare differently to STCK"; we set
@@ -3905,11 +3908,13 @@ static ExitStatus op_stcke(DisasContext *s, DisasOps *o)
     tcg_gen_shli_i64(c2, c1, 56);
     tcg_gen_shri_i64(c1, c1, 8);
     tcg_gen_ori_i64(c2, c2, 0x10000);
+    tcg_gen_or_i64(c2, c2, todpr);
     tcg_gen_qemu_st64(c1, o->in2, get_mem_index(s));
     tcg_gen_addi_i64(o->in2, o->in2, 8);
     tcg_gen_qemu_st64(c2, o->in2, get_mem_index(s));
     tcg_temp_free_i64(c1);
     tcg_temp_free_i64(c2);
+    tcg_temp_free_i64(todpr);
     /* ??? We don't implement clock states.  */
     gen_op_movi_cc(s, 0);
     return NO_EXIT;
-- 
2.14.3

[Qemu-devel] [PATCH v2 for-2.12 5/5] s390x/tcg: wire up STORE CHANNEL REPORT WORD

[David Hildenbrand]

CRW machine check handling requires STCRW. So let's wire it up.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/helper.h      | 1 +
 target/s390x/insn-data.def | 1 +
 target/s390x/misc_helper.c | 9 +++++++++
 target/s390x/translate.c   | 8 ++++++++
 4 files changed, 19 insertions(+)

diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 0281c286b8..2ce57edc14 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -166,6 +166,7 @@ DEF_HELPER_3(msch, void, env, i64, i64)
 DEF_HELPER_2(rchp, void, env, i64)
 DEF_HELPER_2(rsch, void, env, i64)
 DEF_HELPER_3(ssch, void, env, i64, i64)
+DEF_HELPER_2(stcrw, void, env, i64)
 DEF_HELPER_3(stsch, void, env, i64, i64)
 DEF_HELPER_3(tsch, void, env, i64, i64)
 DEF_HELPER_2(chsc, void, env, i64)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 8c2541f545..43ab1963c8 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -1055,6 +1055,7 @@
     C(0xb23b, RCHP,    S,     Z,   0, 0, 0, 0, rchp, 0)
     C(0xb238, RSCH,    S,     Z,   0, 0, 0, 0, rsch, 0)
     C(0xb233, SSCH,    S,     Z,   0, insn, 0, 0, ssch, 0)
+    C(0xb239, STCRW,   S,     Z,   0, insn, 0, 0, stcrw, 0)
     C(0xb234, STSCH,   S,     Z,   0, insn, 0, 0, stsch, 0)
     C(0xb235, TSCH,    S,     Z,   0, insn, 0, 0, tsch, 0)
     /* ??? Not listed in PoO ninth edition, but there's a linux driver that
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 25b71f08ee..3541e47114 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -385,6 +385,15 @@ void HELPER(ssch)(CPUS390XState *env, uint64_t r1, uint64_t inst)
     qemu_mutex_unlock_iothread();
 }
 
+void HELPER(stcrw)(CPUS390XState *env, uint64_t inst)
+{
+    S390CPU *cpu = s390_env_get_cpu(env);
+
+    qemu_mutex_lock_iothread();
+    ioinst_handle_stcrw(cpu, inst >> 16, GETPC());
+    qemu_mutex_unlock_iothread();
+}
+
 void HELPER(stsch)(CPUS390XState *env, uint64_t r1, uint64_t inst)
 {
     S390CPU *cpu = s390_env_get_cpu(env);
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 8da8610839..5e051fdd03 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -4071,6 +4071,14 @@ static ExitStatus op_stsch(DisasContext *s, DisasOps *o)
     return NO_EXIT;
 }
 
+static ExitStatus op_stcrw(DisasContext *s, DisasOps *o)
+{
+    check_privileged(s);
+    gen_helper_stcrw(cpu_env, o->in2);
+    set_cc_static(s);
+    return NO_EXIT;
+}
+
 static ExitStatus op_tsch(DisasContext *s, DisasOps *o)
 {
     check_privileged(s);
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 for-2.12 3/5] s390x/tcg: implement SET CLOCK PROGRAMMABLE FIELD

[Richard Henderson]

On 12/07/2017 07:31 AM, David Hildenbrand wrote:
> +DEF_HELPER_FLAGS_1(sckpf, TCG_CALL_NO_RWG, void, env)
...
> +/* Set Tod Programmable Field */
> +void HELPER(sckpf)(CPUS390XState *env)
> +{
> +    uint32_t val = env->regs[0];
> +
> +    if (val & 0xffff0000) {
> +        s390_program_interrupt(env, PGM_SPECIFICATION, 2, GETPC());
> +    }
> +    env->todpr = val;
> +}

You do read a tcg global -- regs[0].  Either pass in r0 as a parameter or use
TCG_CALL_NO_WG.


r~

Re: [Qemu-devel] [PATCH v2 for-2.12 4/5] s390x/tcg: indicate value of TODPR in STCKE

[Richard Henderson]

On 12/07/2017 07:31 AM, David Hildenbrand wrote:
> +    /* 16 bit value store in an uint32_t (only valid bits set) */
> +    tcg_gen_ld32u_i64(todpr, cpu_env, offsetof(CPUS390XState, todpr));

Any reason not to use a uint16_t and use tcg_gen_ld16u_i64 here?


r~

Re: [Qemu-devel] [PATCH v2 for-2.12 5/5] s390x/tcg: wire up STORE CHANNEL REPORT WORD

[Richard Henderson]

On 12/07/2017 07:31 AM, David Hildenbrand wrote:
> CRW machine check handling requires STCRW. So let's wire it up.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/helper.h      | 1 +
>  target/s390x/insn-data.def | 1 +
>  target/s390x/misc_helper.c | 9 +++++++++
>  target/s390x/translate.c   | 8 ++++++++
>  4 files changed, 19 insertions(+)

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


r~

Re: [Qemu-devel] [PATCH v2 for-2.12 1/5] s390x/kvm: factor out build_channel_report_mcic() into cpu.h

[Cornelia Huck]

On Thu,  7 Dec 2017 16:31:37 +0100
David Hildenbrand <david@redhat.com> wrote:

> We'll need it later on in two places. Refactor it to just indicate the
> validity bits. While at it, introduce a define for the used CR14 bit (we'll
> also need later on).
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/cpu.h | 22 ++++++++++++++++++++++
>  target/s390x/kvm.c | 25 ++-----------------------
>  2 files changed, 24 insertions(+), 23 deletions(-)
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 9cfbbbac04..f29f4ccd8b 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -351,6 +351,9 @@ extern const struct VMStateDescription vmstate_s390_cpu;
>  #define CR0_CPU_TIMER_SC        0x0000000000000400ULL
>  #define CR0_SERVICE_SC          0x0000000000000200ULL
>  
> +/* Control register 14 bits */
> +#define CR14_CHANNEL_REPORT_SC  0x0000000010000000ULL
> +
>  /* MMU */
>  #define MMU_PRIMARY_IDX         0
>  #define MMU_SECONDARY_IDX       1
> @@ -674,6 +677,25 @@ struct sysib_322 {
>  #define MCIC_VB_CT 0x0000000000020000ULL
>  #define MCIC_VB_CC 0x0000000000010000ULL
>  
> +static inline uint64_t s390_build_validity_mcic(void)
> +{
> +    uint64_t mcic;
> +
> +    /* Indicate all validity bits (no damage) only. Other bits have to be
> +     * added by the caller. (storage errors, subclasses and subclass modifiers)
> +     */

Looks good. I prefer the classic comment style (empty first line),
though.

> +    mcic = MCIC_VB_WP | MCIC_VB_MS | MCIC_VB_PM | MCIC_VB_IA | MCIC_VB_FP |
> +           MCIC_VB_GR | MCIC_VB_CR | MCIC_VB_ST | MCIC_VB_AR | MCIC_VB_PR |
> +           MCIC_VB_FC | MCIC_VB_CT | MCIC_VB_CC;
> +    if (s390_has_feat(S390_FEAT_VECTOR)) {
> +        mcic |= MCIC_VB_VR;
> +    }
> +    if (s390_has_feat(S390_FEAT_GUARDED_STORAGE)) {
> +        mcic |= MCIC_VB_GS;
> +    }
> +    return mcic;
> +}
> +
>  
>  /* cpu.c */
>  int s390_get_clock(uint8_t *tod_high, uint64_t *tod_low);
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 97c45d5537..9b8b59f2a2 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1852,33 +1852,12 @@ void kvm_s390_io_interrupt(uint16_t subchannel_id,
>      kvm_s390_floating_interrupt(&irq);
>  }
>  
> -static uint64_t build_channel_report_mcic(void)
> -{
> -    uint64_t mcic;
> -
> -    /* subclass: indicate channel report pending */
> -    mcic = MCIC_SC_CP |
> -    /* subclass modifiers: none */
> -    /* storage errors: none */
> -    /* validity bits: no damage */
> -        MCIC_VB_WP | MCIC_VB_MS | MCIC_VB_PM | MCIC_VB_IA | MCIC_VB_FP |
> -        MCIC_VB_GR | MCIC_VB_CR | MCIC_VB_ST | MCIC_VB_AR | MCIC_VB_PR |
> -        MCIC_VB_FC | MCIC_VB_CT | MCIC_VB_CC;
> -    if (s390_has_feat(S390_FEAT_VECTOR)) {
> -        mcic |= MCIC_VB_VR;
> -    }
> -    if (s390_has_feat(S390_FEAT_GUARDED_STORAGE)) {
> -        mcic |= MCIC_VB_GS;
> -    }
> -    return mcic;
> -}
> -
>  void kvm_s390_crw_mchk(void)
>  {
>      struct kvm_s390_irq irq = {
>          .type = KVM_S390_MCHK,
> -        .u.mchk.cr14 = 1 << 28,
> -        .u.mchk.mcic = build_channel_report_mcic(),
> +        .u.mchk.cr14 = CR14_CHANNEL_REPORT_SC,
> +        .u.mchk.mcic = s390_build_validity_mcic() | MCIC_SC_CP,
>      };
>      kvm_s390_floating_interrupt(&irq);
>  }

Re: [Qemu-devel] [PATCH v2 for-2.12 2/5] s390x/tcg: fix and cleanup mcck injection

[Cornelia Huck]

On Thu,  7 Dec 2017 16:31:38 +0100
David Hildenbrand <david@redhat.com> wrote:

> The architecture mode indication wasn't stored. The split of certain
> 64bit fields was unnecessary. Also, the complete clock comparator, not
> just bit 0-55 (starting at byte 1) was stored.
> 
> We now generate a proper MCIC via the same helper we use for KVM.
> 
> There is be more to clean up, but we will change the other parts later on

s/is be/is/

> either way.

No surprise as I had no idea what I was doing when I initially wrote
this :)

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

Re: [Qemu-devel] [PATCH v2 for-2.12 3/5] s390x/tcg: implement SET CLOCK PROGRAMMABLE FIELD

[David Hildenbrand]

On 08.12.2017 01:00, Richard Henderson wrote:
> On 12/07/2017 07:31 AM, David Hildenbrand wrote:
>> +DEF_HELPER_FLAGS_1(sckpf, TCG_CALL_NO_RWG, void, env)
> ...
>> +/* Set Tod Programmable Field */
>> +void HELPER(sckpf)(CPUS390XState *env)
>> +{
>> +    uint32_t val = env->regs[0];
>> +
>> +    if (val & 0xffff0000) {
>> +        s390_program_interrupt(env, PGM_SPECIFICATION, 2, GETPC());
>> +    }
>> +    env->todpr = val;
>> +}
> 
> You do read a tcg global -- regs[0].  Either pass in r0 as a parameter or use
> TCG_CALL_NO_WG.
> 
> 

Indeed, I finally understood what the term "global" in this context
means. (global tcg vals defined in translate.c)

> r~
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 for-2.12 4/5] s390x/tcg: indicate value of TODPR in STCKE

[David Hildenbrand]

On 08.12.2017 01:02, Richard Henderson wrote:
> On 12/07/2017 07:31 AM, David Hildenbrand wrote:
>> +    /* 16 bit value store in an uint32_t (only valid bits set) */
>> +    tcg_gen_ld32u_i64(todpr, cpu_env, offsetof(CPUS390XState, todpr));
> 
> Any reason not to use a uint16_t and use tcg_gen_ld16u_i64 here?
> 

Yes, KVM introduced and uses this field. As it is used for migration, we
cannot easily change it. (any maybe it was designed that way for future
changes)

> 
> r~
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 for-2.12 4/5] s390x/tcg: indicate value of TODPR in STCKE

[Christian Borntraeger]

On 12/08/2017 01:45 PM, David Hildenbrand wrote:
> On 08.12.2017 01:02, Richard Henderson wrote:
>> On 12/07/2017 07:31 AM, David Hildenbrand wrote:
>>> +    /* 16 bit value store in an uint32_t (only valid bits set) */
>>> +    tcg_gen_ld32u_i64(todpr, cpu_env, offsetof(CPUS390XState, todpr));
>>
>> Any reason not to use a uint16_t and use tcg_gen_ld16u_i64 here?
>>
> 
> Yes, KVM introduced and uses this field. As it is used for migration, we
> cannot easily change it. (any maybe it was designed that way for future
> changes)

FWIW, if you look at the POP the TOD programmable register is 32bit (of which
bits 0-15 in IBM speak) are 0.