Re: [PATCH 13/27] arm64/sve: Signal handling support

2017-08-23 Thread Dave Martin 
On Wed, Aug 23, 2017 at 10:38:51AM +0100, Alex Bennée wrote:
> 
> Dave Martin  writes:
> 
> > This patch implements support for saving and restoring the SVE
> > registers around signals.
> >
> > A fixed-size header struct sve_context is always included in the
> > signal frame encoding the thread's vector length at the time of
> > signal delivery, optionally followed by a variable-layout structure
> > encoding the SVE registers.
> >
> > Because of the need to preserve backwards compatibility, the FPSIMD
> > view of the SVE registers is always dumped as a struct
> > fpsimd_context in the usual way, in addition to any sve_context.
> >
> > The SVE vector registers are dumped in full, including bits 127:0
> > of each register which alias the corresponding FPSIMD vector
> > registers in the hardware.  To avoid any ambiguity about which
> > alias to restore during sigreturn, the kernel always restores bits
> > 127:0 of each SVE vector register from the fpsimd_context in the
> > signal frame (which must be present): userspace needs to take this
> > into account if it wants to modify the SVE vector register contents
> > on return from a signal.
> >
> > FPSR and FPCR, which are used by both FPSIMD and SVE, are not
> > included in sve_context because they are always present in
> > fpsimd_context anyway.
> >
> > For signal delivery, a new helper
> > fpsimd_signal_preserve_current_state() is added to update _both_
> > the FPSIMD and SVE views in the task struct, to make it easier to
> > populate this information into the signal frame.  Because of the
> > redundancy between the two views of the state, only one is updated
> > otherwise.  In order to avoid racing with a pending discard of the
> > SVE state, this flush is hoisted before the sigframe layout phase,
> > so that the layout and population phases see a consistent view of
> > the thread.
> >
> > Signed-off-by: Dave Martin 
> > ---

[...]

> > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> > index 80ecb2d..e8674f6 100644
> > --- a/arch/arm64/kernel/fpsimd.c
> > +++ b/arch/arm64/kernel/fpsimd.c
> > @@ -148,8 +148,6 @@ static void change_cpacr(u64 old, u64 new)
> > write_sysreg(new, CPACR_EL1);
> >  }
> >
> > -#ifdef CONFIG_ARM64_SVE
> > -
> >  #define ZREG(sve_state, vq, n) ((char *)(sve_state) +  \
> > (SVE_SIG_ZREG_OFFSET(vq, n) - SVE_SIG_REGS_OFFSET))
> >
> > @@ -191,6 +189,8 @@ static void fpsimd_to_sve(struct task_struct *task)
> >sizeof(fst->vregs[i]));
> >  }
> >
> > +#ifdef CONFIG_ARM64_SVE
> > +
> 
> Hmm have sve_to_fpsimd and fpsimd_to_sve only just started being used by
> the generic code here?

Yes, the signal code now makes use of these via
fpsimd_signal_preserve_current_state() and
fpsimd_update_current_state(), with this patch.

[...]

> > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> > index 4991e87..2694143 100644
> > --- a/arch/arm64/kernel/signal.c
> > +++ b/arch/arm64/kernel/signal.c

[...]

> > +#ifdef CONFIG_ARM64_SVE
> > +
> > +static int preserve_sve_context(struct sve_context __user *ctx)
> > +{
> > +   int err = 0;
> > +   u16 reserved[ARRAY_SIZE(ctx->__reserved)];
> > +   unsigned int vl = current->thread.sve_vl;
> > +   unsigned int vq = 0;
> > +
> > +   BUG_ON(!sve_vl_valid(vl));
> > +   if (test_thread_flag(TIF_SVE))
> > +   vq = sve_vq_from_vl(vl);
> > +
> > +   memset(reserved, 0, sizeof(reserved));
> > +
> > +   __put_user_error(SVE_MAGIC, >head.magic, err);
> > +   __put_user_error(round_up(SVE_SIG_CONTEXT_SIZE(vq), 16),
> > +>head.size, err);
> > +   __put_user_error(vl, >vl, err);
> > +   BUILD_BUG_ON(sizeof(ctx->__reserved) != sizeof(reserved));
> > +   err |= copy_to_user(>__reserved, reserved, sizeof(reserved));
> > +
> > +   if (vq) {
> > +   /*
> > +* This assumes that the SVE state has already been saved to
> > +* the task struct by calling preserve_fpsimd_context().
> > +*/
> > +   BUG_ON(SVE_SIG_REGS_SIZE(vq) !=
> > sve_state_size(current));
> 
> I think others have mentioned the excessive BUG_ON()s here but I think
> you are planning on cleaning some up on the next version. Assuming
> sve_vq_from_vl() can't give you an invalid answer from a
> sve_vl_valid(vl) then I wouldn't expect this test to add much.

Agreed.  Some BUG_ON()s test for things that are now obviously true
by construction, which was not initially the case during development.
This one can go away.

I'm very interested in any BUG_ON() that you think can actually fire.
There _shouldn't_ be any of those, but the reasons are non-trivial in
some cases.

> > +   err |= copy_to_user((char __user *)ctx + SVE_SIG_REGS_OFFSET,
> > +   current->thread.sve_state,
> > +   SVE_SIG_REGS_SIZE(vq));
> > +   }
> > +
> > +   return err ? -EFAULT : 0;
> > +}
> > +
> > +static int

