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

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

[Janosch Frank]

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;
+
+    if (!(spsw & 0x8000000000000UL)) {
+        s390_program_interrupt(&cpu->env, PGM_SPECIFICATION, 0, RA_IGNORED);
+    }
     s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
 }
 #endif
-- 
2.20.1

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

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

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

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 2710 bytes --]

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.

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





[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Hildenbrand]

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

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

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1063 bytes --]

On 11/5/19 9:07 PM, David Hildenbrand wrote:
> 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:
[...]
> 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).
> 

After some discussion I got an interesting answer:
For KVM we need to expand the short psw and invert bit 12.
The next SIE entry will automatically report a PIC 6.

I'd propose that I'll fix this patch and send a v2 and you can fix TCG :)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Hildenbrand]

> Am 11.11.2019 um 14:52 schrieb Janosch Frank <frankja@linux.ibm.com>:
> 
> On 11/5/19 9:07 PM, David Hildenbrand wrote:
>>> 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:
> [...]
>> 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).
>> 
> 
> After some discussion I got an interesting answer:
> For KVM we need to expand the short psw and invert bit 12.
> The next SIE entry will automatically report a PIC 6.

He, that‘s a nice trick.

> 
> I'd propose that I'll fix this patch and send a v2 and you can fix TCG :)

Yes please. Perform the conversion unconditionally and add a comment - we should add the same checks when running the TCG main loop after modifying the PSW mask.

>