[PATCH 0/6] KVM: kvm_set_memory_region() cleanups

[PATCH 0/6] KVM: kvm_set_memory_region() cleanups

[Sean Christopherson]

Cleanups related to kvm_set_memory_region(), salvaged from similar patches
that were flying around when we were sorting out KVM_SET_USER_MEMORY_REGION2.

I'm honestly 50/50 on whether this is all worthwhile, but in the end, I
decided I like having kvm_set_internal_memslot().  I think.

Sean Christopherson (6):
  KVM: Open code kvm_set_memory_region() into its sole caller (ioctl()
    API)
  KVM: Assert slots_lock is held in  __kvm_set_memory_region()
  KVM: Add a dedicated API for setting KVM-internal memslots
  KVM: x86: Drop double-underscores from __kvm_set_memory_region()
  KVM: Disallow all flags for KVM-internal memslots
  KVM: Move flags check for user memory regions to the ioctl() specific
    API

 arch/x86/kvm/x86.c       |  4 +-
 include/linux/kvm_host.h |  8 ++--
 virt/kvm/kvm_main.c      | 87 +++++++++++++++++-----------------------
 3 files changed, 41 insertions(+), 58 deletions(-)


base-commit: 332d2c1d713e232e163386c35a3ba0c1b90df83f
-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH 1/6] KVM: Open code kvm_set_memory_region() into its sole caller (ioctl() API)

[Sean Christopherson]

Open code kvm_set_memory_region() into its sole caller in preparation for
adding a dedicated API for setting internal memslots.

Oppurtunistically use the fancy new guard(mutex) to avoid a local 'r'
variable.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 include/linux/kvm_host.h |  2 --
 virt/kvm/kvm_main.c      | 15 ++-------------
 2 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 689e8be873a7..b341d00aae37 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1179,8 +1179,6 @@ enum kvm_mr_change {
 	KVM_MR_FLAGS_ONLY,
 };
 
-int kvm_set_memory_region(struct kvm *kvm,
-			  const struct kvm_userspace_memory_region2 *mem);
 int __kvm_set_memory_region(struct kvm *kvm,
 			    const struct kvm_userspace_memory_region2 *mem);
 void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index d0788d0a72cc..0557d663b69b 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2105,25 +2105,14 @@ int __kvm_set_memory_region(struct kvm *kvm,
 }
 EXPORT_SYMBOL_GPL(__kvm_set_memory_region);
 
-int kvm_set_memory_region(struct kvm *kvm,
-			  const struct kvm_userspace_memory_region2 *mem)
-{
-	int r;
-
-	mutex_lock(&kvm->slots_lock);
-	r = __kvm_set_memory_region(kvm, mem);
-	mutex_unlock(&kvm->slots_lock);
-	return r;
-}
-EXPORT_SYMBOL_GPL(kvm_set_memory_region);
-
 static int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
 					  struct kvm_userspace_memory_region2 *mem)
 {
 	if ((u16)mem->slot >= KVM_USER_MEM_SLOTS)
 		return -EINVAL;
 
-	return kvm_set_memory_region(kvm, mem);
+	guard(mutex)(&kvm->slots_lock);
+	return  __kvm_set_memory_region(kvm, mem);
 }
 
 #ifndef CONFIG_KVM_GENERIC_DIRTYLOG_READ_PROTECT
-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH 2/6] KVM: Assert slots_lock is held in __kvm_set_memory_region()

[Sean Christopherson]

Add a proper lockdep assertion in __kvm_set_memory_region() instead of
relying on a function comment.  Opportunistically delete the entire
function comment as the API doesn't allocate memory or select a gfn,
and the "mostly for framebuffers" comment hasn't been true for a very long
time.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/kvm_main.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 0557d663b69b..f202bdbfca9e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1973,14 +1973,6 @@ static bool kvm_check_memslot_overlap(struct kvm_memslots *slots, int id,
 	return false;
 }
 
