[PATCH v5 0/7] KVM: s390: Misc bugfixes for 7.4

[PATCH v5 0/7] KVM: s390: Misc bugfixes for 7.4

[Claudio Imbrenda]

Misc bugfixes.

Some Fixes tags are approximate, since the bugs arise from slow changes
behind the scenes in the kernel, and not from an actual bug at the moment
of the introduction of the original code.

v4->v5:
* fix check gmap_unmap_gfn_range_fast()
* refactor and rename the function to enable cmma, now have a lockless
  version called gmap_enable_cmm_already_locked() and a wrapper called
  gmap_enable_cmm() that performs the locking
* fix missing DAT_WALK_USES_SKEYS flag
* fix some comments

v3->v4:
* oops! some stuff removed in the previous iteration was actually needed
* improve patch descriptions
* fix idte_crste() clobber and table type (partially picked from v2)
* s/IPTE_GUEST_ASCE/IDTE_GUEST_ASCE/ when using idte_crste()
* improve comments in gmap_unmap_gfn_range()
* do not attempt unmapping prefixes if the unmap range is above 2G
* remove useless !hpa check in pin_blocks()
* fix cmma issue when using large pages that caused the whole guest
  memory to be allocated

v2->v3:
* remove all non-bugfix patches
* rebase onto master

v1->v2:
* add "memory" clobber to idte_crste()
* remove superflouos gpa parameter for unpin_scb()
* clear the zero bit in the PGSTE when unmapping, add missing barrier


Claudio Imbrenda (7):
  KVM: s390: Fix UCONTROL inconsistencies
  KVM: s390: Move some skey functions to headers
  KVM: s390: Fix idte_crste() clobber and table type
  KVM: s390: Improve unmapping of large ranges
  KVM: s390: vsie: Fix pinning using the wrong flag
  KVM: s390: Introduce gmap_enable_cmm()
  KVM: s390: Allocate page tables when CMMA is used

 arch/s390/kvm/gmap/dat.c  |  26 -----
 arch/s390/kvm/gmap/dat.h  |  48 +++++++-
 arch/s390/kvm/gmap/gmap.c | 227 +++++++++++++++++++++++++++++++-------
 arch/s390/kvm/gmap/gmap.h |  11 ++
 arch/s390/kvm/s390/priv.c |   8 +-
 arch/s390/kvm/s390/s390.c |   4 +-
 arch/s390/kvm/s390/vsie.c | 135 ++++++++---------------
 7 files changed, 299 insertions(+), 160 deletions(-)

-- 
2.56.0

[PATCH v5 1/7] KVM: s390: Fix UCONTROL inconsistencies

[Claudio Imbrenda]

For UCONTROL VMs, when a vCPU segment entry is mapped but unallocated,
the tl field was set to 1. This collides with the present bit, which
means that other parts of gmap would mistake the entry for a present
one.

Refactor the code to use a different combination of flags to indicate a
mapped but unallocated segment.

The new flags combination will result in several functions returning
-EFAULT when used on UCONTROL guests. This is fine, since those
interfaces should not be used for UCONTROL guests to begin with.

Proper fencing of the interfaces that are not allowed for UCONTROL
guests is outside the scope of this patch.

Fixes: a2c17f9270cc ("KVM: s390: New gmap code")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Cc: stable@vger.kernel.org # 7.0+
---
 arch/s390/kvm/gmap/dat.h  | 16 +++++++++++-
 arch/s390/kvm/gmap/gmap.c | 54 +++++++++++++++++++--------------------
 2 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/arch/s390/kvm/gmap/dat.h b/arch/s390/kvm/gmap/dat.h
index 90389d47ba4e..4665c1112bb9 100644
--- a/arch/s390/kvm/gmap/dat.h
+++ b/arch/s390/kvm/gmap/dat.h
@@ -109,6 +109,8 @@ union pte {
 #define _REGION3_FR_MASK	(_REGION3_MASK >> PAGE_SHIFT)
 #define _PAGES_PER_SEGMENT	_PAGE_ENTRIES
 #define _PAGES_PER_REGION3	(_PAGES_PER_SEGMENT * _CRST_ENTRIES)
+#define _UCAS_ENTRY_LENGTH	0
+#define _UCAS_ENTRY_OFFSET	3
 
 /* Soft dirty, needed as macro for atomic operations on ptes */
 #define _PAGE_SD 0x002
@@ -421,6 +423,17 @@ static inline union crste _crste_fc0(kvm_pfn_t pfn, int tt)
 	return res;
 }
 
+static inline union crste _crste_ucas(kvm_pfn_t pfn)
+{
+	union crste res = { .val = PFN_PHYS(pfn) };
+
+	res.h.i = 1;
+	res.h.tt = TABLE_TYPE_SEGMENT;
+	res.h.fc0.tl = _UCAS_ENTRY_LENGTH;
+	res.h.fc0.tf = _UCAS_ENTRY_OFFSET;
+	return res;
+}
+
 /**
  * _crste() - Useful constructor for union crste with FC=1
  * @pfn: the pfn this pte should point to.
@@ -985,7 +998,8 @@ static inline int dat_create_slot(struct kvm_s390_mmu_cache *mc, union asce asce
 
 static inline bool crste_is_ucas(union crste crste)
 {
-	return is_pmd(crste) && crste.h.i && crste.h.fc0.tl == 1 && crste.h.fc == 0;
+	return is_pmd(crste) && crste.h.i && !crste.h.fc &&
+	       crste.h.fc0.tl == _UCAS_ENTRY_LENGTH && crste.h.fc0.tf == _UCAS_ENTRY_OFFSET;
 }
 
 #endif /* ARCH_KVM_GMAP_DAT_H */
