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

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

On 2/20/25 1:31 PM, 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;

Not related to your changes but it would be nice if we made more use of
QEMU_IS_ALIGNED() instead of doing these bitwise ops to check alignment.
E.g. to check if pc is aligned to 4: QEMU_IS_ALIGNED(GETPC(), 4).


> +    if (!(val & RVC) && (env->misa_ext & RVC) && (GETPC() & 0x3)) {
> +        return RISCV_EXCP_NONE;
>       }

This will abort the write altogether, skipping valid changes to other bits. What
we want is a "val &= ~RVC" if the proper conditions for clearing RVC are met.

Thanks,

Daniel

>   
>       /* Disable RVG if any of its dependencies are disabled */