[PATCH 0/4] KVM: arm64: FPSIMD/SVE fixes for 4.17

[PATCH 0/4] KVM: arm64: FPSIMD/SVE fixes for 4.17

[Dave Martin]

This series contains fixes for some issues observed since the KVM arm64
pull request for 4.17.

The first patch (from Mark Rutland) adds a straightforward helper to
modify particular bits in a system register conditionally.  This is used
by patch 3 (though obviously it could be coded round and patch 1
dropped, if preferred).

See other patches for details of the individual fixes.

Dave Martin (3):
  KVM: arm64: Don't mask softirq with IRQs disabled in vcpu_put()
  KVM: arm64/sve: Fix SVE trap restoration for non-current tasks
  KVM: arm64: Avoid mistaken attempts to save SVE state for vcpus

Mark Rutland (1):
  arm64: introduce sysreg_clear_set()

 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/include/asm/sysreg.h   | 11 ++++++++++
 arch/arm64/kvm/fpsimd.c           | 46 +++++++++++++++++++++++++++++----------
 3 files changed, 47 insertions(+), 11 deletions(-)

-- 
2.1.4

[PATCH 0/4] KVM: arm64: FPSIMD/SVE fixes for 4.17

[Dave Martin]

This series contains fixes for some issues observed since the KVM arm64
pull request for 4.17.

The first patch (from Mark Rutland) adds a straightforward helper to
modify particular bits in a system register conditionally.  This is used
by patch 3 (though obviously it could be coded round and patch 1
dropped, if preferred).

See other patches for details of the individual fixes.

Dave Martin (3):
  KVM: arm64: Don't mask softirq with IRQs disabled in vcpu_put()
  KVM: arm64/sve: Fix SVE trap restoration for non-current tasks
  KVM: arm64: Avoid mistaken attempts to save SVE state for vcpus

Mark Rutland (1):
  arm64: introduce sysreg_clear_set()

 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/include/asm/sysreg.h   | 11 ++++++++++
 arch/arm64/kvm/fpsimd.c           | 46 +++++++++++++++++++++++++++++----------
 3 files changed, 47 insertions(+), 11 deletions(-)

-- 
2.1.4

[PATCH 1/4] arm64: introduce sysreg_clear_set()

[Dave Martin]

From: Mark Rutland <mark.rutland@arm.com>

Currently we have a couple of helpers to manipulate bits in particular
sysregs:

 * config_sctlr_el1(u32 clear, u32 set)

 * change_cpacr(u64 val, u64 mask)

The parameters of these differ in naming convention, order, and size,
which is unfortunate. They also differ slightly in behaviour, as
change_cpacr() skips the sysreg write if the bits are unchanged, which
is a useful optimization when sysreg writes are expensive.

Before we gain more yet another sysreg manipulation function, let's
unify these with a common helper, providing a consistent order for
clear/set operands, and the write skipping behaviour from
change_cpacr(). Code will be migrated to the new helper in subsequent
patches.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Dave Martin <dave.martin@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/sysreg.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 6171178..a8f8481 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -728,6 +728,17 @@ asm(
 	asm volatile("msr_s " __stringify(r) ", %x0" : : "rZ" (__val));	\
 } while (0)
 
