From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
To: Alistair Francis <alistair23@gmail.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 RESEND v7 11/12] target/riscv: rework write_misa()
Date: Fri, 21 Apr 2023 08:34:16 -0300 [thread overview]
Message-ID: <981c651c-cc95-4cc2-df9c-eea0e4fca9ac@ventanamicro.com> (raw)
In-Reply-To: <CAKmqyKPd_5nVUvcd20AxrrH9UKT88pU-KLqQMbCh_nUCyh9LvA@mail.gmail.com>
On 4/20/23 20:45, Alistair Francis wrote:
> On Thu, Apr 20, 2023 at 7:22 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> 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 is still == 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 to the previous values of misa_ext and misa_ext_mask;
>>
>> - on success, check if there's a chance that misa_ext_mask was
>> overwritten during the process and restore it;
>
> Is this right? If the guest does a combined valid/invalid modification
> shouldn't the valid modifications stick?
At this moment we're doing misa_ext_mask = misa_ext at the start of validate_set_extensions()
in case we need to set RVG.
So, even if we validated everything, there's still a chance that we're setting
misa_ext_mask. Since this value is defined during realize() and it indicates the
maximum extensions allowed in the CPU, we shouldn't be touching it during runtime.
In fact, I believe this patch is not correct. Down there:
>
> Alistair
>
>>
>> - 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 865ee9efda..d449da2657 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;
In this rollback we're not restoring the original env->misa_ext_mask.
I think that a better alternative, instead of rolling back misa_ext_mask in
write_misa(), is to pass a flag to validate_set_extensions() to allow us to
change misa_ext_mask only during realize() time.
I'll make this change and re-send. Thanks,
Daniel
>> +
>> + 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;
>> }
>> --
>> 2.40.0
>>
>>
next prev parent reply other threads:[~2023-04-21 11:34 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-20 9:20 [PATCH RESEND v7 00/12] target/riscv: rework CPU extensions validation Daniel Henrique Barboza
2023-04-20 9:20 ` [PATCH RESEND v7 01/12] target/riscv/cpu.c: add riscv_cpu_validate_v() Daniel Henrique Barboza
2023-04-20 9:20 ` [PATCH RESEND v7 02/12] target/riscv/cpu.c: remove set_vext_version() Daniel Henrique Barboza
2023-04-20 9:20 ` [PATCH RESEND v7 03/12] target/riscv/cpu.c: remove set_priv_version() Daniel Henrique Barboza
2023-04-20 9:20 ` [PATCH RESEND v7 04/12] target/riscv: add PRIV_VERSION_LATEST Daniel Henrique Barboza
2023-04-20 9:20 ` [PATCH RESEND v7 05/12] target/riscv: Mask the implicitly enabled extensions in isa_string based on priv version Daniel Henrique Barboza
2023-04-20 23:00 ` Alistair Francis
2023-04-20 9:20 ` [PATCH RESEND v7 06/12] target/riscv: Update check for Zca/Zcf/Zcd Daniel Henrique Barboza
2023-04-20 9:20 ` [PATCH RESEND v7 07/12] target/riscv/cpu.c: add priv_spec validate/disable_exts helpers Daniel Henrique Barboza
2023-04-20 9:20 ` [PATCH RESEND v7 08/12] target/riscv/cpu.c: add riscv_cpu_validate_misa_mxl() Daniel Henrique Barboza
2023-04-20 9:20 ` [PATCH RESEND v7 09/12] target/riscv/cpu.c: validate extensions before riscv_timer_init() Daniel Henrique Barboza
2023-04-20 9:20 ` [PATCH RESEND v7 10/12] target/riscv/cpu.c: remove cfg setup from riscv_cpu_init() Daniel Henrique Barboza
2023-04-20 9:20 ` [PATCH RESEND v7 11/12] target/riscv: rework write_misa() Daniel Henrique Barboza
2023-04-20 23:45 ` Alistair Francis
2023-04-21 11:34 ` Daniel Henrique Barboza [this message]
2023-04-21 13:06 ` Daniel Henrique Barboza
2023-04-21 13:14 ` Daniel Henrique Barboza
2023-04-20 9:21 ` [PATCH RESEND v7 12/12] target/riscv: forbid write_misa() for static CPUs Daniel Henrique Barboza
2023-04-20 23:48 ` Alistair Francis
2023-04-21 11:36 ` Daniel Henrique Barboza
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=981c651c-cc95-4cc2-df9c-eea0e4fca9ac@ventanamicro.com \
--to=dbarboza@ventanamicro.com \
--cc=alistair.francis@wdc.com \
--cc=alistair23@gmail.com \
--cc=bmeng@tinylab.org \
--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).