| Message ID | 20210930181144.10029-9-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64/sme: Initial support for the Scalable Matrix Extension | expand |
On Thu, 30 Sep 2021 19:11:14 +0100 Mark Brown <broonie@kernel.org> wrote: > As for SVE we will track a per task SME vector length for tasks. Convert > the existing storage for the vector length into an array and update > fpsimd_flush_task() to initialise this in a function. > > Signed-off-by: Mark Brown <broonie@kernel.org> I'm clearly having a trivial comment day. Given reduction in indenting it would be nice perhaps to reformat comments to take that into account. I'm also unconvinced the trivial wrappers are worthwhile. (maybe you drop those later?) Jonathan > --- > arch/arm64/include/asm/processor.h | 44 +++++++++++-- > arch/arm64/include/asm/thread_info.h | 2 +- > arch/arm64/kernel/fpsimd.c | 97 ++++++++++++++++------------ > 3 files changed, 95 insertions(+), 48 deletions(-) > > diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h > index fb0608fe9ded..9b854e8196df 100644 > --- a/arch/arm64/include/asm/processor.h > +++ b/arch/arm64/include/asm/processor.h > @@ -152,8 +152,8 @@ struct thread_struct { > > unsigned int fpsimd_cpu; > void *sve_state; /* SVE registers, if any */ > - unsigned int sve_vl; /* SVE vector length */ > - unsigned int sve_vl_onexec; /* SVE vl after next exec */ > + unsigned int vl[ARM64_VEC_MAX]; /* vector length */ > + unsigned int vl_onexec[ARM64_VEC_MAX]; /* vl after next exec */ > unsigned long fault_address; /* fault info */ > unsigned long fault_code; /* ESR_EL1 value */ > struct debug_info debug; /* debugging */ > @@ -169,15 +169,45 @@ struct thread_struct { > u64 sctlr_user; > }; > > +static inline unsigned int thread_get_vl(struct thread_struct *thread, > + enum vec_type type) > +{ > + return thread->vl[type]; > +} > + > static inline unsigned int thread_get_sve_vl(struct thread_struct *thread) > { > - return thread->sve_vl; > + return thread_get_vl(thread, ARM64_VEC_SVE); > +} > + > +unsigned int task_get_vl(const struct task_struct *task, enum vec_type type); > +void task_set_vl(struct task_struct *task, enum vec_type type, > + unsigned long vl); > +void task_set_vl_onexec(struct task_struct *task, enum vec_type type, > + unsigned long vl); > +unsigned int task_get_vl_onexec(const struct task_struct *task, > + enum vec_type type); > + > +static inline unsigned int task_get_sve_vl(const struct task_struct *task) > +{ > + return task_get_vl(task, ARM64_VEC_SVE); > } > > -unsigned int task_get_sve_vl(const struct task_struct *task); > -void task_set_sve_vl(struct task_struct *task, unsigned long vl); > -unsigned int task_get_sve_vl_onexec(const struct task_struct *task); > -void task_set_sve_vl_onexec(struct task_struct *task, unsigned long vl); > +static inline void task_set_sve_vl(struct task_struct *task, unsigned long vl) > +{ > + task_set_vl(task, ARM64_VEC_SVE, vl); Are these really worth having? They make the refactor simpler perhaps, but don't add any meaning in of themselves over just having the calls inline. > +} > + > +static inline unsigned int task_get_sve_vl_onexec(const struct task_struct *task) > +{ > + return task_get_vl_onexec(task, ARM64_VEC_SVE); > +} > + > +static inline void task_set_sve_vl_onexec(struct task_struct *task, > + unsigned long vl) > +{ > + task_set_vl_onexec(task, ARM64_VEC_SVE, vl); > +} > > #define SCTLR_USER_MASK \ > (SCTLR_ELx_ENIA | SCTLR_ELx_ENIB | SCTLR_ELx_ENDA | SCTLR_ELx_ENDB | \ > diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h > index 6623c99f0984..d5c8ac81ce11 100644 > --- a/arch/arm64/include/asm/thread_info.h > +++ b/arch/arm64/include/asm/thread_info.h > @@ -78,7 +78,7 @@ int arch_dup_task_struct(struct task_struct *dst, > #define TIF_SINGLESTEP 21 > #define TIF_32BIT 22 /* 32bit process */ > #define TIF_SVE 23 /* Scalable Vector Extension in use */ > -#define TIF_SVE_VL_INHERIT 24 /* Inherit sve_vl_onexec across exec */ > +#define TIF_SVE_VL_INHERIT 24 /* Inherit SVE vl_onexec across exec */ > #define TIF_SSBD 25 /* Wants SSB mitigation */ > #define TIF_TAGGED_ADDR 26 /* Allow tagged user addresses */ > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index 44bb3203c9d1..814080209093 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -133,6 +133,17 @@ __ro_after_init struct vl_info vl_info[ARM64_VEC_MAX] = { > #endif > }; > > +static unsigned int vec_vl_inherit_flag(enum vec_type type) > +{ > + switch (type) { > + case ARM64_VEC_SVE: > + return TIF_SVE_VL_INHERIT; > + default: > + WARN_ON_ONCE(1); > + return 0; > + } > +} > + > struct vl_config { > int __default_vl; /* Default VL for tasks */ > }; > @@ -239,24 +250,27 @@ static void sve_free(struct task_struct *task) > __sve_free(task); > } > > -unsigned int task_get_sve_vl(const struct task_struct *task) > +unsigned int task_get_vl(const struct task_struct *task, enum vec_type type) > { > - return task->thread.sve_vl; > + return task->thread.vl[type]; > } > > -void task_set_sve_vl(struct task_struct *task, unsigned long vl) > +void task_set_vl(struct task_struct *task, enum vec_type type, > + unsigned long vl) > { > - task->thread.sve_vl = vl; > + task->thread.vl[type] = vl; > } > > -unsigned int task_get_sve_vl_onexec(const struct task_struct *task) > +unsigned int task_get_vl_onexec(const struct task_struct *task, > + enum vec_type type) > { > - return task->thread.sve_vl_onexec; > + return task->thread.vl_onexec[type]; > } > > -void task_set_sve_vl_onexec(struct task_struct *task, unsigned long vl) > +void task_set_vl_onexec(struct task_struct *task, enum vec_type type, > + unsigned long vl) > { > - task->thread.sve_vl_onexec = vl; > + task->thread.vl_onexec[type] = vl; > } > > /* > @@ -1074,10 +1088,43 @@ void fpsimd_thread_switch(struct task_struct *next) > __put_cpu_fpsimd_context(); > } > > -void fpsimd_flush_thread(void) > +static void fpsimd_flush_thread_vl(enum vec_type type) > { > int vl, supported_vl; > > + /* > + * Reset the task vector length as required. This is where we > + * ensure that all user tasks have a valid vector length Nice to rewrap this to 80 chars whilst you are here? > + * configured: no kernel task can become a user task without > + * an exec and hence a call to this function. By the time the > + * first call to this function is made, all early hardware > + * probing is complete, so __sve_default_vl should be valid. > + * If a bug causes this to go wrong, we make some noise and > + * try to fudge thread.sve_vl to a safe value here. > + */ > + vl = task_get_vl_onexec(current, type); > + if (!vl) > + vl = get_default_vl(type); > + > + if (WARN_ON(!sve_vl_valid(vl))) > + vl = SVE_VL_MIN; > + > + supported_vl = find_supported_vector_length(type, vl); > + if (WARN_ON(supported_vl != vl)) > + vl = supported_vl; > + > + task_set_vl(current, type, vl); > + > + /* > + * If the task is not set to inherit, ensure that the vector > + * length will be reset by a subsequent exec: > + */ > + if (!test_thread_flag(vec_vl_inherit_flag(type))) > + task_set_vl_onexec(current, type, 0); > +} > + > +void fpsimd_flush_thread(void) > +{ > if (!system_supports_fpsimd()) > return; > > @@ -1090,37 +1137,7 @@ void fpsimd_flush_thread(void) > if (system_supports_sve()) { > clear_thread_flag(TIF_SVE); > sve_free(current); > - > - /* > - * Reset the task vector length as required. > - * This is where we ensure that all user tasks have a valid > - * vector length configured: no kernel task can become a user > - * task without an exec and hence a call to this function. > - * By the time the first call to this function is made, all > - * early hardware probing is complete, so __sve_default_vl > - * should be valid. > - * If a bug causes this to go wrong, we make some noise and > - * try to fudge thread.sve_vl to a safe value here. > - */ > - vl = task_get_sve_vl_onexec(current); > - if (!vl) > - vl = get_sve_default_vl(); > - > - if (WARN_ON(!sve_vl_valid(vl))) > - vl = SVE_VL_MIN; > - > - supported_vl = find_supported_vector_length(ARM64_VEC_SVE, vl); > - if (WARN_ON(supported_vl != vl)) > - vl = supported_vl; > - > - task_set_sve_vl(current, vl); > - > - /* > - * If the task is not set to inherit, ensure that the vector > - * length will be reset by a subsequent exec: > - */ > - if (!test_thread_flag(TIF_SVE_VL_INHERIT)) > - task_set_sve_vl_onexec(current, 0); > + fpsimd_flush_thread_vl(ARM64_VEC_SVE); > } > > put_cpu_fpsimd_context();
On Mon, Oct 11, 2021 at 11:20:57AM +0100, Jonathan Cameron wrote: > Mark Brown <broonie@kernel.org> wrote: > > As for SVE we will track a per task SME vector length for tasks. Convert > > the existing storage for the vector length into an array and update > > fpsimd_flush_task() to initialise this in a function. > I'm clearly having a trivial comment day. Given reduction in indenting > it would be nice perhaps to reformat comments to take that into account. Again I'll have done this to make it clearer that things are just being moved about - as a reviewer I do find things like that very helpful. > I'm also unconvinced the trivial wrappers are worthwhile. (maybe you drop > those later?) They do hide what we're doing from the rest of the series which makes the whole thing a lot easier to work with, especially if we change what data structures we use or there's some debate as to the names of the constants. A bunch of them directly map onto existing trivial wrappers too, there's been some stylistic preference for that. If people want to remove the wrappers I'd propose leaving them for now then adding a patch afterwards which removes them, or at least waiting until we've got very firm agreement from Catalin and Will on the data structures and constants.
On Mon, 11 Oct 2021 14:14:16 +0100 Mark Brown <broonie@kernel.org> wrote: > On Mon, Oct 11, 2021 at 11:20:57AM +0100, Jonathan Cameron wrote: > > Mark Brown <broonie@kernel.org> wrote: > > > > As for SVE we will track a per task SME vector length for tasks. Convert > > > the existing storage for the vector length into an array and update > > > fpsimd_flush_task() to initialise this in a function. > > > I'm clearly having a trivial comment day. Given reduction in indenting > > it would be nice perhaps to reformat comments to take that into account. > > Again I'll have done this to make it clearer that things are just being > moved about - as a reviewer I do find things like that very helpful. > > > I'm also unconvinced the trivial wrappers are worthwhile. (maybe you drop > > those later?) > > They do hide what we're doing from the rest of the series which makes > the whole thing a lot easier to work with, especially if we change what > data structures we use or there's some debate as to the names of the > constants. A bunch of them directly map onto existing trivial wrappers > too, there's been some stylistic preference for that. > > If people want to remove the wrappers I'd propose leaving them for now > then adding a patch afterwards which removes them, or at least waiting > until we've got very firm agreement from Catalin and Will on the data > structures and constants. > Makes sense. J
diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h index fb0608fe9ded..9b854e8196df 100644 --- a/arch/arm64/include/asm/processor.h +++ b/arch/arm64/include/asm/processor.h @@ -152,8 +152,8 @@ struct thread_struct { unsigned int fpsimd_cpu; void *sve_state; /* SVE registers, if any */ - unsigned int sve_vl; /* SVE vector length */ - unsigned int sve_vl_onexec; /* SVE vl after next exec */ + unsigned int vl[ARM64_VEC_MAX]; /* vector length */ + unsigned int vl_onexec[ARM64_VEC_MAX]; /* vl after next exec */ unsigned long fault_address; /* fault info */ unsigned long fault_code; /* ESR_EL1 value */ struct debug_info debug; /* debugging */ @@ -169,15 +169,45 @@ struct thread_struct { u64 sctlr_user; }; +static inline unsigned int thread_get_vl(struct thread_struct *thread, + enum vec_type type) +{ + return thread->vl[type]; +} + static inline unsigned int thread_get_sve_vl(struct thread_struct *thread) { - return thread->sve_vl; + return thread_get_vl(thread, ARM64_VEC_SVE); +} + +unsigned int task_get_vl(const struct task_struct *task, enum vec_type type); +void task_set_vl(struct task_struct *task, enum vec_type type, + unsigned long vl); +void task_set_vl_onexec(struct task_struct *task, enum vec_type type, + unsigned long vl); +unsigned int task_get_vl_onexec(const struct task_struct *task, + enum vec_type type); + +static inline unsigned int task_get_sve_vl(const struct task_struct *task) +{ + return task_get_vl(task, ARM64_VEC_SVE); } -unsigned int task_get_sve_vl(const struct task_struct *task); -void task_set_sve_vl(struct task_struct *task, unsigned long vl); -unsigned int task_get_sve_vl_onexec(const struct task_struct *task); -void task_set_sve_vl_onexec(struct task_struct *task, unsigned long vl); +static inline void task_set_sve_vl(struct task_struct *task, unsigned long vl) +{ + task_set_vl(task, ARM64_VEC_SVE, vl); +} + +static inline unsigned int task_get_sve_vl_onexec(const struct task_struct *task) +{ + return task_get_vl_onexec(task, ARM64_VEC_SVE); +} + +static inline void task_set_sve_vl_onexec(struct task_struct *task, + unsigned long vl) +{ + task_set_vl_onexec(task, ARM64_VEC_SVE, vl); +} #define SCTLR_USER_MASK \ (SCTLR_ELx_ENIA | SCTLR_ELx_ENIB | SCTLR_ELx_ENDA | SCTLR_ELx_ENDB | \ diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h index 6623c99f0984..d5c8ac81ce11 100644 --- a/arch/arm64/include/asm/thread_info.h +++ b/arch/arm64/include/asm/thread_info.h @@ -78,7 +78,7 @@ int arch_dup_task_struct(struct task_struct *dst, #define TIF_SINGLESTEP 21 #define TIF_32BIT 22 /* 32bit process */ #define TIF_SVE 23 /* Scalable Vector Extension in use */ -#define TIF_SVE_VL_INHERIT 24 /* Inherit sve_vl_onexec across exec */ +#define TIF_SVE_VL_INHERIT 24 /* Inherit SVE vl_onexec across exec */ #define TIF_SSBD 25 /* Wants SSB mitigation */ #define TIF_TAGGED_ADDR 26 /* Allow tagged user addresses */ diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index 44bb3203c9d1..814080209093 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -133,6 +133,17 @@ __ro_after_init struct vl_info vl_info[ARM64_VEC_MAX] = { #endif }; +static unsigned int vec_vl_inherit_flag(enum vec_type type) +{ + switch (type) { + case ARM64_VEC_SVE: + return TIF_SVE_VL_INHERIT; + default: + WARN_ON_ONCE(1); + return 0; + } +} + struct vl_config { int __default_vl; /* Default VL for tasks */ }; @@ -239,24 +250,27 @@ static void sve_free(struct task_struct *task) __sve_free(task); } -unsigned int task_get_sve_vl(const struct task_struct *task) +unsigned int task_get_vl(const struct task_struct *task, enum vec_type type) { - return task->thread.sve_vl; + return task->thread.vl[type]; } -void task_set_sve_vl(struct task_struct *task, unsigned long vl) +void task_set_vl(struct task_struct *task, enum vec_type type, + unsigned long vl) { - task->thread.sve_vl = vl; + task->thread.vl[type] = vl; } -unsigned int task_get_sve_vl_onexec(const struct task_struct *task) +unsigned int task_get_vl_onexec(const struct task_struct *task, + enum vec_type type) { - return task->thread.sve_vl_onexec; + return task->thread.vl_onexec[type]; } -void task_set_sve_vl_onexec(struct task_struct *task, unsigned long vl) +void task_set_vl_onexec(struct task_struct *task, enum vec_type type, + unsigned long vl) { - task->thread.sve_vl_onexec = vl; + task->thread.vl_onexec[type] = vl; } /* @@ -1074,10 +1088,43 @@ void fpsimd_thread_switch(struct task_struct *next) __put_cpu_fpsimd_context(); } -void fpsimd_flush_thread(void) +static void fpsimd_flush_thread_vl(enum vec_type type) { int vl, supported_vl; + /* + * Reset the task vector length as required. This is where we + * ensure that all user tasks have a valid vector length + * configured: no kernel task can become a user task without + * an exec and hence a call to this function. By the time the + * first call to this function is made, all early hardware + * probing is complete, so __sve_default_vl should be valid. + * If a bug causes this to go wrong, we make some noise and + * try to fudge thread.sve_vl to a safe value here. + */ + vl = task_get_vl_onexec(current, type); + if (!vl) + vl = get_default_vl(type); + + if (WARN_ON(!sve_vl_valid(vl))) + vl = SVE_VL_MIN; + + supported_vl = find_supported_vector_length(type, vl); + if (WARN_ON(supported_vl != vl)) + vl = supported_vl; + + task_set_vl(current, type, vl); + + /* + * If the task is not set to inherit, ensure that the vector + * length will be reset by a subsequent exec: + */ + if (!test_thread_flag(vec_vl_inherit_flag(type))) + task_set_vl_onexec(current, type, 0); +} + +void fpsimd_flush_thread(void) +{ if (!system_supports_fpsimd()) return; @@ -1090,37 +1137,7 @@ void fpsimd_flush_thread(void) if (system_supports_sve()) { clear_thread_flag(TIF_SVE); sve_free(current); - - /* - * Reset the task vector length as required. - * This is where we ensure that all user tasks have a valid - * vector length configured: no kernel task can become a user - * task without an exec and hence a call to this function. - * By the time the first call to this function is made, all - * early hardware probing is complete, so __sve_default_vl - * should be valid. - * If a bug causes this to go wrong, we make some noise and - * try to fudge thread.sve_vl to a safe value here. - */ - vl = task_get_sve_vl_onexec(current); - if (!vl) - vl = get_sve_default_vl(); - - if (WARN_ON(!sve_vl_valid(vl))) - vl = SVE_VL_MIN; - - supported_vl = find_supported_vector_length(ARM64_VEC_SVE, vl); - if (WARN_ON(supported_vl != vl)) - vl = supported_vl; - - task_set_sve_vl(current, vl); - - /* - * If the task is not set to inherit, ensure that the vector - * length will be reset by a subsequent exec: - */ - if (!test_thread_flag(TIF_SVE_VL_INHERIT)) - task_set_sve_vl_onexec(current, 0); + fpsimd_flush_thread_vl(ARM64_VEC_SVE); } put_cpu_fpsimd_context();
As for SVE we will track a per task SME vector length for tasks. Convert the existing storage for the vector length into an array and update fpsimd_flush_task() to initialise this in a function. Signed-off-by: Mark Brown <broonie@kernel.org> --- arch/arm64/include/asm/processor.h | 44 +++++++++++-- arch/arm64/include/asm/thread_info.h | 2 +- arch/arm64/kernel/fpsimd.c | 97 ++++++++++++++++------------ 3 files changed, 95 insertions(+), 48 deletions(-)