| Message ID | 20230216130444.795997-2-dbarboza@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | make write_misa a no-op and FEATURE_* cleanups | expand |
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>
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 --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; }
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(-)