On Mon, Jun 02, 2025 at 06:05:32PM -0700, Vishal Annapurve wrote:

On Tue, May 20, 2025 at 11:49 PM Yan Zhao [off-list ref] wrote:

quoted

On Mon, May 19, 2025 at 10:04:45AM -0700, Ackerley Tng wrote:

quoted

Ackerley Tng [off-list ref] writes:

quoted

Yan Zhao [off-list ref] writes:

quoted

On Fri, Mar 14, 2025 at 05:20:21PM +0800, Yan Zhao wrote:

quoted

This patch would cause host deadlock when booting up a TDX VM even if huge page
is turned off. I currently reverted this patch. No further debug yet.

This is because kvm_gmem_populate() takes filemap invalidation lock, and for
TDX, kvm_gmem_populate() further invokes kvm_gmem_get_pfn(), causing deadlock.

kvm_gmem_populate
  filemap_invalidate_lock
  post_populate
    tdx_gmem_post_populate
      kvm_tdp_map_page
       kvm_mmu_do_page_fault
         kvm_tdp_page_fault
      kvm_tdp_mmu_page_fault
        kvm_mmu_faultin_pfn
          __kvm_mmu_faultin_pfn
            kvm_mmu_faultin_pfn_private
              kvm_gmem_get_pfn
                filemap_invalidate_lock_shared

Though, kvm_gmem_populate() is able to take shared filemap invalidation lock,
(then no deadlock), lockdep would still warn "Possible unsafe locking scenario:
...DEADLOCK" due to the recursive shared lock, since commit e918188611f0
("locking: More accurate annotations for read_lock()").

Thank you for investigating. This should be fixed in the next revision.

This was not fixed in v2 [1], I misunderstood this locking issue.

IIUC kvm_gmem_populate() gets a pfn via __kvm_gmem_get_pfn(), then calls
part of the KVM fault handler to map the pfn into secure EPTs, then
calls the TDX module for the copy+encrypt.

Regarding this lock, seems like KVM'S MMU lock is already held while TDX
does the copy+encrypt. Why must the filemap_invalidate_lock() also be
held throughout the process?

If kvm_gmem_populate() does not hold filemap invalidate lock around all
requested pages, what value should it return after kvm_gmem_punch_hole() zaps a
mapping it just successfully installed?

TDX currently only holds the read kvm->mmu_lock in tdx_gmem_post_populate() when
CONFIG_KVM_PROVE_MMU is enabled, due to both slots_lock and the filemap
invalidate lock being taken in kvm_gmem_populate().

Does TDX need kvm_gmem_populate path just to ensure SEPT ranges are
not zapped during tdh_mem_page_add and tdh_mr_extend operations? Would
holding KVM MMU read lock during these operations sufficient to avoid
having to do this back and forth between TDX and gmem layers?

I think the problem here is because in kvm_gmem_populate(),
"__kvm_gmem_get_pfn(), post_populate(), and kvm_gmem_mark_prepared()"
must be wrapped in filemap invalidate lock (shared or exclusive), right?

Then, in TDX's post_populate() callback, the filemap invalidate lock is held
again by kvm_tdp_map_page() --> ... ->kvm_gmem_get_pfn().


As in kvm_gmem_get_pfn(), the filemap invalidate lock also wraps both
__kvm_gmem_get_pfn() and kvm_gmem_prepare_folio():

filemap_invalidate_lock_shared();
__kvm_gmem_get_pfn();
kvm_gmem_prepare_folio();
filemap_invalidate_unlock_shared(),

I don't find a good reason for kvm_gmem_populate() to release filemap lock
before invoking post_populate().

Could we change the lock to filemap_invalidate_lock_shared() in
kvm_gmem_populate() and relax the warning in commit e918188611f0 ("locking: More
accurate annotations for read_lock()") ?

quoted

Looks sev_gmem_post_populate() does not take kvm->mmu_lock either.

I think kvm_gmem_populate() needs to hold the filemap invalidate lock at least
around each __kvm_gmem_get_pfn(), post_populate() and kvm_gmem_mark_prepared().

quoted

If we don't have to hold the filemap_invalidate_lock() throughout,

1. Would it be possible to call kvm_gmem_get_pfn() to get the pfn
   instead of calling __kvm_gmem_get_pfn() and managing the lock in a
   loop?

2. Would it be possible to trigger the kvm fault path from
   kvm_gmem_populate() so that we don't rebuild the get_pfn+mapping
   logic and reuse the entire faulting code? That way the
   filemap_invalidate_lock() will only be held while getting a pfn.

The kvm fault path is invoked in TDX's post_populate() callback.
I don't find a good way to move it to kvm_gmem_populate().

quoted

[1] https://lore.kernel.org/all/cover.1747264138.git.ackerleytng@google.com/T/ (local)

quoted

quoted

quoted

quoted

@@ -819,12 +827,16 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
        pgoff_t index = kvm_gmem_get_index(slot, gfn);
        struct file *file = kvm_gmem_get_file(slot);
        int max_order_local;
+       struct address_space *mapping;
        struct folio *folio;
        int r = 0;

        if (!file)
                return -EFAULT;

+       mapping = file->f_inode->i_mapping;
+       filemap_invalidate_lock_shared(mapping);
+
        /*
         * The caller might pass a NULL 'max_order', but internally this
         * function needs to be aware of any order limitations set by

@@ -838,6 +850,7 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
        folio = __kvm_gmem_get_pfn(file, slot, index, pfn, &max_order_local);
        if (IS_ERR(folio)) {
                r = PTR_ERR(folio);
+               filemap_invalidate_unlock_shared(mapping);
                goto out;
        }

@@ -845,6 +858,7 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
                r = kvm_gmem_prepare_folio(kvm, file, slot, gfn, folio, max_order_local);

        folio_unlock(folio);
+       filemap_invalidate_unlock_shared(mapping);

        if (!r)
                *page = folio_file_page(folio, index);
--

2.25.1