Skip to contentSkip to conversation

s390: Remove or replace cond_resched() calls

PATCH Applied Heiko Carstens 4 participants 12 messages Jul 31 – Aug 3

Subsystems (8)
Crypto API s390 s390 MM KVM s390 +4 more subsystems
lore
Share & tools
lore

Heiko Carstens

author maintainer
#0 lore raw

v4 cover s390: Remove or replace cond_resched() calls

v4:
- Replaced first patch with version from Peter Zijlstra

v3:
- Drop uv_call_sched() [Claudio Imbrenda]

v2:
- Use msleep() in crypto code (Holger Dengler)
- Add Reviewed-by tag from Peter Oberparleiter

v1:
Since [1] it is only possible to select the PREEMPT or PREEMPT_LAZY preemption models for s390. This has caused some minor problems, but everything has been addressed.

Given that for both preemption models cond_resched() is a no-op remove or replace it everywhere.

[1] commit 7dadeaa6e851 ("sched: Further restrict the preemption modes")

Heiko Carstens (3):
KVM: s390: Remove cond_resched() calls
s390: Remove cond_resched() calls
s390/cio: Remove cond_resched() calls

Peter Zijlstra (1):
s390/crypto: Replace cond_resched() with msleep(1)

 arch/s390/crypto/paes_s390.c    | 44 +++------------------------------
 arch/s390/crypto/phmac_s390.c   | 25 +++----------------
 arch/s390/include/asm/pkey.h    | 16 ++++++++++++
 arch/s390/include/asm/uv.h      | 16 ++----------
 arch/s390/kernel/perf_cpum_cf.c |  1 -
 arch/s390/kernel/uv.c           |  2 +-
 arch/s390/kvm/gaccess.c         |  8 ++----
 arch/s390/kvm/gmap.c            |  4 ---
 arch/s390/kvm/kvm-s390.c        |  1 -
 arch/s390/kvm/pv.c              | 11 ++++-----
 arch/s390/kvm/vsie.c            |  1 -
 arch/s390/mm/cmm.c              |  2 --
 arch/s390/mm/gmap_helpers.c     |  1 -
 arch/s390/mm/pageattr.c         |  5 ----
 arch/s390/mm/pgalloc.c          |  1 -
 arch/s390/pci/pci.c             |  4 +--
 drivers/s390/char/uvdevice.c    |  6 ++---
 drivers/s390/cio/css.c          |  8 ------
 18 files changed, 37 insertions(+), 119 deletions(-)
Signature · 1 line
2.53.0
author maintainer
#1 lore raw

PATCH v4 1/4 s390/crypto: Replace cond_resched() with msleep(1)

From: Peter Zijlstra <[email protected]>

With [1] cond_resched() is always compiled away and becomes a no-op.

The comments for all cond_resched() calls in crypto code however indicate that the current process should be scheduled away to avoid instant re-invocation of a callback. This is not what cond_resched() would do or did.

Instead of just removing the cond_resched() calls, replace them with msleep() calls, as suggested by Holger Dengler. This forces the current task to be scheduled away (sleeps) like originally intended.

Holger provided information why this intended [2]:
"
Let me give a bit of background here: The protected key can only get invalid, if the Linux instance (z/VM or KVM guest) is moved to another hypervisor on a different machine (aka life guest relocation). In such a case, the crypto accelerator card and the host has to exchange the "real key", which is wrapped by the host and handed back to the guest as the re-newed protected key. Unfortunately there is no asynchronous trigger on completion, you have to re-try (and maybe get another "in progress" return).

And as if that weren't bad enough, if this key exchange between card and host is the first one, card and host has to instantiate a secure communication channel (including a key exchange for the transport layer). "

[1] commit 7dadeaa6e851 ("sched: Further restrict the preemption modes")
[2] https://lore.kernel.org/all/[email protected]/
[3] https://lore.kernel.org/all/[email protected]/

