Skip to content

Commit 5276c61

Browse files
sean-jcbonzini
sean-jc
authored and
bonzini
committed
KVM: x86/mmu: Add RET_PF_CONTINUE to eliminate bool+int* "returns"
Add RET_PF_CONTINUE and use it in handle_abnormal_pfn() and kvm_faultin_pfn() to signal that the page fault handler should continue doing its thing. Aside from being gross and inefficient, using a boolean return to signal continue vs. stop makes it extremely difficult to add more helpers and/or move existing code to a helper. E.g. hypothetically, if nested MMUs were to gain a separate page fault handler in the future, everything up to the "is self-modifying PTE" check can be shared by all shadow MMUs, but communicating up the stack whether to continue on or stop becomes a nightmare. More concretely, proposed support for private guest memory ran into a similar issue, where it'll be forced to forego a helper in order to yield sane code: https://lore.kernel.org/all/YkJbxiL%2FAz7olWlq@google.com. No functional change intended. Cc: David Matlack <dmatlack@google.com> Cc: Chao Peng <chao.p.peng@linux.intel.com> Signed-off-by: Sean Christopherson <seanjc@google.com> Message-Id: <20220423034752.1161007-7-seanjc@google.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
1 parent 5c64aba commit 5276c61

4 files changed

Lines changed: 35 additions & 32 deletions

File tree

‎arch/x86/kvm/mmu/mmu.c‎

Lines changed: 22 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -3007,14 +3007,12 @@ static int kvm_handle_bad_page(struct kvm_vcpu *vcpu, gfn_t gfn, kvm_pfn_t pfn)
30073007
return -EFAULT;
30083008
}
30093009

3010-
static bool handle_abnormal_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault,
3011-
unsigned int access, int *ret_val)
3010+
static int handle_abnormal_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault,
3011+
unsigned int access)
30123012
{
30133013
/* The pfn is invalid, report the error! */
3014-
if (unlikely(is_error_pfn(fault->pfn))) {
3015-
*ret_val = kvm_handle_bad_page(vcpu, fault->gfn, fault->pfn);
3016-
return true;
3017-
}
3014+
if (unlikely(is_error_pfn(fault->pfn)))
3015+
return kvm_handle_bad_page(vcpu, fault->gfn, fault->pfn);
30183016

30193017
if (unlikely(!fault->slot)) {
30203018
gva_t gva = fault->is_tdp ? 0 : fault->addr;
@@ -3032,13 +3030,11 @@ static bool handle_abnormal_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fa
30323030
* the hardware's).
30333031
*/
30343032
if (unlikely(!enable_mmio_caching) ||
3035-
unlikely(fault->gfn > kvm_mmu_max_gfn())) {
3036-
*ret_val = RET_PF_EMULATE;
3037-
return true;
3038-
}
3033+
unlikely(fault->gfn > kvm_mmu_max_gfn()))
3034+
return RET_PF_EMULATE;
30393035
}
30403036

3041-
return false;
3037+
return RET_PF_CONTINUE;
30423038
}
30433039

30443040
static bool page_fault_can_be_fast(struct kvm_page_fault *fault)
@@ -3946,7 +3942,7 @@ static bool kvm_arch_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
39463942
kvm_vcpu_gfn_to_hva(vcpu, gfn), &arch);
39473943
}
39483944

3949-
static bool kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault, int *r)
3945+
static int kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
39503946
{
39513947
struct kvm_memory_slot *slot = fault->slot;
39523948
bool async;
@@ -3957,15 +3953,15 @@ static bool kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault,
39573953
* be zapped before KVM inserts a new MMIO SPTE for the gfn.
39583954
*/
39593955
if (slot && (slot->flags & KVM_MEMSLOT_INVALID))
3960-
goto out_retry;
3956+
return RET_PF_RETRY;
39613957

39623958
if (!kvm_is_visible_memslot(slot)) {
39633959
/* Don't expose private memslots to L2. */
39643960
if (is_guest_mode(vcpu)) {
39653961
fault->slot = NULL;
39663962
fault->pfn = KVM_PFN_NOSLOT;
39673963
fault->map_writable = false;
3968-
return false;
3964+
return RET_PF_CONTINUE;
39693965
}
39703966
/*
39713967
* If the APIC access page exists but is disabled, go directly
@@ -3974,37 +3970,32 @@ static bool kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault,
39743970
* when the AVIC is re-enabled.
39753971
*/
39763972
if (slot && slot->id == APIC_ACCESS_PAGE_PRIVATE_MEMSLOT &&
3977-
!kvm_apicv_activated(vcpu->kvm)) {
3978-
*r = RET_PF_EMULATE;
3979-
return true;
3980-
}
3973+
!kvm_apicv_activated(vcpu->kvm))
3974+
return RET_PF_EMULATE;
39813975
}
39823976

