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

[PATCH v4 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.

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_allocate_all_page_tables_best_effort()
  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 |   1 +
 arch/s390/kvm/s390/priv.c |   5 +-
 arch/s390/kvm/s390/s390.c |   5 +-
 arch/s390/kvm/s390/vsie.c | 135 ++++++++---------------
 7 files changed, 291 insertions(+), 156 deletions(-)

-- 
2.56.0

[PATCH v4 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

Re: [PATCH v4 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() entries are misclassified by _crste_hole()
--

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

KVM: s390: Fix UCONTROL inconsistencies

This patch updates mapped-but-unallocated UCONTROL segment entries to use a
new combination of flags (_UCAS_ENTRY_LENGTH and _UCAS_ENTRY_OFFSET) so they
no longer collide with the present bit, and adds DAT_WALK_IGN_HOLES to the
UCAS walk paths.

> 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() entries be misclassified by _crste_hole() and
dat_entry_walk() when walking a UCONTROL gmap without DAT_WALK_IGN_HOLES?

In _crste_ucas(), PFN_PHYS(pfn) is stored in bits 0..51 of res.val while
res.h.fc0.tl = 0 (clearing tok.pr) and res.h.fc0.tf = 3 (in bits 56..57).
However, _crste_hole() and _crste_none() only inspect crste.tok.type (bits
0..15 of res.val) and do not check crste_is_ucas():

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

static inline bool _crste_none(union crste crste)
{
	return crste.h.i && !crste.tok.pr && crste.tok.type == _DAT_TOKEN_NONE;
}

And in dat_entry_walk():

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);
	...
}

Because crste.tok.type overlaps (PFN_PHYS(pfn) >> 48) & 0xffff and
crste.tok.par overlaps (PFN_PHYS(pfn) >> 32) & 0xffff, two problems can
occur when a caller walks vcpu->arch.gmap->asce without DAT_WALK_IGN_HOLES
(for example via KVM_S390_MEM_OP -> vcpu_check_access_key_gpa() ->
dat_get_storage_key(), or storage-key and CMMA handlers such as
handle_sske(), handle_pfmf(), or dat_perform_essa()):

1. When PFN_PHYS(pfn) < (1UL << 48) (256 TiB, which is true for all 3-level
   s390x user address spaces), crste.tok.type is 0 (_DAT_TOKEN_NONE).
   _crste_hole() returns false instead of treating the UCAS entry as a hole.
   If dat_entry_walk() was called with DAT_WALK_ALLOC (for instance via
   dat_set_storage_key(), dat_cond_set_storage_key(), or
   dat_set_cmma_bits()), it falls through to dat_split_ste(), overwriting
   the UCAS segment entry with a newly allocated page table that has
   PTVAL_VMADDR == 0, wasn't allocated in gmap->parent->asce, and is leaked
   when the child gmap is destroyed because GMAP_FLAG_OWNS_PAGETABLES is not
   set on vcpu->arch.gmap.

2. When (PFN_PHYS(pfn) >> 48) == _DAT_TOKEN_PIC (1, i.e. 256 TiB <=
   user_addr < 512 TiB under 5-level page tables), _crste_hole() returns
   true, but dat_entry_walk() sees entry.tok.type == _DAT_TOKEN_PIC and
   returns entry.tok.par (bits 16..31 of the user address) instead of
   -EFAULT. Callers interpret positive return values as a guest Program
   Interruption Code, or if bits 16..31 happen to be 0, dat_entry_walk()
   returns 0 with *ptepp == NULL, causing callers like dat_perform_essa()
   to dereference a NULL ptep.

Should _crste_hole() (and _crste_none()) account for crste_is_ucas(crste),
or should dat_entry_walk() explicitly check for crste_is_ucas(entry) before
inspecting entry.tok.type?

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

[PATCH v4 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

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

[sashiko-bot]

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

Sashiko has reviewed this patch and found no issues.

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

[PATCH v4 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

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

[sashiko-bot]

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

Sashiko has reviewed this patch and found no issues.

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

[PATCH v4 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..04f3ab03a041 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 < _REGION3_SIZE)
+		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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: s390: unit mismatch comparing gfn against _REGION3_SIZE in gmap_unmap_gfn_range_fast()
--

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

KVM: s390: Improve unmapping of large ranges

This commit speeds up unmapping of guest address ranges larger than 1M by
detaching PMD entries, clearing the detached PTEs, and reattaching the PMDs
instead of invalidating each leaf PTE individually.

> diff --git a/arch/s390/kvm/gmap/gmap.c b/arch/s390/kvm/gmap/gmap.c
> index 225fdae3d57ca..04f3ab03a041a 100644
> --- a/arch/s390/kvm/gmap/gmap.c
> +++ b/arch/s390/kvm/gmap/gmap.c
[ ... ]
> @@ -462,6 +453,129 @@ bool gmap_unmap_gfn_range(struct gmap *gmap, struct kvm_memory_slot *slot, gfn_t
[ ... ]
> +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 < _REGION3_SIZE)
> +		gmap_unmap_prefix(gmap, aligned_start, aligned_end);

[Severity: Medium]
In gmap_unmap_gfn_range_fast(), aligned_start is a gfn_t (in 4K page frames),
whereas _REGION3_SIZE is 2G in bytes (1UL << _REGION3_SHIFT, or 0x80000000).

Comparing aligned_start against _REGION3_SIZE checks whether the guest address
is below 8T rather than 2G, causing gmap_unmap_prefix() to walk all vCPUs
while holding kvm->mmu_lock in write mode for any fast-path unmap starting
between 2G and 8T.

Elsewhere in arch/s390/kvm/gmap/gmap.c, gmap_limit_to_type() shifts
_REGION3_SIZE when comparing against a gfn_t:

	if (limit <= _REGION3_SIZE >> PAGE_SHIFT)
		return TABLE_TYPE_SEGMENT;

And arch/s390/kvm/gmap/dat.h defines the region-3 page count directly:

#define _PAGES_PER_SEGMENT	_PAGE_ENTRIES
#define _PAGES_PER_REGION3	(_PAGES_PER_SEGMENT * _CRST_ENTRIES)

Should this check compare aligned_start against _PAGES_PER_REGION3 (or
_REGION3_SIZE >> PAGE_SHIFT) instead of _REGION3_SIZE?

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

[PATCH v4 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

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

[sashiko-bot]

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

Sashiko has reviewed this patch and found no issues.

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

[PATCH v4 6/7] KVM: s390: Introduce gmap_allocate_all_page_tables_best_effort()

[Claudio Imbrenda]

Introduce gmap_allocate_all_page_tables_best_effort() 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 |  1 +
 2 files changed, 38 insertions(+)

diff --git a/arch/s390/kvm/gmap/gmap.c b/arch/s390/kvm/gmap/gmap.c
index 04f3ab03a041..3ff08232cbb3 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_allocate_all_page_tables_best_effort(struct gmap *gmap)
+{
+	struct kvm_s390_mmu_cache *mc __free(kvm_s390_mmu_cache) = NULL;
+	struct kvm_memory_slot *ms;
+	struct kvm_memslots *slots;
+	gfn_t gfn, start, end;
+	union crste *crstep;
+	union pte *ptep;
+	int bkt;
+
+	mc = kvm_s390_new_mmu_cache();
+	if (!mc)
+		return;
+
+	guard(srcu)(&gmap->kvm->srcu);
+
+	slots = kvm_memslots(gmap->kvm);
+	if (!slots || kvm_memslots_empty(slots))
+		return;
+
+	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))
+				break;
+
+			guard(write_lock)(&gmap->kvm->mmu_lock);
+
+			dat_entry_walk(mc, gfn, gmap->asce, DAT_WALK_ALLOC | DAT_WALK_SPLIT,
+				       TABLE_TYPE_PAGE_TABLE, &crstep, &ptep);
+		}
+	}
+}
diff --git a/arch/s390/kvm/gmap/gmap.h b/arch/s390/kvm/gmap/gmap.h
index 8f47f29c89fd..9b5d394e9146 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_allocate_all_page_tables_best_effort(struct gmap *gmap);
 
 static inline bool uses_skeys(struct gmap *gmap)
 {
-- 
2.56.0

Re: [PATCH v4 6/7] KVM: s390: Introduce gmap_allocate_all_page_tables_best_effort()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] kvm: s390: pass DAT_WALK_USES_SKEYS to dat_entry_walk() on split
- [Low] kvm: s390: use return on kvm_s390_mmu_cache_topup() failure
--

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

