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

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

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 */
>>
>>
> 
>