[PATCH 0/2] riscv: Correct the initialized flow of FP and __fstate_clean()

[PATCH 0/2] riscv: Correct the initialized flow of FP and __fstate_clean()

[Vincent Chen]

The following two reasons cause FP registers are sometimes not
initialized before starting the user program.
1. Currently, the FP context is initialized in flush_thread() function
   and we expect these initial values to be restored to FP register when
   doing FP context switch. However, the FP context switch only occurs in
   switch_to function. Hence, if this process does not be scheduled out
   and scheduled in before entering the user space, the FP registers
   have no chance to initialize.
2. In flush_thread(), the state of reg->sstatus.FS inherits from the
   parent. Hence, the state of reg->sstatus.FS may be dirty. If this
   process is scheduled out during flush_thread() and initializing the
   FP register, the fstate_save() in switch_to will corrupt the FP context
   which has been initialized until flush_thread().
In addition, the __fstate_clean() function cannot correctly set the state
of sstatus.FS to SR_FS_CLEAN. These problems will be solved in this patch
set.


Vincent Chen (2):
  riscv: Correct the initialized flow of FP register
  riscv: Make __fstate_clean() can work correctly.

 arch/riscv/include/asm/switch_to.h |  8 +++++++-
 arch/riscv/kernel/process.c        | 13 +++++++++++--
 2 files changed, 18 insertions(+), 3 deletions(-)

-- 
2.7.4


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 1/2] riscv: Correct the initialized flow of FP register

[Vincent Chen]

  The following two reasons cause FP registers are sometimes not
initialized before starting the user program.
1. Currently, the FP context is initialized in flush_thread() function
   and we expect these initial values to be restored to FP register when
   doing FP context switch. However, the FP context switch only occurs in
   switch_to function. Hence, if this process does not be scheduled out
   and scheduled in before entering the user space, the FP registers
   have no chance to initialize.
2. In flush_thread(), the state of reg->sstatus.FS inherits from the
   parent. Hence, the state of reg->sstatus.FS may be dirty. If this
   process is scheduled out during flush_thread() and initializing the
   FP register, the fstate_save() in switch_to will corrupt the FP context
   which has been initialized until flush_thread().

  To solve the 1st case, the initialization of the FP register will be
completed in start_thread(). It makes sure all FP registers are initialized
before starting the user program. For the 2nd case, the state of
reg->sstatus.FS in start_thread will be set to SR_FS_OFF to prevent this
process from corrupting FP context in doing context save. The FP state is
set to SR_FS_INITIAL in start_trhead().

Tested on both QEMU and HiFive Unleashed using BBL + Linux.

Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
---
 arch/riscv/include/asm/switch_to.h |  6 ++++++
 arch/riscv/kernel/process.c        | 13 +++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/include/asm/switch_to.h b/arch/riscv/include/asm/switch_to.h
index 853b65e..d5fe573 100644
--- a/arch/riscv/include/asm/switch_to.h
+++ b/arch/riscv/include/asm/switch_to.h
@@ -19,6 +19,12 @@ static inline void __fstate_clean(struct pt_regs *regs)
 	regs->sstatus |= (regs->sstatus & ~(SR_FS)) | SR_FS_CLEAN;
 }
 
