| Message ID | 20170225233845.25828-1-mmarek@suse.com |
|---|---|
| State | New |
| Headers | show |
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
Dne 26.2.2017 v 12:22 Thomas Huth napsal(a): > 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. Maybe. If we are doing this, a comment in the s390_features definition should be added. >> +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. Oops. It was quite late in my timezone when I sent the v3. v2 was correct. >> + 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. STFL is storing a 32bit number. But this needs fixing anyway, res needs to be rounded up to a multiple of 8. > I think you could even use s390_cpu_virt_mem_write() here to avoid the > loop completely. _That_ is the function I was looking for. >> +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)? Dumb question, but how do I signal a specification exception? s390_cpu_do_interrupt() does not seem to be prepared for it. >> +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. OK. Michal
On 26.02.2017 19:57, Michal Marek wrote: > Dne 26.2.2017 v 12:22 Thomas Huth napsal(a): >> 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(-) [...] >>> +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)? > > Dumb question, but how do I signal a specification exception? > s390_cpu_do_interrupt() does not seem to be prepared for it. Not sure, but I think something like program_interrupt(env, PGM_SPECIFICATION, 4); should do the job here. Thomas
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); } feat = find_next_bit(features, S390_FEAT_MAX, feat + 1); } + return res; } void s390_add_from_feat_block(S390FeatBitmap features, S390FeatType type, diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h index d66912178680..e3c41be08060 100644 --- a/target/s390x/cpu_features.h +++ b/target/s390x/cpu_features.h @@ -56,7 +56,7 @@ typedef uint64_t S390FeatInit[S390_FEAT_MAX / 64 + 1]; const S390FeatDef *s390_feat_def(S390Feat feat); S390Feat s390_feat_by_type_and_bit(S390FeatType type, int bit); 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); void s390_add_from_feat_block(S390FeatBitmap features, S390FeatType type, uint8_t *data); diff --git a/target/s390x/helper.h b/target/s390x/helper.h index 9102071d0aa4..fa4a617cd1ab 100644 --- a/target/s390x/helper.h +++ b/target/s390x/helper.h @@ -95,6 +95,8 @@ DEF_HELPER_FLAGS_1(stckc, TCG_CALL_NO_RWG, i64, env) DEF_HELPER_FLAGS_2(spt, TCG_CALL_NO_RWG, void, env, i64) DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env) DEF_HELPER_4(stsi, i32, env, i64, i64, i64) +DEF_HELPER_FLAGS_1(stfl, TCG_CALL_NO_RWG, void, env) +DEF_HELPER_3(stfle, i64, env, i64, i64) DEF_HELPER_FLAGS_4(lctl, TCG_CALL_NO_WG, void, env, i32, i64, i32) DEF_HELPER_FLAGS_4(lctlg, TCG_CALL_NO_WG, void, env, i32, i64, i32) DEF_HELPER_FLAGS_4(stctl, TCG_CALL_NO_WG, void, env, i32, i64, i32) diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def index 075ff597c3de..be830a42ed8d 100644 --- a/target/s390x/insn-data.def +++ b/target/s390x/insn-data.def @@ -899,6 +899,8 @@ C(0xb202, STIDP, S, Z, la2, 0, new, m1_64, stidp, 0) /* STORE CPU TIMER */ C(0xb209, STPT, S, Z, la2, 0, new, m1_64, stpt, 0) +/* STORE FACILITY LIST EXTENDED */ + C(0xb2b0, STFLE, S, SFLE, 0, a2, 0, 0, stfle, 0) /* STORE FACILITY LIST */ C(0xb2b1, STFL, S, Z, 0, 0, 0, 0, stfl, 0) /* STORE PREFIX */ 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)); + 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]); + } + + return res; +} + +uint64_t HELPER(stfle)(CPUS390XState *env, uint64_t a0, uint64_t r0) +{ + int need, len = r0 & 0xff; + + 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); +} + 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; }
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(-)