+/*
+ * Modify bits in a sysreg. Bits in the clear mask are zeroed, then bits in the
+ * set mask are set. Other bits are left as-is.
+ */
+#define sysreg_clear_set(sysreg, clear, set) do {			\
+	u64 __scs_val = read_sysreg(sysreg);				\
+	u64 __scs_new = (__scs_val & ~(u64)(clear)) | (set);		\
+	if (__scs_new != __scs_val)					\
+		write_sysreg(__scs_new, sysreg);			\
+} while (0)
+
 static inline void config_sctlr_el1(u32 clear, u32 set)
 {
 	u32 val;
-- 
2.1.4

[PATCH 1/4] arm64: introduce sysreg_clear_set()

[Dave Martin]

From: Mark Rutland <mark.rutland@arm.com>

Currently we have a couple of helpers to manipulate bits in particular
sysregs:

 * config_sctlr_el1(u32 clear, u32 set)

 * change_cpacr(u64 val, u64 mask)

The parameters of these differ in naming convention, order, and size,
which is unfortunate. They also differ slightly in behaviour, as
change_cpacr() skips the sysreg write if the bits are unchanged, which
is a useful optimization when sysreg writes are expensive.

Before we gain more yet another sysreg manipulation function, let's
unify these with a common helper, providing a consistent order for
clear/set operands, and the write skipping behaviour from
change_cpacr(). Code will be migrated to the new helper in subsequent
patches.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Dave Martin <dave.martin@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/sysreg.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 6171178..a8f8481 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -728,6 +728,17 @@ asm(
 	asm volatile("msr_s " __stringify(r) ", %x0" : : "rZ" (__val));	\
 } while (0)
 
+/*
+ * Modify bits in a sysreg. Bits in the clear mask are zeroed, then bits in the
+ * set mask are set. Other bits are left as-is.
+ */
+#define sysreg_clear_set(sysreg, clear, set) do {			\
+	u64 __scs_val = read_sysreg(sysreg);				\
+	u64 __scs_new = (__scs_val & ~(u64)(clear)) | (set);		\
+	if (__scs_new != __scs_val)					\
+		write_sysreg(__scs_new, sysreg);			\
+} while (0)
+
 static inline void config_sctlr_el1(u32 clear, u32 set)
 {
 	u32 val;
-- 
2.1.4

[PATCH 2/4] KVM: arm64: Don't mask softirq with IRQs disabled in vcpu_put()

[Dave Martin]

Commit e6b673b ("KVM: arm64: Optimise FPSIMD handling to reduce
guest/host thrashing") introduces a specific helper
kvm_arch_vcpu_put_fp() for saving the vcpu FPSIMD state during
vcpu_put().

This function uses local_bh_disable()/_enable() to protect the
FPSIMD context manipulation from interruption by softirqs.

This approach is not correct, because vcpu_put() can be invoked
either from the KVM host vcpu thread (when exiting the vcpu run
loop), or via a preempt notifier.  In the former case, only
preemption is disabled.  In the latter case, the function is called
from inside __schedule(), which means that IRQs are disabled.

Use of local_bh_disable()/_enable() with IRQs disabled is considerd
an error, resulting in lockdep splats while running VMs if lockdep
is enabled.

It is probably possible in theory to relax this restriction on
local_bh_disable()/_enable() usage, but for now this patch takes
the simple approach of managing softirq masking only if IRQs happen
to be enabled when kvm_arch_vcpu_put_fp() is called.

Fixes: e6b673b741ea ("KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing")
Reported-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/fpsimd.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index dc6ecfa..b51ff80 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -90,10 +90,8 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
  * disappears and another task or vcpu appears that recycles the same
  * struct fpsimd_state.
  */
-void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
+static void __kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
 {
-	local_bh_disable();
-
 	update_thread_flag(TIF_SVE,
 			   vcpu->arch.flags & KVM_ARM64_HOST_SVE_IN_USE);
 
@@ -105,6 +103,16 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
 		/* Ensure user trap controls are correctly restored */
 		fpsimd_bind_task_to_cpu();
 	}
+}
+
 
-	local_bh_enable();
+void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
+{
+	if (irqs_disabled())
+		__kvm_arch_vcpu_put_fp(vcpu);
+	else {
+		local_bh_disable();
+		__kvm_arch_vcpu_put_fp(vcpu);
+		local_bh_enable();
+	}
 }
-- 
2.1.4

[PATCH 2/4] KVM: arm64: Don't mask softirq with IRQs disabled in vcpu_put()

[Dave Martin]

Commit e6b673b ("KVM: arm64: Optimise FPSIMD handling to reduce
guest/host thrashing") introduces a specific helper
kvm_arch_vcpu_put_fp() for saving the vcpu FPSIMD state during
vcpu_put().

This function uses local_bh_disable()/_enable() to protect the
FPSIMD context manipulation from interruption by softirqs.

This approach is not correct, because vcpu_put() can be invoked
either from the KVM host vcpu thread (when exiting the vcpu run
loop), or via a preempt notifier.  In the former case, only
preemption is disabled.  In the latter case, the function is called
from inside __schedule(), which means that IRQs are disabled.

Use of local_bh_disable()/_enable() with IRQs disabled is considerd
an error, resulting in lockdep splats while running VMs if lockdep
is enabled.

It is probably possible in theory to relax this restriction on
local_bh_disable()/_enable() usage, but for now this patch takes
the simple approach of managing softirq masking only if IRQs happen
to be enabled when kvm_arch_vcpu_put_fp() is called.

Fixes: e6b673b741ea ("KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing")
Reported-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/fpsimd.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index dc6ecfa..b51ff80 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -90,10 +90,8 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
  * disappears and another task or vcpu appears that recycles the same
  * struct fpsimd_state.
  */
-void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
+static void __kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
 {
-	local_bh_disable();
-
 	update_thread_flag(TIF_SVE,
 			   vcpu->arch.flags & KVM_ARM64_HOST_SVE_IN_USE);
 
@@ -105,6 +103,16 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
 		/* Ensure user trap controls are correctly restored */
 		fpsimd_bind_task_to_cpu();
 	}
+}
+
 
-	local_bh_enable();
+void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
+{
+	if (irqs_disabled())
+		__kvm_arch_vcpu_put_fp(vcpu);
+	else {
+		local_bh_disable();
+		__kvm_arch_vcpu_put_fp(vcpu);
+		local_bh_enable();
+	}
 }
-- 
2.1.4

Re: [PATCH 2/4] KVM: arm64: Don't mask softirq with IRQs disabled in vcpu_put()

[Marc Zyngier]

Hi Dave,

On 14/06/18 12:42, Dave Martin wrote:
> Commit e6b673b ("KVM: arm64: Optimise FPSIMD handling to reduce
> guest/host thrashing") introduces a specific helper
> kvm_arch_vcpu_put_fp() for saving the vcpu FPSIMD state during
> vcpu_put().
> 
> This function uses local_bh_disable()/_enable() to protect the
> FPSIMD context manipulation from interruption by softirqs.
> 
> This approach is not correct, because vcpu_put() can be invoked
> either from the KVM host vcpu thread (when exiting the vcpu run
> loop), or via a preempt notifier.  In the former case, only
> preemption is disabled.  In the latter case, the function is called
> from inside __schedule(), which means that IRQs are disabled.
> 
> Use of local_bh_disable()/_enable() with IRQs disabled is considerd
> an error, resulting in lockdep splats while running VMs if lockdep
> is enabled.
> 
> It is probably possible in theory to relax this restriction on
> local_bh_disable()/_enable() usage, but for now this patch takes
> the simple approach of managing softirq masking only if IRQs happen
> to be enabled when kvm_arch_vcpu_put_fp() is called.
> 
> Fixes: e6b673b741ea ("KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing")
> Reported-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  arch/arm64/kvm/fpsimd.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> index dc6ecfa..b51ff80 100644
> --- a/arch/arm64/kvm/fpsimd.c
> +++ b/arch/arm64/kvm/fpsimd.c
> @@ -90,10 +90,8 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
>   * disappears and another task or vcpu appears that recycles the same
>   * struct fpsimd_state.
>   */
> -void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> +static void __kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
>  {
> -	local_bh_disable();
> -
>  	update_thread_flag(TIF_SVE,
>  			   vcpu->arch.flags & KVM_ARM64_HOST_SVE_IN_USE);
>  
> @@ -105,6 +103,16 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
>  		/* Ensure user trap controls are correctly restored */
>  		fpsimd_bind_task_to_cpu();
>  	}
> +}
> +
>  
> -	local_bh_enable();
> +void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> +{
> +	if (irqs_disabled())
> +		__kvm_arch_vcpu_put_fp(vcpu);
> +	else {
> +		local_bh_disable();
> +		__kvm_arch_vcpu_put_fp(vcpu);
> +		local_bh_enable();
> +	}
>  }
> 

I'm of the opinion to always run kvm_arch_vcpu_put_fp() with interrupt
disabled. local_bh_enable() does quite a lot of stuff (running the
softirqs), which adds overhead we could do without.

I'd replace local_bh_{disable,enable} with local_irq_{save,restore).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 2/4] KVM: arm64: Don't mask softirq with IRQs disabled in vcpu_put()

[Marc Zyngier]

Hi Dave,

On 14/06/18 12:42, Dave Martin wrote:
> Commit e6b673b ("KVM: arm64: Optimise FPSIMD handling to reduce
> guest/host thrashing") introduces a specific helper
> kvm_arch_vcpu_put_fp() for saving the vcpu FPSIMD state during
> vcpu_put().
> 
> This function uses local_bh_disable()/_enable() to protect the
> FPSIMD context manipulation from interruption by softirqs.
> 
> This approach is not correct, because vcpu_put() can be invoked
> either from the KVM host vcpu thread (when exiting the vcpu run
> loop), or via a preempt notifier.  In the former case, only
> preemption is disabled.  In the latter case, the function is called
> from inside __schedule(), which means that IRQs are disabled.
> 
> Use of local_bh_disable()/_enable() with IRQs disabled is considerd
> an error, resulting in lockdep splats while running VMs if lockdep
> is enabled.
> 
> It is probably possible in theory to relax this restriction on
> local_bh_disable()/_enable() usage, but for now this patch takes
> the simple approach of managing softirq masking only if IRQs happen
> to be enabled when kvm_arch_vcpu_put_fp() is called.
> 
> Fixes: e6b673b741ea ("KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing")
> Reported-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  arch/arm64/kvm/fpsimd.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> index dc6ecfa..b51ff80 100644
> --- a/arch/arm64/kvm/fpsimd.c
> +++ b/arch/arm64/kvm/fpsimd.c
> @@ -90,10 +90,8 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
>   * disappears and another task or vcpu appears that recycles the same
>   * struct fpsimd_state.
>   */
> -void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> +static void __kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
>  {
> -	local_bh_disable();
> -
>  	update_thread_flag(TIF_SVE,
>  			   vcpu->arch.flags & KVM_ARM64_HOST_SVE_IN_USE);
>  
> @@ -105,6 +103,16 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
>  		/* Ensure user trap controls are correctly restored */
>  		fpsimd_bind_task_to_cpu();
>  	}
> +}
> +
>  
> -	local_bh_enable();
> +void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> +{
> +	if (irqs_disabled())
> +		__kvm_arch_vcpu_put_fp(vcpu);
> +	else {
> +		local_bh_disable();
> +		__kvm_arch_vcpu_put_fp(vcpu);
> +		local_bh_enable();
> +	}
>  }
> 

I'm of the opinion to always run kvm_arch_vcpu_put_fp() with interrupt
disabled. local_bh_enable() does quite a lot of stuff (running the
softirqs), which adds overhead we could do without.

I'd replace local_bh_{disable,enable} with local_irq_{save,restore).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 2/4] KVM: arm64: Don't mask softirq with IRQs disabled in vcpu_put()

[Dave Martin]

On Thu, Jun 14, 2018 at 01:50:00PM +0100, Marc Zyngier wrote:
> Hi Dave,
> 
> On 14/06/18 12:42, Dave Martin wrote:
> > Commit e6b673b ("KVM: arm64: Optimise FPSIMD handling to reduce
> > guest/host thrashing") introduces a specific helper
> > kvm_arch_vcpu_put_fp() for saving the vcpu FPSIMD state during
> > vcpu_put().
> > 
> > This function uses local_bh_disable()/_enable() to protect the
> > FPSIMD context manipulation from interruption by softirqs.
> > 
> > This approach is not correct, because vcpu_put() can be invoked
> > either from the KVM host vcpu thread (when exiting the vcpu run
> > loop), or via a preempt notifier.  In the former case, only
> > preemption is disabled.  In the latter case, the function is called
> > from inside __schedule(), which means that IRQs are disabled.
> > 
> > Use of local_bh_disable()/_enable() with IRQs disabled is considerd
> > an error, resulting in lockdep splats while running VMs if lockdep
> > is enabled.
> > 
> > It is probably possible in theory to relax this restriction on
> > local_bh_disable()/_enable() usage, but for now this patch takes
> > the simple approach of managing softirq masking only if IRQs happen
> > to be enabled when kvm_arch_vcpu_put_fp() is called.
> > 
> > Fixes: e6b673b741ea ("KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing")
> > Reported-by: Andre Przywara <andre.przywara@arm.com>
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> >  arch/arm64/kvm/fpsimd.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> > index dc6ecfa..b51ff80 100644
> > --- a/arch/arm64/kvm/fpsimd.c
> > +++ b/arch/arm64/kvm/fpsimd.c
> > @@ -90,10 +90,8 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
> >   * disappears and another task or vcpu appears that recycles the same
> >   * struct fpsimd_state.
> >   */
> > -void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> > +static void __kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> >  {
> > -	local_bh_disable();
> > -
> >  	update_thread_flag(TIF_SVE,
> >  			   vcpu->arch.flags & KVM_ARM64_HOST_SVE_IN_USE);
> >  
> > @@ -105,6 +103,16 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> >  		/* Ensure user trap controls are correctly restored */
> >  		fpsimd_bind_task_to_cpu();
> >  	}
> > +}
> > +
> >  
> > -	local_bh_enable();
> > +void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> > +{
> > +	if (irqs_disabled())
> > +		__kvm_arch_vcpu_put_fp(vcpu);
> > +	else {
> > +		local_bh_disable();
> > +		__kvm_arch_vcpu_put_fp(vcpu);
> > +		local_bh_enable();
> > +	}
> >  }
> > 
> 
> I'm of the opinion to always run kvm_arch_vcpu_put_fp() with interrupt
> disabled. local_bh_enable() does quite a lot of stuff (running the
> softirqs), which adds overhead we could do without.
> 
> I'd replace local_bh_{disable,enable} with local_irq_{save,restore).

I don't have a huge problem with that.  This creates interrupt blackout
on run loop exit, but it's a) not worse than the blackout in
__schedule() and b) presumably the rare case compared with run loop
preemption.

