[v3] target/riscv/csr.c: Fix an access to VXSAT

The register VXSAT should be RW only to the first bit.
The remaining bits should be 0.

The RISC-V Instruction Set Manual Volume I: Unprivileged Architecture

The vxsat CSR has a single read-write least-significant bit (vxsat[0])
that indicates if a fixed-point instruction has had to saturate an output
value to fit into a destination format. Bits vxsat[XLEN-1:1]
should be written as zeros.

Signed-off-by: Evgenii Prokopiev <evgenii.prokopiev@syntacore.com>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
Changes since v2:
    - Added reviewed-by tag
 target/riscv/csr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

On 10/2/24 01:44, Evgenii Prokopiev wrote:
> The register VXSAT should be RW only to the first bit.
> The remaining bits should be 0.
> 
> The RISC-V Instruction Set Manual Volume I: Unprivileged Architecture
> 
> The vxsat CSR has a single read-write least-significant bit (vxsat[0])
> that indicates if a fixed-point instruction has had to saturate an output
> value to fit into a destination format. Bits vxsat[XLEN-1:1]
> should be written as zeros.
> 
> Signed-off-by: Evgenii Prokopiev <evgenii.prokopiev@syntacore.com>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> Changes since v2:
>      - Added reviewed-by tag
>   target/riscv/csr.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

New versions should not be replies to previous versions.
No need to re-spin *only* to collect tags; tools can do that.

