lkml.org 
[lkml]   [2022]   [Jul]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V3 02/12] KVM: X86/MMU: Add using_local_root_page()
    On Sat, May 21, 2022, Lai Jiangshan wrote:
    > +static bool using_local_root_page(struct kvm_mmu *mmu)

    Hmm, I agree with David that "local" isn't the most intuitive terminology. But
    I also do want to avoid private vs. shared to avoid confusion with confidential VMs.

    Luckily, I don't think we need to come up with new terminology, just be literal
    and call 'em "per-vCPU root pages". E.g.

    static bool kvm_mmu_has_per_vcpu_root_page()

    That way readers don't have to understand what "local" means, and that also captures
    per-vCPU roots are an exception, i.e. that most roots are NOT per-vCPU.

    > +{
    > + return mmu->root_role.level == PT32E_ROOT_LEVEL ||
    > + (!mmu->root_role.direct && mmu->cpu_role.base.level <= PT32E_ROOT_LEVEL);
    > +}
    > +
    > static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu, int direct)
    > {
    > struct kvm_mmu_page *sp;
    > @@ -4252,10 +4285,11 @@ static bool fast_pgd_switch(struct kvm *kvm, struct kvm_mmu *mmu,
    > {
    > /*
    > * For now, limit the caching to 64-bit hosts+VMs in order to avoid
    > - * having to deal with PDPTEs. We may add support for 32-bit hosts/VMs
    > - * later if necessary.
    > + * having to deal with PDPTEs. Local roots can not be put into
    > + * mmu->prev_roots[] because mmu->pae_root can not be shared for
    > + * different roots at the same time.
    > */
    > - if (VALID_PAGE(mmu->root.hpa) && !to_shadow_page(mmu->root.hpa))
    > + if (unlikely(using_local_root_page(mmu)))

    I don't know that I like using the local/per-vCPU helper. The problem isn't _just_
    that KVM is using a per-vCPU root, KVM is also deliberately punting on dealing with
    PDTPRs. E.g. the per-vCPU aspect doesn't explain why KVM doesn't allow reusing the
    current root. I don't like that the using_local_root_page() obfuscates that check.

    My preference for this would be to revert back to a streamlined variation of the
    code prior to commit 5499ea73e7db ("KVM: x86/mmu: look for a cached PGD when going
    from 32-bit to 64-bit").

    KVM switched to the !to_shadow_page() check to _avoid_ consuming (what is now)
    mmu->root_role because, at the time of the patch, mmu held the _old_ data, which
    was wrong/stale for nested virtualization transitions.

    In other words, I would prefer that explicitly do (in a separate patch):

    /*
    * For now, limit the fast switch to 64-bit VMs in order to avoid having
    * to deal with PDPTEs. 32-bit VMs can be supported later if necessary.
    */
    if (new_role.level < PT64_ROOT_LEVEL4)
    kvm_mmu_free_roots(kvm, mmu, KVM_MMU_ROOT_CURRENT);

    The "hosts+VMs" can be shortened to just "VMs", because running a 64-bit VM with
    a 32-bit host just doesn't work for a variety of reasons, i.e. doesn't need to be
    called out here.

    \
     
     \ /
      Last update: 2022-07-20 00:05    [W:4.431 / U:0.056 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site