-/*
- * Allocate some memory and give it an address in the guest physical address
- * space.
- *
- * Discontiguous memory is allowed, mostly for framebuffers.
- *
- * Must be called holding kvm->slots_lock for write.
- */
 int __kvm_set_memory_region(struct kvm *kvm,
 			    const struct kvm_userspace_memory_region2 *mem)
 {
@@ -1992,6 +1984,8 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	int as_id, id;
 	int r;
 
+	lockdep_assert_held(&kvm->slots_lock);
+
 	r = check_memory_region_flags(kvm, mem);
 	if (r)
 		return r;
-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH 3/6] KVM: Add a dedicated API for setting KVM-internal memslots

[Sean Christopherson]

Add a dedicated API for setting internal memslots, and have it explicitly
disallow setting userspace memslots.  Setting a userspace memslots without
a direct command from userspace would result in all manner of issues.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c       |  2 +-
 include/linux/kvm_host.h |  4 ++--
 virt/kvm/kvm_main.c      | 15 ++++++++++++---
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index af6c8cf6a37a..77949fee13f7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12794,7 +12794,7 @@ void __user * __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa,
 		m.guest_phys_addr = gpa;
 		m.userspace_addr = hva;
 		m.memory_size = size;
-		r = __kvm_set_memory_region(kvm, &m);
+		r = kvm_set_internal_memslot(kvm, &m);
 		if (r < 0)
 			return ERR_PTR_USR(r);
 	}
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index b341d00aae37..cefa274c0852 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1179,8 +1179,8 @@ enum kvm_mr_change {
 	KVM_MR_FLAGS_ONLY,
 };
 
-int __kvm_set_memory_region(struct kvm *kvm,
-			    const struct kvm_userspace_memory_region2 *mem);
+int kvm_set_internal_memslot(struct kvm *kvm,
+			     const struct kvm_userspace_memory_region2 *mem);
 void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot);
 void kvm_arch_memslots_updated(struct kvm *kvm, u64 gen);
 int kvm_arch_prepare_memory_region(struct kvm *kvm,
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index f202bdbfca9e..63b43644ed9f 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1973,8 +1973,8 @@ static bool kvm_check_memslot_overlap(struct kvm_memslots *slots, int id,
 	return false;
 }
 
-int __kvm_set_memory_region(struct kvm *kvm,
-			    const struct kvm_userspace_memory_region2 *mem)
+static int __kvm_set_memory_region(struct kvm *kvm,
+				   const struct kvm_userspace_memory_region2 *mem)
 {
 	struct kvm_memory_slot *old, *new;
 	struct kvm_memslots *slots;
@@ -2097,7 +2097,16 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	kfree(new);
 	return r;
 }
-EXPORT_SYMBOL_GPL(__kvm_set_memory_region);
+
+int kvm_set_internal_memslot(struct kvm *kvm,
+			     const struct kvm_userspace_memory_region2 *mem)
+{
+	if (WARN_ON_ONCE(mem->slot < KVM_USER_MEM_SLOTS))
+		return -EINVAL;
+
+	return  __kvm_set_memory_region(kvm, mem);
+}
+EXPORT_SYMBOL_GPL(kvm_set_internal_memslot);
 
 static int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
 					  struct kvm_userspace_memory_region2 *mem)
-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH 4/6] KVM: x86: Drop double-underscores from __kvm_set_memory_region()

[Sean Christopherson]

