Re: [PATCH v5 04/12] KVM: guest_memfd: Add flag to remove from direct map

["Roy, Patrick" <roypat@amazon.co.uk>]

On Thu, 2025-08-28 at 15:54 +0100, Mike Rapoport wrote:
> On Thu, Aug 28, 2025 at 09:39:21AM +0000, Roy, Patrick wrote:
>> Add GUEST_MEMFD_FLAG_NO_DIRECT_MAP flag for KVM_CREATE_GUEST_MEMFD()
>> ioctl. When set, guest_memfd folios will be removed from the direct map
>> after preparation, with direct map entries only restored when the folios
>> are freed.
>>
>> To ensure these folios do not end up in places where the kernel cannot
>> deal with them, set AS_NO_DIRECT_MAP on the guest_memfd's struct
>> address_space if GUEST_MEMFD_FLAG_NO_DIRECT_MAP is requested.
>>
>> Add KVM_CAP_GUEST_MEMFD_NO_DIRECT_MAP to let userspace discover whether
>> guest_memfd supports GUEST_MEMFD_FLAG_NO_DIRECT_MAP. Support depends on
>> guest_memfd itself being supported, but also on whether KVM can
>> manipulate the direct map at page granularity at all (possible most of
>> the time, just arm64 is a notable outlier where its impossible if the
>> direct map has been setup using hugepages, as arm64 cannot break these
>> apart due to break-before-make semantics).
> 
> There's also powerpc that does not select ARCH_HAS_SET_DIRECT_MAP

Ah, thanks! Although powerpc also doesnt support guest_memfd in the first
place, but will mention.

>> Note that this flag causes removal of direct map entries for all
>> guest_memfd folios independent of whether they are "shared" or "private"
>> (although current guest_memfd only supports either all folios in the
>> "shared" state, or all folios in the "private" state if
>> GUEST_MEMFD_FLAG_MMAP is not set). The usecase for removing direct map
>> entries of also the shared parts of guest_memfd are a special type of
>> non-CoCo VM where, host userspace is trusted to have access to all of
>> guest memory, but where Spectre-style transient execution attacks
>> through the host kernel's direct map should still be mitigated.  In this
>> setup, KVM retains access to guest memory via userspace mappings of
>> guest_memfd, which are reflected back into KVM's memslots via
>> userspace_addr. This is needed for things like MMIO emulation on x86_64
>> to work.
>>
>> Do not perform TLB flushes after direct map manipulations. This is
>> because TLB flushes resulted in a up to 40x elongation of page faults in
>> guest_memfd (scaling with the number of CPU cores), or a 5x elongation
>> of memory population. TLB flushes are not needed for functional
>> correctness (the virt->phys mapping technically stays "correct",  the
>> kernel should simply to not it for a while). On the other hand, it means
> 
>                           ^ not use it?

Yup, thanks!

