KVM ARM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/7] KVM: arm64: Fixes to early stage-2 fault handling
@ 2020-07-24 14:34 Will Deacon
  2020-07-24 14:35 ` [PATCH 1/7] KVM: arm64: Update comment when skipping guest MMIO access instruction Will Deacon
                   ` (6 more replies)
  0 siblings, 7 replies; 16+ messages in thread
From: Will Deacon @ 2020-07-24 14:34 UTC (permalink / raw)
  To: kvmarm; +Cc: kernel-team, Marc Zyngier, Will Deacon, linux-arm-kernel

Hi folks,

Continuing my journey into the KVM stage-2 page-table code, here are some fixes
for a bunch of issues I spotted purely by code inspection. Most of these
involve really unusual scenarios, but I'm a bit worried about the stage-2 fault
on stage-1 page-table walk during instruction fetch from a read-only memslot,
as that feels like it might be hittable with EFI.

Anyway, feedback welcome, especially as this is a user-visible change.

Cheers,

Will

Cc: Marc Zyngier <maz@kernel.org>
Cc: Quentin Perret <qperret@google.com>
Cc: James Morse <james.morse@arm.com>
Cc: Suzuki Poulose <suzuki.poulose@arm.com>

--->8

Will Deacon (7):
  KVM: arm64: Update comment when skipping guest MMIO access instruction
  KVM: arm64: Rename kvm_vcpu_dabt_isextabt()
  KVM: arm64: Handle data and instruction external aborts the same way
  KVM: arm64: Remove useless local variable
  KVM: arm64: Move 'invalid syndrome' logic out of io_mem_abort()
  KVM: arm64: Handle stage-2 faults on stage-1 page-table walks earlier
  KVM: arm64: Separate write faults on read-only memslots from MMIO

 arch/arm64/include/asm/kvm_emulate.h |  2 +-
 arch/arm64/kvm/hyp/switch.c          |  2 +-
 arch/arm64/kvm/mmio.c                | 29 +++-------
 arch/arm64/kvm/mmu.c                 | 87 +++++++++++++++++++---------
 4 files changed, 69 insertions(+), 51 deletions(-)

-- 
2.28.0.rc0.142.g3c755180ce-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/7] KVM: arm64: Update comment when skipping guest MMIO access instruction
  2020-07-24 14:34 [PATCH 0/7] KVM: arm64: Fixes to early stage-2 fault handling Will Deacon
@ 2020-07-24 14:35 ` Will Deacon
  2020-07-26 11:08   ` Marc Zyngier
  2020-07-24 14:35 ` [PATCH 2/7] KVM: arm64: Rename kvm_vcpu_dabt_isextabt() Will Deacon
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Will Deacon @ 2020-07-24 14:35 UTC (permalink / raw)
  To: kvmarm; +Cc: kernel-team, Marc Zyngier, Will Deacon, linux-arm-kernel

If a 32-bit guest accesses MMIO using a 16-bit Thumb-2 instruction that
is reported to the hypervisor without a valid syndrom (for example,
because of the addressing mode), then we may hand off the fault to
userspace. When resuming the guest, we unconditionally advance the PC
by 4 bytes, since ESR_EL2.IL is always 1 for data aborts generated without
a valid syndrome. This is a bit rubbish, but it's also difficult to see
how we can fix it without potentially introducing regressions in userspace
MMIO fault handling.

Update the comment when skipping a guest MMIO access instruction so that
this corner case is at least written down.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Quentin Perret <qperret@google.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kvm/mmio.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/kvm/mmio.c b/arch/arm64/kvm/mmio.c
index 4e0366759726..b54ea5aa6c06 100644
--- a/arch/arm64/kvm/mmio.c
+++ b/arch/arm64/kvm/mmio.c
@@ -113,6 +113,13 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	/*
 	 * The MMIO instruction is emulated and should not be re-executed
 	 * in the guest.
+	 *
+	 * Note: If user space handled the emulation because the abort
+	 * symdrome information was not valid (ISV set in the ESR), then
+	 * this will assume that the faulting instruction was 32-bit.
+	 * If the faulting instruction was a 16-bit Thumb instruction,
+	 * then userspace would need to rewind the PC by 2 bytes prior to
+	 * resuming the vCPU (yuck!).
 	 */
 	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
 
-- 
2.28.0.rc0.142.g3c755180ce-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/7] KVM: arm64: Rename kvm_vcpu_dabt_isextabt()
  2020-07-24 14:34 [PATCH 0/7] KVM: arm64: Fixes to early stage-2 fault handling Will Deacon
  2020-07-24 14:35 ` [PATCH 1/7] KVM: arm64: Update comment when skipping guest MMIO access instruction Will Deacon