diff --git a/arch/s390/kvm/gmap/gmap.c b/arch/s390/kvm/gmap/gmap.c
index bf91e3d5013b..225fdae3d57c 100644
--- a/arch/s390/kvm/gmap/gmap.c
+++ b/arch/s390/kvm/gmap/gmap.c
@@ -777,16 +777,14 @@ static int gmap_ucas_map_one(struct kvm_s390_mmu_cache *mc, struct gmap *gmap,
 	if (rc)
 		return rc;
 	if (!ptep) {
-		newcrste = _crste_fc0(p_gfn, TABLE_TYPE_SEGMENT);
-		newcrste.h.i = 1;
-		newcrste.h.fc0.tl = 1;
+		newcrste = _crste_ucas(p_gfn);
 	} else {
 		pt = pte_table_start(ptep);
 		dat_set_ptval(pt, PTVAL_VMADDR, p_gfn >> (_SEGMENT_SHIFT - PAGE_SHIFT));
 		newcrste = _crste_fc0(virt_to_pfn(pt), TABLE_TYPE_SEGMENT);
 	}
-	rc = dat_entry_walk(mc, c_gfn, gmap->asce, DAT_WALK_ALLOC, TABLE_TYPE_SEGMENT,
-			    &crstep, &ptep);
+	rc = dat_entry_walk(mc, c_gfn, gmap->asce, DAT_WALK_ALLOC | DAT_WALK_IGN_HOLES,
+			    TABLE_TYPE_SEGMENT, &crstep, &ptep);
 	if (rc)
 		return rc;
 	do {
@@ -802,8 +800,9 @@ static int gmap_ucas_translate_simple(struct gmap *gmap, gpa_t *gaddr, union crs
 	union pte *ptep;
 	int rc;
 
-	rc = dat_entry_walk(NULL, gpa_to_gfn(*gaddr), gmap->asce, DAT_WALK_CONTINUE,
-			    TABLE_TYPE_SEGMENT, crstepp, &ptep);
+	rc = dat_entry_walk(NULL, gpa_to_gfn(*gaddr), gmap->asce,
+			    DAT_WALK_IGN_HOLES | DAT_WALK_ANY,
+			    TABLE_TYPE_PAGE_TABLE, crstepp, &ptep);
 	if (rc || (!ptep && !crste_is_ucas(**crstepp)))
 		return -EREMOTE;
 	if (!ptep)
@@ -840,26 +839,24 @@ int gmap_ucas_translate(struct kvm_s390_mmu_cache *mc, struct gmap *gmap, gpa_t
 		if (rc <= 0)
 			return rc;
 	}
-	do {
-		scoped_guard(write_lock, &gmap->kvm->mmu_lock) {
-			rc = gmap_ucas_translate_simple(gmap, gaddr, &crstep);
-			if (rc <= 0)
-				return rc;
-			translated_address = (*gaddr & ~_SEGMENT_MASK) |
-					     (crstep->val & _SEGMENT_MASK);
-			rc = gmap_ucas_map_one(mc, gmap, gpa_to_gfn(translated_address), gfn, true);
-		}
-		if (!rc) {
-			*gaddr = translated_address;
-			return 0;
-		}
-		if (rc != -ENOMEM)
-			return -EREMOTE;
-		rc = kvm_s390_mmu_cache_topup(mc);
-		if (rc)
+	rc = kvm_s390_mmu_cache_topup(mc);
+	if (rc)
+		return rc;
+
+	scoped_guard(write_lock, &gmap->kvm->mmu_lock) {
+		rc = gmap_ucas_translate_simple(gmap, gaddr, &crstep);
+		if (rc <= 0)
 			return rc;
-	} while (1);
-	return 0;
+		translated_address = (*gaddr & ~_SEGMENT_MASK) |
+				     (crstep->val & _SEGMENT_MASK);
+		rc = gmap_ucas_map_one(mc, gmap, gpa_to_gfn(translated_address), gfn, true);
+	}
+
+	if (!rc)
+		*gaddr = translated_address;
+	if (!rc || rc == -ENOMEM)
+		return rc;
+	return -EREMOTE;
 }
 
 int gmap_ucas_map(struct gmap *gmap, gfn_t p_gfn, gfn_t c_gfn, unsigned long count)
@@ -896,8 +893,9 @@ static void gmap_ucas_unmap_one(struct gmap *gmap, gfn_t c_gfn)
 	union pte *ptep;
 	int rc;
 
-	rc = dat_entry_walk(NULL, c_gfn, gmap->asce, 0, TABLE_TYPE_SEGMENT, &crstep, &ptep);
-	if (rc)
+	rc = dat_entry_walk(NULL, c_gfn, gmap->asce, DAT_WALK_IGN_HOLES,
+			    TABLE_TYPE_SEGMENT, &crstep, &ptep);
+	if (rc || !is_pmd(*crstep))
 		return;
 	while (!dat_crstep_xchg_atomic(crstep, READ_ONCE(*crstep), _PMD_EMPTY, c_gfn, gmap->asce))
 		;
-- 
2.56.0

[PATCH v5 2/7] KVM: s390: Move some skey functions to headers

[Claudio Imbrenda]

Move some storage key handling functions to headers, since they will be
needed from outside, and they are small, so inlining is acceptable.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Cc: stable@vger.kernel.org # 7.0+: prerequisite for the next patch
---
 arch/s390/kvm/gmap/dat.c | 26 --------------------------
 arch/s390/kvm/gmap/dat.h | 26 ++++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/arch/s390/kvm/gmap/dat.c b/arch/s390/kvm/gmap/dat.c
index ff80d02c9f56..b9507daaf327 100644
--- a/arch/s390/kvm/gmap/dat.c
+++ b/arch/s390/kvm/gmap/dat.c
@@ -159,32 +159,6 @@ bool __must_check dat_crstep_xchg_atomic(union crste *crstep, union crste old, u
 	return cspg_crste(crstep, old, new);
 }
 
-static void dat_set_storage_key_from_pgste(union pte pte, union pgste pgste)
-{
-	union skey nkey = { .acc = pgste.acc, .fp = pgste.fp };
-
-	page_set_storage_key(pte_origin(pte), nkey.skey, 0);
-}
-
-static void dat_move_storage_key(union pte old, union pte new)
-{
-	page_set_storage_key(pte_origin(new), page_get_storage_key(pte_origin(old)), 1);
-}
-
-static union pgste dat_save_storage_key_into_pgste(union pte pte, union pgste pgste)
-{
-	union skey skey;
-
-	skey.skey = page_get_storage_key(pte_origin(pte));
-
-	pgste.acc = skey.acc;
-	pgste.fp = skey.fp;
-	pgste.gr |= skey.r;
-	pgste.gc |= skey.c;
-
-	return pgste;
-}
-
 union pgste __dat_ptep_xchg(union pte *ptep, union pgste pgste, union pte new, gfn_t gfn,
 			    union asce asce, bool uses_skeys)
 {
diff --git a/arch/s390/kvm/gmap/dat.h b/arch/s390/kvm/gmap/dat.h
index 4665c1112bb9..687e1fa904d6 100644
--- a/arch/s390/kvm/gmap/dat.h
+++ b/arch/s390/kvm/gmap/dat.h
@@ -1002,4 +1002,30 @@ static inline bool crste_is_ucas(union crste crste)
 	       crste.h.fc0.tl == _UCAS_ENTRY_LENGTH && crste.h.fc0.tf == _UCAS_ENTRY_OFFSET;
 }
 
+static inline void dat_set_storage_key_from_pgste(union pte pte, union pgste pgste)
+{
+	union skey nkey = { .acc = pgste.acc, .fp = pgste.fp };
+
+	page_set_storage_key(pte_origin(pte), nkey.skey, 0);
+}
+
+static inline void dat_move_storage_key(union pte old, union pte new)
+{
+	page_set_storage_key(pte_origin(new), page_get_storage_key(pte_origin(old)), 1);
+}
+
+static inline union pgste dat_save_storage_key_into_pgste(union pte pte, union pgste pgste)
+{
+	union skey skey;
+
+	skey.skey = page_get_storage_key(pte_origin(pte));
+
+	pgste.acc = skey.acc;
+	pgste.fp = skey.fp;
+	pgste.gr |= skey.r;
+	pgste.gc |= skey.c;
+
+	return pgste;
+}
+
 #endif /* ARCH_KVM_GMAP_DAT_H */
-- 
2.56.0

[PATCH v5 3/7] KVM: s390: Fix idte_crste() clobber and table type

[Claudio Imbrenda]

Fix idte_crste() to add the missing table-type bits and the missing
"memory" clobber.

Fixes: 5a74e3d93417 ("KVM: s390: KVM-specific bitfields and helper functions")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Cc: stable@vger.kernel.org # 7.0+
---
 arch/s390/kvm/gmap/dat.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/gmap/dat.h b/arch/s390/kvm/gmap/dat.h
index 687e1fa904d6..fb7e839a355f 100644
--- a/arch/s390/kvm/gmap/dat.h
+++ b/arch/s390/kvm/gmap/dat.h
@@ -640,20 +640,22 @@ static __always_inline void idte_crste(union crste *crstep, gfn_t gfn, unsigned
 	unsigned long table_origin = __pa(crste_table_start(crstep));
 	unsigned long gaddr = gfn_to_gpa(gfn) & HPAGE_MASK;
 
+	table_origin |= crstep->val & _REGION_ENTRY_TYPE_MASK;
+
 	if (__builtin_constant_p(opt) && opt == 0) {
 		/* flush without guest asce */
 		asm volatile("idte	%[table_origin],0,%[gaddr],%[local]"
 			: "+m" (*crstep)
 			: [table_origin] "a" (table_origin), [gaddr] "a" (gaddr),
 			  [local] "i" (local)
-			: "cc");
+			: "cc", "memory");
 	} else {
 		/* flush with guest asce */
 		asm volatile("idte %[table_origin],%[asce],%[gaddr_opt],%[local]"
 			: "+m" (*crstep)
 			: [table_origin] "a" (table_origin), [gaddr_opt] "a" (gaddr | opt),
 			  [asce] "a" (asce.val), [local] "i" (local)
-			: "cc");
+			: "cc", "memory");
 	}
 }
 
-- 
2.56.0

[PATCH v5 4/7] KVM: s390: Improve unmapping of large ranges

[Claudio Imbrenda]

When unmapping guest ranges, each individual leaf DAT entry is
invalidated. When unmapping large ranges, this results in a very large
amount of invalidations, which is time consuming. Moreover, since the
unmap callback is called by KVM while holding a spinlock, this also
leads to stall warnings.

Improve the situation by invalidating DAT entries higher in the tree,
instead of each leaf individually.

For small invalidations (up to 1 segment = 1M of guest memory), and
special cases (like protected VMs) regardless of the size, the old and
slow invalidation is performed; all leaf entries are invalidated one by
one.

For large invalidations (larger than 1M of guest memory), the new
strategy is used:
 - Invalidate prefixes on the whole range, extending start and end to
   1M granularity
 - Iterate through the page table pointed to by each pmd and unshadow
   entries as needed
 - Detach (invalidate) the pmd
 - Save storage keys as needed
 - Invalidate each (now detached) pte by simply overwriting it
 - Reattach the old pmd to point to the updated page table