Re: [PATCH 13/27] arm64/sve: Signal handling support

2017-08-23 Thread Alex Bennée 

Dave Martin  writes:

> This patch implements support for saving and restoring the SVE
> registers around signals.
>
> A fixed-size header struct sve_context is always included in the
> signal frame encoding the thread's vector length at the time of
> signal delivery, optionally followed by a variable-layout structure
> encoding the SVE registers.
>
> Because of the need to preserve backwards compatibility, the FPSIMD
> view of the SVE registers is always dumped as a struct
> fpsimd_context in the usual way, in addition to any sve_context.
>
> The SVE vector registers are dumped in full, including bits 127:0
> of each register which alias the corresponding FPSIMD vector
> registers in the hardware.  To avoid any ambiguity about which
> alias to restore during sigreturn, the kernel always restores bits
> 127:0 of each SVE vector register from the fpsimd_context in the
> signal frame (which must be present): userspace needs to take this
> into account if it wants to modify the SVE vector register contents
> on return from a signal.
>
> FPSR and FPCR, which are used by both FPSIMD and SVE, are not
> included in sve_context because they are always present in
> fpsimd_context anyway.
>
> For signal delivery, a new helper
> fpsimd_signal_preserve_current_state() is added to update _both_
> the FPSIMD and SVE views in the task struct, to make it easier to
> populate this information into the signal frame.  Because of the
> redundancy between the two views of the state, only one is updated
> otherwise.  In order to avoid racing with a pending discard of the
> SVE state, this flush is hoisted before the sigframe layout phase,
> so that the layout and population phases see a consistent view of
> the thread.
>
> Signed-off-by: Dave Martin 
> ---
>  arch/arm64/include/asm/fpsimd.h |   1 +
>  arch/arm64/kernel/fpsimd.c  |  23 --
>  arch/arm64/kernel/signal.c  | 169 
> ++--
>  arch/arm64/kernel/signal32.c|   2 +-
>  4 files changed, 179 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> index 72090a1..7efd04e 100644
> --- a/arch/arm64/include/asm/fpsimd.h
> +++ b/arch/arm64/include/asm/fpsimd.h
> @@ -63,6 +63,7 @@ extern void fpsimd_load_state(struct fpsimd_state *state);
>  extern void fpsimd_thread_switch(struct task_struct *next);
>  extern void fpsimd_flush_thread(void);
>
> +extern void fpsimd_signal_preserve_current_state(void);
>  extern void fpsimd_preserve_current_state(void);
>  extern void fpsimd_restore_current_state(void);
>  extern void fpsimd_update_current_state(struct fpsimd_state *state);
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 80ecb2d..e8674f6 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -148,8 +148,6 @@ static void change_cpacr(u64 old, u64 new)
>   write_sysreg(new, CPACR_EL1);
>  }
>
> -#ifdef CONFIG_ARM64_SVE
> -
>  #define ZREG(sve_state, vq, n) ((char *)(sve_state) +\
>   (SVE_SIG_ZREG_OFFSET(vq, n) - SVE_SIG_REGS_OFFSET))
>
> @@ -191,6 +189,8 @@ static void fpsimd_to_sve(struct task_struct *task)
>  sizeof(fst->vregs[i]));
>  }
>
> +#ifdef CONFIG_ARM64_SVE
> +

Hmm have sve_to_fpsimd and fpsimd_to_sve only just started being used by
the generic code here?