[[email protected]: took Peter's patch [3] and provided commit message]

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Signed-off-by Heiko Carstens <[email protected]>
---
 arch/s390/crypto/paes_s390.c  | 44 ++++-------------------------------
 arch/s390/crypto/phmac_s390.c | 25 +++-----------------
 arch/s390/include/asm/pkey.h  | 16 +++++++++++++
 3 files changed, 23 insertions(+), 62 deletions(-)
arch/s390/crypto/paes_s390.c +4 -40
index 8cfe6166c193..973436592318 100644
@@ -548,16 +548,7 @@ static int ecb_paes_do_one_request(struct crypto_engine *engine, void *areq)
 
 	rc = ecb_paes_do_crypt(ctx, req_ctx, tested, true);
 	if (rc == -EKEYEXPIRED) {
-		/*
-		 * Protected key expired, conversion is in process.
-		 * Trigger a re-schedule of this request by returning
-		 * -ENOSPC ("hardware queue is full") to the crypto engine.
-		 * To avoid immediately re-invocation of this callback,
-		 * tell the scheduler to voluntarily give up the CPU here.
-		 */
-		cond_resched();
-		pr_debug("rescheduling request\n");
-		return -ENOSPC;
+		return pkey_handle_expired();
 	} else if (rc) {
 		skcipher_walk_done(walk, rc);
 	}

@@ -814,16 +805,7 @@ static int cbc_paes_do_one_request(struct crypto_engine *engine, void *areq)
 
 	rc = cbc_paes_do_crypt(ctx, req_ctx, tested, true);
 	if (rc == -EKEYEXPIRED) {
-		/*
-		 * Protected key expired, conversion is in process.
-		 * Trigger a re-schedule of this request by returning
-		 * -ENOSPC ("hardware queue is full") to the crypto engine.
-		 * To avoid immediately re-invocation of this callback,
-		 * tell the scheduler to voluntarily give up the CPU here.
-		 */
-		cond_resched();
-		pr_debug("rescheduling request\n");
-		return -ENOSPC;
+		return pkey_handle_expired();
 	} else if (rc) {
 		skcipher_walk_done(walk, rc);
 	}

@@ -1122,16 +1104,7 @@ static int ctr_paes_do_one_request(struct crypto_engine *engine, void *areq)
 
 	rc = ctr_paes_do_crypt(ctx, req_ctx, tested, true);
 	if (rc == -EKEYEXPIRED) {
-		/*
-		 * Protected key expired, conversion is in process.
-		 * Trigger a re-schedule of this request by returning
-		 * -ENOSPC ("hardware queue is full") to the crypto engine.
-		 * To avoid immediately re-invocation of this callback,
-		 * tell the scheduler to voluntarily give up the CPU here.
-		 */
-		cond_resched();
-		pr_debug("rescheduling request\n");
-		return -ENOSPC;
+		return pkey_handle_expired();
 	} else if (rc) {
 		skcipher_walk_done(walk, rc);
 	}

@@ -1565,16 +1538,7 @@ static int xts_paes_do_one_request(struct crypto_engine *engine, void *areq)
 
 	rc = xts_paes_do_crypt(ctx, req_ctx, tested, true);
 	if (rc == -EKEYEXPIRED) {
-		/*
-		 * Protected key expired, conversion is in process.
-		 * Trigger a re-schedule of this request by returning
-		 * -ENOSPC ("hardware queue is full") to the crypto engine.
-		 * To avoid immediately re-invocation of this callback,
-		 * tell the scheduler to voluntarily give up the CPU here.
-		 */
-		cond_resched();
-		pr_debug("rescheduling request\n");
-		return -ENOSPC;
+		return pkey_handle_expired();
 	} else if (rc) {
 		skcipher_walk_done(walk, rc);
 	}
arch/s390/crypto/phmac_s390.c +3 -22
index 03ca33ffe6cc..020a1beb2e22 100644
@@ -887,16 +887,7 @@ static int phmac_do_one_request(struct crypto_engine *engine, void *areq)
 	case OP_FINUP:
 		rc = phmac_kmac_update(req, true);
 		if (rc == -EKEYEXPIRED) {
-			/*
-			 * Protected key expired, conversion is in process.
-			 * Trigger a re-schedule of this request by returning
-			 * -ENOSPC ("hardware queue full") to the crypto engine.
-			 * To avoid immediately re-invocation of this callback,
-			 * tell scheduler to voluntarily give up the CPU here.
-			 */
-			pr_debug("rescheduling request\n");
-			cond_resched();
-			return -ENOSPC;
+			return pkey_handle_expired();
 		} else if (rc) {
 			hwh_advance(hwh, rc);
 			goto out;

@@ -907,18 +898,8 @@ static int phmac_do_one_request(struct crypto_engine *engine, void *areq)
 		fallthrough;
 	case OP_FINAL:
 		rc = phmac_kmac_final(req, true);
-		if (rc == -EKEYEXPIRED) {
-			/*
-			 * Protected key expired, conversion is in process.
-			 * Trigger a re-schedule of this request by returning
-			 * -ENOSPC ("hardware queue full") to the crypto engine.
-			 * To avoid immediately re-invocation of this callback,
-			 * tell scheduler to voluntarily give up the CPU here.
-			 */
-			pr_debug("rescheduling request\n");
-			cond_resched();
-			return -ENOSPC;
-		}
+		if (rc == -EKEYEXPIRED)
+			return pkey_handle_expired();
 		break;
 	default:
 		/* unknown/unsupported/unimplemented asynch op */
arch/s390/include/asm/pkey.h +16 -0
index 0af5ac4f646b..82e403322e89 100644
@@ -13,6 +13,7 @@
 
 #include <linux/ioctl.h>
 #include <linux/types.h>
+#include <linux/delay.h>
 #include <uapi/asm/pkey.h>
 
 /*

@@ -44,4 +45,19 @@ int pkey_key2protkey(const u8 *key, u32 keylen,
  */
 #define PKEY_XFLAG_NOCLEARKEY 0x0002
 
+static inline int pkey_handle_expired(void)
+{
+	/*
+	 * Protected key expired due to relocation to another host. The long
+	 * running re-wrap has no asynchronous completion notification, so
+	 * polling is required. Trigger a re-schedule of this request by
+	 * returning -ENOSPC ("hardware queue full") to the crypto engine.
+	 * To avoid immediately re-invocation of this callback,
+	 * tell the scheduler to voluntarily give up the CPU here.
+	 */
+	msleep(1);
+	pr_debug("rescheduling request\n");
+	return -ENOSPC;
+}
+
 #endif /* _KAPI_PKEY_H */
Signature · 1 line
2.53.0
AI review
#2 lore raw

Re: [PATCH v4 1/4] s390/crypto: Replace cond_resched() with msleep(1)

Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 13 lines

From: Peter Zijlstra <[email protected]>

With [1] cond_resched() is always compiled away and becomes a no-op.

The comments for all cond_resched() calls in crypto code however indicate that the current process should be scheduled away to avoid instant re-invocation of a callback. This is not what cond_resched() would do or did.

Instead of just removing the cond_resched() calls, replace them with msleep() calls, as suggested by Holger Dengler. This forces the current task to be scheduled away (sleeps) like originally intended.

Quoted 8 lines

Holger provided information why this intended [2]:
"
[ ... ]

[[email protected]: took Peter's patch [3] and provided commit message]

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Signed-off-by Heiko Carstens <[email protected]>

Sashiko has reviewed this patch and found no issues. It looks great!

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
maintainer Reviewed-by on v4 1/4
#3 lore raw

Re: [PATCH v4 1/4] s390/crypto: Replace cond_resched() with msleep(1)

In reply to Heiko Carstens
Quoted 29 lines · Heiko Carstens

From: Peter Zijlstra <[email protected]>

With [1] cond_resched() is always compiled away and becomes a no-op.

The comments for all cond_resched() calls in crypto code however indicate that the current process should be scheduled away to avoid instant re-invocation of a callback. This is not what cond_resched() would do or did.

Instead of just removing the cond_resched() calls, replace them with msleep() calls, as suggested by Holger Dengler. This forces the current task to be scheduled away (sleeps) like originally intended.

Holger provided information why this intended [2]:
"
Let me give a bit of background here: The protected key can only get invalid, if the Linux instance (z/VM or KVM guest) is moved to another hypervisor on a different machine (aka life guest relocation). In such a case, the crypto accelerator card and the host has to exchange the "real key", which is wrapped by the host and handed back to the guest as the re-newed protected key. Unfortunately there is no asynchronous trigger on completion, you have to re-try (and maybe get another "in progress" return).

And as if that weren't bad enough, if this key exchange between card and host is the first one, card and host has to instantiate a secure communication channel (including a key exchange for the transport layer). "

Quoted 8 lines

[1] commit 7dadeaa6e851 ("sched: Further restrict the preemption modes")
[2] https://lore.kernel.org/all/[email protected]/
[3] https://lore.kernel.org/all/[email protected]/

[[email protected]: took Peter's patch [3] and provided commit message]

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Signed-off-by Heiko Carstens <[email protected]>
Reviewed-by Holger Dengler <[email protected]>
Signature · 2 lines
Mit freundlichen Grüßen / Kind regards
Holger Dengler
author maintainer
#4 lore raw

PATCH v4 2/4 KVM: s390: Remove cond_resched() calls

In reply to Heiko Carstens

Since [1] cond_resched() is a no-op on s390. Remove all calls.

This also entirely removes uv_call_sched() and replaces all call sites with uv_call(), since both functions are identical after the removal of cond_resched().

[1] commit 7dadeaa6e851 ("sched: Further restrict the preemption modes")

Reviewed-by Claudio Imbrenda <[email protected]>
Signed-off-by Heiko Carstens <[email protected]>
---
 arch/s390/include/asm/uv.h   | 16 ++--------------
 arch/s390/kernel/uv.c        |  2 +-
 arch/s390/kvm/gaccess.c      |  8 ++------
 arch/s390/kvm/gmap.c         |  4 ----
 arch/s390/kvm/kvm-s390.c     |  1 -
 arch/s390/kvm/pv.c           | 11 +++++------
 arch/s390/kvm/vsie.c         |  1 -
 arch/s390/mm/gmap_helpers.c  |  1 -
 drivers/s390/char/uvdevice.c |  6 +++---
 9 files changed, 13 insertions(+), 37 deletions(-)
arch/s390/include/asm/uv.h +2 -14
index d919e69662f5..89c9bb56c7e6 100644
@@ -454,18 +454,6 @@ static inline int uv_call(unsigned long r1, unsigned long r2)
 	return cc;
 }
 
-/* Low level uv_call that avoids stalls for long running busy conditions  */
-static inline int uv_call_sched(unsigned long r1, unsigned long r2)
-{
-	int cc;
-
-	do {
-		cc = __uv_call(r1, r2);
-		cond_resched();
-	} while (cc > 1);
-	return cc;
-}
-
 /*
  * special variant of uv_call that only transports the cpu or guest
  * handle and the command, like destroy or verify.

@@ -480,7 +468,7 @@ static inline int uv_cmd_nodata(u64 handle, u16 cmd, u16 *rc, u16 *rrc)
 	int cc;
 
 	WARN(!handle, "No handle provided to Ultravisor call cmd %x\n", cmd);
-	cc = uv_call_sched(0, (u64)&uvcb);
+	cc = uv_call(0, (u64)&uvcb);
 	*rc = uvcb.header.rc;
 	*rrc = uvcb.header.rrc;
 	return cc ? -EINVAL : 0;

@@ -518,7 +506,7 @@ static inline int uv_list_secrets(struct uv_secret_list *buf, u16 start_idx,
 		.start_idx = start_idx,
 		.list_addr = (u64)buf,
 	};
-	int cc = uv_call_sched(0, (u64)&uvcb);
+	int cc = uv_call(0, (u64)&uvcb);
 
 	if (rc)
 		*rc = uvcb.header.rc;
arch/s390/kernel/uv.c +1 -1
index a284f98d9716..62841bb00a26 100644
@@ -831,7 +831,7 @@ int uv_retrieve_secret(u16 secret_idx, u8 *buf, size_t buf_size)
 		.buf_size = buf_size,
 	};
 
-	uv_call_sched(0, (u64)&uvcb);
+	uv_call(0, (u64)&uvcb);
 
 	switch (uvcb.header.rc) {
 	case UVC_RC_EXECUTED:
arch/s390/kvm/gaccess.c +2 -6
index 36102b2727fb..0d389a302a70 100644
@@ -151,10 +151,8 @@ static void ipte_lock_simple(struct kvm *kvm)
 	ic = &kvm->arch.sca->ipte_control;
 	old = READ_ONCE(*ic);
 	do {
-		if (old.k) {
-			cond_resched();
+		if (old.k)
 			goto retry;
-		}
 		new = old;
 		new.k = 1;
 	} while (!try_cmpxchg(&ic->val, &old.val, new.val));

@@ -189,10 +187,8 @@ static void ipte_lock_siif(struct kvm *kvm)
 	ic = &kvm->arch.sca->ipte_control;
 	old = READ_ONCE(*ic);
 	do {
-		if (old.kg) {
-			cond_resched();
+		if (old.kg)
 			goto retry;
-		}
 		new = old;
 		new.k = 1;
 		new.kh++;
arch/s390/kvm/gmap.c +0 -4
index 8abb4f55b306..3758e4009709 100644
@@ -941,7 +941,6 @@ void gmap_split_huge_pages(struct gmap *gmap)
 		scoped_guard(read_lock, &gmap->kvm->mmu_lock)
 			start = _dat_walk_gfn_range(start, asce_end(gmap->asce), gmap->asce,
 						    &ops, DAT_WALK_IGN_HOLES, gmap);
-		cond_resched();
 	} while (start);
 }
 

@@ -963,7 +962,6 @@ static int _gmap_enable_skeys(struct gmap *gmap)
 	do {
 		scoped_guard(write_lock, &gmap->kvm->mmu_lock)
 			start = dat_reset_skeys(gmap->asce, start);
-		cond_resched();
 	} while (start);
 	return 0;
 }

@@ -1019,7 +1017,6 @@ int gmap_pv_destroy_range(struct gmap *gmap, gfn_t start, gfn_t end, bool interr
 						    DAT_WALK_IGN_HOLES, NULL);
 		if (interruptible && fatal_signal_pending(current))
 			return -EINTR;
-		cond_resched();
 	} while (start && start < end);
 	return 0;
 }

@@ -1138,7 +1135,6 @@ void _gmap_set_cmma_all(struct gmap *gmap, bool dirty)
 			gfn = _dat_walk_gfn_range(gfn, asce_end(gmap->asce), gmap->asce, &ops,
 						  DAT_WALK_IGN_HOLES,
 						  &gmap->kvm->arch.cmma_dirty_pages);
-		cond_resched();
 	} while (gfn);
 }
arch/s390/kvm/kvm-s390.c +0 -1
index 150b5dd2170e..2ede8f035a38 100644
@@ -1027,7 +1027,6 @@ static int kvm_s390_set_mem_control(struct kvm *kvm, struct kvm_device_attr *att
 		do {
 			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;
 		break;
arch/s390/kvm/pv.c +5 -6
index 1beacc841ca8..54c17ddb6b11 100644
@@ -429,7 +429,7 @@ static int kvm_s390_pv_deinit_vm_fast(struct kvm *kvm, u16 *rc, u16 *rrc)
 	};
 	int cc;
 
-	cc = uv_call_sched(0, (u64)&uvcb);
+	cc = uv_call(0, (u64)&uvcb);
 	if (rc)
 		*rc = uvcb.header.rc;
 	if (rrc)

@@ -746,7 +746,7 @@ int kvm_s390_pv_init_vm(struct kvm *kvm, u16 *rc, u16 *rrc)
 	}
 	gmap_split_huge_pages(kvm->arch.gmap);
 
-	cc = uv_call_sched(0, (u64)&uvcb);
+	cc = uv_call(0, (u64)&uvcb);
 	*rc = uvcb.header.rc;
 	*rrc = uvcb.header.rrc;
 	KVM_UV_EVENT(kvm, 3, "PROTVIRT CREATE VM: handle %llx len %llx rc %x rrc %x flags %04x",

@@ -832,7 +832,6 @@ int kvm_s390_pv_unpack(struct kvm *kvm, unsigned long addr, unsigned long size,
 	while (offset < size) {
 		ret = unpack_one(kvm, addr, tweak, offset, rc, rrc);
 		if (ret == -EAGAIN) {
-			cond_resched();
 			if (fatal_signal_pending(current))
 				break;
 			continue;

@@ -875,7 +874,7 @@ int kvm_s390_pv_dump_cpu(struct kvm_vcpu *vcpu, void *buff, u16 *rc, u16 *rrc)
 	};
 	int cc;
 
-	cc = uv_call_sched(0, (u64)&uvcb);
+	cc = uv_call(0, (u64)&uvcb);
 	*rc = uvcb.header.rc;
 	*rrc = uvcb.header.rrc;
 	return cc;

@@ -959,7 +958,7 @@ int kvm_s390_pv_dump_stor_state(struct kvm *kvm, void __user *buff_user,
 	/* We will loop until the user buffer is filled or an error occurs */
 	do {
 		/* Get 1MB worth of guest storage state data */
-		cc = uv_call_sched(0, (u64)&uvcb);
+		cc = uv_call(0, (u64)&uvcb);
 
 		/* All or nothing */
 		if (cc) {

@@ -1037,7 +1036,7 @@ int kvm_s390_pv_dump_complete(struct kvm *kvm, void __user *buff_user,
 		return -ENOMEM;
 	complete.dump_area_origin = (u64)compl_data;
 
-	ret = uv_call_sched(0, (u64)&complete);
+	ret = uv_call(0, (u64)&complete);
 	*rc = complete.header.rc;
 	*rrc = complete.header.rrc;
 	KVM_UV_EVENT(kvm, 3, "PROTVIRT DUMP COMPLETE: rc %x rrc %x",
arch/s390/kvm/vsie.c +0 -1
index eea24562e7db..6dcd3b110c17 100644
@@ -1434,7 +1434,6 @@ static int vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 		}
 		if (sg)
 			sg = gmap_put(sg);
-		cond_resched();
 	}
 	if (sg)
 		sg = gmap_put(sg);
arch/s390/mm/gmap_helpers.c +0 -1
index 4bf7c9012feb..d564cdefb90f 100644
@@ -272,7 +272,6 @@ static int __gmap_helper_unshare_zeropages(struct mm_struct *mm)
 		 * truncation. In that case, the shared zeropage would be gone
 		 * and we can simply retry and make progress.
 		 */
-		cond_resched();
 		goto retry;
 	}
drivers/s390/char/uvdevice.c +3 -3
index e6a264c996ce..4d274a2ee84d 100644
@@ -224,7 +224,7 @@ static int uvio_attestation(struct uvio_ioctl_cb *uv_ioctl)
 	if (ret)
 		goto out;
 
-	uv_call_sched(0, (u64)uvcb_attest);
+	uv_call(0, (u64)uvcb_attest);
 
 	uv_ioctl->uv_rc = uvcb_attest->header.rc;
 	uv_ioctl->uv_rrc = uvcb_attest->header.rrc;

@@ -291,7 +291,7 @@ static int uvio_add_secret(struct uvio_ioctl_cb *uv_ioctl)
 
 	ret = 0;
 	uvcb.addr = (u64)asrcb;
-	uv_call_sched(0, (u64)&uvcb);
+	uv_call(0, (u64)&uvcb);
 	uv_ioctl->uv_rc = uvcb.header.rc;
 	uv_ioctl->uv_rrc = uvcb.header.rrc;
 

@@ -457,7 +457,7 @@ static int uvio_retr_secret(struct uvio_ioctl_cb *uv_ioctl)
 
 	uvcb.buf_addr = (u64)buf;
 	uvcb.buf_size = buf_len;
-	uv_call_sched(0, (u64)&uvcb);
+	uv_call(0, (u64)&uvcb);
 
 	if (copy_to_user((__user void *)uv_ioctl->argument_addr, buf, buf_len))
 		goto err;
Signature · 1 line
2.53.0
AI review
#5 lore raw

Re: [PATCH v4 2/4] KVM: s390: Remove cond_resched() calls

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing bounds check on user_data_len allows an out-of-bounds read of kernel memory by the Ultravisor. --

--- Patch [2]: [PATCH v4 2/4] KVM: s390: Remove cond_resched() calls --- commit 6586b58419c33753e59827884fdaa5a7f473b641
Author: Heiko Carstens <[email protected]>

KVM: s390: Remove cond_resched() calls

Since [1] cond_resched() is a no-op on s390. Remove all calls.

This also entirely removes uv_call_sched() and replaces all call sites with uv_call(), since both functions are identical after the removal of cond_resched().

[1] commit 7dadeaa6e851 ("sched: Further restrict the preemption modes")

Quoted 4 lines
drivers/s390/char/uvdevice.c +0 -0
index e6a264c996ce3..4d274a2ee84dd 100644

[ ... ]

drivers/s390/char/uvdevice.c static int uvio_attestation(struct uvio_ioctl_cb *uv_ioctl) · line 227

Quoted 6 lines
drivers/s390/char/uvdevice.c +1 -1
@@ -224,7 +224,7 @@ static int uvio_attestation(struct uvio_ioctl_cb *uv_ioctl)
 	if (ret)
 		goto out;
 
-	uv_call_sched(0, (u64)uvcb_attest);
+	uv_call(0, (u64)uvcb_attest);

[Severity: High]
This is a pre-existing issue, but does get_uvio_attest() validate the size of user_data_len?

Looking at uvio_attestation(), it calls get_uvio_attest() to populate uvio_attest, but there appears to be no bounds check on uvio_attest->user_data_len. Then in uvio_build_uvcb_attest(), this unvalidated length is copied to uvcb_attest->user_data_len before making this Ultravisor call.

Since uvcb_attest is allocated in kernel memory, could a malicious user provide a large user_data_len (up to 65535) and cause the Ultravisor firmware to read beyond the bounds of the uvcb_attest allocation? This might lead to a system crash if it accesses unmapped physical memory, or it could leak adjacent kernel heap contents by incorporating them into the cryptographic measurement returned to the user.

Quoted 3 lines
drivers/s390/char/uvdevice.c +0 -0
 	uv_ioctl->uv_rc = uvcb_attest->header.rc;
 	uv_ioctl->uv_rrc = uvcb_attest->header.rrc;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
author maintainer
#6 lore raw

PATCH v4 3/4 s390: Remove cond_resched() calls

In reply to Heiko Carstens

Since [1] cond_resched() is a no-op on s390. Remove all calls.

[1] commit 7dadeaa6e851 ("sched: Further restrict the preemption modes")

Signed-off-by Heiko Carstens <[email protected]>
---
 arch/s390/kernel/perf_cpum_cf.c | 1 -
 arch/s390/mm/cmm.c              | 2 --
 arch/s390/mm/pageattr.c         | 5 -----
 arch/s390/mm/pgalloc.c          | 1 -
 arch/s390/pci/pci.c             | 4 +---
 5 files changed, 1 insertion(+), 12 deletions(-)
arch/s390/kernel/perf_cpum_cf.c +0 -1
index 2076ac22e2c4..5f2596be6c1e 100644
@@ -1496,7 +1496,6 @@ static int cfset_all_copy(unsigned long arg, cpumask_t *mask)
 			goto out;
 		}
 		uptr += sizeof(struct s390_ctrset_cpudata) + cpuhw->used;
-		cond_resched();
 	}
 	cpus = cpumask_weight(mask);
 	if (put_user(cpus, &ctrset_read->no_cpus))
arch/s390/mm/cmm.c +0 -2
index eb7ef63fab1e..d342b68ee9be 100644
@@ -95,7 +95,6 @@ static long cmm_alloc_pages(long nr, long *counter,
 		(*counter)++;
 		spin_unlock(&cmm_lock);
 		nr--;
-		cond_resched();
 	}
 	return nr;
 }

@@ -134,7 +133,6 @@ static long cmm_free_pages(long nr, long *counter, struct cmm_page_array **list)
 		inc = __cmm_free_pages(inc, counter, list);
 		if (inc)
 			break;
-		cond_resched();
 	}
 	return nr + inc;
 }
