On 06.07.22 10:20, Chao Peng wrote:

Normally, a write to unallocated space of a file or the hole of a sparse file automatically causes space allocation, for memfd, this equals to memory allocation. This new seal prevents such automatically allocating, either this is from a direct write() or a write on the previously mmap-ed area. The seal does not prevent fallocate() so an explicit fallocate() can still cause allocating and can be used to reserve memory.

This is used to prevent unintentional allocation from userspace on a stray or careless write and any intentional allocation should use an explicit fallocate(). One of the main usecases is to avoid memory double allocation for confidential computing usage where we use two memfds to back guest memory and at a single point only one memfd is alive and we want to prevent memory allocation for the other memfd which may have been mmap-ed previously. More discussion can be found at:

https://lkml.org/lkml/2022/6/14/1255

Suggested-by: Sean Christopherson seanjc@google.com Signed-off-by: Chao Peng chao.p.peng@linux.intel.com

---

include/uapi/linux/fcntl.h | 1 + mm/memfd.c | 3 ++- mm/shmem.c | 16 ++++++++++++++-- 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h index 2f86b2ad6d7e..98bdabc8e309 100644 --- a/include/uapi/linux/fcntl.h +++ b/include/uapi/linux/fcntl.h @@ -43,6 +43,7 @@ #define F_SEAL_GROW 0x0004 /* prevent file from growing */ #define F_SEAL_WRITE 0x0008 /* prevent writes */ #define F_SEAL_FUTURE_WRITE 0x0010 /* prevent future writes while mapped */ +#define F_SEAL_AUTO_ALLOCATE 0x0020 /* prevent allocation for writes */

Why only "on writes" and not "on reads". IIRC, shmem doesn't support the shared zeropage, so you'll simply allocate a new page via read() or on read faults.

Also, I *think* you can place pages via userfaultfd into shmem. Not sure if that would count "auto alloc", but it would certainly bypass fallocate().

On Thu, Jul 21, 2022, David Hildenbrand wrote:

On 21.07.22 11:44, David Hildenbrand wrote:

...

On 06.07.22 10:20, Chao Peng wrote:

...

Normally, a write to unallocated space of a file or the hole of a sparse file automatically causes space allocation, for memfd, this equals to memory allocation. This new seal prevents such automatically allocating, either this is from a direct write() or a write on the previously mmap-ed area. The seal does not prevent fallocate() so an explicit fallocate() can still cause allocating and can be used to reserve memory.

This is used to prevent unintentional allocation from userspace on a stray or careless write and any intentional allocation should use an explicit fallocate(). One of the main usecases is to avoid memory double allocation for confidential computing usage where we use two memfds to back guest memory and at a single point only one memfd is alive and we want to prevent memory allocation for the other memfd which may have been mmap-ed previously. More discussion can be found at:

https://lkml.org/lkml/2022/6/14/1255

Suggested-by: Sean Christopherson seanjc@google.com Signed-off-by: Chao Peng chao.p.peng@linux.intel.com

---

include/uapi/linux/fcntl.h | 1 + mm/memfd.c | 3 ++- mm/shmem.c | 16 ++++++++++++++-- 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h index 2f86b2ad6d7e..98bdabc8e309 100644 --- a/include/uapi/linux/fcntl.h +++ b/include/uapi/linux/fcntl.h @@ -43,6 +43,7 @@ #define F_SEAL_GROW 0x0004 /* prevent file from growing */ #define F_SEAL_WRITE 0x0008 /* prevent writes */ #define F_SEAL_FUTURE_WRITE 0x0010 /* prevent future writes while mapped */ +#define F_SEAL_AUTO_ALLOCATE 0x0020 /* prevent allocation for writes */

Why only "on writes" and not "on reads". IIRC, shmem doesn't support the shared zeropage, so you'll simply allocate a new page via read() or on read faults.

Correction: on read() we don't allocate a fresh page. But on read faults we would. So this comment here needs clarification.

Not just the comment, the code too. The intent of F_SEAL_AUTO_ALLOCATE is very much to block _all_ implicit allocations (or maybe just fault-based allocations if "implicit" is too broad of a description).

...

Normally, a write to unallocated space of a file or the hole of a sparse file automatically causes space allocation, for memfd, this equals to memory allocation. This new seal prevents such automatically allocating, either this is from a direct write() or a write on the previously mmap-ed area. The seal does not prevent fallocate() so an explicit fallocate() can still cause allocating and can be used to reserve memory.

This is used to prevent unintentional allocation from userspace on a stray or careless write and any intentional allocation should use an explicit fallocate(). One of the main usecases is to avoid memory double allocation for confidential computing usage where we use two memfds to back guest memory and at a single point only one memfd is alive and we want to prevent memory allocation for the other memfd which may have been mmap-ed previously. More discussion can be found at:

https://lkml.org/lkml/2022/6/14/1255

Suggested-by: Sean Christopherson seanjc@google.com Signed-off-by: Chao Peng chao.p.peng@linux.intel.com

---

include/uapi/linux/fcntl.h | 1 + mm/memfd.c | 3 ++- mm/shmem.c | 16 ++++++++++++++-- 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h index 2f86b2ad6d7e..98bdabc8e309 100644 --- a/include/uapi/linux/fcntl.h +++ b/include/uapi/linux/fcntl.h @@ -43,6 +43,7 @@ #define F_SEAL_GROW 0x0004 /* prevent file from growing */ #define F_SEAL_WRITE 0x0008 /* prevent writes */ #define F_SEAL_FUTURE_WRITE 0x0010 /* prevent future writes while mapped */ +#define F_SEAL_AUTO_ALLOCATE 0x0020 /* prevent allocation for writes */

Why only "on writes" and not "on reads". IIRC, shmem doesn't support the shared zeropage, so you'll simply allocate a new page via read() or on read faults.

Also, I *think* you can place pages via userfaultfd into shmem. Not sure if that would count "auto alloc", but it would certainly bypass fallocate().

I was also thinking this at the same time, but for different reason:

"Want to populate private preboot memory with firmware payload", so was thinking userfaulftd could be an option as direct writes are restricted?

Thanks, Pankaj

...

...

...

Normally, a write to unallocated space of a file or the hole of a sparse file automatically causes space allocation, for memfd, this equals to memory allocation. This new seal prevents such automatically allocating, either this is from a direct write() or a write on the previously mmap-ed area. The seal does not prevent fallocate() so an explicit fallocate() can still cause allocating and can be used to reserve memory.

This is used to prevent unintentional allocation from userspace on a stray or careless write and any intentional allocation should use an explicit fallocate(). One of the main usecases is to avoid memory double allocation for confidential computing usage where we use two memfds to back guest memory and at a single point only one memfd is alive and we want to prevent memory allocation for the other memfd which may have been mmap-ed previously. More discussion can be found at:

https://lkml.org/lkml/2022/6/14/1255

Suggested-by: Sean Christopherson seanjc@google.com Signed-off-by: Chao Peng chao.p.peng@linux.intel.com

---

include/uapi/linux/fcntl.h | 1 + mm/memfd.c | 3 ++- mm/shmem.c | 16 ++++++++++++++-- 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h index 2f86b2ad6d7e..98bdabc8e309 100644 --- a/include/uapi/linux/fcntl.h +++ b/include/uapi/linux/fcntl.h @@ -43,6 +43,7 @@ #define F_SEAL_GROW 0x0004 /* prevent file from growing */ #define F_SEAL_WRITE 0x0008 /* prevent writes */ #define F_SEAL_FUTURE_WRITE 0x0010 /* prevent future writes while mapped */ +#define F_SEAL_AUTO_ALLOCATE 0x0020 /* prevent allocation for writes */

Why only "on writes" and not "on reads". IIRC, shmem doesn't support the shared zeropage, so you'll simply allocate a new page via read() or on read faults.

Also, I *think* you can place pages via userfaultfd into shmem. Not sure if that would count "auto alloc", but it would certainly bypass fallocate().

I was also thinking this at the same time, but for different reason:

"Want to populate private preboot memory with firmware payload", so was thinking userfaulftd could be an option as direct writes are restricted?

If that can be a side effect, I definitely glad to see it, though I'm still not clear how userfaultfd can be particularly helpful for that.

Was thinking if we can use userfaultfd to monitor the pagefault on virtual firmware memory range and use to populate the private memory.

Not sure if it is a side effect. Was just theoretically thinking (for now kept the idea aside as these enhancements can be worked later).

Thanks, Pankaj

Hi Chao,

On Wed, Jul 6, 2022 at 9:25 AM Chao Peng chao.p.peng@linux.intel.com wrote:

Normally, a write to unallocated space of a file or the hole of a sparse file automatically causes space allocation, for memfd, this equals to memory allocation. This new seal prevents such automatically allocating, either this is from a direct write() or a write on the previously mmap-ed area. The seal does not prevent fallocate() so an explicit fallocate() can still cause allocating and can be used to reserve memory.

This is used to prevent unintentional allocation from userspace on a stray or careless write and any intentional allocation should use an explicit fallocate(). One of the main usecases is to avoid memory double allocation for confidential computing usage where we use two memfds to back guest memory and at a single point only one memfd is alive and we want to prevent memory allocation for the other memfd which may have been mmap-ed previously. More discussion can be found at:

https://lkml.org/lkml/2022/6/14/1255

Suggested-by: Sean Christopherson seanjc@google.com Signed-off-by: Chao Peng chao.p.peng@linux.intel.com

---

include/uapi/linux/fcntl.h | 1 + mm/memfd.c | 3 ++- mm/shmem.c | 16 ++++++++++++++-- 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h index 2f86b2ad6d7e..98bdabc8e309 100644 --- a/include/uapi/linux/fcntl.h +++ b/include/uapi/linux/fcntl.h @@ -43,6 +43,7 @@ #define F_SEAL_GROW 0x0004 /* prevent file from growing */ #define F_SEAL_WRITE 0x0008 /* prevent writes */ #define F_SEAL_FUTURE_WRITE 0x0010 /* prevent future writes while mapped */ +#define F_SEAL_AUTO_ALLOCATE 0x0020 /* prevent allocation for writes */

I think this should also be added to tools/include/uapi/linux/fcntl.h

Cheers, /fuad

/* (1U << 31) is reserved for signed error codes */

/* diff --git a/mm/memfd.c b/mm/memfd.c index 08f5f8304746..2afd898798e4 100644 --- a/mm/memfd.c +++ b/mm/memfd.c @@ -150,7 +150,8 @@ static unsigned int *memfd_file_seals_ptr(struct file *file) F_SEAL_SHRINK | \ F_SEAL_GROW | \ F_SEAL_WRITE | \

•                F_SEAL_FUTURE_WRITE)
  

•                F_SEAL_FUTURE_WRITE | \
  

•                F_SEAL_AUTO_ALLOCATE)
  
  

static int memfd_add_seals(struct file *file, unsigned int seals) { diff --git a/mm/shmem.c b/mm/shmem.c index a6f565308133..6c8aef15a17d 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -2051,6 +2051,8 @@ static vm_fault_t shmem_fault(struct vm_fault *vmf) struct vm_area_struct *vma = vmf->vma; struct inode *inode = file_inode(vma->vm_file); gfp_t gfp = mapping_gfp_mask(inode->i_mapping);

•   struct shmem_inode_info *info = SHMEM_I(inode);
  

•   enum sgp_type sgp;
    int err;
    vm_fault_t ret = VM_FAULT_LOCKED;
  
  

@@ -2113,7 +2115,12 @@ static vm_fault_t shmem_fault(struct vm_fault *vmf) spin_unlock(&inode->i_lock); }

•   err = shmem_getpage_gfp(inode, vmf->pgoff, &vmf->page, SGP_CACHE,
  

•   if (unlikely(info->seals & F_SEAL_AUTO_ALLOCATE))
  

•           sgp = SGP_NOALLOC;
  

•   else
  

•           sgp = SGP_CACHE;
  

•   err = shmem_getpage_gfp(inode, vmf->pgoff, &vmf->page, sgp,
                              gfp, vma, vmf, &ret);
    if (err)
            return vmf_error(err);
  

@@ -2459,6 +2466,7 @@ shmem_write_begin(struct file *file, struct address_space *mapping, struct inode *inode = mapping->host; struct shmem_inode_info *info = SHMEM_I(inode); pgoff_t index = pos >> PAGE_SHIFT;

•   enum sgp_type sgp;
    int ret = 0;
  
    /* i_rwsem is held by caller */
  

