On Tue, Aug 30, 2022 at 4:57 PM Isaku Yamahata <isaku.yamahata@xxxxxxxxx> wrote:
>
> On Fri, Aug 26, 2022 at 04:12:25PM -0700,
> David Matlack <dmatlack@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