arch/s390/mm/pageattr.c +0 -5
index e6f788696dd1..1e202e3d08e7 100644
@@ -105,7 +105,6 @@ static int walk_pte_level(pmd_t *pmdp, unsigned long addr, unsigned long end,
 		pgt_set((unsigned long *)ptep, pte_val(new), addr, CRDTE_DTT_PAGE);
 		ptep++;
 		addr += PAGE_SIZE;
-		cond_resched();
 	} while (addr < end);
 	return 0;
 }

@@ -194,7 +193,6 @@ static int walk_pmd_level(pud_t *pudp, unsigned long addr, unsigned long end,
 		}
 		pmdp++;
 		addr = next;
-		cond_resched();
 	} while (addr < end);
 	return rc;
 }

@@ -281,7 +279,6 @@ static int walk_pud_level(p4d_t *p4d, unsigned long addr, unsigned long end,
 		}
 		pudp++;
 		addr = next;
-		cond_resched();
 	} while (addr < end && !rc);
 	return rc;
 }

@@ -301,7 +298,6 @@ static int walk_p4d_level(pgd_t *pgd, unsigned long addr, unsigned long end,
 		rc = walk_pud_level(p4dp, addr, next, flags);
 		p4dp++;
 		addr = next;
-		cond_resched();
 	} while (addr < end && !rc);
 	return rc;
 }

