Re: [PATCH v3 02/10] target/riscv: always allow write_misa() to write MISA

[weiwei]

On 2023/2/16 02:57, 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.
>
> Regardless, the spec says that MISA is a WARL read-write CSR, and gating
> the writes in the register doesn't make sense. OS and applications
> should be wary of the consequences when writing it, but the write itself
> must always be allowed.
>
> Remove the RISCV_FEATURE_MISA verification at the start of write_misa(),
> removing RISCV_FEATURE_MISA altogether since there will be no more
> callers of this enum.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/cpu.h | 1 -
>   target/riscv/csr.c | 5 -----
>   2 files changed, 6 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 7128438d8e..01803a020d 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -89,7 +89,6 @@ enum {
>       RISCV_FEATURE_MMU,
>       RISCV_FEATURE_PMP,
>       RISCV_FEATURE_EPMP,
> -    RISCV_FEATURE_MISA,
>       RISCV_FEATURE_DEBUG
>   };
>   
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index e149b453da..5bd4cdbef5 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1329,11 +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 have a question here:

If we directly remove this here without other limits, the bugs 
introduced by following code

will be exposed to users, such as:

- V may be still enabled when misa.D is cleared.

- Zfh/Zfhmin may be still enabled when misa.D is cleared

- Misa.U may be cleared when Misa.S is still set.

...

Should we fix these bugs before this patch? Or fix them in following 
patchset?

If we choose the latter, I think it's better to add a limitation(such 
as  writable_mask) to the changable fields of misa here.

Regards,

Weiwei Li

>       /* 'I' or 'E' must be present */
>       if (!(val & (RVI | RVE))) {
>           /* It is not, drop write to misa */