[PATCH v5 0/9] KVM: s390: A bunch of gmap-related fixes

[PATCH v5 0/9] KVM: s390: A bunch of gmap-related fixes

[Claudio Imbrenda]

Fix handling of _PAGE_UNUSED pte bit. The bit was lingering around
longer than it should have, causing issues. This is yet a different
approach from the previous version.

Fix some cmma dirty tracking functions.

Also fix a few other small issues in gmap.

v4-v5:
* Make sure the counter of cmma-dirty pages does not explode
* Improve comments in cmma selftest
* Rework and fix locking in kvm_s390_set_mem_control()
* Do not return success if an allocation in kvm_s390_set_cmma_bits() fails

v3->v4:
* Improve/fix descriptions of patches 1 and 3
* Fix handle_{sske,pfmf} under memory pressure
* Fix cmma dirty tracking and its selftest

v2->v3:
* Fix _PAGE_UNUSED in yet a different way
* Fix a potential overflow in dat_peek_cmma()
* Some minor gmap fixes that are not actual bugs yet

v1->v2:
* Completely different approach, entirely new patch

Claudio Imbrenda (9):
  s390/mm: Fix handling of _PAGE_UNUSED pte bit
  KVM: s390: Fix dat_peek_cmma() overflow
  KVM: s390: Do not set special large pages dirty
  KVM: s390: Fix code typo in gmap_protect_asce_top_level()
  KVM: s390: Fix handle_{sske,pfmf} under memory pressure
  KVM: s390: Fix locking in kvm_s390_set_mem_control()
  KVM: s390: Fix cmma dirty tracking
  KVM: s390: selftests: Fix cmma selftest
  KVM: s390: Return failure in case of failure in
    kvm_s390_set_cmma_bits()

 arch/s390/include/asm/pgtable.h              |  4 +--
 arch/s390/kvm/dat.c                          |  5 ++-
 arch/s390/kvm/gmap.c                         | 15 ++++++---
 arch/s390/kvm/kvm-s390.c                     | 35 ++++++++------------
 arch/s390/kvm/priv.c                         | 10 ++++--
 arch/s390/mm/gmap_helpers.c                  |  3 +-
 tools/testing/selftests/kvm/s390/cmma_test.c |  6 ++++
 7 files changed, 46 insertions(+), 32 deletions(-)

-- 
2.54.0

[PATCH v5 1/9] s390/mm: Fix handling of _PAGE_UNUSED pte bit

[Claudio Imbrenda]

The _PAGE_UNUSED softbit should not really be lying around. Its sole
purpose is to signal to try_to_unmap_one() and try_to_migrate_one()
that the page can be discarded instead of being moved / swapped.

KVM has no way to know why a page is being unmapped, so it sets the bit
on userspace ptes corresponding to unused guest pages every time they
get unmapped. KVM has no reasonable way to clear the bit once the page
is in use again.

While set_ptes() checks and clears the bit, other paths that set new
ptes did not. This led to used pages being thrown out as if they were
unused, causing guest corruption.

Fix the issue by clearing the _PAGE_UNUSED bit for present ptes in
set_pte(), i.e. whenever a present pte is getting set. The check in
set_ptes() is then redundant and can be removed.

Also fix gmap_helper_try_set_pte_unused() to only set the bit if the
pte is present; the _PAGE_UNUSED bit is only defined for present ptes
and thus should not be set for non-present ptes.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Fixes: c98175b7917f ("KVM: s390: Add gmap_helper_set_unused()")
Acked-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/s390/include/asm/pgtable.h | 4 ++--
 arch/s390/mm/gmap_helpers.c     | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
index ca376a9b8e41..d03663483f76 100644
--- a/arch/s390/include/asm/pgtable.h
+++ b/arch/s390/include/asm/pgtable.h
@@ -980,6 +980,8 @@ static inline void set_pmd(pmd_t *pmdp, pmd_t pmd)
 
 static inline void set_pte(pte_t *ptep, pte_t pte)
 {
+	if (pte_present(pte))
+		pte = clear_pte_bit(pte, __pgprot(_PAGE_UNUSED));
 	WRITE_ONCE(*ptep, pte);
 }
 