KVM: s390: Introduce gmap_allocate_all_page_tables_best_effort()

This commit introduces gmap_allocate_all_page_tables_best_effort() to
walk all memslots of a running guest and allocate 4K page tables while
splitting existing large page mappings on a best-effort basis.

> diff --git a/arch/s390/kvm/gmap/gmap.c b/arch/s390/kvm/gmap/gmap.c
> index 04f3ab03a041a..3ff08232cbb36 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_allocate_all_page_tables_best_effort(struct gmap *gmap)
> +{
> +	struct kvm_s390_mmu_cache *mc __free(kvm_s390_mmu_cache) = NULL;
> +	struct kvm_memory_slot *ms;
> +	struct kvm_memslots *slots;
> +	gfn_t gfn, start, end;
> +	union crste *crstep;
> +	union pte *ptep;
> +	int bkt;
> +
> +	mc = kvm_s390_new_mmu_cache();
> +	if (!mc)
> +		return;
> +
> +	guard(srcu)(&gmap->kvm->srcu);
> +
> +	slots = kvm_memslots(gmap->kvm);
> +	if (!slots || kvm_memslots_empty(slots))
> +		return;
> +
> +	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))
> +				break;

[Severity: Low]
Should this be a return instead of break?

Using break here only exits the inner GFN loop for the current memslot,
so kvm_for_each_memslot() will continue iterating over any remaining
memslots and retrying allocations after kvm_s390_mmu_cache_topup(mc) has
failed.

