lkml.org 
[lkml]   [2022]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v10 045/108] KVM: x86/mmu: Add a private pointer to struct kvm_mmu_page
    Date
    On Wed, 2022-11-16 at 10:32 +0000, Huang, Kai wrote:
    > > +
    > > +static inline void kvm_mmu_alloc_private_spt(struct kvm_vcpu *vcpu,
    > > +      struct kvm_mmu_memory_cache
    > > *private_spt_cache,
    > > +      struct kvm_mmu_page *sp)
    >
    > This function is very weird in the context of this patch.  _Only_ a new vcpu-
    > scope 'mmu_private_spte_cache' is added in this patch, but here you allow
    > caller
    > to pass an additional argument of private_spt_cache.  So there must be another
    > cache which is not introduced in this patch?
    >
    > > +{
    > > + /*
    > > + * vcpu == NULL means non-root SPT:
    > > + * vcpu == NULL is used to split a large SPT into smaller SPT. 
    > > Root SPT
    > > + * is not a large SPT.
    >
    > I am guessing this "vcpu == NULL" case is for "Eager Splitting"?
    >
    > If so, why not adding a global MMU cache for private_spt allocation, and make
    > vcpu->arch.mmu_private_spt_cache point to the global cache?  In this case, in
    > the context where you only have 'kvm', you can use the global cache directly.
    > And in the context where you have a 'vcpu', you just use vcpu's cache.

    So I went through all MMU related patches in this series, but I cannot find a
    place where this function is called with 'vcpu == NULL' and a valid cache is
    passed in, if I am reading correctly.

    Also checked that "Eager Splitting" uses a kvm-scope cache for legacy MMU, but
    just uses __get_free_page() for TDP MMU. And in later patch "KVM: x86/tdp_mmu:
    Support TDX private mapping for TDP MMU", __get_free_page() is also used to
    allocate the private_spt (which is consistent with existing eager splitting
    code).

    So IIUC only legacy MMU code will call this function with 'vcpu == NULL' and a
    valid cache. In this case, please remove the 'private_spt_cache' argument for
    now, and make the function always allocate from the vcpu-
    >arch.mmu_private_spt_cache.  

    You can add the additional argument when TDX gets legacy MMU support.

    Also, I think you need to move eager splitting support part (whether that
    handling is correct is another story) from the later patch to this patch.
    Otherwise this patch is not complete.


    >  
    > > + */
    > > + bool is_root = vcpu &&
    > > + vcpu->arch.root_mmu.root_role.level == sp->role.level;
    > > +
    > > + if (vcpu)
    > > + private_spt_cache = &vcpu->arch.mmu_private_spt_cache;
    > > + KVM_BUG_ON(!kvm_mmu_page_role_is_private(sp->role), vcpu->kvm);
    > > + if (is_root)
    > > + /*
    > > + * Because TDX module assigns root Secure-EPT page and set
    > > it to
    > > + * Secure-EPTP when TD vcpu is created, secure page table
    > > for
    > > + * root isn't needed.
    > > + */
    > > + sp->private_spt = NULL;
    > > + else {
    > > + sp->private_spt =
    > > kvm_mmu_memory_cache_alloc(private_spt_cache);
    > > + /*
    > > + * Because mmu_private_spt_cache is topped up before
    > > staring kvm
    > > + * page fault resolving, the allocation above shouldn't
    > > fail.
    > > + */
    > > + WARN_ON_ONCE(!sp->private_spt);
    > > + }
    > > +}
    > > +

    \
     
     \ /
      Last update: 2022-11-16 13:02    [W:4.226 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site