@ 2020-07-24 14:35 ` Will Deacon
  2020-07-26 11:15   ` Marc Zyngier
  2020-07-24 14:35 ` [PATCH 3/7] KVM: arm64: Handle data and instruction external aborts the same way Will Deacon
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Will Deacon @ 2020-07-24 14:35 UTC (permalink / raw)
  To: kvmarm; +Cc: kernel-team, Marc Zyngier, Will Deacon, linux-arm-kernel

kvm_vcpu_dabt_isextabt() is not specific to data aborts and has nothing
to do with sign extension.

Rename it.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Quentin Perret <qperret@google.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/kvm_emulate.h | 2 +-
 arch/arm64/kvm/hyp/switch.c          | 2 +-
 arch/arm64/kvm/mmu.c                 | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 4d0f8ea600ba..9d6beb532536 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -366,7 +366,7 @@ static __always_inline u8 kvm_vcpu_trap_get_fault_type(const struct kvm_vcpu *vc
 	return kvm_vcpu_get_hsr(vcpu) & ESR_ELx_FSC_TYPE;
 }
 
-static __always_inline bool kvm_vcpu_dabt_isextabt(const struct kvm_vcpu *vcpu)
+static __always_inline bool kvm_vcpu_abt_is_extabt(const struct kvm_vcpu *vcpu)
 {
 	switch (kvm_vcpu_trap_get_fault(vcpu)) {
 	case FSC_SEA:
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index db1c4487d95d..9aa1092bbafd 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -594,7 +594,7 @@ static bool __hyp_text fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code)
 		valid = kvm_vcpu_trap_get_class(vcpu) == ESR_ELx_EC_DABT_LOW &&
 			kvm_vcpu_trap_get_fault_type(vcpu) == FSC_FAULT &&
 			kvm_vcpu_dabt_isvalid(vcpu) &&
-			!kvm_vcpu_dabt_isextabt(vcpu) &&
+			!kvm_vcpu_abt_is_extabt(vcpu) &&
 			!kvm_vcpu_dabt_iss1tw(vcpu);
 
 		if (valid) {
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index df2a8025ec8a..660a83a172e4 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -2074,7 +2074,7 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	is_iabt = kvm_vcpu_trap_is_iabt(vcpu);
 
 	/* Synchronous External Abort? */
-	if (kvm_vcpu_dabt_isextabt(vcpu)) {
+	if (kvm_vcpu_abt_is_extabt(vcpu)) {
 		/*
 		 * For RAS the host kernel may handle this abort.
 		 * There is no need to pass the error into the guest.
-- 
2.28.0.rc0.142.g3c755180ce-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 3/7] KVM: arm64: Handle data and instruction external aborts the same way
  2020-07-24 14:34 [PATCH 0/7] KVM: arm64: Fixes to early stage-2 fault handling Will Deacon
  2020-07-24 14:35 ` [PATCH 1/7] KVM: arm64: Update comment when skipping guest MMIO access instruction Will Deacon
  2020-07-24 14:35 ` [PATCH 2/7] KVM: arm64: Rename kvm_vcpu_dabt_isextabt() Will Deacon
@ 2020-07-24 14:35 ` Will Deacon
  2020-07-24 14:35 ` [PATCH 4/7] KVM: arm64: Remove useless local variable Will Deacon
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Will Deacon @ 2020-07-24 14:35 UTC (permalink / raw)
  To: kvmarm; +Cc: kernel-team, Marc Zyngier, Will Deacon, linux-arm-kernel

If the guest generates a synchronous external abort which is not handled
by the host, we inject it back into the guest as a virtual SError, but
only if the original fault was reported on the data side. Instruction
faults are reported as "Unsupported FSC", causing the vCPU run loop to
bail with -EFAULT.

Although synchronous external aborts from a guest are pretty unusual,
treat them the same regardless of whether they are taken as data or
instruction aborts by EL2.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Quentin Perret <qperret@google.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kvm/mmu.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 660a83a172e4..ae17a99243e2 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -2079,13 +2079,10 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		 * For RAS the host kernel may handle this abort.
 		 * There is no need to pass the error into the guest.
 		 */
-		if (!kvm_handle_guest_sea(fault_ipa, kvm_vcpu_get_hsr(vcpu)))
-			return 1;
-
-		if (unlikely(!is_iabt)) {
+		if (kvm_handle_guest_sea(fault_ipa, kvm_vcpu_get_hsr(vcpu)))
 			kvm_inject_vabt(vcpu);
-			return 1;
-		}
+
+		return 1;
 	}
 
 	trace_kvm_guest_fault(*vcpu_pc(vcpu), kvm_vcpu_get_hsr(vcpu),
-- 
2.28.0.rc0.142.g3c755180ce-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 4/7] KVM: arm64: Remove useless local variable
  2020-07-24 14:34 [PATCH 0/7] KVM: arm64: Fixes to early stage-2 fault handling Will Deacon
                   ` (2 preceding siblings ...)
  2020-07-24 14:35 ` [PATCH 3/7] KVM: arm64: Handle data and instruction external aborts the same way Will Deacon
@ 2020-07-24 14:35 ` Will Deacon
  2020-07-24 14:35 ` [PATCH 5/7] KVM: arm64: Move 'invalid syndrome' logic out of io_mem_abort() Will Deacon
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Will Deacon @ 2020-07-24 14:35 UTC (permalink / raw)
  To: kvmarm; +Cc: kernel-team, Marc Zyngier, Will Deacon, linux-arm-kernel

'is_iabt' is used exactly once in kvm_handle_guest_abort(). Remove it,
and inlined its definition instead.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Quentin Perret <qperret@google.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kvm/mmu.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index ae17a99243e2..73e62d360a36 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -2064,14 +2064,13 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	phys_addr_t fault_ipa;
 	struct kvm_memory_slot *memslot;
 	unsigned long hva;
-	bool is_iabt, write_fault, writable;
+	bool write_fault, writable;
 	gfn_t gfn;
 	int ret, idx;
 
 	fault_status = kvm_vcpu_trap_get_fault_type(vcpu);
 
 	fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
-	is_iabt = kvm_vcpu_trap_is_iabt(vcpu);
 
 	/* Synchronous External Abort? */
 	if (kvm_vcpu_abt_is_extabt(vcpu)) {
@@ -2105,7 +2104,7 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	hva = gfn_to_hva_memslot_prot(memslot, gfn, &writable);
 	write_fault = kvm_is_write_fault(vcpu);
 	if (kvm_is_error_hva(hva) || (write_fault && !writable)) {
-		if (is_iabt) {
+		if (kvm_vcpu_trap_is_iabt(vcpu)) {
 			/* Prefetch Abort on I/O address */
 			ret = -ENOEXEC;
 			goto out;
-- 
2.28.0.rc0.142.g3c755180ce-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 5/7] KVM: arm64: Move 'invalid syndrome' logic out of io_mem_abort()
  2020-07-24 14:34 [PATCH 0/7] KVM: arm64: Fixes to early stage-2 fault handling Will Deacon
                   ` (3 preceding siblings ...)
  2020-07-24 14:35 ` [PATCH 4/7] KVM: arm64: Remove useless local variable Will Deacon
@ 2020-07-24 14:35 ` Will Deacon
  2020-07-26 11:55   ` Marc Zyngier
  2020-07-24 14:35 ` [PATCH 6/7] KVM: arm64: Handle stage-2 faults on stage-1 page-table walks earlier Will Deacon
  2020-07-24 14:35 ` [PATCH 7/7] KVM: arm64: Separate write faults on read-only memslots from MMIO Will Deacon
  6 siblings, 1 reply; 16+ messages in thread
From: Will Deacon @ 2020-07-24 14:35 UTC (permalink / raw)
  To: kvmarm; +Cc: kernel-team, Marc Zyngier, Will Deacon, linux-arm-kernel

In preparation for handling stage-2 faults on stage-1 page-table walks
earlier, move the 'invalid syndrome' logic out of io_mem_abort() and
into its own function, which can be called from kvm_handle_guest_abort()
directly.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Quentin Perret <qperret@google.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kvm/mmio.c | 22 ----------------------
 arch/arm64/kvm/mmu.c  | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 22 deletions(-)

diff --git a/arch/arm64/kvm/mmio.c b/arch/arm64/kvm/mmio.c
index b54ea5aa6c06..45a630e480e1 100644
--- a/arch/arm64/kvm/mmio.c
+++ b/arch/arm64/kvm/mmio.c
@@ -136,28 +136,6 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
 	int len;
 	u8 data_buf[8];
 
-	/*
-	 * No valid syndrome? Ask userspace for help if it has
-	 * volunteered to do so, and bail out otherwise.
-	 */
-	if (!kvm_vcpu_dabt_isvalid(vcpu)) {
-		if (vcpu->kvm->arch.return_nisv_io_abort_to_user) {
-			run->exit_reason = KVM_EXIT_ARM_NISV;
-			run->arm_nisv.esr_iss = kvm_vcpu_dabt_iss_nisv_sanitized(vcpu);
-			run->arm_nisv.fault_ipa = fault_ipa;
-			return 0;
-		}
-
-		kvm_pr_unimpl("Data abort outside memslots with no valid syndrome info\n");
-		return -ENOSYS;
-	}
-
-	/* Page table accesses IO mem: tell guest to fix its TTBR */
-	if (kvm_vcpu_dabt_iss1tw(vcpu)) {
-		kvm_inject_dabt(vcpu, kvm_vcpu_get_hfar(vcpu));
-		return 1;
-	}
-
 	/*
 	 * Prepare MMIO operation. First decode the syndrome data we get
 	 * from the CPU. Then try if some in-kernel emulation feels
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 73e62d360a36..adb933ecd177 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -2046,6 +2046,20 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
 		kvm_set_pfn_accessed(pfn);
 }
 
+static int handle_error_invalid_dabt(struct kvm_vcpu *vcpu, struct kvm_run *run,
+				     phys_addr_t fault_ipa)
+{
+	if (!vcpu->kvm->arch.return_nisv_io_abort_to_user) {
+		kvm_pr_unimpl("Data abort outside memslots with no valid syndrome info\n");
+		return -ENOSYS;
+	}
+
+	run->exit_reason = KVM_EXIT_ARM_NISV;
+	run->arm_nisv.esr_iss = kvm_vcpu_dabt_iss_nisv_sanitized(vcpu);
+	run->arm_nisv.fault_ipa = fault_ipa;
+	return 0;
+}
+
 /**
  * kvm_handle_guest_abort - handles all 2nd stage aborts
  * @vcpu:	the VCPU pointer
@@ -2133,6 +2147,21 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		 * of the page size.
 		 */
 		fault_ipa |= kvm_vcpu_get_hfar(vcpu) & ((1 << 12) - 1);
+
+		/*
+		 * No valid syndrome? Ask userspace for help if it has
+		 * volunteered to do so, and bail out otherwise.
+		 */
+		if (!kvm_vcpu_dabt_isvalid(vcpu)) {
+			ret = handle_error_invalid_dabt(vcpu, run, fault_ipa);
+			goto out_unlock;
+		}
+
+		if (kvm_vcpu_dabt_iss1tw(vcpu)) {
+			ret = -ENXIO;
+			goto out;
+		}
+
 		ret = io_mem_abort(vcpu, run, fault_ipa);
 		goto out_unlock;
 	}
@@ -2153,6 +2182,9 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	if (ret == -ENOEXEC) {
 		kvm_inject_pabt(vcpu, kvm_vcpu_get_hfar(vcpu));
 		ret = 1;
+	} else if (ret == -ENXIO) {
+		kvm_inject_dabt(vcpu, kvm_vcpu_get_hfar(vcpu));
+		ret = 1;
 	}
 out_unlock:
 	srcu_read_unlock(&vcpu->kvm->srcu, idx);
-- 
2.28.0.rc0.142.g3c755180ce-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 6/7] KVM: arm64: Handle stage-2 faults on stage-1 page-table walks earlier
  2020-07-24 14:34 [PATCH 0/7] KVM: arm64: Fixes to early stage-2 fault handling Will Deacon
                   ` (4 preceding siblings ...)
  2020-07-24 14:35 ` [PATCH 5/7] KVM: arm64: Move 'invalid syndrome' logic out of io_mem_abort() Will Deacon
@ 2020-07-24 14:35 ` Will Deacon
  2020-07-26 13:38   ` Marc Zyngier
  2020-07-24 14:35 ` [PATCH 7/7] KVM: arm64: Separate write faults on read-only memslots from MMIO Will Deacon
  6 siblings, 1 reply; 16+ messages in thread
