On Tue, May 10, 2022, Lai Jiangshan wrote:
> ()
>
> On Tue, May 10, 2022 at 5:04 AM David Matlack <dmatlack@xxxxxxxxxx> wrote:
> >
> > On Sat, May 7, 2022 at 1:28 AM Lai Jiangshan <jiangshanlai@xxxxxxxxx> wrote:
> > > > +static union kvm_mmu_page_role kvm_mmu_child_role(u64 *sptep, bool direct, u32 access)
> > > > +{
> > > > + struct kvm_mmu_page *parent_sp = sptep_to_sp(sptep);
> > > > + union kvm_mmu_page_role role;
> > > > +
> > > > + role = parent_sp->role;
> > > > + role.level--;
> > > > + role.access = access;
> > > > + role.direct = direct;
> > > > +
> > > > + /*
> > > > + * If the guest has 4-byte PTEs then that means it's using 32-bit,
> > > > + * 2-level, non-PAE paging. KVM shadows such guests using 4 PAE page
> > > > + * directories, each mapping 1/4 of the guest's linear address space
> > > > + * (1GiB). The shadow pages for those 4 page directories are
> > > > + * pre-allocated and assigned a separate quadrant in their role.
> > >
> > >
> > > It is not going to be true in patchset:
> > > [PATCH V2 0/7] KVM: X86/MMU: Use one-off special shadow page for special roots
> > >
> > > https://lore.kernel.org/lkml/20220503150735.32723-1-jiangshanlai@xxxxxxxxx/
> > >
> > > The shadow pages for those 4 page directories are also allocated on demand.
> >
> > Ack. I can even just drop this sentence in v5, it's just background information.
>
> No, if one-off special shadow pages are used.
>
> kvm_mmu_child_role() should be:
>
> + if (role.has_4_byte_gpte) {
> + if (role.level == PG_LEVEL_4K)
> + role.quadrant = (sptep - parent_sp->spt) % 2;
> + if (role.level == PG_LEVEL_2M)
If the code ends up looking anything like this, please use PT32_ROOT_LEVEL instead
of PG_LEVEL_2M. PSE paging has 4M huge pages, using PG_LEVEL_2M is confusing.
Or even better might be to do:
if (role.level == PG_LEVEL_4k)
...
else
...
Or arithmetic using role.level directly, a la the current code.
