Re: [PATCH v3 1/3] target/riscv: Fix IALIGN check in misa write

[Nicholas Piggin <npiggin@gmail.com>]

On Wed, Mar 25, 2026 at 11:40:18AM +0800, Chao Liu wrote:
> On Wed, Mar 25, 2026 at 01:26:56PM +1000, Alistair Francis wrote:
> > On Wed, Mar 25, 2026 at 1:09 PM Chao Liu <chao.liu.zevorn@gmail.com> wrote:
> > >
> > > On Sun, Mar 22, 2026 at 12:45:52AM +1000, Nicholas Piggin wrote:
> > > > The instruction alignment check for the C extension was inverted.
> > > > The new value should be checked for C bit clear (thus increasing
> > > > IALIGN). If IALIGN is incompatible, then the write to misa should
> > > > be suppressed, not just ignoring the update to the C bit.
> > > >
> > > > From the ISA:
> > > >
> > > >   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.
> > > >
> > > > This was found with a verification test generator based on RiESCUE.
> > > >
> > > > Reported-by: Nicholas Joaquin <njoaquin@tenstorrent.com>
> > > > Reported-by: Ganesh Valliappan <gvalliappan@tenstorrent.com>
> > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > > > ---
> > > >  target/riscv/csr.c                        | 16 ++++-
> > > >  tests/tcg/riscv64/Makefile.softmmu-target |  5 ++
> > > >  tests/tcg/riscv64/misa-ialign.S           | 88 +++++++++++++++++++++++
> > > >  3 files changed, 106 insertions(+), 3 deletions(-)
> > > >  create mode 100644 tests/tcg/riscv64/misa-ialign.S
> > > >
> > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > > index 5064483917..91421a2dd8 100644
> > > > --- a/target/riscv/csr.c
> > > > +++ b/target/riscv/csr.c
> > > > @@ -2129,9 +2129,19 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
> > > >      /* Mask extensions that are not supported by this hart */
> > > >      val &= env->misa_ext_mask;
> > > >
> > > > -    /* Suppress 'C' if next instruction is not aligned. */
> > > > -    if ((val & RVC) && (get_next_pc(env, ra) & 3) != 0) {
> > > > -        val &= ~RVC;
> > > > +    /*
> > > > +     * misa writes that increase IALIGN beyond alignment of the next
> > > > +     * instruction cause the write to misa to be suppressed. Clearing
> > > > +     * "C" extension increases IALIGN.
> > > > +     */
> > > > +    if (!(val & RVC) && (get_next_pc(env, ra) & 3) != 0) {
> > > > +        /*
> > > > +         * If the next instruction is unaligned mod 4 then "C" must be
> > > > +         * set or this instruction could not be executing, so we know
> > > > +         * this is is clearing "C" (and not just keeping it clear).
> > > "this is is clearing" — double "is"
> > >
> > > > +         */
> > > > +        g_assert(env->misa_ext & RVC);
> > > > +        return RISCV_EXCP_NONE;
> > > write_misa() is also reachable via riscv_csrrw_debug()
> > > with ra=0, where get_next_pc() falls back to env->pc.
> > 
> > Ah good catch
> > 
> > > A debugger can set PC to a 2-byte-aligned address while C is
> > > already disabled, then write misa keeping C=0. This hits
> > > the condition and fires the g_assert.
> > 
> > I'm not convinced that that's necessarily bad, as that's an odd and
> > invalid thing to be writing. But we probably shouldn't assert
> > 
> > >
> > > The ISA spec language:
> > >
> > >     "if an instruction that would write misa..."
> > >
> > > does not cover debug writes, so the IALIGN suppression
> > > arguably should not apply in that case at all.
> > >
> > > We can:
> > >
> > >     if (ra && !(val & RVC)
> > >         && (get_next_pc(env, ra) & 3) != 0) {
> > >         g_assert(env->misa_ext & RVC);
> > >         return RISCV_EXCP_NONE;
> > >     }
> > 
> > Maybe it's best to change the `g_assert()` to a log GUEST_ERROR
> > instead. That way we flag that something fishy is going on, but don't
> > exit QEMU
> > 
> Agreed!
> 
> Replacing the g_assert() with qemu_log_mask(LOG_GUEST_ERROR, ...)
> sounds like the right balance — it still surfaces the anomaly for
> anyone debugging, without taking down QEMU over what is ultimately
> a debugger-induced corner case.
> 
> The IALIGN suppression logic itself is still correct for the normal
> execution path, so there's no reason to skip it entirely; just don't
> crash on the weird one.

Okay good feedback, thanks to you both. I agree, I'll make the changes
and resend.

Thanks,
Nick