[Qemu-devel] [PATCH 1/2] s390x: Add some documentation in opcode list

[Qemu-devel] [PATCH 1/2] s390x: Add some documentation in opcode list

[Alexander Graf]

I find it really hard to grasp what each field in the opcode list means.
Slowly walking through its semantics myself, I figured I'd write a small
summary at the top of the file to make life easier for me and whoever
looks at the file next.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 target-s390x/insn-data.def | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/target-s390x/insn-data.def b/target-s390x/insn-data.def
index 8d8e47e..48e979e 100644
--- a/target-s390x/insn-data.def
+++ b/target-s390x/insn-data.def
@@ -1,3 +1,24 @@
+/*
+ *  Arguments to the opcode prototypes
+ *
+ *  C(OPC,    NAME,    FMT,   FAC, I1, I2, P, W, OP, CC)
+ *  D(OPC,    NAME,    FMT,   FAC, I1, I2, P, W, OP, CC, DATA)
+ *
+ *  OPC  = (op << 8) | op2 where op is the major, op2 the minor opcode
+ *  NAME = name of the opcode, used internally
+ *  FMT  = format of the opcode (defined in insn-format.def)
+ *  FAC  = facility the opcode is available in (defined in DisasFacility)
+ *  I1   = func in1_xx fills o->in1
+ *  I2   = func in2_xx fills o->in2
+ *  P    = func prep_xx initializes o->*out*
+ *  W    = func wout_xx writes o->*out* somewhere
+ *  OP   = func op_xx does the bulk of the operation
+ *  CC   = func cout_xx defines how cc should get set
+ *  DATA = immediate argument to op_xx function
+ *
+ *  The helpers get called in order: I1, I2, P, OP, W, CC
+ */
+
 /* ADD */
     C(0x1a00, AR,      RR_a,  Z,   r1, r2, new, r1_32, add, adds32)
     C(0xb9f8, ARK,     RRF_a, DO,  r2, r3, new, r1_32, add, adds32)
-- 
1.7.12.4

[Qemu-devel] [PATCH 2/2] s390x: Add laa and laag instructions

[Alexander Graf]

We're currently missing the laa and laag instructions in our emulation.
In fact, we're missing the complete "interlocked-access facility 1" which
is part of zEC12. However, I really only needed the laa instruction for now.

Signed-off-by: Alexander Graf <agraf@suse.de>

---

This really should implement all the other atomic load&modify instructions,
but I'd like to make sure we have a smart scheme to implement them. I couldn't
really see a good way on making them fit into the current load/store mechanism
we have in place.

So the best thing I can think of right now for those instructions is to add
flags in the data field what type of operation we're looking at and just map
them all to the same op function.
---
 target-s390x/insn-data.def |  3 +++
 target-s390x/translate.c   | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/target-s390x/insn-data.def b/target-s390x/insn-data.def
index 48e979e..80a640c 100644
--- a/target-s390x/insn-data.def
+++ b/target-s390x/insn-data.def
@@ -359,6 +359,9 @@
     C(0xe371, LAY,     RXY_a, LD,  0, a2, 0, r1, mov2, 0)
 /* LOAD ADDRESS RELATIVE LONG */
     C(0xc000, LARL,    RIL_b, Z,   0, ri2, 0, r1, mov2, 0)
+/* LOAD AND ADD */
+    C(0xebf8, LAA,     RSY_a, ILA, r3_32s, a2, new, r1, laa, adds32)
+    C(0xebe8, LAAG,    RSY_a, ILA, r3, a2, new, r1, laag, adds64)
 /* LOAD AND TEST */
     C(0x1200, LTR,     RR_a,  Z,   0, r2_o, 0, cond_r1r2_32, mov2, s32)
     C(0xb902, LTGR,    RRE,   Z,   0, r2_o, 0, r1, mov2, s64)
diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 8784112..40e7337 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -1118,6 +1118,7 @@ typedef enum DisasFacility {
     FAC_PC,                 /* population count */
     FAC_SCF,                /* store clock fast */
     FAC_SFLE,               /* store facility list extended */
+    FAC_ILA,                /* interlocked access facility 1 */
 } DisasFacility;
 
 struct DisasInsn {
@@ -2505,6 +2506,48 @@ static ExitStatus op_lurag(DisasContext *s, DisasOps *o)
 }
 #endif
 
