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
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 <[email protected]>
---
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(¤t->thread.fstate, 0, sizeof(current->thread.fstate));
#endif
}
--
2.7.4
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 <[email protected]>
---
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
On Thu, Aug 8, 2019 at 1:30 PM Vincent Chen <[email protected]> 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 <[email protected]>
> ---
> 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(¤t->thread.fstate, 0, sizeof(current->thread.fstate));
> #endif
> }
> --
> 2.7.4
>
Apart from above minor comment, looks good to me.
Reviewed-by: Anup Patel <[email protected]>
Regards,
Anup
On Thu, Aug 8, 2019 at 1:30 PM Vincent Chen <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
This should be a RC fix.
Please add "Fixes:" in your commit description and
CC stable kernel.
Regards,
Anup
On Thu, 8 Aug 2019, Anup Patel wrote:
> On Thu, Aug 8, 2019 at 1:30 PM Vincent Chen <[email protected]> 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 <[email protected]>
Will add your Reviewed-by: tag - let us know if you want me to drop it or
caveat it.
- Paul
> +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 <[email protected]>
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 <[email protected]>
On Thu, Aug 8, 2019 at 11:50 PM Paul Walmsley <[email protected]> wrote:
>
> On Thu, 8 Aug 2019, Anup Patel wrote:
>
> > On Thu, Aug 8, 2019 at 1:30 PM Vincent Chen <[email protected]> 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 <[email protected]>
>
> 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
On Mon, Aug 12, 2019 at 10:58 PM Christoph Hellwig <[email protected]> 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 <[email protected]>
Thanks for your comments.
Regards,
Vincent Chen
On Thu, Aug 8, 2019 at 6:17 PM Anup Patel <[email protected]> wrote:
>
> On Thu, Aug 8, 2019 at 1:30 PM Vincent Chen <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
>
> 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
On Mon, Aug 12, 2019 at 10:59 PM Christoph Hellwig <[email protected]> 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 <[email protected]>
Thanks for your comments
Regards,
Vincent Chen
On Wed, Aug 14, 2019 at 7:15 AM Vincent Chen <[email protected]> wrote:
>
> On Thu, Aug 8, 2019 at 11:50 PM Paul Walmsley <[email protected]> wrote:
> >
> > On Thu, 8 Aug 2019, Anup Patel wrote:
> >
> > > On Thu, Aug 8, 2019 at 1:30 PM Vincent Chen <[email protected]> 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 <[email protected]>
> >
> > 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