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

[Alistair Francis <alistair23@gmail.com>]

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

Alistair

>
> Thanks,
> Chao
> >      }
> >
> >      /* Disable RVG if any of its dependencies are disabled */
> > diff --git a/tests/tcg/riscv64/Makefile.softmmu-target b/tests/tcg/riscv64/Makefile.softmmu-target
> > index eb1ce6504a..f176f87ed0 100644
> > --- a/tests/tcg/riscv64/Makefile.softmmu-target
> > +++ b/tests/tcg/riscv64/Makefile.softmmu-target
> > @@ -36,5 +36,10 @@ run-plugin-interruptedmemory: interruptedmemory
> >         $(QEMU) -plugin ../plugins/libdiscons.so -d plugin -D $<.pout \
> >         $(QEMU_OPTS)$<)
> >
> > +EXTRA_RUNS += run-misa-ialign
> > +run-misa-ialign: QEMU_OPTS := -cpu rv64,c=true,v=true,x-misa-w=on $(QEMU_OPTS)
> > +run-misa-ialign: misa-ialign
> > +     $(call run-test, $<, $(QEMU) $(QEMU_OPTS)$<)
> > +
> >  # We don't currently support the multiarch system tests
> >  undefine MULTIARCH_TESTS
> > diff --git a/tests/tcg/riscv64/misa-ialign.S b/tests/tcg/riscv64/misa-ialign.S
> > new file mode 100644
> > index 0000000000..7f1eb30023
> > --- /dev/null
> > +++ b/tests/tcg/riscv64/misa-ialign.S
> > @@ -0,0 +1,88 @@
> > +/*
> > + * Test for MISA changing C and related IALIGN alignment cases
> > + *
> > + * This test verifies that the "C" extension can be cleared and set in MISA,
> > + * that a branch to 2-byte aligned instructions can be executed when "C" is
> > + * enabled, and that a write to MISA which would increase IALIGN and cause
> > + * the next instruction to be unaligned is ignored.
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + */
> > +
> > +#define RVC (1 << ('C'-'A'))
> > +#define RVV (1 << ('V'-'A'))
> > +
> > +.option norvc
> > +     .text
> > +     .global _start
> > +_start:
> > +     lla     t0, trap
> > +     csrw    mtvec, t0
> > +
> > +     csrr    t0, misa
> > +     li      t1, RVC
> > +     not     t1, t1
> > +     and     t0, t0, t1
> > +     csrw    misa, t0
> > +     csrr    t1, misa
> > +     li      a0, 2 # fail code
> > +     bne     t0, t1, _exit # Could not clear RVC in MISA
> > +
> > +     li      t1, RVC
> > +     or      t0, t0, t1
> > +     csrw    misa, t0
> > +     csrr    t1, misa
> > +     li      a0, 3 # fail code
> > +     bne     t0, t1, _exit # Could not set RVC in MISA
> > +
> > +     j       unalign
> > +. = . + 2
> > +unalign:
> > +
> > +     li      t1, RVC
> > +     not     t1, t1
> > +     and     t0, t0, t1
> > +     csrw    misa, t0
> > +     csrr    t1, misa
> > +     li      a0, 4 # fail code
> > +     beq     t0, t1, _exit # Was able to clear RVC in MISA
> > +
> > +     li      t0, (RVC|RVV)
> > +     not     t0, t0
> > +     and     t0, t0, t1
> > +     csrw    misa, t0
> > +     csrr    t0, misa
> > +     li      a0, 5 # fail code
> > +     bne     t0, t1, _exit # MISA write was not ignored (RVV was cleared)
> > +
> > +     j       realign
> > +. = . + 2
> > +realign:
> > +
> > +     # Success!
> > +     li      a0, 0
> > +     j       _exit
> > +
> > +trap:
> > +     # Any trap is a fail code 1
> > +     li      a0, 1
> > +
> > +# Exit code in a0
> > +_exit:
> > +     lla     a1, semiargs
> > +     li      t0, 0x20026     # ADP_Stopped_ApplicationExit
> > +     sd      t0, 0(a1)
> > +     sd      a0, 8(a1)
> > +     li      a0, 0x20        # TARGET_SYS_EXIT_EXTENDED
> > +
> > +     # Semihosting call sequence
> > +     .balign 16
> > +     slli    zero, zero, 0x1f
> > +     ebreak
> > +     srai    zero, zero, 0x7
> > +     j       .
> > +
> > +     .data
> > +     .balign 16
> > +semiargs:
> > +     .space  16
> > --
> > 2.51.0
> >
> >
>