[PULL 0/4] KVM/ARM Fixes for v4.19

[PULL 0/4] KVM/ARM Fixes for v4.19

[Christoffer Dall]

Hi Radim and Paolo,

Here is the first small round of fixes for KVM/ARM for v4.19.  The
changes include:

 - Fix a VFP corruption in 32-bit guest
 - Add missing cache invalidation for CoW pages
 - Two small cleanups

Note that I have a question out on the CoW cache issue relating to
zero-page backed PMD mappings, but if there is any additional fixes to
be done, that doesn't invalidate the current patch and we can follow up
later.

The following changes since commit 5b394b2ddf0347bef56e50c69a58773c94343ff3:

  Linux 4.19-rc1 (2018-08-26 14:11:59 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git tags/kvm-arm-fixes-for-v4.19

for you to fetch changes up to f901c848464922bdf85a3b993539d56eedd20809:

  arm64: KVM: Remove pgd_lock (2018-08-30 10:08:01 +0200)

---

Marc Zyngier (3):
  KVM: arm/arm64: Clean dcache to PoC when changing PTE due to CoW
  arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD
  KVM: Remove obsolete kvm_unmap_hva notifier backend

Steven Price (1):
  arm64: KVM: Remove pgd_lock

 arch/arm/include/asm/kvm_host.h   |  1 -
 arch/arm64/include/asm/kvm_host.h |  4 +---
 arch/arm64/kvm/hyp/switch.c       |  9 ++++++---
 arch/mips/include/asm/kvm_host.h  |  1 -
 arch/mips/kvm/mmu.c               | 10 ----------
 arch/x86/include/asm/kvm_host.h   |  1 -
 arch/x86/kvm/mmu.c                |  5 -----
 virt/kvm/arm/mmu.c                | 21 ++++++++-------------
 virt/kvm/arm/trace.h              | 15 ---------------
 9 files changed, 15 insertions(+), 52 deletions(-)

-- 
2.18.0

[PULL 0/4] KVM/ARM Fixes for v4.19

[Christoffer Dall]

Hi Radim and Paolo,

Here is the first small round of fixes for KVM/ARM for v4.19.  The
changes include:

 - Fix a VFP corruption in 32-bit guest
 - Add missing cache invalidation for CoW pages
 - Two small cleanups

Note that I have a question out on the CoW cache issue relating to
zero-page backed PMD mappings, but if there is any additional fixes to
be done, that doesn't invalidate the current patch and we can follow up
later.

The following changes since commit 5b394b2ddf0347bef56e50c69a58773c94343ff3:

  Linux 4.19-rc1 (2018-08-26 14:11:59 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git tags/kvm-arm-fixes-for-v4.19

for you to fetch changes up to f901c848464922bdf85a3b993539d56eedd20809:

  arm64: KVM: Remove pgd_lock (2018-08-30 10:08:01 +0200)

---

Marc Zyngier (3):
  KVM: arm/arm64: Clean dcache to PoC when changing PTE due to CoW
  arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD
  KVM: Remove obsolete kvm_unmap_hva notifier backend

Steven Price (1):
  arm64: KVM: Remove pgd_lock

 arch/arm/include/asm/kvm_host.h   |  1 -
 arch/arm64/include/asm/kvm_host.h |  4 +---
 arch/arm64/kvm/hyp/switch.c       |  9 ++++++---
 arch/mips/include/asm/kvm_host.h  |  1 -
 arch/mips/kvm/mmu.c               | 10 ----------
 arch/x86/include/asm/kvm_host.h   |  1 -
 arch/x86/kvm/mmu.c                |  5 -----
 virt/kvm/arm/mmu.c                | 21 ++++++++-------------
 virt/kvm/arm/trace.h              | 15 ---------------
 9 files changed, 15 insertions(+), 52 deletions(-)

-- 
2.18.0

[PULL 1/4] KVM: arm/arm64: Clean dcache to PoC when changing PTE due to CoW

[Christoffer Dall]

From: Marc Zyngier <marc.zyngier@arm.com>

When triggering a CoW, we unmap the RO page via an MMU notifier
(invalidate_range_start), and then populate the new PTE using another
one (change_pte). In the meantime, we'll have copied the old page
into the new one.

The problem is that the data for the new page is sitting in the
cache, and should the guest have an uncached mapping to that page
(or its MMU off), following accesses will bypass the cache.

In a way, this is similar to what happens on a translation fault:
We need to clean the page to the PoC before mapping it. So let's just
do that.

This fixes a KVM unit test regression observed on a HiSilicon platform,
and subsequently reproduced on Seattle.

Fixes: a9c0e12ebee5 ("KVM: arm/arm64: Only clean the dcache on translation fault")
Reported-by: Mike Galbraith <efault@gmx.de>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 virt/kvm/arm/mmu.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
index 91aaf73b00df..111a660be3be 100644
--- a/virt/kvm/arm/mmu.c
+++ b/virt/kvm/arm/mmu.c
@@ -1860,13 +1860,20 @@ static int kvm_set_spte_handler(struct kvm *kvm, gpa_t gpa, u64 size, void *data
 void kvm_set_spte_hva(struct kvm *kvm, unsigned long hva, pte_t pte)
 {
 	unsigned long end = hva + PAGE_SIZE;
+	kvm_pfn_t pfn = pte_pfn(pte);
 	pte_t stage2_pte;
 
 	if (!kvm->arch.pgd)
 		return;
 
 	trace_kvm_set_spte_hva(hva);
-	stage2_pte = pfn_pte(pte_pfn(pte), PAGE_S2);
+
+	/*
+	 * We've moved a page around, probably through CoW, so let's treat it
+	 * just like a translation fault and clean the cache to the PoC.
+	 */
+	clean_dcache_guest_page(pfn, PAGE_SIZE);
+	stage2_pte = pfn_pte(pfn, PAGE_S2);
 	handle_hva_to_gpa(kvm, hva, end, &kvm_set_spte_handler, &stage2_pte);
 }
 
-- 
2.18.0

[PULL 1/4] KVM: arm/arm64: Clean dcache to PoC when changing PTE due to CoW

[Christoffer Dall]

From: Marc Zyngier <marc.zyngier@arm.com>

When triggering a CoW, we unmap the RO page via an MMU notifier
(invalidate_range_start), and then populate the new PTE using another
one (change_pte). In the meantime, we'll have copied the old page
into the new one.

The problem is that the data for the new page is sitting in the
cache, and should the guest have an uncached mapping to that page
(or its MMU off), following accesses will bypass the cache.

In a way, this is similar to what happens on a translation fault:
We need to clean the page to the PoC before mapping it. So let's just
do that.

This fixes a KVM unit test regression observed on a HiSilicon platform,
and subsequently reproduced on Seattle.

Fixes: a9c0e12ebee5 ("KVM: arm/arm64: Only clean the dcache on translation fault")
Reported-by: Mike Galbraith <efault@gmx.de>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 virt/kvm/arm/mmu.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
index 91aaf73b00df..111a660be3be 100644
--- a/virt/kvm/arm/mmu.c
+++ b/virt/kvm/arm/mmu.c
@@ -1860,13 +1860,20 @@ static int kvm_set_spte_handler(struct kvm *kvm, gpa_t gpa, u64 size, void *data
 void kvm_set_spte_hva(struct kvm *kvm, unsigned long hva, pte_t pte)
 {
 	unsigned long end = hva + PAGE_SIZE;
+	kvm_pfn_t pfn = pte_pfn(pte);
 	pte_t stage2_pte;
 
 	if (!kvm->arch.pgd)
 		return;
 
 	trace_kvm_set_spte_hva(hva);
-	stage2_pte = pfn_pte(pte_pfn(pte), PAGE_S2);
+
+	/*
+	 * We've moved a page around, probably through CoW, so let's treat it
+	 * just like a translation fault and clean the cache to the PoC.
+	 */
+	clean_dcache_guest_page(pfn, PAGE_SIZE);
+	stage2_pte = pfn_pte(pfn, PAGE_S2);
 	handle_hva_to_gpa(kvm, hva, end, &kvm_set_spte_handler, &stage2_pte);
 }
 
-- 
2.18.0

[PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD

[Christoffer Dall]

From: Marc Zyngier <marc.zyngier@arm.com>

If trapping FPSIMD in the context of an AArch32 guest, it is critical
to set FPEXC32_EL2.EN to 1 so that the trapping is taken to EL2 and
not EL1.

Conversely, it is just as critical *not* to set FPEXC32_EL2.EN to 1
if we're not going to trap FPSIMD, as we then corrupt the existing
VFP state.

Moving the call to __activate_traps_fpsimd32 to the point where we
know for sure that we are going to trap ensures that we don't set that
bit spuriously.

Fixes: e6b673b741ea ("KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing")
Cc: Dave Martin <dave.martin@arm.com>
Reported-by: Alexander Graf <agraf@suse.de>
Tested-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 arch/arm64/kvm/hyp/switch.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index d496ef579859..ca46153d7915 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -98,8 +98,10 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
 	val = read_sysreg(cpacr_el1);
 	val |= CPACR_EL1_TTA;
 	val &= ~CPACR_EL1_ZEN;
-	if (!update_fp_enabled(vcpu))
+	if (!update_fp_enabled(vcpu)) {
 		val &= ~CPACR_EL1_FPEN;
+		__activate_traps_fpsimd32(vcpu);
+	}
 
 	write_sysreg(val, cpacr_el1);
 
@@ -114,8 +116,10 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
 
 	val = CPTR_EL2_DEFAULT;
 	val |= CPTR_EL2_TTA | CPTR_EL2_TZ;
-	if (!update_fp_enabled(vcpu))
+	if (!update_fp_enabled(vcpu)) {
 		val |= CPTR_EL2_TFP;
+		__activate_traps_fpsimd32(vcpu);
+	}
 
 	write_sysreg(val, cptr_el2);
 }
@@ -129,7 +133,6 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
 	if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && (hcr & HCR_VSE))
 		write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2);
 
-	__activate_traps_fpsimd32(vcpu);
 	if (has_vhe())
 		activate_traps_vhe(vcpu);
 	else
-- 
2.18.0

[PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD

[Christoffer Dall]

From: Marc Zyngier <marc.zyngier@arm.com>

If trapping FPSIMD in the context of an AArch32 guest, it is critical
to set FPEXC32_EL2.EN to 1 so that the trapping is taken to EL2 and
not EL1.

Conversely, it is just as critical *not* to set FPEXC32_EL2.EN to 1
if we're not going to trap FPSIMD, as we then corrupt the existing
VFP state.

Moving the call to __activate_traps_fpsimd32 to the point where we
know for sure that we are going to trap ensures that we don't set that
bit spuriously.

Fixes: e6b673b741ea ("KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing")
Cc: Dave Martin <dave.martin@arm.com>
Reported-by: Alexander Graf <agraf@suse.de>
Tested-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 arch/arm64/kvm/hyp/switch.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index d496ef579859..ca46153d7915 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -98,8 +98,10 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
 	val = read_sysreg(cpacr_el1);
 	val |= CPACR_EL1_TTA;
 	val &= ~CPACR_EL1_ZEN;
-	if (!update_fp_enabled(vcpu))
+	if (!update_fp_enabled(vcpu)) {
 		val &= ~CPACR_EL1_FPEN;
+		__activate_traps_fpsimd32(vcpu);
+	}
 
 	write_sysreg(val, cpacr_el1);
 
@@ -114,8 +116,10 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
 
 	val = CPTR_EL2_DEFAULT;
 	val |= CPTR_EL2_TTA | CPTR_EL2_TZ;
-	if (!update_fp_enabled(vcpu))
+	if (!update_fp_enabled(vcpu)) {
 		val |= CPTR_EL2_TFP;
+		__activate_traps_fpsimd32(vcpu);
+	}
 
 	write_sysreg(val, cptr_el2);
 }
@@ -129,7 +133,6 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
 	if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && (hcr & HCR_VSE))
 		write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2);
 
