Re: [PATCH] target/riscv: fix C extension disabling on misa write

[Richard Henderson <richard.henderson@linaro.org>]

On 2/20/25 08:31, Vladimir Isaev wrote:
> According to spec:
> Writing misa may increase IALIGN, e.g., by disabling the "C" extension. If an instruction that would
> write misa increases IALIGN, and the subsequent instruction’s address is not IALIGN-bit aligned, the
> write to misa is suppressed, leaving misa unchanged.
> 
> So we should suppress disabling "C" if it is already enabled and
> next instruction is not aligned to 4.
> 
> Fixes: f18637cd611c ("RISC-V: Add misa runtime write support")
> Signed-off-by: Vladimir Isaev <vladimir.isaev@syntacore.com>
> ---
>   target/riscv/csr.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index afb7544f0780..32f9b7b16f6f 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2067,11 +2067,12 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>       val &= env->misa_ext_mask;
>   
>       /*
> -     * Suppress 'C' if next instruction is not aligned
> +     * Disabling 'C' increases IALIGN to 32. If subsequent instruction's address
> +     * is not 32-bit aligned, write to misa is suppressed.
>        * TODO: this should check next_pc
>        */
> -    if ((val & RVC) && (GETPC() & ~3) != 0) {
> -        val &= ~RVC;
> +    if (!(val & RVC) && (env->misa_ext & RVC) && (GETPC() & 0x3)) {
> +        return RISCV_EXCP_NONE;
>       }

GETPC is a host thing, not a target thing.
Both the old and new code are very wrong.

You wanted to check env->pc, but this isn't updated before calling gen_helper_csr*w.

The simplest fix is to make sure that happens in the translator.

A slightly more complex fix that does not require all csr writes to update the pc would 
involve using cpu_unwind_state_data.  See target/openrisc/sys_helper.c for an example.


r~