On Thu, Apr 14, 2022 at 11:51 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> 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).
glevel in the patchset is the page level of the corresponding page
table in the guest, not the level of the guest *root* page table.
role.glevel is initialized as the guest root level, and changed to the
level of the guest page in kvm_mmu_get_page().
role.glevel is a bad name and is not sufficient to handle other
purposes like role.pae_root, role.guest_pae_root.
role.root_level is much better.
role.root_level is a combination of CR0.PG, CR4.PAE, EFER.LMA, and
CR4.LA57 as you said.
>
> 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.
>
I'm going to add
static inline bool sp_has_gpte(struct kvm_mmu_page *sp)
{
return !sp->role.direct && (
/* passthrough */
sp->role.level > role_to_root_level(sp->role) ||
/* guest pae root */
(sp->role.level == 3 && role_to_root_level(sp->role) == 3)
);
}
And rename for_each_gfn_indirect_valid_sp() to
for_each_gfn_valid_sp_has_gpte() which use sp_has_gpte() instead.
I'm not objecting using efer_lma and cr4_la57. But I think role.root_level
is more convenient than role_to_root_level(sp->role).
cr4_pae, efer_lma and cr4_la57 are more natrual than has_4_byte_gpte
and root_level.
