[v1] target/riscv: corner case fixes

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

Posted by Nicholas Piggin 5 months, 1 week ago

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 8842e07a73..64b55b7add 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -2140,9 +2140,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).
+         */
+        g_assert(env->misa_ext & RVC);
+        return RISCV_EXCP_NONE;
     }
 
     /* 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 3ca595335d..6e470a028f 100644
--- a/tests/tcg/riscv64/Makefile.softmmu-target
+++ b/tests/tcg/riscv64/Makefile.softmmu-target
@@ -24,5 +24,10 @@ EXTRA_RUNS += run-test-mepc-masking
 run-test-mepc-masking: test-mepc-masking
 	$(call run-test, $<, $(QEMU) $(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

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

Posted by Daniel Henrique Barboza 5 months, 1 week ago


On 9/3/25 12:01 AM, 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>

Fixes: f18637cd61 ("RISC-V: Add misa runtime write support")

> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.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 8842e07a73..64b55b7add 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2140,9 +2140,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).
> +         */
> +        g_assert(env->misa_ext & RVC);
> +        return RISCV_EXCP_NONE;
>       }
>   
>       /* 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 3ca595335d..6e470a028f 100644
> --- a/tests/tcg/riscv64/Makefile.softmmu-target
> +++ b/tests/tcg/riscv64/Makefile.softmmu-target
> @@ -24,5 +24,10 @@ EXTRA_RUNS += run-test-mepc-masking
>   run-test-mepc-masking: test-mepc-masking
>   	$(call run-test, $<, $(QEMU) $(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

[PATCH 1/3] target/riscv: Fix IALIGN check in misa write
[PATCH 2/3] target/risvc: Fix vector whole ldst vstart check
[PATCH 3/3] tests/tcg: Add riscv test for interrupted vector ops