@@ -1332,8 +1334,6 @@ pgprot_t pgprot_writecombine(pgprot_t prot);
 static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
 			      pte_t *ptep, pte_t entry, unsigned int nr)
 {
-	if (pte_present(entry))
-		entry = clear_pte_bit(entry, __pgprot(_PAGE_UNUSED));
 	page_table_check_ptes_set(mm, addr, ptep, entry, nr);
 	for (;;) {
 		set_pte(ptep, entry);
diff --git a/arch/s390/mm/gmap_helpers.c b/arch/s390/mm/gmap_helpers.c
index 1cfe4724fbe2..60023b6fdcb1 100644
--- a/arch/s390/mm/gmap_helpers.c
+++ b/arch/s390/mm/gmap_helpers.c
@@ -181,7 +181,8 @@ void gmap_helper_try_set_pte_unused(struct mm_struct *mm, unsigned long vmaddr)
 	if (IS_ERR_OR_NULL(ptep))
 		return;
 
-	__atomic64_or(_PAGE_UNUSED, (long *)ptep);
+	if (pte_present(*ptep))
+		__atomic64_or(_PAGE_UNUSED, (long *)ptep);
 	pte_unmap_unlock(ptep, ptl);
 }
 EXPORT_SYMBOL_GPL(gmap_helper_try_set_pte_unused);
-- 
2.54.0

[PATCH v5 2/9] KVM: s390: Fix dat_peek_cmma() overflow

[Claudio Imbrenda]

If userspace passes a start address that is out of bounds,
_dat_walk_gfn_range() will fail with -EFAULT, but state.end will not be
touched and will stay 0. This will cause *count to underflow and report
a very high number, and the function will end up erroneously reporting
success.

Fix by only setting *count if the end address is not smaller than the
starting address. This way invalid starting addresses will correctly
return -EFAULT and *count will correctly indicate that no values have
been returned.

Fixes: 7b368470e1a4 ("KVM: s390: KVM page table management functions: CMMA")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/dat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
index 4a41c0247ffa..cffac7782c4b 100644
--- a/arch/s390/kvm/dat.c
+++ b/arch/s390/kvm/dat.c
@@ -1209,7 +1209,7 @@ int dat_peek_cmma(gfn_t start, union asce asce, unsigned int *count, u8 *values)
 	int rc;
 
 	rc = _dat_walk_gfn_range(start, start + *count, asce, &ops, DAT_WALK_DEFAULT, &state);
-	*count = state.end - start;
+	*count = state.end >= start ? state.end - start : 0;
 	/* Return success if at least one value was saved, otherwise an error. */
 	return (rc == -EFAULT && *count > 0) ? 0 : rc;
 }
-- 
2.54.0

Re: [PATCH v5 2/9] KVM: s390: Fix dat_peek_cmma() overflow

[Christian Borntraeger]

Am 22.06.26 um 18:07 schrieb Claudio Imbrenda:
> If userspace passes a start address that is out of bounds,
> _dat_walk_gfn_range() will fail with -EFAULT, but state.end will not be
> touched and will stay 0. This will cause *count to underflow and report
> a very high number, and the function will end up erroneously reporting
> success.
> 
> Fix by only setting *count if the end address is not smaller than the
> starting address. This way invalid starting addresses will correctly
> return -EFAULT and *count will correctly indicate that no values have
> been returned.
> 
> Fixes: 7b368470e1a4 ("KVM: s390: KVM page table management functions: CMMA")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>

