Hi Ackerley,
On Mon, 5 Aug 2024 at 18:14, Ackerley Tng <ackerleytng@xxxxxxxxxx> wrote:
>
> Fuad Tabba <tabba@xxxxxxxxxx> writes:
>
> > Add support for mmap() and fault() for guest_memfd in the host.
> > The ability to fault in a guest page is contingent on that page
> > being shared with the host. To track this, this patch adds a new
> > xarray to each guest_memfd object, which tracks the mappability
> > of guest frames.
> >
> > The guest_memfd PRIVATE memory attribute is not used for two
> > reasons. First because it reflects the userspace expectation for
> > that memory location, and therefore can be toggled by userspace.
>
> Thank you for clarifying why the PRIVATE memory attribute cannot be
> used. I think that having the attributes with guest_memfd is a good
> idea.
>
> Since faultability is a property of the memory contents, shouldn't
> faultability be stored on the inode rather than the file?
I don't have a strong opinion about this, but by not mappable, this
means not having a valid mapping in the host, not that the "mmap"
call in and of itself is not allowed.
>
> > The second is, although each guest_memfd file has a 1:1 binding
> > with a KVM instance, the plan is to allow multiple files per
> > inode, e.g. to allow intra-host migration to a new KVM instance,
> > without destroying guest_memfd.
>
> I think you also alluded to the concept of inodes vs files above.
>
> To store the xarray on the inode, we would probably have to make
> guest_memfd use its own mount so that we can use .evict_inode() from
> struct address_space_operations to clean up the inode neatly, perhaps
> the way it was done in guest_memfd v12 [1].
>
> This RFC to enable intra-host migration [2] was built with the version
> of guest_memfd that used its own mount. IIUC, the gmem struct stored on
> the file is meant to be the binding between struct kvm and the memory
> contents [3], so the gmem struct won't be transferred and a new gmem
> struct will be created.
I thought that the gmem object itself is unique, and that even if
we're transferring it from one inode to another it would still be
there. If not, then you're right and we need to store this data in the
inode.
Thanks for pointing me to the RFC. I'll have a look at it and use it
as a reference when I respin this.
> >
> > This new feature is gated with a new configuration option,
> > CONFIG_KVM_PRIVATE_MEM_MAPPABLE.
> >
> > Signed-off-by: Fuad Tabba <tabba@xxxxxxxxxx>
> > ---
> > include/linux/kvm_host.h | 61 ++++++++++++++++++++
> > virt/kvm/Kconfig | 4 ++
> > virt/kvm/guest_memfd.c | 110 +++++++++++++++++++++++++++++++++++
> > virt/kvm/kvm_main.c | 122 +++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 297 insertions(+)
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 43a157f8171a..ab1344327e57 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -2452,4 +2452,65 @@ static inline int kvm_gmem_get_pfn_locked(struct kvm *kvm,
> > }
> > #endif /* CONFIG_KVM_PRIVATE_MEM */
> >
> > +#ifdef CONFIG_KVM_PRIVATE_MEM_MAPPABLE
> > +bool kvm_gmem_is_mappable(struct kvm *kvm, gfn_t gfn, gfn_t end);
> > +bool kvm_gmem_is_mapped(struct kvm *kvm, gfn_t start, gfn_t end);
> > +int kvm_gmem_set_mappable(struct kvm *kvm, gfn_t start, gfn_t end);
>
> How will kvm_gmem_is_mapped() and kvm_gmem_set_mappable() be used?
In this patch series I only use __ kvm_gmem_is_mapped(), you're right.
In later patches (not posted, pkvm specific), I use
kvm_gmem_is_mapped() in the processing of an unshare call from the
guest.
> > +int kvm_gmem_clear_mappable(struct kvm *kvm, gfn_t start, gfn_t end);
> > +int kvm_slot_gmem_toggle_mappable(struct kvm_memory_slot *slot, gfn_t start,
> > + gfn_t end, bool is_mappable);
> > +int kvm_slot_gmem_set_mappable(struct kvm_memory_slot *slot, gfn_t start,
> > + gfn_t end);
> > +int kvm_slot_gmem_clear_mappable(struct kvm_memory_slot *slot, gfn_t start,
> > + gfn_t end);
> > +bool kvm_slot_gmem_is_mappable(struct kvm_memory_slot *slot, gfn_t gfn);
> > +#else
> > +static inline bool kvm_gmem_is_mappable(struct kvm *kvm, gfn_t gfn, gfn_t end)
> > +{
> > + WARN_ON_ONCE(1);
> > + return false;
> > +}
> > +static inline bool kvm_gmem_is_mapped(struct kvm *kvm, gfn_t start, gfn_t end)
> > +{
> > + WARN_ON_ONCE(1);
> > + return false;
> > +}
> > +static inline int kvm_gmem_set_mappable(struct kvm *kvm, gfn_t start, gfn_t end)
> > +{
> > + WARN_ON_ONCE(1);
> > + return -EINVAL;
> > +}
> > +static inline int kvm_gmem_clear_mappable(struct kvm *kvm, gfn_t start,
> > + gfn_t end)
> > +{
> > + WARN_ON_ONCE(1);
> > + return -EINVAL;
> > +}
> > +static inline int kvm_slot_gmem_toggle_mappable(struct kvm_memory_slot *slot,
> > + gfn_t start, gfn_t end,
> > + bool is_mappable)
> > +{
> > + WARN_ON_ONCE(1);
> > + return -EINVAL;
> > +}
> > +static inline int kvm_slot_gmem_set_mappable(struct kvm_memory_slot *slot,
> > + gfn_t start, gfn_t end)
> > +{
> > + WARN_ON_ONCE(1);
> > + return -EINVAL;
> > +}
> > +static inline int kvm_slot_gmem_clear_mappable(struct kvm_memory_slot *slot,
> > + gfn_t start, gfn_t end)
> > +{
> > + WARN_ON_ONCE(1);
> > + return -EINVAL;
> > +}
> > +static inline bool kvm_slot_gmem_is_mappable(struct kvm_memory_slot *slot,
> > + gfn_t gfn)
> > +{
> > + WARN_ON_ONCE(1);
> > + return false;
> > +}
> > +#endif /* CONFIG_KVM_PRIVATE_MEM_MAPPABLE */
> > +
> > #endif
> > diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
> > index 29b73eedfe74..a3970c5eca7b 100644
> > --- a/virt/kvm/Kconfig
> > +++ b/virt/kvm/Kconfig
> > @@ -109,3 +109,7 @@ config KVM_GENERIC_PRIVATE_MEM
> > select KVM_GENERIC_MEMORY_ATTRIBUTES
> > select KVM_PRIVATE_MEM
> > bool
> > +
> > +config KVM_PRIVATE_MEM_MAPPABLE
> > + select KVM_PRIVATE_MEM
> > + bool
> > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > index f3f4334a9ccb..0a1f266a16f9 100644
> > --- a/virt/kvm/guest_memfd.c
> > +++ b/virt/kvm/guest_memfd.c
> > @@ -11,6 +11,9 @@ struct kvm_gmem {
> > struct kvm *kvm;
> > struct xarray bindings;
> > struct list_head entry;
> > +#ifdef CONFIG_KVM_PRIVATE_MEM_MAPPABLE
> > + struct xarray unmappable_gfns;
> > +#endif
> > };
> >
> > static struct folio *kvm_gmem_get_folio(struct inode *inode, pgoff_t index)
> > @@ -230,6 +233,11 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
> > mutex_unlock(&kvm->slots_lock);
> >
> > xa_destroy(&gmem->bindings);
> > +
> > +#ifdef CONFIG_KVM_PRIVATE_MEM_MAPPABLE
> > + xa_destroy(&gmem->unmappable_gfns);
> > +#endif
> > +
> > kfree(gmem);
> >
> > kvm_put_kvm(kvm);
> > @@ -248,7 +256,105 @@ static inline struct file *kvm_gmem_get_file(struct kvm_memory_slot *slot)
> > return get_file_active(&slot->gmem.file);
> > }
> >
> > +#ifdef CONFIG_KVM_PRIVATE_MEM_MAPPABLE
> > +int kvm_slot_gmem_toggle_mappable(struct kvm_memory_slot *slot, gfn_t start,
> > + gfn_t end, bool is_mappable)
> > +{
> > + struct kvm_gmem *gmem = slot->gmem.file->private_data;
> > + void *xval = is_mappable ? NULL : xa_mk_value(true);
>
> IIUC storing stuff in the xarray takes memory, so if we want to save
> memory, we should minimize entries in the xarray. For pKVM, do you
> expect more mappable, or more unmappable gfns?
>
> For TDX most of the memory will be private, so we expect fewer mappable
> gfns, and we'd prefer "entry in xarray == mappable".
This is similar to a discussion I've had earlier regarding the PRIVATE
attribute [*].
I don't have a strong opinion. It's trivial to reverse the polarity.
But to answer your question, most of the memory in pKVM would be
private for protected guests, but shared for non-protected guests. So,
it's not easy to know which would be the more common use case.
Cheers,
/fuad
[*] https://lore.kernel.org/lkml/20231027182217.3615211-1-seanjc@xxxxxxxxxx/T/#:~:text=consistent%20across%20implementations.-,Yeah%2C%20we%20discussed%20this%20in%20v12%5B*%5D.%20%20The%20default%20really%20doesn%27t%20matter%20for%20memory,-overheads%20or%20performances
> > + void *r;
> > +
> > + r = xa_store_range(&gmem->unmappable_gfns, start, end - 1, xval, GFP_KERNEL);
> > +
> > + return xa_err(r);
> > +}
> > +
> > +int kvm_slot_gmem_set_mappable(struct kvm_memory_slot *slot, gfn_t start, gfn_t end)
> > +{
> > + return kvm_slot_gmem_toggle_mappable(slot, start, end, true);
> > +}
> > +
> > +int kvm_slot_gmem_clear_mappable(struct kvm_memory_slot *slot, gfn_t start, gfn_t end)
> > +{
> > + return kvm_slot_gmem_toggle_mappable(slot, start, end, false);
> > +}
> > +
> > +bool kvm_slot_gmem_is_mappable(struct kvm_memory_slot *slot, gfn_t gfn)
> > +{
> > + struct kvm_gmem *gmem = slot->gmem.file->private_data;
> > + unsigned long _gfn = gfn;
> > +
> > + return !xa_find(&gmem->unmappable_gfns, &_gfn, ULONG_MAX, XA_PRESENT);
> > +}
> > +
> > +static bool kvm_gmem_isfaultable(struct vm_fault *vmf)
> > +{
> > + struct kvm_gmem *gmem = vmf->vma->vm_file->private_data;
> > + struct inode *inode = file_inode(vmf->vma->vm_file);
> > + pgoff_t pgoff = vmf->pgoff;
> > + struct kvm_memory_slot *slot;
> > + unsigned long index;
> > + bool r = true;
> > +
> > + filemap_invalidate_lock(inode->i_mapping);
> > +
> > + xa_for_each_range(&gmem->bindings, index, slot, pgoff, pgoff) {
> > + pgoff_t base_gfn = slot->base_gfn;
> > + pgoff_t gfn_pgoff = slot->gmem.pgoff;
> > + pgoff_t gfn = base_gfn + max(gfn_pgoff, pgoff) - gfn_pgoff;
> > +
> > + if (!kvm_slot_gmem_is_mappable(slot, gfn)) {
> > + r = false;
> > + break;
> > + }
> > + }
> > +
> > + filemap_invalidate_unlock(inode->i_mapping);
> > +
> > + return r;
> > +}
> > +
> > +static vm_fault_t kvm_gmem_fault(struct vm_fault *vmf)
> > +{
> > + struct folio *folio;
> > +
> > + folio = kvm_gmem_get_folio(file_inode(vmf->vma->vm_file), vmf->pgoff);
> > + if (!folio)
> > + return VM_FAULT_SIGBUS;
> > +
> > + if (!kvm_gmem_isfaultable(vmf)) {
> > + folio_unlock(folio);
> > + folio_put(folio);
> > + return VM_FAULT_SIGBUS;
> > + }
> > +
> > + vmf->page = folio_file_page(folio, vmf->pgoff);
> > + return VM_FAULT_LOCKED;
> > +}
> > +
> > +static const struct vm_operations_struct kvm_gmem_vm_ops = {
> > + .fault = kvm_gmem_fault,
> > +};
> > +
> > +static int kvm_gmem_mmap(struct file *file, struct vm_area_struct *vma)
> > +{
> > + if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) !=
> > + (VM_SHARED | VM_MAYSHARE)) {
> > + return -EINVAL;
> > + }
> > +
> > + file_accessed(file);
> > + vm_flags_set(vma, VM_DONTDUMP);
> > + vma->vm_ops = &kvm_gmem_vm_ops;
> > +
> > + return 0;
> > +}
> > +#else
> > +#define kvm_gmem_mmap NULL
> > +#endif /* CONFIG_KVM_PRIVATE_MEM_MAPPABLE */
> > +
> > static struct file_operations kvm_gmem_fops = {
> > + .mmap = kvm_gmem_mmap,
> > .open = generic_file_open,
> > .release = kvm_gmem_release,
> > .fallocate = kvm_gmem_fallocate,
> > @@ -369,6 +475,10 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
> > xa_init(&gmem->bindings);
> > list_add(&gmem->entry, &inode->i_mapping->i_private_list);
> >
> > +#ifdef CONFIG_KVM_PRIVATE_MEM_MAPPABLE
> > + xa_init(&gmem->unmappable_gfns);
> > +#endif
> > +
> > fd_install(fd, file);
> > return fd;
> >
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 1192942aef91..f4b4498d4de6 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -3265,6 +3265,128 @@ static int next_segment(unsigned long len, int offset)
> > return len;
> > }
> >
> > +#ifdef CONFIG_KVM_PRIVATE_MEM_MAPPABLE
> > +static bool __kvm_gmem_is_mappable(struct kvm *kvm, gfn_t start, gfn_t end)
> > +{
> > + struct kvm_memslot_iter iter;
> > +
> > + lockdep_assert_held(&kvm->slots_lock);
> > +
> > + kvm_for_each_memslot_in_gfn_range(&iter, kvm_memslots(kvm), start, end) {
> > + struct kvm_memory_slot *memslot = iter.slot;
> > + gfn_t gfn_start, gfn_end, i;
> > +
> > + gfn_start = max(start, memslot->base_gfn);
> > + gfn_end = min(end, memslot->base_gfn + memslot->npages);
> > + if (WARN_ON_ONCE(gfn_start >= gfn_end))
> > + continue;
> > +
> > + for (i = gfn_start; i < gfn_end; i++) {
> > + if (!kvm_slot_gmem_is_mappable(memslot, i))
> > + return false;
> > + }
> > + }
> > +
> > + return true;
> > +}
> > +
> > +bool kvm_gmem_is_mappable(struct kvm *kvm, gfn_t start, gfn_t end)
> > +{
> > + bool r;
> > +
> > + mutex_lock(&kvm->slots_lock);
> > + r = __kvm_gmem_is_mappable(kvm, start, end);
> > + mutex_unlock(&kvm->slots_lock);
> > +
> > + return r;
> > +}
> > +
> > +static bool __kvm_gmem_is_mapped(struct kvm *kvm, gfn_t start, gfn_t end)
> > +{
> > + struct kvm_memslot_iter iter;
> > +
> > + lockdep_assert_held(&kvm->slots_lock);
> > +
> > + kvm_for_each_memslot_in_gfn_range(&iter, kvm_memslots(kvm), start, end) {
> > + struct kvm_memory_slot *memslot = iter.slot;
> > + gfn_t gfn_start, gfn_end, i;
> > +
> > + gfn_start = max(start, memslot->base_gfn);
> > + gfn_end = min(end, memslot->base_gfn + memslot->npages);
> > + if (WARN_ON_ONCE(gfn_start >= gfn_end))
> > + continue;
> > +
> > + for (i = gfn_start; i < gfn_end; i++) {
> > + struct page *page;
> > + bool is_mapped;
> > + kvm_pfn_t pfn;
> > +
> > + if (WARN_ON_ONCE(kvm_gmem_get_pfn_locked(kvm, memslot, i, &pfn, NULL)))
> > + continue;
> > +
> > + page = pfn_to_page(pfn);
> > + is_mapped = page_mapped(page) || page_maybe_dma_pinned(page);
> > + unlock_page(page);
> > + put_page(page);
> > +
> > + if (is_mapped)
> > + return true;
> > + }
> > + }
> > +
> > + return false;
> > +}
> > +
> > +bool kvm_gmem_is_mapped(struct kvm *kvm, gfn_t start, gfn_t end)
> > +{
> > + bool r;
> > +
> > + mutex_lock(&kvm->slots_lock);
> > + r = __kvm_gmem_is_mapped(kvm, start, end);
> > + mutex_unlock(&kvm->slots_lock);
> > +
> > + return r;
> > +}
> > +
> > +static int kvm_gmem_toggle_mappable(struct kvm *kvm, gfn_t start, gfn_t end,
> > + bool is_mappable)
> > +{
> > + struct kvm_memslot_iter iter;
> > + int r = 0;
> > +
> > + mutex_lock(&kvm->slots_lock);
> > +
> > + kvm_for_each_memslot_in_gfn_range(&iter, kvm_memslots(kvm), start, end) {
> > + struct kvm_memory_slot *memslot = iter.slot;
> > + gfn_t gfn_start, gfn_end;
> > +
> > + gfn_start = max(start, memslot->base_gfn);
> > + gfn_end = min(end, memslot->base_gfn + memslot->npages);
> > + if (WARN_ON_ONCE(start >= end))
> > + continue;
> > +
> > + r = kvm_slot_gmem_toggle_mappable(memslot, gfn_start, gfn_end, is_mappable);
> > + if (WARN_ON_ONCE(r))
> > + break;
> > + }
> > +
> > + mutex_unlock(&kvm->slots_lock);
> > +
> > + return r;
> > +}
> > +
> > +int kvm_gmem_set_mappable(struct kvm *kvm, gfn_t start, gfn_t end)
> > +{
> > + return kvm_gmem_toggle_mappable(kvm, start, end, true);
> > +}
> > +
> > +int kvm_gmem_clear_mappable(struct kvm *kvm, gfn_t start, gfn_t end)
> > +{
> > + return kvm_gmem_toggle_mappable(kvm, start, end, false);
> > +}
> > +
> > +#endif /* CONFIG_KVM_PRIVATE_MEM_MAPPABLE */
> > +
> > /* Copy @len bytes from guest memory at '(@gfn * PAGE_SIZE) + @offset' to @data */
> > static int __kvm_read_guest_page(struct kvm_memory_slot *slot, gfn_t gfn,
> > void *data, int offset, int len)
>
> [1] https://lore.kernel.org/all/20230914015531.1419405-15-seanjc@xxxxxxxxxx/
> [2] https://lore.kernel.org/all/cover.1691446946.git.ackerleytng@xxxxxxxxxx/T/
> [3] https://lore.kernel.org/all/ZQOmcc969s90DwNz@xxxxxxxxxx/