-	__activate_traps_fpsimd32(vcpu);
 	if (has_vhe())
 		activate_traps_vhe(vcpu);
 	else
-- 
2.18.0

[PULL 3/4] KVM: Remove obsolete kvm_unmap_hva notifier backend

[Christoffer Dall]

From: Marc Zyngier <marc.zyngier@arm.com>

kvm_unmap_hva is long gone, and we only have kvm_unmap_hva_range to
deal with. Drop the now obsolete code.

Fixes: fb1522e099f0 ("KVM: update to new mmu_notifier semantic v2")
Cc: James Hogan <jhogan@kernel.org>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 arch/arm/include/asm/kvm_host.h   |  1 -
 arch/arm64/include/asm/kvm_host.h |  1 -
 arch/mips/include/asm/kvm_host.h  |  1 -
 arch/mips/kvm/mmu.c               | 10 ----------
 arch/x86/include/asm/kvm_host.h   |  1 -
 arch/x86/kvm/mmu.c                |  5 -----
 virt/kvm/arm/mmu.c                | 12 ------------
 virt/kvm/arm/trace.h              | 15 ---------------
 8 files changed, 46 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 79906cecb091..3ad482d2f1eb 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -223,7 +223,6 @@ int __kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
 			      struct kvm_vcpu_events *events);
 
 #define KVM_ARCH_WANT_MMU_NOTIFIER
-int kvm_unmap_hva(struct kvm *kvm, unsigned long hva);
 int kvm_unmap_hva_range(struct kvm *kvm,
 			unsigned long start, unsigned long end);
 void kvm_set_spte_hva(struct kvm *kvm, unsigned long hva, pte_t pte);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index f26055f2306e..8e6d46df38aa 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -357,7 +357,6 @@ int __kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
 			      struct kvm_vcpu_events *events);
 
 #define KVM_ARCH_WANT_MMU_NOTIFIER
-int kvm_unmap_hva(struct kvm *kvm, unsigned long hva);
 int kvm_unmap_hva_range(struct kvm *kvm,
 			unsigned long start, unsigned long end);
 void kvm_set_spte_hva(struct kvm *kvm, unsigned long hva, pte_t pte);
diff --git a/arch/mips/include/asm/kvm_host.h b/arch/mips/include/asm/kvm_host.h
index a9af1d2dcd69..2c1c53d12179 100644
--- a/arch/mips/include/asm/kvm_host.h
+++ b/arch/mips/include/asm/kvm_host.h
@@ -931,7 +931,6 @@ enum kvm_mips_fault_result kvm_trap_emul_gva_fault(struct kvm_vcpu *vcpu,
 						   bool write);
 
 #define KVM_ARCH_WANT_MMU_NOTIFIER
-int kvm_unmap_hva(struct kvm *kvm, unsigned long hva);
 int kvm_unmap_hva_range(struct kvm *kvm,
 			unsigned long start, unsigned long end);
 void kvm_set_spte_hva(struct kvm *kvm, unsigned long hva, pte_t pte);
diff --git a/arch/mips/kvm/mmu.c b/arch/mips/kvm/mmu.c
index ee64db032793..d8dcdb350405 100644
--- a/arch/mips/kvm/mmu.c
+++ b/arch/mips/kvm/mmu.c
@@ -512,16 +512,6 @@ static int kvm_unmap_hva_handler(struct kvm *kvm, gfn_t gfn, gfn_t gfn_end,
 	return 1;
 }
 
