Re: [PATCH 19/21] KVM: TDX: Add an ioctl to create initial guest memory

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Wed, 2024-09-04 at 07:01 -0700, Rick Edgecombe wrote:
> On Wed, 2024-09-04 at 12:53 +0800, Yan Zhao wrote:
> > > +       if (!kvm_mem_is_private(kvm, gfn)) {
> > > +               ret = -EFAULT;
> > > +               goto out_put_page;
> > > +       }
> > > +
> > > +       ret = kvm_tdp_map_page(vcpu, gpa, error_code, &level);
> > > +       if (ret < 0)
> > > +               goto out_put_page;
> > > +
> > > +       read_lock(&kvm->mmu_lock);
> > Although mirrored root can't be zapped with shared lock currently, is it
> > better to hold write_lock() here?
> > 
> > It should bring no extra overhead in a normal condition when the
> > tdx_gmem_post_populate() is called.
> 
> I think we should hold the weakest lock we can. Otherwise someday someone
> could
> run into it and think the write_lock() is required. It will add confusion.
> 
> What was the benefit of a write lock? Just in case we got it wrong?

I just tried to draft a comment to make it look less weird, but I think actually
even the mmu_read lock is technically unnecessary because we hold both
filemap_invalidate_lock() and slots_lock. The cases we care about:
 memslot deletion - slots_lock protects
 gmem hole punch - filemap_invalidate_lock() protects
 set attributes - slots_lock protects
 others?

So I guess all the mirror zapping cases that could execute concurrently are
already covered by other locks. If we skipped grabbing the mmu lock completely
it would trigger the assertion in kvm_tdp_mmu_gpa_is_mapped(). Removing the
assert would probably make kvm_tdp_mmu_gpa_is_mapped() a bit dangerous. Hmm. 

Maybe a comment like this:
/*
 * The case to care about here is a PTE getting zapped concurrently and 
 * this function erroneously thinking a page is mapped in the mirror EPT.
 * The private mem zapping paths are already covered by other locks held
 * here, but grab an mmu read_lock to not trigger the assert in
 * kvm_tdp_mmu_gpa_is_mapped().
 */

Yan, do you think it is sufficient?