| Message ID | 20230214192356.319991-4-dbarboza@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | enable write_misa() and RISCV_FEATURE_* cleanups | expand |
On Wed, Feb 15, 2023 at 3:26 AM Daniel Henrique Barboza <dbarboza@ventanamicro.com> 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 properly 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. > > There is a good chance that the code in write_misa() hasn't been > properly tested. Allowing users to write MISA can open the floodgates of > new breeds of bugs. We could instead remove most (if not all) of > write_misa() since it's never used. Well, as a hardware emulator, > dealing with crashes because a register write went wrong is what we're > here for. > > Create a 'misa-w' CPU property to allow users to choose whether writes > to MISA should be allowed. The default is set to 'false' for all RISC-V > machines to keep compatibility with what we´ve been doing so far. > > Read cpu->cfg.misa_w directly in write_misa(), instead of executing > riscv_set_feature(RISCV_FEATURE_MISA) in riscv_cpu_realize(), that would > simply reflect the cpu->cfg.misa_w bool value in 'env->features' and > require a riscv_feature() call to read it back. I am surprised to see we have a RISCV_FEATURE_MISA. Per spec MISA is a WARL read-write CSR. I don't think creating a special config property for a read-write CSR makes sense. We should drop the feature enum and the feature check in write_misa() directly. > > Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > target/riscv/cpu.c | 1 + > target/riscv/cpu.h | 1 + > target/riscv/csr.c | 2 +- > 3 files changed, 3 insertions(+), 1 deletion(-) > Regards, Bin
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 93b52b826c..69fb9e123f 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -1197,6 +1197,7 @@ static void register_cpu_props(DeviceState *dev) static Property riscv_cpu_properties[] = { DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true), + DEFINE_PROP_BOOL("misa-w", RISCVCPU, cfg.misa_w, false), DEFINE_PROP_UINT32("mvendorid", RISCVCPU, cfg.mvendorid, 0), DEFINE_PROP_UINT64("marchid", RISCVCPU, cfg.marchid, RISCV_CPU_MARCHID), diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index 5e4d056772..fe572b83e9 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -498,6 +498,7 @@ struct RISCVCPUConfig { bool pmp; bool epmp; bool debug; + bool misa_w; bool short_isa_string; }; diff --git a/target/riscv/csr.c b/target/riscv/csr.c index e149b453da..e949e6248a 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -1329,7 +1329,7 @@ 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)) { + if (!riscv_cpu_cfg(env).misa_w) { /* drop write to misa */ return RISCV_EXCP_NONE; }