[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 01/10] target-s390: Move facilities bits to env
From: |
Richard Henderson |
Subject: |
Re: [Qemu-devel] [PATCH 01/10] target-s390: Move facilities bits to env |
Date: |
Tue, 01 Oct 2013 08:52:11 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130805 Thunderbird/17.0.8 |
On 10/01/2013 08:48 AM, Alexander Graf wrote:
> On 09/30/2013 09:15 PM, Richard Henderson wrote:
>> 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<address@hidden>
>>>> ---
>>>> 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.
>
> I'll leave the answer to that to Andreas :).
Can we leave that for a separate patch series then?
>>>> - 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.
>
> No, it only seemed more straight forward to me from a "single source of
> information" point of view. But it really doesn't matter. Shifting in C seems
> to be easier to read :).
Fair enough. I'll rearrange the order of the patches so that we can
update STFL to use the DisasContext data.
r~