-int kvm_unmap_hva(struct kvm *kvm, unsigned long hva)
-{
-	unsigned long end = hva + PAGE_SIZE;
-
-	handle_hva_to_gpa(kvm, hva, end, &kvm_unmap_hva_handler, NULL);
-
-	kvm_mips_callbacks->flush_shadow_all(kvm);
-	return 0;
-}
-
 int kvm_unmap_hva_range(struct kvm *kvm, unsigned long start, unsigned long end)
 {
 	handle_hva_to_gpa(kvm, start, end, &kvm_unmap_hva_handler, NULL);
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 00ddb0c9e612..e6a33420b871 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1450,7 +1450,6 @@ asmlinkage void kvm_spurious_fault(void);
 	____kvm_handle_fault_on_reboot(insn, "")
 
 #define KVM_ARCH_WANT_MMU_NOTIFIER
-int kvm_unmap_hva(struct kvm *kvm, unsigned long hva);
 int kvm_unmap_hva_range(struct kvm *kvm, unsigned long start, unsigned long end);
 int kvm_age_hva(struct kvm *kvm, unsigned long start, unsigned long end);
 int kvm_test_age_hva(struct kvm *kvm, unsigned long hva);
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index a282321329b5..d440154e8938 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1853,11 +1853,6 @@ static int kvm_handle_hva(struct kvm *kvm, unsigned long hva,
 	return kvm_handle_hva_range(kvm, hva, hva + 1, data, handler);
 }
 
-int kvm_unmap_hva(struct kvm *kvm, unsigned long hva)
-{
-	return kvm_handle_hva(kvm, hva, 0, kvm_unmap_rmapp);
-}
-
 int kvm_unmap_hva_range(struct kvm *kvm, unsigned long start, unsigned long end)
 {
 	return kvm_handle_hva_range(kvm, start, end, 0, kvm_unmap_rmapp);
diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
index 111a660be3be..ed162a6c57c5 100644
--- a/virt/kvm/arm/mmu.c
+++ b/virt/kvm/arm/mmu.c
@@ -1817,18 +1817,6 @@ static int kvm_unmap_hva_handler(struct kvm *kvm, gpa_t gpa, u64 size, void *dat
 	return 0;
 }
 
-int kvm_unmap_hva(struct kvm *kvm, unsigned long hva)
-{
-	unsigned long end = hva + PAGE_SIZE;
-
-	if (!kvm->arch.pgd)
-		return 0;
-
-	trace_kvm_unmap_hva(hva);
-	handle_hva_to_gpa(kvm, hva, end, &kvm_unmap_hva_handler, NULL);
-	return 0;
-}
-
 int kvm_unmap_hva_range(struct kvm *kvm,
 			unsigned long start, unsigned long end)
 {
diff --git a/virt/kvm/arm/trace.h b/virt/kvm/arm/trace.h
index e53b596f483b..57b3edebbb40 100644
--- a/virt/kvm/arm/trace.h
+++ b/virt/kvm/arm/trace.h
@@ -134,21 +134,6 @@ TRACE_EVENT(kvm_mmio_emulate,
 		  __entry->vcpu_pc, __entry->instr, __entry->cpsr)
 );
 
-TRACE_EVENT(kvm_unmap_hva,
-	TP_PROTO(unsigned long hva),
-	TP_ARGS(hva),
-
-	TP_STRUCT__entry(
-		__field(	unsigned long,	hva		)
-	),
-
-	TP_fast_assign(
-		__entry->hva		= hva;
-	),
-
-	TP_printk("mmu notifier unmap hva: %#08lx", __entry->hva)
-);
-
 TRACE_EVENT(kvm_unmap_hva_range,
 	TP_PROTO(unsigned long start, unsigned long end),
 	TP_ARGS(start, end),
-- 
2.18.0

[PULL 3/4] KVM: Remove obsolete kvm_unmap_hva notifier backend

[Christoffer Dall]

From: Marc Zyngier <marc.zyngier@arm.com>

kvm_unmap_hva is long gone, and we only have kvm_unmap_hva_range to
deal with. Drop the now obsolete code.

Fixes: fb1522e099f0 ("KVM: update to new mmu_notifier semantic v2")
Cc: James Hogan <jhogan@kernel.org>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 arch/arm/include/asm/kvm_host.h   |  1 -
 arch/arm64/include/asm/kvm_host.h |  1 -
 arch/mips/include/asm/kvm_host.h  |  1 -
 arch/mips/kvm/mmu.c               | 10 ----------
 arch/x86/include/asm/kvm_host.h   |  1 -
 arch/x86/kvm/mmu.c                |  5 -----
 virt/kvm/arm/mmu.c                | 12 ------------
 virt/kvm/arm/trace.h              | 15 ---------------
 8 files changed, 46 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 79906cecb091..3ad482d2f1eb 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -223,7 +223,6 @@ int __kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
 			      struct kvm_vcpu_events *events);
 
 #define KVM_ARCH_WANT_MMU_NOTIFIER
-int kvm_unmap_hva(struct kvm *kvm, unsigned long hva);
 int kvm_unmap_hva_range(struct kvm *kvm,
 			unsigned long start, unsigned long end);
 void kvm_set_spte_hva(struct kvm *kvm, unsigned long hva, pte_t pte);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index f26055f2306e..8e6d46df38aa 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -357,7 +357,6 @@ int __kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
 			      struct kvm_vcpu_events *events);
 
 #define KVM_ARCH_WANT_MMU_NOTIFIER
-int kvm_unmap_hva(struct kvm *kvm, unsigned long hva);
 int kvm_unmap_hva_range(struct kvm *kvm,
 			unsigned long start, unsigned long end);
 void kvm_set_spte_hva(struct kvm *kvm, unsigned long hva, pte_t pte);
diff --git a/arch/mips/include/asm/kvm_host.h b/arch/mips/include/asm/kvm_host.h
index a9af1d2dcd69..2c1c53d12179 100644
--- a/arch/mips/include/asm/kvm_host.h
+++ b/arch/mips/include/asm/kvm_host.h
@@ -931,7 +931,6 @@ enum kvm_mips_fault_result kvm_trap_emul_gva_fault(struct kvm_vcpu *vcpu,
 						   bool write);
 
 #define KVM_ARCH_WANT_MMU_NOTIFIER
-int kvm_unmap_hva(struct kvm *kvm, unsigned long hva);
 int kvm_unmap_hva_range(struct kvm *kvm,
 			unsigned long start, unsigned long end);
 void kvm_set_spte_hva(struct kvm *kvm, unsigned long hva, pte_t pte);
diff --git a/arch/mips/kvm/mmu.c b/arch/mips/kvm/mmu.c
index ee64db032793..d8dcdb350405 100644
--- a/arch/mips/kvm/mmu.c
+++ b/arch/mips/kvm/mmu.c
@@ -512,16 +512,6 @@ static int kvm_unmap_hva_handler(struct kvm *kvm, gfn_t gfn, gfn_t gfn_end,
 	return 1;
 }
 
-int kvm_unmap_hva(struct kvm *kvm, unsigned long hva)
-{
-	unsigned long end = hva + PAGE_SIZE;
-
-	handle_hva_to_gpa(kvm, hva, end, &kvm_unmap_hva_handler, NULL);
-
-	kvm_mips_callbacks->flush_shadow_all(kvm);
-	return 0;
-}
-
 int kvm_unmap_hva_range(struct kvm *kvm, unsigned long start, unsigned long end)
 {
 	handle_hva_to_gpa(kvm, start, end, &kvm_unmap_hva_handler, NULL);
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 00ddb0c9e612..e6a33420b871 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1450,7 +1450,6 @@ asmlinkage void kvm_spurious_fault(void);
 	____kvm_handle_fault_on_reboot(insn, "")
 
 #define KVM_ARCH_WANT_MMU_NOTIFIER
-int kvm_unmap_hva(struct kvm *kvm, unsigned long hva);
 int kvm_unmap_hva_range(struct kvm *kvm, unsigned long start, unsigned long end);
 int kvm_age_hva(struct kvm *kvm, unsigned long start, unsigned long end);
 int kvm_test_age_hva(struct kvm *kvm, unsigned long hva);
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index a282321329b5..d440154e8938 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1853,11 +1853,6 @@ static int kvm_handle_hva(struct kvm *kvm, unsigned long hva,
 	return kvm_handle_hva_range(kvm, hva, hva + 1, data, handler);
 }
 
-int kvm_unmap_hva(struct kvm *kvm, unsigned long hva)
-{
-	return kvm_handle_hva(kvm, hva, 0, kvm_unmap_rmapp);
-}
-
 int kvm_unmap_hva_range(struct kvm *kvm, unsigned long start, unsigned long end)
 {
 	return kvm_handle_hva_range(kvm, start, end, 0, kvm_unmap_rmapp);
diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
index 111a660be3be..ed162a6c57c5 100644
--- a/virt/kvm/arm/mmu.c
+++ b/virt/kvm/arm/mmu.c
@@ -1817,18 +1817,6 @@ static int kvm_unmap_hva_handler(struct kvm *kvm, gpa_t gpa, u64 size, void *dat
 	return 0;
 }
 
-int kvm_unmap_hva(struct kvm *kvm, unsigned long hva)
-{
-	unsigned long end = hva + PAGE_SIZE;
-
-	if (!kvm->arch.pgd)
-		return 0;
-
-	trace_kvm_unmap_hva(hva);
-	handle_hva_to_gpa(kvm, hva, end, &kvm_unmap_hva_handler, NULL);
-	return 0;
-}
-
 int kvm_unmap_hva_range(struct kvm *kvm,
 			unsigned long start, unsigned long end)
 {
diff --git a/virt/kvm/arm/trace.h b/virt/kvm/arm/trace.h
index e53b596f483b..57b3edebbb40 100644
--- a/virt/kvm/arm/trace.h
+++ b/virt/kvm/arm/trace.h
@@ -134,21 +134,6 @@ TRACE_EVENT(kvm_mmio_emulate,
 		  __entry->vcpu_pc, __entry->instr, __entry->cpsr)
 );
 
-TRACE_EVENT(kvm_unmap_hva,
-	TP_PROTO(unsigned long hva),
-	TP_ARGS(hva),
-
-	TP_STRUCT__entry(
-		__field(	unsigned long,	hva		)
-	),
-
-	TP_fast_assign(
-		__entry->hva		= hva;
-	),
-
-	TP_printk("mmu notifier unmap hva: %#08lx", __entry->hva)
-);
-
 TRACE_EVENT(kvm_unmap_hva_range,
 	TP_PROTO(unsigned long start, unsigned long end),
 	TP_ARGS(start, end),
-- 
2.18.0

[PULL 4/4] arm64: KVM: Remove pgd_lock

[Christoffer Dall]

From: Steven Price <steven.price@arm.com>

The lock has never been used and the page tables are protected by
mmu_lock in struct kvm.

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Steven Price <steven.price@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 arch/arm64/include/asm/kvm_host.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 8e6d46df38aa..3d6d7336f871 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -61,8 +61,7 @@ struct kvm_arch {
 	u64    vmid_gen;
 	u32    vmid;
 
-	/* 1-level 2nd stage table and lock */
-	spinlock_t pgd_lock;
+	/* 1-level 2nd stage table, protected by kvm->mmu_lock */
 	pgd_t *pgd;
 
 	/* VTTBR value associated with above pgd and vmid */
-- 
2.18.0

[PULL 4/4] arm64: KVM: Remove pgd_lock

[Christoffer Dall]

From: Steven Price <steven.price@arm.com>

The lock has never been used and the page tables are protected by
mmu_lock in struct kvm.

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Steven Price <steven.price@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
---
 arch/arm64/include/asm/kvm_host.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 8e6d46df38aa..3d6d7336f871 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -61,8 +61,7 @@ struct kvm_arch {
 	u64    vmid_gen;
 	u32    vmid;
 
-	/* 1-level 2nd stage table and lock */
-	spinlock_t pgd_lock;
+	/* 1-level 2nd stage table, protected by kvm->mmu_lock */
 	pgd_t *pgd;
 
 	/* VTTBR value associated with above pgd and vmid */
-- 
2.18.0

Re: [PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD

[Dave Martin]

On Mon, Sep 03, 2018 at 03:45:14PM +0200, Christoffer Dall wrote:
> From: Marc Zyngier <marc.zyngier@arm.com>
> 
> If trapping FPSIMD in the context of an AArch32 guest, it is critical
> to set FPEXC32_EL2.EN to 1 so that the trapping is taken to EL2 and
> not EL1.
> 
> Conversely, it is just as critical *not* to set FPEXC32_EL2.EN to 1
> if we're not going to trap FPSIMD, as we then corrupt the existing
> VFP state.
> 
> Moving the call to __activate_traps_fpsimd32 to the point where we
> know for sure that we are going to trap ensures that we don't set that
> bit spuriously.


Hmmm, this looks like a viable fix but actually the way the code is
structured is a bit confusing here.

It looks like some unnecessary functionality has been inherited from
arch/arm here: for a 32-bit host, FPEXC controls FPSIMD trapping even
at Hyp, so we need to handle it specially in order for the Hyp FPSIMD
context switching code to run without generating additional traps.

For a 64-bit host, this doesn't apply: FPEXC32_EL2 is just guest
context and doesn't affect EL2 execution.

The wrinkle this patch fixes is that implicit reads of FPEXC by
the guest are in effect not trappable, so if this register is wrongly
configured for the guest, spurious traps to EL1 may occur that are
not architecturally expected by the guest, which is precisely the
problem that Alexander hit.  I missed this issue in my previous
refactoring...


So, maybe FPEXC32_EL2 should just be treated as another guest system
register that must be eagerly switched.

This would also avoid the double write of FPEXC32_EL2 that we currently
do (once when enabling traps, then again when writing the correct guest
value after a trap is taken), and the isb() can be removed too since
host FPSIMD accesses won't trap on arm64 irrespective of FPEXC32_EL2.


I'll try to cook up a cleanup patch to apply on top, but it can be
treated as non-urgent.

Cheers
---Dave

> 
> Fixes: e6b673b741ea ("KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing")
> Cc: Dave Martin <dave.martin@arm.com>
> Reported-by: Alexander Graf <agraf@suse.de>
> Tested-by: Alexander Graf <agraf@suse.de>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  arch/arm64/kvm/hyp/switch.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index d496ef579859..ca46153d7915 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -98,8 +98,10 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
>  	val = read_sysreg(cpacr_el1);
>  	val |= CPACR_EL1_TTA;
>  	val &= ~CPACR_EL1_ZEN;
> -	if (!update_fp_enabled(vcpu))
> +	if (!update_fp_enabled(vcpu)) {
>  		val &= ~CPACR_EL1_FPEN;
> +		__activate_traps_fpsimd32(vcpu);
> +	}
>  
>  	write_sysreg(val, cpacr_el1);
>  
> @@ -114,8 +116,10 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
>  
>  	val = CPTR_EL2_DEFAULT;
>  	val |= CPTR_EL2_TTA | CPTR_EL2_TZ;
> -	if (!update_fp_enabled(vcpu))
> +	if (!update_fp_enabled(vcpu)) {
>  		val |= CPTR_EL2_TFP;
> +		__activate_traps_fpsimd32(vcpu);
> +	}
>  
>  	write_sysreg(val, cptr_el2);
>  }
> @@ -129,7 +133,6 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
>  	if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && (hcr & HCR_VSE))
>  		write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2);
>  
> -	__activate_traps_fpsimd32(vcpu);
>  	if (has_vhe())
>  		activate_traps_vhe(vcpu);
>  	else
> -- 
> 2.18.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD

[Dave Martin]

On Mon, Sep 03, 2018 at 03:45:14PM +0200, Christoffer Dall wrote:
> From: Marc Zyngier <marc.zyngier@arm.com>
> 
> If trapping FPSIMD in the context of an AArch32 guest, it is critical
> to set FPEXC32_EL2.EN to 1 so that the trapping is taken to EL2 and
> not EL1.
> 
> Conversely, it is just as critical *not* to set FPEXC32_EL2.EN to 1
> if we're not going to trap FPSIMD, as we then corrupt the existing
> VFP state.
> 
> Moving the call to __activate_traps_fpsimd32 to the point where we
> know for sure that we are going to trap ensures that we don't set that
> bit spuriously.


Hmmm, this looks like a viable fix but actually the way the code is
structured is a bit confusing here.

It looks like some unnecessary functionality has been inherited from
arch/arm here: for a 32-bit host, FPEXC controls FPSIMD trapping even
at Hyp, so we need to handle it specially in order for the Hyp FPSIMD
context switching code to run without generating additional traps.

For a 64-bit host, this doesn't apply: FPEXC32_EL2 is just guest
context and doesn't affect EL2 execution.

The wrinkle this patch fixes is that implicit reads of FPEXC by
the guest are in effect not trappable, so if this register is wrongly
configured for the guest, spurious traps to EL1 may occur that are
not architecturally expected by the guest, which is precisely the
problem that Alexander hit.  I missed this issue in my previous
refactoring...


So, maybe FPEXC32_EL2 should just be treated as another guest system
register that must be eagerly switched.

This would also avoid the double write of FPEXC32_EL2 that we currently
do (once when enabling traps, then again when writing the correct guest
value after a trap is taken), and the isb() can be removed too since
host FPSIMD accesses won't trap on arm64 irrespective of FPEXC32_EL2.


I'll try to cook up a cleanup patch to apply on top, but it can be
treated as non-urgent.

Cheers
---Dave

> 
> Fixes: e6b673b741ea ("KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing")
> Cc: Dave Martin <dave.martin@arm.com>
> Reported-by: Alexander Graf <agraf@suse.de>
> Tested-by: Alexander Graf <agraf@suse.de>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
>  arch/arm64/kvm/hyp/switch.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index d496ef579859..ca46153d7915 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -98,8 +98,10 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
>  	val = read_sysreg(cpacr_el1);
>  	val |= CPACR_EL1_TTA;
>  	val &= ~CPACR_EL1_ZEN;
> -	if (!update_fp_enabled(vcpu))
> +	if (!update_fp_enabled(vcpu)) {
>  		val &= ~CPACR_EL1_FPEN;
> +		__activate_traps_fpsimd32(vcpu);
> +	}
>  
>  	write_sysreg(val, cpacr_el1);
>  
> @@ -114,8 +116,10 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
>  
>  	val = CPTR_EL2_DEFAULT;
>  	val |= CPTR_EL2_TTA | CPTR_EL2_TZ;
> -	if (!update_fp_enabled(vcpu))
> +	if (!update_fp_enabled(vcpu)) {
>  		val |= CPTR_EL2_TFP;
> +		__activate_traps_fpsimd32(vcpu);
> +	}
>  
>  	write_sysreg(val, cptr_el2);
>  }
> @@ -129,7 +133,6 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
>  	if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && (hcr & HCR_VSE))
>  		write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2);
>  
> -	__activate_traps_fpsimd32(vcpu);
>  	if (has_vhe())
>  		activate_traps_vhe(vcpu);
>  	else
> -- 
> 2.18.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD

[Christoffer Dall]

On Tue, Sep 04, 2018 at 12:39:54PM +0100, Dave Martin wrote:
> On Mon, Sep 03, 2018 at 03:45:14PM +0200, Christoffer Dall wrote:
> > From: Marc Zyngier <marc.zyngier@arm.com>
> > 
> > If trapping FPSIMD in the context of an AArch32 guest, it is critical
> > to set FPEXC32_EL2.EN to 1 so that the trapping is taken to EL2 and
> > not EL1.
> > 
> > Conversely, it is just as critical *not* to set FPEXC32_EL2.EN to 1
> > if we're not going to trap FPSIMD, as we then corrupt the existing
> > VFP state.
> > 
> > Moving the call to __activate_traps_fpsimd32 to the point where we
> > know for sure that we are going to trap ensures that we don't set that
> > bit spuriously.
> 
> 
> Hmmm, this looks like a viable fix but actually the way the code is
> structured is a bit confusing here.
> 
> It looks like some unnecessary functionality has been inherited from
> arch/arm here: for a 32-bit host, FPEXC controls FPSIMD trapping even
> at Hyp, so we need to handle it specially in order for the Hyp FPSIMD
> context switching code to run without generating additional traps.
> 
> For a 64-bit host, this doesn't apply: FPEXC32_EL2 is just guest
> context and doesn't affect EL2 execution.

Hmm, is that really true?  Earlier versions of the Armv8 Arm ARM, for
the description of CPTR_EL2.TFP:

  This causes instructions that access the registers associated with
  Floating Point and Advanced SIMD execution to trap to EL2 when
  executed from EL0, EL1, or EL2, unless trapped to EL1.

In the current version I think you can arrive at the same conclusion by
looking at the FPEXC_EL2.EN description which tells you that if EN==0,
then the instruction is UNDEFINED, and the Synchronous exception
prioritization rules specify that undefined instructions are taken
before trapped intructions because of CPTR_EL2.

But there does seem to be some inconsistency with the description of
generally trapping floating point access to EL2.

> 
> The wrinkle this patch fixes is that implicit reads of FPEXC by

what do you mean by implicit reads here?

> the guest are in effect not trappable, so if this register is wrongly
> configured for the guest, spurious traps to EL1 may occur that are
> not architecturally expected by the guest, which is precisely the
> problem that Alexander hit.  I missed this issue in my previous
> refactoring...
> 

Thanks,

    Christoffer

> 
> So, maybe FPEXC32_EL2 should just be treated as another guest system
> register that must be eagerly switched.
> 
> This would also avoid the double write of FPEXC32_EL2 that we currently
> do (once when enabling traps, then again when writing the correct guest
> value after a trap is taken), and the isb() can be removed too since
> host FPSIMD accesses won't trap on arm64 irrespective of FPEXC32_EL2.
> 
> 
> I'll try to cook up a cleanup patch to apply on top, but it can be
> treated as non-urgent.
> 
> Cheers
> ---Dave
> 
> > 
> > Fixes: e6b673b741ea ("KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing")
> > Cc: Dave Martin <dave.martin@arm.com>
> > Reported-by: Alexander Graf <agraf@suse.de>
> > Tested-by: Alexander Graf <agraf@suse.de>
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > ---
> >  arch/arm64/kvm/hyp/switch.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> > index d496ef579859..ca46153d7915 100644
> > --- a/arch/arm64/kvm/hyp/switch.c
> > +++ b/arch/arm64/kvm/hyp/switch.c
> > @@ -98,8 +98,10 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
> >  	val = read_sysreg(cpacr_el1);
> >  	val |= CPACR_EL1_TTA;
> >  	val &= ~CPACR_EL1_ZEN;
> > -	if (!update_fp_enabled(vcpu))
> > +	if (!update_fp_enabled(vcpu)) {
> >  		val &= ~CPACR_EL1_FPEN;
> > +		__activate_traps_fpsimd32(vcpu);
> > +	}
> >  
> >  	write_sysreg(val, cpacr_el1);
> >  
> > @@ -114,8 +116,10 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
> >  
> >  	val = CPTR_EL2_DEFAULT;
> >  	val |= CPTR_EL2_TTA | CPTR_EL2_TZ;
> > -	if (!update_fp_enabled(vcpu))
> > +	if (!update_fp_enabled(vcpu)) {
> >  		val |= CPTR_EL2_TFP;
> > +		__activate_traps_fpsimd32(vcpu);
> > +	}
> >  
> >  	write_sysreg(val, cptr_el2);
> >  }
> > @@ -129,7 +133,6 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
> >  	if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && (hcr & HCR_VSE))
> >  		write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2);
> >  
> > -	__activate_traps_fpsimd32(vcpu);
> >  	if (has_vhe())
> >  		activate_traps_vhe(vcpu);
> >  	else
> > -- 
> > 2.18.0
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD

