Re: [PATCH 1/1] target/riscv: Fix vsip vsie CSR ops in M and HS mode

[Alistair Francis]

On Thu, May 27, 2021 at 7:01 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>
> When V=1, instructions that normally read or modify a supervisor CSR
> shall instead access the corresponding VS CSR. And the VS CSRs can be
> accessed as themselves from M-mode or HS-mode.
>
> In M and HS mode, VSIP or VSIE should be written normally instead of
> shift by 1.
>
> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> ---
>  target/riscv/csr.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index fe5628fea6..0cce474d3d 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -837,16 +837,16 @@ static RISCVException read_sie(CPURISCVState *env, int 
> csrno,
>  static RISCVException write_vsie(CPURISCVState *env, int csrno,
>                                   target_ulong val)
>  {
> -    /* Shift the S bits to their VS bit location in mie */
>      target_ulong newval = (env->mie & ~VS_MODE_INTERRUPTS) |
> -                          ((val << 1) & env->hideleg & VS_MODE_INTERRUPTS);
> +                          (val & env->hideleg & VS_MODE_INTERRUPTS);

Ok, so when a Hypervisor writes to vsie it should actually set SSIE
instead of VSSIE. That looks right.

The problem here now is that you are still using the VS mask, so this
won't set anything.

>      return write_mie(env, CSR_MIE, newval);
>  }
>
>  static int write_sie(CPURISCVState *env, int csrno, target_ulong val)
>  {
>      if (riscv_cpu_virt_enabled(env)) {
> -        write_vsie(env, CSR_VSIE, val);
> +        /* Shift the S bits to their VS bit location in mie */
> +        write_vsie(env, CSR_VSIE, val << 1);

A write to SIE when virtulised actually sets VSIE, sounds good.

>      } else {
>          target_ulong newval = (env->mie & ~S_MODE_INTERRUPTS) |
>                                (val & S_MODE_INTERRUPTS);
> @@ -950,12 +950,9 @@ static RISCVException rmw_vsip(CPURISCVState *env, int 
> csrno,
>                                 target_ulong *ret_value,
>                                 target_ulong new_value, target_ulong 
> write_mask)
>  {
> -    /* Shift the S bits to their VS bit location in mip */
> -    int ret = rmw_mip(env, 0, ret_value, new_value << 1,
> -                      (write_mask << 1) & vsip_writable_mask & env->hideleg);
> +    int ret = rmw_mip(env, 0, ret_value, new_value,
> +                      write_mask & vsip_writable_mask & env->hideleg);

The mask seems wrong here as well.

Alistair

>      *ret_value &= VS_MODE_INTERRUPTS;
> -    /* Shift the VS bits to their S bit location in vsip */
> -    *ret_value >>= 1;
>      return ret;
>  }
>
> @@ -966,7 +963,11 @@ static RISCVException rmw_sip(CPURISCVState *env, int 
> csrno,
>      int ret;
>
>      if (riscv_cpu_virt_enabled(env)) {
> -        ret = rmw_vsip(env, CSR_VSIP, ret_value, new_value, write_mask);
> +        /* Shift the S bits to their VS bit location in mip */
> +        ret = rmw_vsip(env, CSR_VSIP, ret_value, new_value << 1,
> +                       write_mask << 1);
> +        /* Shift the VS bits to their S bit location in vsip */
> +        *ret_value >>= 1;
>      } else {
>          ret = rmw_mip(env, CSR_MSTATUS, ret_value, new_value,
>                        write_mask & env->mideleg & sip_writable_mask);
> --
> 2.25.1
>
>