> +
> +			guard(write_lock)(&gmap->kvm->mmu_lock);
> +
> +			dat_entry_walk(mc, gfn, gmap->asce, DAT_WALK_ALLOC | DAT_WALK_SPLIT,
> +				       TABLE_TYPE_PAGE_TABLE, &crstep, &ptep);

[Severity: High]
Can splitting an aged large page here lose guest storage keys when
uses_skeys(gmap) is true?

When a 1M or 2G large page is aged via gmap_age_gfn() ->
gmap_clear_young_crste(), the entry is marked invalid (h.i = 1) while
the guest storage keys for its 4K frames remain in hardware storage keys.

When dat_entry_walk() splits that invalid large PMD via dat_split_ste(),
saving the hardware storage keys into the newly allocated 4K PGSTEs is
gated on uses_skeys (from DAT_WALK_USES_SKEYS):

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);
			}
    ...
}

Because DAT_WALK_USES_SKEYS is not passed to dat_entry_walk(),
pgste_init.pcl is false and the new PGSTEs are left zeroed while the new
PTEs have h.i = 1. When a subsequent minor fault validates the PTE via
_gmap_handle_minor_pte_fault() -> gmap_ptep_xchg() -> __dat_ptep_xchg(),
uses_skeys(gmap) is true, so __dat_ptep_xchg() restores the zeroed PGSTE
over the hardware storage key:

