On Mon, Apr 11, 2022 at 1:12 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > On Mon, Apr 11, 2022, David Matlack wrote: > > On Mon, Apr 11, 2022 at 10:12 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > > Circling back to eager page splitting, this series could be reworked to take the > > > first step of forking FNAME(page_fault), FNAME(fetch) and kvm_mmu_get_page() in > > > order to provide the necessary path for reworking nested MMU page faults. Then it > > > can remove unsync and shrinker support for nested MMUs. With those gone, > > > dissecting the nested MMU variant of kvm_mmu_get_page() should be simpler/cleaner > > > than dealing with the existing kvm_mmu_get_page(), i.e. should eliminate at least > > > some of the complexity/churn. > > > > These sound like useful improvements but I am not really seeing the > > value of sequencing them before this series: > > > > - IMO the "churn" in patches 1-14 are a net improvement to the > > existing code. They improve readability by decomposing the shadow page > > creation path into smaller functions with better names, reduce the > > amount of redundant calculations, and reduce the dependence on struct > > kvm_vcpu where it is not needed. Even if eager page splitting is > > completely dropped I think they would be useful to merge. > > I definitely like some of patches 1-14, probably most after a few read throughs. > But there are key parts that I do not like that are motivated almost entirely by > the desire to support page splitting. Specifically, I don't like splitting the > logic of finding a page, and I don't like having a separate alloc vs. initializer > (though I'm guessing this will be needed somewhere to split huge pages for nested > MMUs). > > E.g. I'd prefer the "get" flow look like the below (completely untested, for > discussion purposes only). There's still churn, but the core loop is almost > entirely unchanged. > > And it's not just this series, I don't want future improvements nested TDP to have > to deal with the legacy baggage. 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.
