| Message ID | 1565251121-28490-2-git-send-email-vincent.chen@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: Correct the initialized flow of FP and __fstate_clean() | expand |
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(¤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 <anup@brainfault.org> Regards, Anup
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
> +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>
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
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
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
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 }
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(-)