So, while disabling interrupts seemed a bit brutal, in context it
doesn't look like such a big deal.

Cheers
---Dave

[PATCH 2/4] KVM: arm64: Don't mask softirq with IRQs disabled in vcpu_put()

[Dave Martin]

On Thu, Jun 14, 2018 at 01:50:00PM +0100, Marc Zyngier wrote:
> Hi Dave,
> 
> On 14/06/18 12:42, Dave Martin wrote:
> > Commit e6b673b ("KVM: arm64: Optimise FPSIMD handling to reduce
> > guest/host thrashing") introduces a specific helper
> > kvm_arch_vcpu_put_fp() for saving the vcpu FPSIMD state during
> > vcpu_put().
> > 
> > This function uses local_bh_disable()/_enable() to protect the
> > FPSIMD context manipulation from interruption by softirqs.
> > 
> > This approach is not correct, because vcpu_put() can be invoked
> > either from the KVM host vcpu thread (when exiting the vcpu run
> > loop), or via a preempt notifier.  In the former case, only
> > preemption is disabled.  In the latter case, the function is called
> > from inside __schedule(), which means that IRQs are disabled.
> > 
> > Use of local_bh_disable()/_enable() with IRQs disabled is considerd
> > an error, resulting in lockdep splats while running VMs if lockdep
> > is enabled.
> > 
> > It is probably possible in theory to relax this restriction on
> > local_bh_disable()/_enable() usage, but for now this patch takes
> > the simple approach of managing softirq masking only if IRQs happen
> > to be enabled when kvm_arch_vcpu_put_fp() is called.
> > 
> > Fixes: e6b673b741ea ("KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing")
> > Reported-by: Andre Przywara <andre.przywara@arm.com>
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> >  arch/arm64/kvm/fpsimd.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> > index dc6ecfa..b51ff80 100644
> > --- a/arch/arm64/kvm/fpsimd.c
> > +++ b/arch/arm64/kvm/fpsimd.c
> > @@ -90,10 +90,8 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
> >   * disappears and another task or vcpu appears that recycles the same
> >   * struct fpsimd_state.
> >   */
> > -void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> > +static void __kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> >  {
> > -	local_bh_disable();
> > -
> >  	update_thread_flag(TIF_SVE,
> >  			   vcpu->arch.flags & KVM_ARM64_HOST_SVE_IN_USE);
> >  
> > @@ -105,6 +103,16 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> >  		/* Ensure user trap controls are correctly restored */
> >  		fpsimd_bind_task_to_cpu();
> >  	}
> > +}
> > +
> >  
> > -	local_bh_enable();
> > +void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> > +{
> > +	if (irqs_disabled())
> > +		__kvm_arch_vcpu_put_fp(vcpu);
> > +	else {
> > +		local_bh_disable();
> > +		__kvm_arch_vcpu_put_fp(vcpu);
> > +		local_bh_enable();
> > +	}
> >  }
> > 
> 
> I'm of the opinion to always run kvm_arch_vcpu_put_fp() with interrupt
> disabled. local_bh_enable() does quite a lot of stuff (running the
> softirqs), which adds overhead we could do without.
> 
> I'd replace local_bh_{disable,enable} with local_irq_{save,restore).