+static inline void fstate_off(struct task_struct *task,
+			       struct pt_regs *regs)
+{
+	regs->sstatus = (regs->sstatus & ~(SR_FS)) | SR_FS_OFF;
+}
+
 static inline void fstate_save(struct task_struct *task,
 			       struct pt_regs *regs)
 {
diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
index f23794b..e3077ee 100644
--- a/arch/riscv/kernel/process.c
+++ b/arch/riscv/kernel/process.c
@@ -64,8 +64,16 @@ void start_thread(struct pt_regs *regs, unsigned long pc,
 	unsigned long sp)
 {
 	regs->sstatus = SR_SPIE;
-	if (has_fpu)
+	if (has_fpu) {
 		regs->sstatus |= SR_FS_INITIAL;
+#ifdef CONFIG_FPU
+		/*
+		 * Restore the initial value to the FP register
+		 * before starting the user program.
+		 */
+		fstate_restore(current, regs);
+#endif
+	}
 	regs->sepc = pc;
 	regs->sp = sp;
 	set_fs(USER_DS);
@@ -75,10 +83,11 @@ void flush_thread(void)
 {
 #ifdef CONFIG_FPU
 	/*
-	 * Reset FPU context
+	 * Reset FPU state and context
 	 *	frm: round to nearest, ties to even (IEEE default)
 	 *	fflags: accrued exceptions cleared
 	 */
+	fstate_off(current, task_pt_regs(current));
 	memset(&current->thread.fstate, 0, sizeof(current->thread.fstate));
 #endif
 }
-- 
2.7.4


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 2/2] riscv: Make __fstate_clean() can work correctly.

[Vincent Chen]

Make the __fstate_clean() function can correctly set the
state of sstatus.FS in pt_regs to SR_FS_CLEAN.

Tested on both QEMU and HiFive Unleashed using BBL + Linux.

Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
---
 arch/riscv/include/asm/switch_to.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/switch_to.h b/arch/riscv/include/asm/switch_to.h
index d5fe573..544f99a 100644
--- a/arch/riscv/include/asm/switch_to.h
+++ b/arch/riscv/include/asm/switch_to.h
@@ -16,7 +16,7 @@ extern void __fstate_restore(struct task_struct *restore_from);
 
 static inline void __fstate_clean(struct pt_regs *regs)
 {
-	regs->sstatus |= (regs->sstatus & ~(SR_FS)) | SR_FS_CLEAN;
+	regs->sstatus = (regs->sstatus & ~(SR_FS)) | SR_FS_CLEAN;
 }
 
 static inline void fstate_off(struct task_struct *task,
-- 
2.7.4


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/2] riscv: Correct the initialized flow of FP register

[Anup Patel]

On Thu, Aug 8, 2019 at 1:30 PM Vincent Chen <vincent.chen@sifive.com> wrote:
>
>   The following two reasons cause FP registers are sometimes not
> initialized before starting the user program.
> 1. Currently, the FP context is initialized in flush_thread() function
>    and we expect these initial values to be restored to FP register when
>    doing FP context switch. However, the FP context switch only occurs in
>    switch_to function. Hence, if this process does not be scheduled out
>    and scheduled in before entering the user space, the FP registers
>    have no chance to initialize.
> 2. In flush_thread(), the state of reg->sstatus.FS inherits from the
>    parent. Hence, the state of reg->sstatus.FS may be dirty. If this
>    process is scheduled out during flush_thread() and initializing the
>    FP register, the fstate_save() in switch_to will corrupt the FP context
>    which has been initialized until flush_thread().
>
>   To solve the 1st case, the initialization of the FP register will be
> completed in start_thread(). It makes sure all FP registers are initialized
> before starting the user program. For the 2nd case, the state of
> reg->sstatus.FS in start_thread will be set to SR_FS_OFF to prevent this
> process from corrupting FP context in doing context save. The FP state is
> set to SR_FS_INITIAL in start_trhead().
>
> Tested on both QEMU and HiFive Unleashed using BBL + Linux.
>
> Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> ---
>  arch/riscv/include/asm/switch_to.h |  6 ++++++
>  arch/riscv/kernel/process.c        | 13 +++++++++++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/include/asm/switch_to.h b/arch/riscv/include/asm/switch_to.h
> index 853b65e..d5fe573 100644
> --- a/arch/riscv/include/asm/switch_to.h
> +++ b/arch/riscv/include/asm/switch_to.h
> @@ -19,6 +19,12 @@ static inline void __fstate_clean(struct pt_regs *regs)
>         regs->sstatus |= (regs->sstatus & ~(SR_FS)) | SR_FS_CLEAN;
>  }
>
> +static inline void fstate_off(struct task_struct *task,
> +                              struct pt_regs *regs)
> +{
> +       regs->sstatus = (regs->sstatus & ~(SR_FS)) | SR_FS_OFF;

The SR_FS_OFF is 0x0 so no need for ORing it.

> +}
> +
>  static inline void fstate_save(struct task_struct *task,
>                                struct pt_regs *regs)
>  {
> diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
> index f23794b..e3077ee 100644
> --- a/arch/riscv/kernel/process.c
> +++ b/arch/riscv/kernel/process.c
> @@ -64,8 +64,16 @@ void start_thread(struct pt_regs *regs, unsigned long pc,
>         unsigned long sp)
>  {
>         regs->sstatus = SR_SPIE;
> -       if (has_fpu)
> +       if (has_fpu) {
>                 regs->sstatus |= SR_FS_INITIAL;
> +#ifdef CONFIG_FPU
> +               /*
> +                * Restore the initial value to the FP register
> +                * before starting the user program.
> +                */
> +               fstate_restore(current, regs);
> +#endif
> +       }
>         regs->sepc = pc;
>         regs->sp = sp;
>         set_fs(USER_DS);
> @@ -75,10 +83,11 @@ void flush_thread(void)
>  {
>  #ifdef CONFIG_FPU
>         /*
> -        * Reset FPU context
> +        * Reset FPU state and context
>          *      frm: round to nearest, ties to even (IEEE default)
>          *      fflags: accrued exceptions cleared
>          */
> +       fstate_off(current, task_pt_regs(current));
>         memset(&current->thread.fstate, 0, sizeof(current->thread.fstate));
>  #endif
>  }
> --
> 2.7.4
>

Apart from above minor comment, looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/2] riscv: Make __fstate_clean() can work correctly.

[Anup Patel]

On Thu, Aug 8, 2019 at 1:30 PM Vincent Chen <vincent.chen@sifive.com> wrote:
>
> Make the __fstate_clean() function can correctly set the
> state of sstatus.FS in pt_regs to SR_FS_CLEAN.
>
> Tested on both QEMU and HiFive Unleashed using BBL + Linux.
>
> Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> ---
>  arch/riscv/include/asm/switch_to.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/include/asm/switch_to.h b/arch/riscv/include/asm/switch_to.h
> index d5fe573..544f99a 100644
> --- a/arch/riscv/include/asm/switch_to.h
> +++ b/arch/riscv/include/asm/switch_to.h
> @@ -16,7 +16,7 @@ extern void __fstate_restore(struct task_struct *restore_from);
>
>  static inline void __fstate_clean(struct pt_regs *regs)
>  {
> -       regs->sstatus |= (regs->sstatus & ~(SR_FS)) | SR_FS_CLEAN;
> +       regs->sstatus = (regs->sstatus & ~(SR_FS)) | SR_FS_CLEAN;
>  }
>
>  static inline void fstate_off(struct task_struct *task,
> --
> 2.7.4
>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

This should be a RC fix.

Please add "Fixes:" in your commit description and
CC stable kernel.

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/2] riscv: Correct the initialized flow of FP register

[Paul Walmsley]

On Thu, 8 Aug 2019, Anup Patel wrote:

> On Thu, Aug 8, 2019 at 1:30 PM Vincent Chen <vincent.chen@sifive.com> wrote:
> >
> > +static inline void fstate_off(struct task_struct *task,
> > +                              struct pt_regs *regs)
> > +{
> > +       regs->sstatus = (regs->sstatus & ~(SR_FS)) | SR_FS_OFF;
> 
> The SR_FS_OFF is 0x0 so no need for ORing it.

That one looks OK to me, since it makes it more obvious to humans what's 
happening here - reviewers won't need to know that "off" is 0x0.  The 
compiler should drop it internally, so it won't affect the generated 
code.

> Apart from above minor comment, looks good to me.
> 
> Reviewed-by: Anup Patel <anup@brainfault.org>

Will add your Reviewed-by: tag - let us know if you want me to drop it or 
caveat it.


- Paul

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/2] riscv: Correct the initialized flow of FP register

[Christoph Hellwig]

> +static inline void fstate_off(struct task_struct *task,
> +			       struct pt_regs *regs)
> +{
> +	regs->sstatus = (regs->sstatus & ~(SR_FS)) | SR_FS_OFF;

No need for the inner braces here.

> +}
> +
>  static inline void fstate_save(struct task_struct *task,
>  			       struct pt_regs *regs)
>  {
> diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
> index f23794b..e3077ee 100644
> --- a/arch/riscv/kernel/process.c
> +++ b/arch/riscv/kernel/process.c
> @@ -64,8 +64,16 @@ void start_thread(struct pt_regs *regs, unsigned long pc,
>  	unsigned long sp)
>  {
>  	regs->sstatus = SR_SPIE;
> -	if (has_fpu)
> +	if (has_fpu) {
>  		regs->sstatus |= SR_FS_INITIAL;
> +#ifdef CONFIG_FPU
> +		/*
> +		 * Restore the initial value to the FP register
> +		 * before starting the user program.
> +		 */
> +		fstate_restore(current, regs);
> +#endif

fstate_restore has a no-op stub for the !CONFIG_FPU case, so the ifdef
here is not needed.

Otherwise this looks good to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/2] riscv: Make __fstate_clean() can work correctly.

[Christoph Hellwig]

Maybe s/can //g in the subject?

> +	regs->sstatus = (regs->sstatus & ~(SR_FS)) | SR_FS_CLEAN;

No need for the inner braces here either.

Otherwise:

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/2] riscv: Correct the initialized flow of FP register

[Vincent Chen]

On Thu, Aug 8, 2019 at 11:50 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
>
> On Thu, 8 Aug 2019, Anup Patel wrote:
>
> > On Thu, Aug 8, 2019 at 1:30 PM Vincent Chen <vincent.chen@sifive.com> wrote:
> > >
> > > +static inline void fstate_off(struct task_struct *task,
> > > +                              struct pt_regs *regs)
> > > +{
> > > +       regs->sstatus = (regs->sstatus & ~(SR_FS)) | SR_FS_OFF;
> >
> > The SR_FS_OFF is 0x0 so no need for ORing it.
>
> That one looks OK to me, since it makes it more obvious to humans what's
> happening here - reviewers won't need to know that "off" is 0x0.  The
> compiler should drop it internally, so it won't affect the generated
> code.
>
Thanks for Paul's comment
My thought is the same as Paul.


> > Apart from above minor comment, looks good to me.
> >
> > Reviewed-by: Anup Patel <anup@brainfault.org>
>
> Will add your Reviewed-by: tag - let us know if you want me to drop it or
> caveat it.
>
>
> - Paul

Dear Anup,
I suppose you can accept our thought about using the SR_FS_OFF flag
because I didn't receive any reply from you.
Thanks for your review and comments.

Regards,
Vincent

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/2] riscv: Correct the initialized flow of FP register

[Vincent Chen]

On Mon, Aug 12, 2019 at 10:58 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> > +static inline void fstate_off(struct task_struct *task,
> > +                            struct pt_regs *regs)
> > +{
> > +     regs->sstatus = (regs->sstatus & ~(SR_FS)) | SR_FS_OFF;
>
> No need for the inner braces here.

Ok.

>
> > +}
> > +
> >  static inline void fstate_save(struct task_struct *task,
> >                              struct pt_regs *regs)
> >  {
> > diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
> > index f23794b..e3077ee 100644
> > --- a/arch/riscv/kernel/process.c
> > +++ b/arch/riscv/kernel/process.c
> > @@ -64,8 +64,16 @@ void start_thread(struct pt_regs *regs, unsigned long pc,
> >       unsigned long sp)
> >  {
> >       regs->sstatus = SR_SPIE;
> > -     if (has_fpu)
> > +     if (has_fpu) {
> >               regs->sstatus |= SR_FS_INITIAL;
> > +#ifdef CONFIG_FPU
> > +             /*
> > +              * Restore the initial value to the FP register
> > +              * before starting the user program.
> > +              */
> > +             fstate_restore(current, regs);
> > +#endif
>
> fstate_restore has a no-op stub for the !CONFIG_FPU case, so the ifdef
> here is not needed.
>
You are right. I will remove the Ifdef condition.

> Otherwise this looks good to me:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks for your comments.

Regards,
Vincent Chen

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/2] riscv: Make __fstate_clean() can work correctly.

[Vincent Chen]

On Thu, Aug 8, 2019 at 6:17 PM Anup Patel <anup@brainfault.org> wrote:
>
> On Thu, Aug 8, 2019 at 1:30 PM Vincent Chen <vincent.chen@sifive.com> wrote:
> >
> > Make the __fstate_clean() function can correctly set the
> > state of sstatus.FS in pt_regs to SR_FS_CLEAN.
> >
> > Tested on both QEMU and HiFive Unleashed using BBL + Linux.
> >
> > Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> > ---
> >  arch/riscv/include/asm/switch_to.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/include/asm/switch_to.h b/arch/riscv/include/asm/switch_to.h
> > index d5fe573..544f99a 100644
> > --- a/arch/riscv/include/asm/switch_to.h
> > +++ b/arch/riscv/include/asm/switch_to.h
> > @@ -16,7 +16,7 @@ extern void __fstate_restore(struct task_struct *restore_from);
> >
> >  static inline void __fstate_clean(struct pt_regs *regs)
> >  {
> > -       regs->sstatus |= (regs->sstatus & ~(SR_FS)) | SR_FS_CLEAN;
> > +       regs->sstatus = (regs->sstatus & ~(SR_FS)) | SR_FS_CLEAN;
> >  }
> >
> >  static inline void fstate_off(struct task_struct *task,
> > --
> > 2.7.4
> >
>
> Looks good to me.
>
> Reviewed-by: Anup Patel <anup@brainfault.org>
>
> This should be a RC fix.
>
> Please add "Fixes:" in your commit description and
> CC stable kernel.
>
OK, I will follow your suggestions and resend this patch
Thanks for your comments.

Regards,
Vincent Chen

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/2] riscv: Make __fstate_clean() can work correctly.

[Vincent Chen]

On Mon, Aug 12, 2019 at 10:59 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> Maybe s/can //g in the subject?
>
> > +     regs->sstatus = (regs->sstatus & ~(SR_FS)) | SR_FS_CLEAN;
>
> No need for the inner braces here either.
OK, I will remove them.

>
> Otherwise:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks for your comments

Regards,
Vincent Chen

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/2] riscv: Correct the initialized flow of FP register

[Anup Patel]

On Wed, Aug 14, 2019 at 7:15 AM Vincent Chen <vincent.chen@sifive.com> wrote:
>
> On Thu, Aug 8, 2019 at 11:50 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
> >
> > On Thu, 8 Aug 2019, Anup Patel wrote:
> >
> > > On Thu, Aug 8, 2019 at 1:30 PM Vincent Chen <vincent.chen@sifive.com> wrote:
> > > >
> > > > +static inline void fstate_off(struct task_struct *task,
> > > > +                              struct pt_regs *regs)
> > > > +{
> > > > +       regs->sstatus = (regs->sstatus & ~(SR_FS)) | SR_FS_OFF;
> > >
> > > The SR_FS_OFF is 0x0 so no need for ORing it.
> >
> > That one looks OK to me, since it makes it more obvious to humans what's
> > happening here - reviewers won't need to know that "off" is 0x0.  The
> > compiler should drop it internally, so it won't affect the generated
> > code.
> >
> Thanks for Paul's comment
> My thought is the same as Paul.
>
>
> > > Apart from above minor comment, looks good to me.
> > >
> > > Reviewed-by: Anup Patel <anup@brainfault.org>
> >
> > Will add your Reviewed-by: tag - let us know if you want me to drop it or
> > caveat it.
> >
> >
> > - Paul
>
> Dear Anup,
> I suppose you can accept our thought about using the SR_FS_OFF flag
> because I didn't receive any reply from you.
> Thanks for your review and comments.

No problem, go ahead without dropping SR_FS_OFF flag.

You can include my Reviewed-by.

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv