[Qemu-devel] [PATCH] s390x: Implement SAM{24,31,64}

[Qemu-devel] [PATCH] s390x: Implement SAM{24,31,64}

[Alexander Graf]

The SAM instructions simply change 2 bits in PSW.MASK to advertise
the current memory mode. While we can't fully guarantee that 31 bit
mode (or even remotely 24 bit mode) actually work correctly, we don't
check whether lpswe modifies these bits, so we shouldn't keep the
guest from executing SAM instructions either.

This patch implements all SAM instrutions with their actual PSW changing
semantics, making more recent Linux kernels boot properly which do issue
a SAM31 call during early boot.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 target-s390x/insn-data.def |  6 +++---
 target-s390x/translate.c   | 12 ++++++++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/target-s390x/insn-data.def b/target-s390x/insn-data.def
index b42ebb6..4d2feb6 100644
--- a/target-s390x/insn-data.def
+++ b/target-s390x/insn-data.def
@@ -744,9 +744,9 @@
 /* SERVICE CALL LOGICAL PROCESSOR (PV hypercall) */
     C(0xb220, SERVC,   RRE,   Z,   r1_o, r2_o, 0, 0, servc, 0)
 /* SET ADDRESSING MODE */
-    /* We only do 64-bit, so accept this as a no-op.
-       Let SAM24 and SAM31 signal illegal instruction.  */
-    C(0x010e, SAM64,   E,     Z,   0, 0, 0, 0, 0, 0)
+    D(0x010c, SAM24,   E,     Z,   0, 0, 0, 0, sam, 0, 0)
+    D(0x010d, SAM31,   E,     Z,   0, 0, 0, 0, sam, 0, 1)
+    D(0x010e, SAM64,   E,     Z,   0, 0, 0, 0, sam, 0, 3)
 /* SET ADDRESS SPACE CONTROL FAST */
     C(0xb279, SACF,    S,     Z,   0, a2, 0, 0, sacf, 0)
 /* SET CLOCK */
diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 0cb036f..827cda4 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2927,6 +2927,18 @@ static ExitStatus op_sacf(DisasContext *s, DisasOps *o)
 }
 #endif
 
+static ExitStatus op_sam(DisasContext *s, DisasOps *o)
+{
+    int sam = s->insn->data;
+    TCGv_i64 tsam = tcg_const_i64(sam);
+
+    /* Overwrite PSW_MASK_64 and PSW_MASK_32 */
+    tcg_gen_deposit_i64(psw_mask, psw_mask, tsam, 31, 2);
+
+    tcg_temp_free_i64(tsam);
+    return EXIT_PC_STALE;
+}
+
 static ExitStatus op_sar(DisasContext *s, DisasOps *o)
 {
     int r1 = get_field(s->fields, r1);
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH] s390x: Implement SAM{24,31,64}

[Bastian Koppelmann]

On 11/04/2014 08:19 PM, Alexander Graf wrote:
> +static ExitStatus op_sam(DisasContext *s, DisasOps *o)
> +{
> +    int sam = s->insn->data;
> +    TCGv_i64 tsam = tcg_const_i64(sam);
> +
> +    /* Overwrite PSW_MASK_64 and PSW_MASK_32 */
> +    tcg_gen_deposit_i64(psw_mask, psw_mask, tsam, 31, 2);
> +
> +    tcg_temp_free_i64(tsam);
> +    return EXIT_PC_STALE;
> +}
> +
You forgot to zero out bits 64-103 of psw, in case of sam24 and bits 
64-96 in case of sam31.
Also you forgot to add 2 (the instruction length) to bits 64-127 of psw 
or if this is a target of EXECUTE/EXECUTE RELATIVE LONG add 4/6.
>   static ExitStatus op_sar(DisasContext *s, DisasOps *o)
>   {
>       int r1 = get_field(s->fields, r1);

Re: [Qemu-devel] [PATCH] s390x: Implement SAM{24,31,64}

[Alexander Graf]

On 04.11.14 23:02, Bastian Koppelmann wrote:
> 
> On 11/04/2014 08:19 PM, Alexander Graf wrote:
>> +static ExitStatus op_sam(DisasContext *s, DisasOps *o)
>> +{
>> +    int sam = s->insn->data;
>> +    TCGv_i64 tsam = tcg_const_i64(sam);
>> +
>> +    /* Overwrite PSW_MASK_64 and PSW_MASK_32 */
>> +    tcg_gen_deposit_i64(psw_mask, psw_mask, tsam, 31, 2);
>> +
>> +    tcg_temp_free_i64(tsam);
>> +    return EXIT_PC_STALE;
>> +}
>> +
> You forgot to zero out bits 64-103 of psw, in case of sam24 and bits
> 64-96 in case of sam31.

These are the address bits. PSW contains an "addr" and a "mask" field.
"addr" is PC, "mask" is similar to MSR on PPC or EFER on x86.

Other bits of the code will take care of masking out unused address bits
for 31 bit mode (check out fix_address() in mem_helper.c for example).

We don't really implent 24bit addressing mode - and I doubt we will in
the near future. Today our only target is Linux - and there simply is no
24bit Linux out there ;).

