diff mbox series

[v5,08/26] arm64/sve: Enable SVE state tracking for non-task contexts

Message ID 1550519559-15915-9-git-send-email-Dave.Martin@arm.com
State New
Headers show
Series KVM: arm64: SVE guest support | expand

Commit Message

Dave Martin Feb. 18, 2019, 7:52 p.m. UTC 
The current FPSIMD/SVE context handling support for non-task (i.e.,
KVM vcpu) contexts does not take SVE into account.  This means that
only task contexts can safely use SVE at present.

In preparation for enabling KVM guests to use SVE, it is necessary
to keep track of SVE state for non-task contexts too.

This patch adds the necessary support, removing assumptions from
the context switch code about the location of the SVE context
storage.

When binding a vcpu context, its vector length is arbitrarily
specified as SVE_VL_MIN for now.  In any case, because TIF_SVE is
presently cleared at vcpu context bind time, the specified vector
length will not be used for anything yet.  In later patches TIF_SVE
will be set here as appropriate, and the appropriate maximum vector
length for the vcpu will be passed when binding.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 arch/arm64/include/asm/fpsimd.h |  3 ++-
 arch/arm64/kernel/fpsimd.c      | 20 +++++++++++++++-----
 arch/arm64/kvm/fpsimd.c         |  5 ++++-
 3 files changed, 21 insertions(+), 7 deletions(-)

Comments

Julien Grall Feb. 22, 2019, 3:26 p.m. UTC | #1 
Hi Dave,

On 18/02/2019 19:52, Dave Martin wrote:
> The current FPSIMD/SVE context handling support for non-task (i.e.,
> KVM vcpu) contexts does not take SVE into account.  This means that

NIT: Double-space before "This".

> only task contexts can safely use SVE at present.
> 
> In preparation for enabling KVM guests to use SVE, it is necessary
> to keep track of SVE state for non-task contexts too.
> 
> This patch adds the necessary support, removing assumptions from
> the context switch code about the location of the SVE context
> storage.
> 
> When binding a vcpu context, its vector length is arbitrarily
> specified as SVE_VL_MIN for now.  In any case, because TIF_SVE is

NIT: Double-space before "In".

> presently cleared at vcpu context bind time, the specified vector
> length will not be used for anything yet.  In later patches TIF_SVE

NIT: Double-space before "In".

> will be set here as appropriate, and the appropriate maximum vector
> length for the vcpu will be passed when binding.

Cheers,
Dave Martin Feb. 26, 2019, 12:07 p.m. UTC | #2 
On Fri, Feb 22, 2019 at 03:26:51PM +0000, Julien Grall wrote:
> Hi Dave,
> 
> On 18/02/2019 19:52, Dave Martin wrote:
> >The current FPSIMD/SVE context handling support for non-task (i.e.,
> >KVM vcpu) contexts does not take SVE into account.  This means that
> 
> NIT: Double-space before "This".

See patch 2...

[...]

Does the code look reasonable to you?  This interacts with FPSIMD/SVE
context switch in the host, so it would be good to have your view on it.

Cheers
---Dave
Julien Grall Feb. 26, 2019, 3:49 p.m. UTC | #3 
Hi Dave,

On 26/02/2019 12:07, Dave Martin wrote:
> On Fri, Feb 22, 2019 at 03:26:51PM +0000, Julien Grall wrote:
>> Hi Dave,
>>
>> On 18/02/2019 19:52, Dave Martin wrote:
>>> The current FPSIMD/SVE context handling support for non-task (i.e.,
>>> KVM vcpu) contexts does not take SVE into account.  This means that
>>
>> NIT: Double-space before "This".
> 
> See patch 2...
> 
> [...]
> 
> Does the code look reasonable to you?  This interacts with FPSIMD/SVE
> context switch in the host, so it would be good to have your view on it.

I wanted to look at the rest before giving my reviewed-by tag.
FWIW, this patch looks reasonable to me.

Cheers,
Dave Martin Feb. 26, 2019, 3:58 p.m. UTC | #4 
On Tue, Feb 26, 2019 at 03:49:00PM +0000, Julien Grall wrote:
> Hi Dave,
> 
> On 26/02/2019 12:07, Dave Martin wrote:
> >On Fri, Feb 22, 2019 at 03:26:51PM +0000, Julien Grall wrote:
> >>Hi Dave,
> >>
> >>On 18/02/2019 19:52, Dave Martin wrote:
> >>>The current FPSIMD/SVE context handling support for non-task (i.e.,
> >>>KVM vcpu) contexts does not take SVE into account.  This means that
> >>
> >>NIT: Double-space before "This".
> >
> >See patch 2...
> >
> >[...]
> >
> >Does the code look reasonable to you?  This interacts with FPSIMD/SVE
> >context switch in the host, so it would be good to have your view on it.
> 
> I wanted to look at the rest before giving my reviewed-by tag.
> FWIW, this patch looks reasonable to me.

OK, does that amount to a Reviewed-by, or do you have other comments?

Cheers
---Dave
Julien Grall Feb. 26, 2019, 3:59 p.m. UTC | #5 
Hi Dave,

On 26/02/2019 15:58, Dave Martin wrote:
> On Tue, Feb 26, 2019 at 03:49:00PM +0000, Julien Grall wrote:
>> Hi Dave,
>>
>> On 26/02/2019 12:07, Dave Martin wrote:
>>> On Fri, Feb 22, 2019 at 03:26:51PM +0000, Julien Grall wrote:
>>>> Hi Dave,
>>>>
>>>> On 18/02/2019 19:52, Dave Martin wrote:
>>>>> The current FPSIMD/SVE context handling support for non-task (i.e.,
>>>>> KVM vcpu) contexts does not take SVE into account.  This means that
>>>>
>>>> NIT: Double-space before "This".
>>>
>>> See patch 2...
>>>
>>> [...]
>>>
>>> Does the code look reasonable to you?  This interacts with FPSIMD/SVE
>>> context switch in the host, so it would be good to have your view on it.
>>
>> I wanted to look at the rest before giving my reviewed-by tag.
>> FWIW, this patch looks reasonable to me.
> 
> OK, does that amount to a Reviewed-by, or do you have other comments?

