From: Richard Henderson <richard.henderson@linaro.org>
To: LIU Zhiwei <zhiwei_liu@c-sky.com>,
alistair23@gmail.com, chihmin.chao@sifive.com,
palmer@dabbelt.com
Cc: wenmeng_zhang@c-sky.com, wxy194768@alibaba-inc.com,
linux-csky@vger.kernel.org, qemu-devel@nongnu.org,
qemu-riscv@nongnu.org
Subject: Re: [PATCH v4 5/5] target/riscv: add vector amo operations
Date: Thu, 27 Feb 2020 21:38:10 -0800 [thread overview]
Message-ID: <03bf483e-d6bb-9de4-9934-12bfa7093ad3@linaro.org> (raw)
In-Reply-To: <20200225103508.7651-6-zhiwei_liu@c-sky.com>
On 2/25/20 2:35 AM, LIU Zhiwei wrote:
> + if (s->sew < 2) {
> + return false;
> + }
This could just as easily be in amo_check?
> +
> + if (tb_cflags(s->base.tb) & CF_PARALLEL) {
> +#ifdef CONFIG_ATOMIC64
> + fn = fns[0][seq][s->sew - 2];
> +#else
> + gen_helper_exit_atomic(cpu_env);
> + s->base.is_jmp = DISAS_NORETURN;
> + return true;
> +#endif
Why are you raising exit_atomic without first checking that s->sew == 3? We
can do 32-bit atomic operations always.
> + } else {
> + fn = fns[1][seq][s->sew - 2];
> + }
> + if (fn == NULL) {
> + return false;
> + }
> +
> + return amo_trans(a->rd, a->rs1, a->rs2, data, fn, s);
> +}
> +
> +static bool amo_check(DisasContext *s, arg_rwdvm* a)
> +{
> + return (vext_check_isa_ill(s, RVV | RVA) &&
> + (a->wd ? vext_check_overlap_mask(s, a->rd, a->vm) : 1) &&
> + vext_check_reg(s, a->rd, false) &&
> + vext_check_reg(s, a->rs2, false));
> +}
I guess the "If SEW is greater than XLEN, an illegal instruction exception is
raised" requirement is currently in the column of NULLs in the !CONFIG_RISCV64
block. But it might be better to have it explicit and save the column of NULLs.
It makes sense to me to do both sew checks together, whether in amo_check or in
amo_op.
> +#define GEN_VEXT_AMO_NOATOMIC_OP(NAME, ETYPE, MTYPE, H, DO_OP, SUF) \
> +static void vext_##NAME##_noatomic_op(void *vs3, target_ulong addr, \
> + uint32_t wd, uint32_t idx, CPURISCVState *env, uintptr_t retaddr)\
> +{ \
> + ETYPE ret; \
> + target_ulong tmp; \
> + int mmu_idx = cpu_mmu_index(env, false); \
> + tmp = cpu_ld##SUF##_mmuidx_ra(env, addr, mmu_idx, retaddr); \
> + ret = DO_OP((ETYPE)(MTYPE)tmp, *((ETYPE *)vs3 + H(idx))); \
> + cpu_st##SUF##_mmuidx_ra(env, addr, ret, mmu_idx, retaddr); \
> + if (wd) { \
> + *((ETYPE *)vs3 + H(idx)) = (target_long)(MTYPE)tmp; \
The target_long cast is wrong; should be ETYPE.
You can use cpu_ldX/stX_data (no mmu_idx or retaddr argument). There should be
no faults, since you've already checked for read+write.
> +/* atomic opreation for vector atomic insructions */
> +#ifndef CONFIG_USER_ONLY
> +#define GEN_VEXT_ATOMIC_OP(NAME, ETYPE, MTYPE, MOFLAG, H, AMO) \
> +static void vext_##NAME##_atomic_op(void *vs3, target_ulong addr, \
> + uint32_t wd, uint32_t idx, CPURISCVState *env) \
> +{ \
> + target_ulong tmp; \
> + int mem_idx = cpu_mmu_index(env, false); \
> + tmp = helper_atomic_##AMO##_le(env, addr, *((ETYPE *)vs3 + H(idx)), \
> + make_memop_idx(MO_ALIGN | MOFLAG, mem_idx)); \
> + if (wd) { \
> + *((ETYPE *)vs3 + H(idx)) = (target_long)(MTYPE)tmp; \
> + } \
> +}
> +#else
> +#define GEN_VEXT_ATOMIC_OP(NAME, ETYPE, MTYPE, MOFLAG, H, AMO) \
> +static void vext_##NAME##_atomic_op(void *vs3, target_ulong addr, \
> + uint32_t wd, uint32_t idx, CPURISCVState *env) \
> +{ \
> + target_ulong tmp; \
> + tmp = helper_atomic_##AMO##_le(env, addr, *((ETYPE *)vs3 + H(idx))); \
> + if (wd) { \
> + *((ETYPE *)vs3 + H(idx)) = (target_long)(MTYPE)tmp; \
> + } \
> +}
> +#endif
This is not right. It is not legal to call these helpers from another helper
-- they will use the wrong GETPC() and will not unwind properly.
> +static inline void vext_amo_atomic(void *vs3, void *v0, target_ulong base,
> + void *vs2, CPURISCVState *env, uint32_t desc,
> + vext_get_index_addr get_index_addr,
> + vext_amo_atomic_fn atomic_op,
> + vext_ld_clear_elem clear_elem,
> + uint32_t esz, uint32_t msz, uintptr_t ra)
> +{
> + uint32_t i;
> + target_long addr;
> + uint32_t wd = vext_wd(desc);
> + uint32_t vm = vext_vm(desc);
> + uint32_t mlen = vext_mlen(desc);
> + uint32_t vlmax = vext_maxsz(desc) / esz;
> +
> + for (i = 0; i < env->vl; i++) {
> + if (!vm && !vext_elem_mask(v0, mlen, i)) {
> + continue;
> + }
> + probe_read_access(env, get_index_addr(base, i, vs2), msz, ra);
> + probe_write_access(env, get_index_addr(base, i, vs2), msz, ra);
> + }
You probably need to check for aligned address here too, probably first so an
unaligned fault has priority over an invalid page fault.
The missing aligned address check is the only remaining exception that the
helper_atomic_* functions would raise, since you have properly checked for
read+write. So it might be possible to get away with using the helpers, but I
don't like it.
But I do think it would be better to write your own helpers for the atomic
paths. They need not check quite so much, since we have already done the
validation above. You pretty much only need to use tlb_vaddr_to_host.
If that gets too ugly, we can talk about rearranging
accel/tcg/atomic_template.h so that it could be reused.
Alternately, we could simply *always* use the non-atomic helpers, and raise
exit_atomic if PARALLEL.
> +static inline void vext_amo_noatomic(void *vs3, void *v0, target_ulong base,
> + void *vs2, CPURISCVState *env, uint32_t desc,
> + vext_get_index_addr get_index_addr,
> + vext_amo_noatomic_fn noatomic_op,
> + vext_ld_clear_elem clear_elem,
> + uint32_t esz, uint32_t msz, uintptr_t ra)
Without the retaddr argument to the noatomic_fn, as described above,
vext_amo_noatomic and vext_amo_atomic are identical.
r~
next prev parent reply other threads:[~2020-02-28 5:38 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-25 10:35 [PATCH v4 0/5] target/riscv: support vector extension part 2 LIU Zhiwei
2020-02-25 10:35 ` [PATCH v4 1/5] target/riscv: add vector unit stride load and store instructions LIU Zhiwei
2020-02-27 19:17 ` Richard Henderson
[not found] ` <287bde05-421c-f49c-2404-fdee183c9e12@c-sky.com>
2020-02-28 3:33 ` Richard Henderson
2020-02-28 6:16 ` LIU Zhiwei
2020-03-07 4:36 ` LIU Zhiwei
2020-03-07 17:44 ` Richard Henderson
2020-02-25 10:35 ` [PATCH v4 2/5] target/riscv: add vector " LIU Zhiwei
2020-02-27 19:36 ` Richard Henderson
2020-02-28 2:11 ` LIU Zhiwei
2020-03-07 4:29 ` LIU Zhiwei
2020-02-25 10:35 ` [PATCH v4 3/5] target/riscv: add vector index " LIU Zhiwei
2020-02-27 19:49 ` Richard Henderson
2020-02-28 2:13 ` LIU Zhiwei
2020-02-25 10:35 ` [PATCH v4 4/5] target/riscv: add fault-only-first unit stride load LIU Zhiwei
2020-02-27 20:03 ` Richard Henderson
2020-02-28 2:17 ` LIU Zhiwei
2020-02-25 10:35 ` [PATCH v4 5/5] target/riscv: add vector amo operations LIU Zhiwei
2020-02-28 5:38 ` Richard Henderson [this message]
2020-02-28 9:19 ` LIU Zhiwei
2020-02-28 18:46 ` Richard Henderson
2020-02-29 13:16 ` LIU Zhiwei
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=03bf483e-d6bb-9de4-9934-12bfa7093ad3@linaro.org \
--to=richard.henderson@linaro.org \
--cc=alistair23@gmail.com \
--cc=chihmin.chao@sifive.com \
--cc=linux-csky@vger.kernel.org \
--cc=palmer@dabbelt.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
--cc=wenmeng_zhang@c-sky.com \
--cc=wxy194768@alibaba-inc.com \
--cc=zhiwei_liu@c-sky.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).