I don't have a huge problem with that.  This creates interrupt blackout
on run loop exit, but it's a) not worse than the blackout in
__schedule() and b) presumably the rare case compared with run loop
preemption.

So, while disabling interrupts seemed a bit brutal, in context it
doesn't look like such a big deal.

Cheers
---Dave

[PATCH 3/4] KVM: arm64/sve: Fix SVE trap restoration for non-current tasks

[Dave Martin]

Commit e6b673b ("KVM: arm64: Optimise FPSIMD handling to reduce
guest/host thrashing") attempts to restore the configuration of
userspace SVE trapping via a call to fpsimd_bind_task_to_cpu(), but
the logic for determining when to do this is not correct.

The patch makes the errnoenous assumption that the only task that
may try to enter userspace with the currently loaded FPSIMD/SVE
register content is current.  This may not be the case however:  if
some other user task T is scheduled on the CPU during the execution
of the KVM run loop, and the vcpu does not try to use the registers
in the meantime, then T's state may be left there intact.  If T
happens to be the next task to enter userspace on this CPU then the
hooks for reloading the register state and configuring traps will
be skipped.

(Also, current never has SVE state at this point anyway and should
always have the trap enabled, as a side-effect of the ioctl()
syscall needed to reach the KVM run loop in the first place.)

This patch instead restores the state of the EL0 trap from the
state observed at the most recent vcpu_load(), ensuring that the
trap is set correctly for the loaded context (if any).

Fixes: e6b673b741ea ("KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing")
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/fpsimd.c           | 24 ++++++++++++++++++++----
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index fda9a8c..fe8777b 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -306,6 +306,7 @@ struct kvm_vcpu_arch {
 #define KVM_ARM64_FP_ENABLED		(1 << 1) /* guest FP regs loaded */
 #define KVM_ARM64_FP_HOST		(1 << 2) /* host FP regs loaded */
 #define KVM_ARM64_HOST_SVE_IN_USE	(1 << 3) /* backup for host TIF_SVE */
+#define KVM_ARM64_HOST_SVE_ENABLED	(1 << 4) /* SVE enabled for EL0 */
 
 #define vcpu_gp_regs(v)		(&(v)->arch.ctxt.gp_regs)
 
diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index b51ff80..30c7a34 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -12,6 +12,7 @@
 #include <asm/kvm_asm.h>
 #include <asm/kvm_host.h>
 #include <asm/kvm_mmu.h>
+#include <asm/sysreg.h>
 
 /*
  * Called on entry to KVM_RUN unless this vcpu previously ran at least
@@ -61,10 +62,16 @@ void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
 {
 	BUG_ON(!current->mm);
 
-	vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED | KVM_ARM64_HOST_SVE_IN_USE);
+	vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
+			      KVM_ARM64_HOST_SVE_IN_USE |
+			      KVM_ARM64_HOST_SVE_ENABLED);
 	vcpu->arch.flags |= KVM_ARM64_FP_HOST;
+
 	if (test_thread_flag(TIF_SVE))
 		vcpu->arch.flags |= KVM_ARM64_HOST_SVE_IN_USE;
+
+	if (read_sysreg(cpacr_el1) & CPACR_EL1_ZEN_EL0EN)
+		vcpu->arch.flags |= KVM_ARM64_HOST_SVE_ENABLED;
 }
 
 /*
@@ -99,9 +106,18 @@ static void __kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
 		/* Clean guest FP state to memory and invalidate cpu view */
 		fpsimd_save();
 		fpsimd_flush_cpu_state();
-	} else if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) {
-		/* Ensure user trap controls are correctly restored */
-		fpsimd_bind_task_to_cpu();
+	} else if (system_supports_sve()) {
+		/*
+		 * The FPSIMD/SVE state in the CPU has not been touched, and we
+		 * have SVE (and VHE): CPACR_EL1 (alias CPTR_EL2) has been
+		 * reset to CPACR_EL1_DEFAULT by the Hyp code, disabling SVE
+		 * for EL0.  To avoid spurious traps, restore the trap state
+		 * seen by kvm_arch_vcpu_load_fp():
+		 */
+		if (vcpu->arch.flags & KVM_ARM64_HOST_SVE_ENABLED)
+			sysreg_clear_set(CPACR_EL1, 0, CPACR_EL1_ZEN_EL0EN);
+		else
+			sysreg_clear_set(CPACR_EL1, CPACR_EL1_ZEN_EL0EN, 0);
 	}
 }
 