>> that the desired protection from Spectre-style attacks is not perfect,
>> as an attacker could try to prevent a stale TLB entry from getting
>> evicted, keeping it alive until the page it refers to is used by the
>> guest for some sensitive data, and then targeting it using a
>> spectre-gadget.
>>
>> Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
>> ---
>>  arch/arm64/include/asm/kvm_host.h | 12 ++++++++++++
>>  include/linux/kvm_host.h          |  7 +++++++
>>  include/uapi/linux/kvm.h          |  2 ++
>>  virt/kvm/guest_memfd.c            | 29 +++++++++++++++++++++++++----
>>  virt/kvm/kvm_main.c               |  5 +++++
>>  5 files changed, 51 insertions(+), 4 deletions(-)
> 
> ...
> 
>> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
>> index 9ec4c45e3cf2..e3696880405c 100644
>> --- a/virt/kvm/guest_memfd.c
>> +++ b/virt/kvm/guest_memfd.c
>> @@ -4,6 +4,7 @@
>>  #include <linux/kvm_host.h>
>>  #include <linux/pagemap.h>
>>  #include <linux/anon_inodes.h>
>> +#include <linux/set_memory.h>
>>
>>  #include "kvm_mm.h"
>>
>> @@ -42,8 +43,18 @@ static int __kvm_gmem_prepare_folio(struct kvm *kvm, struct kvm_memory_slot *slo
>>       return 0;
>>  }
>>
>> +static bool kvm_gmem_test_no_direct_map(struct inode *inode)
>> +{
>> +     return ((unsigned long) inode->i_private) & GUEST_MEMFD_FLAG_NO_DIRECT_MAP;
>> +}
>> +
>>  static inline void kvm_gmem_mark_prepared(struct folio *folio)
>>  {
>> +     struct inode *inode = folio_inode(folio);
>> +
>> +     if (kvm_gmem_test_no_direct_map(inode))
>> +             set_direct_map_valid_noflush(folio_page(folio, 0), folio_nr_pages(folio), false);
> 
> This may fail to split large mapping in the direct map. Why not move this
> to kvm_gmem_prepare_folio() where you can handle returned error?

Argh, yeah, got that the wrong way around. Will update the error handling.

> I think that using set_direct_map_invalid_noflush() here and
> set_direct_map_default_noflush() in kvm_gmem_free_folio() better is
> clearer and makes it more obvious that here the folio is removed from the
> direct map and when freed it's direct mapping is restored.
> 
> This requires to export two symbols in patch 2, but I think it's worth it.

Mh, but set_direct_map_[default|invalid]_noflush() only take a single struct
page * argument, so they'd either need to gain a npages argument, or we add yet
more functions to set_memory.h.  Do you still think that's worth it? 

>>       folio_mark_uptodate(folio);
>>  }
>>
>> @@ -429,25 +440,29 @@ static int kvm_gmem_error_folio(struct address_space *mapping, struct folio *fol
>>       return MF_DELAYED;
>>  }
>>
>> -#ifdef CONFIG_HAVE_KVM_ARCH_GMEM_INVALIDATE
>>  static void kvm_gmem_free_folio(struct address_space *mapping,
>>                               struct folio *folio)
>>  {
>>       struct page *page = folio_page(folio, 0);
>> +
>> +#ifdef CONFIG_HAVE_KVM_ARCH_GMEM_INVALIDATE
>>       kvm_pfn_t pfn = page_to_pfn(page);
>>       int order = folio_order(folio);
>> +#endif
>>
>> +     if (kvm_gmem_test_no_direct_map(mapping->host))
>> +             WARN_ON_ONCE(set_direct_map_valid_noflush(page, folio_nr_pages(folio), true));
> 
> I don't think it can fail here. The direct map was split when you removed
> the folio so here it will merely update the prot bits.

Yup, will drop this WARN_ON_ONCE.

>> +#ifdef CONFIG_HAVE_KVM_ARCH_GMEM_INVALIDATE
>>       kvm_arch_gmem_invalidate(pfn, pfn + (1ul << order));
>> -}
>>  #endif
>> +}
> 
> Instead of moving #ifdefs into kvm_gmem_free_folio() it's better to add, say,
> kvm_gmem_invalidate() and move ifdefery there or even better have a static
> inline stub for !CONFIG_HAVE_KVM_ARCH_GMEM_INVALIDATE case.

Ack, will do the latter

>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 18f29ef93543..0dbfd17e1191 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -65,6 +65,7 @@
>>  #include <trace/events/kvm.h>
>>
>>  #include <linux/kvm_dirty_ring.h>
>> +#include <linux/set_memory.h>
>>
>>
>>  /* Worst case buffer size needed for holding an integer. */
>> @@ -4916,6 +4917,10 @@ static int kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
>>               return kvm_supported_mem_attributes(kvm);
>>  #endif
>>  #ifdef CONFIG_KVM_GUEST_MEMFD
>> +     case KVM_CAP_GUEST_MEMFD_NO_DIRECT_MAP:
>> +             if (!can_set_direct_map())
> 
> Shouldn't this check with kvm_arch_gmem_supports_no_direct_map()?

Absolutely, thanks for catching!

>> +                     return false;
>> +             fallthrough;
>>       case KVM_CAP_GUEST_MEMFD:
>>               return 1;
>>       case KVM_CAP_GUEST_MEMFD_MMAP:
>> --
>> 2.50.1
>>
> 
> --
> Sincerely yours,
> Mike.

Best,
Patrick