| Message ID | 1550519559-15915-3-git-send-email-Dave.Martin@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: arm64: SVE guest support | expand |
Hi Dave, On 18/02/2019 19:52, Dave Martin wrote: > This patch updates fpsimd_flush_task_state() to mirror the new > semantics of fpsimd_flush_cpu_state() introduced by commit > d8ad71fa38a9 ("arm64: fpsimd: Fix TIF_FOREIGN_FPSTATE after > invalidating cpu regs"). Both functions now implicitly set NIT: Double-space before "Both" > TIF_FOREIGN_FPSTATE to indicate that the task's FPSIMD state is not > loaded into the cpu. > > As a side-effect, fpsimd_flush_task_state() now sets > TIF_FOREIGN_FPSTATE even for non-running tasks. In the case of NIT: Double sppace before "In". > non-running tasks this is not useful but also harmless, because the > flag is live only while the corresponding task is running. This > function is not called from fast paths, so special-casing this for > the task == current case is not really worth it. > > Compiler barriers previously present in restore_sve_fpsimd_context() > are pulled into fpsimd_flush_task_state() so that it can be safely > called with preemption enabled if necessary. > > Explicit calls to set TIF_FOREIGN_FPSTATE that accompany > fpsimd_flush_task_state() calls and are now redundant are removed > as appropriate. > > fpsimd_flush_task_state() is used to get exclusive access to the > representation of the task's state via task_struct, for the purpose > of replacing the state. Thus, the call to this function should NIT: Double-space before "Thus". > happen before manipulating fpsimd_state or sve_state etc. in > task_struct. Anomalous cases are reordered appropriately in order NIT: Double-space before "Anomalous". > to make the code more consistent, although there should be no > functional difference since these cases are protected by > local_bh_disable() anyway. > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Julien Grall <julien.grall@arm.com> Cheers,
On Thu, Feb 21, 2019 at 12:39:39PM +0000, Julien Grall wrote: > Hi Dave, > > On 18/02/2019 19:52, Dave Martin wrote: > >This patch updates fpsimd_flush_task_state() to mirror the new > >semantics of fpsimd_flush_cpu_state() introduced by commit > >d8ad71fa38a9 ("arm64: fpsimd: Fix TIF_FOREIGN_FPSTATE after > >invalidating cpu regs"). Both functions now implicitly set > > NIT: Double-space before "Both" > > >TIF_FOREIGN_FPSTATE to indicate that the task's FPSIMD state is not > >loaded into the cpu. > > > >As a side-effect, fpsimd_flush_task_state() now sets > >TIF_FOREIGN_FPSTATE even for non-running tasks. In the case of > > NIT: Double sppace before "In". > > >non-running tasks this is not useful but also harmless, because the > >flag is live only while the corresponding task is running. This > >function is not called from fast paths, so special-casing this for > >the task == current case is not really worth it. > > > >Compiler barriers previously present in restore_sve_fpsimd_context() > >are pulled into fpsimd_flush_task_state() so that it can be safely > >called with preemption enabled if necessary. > > > >Explicit calls to set TIF_FOREIGN_FPSTATE that accompany > >fpsimd_flush_task_state() calls and are now redundant are removed > >as appropriate. > > > >fpsimd_flush_task_state() is used to get exclusive access to the > >representation of the task's state via task_struct, for the purpose > >of replacing the state. Thus, the call to this function should > > NIT: Double-space before "Thus". > > >happen before manipulating fpsimd_state or sve_state etc. in > >task_struct. Anomalous cases are reordered appropriately in order > > NIT: Double-space before "Anomalous". A habit rather than a mistake [1], and I don't propose to change it ;) > >to make the code more consistent, although there should be no > >functional difference since these cases are protected by > >local_bh_disable() anyway. > > > >Signed-off-by: Dave Martin <Dave.Martin@arm.com> > >Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > > Reviewed-by: Julien Grall <julien.grall@arm.com> Thanks for the review --Dave [1] https://en.wikipedia.org/wiki/Sentence_spacing Interestingly around 11% of commits in mainline appear to follow the two-space convention for their commit messages. I won't speculate as to why... $ git log --oneline --grep='[a-z]\. [A-Z]' | wc -l 94638 $ git log --oneline | wc -l 812542
On 26/02/2019 12:06, Dave Martin wrote: > On Thu, Feb 21, 2019 at 12:39:39PM +0000, Julien Grall wrote: >> Hi Dave, >> >> On 18/02/2019 19:52, Dave Martin wrote: >>> This patch updates fpsimd_flush_task_state() to mirror the new >>> semantics of fpsimd_flush_cpu_state() introduced by commit >>> d8ad71fa38a9 ("arm64: fpsimd: Fix TIF_FOREIGN_FPSTATE after >>> invalidating cpu regs"). Both functions now implicitly set >> >> NIT: Double-space before "Both" >> >>> TIF_FOREIGN_FPSTATE to indicate that the task's FPSIMD state is not >>> loaded into the cpu. >>> >>> As a side-effect, fpsimd_flush_task_state() now sets >>> TIF_FOREIGN_FPSTATE even for non-running tasks. In the case of >> >> NIT: Double sppace before "In". >> >>> non-running tasks this is not useful but also harmless, because the >>> flag is live only while the corresponding task is running. This >>> function is not called from fast paths, so special-casing this for >>> the task == current case is not really worth it. >>> >>> Compiler barriers previously present in restore_sve_fpsimd_context() >>> are pulled into fpsimd_flush_task_state() so that it can be safely >>> called with preemption enabled if necessary. >>> >>> Explicit calls to set TIF_FOREIGN_FPSTATE that accompany >>> fpsimd_flush_task_state() calls and are now redundant are removed >>> as appropriate. >>> >>> fpsimd_flush_task_state() is used to get exclusive access to the >>> representation of the task's state via task_struct, for the purpose >>> of replacing the state. Thus, the call to this function should >> >> NIT: Double-space before "Thus". >> >>> happen before manipulating fpsimd_state or sve_state etc. in >>> task_struct. Anomalous cases are reordered appropriately in order >> >> NIT: Double-space before "Anomalous". > > A habit rather than a mistake [1], and I don't propose to change it ;) I wasn't aware of this. Thank you for the pointer! Please ignore the comments on it :). Cheers,
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index 5ebe73b..62c37f0 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -550,7 +550,6 @@ int sve_set_vector_length(struct task_struct *task, local_bh_disable(); fpsimd_save(); - set_thread_flag(TIF_FOREIGN_FPSTATE); } fpsimd_flush_task_state(task); @@ -816,12 +815,11 @@ asmlinkage void do_sve_acc(unsigned int esr, struct pt_regs *regs) local_bh_disable(); fpsimd_save(); - fpsimd_to_sve(current); /* Force ret_to_user to reload the registers: */ fpsimd_flush_task_state(current); - set_thread_flag(TIF_FOREIGN_FPSTATE); + fpsimd_to_sve(current); if (test_and_set_thread_flag(TIF_SVE)) WARN_ON(1); /* SVE access shouldn't have trapped */ @@ -894,9 +892,9 @@ void fpsimd_flush_thread(void) local_bh_disable(); + fpsimd_flush_task_state(current); memset(¤t->thread.uw.fpsimd_state, 0, sizeof(current->thread.uw.fpsimd_state)); - fpsimd_flush_task_state(current); if (system_supports_sve()) { clear_thread_flag(TIF_SVE); @@ -933,8 +931,6 @@ void fpsimd_flush_thread(void) current->thread.sve_vl_onexec = 0; } - set_thread_flag(TIF_FOREIGN_FPSTATE); - local_bh_enable(); } @@ -1043,12 +1039,29 @@ void fpsimd_update_current_state(struct user_fpsimd_state const *state) /* * Invalidate live CPU copies of task t's FPSIMD state + * + * This function may be called with preemption enabled. The barrier() + * ensures that the assignment to fpsimd_cpu is visible to any + * preemption/softirq that could race with set_tsk_thread_flag(), so + * that TIF_FOREIGN_FPSTATE cannot be spuriously re-cleared. + * + * The final barrier ensures that TIF_FOREIGN_FPSTATE is seen set by any + * subsequent code. */ void fpsimd_flush_task_state(struct task_struct *t) { t->thread.fpsimd_cpu = NR_CPUS; + + barrier(); + set_tsk_thread_flag(t, TIF_FOREIGN_FPSTATE); + + barrier(); } +/* + * Invalidate any task's FPSIMD state that is present on this cpu. + * This function must be called with softirqs disabled. + */ void fpsimd_flush_cpu_state(void) { __this_cpu_write(fpsimd_last_state.st, NULL); diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index 867a7ce..a9b0485 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -296,11 +296,6 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user) */ fpsimd_flush_task_state(current); - barrier(); - /* From now, fpsimd_thread_switch() won't clear TIF_FOREIGN_FPSTATE */ - - set_thread_flag(TIF_FOREIGN_FPSTATE); - barrier(); /* From now, fpsimd_thread_switch() won't touch thread.sve_state */ sve_alloc(current);
This patch updates fpsimd_flush_task_state() to mirror the new semantics of fpsimd_flush_cpu_state() introduced by commit d8ad71fa38a9 ("arm64: fpsimd: Fix TIF_FOREIGN_FPSTATE after invalidating cpu regs"). Both functions now implicitly set TIF_FOREIGN_FPSTATE to indicate that the task's FPSIMD state is not loaded into the cpu. As a side-effect, fpsimd_flush_task_state() now sets TIF_FOREIGN_FPSTATE even for non-running tasks. In the case of non-running tasks this is not useful but also harmless, because the flag is live only while the corresponding task is running. This function is not called from fast paths, so special-casing this for the task == current case is not really worth it. Compiler barriers previously present in restore_sve_fpsimd_context() are pulled into fpsimd_flush_task_state() so that it can be safely called with preemption enabled if necessary. Explicit calls to set TIF_FOREIGN_FPSTATE that accompany fpsimd_flush_task_state() calls and are now redundant are removed as appropriate. fpsimd_flush_task_state() is used to get exclusive access to the representation of the task's state via task_struct, for the purpose of replacing the state. Thus, the call to this function should happen before manipulating fpsimd_state or sve_state etc. in task_struct. Anomalous cases are reordered appropriately in order to make the code more consistent, although there should be no functional difference since these cases are protected by local_bh_disable() anyway. Signed-off-by: Dave Martin <Dave.Martin@arm.com> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> --- arch/arm64/kernel/fpsimd.c | 25 +++++++++++++++++++------ arch/arm64/kernel/signal.c | 5 ----- 2 files changed, 19 insertions(+), 11 deletions(-)