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

[David Hildenbrand <david@redhat.com>]

On 05.11.19 20:34, Janosch Frank wrote:
> 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 <frankja@linux.ibm.com>
>>> ---
>>>    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.

This should fall under "Exceptions Associated with the PSW" - PoP 6-9. I 
assume we talk about early exceptions.


"For the following error conditions, a program interrup-
tion for a specification exception occurs immediately
after the PSW becomes active:
...
Any of the unassigned bits (0, 2-4, 25-30, or
33-63) is a one.
...
Bit 12 is a one.
...
Bits 31 and 32 are zero and one, respectively,
and bits 64-96 are not all zeros.
...
Bits 31 and 32 are both zero, and bits 64-103 are
not all zeros.
Bits 31 and 32 are one and zero, respectively.
...
Programming Note: Bit 12 of an 8-byte short-format
PSW in storage is inverted when the 16-byte current
PSW is loaded from the following locations:
...
An assigned storage location in the ESA/390-
compatibility mode.
"

The ILC is usually 0 (with exceptions) and the PSW address was updated.


Note: For TCG we miss many of these validity checks. For KVM, most 
should be triggered when running the VCPU AFAIK (that means, we don't 
have to check for any other scenarios here). Checking for the special 
case as given in the programming note should be sufficient.


I'll have to think about how to best handle that for TCG (mazbe what I 
proposed works). We could ignore TCG for now and add a TODO. Then, just 
wrap the exception in a "if (kvm_enabled())". You could also document 
why we only have to check for this very specific bit and not the other 
bits (handled by HW later).

-- 

Thanks,

David / dhildenb