| Message ID | 20220826231227.4096391-9-dmatlack@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86/mmu: Make tdp_mmu read-only and clean up TPD MMU fault handler | expand |
On Fri, Aug 26, 2022 at 04:12:25PM -0700, David Matlack <dmatlack@google.com> wrote: > Split out the page fault handling for the TDP MMU to a separate > function. This creates some duplicate code, but makes the TDP MMU fault > handler simpler to read by eliminating branches and will enable future > cleanups by allowing the TDP MMU and non-TDP MMU fault paths to diverge. > > Only compile in the TDP MMU fault handler for 64-bit builds since > kvm_tdp_mmu_map() does not exist in 32-bit builds. > > No functional change intended. > > Signed-off-by: David Matlack <dmatlack@google.com> > --- > arch/x86/kvm/mmu/mmu.c | 62 ++++++++++++++++++++++++++++++++---------- > 1 file changed, 48 insertions(+), 14 deletions(-) > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index a185599f4d1d..8f124a23ab4c 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -4242,7 +4242,6 @@ static bool is_page_fault_stale(struct kvm_vcpu *vcpu, > > static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) > { > - bool is_tdp_mmu_fault = is_tdp_mmu(vcpu->arch.mmu); > int r; > > if (page_fault_handle_page_track(vcpu, fault)) > @@ -4261,11 +4260,7 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault > return r; > > r = RET_PF_RETRY; > - > - if (is_tdp_mmu_fault) > - read_lock(&vcpu->kvm->mmu_lock); > - else > - write_lock(&vcpu->kvm->mmu_lock); > + write_lock(&vcpu->kvm->mmu_lock); > > if (is_page_fault_stale(vcpu, fault)) > goto out_unlock; > @@ -4274,16 +4269,10 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault > if (r) > goto out_unlock; > > - if (is_tdp_mmu_fault) > - r = kvm_tdp_mmu_map(vcpu, fault); > - else > - r = __direct_map(vcpu, fault); > + r = __direct_map(vcpu, fault); > > out_unlock: > - if (is_tdp_mmu_fault) > - read_unlock(&vcpu->kvm->mmu_lock); > - else > - write_unlock(&vcpu->kvm->mmu_lock); > + write_unlock(&vcpu->kvm->mmu_lock); > kvm_release_pfn_clean(fault->pfn); > return r; > } > @@ -4331,6 +4320,46 @@ int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code, > } > EXPORT_SYMBOL_GPL(kvm_handle_page_fault); > > +#ifdef CONFIG_X86_64 > +int kvm_tdp_mmu_page_fault(struct kvm_vcpu *vcpu, > + struct kvm_page_fault *fault) nitpick: static > +{ > + int r; > + > + if (page_fault_handle_page_track(vcpu, fault)) > + return RET_PF_EMULATE; > + > + r = fast_page_fault(vcpu, fault); > + if (r != RET_PF_INVALID) > + return r; > + > + r = mmu_topup_memory_caches(vcpu, false); > + if (r) > + return r; > + > + r = kvm_faultin_pfn(vcpu, fault, ACC_ALL); > + if (r != RET_PF_CONTINUE) > + return r; > + > + r = RET_PF_RETRY; > + read_lock(&vcpu->kvm->mmu_lock); > + > + if (is_page_fault_stale(vcpu, fault)) > + goto out_unlock; > + > + r = make_mmu_pages_available(vcpu); > + if (r) > + goto out_unlock; > + > + r = kvm_tdp_mmu_map(vcpu, fault); > + > +out_unlock: > + read_unlock(&vcpu->kvm->mmu_lock); > + kvm_release_pfn_clean(fault->pfn); > + return r; > +} > +#endif > + > int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) > { > /* > @@ -4355,6 +4384,11 @@ int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) > } > } > > +#ifdef CONFIG_X86_64 > + if (tdp_mmu_enabled) > + return kvm_tdp_mmu_page_fault(vcpu, fault); > +#endif > + > return direct_page_fault(vcpu, fault); > } Now we mostly duplicated page_fault method. We can go one step further. kvm->arch.mmu.page_fault can be set for each case. Maybe we can do it later if necessary.
On Tue, Aug 30, 2022 at 4:57 PM Isaku Yamahata <isaku.yamahata@gmail.com> wrote: > > On Fri, Aug 26, 2022 at 04:12:25PM -0700, > David Matlack <dmatlack@google.com> wrote: > > > Split out the page fault handling for the TDP MMU to a separate > > function. This creates some duplicate code, but makes the TDP MMU fault > > handler simpler to read by eliminating branches and will enable future > > cleanups by allowing the TDP MMU and non-TDP MMU fault paths to diverge. > > > > Only compile in the TDP MMU fault handler for 64-bit builds since > > kvm_tdp_mmu_map() does not exist in 32-bit builds. > > > > No functional change intended. > > > > Signed-off-by: David Matlack <dmatlack@google.com> > > --- > > arch/x86/kvm/mmu/mmu.c | 62 ++++++++++++++++++++++++++++++++---------- > > 1 file changed, 48 insertions(+), 14 deletions(-) > > > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > > index a185599f4d1d..8f124a23ab4c 100644 > > --- a/arch/x86/kvm/mmu/mmu.c > > +++ b/arch/x86/kvm/mmu/mmu.c > > @@ -4242,7 +4242,6 @@ static bool is_page_fault_stale(struct kvm_vcpu *vcpu, > > > > static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) > > { > > - bool is_tdp_mmu_fault = is_tdp_mmu(vcpu->arch.mmu); > > int r; > > > > if (page_fault_handle_page_track(vcpu, fault)) > > @@ -4261,11 +4260,7 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault > > return r; > > > > r = RET_PF_RETRY; > > - > > - if (is_tdp_mmu_fault) > > - read_lock(&vcpu->kvm->mmu_lock); > > - else > > - write_lock(&vcpu->kvm->mmu_lock); > > + write_lock(&vcpu->kvm->mmu_lock); > > > > if (is_page_fault_stale(vcpu, fault)) > > goto out_unlock; > > @@ -4274,16 +4269,10 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault > > if (r) > > goto out_unlock; > > > > - if (is_tdp_mmu_fault) > > - r = kvm_tdp_mmu_map(vcpu, fault); > > - else > > - r = __direct_map(vcpu, fault); > > + r = __direct_map(vcpu, fault); > > > > out_unlock: > > - if (is_tdp_mmu_fault) > > - read_unlock(&vcpu->kvm->mmu_lock); > > - else > > - write_unlock(&vcpu->kvm->mmu_lock); > > + write_unlock(&vcpu->kvm->mmu_lock); > > kvm_release_pfn_clean(fault->pfn); > > return r; > > } > > @@ -4331,6 +4320,46 @@ int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code, > > } > > EXPORT_SYMBOL_GPL(kvm_handle_page_fault); > > > > +#ifdef CONFIG_X86_64 > > +int kvm_tdp_mmu_page_fault(struct kvm_vcpu *vcpu, > > + struct kvm_page_fault *fault) > > nitpick: static Will do. > > > +{ > > + int r; > > + > > + if (page_fault_handle_page_track(vcpu, fault)) > > + return RET_PF_EMULATE; > > + > > + r = fast_page_fault(vcpu, fault); > > + if (r != RET_PF_INVALID) > > + return r; > > + > > + r = mmu_topup_memory_caches(vcpu, false); > > + if (r) > > + return r; > > + > > + r = kvm_faultin_pfn(vcpu, fault, ACC_ALL); > > + if (r != RET_PF_CONTINUE) > > + return r; > > + > > + r = RET_PF_RETRY; > > + read_lock(&vcpu->kvm->mmu_lock); > > + > > + if (is_page_fault_stale(vcpu, fault)) > > + goto out_unlock; > > + > > + r = make_mmu_pages_available(vcpu); > > + if (r) > > + goto out_unlock; > > + > > + r = kvm_tdp_mmu_map(vcpu, fault); > > + > > +out_unlock: > > + read_unlock(&vcpu->kvm->mmu_lock); > > + kvm_release_pfn_clean(fault->pfn); > > + return r; > > +} > > +#endif > > + > > int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) > > { > > /* > > @@ -4355,6 +4384,11 @@ int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) > > } > > } > > > > +#ifdef CONFIG_X86_64 > > + if (tdp_mmu_enabled) > > + return kvm_tdp_mmu_page_fault(vcpu, fault); > > +#endif > > + > > return direct_page_fault(vcpu, fault); > > } > > Now we mostly duplicated page_fault method. We can go one step further. > kvm->arch.mmu.page_fault can be set for each case. Maybe we can do it later > if necessary. Hm, interesting idea. We would have to refactor the MTRR max_level code in kvm_tdp_page_fault() into a helper function, but otherwise that idea would work. I will give it a try in the next version. > -- > Isaku Yamahata <isaku.yamahata@gmail.com>
On Thu, Sep 01, 2022 at 09:50:10AM -0700, David Matlack wrote: > On Tue, Aug 30, 2022 at 4:57 PM Isaku Yamahata <isaku.yamahata@gmail.com> wrote: > > On Fri, Aug 26, 2022 at 04:12:25PM -0700, David Matlack <dmatlack@google.com> wrote: > > > int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) > > > { > > > /* > > > @@ -4355,6 +4384,11 @@ int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) > > > } > > > } > > > > > > +#ifdef CONFIG_X86_64 > > > + if (tdp_mmu_enabled) > > > + return kvm_tdp_mmu_page_fault(vcpu, fault); > > > +#endif > > > + > > > return direct_page_fault(vcpu, fault); > > > } > > > > Now we mostly duplicated page_fault method. We can go one step further. > > kvm->arch.mmu.page_fault can be set for each case. Maybe we can do it later > > if necessary. > > Hm, interesting idea. We would have to refactor the MTRR max_level > code in kvm_tdp_page_fault() into a helper function, but otherwise > that idea would work. I will give it a try in the next version. So I took a stab at this. Refactoring the max_level adjustment for MTRRs into a helper function is easy of course. But changing page_fault also requires changes in kvm_mmu_do_page_fault() for CONFIG_RETPOLINE and fault->is_tdp. I'm not saying it's not possible (it definitely is) but it's not trivial to do it in a clean way, so I suggest we table this for the time being.
On Tue, Sep 20, 2022 at 02:17:20PM -0700, David Matlack <dmatlack@google.com> wrote: > On Thu, Sep 01, 2022 at 09:50:10AM -0700, David Matlack wrote: > > On Tue, Aug 30, 2022 at 4:57 PM Isaku Yamahata <isaku.yamahata@gmail.com> wrote: > > > On Fri, Aug 26, 2022 at 04:12:25PM -0700, David Matlack <dmatlack@google.com> wrote: > > > > int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) > > > > { > > > > /* > > > > @@ -4355,6 +4384,11 @@ int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) > > > > } > > > > } > > > > > > > > +#ifdef CONFIG_X86_64 > > > > + if (tdp_mmu_enabled) > > > > + return kvm_tdp_mmu_page_fault(vcpu, fault); > > > > +#endif > > > > + > > > > return direct_page_fault(vcpu, fault); > > > > } > > > > > > Now we mostly duplicated page_fault method. We can go one step further. > > > kvm->arch.mmu.page_fault can be set for each case. Maybe we can do it later > > > if necessary. > > > > Hm, interesting idea. We would have to refactor the MTRR max_level > > code in kvm_tdp_page_fault() into a helper function, but otherwise > > that idea would work. I will give it a try in the next version. > > So I took a stab at this. Refactoring the max_level adjustment for MTRRs > into a helper function is easy of course. But changing page_fault also > requires changes in kvm_mmu_do_page_fault() for CONFIG_RETPOLINE and > fault->is_tdp. I'm not saying it's not possible (it definitely is) but > it's not trivial to do it in a clean way, so I suggest we table this for > the time being. Fair enough.
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index a185599f4d1d..8f124a23ab4c 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -4242,7 +4242,6 @@ static bool is_page_fault_stale(struct kvm_vcpu *vcpu, static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) { - bool is_tdp_mmu_fault = is_tdp_mmu(vcpu->arch.mmu); int r; if (page_fault_handle_page_track(vcpu, fault)) @@ -4261,11 +4260,7 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault return r; r = RET_PF_RETRY; - - if (is_tdp_mmu_fault) - read_lock(&vcpu->kvm->mmu_lock); - else - write_lock(&vcpu->kvm->mmu_lock); + write_lock(&vcpu->kvm->mmu_lock); if (is_page_fault_stale(vcpu, fault)) goto out_unlock; @@ -4274,16 +4269,10 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault if (r) goto out_unlock; - if (is_tdp_mmu_fault) - r = kvm_tdp_mmu_map(vcpu, fault); - else - r = __direct_map(vcpu, fault); + r = __direct_map(vcpu, fault); out_unlock: - if (is_tdp_mmu_fault) - read_unlock(&vcpu->kvm->mmu_lock); - else - write_unlock(&vcpu->kvm->mmu_lock); + write_unlock(&vcpu->kvm->mmu_lock); kvm_release_pfn_clean(fault->pfn); return r; } @@ -4331,6 +4320,46 @@ int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code, } EXPORT_SYMBOL_GPL(kvm_handle_page_fault); +#ifdef CONFIG_X86_64 +int kvm_tdp_mmu_page_fault(struct kvm_vcpu *vcpu, + struct kvm_page_fault *fault) +{ + int r; + + if (page_fault_handle_page_track(vcpu, fault)) + return RET_PF_EMULATE; + + r = fast_page_fault(vcpu, fault); + if (r != RET_PF_INVALID) + return r; + + r = mmu_topup_memory_caches(vcpu, false); + if (r) + return r; + + r = kvm_faultin_pfn(vcpu, fault, ACC_ALL); + if (r != RET_PF_CONTINUE) + return r; + + r = RET_PF_RETRY; + read_lock(&vcpu->kvm->mmu_lock); + + if (is_page_fault_stale(vcpu, fault)) + goto out_unlock; + + r = make_mmu_pages_available(vcpu); + if (r) + goto out_unlock; + + r = kvm_tdp_mmu_map(vcpu, fault); + +out_unlock: + read_unlock(&vcpu->kvm->mmu_lock); + kvm_release_pfn_clean(fault->pfn); + return r; +} +#endif + int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) { /* @@ -4355,6 +4384,11 @@ int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) } } +#ifdef CONFIG_X86_64 + if (tdp_mmu_enabled) + return kvm_tdp_mmu_page_fault(vcpu, fault); +#endif + return direct_page_fault(vcpu, fault); }
Split out the page fault handling for the TDP MMU to a separate function. This creates some duplicate code, but makes the TDP MMU fault handler simpler to read by eliminating branches and will enable future cleanups by allowing the TDP MMU and non-TDP MMU fault paths to diverge. Only compile in the TDP MMU fault handler for 64-bit builds since kvm_tdp_mmu_map() does not exist in 32-bit builds. No functional change intended. Signed-off-by: David Matlack <dmatlack@google.com> --- arch/x86/kvm/mmu/mmu.c | 62 ++++++++++++++++++++++++++++++++---------- 1 file changed, 48 insertions(+), 14 deletions(-)