From: Alistair Francis <alistair23@gmail.com>
To: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Cc: qemu-devel@nongnu.org, qemu-riscv@nongnu.org,
alistair.francis@wdc.com, bmeng@tinylab.org,
liweiwei@iscas.ac.cn, zhiwei_liu@linux.alibaba.com,
palmer@rivosinc.com
Subject: Re: [PATCH v8 11/11] target/riscv: rework write_misa()
Date: Wed, 17 May 2023 14:49:06 +1000 [thread overview]
Message-ID: <CAKmqyKNt6qANXLLv2c_a6w-FS9pyqHhShbcDWO_vW8jvuqL_8Q@mail.gmail.com> (raw)
In-Reply-To: <f892567b-521c-5cbb-14df-e04f4e6415a5@ventanamicro.com>
On Fri, May 12, 2023 at 10:42 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Alistair,
>
>
> Since this is the only patch that is being contested is it possible to apply all
> the other ones to riscv-to-apply.next?
>
> I'm asking because I'm going to send more code that will affect cpu_init() and
> riscv_cpu_realize() functions, and it would be easier if the cleanups were already
> in 'next'. Otherwise I'll either have to base the new code on top of this pending
> series or I'll base them under riscv-to-apply.next and we'll have to deal with
> conflicts.
Urgh, that's fair.
I just replied to your other thread. Let's throw a guest error print
in here and then we can merge it
Alistair
>
>
> Thanks,
>
> Daniel
>
> On 4/21/23 10:27, Daniel Henrique Barboza wrote:
> > write_misa() must use as much common logic as possible. We want to open
> > code just the bits that are exclusive to the CSR write operation and TCG
> > internals.
> >
> > Our validation is done with riscv_cpu_validate_set_extensions(), but we
> > need a small tweak first. When enabling RVG we're doing:
> >
> > env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
> > env->misa_ext_mask = env->misa_ext;
> >
> > This works fine for realize() time but this can potentially overwrite
> > env->misa_ext_mask if we reutilize the function for write_misa().
> >
> > Instead of doing misa_ext_mask = misa_ext, sum up the RVG extensions in
> > misa_ext_mask as well. This won't change realize() time behavior
> > (misa_ext_mask will be == misa_ext) and will ensure that write_misa()
> > won't change misa_ext_mask by accident.
> >
> > After that, rewrite write_misa() to work as follows:
> >
> > - mask the write using misa_ext_mask to avoid enabling unsupported
> > extensions;
> >
> > - suppress RVC if the next insn isn't aligned;
> >
> > - disable RVG if any of RVG dependencies are being disabled by the user;
> >
> > - assign env->misa_ext and run riscv_cpu_validate_set_extensions(). On
> > error, rollback env->misa_ext to its original value;
> >
> > - handle RVF and MSTATUS_FS and continue as usual.
> >
> > Let's keep write_misa() as experimental for now until this logic gains
> > enough mileage.
> >
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> > ---
> > target/riscv/cpu.c | 4 ++--
> > target/riscv/cpu.h | 1 +
> > target/riscv/csr.c | 47 ++++++++++++++++++++--------------------------
> > 3 files changed, 23 insertions(+), 29 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 7d407321aa..4fa720a39d 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -944,7 +944,7 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
> > * Check consistency between chosen extensions while setting
> > * cpu->cfg accordingly.
> > */
> > -static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> > +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> > {
> > CPURISCVState *env = &cpu->env;
> > Error *local_err = NULL;
> > @@ -960,7 +960,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> > cpu->cfg.ext_ifencei = true;
> >
> > env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
> > - env->misa_ext_mask = env->misa_ext;
> > + env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
> > }
> >
> > if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 15423585d0..1f39edc687 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -548,6 +548,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> > bool probe, uintptr_t retaddr);
> > char *riscv_isa_string(RISCVCPU *cpu);
> > void riscv_cpu_list(void);
> > +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp);
> >
> > #define cpu_list riscv_cpu_list
> > #define cpu_mmu_index riscv_cpu_mmu_index
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 4451bd1263..4a3c57ea6f 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -1387,39 +1387,18 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
> > static RISCVException write_misa(CPURISCVState *env, int csrno,
> > target_ulong val)
> > {
> > + RISCVCPU *cpu = env_archcpu(env);
> > + uint32_t orig_misa_ext = env->misa_ext;
> > + Error *local_err = NULL;
> > +
> > if (!riscv_cpu_cfg(env)->misa_w) {
> > /* drop write to misa */
> > return RISCV_EXCP_NONE;
> > }
> >
> > - /* 'I' or 'E' must be present */
> > - if (!(val & (RVI | RVE))) {
> > - /* It is not, drop write to misa */
> > - return RISCV_EXCP_NONE;
> > - }
> > -
> > - /* 'E' excludes all other extensions */
> > - if (val & RVE) {
> > - /*
> > - * when we support 'E' we can do "val = RVE;" however
> > - * for now we just drop writes if 'E' is present.
> > - */
> > - return RISCV_EXCP_NONE;
> > - }
> > -
> > - /*
> > - * misa.MXL writes are not supported by QEMU.
> > - * Drop writes to those bits.
> > - */
> > -
> > /* Mask extensions that are not supported by this hart */
> > val &= env->misa_ext_mask;
> >
> > - /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
> > - if ((val & RVD) && !(val & RVF)) {
> > - val &= ~RVD;
> > - }
> > -
> > /*
> > * Suppress 'C' if next instruction is not aligned
> > * TODO: this should check next_pc
> > @@ -1428,18 +1407,32 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
> > val &= ~RVC;
> > }
> >
> > + /* Disable RVG if any of its dependencies are disabled */
> > + if (!(val & RVI && val & RVM && val & RVA &&
> > + val & RVF && val & RVD)) {
> > + val &= ~RVG;
> > + }
> > +
> > /* If nothing changed, do nothing. */
> > if (val == env->misa_ext) {
> > return RISCV_EXCP_NONE;
> > }
> >
> > - if (!(val & RVF)) {
> > + env->misa_ext = val;
> > + riscv_cpu_validate_set_extensions(cpu, &local_err);
> > + if (local_err != NULL) {
> > + /* Rollback on validation error */
> > + env->misa_ext = orig_misa_ext;
> > +
> > + return RISCV_EXCP_NONE;
> > + }
> > +
> > + if (!(env->misa_ext & RVF)) {
> > env->mstatus &= ~MSTATUS_FS;
> > }
> >
> > /* flush translation cache */
> > tb_flush(env_cpu(env));
> > - env->misa_ext = val;
> > env->xl = riscv_cpu_mxl(env);
> > return RISCV_EXCP_NONE;
> > }
>
prev parent reply other threads:[~2023-05-17 4:50 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-21 13:27 [PATCH v8 00/11] target/riscv: rework CPU extension validation Daniel Henrique Barboza
2023-04-21 13:27 ` [PATCH v8 01/11] target/riscv/cpu.c: add riscv_cpu_validate_v() Daniel Henrique Barboza
2023-04-21 13:27 ` [PATCH v8 02/11] target/riscv/cpu.c: remove set_vext_version() Daniel Henrique Barboza
2023-04-21 13:27 ` [PATCH v8 03/11] target/riscv/cpu.c: remove set_priv_version() Daniel Henrique Barboza
2023-04-21 13:27 ` [PATCH v8 04/11] target/riscv: add PRIV_VERSION_LATEST Daniel Henrique Barboza
2023-04-21 13:27 ` [PATCH v8 05/11] target/riscv: Mask the implicitly enabled extensions in isa_string based on priv version Daniel Henrique Barboza
2023-04-21 13:27 ` [PATCH v8 06/11] target/riscv: Update check for Zca/Zcf/Zcd Daniel Henrique Barboza
2023-04-21 13:27 ` [PATCH v8 07/11] target/riscv/cpu.c: add priv_spec validate/disable_exts helpers Daniel Henrique Barboza
2023-04-21 13:27 ` [PATCH v8 08/11] target/riscv/cpu.c: add riscv_cpu_validate_misa_mxl() Daniel Henrique Barboza
2023-04-21 13:27 ` [PATCH v8 09/11] target/riscv/cpu.c: validate extensions before riscv_timer_init() Daniel Henrique Barboza
2023-04-21 13:27 ` [PATCH v8 10/11] target/riscv/cpu.c: remove cfg setup from riscv_cpu_init() Daniel Henrique Barboza
2023-04-21 13:27 ` [PATCH v8 11/11] target/riscv: rework write_misa() Daniel Henrique Barboza
2023-05-07 23:25 ` Alistair Francis
2023-05-08 10:28 ` Daniel Henrique Barboza
2023-05-17 4:48 ` Alistair Francis
2023-05-17 9:43 ` Daniel Henrique Barboza
2023-05-12 12:41 ` Daniel Henrique Barboza
2023-05-17 4:49 ` Alistair Francis [this message]
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=CAKmqyKNt6qANXLLv2c_a6w-FS9pyqHhShbcDWO_vW8jvuqL_8Q@mail.gmail.com \
--to=alistair23@gmail.com \
--cc=alistair.francis@wdc.com \
--cc=bmeng@tinylab.org \
--cc=dbarboza@ventanamicro.com \
--cc=liweiwei@iscas.ac.cn \
--cc=palmer@rivosinc.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
--cc=zhiwei_liu@linux.alibaba.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).