Now that there's no outer wrapper for __kvm_set_memory_region() and it's
static, drop its double-underscore prefix.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c       | 2 +-
 include/linux/kvm_host.h | 2 +-
 virt/kvm/kvm_main.c      | 8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 77949fee13f7..bd365fb8ab6e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12895,7 +12895,7 @@ static int kvm_alloc_memslot_metadata(struct kvm *kvm,
 
 	/*
 	 * Clear out the previous array pointers for the KVM_MR_MOVE case.  The
-	 * old arrays will be freed by __kvm_set_memory_region() if installing
+	 * old arrays will be freed by kvm_set_memory_region() if installing
 	 * the new memslot is successful.
 	 */
 	memset(&slot->arch, 0, sizeof(slot->arch));
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index cefa274c0852..b5c048858fc4 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1170,7 +1170,7 @@ static inline bool kvm_memslot_iter_is_valid(struct kvm_memslot_iter *iter, gfn_
  *   -- just change its flags
  *
  * Since flags can be changed by some of these operations, the following
- * differentiation is the best we can do for __kvm_set_memory_region():
+ * differentiation is the best we can do for kvm_set_memory_region():
  */
 enum kvm_mr_change {
 	KVM_MR_CREATE,
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 63b43644ed9f..42ec817d6a7e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1973,8 +1973,8 @@ static bool kvm_check_memslot_overlap(struct kvm_memslots *slots, int id,
 	return false;
 }
 
-static int __kvm_set_memory_region(struct kvm *kvm,
-				   const struct kvm_userspace_memory_region2 *mem)
+static int kvm_set_memory_region(struct kvm *kvm,
+				 const struct kvm_userspace_memory_region2 *mem)
 {
 	struct kvm_memory_slot *old, *new;
 	struct kvm_memslots *slots;
@@ -2104,7 +2104,7 @@ int kvm_set_internal_memslot(struct kvm *kvm,
 	if (WARN_ON_ONCE(mem->slot < KVM_USER_MEM_SLOTS))
 		return -EINVAL;
 
-	return  __kvm_set_memory_region(kvm, mem);
+	return  kvm_set_memory_region(kvm, mem);
 }
 EXPORT_SYMBOL_GPL(kvm_set_internal_memslot);
 
@@ -2115,7 +2115,7 @@ static int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
 		return -EINVAL;
 
 	guard(mutex)(&kvm->slots_lock);
-	return  __kvm_set_memory_region(kvm, mem);
+	return  kvm_set_memory_region(kvm, mem);
 }
 
 #ifndef CONFIG_KVM_GENERIC_DIRTYLOG_READ_PROTECT
-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH 5/6] KVM: Disallow all flags for KVM-internal memslots

[Sean Christopherson]

Disallow all flags for KVM-internal memslots as all existing flags require
some amount of userspace interaction to have any meaning.  This will allow
moving the flags checking from __kvm_set_memory_region() to
kvm_vm_ioctl_set_memory_region() without creating a hole where a KVM bug
could silently succeed and create a bogus memslot.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/kvm_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 42ec817d6a7e..84fcb20e3e1c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2104,6 +2104,9 @@ int kvm_set_internal_memslot(struct kvm *kvm,
 	if (WARN_ON_ONCE(mem->slot < KVM_USER_MEM_SLOTS))
 		return -EINVAL;
 
+	if (WARN_ON_ONCE(mem->flags))
+		return -EINVAL;
+
 	return  kvm_set_memory_region(kvm, mem);
 }
 EXPORT_SYMBOL_GPL(kvm_set_internal_memslot);
-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH 6/6] KVM: Move flags check for user memory regions to the ioctl() specific API

[Sean Christopherson]

Move the check on memory region flags to kvm_vm_ioctl_set_memory_region()
now that the internal API, kvm_set_internal_memslot(), disallows any and
all flags.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/kvm_main.c | 54 ++++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 32 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 84fcb20e3e1c..09cc261b080a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1566,34 +1566,6 @@ static void kvm_replace_memslot(struct kvm *kvm,
 #define KVM_SET_USER_MEMORY_REGION_V1_FLAGS \
 	(KVM_MEM_LOG_DIRTY_PAGES | KVM_MEM_READONLY)
 
-static int check_memory_region_flags(struct kvm *kvm,
-				     const struct kvm_userspace_memory_region2 *mem)
-{
-	u32 valid_flags = KVM_MEM_LOG_DIRTY_PAGES;
-
-	if (kvm_arch_has_private_mem(kvm))
-		valid_flags |= KVM_MEM_GUEST_MEMFD;
-
-	/* Dirty logging private memory is not currently supported. */
-	if (mem->flags & KVM_MEM_GUEST_MEMFD)
-		valid_flags &= ~KVM_MEM_LOG_DIRTY_PAGES;
-
-#ifdef CONFIG_HAVE_KVM_READONLY_MEM
-	/*
-	 * GUEST_MEMFD is incompatible with read-only memslots, as writes to
-	 * read-only memslots have emulated MMIO, not page fault, semantics,
-	 * and KVM doesn't allow emulated MMIO for private memory.
-	 */
-	if (!(mem->flags & KVM_MEM_GUEST_MEMFD))
-		valid_flags |= KVM_MEM_READONLY;
-#endif
-
-	if (mem->flags & ~valid_flags)
-		return -EINVAL;
-
-	return 0;
-}
-
 static void kvm_swap_active_memslots(struct kvm *kvm, int as_id)
 {
 	struct kvm_memslots *slots = kvm_get_inactive_memslots(kvm, as_id);
@@ -1986,10 +1958,6 @@ static int kvm_set_memory_region(struct kvm *kvm,
 
 	lockdep_assert_held(&kvm->slots_lock);
 
-	r = check_memory_region_flags(kvm, mem);
-	if (r)
-		return r;
-
 	as_id = mem->slot >> 16;
 	id = (u16)mem->slot;
 
@@ -2114,6 +2082,28 @@ EXPORT_SYMBOL_GPL(kvm_set_internal_memslot);
 static int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
 					  struct kvm_userspace_memory_region2 *mem)
 {
+	u32 valid_flags = KVM_MEM_LOG_DIRTY_PAGES;
+
+	if (kvm_arch_has_private_mem(kvm))
+		valid_flags |= KVM_MEM_GUEST_MEMFD;
+
+	/* Dirty logging private memory is not currently supported. */
+	if (mem->flags & KVM_MEM_GUEST_MEMFD)
+		valid_flags &= ~KVM_MEM_LOG_DIRTY_PAGES;
+
+#ifdef CONFIG_HAVE_KVM_READONLY_MEM
+	/*
+	 * GUEST_MEMFD is incompatible with read-only memslots, as writes to
+	 * read-only memslots have emulated MMIO, not page fault, semantics,
+	 * and KVM doesn't allow emulated MMIO for private memory.
+	 */
+	if (!(mem->flags & KVM_MEM_GUEST_MEMFD))
+		valid_flags |= KVM_MEM_READONLY;
+#endif
+
+	if (mem->flags & ~valid_flags)
+		return -EINVAL;
+
 	if ((u16)mem->slot >= KVM_USER_MEM_SLOTS)
 		return -EINVAL;
 
-- 
2.46.0.rc2.264.g509ed76dc8-goog

Re: [PATCH 1/6] KVM: Open code kvm_set_memory_region() into its sole caller (ioctl() API)

[Tao Su]

On Fri, Aug 02, 2024 at 01:49:58PM -0700, Sean Christopherson wrote:
> Open code kvm_set_memory_region() into its sole caller in preparation for
> adding a dedicated API for setting internal memslots.
> 
> Oppurtunistically use the fancy new guard(mutex) to avoid a local 'r'
> variable.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  include/linux/kvm_host.h |  2 --
>  virt/kvm/kvm_main.c      | 15 ++-------------
>  2 files changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 689e8be873a7..b341d00aae37 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1179,8 +1179,6 @@ enum kvm_mr_change {
>  	KVM_MR_FLAGS_ONLY,
>  };
>  
> -int kvm_set_memory_region(struct kvm *kvm,
> -			  const struct kvm_userspace_memory_region2 *mem);
>  int __kvm_set_memory_region(struct kvm *kvm,
>  			    const struct kvm_userspace_memory_region2 *mem);
>  void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot);
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index d0788d0a72cc..0557d663b69b 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2105,25 +2105,14 @@ int __kvm_set_memory_region(struct kvm *kvm,
>  }
>  EXPORT_SYMBOL_GPL(__kvm_set_memory_region);
>  
> -int kvm_set_memory_region(struct kvm *kvm,
> -			  const struct kvm_userspace_memory_region2 *mem)
> -{
> -	int r;
> -
> -	mutex_lock(&kvm->slots_lock);
> -	r = __kvm_set_memory_region(kvm, mem);
> -	mutex_unlock(&kvm->slots_lock);
> -	return r;
> -}
> -EXPORT_SYMBOL_GPL(kvm_set_memory_region);
> -
>  static int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
>  					  struct kvm_userspace_memory_region2 *mem)
>  {
>  	if ((u16)mem->slot >= KVM_USER_MEM_SLOTS)
>  		return -EINVAL;
>  
> -	return kvm_set_memory_region(kvm, mem);
> +	guard(mutex)(&kvm->slots_lock);
> +	return  __kvm_set_memory_region(kvm, mem);
              ^^
Two spaces are introduced here.

>  }
>  
>  #ifndef CONFIG_KVM_GENERIC_DIRTYLOG_READ_PROTECT
> -- 
> 2.46.0.rc2.264.g509ed76dc8-goog
> 
> 

Re: [PATCH 2/6] KVM: Assert slots_lock is held in __kvm_set_memory_region()

[Tao Su]

On Fri, Aug 02, 2024 at 01:49:59PM -0700, Sean Christopherson wrote:
> Add a proper lockdep assertion in __kvm_set_memory_region() instead of
> relying on a function comment.  Opportunistically delete the entire
> function comment as the API doesn't allocate memory or select a gfn,
> and the "mostly for framebuffers" comment hasn't been true for a very long
> time.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  virt/kvm/kvm_main.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 0557d663b69b..f202bdbfca9e 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1973,14 +1973,6 @@ static bool kvm_check_memslot_overlap(struct kvm_memslots *slots, int id,
>  	return false;
>  }
>  
> -/*
> - * Allocate some memory and give it an address in the guest physical address
> - * space.
> - *
> - * Discontiguous memory is allowed, mostly for framebuffers.
> - *
> - * Must be called holding kvm->slots_lock for write.
> - */
>  int __kvm_set_memory_region(struct kvm *kvm,
>  			    const struct kvm_userspace_memory_region2 *mem)
>  {
> @@ -1992,6 +1984,8 @@ int __kvm_set_memory_region(struct kvm *kvm,
>  	int as_id, id;
>  	int r;
>  
> +	lockdep_assert_held(&kvm->slots_lock);

How about adding this lockdep assertion in __x86_set_memory_region() to replace
this comment "/* Called with kvm->slots_lock held.  */" as well?

> +
>  	r = check_memory_region_flags(kvm, mem);
>  	if (r)
>  		return r;
> -- 
> 2.46.0.rc2.264.g509ed76dc8-goog
> 
> 

Re: [PATCH 3/6] KVM: Add a dedicated API for setting KVM-internal memslots

[Tao Su]

On Fri, Aug 02, 2024 at 01:50:00PM -0700, Sean Christopherson wrote:
> Add a dedicated API for setting internal memslots, and have it explicitly
> disallow setting userspace memslots.  Setting a userspace memslots without
> a direct command from userspace would result in all manner of issues.
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/x86.c       |  2 +-
>  include/linux/kvm_host.h |  4 ++--
>  virt/kvm/kvm_main.c      | 15 ++++++++++++---
>  3 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index af6c8cf6a37a..77949fee13f7 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -12794,7 +12794,7 @@ void __user * __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa,
>  		m.guest_phys_addr = gpa;
>  		m.userspace_addr = hva;
>  		m.memory_size = size;
> -		r = __kvm_set_memory_region(kvm, &m);
> +		r = kvm_set_internal_memslot(kvm, &m);
>  		if (r < 0)
>  			return ERR_PTR_USR(r);
>  	}
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index b341d00aae37..cefa274c0852 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1179,8 +1179,8 @@ enum kvm_mr_change {
>  	KVM_MR_FLAGS_ONLY,
>  };
>  
> -int __kvm_set_memory_region(struct kvm *kvm,
> -			    const struct kvm_userspace_memory_region2 *mem);
> +int kvm_set_internal_memslot(struct kvm *kvm,
> +			     const struct kvm_userspace_memory_region2 *mem);
>  void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot);
>  void kvm_arch_memslots_updated(struct kvm *kvm, u64 gen);
>  int kvm_arch_prepare_memory_region(struct kvm *kvm,
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index f202bdbfca9e..63b43644ed9f 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1973,8 +1973,8 @@ static bool kvm_check_memslot_overlap(struct kvm_memslots *slots, int id,
>  	return false;
>  }
>  
> -int __kvm_set_memory_region(struct kvm *kvm,
> -			    const struct kvm_userspace_memory_region2 *mem)
> +static int __kvm_set_memory_region(struct kvm *kvm,
> +				   const struct kvm_userspace_memory_region2 *mem)
>  {
>  	struct kvm_memory_slot *old, *new;
>  	struct kvm_memslots *slots;
> @@ -2097,7 +2097,16 @@ int __kvm_set_memory_region(struct kvm *kvm,
>  	kfree(new);
>  	return r;
>  }
> -EXPORT_SYMBOL_GPL(__kvm_set_memory_region);
> +
> +int kvm_set_internal_memslot(struct kvm *kvm,
> +			     const struct kvm_userspace_memory_region2 *mem)
> +{
> +	if (WARN_ON_ONCE(mem->slot < KVM_USER_MEM_SLOTS))
> +		return -EINVAL;
> +
> +	return  __kvm_set_memory_region(kvm, mem);
              ^^
Two spaces are introduced here.

> +}
> +EXPORT_SYMBOL_GPL(kvm_set_internal_memslot);
>  
>  static int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
>  					  struct kvm_userspace_memory_region2 *mem)
> -- 
> 2.46.0.rc2.264.g509ed76dc8-goog
> 
> 

Re: [PATCH 2/6] KVM: Assert slots_lock is held in __kvm_set_memory_region()

[Sean Christopherson]

On Mon, Aug 05, 2024, Tao Su wrote:
> On Fri, Aug 02, 2024 at 01:49:59PM -0700, Sean Christopherson wrote:
> > Add a proper lockdep assertion in __kvm_set_memory_region() instead of
> > relying on a function comment.  Opportunistically delete the entire
> > function comment as the API doesn't allocate memory or select a gfn,
> > and the "mostly for framebuffers" comment hasn't been true for a very long
> > time.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  virt/kvm/kvm_main.c | 10 ++--------
> >  1 file changed, 2 insertions(+), 8 deletions(-)
> > 
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 0557d663b69b..f202bdbfca9e 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -1973,14 +1973,6 @@ static bool kvm_check_memslot_overlap(struct kvm_memslots *slots, int id,
> >  	return false;
> >  }
> >  
> > -/*
> > - * Allocate some memory and give it an address in the guest physical address
> > - * space.
> > - *
> > - * Discontiguous memory is allowed, mostly for framebuffers.
> > - *
> > - * Must be called holding kvm->slots_lock for write.
> > - */
> >  int __kvm_set_memory_region(struct kvm *kvm,
> >  			    const struct kvm_userspace_memory_region2 *mem)
> >  {
> > @@ -1992,6 +1984,8 @@ int __kvm_set_memory_region(struct kvm *kvm,
> >  	int as_id, id;
> >  	int r;
> >  
> > +	lockdep_assert_held(&kvm->slots_lock);
> 
> How about adding this lockdep assertion in __x86_set_memory_region() to replace
> this comment "/* Called with kvm->slots_lock held.  */" as well?

Ya, will do, I didn't see that comment.

Thanks!

Re: [PATCH 6/6] KVM: Move flags check for user memory regions to the ioctl() specific API

[Xiaoyao Li]

On 8/3/2024 4:50 AM, Sean Christopherson wrote:
> Move the check on memory region flags to kvm_vm_ioctl_set_memory_region()
> now that the internal API, kvm_set_internal_memslot(), disallows any and
> all flags.
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   virt/kvm/kvm_main.c | 54 ++++++++++++++++++---------------------------
>   1 file changed, 22 insertions(+), 32 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 84fcb20e3e1c..09cc261b080a 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1566,34 +1566,6 @@ static void kvm_replace_memslot(struct kvm *kvm,
>   #define KVM_SET_USER_MEMORY_REGION_V1_FLAGS \
>   	(KVM_MEM_LOG_DIRTY_PAGES | KVM_MEM_READONLY)
>   
> -static int check_memory_region_flags(struct kvm *kvm,
> -				     const struct kvm_userspace_memory_region2 *mem)
> -{
> -	u32 valid_flags = KVM_MEM_LOG_DIRTY_PAGES;
> -
> -	if (kvm_arch_has_private_mem(kvm))
> -		valid_flags |= KVM_MEM_GUEST_MEMFD;
> -
> -	/* Dirty logging private memory is not currently supported. */
> -	if (mem->flags & KVM_MEM_GUEST_MEMFD)
> -		valid_flags &= ~KVM_MEM_LOG_DIRTY_PAGES;
> -
> -#ifdef CONFIG_HAVE_KVM_READONLY_MEM
> -	/*
> -	 * GUEST_MEMFD is incompatible with read-only memslots, as writes to
> -	 * read-only memslots have emulated MMIO, not page fault, semantics,
> -	 * and KVM doesn't allow emulated MMIO for private memory.
> -	 */
> -	if (!(mem->flags & KVM_MEM_GUEST_MEMFD))
> -		valid_flags |= KVM_MEM_READONLY;
> -#endif
> -
> -	if (mem->flags & ~valid_flags)
> -		return -EINVAL;
> -
> -	return 0;
> -}

I would vote for keeping it instead of open coding it.

> -
>   static void kvm_swap_active_memslots(struct kvm *kvm, int as_id)
>   {
>   	struct kvm_memslots *slots = kvm_get_inactive_memslots(kvm, as_id);
> @@ -1986,10 +1958,6 @@ static int kvm_set_memory_region(struct kvm *kvm,
>   
>   	lockdep_assert_held(&kvm->slots_lock);
>   
> -	r = check_memory_region_flags(kvm, mem);
> -	if (r)
> -		return r;
> -
>   	as_id = mem->slot >> 16;
>   	id = (u16)mem->slot;
>   
> @@ -2114,6 +2082,28 @@ EXPORT_SYMBOL_GPL(kvm_set_internal_memslot);
>   static int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
>   					  struct kvm_userspace_memory_region2 *mem)
>   {
> +	u32 valid_flags = KVM_MEM_LOG_DIRTY_PAGES;
> +
> +	if (kvm_arch_has_private_mem(kvm))
> +		valid_flags |= KVM_MEM_GUEST_MEMFD;
> +
> +	/* Dirty logging private memory is not currently supported. */
> +	if (mem->flags & KVM_MEM_GUEST_MEMFD)
> +		valid_flags &= ~KVM_MEM_LOG_DIRTY_PAGES;
> +
> +#ifdef CONFIG_HAVE_KVM_READONLY_MEM
> +	/*
> +	 * GUEST_MEMFD is incompatible with read-only memslots, as writes to
> +	 * read-only memslots have emulated MMIO, not page fault, semantics,
> +	 * and KVM doesn't allow emulated MMIO for private memory.
> +	 */
> +	if (!(mem->flags & KVM_MEM_GUEST_MEMFD))
> +		valid_flags |= KVM_MEM_READONLY;
> +#endif
> +
> +	if (mem->flags & ~valid_flags)
> +		return -EINVAL;
> +
>   	if ((u16)mem->slot >= KVM_USER_MEM_SLOTS)
>   		return -EINVAL;
>