arch/s390/kvm/gmap/dat.c:__dat_ptep_xchg() {
    ...
	if (uses_skeys) {
		if (old.h.i && !new.h.i)
			/* Invalid to valid: restore storage keys from PGSTE. */
			dat_set_storage_key_from_pgste(new, pgste);
    ...
}

Should this pass (uses_skeys(gmap) ? DAT_WALK_USES_SKEYS : 0) in the
dat_entry_walk() flags, similar to gmap_protect_rmap()?

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

[PATCH v4 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 | 5 ++++-
 arch/s390/kvm/s390/s390.c | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/s390/priv.c b/arch/s390/kvm/s390/priv.c
index 856939acf012..4f0dd3173ab9 100644
--- a/arch/s390/kvm/s390/priv.c
+++ b/arch/s390/kvm/s390/priv.c
@@ -1248,7 +1248,10 @@ static int handle_essa(struct kvm_vcpu *vcpu)
 		 * 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);
+		if (!uses_cmm(vcpu->arch.gmap)) {
+			set_bit(GMAP_FLAG_USES_CMM, &vcpu->arch.gmap->flags);
+			gmap_allocate_all_page_tables_best_effort(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..739e8c23c3ae 100644
--- a/arch/s390/kvm/s390/s390.c
+++ b/arch/s390/kvm/s390/s390.c
@@ -2359,7 +2359,10 @@ static int kvm_s390_set_cmma_bits(struct kvm *kvm,
 		}
 	} while (r == -ENOMEM);
 
-	set_bit(GMAP_FLAG_USES_CMM, &kvm->arch.gmap->flags);
+	if (!uses_cmm(kvm->arch.gmap)) {
+		set_bit(GMAP_FLAG_USES_CMM, &kvm->arch.gmap->flags);
+		gmap_allocate_all_page_tables_best_effort(kvm->arch.gmap);
+	}
 
 	return r <= 0 ? r : -EFAULT;
 }
-- 
2.56.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] KVM: s390: gmap_allocate_all_page_tables_best_effort() drops skeys on split
- [High] KVM: s390: hugepage flags left enabled when enabling CMMA in handle_essa()
- [Medium] KVM: s390: race in handle_essa() setting GMAP_FLAG_USES_CMM before allocation
- [Medium] KVM: s390: page tables allocated after dat_set_cmma_bits() in kvm_s390_set_cmma_bits()
--

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

KVM: s390: Allocate page tables when CMMA is used

This commit calls gmap_allocate_all_page_tables_best_effort() in
handle_essa() and kvm_s390_set_cmma_bits() when GMAP_FLAG_USES_CMM is first
set on the gmap, so that page tables are pre-allocated and existing large
page mappings are split when CMMA is used.

> diff --git a/arch/s390/kvm/s390/priv.c b/arch/s390/kvm/s390/priv.c
> index 856939acf012f..4f0dd3173ab91 100644
> --- a/arch/s390/kvm/s390/priv.c
> +++ b/arch/s390/kvm/s390/priv.c
> @@ -1248,7 +1248,10 @@ static int handle_essa(struct kvm_vcpu *vcpu)
>  		 * 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);
> +		if (!uses_cmm(vcpu->arch.gmap)) {
> +			set_bit(GMAP_FLAG_USES_CMM, &vcpu->arch.gmap->flags);
> +			gmap_allocate_all_page_tables_best_effort(vcpu->arch.gmap);
> +		}

[Severity: Medium]
What happens if two vCPUs in a multi-vCPU guest intercept on ESSA
concurrently in handle_essa() when !READ_ONCE(vcpu->kvm->arch.migration_mode)?

In this path only vcpu->mutex and the kvm->srcu read lock are held. When the
first vCPU sees !uses_cmm(vcpu->arch.gmap), it immediately sets
GMAP_FLAG_USES_CMM via set_bit() and then enters
gmap_allocate_all_page_tables_best_effort(), which iterates over all memslots
while repeatedly dropping gmap->kvm->mmu_lock and sleeping in
kvm_s390_mmu_cache_topup():

arch/s390/kvm/gmap/gmap.c:gmap_allocate_all_page_tables_best_effort() {
    ...
		for (gfn = start; gfn < end; gfn += _PAGE_ENTRIES) {
			if (kvm_s390_mmu_cache_topup(mc))
				break;

			guard(write_lock)(&gmap->kvm->mmu_lock);

			dat_entry_walk(mc, gfn, gmap->asce, DAT_WALK_ALLOC | DAT_WALK_SPLIT,
				       TABLE_TYPE_PAGE_TABLE, &crstep, &ptep);
		}
}

