| Message ID | 20180514094640.27569-7-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 14, 2018 at 10:46:28AM +0100, Mark Rutland wrote: > In subsequent patches, we'll want to make use of sve_user_enable() and > sve_user_disable() outside of kernel/fpsimd.c. Let's move these to > <asm/fpsimd.h> where we can make use of them. > > To avoid ifdeffery in sequences like: > > if (system_supports_sve() && some_condition > sve_user_disable(); > > ... empty stubs are provided when support for SVE is not enabled. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Dave Martin <dave.martin@arm.com> > Cc: Will Deacon <will.deacon@arm.com> > --- > arch/arm64/include/asm/fpsimd.h | 17 ++++++++++++++++- > arch/arm64/kernel/fpsimd.c | 11 ----------- > 2 files changed, 16 insertions(+), 12 deletions(-) > > diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h > index aa7162ae93e3..7377d7593c06 100644 > --- a/arch/arm64/include/asm/fpsimd.h > +++ b/arch/arm64/include/asm/fpsimd.h > @@ -16,11 +16,13 @@ > #ifndef __ASM_FP_H > #define __ASM_FP_H > > -#include <asm/ptrace.h> > #include <asm/errno.h> > +#include <asm/ptrace.h> > +#include <asm/sysreg.h> > > #ifndef __ASSEMBLY__ > > +#include <linux/build_bug.h> > #include <linux/cache.h> > #include <linux/init.h> > #include <linux/stddef.h> > @@ -81,6 +83,16 @@ extern int sve_set_vector_length(struct task_struct *task, > extern int sve_set_current_vl(unsigned long arg); > extern int sve_get_current_vl(void); > > +static inline void sve_user_disable(void) > +{ > + sysreg_clear_set(cpacr_el1, CPACR_EL1_ZEN_EL0EN, 0); > +} > + > +static inline void sve_user_enable(void) > +{ > + sysreg_clear_set(cpacr_el1, 0, CPACR_EL1_ZEN_EL0EN); > +} > + > /* > * Probing and setup functions. > * Calls to these functions must be serialised with one another. > @@ -107,6 +119,9 @@ static inline int sve_get_current_vl(void) > return -EINVAL; > } > > +static inline void sve_user_disable(void) { } > +static inline void sve_user_enable(void) { } > + Alternatively, just move the full definitions outside the #ifdef CONFIG_ARM64_SVE. All calls to these should be shadowed by an if (system_supports_sve()) in any case, and setting/clearing ZEN_EL0EN in the CPACR_EL1 ought to be harmless now that the meaning of these bits architecturally committed. Ideally we would have a BUG_ON(!system_supports_sve()) in those functions, but we won't won't to pay the cost in a production kernel. > static inline void sve_init_vq_map(void) { } > static inline void sve_update_vq_map(void) { } > static inline int sve_verify_vq_map(void) { return 0; } > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index 088940387a4d..79a81c7d85c6 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -159,7 +159,6 @@ static void sve_free(struct task_struct *task) > __sve_free(task); > } > > - Hmmm, Ack. Check for conflicts with the KVM FPSIMD rework [1] (though trivial). [...] Cheers ---Dave [1] [PATCH v7 10/16] arm64/sve: Switch sve_pffr() argument from task to thread http://lists.infradead.org/pipermail/linux-arm-kernel/2018-May/576601.html [PATCH v7 11/16] arm64/sve: Move sve_pffr() to fpsimd.h and make inline http://lists.infradead.org/pipermail/linux-arm-kernel/2018-May/576606.html
On Mon, May 14, 2018 at 12:06:50PM +0100, Dave Martin wrote: > On Mon, May 14, 2018 at 10:46:28AM +0100, Mark Rutland wrote: > > In subsequent patches, we'll want to make use of sve_user_enable() and > > sve_user_disable() outside of kernel/fpsimd.c. Let's move these to > > <asm/fpsimd.h> where we can make use of them. > > > > To avoid ifdeffery in sequences like: > > > > if (system_supports_sve() && some_condition > > sve_user_disable(); > > > > ... empty stubs are provided when support for SVE is not enabled. > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Dave Martin <dave.martin@arm.com> > > Cc: Will Deacon <will.deacon@arm.com> > > --- > > arch/arm64/include/asm/fpsimd.h | 17 ++++++++++++++++- > > arch/arm64/kernel/fpsimd.c | 11 ----------- > > 2 files changed, 16 insertions(+), 12 deletions(-) > > > > diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h > > index aa7162ae93e3..7377d7593c06 100644 > > --- a/arch/arm64/include/asm/fpsimd.h > > +++ b/arch/arm64/include/asm/fpsimd.h > > @@ -16,11 +16,13 @@ > > #ifndef __ASM_FP_H > > #define __ASM_FP_H > > > > -#include <asm/ptrace.h> > > #include <asm/errno.h> > > +#include <asm/ptrace.h> > > +#include <asm/sysreg.h> > > > > #ifndef __ASSEMBLY__ > > > > +#include <linux/build_bug.h> > > #include <linux/cache.h> > > #include <linux/init.h> > > #include <linux/stddef.h> > > @@ -81,6 +83,16 @@ extern int sve_set_vector_length(struct task_struct *task, > > extern int sve_set_current_vl(unsigned long arg); > > extern int sve_get_current_vl(void); > > > > +static inline void sve_user_disable(void) > > +{ > > + sysreg_clear_set(cpacr_el1, CPACR_EL1_ZEN_EL0EN, 0); > > +} > > + > > +static inline void sve_user_enable(void) > > +{ > > + sysreg_clear_set(cpacr_el1, 0, CPACR_EL1_ZEN_EL0EN); > > +} > > + > > /* > > * Probing and setup functions. > > * Calls to these functions must be serialised with one another. > > @@ -107,6 +119,9 @@ static inline int sve_get_current_vl(void) > > return -EINVAL; > > } > > > > +static inline void sve_user_disable(void) { } > > +static inline void sve_user_enable(void) { } > > + > > Alternatively, just move the full definitions outside the #ifdef > CONFIG_ARM64_SVE. Can do, though I was trying to keep the exsting pattern with empty inlines for the !CONFIG_ARM64_SVE case. > > All calls to these should be shadowed by an if > (system_supports_sve()) in any case, and setting/clearing ZEN_EL0EN > in the CPACR_EL1 ought to be harmless now that the meaning of these > bits architecturally committed. > > Ideally we would have a BUG_ON(!system_supports_sve()) in those > functions, but we won't won't to pay the cost in a production kernel. Earlier I'd put BUILD_BUG() in the body for the !CONFIG_ARM64_SVE case, to catch that kind of thing -- I could restore that. > > static inline void sve_init_vq_map(void) { } > > static inline void sve_update_vq_map(void) { } > > static inline int sve_verify_vq_map(void) { return 0; } > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > > index 088940387a4d..79a81c7d85c6 100644 > > --- a/arch/arm64/kernel/fpsimd.c > > +++ b/arch/arm64/kernel/fpsimd.c > > @@ -159,7 +159,6 @@ static void sve_free(struct task_struct *task) > > __sve_free(task); > > } > > > > - > > Hmmm, Ack. Check for conflicts with the KVM FPSIMD rework [1] (though > trivial). I'll assume that Ack stands regardless. :) Thanks, Mark.
On Tue, May 15, 2018 at 11:39:36AM +0100, Mark Rutland wrote: > On Mon, May 14, 2018 at 12:06:50PM +0100, Dave Martin wrote: > > On Mon, May 14, 2018 at 10:46:28AM +0100, Mark Rutland wrote: > > > In subsequent patches, we'll want to make use of sve_user_enable() and > > > sve_user_disable() outside of kernel/fpsimd.c. Let's move these to > > > <asm/fpsimd.h> where we can make use of them. > > > > > > To avoid ifdeffery in sequences like: > > > > > > if (system_supports_sve() && some_condition > > > sve_user_disable(); > > > > > > ... empty stubs are provided when support for SVE is not enabled. > > > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > > Cc: Dave Martin <dave.martin@arm.com> > > > Cc: Will Deacon <will.deacon@arm.com> > > > --- > > > arch/arm64/include/asm/fpsimd.h | 17 ++++++++++++++++- > > > arch/arm64/kernel/fpsimd.c | 11 ----------- > > > 2 files changed, 16 insertions(+), 12 deletions(-) > > > > > > diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h > > > index aa7162ae93e3..7377d7593c06 100644 > > > --- a/arch/arm64/include/asm/fpsimd.h > > > +++ b/arch/arm64/include/asm/fpsimd.h > > > @@ -16,11 +16,13 @@ > > > #ifndef __ASM_FP_H > > > #define __ASM_FP_H > > > > > > -#include <asm/ptrace.h> > > > #include <asm/errno.h> > > > +#include <asm/ptrace.h> > > > +#include <asm/sysreg.h> > > > > > > #ifndef __ASSEMBLY__ > > > > > > +#include <linux/build_bug.h> > > > #include <linux/cache.h> > > > #include <linux/init.h> > > > #include <linux/stddef.h> > > > @@ -81,6 +83,16 @@ extern int sve_set_vector_length(struct task_struct *task, > > > extern int sve_set_current_vl(unsigned long arg); > > > extern int sve_get_current_vl(void); > > > > > > +static inline void sve_user_disable(void) > > > +{ > > > + sysreg_clear_set(cpacr_el1, CPACR_EL1_ZEN_EL0EN, 0); > > > +} > > > + > > > +static inline void sve_user_enable(void) > > > +{ > > > + sysreg_clear_set(cpacr_el1, 0, CPACR_EL1_ZEN_EL0EN); > > > +} > > > + > > > /* > > > * Probing and setup functions. > > > * Calls to these functions must be serialised with one another. > > > @@ -107,6 +119,9 @@ static inline int sve_get_current_vl(void) > > > return -EINVAL; > > > } > > > > > > +static inline void sve_user_disable(void) { } > > > +static inline void sve_user_enable(void) { } > > > + > > > > Alternatively, just move the full definitions outside the #ifdef > > CONFIG_ARM64_SVE. > > Can do, though I was trying to keep the exsting pattern with empty > inlines for the !CONFIG_ARM64_SVE case. There isn't really a pattern. I tried to avoid dummy versions where there's no real reason to have them. I don't _think_ they're really needed here, unless I missed something. Did you get build failures without them? > > All calls to these should be shadowed by an if > > (system_supports_sve()) in any case, and setting/clearing ZEN_EL0EN > > in the CPACR_EL1 ought to be harmless now that the meaning of these > > bits architecturally committed. > > > > Ideally we would have a BUG_ON(!system_supports_sve()) in those > > functions, but we won't won't to pay the cost in a production kernel. > > Earlier I'd put BUILD_BUG() in the body for the !CONFIG_ARM64_SVE case, > to catch that kind of thing -- I could restore that. IIUC: if (0) { BUILD_BUG_ON(1); } can still fire, in which case it's futile checking for CONFIG_ARM64_SVE in most of the SVE support code. Anyway, CONFIG_ARM64_SVE doesn't capture the whole condition. > > > > static inline void sve_init_vq_map(void) { } > > > static inline void sve_update_vq_map(void) { } > > > static inline int sve_verify_vq_map(void) { return 0; } > > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > > > index 088940387a4d..79a81c7d85c6 100644 > > > --- a/arch/arm64/kernel/fpsimd.c > > > +++ b/arch/arm64/kernel/fpsimd.c > > > @@ -159,7 +159,6 @@ static void sve_free(struct task_struct *task) > > > __sve_free(task); > > > } > > > > > > - > > > > Hmmm, Ack. Check for conflicts with the KVM FPSIMD rework [1] (though > > trivial). > > I'll assume that Ack stands regardless. :) Actually, I was just commenting on the deleted blank line... not that there is any massive issue with this patch, though. Cheers ---Dave
On Tue, May 15, 2018 at 01:19:26PM +0100, Dave Martin wrote: > On Tue, May 15, 2018 at 11:39:36AM +0100, Mark Rutland wrote: > > On Mon, May 14, 2018 at 12:06:50PM +0100, Dave Martin wrote: > > > On Mon, May 14, 2018 at 10:46:28AM +0100, Mark Rutland wrote: > > > > +static inline void sve_user_disable(void) > > > > +{ > > > > + sysreg_clear_set(cpacr_el1, CPACR_EL1_ZEN_EL0EN, 0); > > > > +} > > > > + > > > > +static inline void sve_user_enable(void) > > > > +{ > > > > + sysreg_clear_set(cpacr_el1, 0, CPACR_EL1_ZEN_EL0EN); > > > > +} > > > > + > > > > /* > > > > * Probing and setup functions. > > > > * Calls to these functions must be serialised with one another. > > > > @@ -107,6 +119,9 @@ static inline int sve_get_current_vl(void) > > > > return -EINVAL; > > > > } > > > > > > > > +static inline void sve_user_disable(void) { } > > > > +static inline void sve_user_enable(void) { } > > > > + > > > > > > Alternatively, just move the full definitions outside the #ifdef > > > CONFIG_ARM64_SVE. > > > > Can do, though I was trying to keep the exsting pattern with empty > > inlines for the !CONFIG_ARM64_SVE case. > > There isn't really a pattern. I tried to avoid dummy versions where > there's no real reason to have them. I don't _think_ they're really > needed here, unless I missed something. Did you get build failures > without them? I need *some* definition so that sve_user_reset() in the syscall path can compile without ifdeferry. In sve_user_reset() I first check system_supports_sve(), which checks IS_ENABLED(CONFIG_ARM64_SVE), so the call should be optimised away when !CONFIG_ARM64_SVE, but I need a prototype regardless. > > > All calls to these should be shadowed by an if > > > (system_supports_sve()) in any case, and setting/clearing ZEN_EL0EN > > > in the CPACR_EL1 ought to be harmless now that the meaning of these > > > bits architecturally committed. > > > > > > Ideally we would have a BUG_ON(!system_supports_sve()) in those > > > functions, but we won't won't to pay the cost in a production kernel. > > > > Earlier I'd put BUILD_BUG() in the body for the !CONFIG_ARM64_SVE case, > > to catch that kind of thing -- I could restore that. > > IIUC: > > if (0) { > BUILD_BUG_ON(1); > } > > can still fire, in which case it's futile checking for CONFIG_ARM64_SVE > in most of the SVE support code. We already rely on BUILD_BUG() not firing in paths that can be trivially optimized away. e.g. in the cmpxchg code. > > > > static inline void sve_init_vq_map(void) { } > > > > static inline void sve_update_vq_map(void) { } > > > > static inline int sve_verify_vq_map(void) { return 0; } > > > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > > > > index 088940387a4d..79a81c7d85c6 100644 > > > > --- a/arch/arm64/kernel/fpsimd.c > > > > +++ b/arch/arm64/kernel/fpsimd.c > > > > @@ -159,7 +159,6 @@ static void sve_free(struct task_struct *task) > > > > __sve_free(task); > > > > } > > > > > > > > - > > > > > > Hmmm, Ack. Check for conflicts with the KVM FPSIMD rework [1] (though > > > trivial). > > > > I'll assume that Ack stands regardless. :) > > Actually, I was just commenting on the deleted blank line... Ah. I've restored that now. Thanks, Mark.
On Tue, May 15, 2018 at 05:33:52PM +0100, Mark Rutland wrote: > On Tue, May 15, 2018 at 01:19:26PM +0100, Dave Martin wrote: > > On Tue, May 15, 2018 at 11:39:36AM +0100, Mark Rutland wrote: > > > On Mon, May 14, 2018 at 12:06:50PM +0100, Dave Martin wrote: > > > > On Mon, May 14, 2018 at 10:46:28AM +0100, Mark Rutland wrote: [...] > > > > > @@ -107,6 +119,9 @@ static inline int sve_get_current_vl(void) > > > > > return -EINVAL; > > > > > } > > > > > > > > > > +static inline void sve_user_disable(void) { } > > > > > +static inline void sve_user_enable(void) { } > > > > > + > > > > > > > > Alternatively, just move the full definitions outside the #ifdef > > > > CONFIG_ARM64_SVE. > > > > > > Can do, though I was trying to keep the exsting pattern with empty > > > inlines for the !CONFIG_ARM64_SVE case. > > > > There isn't really a pattern. I tried to avoid dummy versions where > > there's no real reason to have them. I don't _think_ they're really > > needed here, unless I missed something. Did you get build failures > > without them? > > I need *some* definition so that sve_user_reset() in the syscall path > can compile without ifdeferry. > > In sve_user_reset() I first check system_supports_sve(), which checks > IS_ENABLED(CONFIG_ARM64_SVE), so the call should be optimised away when > !CONFIG_ARM64_SVE, but I need a prototype regardless. What I envisaged is that you move the real definitions outside the #ifdef so that they're defined unconditionally, and get rid of the dummies. Having a dummy definition of sve_user_enable() really feels like it's papering over something. How could it be appropriate to call this in a non-SVE enabled system? You _do_ guard the call to this already, so hiding the real function body for CONFIG_ARM64_SVE=n doesn't appear to achieve anything. Maybe I missed something somewhere. A dummy sve_user_disable() is a bit more reasonable though, but we want this to be a nop on non-SVE hardware even if CONFIG_ARM64_SVE=y. What about moving the system_supports_sve() check inside sve_user_disable()? [...] > > > Earlier I'd put BUILD_BUG() in the body for the !CONFIG_ARM64_SVE case, > > > to catch that kind of thing -- I could restore that. > > > > IIUC: > > > > if (0) { > > BUILD_BUG_ON(1); > > } > > > > can still fire, in which case it's futile checking for CONFIG_ARM64_SVE > > in most of the SVE support code. > > We already rely on BUILD_BUG() not firing in paths that can be trivially > optimized away. e.g. in the cmpxchg code. Fair enough. I had been unsure on this point. If you want to put a BUILD_BUG_ON(!IS_ENABLED(CONFIG_ARM64_SVE)) in sve_user_enable() and build with CONFIG_ARM64_SVE=n to check it works, then I'd be fine with that. This doesn't capture the runtime part of the condition, but it's better than nothing. [...] > > > > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > > > > > index 088940387a4d..79a81c7d85c6 100644 > > > > > --- a/arch/arm64/kernel/fpsimd.c > > > > > +++ b/arch/arm64/kernel/fpsimd.c > > > > > @@ -159,7 +159,6 @@ static void sve_free(struct task_struct *task) > > > > > __sve_free(task); > > > > > } > > > > > > > > > > - > > > > > > > > Hmmm, Ack. Check for conflicts with the KVM FPSIMD rework [1] (though > > > > trivial). > > > > > > I'll assume that Ack stands regardless. :) > > > > Actually, I was just commenting on the deleted blank line... > > Ah. I've restored that now. I meant Ack to the deletion. It looks like the blank line was spuriously introduced in the first place. But it doesn't hugely matter either way. Cheers ---Dave
On Wed, May 16, 2018 at 10:01:32AM +0100, Dave Martin wrote: > On Tue, May 15, 2018 at 05:33:52PM +0100, Mark Rutland wrote: > > On Tue, May 15, 2018 at 01:19:26PM +0100, Dave Martin wrote: > > > On Tue, May 15, 2018 at 11:39:36AM +0100, Mark Rutland wrote: > > > > Earlier I'd put BUILD_BUG() in the body for the !CONFIG_ARM64_SVE case, > > > > to catch that kind of thing -- I could restore that. > > > > > > IIUC: > > > > > > if (0) { > > > BUILD_BUG_ON(1); > > > } > > > > > > can still fire, in which case it's futile checking for CONFIG_ARM64_SVE > > > in most of the SVE support code. > > > > We already rely on BUILD_BUG() not firing in paths that can be trivially > > optimized away. e.g. in the cmpxchg code. > > Fair enough. I had been unsure on this point. > > If you want to put a BUILD_BUG_ON(!IS_ENABLED(CONFIG_ARM64_SVE)) in > sve_user_enable() and build with CONFIG_ARM64_SVE=n to check it works, > then I'd be fine with that. > > This doesn't capture the runtime part of the condition, but it's better > than nothing. For the moment, I've kept the stubs, but placed a BUILD_BUG() in each, as per the above rationale. We generally do that rather than than BUILD_BUG_ON(!IS_ENABLED(...)) in a common definition, and it's more in keeping with the other stubs in <asm/fpsimd.h>. > > > > > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > > > > > > index 088940387a4d..79a81c7d85c6 100644 > > > > > > --- a/arch/arm64/kernel/fpsimd.c > > > > > > +++ b/arch/arm64/kernel/fpsimd.c > > > > > > @@ -159,7 +159,6 @@ static void sve_free(struct task_struct *task) > > > > > > __sve_free(task); > > > > > > } > > > > > > > > > > > > - > > > > > > > > > > Hmmm, Ack. Check for conflicts with the KVM FPSIMD rework [1] (though > > > > > trivial). > > > > > > > > I'll assume that Ack stands regardless. :) > > > > > > Actually, I was just commenting on the deleted blank line... > > > > Ah. I've restored that now. > > I meant Ack to the deletion. It looks like the blank line was > spuriously introduced in the first place. But it doesn't hugely matter > either way. Ok. I've dropped that for now to minimize the potential for conflicts, but we can clean this up later. Thanks, Mark.
On Fri, Jun 01, 2018 at 11:29:13AM +0100, Mark Rutland wrote: > On Wed, May 16, 2018 at 10:01:32AM +0100, Dave Martin wrote: > > On Tue, May 15, 2018 at 05:33:52PM +0100, Mark Rutland wrote: > > > On Tue, May 15, 2018 at 01:19:26PM +0100, Dave Martin wrote: > > > > On Tue, May 15, 2018 at 11:39:36AM +0100, Mark Rutland wrote: > > > > > Earlier I'd put BUILD_BUG() in the body for the !CONFIG_ARM64_SVE case, > > > > > to catch that kind of thing -- I could restore that. > > > > > > > > IIUC: > > > > > > > > if (0) { > > > > BUILD_BUG_ON(1); > > > > } > > > > > > > > can still fire, in which case it's futile checking for CONFIG_ARM64_SVE > > > > in most of the SVE support code. > > > > > > We already rely on BUILD_BUG() not firing in paths that can be trivially > > > optimized away. e.g. in the cmpxchg code. > > > > Fair enough. I had been unsure on this point. > > > > If you want to put a BUILD_BUG_ON(!IS_ENABLED(CONFIG_ARM64_SVE)) in > > sve_user_enable() and build with CONFIG_ARM64_SVE=n to check it works, > > then I'd be fine with that. > > > > This doesn't capture the runtime part of the condition, but it's better > > than nothing. > > For the moment, I've kept the stubs, but placed a BUILD_BUG() in each, > as per the above rationale. > > We generally do that rather than than BUILD_BUG_ON(!IS_ENABLED(...)) in > a common definition, and it's more in keeping with the other stubs in > <asm/fpsimd.h>. OK, fine by me. > > > > > > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > > > > > > > index 088940387a4d..79a81c7d85c6 100644 > > > > > > > --- a/arch/arm64/kernel/fpsimd.c > > > > > > > +++ b/arch/arm64/kernel/fpsimd.c > > > > > > > @@ -159,7 +159,6 @@ static void sve_free(struct task_struct *task) > > > > > > > __sve_free(task); > > > > > > > } > > > > > > > > > > > > > > - > > > > > > > > > > > > Hmmm, Ack. Check for conflicts with the KVM FPSIMD rework [1] (though > > > > > > trivial). > > > > > > > > > > I'll assume that Ack stands regardless. :) > > > > > > > > Actually, I was just commenting on the deleted blank line... > > > > > > Ah. I've restored that now. > > > > I meant Ack to the deletion. It looks like the blank line was > > spuriously introduced in the first place. But it doesn't hugely matter > > either way. > > Ok. I've dropped that for now to minimize the potential for conflicts, > but we can clean this up later. No big deal either way. Cheers ---Dave
diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h index aa7162ae93e3..7377d7593c06 100644 --- a/arch/arm64/include/asm/fpsimd.h +++ b/arch/arm64/include/asm/fpsimd.h @@ -16,11 +16,13 @@ #ifndef __ASM_FP_H #define __ASM_FP_H -#include <asm/ptrace.h> #include <asm/errno.h> +#include <asm/ptrace.h> +#include <asm/sysreg.h> #ifndef __ASSEMBLY__ +#include <linux/build_bug.h> #include <linux/cache.h> #include <linux/init.h> #include <linux/stddef.h> @@ -81,6 +83,16 @@ extern int sve_set_vector_length(struct task_struct *task, extern int sve_set_current_vl(unsigned long arg); extern int sve_get_current_vl(void); +static inline void sve_user_disable(void) +{ + sysreg_clear_set(cpacr_el1, CPACR_EL1_ZEN_EL0EN, 0); +} + +static inline void sve_user_enable(void) +{ + sysreg_clear_set(cpacr_el1, 0, CPACR_EL1_ZEN_EL0EN); +} + /* * Probing and setup functions. * Calls to these functions must be serialised with one another. @@ -107,6 +119,9 @@ static inline int sve_get_current_vl(void) return -EINVAL; } +static inline void sve_user_disable(void) { } +static inline void sve_user_enable(void) { } + static inline void sve_init_vq_map(void) { } static inline void sve_update_vq_map(void) { } static inline int sve_verify_vq_map(void) { return 0; } diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index 088940387a4d..79a81c7d85c6 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -159,7 +159,6 @@ static void sve_free(struct task_struct *task) __sve_free(task); } - /* Offset of FFR in the SVE register dump */ static size_t sve_ffr_offset(int vl) { @@ -172,16 +171,6 @@ static void *sve_pffr(struct task_struct *task) sve_ffr_offset(task->thread.sve_vl); } -static void sve_user_disable(void) -{ - sysreg_clear_set(cpacr_el1, CPACR_EL1_ZEN_EL0EN, 0); -} - -static void sve_user_enable(void) -{ - sysreg_clear_set(cpacr_el1, 0, CPACR_EL1_ZEN_EL0EN); -} - /* * TIF_SVE controls whether a task can use SVE without trapping while * in userspace, and also the way a task's FPSIMD/SVE state is stored
In subsequent patches, we'll want to make use of sve_user_enable() and sve_user_disable() outside of kernel/fpsimd.c. Let's move these to <asm/fpsimd.h> where we can make use of them. To avoid ifdeffery in sequences like: if (system_supports_sve() && some_condition sve_user_disable(); ... empty stubs are provided when support for SVE is not enabled. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Dave Martin <dave.martin@arm.com> Cc: Will Deacon <will.deacon@arm.com> --- arch/arm64/include/asm/fpsimd.h | 17 ++++++++++++++++- arch/arm64/kernel/fpsimd.c | 11 ----------- 2 files changed, 16 insertions(+), 12 deletions(-)