-- 
2.1.4

[PATCH 3/4] KVM: arm64/sve: Fix SVE trap restoration for non-current tasks

[Dave Martin]

Commit e6b673b ("KVM: arm64: Optimise FPSIMD handling to reduce
guest/host thrashing") attempts to restore the configuration of
userspace SVE trapping via a call to fpsimd_bind_task_to_cpu(), but
the logic for determining when to do this is not correct.

The patch makes the errnoenous assumption that the only task that
may try to enter userspace with the currently loaded FPSIMD/SVE
register content is current.  This may not be the case however:  if
some other user task T is scheduled on the CPU during the execution
of the KVM run loop, and the vcpu does not try to use the registers
in the meantime, then T's state may be left there intact.  If T
happens to be the next task to enter userspace on this CPU then the
hooks for reloading the register state and configuring traps will
be skipped.

(Also, current never has SVE state at this point anyway and should
always have the trap enabled, as a side-effect of the ioctl()
syscall needed to reach the KVM run loop in the first place.)

This patch instead restores the state of the EL0 trap from the
state observed at the most recent vcpu_load(), ensuring that the
trap is set correctly for the loaded context (if any).

Fixes: e6b673b741ea ("KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing")
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/fpsimd.c           | 24 ++++++++++++++++++++----
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index fda9a8c..fe8777b 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -306,6 +306,7 @@ struct kvm_vcpu_arch {
 #define KVM_ARM64_FP_ENABLED		(1 << 1) /* guest FP regs loaded */
 #define KVM_ARM64_FP_HOST		(1 << 2) /* host FP regs loaded */
 #define KVM_ARM64_HOST_SVE_IN_USE	(1 << 3) /* backup for host TIF_SVE */
+#define KVM_ARM64_HOST_SVE_ENABLED	(1 << 4) /* SVE enabled for EL0 */
 
 #define vcpu_gp_regs(v)		(&(v)->arch.ctxt.gp_regs)
 
diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index b51ff80..30c7a34 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -12,6 +12,7 @@
 #include <asm/kvm_asm.h>
 #include <asm/kvm_host.h>
 #include <asm/kvm_mmu.h>
+#include <asm/sysreg.h>
 
 /*
  * Called on entry to KVM_RUN unless this vcpu previously ran at least
@@ -61,10 +62,16 @@ void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
 {
 	BUG_ON(!current->mm);
 
-	vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED | KVM_ARM64_HOST_SVE_IN_USE);
+	vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
+			      KVM_ARM64_HOST_SVE_IN_USE |
+			      KVM_ARM64_HOST_SVE_ENABLED);
 	vcpu->arch.flags |= KVM_ARM64_FP_HOST;
+
 	if (test_thread_flag(TIF_SVE))
 		vcpu->arch.flags |= KVM_ARM64_HOST_SVE_IN_USE;
+
+	if (read_sysreg(cpacr_el1) & CPACR_EL1_ZEN_EL0EN)
+		vcpu->arch.flags |= KVM_ARM64_HOST_SVE_ENABLED;
 }
 
 /*
@@ -99,9 +106,18 @@ static void __kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
 		/* Clean guest FP state to memory and invalidate cpu view */
 		fpsimd_save();
 		fpsimd_flush_cpu_state();
-	} else if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) {
-		/* Ensure user trap controls are correctly restored */
-		fpsimd_bind_task_to_cpu();
+	} else if (system_supports_sve()) {
+		/*
+		 * The FPSIMD/SVE state in the CPU has not been touched, and we
+		 * have SVE (and VHE): CPACR_EL1 (alias CPTR_EL2) has been
+		 * reset to CPACR_EL1_DEFAULT by the Hyp code, disabling SVE
+		 * for EL0.  To avoid spurious traps, restore the trap state
+		 * seen by kvm_arch_vcpu_load_fp():
+		 */
+		if (vcpu->arch.flags & KVM_ARM64_HOST_SVE_ENABLED)
+			sysreg_clear_set(CPACR_EL1, 0, CPACR_EL1_ZEN_EL0EN);
+		else
+			sysreg_clear_set(CPACR_EL1, CPACR_EL1_ZEN_EL0EN, 0);
 	}
 }
 
