Re: [Qemu-devel] [PATCH v3] target-s390x: Implement stfl and stfle

[Thomas Huth <thuth@redhat.com>]

On 26.02.2017 00:38, Michal Marek wrote:
> The implementation is partially cargo cult based, but it works for the
> linux kernel use case.
> 
> Signed-off-by: Michal Marek <mmarek@suse.com>
> ---
> v3:
>  - Initialize the buffer in do_stfle()
> v2:
>  - STFLE is not a privileged instruction, go through the MMU to store the
>    result
>  - annotate the stfl helper with TCG_CALL_NO_RWG
>  - Use a large enough buffer to hold the feature bitmap
>  - Fix coding style of the stfle helper
> ---
>  target/s390x/cpu_features.c |  6 ++++--
>  target/s390x/cpu_features.h |  2 +-
>  target/s390x/helper.h       |  2 ++
>  target/s390x/insn-data.def  |  2 ++
>  target/s390x/misc_helper.c  | 36 ++++++++++++++++++++++++++++++++++++
>  target/s390x/translate.c    | 17 +++++++++--------
>  6 files changed, 54 insertions(+), 11 deletions(-)
> 
> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> index 42fd9d792bc8..d77c560380c4 100644
> --- a/target/s390x/cpu_features.c
> +++ b/target/s390x/cpu_features.c
> @@ -286,11 +286,11 @@ void s390_init_feat_bitmap(const S390FeatInit init, S390FeatBitmap bitmap)
>      }
>  }
>  
> -void s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
> +int s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
>                            uint8_t *data)
>  {
>      S390Feat feat;
> -    int bit_nr;
> +    int bit_nr, res = 0;
>  
>      if (type == S390_FEAT_TYPE_STFL && test_bit(S390_FEAT_ZARCH, features)) {
>          /* z/Architecture is always active if around */
> @@ -303,9 +303,11 @@ void s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
>              bit_nr = s390_features[feat].bit;
>              /* big endian on uint8_t array */
>              data[bit_nr / 8] |= 0x80 >> (bit_nr % 8);
> +            res = MAX(res, bit_nr / 8 + 1);

Not sure whether the assumption is valid, but it seems like the bit
numbers are stored in ascending order in the s390_features array, so you
could theoretically also get along without the res variable and the MAX
calculation and just return the last bit_nr / 8 + 1 at the end.

>          }
>          feat = find_next_bit(features, S390_FEAT_MAX, feat + 1);
>      }
> +    return res;
>  }
[...]
> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> index c9604ea9c728..b51454ea7861 100644
> --- a/target/s390x/misc_helper.c
> +++ b/target/s390x/misc_helper.c
> @@ -500,6 +500,42 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0,
>      return cc;
>  }
>  
> +static int do_stfle(CPUS390XState *env, uint64_t addr, int len)
> +{
> +    S390CPU *cpu = s390_env_get_cpu(env);
> +    /* 256 doublewords as per STFLE documentation */
> +    uint8_t data[256 * 8] = { 0 };
> +    int i, res;
> +
> +    memset(data, 0, sizeof(data));

You've alread set data to 0 with the "= { 0 }" initializer, so the
memset() is redundant here (or you can remove the "= { 0 }" initializer
instead.

> +    res = s390_fill_feat_block(cpu->model->features, S390_FEAT_TYPE_STFL, data);
> +    for (i = 0; i < MIN(res, len); i++) {
> +        cpu_stb_data(env, addr + i, data[i]);

Since we know that we're using 64-bit values here, why not using
cpu_stq_data instead? That should be faster.
I think you could even use s390_cpu_virt_mem_write() here to avoid the
loop completely.

> +    }
> +
> +    return res;
> +}
> +
> +uint64_t HELPER(stfle)(CPUS390XState *env, uint64_t a0, uint64_t r0)
> +{
> +    int need, len = r0 & 0xff;

According to the POP spec, the address "must be designated on a
doubleword boundary; otherwise, a specification exception is recognized."
Could you please add this check here (or in translate.c)?

> +    need = do_stfle(env, a0, len * 8);
> +    need = DIV_ROUND_UP(need, 8);
> +    if (need <= len) {
> +        env->cc_op = 0;
> +    } else {
> +        env->cc_op = 3;
> +    }
> +
> +    return (r0 & ~0xffLL) | (need - 1);
> +}
> +
> +void HELPER(stfl)(CPUS390XState *env)
> +{
> +    do_stfle(env, 200, 4);

It's maybe nicer to use offsetof(LowCore, stfl_fac_list) instead of the
magic value 200 here.

> +}
> +
>  uint32_t HELPER(sigp)(CPUS390XState *env, uint64_t order_code, uint32_t r1,
>                        uint64_t cpu_addr)
>  {
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index 01c62176bf70..3a569d3cc0ad 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -3628,15 +3628,16 @@ 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.  */
>      check_privileged(s);
> -    f = tcg_const_i64(0xc0000000);
> -    a = tcg_const_i64(200);
> -    tcg_gen_qemu_st32(f, a, get_mem_index(s));
> -    tcg_temp_free_i64(f);
> -    tcg_temp_free_i64(a);
> +    gen_helper_stfl(cpu_env);
> +    return NO_EXIT;
> +}
> +
> +static ExitStatus op_stfle(DisasContext *s, DisasOps *o)
> +{
> +    potential_page_fault(s);
> +    gen_helper_stfle(regs[0], cpu_env, o->in2, regs[0]);
> +    set_cc_static(s);
>      return NO_EXIT;
>  }

 Thomas