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