diff mbox series

[v4,01/10] target/riscv: turn write_misa() into an official no-op

Message ID 20230216130444.795997-2-dbarboza@ventanamicro.com (mailing list archive)
State New, archived
Headers show
Series make write_misa a no-op and FEATURE_* cleanups | expand

Commit Message

Daniel Henrique Barboza Feb. 16, 2023, 1:04 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 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.

It is important to mention that the spec says that MISA is a WARL (Write
Any Read Legal) CSR, and having the write operations as a no-op is a
valid spec implementation. Allowing the dormant code to write MISA can
cause tricky bugs to solve later on. Given that we don't have a
particularly interesting case of writing MISA to support today, the
risks outweights the benefits.

Let's make it official and erase all the body of write_misa(), making it
an official no-op.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/csr.c | 55 ----------------------------------------------
 1 file changed, 55 deletions(-)

Comments

Bin Meng Feb. 16, 2023, 1:26 p.m. UTC | #1
On Thu, Feb 16, 2023 at 9:06 PM 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 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.
>
> It is important to mention that the spec says that MISA is a WARL (Write
> Any Read Legal) CSR, and having the write operations as a no-op is a
> valid spec implementation. Allowing the dormant code to write MISA can
> cause tricky bugs to solve later on. Given that we don't have a
> particularly interesting case of writing MISA to support today, the
> risks outweights the benefits.

typo, outweigh?

>
> Let's make it official and erase all the body of write_misa(), making it
> an official no-op.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/csr.c | 55 ----------------------------------------------
>  1 file changed, 55 deletions(-)
>

Reviewed-by: Bin Meng <bmeng@tinylab.org>
Andrew Jones Feb. 16, 2023, 2:51 p.m. UTC | #2
On Thu, Feb 16, 2023 at 10:04:35AM -0300, 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.
> 
> It is important to mention that the spec says that MISA is a WARL (Write
> Any Read Legal) CSR, and having the write operations as a no-op is a
> valid spec implementation.

This sort of reads like all WARL CSRs can ignore their writes. That's not
generally true, but this CSR can, because the spec says an implementation
may/can make its bits modifiable.

> Allowing the dormant code to write MISA can
> cause tricky bugs to solve later on. Given that we don't have a
> particularly interesting case of writing MISA to support today, the
> risks outweights the benefits.
> 
> Let's make it official and erase all the body of write_misa(), making it
> an official no-op.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/csr.c | 55 ----------------------------------------------
>  1 file changed, 55 deletions(-)
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew
diff mbox series

Patch

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