Fixes: a2c17f9270cc ("KVM: s390: New gmap code")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Cc: stable@vger.kernel.org # 7.0+
---
 arch/s390/kvm/gmap/gmap.c | 136 +++++++++++++++++++++++++++++++++++---
 1 file changed, 125 insertions(+), 11 deletions(-)

diff --git a/arch/s390/kvm/gmap/gmap.c b/arch/s390/kvm/gmap/gmap.c
index 225fdae3d57c..b9d92bef5459 100644
--- a/arch/s390/kvm/gmap/gmap.c
+++ b/arch/s390/kvm/gmap/gmap.c
@@ -434,17 +434,8 @@ static long _gmap_unmap_crste(union crste *crstep, gfn_t gfn, gfn_t next, struct
 	return 0;
 }
 
-/**
- * gmap_unmap_gfn_range() - Unmap a range of guest addresses.
- * @gmap: The gmap to act on.
- * @slot: The memslot in which the range is located.
- * @start: The first gfn to unmap.
- * @end: The gfn after the last one to unmap.
- *
- * Context: Called with the kvm mmu write lock held.
- * Return: false
- */
-bool gmap_unmap_gfn_range(struct gmap *gmap, struct kvm_memory_slot *slot, gfn_t start, gfn_t end)
+static bool gmap_unmap_gfn_range_slow(struct gmap *gmap, struct kvm_memory_slot *slot,
+				      gfn_t start, gfn_t end)
 {
 	const struct dat_walk_ops ops = {
 		.pte_entry = _gmap_unmap_pte,
@@ -462,6 +453,129 @@ bool gmap_unmap_gfn_range(struct gmap *gmap, struct kvm_memory_slot *slot, gfn_t
 	return false;
 }
 
+static long _gmap_unmap_pud_fast(union crste *crstep, gfn_t gfn, gfn_t next, struct dat_walk *w)
+{
+	union crste old = *crstep;
+
+	/* We only care about large puds */
+	if (!old.h.fc)
+		return 0;
+	/*
+	 * The caller did not unmap prefixes outside the original range, which
+	 * does not necessarily cover the whole pud.
+	 * Since all prefixes are below 2G, the first pud covers them all;
+	 * make sure all prefixes are unmapped whenever the first pud is being
+	 * unmapped.
+	 */
+	if (!gfn)
+		gmap_unmap_prefix(w->priv, 0, next);
+	if (old.s.fc1.vsie_notif)
+		gmap_handle_vsie_unshadow_event(w->priv, gfn);
+
+	if (machine_has_tlb_guest())
+		idte_crste(crstep, gfn, IDTE_GUEST_ASCE, w->asce, 0);
+	else
+		idte_crste(crstep, gfn, 0, w->asce, 0);
+	*crstep = _CRSTE_EMPTY(old.h.tt);
+
+	return 0;
+}
+
+static long _gmap_unmap_pmd_fast(union crste *crstep, gfn_t gfn, gfn_t next, struct dat_walk *w)
+{
+	union crste old = *crstep;
+	struct page_table *pt;
+	gfn_t first, last;
+	unsigned int i;
+
+	if (old.h.fc) {
+		if (old.s.fc1.vsie_notif)
+			gmap_handle_vsie_unshadow_event(w->priv, gfn);
+
+		if (machine_has_tlb_guest())
+			idte_crste(crstep, gfn, IDTE_GUEST_ASCE, w->asce, 0);
+		else
+			idte_crste(crstep, gfn, 0, w->asce, 0);
+		WRITE_ONCE(*crstep, _CRSTE_EMPTY(old.h.tt));
+
+		return 0;
+	}
+
+	if (old.h.i)
+		return 0;
+
+	pt = dereference_pmd(old.pmd);
+	first = max(gfn, w->start) & (PTRS_PER_PTE - 1);
+	last = (min(next, w->end) - 1) & (PTRS_PER_PTE - 1);
+
+	for (i = first; i <= last; i++)
+		if (pte_needs_unshadow(pt->ptes[i], _PTE_EMPTY, pt->pgstes[i]))
+			gmap_handle_vsie_unshadow_event(w->priv, gfn + i);
+
+	/* Detach the page table */
+	if (machine_has_tlb_guest())
+		idte_crste(crstep, gfn, IDTE_GUEST_ASCE, w->asce, 0);
+	else
+		idte_crste(crstep, gfn, 0, w->asce, 0);
+
+	for (i = first; i <= last; i++) {
+		if (pte_hole(pt->ptes[i]))
+			continue;
+		if (!pt->ptes[i].h.i && uses_skeys(w->priv))
+			pt->pgstes[i] = dat_save_storage_key_into_pgste(pt->ptes[i], pt->pgstes[i]);
+		pt->ptes[i] = _PTE_EMPTY;
+		pt->pgstes[i].vsie_notif = 0;
+		pt->pgstes[i].vsie_gmem = 0;
+		pt->pgstes[i].zero = 0;
+		pt->pgstes[i].prefix_notif = 0;
+	}
+
+	barrier();
+	/* Reattach the modified page table */
+	WRITE_ONCE(*crstep, old);
+
+	return 0;
+}
+
+static bool gmap_unmap_gfn_range_fast(struct gmap *gmap, gfn_t start, gfn_t end)
+{
+	const struct dat_walk_ops ops = {
+		.pud_entry = _gmap_unmap_pud_fast,
+		.pmd_entry = _gmap_unmap_pmd_fast,
+	};
+	gfn_t aligned_start = ALIGN_DOWN(start, _PAGE_ENTRIES);
+	gfn_t aligned_end = ALIGN(end, _PAGE_ENTRIES);
+	bool failed;
+
+	lockdep_assert_held_write(&gmap->kvm->mmu_lock);
+
+	/* Prefixes are below 2G; if the start is above 2G, nothing to do */
+	if (aligned_start < _PAGES_PER_REGION3)
+		gmap_unmap_prefix(gmap, aligned_start, aligned_end);
+
+	failed = _dat_walk_gfn_range(start, end, gmap->asce, &ops, DAT_WALK_IGN_HOLES, gmap);
+	KVM_BUG_ON(failed, gmap->kvm);
+	return false;
+}
+
+/**
+ * gmap_unmap_gfn_range() - Unmap a range of guest addresses.
+ * @gmap: The gmap to act on.
+ * @slot: The memslot in which the range is located.
+ * @start: The first gfn to unmap.
+ * @end: The gfn after the last one to unmap.
+ *
+ * Context: Called with the kvm mmu write lock held.
+ * Return: false
+ */
+bool gmap_unmap_gfn_range(struct gmap *gmap, struct kvm_memory_slot *slot, gfn_t start, gfn_t end)
+{
+	if ((end - start <= _PAGES_PER_SEGMENT) ||
+	    test_bit(GMAP_FLAG_EXPORT_ON_UNMAP, &gmap->flags) || is_ucontrol(gmap))
+		return gmap_unmap_gfn_range_slow(gmap, slot, start, end);
+	return gmap_unmap_gfn_range_fast(gmap, start, end);
+}
+
 static union pgste __pte_test_and_clear_softdirty(union pte *ptep, union pgste pgste, gfn_t gfn,
 						  struct gmap *gmap)
 {
-- 
2.56.0

[PATCH v5 5/7] KVM: s390: vsie: Fix pinning using the wrong flag

[Claudio Imbrenda]

When pinning guest memory using pin_blocks(), gfn_to_page() was used,
which under the hood uses FOLL_GET, which is the wrong type of flag for
memory that is accessed directly by hardware.

Fix by using kvm_vcpu_{,un}map(), which uses FOLL_PIN and is the
correct thing to do.

While technically a bugfix, the previous behaviour can only lead to
guest corruption in extremely convoluted test scenarios that cannot
happen in real/production systems.

Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Cc: stable@vger.kernel.org #7.0+
---
 arch/s390/kvm/s390/vsie.c | 135 ++++++++++++++------------------------
 1 file changed, 49 insertions(+), 86 deletions(-)

diff --git a/arch/s390/kvm/s390/vsie.c b/arch/s390/kvm/s390/vsie.c
index 5926689fa226..6dd06ed52bb8 100644
--- a/arch/s390/kvm/s390/vsie.c
+++ b/arch/s390/kvm/s390/vsie.c
@@ -49,20 +49,21 @@ struct vsie_page {
 	/* address of the last reported fault to guest2 */
 	unsigned long fault_addr;		/* 0x0228 */
 	/* calculated guest addresses of satellite control blocks */
-	gpa_t sca_gpa;				/* 0x0230 */
-	gpa_t itdba_gpa;			/* 0x0238 */
-	gpa_t gvrd_gpa;				/* 0x0240 */
-	gpa_t riccbd_gpa;			/* 0x0248 */
-	gpa_t sdnx_gpa;				/* 0x0250 */
+	struct kvm_host_map sca_map;		/* 0x0230 */
+	struct kvm_host_map itdba_map;		/* 0x0260 */
+	struct kvm_host_map gvrd_map;		/* 0x0290 */
+	struct kvm_host_map riccbd_map;		/* 0x02c0 */
+	struct kvm_host_map sdnx_map;		/* 0x02f0 */
+	struct kvm_host_map scb_map;		/* 0x0320 */
 	/*
 	 * guest address of the original SCB. Remains set for free vsie
 	 * pages, so we can properly look them up in our addr_to_page
 	 * radix tree.
 	 */
-	gpa_t scb_gpa;				/* 0x0258 */
+	gpa_t scb_gpa;				/* 0x0350 */
 	/* the shadow gmap in use by the vsie_page */
-	struct gmap_cache gmap_cache;		/* 0x0260 */
-	__u8 reserved[0x06f8 - 0x0278];		/* 0x0278 */
+	struct gmap_cache gmap_cache;		/* 0x0358 */
+	__u8 reserved[0x06f8 - 0x0370];		/* 0x0370 */
 	struct kvm_s390_crypto_cb crycb;	/* 0x06f8 */
 	__u8 fac[8 + S390_ARCH_FAC_LIST_SIZE_BYTE];/* 0x07f8 */
 };
@@ -681,72 +682,45 @@ static int map_prefix(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page, struct
 }
 
 /*
- * Pin the guest page given by gpa and set hpa to the pinned host address.
+ * pin_guest_page() - Pin the given guest page and return its hpa.
+ * @vcpu: the vCPU
+ * @gpa: the gpa to pin
+ * @map: the map representing this pinning.
+ *
  * Will always be pinned writable.
  *
- * Returns: - 0 on success
- *          - -EINVAL if the gpa is not valid guest storage
+ * Returns: - 0 if the gpa is not valid guest storage,
+ *          - The host hpa of the pinned page in case of success
  */
-static int pin_guest_page(struct kvm *kvm, gpa_t gpa, hpa_t *hpa)
+static hpa_t pin_guest_page(struct kvm_vcpu *vcpu, gpa_t gpa, struct kvm_host_map *map)
 {
-	struct page *page;
-
-	page = gfn_to_page(kvm, gpa_to_gfn(gpa));
-	if (!page)
-		return -EINVAL;
-	*hpa = (hpa_t)page_to_phys(page) + (gpa & ~PAGE_MASK);
-	return 0;
-}
-
-/* Unpins a page previously pinned via pin_guest_page, marking it as dirty. */
-static void unpin_guest_page(struct kvm *kvm, gpa_t gpa, hpa_t hpa)
-{
-	kvm_release_page_dirty(pfn_to_page(phys_to_pfn(hpa)));
-	/* mark the page always as dirty for migration */
-	mark_page_dirty(kvm, gpa_to_gfn(gpa));
+	if (kvm_vcpu_map(vcpu, gpa_to_gfn(gpa), map))
+		return 0;
+	if (!kvm_vcpu_mapped(map))
+		return 0;
+	return (hpa_t)PFN_PHYS(map->pfn) + (gpa & ~PAGE_MASK);
 }
 
 /* unpin all blocks previously pinned by pin_blocks(), marking them dirty */
 static void unpin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 {
 	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
-	hpa_t hpa;
 
-	hpa = (u64) scb_s->scaoh << 32 | scb_s->scaol;
-	if (hpa) {
-		unpin_guest_page(vcpu->kvm, vsie_page->sca_gpa, hpa);
-		vsie_page->sca_gpa = 0;
-		scb_s->scaol = 0;
-		scb_s->scaoh = 0;
-	}
+	kvm_vcpu_unmap(vcpu, &vsie_page->sca_map);
+	scb_s->scaol = 0;
+	scb_s->scaoh = 0;
 
-	hpa = scb_s->itdba;
-	if (hpa) {
-		unpin_guest_page(vcpu->kvm, vsie_page->itdba_gpa, hpa);
-		vsie_page->itdba_gpa = 0;
-		scb_s->itdba = 0;
-	}
+	kvm_vcpu_unmap(vcpu, &vsie_page->itdba_map);
+	scb_s->itdba = 0;
 
-	hpa = scb_s->gvrd;
-	if (hpa) {
-		unpin_guest_page(vcpu->kvm, vsie_page->gvrd_gpa, hpa);
-		vsie_page->gvrd_gpa = 0;
-		scb_s->gvrd = 0;
-	}
+	kvm_vcpu_unmap(vcpu, &vsie_page->gvrd_map);
+	scb_s->gvrd = 0;
 
-	hpa = scb_s->riccbd;
-	if (hpa) {
-		unpin_guest_page(vcpu->kvm, vsie_page->riccbd_gpa, hpa);
-		vsie_page->riccbd_gpa = 0;
-		scb_s->riccbd = 0;
-	}
+	kvm_vcpu_unmap(vcpu, &vsie_page->riccbd_map);
+	scb_s->riccbd = 0;
 
-	hpa = scb_s->sdnxo;
-	if (hpa) {
-		unpin_guest_page(vcpu->kvm, vsie_page->sdnx_gpa, hpa);
-		vsie_page->sdnx_gpa = 0;
-		scb_s->sdnxo = 0;
-	}
+	kvm_vcpu_unmap(vcpu, &vsie_page->sdnx_map);
+	scb_s->sdnxo = 0;
 }
 
 /*
@@ -783,13 +757,12 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 			 ((gpa + offsetof(struct bsca_block, cpu[0]) - 1) & PAGE_MASK))
 			rc = set_validity_icpt(scb_s, 0x003bU);
 		if (!rc) {
-			rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
-			if (rc)
+			hpa = pin_guest_page(vcpu, gpa, &vsie_page->sca_map);
+			if (!hpa)
 				rc = set_validity_icpt(scb_s, 0x0034U);
 		}
 		if (rc)
 			goto unpin;
-		vsie_page->sca_gpa = gpa;
 		scb_s->scaoh = (u32)((u64)hpa >> 32);
 		scb_s->scaol = (u32)(u64)hpa;
 	}
@@ -801,12 +774,11 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 			goto unpin;
 		}
 		/* 256 bytes cannot cross page boundaries */
-		rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
-		if (rc) {
+		hpa = pin_guest_page(vcpu, gpa, &vsie_page->itdba_map);
+		if (!hpa) {
 			rc = set_validity_icpt(scb_s, 0x0080U);
 			goto unpin;
 		}
-		vsie_page->itdba_gpa = gpa;
 		scb_s->itdba = hpa;
 	}
 
@@ -820,12 +792,11 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 		 * 512 bytes vector registers cannot cross page boundaries
 		 * if this block gets bigger, we have to shadow it.
 		 */
-		rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
-		if (rc) {
+		hpa = pin_guest_page(vcpu, gpa, &vsie_page->gvrd_map);
+		if (!hpa) {
 			rc = set_validity_icpt(scb_s, 0x1310U);
 			goto unpin;
 		}
-		vsie_page->gvrd_gpa = gpa;
 		scb_s->gvrd = hpa;
 	}
 
@@ -836,13 +807,12 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 			goto unpin;
 		}
 		/* 64 bytes cannot cross page boundaries */
-		rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
-		if (rc) {
+		hpa = pin_guest_page(vcpu, gpa, &vsie_page->riccbd_map);
+		if (!hpa) {
 			rc = set_validity_icpt(scb_s, 0x0043U);
 			goto unpin;
 		}
 		/* Validity 0x0044 will be checked by SIE */
-		vsie_page->riccbd_gpa = gpa;
 		scb_s->riccbd = hpa;
 	}
 	if (((scb_s->ecb & ECB_GS) && !(scb_s->ecd & ECD_HOSTREGMGMT)) ||
@@ -866,12 +836,11 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 		/* Due to alignment rules (checked above) this cannot
 		 * cross page boundaries
 		 */
-		rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
-		if (rc) {
+		hpa = pin_guest_page(vcpu, gpa, &vsie_page->sdnx_map);
+		if (!hpa) {
 			rc = set_validity_icpt(scb_s, 0x10b0U);
 			goto unpin;
 		}
-		vsie_page->sdnx_gpa = gpa;
 		scb_s->sdnxo = hpa | sdnxc;
 	}
 	return 0;
@@ -881,13 +850,9 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 }
 
 /* unpin the scb provided by guest 2, marking it as dirty */
-static void unpin_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page,
-		      gpa_t gpa)
+static void unpin_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 {
-	hpa_t hpa = virt_to_phys(vsie_page->scb_o);
-
-	if (hpa)
-		unpin_guest_page(vcpu->kvm, gpa, hpa);
+	kvm_vcpu_unmap(vcpu, &vsie_page->scb_map);
 	vsie_page->scb_o = NULL;
 }
 
@@ -901,12 +866,10 @@ static int pin_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page,
 		   gpa_t gpa)
 {
 	hpa_t hpa;
-	int rc;
 
-	rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
-	if (rc) {
-		rc = kvm_s390_inject_program_int(vcpu, PGM_ADDRESSING);
-		WARN_ON_ONCE(rc);
+	hpa = pin_guest_page(vcpu, gpa, &vsie_page->scb_map);
+	if (!hpa) {
+		WARN_ON_ONCE(kvm_s390_inject_program_int(vcpu, PGM_ADDRESSING));
 		return 1;
 	}
 	vsie_page->scb_o = phys_to_virt(hpa);
@@ -1601,7 +1564,7 @@ int kvm_s390_handle_vsie(struct kvm_vcpu *vcpu)
 out_unshadow:
 	unshadow_scb(vcpu, vsie_page);
 out_unpin_scb:
-	unpin_scb(vcpu, vsie_page, scb_addr);
+	unpin_scb(vcpu, vsie_page);
 out_put:
 	put_vsie_page(vsie_page);
 
-- 
2.56.0

[PATCH v5 6/7] KVM: s390: Introduce gmap_enable_cmm()

[Claudio Imbrenda]

Introduce gmap_enable_cmm_already_locked() and its locking wrapper
gmap_enable_cmm() to attempt to allocate all page tables of a running
guest. This means that all hugepages will be mapped with 4k pages
regardless of the hpage parameter.

Existing mappings with large pages will be split.

If any memory allocation fails, ignore the error and give up. This is
only a best effort.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Cc: stable@vger.kernel.org # 7.0+: prerequisite for the next patch
---
 arch/s390/kvm/gmap/gmap.c | 37 +++++++++++++++++++++++++++++++++++++
 arch/s390/kvm/gmap/gmap.h | 11 +++++++++++
 2 files changed, 48 insertions(+)

diff --git a/arch/s390/kvm/gmap/gmap.c b/arch/s390/kvm/gmap/gmap.c
index b9d92bef5459..e0c584ec6906 100644
--- a/arch/s390/kvm/gmap/gmap.c
+++ b/arch/s390/kvm/gmap/gmap.c
@@ -1524,3 +1524,40 @@ struct gmap *gmap_create_shadow(struct kvm_s390_mmu_cache *mc, struct gmap *pare
 	}
 	return new;
 }
+
+/* Bail out for any allocation failure. This is only a best effort to allocate all page tables. */
+void gmap_enable_cmm_already_locked(struct kvm_s390_mmu_cache *mc, struct gmap *gmap)
+{
+	struct kvm_memory_slot *ms;
+	struct kvm_memslots *slots;
+	gfn_t gfn, start, end;
+	union crste *crstep;
+	union pte *ptep;
+	int flags, bkt;
+
+	if (uses_cmm(gmap))
+		return;
+
+	set_bit(GMAP_FLAG_USES_CMM, &gmap->flags);
+
+	slots = kvm_memslots(gmap->kvm);
+	if (!slots || kvm_memslots_empty(slots))
+		return;
+
+	flags = DAT_WALK_ALLOC | DAT_WALK_SPLIT | (uses_skeys(gmap) ? DAT_WALK_USES_SKEYS : 0);
+
+	kvm_for_each_memslot(ms, bkt, slots) {
+		start = ALIGN(ms->base_gfn, _PAGE_ENTRIES);
+		end = ALIGN_DOWN(ms->base_gfn + ms->npages, _PAGE_ENTRIES);
+
+		for (gfn = start; gfn < end; gfn += _PAGE_ENTRIES) {
+			if (kvm_s390_mmu_cache_topup(mc))
+				return;
+
+			guard(write_lock)(&gmap->kvm->mmu_lock);
+
+			dat_entry_walk(mc, gfn, gmap->asce, flags, TABLE_TYPE_PAGE_TABLE,
+				       &crstep, &ptep);
+		}
+	}
+}
diff --git a/arch/s390/kvm/gmap/gmap.h b/arch/s390/kvm/gmap/gmap.h
index 8f47f29c89fd..226fb6595fe4 100644
--- a/arch/s390/kvm/gmap/gmap.h
+++ b/arch/s390/kvm/gmap/gmap.h
@@ -113,6 +113,7 @@ void _gmap_handle_vsie_unshadow_event(struct gmap *parent, gfn_t gfn);
 struct gmap *gmap_create_shadow(struct kvm_s390_mmu_cache *mc, struct gmap *gmap,
 				union asce asce, int edat_level);
 void gmap_split_huge_pages(struct gmap *gmap);
+void gmap_enable_cmm_already_locked(struct kvm_s390_mmu_cache *mc, struct gmap *gmap);
 
 static inline bool uses_skeys(struct gmap *gmap)
 {
@@ -345,4 +346,14 @@ static inline bool gmap_is_shadow_valid(struct gmap *sg, union asce asce, int ed
 	return sg->guest_asce.val == asce.val && sg->edat_level == edat_level;
 }
 
+static inline void gmap_enable_cmm(struct kvm_s390_mmu_cache *mc, struct gmap *gmap)
+{
+	/* CMMA already enabled, nothing to do */
+	if (uses_cmm(gmap))
+		return;
+
+	guard(mutex)(&gmap->kvm->slots_arch_lock);
+
+	gmap_enable_cmm_already_locked(mc, gmap);
+}
 #endif /* ARCH_KVM_GMAP_GMAP_H */
-- 
2.56.0

[PATCH v5 7/7] KVM: s390: Allocate page tables when CMMA is used

[Claudio Imbrenda]

When ESSA is executed in the guest for the first time, allocate all
page tables and split all existing large page mappings, regardless of
the hpage parameter.

With the page tables present (even if all the entries within are not
present), ESSA will not cause page faults.

Without this fix, ESSA will cause a fault on the first page of each
segment (1M block). If userspace is backing the guest with huge pages
(hugetlbfs or THP), the page fault will be resolved by faulting in the
whole huge page. The result is that at startup the whole guest memory
is faulted in, since the guest will mark all of its memory at boot.

With this fix, no extra guest memory is allocated. This will also
prevent the first page in each segment from being faulted in, regardless
of hugepages.

Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/s390/priv.c | 8 ++++----
 arch/s390/kvm/s390/s390.c | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/s390/kvm/s390/priv.c b/arch/s390/kvm/s390/priv.c
index 856939acf012..f7f49c2338b6 100644
--- a/arch/s390/kvm/s390/priv.c
+++ b/arch/s390/kvm/s390/priv.c
@@ -1241,14 +1241,14 @@ static int handle_essa(struct kvm_vcpu *vcpu)
 	if (!READ_ONCE(vcpu->kvm->arch.migration_mode)) {
 		/*
 		 * CMMA is enabled in the KVM settings, but is disabled in
-		 * the SIE block and in the mm_context, and we are not doing
-		 * a migration. Enable CMMA in the mm_context.
-		 * Since we need to take a write lock to write to the context
+		 * the SIE block and in the gmap, and we are not doing a
+		 * migration. Enable CMMA in the gmap.
+		 * Since we need to take a mutex to write to the gmap
 		 * to avoid races with storage keys handling, we check if the
 		 * value really needs to be written to; if the value is
 		 * already correct, we do nothing and avoid the lock.
 		 */
-		set_bit(GMAP_FLAG_USES_CMM, &vcpu->arch.gmap->flags);
+		gmap_enable_cmm(vcpu->arch.mc, vcpu->arch.gmap);
 		/*
 		 * If we are here, we are supposed to have CMMA enabled in
 		 * the SIE block. Enabling CMMA works on a per-CPU basis,
diff --git a/arch/s390/kvm/s390/s390.c b/arch/s390/kvm/s390/s390.c
index 18fb200f854a..23be83c0c0d8 100644
--- a/arch/s390/kvm/s390/s390.c
+++ b/arch/s390/kvm/s390/s390.c
@@ -2349,6 +2349,8 @@ static int kvm_s390_set_cmma_bits(struct kvm *kvm,
 	if (r)
 		return -EFAULT;
 
+	gmap_enable_cmm_already_locked(mc, kvm->arch.gmap);
+
 	do {
 		r = kvm_s390_mmu_cache_topup(mc);
 		if (r)
@@ -2359,8 +2361,6 @@ static int kvm_s390_set_cmma_bits(struct kvm *kvm,
 		}
 	} while (r == -ENOMEM);
 
-	set_bit(GMAP_FLAG_USES_CMM, &kvm->arch.gmap->flags);
-
 	return r <= 0 ? r : -EFAULT;
 }
 
-- 
2.56.0

Re: [PATCH v5 1/7] KVM: s390: Fix UCONTROL inconsistencies

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: s390: _crste_ucas() collides with crste.tok in _crste_hole()
--

commit de19569965dfbfabc785467bbb1e64c29e58b5d9
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix UCONTROL inconsistencies

This commit changes the flag encoding used for mapped but unallocated UCONTROL
segment table entries so that they do not set the present bit via fc0.tl = 1,
and updates the UCAS walk and translation helpers in gmap accordingly.

> diff --git a/arch/s390/kvm/gmap/dat.h b/arch/s390/kvm/gmap/dat.h
> index 90389d47ba4e0..4665c1112bb91 100644
> --- a/arch/s390/kvm/gmap/dat.h
> +++ b/arch/s390/kvm/gmap/dat.h
> @@ -109,6 +109,8 @@ union pte {
>  #define _REGION3_FR_MASK	(_REGION3_MASK >> PAGE_SHIFT)
>  #define _PAGES_PER_SEGMENT	_PAGE_ENTRIES
>  #define _PAGES_PER_REGION3	(_PAGES_PER_SEGMENT * _CRST_ENTRIES)
> +#define _UCAS_ENTRY_LENGTH	0
> +#define _UCAS_ENTRY_OFFSET	3
>  
>  /* Soft dirty, needed as macro for atomic operations on ptes */
>  #define _PAGE_SD 0x002
> @@ -421,6 +423,17 @@ static inline union crste _crste_fc0(kvm_pfn_t pfn, int tt)
>  	return res;
>  }
>  
> +static inline union crste _crste_ucas(kvm_pfn_t pfn)
> +{
> +	union crste res = { .val = PFN_PHYS(pfn) };
> +
> +	res.h.i = 1;
> +	res.h.tt = TABLE_TYPE_SEGMENT;
> +	res.h.fc0.tl = _UCAS_ENTRY_LENGTH;
> +	res.h.fc0.tf = _UCAS_ENTRY_OFFSET;
> +	return res;
> +}

[Severity: High]
Can _crste_ucas() cause _crste_hole() and dat_entry_walk() to misinterpret the
upper 32 bits of PFN_PHYS(pfn) as crste.tok.type and crste.tok.par?

In union crste, setting res.h.fc0.tl = 0 leaves bit 63 (res.tok.pr) as 0 while
res.h.i is 1, and res.h.fc0.tf sits in the unnamed padding of struct tok.
Meanwhile, PFN_PHYS(pfn) occupies bits 0..51, overlapping res.tok.type (bits
0..15) and res.tok.par (bits 16..31).

Because _crste_hole() and _crste_none() do not check crste_is_ucas():

arch/s390/kvm/gmap/dat.h:_crste_hole() {
	return crste.h.i && !crste.tok.pr && crste.tok.type != _DAT_TOKEN_NONE;
}

arch/s390/kvm/gmap/dat.c:dat_entry_walk() {
	...
	if (crste_hole(entry) && !ign_holes)
		return entry.tok.type == _DAT_TOKEN_PIC ? entry.tok.par : -EFAULT;
	...
	if (entry.pmd.h.i && !entry.pmd.h.fc) {
		if (!allocate)
			return any ? 0 : -ENOENT;
		rc = dat_split_ste(mc, &(*last)->pmd, gfn, asce, uses_skeys);
	...
}

Waiters or walks on vcpu->arch.gmap->asce without DAT_WALK_IGN_HOLES will
behave differently depending on the upper bits of the mapped userspace address
PFN_PHYS(p_gfn):

1. When PFN_PHYS(p_gfn) < (1UL << 48) (any standard 3-level or 4-level
userspace address below 256 TiB), entry.tok.type is 0 (_DAT_TOKEN_NONE), so
crste_hole(entry) is false instead of returning -EFAULT. If a walk with
DAT_WALK_ALLOC is triggered (such as handle_sske() or handle_pfmf() ->
dat_cond_set_storage_key() -> dat_entry_walk()), dat_entry_walk() calls
dat_split_ste() on the UCAS entry. That allocates a struct page_table in the
child gmap (which leaks on unmap/dispose because GMAP_FLAG_OWNS_PAGETABLES is
not set on UCONTROL vCPU gmaps) and zeroes its PGSTEs, leaving PTVAL_VMADDR as
0 and causing subsequent gmap_ucas_translate() calls to translate the guest
segment to host address 0.

2. When 0x0001000000000000UL <= PFN_PHYS(p_gfn) < 0x0002000000000000UL (on
5-level page tables), entry.tok.type is 1 (_DAT_TOKEN_PIC). When entry.tok.par
is non-zero, dat_entry_walk() returns those address bits as a positive program
interruption code instead of -EFAULT.

3. When entry.tok.type is _DAT_TOKEN_PIC and entry.tok.par is 0 (for example,
PFN_PHYS(p_gfn) == 0x0001000000000000UL), dat_entry_walk() returns 0 while
leaving *ptepp as NULL. In handle_essa() -> __do_essa() -> dat_perform_essa(),
this leads to a NULL pointer dereference in pgste_get_lock(ptep):

arch/s390/kvm/gmap/dat.c:dat_perform_essa() {
	...
	if (dat_entry_walk(NULL, gfn, asce, 0, TABLE_TYPE_PAGE_TABLE, &crstep, &ptep)) {
		*state = (union essa_state) { .exception = 1 };
		return -1;
	}

	pgste = pgste_get_lock(ptep);
}

And in dat_cond_set_storage_key(), returning 0 with ptep == NULL on a non-leaf
UCAS crste causes an infinite loop at goto again under read_lock(&kvm->mmu_lock).

Should _crste_hole() and _crste_none() (or dat_entry_walk()) explicitly account
for crste_is_ucas() so that unallocated UCAS entries are consistently treated
as non-PIC holes when DAT_WALK_IGN_HOLES is not set?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029130044.125032-1-imbrenda@linux.ibm.com?part=1

Re: [PATCH v5 7/7] KVM: s390: Allocate page tables when CMMA is used

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] KVM: s390: missing kvm->srcu lock in kvm_s390_set_cmma_bits()
- [High] KVM: s390: gmap_enable_cmm_already_locked() leaves hugepage flags enabled
- [Medium] KVM: s390: premature GMAP_FLAG_USES_CMM in gmap_enable_cmm_already_locked()
--

commit a6917c9210f677a52613d12ec2b02b039accdef5
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Allocate page tables when CMMA is used

This commit replaces direct GMAP_FLAG_USES_CMM bit setting in handle_essa()
and kvm_s390_set_cmma_bits() with calls to gmap_enable_cmm() and
gmap_enable_cmm_already_locked() so that all page tables are allocated and
existing large page mappings are split when CMMA is enabled.

> diff --git a/arch/s390/kvm/s390/priv.c b/arch/s390/kvm/s390/priv.c
> index 856939acf012f..f7f49c2338b6f 100644
> --- a/arch/s390/kvm/s390/priv.c
> +++ b/arch/s390/kvm/s390/priv.c
> @@ -1241,14 +1241,14 @@ static int handle_essa(struct kvm_vcpu *vcpu)
>  	if (!READ_ONCE(vcpu->kvm->arch.migration_mode)) {
>  		/*
>  		 * CMMA is enabled in the KVM settings, but is disabled in
> -		 * the SIE block and in the mm_context, and we are not doing
> -		 * a migration. Enable CMMA in the mm_context.
> -		 * Since we need to take a write lock to write to the context
> +		 * the SIE block and in the gmap, and we are not doing a
> +		 * migration. Enable CMMA in the gmap.
> +		 * Since we need to take a mutex to write to the gmap
>  		 * to avoid races with storage keys handling, we check if the
>  		 * value really needs to be written to; if the value is
>  		 * already correct, we do nothing and avoid the lock.
>  		 */
> -		set_bit(GMAP_FLAG_USES_CMM, &vcpu->arch.gmap->flags);
> +		gmap_enable_cmm(vcpu->arch.mc, vcpu->arch.gmap);

[Severity: Medium]
Can a second vCPU racing in handle_essa() observe GMAP_FLAG_USES_CMM before
gmap_enable_cmm_already_locked() finishes allocating page tables and splitting
large pages?

In arch/s390/kvm/gmap/gmap.h, gmap_enable_cmm() checks uses_cmm(gmap) before
taking slots_arch_lock:

    static inline void gmap_enable_cmm(struct kvm_s390_mmu_cache *mc, struct gmap *gmap)
    {
        /* CMMA already enabled, nothing to do */
        if (uses_cmm(gmap))
            return;

        guard(mutex)(&gmap->kvm->slots_arch_lock);

        gmap_enable_cmm_already_locked(mc, gmap);
    }

However, in arch/s390/kvm/gmap/gmap.c, gmap_enable_cmm_already_locked() sets
GMAP_FLAG_USES_CMM at the very beginning, before iterating over the memslots:

    if (uses_cmm(gmap))
        return;

    set_bit(GMAP_FLAG_USES_CMM, &gmap->flags);

    slots = kvm_memslots(gmap->kvm);
    ...
    kvm_for_each_memslot(ms, bkt, slots) {
        ...
    }

While the first vCPU is still walking memslots in
gmap_enable_cmm_already_locked(), another vCPU intercepting on ESSA will see
uses_cmm(gmap) as true in gmap_enable_cmm(), skip slots_arch_lock, set
ECB2_CMMA in its SIE control block, and retry ESSA in hardware against
segments whose page tables have not yet been allocated or split.

Should GMAP_FLAG_USES_CMM be set only after the memslot walk completes (with
store-release / load-acquire ordering against the lockless uses_cmm() check)?

[Severity: High]
Also in gmap_enable_cmm_already_locked(), what happens if
GMAP_FLAG_ALLOW_HPAGE_1M or GMAP_FLAG_ALLOW_HPAGE_2G is set on gmap->flags?

When gmap_enable_cmm_already_locked() runs, it sets GMAP_FLAG_USES_CMM and
walks existing memslots with DAT_WALK_ALLOC | DAT_WALK_SPLIT, but unlike
kvm_s390_pv_init_vm(), it does not clear GMAP_FLAG_ALLOW_HPAGE_1M or
GMAP_FLAG_ALLOW_HPAGE_2G, and gmap_1m_allowed() / gmap_2g_allowed() do not
check uses_cmm(gmap).

If a memslot is later created or moved, or if gmap_enable_cmm_already_locked()
returns early when kvm_s390_mmu_cache_topup() fails, subsequent faults in
gmap_link() will still see gmap_1m_allowed() or gmap_2g_allowed() as true and
install 1M or 2G mappings without PGSTEs while CMMA is active:

    if (order >= get_order(_REGION3_SIZE) && gmap_2g_allowed(gmap, f, slot))
        level = TABLE_TYPE_REGION3;
    else if (order >= get_order(_SEGMENT_SIZE) && gmap_1m_allowed(gmap, f, slot))
        level = TABLE_TYPE_SEGMENT;

In addition, if GMAP_FLAG_ALLOW_HPAGE_2G remains set after a 2G PUD is split
into a segment table (either while mmu_lock is dropped between 1M iterations
or after an early -ENOMEM return from kvm_s390_mmu_cache_topup()), a fault on
an unvisited PMD will call _gmap_link() with level == TABLE_TYPE_REGION3. In
dat_entry_walk():

    if (walk_level == TABLE_TYPE_REGION3 &&
        continue_anyway && !entry.pud.h.fc && !entry.h.i) {
        walk_level = TABLE_TYPE_PAGE_TABLE;
        allocate = false;
    }

Because allocate and split are false at the segment level, dat_entry_walk()
will fail with -ENOENT or -EFBIG on an unallocated or unsplit PMD, causing
_gmap_link() to return -EAGAIN and kvm_s390_faultin_gfn() to loop.

Should gmap_enable_cmm_already_locked() clear GMAP_FLAG_ALLOW_HPAGE_1M and
GMAP_FLAG_ALLOW_HPAGE_2G (or should gmap_1m_allowed() and gmap_2g_allowed()
reject huge pages when uses_cmm(gmap) is true)?

> diff --git a/arch/s390/kvm/s390/s390.c b/arch/s390/kvm/s390/s390.c
> index 18fb200f854a8..23be83c0c0d8f 100644
> --- a/arch/s390/kvm/s390/s390.c
> +++ b/arch/s390/kvm/s390/s390.c
> @@ -2349,6 +2349,8 @@ static int kvm_s390_set_cmma_bits(struct kvm *kvm,
>  	if (r)
>  		return -EFAULT;
>  
> +	gmap_enable_cmm_already_locked(mc, kvm->arch.gmap);

[Severity: High]
Does calling gmap_enable_cmm_already_locked() from kvm_s390_set_cmma_bits()
violate the SRCU locking requirement for kvm_memslots()?

In kvm_arch_vm_ioctl(), the KVM_S390_SET_CMMA_BITS path only acquires
kvm->slots_arch_lock before calling kvm_s390_set_cmma_bits():

    case KVM_S390_SET_CMMA_BITS: {
        ...
        mutex_lock(&kvm->slots_arch_lock);
        r = kvm_s390_set_cmma_bits(kvm, &args);
        mutex_unlock(&kvm->slots_arch_lock);
        break;
    }

When uses_cmm(gmap) is false, gmap_enable_cmm_already_locked() calls
kvm_memslots(gmap->kvm) and iterates the memslots with kvm_for_each_memslot():

    slots = kvm_memslots(gmap->kvm);
    if (!slots || kvm_memslots_empty(slots))
        return;

    flags = DAT_WALK_ALLOC | DAT_WALK_SPLIT | (uses_skeys(gmap) ? DAT_WALK_USES_SKEYS : 0);

    kvm_for_each_memslot(ms, bkt, slots) {

In include/linux/kvm_host.h, __kvm_memslots() requires either kvm->srcu or
kvm->slots_lock to be held:

    return srcu_dereference_check(kvm->memslots[as_id], &kvm->srcu,
            lockdep_is_held(&kvm->slots_lock) ||
            !refcount_read(&kvm->users_count));

Because neither kvm->srcu nor kvm->slots_lock is held on this ioctl path,
won't this trigger a suspicious RCU usage warning under CONFIG_PROVE_RCU (just
as kvm_s390_vm_set_migration() takes guard(srcu)(&kvm->srcu) outside
guard(mutex)(&kvm->slots_arch_lock) before calling
gmap_enable_cmm_already_locked())?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029130044.125032-1-imbrenda@linux.ibm.com?part=7

Re: [PATCH v5 6/7] KVM: s390: Introduce gmap_enable_cmm()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] KVM: s390: missing srcu lock in gmap_enable_cmm_already_locked()
- [High] KVM: s390: gmap_enable_cmm_already_locked() fails to clear hpage flags
- [Medium] KVM: s390: premature GMAP_FLAG_USES_CMM in gmap_enable_cmm_already_locked()
- [Medium] KVM: s390: stale uses_skeys() cached in gmap_enable_cmm_already_locked()
--

commit 4b6cb7d4ca0dbeda9874449b9419ff9b5958ae26
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Introduce gmap_enable_cmm()

This commit introduces gmap_enable_cmm_already_locked() and its locking
wrapper gmap_enable_cmm() to allocate all 4k page tables of a running guest
and split existing large page mappings on a best-effort basis when enabling
CMMA.

> diff --git a/arch/s390/kvm/gmap/gmap.c b/arch/s390/kvm/gmap/gmap.c
> index b9d92bef5459d..e0c584ec69069 100644
> --- a/arch/s390/kvm/gmap/gmap.c
> +++ b/arch/s390/kvm/gmap/gmap.c
> @@ -1524,3 +1524,40 @@ struct gmap *gmap_create_shadow(struct kvm_s390_mmu_cache *mc, struct gmap *pare
>  	}
>  	return new;
>  }
> +
> +/* Bail out for any allocation failure. This is only a best effort to allocate all page tables. */
> +void gmap_enable_cmm_already_locked(struct kvm_s390_mmu_cache *mc, struct gmap *gmap)
> +{
> +	struct kvm_memory_slot *ms;
> +	struct kvm_memslots *slots;
> +	gfn_t gfn, start, end;
> +	union crste *crstep;
> +	union pte *ptep;
> +	int flags, bkt;
> +
> +	if (uses_cmm(gmap))
> +		return;
> +
> +	set_bit(GMAP_FLAG_USES_CMM, &gmap->flags);

[Severity: Medium]
Should GMAP_FLAG_USES_CMM be set only after the memslot walk finishes?

gmap_enable_cmm() in arch/s390/kvm/gmap/gmap.h checks uses_cmm(gmap) before
acquiring gmap->kvm->slots_arch_lock:

arch/s390/kvm/gmap/gmap.h:gmap_enable_cmm() {
	if (uses_cmm(gmap))
		return;

	guard(mutex)(&gmap->kvm->slots_arch_lock);

	gmap_enable_cmm_already_locked(mc, gmap);
}

Because gmap_enable_cmm_already_locked() sets GMAP_FLAG_USES_CMM before
walking the memslots, and then drops gmap->kvm->mmu_lock and calls
kvm_s390_mmu_cache_topup() on every _PAGE_ENTRIES iteration, a concurrent
caller of gmap_enable_cmm() (such as another vCPU in handle_essa()) or a
reader of uses_cmm(gmap) (such as the KVM_REQ_STOP_MIGRATION handler) can
observe uses_cmm(gmap) as true and proceed while the first caller is still
allocating 4k page tables and splitting large pages.

In addition, if slots are empty or kvm_s390_mmu_cache_topup(mc) fails early,
GMAP_FLAG_USES_CMM remains permanently set, preventing any later call to
gmap_enable_cmm() or gmap_enable_cmm_already_locked() from retrying.

[Severity: High]
Also, what prevents gmap_link() from continuing to request 1M or 2G mappings
after GMAP_FLAG_USES_CMM is set if GMAP_FLAG_ALLOW_HPAGE_1M or
GMAP_FLAG_ALLOW_HPAGE_2G was enabled on gmap->flags?

Unlike kvm_s390_pv_init_vm(), gmap_enable_cmm_already_locked() does not clear
GMAP_FLAG_ALLOW_HPAGE_1M or GMAP_FLAG_ALLOW_HPAGE_2G on gmap->flags, and
gmap_1m_allowed() / gmap_2g_allowed() do not check !uses_cmm(gmap):

arch/s390/kvm/gmap/gmap.c:gmap_link() {
	if (f->page) {
		order = folio_order(page_folio(f->page));
		if (order >= get_order(_REGION3_SIZE) && gmap_2g_allowed(gmap, f, slot))
			level = TABLE_TYPE_REGION3;
		else if (order >= get_order(_SEGMENT_SIZE) && gmap_1m_allowed(gmap, f, slot))
			level = TABLE_TYPE_SEGMENT;
	}
	return _gmap_link(mc, gmap, level, f);
}

If GMAP_FLAG_ALLOW_HPAGE_2G is set on gmap->flags and
gmap_enable_cmm_already_locked() splits a 2G region3 entry into a segment
table on the first _PAGE_ENTRIES step, but then bails out on a subsequent
step in the same 2G range when kvm_s390_mmu_cache_topup(mc) fails (or races
with a fault while mmu_lock is dropped between iterations), the remaining
segment entries in that 2G range are still empty (i = 1) or 1M mappings
(fc = 1).

When a fault subsequently runs gmap_link() -> _gmap_link() ->
dat_entry_walk() with walk_level == TABLE_TYPE_REGION3 and
DAT_WALK_ALLOC_CONTINUE, dat_entry_walk() sees the split region3 table entry
and clears allocate:

arch/s390/kvm/gmap/dat.c:dat_entry_walk() {
		if (walk_level == TABLE_TYPE_REGION3 &&
		    continue_anyway && !entry.pud.h.fc && !entry.h.i) {
			walk_level = TABLE_TYPE_PAGE_TABLE;
			allocate = false;
		}
}

At the segment level, dat_entry_walk() then returns -ENOENT (for empty
segment entries because allocate is false) or -EFBIG (for 1M segment entries
because split is false), which _gmap_link() translates into -EAGAIN, causing
kvm_s390_faultin_gfn() to retry in an unbounded loop.

Similarly, if kvm_s390_mmu_cache_topup(mc) fails before a region is split, or
if a memslot is later created or moved via
s390_kvm_mmu_prepare_memory_region() -> dat_create_slot(), subsequent faults
will still see GMAP_FLAG_ALLOW_HPAGE_1M or GMAP_FLAG_ALLOW_HPAGE_2G set and
install 1M or 2G large mappings without PGSTEs.

> +
> +	slots = kvm_memslots(gmap->kvm);
> +	if (!slots || kvm_memslots_empty(slots))
> +		return;
> +
> +	flags = DAT_WALK_ALLOC | DAT_WALK_SPLIT | (uses_skeys(gmap) ? DAT_WALK_USES_SKEYS : 0);
> +
> +	kvm_for_each_memslot(ms, bkt, slots) {

[Severity: High]
Can kvm_memslots(gmap->kvm) and kvm_for_each_memslot() be called here without
holding gmap->kvm->srcu or gmap->kvm->slots_lock?

kvm_memslots() calls __kvm_memslots(), which checks that either kvm->srcu or
kvm->slots_lock is held:

include/linux/kvm_host.h:__kvm_memslots() {
	as_id = array_index_nospec(as_id, KVM_MAX_NR_ADDRESS_SPACES);
	return srcu_dereference_check(kvm->memslots[as_id], &kvm->srcu,
			lockdep_is_held(&kvm->slots_lock) ||
			!refcount_read(&kvm->users_count));
}

gmap_enable_cmm() only acquires gmap->kvm->slots_arch_lock, and callers such
as KVM_S390_SET_CMMA_BITS -> kvm_s390_set_cmma_bits() also hold only
kvm->slots_arch_lock without acquiring kvm->srcu.

Holding slots_arch_lock alone does not satisfy srcu_dereference_check() or
protect kvm->memslots against concurrent memslot updates (unlike
kvm_s390_vm_set_migration(), which acquires both guard(srcu)(&kvm->srcu) and
guard(mutex)(&kvm->slots_arch_lock)). Should gmap_enable_cmm_already_locked()
acquire guard(srcu)(&gmap->kvm->srcu)?

> +		start = ALIGN(ms->base_gfn, _PAGE_ENTRIES);
> +		end = ALIGN_DOWN(ms->base_gfn + ms->npages, _PAGE_ENTRIES);
> +
> +		for (gfn = start; gfn < end; gfn += _PAGE_ENTRIES) {
> +			if (kvm_s390_mmu_cache_topup(mc))
> +				return;
> +
> +			guard(write_lock)(&gmap->kvm->mmu_lock);
> +
> +			dat_entry_walk(mc, gfn, gmap->asce, flags, TABLE_TYPE_PAGE_TABLE,
> +				       &crstep, &ptep);

[Severity: Medium]
Could evaluating uses_skeys(gmap) only once before the memslot loop leave
flags with a stale value across mmu_lock drops?

gmap_enable_skeys() -> _gmap_enable_skeys() sets GMAP_FLAG_USES_SKEYS under
mmap_write_lock(gmap->kvm->mm) and gmap->kvm->mmu_lock without acquiring
gmap->kvm->slots_arch_lock.

If a concurrent vCPU executing a storage-key instruction or a
KVM_S390_SET_SKEYS ioctl enables GMAP_FLAG_USES_SKEYS while
gmap_enable_cmm_already_locked() has dropped gmap->kvm->mmu_lock between
iterations, subsequent dat_entry_walk() calls in this loop will still pass
flags without DAT_WALK_USES_SKEYS.

When dat_split_ste() splits an invalidated large PMD (old.h.fc == 1 and
old.h.i == 1) with uses_skeys == false:

arch/s390/kvm/gmap/dat.c:dat_split_ste() {
			pgste_init.pcl = uses_skeys && init.h.i;
			dat_init_pgstes(pt, pgste_init.val);
		} else {
			dat_init_page_table(pt, init.val, 0);
		}

		if (dat_pmdp_xchg_atomic(pmdp, old, new, gfn, asce)) {
			if (!pgste_init.pcl)
				return 0;
			for (i = 0; i < _PAGE_ENTRIES; i++) {
				union pgste pgste = pt->pgstes[i];

				pgste = dat_save_storage_key_into_pgste(pt->ptes[i], pgste);
				pgste_set_unlock(pt->ptes + i, pgste);
			}
			return 0;
		}
}

pgste_init.pcl will be false and dat_save_storage_key_into_pgste() will be
skipped, leaving the new PGSTEs zeroed so that subsequent storage key reads
or minor faults lose the guest storage keys on those pages.

Should uses_skeys(gmap) be checked inside the write_lock(&gmap->kvm->mmu_lock)
critical section on each iteration?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029130044.125032-1-imbrenda@linux.ibm.com?part=6