From: Will Deacon @ 2020-07-24 14:35 UTC (permalink / raw)
  To: kvmarm; +Cc: kernel-team, Marc Zyngier, Will Deacon, linux-arm-kernel

Stage-2 faults on stage-1 page-table walks can occur on both the I-side
and the D-side. It is IMPLEMENTATATION DEFINED whether they are reported
as reads or writes and, in the case that they are generated by an AT
instruction, they are reported with the CM bit set.

All of this deeply confuses the logic in kvm_handle_guest_abort();
userspace may or may not see the fault, depending on whether it occurs
on the data or the instruction side, and an AT instruction may be skipped
if the translation tables are held in a read-only memslot.

Move the handling of stage-2 faults on stage-1 page-table walks earlier
so that they consistently result in either a data or an instruction abort
being re-injected back to the guest.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Quentin Perret <qperret@google.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kvm/mmu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index adb933ecd177..9e72e7f4a2c2 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -2124,6 +2124,11 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			goto out;
 		}
 
+		if (kvm_vcpu_dabt_iss1tw(vcpu)) {
+			ret = -ENXIO;
+			goto out;
+		}
+
 		/*
 		 * Check for a cache maintenance operation. Since we
 		 * ended-up here, we know it is outside of any memory
@@ -2157,11 +2162,6 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			goto out_unlock;
 		}
 
-		if (kvm_vcpu_dabt_iss1tw(vcpu)) {
-			ret = -ENXIO;
-			goto out;
-		}
-
 		ret = io_mem_abort(vcpu, run, fault_ipa);
 		goto out_unlock;
 	}
-- 
2.28.0.rc0.142.g3c755180ce-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 7/7] KVM: arm64: Separate write faults on read-only memslots from MMIO
  2020-07-24 14:34 [PATCH 0/7] KVM: arm64: Fixes to early stage-2 fault handling Will Deacon
                   ` (5 preceding siblings ...)
  2020-07-24 14:35 ` [PATCH 6/7] KVM: arm64: Handle stage-2 faults on stage-1 page-table walks earlier Will Deacon
@ 2020-07-24 14:35 ` Will Deacon
  6 siblings, 0 replies; 16+ messages in thread
From: Will Deacon @ 2020-07-24 14:35 UTC (permalink / raw)
  To: kvmarm; +Cc: kernel-team, Marc Zyngier, Will Deacon, linux-arm-kernel

Although taking a write-fault on a read-only memslot triggers an MMIO
exit back to userspace, lumping the handling together in
kvm_handle_guest_abort() causes some of the early triage to have weird
effects on userspace. For example, if a guest generates a stage-2 fault
on a stage-1 translation fault when trying to fetch an instruction from
a read-only memslot, it will be mistakenly treated as an attempt to
execute from MMIO and a prefetch abort will be re-injected into the
guest.

Separate the MMIO handling from the read-only memslot handling, so that
the latter is left entirely up to userspace. Note that this _will_
result in more exits than before for read-only memslots, since userspace
will now see some cache maintenance and instruction-side aborts.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Quentin Perret <qperret@google.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kvm/mmu.c | 45 ++++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 9e72e7f4a2c2..2edc6f2412dc 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -2117,9 +2117,30 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	memslot = gfn_to_memslot(vcpu->kvm, gfn);
 	hva = gfn_to_hva_memslot_prot(memslot, gfn, &writable);
 	write_fault = kvm_is_write_fault(vcpu);
-	if (kvm_is_error_hva(hva) || (write_fault && !writable)) {
+
+	/*
+	 * The IPA is reported as [MAX:12], so we need to complement it with
+	 * the bottom 12 bits from the faulting VA. This is always 12 bits,
+	 * irrespective of the page size.
+	 */
+	fault_ipa |= kvm_vcpu_get_hfar(vcpu) & ((1 << 12) - 1);
+
+	/*
+	 * We can perform some early fault triage based purely on the memslot
+	 * information:
+	 *
+	 * Faults on IPAs falling outside of any memslot are re-injected
+	 * into the guest as external aborts if they were either signalled as
+	 * instruction aborts or as a stage-2 fault on a translation table walk.
+	 * If the instruction was a cache maintenance instruction then it is
+	 * quietly skipped, otherwise we exit to userspace for MMIO emulation.
+	 *
+	 * Write faults on IPAs falling within a read-only memslot are reported
+	 * to userspace as MMIO exits. This includes cache maintenance and
+	 * stage-2 faults on translation table walks,
+	 */
+	if (kvm_is_error_hva(hva)) {
 		if (kvm_vcpu_trap_is_iabt(vcpu)) {
-			/* Prefetch Abort on I/O address */
 			ret = -ENOEXEC;
 			goto out;
 		}
@@ -2129,30 +2150,12 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			goto out;
 		}
 