@@ -323,7 +319,6 @@ static int change_page_attr(unsigned long addr, unsigned long end,
 		rc = walk_p4d_level(pgdp, addr, next, flags);
 		if (rc)
 			break;
-		cond_resched();
 	} while (pgdp++, addr = next, addr < end && !rc);
 	return rc;
 }
arch/s390/mm/pgalloc.c +0 -1
index 7ac44543e051..9610770fcf6d 100644
@@ -268,7 +268,6 @@ static int base_segment_walk(unsigned long *origin, unsigned long addr,
 			return rc;
 		if (!alloc)
 			base_pgt_free(table);
-		cond_resched();
 	} while (ste++, addr = next, addr < end);
 	return 0;
 }
arch/s390/pci/pci.c +1 -3
index 39bd2adfc240..2dec238f3f12 100644
@@ -1173,10 +1173,8 @@ int zpci_scan_devices(void)
 		return rc;
 
 	zpci_add_devices(&scan_list);
-	zpci_bus_for_each(zbus) {
+	zpci_bus_for_each(zbus)
 		zpci_bus_scan_bus(zbus);
-		cond_resched();
-	}
 	return 0;
 }
Signature · 1 line
2.53.0
AI review
#7 lore raw

Re: [PATCH v4 3/4] s390: Remove cond_resched() calls

Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 5 lines

