| Message ID | 20170508151707.5434-2-rth@twiddle.net |
|---|---|
| State | New |
| Headers | show |
On 2017-05-08 08:17, Richard Henderson wrote: > At the same time, improve STORE FACILITIES LIST > so that we don't hard-code the list for all cpus. > > Signed-off-by: Richard Henderson <rth@twiddle.net> > --- > target/s390x/helper.h | 2 ++ > target/s390x/insn-data.def | 2 ++ > target/s390x/misc_helper.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++ > target/s390x/translate.c | 17 ++++++++------- > 4 files changed, 67 insertions(+), 8 deletions(-) > > diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c > index eca8244..5263726 100644 > --- a/target/s390x/misc_helper.c > +++ b/target/s390x/misc_helper.c > @@ -678,3 +678,57 @@ void HELPER(per_ifetch)(CPUS390XState *env, uint64_t addr) > } > } > #endif > + > +/* The maximum bit defined at the moment is 129. */ > +#define MAX_STFL_WORDS 3 Could it be computed from S390_FEAT_MAX? in gen-features.c, S390_FEAT_MAX / 64 + 1 is used. > + > +/* Canonicalize the current cpu's features into the 64-bit words required > + by STFLE. Return the index-1 of the max word that is non-zero. */ > +static unsigned do_stfle(CPUS390XState *env, uint64_t words[MAX_STFL_WORDS]) > +{ > + S390CPU *cpu = s390_env_get_cpu(env); > + const unsigned long *features = cpu->model->features; > + unsigned max_bit = 0; > + S390Feat feat; > + > + memset(words, 0, sizeof(uint64_t) * MAX_STFL_WORDS); > + > + for (feat = find_first_bit(features, S390_FEAT_MAX); > + feat < S390_FEAT_MAX; > + feat = find_next_bit(features, S390_FEAT_MAX, feat + 1)) { > + const S390FeatDef *def = s390_feat_def(feat); > + if (def->type == S390_FEAT_TYPE_STFL) { > + unsigned bit = def->bit; > + if (bit > max_bit) { > + max_bit = bit; > + } > + assert(bit / 64 < MAX_STFL_WORDS); > + words[bit / 64] |= 1ULL << (63 - bit % 64); > + } > + } > + > + return max_bit / 64; > +} Is there a reason to not use s390_fill_feat_block here? I guess max_bit can be compute using find_last_bit. It's probably a bit less optimal, but if we care about STFLE performance, we should probably avoid recomputing the features words each time. Anyway if there is a good reason to not use s390_fill_feat_block, the zArch active bit should also be handled here. Otherwise it looks fine to me. It's probably a discussion for later patches, but should we also implement a TCG feature mask, like for example on PowerPC? Currently the only allowed CPU for TCG is z900, which makes this code almost useless. And while QEMU implements many features from newer CPU, some of them are missing and we don't want them to appear in the feature list. Aurelien
On 05/09/2017 01:14 AM, Aurelien Jarno wrote: >> +/* The maximum bit defined at the moment is 129. */ >> +#define MAX_STFL_WORDS 3 > > Could it be computed from S390_FEAT_MAX? in gen-features.c, > S390_FEAT_MAX / 64 + 1 is used. No, because the features list in cpu_features_def.h bears no relation to the facilities bit index (as seen in the third column of s390_features structure). > Is there a reason to not use s390_fill_feat_block here? Yes. I used s390_fill_feat_block in the v1 patch and changed this in response to review from David Hildenbrand. Using s390_fill_feat_block isn't completely trivial. It stores into a big-endian uint8_t array, which means that (for a little-endian host) we have to be careful how we copy that data to the guest, lest we inadvertently byte swap again. Here in v2 I store into a host-endian uint64_t array, which makes the ultimate users of this helper more straightforward. > Otherwise it looks fine to me. It's probably a discussion for later > patches, but should we also implement a TCG feature mask, like for > example on PowerPC? Currently the only allowed CPU for TCG is z900, > which makes this code almost useless. And while QEMU implements many > features from newer CPU, some of them are missing and we don't want > them to appear in the feature list. This is complicated by the fact that for a long time, the kernel checked for an exact match of facilities present. From linux 3.13 arch/s390/kernel/head.S: # List of facilities that are required. If not all facilities are present # the kernel will crash. Format is number of facility words with bits set, # followed by the facility words. #if defined(CONFIG_64BIT) #if defined(CONFIG_MARCH_ZEC12) .long 3, 0xc100efe3, 0xf46ce800, 0x00400000 #elif defined(CONFIG_MARCH_Z196) .long 2, 0xc100efe3, 0xf46c0000 #elif defined(CONFIG_MARCH_Z10) .long 2, 0xc100efe3, 0xf0680000 #elif defined(CONFIG_MARCH_Z9_109) .long 1, 0xc100efc3 #elif defined(CONFIG_MARCH_Z990) .long 1, 0xc0002000 #elif defined(CONFIG_MARCH_Z900) .long 1, 0xc0000000 #endif I'm pleased to see this appears to have been dropped at some point; I cannot see it present in linux 4.11. However, this still applies to the kernels supplied by the shipping distributions. Which means that we cannot mask *anything* from TCG, including the useless stuff like hexadecimal floating point, and still have the kernel boot. So in practice a feature mask for TCG will be not only useless but actively harmful. r~
On 05/09/2017 07:51 AM, Richard Henderson wrote: > I'm pleased to see this appears to have been dropped at some point; I cannot > see it present in linux 4.11. However, this still applies to the kernels > supplied by the shipping distributions. More accurately, it has been moved to arch/s390/kernel/als.c. But from what I can see from arch/s390/tools/gen_facilities.c, they have now properly pruned the required list to those that are *really* required. r~
diff --git a/target/s390x/helper.h b/target/s390x/helper.h index 9102071..01adb50 100644 --- a/target/s390x/helper.h +++ b/target/s390x/helper.h @@ -83,6 +83,8 @@ DEF_HELPER_FLAGS_5(calc_cc, TCG_CALL_NO_RWG_SE, i32, env, i32, i64, i64, i64) DEF_HELPER_FLAGS_2(sfpc, TCG_CALL_NO_RWG, void, env, i64) DEF_HELPER_FLAGS_2(sfas, TCG_CALL_NO_WG, void, env, i64) DEF_HELPER_FLAGS_1(popcnt, TCG_CALL_NO_RWG_SE, i64, i64) +DEF_HELPER_FLAGS_1(stfl, TCG_CALL_NO_RWG, void, env) +DEF_HELPER_2(stfle, i32, env, i64) #ifndef CONFIG_USER_ONLY DEF_HELPER_3(servc, i32, env, i64, i64) diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def index 075ff59..b6702da 100644 --- a/target/s390x/insn-data.def +++ b/target/s390x/insn-data.def @@ -747,6 +747,8 @@ C(0xe33e, STRV, RXY_a, Z, la2, r1_32u, new, m1_32, rev32, 0) C(0xe32f, STRVG, RXY_a, Z, la2, r1_o, new, m1_64, rev64, 0) +/* STORE FACILITY LIST EXTENDED */ + C(0xb2b0, STFLE, S, SFLE, 0, a2, 0, 0, stfle, 0) /* STORE FPC */ C(0xb29c, STFPC, S, Z, 0, a2, new, m2_32, efpc, 0) diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c index eca8244..5263726 100644 --- a/target/s390x/misc_helper.c +++ b/target/s390x/misc_helper.c @@ -678,3 +678,57 @@ void HELPER(per_ifetch)(CPUS390XState *env, uint64_t addr) } } #endif + +/* The maximum bit defined at the moment is 129. */ +#define MAX_STFL_WORDS 3 + +/* Canonicalize the current cpu's features into the 64-bit words required + by STFLE. Return the index-1 of the max word that is non-zero. */ +static unsigned do_stfle(CPUS390XState *env, uint64_t words[MAX_STFL_WORDS]) +{ + S390CPU *cpu = s390_env_get_cpu(env); + const unsigned long *features = cpu->model->features; + unsigned max_bit = 0; + S390Feat feat; + + memset(words, 0, sizeof(uint64_t) * MAX_STFL_WORDS); + + for (feat = find_first_bit(features, S390_FEAT_MAX); + feat < S390_FEAT_MAX; + feat = find_next_bit(features, S390_FEAT_MAX, feat + 1)) { + const S390FeatDef *def = s390_feat_def(feat); + if (def->type == S390_FEAT_TYPE_STFL) { + unsigned bit = def->bit; + if (bit > max_bit) { + max_bit = bit; + } + assert(bit / 64 < MAX_STFL_WORDS); + words[bit / 64] |= 1ULL << (63 - bit % 64); + } + } + + return max_bit / 64; +} + +void HELPER(stfl)(CPUS390XState *env) +{ + uint64_t words[MAX_STFL_WORDS]; + + do_stfle(env, words); + cpu_stl_data(env, 200, words[0] >> 32); +} + +uint32_t HELPER(stfle)(CPUS390XState *env, uint64_t addr) +{ + uint64_t words[MAX_STFL_WORDS]; + unsigned count_m1 = env->regs[0] & 0xff; + unsigned max_m1 = do_stfle(env, words); + unsigned i; + + for (i = 0; i <= count_m1; ++i) { + cpu_stq_data(env, addr + 8 * i, words[i]); + } + + env->regs[0] = deposit64(env->regs[0], 0, 8, max_m1); + return (count_m1 >= max_m1 ? 0 : 3); +} diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 01c6217..69940e3 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -3628,15 +3628,8 @@ 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; } @@ -3802,6 +3795,14 @@ static ExitStatus op_sturg(DisasContext *s, DisasOps *o) } #endif +static ExitStatus op_stfle(DisasContext *s, DisasOps *o) +{ + potential_page_fault(s); + gen_helper_stfle(cc_op, cpu_env, o->in2); + set_cc_static(s); + return NO_EXIT; +} + static ExitStatus op_st8(DisasContext *s, DisasOps *o) { tcg_gen_qemu_st8(o->in1, o->in2, get_mem_index(s));
At the same time, improve STORE FACILITIES LIST so that we don't hard-code the list for all cpus. Signed-off-by: Richard Henderson <rth@twiddle.net> --- target/s390x/helper.h | 2 ++ target/s390x/insn-data.def | 2 ++ target/s390x/misc_helper.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++ target/s390x/translate.c | 17 ++++++++------- 4 files changed, 67 insertions(+), 8 deletions(-)