[PATCH] riscv: vector: Check SR_SD before saving vstate

[Andy Chiu]

On Thu, Dec 21, 2023 at 3:37 PM Wang, Xiao W <xiao.w.wang at intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Song Shuai <songshuaishuai at tinylab.org>
> > Sent: Thursday, December 21, 2023 3:05 PM
> > To: paul.walmsley at sifive.com; palmer at dabbelt.com;
> > aou at eecs.berkeley.edu; andy.chiu at sifive.com; greentime.hu at sifive.com;
> > conor.dooley at microchip.com; guoren at kernel.org;
> > songshuaishuai at tinylab.org; bjorn at rivosinc.com; Wang, Xiao W
> > <xiao.w.wang at intel.com>; heiko at sntech.de; ruinland.tsai at sifive.com
> > Cc: linux-riscv at lists.infradead.org; linux-kernel at vger.kernel.org
> > Subject: [PATCH] riscv: vector: Check SR_SD before saving vstate
> >
> > The SD bit summarizes the dirty states of FS, VS, or XS fields,
> > providing a "fast check" before saving fstate or vstate.
> >
> > Let __switch_to_vector() check SD bit as __switch_to_fpu() does.
>
> It looks a duplication of status check since the __switch_to_*() internally will check the ext specific status bit.
> Can we just remove SR_SD check for the fpu() case?

Hi, I came to the same question when adding the Vector context switch.
I think the better solution is to skip saving both fpu and vector if
the SD is clear. However, this may make code less maintainable because
fpu and vector code are scattered and we must mix code together by
doing that. Also, we will have to duplicate has_fpu and has_vector
check because of the branch dependency

e.g.

if (likely((regs->status & SR_SD))) {
    if (has_fpu())
        fstate_save()
    if (has_vector())
        vstate_save()
}

if (has_fpu()) <--- duplicated check (nop)
    fstate_restore()
if (has_vector()) <--- same
    vstate_restore()

So, it really is "Is it worth to trade extra nop with SR_SD that
potentially skip one SR_*S check"

Besides, with user space libraries start embracing Vector, I don't
expect SR_SD would be in "cleared" state very often. Though I haven't
done any real-world experiment yet.

>
> BRs,
> Xiao
>
> >
> > Fixes: 3a2df6323def ("riscv: Add task switch support for vector")
> > Signed-off-by: Song Shuai <songshuaishuai at tinylab.org>
> > ---
> >  arch/riscv/include/asm/vector.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> > index 87aaef656257..d30fa56f67c6 100644
> > --- a/arch/riscv/include/asm/vector.h
> > +++ b/arch/riscv/include/asm/vector.h
> > @@ -190,7 +190,8 @@ static inline void __switch_to_vector(struct
> > task_struct *prev,
> >       struct pt_regs *regs;
> >
> >       regs = task_pt_regs(prev);
> > -     riscv_v_vstate_save(prev, regs);
> > +     if (unlikely(regs->status & SR_SD))
> > +             riscv_v_vstate_save(prev, regs);
> >       riscv_v_vstate_restore(next, task_pt_regs(next));
> >  }
> >
> > --
> > 2.20.1
>

Thanks,
Andy