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

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

[Vladimir Isaev]

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 disabling "C" if it is already enabled and
next instruction is not aligned to 4.

Fixes: f18637cd611c ("RISC-V: Add misa runtime write support")
Signed-off-by: Vladimir Isaev <vladimir.isaev@syntacore.com>
---
 target/riscv/csr.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

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

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

[Daniel Henrique Barboza]

On 2/20/25 1:31 PM, 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 disabling "C" if it is already enabled and
> next instruction is not aligned to 4.
> 
> Fixes: f18637cd611c ("RISC-V: Add misa runtime write support")
> Signed-off-by: Vladimir Isaev <vladimir.isaev@syntacore.com>
> ---
>   target/riscv/csr.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index afb7544f0780..32f9b7b16f6f 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2067,11 +2067,12 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>       val &= env->misa_ext_mask;
>   
>       /*
> -     * Suppress 'C' if next instruction is not aligned
> +     * Disabling 'C' increases IALIGN to 32. If subsequent instruction's address
> +     * is not 32-bit aligned, write to misa is suppressed.
>        * TODO: this should check next_pc
>        */
> -    if ((val & RVC) && (GETPC() & ~3) != 0) {
> -        val &= ~RVC;

Not related to your changes but it would be nice if we made more use of
QEMU_IS_ALIGNED() instead of doing these bitwise ops to check alignment.
E.g. to check if pc is aligned to 4: QEMU_IS_ALIGNED(GETPC(), 4).


> +    if (!(val & RVC) && (env->misa_ext & RVC) && (GETPC() & 0x3)) {
> +        return RISCV_EXCP_NONE;
>       }

This will abort the write altogether, skipping valid changes to other bits. What
we want is a "val &= ~RVC" if the proper conditions for clearing RVC are met.

Thanks,

Daniel

>   
>       /* Disable RVG if any of its dependencies are disabled */

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

[Vladimir Isaev]

20.02.2025 20:59, Daniel Henrique Barboza wrote:
> 
> 
> On 2/20/25 1:31 PM, 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 disabling "C" if it is already enabled and
>> next instruction is not aligned to 4.
>>
>> Fixes: f18637cd611c ("RISC-V: Add misa runtime write support")
>> Signed-off-by: Vladimir Isaev <vladimir.isaev@syntacore.com>
>> ---
>>   target/riscv/csr.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index afb7544f0780..32f9b7b16f6f 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -2067,11 +2067,12 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>>       val &= env->misa_ext_mask;
>>         /*
>> -     * Suppress 'C' if next instruction is not aligned
>> +     * Disabling 'C' increases IALIGN to 32. If subsequent instruction's address
>> +     * is not 32-bit aligned, write to misa is suppressed.
>>        * TODO: this should check next_pc
>>        */
>> -    if ((val & RVC) && (GETPC() & ~3) != 0) {
>> -        val &= ~RVC;
> 
> Not related to your changes but it would be nice if we made more use of
> QEMU_IS_ALIGNED() instead of doing these bitwise ops to check alignment.
> E.g. to check if pc is aligned to 4: QEMU_IS_ALIGNED(GETPC(), 4).

will fix, thank you!

> 
> 
>> +    if (!(val & RVC) && (env->misa_ext & RVC) && (GETPC() & 0x3)) {
>> +        return RISCV_EXCP_NONE;
>>       }
> 
> This will abort the write altogether, skipping valid changes to other bits. What
> we want is a "val &= ~RVC" if the proper conditions for clearing RVC are met.

Not sure since specification says:

> 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.

In my understanding here we should skip all changes to misa, not just C.

Thank you,
Vladimir Isaev

> 
> Thanks,
> 
> Daniel
> 
>>         /* Disable RVG if any of its dependencies are disabled */
> 
> 

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

[Daniel Henrique Barboza]

On 2/21/25 4:58 AM, Vladimir Isaev wrote:
> 
> 
> 20.02.2025 20:59, Daniel Henrique Barboza wrote:
>>
>>
>> On 2/20/25 1:31 PM, 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 disabling "C" if it is already enabled and
>>> next instruction is not aligned to 4.
>>>
>>> Fixes: f18637cd611c ("RISC-V: Add misa runtime write support")
>>> Signed-off-by: Vladimir Isaev <vladimir.isaev@syntacore.com>
>>> ---
>>>    target/riscv/csr.c | 7 ++++---
>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>> index afb7544f0780..32f9b7b16f6f 100644
>>> --- a/target/riscv/csr.c
>>> +++ b/target/riscv/csr.c
>>> @@ -2067,11 +2067,12 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>>>        val &= env->misa_ext_mask;
>>>          /*
>>> -     * Suppress 'C' if next instruction is not aligned
>>> +     * Disabling 'C' increases IALIGN to 32. If subsequent instruction's address
>>> +     * is not 32-bit aligned, write to misa is suppressed.
>>>         * TODO: this should check next_pc
>>>         */
>>> -    if ((val & RVC) && (GETPC() & ~3) != 0) {
>>> -        val &= ~RVC;
>>
>> Not related to your changes but it would be nice if we made more use of
>> QEMU_IS_ALIGNED() instead of doing these bitwise ops to check alignment.
>> E.g. to check if pc is aligned to 4: QEMU_IS_ALIGNED(GETPC(), 4).
> 
> will fix, thank you!
> 
>>
>>
>>> +    if (!(val & RVC) && (env->misa_ext & RVC) && (GETPC() & 0x3)) {
>>> +        return RISCV_EXCP_NONE;
>>>        }
>>
>> This will abort the write altogether, skipping valid changes to other bits. What
>> we want is a "val &= ~RVC" if the proper conditions for clearing RVC are met.
> 
> Not sure since specification says:
> 
>> 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.
> 
> In my understanding here we should skip all changes to misa, not just C.

Oh, and changing RVC would change IALIGN. Makes sense then to keep this
early exit then. Thanks,

Daniel



> 
> Thank you,
> Vladimir Isaev
> 
>>
>> Thanks,
>>
>> Daniel
>>
>>>          /* Disable RVG if any of its dependencies are disabled */
>>
>>
> 
> 

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

[Richard Henderson]

On 2/20/25 08:31, 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 disabling "C" if it is already enabled and
> next instruction is not aligned to 4.
> 
> Fixes: f18637cd611c ("RISC-V: Add misa runtime write support")
> Signed-off-by: Vladimir Isaev <vladimir.isaev@syntacore.com>
> ---
>   target/riscv/csr.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index afb7544f0780..32f9b7b16f6f 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2067,11 +2067,12 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>       val &= env->misa_ext_mask;
>   
>       /*
> -     * Suppress 'C' if next instruction is not aligned
> +     * Disabling 'C' increases IALIGN to 32. If subsequent instruction's address
> +     * is not 32-bit aligned, write to misa is suppressed.
>        * TODO: this should check next_pc
>        */
> -    if ((val & RVC) && (GETPC() & ~3) != 0) {
> -        val &= ~RVC;
> +    if (!(val & RVC) && (env->misa_ext & RVC) && (GETPC() & 0x3)) {
> +        return RISCV_EXCP_NONE;
>       }

GETPC is a host thing, not a target thing.
Both the old and new code are very wrong.

You wanted to check env->pc, but this isn't updated before calling gen_helper_csr*w.

The simplest fix is to make sure that happens in the translator.

A slightly more complex fix that does not require all csr writes to update the pc would 
involve using cpu_unwind_state_data.  See target/openrisc/sys_helper.c for an example.


r~