> Also you forgot to add 2 (the instruction length) to bits 64-127 of psw

This happens automatically. Each instruction carries its length in the
first 2 bits, so the instruction walker can automatically increment PC.

> or if this is a target of EXECUTE/EXECUTE RELATIVE LONG add 4/6.

EXECUTE is tricky. Basically EXECUTE is an instruction that behaves like
the instruction that a memory reference points to, but at the location
the EXECUTE is actually in. Today, we treat EXECUTE as a very special
instruction with only a small number of subinstructions that it handles
(namely the ones gcc emits).

But thanks a lot for the thorough review :)


Alex

Re: [Qemu-devel] [PATCH] s390x: Implement SAM{24,31,64}

[Bastian Koppelmann]

On 11/04/2014 08:19 PM, Alexander Graf wrote:
> The SAM instructions simply change 2 bits in PSW.MASK to advertise
> the current memory mode. While we can't fully guarantee that 31 bit
> mode (or even remotely 24 bit mode) actually work correctly, we don't
> check whether lpswe modifies these bits, so we shouldn't keep the
> guest from executing SAM instructions either.
>
> This patch implements all SAM instrutions with their actual PSW changing
> semantics, making more recent Linux kernels boot properly which do issue
> a SAM31 call during early boot.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>   target-s390x/insn-data.def |  6 +++---
>   target-s390x/translate.c   | 12 ++++++++++++
>   2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/target-s390x/insn-data.def b/target-s390x/insn-data.def
> index b42ebb6..4d2feb6 100644
> --- a/target-s390x/insn-data.def
> +++ b/target-s390x/insn-data.def
> @@ -744,9 +744,9 @@
>   /* SERVICE CALL LOGICAL PROCESSOR (PV hypercall) */
>       C(0xb220, SERVC,   RRE,   Z,   r1_o, r2_o, 0, 0, servc, 0)
>   /* SET ADDRESSING MODE */
> -    /* We only do 64-bit, so accept this as a no-op.
> -       Let SAM24 and SAM31 signal illegal instruction.  */
> -    C(0x010e, SAM64,   E,     Z,   0, 0, 0, 0, 0, 0)
> +    D(0x010c, SAM24,   E,     Z,   0, 0, 0, 0, sam, 0, 0)
> +    D(0x010d, SAM31,   E,     Z,   0, 0, 0, 0, sam, 0, 1)
> +    D(0x010e, SAM64,   E,     Z,   0, 0, 0, 0, sam, 0, 3)
>   /* SET ADDRESS SPACE CONTROL FAST */
>       C(0xb279, SACF,    S,     Z,   0, a2, 0, 0, sacf, 0)
>   /* SET CLOCK */
> diff --git a/target-s390x/translate.c b/target-s390x/translate.c
> index 0cb036f..827cda4 100644
> --- a/target-s390x/translate.c
> +++ b/target-s390x/translate.c
> @@ -2927,6 +2927,18 @@ static ExitStatus op_sacf(DisasContext *s, DisasOps *o)
>   }
>   #endif
>   
> +static ExitStatus op_sam(DisasContext *s, DisasOps *o)
> +{
> +    int sam = s->insn->data;
> +    TCGv_i64 tsam = tcg_const_i64(sam);
> +
> +    /* Overwrite PSW_MASK_64 and PSW_MASK_32 */
> +    tcg_gen_deposit_i64(psw_mask, psw_mask, tsam, 31, 2);
> +
> +    tcg_temp_free_i64(tsam);
> +    return EXIT_PC_STALE;
> +}
> +
>   static ExitStatus op_sar(DisasContext *s, DisasOps *o)
>   {
>       int r1 = get_field(s->fields, r1);
Reviewed-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>

Re: [Qemu-devel] [PATCH] s390x: Implement SAM{24,31,64}

[Richard Henderson]

On 11/04/2014 09:19 PM, Alexander Graf wrote:
> The SAM instructions simply change 2 bits in PSW.MASK to advertise
> the current memory mode. While we can't fully guarantee that 31 bit
> mode (or even remotely 24 bit mode) actually work correctly, we don't
> check whether lpswe modifies these bits, so we shouldn't keep the
> guest from executing SAM instructions either.
> 
> This patch implements all SAM instrutions with their actual PSW changing
> semantics, making more recent Linux kernels boot properly which do issue
> a SAM31 call during early boot.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Ho hum.  Reminds me that I should revive my tgt-s390 branch that
got bogged down last year on facilities management.

I think this is technically missing a SPECIFICATION exception if
the PC is out of range for the new mode.  But otherwise...

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~