[06/18] arm64: move sve_user_{enable,disable} to <asm/fpsimd.h>

Message ID 20180514094640.27569-7-mark.rutland@arm.com
State Superseded
Headers show
Series
  • arm64: invoke syscalls with pt_regs
Related show

Commit Message

Mark Rutland May 14, 2018, 9:46 a.m.
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(-)

-- 
2.11.0

Comments

Dave Martin May 14, 2018, 11:06 a.m. | #1
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
Mark Rutland May 15, 2018, 10:39 a.m. | #2
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.
Dave Martin May 15, 2018, 12:19 p.m. | #3
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
Mark Rutland May 15, 2018, 4:33 p.m. | #4
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.
Dave Martin May 16, 2018, 9:01 a.m. | #5
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
Mark Rutland June 1, 2018, 10:29 a.m. | #6
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.
Dave Martin June 1, 2018, 10:42 a.m. | #7
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

Patch

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