[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending '
From: |
Amed Magdy |
Subject: |
Re: [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes |
Date: |
Tue, 26 Feb 2019 09:58:25 +0200 |
> > 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
Re: [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes, Eric Blake, 2019/02/23