Re: [PATCH] s390x: Properly fetch and test the short psw on diag308 subc 0/1

[David Hildenbrand]

On 05.11.19 19:44, Janosch Frank wrote:
> We need to actually fetch the cpu mask and set it after checking for
> psw bit 12 instead of completely ignoring it.
>
> Signed-off-by: Janosch Frank <address@hidden>
> ---
>   target/s390x/cpu.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 736a7903e2..0acba843a7 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -76,8 +76,15 @@ static bool s390_cpu_has_work(CPUState *cs)
>   static void s390_cpu_load_normal(CPUState *s)
>   {
>       S390CPU *cpu = S390_CPU(s);
> -    cpu->env.psw.addr = ldl_phys(s->as, 4) & PSW_MASK_ESA_ADDR;
> -    cpu->env.psw.mask = PSW_MASK_32 | PSW_MASK_64;
> +    uint64_t spsw = ldq_phys(s->as, 0);
> +
> +    /* Mask out bit 12 and instruction address */
> +    cpu->env.psw.mask = spsw & 0xfff7ffff80000000UL;
> +    cpu->env.psw.addr = spsw & 0x7fffffffUL;

"set it after checking for psw bit 12" does not match your code.

> +
> +    if (!(spsw & 0x8000000000000UL)) {
> +        s390_program_interrupt(&cpu->env, PGM_SPECIFICATION, 0, RA_IGNORED);
> +    }

So, this code is called from s390_machine_reset() via run_on_cpu() - so 
not from a helper. There is no state to rewind. This feels wrong to me.

In tcg_s390_program_interrupt(), we do

1. A cpu_restore_state(), which is bad with a ra of 0
2. A cpu_loop_exit(), which is bad, as we are not in the cpu loop.

We *could* do here instead

/* This code is not called from the CPU loop, but via run_on_cpu() */
if (tcg_enabled()) {
    /*
     * HW injects a PGM exception with ILC 0. We won't rewind.
     */
    env->int_pgm_ilen = 2;
    trigger_pgm_exception(&cpu->env, PGM_SPECIFICATION);
} else {
    kvm_s390_program_interrupt(env_archcpu(&cpu->env),
                               PGM_SPECIFICATION);
}


BUT I do wonder if we should actually get a PGM_SPECIFICATION for the 
*diag* instruction, not on the boot CPU. I think you should check + 
inject inside handle_diag_308() instead. Then that complicated handling 
is gone.

--

Thanks,

David / dhildenb