Since [1] cond_resched() is a no-op on s390. Remove all calls.

[1] commit 7dadeaa6e851 ("sched: Further restrict the preemption modes")

Signed-off-by Heiko Carstens <[email protected]>

Sashiko has reviewed this patch and found no issues. It looks great!

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
maintainer Reviewed-by on v4 3/4
#8 lore raw

Re: [PATCH v4 3/4] s390: Remove cond_resched() calls

In reply to Heiko Carstens
Quoted 4 lines · Heiko Carstens

Since [1] cond_resched() is a no-op on s390. Remove all calls.

[1] commit 7dadeaa6e851 ("sched: Further restrict the preemption modes")

Quoted 8 lines
Signed-off-by Heiko Carstens <[email protected]>
---
 arch/s390/kernel/perf_cpum_cf.c | 1 -
 arch/s390/mm/cmm.c              | 2 --
 arch/s390/mm/pageattr.c         | 5 -----
 arch/s390/mm/pgalloc.c          | 1 -
 arch/s390/pci/pci.c             | 4 +---
 5 files changed, 1 insertion(+), 12 deletions(-)
Reviewed-by Vasily Gorbik <[email protected]>
author maintainer
#9 lore raw

PATCH v4 4/4 s390/cio: Remove cond_resched() calls

In reply to Heiko Carstens

