| Message ID | 20220516232138.1783324-5-dmatlack@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Extend Eager Page Splitting to the shadow MMU | expand |
On Mon, May 16, 2022, David Matlack wrote: > Instead of computing the shadow page role from scratch for every new > page, derive most of the information from the parent shadow page. This > eliminates the dependency on the vCPU root role to allocate shadow page > tables, and reduces the number of parameters to kvm_mmu_get_page(). > > Preemptively split out the role calculation to a separate function for > use in a following commit. > > Note that when calculating the MMU root role, we can take > @role.passthrough, @role.direct, and @role.access directly from > @vcpu->arch.mmu->root_role. Only @role.level and @role.quadrant still > must be overridden for PAE page directories. Nit, instead of "for PAE page directories", something like "when shadowing 32-bit guest page tables with PAE page tables". Not all PAE PDEs need to be overridden. > No functional change intended. > > Reviewed-by: Peter Xu <peterx@redhat.com> > Signed-off-by: David Matlack <dmatlack@google.com> > --- > arch/x86/kvm/mmu/mmu.c | 98 +++++++++++++++++++++++----------- > arch/x86/kvm/mmu/paging_tmpl.h | 9 ++-- > 2 files changed, 71 insertions(+), 36 deletions(-) > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index a9d28bcabcbb..515e0b33144a 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c ... > - if (level <= vcpu->arch.mmu->cpu_role.base.level) > - role.passthrough = 0; > - > sp_list = &vcpu->kvm->arch.mmu_page_hash[kvm_page_table_hashfn(gfn)]; > for_each_valid_sp(vcpu->kvm, sp, sp_list) { > if (sp->gfn != gfn) { ... > +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; > + role.passthrough = 0; I don't love that this subtly relies on passthrough being limited to 5-level nNPT with 4-level L1 NPT. That's really just an implementation oddity, e.g. KVM can and (hopefully) will eventually use passthrough pages for at least level=4 when shadowing 3-level or 2-level NPT. The easiest thing would be to add a WARN so that we don't forget to handle this when this collides with Lai's series, and to document why KVM never sets "passthrough" for child shadow pages. The latter is especially confusing because it does have other passthrough pages, they just don't happen to have an associated "struct kvm_mmu_page". /* * KVM currently doesn't use "struct kvm_mmu_page" to track passthrough * pages when the guest is using 3-level or 2-level NPT, and instead * uses bare page allocations (see pml4/5_root and pae_root). The only * scenario where KVM uses a passthrough "struct kvm_mmu_page" is when * shadowing 4-level NPT with 5-level nNPT. So even though passthrough * child pages do exist, such pages aren't tracked in the list of shadow * pages and so don't need to compute a role. */ WARN_ON_ONCE(role.passthrough && role.level != PT64_ROOT_4LEVEL); role.passthrough = 0; > + > + /* > + * If the guest has 4-byte PTEs then that means it's using 32-bit, > + * 2-level, non-PAE paging. KVM shadows such guests with PAE paging > + * (i.e. 8-byte PTEs). The difference in PTE size means that KVM must > + * shadow each guest page table with multiple shadow page tables, which > + * requires extra bookkeeping in the role. > + * > + * Specifically, to shadow the guest's page directory (which covers a > + * 4GiB address space), KVM uses 4 PAE page directories, each mapping Nit, it's worth explicitly saying "virtual address space" at least once. > + * 1GiB of the address space. @role.quadrant encodes which quarter of > + * the address space each maps. > + * > + * To shadow the guest's page tables (which each map a 4MiB region), KVM > + * uses 2 PAE page tables, each mapping a 2MiB region. For these, > + * @role.quadrant encodes which half of the region they map. Oof, so I really like this comment because it simplifies the concept, but it glosses over one very crucial detail. The 32-bit GPTE consumes bits 21:12, and the 64-bit PTE consumes bits 20:12. So while it's absolutely correct to state the the quadrant encodes which half, bit 21 is consumed when doing a lookup in the _parent_, which is the _least_ significant bit in when indexing PDEs, hence the quadrant essentially becomes evens and odds. Specifically, it does NOT split the parent PD down the middle. Paolo's more concrete comment about bits helps a map things out explicit. Paolo is going to snag the above, so for your looming rebase, how about replacing the paragraph below with a version of Paolo's concrete example to pair with your abstract definition? * * Concretely, a 4-byte PDE consumes bits 31:22, while an 8-byte PDE * consumes bits 29:21. To consume bits 31:30, KVM's uses 4 shadow * PDPTEs; those 4 PAE page directories are pre-allocated and their * quadrant is assigned in mmu_alloc_root(). To consume bit 21, KVM * uses an additional PDE in every PD; the page table being configured * here is what's pointed at by the PDE. Thus, bit 21 is the _least_ * significant bit of the PDE index pointing at the shadow PT. */ [*] https://lore.kernel.org/all/090e701d-6893-ea25-1237-233ff3dd01ee@redhat.com > + * > + * Note, the 4 PAE page directories are pre-allocated and the quadrant > + * assigned in mmu_alloc_root(). So only page tables need to be handled > + * here. > + */ > + if (role.has_4_byte_gpte) { > + WARN_ON_ONCE(role.level != PG_LEVEL_4K); > + role.quadrant = (sptep - parent_sp->spt) % 2; Oh hell no. LOL. It took me a _long_ time to realize you're doing pointer arithmetic on "u64 *". I actually booted a 32-bit VM with printks and even then it still took me a good 20 seconds wondering if I was having a brain fart and simply forgot how mod works. The calculation is also unnecessarily costly; not that anyone is likely to notice, but still. The compiler doesn't know that sptep and parent_sp->spt are intertwined and so can't optimize, i.e. is forced to do the subtraction. A more efficient equivalent that doesn't require pointer arithmetic: role.quadrant = ((unsigned long)sptep / sizeof(*sptep)) & 1;
On Mon, May 16, 2022, David Matlack wrote: > +static struct kvm_mmu_page *kvm_mmu_get_child_sp(struct kvm_vcpu *vcpu, > + u64 *sptep, gfn_t gfn, > + bool direct, u32 access) Please use "unsigned int" for @access, here and everywhere else, so that KVM is consistent in how it refers to access. @access can actually squeeze into a u8, but it's referenced as a "unsigned int" because sp->role.access is an unsigned int. For me at least, when I see "u<size" I always assume there is a specific reason for using an exact size, e.g. variables/fields that track hardware state. Whereas "int" and "unsigned int" give the hint that the variable is KVM metadata. @@ -2201,7 +2201,8 @@ static struct kvm_mmu_page *kvm_mmu_get_shadow_page(struct kvm_vcpu *vcpu, return __kvm_mmu_get_shadow_page(vcpu->kvm, vcpu, &caches, gfn, role); } -static union kvm_mmu_page_role kvm_mmu_child_role(u64 *sptep, bool direct, u32 access) +static union kvm_mmu_page_role kvm_mmu_child_role(u64 *sptep, bool direct, + unsigned int access) { struct kvm_mmu_page *parent_sp = sptep_to_sp(sptep); union kvm_mmu_page_role role; @@ -2242,7 +2243,7 @@ static union kvm_mmu_page_role kvm_mmu_child_role(u64 *sptep, bool direct, u32 a static struct kvm_mmu_page *kvm_mmu_get_child_sp(struct kvm_vcpu *vcpu, u64 *sptep, gfn_t gfn, - bool direct, u32 access) + bool direct, unsigned int access) { union kvm_mmu_page_role role; > +{ > + union kvm_mmu_page_role role; > + > + role = kvm_mmu_child_role(sptep, direct, access); > + return kvm_mmu_get_page(vcpu, gfn, role); > +} > +
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index a9d28bcabcbb..515e0b33144a 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -2019,33 +2019,15 @@ static void clear_sp_write_flooding_count(u64 *spte) __clear_sp_write_flooding_count(sptep_to_sp(spte)); } -static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, - gfn_t gfn, - gva_t gaddr, - unsigned level, - bool direct, - unsigned int access) +static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, gfn_t gfn, + union kvm_mmu_page_role role) { - union kvm_mmu_page_role role; struct hlist_head *sp_list; - unsigned quadrant; struct kvm_mmu_page *sp; int ret; int collisions = 0; LIST_HEAD(invalid_list); - role = vcpu->arch.mmu->root_role; - role.level = level; - role.direct = direct; - role.access = access; - if (role.has_4_byte_gpte) { - quadrant = gaddr >> (PAGE_SHIFT + (PT64_PT_BITS * level)); - quadrant &= (1 << ((PT32_PT_BITS - PT64_PT_BITS) * level)) - 1; - role.quadrant = quadrant; - } - if (level <= vcpu->arch.mmu->cpu_role.base.level) - role.passthrough = 0; - sp_list = &vcpu->kvm->arch.mmu_page_hash[kvm_page_table_hashfn(gfn)]; for_each_valid_sp(vcpu->kvm, sp, sp_list) { if (sp->gfn != gfn) { @@ -2063,7 +2045,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, * Unsync pages must not be left as is, because the new * upper-level page will be write-protected. */ - if (level > PG_LEVEL_4K && sp->unsync) + if (role.level > PG_LEVEL_4K && sp->unsync) kvm_mmu_prepare_zap_page(vcpu->kvm, sp, &invalid_list); continue; @@ -2104,14 +2086,14 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, ++vcpu->kvm->stat.mmu_cache_miss; - sp = kvm_mmu_alloc_page(vcpu, direct); + sp = kvm_mmu_alloc_page(vcpu, role.direct); sp->gfn = gfn; sp->role = role; hlist_add_head(&sp->hash_link, sp_list); if (sp_has_gptes(sp)) { account_shadowed(vcpu->kvm, sp); - if (level == PG_LEVEL_4K && kvm_vcpu_write_protect_gfn(vcpu, gfn)) + if (role.level == PG_LEVEL_4K && kvm_vcpu_write_protect_gfn(vcpu, gfn)) kvm_flush_remote_tlbs_with_address(vcpu->kvm, gfn, 1); } trace_kvm_mmu_get_page(sp, true); @@ -2123,6 +2105,55 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, return sp; } +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; + role.passthrough = 0; + + /* + * If the guest has 4-byte PTEs then that means it's using 32-bit, + * 2-level, non-PAE paging. KVM shadows such guests with PAE paging + * (i.e. 8-byte PTEs). The difference in PTE size means that KVM must + * shadow each guest page table with multiple shadow page tables, which + * requires extra bookkeeping in the role. + * + * Specifically, to shadow the guest's page directory (which covers a + * 4GiB address space), KVM uses 4 PAE page directories, each mapping + * 1GiB of the address space. @role.quadrant encodes which quarter of + * the address space each maps. + * + * To shadow the guest's page tables (which each map a 4MiB region), KVM + * uses 2 PAE page tables, each mapping a 2MiB region. For these, + * @role.quadrant encodes which half of the region they map. + * + * Note, the 4 PAE page directories are pre-allocated and the quadrant + * assigned in mmu_alloc_root(). So only page tables need to be handled + * here. + */ + if (role.has_4_byte_gpte) { + WARN_ON_ONCE(role.level != PG_LEVEL_4K); + role.quadrant = (sptep - parent_sp->spt) % 2; + } + + return role; +} + +static struct kvm_mmu_page *kvm_mmu_get_child_sp(struct kvm_vcpu *vcpu, + u64 *sptep, gfn_t gfn, + bool direct, u32 access) +{ + union kvm_mmu_page_role role; + + role = kvm_mmu_child_role(sptep, direct, access); + return kvm_mmu_get_page(vcpu, gfn, role); +} + static void shadow_walk_init_using_root(struct kvm_shadow_walk_iterator *iterator, struct kvm_vcpu *vcpu, hpa_t root, u64 addr) @@ -2965,8 +2996,7 @@ static int __direct_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) if (is_shadow_present_pte(*it.sptep)) continue; - sp = kvm_mmu_get_page(vcpu, base_gfn, it.addr, - it.level - 1, true, ACC_ALL); + sp = kvm_mmu_get_child_sp(vcpu, it.sptep, base_gfn, true, ACC_ALL); link_shadow_page(vcpu, it.sptep, sp); if (fault->is_tdp && fault->huge_page_disallowed && @@ -3369,13 +3399,18 @@ static int mmu_check_root(struct kvm_vcpu *vcpu, gfn_t root_gfn) return ret; } -static hpa_t mmu_alloc_root(struct kvm_vcpu *vcpu, gfn_t gfn, gva_t gva, +static hpa_t mmu_alloc_root(struct kvm_vcpu *vcpu, gfn_t gfn, int quadrant, u8 level) { - bool direct = vcpu->arch.mmu->root_role.direct; + union kvm_mmu_page_role role = vcpu->arch.mmu->root_role; struct kvm_mmu_page *sp; - sp = kvm_mmu_get_page(vcpu, gfn, gva, level, direct, ACC_ALL); + role.level = level; + + if (role.has_4_byte_gpte) + role.quadrant = quadrant; + + sp = kvm_mmu_get_page(vcpu, gfn, role); ++sp->root_count; return __pa(sp->spt); @@ -3409,8 +3444,8 @@ static int mmu_alloc_direct_roots(struct kvm_vcpu *vcpu) for (i = 0; i < 4; ++i) { WARN_ON_ONCE(IS_VALID_PAE_ROOT(mmu->pae_root[i])); - root = mmu_alloc_root(vcpu, i << (30 - PAGE_SHIFT), - i << 30, PT32_ROOT_LEVEL); + root = mmu_alloc_root(vcpu, i << (30 - PAGE_SHIFT), i, + PT32_ROOT_LEVEL); mmu->pae_root[i] = root | PT_PRESENT_MASK | shadow_me_mask; } @@ -3579,8 +3614,7 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu) root_gfn = pdptrs[i] >> PAGE_SHIFT; } - root = mmu_alloc_root(vcpu, root_gfn, i << 30, - PT32_ROOT_LEVEL); + root = mmu_alloc_root(vcpu, root_gfn, i, PT32_ROOT_LEVEL); mmu->pae_root[i] = root | pm_mask; } diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h index db80f7ccaa4e..fd73c857af90 100644 --- a/arch/x86/kvm/mmu/paging_tmpl.h +++ b/arch/x86/kvm/mmu/paging_tmpl.h @@ -648,8 +648,9 @@ static int FNAME(fetch)(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault, if (!is_shadow_present_pte(*it.sptep)) { table_gfn = gw->table_gfn[it.level - 2]; access = gw->pt_access[it.level - 2]; - sp = kvm_mmu_get_page(vcpu, table_gfn, fault->addr, - it.level-1, false, access); + sp = kvm_mmu_get_child_sp(vcpu, it.sptep, table_gfn, + false, access); + /* * We must synchronize the pagetable before linking it * because the guest doesn't need to flush tlb when @@ -705,8 +706,8 @@ static int FNAME(fetch)(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault, drop_large_spte(vcpu, it.sptep); if (!is_shadow_present_pte(*it.sptep)) { - sp = kvm_mmu_get_page(vcpu, base_gfn, fault->addr, - it.level - 1, true, direct_access); + sp = kvm_mmu_get_child_sp(vcpu, it.sptep, base_gfn, + true, direct_access); link_shadow_page(vcpu, it.sptep, sp); if (fault->huge_page_disallowed && fault->req_level >= it.level)