-		/*
-		 * Check for a cache maintenance operation. Since we
-		 * ended-up here, we know it is outside of any memory
-		 * slot. But we can't find out if that is for a device,
-		 * or if the guest is just being stupid. The only thing
-		 * we know for sure is that this range cannot be cached.
-		 *
-		 * So let's assume that the guest is just being
-		 * cautious, and skip the instruction.
-		 */
 		if (kvm_vcpu_dabt_is_cm(vcpu)) {
 			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
 			ret = 1;
 			goto out_unlock;
 		}
 
-		/*
-		 * The IPA is reported as [MAX:12], so we need to
-		 * complement it with the bottom 12 bits from the
-		 * faulting VA. This is always 12 bits, irrespective
-		 * of the page size.
-		 */
-		fault_ipa |= kvm_vcpu_get_hfar(vcpu) & ((1 << 12) - 1);
-
 		/*
 		 * No valid syndrome? Ask userspace for help if it has
 		 * volunteered to do so, and bail out otherwise.
@@ -2161,7 +2164,9 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			ret = handle_error_invalid_dabt(vcpu, run, fault_ipa);
 			goto out_unlock;
 		}
+	}
 
+	if (kvm_is_error_hva(hva) || (write_fault && !writable)) {
 		ret = io_mem_abort(vcpu, run, fault_ipa);
 		goto out_unlock;
 	}
-- 
2.28.0.rc0.142.g3c755180ce-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/7] KVM: arm64: Update comment when skipping guest MMIO access instruction
  2020-07-24 14:35 ` [PATCH 1/7] KVM: arm64: Update comment when skipping guest MMIO access instruction Will Deacon
@ 2020-07-26 11:08   ` Marc Zyngier
  2020-07-27 10:30     ` Will Deacon
  0 siblings, 1 reply; 16+ messages in thread
From: Marc Zyngier @ 2020-07-26 11:08 UTC (permalink / raw)
  To: Will Deacon; +Cc: kernel-team, kvmarm, linux-arm-kernel

On Fri, 24 Jul 2020 15:35:00 +0100,
Will Deacon <will@kernel.org> wrote:
> 
> If a 32-bit guest accesses MMIO using a 16-bit Thumb-2 instruction that
> is reported to the hypervisor without a valid syndrom (for example,
> because of the addressing mode), then we may hand off the fault to
> userspace. When resuming the guest, we unconditionally advance the PC
> by 4 bytes, since ESR_EL2.IL is always 1 for data aborts generated without
> a valid syndrome. This is a bit rubbish, but it's also difficult to see
> how we can fix it without potentially introducing regressions in userspace
> MMIO fault handling.

Not quite, see below.

> 
> Update the comment when skipping a guest MMIO access instruction so that
> this corner case is at least written down.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Quentin Perret <qperret@google.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kvm/mmio.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/kvm/mmio.c b/arch/arm64/kvm/mmio.c
> index 4e0366759726..b54ea5aa6c06 100644
> --- a/arch/arm64/kvm/mmio.c
> +++ b/arch/arm64/kvm/mmio.c
> @@ -113,6 +113,13 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	/*
>  	 * The MMIO instruction is emulated and should not be re-executed
>  	 * in the guest.
> +	 *
> +	 * Note: If user space handled the emulation because the abort
> +	 * symdrome information was not valid (ISV set in the ESR), then

nits: syndrome, ISV *clear*.

> +	 * this will assume that the faulting instruction was 32-bit.
> +	 * If the faulting instruction was a 16-bit Thumb instruction,
> +	 * then userspace would need to rewind the PC by 2 bytes prior to
> +	 * resuming the vCPU (yuck!).
>  	 */
>  	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
>  

