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

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

[Janosch Frank]

We need to actually fetch the cpu mask and set it. As we invert the
short psw indication in the mask, SIE will report a specification
exception, if it wasn't present in the reset psw.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/cpu.c | 12 ++++++++++--
 target/s390x/cpu.h |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 736a7903e2..40aa42e092 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -76,8 +76,16 @@ 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);
+
+    cpu->env.psw.mask = spsw & 0xffffffff80000000UL;
+    /*
+     * Invert short psw indication, so SIE will report a specification
+     * exception if it was not set.
+     */
+    cpu->env.psw.mask ^= PSW_MASK_SHORTPSW;
+    cpu->env.psw.addr = spsw & 0x7fffffffUL;
+
     s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
 }
 #endif
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 947553386f..2c687185f1 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -261,6 +261,7 @@ extern const struct VMStateDescription vmstate_s390_cpu;
 #define PSW_MASK_EXT            0x0100000000000000ULL
 #define PSW_MASK_KEY            0x00F0000000000000ULL
 #define PSW_SHIFT_KEY           52
+#define PSW_MASK_SHORTPSW	0x0008000000000000ULL
 #define PSW_MASK_MCHECK         0x0004000000000000ULL
 #define PSW_MASK_WAIT           0x0002000000000000ULL
 #define PSW_MASK_PSTATE         0x0001000000000000ULL
-- 
2.20.1

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

[David Hildenbrand]

> Am 11.11.2019 um 16:28 schrieb Janosch Frank <frankja@linux.ibm.com>:
> 
> We need to actually fetch the cpu mask and set it. As we invert the
> short psw indication in the mask, SIE will report a specification
> exception, if it wasn't present in the reset psw.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> target/s390x/cpu.c | 12 ++++++++++--
> target/s390x/cpu.h |  1 +
> 2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 736a7903e2..40aa42e092 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -76,8 +76,16 @@ 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);
> +
> +    cpu->env.psw.mask = spsw & 0xffffffff80000000UL;

ULL

> +    /*
> +     * Invert short psw indication, so SIE will report a specification
> +     * exception if it was not set.
> +     */

It would be interesting to know how the PSW mask in the PGM old PSW looks like on LPAR. IOW, „you forgot to set the short indication, here is an exception. see, the short indication is set now.“ Sounds weird, most probably nobody cares.

> +    cpu->env.psw.mask ^= PSW_MASK_SHORTPSW;
> +    cpu->env.psw.addr = spsw & 0x7fffffffUL;

Eventually also ULL

> +
>     s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
> }
> #endif
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 947553386f..2c687185f1 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -261,6 +261,7 @@ extern const struct VMStateDescription vmstate_s390_cpu;
> #define PSW_MASK_EXT            0x0100000000000000ULL
> #define PSW_MASK_KEY            0x00F0000000000000ULL
> #define PSW_SHIFT_KEY           52
> +#define PSW_MASK_SHORTPSW    0x0008000000000000ULL
> #define PSW_MASK_MCHECK         0x0004000000000000ULL
> #define PSW_MASK_WAIT           0x0002000000000000ULL
> #define PSW_MASK_PSTATE         0x0001000000000000ULL
> -- 
> 2.20.1
> 

Looks good to me

Reviewed-by: David Hildenbrand <david@redhat.com>

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191111152808.13371-1-frankja@linux.ibm.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH v2] s390x: Properly fetch the short psw on diag308 subc 0/1
Type: series
Message-id: 20191111152808.13371-1-frankja@linux.ibm.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
74d60dc s390x: Properly fetch the short psw on diag308 subc 0/1

=== OUTPUT BEGIN ===
ERROR: code indent should never use tabs
#46: FILE: target/s390x/cpu.h:268:
+#define PSW_MASK_SHORTPSW^I0x0008000000000000ULL$

total: 1 errors, 0 warnings, 25 lines checked

Commit 74d60dc486a0 (s390x: Properly fetch the short psw on diag308 subc 0/1) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20191111152808.13371-1-frankja@linux.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[Cornelia Huck]

On Mon, 11 Nov 2019 10:28:08 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> We need to actually fetch the cpu mask and set it. As we invert the
> short psw indication in the mask, SIE will report a specification
> exception, if it wasn't present in the reset psw.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/cpu.c | 12 ++++++++++--
>  target/s390x/cpu.h |  1 +
>  2 files changed, 11 insertions(+), 2 deletions(-)

So, is this change -rc material, or should it go in during the next
release? I'm a bit confused here.

[Also, does this need a change in the tcg code, or is that something
that should just be done eventually? Sorry, drowning a bit in mails
here...]

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

[David Hildenbrand]

> Am 12.11.2019 um 17:58 schrieb Cornelia Huck <cohuck@redhat.com>:
> 
> On Mon, 11 Nov 2019 10:28:08 -0500
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> We need to actually fetch the cpu mask and set it. As we invert the
>> short psw indication in the mask, SIE will report a specification
>> exception, if it wasn't present in the reset psw.
>> 
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> target/s390x/cpu.c | 12 ++++++++++--
>> target/s390x/cpu.h |  1 +
>> 2 files changed, 11 insertions(+), 2 deletions(-)
> 
> So, is this change -rc material, or should it go in during the next
> release? I'm a bit confused here.

IMHO, this is not urgent and can wait.
> 
> [Also, does this need a change in the tcg code, or is that something
> that should just be done eventually? Sorry, drowning a bit in mails
> here...]

We‘re missing many checks when loading/running a new PSW for TCG, not just this scenario. So this should be done at one point but is not urgent at all.