If a second vCPU enters handle_essa() while the first vCPU is still running
gmap_allocate_all_page_tables_best_effort(), the second vCPU will observe
uses_cmm(vcpu->arch.gmap) == true, skip the allocation, set
vcpu->arch.sie_block->ecb2 |= ECB2_CMMA, and retry ESSA in SIE before all
page tables have been allocated and all large pages have been split.
Conversely, because !uses_cmm() and set_bit() are not atomic or serialized by
a VM-wide lock such as kvm->slots_arch_lock, two vCPUs can both pass
!uses_cmm() and run duplicate allocation walks across all memslots.

Should the check, flag update, and page table allocation be serialized under
a VM-wide lock so other vCPUs wait until allocation finishes before re-entering
SIE with ECB2_CMMA enabled?

[Severity: Critical]
Can calling gmap_allocate_all_page_tables_best_effort() here (and in
kvm_s390_set_cmma_bits()) corrupt guest storage keys when splitting aged
1M or 2G large pages?

If storage keys are already enabled (GMAP_FLAG_USES_SKEYS) and a 1M large
page has been aged via gmap_age_gfn() -> gmap_clear_young_crste() so that
crste.h.i is set while the storage keys remain in the physical pages,
gmap_allocate_all_page_tables_best_effort() calls dat_entry_walk() without
DAT_WALK_USES_SKEYS:

arch/s390/kvm/gmap/gmap.c:gmap_allocate_all_page_tables_best_effort()
  -> dat_entry_walk(mc, gfn, gmap->asce, DAT_WALK_ALLOC | DAT_WALK_SPLIT, ...)
     -> dat_split_ste()

Inside dat_split_ste() in arch/s390/kvm/gmap/dat.c, saving the physical
storage keys into the newly allocated PGSTEs for an invalid segment entry is
gated on uses_skeys from the walk flags:

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);
			}
    ...
}

Because DAT_WALK_USES_SKEYS is not passed, pgste_init.pcl is false and the new
PGSTEs are left with zeroed storage keys while pt->ptes[i].h.i == 1. When
those 4K pages are later faulted back in via _gmap_handle_minor_pte_fault() ->
__dat_ptep_xchg() (which does pass uses_skeys(gmap)), old.h.i && !new.h.i
triggers dat_set_storage_key_from_pgste() and overwrites the guest's hardware
storage keys with zero.

Should gmap_allocate_all_page_tables_best_effort() include
(uses_skeys(gmap) ? DAT_WALK_USES_SKEYS : 0) in the flags passed to
dat_entry_walk()?

[Severity: High]
Could leaving GMAP_FLAG_ALLOW_HPAGE_1M and GMAP_FLAG_ALLOW_HPAGE_2G set on
gmap->flags when enabling CMMA lead to a guest vCPU infinite fault loop?

When userspace enables KVM_CAP_S390_HPAGE_1M or KVM_CAP_S390_HPAGE_2G and
also enables KVM_S390_VM_MEM_ENABLE_CMMA, neither handle_essa(),
kvm_s390_set_cmma_bits(), nor gmap_allocate_all_page_tables_best_effort()
clears GMAP_FLAG_ALLOW_HPAGE_1M or GMAP_FLAG_ALLOW_HPAGE_2G, and
gmap_1m_allowed() / gmap_2g_allowed() in arch/s390/kvm/gmap/gmap.c do not
check !uses_cmm(gmap):

arch/s390/kvm/gmap/gmap.c:gmap_1m_allowed() {
	return test_bit(GMAP_FLAG_ALLOW_HPAGE_1M, &gmap->flags) &&
	       !((f->gfn ^ f->pfn) & ~_SEGMENT_FR_MASK) &&
	       slot->base_gfn <= ALIGN_DOWN(f->gfn, _PAGES_PER_SEGMENT) &&
	       slot->base_gfn + slot->npages >= ALIGN(f->gfn + 1, _PAGES_PER_SEGMENT);
}