-- 
2.1.4

[PATCH 4/4] KVM: arm64: Avoid mistaken attempts to save SVE state for vcpus

[Dave Martin]

Commit e6b673b ("KVM: arm64: Optimise FPSIMD handling to reduce
guest/host thrashing") uses fpsimd_save() to save the FPSIMD state
for a vcpu when scheduling the vcpu out.  However, currently
current's value of TIF_SVE is restored before calling fpsimd_save()
which means that fpsimd_save() may erroneously attempt to save SVE
state from the vcpu.  This enables current's vector state to be
polluted with guest data.  current->thread.sve_state may be
unallocated or not large enough, so this can also trigger a NULL
dereference or buffer overrun.

Instead of this, TIF_SVE should be configured properly for the
guest when calling fpsimd_save() with the vcpu context loaded.

This patch ensures this my delaying restoration of current's
TIF_SVE until after the call to fpsimd_save().

Fixes: e6b673b741ea ("KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing")
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/fpsimd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index 30c7a34..4aaf78e 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -99,9 +99,6 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
  */
 static void __kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
 {
-	update_thread_flag(TIF_SVE,
-			   vcpu->arch.flags & KVM_ARM64_HOST_SVE_IN_USE);
-
 	if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED) {
 		/* Clean guest FP state to memory and invalidate cpu view */
 		fpsimd_save();
@@ -119,6 +116,9 @@ static void __kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
 		else
 			sysreg_clear_set(CPACR_EL1, CPACR_EL1_ZEN_EL0EN, 0);
 	}
+
+	update_thread_flag(TIF_SVE,
+			   vcpu->arch.flags & KVM_ARM64_HOST_SVE_IN_USE);
 }
 
 
