qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 02/11] target/riscv: allow users to actually write the MISA C


From: Daniel Henrique Barboza
Subject: Re: [PATCH 02/11] target/riscv: allow users to actually write the MISA CSR
Date: Tue, 14 Feb 2023 14:39:52 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1



On 2/14/23 12:12, weiwei wrote:

On 2023/2/11 19:50, Daniel Henrique Barboza wrote:


On 2/10/23 23:43, weiwei wrote:

On 2023/2/10 21:36, 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 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.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
  target/riscv/cpu.c | 1 +
  target/riscv/cpu.h | 1 +
  target/riscv/csr.c | 4 +++-
  3 files changed, 5 insertions(+), 1 deletion(-)

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 7128438d8e..103963b386 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..4f9cc501b2 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1329,7 +1329,9 @@ 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)) {
+    RISCVCPU *cpu = env_archcpu(env);
+
+    if (!cpu->cfg.misa_w) {

It's Ok to get it directly from cfg. However, personally, I prefer to keep the 
non-isa features in a separate list.

I don't mind a separated non-isa list. cpu->cfg has everything contained in it
though, ISA and non-ISA (e.g. vendor extensions that weren't ratified yet), and
the current RISCV_FEATURES_* list is just a duplicate of it that we need to
update it during riscv_cpu_realize().

In my opinion we can spare the extra effort of keeping a separated, up-to-date
non-ISA extension/features list, by just reading everything from cfg.


Thanks,


Daniel

OK. It's  acceptable to me.

Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

By the way, the riscv_cpu_cfg() in patch 4 can be used here.

Good point. I'll move patch 4 up so I can use that function here.



Daniel


Regards,

Weiwei Li



Regards,

Weiwei Li

          /* drop write to misa */
          return RISCV_EXCP_NONE;
      }





reply via email to

[Prev in Thread] Current Thread [Next in Thread]