@@ -2470,7 +2478,11 @@ shmem_write_begin(struct file *file, struct address_space *mapping, return -EPERM; }

•   ret = shmem_getpage(inode, index, pagep, SGP_WRITE);
  

•   if (unlikely(info->seals & F_SEAL_AUTO_ALLOCATE))
  

•           sgp = SGP_NOALLOC;
  

•   else
  

•           sgp = SGP_WRITE;
  

•   ret = shmem_getpage(inode, index, pagep, sgp);
  
    if (ret)
            return ret;
  

-- 2.25.1

On 7/6/2022 10:20 AM, Chao Peng wrote:

From: "Kirill A. Shutemov" kirill.shutemov@linux.intel.com

Implement shmem as a memfile_notifier backing store. Essentially it interacts with the memfile_notifier feature flags for userspace access/page migration/page reclaiming and implements the necessary memfile_backing_store callbacks.

Signed-off-by: Kirill A. Shutemov kirill.shutemov@linux.intel.com Signed-off-by: Chao Peng chao.p.peng@linux.intel.com

---

include/linux/shmem_fs.h | 2 + mm/shmem.c | 109 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 110 insertions(+), 1 deletion(-)

diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h index a68f982f22d1..6031c0b08d26 100644 --- a/include/linux/shmem_fs.h +++ b/include/linux/shmem_fs.h @@ -9,6 +9,7 @@ #include <linux/percpu_counter.h> #include <linux/xattr.h> #include <linux/fs_parser.h> +#include <linux/memfile_notifier.h> /* inode in-kernel data */ @@ -25,6 +26,7 @@ struct shmem_inode_info { struct simple_xattrs xattrs; /* list of xattrs */ atomic_t stop_eviction; /* hold when working on inode */ struct timespec64 i_crtime; /* file creation time */

• struct memfile_node memfile_node; /* memfile node */ struct inode vfs_inode; };

diff --git a/mm/shmem.c b/mm/shmem.c index 6c8aef15a17d..627e315c3b4d 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -905,6 +905,17 @@ static struct folio *shmem_get_partial_folio(struct inode *inode, pgoff_t index) return page ? page_folio(page) : NULL; } +static void notify_invalidate(struct inode *inode, struct folio *folio,

• 		   pgoff_t start, pgoff_t end)
  

+{

• struct shmem_inode_info *info = SHMEM_I(inode);
• start = max(start, folio->index);
• end = min(end, folio->index + folio_nr_pages(folio));
• memfile_notifier_invalidate(&info->memfile_node, start, end);

+}

• /*
  • Remove range of pages and swap entries from page cache, and free them.
  • If !unfalloc, truncate or punch hole; if unfalloc, undo failed fallocate.

@@ -948,6 +959,8 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend, } index += folio_nr_pages(folio) - 1;

• 	notify_invalidate(inode, folio, start, end);
  

• if (!unfalloc || !folio_test_uptodate(folio))
  	truncate_inode_folio(mapping, folio);
  folio_unlock(folio);
  

@@ -1021,6 +1034,9 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend, index--; break; }

• 		notify_invalidate(inode, folio, start, end);
  

• 	VM_BUG_ON_FOLIO(folio_test_writeback(folio),
  			folio);
  	truncate_inode_folio(mapping, folio);
  

@@ -1092,6 +1108,13 @@ static int shmem_setattr(struct user_namespace *mnt_userns, (newsize > oldsize && (info->seals & F_SEAL_GROW))) return -EPERM;

• if (info->memfile_node.flags & MEMFILE_F_USER_INACCESSIBLE) {
  

• 	if (oldsize)
  

• 		return -EPERM;
  

• 	if (!PAGE_ALIGNED(newsize))
  

• 		return -EINVAL;
  

• }
  

• if (newsize != oldsize) { error = shmem_reacct_size(SHMEM_I(inode)->flags, oldsize, newsize);

@@ -1336,6 +1359,8 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc) goto redirty; if (!total_swap_pages) goto redirty;

• if (info->memfile_node.flags & MEMFILE_F_UNRECLAIMABLE)

• goto redirty;
  

/* * Our capabilities prevent regular writeback or sync from ever calling @@ -2271,6 +2296,9 @@ static int shmem_mmap(struct file *file, struct vm_area_struct *vma) if (ret) return ret;

• if (info->memfile_node.flags & MEMFILE_F_USER_INACCESSIBLE)

• return -EPERM;
  

• /* arm64 - allow memory tagging on RAM-based files */ vma->vm_flags |= VM_MTE_ALLOWED;

@@ -2306,6 +2334,7 @@ static struct inode *shmem_get_inode(struct super_block *sb, const struct inode info->i_crtime = inode->i_mtime; INIT_LIST_HEAD(&info->shrinklist); INIT_LIST_HEAD(&info->swaplist);

• memfile_node_init(&info->memfile_node);
  

  simple_xattrs_init(&info->xattrs); cache_no_acl(inode); mapping_set_large_folios(inode->i_mapping);

@@ -2477,6 +2506,8 @@ shmem_write_begin(struct file *file, struct address_space *mapping, if ((info->seals & F_SEAL_GROW) && pos + len > inode->i_size) return -EPERM; }

• if (unlikely(info->memfile_node.flags & MEMFILE_F_USER_INACCESSIBLE))

• return -EPERM;
  

if (unlikely(info->seals & F_SEAL_AUTO_ALLOCATE)) sgp = SGP_NOALLOC; @@ -2556,6 +2587,13 @@ static ssize_t shmem_file_read_iter(struct kiocb *iocb, struct iov_iter *to) end_index = i_size >> PAGE_SHIFT; if (index > end_index) break;

• if (SHMEM_I(inode)->memfile_node.flags &
  

• 		MEMFILE_F_USER_INACCESSIBLE) {
  

• 	error = -EPERM;
  

• 	break;
  

• }
  

• if (index == end_index) { nr = i_size & ~PAGE_MASK; if (nr <= offset)

@@ -2697,6 +2735,12 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset, goto out; }

• if ((info->memfile_node.flags & MEMFILE_F_USER_INACCESSIBLE) &&
  

•     (!PAGE_ALIGNED(offset) || !PAGE_ALIGNED(len))) {
  

• 	error = -EINVAL;
  

• 	goto out;
  

• }
  

• shmem_falloc.waitq = &shmem_falloc_waitq; shmem_falloc.start = (u64)unmap_start >> PAGE_SHIFT; shmem_falloc.next = (unmap_end + 1) >> PAGE_SHIFT;

@@ -3806,6 +3850,20 @@ static int shmem_error_remove_page(struct address_space *mapping, return 0; } +#ifdef CONFIG_MIGRATION +static int shmem_migrate_page(struct address_space *mapping,

• 	      struct page *newpage, struct page *page,
  

• 	      enum migrate_mode mode)
  

+{

• struct inode *inode = mapping->host;
• struct shmem_inode_info *info = SHMEM_I(inode);
• if (info->memfile_node.flags & MEMFILE_F_UNMOVABLE)

• return -EOPNOTSUPP;
  

• return migrate_page(mapping, newpage, page, mode);

Wondering how well page migrate would work for private pages on shmem memfd based backend?

+} +#endif

• const struct address_space_operations shmem_aops = { .writepage = shmem_writepage, .dirty_folio = noop_dirty_folio,

@@ -3814,7 +3872,7 @@ const struct address_space_operations shmem_aops = { .write_end = shmem_write_end, #endif #ifdef CONFIG_MIGRATION

• .migratepage = migrate_page,

• .migratepage = shmem_migrate_page, #endif .error_remove_page = shmem_error_remove_page, };

@@ -3931,6 +3989,51 @@ static struct file_system_type shmem_fs_type = { .fs_flags = FS_USERNS_MOUNT, }; +#ifdef CONFIG_MEMFILE_NOTIFIER +static struct memfile_node *shmem_lookup_memfile_node(struct file *file) +{

• struct inode *inode = file_inode(file);
• if (!shmem_mapping(inode->i_mapping))

• return NULL;
  

• return &SHMEM_I(inode)->memfile_node;

+}

+static int shmem_get_pfn(struct file *file, pgoff_t offset, pfn_t *pfn,

• 	 int *order)
  

+{

• struct page *page;
• int ret;
• ret = shmem_getpage(file_inode(file), offset, &page, SGP_WRITE);
• if (ret)

• return ret;
  

• unlock_page(page);
• *pfn = page_to_pfn_t(page);
• *order = thp_order(compound_head(page));
• return 0;

+}

+static void shmem_put_pfn(pfn_t pfn) +{

• struct page *page = pfn_t_to_page(pfn);
• if (!page)

• return;
  

• put_page(page);

+}

+static struct memfile_backing_store shmem_backing_store = {

• .lookup_memfile_node = shmem_lookup_memfile_node,
• .get_pfn = shmem_get_pfn,
• .put_pfn = shmem_put_pfn,

+}; +#endif /* CONFIG_MEMFILE_NOTIFIER */

• void __init shmem_init(void) { int error;

@@ -3956,6 +4059,10 @@ void __init shmem_init(void) else shmem_huge = SHMEM_HUGE_NEVER; /* just in case it was patched */ #endif

+#ifdef CONFIG_MEMFILE_NOTIFIER

• memfile_register_backing_store(&shmem_backing_store);

+#endif return; out1:

On Fri, Aug 05, 2022 at 03:26:02PM +0200, David Hildenbrand wrote:

On 06.07.22 10:20, Chao Peng wrote:

...

From: "Kirill A. Shutemov" kirill.shutemov@linux.intel.com

Implement shmem as a memfile_notifier backing store. Essentially it interacts with the memfile_notifier feature flags for userspace access/page migration/page reclaiming and implements the necessary memfile_backing_store callbacks.

Signed-off-by: Kirill A. Shutemov kirill.shutemov@linux.intel.com Signed-off-by: Chao Peng chao.p.peng@linux.intel.com

---

[...]

...

+#ifdef CONFIG_MEMFILE_NOTIFIER +static struct memfile_node *shmem_lookup_memfile_node(struct file *file) +{

• struct inode *inode = file_inode(file);
• if (!shmem_mapping(inode->i_mapping))

• return NULL;
  

• return &SHMEM_I(inode)->memfile_node;

+}

+static int shmem_get_pfn(struct file *file, pgoff_t offset, pfn_t *pfn,

• 	 int *order)
  

+{

• struct page *page;
• int ret;
• ret = shmem_getpage(file_inode(file), offset, &page, SGP_WRITE);
• if (ret)

• return ret;
  

• unlock_page(page);
• *pfn = page_to_pfn_t(page);
• *order = thp_order(compound_head(page));
• return 0;

+}

+static void shmem_put_pfn(pfn_t pfn) +{

• struct page *page = pfn_t_to_page(pfn);
• if (!page)

• return;
  

• put_page(page);

Why do we export shmem_get_pfn/shmem_put_pfn and not simply

get_folio()

and let the caller deal with putting the folio? What's the reason to

a) Operate on PFNs and not folios b) Have these get/put semantics?