Since [1] cond_resched() is a no-op on s390. Remove all calls.

[1] commit 7dadeaa6e851 ("sched: Further restrict the preemption modes")

Reviewed-by Peter Oberparleiter <[email protected]>
Signed-off-by Heiko Carstens <[email protected]>
---
 drivers/s390/cio/css.c | 8 --------
 1 file changed, 8 deletions(-)
drivers/s390/cio/css.c +0 -8
index e5a0ec6b4e3e..54b8b8639694 100644
@@ -618,11 +618,6 @@ static int slow_eval_known_fn(struct subchannel *sch, void *data)
 		rc = css_evaluate_known_subchannel(sch, 1);
 		if (rc == -EAGAIN)
 			css_schedule_eval(sch->schid);
-		/*
-		 * The loop might take long time for platforms with lots of
-		 * known devices. Allow scheduling here.
-		 */
-		cond_resched();
 	}
 	return 0;
 }

@@ -654,9 +649,6 @@ static int slow_eval_unknown_fn(struct subchannel_id schid, void *data)
 		default:
 			rc = 0;
 		}
-		/* Allow scheduling here since the containing loop might
-		 * take a while.  */
-		cond_resched();
 	}
 	return rc;
 }
Signature · 1 line
2.53.0
AI review
#10 lore raw

