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

[Alexander Graf <agraf@suse.de>]

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 :).

> +
>       if (tcg_enabled()&&  !inited) {
>           inited = true;
>           s390x_translate_init();
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 8be5648..746aec8 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -136,6 +136,7 @@ typedef struct CPUS390XState {
>       CPU_COMMON
>
>       /* reset does memset(0) up to here */
> +    uint64_t facilities[2];
>
>       int cpu_num;
>       uint8_t *storage_keys;
> diff --git a/target-s390x/translate.c b/target-s390x/translate.c
> index afe90eb..d4dc8ea 100644
> --- a/target-s390x/translate.c
> +++ b/target-s390x/translate.c
> @@ -3230,12 +3230,12 @@ static ExitStatus op_spt(DisasContext *s, DisasOps *o)
>
>   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.


Alex