On Wed, Apr 13, 2022, Paolo Bonzini wrote:
> On 4/13/22 17:32, Sean Christopherson wrote:
> > > > Are we planning on removing direct?
> > >
> > > I think so, it's redundant and the code almost always checks
> > > direct||passthrough (which would be passthrough_delta > 0 with your scheme).
> >
> > I'm ok dropping direct and rolling it into target_level, just so long as we add
> > helpers, e.g. IIUC they would be
> >
> > static inline bool is_sp_direct(...)
> > {
> > return !sp->role.target_level;
> > }
> >
> > static inline bool is_sp_direct_or_passthrough(...)
> > {
> > return sp->role.target_level != sp->role.level;
> > }
>
> Yes of course. Or respectively:
>
> return sp->role.passthrough_levels == s->role.level;
>
> return sp->role.passthrough_levels > 0;
>
> I'm not sure about a more concise name for the latter. Maybe
> sp_has_gpt(...) but I haven't thought it through very much.
>
> > > > Hmm, it's not a raw level though.
> > >
> > > Hence the plural. :)
> >
> > LOL, I honestly thought that was a typo. Making it plural sounds like it's passing
> > through to multiple levels.
>
> I meant it as number of levels being passed through. I'll leave that to
> Jiangshan, either target_level or passthrough_levels will do for me.
It took me until like 9pm last night to finally understand what you meant by
"passthrough level". Now that I actually have my head wrapped around this...
Stepping back, "glevel" and any of its derivations is actually just a combination
of CR0.PG, CR4.PAE, EFER.LMA, and CR4.LA57. And "has_4_byte_gpte" is CR0.PG && !CR4.PAE.
When running with !tdp_enabled, CR0.PG is tracked by "direct". And with TDP enabled,
CR0.PG is either a don't care (L1 or nested EPT), or is guaranteed to be '1' (nested NPT).
So, rather than add yet more synthetic information to the role, what about using
the info we already have? I don't think it changes the number of bits that need to
be stored, but I think the result would be easier for people to understand, at
least superficially, e.g. "oh, the mode matters, got it". We'd need a beefy comment
to explain the whole "passthrough levels" thing, but I think it the code would be
more approachable for most people.
If we move efer_lma and cr4_la57 from kvm_mmu_extended_role to kvm_mmu_page_role,
and rename has_4_byte_gpte to cr4_pae, then we don't need passthrough_levels.
If needed for performance, we could still have a "passthrough" bit, but otherwise
detecting a passthrough SP would be
static inline bool is_passthrough_sp(struct kvm_mmu_page *sp)
{
return !sp->role.direct && sp->role.level > role_to_root_level(sp->role);
}
where role_to_root_level() is extracted from kvm_calc_cpu_role() is Paolo's series.
Or, if we wanted to optimize "is passthrough", then cr4_la57 could be left in
the extended role, because passthrough is guaranteed to be '0' if CR4.LA57=1.
That would prevent reusing shadow pages between 64-bit paging and PAE paging, but
in practice no sane guest is going to reuse page tables between those mode, so who
cares.