> 
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index bd080f92b5..69c41212e9 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -717,7 +717,7 @@ static RISCVException write_vxrm(CPURISCVState *env, int csrno,
>   static RISCVException read_vxsat(CPURISCVState *env, int csrno,
>                                    target_ulong *val)
>   {
> -    *val = env->vxsat;
> +    *val = env->vxsat & BIT(0);
>       return RISCV_EXCP_NONE;
>   }

Nit: no need to mask on read...

>   
> @@ -727,7 +727,7 @@ static RISCVException write_vxsat(CPURISCVState *env, int csrno,
>   #if !defined(CONFIG_USER_ONLY)
>       env->mstatus |= MSTATUS_VS;
>   #endif
> -    env->vxsat = val;
> +    env->vxsat = val & BIT(0);
>       return RISCV_EXCP_NONE;
>   }

... because you know the value is already correct from the write.


r~

On 04.10.2024 16:38, Evgenii Prokopiev wrote:
> 
> 
> On 03.10.2024 23:13, Richard Henderson wrote:
>> On 10/2/24 01:44, Evgenii Prokopiev wrote:
>>> The register VXSAT should be RW only to the first bit.
>>> The remaining bits should be 0.
>>>
>>> The RISC-V Instruction Set Manual Volume I: Unprivileged Architecture
>>>
>>> The vxsat CSR has a single read-write least-significant bit (vxsat[0])
>>> that indicates if a fixed-point instruction has had to saturate an 
>>> output
>>> value to fit into a destination format. Bits vxsat[XLEN-1:1]
>>> should be written as zeros.
>>>
>>> Signed-off-by: Evgenii Prokopiev <evgenii.prokopiev@syntacore.com>
>>> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>> ---
>>> Changes since v2:
>>>      - Added reviewed-by tag
>>>   target/riscv/csr.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> New versions should not be replies to previous versions.
>> No need to re-spin *only* to collect tags; tools can do that.
>>
> Hi!
> Thanks for your explanation.
>>>
>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>> index bd080f92b5..69c41212e9 100644
>>> --- a/target/riscv/csr.c
>>> +++ b/target/riscv/csr.c
>>> @@ -717,7 +717,7 @@ static RISCVException write_vxrm(CPURISCVState 
>>> *env, int csrno,
>>>   static RISCVException read_vxsat(CPURISCVState *env, int csrno,
>>>                                    target_ulong *val)
>>>   {
>>> -    *val = env->vxsat;
>>> +    *val = env->vxsat & BIT(0);
>>>       return RISCV_EXCP_NONE;
>>>   }
>>
>> Nit: no need to mask on read...
>>
>>> @@ -727,7 +727,7 @@ static RISCVException write_vxsat(CPURISCVState 
>>> *env, int csrno,
>>>   #if !defined(CONFIG_USER_ONLY)
>>>       env->mstatus |= MSTATUS_VS;
>>>   #endif
>>> -    env->vxsat = val;
>>> +    env->vxsat = val & BIT(0);
>>>       return RISCV_EXCP_NONE;
>>>   }
>>
>> ... because you know the value is already correct from the write.
>>
> Yes, we mask the value when we make a write. But this value could be
> changed in other places. So I added a mask when reading happens too.
> If additional verification is not required, I will delete it.
>>
>> r~

On Fri, Oct 4, 2024 at 11:48 PM Evgenii Prokopiev
<evgenii.prokopiev@syntacore.com> wrote:
>
>
>
> On 04.10.2024 16:38, Evgenii Prokopiev wrote:
> >
> >
> > On 03.10.2024 23:13, Richard Henderson wrote:
> >> On 10/2/24 01:44, Evgenii Prokopiev wrote:
> >>> The register VXSAT should be RW only to the first bit.
> >>> The remaining bits should be 0.
> >>>
> >>> The RISC-V Instruction Set Manual Volume I: Unprivileged Architecture
> >>>
> >>> The vxsat CSR has a single read-write least-significant bit (vxsat[0])
> >>> that indicates if a fixed-point instruction has had to saturate an
> >>> output
> >>> value to fit into a destination format. Bits vxsat[XLEN-1:1]
> >>> should be written as zeros.
> >>>
> >>> Signed-off-by: Evgenii Prokopiev <evgenii.prokopiev@syntacore.com>
> >>> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> >>> ---
> >>> Changes since v2:
> >>>      - Added reviewed-by tag
> >>>   target/riscv/csr.c | 4 ++--
> >>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> New versions should not be replies to previous versions.
> >> No need to re-spin *only* to collect tags; tools can do that.
> >>
> > Hi!
> > Thanks for your explanation.
> >>>
> >>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >>> index bd080f92b5..69c41212e9 100644
> >>> --- a/target/riscv/csr.c
> >>> +++ b/target/riscv/csr.c
> >>> @@ -717,7 +717,7 @@ static RISCVException write_vxrm(CPURISCVState
> >>> *env, int csrno,
> >>>   static RISCVException read_vxsat(CPURISCVState *env, int csrno,
> >>>                                    target_ulong *val)
> >>>   {
> >>> -    *val = env->vxsat;
> >>> +    *val = env->vxsat & BIT(0);
> >>>       return RISCV_EXCP_NONE;
> >>>   }
> >>
> >> Nit: no need to mask on read...
> >>
> >>> @@ -727,7 +727,7 @@ static RISCVException write_vxsat(CPURISCVState
> >>> *env, int csrno,
> >>>   #if !defined(CONFIG_USER_ONLY)
> >>>       env->mstatus |= MSTATUS_VS;
> >>>   #endif
> >>> -    env->vxsat = val;
> >>> +    env->vxsat = val & BIT(0);
> >>>       return RISCV_EXCP_NONE;
> >>>   }
> >>
> >> ... because you know the value is already correct from the write.
> >>
> > Yes, we mask the value when we make a write. But this value could be
> > changed in other places. So I added a mask when reading happens too.
> > If additional verification is not required, I will delete it.

When replying you don't want to add any > characters for your response.

It probably makes sense to not mask the read, if other code changes
the value in the future the guest will probably want to know.

This is fine as is though, in the future the mask can be removed if required

Thanks!

Applied to riscv-to-apply.next

Alistair

> >>
> >> r~
>
> --
> Sincerely,
> Evgenii Prokopiev