Re: [PATCH v4 4/4] s390/cio: Remove cond_resched() calls

Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 6 lines

Since [1] cond_resched() is a no-op on s390. Remove all calls.

[1] commit 7dadeaa6e851 ("sched: Further restrict the preemption modes")

Reviewed-by Peter Oberparleiter <[email protected]>
Signed-off-by Heiko Carstens <[email protected]>

Sashiko has reviewed this patch and found no issues. It looks great!

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
maintainer Applied
#11 lore raw
In reply to Heiko Carstens
Quoted 39 lines · Heiko Carstens

v4:
- Replaced first patch with version from Peter Zijlstra

v3:
- Drop uv_call_sched() [Claudio Imbrenda]

v2:
- Use msleep() in crypto code (Holger Dengler)
- Add Reviewed-by tag from Peter Oberparleiter

v1:
Since [1] it is only possible to select the PREEMPT or PREEMPT_LAZY preemption models for s390. This has caused some minor problems, but everything has been addressed.

Given that for both preemption models cond_resched() is a no-op remove or replace it everywhere.

[1] commit 7dadeaa6e851 ("sched: Further restrict the preemption modes")

Heiko Carstens (3):
KVM: s390: Remove cond_resched() calls
s390: Remove cond_resched() calls
s390/cio: Remove cond_resched() calls

