diff mbox series

[v5,06/17] KVM: x86: Annotate -EFAULTs from kvm_handle_error_pfn()

Message ID 20230908222905.1321305-7-amoorthy@google.com (mailing list archive)
State New, archived
Headers show
Series Improve KVM + userfaultfd live migration via annotated memory faults. | expand

Commit Message

Anish Moorthy Sept. 8, 2023, 10:28 p.m. UTC
Implement KVM_CAP_MEMORY_FAULT_INFO for efaults generated by
kvm_handle_error_pfn().

Signed-off-by: Anish Moorthy <amoorthy@google.com>
---
 arch/x86/kvm/mmu/mmu.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Sean Christopherson Oct. 5, 2023, 1:26 a.m. UTC | #1
On Fri, Sep 08, 2023, Anish Moorthy wrote:
> Implement KVM_CAP_MEMORY_FAULT_INFO for efaults generated by
> kvm_handle_error_pfn().

Rewrite with --verbose please.  And avoid function names if possible.  Sometimes
it's better/useful/necessary to reference a function by name, but in this case,
just saying kvm_handle_error_pfn() isn't helpful because it doesn't provide any
insight into the actual impact of the change, i.e. requires the reader to already
know exactly how and when kvm_handle_error_pfn() is used.
Anish Moorthy Oct. 5, 2023, 11:57 p.m. UTC | #2
On Wed, Oct 4, 2023 at 6:26 PM Sean Christopherson <seanjc@google.com> wrote:
>
> ...isn't helpful because it doesn't provide any
> insight into the actual impact of the change, i.e. requires the reader to already
> know exactly how and when kvm_handle_error_pfn() is used.

Ah, true. Hopefully the following is better?

> KVM: x86: Annotate hva->hpa translation failures in stage-2 fault path
>
> Set up a KVM_EXIT_MEMORY_FAULT in cases where the stage-2 fault exit
> handler successfully translates a GPA to an HVA, but fails to translate
> the HVA to an HPA via GUP. This provides information to userspace which it
> could potentially use, for instance to resolve the failure.

Maybe the "via GUP" isn't really doing anything there
Sean Christopherson Oct. 6, 2023, 12:36 a.m. UTC | #3
On Thu, Oct 05, 2023, Anish Moorthy wrote:
> On Wed, Oct 4, 2023 at 6:26 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > ...isn't helpful because it doesn't provide any
> > insight into the actual impact of the change, i.e. requires the reader to already
> > know exactly how and when kvm_handle_error_pfn() is used.
> 
> Ah, true. Hopefully the following is better?
> 
> > KVM: x86: Annotate hva->hpa translation failures in stage-2 fault path
> >
> > Set up a KVM_EXIT_MEMORY_FAULT in cases where the stage-2 fault exit
> > handler successfully translates a GPA to an HVA, but fails to translate
> > the HVA to an HPA via GUP. This provides information to userspace which it
> > could potentially use, for instance to resolve the failure.
> 
> Maybe the "via GUP" isn't really doing anything there

Yeah, drop GUP.  Not all pfns are resolved via gup(), e.g. see hva_to_pfn_remapped().

Actually, I would hedge on the "HVA" part too.  It's an accurate statement for
both ARM and x86, but only because writes to readonly slots get treated as MMIO
(stupid option ROMs).

And what part of the translation fails isn't important, what's important is that
KVM is exiting to userspace with an error.  Using "can't resolve the pfn" is fine
and desirable, but the changelog should first focus on the important outcome,
which is that KVM is now providing more information when throwing an -EFAULT at
userspace.

Something like this?

  When handling a stage-2 fault, set up a KVM_EXIT_MEMORY_FAULT exit if KVM
  returns -EFAULT to userspace, e.g. if resolving the pfn fails for whatever
  reason.  Providing information about the unhandled fault will allow
  userspace to potentially fix the source of the error and avoid terminating
  the guest.
diff mbox series

Patch

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index e1d011c67cc6..deae8ac74d9a 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -3267,6 +3267,8 @@  static void kvm_send_hwpoison_signal(struct kvm_memory_slot *slot, gfn_t gfn)
 
 static int kvm_handle_error_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
 {
+	u64 fault_flags;
+
 	if (is_sigpending_pfn(fault->pfn)) {
 		kvm_handle_signal_exit(vcpu);
 		return -EINTR;
@@ -3285,6 +3287,17 @@  static int kvm_handle_error_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fa
 		return RET_PF_RETRY;
 	}
 
+	WARN_ON_ONCE(fault->goal_level != PG_LEVEL_4K);
+
+	fault_flags = 0;
+	if (fault->write)
+		fault_flags = KVM_MEMORY_FAULT_FLAG_WRITE;
+	else if (fault->exec)
+		fault_flags = KVM_MEMORY_FAULT_FLAG_EXEC;
+	else
+		fault_flags = KVM_MEMORY_FAULT_FLAG_READ;
+	kvm_handle_guest_uaccess_fault(vcpu, gfn_to_gpa(fault->gfn), PAGE_SIZE,
+				       fault_flags);
 	return -EFAULT;
 }