+static ExitStatus op_laa(DisasContext *s, DisasOps *o)
+{
+    TCGv_i64 m2 = tcg_temp_new_i64();
+
+    /* XXX should be atomic */
+    tcg_gen_qemu_ld32s(m2, o->in2, get_mem_index(s));
+
+    /* Set r1 to the unmodified contents of m2 */
+    tcg_gen_mov_i64(o->out, m2);
+
+    /* m2 = r3 + m2 */
+    tcg_gen_add_i64(m2, o->in1, m2);
+    tcg_gen_qemu_st32(m2, o->in2, get_mem_index(s));
+
+    /* Set in2 to the input operand for cc calculation */
+    tcg_gen_mov_i64(o->in2, o->out);
+
+    tcg_temp_free_i64(m2);
+    return NO_EXIT;
+}
+
+static ExitStatus op_laag(DisasContext *s, DisasOps *o)
+{
+    TCGv_i64 m2 = tcg_temp_new_i64();
+
+    /* XXX should be atomic */
+    tcg_gen_qemu_ld64(m2, o->in2, get_mem_index(s));
+
+    /* Set r1 to the unmodified contents of m2 */
+    tcg_gen_mov_i64(o->out, m2);
+
+    /* m2 = r3 + m2 */
+    tcg_gen_add_i64(m2, o->in1, m2);
+    tcg_gen_qemu_st64(m2, o->in2, get_mem_index(s));
+
+    /* Set in2 to the input operand for cc calculation */
+    tcg_gen_mov_i64(o->in2, o->out);
+
+    tcg_temp_free_i64(m2);
+    return NO_EXIT;
+}
+
 static ExitStatus op_mov2(DisasContext *s, DisasOps *o)
 {
     o->out = o->in2;
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 2/2] s390x: Add laa and laag instructions

[Richard Henderson]

On 05/07/2015 06:12 PM, Alexander Graf wrote:
> +static ExitStatus op_laa(DisasContext *s, DisasOps *o)
> +{
> +    TCGv_i64 m2 = tcg_temp_new_i64();
> +
> +    /* XXX should be atomic */
> +    tcg_gen_qemu_ld32s(m2, o->in2, get_mem_index(s));
> +
> +    /* Set r1 to the unmodified contents of m2 */
> +    tcg_gen_mov_i64(o->out, m2);
> +
> +    /* m2 = r3 + m2 */
> +    tcg_gen_add_i64(m2, o->in1, m2);
> +    tcg_gen_qemu_st32(m2, o->in2, get_mem_index(s));
> +
> +    /* Set in2 to the input operand for cc calculation */
> +    tcg_gen_mov_i64(o->in2, o->out);
> +
> +    tcg_temp_free_i64(m2);
> +    return NO_EXIT;
> +}

I don't believe that this computes the right cc.
You need to place m2 into in2, not out.

That said, maybe

  tcg_temp_free_i64(o->in2);
  o->in2 = m2;

is the right way to perform that copy.  Since we
know that in2 was in2_a2, allocating a temp for an
address, we also know we can both (1) free it and
(2) put another temp in there that will be freed.

Don't worry about the atomic-ness of the operation
until some of the many patch sets going round on
that very subject are resolved.  This will be good
enough for system mode for now.

I think maybe the whole target ought to be cleaned
up for the new TCGMemOps.  For this, you could share
the code for LAA and LAAG with the TCGMemOp in insn->data.


r~

Re: [Qemu-devel] [PATCH 2/2] s390x: Add laa and laag instructions

[Alexander Graf]

On 05/12/2015 05:53 AM, Richard Henderson wrote:
> On 05/07/2015 06:12 PM, Alexander Graf wrote:
>> +static ExitStatus op_laa(DisasContext *s, DisasOps *o)
>> +{
>> +    TCGv_i64 m2 = tcg_temp_new_i64();
>> +
>> +    /* XXX should be atomic */
>> +    tcg_gen_qemu_ld32s(m2, o->in2, get_mem_index(s));
>> +
>> +    /* Set r1 to the unmodified contents of m2 */
>> +    tcg_gen_mov_i64(o->out, m2);
>> +
>> +    /* m2 = r3 + m2 */
>> +    tcg_gen_add_i64(m2, o->in1, m2);
>> +    tcg_gen_qemu_st32(m2, o->in2, get_mem_index(s));
>> +
>> +    /* Set in2 to the input operand for cc calculation */
>> +    tcg_gen_mov_i64(o->in2, o->out);
>> +
>> +    tcg_temp_free_i64(m2);
>> +    return NO_EXIT;
>> +}
> I don't believe that this computes the right cc.
> You need to place m2 into in2, not out.
>
> That said, maybe
>
>    tcg_temp_free_i64(o->in2);
>    o->in2 = m2;
>
> is the right way to perform that copy.  Since we
> know that in2 was in2_a2, allocating a temp for an
> address, we also know we can both (1) free it and
> (2) put another temp in there that will be freed.

Well, alternatively I could try to make out be the memory value at the 
end of the op and invent new load/store helpers for atomic instructions. 
That way the op bit should shrink significantly and we get the correct 
operand in s->out automatically.


>
> Don't worry about the atomic-ness of the operation
> until some of the many patch sets going round on
> that very subject are resolved.  This will be good
> enough for system mode for now.
>
> I think maybe the whole target ought to be cleaned
> up for the new TCGMemOps.  For this, you could share
> the code for LAA and LAAG with the TCGMemOp in insn->data.

That's an interesting idea. Let me see what I can pull off.


Alex

Re: [Qemu-devel] [PATCH 1/2] s390x: Add some documentation in opcode list

[Richard Henderson]

On 05/07/2015 06:12 PM, Alexander Graf wrote:
> I find it really hard to grasp what each field in the opcode list means.
> Slowly walking through its semantics myself, I figured I'd write a small
> summary at the top of the file to make life easier for me and whoever
> looks at the file next.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  target-s390x/insn-data.def | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)

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


r~