On Thu, Jan 19, 2023 at 5:18 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > +David and Ben > > On Tue, Dec 06, 2022, Yu Zhang wrote: > > Simplify the code by introducing a wrapper, mmu_is_direct(), > > instead of using vcpu->arch.mmu->root_role.direct everywhere. > > > > Meanwhile, use temporary variable 'direct', in routines such > > as kvm_mmu_load()/kvm_mmu_page_fault() etc. instead of checking > > vcpu->arch.mmu->root_role.direct repeatedly. > > I've looked at this patch at least four times and still can't decide whether or > not I like the helper. On one had, it's shorter and easier to read. On the other > hand, I don't love that mmu_is_nested() looks at a completely different MMU, which > is weird if not confusing. > > Anyone else have an opinion? The slightly shorter line length is kinda nice, but I don't think it really makes the code any clearer because any reader is still going to have to do the mental acrobatics to remember what exactly "direct" means, and why it matters in the given context. If there were some more useful function names we could wrap that check in, that might be nice. I don't see a ton of value otherwise. I'm thinking of something like "mmu_shadows_guest_mappings()" because that actually explains why we, for example, need to sync child SPTEs.
