|
From: | Daniel Henrique Barboza |
Subject: | Re: [PATCH v6 1/9] target/riscv: turn write_misa() into an official no-op |
Date: | Tue, 21 Feb 2023 12:49:11 -0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.2 |
Hey, On 2/16/23 22:42, LIU Zhiwei wrote:
On 2023/2/17 5:55, Daniel Henrique Barboza wrote:At this moment, and apparently since ever, we have no way of enabling RISCV_FEATURE_MISA. This means that all the code from write_misa(), all the nuts and bolts that handles how to write this CSR, has always been a no-op as well because write_misa() will always exit earlier. This seems to be benign in the majority of cases. Booting an Ubuntu 'virt' guest and logging all the calls to 'write_misa' shows that no writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling RISC-V extensions after the machine is powered on, seems to be a niche use. Before proceeding, let's recap what the spec says about MISA. It is a CSR that is divided in 3 fields: - MXL, Machine XLEN, described as "may be writable"; - MXLEN, the XLEN in M-mode, which is given by the setting of MXL or a fixed value if MISA is zero; - Extensions is defined as "a WARL field that can contain writable bits where the implementation allows the supported ISA to be modified" Thus what we have today (write_misa() being a no-op) is already a valid spec implementation. We're not obliged to have a particular set of MISA writable bits, and at this moment we have none.Hi Daniel, I see there has been a discussion on this topic. And as no-op has no harmfulness for current implementation. However, I still think we should make misa writable as default, which is also a valid spec implementation. One reason is that may be we need to dynamic write access for some cpus in the future. The other is we should make QEMU a more useful implementation, not just a legal implementation. We have done in many aspects on this direction. I prefer your implementation before v4. It's not a complicated implementation. And I think the other extensions on QEMU currently can mostly be configurable already.
I don't have a strong opinion in this matter to be honest. My problems with the existing code are: - the code is untested. I cannot say that this was never tested, but I can say that this has been mostly untested ever since introduced. Which is normal for a code that is 'dormant'. - the code is dormant and most likely with bugs, but it's still maintained. For example we have e91a7227 ("target/riscv: Split misa.mxl and misa.ext") that had to make changes here. So we have the upkeep but no benefits. - we don't have an use case for it. Most OSes doesn't seem to care, and afaik no applications seems to care either. All this said, I think we can reach a consensus of keeping it if we can at least come up with a way of testing it.
Your work is a good step towards to unify the configuration and the check. I think two more steps we can go further. 1) Remove RVI/RVF and the similar macros, and add fields for them in the configuration struct. 2) Unify the check about configuration. write_misa and cpu_realize_fn can use the same check function. As we have done these two steps, I think we can go more closely for the profile extension.
Is this the extension you're taking about? https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc This looks like a good reason to keep the code. Let's see if anyone else has an opinion about it. We can do the improvements you mentioned above as a follow-up (this series was really about removing RISC_FEATURE_*) if we decide to keep it. Thanks, Daniel
ZhiweiGiven that allowing the dormant code to write MISA can cause tricky bugs to solve later on, and we don't have a particularly interesting case of writing MISA to support today, and we're already not violating the specification, let's erase all the body of write_misa() and turn it into an official no-op instead of an accidental one. We'll keep consistent with what we provide users today but with 50+ less lines to maintain. RISCV_FEATURE_MISA enum is erased in the process since there's no one else using it. Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Reviewed-by: Bin Meng <bmeng@tinylab.org> Reviewed-by: Andrew Jones <ajones@ventanamicro.com> --- target/riscv/cpu.h | 1 - target/riscv/csr.c | 55 ---------------------------------------------- 2 files changed, 56 deletions(-) diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index 7128438d8e..01803a020d 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -89,7 +89,6 @@ enum { RISCV_FEATURE_MMU, RISCV_FEATURE_PMP, RISCV_FEATURE_EPMP, - RISCV_FEATURE_MISA, RISCV_FEATURE_DEBUG }; diff --git a/target/riscv/csr.c b/target/riscv/csr.c index 1b0a0c1693..f7862ff4a4 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int csrno, static RISCVException write_misa(CPURISCVState *env, int csrno, target_ulong val) { - if (!riscv_feature(env, RISCV_FEATURE_MISA)) { - /* 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; - - /* Mask extensions that are not supported by QEMU */ - val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV); - - /* '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 - */ - if ((val & RVC) && (GETPC() & ~3) != 0) { - val &= ~RVC; - } - - /* If nothing changed, do nothing. */ - if (val == env->misa_ext) { - return RISCV_EXCP_NONE; - } - - if (!(val & 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 in Thread] | Current Thread | [Next in Thread] |