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
Zhiwei
Given 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 <[email protected]>
Reviewed-by: Bin Meng <[email protected]>
Reviewed-by: Andrew Jones <[email protected]>
---
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;
}
