On Fri, Jan 20, 2023, Ben Gardon wrote: > 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. Ugh, right, thanks to nonpaging mode, something like is_shadow_mmu() isn't correct even if/when KVM drops support for 32-bit builds. Yu, Unless you feel strongly about this one, I'm inclined to leave things as-is.