We have a design assumption that somedays this can even support non-page based backing stores. There are some discussions: https://lkml.org/lkml/2022/3/28/1440 I should add document for this two callbacks.

...

+}

+static struct memfile_backing_store shmem_backing_store = {

• .lookup_memfile_node = shmem_lookup_memfile_node,
• .get_pfn = shmem_get_pfn,
• .put_pfn = shmem_put_pfn,

+}; +#endif /* CONFIG_MEMFILE_NOTIFIER */

void __init shmem_init(void) { int error; @@ -3956,6 +4059,10 @@ void __init shmem_init(void) else shmem_huge = SHMEM_HUGE_NEVER; /* just in case it was patched */ #endif

+#ifdef CONFIG_MEMFILE_NOTIFIER

• memfile_register_backing_store(&shmem_backing_store);

Can we instead prove a dummy function that does nothing without CONFIG_MEMFILE_NOTIFIER?

Sounds good.

Chao

...

+#endif return; out1:

-- Thanks,

David / dhildenb

On Fri, Jul 15, 2022 at 01:36:15PM +0200, Gupta, Pankaj wrote:

...

Currently in mmu_notifier validate path, hva range is recorded and then checked in the mmu_notifier_retry_hva() from page fault path. However for the to be introduced private memory, a page fault may not have a hva

As this patch appeared in v7, just wondering did you see an actual bug because of it? And not having corresponding 'hva' occurs only with private memory because its not mapped to host userspace?

The addressed problem is not new in this version, previous versions I also had code to handle it (just in different way). But the problem is: mmu_notifier/memfile_notifier may be in the progress of invalidating a pfn that obtained earlier in the page fault handler, when happens, we should retry the fault. In v6 I used global mmu_notifier_retry() for memfile_notifier but that can block unrelated mmu_notifer invalidation which has hva range specified.

Sean gave a comment at https://lkml.org/lkml/2022/6/17/1001 to separate memfile_notifier from mmu_notifier but during the implementation I realized we actually can reuse the same code for shared and private memory if both using gpa range and that can simplify the code handling in kvm_zap_gfn_range and some other code (e.g. we don't need two versions for memfile_notifier/mmu_notifier).

Adding gpa range for private memory invalidation also relieves the above blocking issue between private memory page fault and mmu_notifier.

Chao

Thanks, Pankaj

...

associated, checking gfn(gpa) makes more sense. For existing non private memory case, gfn is expected to continue to work.

The patch also fixes a potential bug in kvm_zap_gfn_range() which has already been using gfn when calling kvm_inc/dec_notifier_count() in current code.

Signed-off-by: Chao Peng chao.p.peng@linux.intel.com

arch/x86/kvm/mmu/mmu.c | 2 +- include/linux/kvm_host.h | 18 ++++++++---------- virt/kvm/kvm_main.c | 6 +++--- 3 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index f7fa4c31b7c5..0d882fad4bc1 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -4182,7 +4182,7 @@ static bool is_page_fault_stale(struct kvm_vcpu *vcpu, return true; return fault->slot &&

•       mmu_notifier_retry_hva(vcpu->kvm, mmu_seq, fault->hva);
  

•       mmu_notifier_retry_gfn(vcpu->kvm, mmu_seq, fault->gfn);
  

  } static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 0bdb6044e316..e9153b54e2a4 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -767,8 +767,8 @@ struct kvm { struct mmu_notifier mmu_notifier; unsigned long mmu_notifier_seq; long mmu_notifier_count;

• unsigned long mmu_notifier_range_start;
• unsigned long mmu_notifier_range_end;

• gfn_t mmu_notifier_range_start;
• gfn_t mmu_notifier_range_end; #endif struct list_head devices; u64 manual_dirty_log_protect;

@@ -1362,10 +1362,8 @@ void kvm_mmu_free_memory_cache(struct kvm_mmu_memory_cache *mc); void *kvm_mmu_memory_cache_alloc(struct kvm_mmu_memory_cache *mc); #endif -void kvm_inc_notifier_count(struct kvm *kvm, unsigned long start,

• 		   unsigned long end);
  

-void kvm_dec_notifier_count(struct kvm *kvm, unsigned long start,

• 		   unsigned long end);
  

+void kvm_inc_notifier_count(struct kvm *kvm, gfn_t start, gfn_t end); +void kvm_dec_notifier_count(struct kvm *kvm, gfn_t start, gfn_t end); long kvm_arch_dev_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg); @@ -1923,9 +1921,9 @@ static inline int mmu_notifier_retry(struct kvm *kvm, unsigned long mmu_seq) return 0; } -static inline int mmu_notifier_retry_hva(struct kvm *kvm, +static inline int mmu_notifier_retry_gfn(struct kvm *kvm, unsigned long mmu_seq,

• 			 unsigned long hva)
  

• 			 gfn_t gfn)
  

  { lockdep_assert_held(&kvm->mmu_lock); /*

@@ -1935,8 +1933,8 @@ static inline int mmu_notifier_retry_hva(struct kvm *kvm, * positives, due to shortcuts when handing concurrent invalidations. */ if (unlikely(kvm->mmu_notifier_count) &&

•    hva >= kvm->mmu_notifier_range_start &&
  

•    hva < kvm->mmu_notifier_range_end)
  

•    gfn >= kvm->mmu_notifier_range_start &&
  

•    gfn < kvm->mmu_notifier_range_end)
  

  return 1; if (kvm->mmu_notifier_seq != mmu_seq) return 1;

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index da263c370d00..4d7f0e72366f 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -536,8 +536,7 @@ static void kvm_mmu_notifier_invalidate_range(struct mmu_notifier *mn, typedef bool (*hva_handler_t)(struct kvm *kvm, struct kvm_gfn_range *range); -typedef void (*on_lock_fn_t)(struct kvm *kvm, unsigned long start,

• 	     unsigned long end);
  

+typedef void (*on_lock_fn_t)(struct kvm *kvm, gfn_t start, gfn_t end); typedef void (*on_unlock_fn_t)(struct kvm *kvm); @@ -624,7 +623,8 @@ static __always_inline int __kvm_handle_hva_range(struct kvm *kvm, locked = true; KVM_MMU_LOCK(kvm); if (!IS_KVM_NULL_FN(range->on_lock))

• 			range->on_lock(kvm, range->start, range->end);
  

• 			range->on_lock(kvm, gfn_range.start,
  

• 					    gfn_range.end);
  	if (IS_KVM_NULL_FN(range->handler))
  		break;
  }
  

On Thu, Aug 04, 2022 at 12:10:44AM -0700, Isaku Yamahata wrote:

On Wed, Jul 06, 2022 at 04:20:09PM +0800, Chao Peng chao.p.peng@linux.intel.com wrote:

...

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 0bdb6044e316..e9153b54e2a4 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1362,10 +1362,8 @@ void kvm_mmu_free_memory_cache(struct kvm_mmu_memory_cache *mc); void *kvm_mmu_memory_cache_alloc(struct kvm_mmu_memory_cache *mc); #endif -void kvm_inc_notifier_count(struct kvm *kvm, unsigned long start,

• 		   unsigned long end);
  

-void kvm_dec_notifier_count(struct kvm *kvm, unsigned long start,

• 		   unsigned long end);
  

+void kvm_inc_notifier_count(struct kvm *kvm, gfn_t start, gfn_t end); +void kvm_dec_notifier_count(struct kvm *kvm, gfn_t start, gfn_t end); long kvm_arch_dev_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg);

The corresponding changes in kvm_main.c are missing.

Exactly! Actually it's in the next patch while it should indeed in this patch.

Chao

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index b2c79bef61bd..0184e327f6f5 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -711,8 +711,7 @@ static void kvm_mmu_notifier_change_pte(struct mmu_notifier *mn, kvm_handle_hva_range(mn, address, address + 1, pte, kvm_set_spte_gfn); } -void kvm_inc_notifier_count(struct kvm *kvm, unsigned long start,

•                              unsigned long end)
  

