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 <mma...@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