Re: [Qemu-riscv] [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes

[Amed Magdy]

 

> It seems to me that the C extension can be enabled at any point, since if C is
> off, you know that the next insn is aligned modulo 4.
>

 

 Ok, This is mostly right. When C extension is enabled 32-bit base instructions can be aligned on 2 bytes boundaries instead of 4 bytes only. So multiple enables and disables of C bit at different code areas theoretically may require this check on C extension enable. I'm not really sure, may be this is might not  be a practical use scenario.

> It is only if the C extension is enabled, and you want to disable it, that is
> when we must check to see if the next insn is aligned mod 4.  It is trivial to
> arrange for a particular instruction to be aligned, via assembler directives.
> So it seems silly to make the definition of the csr write to misa any more
> complicated than it is.

 I completely agree with you that C extension disable should have alignment check.

> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 960d2b0aa9..8726ef802e 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -370,10 +370,11 @@ static int write_misa(CPURISCVState *env, int csrno,
> target_ulong val)
>          val &= ~RVD;
>    }

> -    /* Suppress 'C' if next instruction is not aligned
> -     * TODO: this should check next_pc
> +    /*
> +     * Suppress 'C' if next instruction is not aligned.
> +     * We updated env->pc to the next insn in the translator.
>      */
> -    if ((val & RVC) && (GETPC() & ~3) != 0) {
> +    if ((val & RVC) && (env->pc & ~3) != 0) {
>         val &= ~RVC;
>     }

 

 Just a hint, (env->pc & 3) instead of (env->pc & ~3) , right ?

 

 Thanks,

 Ahmed