[PATCH v2 0/5] KVM: kvm_set_memory_region() cleanups

[PATCH v2 0/5] 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.

And, hopefully, the KVM-internal memslots hardening will also be useful for
s390's ucontrol stuff (https://lore.kernel.org/all/Z4FJNJ3UND8LSJZz@google.com).

v2:
 - Keep check_memory_region_flags() where it is. [Xiaoyao]
 - Rework the changelog for the last patch to account for the change in
   motiviation.
 - Fix double spaces goofs. [Tao]
 - Add a lockdep assertion in the x86 code, too. [Tao]

v1: https://lore.kernel.org/all/20240802205003.353672-1-seanjc@google.com

Sean Christopherson (5):
  KVM: Open code kvm_set_memory_region() into its sole caller (ioctl()
    API)
  KVM: Assert slots_lock is held when setting memory regions
  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

 arch/x86/kvm/x86.c       |  7 ++++---
 include/linux/kvm_host.h |  8 +++-----
 virt/kvm/kvm_main.c      | 33 ++++++++++++++-------------------
 3 files changed, 21 insertions(+), 27 deletions(-)


base-commit: 10b2c8a67c4b8ec15f9d07d177f63b563418e948
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v2 1/5] 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.

Cc: Tao Su <tao1.su@linux.intel.com>
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 401439bb21e3..7443de24b1d9 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1192,8 +1192,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 de2c11dae231..eb3d0a385077 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2058,25 +2058,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.47.1.613.gc27f4b7a9f-goog

[PATCH v2 2/5] KVM: Assert slots_lock is held when setting memory regions

[Sean Christopherson]

Add proper lockdep assertions in __kvm_set_memory_region() and
__x86_set_memory_region() instead of relying comments.

Opportunistically delete __kvm_set_memory_region()'s 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.

Cc: Tao Su <tao1.su@linux.intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c  |  3 ++-
 virt/kvm/kvm_main.c | 10 ++--------
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1b04092ec76a..a861287a67bd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12793,7 +12793,8 @@ void __user * __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa,
 	struct kvm_memslots *slots = kvm_memslots(kvm);
 	struct kvm_memory_slot *slot;
 
-	/* Called with kvm->slots_lock held.  */
+	lockdep_assert_held(&kvm->slots_lock);
+
 	if (WARN_ON(id >= KVM_MEM_SLOTS_NUM))
 		return ERR_PTR_USR(-EINVAL);
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index eb3d0a385077..7d25b50cb298 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1926,14 +1926,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)
 {
@@ -1945,6 +1937,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.47.1.613.gc27f4b7a9f-goog

[PATCH v2 3/5] 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.

Cc: Tao Su <tao1.su@linux.intel.com>
Cc: Claudio Imbrenda <imbrenda@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
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 a861287a67bd..36b5d06e3904 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12827,7 +12827,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 7443de24b1d9..8707d25a2e5b 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1192,8 +1192,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 7d25b50cb298..e1be2e4e6c9f 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1926,8 +1926,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;
@@ -2050,7 +2050,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.47.1.613.gc27f4b7a9f-goog

[PATCH v2 4/5] 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.

Cc: Tao Su <tao1.su@linux.intel.com>
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 36b5d06e3904..82f389e3910d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12928,7 +12928,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 8707d25a2e5b..dcb59d6e8acb 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1183,7 +1183,7 @@ struct kvm_memory_slot *kvm_vcpu_gfn_to_memslot(struct kvm_vcpu *vcpu, gfn_t 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 e1be2e4e6c9f..ecd4a66b22f3 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1926,8 +1926,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;
@@ -2057,7 +2057,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);
 
@@ -2068,7 +2068,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.47.1.613.gc27f4b7a9f-goog

[PATCH v2 5/5] 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.  In addition to
guarding against KVM goofs, explicitly disallowing dirty logging of KVM-
internal memslots will (hopefully) allow exempting KVM-internal memslots
from the KVM_MEM_MAX_NR_PAGES limit, which appears to exist purely because
the dirty bitmap operations use a 32-bit index.

Cc: Xiaoyao Li <xiaoyao.li@intel.com>
Cc: Claudio Imbrenda <imbrenda@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
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 ecd4a66b22f3..a8a84bf450f9 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2057,6 +2057,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.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v2 0/5] KVM: kvm_set_memory_region() cleanups

[Claudio Imbrenda]

On Fri, 10 Jan 2025 16:20:17 -0800
Sean Christopherson <seanjc@google.com> wrote:

> 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.
> 
> And, hopefully, the KVM-internal memslots hardening will also be useful for
> s390's ucontrol stuff (https://lore.kernel.org/all/Z4FJNJ3UND8LSJZz@google.com).

