diff mbox series

[v2,03/11] target/riscv: allow users to actually write the MISA CSR

Message ID 20230214192356.319991-4-dbarboza@ventanamicro.com (mailing list archive)
State New, archived
Headers show
Series enable write_misa() and RISCV_FEATURE_* cleanups | expand

Commit Message

Daniel Henrique Barboza Feb. 14, 2023, 7:23 p.m. UTC
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.

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(-)

Comments

Bin Meng Feb. 15, 2023, 8:45 a.m. UTC | #1
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 mbox series

Patch

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;
     }