[v2] target/riscv: fix C extension disabling on misa write

[PATCH v2] target/riscv: fix C extension disabling on misa write

Posted by Vladimir Isaev 11 months, 3 weeks ago

According to spec:
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.

So we should suppress write to misa without "C" if it is enabled
and the subsequent instruction is not aligned to 4.

Fixes: f18637cd611c ("RISC-V: Add misa runtime write support")
Signed-off-by: Vladimir Isaev <vladimir.isaev@syntacore.com>
---
v2:
- use env->pc instead of GETPC() since GETPC() is a host pc;
- use !QEMU_IS_ALIGNED(env->pc, 4) instead of GETPC() & 3;

---
 target/riscv/csr.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index afb7544f0780..8aa77c53a0db 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -2067,11 +2067,13 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
     val &= env->misa_ext_mask;
 
     /*
-     * Suppress 'C' if next instruction is not aligned
-     * TODO: this should check next_pc
+     * Disabling 'C' increases IALIGN to 32. If subsequent instruction's address
+     * is not 32-bit aligned, write to misa is suppressed.
+     *
+     * All csr-related instructions are 4-byte, so we can check current pc alignment.
      */
-    if ((val & RVC) && (GETPC() & ~3) != 0) {
-        val &= ~RVC;
+    if (!(val & RVC) && (env->misa_ext & RVC) && !QEMU_IS_ALIGNED(env->pc, 4)) {
+        return RISCV_EXCP_NONE;
     }
 
     /* Disable RVG if any of its dependencies are disabled */
-- 
2.47.2

Re: [PATCH v2] target/riscv: fix C extension disabling on misa write

Posted by Alistair Francis 11 months, 2 weeks ago

On Sat, Feb 22, 2025 at 12:00 AM Vladimir Isaev
<vladimir.isaev@syntacore.com> wrote:
>
> According to spec:
> 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.
>
> So we should suppress write to misa without "C" if it is enabled
> and the subsequent instruction is not aligned to 4.
>
> Fixes: f18637cd611c ("RISC-V: Add misa runtime write support")
> Signed-off-by: Vladimir Isaev <vladimir.isaev@syntacore.com>
> ---
> v2:
> - use env->pc instead of GETPC() since GETPC() is a host pc;
> - use !QEMU_IS_ALIGNED(env->pc, 4) instead of GETPC() & 3;
>
> ---
>  target/riscv/csr.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index afb7544f0780..8aa77c53a0db 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2067,11 +2067,13 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>      val &= env->misa_ext_mask;
>
>      /*
> -     * Suppress 'C' if next instruction is not aligned
> -     * TODO: this should check next_pc
> +     * Disabling 'C' increases IALIGN to 32. If subsequent instruction's address
> +     * is not 32-bit aligned, write to misa is suppressed.
> +     *
> +     * All csr-related instructions are 4-byte, so we can check current pc alignment.
>       */
> -    if ((val & RVC) && (GETPC() & ~3) != 0) {
> -        val &= ~RVC;
> +    if (!(val & RVC) && (env->misa_ext & RVC) && !QEMU_IS_ALIGNED(env->pc, 4)) {

env->pc has a stale PC unfortunately

See https://patchew.org/QEMU/20250220163120.77328-1-vladimir.isaev@syntacore.com/#97bea0ff-f93a-45a5-b8ec-2bb91e37f143@linaro.org
for details on how to fix this

Alistair

> +        return RISCV_EXCP_NONE;
>      }
>
>      /* Disable RVG if any of its dependencies are disabled */
> --
> 2.47.2
>
>
>

Re: [PATCH v2] target/riscv: fix C extension disabling on misa write

Posted by Vladimir Isaev 11 months, 2 weeks ago


24.02.2025 06:53, Alistair Francis wrote:
> On Sat, Feb 22, 2025 at 12:00 AM Vladimir Isaev
> <vladimir.isaev@syntacore.com> wrote:
>>
>> According to spec:
>> 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.
>>
>> So we should suppress write to misa without "C" if it is enabled
>> and the subsequent instruction is not aligned to 4.
>>
>> Fixes: f18637cd611c ("RISC-V: Add misa runtime write support")
>> Signed-off-by: Vladimir Isaev <vladimir.isaev@syntacore.com>
>> ---
>> v2:
>> - use env->pc instead of GETPC() since GETPC() is a host pc;
>> - use !QEMU_IS_ALIGNED(env->pc, 4) instead of GETPC() & 3;
>>
>> ---
>>  target/riscv/csr.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index afb7544f0780..8aa77c53a0db 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -2067,11 +2067,13 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>>      val &= env->misa_ext_mask;
>>
>>      /*
>> -     * Suppress 'C' if next instruction is not aligned
>> -     * TODO: this should check next_pc
>> +     * Disabling 'C' increases IALIGN to 32. If subsequent instruction's address
>> +     * is not 32-bit aligned, write to misa is suppressed.
>> +     *
>> +     * All csr-related instructions are 4-byte, so we can check current pc alignment.
>>       */
>> -    if ((val & RVC) && (GETPC() & ~3) != 0) {
>> -        val &= ~RVC;
>> +    if (!(val & RVC) && (env->misa_ext & RVC) && !QEMU_IS_ALIGNED(env->pc, 4)) {
> 

Hi Alistar,

> env->pc has a stale PC unfortunately
> 
> See https://patchew.org/QEMU/20250220163120.77328-1-vladimir.isaev@syntacore.com/#97bea0ff-f93a-45a5-b8ec-2bb91e37f143@linaro.org
> for details on how to fix this
> 

Thank you for pointing this (I somehow missed that mail:(), but is my understanding correct that env->pc contains
current instruction PC? and if we know that all csrw/csrs instructions are 4-byte wide we can use current
instruction pc?

or env->pc may contain even previous pc?

> Alistair
> 
>> +        return RISCV_EXCP_NONE;
>>      }
>>
>>      /* Disable RVG if any of its dependencies are disabled */
>> --
>> 2.47.2
>>
>>
>>
>

Re: [PATCH v2] target/riscv: fix C extension disabling on misa write

Posted by Daniel Henrique Barboza 11 months, 3 weeks ago


On 2/21/25 10:57 AM, Vladimir Isaev wrote:
> According to spec:
> 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.
> 
> So we should suppress write to misa without "C" if it is enabled
> and the subsequent instruction is not aligned to 4.
> 
> Fixes: f18637cd611c ("RISC-V: Add misa runtime write support")
> Signed-off-by: Vladimir Isaev <vladimir.isaev@syntacore.com>
> ---
> v2:
> - use env->pc instead of GETPC() since GETPC() is a host pc;
> - use !QEMU_IS_ALIGNED(env->pc, 4) instead of GETPC() & 3;
> 
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>


>   target/riscv/csr.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index afb7544f0780..8aa77c53a0db 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2067,11 +2067,13 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>       val &= env->misa_ext_mask;
>   
>       /*
> -     * Suppress 'C' if next instruction is not aligned
> -     * TODO: this should check next_pc
> +     * Disabling 'C' increases IALIGN to 32. If subsequent instruction's address
> +     * is not 32-bit aligned, write to misa is suppressed.
> +     *
> +     * All csr-related instructions are 4-byte, so we can check current pc alignment.
>        */
> -    if ((val & RVC) && (GETPC() & ~3) != 0) {
> -        val &= ~RVC;
> +    if (!(val & RVC) && (env->misa_ext & RVC) && !QEMU_IS_ALIGNED(env->pc, 4)) {
> +        return RISCV_EXCP_NONE;
>       }
>   
>       /* Disable RVG if any of its dependencies are disabled */