Re: [PATCH] KVM: MMU: Add wrapper to check whether MMU is in direct mode

[Sean Christopherson <seanjc@google.com>]

On Fri, Jan 20, 2023, Ben Gardon wrote:
> On Thu, Jan 19, 2023 at 5:18 PM Sean Christopherson <seanjc@google.com> 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.