whole series:

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> 
> v2:
>  - Keep check_memory_region_flags() where it is. [Xiaoyao]
>  - Rework the changelog for the last patch to account for the change in
>    motiviation.
>  - Fix double spaces goofs. [Tao]
>  - Add a lockdep assertion in the x86 code, too. [Tao]
> 
> v1: https://lore.kernel.org/all/20240802205003.353672-1-seanjc@google.com
> 
> Sean Christopherson (5):
>   KVM: Open code kvm_set_memory_region() into its sole caller (ioctl()
>     API)
>   KVM: Assert slots_lock is held when setting memory regions
>   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
> 
>  arch/x86/kvm/x86.c       |  7 ++++---
>  include/linux/kvm_host.h |  8 +++-----
>  virt/kvm/kvm_main.c      | 33 ++++++++++++++-------------------
>  3 files changed, 21 insertions(+), 27 deletions(-)
> 
> 
> base-commit: 10b2c8a67c4b8ec15f9d07d177f63b563418e948

Re: [PATCH v2 0/5] KVM: kvm_set_memory_region() cleanups

[Christoph Schlameuss]

On Sat Jan 11, 2025 at 1:20 AM CET, Sean Christopherson wrote:
> 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.
>
> And, hopefully, the KVM-internal memslots hardening will also be useful for
> s390's ucontrol stuff (https://lore.kernel.org/all/Z4FJNJ3UND8LSJZz@google.com).

Whole series:

Acked-by: Christoph Schlameuss <schlameuss@linux.ibm.com>

>
> v2:
>  - Keep check_memory_region_flags() where it is. [Xiaoyao]
>  - Rework the changelog for the last patch to account for the change in
>    motiviation.
>  - Fix double spaces goofs. [Tao]
>  - Add a lockdep assertion in the x86 code, too. [Tao]
>
> v1: https://lore.kernel.org/all/20240802205003.353672-1-seanjc@google.com
>
> Sean Christopherson (5):
>   KVM: Open code kvm_set_memory_region() into its sole caller (ioctl()
>     API)
>   KVM: Assert slots_lock is held when setting memory regions
>   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
>
>  arch/x86/kvm/x86.c       |  7 ++++---
>  include/linux/kvm_host.h |  8 +++-----
>  virt/kvm/kvm_main.c      | 33 ++++++++++++++-------------------
>  3 files changed, 21 insertions(+), 27 deletions(-)
>
>
> base-commit: 10b2c8a67c4b8ec15f9d07d177f63b563418e948

Re: [PATCH v2 0/5] KVM: kvm_set_memory_region() cleanups

[Xiaoyao Li]

On 1/11/2025 8:20 AM, Sean Christopherson wrote:
> 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.
> 
> And, hopefully, the KVM-internal memslots hardening will also be useful for
> s390's ucontrol stuff (https://lore.kernel.org/all/Z4FJNJ3UND8LSJZz@google.com).

For the series:

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> v2:
>   - Keep check_memory_region_flags() where it is. [Xiaoyao]
>   - Rework the changelog for the last patch to account for the change in
>     motiviation.
>   - Fix double spaces goofs. [Tao]
>   - Add a lockdep assertion in the x86 code, too. [Tao]
> 
> v1: https://lore.kernel.org/all/20240802205003.353672-1-seanjc@google.com
> 
> Sean Christopherson (5):
>    KVM: Open code kvm_set_memory_region() into its sole caller (ioctl()
>      API)
>    KVM: Assert slots_lock is held when setting memory regions
>    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
> 
>   arch/x86/kvm/x86.c       |  7 ++++---
>   include/linux/kvm_host.h |  8 +++-----
>   virt/kvm/kvm_main.c      | 33 ++++++++++++++-------------------
>   3 files changed, 21 insertions(+), 27 deletions(-)
> 
> 
> base-commit: 10b2c8a67c4b8ec15f9d07d177f63b563418e948

Re: [PATCH v2 0/5] KVM: kvm_set_memory_region() cleanups

[Sean Christopherson]

On Fri, 10 Jan 2025 16:20:17 -0800, Sean Christopherson wrote:
> 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.
> 
> And, hopefully, the KVM-internal memslots hardening will also be useful for
> s390's ucontrol stuff (https://lore.kernel.org/all/Z4FJNJ3UND8LSJZz@google.com).
> 
> v2:
>  - Keep check_memory_region_flags() where it is. [Xiaoyao]
>  - Rework the changelog for the last patch to account for the change in
>    motiviation.
>  - Fix double spaces goofs. [Tao]
>  - Add a lockdep assertion in the x86 code, too. [Tao]
> 
> [...]

Applied to kvm-x86 memslots, and pushed

  https://github.com/kvm-x86/linux tags/kvm-memslots-6.14

as well.  Thanks much for the reviews!

[1/5] KVM: Open code kvm_set_memory_region() into its sole caller (ioctl() API)
      https://github.com/kvm-x86/linux/commit/f81a6d12bf8b
[2/5] KVM: Assert slots_lock is held when setting memory regions
      https://github.com/kvm-x86/linux/commit/d131f0042f46