[Christoffer Dall]

On Tue, Sep 04, 2018 at 12:39:54PM +0100, Dave Martin wrote:
> On Mon, Sep 03, 2018 at 03:45:14PM +0200, Christoffer Dall wrote:
> > From: Marc Zyngier <marc.zyngier@arm.com>
> > 
> > If trapping FPSIMD in the context of an AArch32 guest, it is critical
> > to set FPEXC32_EL2.EN to 1 so that the trapping is taken to EL2 and
> > not EL1.
> > 
> > Conversely, it is just as critical *not* to set FPEXC32_EL2.EN to 1
> > if we're not going to trap FPSIMD, as we then corrupt the existing
> > VFP state.
> > 
> > Moving the call to __activate_traps_fpsimd32 to the point where we
> > know for sure that we are going to trap ensures that we don't set that
> > bit spuriously.
> 
> 
> Hmmm, this looks like a viable fix but actually the way the code is
> structured is a bit confusing here.
> 
> It looks like some unnecessary functionality has been inherited from
> arch/arm here: for a 32-bit host, FPEXC controls FPSIMD trapping even
> at Hyp, so we need to handle it specially in order for the Hyp FPSIMD
> context switching code to run without generating additional traps.
> 
> For a 64-bit host, this doesn't apply: FPEXC32_EL2 is just guest
> context and doesn't affect EL2 execution.