That's not how I read it. On ESR_EL2.ISV being clear, and in the
absence of KVM_CAP_ARM_NISV_TO_USER being set, we return a -ENOSYS from
io_mem_abort(), exiting back to userspace *without* advertising a MMIO
access. The VMM is free to do whatever it can to handle it (i.e. not
much), but crucially we don't go via kvm_handle_mmio_return() on
resuming the vcpu (unless the VMM sets run->exit_reason to
KVM_EXIT_MMIO, but that's clearly its own decision).

Instead, the expectation is that userspace willing to handle an exit
resulting in ESR_EL2.ISV being clear would instead request a
KVM_EXIT_ARM_NISV exit type (by enabling KVM_CAP_ARM_NISV_TO_USER),
getting extra information in the process such as as the fault
IPA). KVM_EXIT_ARM_NISV clearly states in the documentation:

  "Note that KVM does not skip the faulting instruction as it does for
   KVM_EXIT_MMIO, but userspace has to emulate any change to the
   processing state if it decides to decode and emulate the instruction."

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/7] KVM: arm64: Rename kvm_vcpu_dabt_isextabt()
  2020-07-24 14:35 ` [PATCH 2/7] KVM: arm64: Rename kvm_vcpu_dabt_isextabt() Will Deacon
@ 2020-07-26 11:15   ` Marc Zyngier
  2020-07-27 10:30     ` Will Deacon
  0 siblings, 1 reply; 16+ messages in thread
From: Marc Zyngier @ 2020-07-26 11:15 UTC (permalink / raw)
  To: Will Deacon; +Cc: kernel-team, kvmarm, linux-arm-kernel

On Fri, 24 Jul 2020 15:35:01 +0100,
Will Deacon <will@kernel.org> wrote:
> 
> kvm_vcpu_dabt_isextabt() is not specific to data aborts and has nothing
> to do with sign extension.

Not sign extension, but external abort (it reads as "is external
abort?").

This is in keeping with all the other helpers (kvm_vcpu_foo_isbar). If
you want to drop the "data" part (which is indeed not correct), how
about kvm_vcpu_abt_isexternal? or kvm_vcpu_abt_issea?

Thanks,

	M.

> 
> Rename it.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Quentin Perret <qperret@google.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_emulate.h | 2 +-
>  arch/arm64/kvm/hyp/switch.c          | 2 +-
>  arch/arm64/kvm/mmu.c                 | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 4d0f8ea600ba..9d6beb532536 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -366,7 +366,7 @@ static __always_inline u8 kvm_vcpu_trap_get_fault_type(const struct kvm_vcpu *vc
>  	return kvm_vcpu_get_hsr(vcpu) & ESR_ELx_FSC_TYPE;
>  }
>  
> -static __always_inline bool kvm_vcpu_dabt_isextabt(const struct kvm_vcpu *vcpu)
> +static __always_inline bool kvm_vcpu_abt_is_extabt(const struct kvm_vcpu *vcpu)
>  {
>  	switch (kvm_vcpu_trap_get_fault(vcpu)) {
>  	case FSC_SEA:
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index db1c4487d95d..9aa1092bbafd 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -594,7 +594,7 @@ static bool __hyp_text fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code)
>  		valid = kvm_vcpu_trap_get_class(vcpu) == ESR_ELx_EC_DABT_LOW &&
>  			kvm_vcpu_trap_get_fault_type(vcpu) == FSC_FAULT &&
>  			kvm_vcpu_dabt_isvalid(vcpu) &&
> -			!kvm_vcpu_dabt_isextabt(vcpu) &&
> +			!kvm_vcpu_abt_is_extabt(vcpu) &&
>  			!kvm_vcpu_dabt_iss1tw(vcpu);
>  
>  		if (valid) {
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index df2a8025ec8a..660a83a172e4 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -2074,7 +2074,7 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	is_iabt = kvm_vcpu_trap_is_iabt(vcpu);
>  
>  	/* Synchronous External Abort? */
> -	if (kvm_vcpu_dabt_isextabt(vcpu)) {
> +	if (kvm_vcpu_abt_is_extabt(vcpu)) {
>  		/*
>  		 * For RAS the host kernel may handle this abort.
>  		 * There is no need to pass the error into the guest.
> -- 
> 2.28.0.rc0.142.g3c755180ce-goog
> 
> 

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 5/7] KVM: arm64: Move 'invalid syndrome' logic out of io_mem_abort()
  2020-07-24 14:35 ` [PATCH 5/7] KVM: arm64: Move 'invalid syndrome' logic out of io_mem_abort() Will Deacon
@ 2020-07-26 11:55   ` Marc Zyngier
  2020-07-27 10:31     ` Will Deacon
  0 siblings, 1 reply; 16+ messages in thread
From: Marc Zyngier @ 2020-07-26 11:55 UTC (permalink / raw)
  To: Will Deacon; +Cc: kernel-team, kvmarm, linux-arm-kernel