+void kvm_inc_notifier_count(struct kvm *kvm, gfn_t start, gfn_t end) { /* * The count increase must become visible at unlock time as no @@ -786,8 +785,7 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, return 0; } -void kvm_dec_notifier_count(struct kvm *kvm, unsigned long start,

•                              unsigned long end)
  

+void kvm_dec_notifier_count(struct kvm *kvm, gfn_t start, gfn_t end) { /* * This sequence increase will notify the kvm page fault that

-- Isaku Yamahata isaku.yamahata@gmail.com

On 2022-07-06 16:20:10, Chao Peng wrote:

The sync mechanism between mmu_notifier and page fault handler employs fields mmu_notifier_seq/count and mmu_notifier_range_start/end. For the to be added private memory, there is the same mechanism needed but not rely on mmu_notifier (It uses new introduced memfile_notifier). This patch renames the existing fields and related helper functions to a neutral name mmu_updating_* so private memory can reuse.

No functional change intended.

Signed-off-by: Chao Peng chao.p.peng@linux.intel.com

arch/arm64/kvm/mmu.c | 8 ++--- arch/mips/kvm/mmu.c | 10 +++--- arch/powerpc/include/asm/kvm_book3s_64.h | 2 +- arch/powerpc/kvm/book3s_64_mmu_host.c | 4 +-- arch/powerpc/kvm/book3s_64_mmu_hv.c | 4 +-- arch/powerpc/kvm/book3s_64_mmu_radix.c | 6 ++-- arch/powerpc/kvm/book3s_hv_nested.c | 2 +- arch/powerpc/kvm/book3s_hv_rm_mmu.c | 8 ++--- arch/powerpc/kvm/e500_mmu_host.c | 4 +-- arch/riscv/kvm/mmu.c | 4 +-- arch/x86/kvm/mmu/mmu.c | 14 ++++---- arch/x86/kvm/mmu/paging_tmpl.h | 4 +-- include/linux/kvm_host.h | 38 ++++++++++----------- virt/kvm/kvm_main.c | 42 +++++++++++------------- virt/kvm/pfncache.c | 14 ++++---- 15 files changed, 81 insertions(+), 83 deletions(-)

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c index 87f1cd0df36e..7ee6fafc24ee 100644 --- a/arch/arm64/kvm/mmu.c +++ b/arch/arm64/kvm/mmu.c @@ -993,7 +993,7 @@ transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot, * THP doesn't start to split while we are adjusting the * refcounts. *

•  * We are sure this doesn't happen, because mmu_notifier_retry
  

•  * We are sure this doesn't happen, because mmu_updating_retry
  

  • was successful and we are holding the mmu_lock, so if this
  • THP is trying to split, it will be blocked in the mmu
  • notifier before touching any of the pages, specifically

@@ -1188,9 +1188,9 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, return ret; }

• mmu_seq = vcpu->kvm->mmu_notifier_seq;

• mmu_seq = vcpu->kvm->mmu_updating_seq; /*

• * Ensure the read of mmu_notifier_seq happens before we call
  

• * Ensure the read of mmu_updating_seq happens before we call
  

  • gfn_to_pfn_prot (which calls get_user_pages), so that we don't risk
  • the page we just got a reference to gets unmapped before we have a
  • chance to grab the mmu_lock, which ensure that if the page gets

@@ -1246,7 +1246,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, else write_lock(&kvm->mmu_lock); pgt = vcpu->arch.hw_mmu->pgt;

• if (mmu_notifier_retry(kvm, mmu_seq))

• if (mmu_updating_retry(kvm, mmu_seq)) goto out_unlock;

/* diff --git a/arch/mips/kvm/mmu.c b/arch/mips/kvm/mmu.c index 1bfd1b501d82..abd468c6a749 100644 --- a/arch/mips/kvm/mmu.c +++ b/arch/mips/kvm/mmu.c @@ -615,17 +615,17 @@ static int kvm_mips_map_page(struct kvm_vcpu *vcpu, unsigned long gpa, * Used to check for invalidations in progress, of the pfn that is * returned by pfn_to_pfn_prot below. */

• mmu_seq = kvm->mmu_notifier_seq;

• mmu_seq = kvm->mmu_updating_seq; /*

• * Ensure the read of mmu_notifier_seq isn't reordered with PTE reads in
  

• * Ensure the read of mmu_updating_seq isn't reordered with PTE reads in
  

  • gfn_to_pfn_prot() (which calls get_user_pages()), so that we don't
  • risk the page we get a reference to getting unmapped before we have a

• * chance to grab the mmu_lock without mmu_notifier_retry() noticing.
  

• * chance to grab the mmu_lock without mmu_updating_retry () noticing.
  

  • This smp_rmb() pairs with the effective smp_wmb() of the combination
  • of the pte_unmap_unlock() after the PTE is zapped, and the
  • spin_lock() in kvm_mmu_notifier_invalidate_<page|range_end>() before

• * mmu_notifier_seq is incremented.
  

• * mmu_updating_seq is incremented.
  

  */ smp_rmb();

@@ -638,7 +638,7 @@ static int kvm_mips_map_page(struct kvm_vcpu *vcpu, unsigned long gpa, spin_lock(&kvm->mmu_lock); /* Check if an invalidation has taken place since we got pfn */

• if (mmu_notifier_retry(kvm, mmu_seq)) {

• if (mmu_updating_retry(kvm, mmu_seq)) { /*
  • This can happen when mappings are changed asynchronously, but
  • also synchronously if a COW is triggered by

diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h index 4def2bd17b9b..4d35fb913de5 100644 --- a/arch/powerpc/include/asm/kvm_book3s_64.h +++ b/arch/powerpc/include/asm/kvm_book3s_64.h @@ -666,7 +666,7 @@ static inline pte_t *find_kvm_host_pte(struct kvm *kvm, unsigned long mmu_seq, VM_WARN(!spin_is_locked(&kvm->mmu_lock), "%s called with kvm mmu_lock not held \n", __func__);

• if (mmu_notifier_retry(kvm, mmu_seq))

• if (mmu_updating_retry(kvm, mmu_seq)) return NULL;

pte = __find_linux_pte(kvm->mm->pgd, ea, NULL, hshift); diff --git a/arch/powerpc/kvm/book3s_64_mmu_host.c b/arch/powerpc/kvm/book3s_64_mmu_host.c index 1ae09992c9ea..78f1aae8cb60 100644 --- a/arch/powerpc/kvm/book3s_64_mmu_host.c +++ b/arch/powerpc/kvm/book3s_64_mmu_host.c @@ -90,7 +90,7 @@ int kvmppc_mmu_map_page(struct kvm_vcpu *vcpu, struct kvmppc_pte *orig_pte, unsigned long pfn; /* used to check for invalidations in progress */

• mmu_seq = kvm->mmu_notifier_seq;

• mmu_seq = kvm->mmu_updating_seq; smp_rmb();

/* Get host physical address for gpa */ @@ -151,7 +151,7 @@ int kvmppc_mmu_map_page(struct kvm_vcpu *vcpu, struct kvmppc_pte *orig_pte, cpte = kvmppc_mmu_hpte_cache_next(vcpu); spin_lock(&kvm->mmu_lock);

• if (!cpte || mmu_notifier_retry(kvm, mmu_seq)) {

• if (!cpte || mmu_updating_retry(kvm, mmu_seq)) { r = -EAGAIN; goto out_unlock; }

diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c index 514fd45c1994..bcdec6a6f2a7 100644 --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c @@ -578,7 +578,7 @@ int kvmppc_book3s_hv_page_fault(struct kvm_vcpu *vcpu, return -EFAULT; /* used to check for invalidations in progress */

• mmu_seq = kvm->mmu_notifier_seq;

• mmu_seq = kvm->mmu_updating_seq; smp_rmb();

ret = -EFAULT; @@ -693,7 +693,7 @@ int kvmppc_book3s_hv_page_fault(struct kvm_vcpu *vcpu, /* Check if we might have been invalidated; let the guest retry if so */ ret = RESUME_GUEST;

• if (mmu_notifier_retry(vcpu->kvm, mmu_seq)) {

• if (mmu_updating_retry(vcpu->kvm, mmu_seq)) { unlock_rmap(rmap); goto out_unlock; }

diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c index 42851c32ff3b..c8890ccc3f40 100644 --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c @@ -639,7 +639,7 @@ int kvmppc_create_pte(struct kvm *kvm, pgd_t *pgtable, pte_t pte, /* Check if we might have been invalidated; let the guest retry if so */ spin_lock(&kvm->mmu_lock); ret = -EAGAIN;

• if (mmu_notifier_retry(kvm, mmu_seq))

• if (mmu_updating_retry(kvm, mmu_seq)) goto out_unlock;

/* Now traverse again under the lock and change the tree */ @@ -829,7 +829,7 @@ int kvmppc_book3s_instantiate_page(struct kvm_vcpu *vcpu, bool large_enable; /* used to check for invalidations in progress */

• mmu_seq = kvm->mmu_notifier_seq;

• mmu_seq = kvm->mmu_updating_seq; smp_rmb();

/* @@ -1190,7 +1190,7 @@ void kvmppc_radix_flush_memslot(struct kvm *kvm, * Increase the mmu notifier sequence number to prevent any page * fault that read the memslot earlier from writing a PTE. */

• kvm->mmu_notifier_seq++;

• kvm->mmu_updating_seq++; spin_unlock(&kvm->mmu_lock);

} diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c index 0644732d1a25..09f841f730da 100644 --- a/arch/powerpc/kvm/book3s_hv_nested.c +++ b/arch/powerpc/kvm/book3s_hv_nested.c @@ -1579,7 +1579,7 @@ static long int __kvmhv_nested_page_fault(struct kvm_vcpu *vcpu, /* 2. Find the host pte for this L1 guest real address */ /* Used to check for invalidations in progress */

• mmu_seq = kvm->mmu_notifier_seq;

• mmu_seq = kvm->mmu_updating_seq; smp_rmb();

/* See if can find translation in our partition scoped tables for L1 */ diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c index 2257fb18cb72..952b504dc98a 100644 --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c @@ -219,7 +219,7 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags, g_ptel = ptel; /* used later to detect if we might have been invalidated */

• mmu_seq = kvm->mmu_notifier_seq;

• mmu_seq = kvm->mmu_updating_seq; smp_rmb();

/* Find the memslot (if any) for this address */ @@ -366,7 +366,7 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags, rmap = real_vmalloc_addr(rmap); lock_rmap(rmap); /* Check for pending invalidations under the rmap chain lock */

• if (mmu_notifier_retry(kvm, mmu_seq)) {
  

• if (mmu_updating_retry(kvm, mmu_seq)) {
  /* inval in progress, write a non-present HPTE */
  pteh |= HPTE_V_ABSENT;
  pteh &= ~HPTE_V_VALID;
  

@@ -932,7 +932,7 @@ static long kvmppc_do_h_page_init_zero(struct kvm_vcpu *vcpu, int i; /* Used later to detect if we might have been invalidated */

• mmu_seq = kvm->mmu_notifier_seq;

• mmu_seq = kvm->mmu_updating_seq; smp_rmb();

arch_spin_lock(&kvm->mmu_lock.rlock.raw_lock); @@ -960,7 +960,7 @@ static long kvmppc_do_h_page_init_copy(struct kvm_vcpu *vcpu, long ret = H_SUCCESS; /* Used later to detect if we might have been invalidated */

• mmu_seq = kvm->mmu_notifier_seq;

• mmu_seq = kvm->mmu_updating_seq; smp_rmb();

arch_spin_lock(&kvm->mmu_lock.rlock.raw_lock); diff --git a/arch/powerpc/kvm/e500_mmu_host.c b/arch/powerpc/kvm/e500_mmu_host.c index 7f16afc331ef..d7636b926f25 100644 --- a/arch/powerpc/kvm/e500_mmu_host.c +++ b/arch/powerpc/kvm/e500_mmu_host.c @@ -339,7 +339,7 @@ static inline int kvmppc_e500_shadow_map(struct kvmppc_vcpu_e500 *vcpu_e500, unsigned long flags; /* used to check for invalidations in progress */

• mmu_seq = kvm->mmu_notifier_seq;

• mmu_seq = kvm->mmu_updating_seq; smp_rmb();

/* @@ -460,7 +460,7 @@ static inline int kvmppc_e500_shadow_map(struct kvmppc_vcpu_e500 *vcpu_e500, } spin_lock(&kvm->mmu_lock);

• if (mmu_notifier_retry(kvm, mmu_seq)) {

• if (mmu_updating_retry(kvm, mmu_seq)) { ret = -EAGAIN; goto out; }

diff --git a/arch/riscv/kvm/mmu.c b/arch/riscv/kvm/mmu.c index 081f8d2b9cf3..a7db374d3861 100644 --- a/arch/riscv/kvm/mmu.c +++ b/arch/riscv/kvm/mmu.c @@ -654,7 +654,7 @@ int kvm_riscv_gstage_map(struct kvm_vcpu *vcpu, return ret; }

• mmu_seq = kvm->mmu_notifier_seq;

• mmu_seq = kvm->mmu_updating_seq;

hfn = gfn_to_pfn_prot(kvm, gfn, is_write, &writeable); if (hfn == KVM_PFN_ERR_HWPOISON) { @@ -674,7 +674,7 @@ int kvm_riscv_gstage_map(struct kvm_vcpu *vcpu, spin_lock(&kvm->mmu_lock);

• if (mmu_notifier_retry(kvm, mmu_seq))

• if (mmu_updating_retry(kvm, mmu_seq)) goto out_unlock;

if (writeable) { diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 0d882fad4bc1..545eb74305fe 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -2908,7 +2908,7 @@ static void direct_pte_prefetch(struct kvm_vcpu *vcpu, u64 *sptep) * If addresses are being invalidated, skip prefetching to avoid * accidentally prefetching those addresses. */

• if (unlikely(vcpu->kvm->mmu_notifier_count))

• if (unlikely(vcpu->kvm->mmu_updating_count)) return;

__direct_pte_prefetch(vcpu, sp, sptep); @@ -2950,7 +2950,7 @@ static int host_pfn_mapping_level(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn, /* * Lookup the mapping level in the current mm. The information * may become stale soon, but it is safe to use as long as

• * 1) mmu_notifier_retry was checked after taking mmu_lock, and
  

• * 1) mmu_updating_retry was checked after taking mmu_lock, and
  

  • 2. mmu_lock is taken now.
  • We still need to disable IRQs to prevent concurrent tear down

@@ -3035,7 +3035,7 @@ void kvm_mmu_hugepage_adjust(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault return; /*

• * mmu_notifier_retry() was successful and mmu_lock is held, so
  

• * mmu_updating_retry was successful and mmu_lock is held, so
  

  • the pmd can't be split from under us.
  */ fault->goal_level = fault->req_level;

@@ -4182,7 +4182,7 @@ static bool is_page_fault_stale(struct kvm_vcpu *vcpu, return true; return fault->slot &&

•       mmu_notifier_retry_gfn(vcpu->kvm, mmu_seq, fault->gfn);
  

•       mmu_updating_retry_gfn(vcpu->kvm, mmu_seq, fault->gfn);
  

} static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) @@ -4206,7 +4206,7 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault if (r) return r;

• mmu_seq = vcpu->kvm->mmu_notifier_seq;

• mmu_seq = vcpu->kvm->mmu_updating_seq; smp_rmb();

r = kvm_faultin_pfn(vcpu, fault); @@ -6023,7 +6023,7 @@ void kvm_zap_gfn_range(struct kvm *kvm, gfn_t gfn_start, gfn_t gfn_end) write_lock(&kvm->mmu_lock);

• kvm_inc_notifier_count(kvm, gfn_start, gfn_end);

• kvm_mmu_updating_begin(kvm, gfn_start, gfn_end);

flush = __kvm_zap_rmaps(kvm, gfn_start, gfn_end); @@ -6037,7 +6037,7 @@ void kvm_zap_gfn_range(struct kvm *kvm, gfn_t gfn_start, gfn_t gfn_end) kvm_flush_remote_tlbs_with_address(kvm, gfn_start, gfn_end - gfn_start);

• kvm_dec_notifier_count(kvm, gfn_start, gfn_end);

• kvm_mmu_updating_end(kvm, gfn_start, gfn_end);

write_unlock(&kvm->mmu_lock); } diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h index 2448fa8d8438..acf7e41aa02b 100644 --- a/arch/x86/kvm/mmu/paging_tmpl.h +++ b/arch/x86/kvm/mmu/paging_tmpl.h @@ -589,7 +589,7 @@ static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, struct guest_walker *gw, * If addresses are being invalidated, skip prefetching to avoid * accidentally prefetching those addresses. */

• if (unlikely(vcpu->kvm->mmu_notifier_count))

• if (unlikely(vcpu->kvm->mmu_updating_count)) return;

if (sp->role.direct) @@ -838,7 +838,7 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault else fault->max_level = walker.level;

• mmu_seq = vcpu->kvm->mmu_notifier_seq;

• mmu_seq = vcpu->kvm->mmu_updating_seq; smp_rmb();

r = kvm_faultin_pfn(vcpu, fault); diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index e9153b54e2a4..c262ebb168a7 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -765,10 +765,10 @@ struct kvm { #if defined(CONFIG_MMU_NOTIFIER) && defined(KVM_ARCH_WANT_MMU_NOTIFIER) struct mmu_notifier mmu_notifier;

• unsigned long mmu_notifier_seq;
• long mmu_notifier_count;
• gfn_t mmu_notifier_range_start;
• gfn_t mmu_notifier_range_end;

• unsigned long mmu_updating_seq;
• long mmu_updating_count;

Can we convert mmu_updating_seq and mmu_updating_count to atomic_t ? I see that not all accesses to these are under the kvm->mmu_lock spinlock. This will also remove the need for putting separate smp_wmb() and smp_rmb() memory barriers while accessing these structure members.

• gfn_t mmu_updating_range_start;
• gfn_t mmu_updating_range_end;

#endif struct list_head devices; u64 manual_dirty_log_protect; @@ -1362,8 +1362,8 @@ void kvm_mmu_free_memory_cache(struct kvm_mmu_memory_cache *mc); void *kvm_mmu_memory_cache_alloc(struct kvm_mmu_memory_cache *mc); #endif -void kvm_inc_notifier_count(struct kvm *kvm, gfn_t start, gfn_t end); -void kvm_dec_notifier_count(struct kvm *kvm, gfn_t start, gfn_t end); +void kvm_mmu_updating_begin(struct kvm *kvm, gfn_t start, gfn_t end); +void kvm_mmu_updating_end(struct kvm *kvm, gfn_t start, gfn_t end); long kvm_arch_dev_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg); @@ -1901,42 +1901,42 @@ extern const struct kvm_stats_header kvm_vcpu_stats_header; extern const struct _kvm_stats_desc kvm_vcpu_stats_desc[]; #if defined(CONFIG_MMU_NOTIFIER) && defined(KVM_ARCH_WANT_MMU_NOTIFIER) -static inline int mmu_notifier_retry(struct kvm *kvm, unsigned long mmu_seq) +static inline int mmu_updating_retry(struct kvm *kvm, unsigned long mmu_seq) {

• if (unlikely(kvm->mmu_notifier_count))

• if (unlikely(kvm->mmu_updating_count)) return 1; /*

• * Ensure the read of mmu_notifier_count happens before the read
  

• * of mmu_notifier_seq.  This interacts with the smp_wmb() in
  

• * Ensure the read of mmu_updating_count happens before the read
  

• * of mmu_updating_seq.  This interacts with the smp_wmb() in
  

  • mmu_notifier_invalidate_range_end to make sure that the caller

• * either sees the old (non-zero) value of mmu_notifier_count or
  

• * the new (incremented) value of mmu_notifier_seq.
  

• * either sees the old (non-zero) value of mmu_updating_count or
  

• * the new (incremented) value of mmu_updating_seq.
  

  • PowerPC Book3s HV KVM calls this under a per-page lock
  • rather than under kvm->mmu_lock, for scalability, so
  • can't rely on kvm->mmu_lock to keep things ordered.
  */ smp_rmb();

• if (kvm->mmu_notifier_seq != mmu_seq)

• if (kvm->mmu_updating_seq != mmu_seq) return 1; return 0;

} -static inline int mmu_notifier_retry_gfn(struct kvm *kvm, +static inline int mmu_updating_retry_gfn(struct kvm *kvm, unsigned long mmu_seq, gfn_t gfn) { lockdep_assert_held(&kvm->mmu_lock); /*

• * If mmu_notifier_count is non-zero, then the range maintained by
  

• * If mmu_updating_count is non-zero, then the range maintained by
  

  • kvm_mmu_notifier_invalidate_range_start contains all addresses that
  • might be being invalidated. Note that it may include some false
  • positives, due to shortcuts when handing concurrent invalidations.
  */

• if (unlikely(kvm->mmu_notifier_count) &&

•    gfn >= kvm->mmu_notifier_range_start &&
  

•    gfn < kvm->mmu_notifier_range_end)
  

• if (unlikely(kvm->mmu_updating_count) &&

•    gfn >= kvm->mmu_updating_range_start &&
  

•    gfn < kvm->mmu_updating_range_end)
  

  return 1;

• if (kvm->mmu_notifier_seq != mmu_seq)

• if (kvm->mmu_updating_seq != mmu_seq) return 1; return 0;

} diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 4d7f0e72366f..3ae4944b9f15 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -698,30 +698,29 @@ static void kvm_mmu_notifier_change_pte(struct mmu_notifier *mn, /* * .change_pte() must be surrounded by .invalidate_range_{start,end}().

• * If mmu_notifier_count is zero, then no in-progress invalidations,
  

• * If mmu_updating_count is zero, then no in-progress invalidations,
  

  • including this one, found a relevant memslot at start(); rechecking
  • memslots here is unnecessary. Note, a false positive (count elevated
  • by a different invalidation) is sub-optimal but functionally ok.
  */ WARN_ON_ONCE(!READ_ONCE(kvm->mn_active_invalidate_count));

• if (!READ_ONCE(kvm->mmu_notifier_count))

• if (!READ_ONCE(kvm->mmu_updating_count)) return;

kvm_handle_hva_range(mn, address, address + 1, pte, kvm_set_spte_gfn); } -void kvm_inc_notifier_count(struct kvm *kvm, unsigned long start,

• 		   unsigned long end)
  

+void kvm_mmu_updating_begin(struct kvm *kvm, gfn_t start, gfn_t end) { /* * The count increase must become visible at unlock time as no * spte can be established without taking the mmu_lock and * count is also read inside the mmu_lock critical section. */

• kvm->mmu_notifier_count++;
• if (likely(kvm->mmu_notifier_count == 1)) {

• kvm->mmu_notifier_range_start = start;
  

• kvm->mmu_notifier_range_end = end;
  

• kvm->mmu_updating_count++;
• if (likely(kvm->mmu_updating_count == 1)) {

• kvm->mmu_updating_range_start = start;
  

• kvm->mmu_updating_range_end = end;
  

  } else { /*
  • Fully tracking multiple concurrent ranges has diminishing

@@ -732,10 +731,10 @@ void kvm_inc_notifier_count(struct kvm *kvm, unsigned long start, * accumulate and persist until all outstanding invalidates * complete. */

• kvm->mmu_notifier_range_start =
  

• 	min(kvm->mmu_notifier_range_start, start);
  

• kvm->mmu_notifier_range_end =
  

• 	max(kvm->mmu_notifier_range_end, end);
  

• kvm->mmu_updating_range_start =
  

• 	min(kvm->mmu_updating_range_start, start);
  

• kvm->mmu_updating_range_end =
  

• 	max(kvm->mmu_updating_range_end, end);
  

  }

} @@ -748,7 +747,7 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, .end = range->end, .pte = __pte(0), .handler = kvm_unmap_gfn_range,

• .on_lock	= kvm_inc_notifier_count,
  

• .on_lock	= kvm_mmu_updating_begin,
  

  .on_unlock = kvm_arch_guest_memory_reclaimed, .flush_on_ret = true, .may_block = mmu_notifier_range_blockable(range),

@@ -759,7 +758,7 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, /* * Prevent memslot modification between range_start() and range_end() * so that conditionally locking provides the same result in both

• * functions.  Without that guarantee, the mmu_notifier_count
  

• * functions.  Without that guarantee, the mmu_updating_count
  

  • adjustments will be imbalanced.
  • Pairs with the decrement in range_end().

@@ -775,7 +774,7 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, * any given time, and the caches themselves can check for hva overlap, * i.e. don't need to rely on memslot overlap checks for performance. * Because this runs without holding mmu_lock, the pfn caches must use

• * mn_active_invalidate_count (see above) instead of mmu_notifier_count.
  

• * mn_active_invalidate_count (see above) instead of mmu_updating_count.
  

  */ gfn_to_pfn_cache_invalidate_start(kvm, range->start, range->end, hva_range.may_block);

@@ -785,22 +784,21 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, return 0; } -void kvm_dec_notifier_count(struct kvm *kvm, unsigned long start,

• 		   unsigned long end)
  

+void kvm_mmu_updating_end(struct kvm *kvm, gfn_t start, gfn_t end) { /* * This sequence increase will notify the kvm page fault that * the page that is going to be mapped in the spte could have * been freed. */

• kvm->mmu_notifier_seq++;

• kvm->mmu_updating_seq++; smp_wmb(); /*
  • The above sequence increase must be visible before the
  • below count decrease, which is ensured by the smp_wmb above

• * in conjunction with the smp_rmb in mmu_notifier_retry().
  

• * in conjunction with the smp_rmb in mmu_updating_retry().
  

  */

• kvm->mmu_notifier_count--;

• kvm->mmu_updating_count--;

} static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn, @@ -812,7 +810,7 @@ static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn, .end = range->end, .pte = __pte(0), .handler = (void *)kvm_null_fn,

• .on_lock	= kvm_dec_notifier_count,
  

• .on_lock	= kvm_mmu_updating_end,
  

  .on_unlock = (void *)kvm_null_fn, .flush_on_ret = false, .may_block = mmu_notifier_range_blockable(range),

@@ -833,7 +831,7 @@ static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn, if (wake) rcuwait_wake_up(&kvm->mn_memslots_update_rcuwait);

• BUG_ON(kvm->mmu_notifier_count < 0);

• BUG_ON(kvm->mmu_updating_count < 0);

} static int kvm_mmu_notifier_clear_flush_young(struct mmu_notifier *mn, diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c index ab519f72f2cd..aa6d24966a76 100644 --- a/virt/kvm/pfncache.c +++ b/virt/kvm/pfncache.c @@ -112,27 +112,27 @@ static inline bool mmu_notifier_retry_cache(struct kvm *kvm, unsigned long mmu_s { /* * mn_active_invalidate_count acts for all intents and purposes

• * like mmu_notifier_count here; but the latter cannot be used
  

• * like mmu_updating_count here; but the latter cannot be used
  

  • here because the invalidation of caches in the mmu_notifier

• * event occurs _before_ mmu_notifier_count is elevated.
  

• * event occurs _before_ mmu_updating_count is elevated.
  

  • Note, it does not matter that mn_active_invalidate_count
  • is not protected by gpc->lock. It is guaranteed to
  • be elevated before the mmu_notifier acquires gpc->lock, and

• * isn't dropped until after mmu_notifier_seq is updated.
  

• * isn't dropped until after mmu_updating_seq is updated.
  

  */ if (kvm->mn_active_invalidate_count) return true;

/* * Ensure mn_active_invalidate_count is read before

• * mmu_notifier_seq.  This pairs with the smp_wmb() in
  

• * mmu_updating_seq.  This pairs with the smp_wmb() in
  

  • mmu_notifier_invalidate_range_end() to guarantee either the
  • old (non-zero) value of mn_active_invalidate_count or the

• * new (incremented) value of mmu_notifier_seq is observed.
  

• * new (incremented) value of mmu_updating_seq is observed.
  

  */ smp_rmb();

• return kvm->mmu_notifier_seq != mmu_seq;

• return kvm->mmu_updating_seq != mmu_seq;

} static kvm_pfn_t hva_to_pfn_retry(struct kvm *kvm, struct gfn_to_pfn_cache *gpc) @@ -155,7 +155,7 @@ static kvm_pfn_t hva_to_pfn_retry(struct kvm *kvm, struct gfn_to_pfn_cache *gpc) gpc->valid = false; do {

• mmu_seq = kvm->mmu_notifier_seq;
  

• mmu_seq = kvm->mmu_updating_seq;
  

  smp_rmb();

write_unlock_irq(&gpc->lock); -- 2.25.1

On 2023-05-23 07:19:43, Sean Christopherson wrote:

On Tue, May 23, 2023, Kautuk Consul wrote:

...

On 2022-07-06 16:20:10, Chao Peng wrote:

...

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index e9153b54e2a4..c262ebb168a7 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -765,10 +765,10 @@ struct kvm { #if defined(CONFIG_MMU_NOTIFIER) && defined(KVM_ARCH_WANT_MMU_NOTIFIER) struct mmu_notifier mmu_notifier;

• unsigned long mmu_notifier_seq;
• long mmu_notifier_count;
• gfn_t mmu_notifier_range_start;
• gfn_t mmu_notifier_range_end;

• unsigned long mmu_updating_seq;
• long mmu_updating_count;

Can we convert mmu_updating_seq and mmu_updating_count to atomic_t ?

Heh, can we? Yes. Should we? No.

...

I see that not all accesses to these are under the kvm->mmu_lock spinlock.

Ya, working as intended. Ignoring gfn_to_pfn_cache for the moment, all accesses to mmu_invalidate_in_progress (was mmu_notifier_count / mmu_updating_count above) are done under mmu_lock. And for for mmu_notifier_seq (mmu_updating_seq above), all writes and some reads are done under mmu_lock. The only reads that are done outside of mmu_lock are the initial snapshots of the sequence number.

gfn_to_pfn_cache uses a different locking scheme, the comments in mmu_notifier_retry_cache() do a good job explaining the ordering.

...

This will also remove the need for putting separate smp_wmb() and smp_rmb() memory barriers while accessing these structure members.

No, the memory barriers aren't there to provide any kind of atomicity. The barriers exist to ensure that stores and loads to/from the sequence and invalidate in-progress counts are ordered relative to the invalidation (stores to counts) and creation (loads) of SPTEs. Making the counts atomic changes nothing because atomic operations don't guarantee the necessary ordering.

I'm not saying that the memory barriers provide atomicity. My comment was based on the assumption that "all atomic operations are implicit memory barriers". If that assumption is true then we won't need the memory barriers here if we use atomic operations for protecting these 2 structure members.

E.g. when handling a page fault, KVM snapshots the sequence outside of mmu_lock _before_ touching any state that is involved in resolving the host pfn, e.g. primary MMU state (VMAs, host page tables, etc.). After the page fault task acquires mmu_lock, KVM checks that there are no in-progress invalidations and that the sequence count is the same. This ensures that if there is a concurrent page fault and invalidation event, the page fault task will either acquire mmu_lock and create SPTEs _before_ the invalidation is processed, or the page fault task will observe either an elevated mmu_invalidate_in_progress or a different sequence count, and thus retry the page fault, if the page fault task acquires mmu_lock after the invalidation event.

On Wed, Jul 06, 2022, Chao Peng wrote:

@@ -1332,9 +1332,18 @@ yet and must be cleared on entry. __u64 userspace_addr; /* start of the userspace allocated memory */ };

• struct kvm_userspace_memory_region_ext {
• struct kvm_userspace_memory_region region;
• __u64 private_offset;
• __u32 private_fd;
• __u32 pad1;
• __u64 pad2[14];

+};

• /* for kvm_memory_region::flags */ #define KVM_MEM_LOG_DIRTY_PAGES (1UL << 0) #define KVM_MEM_READONLY (1UL << 1)
• #define KVM_MEM_PRIVATE (1UL << 2)

Very belatedly following up on prior feedback...

| I think a flag is still needed, the problem is private_fd can be safely | accessed only when this flag is set, e.g. without this flag, we can't | copy_from_user these new fields since they don't exist for previous | kvm_userspace_memory_region callers.

I forgot about that aspect of things. We don't technically need a dedicated PRIVATE flag to handle that, but it does seem to be the least awful soltuion. We could either add a generic KVM_MEM_EXTENDED_REGION or an entirely new ioctl(), e.g. KVM_SET_USER_MEMORY_REGION2, but in both approaches there's a decent chance that we'll end up needed individual "this field is valid" flags anways.

E.g. if KVM requires pad1 and pad2 to be zero to carve out future extensions, then we're right back here if some future extension needs to treat '0' as a legal input.

TL;DR: adding KVM_MEM_PRIVATE still seems like the best approach.

@@ -4631,14 +4658,35 @@ static long kvm_vm_ioctl(struct file *filp, break; } case KVM_SET_USER_MEMORY_REGION: {

• struct kvm_userspace_memory_region kvm_userspace_mem;
  

• struct kvm_user_mem_region mem;
  

• unsigned long size;
  

• u32 flags;
  

• kvm_sanity_check_user_mem_region_alias();
  

• memset(&mem, 0, sizeof(mem));
  

r = -EFAULT;

• if (copy_from_user(&kvm_userspace_mem, argp,
  

• 				sizeof(kvm_userspace_mem)))
  

• if (get_user(flags,
  

• 	(u32 __user *)(argp + offsetof(typeof(mem), flags))))
  

• 	goto out;
  

Indentation is funky. It's hard to massage this into something short and readable What about capturing the offset separately? E.g.

struct kvm_user_mem_region mem; unsigned int flags_offset = offsetof(typeof(mem), flags)); unsigned long size; u32 flags;

kvm_sanity_check_user_mem_region_alias();

memset(&mem, 0, sizeof(mem));

r = -EFAULT; if (get_user(flags, (u32 __user *)(argp + flags_offset))) goto out;

But this can actually be punted until KVM_MEM_PRIVATE is fully supported. As of this patch, KVM doesn't read the extended size, so I believe the diff for this patch can simply be:

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index da263c370d00..5194beb7b52f 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -4640,6 +4640,10 @@ static long kvm_vm_ioctl(struct file *filp, sizeof(kvm_userspace_mem))) goto out;

+ r = -EINVAL; + if (mem.flags & KVM_MEM_PRIVATE) + goto out; + r = kvm_vm_ioctl_set_memory_region(kvm, &kvm_userspace_mem); break; }

On Wed, Aug 03, 2022, Chao Peng wrote:

On Fri, Jul 29, 2022 at 07:51:29PM +0000, Sean Christopherson wrote:

...

On Wed, Jul 06, 2022, Chao Peng wrote:

...

@@ -1332,9 +1332,18 @@ yet and must be cleared on entry. __u64 userspace_addr; /* start of the userspace allocated memory */ };

• struct kvm_userspace_memory_region_ext {
• struct kvm_userspace_memory_region region;
• __u64 private_offset;
• __u32 private_fd;
• __u32 pad1;
• __u64 pad2[14];

+};

• /* for kvm_memory_region::flags */ #define KVM_MEM_LOG_DIRTY_PAGES (1UL << 0) #define KVM_MEM_READONLY (1UL << 1)
• #define KVM_MEM_PRIVATE (1UL << 2)

Very belatedly following up on prior feedback...

| I think a flag is still needed, the problem is private_fd can be safely | accessed only when this flag is set, e.g. without this flag, we can't | copy_from_user these new fields since they don't exist for previous | kvm_userspace_memory_region callers.

I forgot about that aspect of things. We don't technically need a dedicated PRIVATE flag to handle that, but it does seem to be the least awful soltuion. We could either add a generic KVM_MEM_EXTENDED_REGION or an entirely new ioctl(), e.g. KVM_SET_USER_MEMORY_REGION2, but in both approaches there's a decent chance that we'll end up needed individual "this field is valid" flags anways.

E.g. if KVM requires pad1 and pad2 to be zero to carve out future extensions, then we're right back here if some future extension needs to treat '0' as a legal input.

I had such practice (always rejecting none-zero 'pad' value when introducing new user APIs) in other project previously, but I rarely see that in KVM.

Ya, KVM often uses flags to indicate the validity of a field specifically so that KVM doesn't misinterpret a '0' from an older userspace as an intended value.

On Tue, Jul 19, 2022 at 10:00:23AM +0200, Gupta, Pankaj wrote:

...

...

+bool __weak kvm_arch_private_mem_supported(struct kvm *kvm) +{

• return false;

+}

Does this function has to be overriden by SEV and TDX to support the private regions?

Yes it should be overridden by architectures which want to support it.

...

• static int check_memory_region_flags(const struct kvm_user_mem_region *mem) { u32 valid_flags = KVM_MEM_LOG_DIRTY_PAGES;

@@ -4689,6 +4729,22 @@ static long kvm_vm_ioctl(struct file *filp, r = kvm_vm_ioctl_set_memory_region(kvm, &mem); break; } +#ifdef CONFIG_HAVE_KVM_PRIVATE_MEM

• case KVM_MEMORY_ENCRYPT_REG_REGION:
• case KVM_MEMORY_ENCRYPT_UNREG_REGION: {

• struct kvm_enc_region region;
  

• if (!kvm_arch_private_mem_supported(kvm))
  

• 	goto arch_vm_ioctl;
  

• r = -EFAULT;
  

• if (copy_from_user(&region, argp, sizeof(region)))
  

• 	goto out;
  

• r = kvm_vm_ioctl_set_encrypted_region(kvm, ioctl, &region);
  

this is to store private region metadata not only the encrypted region?

Correct.

Also, seems same ioctl can be used to put other regions (e.g firmware, later maybe DAX backend etc) into private memory?

Possibly. Depends on what exactly the semantics is. If just want to set those regions as private current code already support that.

Chao

...

• break;
  

• }

+#endif case KVM_GET_DIRTY_LOG: { struct kvm_dirty_log log; @@ -4842,6 +4898,7 @@ static long kvm_vm_ioctl(struct file *filp, r = kvm_vm_ioctl_get_stats_fd(kvm); break; default: +arch_vm_ioctl: r = kvm_arch_vm_ioctl(filp, ioctl, arg); } out:

On Tue, Jul 19, 2022 at 04:23:52PM +0200, Gupta, Pankaj wrote:

...

...

...

+bool __weak kvm_arch_private_mem_supported(struct kvm *kvm) +{

• return false;

+}

Does this function has to be overriden by SEV and TDX to support the private regions?

Yes it should be overridden by architectures which want to support it.

o.k

...

...

...

• static int check_memory_region_flags(const struct kvm_user_mem_region *mem) { u32 valid_flags = KVM_MEM_LOG_DIRTY_PAGES;

@@ -4689,6 +4729,22 @@ static long kvm_vm_ioctl(struct file *filp, r = kvm_vm_ioctl_set_memory_region(kvm, &mem); break; } +#ifdef CONFIG_HAVE_KVM_PRIVATE_MEM

• case KVM_MEMORY_ENCRYPT_REG_REGION:
• case KVM_MEMORY_ENCRYPT_UNREG_REGION: {

• struct kvm_enc_region region;
  

• if (!kvm_arch_private_mem_supported(kvm))
  

• 	goto arch_vm_ioctl;
  

• r = -EFAULT;
  

• if (copy_from_user(&region, argp, sizeof(region)))
  

• 	goto out;
  

• r = kvm_vm_ioctl_set_encrypted_region(kvm, ioctl, &region);
  

this is to store private region metadata not only the encrypted region?

Correct.

Sorry for not being clear, was suggesting name change of this function from: "kvm_vm_ioctl_set_encrypted_region" to "kvm_vm_ioctl_set_private_region"

Though I don't have strong reason to change it, I'm fine with this and this name matches the above kvm_arch_private_mem_supported perfectly.

Thanks, Chao

...

...

Also, seems same ioctl can be used to put other regions (e.g firmware, later maybe DAX backend etc) into private memory?

Possibly. Depends on what exactly the semantics is. If just want to set those regions as private current code already support that.

Agree. Sure!

Thanks, Pankaj

On Wed, Jul 06, 2022, Chao Peng wrote:

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 230c8ff9659c..bb714c2a4b06 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -914,6 +914,35 @@ static int kvm_init_mmu_notifier(struct kvm *kvm) #endif /* CONFIG_MMU_NOTIFIER && KVM_ARCH_WANT_MMU_NOTIFIER */ +#ifdef CONFIG_HAVE_KVM_PRIVATE_MEM +#define KVM_MEM_ATTR_PRIVATE 0x0001 +static int kvm_vm_ioctl_set_encrypted_region(struct kvm *kvm, unsigned int ioctl,

• 			     struct kvm_enc_region *region)
  

+{

• unsigned long start, end;

As alluded to in a different reply, because this will track GPAs instead of HVAs, the type needs to be "gpa_t", not "unsigned long". Oh, actually, they need to be gfn_t, since those are what gets shoved into the xarray.

• void *entry;
• int r;
• if (region->size == 0 || region->addr + region->size < region->addr)

• return -EINVAL;
  

• if (region->addr & (PAGE_SIZE - 1) || region->size & (PAGE_SIZE - 1))

• return -EINVAL;
  

• start = region->addr >> PAGE_SHIFT;
• end = (region->addr + region->size - 1) >> PAGE_SHIFT;
• entry = ioctl == KVM_MEMORY_ENCRYPT_REG_REGION ?

• 		xa_mk_value(KVM_MEM_ATTR_PRIVATE) : NULL;
  

• r = xa_err(xa_store_range(&kvm->mem_attr_array, start, end,

• 			entry, GFP_KERNEL_ACCOUNT));
  

IIUC, this series treats memory as shared by default. I think we should invert that and have KVM's ABI be that all guest memory as private by default, i.e. require the guest to opt into sharing memory instead of opt out of sharing memory.

And then the xarray would track which regions are shared.

Regarding mem_attr_array, it probably makes sense to explicitly include what it's tracking in the name, i.e. name it {private,shared}_mem_array depending on whether it's used to track private vs. shared memory. If we ever need to track metadata beyond shared/private then we can tweak the name as needed, e.g. if hardware ever supports secondary non-ephemeral encryption keys.

... diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 230c8ff9659c..bb714c2a4b06 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -914,6 +914,35 @@ static int kvm_init_mmu_notifier(struct kvm *kvm)

#endif /* CONFIG_MMU_NOTIFIER && KVM_ARCH_WANT_MMU_NOTIFIER */

+#ifdef CONFIG_HAVE_KVM_PRIVATE_MEM +#define KVM_MEM_ATTR_PRIVATE 0x0001 +static int kvm_vm_ioctl_set_encrypted_region(struct kvm *kvm, unsigned int ioctl,

•                                        struct kvm_enc_region *region)
  

+{

•   unsigned long start, end;
  

•   void *entry;
  

•   int r;
  

•   if (region->size == 0 || region->addr + region->size < region->addr)
  

•           return -EINVAL;
  

•   if (region->addr & (PAGE_SIZE - 1) || region->size & (PAGE_SIZE - 1))
  

•           return -EINVAL;
  

•   start = region->addr >> PAGE_SHIFT;
  

•   end = (region->addr + region->size - 1) >> PAGE_SHIFT;
  

•   entry = ioctl == KVM_MEMORY_ENCRYPT_REG_REGION ?
  

•                           xa_mk_value(KVM_MEM_ATTR_PRIVATE) : NULL;
  

•   r = xa_err(xa_store_range(&kvm->mem_attr_array, start, end,
  

•                                   entry, GFP_KERNEL_ACCOUNT));
  

xa_store_range seems to create multi-index entries by default. Subsequent xa_store_range call changes all the entries stored previously. xa_store needs to be used here instead of xa_store_range to achieve the intended behavior.

•   kvm_zap_gfn_range(kvm, start, end + 1);
  

•   return r;
  

+} +#endif /* CONFIG_HAVE_KVM_PRIVATE_MEM */

...

Hi Chao,

On Wed, Jul 6, 2022 at 9:27 AM Chao Peng chao.p.peng@linux.intel.com wrote:

If CONFIG_HAVE_KVM_PRIVATE_MEM=y, userspace can register/unregister the guest private memory regions through KVM_MEMORY_ENCRYPT_{UN,}REG_REGION ioctls. The patch reuses existing SEV ioctl but differs that the address in the region for private memory is gpa while SEV case it's hva.

The private memory region is stored as xarray in KVM for memory efficiency in normal usages and zapping existing memory mappings is also a side effect of these two ioctls.

Signed-off-by: Chao Peng chao.p.peng@linux.intel.com

Documentation/virt/kvm/api.rst | 17 +++++++--- arch/x86/include/asm/kvm_host.h | 1 + arch/x86/kvm/Kconfig | 1 + arch/x86/kvm/mmu.h | 2 -- include/linux/kvm_host.h | 8 +++++ virt/kvm/kvm_main.c | 57 +++++++++++++++++++++++++++++++++ 6 files changed, 80 insertions(+), 6 deletions(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst index 5ecfc7fbe0ee..dfb4caecab73 100644 --- a/Documentation/virt/kvm/api.rst +++ b/Documentation/virt/kvm/api.rst @@ -4715,10 +4715,19 @@ Documentation/virt/kvm/amd-memory-encryption.rst. This ioctl can be used to register a guest memory region which may contain encrypted data (e.g. guest RAM, SMRAM etc).

-It is used in the SEV-enabled guest. When encryption is enabled, a guest -memory region may contain encrypted data. The SEV memory encryption -engine uses a tweak such that two identical plaintext pages, each at -different locations will have differing ciphertexts. So swapping or +Currently this ioctl supports registering memory regions for two usages: +private memory and SEV-encrypted memory.

+When private memory is enabled, this ioctl is used to register guest private +memory region and the addr/size of kvm_enc_region represents guest physical +address (GPA). In this usage, this ioctl zaps the existing guest memory +mappings in KVM that fallen into the region.

+When SEV-encrypted memory is enabled, this ioctl is used to register guest +memory region which may contain encrypted data for a SEV-enabled guest. The +addr/size of kvm_enc_region represents userspace address (HVA). The SEV +memory encryption engine uses a tweak such that two identical plaintext pages, +each at different locations will have differing ciphertexts. So swapping or moving ciphertext of those pages will not result in plaintext being swapped. So relocating (or migrating) physical backing pages for the SEV guest will require some additional steps. diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index dae190e19fce..92120e3a224e 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -37,6 +37,7 @@ #include <asm/hyperv-tlfs.h>

#define __KVM_HAVE_ARCH_VCPU_DEBUGFS +#define __KVM_HAVE_ZAP_GFN_RANGE

#define KVM_MAX_VCPUS 1024

diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig index 1f160801e2a7..05861b9656a4 100644 --- a/arch/x86/kvm/Kconfig +++ b/arch/x86/kvm/Kconfig @@ -50,6 +50,7 @@ config KVM select HAVE_KVM_PM_NOTIFIER if PM select HAVE_KVM_PRIVATE_MEM if X86_64 select MEMFILE_NOTIFIER if HAVE_KVM_PRIVATE_MEM

•   select XARRAY_MULTI if HAVE_KVM_PRIVATE_MEM
    help
      Support hosting fully virtualized guest machines using hardware
      virtualization extensions.  You will need a fairly recent
  

diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h index a99acec925eb..428cd2e88cbd 100644 --- a/arch/x86/kvm/mmu.h +++ b/arch/x86/kvm/mmu.h @@ -209,8 +209,6 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, return -(u32)fault & errcode; }

-void kvm_zap_gfn_range(struct kvm *kvm, gfn_t gfn_start, gfn_t gfn_end);

int kvm_arch_write_log_dirty(struct kvm_vcpu *vcpu);

int kvm_mmu_post_init_vm(struct kvm *kvm); diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 1b203c8aa696..da33f8828456 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -260,6 +260,10 @@ bool kvm_test_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range); bool kvm_set_spte_gfn(struct kvm *kvm, struct kvm_gfn_range *range); #endif

+#ifdef __KVM_HAVE_ZAP_GFN_RANGE +void kvm_zap_gfn_range(struct kvm *kvm, gfn_t gfn_start, gfn_t gfn_end); +#endif

enum { OUTSIDE_GUEST_MODE, IN_GUEST_MODE, @@ -795,6 +799,9 @@ struct kvm { struct notifier_block pm_notifier; #endif char stats_id[KVM_STATS_NAME_SIZE]; +#ifdef CONFIG_HAVE_KVM_PRIVATE_MEM

•   struct xarray mem_attr_array;
  

+#endif };

#define kvm_err(fmt, ...) \ @@ -1459,6 +1466,7 @@ bool kvm_arch_dy_has_pending_interrupt(struct kvm_vcpu *vcpu); int kvm_arch_post_init_vm(struct kvm *kvm); void kvm_arch_pre_destroy_vm(struct kvm *kvm); int kvm_arch_create_vm_debugfs(struct kvm *kvm); +bool kvm_arch_private_mem_supported(struct kvm *kvm);

#ifndef __KVM_HAVE_ARCH_VM_ALLOC /* diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 230c8ff9659c..bb714c2a4b06 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -914,6 +914,35 @@ static int kvm_init_mmu_notifier(struct kvm *kvm)

#endif /* CONFIG_MMU_NOTIFIER && KVM_ARCH_WANT_MMU_NOTIFIER */

+#ifdef CONFIG_HAVE_KVM_PRIVATE_MEM +#define KVM_MEM_ATTR_PRIVATE 0x0001 +static int kvm_vm_ioctl_set_encrypted_region(struct kvm *kvm, unsigned int ioctl,

•                                        struct kvm_enc_region *region)
  

+{

•   unsigned long start, end;
  

•   void *entry;
  

•   int r;
  

•   if (region->size == 0 || region->addr + region->size < region->addr)
  

•           return -EINVAL;
  

•   if (region->addr & (PAGE_SIZE - 1) || region->size & (PAGE_SIZE - 1))
  

•           return -EINVAL;
  

•   start = region->addr >> PAGE_SHIFT;
  

•   end = (region->addr + region->size - 1) >> PAGE_SHIFT;
  

•   entry = ioctl == KVM_MEMORY_ENCRYPT_REG_REGION ?
  

•                           xa_mk_value(KVM_MEM_ATTR_PRIVATE) : NULL;
  

•   r = xa_err(xa_store_range(&kvm->mem_attr_array, start, end,
  

•                                   entry, GFP_KERNEL_ACCOUNT));
  

•   kvm_zap_gfn_range(kvm, start, end + 1);
  

•   return r;
  

+} +#endif /* CONFIG_HAVE_KVM_PRIVATE_MEM */

#ifdef CONFIG_HAVE_KVM_PM_NOTIFIER static int kvm_pm_notifier_call(struct notifier_block *bl, unsigned long state, @@ -1138,6 +1167,9 @@ static struct kvm *kvm_create_vm(unsigned long type) spin_lock_init(&kvm->mn_invalidate_lock); rcuwait_init(&kvm->mn_memslots_update_rcuwait); xa_init(&kvm->vcpu_array); +#ifdef CONFIG_HAVE_KVM_PRIVATE_MEM

•   xa_init(&kvm->mem_attr_array);
  

+#endif

    INIT_LIST_HEAD(&kvm->gpc_list);
    spin_lock_init(&kvm->gpc_lock);

@@ -1305,6 +1337,9 @@ static void kvm_destroy_vm(struct kvm *kvm) kvm_free_memslots(kvm, &kvm->__memslots[i][0]); kvm_free_memslots(kvm, &kvm->__memslots[i][1]); } +#ifdef CONFIG_HAVE_KVM_PRIVATE_MEM

•   xa_destroy(&kvm->mem_attr_array);
  

+#endif cleanup_srcu_struct(&kvm->irq_srcu); cleanup_srcu_struct(&kvm->srcu); kvm_arch_free_vm(kvm); @@ -1508,6 +1543,11 @@ static void kvm_replace_memslot(struct kvm *kvm, } }

+bool __weak kvm_arch_private_mem_supported(struct kvm *kvm) +{

•   return false;
  

+}

static int check_memory_region_flags(const struct kvm_user_mem_region *mem) { u32 valid_flags = KVM_MEM_LOG_DIRTY_PAGES; @@ -4689,6 +4729,22 @@ static long kvm_vm_ioctl(struct file *filp, r = kvm_vm_ioctl_set_memory_region(kvm, &mem); break; } +#ifdef CONFIG_HAVE_KVM_PRIVATE_MEM

•   case KVM_MEMORY_ENCRYPT_REG_REGION:
  

•   case KVM_MEMORY_ENCRYPT_UNREG_REGION: {
  

•           struct kvm_enc_region region;
  

•           if (!kvm_arch_private_mem_supported(kvm))
  

•                   goto arch_vm_ioctl;
  

•           r = -EFAULT;
  

•           if (copy_from_user(&region, argp, sizeof(region)))
  

•                   goto out;
  

•           r = kvm_vm_ioctl_set_encrypted_region(kvm, ioctl, &region);
  

•           break;
  

•   }
  

+#endif case KVM_GET_DIRTY_LOG: { struct kvm_dirty_log log;

@@ -4842,6 +4898,7 @@ static long kvm_vm_ioctl(struct file *filp, r = kvm_vm_ioctl_get_stats_fd(kvm); break; default: +arch_vm_ioctl:

It might be good to make this label conditional on CONFIG_HAVE_KVM_PRIVATE_MEM, otherwise you get a warning if CONFIG_HAVE_KVM_PRIVATE_MEM isn't defined.

+#ifdef CONFIG_HAVE_KVM_PRIVATE_MEM arch_vm_ioctl: +#endif

Cheers, /fuad

            r = kvm_arch_vm_ioctl(filp, ioctl, arg);
    }

out:

2.25.1

On Fri, Jul 29, 2022 at 08:58:41PM +0000, Sean Christopherson wrote:

On Wed, Jul 06, 2022, Chao Peng wrote:

...

A page fault can carry the private/shared information for KVM_MEM_PRIVATE memslot, this can be filled by architecture code(like TDX code). To handle page fault for such access, KVM maps the page only when this private property matches the host's view on the page.

For a successful match, private pfn is obtained with memfile_notifier callbacks from private fd and shared pfn is obtained with existing get_user_pages.

For a failed match, KVM causes a KVM_EXIT_MEMORY_FAULT exit to userspace. Userspace then can convert memory between private/shared from host's view then retry the access.

Co-developed-by: Yu Zhang yu.c.zhang@linux.intel.com Signed-off-by: Yu Zhang yu.c.zhang@linux.intel.com Signed-off-by: Chao Peng chao.p.peng@linux.intel.com

---

arch/x86/kvm/mmu/mmu.c | 60 ++++++++++++++++++++++++++++++++- arch/x86/kvm/mmu/mmu_internal.h | 18 ++++++++++ arch/x86/kvm/mmu/mmutrace.h | 1 + include/linux/kvm_host.h | 35 ++++++++++++++++++- 4 files changed, 112 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 545eb74305fe..27dbdd4fe8d1 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -3004,6 +3004,9 @@ int kvm_mmu_max_mapping_level(struct kvm *kvm, if (max_level == PG_LEVEL_4K) return PG_LEVEL_4K;

• if (kvm_mem_is_private(kvm, gfn))

• return max_level;
  

• host_level = host_pfn_mapping_level(kvm, gfn, pfn, slot); return min(host_level, max_level);

} @@ -4101,10 +4104,52 @@ void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu, struct kvm_async_pf *work) kvm_mmu_do_page_fault(vcpu, work->cr2_or_gpa, 0, true); } +static inline u8 order_to_level(int order) +{

• enum pg_level level;
• for (level = KVM_MAX_HUGEPAGE_LEVEL; level > PG_LEVEL_4K; level--)

Curly braces needed for the for-loop.

And I think it makes sense to take in the fault->max_level, that way this is slightly more performant when the guest mapping is smaller than the host, e.g.

for (level = max_level; level > PG_LEVEL_4K; level--) ...

return level;

Though I think I'd vote to avoid a loop entirely and do:

BUILD_BUG_ON(KVM_MAX_HUGEPAGE_LEVEL > PG_LEVEL_1G);

if (order > ???) return PG_LEVEL_1G; if (order > ???) return PG_LEVEL_2M;

return PG_LEVEL_4K;

Sounds good.

...

• if (order >= page_level_shift(level) - PAGE_SHIFT)
  

• 	return level;
  

• return level;

+}

+static int kvm_faultin_pfn_private(struct kvm_vcpu *vcpu,

• 		   struct kvm_page_fault *fault)
  

+{

• int order;
• struct kvm_memory_slot *slot = fault->slot;
• bool private_exist = kvm_mem_is_private(vcpu->kvm, fault->gfn);
• if (fault->is_private != private_exist) {

• vcpu->run->exit_reason = KVM_EXIT_MEMORY_FAULT;
  

• if (fault->is_private)
  

• 	vcpu->run->memory.flags = KVM_MEMORY_EXIT_FLAG_PRIVATE;
  

• else
  

• 	vcpu->run->memory.flags = 0;
  

• vcpu->run->memory.padding = 0;
  

• vcpu->run->memory.gpa = fault->gfn << PAGE_SHIFT;
  

• vcpu->run->memory.size = PAGE_SIZE;
  

• return RET_PF_USER;
  

• }
• if (fault->is_private) {

• if (kvm_private_mem_get_pfn(slot, fault->gfn, &fault->pfn, &order))
  

• 	return RET_PF_RETRY;
  

• fault->max_level = min(order_to_level(order), fault->max_level);
  

• fault->map_writable = !(slot->flags & KVM_MEM_READONLY);
  

• return RET_PF_FIXED;
  

• }
• /* Fault is shared, fallthrough. */
• return RET_PF_CONTINUE;

+}

static int kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) { struct kvm_memory_slot *slot = fault->slot; bool async;

• int r;

/* * Retry the page fault if the gfn hit a memslot that is being deleted @@ -4133,6 +4178,12 @@ static int kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) return RET_PF_EMULATE; }

• if (kvm_slot_can_be_private(slot)) {

• r = kvm_faultin_pfn_private(vcpu, fault);
  

• if (r != RET_PF_CONTINUE)
  

• 	return r == RET_PF_FIXED ? RET_PF_CONTINUE : r;
  

I apologize if I've given you conflicting feedback in the past. Now that this returns RET_PF_* directly, I definitely think it makes sense to do:

if (kvm_slot_can_be_private(slot) && fault->is_private != kvm_mem_is_private(vcpu->kvm, fault->gfn)) { vcpu->run->exit_reason = KVM_EXIT_MEMORY_FAULT; if (fault->is_private) vcpu->run->memory.flags = KVM_MEMORY_EXIT_FLAG_PRIVATE; else vcpu->run->memory.flags = 0; vcpu->run->memory.padding = 0; vcpu->run->memory.gpa = fault->gfn << PAGE_SHIFT; vcpu->run->memory.size = PAGE_SIZE; return RET_PF_USER; }

if (fault->is_private) return kvm_faultin_pfn_private(vcpu, fault);

That way kvm_faultin_pfn_private() only handles private faults, and this doesn't need to play games with RET_PF_FIXED.