Hmm, is that really true?  Earlier versions of the Armv8 Arm ARM, for
the description of CPTR_EL2.TFP:

  This causes instructions that access the registers associated with
  Floating Point and Advanced SIMD execution to trap to EL2 when
  executed from EL0, EL1, or EL2, unless trapped to EL1.

In the current version I think you can arrive at the same conclusion by
looking at the FPEXC_EL2.EN description which tells you that if EN==0,
then the instruction is UNDEFINED, and the Synchronous exception
prioritization rules specify that undefined instructions are taken
before trapped intructions because of CPTR_EL2.

But there does seem to be some inconsistency with the description of
generally trapping floating point access to EL2.

> 
> The wrinkle this patch fixes is that implicit reads of FPEXC by

what do you mean by implicit reads here?

> the guest are in effect not trappable, so if this register is wrongly
> configured for the guest, spurious traps to EL1 may occur that are
> not architecturally expected by the guest, which is precisely the
> problem that Alexander hit.  I missed this issue in my previous
> refactoring...
> 

Thanks,

    Christoffer

> 
> So, maybe FPEXC32_EL2 should just be treated as another guest system
> register that must be eagerly switched.
> 
> This would also avoid the double write of FPEXC32_EL2 that we currently
> do (once when enabling traps, then again when writing the correct guest
> value after a trap is taken), and the isb() can be removed too since
> host FPSIMD accesses won't trap on arm64 irrespective of FPEXC32_EL2.
> 
> 
> I'll try to cook up a cleanup patch to apply on top, but it can be
> treated as non-urgent.
> 
> Cheers
> ---Dave
> 
> > 
> > Fixes: e6b673b741ea ("KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing")
> > Cc: Dave Martin <dave.martin@arm.com>
> > Reported-by: Alexander Graf <agraf@suse.de>
> > Tested-by: Alexander Graf <agraf@suse.de>
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > ---
> >  arch/arm64/kvm/hyp/switch.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> > index d496ef579859..ca46153d7915 100644
> > --- a/arch/arm64/kvm/hyp/switch.c
> > +++ b/arch/arm64/kvm/hyp/switch.c
> > @@ -98,8 +98,10 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
> >  	val = read_sysreg(cpacr_el1);
> >  	val |= CPACR_EL1_TTA;
> >  	val &= ~CPACR_EL1_ZEN;
> > -	if (!update_fp_enabled(vcpu))
> > +	if (!update_fp_enabled(vcpu)) {
> >  		val &= ~CPACR_EL1_FPEN;
> > +		__activate_traps_fpsimd32(vcpu);
> > +	}
> >  
> >  	write_sysreg(val, cpacr_el1);
> >  
> > @@ -114,8 +116,10 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
> >  
> >  	val = CPTR_EL2_DEFAULT;
> >  	val |= CPTR_EL2_TTA | CPTR_EL2_TZ;
> > -	if (!update_fp_enabled(vcpu))
> > +	if (!update_fp_enabled(vcpu)) {
> >  		val |= CPTR_EL2_TFP;
> > +		__activate_traps_fpsimd32(vcpu);
> > +	}
> >  
> >  	write_sysreg(val, cptr_el2);
> >  }
> > @@ -129,7 +133,6 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
> >  	if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && (hcr & HCR_VSE))
> >  		write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2);
> >  
> > -	__activate_traps_fpsimd32(vcpu);
> >  	if (has_vhe())
> >  		activate_traps_vhe(vcpu);
> >  	else
> > -- 
> > 2.18.0
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD

[Dave Martin]

On Tue, Sep 04, 2018 at 02:51:21PM +0200, Christoffer Dall wrote:
> On Tue, Sep 04, 2018 at 12:39:54PM +0100, Dave Martin wrote:
> > On Mon, Sep 03, 2018 at 03:45:14PM +0200, Christoffer Dall wrote:
> > > From: Marc Zyngier <marc.zyngier@arm.com>
> > > 
> > > If trapping FPSIMD in the context of an AArch32 guest, it is critical
> > > to set FPEXC32_EL2.EN to 1 so that the trapping is taken to EL2 and
> > > not EL1.
> > > 
> > > Conversely, it is just as critical *not* to set FPEXC32_EL2.EN to 1
> > > if we're not going to trap FPSIMD, as we then corrupt the existing
> > > VFP state.
> > > 
> > > Moving the call to __activate_traps_fpsimd32 to the point where we
> > > know for sure that we are going to trap ensures that we don't set that
> > > bit spuriously.
> > 
> > 
> > Hmmm, this looks like a viable fix but actually the way the code is
> > structured is a bit confusing here.
> > 
> > It looks like some unnecessary functionality has been inherited from
> > arch/arm here: for a 32-bit host, FPEXC controls FPSIMD trapping even
> > at Hyp, so we need to handle it specially in order for the Hyp FPSIMD
> > context switching code to run without generating additional traps.
> > 
> > For a 64-bit host, this doesn't apply: FPEXC32_EL2 is just guest
> > context and doesn't affect EL2 execution.
> 
> Hmm, is that really true?  Earlier versions of the Armv8 Arm ARM, for
> the description of CPTR_EL2.TFP:
> 
>   This causes instructions that access the registers associated with
>   Floating Point and Advanced SIMD execution to trap to EL2 when
>   executed from EL0, EL1, or EL2, unless trapped to EL1.
> 
> In the current version I think you can arrive at the same conclusion by
> looking at the FPEXC_EL2.EN description which tells you that if EN==0,
> then the instruction is UNDEFINED, and the Synchronous exception
> prioritization rules specify that undefined instructions are taken
> before trapped intructions because of CPTR_EL2.
> 
> But there does seem to be some inconsistency with the description of
> generally trapping floating point access to EL2.