> ---
>   arch/s390/kvm/dat.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
> index 4a41c0247ffa..cffac7782c4b 100644
> --- a/arch/s390/kvm/dat.c
> +++ b/arch/s390/kvm/dat.c
> @@ -1209,7 +1209,7 @@ int dat_peek_cmma(gfn_t start, union asce asce, unsigned int *count, u8 *values)
>   	int rc;
>   
>   	rc = _dat_walk_gfn_range(start, start + *count, asce, &ops, DAT_WALK_DEFAULT, &state);
> -	*count = state.end - start;
> +	*count = state.end >= start ? state.end - start : 0;
>   	/* Return success if at least one value was saved, otherwise an error. */
>   	return (rc == -EFAULT && *count > 0) ? 0 : rc;
>   }

[PATCH v5 3/9] KVM: s390: Do not set special large pages dirty

[Claudio Imbrenda]

Special pages / folios should not be set dirty. This also applies to
large pages.

Add a missing check in gmap_clear_young_crste() to prevent setting the
large page dirty if it is a special page.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Fixes: a2c17f9270cc ("KVM: s390: New gmap code")
---
 arch/s390/kvm/gmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
index 52d55ddea8d4..3192f610f696 100644
--- a/arch/s390/kvm/gmap.c
+++ b/arch/s390/kvm/gmap.c
@@ -327,7 +327,7 @@ static long gmap_clear_young_crste(union crste *crstep, gfn_t gfn, gfn_t end, st
 		new.h.i = 1;
 		new.s.fc1.y = 0;
 		new.s.fc1.prefix_notif = 0;
-		if (new.s.fc1.d || !new.h.p)
+		if ((new.s.fc1.d || !new.h.p) && !new.s.fc1.s)
 			folio_set_dirty(phys_to_folio(crste_origin_large(crste)));
 		new.s.fc1.d = 0;
 		new.h.p = 1;
-- 
2.54.0

[PATCH v5 4/9] KVM: s390: Fix code typo in gmap_protect_asce_top_level()

[Claudio Imbrenda]

The correct length to pass to kvm_s390_get_guest_pages() is asce.tl + 1,
not asce.dt + 1. It was a typo, which, due to fortuitous circumstances,
did not cause bugs. It should nonetheless be fixed.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Fixes: e5f98a6899bd ("KVM: s390: Add some helper functions needed for vSIE")
---
 arch/s390/kvm/gmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
index 3192f610f696..e6e786811db8 100644
--- a/arch/s390/kvm/gmap.c
+++ b/arch/s390/kvm/gmap.c
@@ -1262,7 +1262,7 @@ static int gmap_protect_asce_top_level(struct kvm_s390_mmu_cache *mc, struct gma
 	/* Pairs with the smp_wmb() in kvm_mmu_invalidate_end(). */
 	smp_rmb();
 
-	rc = kvm_s390_get_guest_pages(sg->kvm, context.f, asce.rsto, asce.dt + 1, false);
+	rc = kvm_s390_get_guest_pages(sg->kvm, context.f, asce.rsto, asce.tl + 1, false);
 	if (rc > 0)
 		rc = -EFAULT;
 	if (!rc)
-- 
2.54.0

[PATCH v5 5/9] KVM: s390: Fix handle_{sske,pfmf} under memory pressure

[Claudio Imbrenda]

Under heavy memory pressure, handle_sske() and handle_pfmf() might
cause an endless loop if the mmu cache runs empty, the atomic
allocations fail, and the top-up function also fails. While quite
unlikely, that scenario is not impossible.

Fix the issue by not ignoring the return value of
kvm_s390_mmu_cache_topup(), and appropriately returning an error code
in case of failure.

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

diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index 447ec7ed423d..9bc6fd02ff77 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -366,7 +366,9 @@ static int handle_sske(struct kvm_vcpu *vcpu)
 		if (rc > 1)
 			return kvm_s390_inject_program_int(vcpu, PGM_ADDRESSING);
 		if (rc == -ENOMEM) {
-			kvm_s390_mmu_cache_topup(vcpu->arch.mc);
+			rc = kvm_s390_mmu_cache_topup(vcpu->arch.mc);
+			if (rc)
+				return rc;
 			continue;
 		}
 		if (rc < 0)
@@ -1122,7 +1124,9 @@ static int handle_pfmf(struct kvm_vcpu *vcpu)
 			if (rc > 1)
 				return kvm_s390_inject_program_int(vcpu, rc);
 			if (rc == -ENOMEM) {
-				kvm_s390_mmu_cache_topup(vcpu->arch.mc);
+				rc = kvm_s390_mmu_cache_topup(vcpu->arch.mc);
+				if (rc)
+					return rc;
 				continue;
 			}
 			if (rc < 0)
-- 
2.54.0

[PATCH v5 6/9] KVM: s390: Fix locking in kvm_s390_set_mem_control()

[Claudio Imbrenda]

All subfunctions in kvm_s390_set_mem_control() need to take the
kvm->lock, so factor it out of the switch.

Also add the missing locking around dat_reset_cmma().

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

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index ffb20a64d328..93141a68e0dd 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -962,6 +962,8 @@ static int kvm_s390_set_mem_control(struct kvm *kvm, struct kvm_device_attr *att
 {
 	int ret;
 
+	guard(mutex)(&kvm->lock);
+
 	switch (attr->attr) {
 	case KVM_S390_VM_MEM_ENABLE_CMMA:
 		ret = -ENXIO;
@@ -969,7 +971,6 @@ static int kvm_s390_set_mem_control(struct kvm *kvm, struct kvm_device_attr *att
 			break;
 
 		VM_EVENT(kvm, 3, "%s", "ENABLE: CMMA support");
-		mutex_lock(&kvm->lock);
 		if (kvm->created_vcpus)
 			ret = -EBUSY;
 		else {
@@ -978,7 +979,6 @@ static int kvm_s390_set_mem_control(struct kvm *kvm, struct kvm_device_attr *att
 			kvm->arch.use_pfmfi = 0;
 			ret = 0;
 		}
-		mutex_unlock(&kvm->lock);
 		break;
 	case KVM_S390_VM_MEM_CLR_CMMA: {
 		gfn_t start_gfn = 0;
@@ -992,7 +992,8 @@ static int kvm_s390_set_mem_control(struct kvm *kvm, struct kvm_device_attr *att
 
 		VM_EVENT(kvm, 3, "%s", "RESET: CMMA states");
 		do {
-			start_gfn = dat_reset_cmma(kvm->arch.gmap->asce, start_gfn);
+			scoped_guard(read_lock, &kvm->mmu_lock)
+				start_gfn = dat_reset_cmma(kvm->arch.gmap->asce, start_gfn);
 			cond_resched();
 		} while (start_gfn);
 		ret = 0;
@@ -1010,8 +1011,6 @@ static int kvm_s390_set_mem_control(struct kvm *kvm, struct kvm_device_attr *att
 		if (get_user(new_limit, (u64 __user *)attr->addr))
 			return -EFAULT;
 
-		guard(mutex)(&kvm->lock);
-
 		new_limit = ALIGN(new_limit, HPAGE_SIZE);
 		if (kvm->arch.mem_limit != KVM_S390_NO_MEM_LIMIT &&
 		    new_limit > kvm->arch.mem_limit)
-- 
2.54.0

[PATCH v5 7/9] KVM: s390: Fix cmma dirty tracking

[Claudio Imbrenda]

It is possible that some guest memory areas have not been touched yet
when starting migration mode, and thus have no ptes allocated. Only
existing and allocated ptes should count toward the total of dirty cmma
entries.

When starting migration mode, count how many pages actually have a pte
(and PGSTE), instead of blindly counting the number of pages in all
memslots.

Also fix dat_get_cmma() to properly wrap around if the first attempt
reached the end of guest memory without finding cmma-dirty pages.

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

diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
index cffac7782c4b..0ad4ebc80eba 100644
--- a/arch/s390/kvm/dat.c
+++ b/arch/s390/kvm/dat.c
@@ -1253,6 +1253,9 @@ int dat_get_cmma(union asce asce, gfn_t *start, unsigned int *count, u8 *values,
 	};
 
 	_dat_walk_gfn_range(*start, asce_end(asce), asce, &ops, DAT_WALK_IGN_HOLES, &state);
+	/* If no dirty pages were found, wrap around and continue searching */
+	if (*start && state.start == -1)
+		_dat_walk_gfn_range(0, *start, asce, &ops, DAT_WALK_IGN_HOLES, &state);
 
 	if (state.start == -1) {
 		*count = 0;
diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
index e6e786811db8..a9a37fdeb809 100644
--- a/arch/s390/kvm/gmap.c
+++ b/arch/s390/kvm/gmap.c
@@ -1075,7 +1075,13 @@ int gmap_protect_rmap(struct kvm_s390_mmu_cache *mc, struct gmap *sg, gfn_t p_gf
 
 static long __set_cmma_dirty_pte(union pte *ptep, gfn_t gfn, gfn_t next, struct dat_walk *walk)
 {
-	__atomic64_or(PGSTE_CMMA_D_BIT, &pgste_of(ptep)->val);
+	union pgste pgste;
+
+	pgste = pgste_get_lock(ptep);
+	pgste.cmma_d = 1;
+	pgste_set_unlock(ptep, pgste);
+	atomic64_inc(walk->priv);
+
 	if (need_resched())
 		return next;
 	return 0;
@@ -1089,7 +1095,8 @@ void gmap_set_cmma_all_dirty(struct gmap *gmap)
 	do {
 		scoped_guard(read_lock, &gmap->kvm->mmu_lock)
 			gfn = _dat_walk_gfn_range(gfn, asce_end(gmap->asce), gmap->asce, &ops,
-						  DAT_WALK_IGN_HOLES, NULL);
+						  DAT_WALK_IGN_HOLES,
+						  &gmap->kvm->arch.cmma_dirty_pages);
 		cond_resched();
 	} while (gfn);
 }
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 93141a68e0dd..cdd8b41d24ed 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1190,7 +1190,6 @@ static int kvm_s390_vm_start_migration(struct kvm *kvm)
 {
 	struct kvm_memory_slot *ms;
 	struct kvm_memslots *slots;
-	unsigned long ram_pages = 0;
 	int bkt;
 
 	/* migration mode already enabled */
@@ -1207,12 +1206,11 @@ static int kvm_s390_vm_start_migration(struct kvm *kvm)
 	kvm_for_each_memslot(ms, bkt, slots) {
 		if (!ms->dirty_bitmap)
 			return -EINVAL;
-		ram_pages += ms->npages;
 	}
 	/* mark all the pages as dirty */
+	atomic64_set(&kvm->arch.cmma_dirty_pages, 0);
 	gmap_set_cmma_all_dirty(kvm->arch.gmap);
-	atomic64_set(&kvm->arch.cmma_dirty_pages, ram_pages);
-	kvm->arch.migration_mode = 1;
+	WRITE_ONCE(kvm->arch.migration_mode, 1);
 	kvm_s390_sync_request_broadcast(kvm, KVM_REQ_START_MIGRATION);
 	return 0;
 }
@@ -1226,7 +1224,7 @@ static int kvm_s390_vm_stop_migration(struct kvm *kvm)
 	/* migration mode already disabled */
 	if (!kvm->arch.migration_mode)
 		return 0;
-	kvm->arch.migration_mode = 0;
+	WRITE_ONCE(kvm->arch.migration_mode, 0);
 	if (kvm->arch.use_cmma)
 		kvm_s390_sync_request_broadcast(kvm, KVM_REQ_STOP_MIGRATION);
 	return 0;
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index 9bc6fd02ff77..ad0ddc433a73 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -1236,7 +1236,7 @@ static int handle_essa(struct kvm_vcpu *vcpu)
 						: ESSA_SET_STABLE_IF_RESIDENT))
 		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
 
-	if (!vcpu->kvm->arch.migration_mode) {
+	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
-- 
2.54.0

[PATCH v5 8/9] KVM: s390: selftests: Fix cmma selftest

[Claudio Imbrenda]

The existing cmma selftest depended on the host allocating page tables
for all present memslots. Since the gmap rewrite, memory that is not
accessed by the guest might not have page tables allocated yet.

This caused the test to fail due to a mismatch in the assertion.

Fix by having the guest access also the second half of the test
memslot, thus guaranteeing that its page tables are present.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
---
 tools/testing/selftests/kvm/s390/cmma_test.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/testing/selftests/kvm/s390/cmma_test.c b/tools/testing/selftests/kvm/s390/cmma_test.c
index e39a724fe860..15d81b2ed7ad 100644
--- a/tools/testing/selftests/kvm/s390/cmma_test.c
+++ b/tools/testing/selftests/kvm/s390/cmma_test.c
@@ -34,16 +34,22 @@ static char cmma_value_buf[MAIN_PAGE_COUNT + TEST_DATA_PAGE_COUNT];
 /**
  * Dirty CMMA attributes of exactly one page in the TEST_DATA memslot,
  * so use_cmma goes on and the CMMA related ioctls do something.
+ * Touch the page at offset 1M inside TEST_DATA to make sure its page
+ * tables are allocated in the host.
  */
 static void guest_do_one_essa(void)
 {
 	asm volatile(
 		/* load TEST_DATA_START_GFN into r1 */
+		"	xgr 1,1\n"
 		"	llilf 1,%[start_gfn]\n"
 		/* calculate the address from the gfn */
 		"	sllg 1,1,12(0)\n"
 		/* set the first page in TEST_DATA memslot to STABLE */
 		"	.insn rrf,0xb9ab0000,2,1,1,0\n"
+		"	agfi 1,0x100000\n"
+		/* also touch the first page of the second MB of TEST_DATA */
+		"	.insn rrf,0xb9ab0000,2,1,1,0\n"
 		/* hypercall */
 		"	diag 0,0,0x501\n"
 		"0:	j 0b"
-- 
2.54.0

[PATCH v5 9/9] KVM: s390: Return failure in case of failure in kvm_s390_set_cmma_bits()

[Claudio Imbrenda]

If the allocation of the bits array failed, kvm_s390_set_cmma_bits()
would return 0 instead of an error code.

Rework the function to use the __free() macros and thus simplify the
code flow; when the above mentioned allocation fails, simply return
-ENOMEM.

Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index cdd8b41d24ed..27d6004132d2 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2282,8 +2282,8 @@ static int kvm_s390_get_cmma_bits(struct kvm *kvm,
 static int kvm_s390_set_cmma_bits(struct kvm *kvm,
 				  const struct kvm_s390_cmma_log *args)
 {
-	struct kvm_s390_mmu_cache *mc;
-	u8 *bits = NULL;
+	struct kvm_s390_mmu_cache *mc __free(kvm_s390_mmu_cache) = NULL;
+	u8 *bits __free(kvfree) = NULL;
 	int r = 0;
 
 	if (!kvm->arch.use_cmma)
@@ -2303,18 +2303,16 @@ static int kvm_s390_set_cmma_bits(struct kvm *kvm,
 		return -ENOMEM;
 	bits = vmalloc(array_size(sizeof(*bits), args->count));
 	if (!bits)
-		goto out;
+		return -ENOMEM;
 
 	r = copy_from_user(bits, (void __user *)args->values, args->count);
-	if (r) {
-		r = -EFAULT;
-		goto out;
-	}
+	if (r)
+		return -EFAULT;
 
 	do {
 		r = kvm_s390_mmu_cache_topup(mc);
 		if (r)
-			break;
+			return r;
 		scoped_guard(read_lock, &kvm->mmu_lock) {
 			r = dat_set_cmma_bits(mc, kvm->arch.gmap->asce, args->start_gfn,
 					      args->count, args->mask, bits);
@@ -2322,9 +2320,7 @@ static int kvm_s390_set_cmma_bits(struct kvm *kvm,
 	} while (r == -ENOMEM);
 
 	set_bit(GMAP_FLAG_USES_CMM, &kvm->arch.gmap->flags);
-out:
-	kvm_s390_free_mmu_cache(mc);
-	vfree(bits);
+
 	return r;
 }
 
-- 
2.54.0

Re: [PATCH v5 9/9] KVM: s390: Return failure in case of failure in kvm_s390_set_cmma_bits()

[Christian Borntraeger]

Am 22.06.26 um 18:07 schrieb Claudio Imbrenda:
> If the allocation of the bits array failed, kvm_s390_set_cmma_bits()
> would return 0 instead of an error code.
> 
> Rework the function to use the __free() macros and thus simplify the
> code flow; when the above mentioned allocation fails, simply return
> -ENOMEM.
> 
> Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>   arch/s390/kvm/kvm-s390.c | 18 +++++++-----------
>   1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index cdd8b41d24ed..27d6004132d2 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2282,8 +2282,8 @@ static int kvm_s390_get_cmma_bits(struct kvm *kvm,
>   static int kvm_s390_set_cmma_bits(struct kvm *kvm,
>   				  const struct kvm_s390_cmma_log *args)
>   {
> -	struct kvm_s390_mmu_cache *mc;
> -	u8 *bits = NULL;
> +	struct kvm_s390_mmu_cache *mc __free(kvm_s390_mmu_cache) = NULL;
> +	u8 *bits __free(kvfree) = NULL;

why kvfree and not vfree?

>   	int r = 0;
>   
>   	if (!kvm->arch.use_cmma)
> @@ -2303,18 +2303,16 @@ static int kvm_s390_set_cmma_bits(struct kvm *kvm,
>   		return -ENOMEM;
>   	bits = vmalloc(array_size(sizeof(*bits), args->count));
>   	if (!bits)
> -		goto out;
> +		return -ENOMEM;
>   
>   	r = copy_from_user(bits, (void __user *)args->values, args->count);
> -	if (r) {
> -		r = -EFAULT;
> -		goto out;
> -	}
> +	if (r)
> +		return -EFAULT;
>   
>   	do {
>   		r = kvm_s390_mmu_cache_topup(mc);
>   		if (r)
> -			break;
> +			return r;
>   		scoped_guard(read_lock, &kvm->mmu_lock) {
>   			r = dat_set_cmma_bits(mc, kvm->arch.gmap->asce, args->start_gfn,
>   					      args->count, args->mask, bits);
> @@ -2322,9 +2320,7 @@ static int kvm_s390_set_cmma_bits(struct kvm *kvm,
>   	} while (r == -ENOMEM);
>   
>   	set_bit(GMAP_FLAG_USES_CMM, &kvm->arch.gmap->flags);
> -out:
> -	kvm_s390_free_mmu_cache(mc);
> -	vfree(bits);
> +
>   	return r;
>   }
>   

Re: [PATCH v5 9/9] KVM: s390: Return failure in case of failure in kvm_s390_set_cmma_bits()

[Claudio Imbrenda]

On Tue, 23 Jun 2026 11:30:24 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> Am 22.06.26 um 18:07 schrieb Claudio Imbrenda:
> > If the allocation of the bits array failed, kvm_s390_set_cmma_bits()
> > would return 0 instead of an error code.
> > 
> > Rework the function to use the __free() macros and thus simplify the
> > code flow; when the above mentioned allocation fails, simply return
> > -ENOMEM.
> > 
> > Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > ---
> >   arch/s390/kvm/kvm-s390.c | 18 +++++++-----------
> >   1 file changed, 7 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index cdd8b41d24ed..27d6004132d2 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -2282,8 +2282,8 @@ static int kvm_s390_get_cmma_bits(struct kvm *kvm,
> >   static int kvm_s390_set_cmma_bits(struct kvm *kvm,
> >   				  const struct kvm_s390_cmma_log *args)
> >   {
> > -	struct kvm_s390_mmu_cache *mc;
> > -	u8 *bits = NULL;
> > +	struct kvm_s390_mmu_cache *mc __free(kvm_s390_mmu_cache) = NULL;
> > +	u8 *bits __free(kvfree) = NULL;  
> 
> why kvfree and not vfree?

because vfree does not exist as cleanup macro: __free(vfree) would not
compile

adding vfree would be outside the scope of this patch

> 
> >   	int r = 0;
> >   
> >   	if (!kvm->arch.use_cmma)
> > @@ -2303,18 +2303,16 @@ static int kvm_s390_set_cmma_bits(struct kvm *kvm,
> >   		return -ENOMEM;
> >   	bits = vmalloc(array_size(sizeof(*bits), args->count));
> >   	if (!bits)
> > -		goto out;
> > +		return -ENOMEM;
> >   
> >   	r = copy_from_user(bits, (void __user *)args->values, args->count);
> > -	if (r) {
> > -		r = -EFAULT;
> > -		goto out;
> > -	}
> > +	if (r)
> > +		return -EFAULT;
> >   
> >   	do {
> >   		r = kvm_s390_mmu_cache_topup(mc);
> >   		if (r)
> > -			break;
> > +			return r;
> >   		scoped_guard(read_lock, &kvm->mmu_lock) {
> >   			r = dat_set_cmma_bits(mc, kvm->arch.gmap->asce, args->start_gfn,
> >   					      args->count, args->mask, bits);
> > @@ -2322,9 +2320,7 @@ static int kvm_s390_set_cmma_bits(struct kvm *kvm,
> >   	} while (r == -ENOMEM);
> >   
> >   	set_bit(GMAP_FLAG_USES_CMM, &kvm->arch.gmap->flags);
> > -out:
> > -	kvm_s390_free_mmu_cache(mc);
> > -	vfree(bits);
> > +
> >   	return r;
> >   }
> >     
> 

Re: [PATCH v5 9/9] KVM: s390: Return failure in case of failure in kvm_s390_set_cmma_bits()

[Christian Borntraeger]

Am 23.06.26 um 12:33 schrieb Claudio Imbrenda:
> On Tue, 23 Jun 2026 11:30:24 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> Am 22.06.26 um 18:07 schrieb Claudio Imbrenda:
>>> If the allocation of the bits array failed, kvm_s390_set_cmma_bits()
>>> would return 0 instead of an error code.
>>>
>>> Rework the function to use the __free() macros and thus simplify the
>>> code flow; when the above mentioned allocation fails, simply return
>>> -ENOMEM.
>>>
>>> Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
>>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>>> ---
>>>    arch/s390/kvm/kvm-s390.c | 18 +++++++-----------
>>>    1 file changed, 7 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index cdd8b41d24ed..27d6004132d2 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -2282,8 +2282,8 @@ static int kvm_s390_get_cmma_bits(struct kvm *kvm,
>>>    static int kvm_s390_set_cmma_bits(struct kvm *kvm,
>>>    				  const struct kvm_s390_cmma_log *args)
>>>    {
>>> -	struct kvm_s390_mmu_cache *mc;
>>> -	u8 *bits = NULL;
>>> +	struct kvm_s390_mmu_cache *mc __free(kvm_s390_mmu_cache) = NULL;
>>> +	u8 *bits __free(kvfree) = NULL;
>>
>> why kvfree and not vfree?
> 
> because vfree does not exist as cleanup macro: __free(vfree) would not
> compile
> 
> adding vfree would be outside the scope of this patch

With that

Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>