-- 
2.1.4

[PATCH 4/4] KVM: arm64: Avoid mistaken attempts to save SVE state for vcpus

[Dave Martin]

Commit e6b673b ("KVM: arm64: Optimise FPSIMD handling to reduce
guest/host thrashing") uses fpsimd_save() to save the FPSIMD state
for a vcpu when scheduling the vcpu out.  However, currently
current's value of TIF_SVE is restored before calling fpsimd_save()
which means that fpsimd_save() may erroneously attempt to save SVE
state from the vcpu.  This enables current's vector state to be
polluted with guest data.  current->thread.sve_state may be
unallocated or not large enough, so this can also trigger a NULL
dereference or buffer overrun.

Instead of this, TIF_SVE should be configured properly for the
guest when calling fpsimd_save() with the vcpu context loaded.

This patch ensures this my delaying restoration of current's
TIF_SVE until after the call to fpsimd_save().

Fixes: e6b673b741ea ("KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing")
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/fpsimd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index 30c7a34..4aaf78e 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -99,9 +99,6 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
  */
 static void __kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
 {
-	update_thread_flag(TIF_SVE,
-			   vcpu->arch.flags & KVM_ARM64_HOST_SVE_IN_USE);
-
 	if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED) {
 		/* Clean guest FP state to memory and invalidate cpu view */
 		fpsimd_save();
@@ -119,6 +116,9 @@ static void __kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
 		else
 			sysreg_clear_set(CPACR_EL1, CPACR_EL1_ZEN_EL0EN, 0);
 	}
+
+	update_thread_flag(TIF_SVE,
+			   vcpu->arch.flags & KVM_ARM64_HOST_SVE_IN_USE);
 }
 
 