You can try taking a look at the archietctural pseudocode -- see the
CheckFPAdvSIMDEnabled64() (for AArch64) and CheckFPAdvSIMDEnabled() (for
AArch32) function hierarchies on developer.arm.com.

ARM DDI 0487B.b says for FPEXC32_EL2:

"Purpose:  Allows access to the AArch32 register FPEXC from AArch64
state only. Its value has no effect on execution in AArch64 state."

This could arguably be worded better, but the second sentence is
pretty unequivocal.

> 
> > 
> > The wrinkle this patch fixes is that implicit reads of FPEXC by
> 
> what do you mean by implicit reads here?

In effect any trappable instruction can be thought of as reading a bunch
of system registers in order to decide whether to take a trap or execute
the instruction normally.  The pseudocode adopts this model.  These
reads are not done by actual MRS instructions in the instruction stream,
so they are deemed implicit.

[...]

Cheers
---Dave

[PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD

[Dave Martin]

On Tue, Sep 04, 2018 at 02:51:21PM +0200, Christoffer Dall wrote:
> On Tue, Sep 04, 2018 at 12:39:54PM +0100, Dave Martin wrote:
> > On Mon, Sep 03, 2018 at 03:45:14PM +0200, Christoffer Dall wrote:
> > > From: Marc Zyngier <marc.zyngier@arm.com>
> > > 
> > > If trapping FPSIMD in the context of an AArch32 guest, it is critical
> > > to set FPEXC32_EL2.EN to 1 so that the trapping is taken to EL2 and
> > > not EL1.
> > > 
> > > Conversely, it is just as critical *not* to set FPEXC32_EL2.EN to 1
> > > if we're not going to trap FPSIMD, as we then corrupt the existing
> > > VFP state.
> > > 
> > > Moving the call to __activate_traps_fpsimd32 to the point where we
> > > know for sure that we are going to trap ensures that we don't set that
> > > bit spuriously.
> > 
> > 
> > Hmmm, this looks like a viable fix but actually the way the code is
> > structured is a bit confusing here.
> > 
> > It looks like some unnecessary functionality has been inherited from
> > arch/arm here: for a 32-bit host, FPEXC controls FPSIMD trapping even
> > at Hyp, so we need to handle it specially in order for the Hyp FPSIMD
> > context switching code to run without generating additional traps.
> > 
> > For a 64-bit host, this doesn't apply: FPEXC32_EL2 is just guest
> > context and doesn't affect EL2 execution.
> 
> Hmm, is that really true?  Earlier versions of the Armv8 Arm ARM, for
> the description of CPTR_EL2.TFP:
> 
>   This causes instructions that access the registers associated with
>   Floating Point and Advanced SIMD execution to trap to EL2 when
>   executed from EL0, EL1, or EL2, unless trapped to EL1.
> 
> In the current version I think you can arrive at the same conclusion by
> looking at the FPEXC_EL2.EN description which tells you that if EN==0,
> then the instruction is UNDEFINED, and the Synchronous exception
> prioritization rules specify that undefined instructions are taken
> before trapped intructions because of CPTR_EL2.
> 
> But there does seem to be some inconsistency with the description of
> generally trapping floating point access to EL2.

You can try taking a look at the archietctural pseudocode -- see the
CheckFPAdvSIMDEnabled64() (for AArch64) and CheckFPAdvSIMDEnabled() (for
AArch32) function hierarchies on developer.arm.com.

ARM DDI 0487B.b says for FPEXC32_EL2:

"Purpose:  Allows access to the AArch32 register FPEXC from AArch64
state only. Its value has no effect on execution in AArch64 state."

This could arguably be worded better, but the second sentence is
pretty unequivocal.

> 
> > 
> > The wrinkle this patch fixes is that implicit reads of FPEXC by
> 
> what do you mean by implicit reads here?

In effect any trappable instruction can be thought of as reading a bunch
of system registers in order to decide whether to take a trap or execute
the instruction normally.  The pseudocode adopts this model.  These
reads are not done by actual MRS instructions in the instruction stream,
so they are deemed implicit.

[...]

Cheers
---Dave

Re: [PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD

[Christoffer Dall]

On Wed, Sep 05, 2018 at 12:28:46PM +0100, Dave Martin wrote:
> On Tue, Sep 04, 2018 at 02:51:21PM +0200, Christoffer Dall wrote:
> > On Tue, Sep 04, 2018 at 12:39:54PM +0100, Dave Martin wrote:
> > > On Mon, Sep 03, 2018 at 03:45:14PM +0200, Christoffer Dall wrote:
> > > > From: Marc Zyngier <marc.zyngier@arm.com>
> > > > 
> > > > If trapping FPSIMD in the context of an AArch32 guest, it is critical
> > > > to set FPEXC32_EL2.EN to 1 so that the trapping is taken to EL2 and
> > > > not EL1.
> > > > 
> > > > Conversely, it is just as critical *not* to set FPEXC32_EL2.EN to 1
> > > > if we're not going to trap FPSIMD, as we then corrupt the existing
> > > > VFP state.
> > > > 
> > > > Moving the call to __activate_traps_fpsimd32 to the point where we
> > > > know for sure that we are going to trap ensures that we don't set that
> > > > bit spuriously.
> > > 
> > > 
> > > Hmmm, this looks like a viable fix but actually the way the code is
> > > structured is a bit confusing here.
> > > 
> > > It looks like some unnecessary functionality has been inherited from
> > > arch/arm here: for a 32-bit host, FPEXC controls FPSIMD trapping even
> > > at Hyp, so we need to handle it specially in order for the Hyp FPSIMD
> > > context switching code to run without generating additional traps.
> > > 
> > > For a 64-bit host, this doesn't apply: FPEXC32_EL2 is just guest
> > > context and doesn't affect EL2 execution.
> > 
> > Hmm, is that really true?  Earlier versions of the Armv8 Arm ARM, for
> > the description of CPTR_EL2.TFP:
> > 
> >   This causes instructions that access the registers associated with
> >   Floating Point and Advanced SIMD execution to trap to EL2 when
> >   executed from EL0, EL1, or EL2, unless trapped to EL1.
> > 
> > In the current version I think you can arrive at the same conclusion by
> > looking at the FPEXC_EL2.EN description which tells you that if EN==0,
> > then the instruction is UNDEFINED, and the Synchronous exception
> > prioritization rules specify that undefined instructions are taken
> > before trapped intructions because of CPTR_EL2.
> > 
> > But there does seem to be some inconsistency with the description of
> > generally trapping floating point access to EL2.
> 
> You can try taking a look at the archietctural pseudocode -- see the
> CheckFPAdvSIMDEnabled64() (for AArch64) and CheckFPAdvSIMDEnabled() (for
> AArch32) function hierarchies on developer.arm.com.
> 
> ARM DDI 0487B.b says for FPEXC32_EL2:
> 
> "Purpose:  Allows access to the AArch32 register FPEXC from AArch64
> state only. Its value has no effect on execution in AArch64 state."
> 
> This could arguably be worded better, but the second sentence is
> pretty unequivocal.
> 

I think I was confused about your point.  I thought you were arguing
that CPTR_EL2.TFP==1 + FPEXC_EL2.EN==0, and running in AArch32 EL1+EL0,
and accessing VFP at EL0 would trap to EL2, when in fact it traps to
EL1.  But now I think you're arguing that EL2 is unaffected by
FPEXC32_EL2, and I agree with that.

However, given the above, I don't understand how we can let the guest be
in control of FPEXC, if we want to make sure we trap VFP accesses?

I'll comment on the other patch.

Thanks,
     Christoffer

[PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD

[Christoffer Dall]

On Wed, Sep 05, 2018 at 12:28:46PM +0100, Dave Martin wrote:
> On Tue, Sep 04, 2018 at 02:51:21PM +0200, Christoffer Dall wrote:
> > On Tue, Sep 04, 2018 at 12:39:54PM +0100, Dave Martin wrote:
> > > On Mon, Sep 03, 2018 at 03:45:14PM +0200, Christoffer Dall wrote:
> > > > From: Marc Zyngier <marc.zyngier@arm.com>
> > > > 
> > > > If trapping FPSIMD in the context of an AArch32 guest, it is critical
> > > > to set FPEXC32_EL2.EN to 1 so that the trapping is taken to EL2 and
> > > > not EL1.
> > > > 
> > > > Conversely, it is just as critical *not* to set FPEXC32_EL2.EN to 1
> > > > if we're not going to trap FPSIMD, as we then corrupt the existing
> > > > VFP state.
> > > > 
> > > > Moving the call to __activate_traps_fpsimd32 to the point where we
> > > > know for sure that we are going to trap ensures that we don't set that
> > > > bit spuriously.
> > > 
> > > 
> > > Hmmm, this looks like a viable fix but actually the way the code is
> > > structured is a bit confusing here.
> > > 
> > > It looks like some unnecessary functionality has been inherited from
> > > arch/arm here: for a 32-bit host, FPEXC controls FPSIMD trapping even
> > > at Hyp, so we need to handle it specially in order for the Hyp FPSIMD
> > > context switching code to run without generating additional traps.
> > > 
> > > For a 64-bit host, this doesn't apply: FPEXC32_EL2 is just guest
> > > context and doesn't affect EL2 execution.
> > 
> > Hmm, is that really true?  Earlier versions of the Armv8 Arm ARM, for
> > the description of CPTR_EL2.TFP:
> > 
> >   This causes instructions that access the registers associated with
> >   Floating Point and Advanced SIMD execution to trap to EL2 when
> >   executed from EL0, EL1, or EL2, unless trapped to EL1.
> > 
> > In the current version I think you can arrive at the same conclusion by
> > looking at the FPEXC_EL2.EN description which tells you that if EN==0,
> > then the instruction is UNDEFINED, and the Synchronous exception
> > prioritization rules specify that undefined instructions are taken
> > before trapped intructions because of CPTR_EL2.
> > 
> > But there does seem to be some inconsistency with the description of
> > generally trapping floating point access to EL2.
> 
> You can try taking a look at the archietctural pseudocode -- see the
> CheckFPAdvSIMDEnabled64() (for AArch64) and CheckFPAdvSIMDEnabled() (for
> AArch32) function hierarchies on developer.arm.com.
> 
> ARM DDI 0487B.b says for FPEXC32_EL2:
> 
> "Purpose:  Allows access to the AArch32 register FPEXC from AArch64
> state only. Its value has no effect on execution in AArch64 state."
> 
> This could arguably be worded better, but the second sentence is
> pretty unequivocal.
> 

I think I was confused about your point.  I thought you were arguing
that CPTR_EL2.TFP==1 + FPEXC_EL2.EN==0, and running in AArch32 EL1+EL0,
and accessing VFP at EL0 would trap to EL2, when in fact it traps to
EL1.  But now I think you're arguing that EL2 is unaffected by
FPEXC32_EL2, and I agree with that.

However, given the above, I don't understand how we can let the guest be
in control of FPEXC, if we want to make sure we trap VFP accesses?

I'll comment on the other patch.

Thanks,
     Christoffer

Re: [PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD

[Dave Martin]

On Wed, Sep 05, 2018 at 05:03:21PM +0200, Christoffer Dall wrote:
> On Wed, Sep 05, 2018 at 12:28:46PM +0100, Dave Martin wrote:

[...]

> > You can try taking a look at the archietctural pseudocode -- see the
> > CheckFPAdvSIMDEnabled64() (for AArch64) and CheckFPAdvSIMDEnabled() (for
> > AArch32) function hierarchies on developer.arm.com.
> > 
> > ARM DDI 0487B.b says for FPEXC32_EL2:
> > 
> > "Purpose:  Allows access to the AArch32 register FPEXC from AArch64
> > state only. Its value has no effect on execution in AArch64 state."
> > 
> > This could arguably be worded better, but the second sentence is
> > pretty unequivocal.
> > 
> 
> I think I was confused about your point.  I thought you were arguing
> that CPTR_EL2.TFP==1 + FPEXC_EL2.EN==0, and running in AArch32 EL1+EL0,
> and accessing VFP at EL0 would trap to EL2, when in fact it traps to
> EL1.  But now I think you're arguing that EL2 is unaffected by
> FPEXC32_EL2, and I agree with that.
> 
> However, given the above, I don't understand how we can let the guest be
> in control of FPEXC, if we want to make sure we trap VFP accesses?

If the guest set FPEXC.EN to 0, then it is expecting its own VFP
accesses to trap to EL1.  If a VFP access attempt traps to EL1 and the
guest is expecting it, why does hyp need to get involved?

Hyp only needs to interfere when an otherwise untrapped access to the
VFP state would occur.

If we don't have the guest context loaded, we would still trap any
attempt by the guest to modify FPEXC to EL2 as a result of CPTR_EL2.TFP
still being set (or equivalent).

Cheers
---Dave

[PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD

[Dave Martin]

On Wed, Sep 05, 2018 at 05:03:21PM +0200, Christoffer Dall wrote:
> On Wed, Sep 05, 2018 at 12:28:46PM +0100, Dave Martin wrote:

[...]

> > You can try taking a look at the archietctural pseudocode -- see the
> > CheckFPAdvSIMDEnabled64() (for AArch64) and CheckFPAdvSIMDEnabled() (for
> > AArch32) function hierarchies on developer.arm.com.
> > 
> > ARM DDI 0487B.b says for FPEXC32_EL2:
> > 
> > "Purpose:  Allows access to the AArch32 register FPEXC from AArch64
> > state only. Its value has no effect on execution in AArch64 state."
> > 
> > This could arguably be worded better, but the second sentence is
> > pretty unequivocal.
> > 
> 
> I think I was confused about your point.  I thought you were arguing
> that CPTR_EL2.TFP==1 + FPEXC_EL2.EN==0, and running in AArch32 EL1+EL0,
> and accessing VFP at EL0 would trap to EL2, when in fact it traps to
> EL1.  But now I think you're arguing that EL2 is unaffected by
> FPEXC32_EL2, and I agree with that.
> 
> However, given the above, I don't understand how we can let the guest be
> in control of FPEXC, if we want to make sure we trap VFP accesses?

If the guest set FPEXC.EN to 0, then it is expecting its own VFP
accesses to trap to EL1.  If a VFP access attempt traps to EL1 and the
guest is expecting it, why does hyp need to get involved?

Hyp only needs to interfere when an otherwise untrapped access to the
VFP state would occur.

If we don't have the guest context loaded, we would still trap any
attempt by the guest to modify FPEXC to EL2 as a result of CPTR_EL2.TFP
still being set (or equivalent).

Cheers
---Dave