| Message ID | 20200320212833.3507-6-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: TLB flushing fixes and enhancements | expand |
Sean Christopherson <sean.j.christopherson@intel.com> writes: > Export the page fault propagation helper so that VMX can use it to > correctly emulate TLB invalidation on page faults in an upcoming patch. > > In the (hopefully) not-too-distant future, SGX virtualization will also > want access to the helper for injecting page faults to the correct level > (L1 vs. L2) when emulating ENCLS instructions. > > Rename the function to kvm_inject_emulated_page_fault() to clarify that > it is (a) injecting a fault and (b) only for page faults. WARN if it's > invoked with an exception other than PF_VECTOR. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/include/asm/kvm_host.h | 2 ++ > arch/x86/kvm/x86.c | 8 ++++++-- > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 9a183e9d4cb1..328b1765ff76 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1447,6 +1447,8 @@ void kvm_queue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code); > void kvm_requeue_exception(struct kvm_vcpu *vcpu, unsigned nr); > void kvm_requeue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code); > void kvm_inject_page_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault); > +bool kvm_inject_emulated_page_fault(struct kvm_vcpu *vcpu, > + struct x86_exception *fault); > int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, > gfn_t gfn, void *data, int offset, int len, > u32 access); > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index e54c6ad628a8..64ed6e6e2b56 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -611,8 +611,11 @@ void kvm_inject_page_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault) > } > EXPORT_SYMBOL_GPL(kvm_inject_page_fault); > > -static bool kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault) > +bool kvm_inject_emulated_page_fault(struct kvm_vcpu *vcpu, > + struct x86_exception *fault) > { > + WARN_ON_ONCE(fault->vector != PF_VECTOR); > + > if (mmu_is_nested(vcpu) && !fault->nested_page_fault) > vcpu->arch.nested_mmu.inject_page_fault(vcpu, fault); > else > @@ -620,6 +623,7 @@ static bool kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fau > > return fault->nested_page_fault; > } > +EXPORT_SYMBOL_GPL(kvm_inject_emulated_page_fault); We don't seem to use the return value a lot, actually, inject_emulated_exception() seems to be the only one, the rest just call it without checking the return value. Judging by the new name, I'd guess that the function returns whether it was able to inject the exception or not but this doesn't seem to be the case. My suggestion would then be to make it return 'void' and return 'fault->nested_page_fault' separately in inject_emulated_exception(). > > void kvm_inject_nmi(struct kvm_vcpu *vcpu) > { > @@ -6373,7 +6377,7 @@ static bool inject_emulated_exception(struct kvm_vcpu *vcpu) > { > struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt; > if (ctxt->exception.vector == PF_VECTOR) > - return kvm_propagate_fault(vcpu, &ctxt->exception); > + return kvm_inject_emulated_page_fault(vcpu, &ctxt->exception); > > if (ctxt->exception.error_code_valid) > kvm_queue_exception_e(vcpu, ctxt->exception.vector, With or without the change suggested above, Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
On Mon, Mar 23, 2020 at 04:47:49PM +0100, Vitaly Kuznetsov wrote: > Sean Christopherson <sean.j.christopherson@intel.com> writes: > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index e54c6ad628a8..64ed6e6e2b56 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -611,8 +611,11 @@ void kvm_inject_page_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault) > > } > > EXPORT_SYMBOL_GPL(kvm_inject_page_fault); > > > > -static bool kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault) > > +bool kvm_inject_emulated_page_fault(struct kvm_vcpu *vcpu, > > + struct x86_exception *fault) > > { > > + WARN_ON_ONCE(fault->vector != PF_VECTOR); > > + > > if (mmu_is_nested(vcpu) && !fault->nested_page_fault) > > vcpu->arch.nested_mmu.inject_page_fault(vcpu, fault); > > else > > @@ -620,6 +623,7 @@ static bool kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fau > > > > return fault->nested_page_fault; > > } > > +EXPORT_SYMBOL_GPL(kvm_inject_emulated_page_fault); > > We don't seem to use the return value a lot, actually, > inject_emulated_exception() seems to be the only one, the rest just call > it without checking the return value. Judging by the new name, I'd guess > that the function returns whether it was able to inject the exception or > not but this doesn't seem to be the case. My suggestion would then be to > make it return 'void' and return 'fault->nested_page_fault' separately > in inject_emulated_exception(). Oooh, I like that idea. The return from the common helper also confuses me every time I look at it. > > void kvm_inject_nmi(struct kvm_vcpu *vcpu) > > { > > @@ -6373,7 +6377,7 @@ static bool inject_emulated_exception(struct kvm_vcpu *vcpu) > > { > > struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt; > > if (ctxt->exception.vector == PF_VECTOR) > > - return kvm_propagate_fault(vcpu, &ctxt->exception); > > + return kvm_inject_emulated_page_fault(vcpu, &ctxt->exception); > > > > if (ctxt->exception.error_code_valid) > > kvm_queue_exception_e(vcpu, ctxt->exception.vector, > > With or without the change suggested above, > > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > -- > Vitaly >
On 23/03/20 17:24, Sean Christopherson wrote: >> We don't seem to use the return value a lot, actually, >> inject_emulated_exception() seems to be the only one, the rest just call >> it without checking the return value. Judging by the new name, I'd guess >> that the function returns whether it was able to inject the exception or >> not but this doesn't seem to be the case. My suggestion would then be to >> make it return 'void' and return 'fault->nested_page_fault' separately >> in inject_emulated_exception(). > Oooh, I like that idea. The return from the common helper also confuses me > every time I look at it. > Separate patch, please. I'm not sure it makes a great difference though. Paolo
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 9a183e9d4cb1..328b1765ff76 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1447,6 +1447,8 @@ void kvm_queue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code); void kvm_requeue_exception(struct kvm_vcpu *vcpu, unsigned nr); void kvm_requeue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code); void kvm_inject_page_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault); +bool kvm_inject_emulated_page_fault(struct kvm_vcpu *vcpu, + struct x86_exception *fault); int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, gfn_t gfn, void *data, int offset, int len, u32 access); diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index e54c6ad628a8..64ed6e6e2b56 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -611,8 +611,11 @@ void kvm_inject_page_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault) } EXPORT_SYMBOL_GPL(kvm_inject_page_fault); -static bool kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault) +bool kvm_inject_emulated_page_fault(struct kvm_vcpu *vcpu, + struct x86_exception *fault) { + WARN_ON_ONCE(fault->vector != PF_VECTOR); + if (mmu_is_nested(vcpu) && !fault->nested_page_fault) vcpu->arch.nested_mmu.inject_page_fault(vcpu, fault); else @@ -620,6 +623,7 @@ static bool kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fau return fault->nested_page_fault; } +EXPORT_SYMBOL_GPL(kvm_inject_emulated_page_fault); void kvm_inject_nmi(struct kvm_vcpu *vcpu) { @@ -6373,7 +6377,7 @@ static bool inject_emulated_exception(struct kvm_vcpu *vcpu) { struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt; if (ctxt->exception.vector == PF_VECTOR) - return kvm_propagate_fault(vcpu, &ctxt->exception); + return kvm_inject_emulated_page_fault(vcpu, &ctxt->exception); if (ctxt->exception.error_code_valid) kvm_queue_exception_e(vcpu, ctxt->exception.vector,
Export the page fault propagation helper so that VMX can use it to correctly emulate TLB invalidation on page faults in an upcoming patch. In the (hopefully) not-too-distant future, SGX virtualization will also want access to the helper for injecting page faults to the correct level (L1 vs. L2) when emulating ENCLS instructions. Rename the function to kvm_inject_emulated_page_fault() to clarify that it is (a) injecting a fault and (b) only for page faults. WARN if it's invoked with an exception other than PF_VECTOR. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/include/asm/kvm_host.h | 2 ++ arch/x86/kvm/x86.c | 8 ++++++-- 2 files changed, 8 insertions(+), 2 deletions(-)