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

[Alexander Graf <agraf@suse.de>]

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