[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3] target-s390x: Implement stfl and stfle
From: |
Thomas Huth |
Subject: |
Re: [Qemu-devel] [PATCH v3] target-s390x: Implement stfl and stfle |
Date: |
Sun, 26 Feb 2017 12:22:49 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 |
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 <address@hidden>
> ---
> 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