On Fri, 24 Jul 2020 15:35:04 +0100,
Will Deacon <will@kernel.org> wrote:
> 
> In preparation for handling stage-2 faults on stage-1 page-table walks
> earlier, move the 'invalid syndrome' logic out of io_mem_abort() and
> into its own function, which can be called from kvm_handle_guest_abort()
> directly.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Quentin Perret <qperret@google.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kvm/mmio.c | 22 ----------------------
>  arch/arm64/kvm/mmu.c  | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kvm/mmio.c b/arch/arm64/kvm/mmio.c
> index b54ea5aa6c06..45a630e480e1 100644
> --- a/arch/arm64/kvm/mmio.c
> +++ b/arch/arm64/kvm/mmio.c
> @@ -136,28 +136,6 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  	int len;
>  	u8 data_buf[8];
>  
> -	/*
> -	 * No valid syndrome? Ask userspace for help if it has
> -	 * volunteered to do so, and bail out otherwise.
> -	 */
> -	if (!kvm_vcpu_dabt_isvalid(vcpu)) {
> -		if (vcpu->kvm->arch.return_nisv_io_abort_to_user) {
> -			run->exit_reason = KVM_EXIT_ARM_NISV;
> -			run->arm_nisv.esr_iss = kvm_vcpu_dabt_iss_nisv_sanitized(vcpu);
> -			run->arm_nisv.fault_ipa = fault_ipa;
> -			return 0;
> -		}
> -
> -		kvm_pr_unimpl("Data abort outside memslots with no valid syndrome info\n");
> -		return -ENOSYS;
> -	}
> -
> -	/* Page table accesses IO mem: tell guest to fix its TTBR */
> -	if (kvm_vcpu_dabt_iss1tw(vcpu)) {
> -		kvm_inject_dabt(vcpu, kvm_vcpu_get_hfar(vcpu));
> -		return 1;
> -	}
> -
>  	/*
>  	 * Prepare MMIO operation. First decode the syndrome data we get
>  	 * from the CPU. Then try if some in-kernel emulation feels
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 73e62d360a36..adb933ecd177 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -2046,6 +2046,20 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
>  		kvm_set_pfn_accessed(pfn);
>  }
>  
> +static int handle_error_invalid_dabt(struct kvm_vcpu *vcpu, struct kvm_run *run,

Nit: why the "_error_"? There isn't any error here, only an awkward
part of the architecture. I'd rather see something like
handle_nisv_dabt(), which matches what this function actually does.

> +				     phys_addr_t fault_ipa)
> +{
> +	if (!vcpu->kvm->arch.return_nisv_io_abort_to_user) {
> +		kvm_pr_unimpl("Data abort outside memslots with no valid syndrome info\n");
> +		return -ENOSYS;
> +	}
> +
> +	run->exit_reason = KVM_EXIT_ARM_NISV;
> +	run->arm_nisv.esr_iss = kvm_vcpu_dabt_iss_nisv_sanitized(vcpu);
> +	run->arm_nisv.fault_ipa = fault_ipa;
> +	return 0;
> +}
> +
>  /**
>   * kvm_handle_guest_abort - handles all 2nd stage aborts
>   * @vcpu:	the VCPU pointer
> @@ -2133,6 +2147,21 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		 * of the page size.
>  		 */
>  		fault_ipa |= kvm_vcpu_get_hfar(vcpu) & ((1 << 12) - 1);
> +
> +		/*
> +		 * No valid syndrome? Ask userspace for help if it has
> +		 * volunteered to do so, and bail out otherwise.
> +		 */
> +		if (!kvm_vcpu_dabt_isvalid(vcpu)) {
> +			ret = handle_error_invalid_dabt(vcpu, run, fault_ipa);
> +			goto out_unlock;
> +		}
> +
> +		if (kvm_vcpu_dabt_iss1tw(vcpu)) {
> +			ret = -ENXIO;
> +			goto out;
> +		}
> +
>  		ret = io_mem_abort(vcpu, run, fault_ipa);
>  		goto out_unlock;
>  	}
> @@ -2153,6 +2182,9 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	if (ret == -ENOEXEC) {
>  		kvm_inject_pabt(vcpu, kvm_vcpu_get_hfar(vcpu));
>  		ret = 1;
> +	} else if (ret == -ENXIO) {
> +		kvm_inject_dabt(vcpu, kvm_vcpu_get_hfar(vcpu));
> +		ret = 1;
>  	}
>  out_unlock:
>  	srcu_read_unlock(&vcpu->kvm->srcu, idx);
> -- 
> 2.28.0.rc0.142.g3c755180ce-goog
> 
> 

Otherwise looks OK.

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 6/7] KVM: arm64: Handle stage-2 faults on stage-1 page-table walks earlier
  2020-07-24 14:35 ` [PATCH 6/7] KVM: arm64: Handle stage-2 faults on stage-1 page-table walks earlier Will Deacon
@ 2020-07-26 13:38   ` Marc Zyngier
  2020-07-27 10:29     ` Will Deacon
  0 siblings, 1 reply; 16+ messages in thread
From: Marc Zyngier @ 2020-07-26 13:38 UTC (permalink / raw)
  To: Will Deacon; +Cc: kernel-team, kvmarm, linux-arm-kernel

On Fri, 24 Jul 2020 15:35:05 +0100,
Will Deacon <will@kernel.org> wrote:
> 
> Stage-2 faults on stage-1 page-table walks can occur on both the I-side
> and the D-side. It is IMPLEMENTATATION DEFINED whether they are reported
> as reads or writes and, in the case that they are generated by an AT
> instruction, they are reported with the CM bit set.
> 
> All of this deeply confuses the logic in kvm_handle_guest_abort();
> userspace may or may not see the fault, depending on whether it occurs
> on the data or the instruction side, and an AT instruction may be skipped
> if the translation tables are held in a read-only memslot.

Yuk, that's indeed ugly. Well spotted. I guess the saving grace is
that a S2 trap caused by an ATS1 instruction will be reported as
S1PTW+CM, while the fault caused by a CMO is reported as *either*
S1PTW *or* CM, but never both.

> 
> Move the handling of stage-2 faults on stage-1 page-table walks earlier
> so that they consistently result in either a data or an instruction abort
> being re-injected back to the guest.

The instruction abort seems to be happening as the side effect of
executing outside of a memslot, not really because of a S1PTW.  I
wonder whether these S1PTW faults should be classified as external
aborts instead (because putting your page tables outside of a memslot
seems a bit bonkers).

> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Quentin Perret <qperret@google.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kvm/mmu.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index adb933ecd177..9e72e7f4a2c2 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -2124,6 +2124,11 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  			goto out;
>  		}
>  
> +		if (kvm_vcpu_dabt_iss1tw(vcpu)) {
> +			ret = -ENXIO;
> +			goto out;
> +		}
> +
>  		/*
>  		 * Check for a cache maintenance operation. Since we
>  		 * ended-up here, we know it is outside of any memory
> @@ -2157,11 +2162,6 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  			goto out_unlock;
>  		}
>  
> -		if (kvm_vcpu_dabt_iss1tw(vcpu)) {
> -			ret = -ENXIO;
> -			goto out;
> -		}
> -
>  		ret = io_mem_abort(vcpu, run, fault_ipa);
>  		goto out_unlock;
>  	}
> -- 
> 2.28.0.rc0.142.g3c755180ce-goog
> 
> 

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 6/7] KVM: arm64: Handle stage-2 faults on stage-1 page-table walks earlier
  2020-07-26 13:38   ` Marc Zyngier
