Re: [qemu-s390x] [PATCH v1 for-2.12 2/5] s390x/tcg: fix and cleanup mcck injection

[Cornelia Huck]

On Mon,  4 Dec 2017 13:55:02 +0100
David Hildenbrand <address@hidden> 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.
> 
> While at it, also get rid of two local variables. There is be more to
> clean up, but we will change the other parts later on either way.
> 
> Signed-off-by: David Hildenbrand <address@hidden>
> ---
>  target/s390x/excp_helper.c | 18 ++++++++----------
>  target/s390x/internal.h    |  6 +++---
>  2 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
> index d831537544..840cf7641a 100644
> --- a/target/s390x/excp_helper.c
> +++ b/target/s390x/excp_helper.c
> @@ -369,7 +369,6 @@ static void do_io_interrupt(CPUS390XState *env)
>  static void do_mchk_interrupt(CPUS390XState *env)
>  {
>      S390CPU *cpu = s390_env_get_cpu(env);
> -    uint64_t mask, addr;
>      LowCore *lowcore;
>      MchkQueue *q;
>      int i;

[BTW, there's also a CR 14 check in here that probably could use the
new #define.]

> @@ -395,6 +394,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,17 +406,12 @@ 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);

Hm... I'm not sure that is a good idea (the nature of the helper, not
that you remove the magic values).

This function is called do_mchk_interrupt(), which sounds like a more
generic thing. Maybe we need to enhance the machine check code to save
the mcic etc. somewhere (after it has been generated) and just inject
it here? Similar to what the kernel does.

If you want to avoid rework, just add a TODO here?

>      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);
> -    addr = be64_to_cpu(lowcore->mcck_new_psw.addr);
>  
>      cpu_unmap_lowcore(lowcore);
>