>  size_t sve_state_size(struct task_struct const *task)
>  {
>   unsigned int vl = task->thread.sve_vl;
> @@ -431,13 +431,17 @@ void fpsimd_preserve_current_state(void)
>   return;
>
>   local_bh_disable();
> -
> - if (!test_thread_flag(TIF_FOREIGN_FPSTATE))
> - fpsimd_save_state(>thread.fpsimd_state);
> -
> + task_fpsimd_save();
>   local_bh_enable();
>  }
>
> +void fpsimd_signal_preserve_current_state(void)
> +{
> + fpsimd_preserve_current_state();
> + if (system_supports_sve() && test_thread_flag(TIF_SVE))
> + sve_to_fpsimd(current);
> +}
> +
>  /*
>   * Load the userland FPSIMD state of 'current' from memory, but only if the
>   * FPSIMD state already held in the registers is /not/ the most recent FPSIMD
> @@ -473,7 +477,12 @@ void fpsimd_update_current_state(struct fpsimd_state 
> *state)
>
>   local_bh_disable();
>
> - fpsimd_load_state(state);
> + if (system_supports_sve() && test_thread_flag(TIF_SVE)) {
> + current->thread.fpsimd_state = *state;
> + fpsimd_to_sve(current);
> + }
> + task_fpsimd_load();
> +
>   if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
>   struct fpsimd_state *st = >thread.fpsimd_state;
>
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index 4991e87..2694143 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -62,6 +62,7 @@ struct rt_sigframe_user_layout {
>
>   unsigned long fpsimd_offset;
>   unsigned long esr_offset;
> + unsigned

[PATCH 13/27] arm64/sve: Signal handling support

2017-08-09 Thread Dave Martin 
This patch implements support for saving and restoring the SVE
registers around signals.

A fixed-size header struct sve_context is always included in the
signal frame encoding the thread's vector length at the time of
signal delivery, optionally followed by a variable-layout structure
encoding the SVE registers.

Because of the need to preserve backwards compatibility, the FPSIMD
view of the SVE registers is always dumped as a struct
fpsimd_context in the usual way, in addition to any sve_context.

The SVE vector registers are dumped in full, including bits 127:0
of each register which alias the corresponding FPSIMD vector
registers in the hardware.  To avoid any ambiguity about which
alias to restore during sigreturn, the kernel always restores bits
127:0 of each SVE vector register from the fpsimd_context in the
signal frame (which must be present): userspace needs to take this
into account if it wants to modify the SVE vector register contents
on return from a signal.

FPSR and FPCR, which are used by both FPSIMD and SVE, are not
included in sve_context because they are always present in
fpsimd_context anyway.

For signal delivery, a new helper
fpsimd_signal_preserve_current_state() is added to update _both_
the FPSIMD and SVE views in the task struct, to make it easier to
populate this information into the signal frame.  Because of the
redundancy between the two views of the state, only one is updated
otherwise.  In order to avoid racing with a pending discard of the
SVE state, this flush is hoisted before the sigframe layout phase,
so that the layout and population phases see a consistent view of
the thread.

Signed-off-by: Dave Martin 
---
 arch/arm64/include/asm/fpsimd.h |   1 +
 arch/arm64/kernel/fpsimd.c  |  23 --
 arch/arm64/kernel/signal.c  | 169 ++--
 arch/arm64/kernel/signal32.c|   2 +-
 4 files changed, 179 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index 72090a1..7efd04e 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -63,6 +63,7 @@ extern void fpsimd_load_state(struct fpsimd_state *state);
 extern void fpsimd_thread_switch(struct task_struct *next);
 extern void fpsimd_flush_thread(void);
 
+extern void fpsimd_signal_preserve_current_state(void);
 extern void fpsimd_preserve_current_state(void);
 extern void fpsimd_restore_current_state(void);
 extern void fpsimd_update_current_state(struct fpsimd_state *state);
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 80ecb2d..e8674f6 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -148,8 +148,6 @@ static void change_cpacr(u64 old, u64 new)
write_sysreg(new, CPACR_EL1);
 }
 
-#ifdef CONFIG_ARM64_SVE
-
 #define ZREG(sve_state, vq, n) ((char *)(sve_state) +  \
(SVE_SIG_ZREG_OFFSET(vq, n) - SVE_SIG_REGS_OFFSET))
 
@@ -191,6 +189,8 @@ static void fpsimd_to_sve(struct task_struct *task)
   sizeof(fst->vregs[i]));
 }
 
+#ifdef CONFIG_ARM64_SVE
+
 size_t sve_state_size(struct task_struct const *task)
 {
unsigned int vl = task->thread.sve_vl;
@@ -431,13 +431,17 @@ void fpsimd_preserve_current_state(void)
return;
 
local_bh_disable();
-
-   if (!test_thread_flag(TIF_FOREIGN_FPSTATE))
-   fpsimd_save_state(>thread.fpsimd_state);
-
+   task_fpsimd_save();
local_bh_enable();
 }
 
+void fpsimd_signal_preserve_current_state(void)
+{
+   fpsimd_preserve_current_state();
+   if (system_supports_sve() && test_thread_flag(TIF_SVE))
+   sve_to_fpsimd(current);
+}
+
 /*
  * Load the userland FPSIMD state of 'current' from memory, but only if the
  * FPSIMD state already held in the registers is /not/ the most recent FPSIMD
@@ -473,7 +477,12 @@ void fpsimd_update_current_state(struct fpsimd_state 
*state)
 
local_bh_disable();
 
-   fpsimd_load_state(state);
+   if (system_supports_sve() && test_thread_flag(TIF_SVE)) {
+   current->thread.fpsimd_state = *state;
+   fpsimd_to_sve(current);
+   }
+   task_fpsimd_load();
+
if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
struct fpsimd_state *st = >thread.fpsimd_state;
 
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 4991e87..2694143 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -62,6 +62,7 @@ struct rt_sigframe_user_layout {
 
unsigned long fpsimd_offset;
unsigned long esr_offset;
+   unsigned long sve_offset;
unsigned long extra_offset;
unsigned long end_offset;
 };
@@ -178,9 +179,6 @@ static int preserve_fpsimd_context(struct fpsimd_context 
__user *ctx)
struct fpsimd_state *fpsimd = >thread.fpsimd_state;
int err;
 
-   /* dump the hardware registers to the fpsimd_state