Re: [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