39833977
async = false;
39843978
fault->pfn = __gfn_to_pfn_memslot(slot, fault->gfn, false, &async,
39853979
fault->write, &fault->map_writable,
39863980
&fault->hva);
39873981
if (!async)
3988-
return false; /* *pfn has correct page already */
3982+
return RET_PF_CONTINUE; /* *pfn has correct page already */
39893983

39903984
if (!fault->prefetch && kvm_can_do_async_pf(vcpu)) {
39913985
trace_kvm_try_async_get_page(fault->addr, fault->gfn);
39923986
if (kvm_find_async_pf_gfn(vcpu, fault->gfn)) {
39933987
trace_kvm_async_pf_doublefault(fault->addr, fault->gfn);
39943988
kvm_make_request(KVM_REQ_APF_HALT, vcpu);
3995-
goto out_retry;
3996-
} else if (kvm_arch_setup_async_pf(vcpu, fault->addr, fault->gfn))
3997-
goto out_retry;
3989+
return RET_PF_RETRY;
3990+
} else if (kvm_arch_setup_async_pf(vcpu, fault->addr, fault->gfn)) {
3991+
return RET_PF_RETRY;
3992+
}
39983993
}
39993994

40003995
fault->pfn = __gfn_to_pfn_memslot(slot, fault->gfn, false, NULL,
40013996
fault->write, &fault->map_writable,
40023997
&fault->hva);
4003-
return false;
4004-
4005-
out_retry:
4006-
*r = RET_PF_RETRY;
4007-
return true;
3998+
return RET_PF_CONTINUE;
40083999
}
40094000

40104001
/*
@@ -4059,10 +4050,12 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
40594050
mmu_seq = vcpu->kvm->mmu_notifier_seq;
40604051
smp_rmb();
40614052

4062-
if (kvm_faultin_pfn(vcpu, fault, &r))
4053+
r = kvm_faultin_pfn(vcpu, fault);
4054+
if (r != RET_PF_CONTINUE)
40634055
return r;
40644056

4065-
if (handle_abnormal_pfn(vcpu, fault, ACC_ALL, &r))
4057+
r = handle_abnormal_pfn(vcpu, fault, ACC_ALL);
4058+
if (r != RET_PF_CONTINUE)
40664059
return r;
40674060

40684061
r = RET_PF_RETRY;

‎arch/x86/kvm/mmu/mmu_internal.h‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,7 @@ unsigned int pte_list_count(struct kvm_rmap_head *rmap_head);
143143
/*
144144
* Return values of handle_mmio_page_fault, mmu.page_fault, and fast_page_fault().
145145
*
146+
* RET_PF_CONTINUE: So far, so good, keep handling the page fault.
146147
* RET_PF_RETRY: let CPU fault again on the address.
147148
* RET_PF_EMULATE: mmio page fault, emulate the instruction directly.
148149
* RET_PF_INVALID: the spte is invalid, let the real page fault path update it.
@@ -151,9 +152,15 @@ unsigned int pte_list_count(struct kvm_rmap_head *rmap_head);
151152
*
152153
* Any names added to this enum should be exported to userspace for use in
153154
* tracepoints via TRACE_DEFINE_ENUM() in mmutrace.h
155+
*
156+
* Note, all values must be greater than or equal to zero so as not to encroach
157+
* on -errno return values. Somewhat arbitrarily use '0' for CONTINUE, which
158+
* will allow for efficient machine code when checking for CONTINUE, e.g.
159+
* "TEST %rax, %rax, JNZ", as all "stop!" values are non-zero.
154160
*/
155161
enum {
156-
RET_PF_RETRY = 0,
162+
RET_PF_CONTINUE = 0,
163+
RET_PF_RETRY,
157164
RET_PF_EMULATE,
158165
RET_PF_INVALID,
159166
RET_PF_FIXED,

‎arch/x86/kvm/mmu/mmutrace.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@
5454
{ PFERR_RSVD_MASK, "RSVD" }, \
5555
{ PFERR_FETCH_MASK, "F" }
5656

57+
TRACE_DEFINE_ENUM(RET_PF_CONTINUE);
5758
TRACE_DEFINE_ENUM(RET_PF_RETRY);
5859
TRACE_DEFINE_ENUM(RET_PF_EMULATE);
5960
TRACE_DEFINE_ENUM(RET_PF_INVALID);

‎arch/x86/kvm/mmu/paging_tmpl.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -838,10 +838,12 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
838838
mmu_seq = vcpu->kvm->mmu_notifier_seq;
839839
smp_rmb();
840840

841-
if (kvm_faultin_pfn(vcpu, fault, &r))
841+
r = kvm_faultin_pfn(vcpu, fault);
842+
if (r != RET_PF_CONTINUE)
842843
return r;
843844

844-
if (handle_abnormal_pfn(vcpu, fault, walker.pte_access, &r))
845+
r = handle_abnormal_pfn(vcpu, fault, walker.pte_access);
846+
if (r != RET_PF_CONTINUE)
845847
return r;
846848

847849
/*

0 commit comments

Comments
 (0)