@ 2020-07-27 10:29     ` Will Deacon
  0 siblings, 0 replies; 16+ messages in thread
From: Will Deacon @ 2020-07-27 10:29 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: kernel-team, kvmarm, linux-arm-kernel

On Sun, Jul 26, 2020 at 02:38:38PM +0100, Marc Zyngier wrote:
> On Fri, 24 Jul 2020 15:35:05 +0100,
> Will Deacon <will@kernel.org> wrote:
> > 
> > Stage-2 faults on stage-1 page-table walks can occur on both the I-side
> > and the D-side. It is IMPLEMENTATATION DEFINED whether they are reported
> > as reads or writes and, in the case that they are generated by an AT
> > instruction, they are reported with the CM bit set.
> > 
> > All of this deeply confuses the logic in kvm_handle_guest_abort();
> > userspace may or may not see the fault, depending on whether it occurs
> > on the data or the instruction side, and an AT instruction may be skipped
> > if the translation tables are held in a read-only memslot.
> 
> Yuk, that's indeed ugly. Well spotted. I guess the saving grace is
> that a S2 trap caused by an ATS1 instruction will be reported as
> S1PTW+CM, while the fault caused by a CMO is reported as *either*
> S1PTW *or* CM, but never both.

Hmm, is that right? If the translation faults at S2 for a CM instruction,
wouldn't it have both bits set?

> > Move the handling of stage-2 faults on stage-1 page-table walks earlier
> > so that they consistently result in either a data or an instruction abort
> > being re-injected back to the guest.
> 
> The instruction abort seems to be happening as the side effect of
> executing outside of a memslot, not really because of a S1PTW.

Not sure about that. If the instruction fetch generates an S2 abort during
translation, then we could be executing from within a memslot; it's the
location of the page-tables that matters.

However, I think that means things still aren't quite right with my patches
because we can end up calling io_mem_abort() from an instruction abort,
which won't have enough syndrome information to do anything useful. Hmm.

Stepping back, here's what I _think_ we want, although see the '(?)'
bits where I'm particularly unsure:


S2 instruction abort:
  * Not in memslot:		inject external iabt to guest
  * In R/O memslot:
    - S2 fault on S1 walk:	either EXIT_NISV or inject external iabt
				to guest (?)

S2 data abort:
  * Not in memslot:
    - S2 fault on S1 walk:	inject external dabt to guest
    - Cache maintenance:	skip instr
    - Syndrome valid		EXIT_MMIO
    - Syndrome invalid		EXIT_NISV
  * In R/O memslot:
    - S2 fault on S1 walk:	either EXIT_NISV or inject external dabt
				to guest (?)
    - Access is write (including cache maintenance (?)):
      - Syndrome valid		EXIT_MMIO
      - Syndrome invalid	EXIT_NISV


Everything else gets handled by handle_access_fault()/user_mem_abort().

What do you think?

> I wonder whether these S1PTW faults should be classified as external
> aborts instead (because putting your page tables outside of a memslot
> seems a bit bonkers).

I think that's what this patch does, since we end up in kvm_inject_dabt().

Will
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/7] KVM: arm64: Rename kvm_vcpu_dabt_isextabt()
  2020-07-26 11:15   ` Marc Zyngier
@ 2020-07-27 10:30     ` Will Deacon
  0 siblings, 0 replies; 16+ messages in thread
From: Will Deacon @ 2020-07-27 10:30 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: kernel-team, kvmarm, linux-arm-kernel

On Sun, Jul 26, 2020 at 12:15:44PM +0100, Marc Zyngier wrote:
> On Fri, 24 Jul 2020 15:35:01 +0100,
> Will Deacon <will@kernel.org> wrote:
> > 
> > kvm_vcpu_dabt_isextabt() is not specific to data aborts and has nothing
> > to do with sign extension.
> 
> Not sign extension, but external abort (it reads as "is external
> abort?").
> 
> This is in keeping with all the other helpers (kvm_vcpu_foo_isbar). If
> you want to drop the "data" part (which is indeed not correct), how
> about kvm_vcpu_abt_isexternal? or kvm_vcpu_abt_issea?

kvm_vcpu_abt_issea() would be great -- the problem is that I confused
kvm_vcpu_dabt_issext() and kvm_vcpu_dabt_isextabt() when I was hacking
and if you get the wrong one then you're really hosed!

I'll update for v2.

Cheers,

Will
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/7] KVM: arm64: Update comment when skipping guest MMIO access instruction
  2020-07-26 11:08   ` Marc Zyngier