I have no further comments on this patch.

Reviewed-by: Julien Grall <julien.grall@arm.com>

Cheers,
Dave Martin Feb. 26, 2019, 4:03 p.m. UTC | #6 
On Tue, Feb 26, 2019 at 03:59:40PM +0000, Julien Grall wrote:
> Hi Dave,
> 
> On 26/02/2019 15:58, Dave Martin wrote:
> >On Tue, Feb 26, 2019 at 03:49:00PM +0000, Julien Grall wrote:
> >>Hi Dave,
> >>
> >>On 26/02/2019 12:07, Dave Martin wrote:

[...]

> >>>Does the code look reasonable to you?  This interacts with FPSIMD/SVE
> >>>context switch in the host, so it would be good to have your view on it.
> >>
> >>I wanted to look at the rest before giving my reviewed-by tag.
> >>FWIW, this patch looks reasonable to me.
> >
> >OK, does that amount to a Reviewed-by, or do you have other comments?
> 
> I have no further comments on this patch.
> 
> Reviewed-by: Julien Grall <julien.grall@arm.com>

Thanks
---Dave
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index 964adc9..df7a143 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -56,7 +56,8 @@  extern void fpsimd_restore_current_state(void);
 extern void fpsimd_update_current_state(struct user_fpsimd_state const *state);
 
 extern void fpsimd_bind_task_to_cpu(void);
-extern void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *state);
+extern void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *state,
+				     void *sve_state, unsigned int sve_vl);
 
 extern void fpsimd_flush_task_state(struct task_struct *target);
 extern void fpsimd_flush_cpu_state(void);
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 92c2331..09ee264 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -121,6 +121,8 @@ 
  */
 struct fpsimd_last_state_struct {
 	struct user_fpsimd_state *st;
+	void *sve_state;
+	unsigned int sve_vl;
 };
 
 static DEFINE_PER_CPU(struct fpsimd_last_state_struct, fpsimd_last_state);
@@ -241,14 +243,15 @@  static void task_fpsimd_load(void)
  */
 void fpsimd_save(void)
 {
-	struct user_fpsimd_state *st = __this_cpu_read(fpsimd_last_state.st);
+	struct fpsimd_last_state_struct const *last =
+		this_cpu_ptr(&fpsimd_last_state);
 	/* set by fpsimd_bind_task_to_cpu() or fpsimd_bind_state_to_cpu() */
 
 	WARN_ON(!in_softirq() && !irqs_disabled());
 
 	if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) {
 		if (system_supports_sve() && test_thread_flag(TIF_SVE)) {
-			if (WARN_ON(sve_get_vl() != current->thread.sve_vl)) {
+			if (WARN_ON(sve_get_vl() != last->sve_vl)) {
 				/*
 				 * Can't save the user regs, so current would
 				 * re-enter user with corrupt state.
@@ -258,9 +261,11 @@  void fpsimd_save(void)
 				return;
 			}
 
-			sve_save_state(sve_pffr(&current->thread), &st->fpsr);
+			sve_save_state((char *)last->sve_state +
+						sve_ffr_offset(last->sve_vl),
+				       &last->st->fpsr);
 		} else
-			fpsimd_save_state(st);
+			fpsimd_save_state(last->st);
 	}
 }
 
@@ -1034,6 +1039,8 @@  void fpsimd_bind_task_to_cpu(void)
 		this_cpu_ptr(&fpsimd_last_state);
 
 	last->st = &current->thread.uw.fpsimd_state;
+	last->sve_state = current->thread.sve_state;
+	last->sve_vl = current->thread.sve_vl;
 	current->thread.fpsimd_cpu = smp_processor_id();
 
 	if (system_supports_sve()) {
@@ -1047,7 +1054,8 @@  void fpsimd_bind_task_to_cpu(void)
 	}
 }
 
-void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st)
+void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st, void *sve_state,
+			      unsigned int sve_vl)
 {
 	struct fpsimd_last_state_struct *last =
 		this_cpu_ptr(&fpsimd_last_state);
@@ -1055,6 +1063,8 @@  void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st)
 	WARN_ON(!in_softirq() && !irqs_disabled());
 
 	last->st = st;
+	last->sve_state = sve_state;
+	last->sve_vl = sve_vl;
 }
 
 /*
diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index aac7808..1cf4f02 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -9,6 +9,7 @@ 
 #include <linux/sched.h>
 #include <linux/thread_info.h>
 #include <linux/kvm_host.h>
+#include <asm/fpsimd.h>
 #include <asm/kvm_asm.h>
 #include <asm/kvm_host.h>
 #include <asm/kvm_mmu.h>
@@ -85,7 +86,9 @@  void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
 	WARN_ON_ONCE(!irqs_disabled());
 
 	if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED) {
-		fpsimd_bind_state_to_cpu(&vcpu->arch.ctxt.gp_regs.fp_regs);
+		fpsimd_bind_state_to_cpu(&vcpu->arch.ctxt.gp_regs.fp_regs,
+					 NULL, SVE_VL_MIN);
+
 		clear_thread_flag(TIF_FOREIGN_FPSTATE);
 		clear_thread_flag(TIF_SVE);
 	}