On Wed, Apr 13, 2022 at 01:02:51AM +0000, Sean Christopherson wrote:
> On Tue, Apr 12, 2022, David Matlack wrote:
> > On Mon, Apr 11, 2022 at 5:39 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > >
> > > On Mon, Apr 11, 2022, David Matlack wrote:
> > > >
> > > > One thing that would be helpful is if you can explain in a bit more
> > > > specifically what you'd like to see. Part of the reason why I prefer
> > > > to sequence your proposal after eager page splitting is that I do not
> > > > fully understand what you're proposing, and how complex it would be.
> > > > e.g. Forking FNAME(fetch), FNAME(page_fault), and kvm_mmu_get_page()
> > > > for nested MMUs does not sound like less churn.
> > >
> > > Oh, it's most definitely not less code, and probably more churn. But, it's churn
> > > that pushes us in a more favorable direction and that is desirable long term. I
> > > don't mind churning code, but I want the churn to make future life easier, not
> > > harder. Details below.
> >
> > Of course. Let's make sure we're on the same page about what churn
> > introduced by this series will make future life harder that we hope to
> > avoid. If I understand you correctly, it's the following 2 changes:
> >
> > (a.) Using separate functions to allocate SPs and initialize SPs.
> > (b.) Separating kvm_mmu_find_shadow_page() from __kvm_mmu_find_shadow_page().
> >
> > (a.) stems from the fact that SP allocation during eager page
> > splitting is made directly rather than through kvm_mmu_memory_caches,
> > which was what you pushed for in the TDP MMU implementation. We could
> > instead use kvm_mmu_memory_caches for the shadow MMU eager page
>
> ...
>
> > So even if we did everything you proposed (which seems like an awful
> > lot just to avoid __kvm_mmu_find_shadow_page()), there's a chance we
> > would still end up with the exact same code. i.e.
> > kvm_mmu_nested_tdp_find_sp() would be implemented by calling
> > __kvm_mmu_find_shadow_page(), because it would be a waste to
> > re-implement an almost identical function?
>
> I went far enough down this path to know that my idea isn't completely awful,
> and wouldn't actually need to fork FNAME(page_fault) at this time, but sadly I
> still dislike the end result.
Thanks for looking into it so quickly so we could figure out a path
forward.
>
> Your assessment that the we'd still end up with very similar (if not quite exact)
> code is spot on. Ditto for your other assertion in (a) about using the caches.
>
> My vote for this series is to go the cache route, e.g. wrap kvm_mmu_memory_caches
> in a struct and pass that into kvm_mmu_get_page(). I still think it was the right
> call to ignore the caches for the TDP MMU, it gives the TDP MMU more flexibility
> and it was trivial to bypass the caches since the TDP MMU was doing its own thing
> anyways.
>
> But for the shadow MMU, IMO the cons outweigh the pros. E.g. in addition to
> ending up with two similar but subtly different "get page" flows, passing around
> "struct kvm_mmu_page **spp" is a bit unpleasant. Ditto for having a partially
> initialized kvm_mmu_page. The split code also ends up in a wierd state where it
> uses the caches for the pte_list, but not the other allocations.
Sounds good. I will rework the series to use kvm_mmu_memory_cache
structs for the SP allocation during eager page splitting. That will
eliminate the separate allocation and initialization which will be a
nice cleanup. And it will be great to get rid of the spp crud.
And per your earlier feedback, I will also limit eager page splitting to
nested MMUs.
>
> There will be one wart due to unsync pages needing @vcpu, but we can pass in NULL
> for the split case and assert that @vcpu is non-null since all of the children
> should be direct.
The NULL vcpu check will be a little gross, but it should never trigger
in practice since eager page splitting always requests direct SPs. My
preference has been to enforce that in code by splitting out
__kvm_mmu_find_shadow_page(), but I can see the advantage of your
proposal is that eager page splitting and faults will go through the
exact same code path to get a kvm_mmu_page.
>
> if (sp->unsync) {
> if (WARN_ON_ONCE(!vcpu)) {
> kvm_mmu_prepare_zap_page(kvm, sp,
> &invalid_list);
> continue;
> }
>
> /*
> * The page is good, but is stale. kvm_sync_page does
> * get the latest guest state, but (unlike mmu_unsync_children)
> * it doesn't write-protect the page or mark it synchronized!
> * This way the validity of the mapping is ensured, but the
> * overhead of write protection is not incurred until the
> * guest invalidates the TLB mapping. This allows multiple
> * SPs for a single gfn to be unsync.
> *
> * If the sync fails, the page is zapped. If so, break
> * in order to rebuild it.
> */
> if (!kvm_sync_page(vcpu, sp, &invalid_list))
> break;
>
> WARN_ON(!list_empty(&invalid_list));
> kvm_flush_remote_tlbs(kvm);
> }