@ 2020-07-27 10:30     ` Will Deacon
  0 siblings, 0 replies; 16+ messages in thread
From: Will Deacon @ 2020-07-27 10:30 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: kernel-team, kvmarm, linux-arm-kernel

On Sun, Jul 26, 2020 at 12:08:28PM +0100, Marc Zyngier wrote:
> On Fri, 24 Jul 2020 15:35:00 +0100,
> Will Deacon <will@kernel.org> wrote:
> > 
> > If a 32-bit guest accesses MMIO using a 16-bit Thumb-2 instruction that
> > is reported to the hypervisor without a valid syndrom (for example,
> > because of the addressing mode), then we may hand off the fault to
> > userspace. When resuming the guest, we unconditionally advance the PC
> > by 4 bytes, since ESR_EL2.IL is always 1 for data aborts generated without
> > a valid syndrome. This is a bit rubbish, but it's also difficult to see
> > how we can fix it without potentially introducing regressions in userspace
> > MMIO fault handling.
> 
> Not quite, see below.
> 
> > 
> > Update the comment when skipping a guest MMIO access instruction so that
> > this corner case is at least written down.
> > 
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Quentin Perret <qperret@google.com>
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> >  arch/arm64/kvm/mmio.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/arch/arm64/kvm/mmio.c b/arch/arm64/kvm/mmio.c
> > index 4e0366759726..b54ea5aa6c06 100644
> > --- a/arch/arm64/kvm/mmio.c
> > +++ b/arch/arm64/kvm/mmio.c
> > @@ -113,6 +113,13 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >  	/*
> >  	 * The MMIO instruction is emulated and should not be re-executed
> >  	 * in the guest.
> > +	 *
> > +	 * Note: If user space handled the emulation because the abort
> > +	 * symdrome information was not valid (ISV set in the ESR), then
> 
> nits: syndrome, ISV *clear*.

Duh, thanks.

> > +	 * this will assume that the faulting instruction was 32-bit.
> > +	 * If the faulting instruction was a 16-bit Thumb instruction,
> > +	 * then userspace would need to rewind the PC by 2 bytes prior to
> > +	 * resuming the vCPU (yuck!).
> >  	 */
> >  	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> >  
> 
> That's not how I read it. On ESR_EL2.ISV being clear, and in the
> absence of KVM_CAP_ARM_NISV_TO_USER being set, we return a -ENOSYS from
> io_mem_abort(), exiting back to userspace *without* advertising a MMIO
> access. The VMM is free to do whatever it can to handle it (i.e. not
> much), but crucially we don't go via kvm_handle_mmio_return() on
> resuming the vcpu (unless the VMM sets run->exit_reason to
> KVM_EXIT_MMIO, but that's clearly its own decision).
> 
> Instead, the expectation is that userspace willing to handle an exit
> resulting in ESR_EL2.ISV being clear would instead request a
> KVM_EXIT_ARM_NISV exit type (by enabling KVM_CAP_ARM_NISV_TO_USER),
> getting extra information in the process such as as the fault
> IPA). KVM_EXIT_ARM_NISV clearly states in the documentation:
> 
>   "Note that KVM does not skip the faulting instruction as it does for
>    KVM_EXIT_MMIO, but userspace has to emulate any change to the
>    processing state if it decides to decode and emulate the instruction."

Thanks, I think you're right. I _thought_ we always reported EXIT_MMIO
for write faults on read-only memslots (as per the documented behaviour),
but actually that goes down the io_mem_abort() path too and so the
skipping only ever occurs when the syndrome is valid.

I'll drop this patch.

Will
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 5/7] KVM: arm64: Move 'invalid syndrome' logic out of io_mem_abort()
  2020-07-26 11:55   ` Marc Zyngier
@ 2020-07-27 10:31     ` Will Deacon
  0 siblings, 0 replies; 16+ messages in thread
From: Will Deacon @ 2020-07-27 10:31 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: kernel-team, kvmarm, linux-arm-kernel

On Sun, Jul 26, 2020 at 12:55:16PM +0100, Marc Zyngier wrote:
> On Fri, 24 Jul 2020 15:35:04 +0100,
> Will Deacon <will@kernel.org> wrote:
> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > index 73e62d360a36..adb933ecd177 100644
> > --- a/arch/arm64/kvm/mmu.c
> > +++ b/arch/arm64/kvm/mmu.c
> > @@ -2046,6 +2046,20 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
> >  		kvm_set_pfn_accessed(pfn);
> >  }
> >  
> > +static int handle_error_invalid_dabt(struct kvm_vcpu *vcpu, struct kvm_run *run,
> 
> Nit: why the "_error_"? There isn't any error here, only an awkward
> part of the architecture. I'd rather see something like
> handle_nisv_dabt(), which matches what this function actually does.

I chose "_error_" because this handling the case when kvm_is_error_hva() is
true (but I agree that "error" is misleading in both cases).

Will
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, back to index

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-24 14:34 [PATCH 0/7] KVM: arm64: Fixes to early stage-2 fault handling Will Deacon
2020-07-24 14:35 ` [PATCH 1/7] KVM: arm64: Update comment when skipping guest MMIO access instruction Will Deacon
2020-07-26 11:08   ` Marc Zyngier
2020-07-27 10:30     ` Will Deacon
2020-07-24 14:35 ` [PATCH 2/7] KVM: arm64: Rename kvm_vcpu_dabt_isextabt() Will Deacon
2020-07-26 11:15   ` Marc Zyngier
2020-07-27 10:30     ` Will Deacon
2020-07-24 14:35 ` [PATCH 3/7] KVM: arm64: Handle data and instruction external aborts the same way Will Deacon
2020-07-24 14:35 ` [PATCH 4/7] KVM: arm64: Remove useless local variable Will Deacon
2020-07-24 14:35 ` [PATCH 5/7] KVM: arm64: Move 'invalid syndrome' logic out of io_mem_abort() Will Deacon
2020-07-26 11:55   ` Marc Zyngier
2020-07-27 10:31     ` Will Deacon
2020-07-24 14:35 ` [PATCH 6/7] KVM: arm64: Handle stage-2 faults on stage-1 page-table walks earlier Will Deacon
2020-07-26 13:38   ` Marc Zyngier
2020-07-27 10:29     ` Will Deacon
2020-07-24 14:35 ` [PATCH 7/7] KVM: arm64: Separate write faults on read-only memslots from MMIO Will Deacon

KVM ARM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvmarm/0 kvmarm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvmarm kvmarm/ https://lore.kernel.org/kvmarm \
		kvmarm@lists.cs.columbia.edu
	public-inbox-index kvmarm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/edu.columbia.cs.lists.kvmarm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git