[3/5] KVM: Add a dedicated API for setting KVM-internal memslots
      https://github.com/kvm-x86/linux/commit/156bffdb2b49
[4/5] KVM: x86: Drop double-underscores from __kvm_set_memory_region()
      https://github.com/kvm-x86/linux/commit/344315e93dbc
[5/5] KVM: Disallow all flags for KVM-internal memslots
      https://github.com/kvm-x86/linux/commit/0cc3cb2151f9

--
https://github.com/kvm-x86/linux/tree/next

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

[Christoph Schlameuss]

On Sat Jan 11, 2025 at 1:20 AM CET, 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.
>
> Cc: Tao Su <tao1.su@linux.intel.com>
> Cc: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> 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(-)

[...]

> +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;
> +

Looking at Claudios changes I found that this is missing to acquire the
slots_lock here.

guard(mutex)(&kvm->slots_lock);

> +	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)

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

[Sean Christopherson]

On Mon, Jan 20, 2025, Christoph Schlameuss wrote:
> On Sat Jan 11, 2025 at 1:20 AM CET, 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.
> >
> > Cc: Tao Su <tao1.su@linux.intel.com>
> > Cc: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > 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(-)
> 
> [...]
> 
> > +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;
> > +
> 
> Looking at Claudios changes I found that this is missing to acquire the
> slots_lock here.
> 
> guard(mutex)(&kvm->slots_lock);

It's not missing.  As of this patch, x86 is the only user of KVM-internal memslots,
and x86 acquires slots_lock outside of kvm_set_internal_memslot() because x86 can
have multiple address spaces (regular vs SMM) and KVM's internal memslots need to
be created for both, i.e. it's desirable to holds slots_lock in the caller.

If it's annoying for s390 to acquire slots_lock, we could add a wrapper, i.e. turn
this into __kvm_set_internal_memslot() and then re-add kvm_set_internal_memslot()
as a version that acquires and releases slots_lock.

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

[Claudio Imbrenda]

On Tue, 21 Jan 2025 08:05:57 -0800
Sean Christopherson <seanjc@google.com> wrote:

> On Mon, Jan 20, 2025, Christoph Schlameuss wrote:
> > On Sat Jan 11, 2025 at 1:20 AM CET, 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.
> > >
> > > Cc: Tao Su <tao1.su@linux.intel.com>
> > > Cc: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > > 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(-)  
> > 
> > [...]
> >   
> > > +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;
> > > +  
> > 
> > Looking at Claudios changes I found that this is missing to acquire the
> > slots_lock here.
> > 
> > guard(mutex)(&kvm->slots_lock);  
> 
> It's not missing.  As of this patch, x86 is the only user of KVM-internal memslots,
> and x86 acquires slots_lock outside of kvm_set_internal_memslot() because x86 can
> have multiple address spaces (regular vs SMM) and KVM's internal memslots need to
> be created for both, i.e. it's desirable to holds slots_lock in the caller.
> 
> If it's annoying for s390 to acquire slots_lock, we could add a wrapper, i.e. turn
> this into __kvm_set_internal_memslot() and then re-add kvm_set_internal_memslot()
> as a version that acquires and releases slots_lock.

I think it's fine as it is, just document that the lock needs to be
held

I'll add the necessary locking in the s390 code

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

[Christoph Schlameuss]

On Tue Jan 21, 2025 at 5:17 PM CET, Claudio Imbrenda wrote:
> On Tue, 21 Jan 2025 08:05:57 -0800
> Sean Christopherson <seanjc@google.com> wrote:
>
> > On Mon, Jan 20, 2025, Christoph Schlameuss wrote:
> > > On Sat Jan 11, 2025 at 1:20 AM CET, 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.
> > > >
> > > > Cc: Tao Su <tao1.su@linux.intel.com>
> > > > Cc: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > > > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > > > 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(-)  
> > > 
> > > [...]
> > >   
> > > > +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;
> > > > +  
> > > 
> > > Looking at Claudios changes I found that this is missing to acquire the
> > > slots_lock here.
> > > 
> > > guard(mutex)(&kvm->slots_lock);  
> > 
> > It's not missing.  As of this patch, x86 is the only user of KVM-internal memslots,
> > and x86 acquires slots_lock outside of kvm_set_internal_memslot() because x86 can
> > have multiple address spaces (regular vs SMM) and KVM's internal memslots need to
> > be created for both, i.e. it's desirable to holds slots_lock in the caller.
> > 
> > If it's annoying for s390 to acquire slots_lock, we could add a wrapper, i.e. turn
> > this into __kvm_set_internal_memslot() and then re-add kvm_set_internal_memslot()
> > as a version that acquires and releases slots_lock.
>
> I think it's fine as it is, just document that the lock needs to be
> held
>
> I'll add the necessary locking in the s390 code

I see. Thank you for the elaboration, Sean!