If gmap_allocate_all_page_tables_best_effort() bails out early on an
allocation failure, if another vCPU faults on a segment not yet visited while
mmu_lock is dropped between segments, or if a memslot is added or moved after
GMAP_FLAG_USES_CMM is set (where s390_kvm_mmu_prepare_memory_region() ->
dat_create_slot() installs _CRSTE_EMPTY entries without page tables),
gmap_link() will install a valid 1M or 2G large CRSTE.

When a vCPU with ECB2_CMMA set subsequently faults on that valid large page,
kvm_s390_faultin_gfn() -> gmap_try_fixup_minor() ->
gmap_handle_minor_crste_fault() sees:

arch/s390/kvm/gmap/gmap.c:gmap_handle_minor_crste_fault() {
    ...
	/* Appropriate permissions already (race with another handler), nothing to do. */
	if (!oldcrste.h.i && !(f->write_attempt && oldcrste.h.p))
		return 0;
    ...
}

and returns 0 without splitting the large page, causing the vCPU to fault
indefinitely. Should GMAP_FLAG_ALLOW_HPAGE_1M and GMAP_FLAG_ALLOW_HPAGE_2G be
cleared (or checked against uses_cmm(gmap)) before splitting existing large
pages?

> diff --git a/arch/s390/kvm/s390/s390.c b/arch/s390/kvm/s390/s390.c
> index 18fb200f854a8..739e8c23c3ae0 100644
> --- a/arch/s390/kvm/s390/s390.c
> +++ b/arch/s390/kvm/s390/s390.c
> @@ -2359,7 +2359,10 @@ static int kvm_s390_set_cmma_bits(struct kvm *kvm,
>  		}
>  	} while (r == -ENOMEM);
>  
> -	set_bit(GMAP_FLAG_USES_CMM, &kvm->arch.gmap->flags);
> +	if (!uses_cmm(kvm->arch.gmap)) {
> +		set_bit(GMAP_FLAG_USES_CMM, &kvm->arch.gmap->flags);
> +		gmap_allocate_all_page_tables_best_effort(kvm->arch.gmap);
> +	}

[Severity: Medium]
In kvm_s390_set_cmma_bits(), is it intentional that
gmap_allocate_all_page_tables_best_effort() is called after
dat_set_cmma_bits() rather than before it?

Inside dat_set_cmma_bits() in arch/s390/kvm/gmap/dat.c, dat_entry_walk() is
called under read_lock(&kvm->mmu_lock) with DAT_WALK_ALLOC and without
DAT_WALK_SPLIT:

arch/s390/kvm/gmap/dat.c:dat_set_cmma_bits() {
    ...
	for (cur = ALIGN_DOWN(gfn, _PAGE_ENTRIES); cur < gfn + count; cur += _PAGE_ENTRIES) {
		rc = dat_entry_walk(mc, cur, asce, DAT_WALK_ALLOC, TABLE_TYPE_PAGE_TABLE,
				    &crstep, &ptep);
		if (rc)
			return rc;
	}
    ...
}

If any 1M or 2G large page mappings already exist in the target GFN range
when KVM_S390_SET_CMMA_BITS is first invoked, dat_set_cmma_bits() will hit
the unsplit large page entry in dat_entry_walk() and fail with -EFBIG before
gmap_allocate_all_page_tables_best_effort() has had a chance to split it.

In addition, because the !uses_cmm(kvm->arch.gmap) block runs unconditionally
after the loop even when r != 0, any error from dat_set_cmma_bits() (such as
-EFBIG or PGM_ADDRESSING on an invalid GFN range) will still set
GMAP_FLAG_USES_CMM and allocate page tables across all memslots before
returning the error.

Should the !uses_cmm(kvm->arch.gmap) check and
gmap_allocate_all_page_tables_best_effort() happen before the
dat_set_cmma_bits() loop (or be gated on success if placed after)?

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