Peter Zijlstra (1):
s390/crypto: Replace cond_resched() with msleep(1)

 arch/s390/crypto/paes_s390.c    | 44 +++------------------------------
 arch/s390/crypto/phmac_s390.c   | 25 +++----------------
 arch/s390/include/asm/pkey.h    | 16 ++++++++++++
 arch/s390/include/asm/uv.h      | 16 ++----------
 arch/s390/kernel/perf_cpum_cf.c |  1 -
 arch/s390/kernel/uv.c           |  2 +-
 arch/s390/kvm/gaccess.c         |  8 ++----
 arch/s390/kvm/gmap.c            |  4 ---
 arch/s390/kvm/kvm-s390.c        |  1 -
 arch/s390/kvm/pv.c              | 11 ++++-----
 arch/s390/kvm/vsie.c            |  1 -
Quoted 8 lines
 arch/s390/mm/cmm.c              |  2 --
 arch/s390/mm/gmap_helpers.c     |  1 -
 arch/s390/mm/pageattr.c         |  5 ----
 arch/s390/mm/pgalloc.c          |  1 -
 arch/s390/pci/pci.c             |  4 +--
 drivers/s390/char/uvdevice.c    |  6 ++---
 drivers/s390/cio/css.c          |  8 ------
 18 files changed, 37 insertions(+), 119 deletions(-)

Applied, thank you!