Re: [Qemu-devel] [PATCH 01/10] target-s390: Move facilities bits to env

[Richard Henderson <rth@twiddle.net>]

On 09/30/2013 11:03 AM, Alexander Graf wrote:
> On 09/23/2013 04:04 PM, Richard Henderson wrote:
>> Rather than simply hard-coding them in STFL instruction.
>>
>> Signed-off-by: Richard Henderson<rth@twiddle.net>
>> ---
>>   target-s390x/cpu.c       |  3 +++
>>   target-s390x/cpu.h       |  1 +
>>   target-s390x/translate.c | 10 +++++-----
>>   3 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>> index 3c89f8a..ff691df 100644
>> --- a/target-s390x/cpu.c
>> +++ b/target-s390x/cpu.c
>> @@ -181,6 +181,9 @@ static void s390_cpu_initfn(Object *obj)
>>       env->cpu_num = cpu_num++;
>>       env->ext_index = -1;
>>
>> +    env->facilities[0] = 0xc000000000000000ull;
>> +    env->facilities[1] = 0;
> 
> Could we add CPU definitions along the way here? I'm fine with making z9 the
> default CPU type, but we should make this explicit :).

Certainly that's what we should do.  I just hadn't yet researched the
currently correct way to do that.  I know there's some amount of out-of-date
examples in the current source base.

Pointers?

>>   static ExitStatus op_stfl(DisasContext *s, DisasOps *o)
>>   {
>> -    TCGv_i64 f, a;
>> -    /* We really ought to have more complete indication of facilities
>> -       that we implement.  Address this when STFLE is implemented.  */
>> +    TCGv_i64 f = tcg_temp_new_i64();
>> +    TCGv_i64 a = tcg_const_i64(200);
>> +
>>       check_privileged(s);
>> -    f = tcg_const_i64(0xc0000000);
>> -    a = tcg_const_i64(200);
>> +    tcg_gen_ld_i64(f, cpu_env, offsetof(CPUS390XState, facilities[0]));
>> +    tcg_gen_shri_i64(f, f, 32);
> 
> IMHO the facility list should be stored in DisasContext. That way we can check
> whether we're generating code against the correct target.

See patch 4.

As for the code we generate here, does it really matter if we load the value
from env, or have it encoded as a constant?  It still has to get stored to
memory, so it's not like the TCG optimizer is going to do anything with the
constant.


r~