-- 
2.1.4

Re: [PATCH 4/4] KVM: arm64: Avoid mistaken attempts to save SVE state for vcpus

[Alex Bennée]

Dave Martin <Dave.Martin@arm.com> writes:
> This patch ensures this my delaying restoration of current's
> TIF_SVE until after the call to fpsimd_save().

this *by* delaying

--
Alex Bennée

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/4] KVM: arm64: Avoid mistaken attempts to save SVE state for vcpus

[Alex Bennée]

Dave Martin <Dave.Martin@arm.com> writes:
> This patch ensures this my delaying restoration of current's
> TIF_SVE until after the call to fpsimd_save().

this *by* delaying

--
Alex Benn?e

Re: [PATCH 4/4] KVM: arm64: Avoid mistaken attempts to save SVE state for vcpus

[Dave Martin]

On Fri, Jun 15, 2018 at 09:50:54AM +0100, Alex Bennée wrote:
> 
> Dave Martin <Dave.Martin@arm.com> writes:
> > This patch ensures this my delaying restoration of current's
> > TIF_SVE until after the call to fpsimd_save().
> 
> this *by* delaying

No Reviewed-my? ;)

I'll repost for the bh_disable/irqsave changes anyway, so I can fix
typos at low effort.

Cheers
---Dave

[PATCH 4/4] KVM: arm64: Avoid mistaken attempts to save SVE state for vcpus

[Dave Martin]

On Fri, Jun 15, 2018 at 09:50:54AM +0100, Alex Benn?e wrote:
> 
> Dave Martin <Dave.Martin@arm.com> writes:
> > This patch ensures this my delaying restoration of current's
> > TIF_SVE until after the call to fpsimd_save().
> 
> this *by* delaying

No Reviewed-my? ;)

I'll repost for the bh_disable/irqsave changes anyway, so I can fix
typos at low effort.

Cheers
---Dave

Re: [PATCH 0/4] KVM: arm64: FPSIMD/SVE fixes for 4.17

[Dave Martin]

On Thu, Jun 14, 2018 at 12:33:55PM +0100, Dave Martin wrote:
> This series contains fixes for some issues observed since the KVM arm64
> pull request for 4.17.

*ahem*, that should read 4.18.

> The first patch (from Mark Rutland) adds a straightforward helper to
> modify particular bits in a system register conditionally.  This is used
> by patch 3 (though obviously it could be coded round and patch 1
> dropped, if preferred).
> 
> See other patches for details of the individual fixes.
> 
> Dave Martin (3):
>   KVM: arm64: Don't mask softirq with IRQs disabled in vcpu_put()
>   KVM: arm64/sve: Fix SVE trap restoration for non-current tasks
>   KVM: arm64: Avoid mistaken attempts to save SVE state for vcpus
> 
> Mark Rutland (1):
>   arm64: introduce sysreg_clear_set()
> 
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/include/asm/sysreg.h   | 11 ++++++++++
>  arch/arm64/kvm/fpsimd.c           | 46 +++++++++++++++++++++++++++++----------
>  3 files changed, 47 insertions(+), 11 deletions(-)
> 
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 0/4] KVM: arm64: FPSIMD/SVE fixes for 4.17

[Dave Martin]

On Thu, Jun 14, 2018 at 12:33:55PM +0100, Dave Martin wrote:
> This series contains fixes for some issues observed since the KVM arm64
> pull request for 4.17.

*ahem*, that should read 4.18.

> The first patch (from Mark Rutland) adds a straightforward helper to
> modify particular bits in a system register conditionally.  This is used
> by patch 3 (though obviously it could be coded round and patch 1
> dropped, if preferred).
> 
> See other patches for details of the individual fixes.
> 
> Dave Martin (3):
>   KVM: arm64: Don't mask softirq with IRQs disabled in vcpu_put()
>   KVM: arm64/sve: Fix SVE trap restoration for non-current tasks
>   KVM: arm64: Avoid mistaken attempts to save SVE state for vcpus
> 
> Mark Rutland (1):
>   arm64: introduce sysreg_clear_set()
> 
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/include/asm/sysreg.h   | 11 ++++++++++
>  arch/arm64/kvm/fpsimd.c           | 46 +++++++++++++++++++++++++++++----------
>  3 files changed, 47 insertions(+), 11 deletions(-)
> 
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel