| Message ID | 20170224134411.28882-1-mmarek@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20170224134411.28882-1-mmarek@suse.com
Type: series
Subject: [Qemu-devel] [PATCH] target-s390x: Implement stfl and stfle
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/20170224134411.28882-1-mmarek@suse.com -> patchew/20170224134411.28882-1-mmarek@suse.com
* [new tag] patchew/20170224135045.10613-1-mbenes@suse.cz -> patchew/20170224135045.10613-1-mbenes@suse.cz
Switched to a new branch 'test'
3daba9b target-s390x: Implement stfl and stfle
=== OUTPUT BEGIN ===
Checking PATCH 1/1: target-s390x: Implement stfl and stfle...
ERROR: braces {} are necessary for all arms of this statement
#108: FILE: target/s390x/misc_helper.c:522:
+ if (need <= len)
[...]
+ else
[...]
total: 1 errors, 0 warnings, 111 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
On 2017-02-24 15:51, no-reply@patchew.org wrote: > Hi, > > This series seems to have some coding style problems. See output below for > more information: [...] > === OUTPUT BEGIN === > Checking PATCH 1/1: target-s390x: Implement stfl and stfle... > ERROR: braces {} are necessary for all arms of this statement > #108: FILE: target/s390x/misc_helper.c:522: > + if (need <= len) > [...] > + else > [...] I was not aware of this coding style requirement. I will wait if I get more feedback from humans and resubmit. Michal
Am 24.02.2017 um 16:22 schrieb Michal Marek: > On 2017-02-24 15:51, no-reply@patchew.org wrote: >> Hi, >> >> This series seems to have some coding style problems. See output below for >> more information: > [...] >> === OUTPUT BEGIN === >> Checking PATCH 1/1: target-s390x: Implement stfl and stfle... >> ERROR: braces {} are necessary for all arms of this statement >> #108: FILE: target/s390x/misc_helper.c:522: >> + if (need <= len) >> [...] >> + else >> [...] > > I was not aware of this coding style requirement. I will wait if I get > more feedback from humans and resubmit. > Yes, QEMU (in contrast to the kernel) always requires {} for if/else/for/while....
On 02/25/2017 12:44 AM, Michal Marek wrote: > +DEF_HELPER_1(stfl, void, env) DEF_HELPER_FLAGS_1(stfl, TCG_CALL_NO_RWG, void, env) since this touches no registers, and only writes to lomem which afaik cannot fault in kernel mode. > +DEF_HELPER_3(stfle, i64, env, i64, i64) Unfortunately, we are writing to cc_op, so we do have a TCG register write, so I guess this is the best we can do here. > +static int do_stfle(CPUS390XState *env, uint64_t addr, int len) > +{ > + S390CPU *cpu = s390_env_get_cpu(env); > + uint8_t data[64]; S390FeatBitmap or S390FeatInit? Or even a sizeof? Hard coding 64 certainly doesn't seem right. > + memset(data, 0, sizeof(data)); > + res = s390_fill_feat_block(cpu->model->features, S390_FEAT_TYPE_STFL, data); > + cpu_physical_memory_write(addr, data, MIN(res, len)); No, not physical memory, you need to write to virtual memory, at least for STFLE. Which, as you'll recall can be used from user-mode. r~
Dne 25.2.2017 v 01:05 Richard Henderson napsal(a): > On 02/25/2017 12:44 AM, Michal Marek wrote: >> +DEF_HELPER_1(stfl, void, env) > > DEF_HELPER_FLAGS_1(stfl, TCG_CALL_NO_RWG, void, env) > > since this touches no registers, and only writes to lomem which afaik > cannot fault in kernel mode. OK. >> +static int do_stfle(CPUS390XState *env, uint64_t addr, int len) >> +{ >> + S390CPU *cpu = s390_env_get_cpu(env); >> + uint8_t data[64]; > > S390FeatBitmap or S390FeatInit? Or even a sizeof? > Hard coding 64 certainly doesn't seem right. I will change it to something more sensible. >> + memset(data, 0, sizeof(data)); >> + res = s390_fill_feat_block(cpu->model->features, >> S390_FEAT_TYPE_STFL, data); >> + cpu_physical_memory_write(addr, data, MIN(res, len)); > > No, not physical memory, you need to write to virtual memory, at least > for STFLE. Which, as you'll recall can be used from user-mode. Oh, I did not realize that STFLE is not a privileged instruction. Thanks for the review! Michal
Dne 25.2.2017 v 21:39 Michal Marek napsal(a): > Dne 25.2.2017 v 01:05 Richard Henderson napsal(a): >> On 02/25/2017 12:44 AM, Michal Marek wrote: >>> +static int do_stfle(CPUS390XState *env, uint64_t addr, int len) >>> +{ >>> + S390CPU *cpu = s390_env_get_cpu(env); >>> + uint8_t data[64]; >> >> S390FeatBitmap or S390FeatInit? Or even a sizeof? >> Hard coding 64 certainly doesn't seem right. > > I will change it to something more sensible. I changed it to 2k, which is the maximum that the STFLE instruction can store. I did not use S390FeatBitmap or S390FeatInit, as the qemu-internal feature bit assignments differ from the PoO facility bit assignments. Michal
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..4d703990d5d7 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_1(stfl, 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..b513c0c66541 100644 --- a/target/s390x/misc_helper.c +++ b/target/s390x/misc_helper.c @@ -500,6 +500,38 @@ 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); + uint8_t data[64]; + int res; + + memset(data, 0, sizeof(data)); + res = s390_fill_feat_block(cpu->model->features, S390_FEAT_TYPE_STFL, data); + cpu_physical_memory_write(addr, data, MIN(res, len)); + + 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..aff9a0bef549 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -3628,15 +3628,17 @@ 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) +{ + check_privileged(s); + 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> --- 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 | 32 ++++++++++++++++++++++++++++++++ target/s390x/translate.c | 18 ++++++++++-------- 6 files changed, 51 insertions(+), 11 deletions(-)