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

[Janosch Frank]

On 11/5/19 8:29 PM, David Hildenbrand wrote:
> 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.

Ok, I need to alter the commit message, see below for the reason.

> 
>> +
>> +    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.
> 

I'm not completely sure if we are allowed to do that.
I think we need to load the PSW, so we can indicate the PSW which caused
the spec exception. At least that's what lpar does and I'll need to
speak with the architecture designers to verify that we absolutely need
to do it.

I would have also preferred to just do a spec on the diag.

signature.asc
Description: OpenPGP digital signature