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

[LIU Zhiwei]

On 5/28/21 6:19 AM, Alistair Francis wrote:
> 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.

Hi Alistair,

Thanks for your reply.
I think when HS or M mode writes to vsie, it should set VSSIE instead of 
SSIE.

Best Regards,
Zhiwei

> 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