[PATCH v4] s390/diag204: Introduce diag204 rework

[PATCH v4] s390/diag204: Introduce diag204 rework

[Mete Durlu]

diag204 is an expensive call, limit its invocation to once per
second at most for all users. Use a caching mechanism to provide diag204
data at demand. Cache buffer gets freed by a delayed work function if
diag204 data is not fetched after a certain amount of time.
Replace the existing diag204 users in kernelspace with the new diag204
interface.

Signed-off-by: Mete Durlu <meted@linux.ibm.com>
---
v4:
- Drop diag204's IOCTL interface patch as there is no users right now.
- Refactor diag204_store() function and utilize wait_event()/wake_up()
  pairs.
- Remove check for the case where user specifies allow_busy but
  busy indication facility is not present. Automatically fallback to
  blocking case.
- Remove diag204_probe() and put its logic to diag204_init().
- More small changes to keep the added line count low.
- Squash patch 1 and 2 

v3:
- Fix memory leak of extended buffer in diag204_probe() in fallback case
  (Sashiko)
- Add an ioctl definition for userspace to extract diag204 subcode
  (Sashiko)
- Fix unlocked cache access and mutex unlock in diag204_store()
  (Sashiko)
- Replace diag204 ioctl struct data types (Sashiko)
- Change diag204's allow_busy and allow_busy_wait flags for
  hypfs_diag_create_files() to match usecase before the rework (Sashiko)
- Add a check for the case where user specifies allow_busy but
  busy indication facility is not present.

v2: 
- prevent redundant work rearm after cache free in diag204_work_fn().
  (Sashiko)
- always fallback to subcode 4 in diag204_probe(). (Sashiko)
- fix errno check during busy wait loop and prevent uncessary
  sleeps. (Sashiko)
- simplify diag204 cache update function, allow for diag to return busy
  even when diag cache is empty. (Sashiko)
- add a check for diag204 subcode in sthyi as it depends on subcode 7
  being available. (Sashiko)
- remove CAP_SYS_ADMIN check in IOCTL handler (Hendrik Brueckner)
- add missing READ_ONCE() to g_cache.timestamp
- schedule delayed work only after a successful cache update
- rebase patches

Changes from the earlier series;
* Address mutex lockdep issues (Sumanth Korikkar)
* Add permission check for IOCTL (Sumanth Korikkar)
* Pass buffers set to 0 when calling diag (Sumanth Korikkar)

Background
========================================================================
The goal is to create a single source where all relevant diag204
functionality is exposed and managed.

The main problems being addressed are;
* Diag204 is expensive and calling it too frequently should be avoided.
* Diag204 requires a probe phase to utilize properly which means code
  duplication usage for each user.

This is also a prerequisite for utilizing diag204 data in hiperdispatch.

New Diag204 Usage
========================================================================
Users from kernel are now exposed to a couple of common functions 
which should allow for a simpler way of utilizing diag204.
The new streamlined flow now looks like;

...
{
	void *buf = diag204_allocate_buffer();
	int rc = diag204_store(buf, allow_busy, allow_busy_wait);

	/* Do whatever with the diag data*/

	diag204_free_buffer(buf);
...

Most of the details of diag204 is now hidden away. Initial probing and
availability check is done via a device_initcall().

Implementation
========================================================================
Initial checks for diag204 support, determining which subcode to use and
querying page count required to hold diag data are all done in a
device_initcall() once. Page count returned is the maximum amount
required for that CEC, subcodes and pages required should stay the same
during runtime.

Diag204 should be rate limited to a diag call per second at most.
In order to achive the rate limiting while also allowing users to
acquire data a caching mechanism similar to Sumanth's diag324
implementation with commit 90e6f191e1ee ("s390/diag324: Retrieve power
readings via diag 0x324") is proposed.
When the cache is valid users can take a mutex copy the cache and
release. If the cache is expired, only the first user trying to acquire
it should update it and after the update wake up any subsequent readers
waiting to read the cache.

The caching mechanism is slightly more complicated to allow support for
interruptible busy waiting. When diag204's busy indication facility is
installed users can specify a flag to put diag204_store() into a
sleepable and interruptible loop that lasts until diag204 is no longer
busy. This way the amount of diag calls and system calls from userspace
are reduced.
If busy indication facility is not present default fallback is the
blocking diag204 call as before. Users don't get an indication about
this.(IMO, it wouldn't be useful)

Diag204 calls are serialized with a mutex and cache updates are
protected with help of a flag. Since busy wait loops can sleep they have
to release the mutex before sleep. The updating cache flag comes in
here, it is only modified when the mutex is being held and if a cache
update is taking place.

Utilizing diag204 arms a work function which periodically checks if the
current cache is expired. If it finds that the cache is expired, it
frees the cache buffer and stops.

Considerations
========================================================================
* At the moment diag204 size request returns a page count requirement of
  423. That is the maximum amount of space needed currently for a CEC to
  service diag204.
  With this series at times there will be ~6Mb (1.6Mb for each; cache,
  temp, user buffer...) memory allocated for diag204 data per lpar.
---
 arch/s390/hypfs/hypfs_diag.c    | 139 ++-------------------
 arch/s390/hypfs/hypfs_diag.h    |   4 +-
 arch/s390/hypfs/hypfs_diag_fs.c |   9 +-
 arch/s390/include/asm/diag.h    |   8 +-
 arch/s390/kernel/diag/Makefile  |   2 +-
 arch/s390/kernel/diag/diag204.c | 271 ++++++++++++++++++++++++++++++++++++++++
 arch/s390/kernel/sthyi.c        |  30 ++---
 7 files changed, 306 insertions(+), 157 deletions(-)

diff --git a/arch/s390/hypfs/hypfs_diag.c b/arch/s390/hypfs/hypfs_diag.c
index 08777f57bd24..68fa85690580 100644
--- a/arch/s390/hypfs/hypfs_diag.c
+++ b/arch/s390/hypfs/hypfs_diag.c
@@ -22,139 +22,24 @@
 
 #define DBFS_D204_HDR_VERSION	0
 
-static enum diag204_sc diag204_store_sc;	/* used subcode for store */
-static enum diag204_format diag204_info_type;	/* used diag 204 data format */
-
 static void *diag204_buf;		/* 4K aligned buffer for diag204 data */
-static int diag204_buf_pages;		/* number of pages for diag204 data */
-
-enum diag204_format diag204_get_info_type(void)
-{
-	return diag204_info_type;
-}
-
-static void diag204_set_info_type(enum diag204_format type)
-{
-	diag204_info_type = type;
-}
-
-/* Diagnose 204 functions */
-/*
- * For the old diag subcode 4 with simple data format we have to use real
- * memory. If we use subcode 6 or 7 with extended data format, we can (and
- * should) use vmalloc, since we need a lot of memory in that case. Currently
- * up to 93 pages!
- */
 
-static void diag204_free_buffer(void)
+static void hypfs_diag204_free_buffer(void)
 {
-	vfree(diag204_buf);
+	diag204_free_buffer(diag204_buf);
 	diag204_buf = NULL;
 }
 
-void *diag204_get_buffer(enum diag204_format fmt, int *pages)
+void *hypfs_diag204_get_buffer(void)
 {
-	if (diag204_buf) {
-		*pages = diag204_buf_pages;
+	if (diag204_buf)
 		return diag204_buf;
-	}
-	if (fmt == DIAG204_INFO_SIMPLE) {
-		*pages = 1;
-	} else {/* DIAG204_INFO_EXT */
-		*pages = diag204((unsigned long)DIAG204_SUBC_RSI |
-				 (unsigned long)DIAG204_INFO_EXT, 0, NULL);
-		if (*pages <= 0)
-			return ERR_PTR(-EOPNOTSUPP);
-	}
-	diag204_buf = __vmalloc_node(array_size(*pages, PAGE_SIZE),
-				     PAGE_SIZE, GFP_KERNEL, NUMA_NO_NODE,
-				     __builtin_return_address(0));
+	diag204_buf = diag204_allocate_buffer();
 	if (!diag204_buf)
 		return ERR_PTR(-ENOMEM);
-	diag204_buf_pages = *pages;
 	return diag204_buf;
 }
 
-/*
- * diag204_probe() has to find out, which type of diagnose 204 implementation
- * we have on our machine. Currently there are three possible scanarios:
- *   - subcode 4   + simple data format (only one page)
- *   - subcode 4-6 + extended data format
- *   - subcode 4-7 + extended data format
- *
- * Subcode 5 is used to retrieve the size of the data, provided by subcodes
- * 6 and 7. Subcode 7 basically has the same function as subcode 6. In addition
- * to subcode 6 it provides also information about secondary cpus.
- * In order to get as much information as possible, we first try
- * subcode 7, then 6 and if both fail, we use subcode 4.
- */
-
-static int diag204_probe(void)
-{
-	void *buf;
-	int pages, rc;
-
-	buf = diag204_get_buffer(DIAG204_INFO_EXT, &pages);
-	if (!IS_ERR(buf)) {
-		if (diag204((unsigned long)DIAG204_SUBC_STIB7 |
-			    (unsigned long)DIAG204_INFO_EXT, pages, buf) >= 0) {
-			diag204_store_sc = DIAG204_SUBC_STIB7;
-			diag204_set_info_type(DIAG204_INFO_EXT);
-			goto out;
-		}
-		if (diag204((unsigned long)DIAG204_SUBC_STIB6 |
-			    (unsigned long)DIAG204_INFO_EXT, pages, buf) >= 0) {
-			diag204_store_sc = DIAG204_SUBC_STIB6;
-			diag204_set_info_type(DIAG204_INFO_EXT);
-			goto out;
-		}
-		diag204_free_buffer();
-	}
-
-	/* subcodes 6 and 7 failed, now try subcode 4 */
-
-	buf = diag204_get_buffer(DIAG204_INFO_SIMPLE, &pages);
-	if (IS_ERR(buf)) {
-		rc = PTR_ERR(buf);
-		goto fail_alloc;
-	}
-	if (diag204((unsigned long)DIAG204_SUBC_STIB4 |
-		    (unsigned long)DIAG204_INFO_SIMPLE, pages, buf) >= 0) {
-		diag204_store_sc = DIAG204_SUBC_STIB4;
-		diag204_set_info_type(DIAG204_INFO_SIMPLE);
-		goto out;
-	} else {
-		rc = -EOPNOTSUPP;
-		goto fail_store;
-	}
-out:
-	rc = 0;
-fail_store:
-	diag204_free_buffer();
-fail_alloc:
-	return rc;
-}
-
-int diag204_store(void *buf, int pages)
-{
-	unsigned long subcode;
-	int rc;
-
-	subcode = diag204_get_info_type();
-	subcode |= diag204_store_sc;
-	if (diag204_has_bif())
-		subcode |= DIAG204_BIF_BIT;
-	while (1) {
-		rc = diag204(subcode, pages, buf);
-		if (rc != -EBUSY)
-			break;
-		if (signal_pending(current))
-			return -ERESTARTSYS;
-		schedule_timeout_interruptible(DIAG204_BUSY_WAIT);
-	}
-	return rc < 0 ? rc : 0;
-}
-
 struct dbfs_d204_hdr {
 	u64	len;		/* Length of d204 buffer without header */
 	u16	version;	/* Version of header */
@@ -173,19 +58,19 @@ static int dbfs_d204_create(void **data, void **data_free_ptr, size_t *size)
 	int rc, buf_size;
 	void *base;
 
-	buf_size = PAGE_SIZE * (diag204_buf_pages + 1) + sizeof(d204->hdr);
+	buf_size = diag204_get_buf_size() + PAGE_SIZE + sizeof(d204->hdr);
 	base = vzalloc(buf_size);
 	if (!base)
 		return -ENOMEM;
 	d204 = PTR_ALIGN(base + sizeof(d204->hdr), PAGE_SIZE) - sizeof(d204->hdr);
-	rc = diag204_store(d204->buf, diag204_buf_pages);
-	if (rc) {
+	rc = diag204_store(d204->buf, 1, 1);
+	if (rc < 0) {
 		vfree(base);
 		return rc;
 	}
 	d204->hdr.version = DBFS_D204_HDR_VERSION;
-	d204->hdr.len = PAGE_SIZE * diag204_buf_pages;
-	d204->hdr.sc = diag204_store_sc;
+	d204->hdr.len = diag204_get_buf_size();
+	d204->hdr.sc = diag204_get_subcode();
 	*data = d204;
 	*data_free_ptr = base;
 	*size = d204->hdr.len + sizeof(struct dbfs_d204_hdr);
@@ -202,7 +87,7 @@ __init int hypfs_diag_init(void)
 {
 	int rc;
 
-	if (diag204_probe()) {
+	if (!diag204_is_available()) {
 		pr_info("The hardware system does not support hypfs\n");
 		return -ENODATA;
 	}
@@ -219,6 +104,6 @@ __init int hypfs_diag_init(void)
 void hypfs_diag_exit(void)
 {
 	hypfs_diag_fs_exit();
-	diag204_free_buffer();
+	hypfs_diag204_free_buffer();
 	hypfs_dbfs_remove_file(&dbfs_file_d204);
 }
diff --git a/arch/s390/hypfs/hypfs_diag.h b/arch/s390/hypfs/hypfs_diag.h
index b5218135b8fe..6b5bea39b6f9 100644
--- a/arch/s390/hypfs/hypfs_diag.h
+++ b/arch/s390/hypfs/hypfs_diag.h
@@ -12,9 +12,7 @@
 
 #include <asm/diag.h>
 
-enum diag204_format diag204_get_info_type(void);
-void *diag204_get_buffer(enum diag204_format fmt, int *pages);
-int diag204_store(void *buf, int pages);
+void *hypfs_diag204_get_buffer(void);
 
 int __hypfs_diag_fs_init(void);
 void __hypfs_diag_fs_exit(void);
diff --git a/arch/s390/hypfs/hypfs_diag_fs.c b/arch/s390/hypfs/hypfs_diag_fs.c
index 39621c326010..2ce9119abe99 100644
--- a/arch/s390/hypfs/hypfs_diag_fs.c
+++ b/arch/s390/hypfs/hypfs_diag_fs.c
@@ -303,15 +303,14 @@ int hypfs_diag_create_files(struct dentry *root)
 	struct dentry *systems_dir, *hyp_dir;
 	void *time_hdr, *part_hdr;
 	void *buffer, *ptr;
-	int i, rc, pages;
+	int i, rc;
 
-	buffer = diag204_get_buffer(diag204_get_info_type(), &pages);
+	buffer = hypfs_diag204_get_buffer();
 	if (IS_ERR(buffer))
 		return PTR_ERR(buffer);
-	rc = diag204_store(buffer, pages);
-	if (rc)
+	rc = diag204_store(buffer, 1, 1);
+	if (rc < 0)
 		return rc;
-
 	systems_dir = hypfs_mkdir(root, "systems");
 	if (IS_ERR(systems_dir))
 		return PTR_ERR(systems_dir);
diff --git a/arch/s390/include/asm/diag.h b/arch/s390/include/asm/diag.h
index 1416ee1ad323..ca78ca45a8a2 100644
--- a/arch/s390/include/asm/diag.h
+++ b/arch/s390/include/asm/diag.h
@@ -121,7 +121,6 @@ enum diag204_sc {
 
 #define DIAG204_SUBCODE_MASK 0xffff
 #define DIAG204_BIF_BIT 0x80000000
-#define DIAG204_BUSY_WAIT (HZ / 10)
 
 /* The two available diag 204 data formats */
 enum diag204_format {
@@ -336,6 +335,13 @@ static inline bool diag204_has_bif(void)
 	return sclp.has_diag204_bif;
 }
 
+int diag204_store(void *buf, int allow_busy, int allow_busy_wait);
+enum diag204_format diag204_get_info_type(void);
+enum diag204_sc diag204_get_subcode(void);
+void *diag204_allocate_buffer(void);
+void diag204_free_buffer(void *buf);
+bool diag204_is_available(void);
+int diag204_get_buf_size(void);
 int diag204(unsigned long subcode, unsigned long size, void *addr);
 int diag224(void *ptr);
 int diag26c(void *req, void *resp, enum diag26c_sc subcode);
diff --git a/arch/s390/kernel/diag/Makefile b/arch/s390/kernel/diag/Makefile
index 956aee6c4090..6e255b66be44 100644
--- a/arch/s390/kernel/diag/Makefile
+++ b/arch/s390/kernel/diag/Makefile
@@ -1 +1 @@
-obj-y	:= diag_misc.o diag324.o diag.o diag310.o
+obj-y	:= diag_misc.o diag324.o diag.o diag310.o diag204.o
diff --git a/arch/s390/kernel/diag/diag204.c b/arch/s390/kernel/diag/diag204.c
new file mode 100644
index 000000000000..a68961bb8998
--- /dev/null
+++ b/arch/s390/kernel/diag/diag204.c
@@ -0,0 +1,271 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Request CPU utilization information from underlying diag 0x204 facility.
+ *
+ * Copyright IBM Corp. 2026
+ */
+
+#define pr_fmt(fmt) "diag204: " fmt
+
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/timer.h>
+#include <linux/vmalloc.h>
+#include <linux/wait.h>
+
+#include <asm/diag.h>
+
+#define DIAG204_WORK_DELAY (HZ * 10)
+
+struct diag204_cache {
+	wait_queue_head_t wq;		/* wait queue for tasks waiting update */
+	ktime_t timestamp;		/* cache expiration time */
+	enum diag204_format type;	/* used diag204 data format */
+	enum diag204_sc subcode;	/* used diag204 subcode for store */
+	int pages;			/* number of pages for diag204 data */
+	void *buf;			/* page aligned diag204 buffer */
+	bool has_bif;			/* busy indication facility flag */
+	bool updating;			/* ongoing cache update indicator */
+};
+
+static struct diag204_cache g_cache;
+static DEFINE_MUTEX(diag204_mutex);
+
+static void diag204_work_fn(struct work_struct *work);
+static DECLARE_DELAYED_WORK(diag204_work, diag204_work_fn);
+
+bool diag204_is_available(void)
+{
+	return g_cache.pages > 0;
+}
+
+enum diag204_format diag204_get_info_type(void)
+{
+	return g_cache.type;
+}
+
+enum diag204_sc diag204_get_subcode(void)
+{
+	return g_cache.subcode;
+}
+
+int diag204_get_buf_size(void)
+{
+	return g_cache.pages * PAGE_SIZE;
+}
+
+void diag204_free_buffer(void *buf)
+{
+	if (buf)
+		vfree(buf);
+}
+
+static void *__diag204_allocate_buffer(int pages)
+{
+	return __vmalloc_node(array_size(pages, PAGE_SIZE), PAGE_SIZE,
+			      GFP_KERNEL | __GFP_ZERO, NUMA_NO_NODE,
+			      __builtin_return_address(0));
+}
+
+void *diag204_allocate_buffer(void)
+{
+	return __diag204_allocate_buffer(g_cache.pages);
+}
+
+static int diag204_is_cache_valid(void)
+{
+	if (!READ_ONCE(g_cache.buf))
+		return 0;
+	if (ktime_after(ktime_get(), READ_ONCE(g_cache.timestamp)))
+		return 0;
+	return 1;
+}
+
+static void diag204_replace_cache(void *buf)
+{
+	ktime_t expire = 0;
+
+	diag204_free_buffer(READ_ONCE(g_cache.buf));
+	WRITE_ONCE(g_cache.buf, buf);
+	if (buf)
+		expire = ktime_add_ns(ktime_get(), NSEC_PER_SEC);
+	WRITE_ONCE(g_cache.timestamp, expire);
+}
+
+static void diag204_work_fn(struct work_struct *work)
+{
+	mutex_lock(&diag204_mutex);
+	if (!READ_ONCE(g_cache.buf))
+		goto unlock;
+	if (diag204_is_cache_valid())
+		goto rearm;
+	if (READ_ONCE(g_cache.updating))
+		goto rearm;
+	diag204_replace_cache(NULL);
+	wake_up_all(&g_cache.wq);
+	goto unlock;
+rearm:
+	mod_delayed_work(system_dfl_wq, &diag204_work, DIAG204_WORK_DELAY);
+unlock:
+	mutex_unlock(&diag204_mutex);
+}
+
+static int diag204_update_cache(int allow_busy, int allow_busy_wait)
+{
+	unsigned long subcode;
+	void *buf;
+	int rc;
+
+	buf = diag204_allocate_buffer();
+	if (!buf)
+		return -ENOMEM;
+	subcode = g_cache.type | g_cache.subcode;
+	if (g_cache.has_bif && allow_busy)
+		subcode |= DIAG204_BIF_BIT;
+	WRITE_ONCE(g_cache.updating, true);
+	mutex_unlock(&diag204_mutex);
+	do {
+		rc = diag204(subcode, g_cache.pages, buf);
+		if (!allow_busy_wait || rc != -EBUSY)
+			break;
+		if (signal_pending(current)) {
+			rc = -ERESTARTSYS;
+			break;
+		}
+		schedule_timeout_interruptible(HZ);
+	} while (1);
+	mutex_lock(&diag204_mutex);
+	if (rc >= 0) {
+		diag204_replace_cache(buf);
+		mod_delayed_work(system_dfl_wq, &diag204_work, DIAG204_WORK_DELAY);
+	} else {
+		diag204_free_buffer(buf);
+	}
+	WRITE_ONCE(g_cache.updating, false);
+	return rc;
+}
+
+/*
+ * diag204_store() provides users data retrieved from the diag204 facility.
+ * Since diag204 is an expensive call to make, it is limited to a call per
+ * second for now.
+ * diag204_store() first checks if diag204 cache is present and valid. If the
+ * cache is valid, it copies the contents to the users buffer.
+ * If the cache is expired, it first updates the cache and then proceeds with
+ * the same copy operation.
+ *
+ * @buf: Users have to manage their own buffers by using
+ * diag204_allocate_buffer() and diag204_free_buffer().
+ *
+ * @allow_busy: If diag204 supports busy indication facility(bif), users can
+ * specify diag204 to  return BUSY instead of blocking until data becomes ready.
+ *
+ * @allow_busy_wait: If users allow diag204 to use bif, they would also get
+ * the option to specify an interruptible wait behavior by allowing busy wait.
+ * This enables users to interrupt diag204 waiting period via signals.
+ */
+int diag204_store(void *buf, int allow_busy, int allow_busy_wait)
+{
+	int rc;
+
+	if (!diag204_is_available())
+		return -EOPNOTSUPP;
+	if (!allow_busy && allow_busy_wait)
+		return -EOPNOTSUPP;
+	if (!buf)
+		return -ENOMEM;
+	rc = 0;
+	while (1) {
+		rc = mutex_lock_interruptible(&diag204_mutex);
+		if (rc)
+			return rc;
+		/* Exit loop with lock taken if there is no concurrent update */
+		if (!READ_ONCE(g_cache.updating))
+			break;
+		mutex_unlock(&diag204_mutex);
+		if (allow_busy && !allow_busy_wait)
+			return -EBUSY;
+		/* Update in progress, wait until flag is cleared */
+		rc = wait_event_interruptible(g_cache.wq, !READ_ONCE(g_cache.updating));
+		if (rc)
+			return rc;
+	}
+	if (diag204_is_cache_valid()) {
+		memcpy(buf, READ_ONCE(g_cache.buf), diag204_get_buf_size());
+		mutex_unlock(&diag204_mutex);
+		return rc;
+	}
+	/* Update the cache and wake up tasks waiting for the update */
+	rc = diag204_update_cache(allow_busy, allow_busy_wait);
+	if (rc >= 0)
+		memcpy(buf, READ_ONCE(g_cache.buf), diag204_get_buf_size());
+	mutex_unlock(&diag204_mutex);
+	wake_up_all(&g_cache.wq);
+	return rc;
+}
+
+/*
+ * diag204_init() has to find out, which type of diagnose 204 implementation
+ * we have on the machine. Currently there are three possible scanarios:
+ *   - subcode 4   + simple data format (only one page)
+ *   - subcode 4-6 + extended data format
+ *   - subcode 4-7 + extended data format
+ *
+ * Subcode 5 is used to retrieve the size of the data, provided by subcodes
+ * 6 and 7. Subcode 7 basically has the same function as subcode 6. In addition
+ * to subcode 6 it provides also information about secondary cpus.
+ * In order to get as much information as possible, we first try
+ * subcode 7, then 6 and if both fail, we use subcode 4.
+ */
+static int __init diag204_init(void)
+{
+	unsigned long subcode, type;
+	int pages, rc;
+	void *buf;
+
+	pages = diag204((unsigned long)DIAG204_INFO_EXT |
+			(unsigned long)DIAG204_SUBC_RSI, 0, NULL);
+	if (pages > 0) {
+		buf = __diag204_allocate_buffer(pages);
+		if (!buf) {
+			rc = -ENOMEM;
+			goto fail_alloc;
+		}
+		type = DIAG204_INFO_EXT;
+		if (diag204(DIAG204_SUBC_STIB7 | type, pages, buf) >= 0) {
+			subcode = DIAG204_SUBC_STIB7;
+			goto out;
+		}
+		if (diag204(DIAG204_SUBC_STIB6 | type, pages, buf) >= 0) {
+			subcode = DIAG204_SUBC_STIB6;
+			goto out;
+		}
+		vfree(buf);
+	}
+	/* extended data format is not available, fallback to simple */
+	pages = 1;
+	buf = __diag204_allocate_buffer(pages);
+	if (!buf) {
+		rc = -ENOMEM;
+		goto fail_alloc;
+	}
+	type = DIAG204_INFO_SIMPLE;
+	if (diag204(DIAG204_SUBC_STIB4 | type, pages, buf) >= 0) {
+		subcode = DIAG204_SUBC_STIB4;
+		goto out;
+	}
+	rc = -EOPNOTSUPP;
+	goto fail_store;
+out:
+	rc = 0;
+	g_cache.pages = pages;
+	g_cache.subcode = subcode;
+	g_cache.type = type;
+	g_cache.has_bif = diag204_has_bif();
+	init_waitqueue_head(&g_cache.wq);
+fail_store:
+	vfree(buf);
+fail_alloc:
+	return rc;
+}
+device_initcall(diag204_init);
diff --git a/arch/s390/kernel/sthyi.c b/arch/s390/kernel/sthyi.c
index 5eae2e25997a..9adb26588d5b 100644
--- a/arch/s390/kernel/sthyi.c
+++ b/arch/s390/kernel/sthyi.c
@@ -305,29 +305,19 @@ static struct diag204_x_part_block *lpar_cpu_inf(struct lpar_cpu_inf *part_inf,
 
 static void *diag204_get_data(bool diag204_allow_busy)
 {
-	unsigned long subcode;
 	void *diag204_buf;
-	int pages, rc;
-
-	subcode = DIAG204_SUBC_RSI;
-	subcode |= DIAG204_INFO_EXT;
-	pages = diag204(subcode, 0, NULL);
-	if (pages < 0)
-		return ERR_PTR(pages);
-	if (pages == 0)
-		return ERR_PTR(-ENODATA);
-	diag204_buf = __vmalloc_node(array_size(pages, PAGE_SIZE),
-				     PAGE_SIZE, GFP_KERNEL, NUMA_NO_NODE,
-				     __builtin_return_address(0));
+	int rc;
+
+	if (!diag204_is_available())
+		return ERR_PTR(-EOPNOTSUPP);
+	if (diag204_get_subcode() != DIAG204_SUBC_STIB7)
+		return ERR_PTR(-EOPNOTSUPP);
+	diag204_buf = diag204_allocate_buffer();
 	if (!diag204_buf)
 		return ERR_PTR(-ENOMEM);
-	subcode = DIAG204_SUBC_STIB7;
-	subcode |= DIAG204_INFO_EXT;
-	if (diag204_has_bif() && diag204_allow_busy)
-		subcode |= DIAG204_BIF_BIT;
-	rc = diag204(subcode, pages, diag204_buf);
+	rc = diag204_store(diag204_buf, diag204_allow_busy, 0);
 	if (rc < 0) {
-		vfree(diag204_buf);
+		diag204_free_buffer(diag204_buf);
 		return ERR_PTR(rc);
 	}
 	return diag204_buf;
@@ -462,7 +452,7 @@ static int fill_dst(void *dst, u64 *rc)
 	fill_hdr(sctns);
 	fill_stsi(sctns);
 	fill_diag(sctns, diag204_buf);
-	vfree(diag204_buf);
+	diag204_free_buffer(diag204_buf);
 	return 0;
 }
 

Re: [PATCH v4] s390/diag204: Introduce diag204 rework

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] s390/diag204: unexpected signal interruption and -EBUSY in diag204_store()
- [Medium] s390/diag204: misplaced signal_pending() check in diag204_update_cache()
--

commit f09eadeabe189d5ced7bd69c8dae7f95a7f3195f
Author: Mete Durlu <meted@linux.ibm.com>

s390/diag204: Introduce diag204 rework

This commit reworks diag204 support to rate-limit hypervisor calls to at
most once per second using a shared cache, delayed work for buffer cleanup,
and a common kernel interface. Existing in-kernel users in hypfs and sthyi
are updated to use the new interface.

> diff --git a/arch/s390/kernel/diag/diag204.c b/arch/s390/kernel/diag/diag204.c
> new file mode 100644
> index 0000000000000..a68961bb8998d
> --- /dev/null
> +++ b/arch/s390/kernel/diag/diag204.c
> @@ -0,0 +1,271 @@

[ ... ]

> +static int diag204_update_cache(int allow_busy, int allow_busy_wait)
> +{
> +	unsigned long subcode;
> +	void *buf;
> +	int rc;
> +
> +	buf = diag204_allocate_buffer();
> +	if (!buf)
> +		return -ENOMEM;
> +	subcode = g_cache.type | g_cache.subcode;
> +	if (g_cache.has_bif && allow_busy)
> +		subcode |= DIAG204_BIF_BIT;
> +	WRITE_ONCE(g_cache.updating, true);
> +	mutex_unlock(&diag204_mutex);
> +	do {
> +		rc = diag204(subcode, g_cache.pages, buf);
> +		if (!allow_busy_wait || rc != -EBUSY)
> +			break;
> +		if (signal_pending(current)) {
> +			rc = -ERESTARTSYS;
> +			break;
> +		}
> +		schedule_timeout_interruptible(HZ);
> +	} while (1);

[Severity: Medium]
When diag204_update_cache() is called with allow_busy_wait set (for example,
via hypfs_diag_create_files() or dbfs_d204_create()) and diag204() returns
-EBUSY, a signal arriving while the task is sleeping in
schedule_timeout_interruptible(HZ) wakes the task early.

Because the loop immediately starts the next iteration and calls diag204()
before checking signal_pending(current) again, doesn't a signal during the
sleep trigger another expensive diag204() call immediately without waiting
for the 1-second interval, and potentially miss returning -ERESTARTSYS if the
second diag204() call succeeds?

> +	mutex_lock(&diag204_mutex);
> +	if (rc >= 0) {
> +		diag204_replace_cache(buf);
> +		mod_delayed_work(system_dfl_wq, &diag204_work, DIAG204_WORK_DELAY);
> +	} else {
> +		diag204_free_buffer(buf);
> +	}
> +	WRITE_ONCE(g_cache.updating, false);
> +	return rc;
> +}

[ ... ]

> +int diag204_store(void *buf, int allow_busy, int allow_busy_wait)
> +{
> +	int rc;
> +
> +	if (!diag204_is_available())
> +		return -EOPNOTSUPP;
> +	if (!allow_busy && allow_busy_wait)
> +		return -EOPNOTSUPP;
> +	if (!buf)
> +		return -ENOMEM;
> +	rc = 0;
> +	while (1) {
> +		rc = mutex_lock_interruptible(&diag204_mutex);
> +		if (rc)
> +			return rc;
> +		/* Exit loop with lock taken if there is no concurrent update */
> +		if (!READ_ONCE(g_cache.updating))
> +			break;
> +		mutex_unlock(&diag204_mutex);
> +		if (allow_busy && !allow_busy_wait)
> +			return -EBUSY;
> +		/* Update in progress, wait until flag is cleared */
> +		rc = wait_event_interruptible(g_cache.wq, !READ_ONCE(g_cache.updating));
> +		if (rc)
> +			return rc;
> +	}

[Severity: High]
Can this loop in diag204_store() return unexpected -EINTR, -ERESTARTSYS, or
-EBUSY errors to callers that pass allow_busy_wait as 0?

Specifically, sthyi_fill() -> sthyi_update_cache() -> fill_dst() ->
diag204_get_data() calls diag204_store(diag204_buf, diag204_allow_busy, 0):

arch/s390/kernel/sthyi.c:diag204_get_data() {
	...
	rc = diag204_store(diag204_buf, diag204_allow_busy, 0);
	if (rc < 0) {
		diag204_free_buffer(diag204_buf);
		return ERR_PTR(rc);
	}
	...
}

Even though allow_busy_wait is 0, diag204_store() unconditionally uses
mutex_lock_interruptible() and wait_event_interruptible(), which return
-EINTR or -ERESTARTSYS if a signal or notify_signal is pending while
contending on diag204_mutex or waiting for g_cache.updating.

When sthyi_fill() is invoked from KVM's SIE intercept handler in
arch/s390/kvm/s390/intercept.c:handle_sthyi():

	cc = sthyi_fill(sctns, &rc);
	if (cc < 0)
		return cc;

and arch/s390/kvm/s390/s390.c:kvm_arch_vcpu_ioctl_run():

	rc = __vcpu_run(vcpu);

	if (signal_pending(current) && !rc) {
		kvm_run->exit_reason = KVM_EXIT_INTR;
		vcpu->stat.signal_exits++;
		rc = -EINTR;
	}

if a signal is pending on the vCPU thread during a concurrent diag204 cache
update, sthyi_fill() fails with -EINTR or -ERESTARTSYS after SIE has already
advanced the guest PSW past the STHYI instruction, leaving the guest buffer
and condition code unupdated and preventing kvm_arch_vcpu_ioctl_run() from
setting KVM_EXIT_INTR.

Also, when g_cache.updating is true and allow_busy && !allow_busy_wait holds,
diag204_store() returns -EBUSY without checking g_cache.has_bif, whereas
diag204_update_cache() checks g_cache.has_bif && allow_busy. Should the
concurrent update check also gate -EBUSY on g_cache.has_bif, and use
uninterruptible locking and waiting when allow_busy_wait is 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-diag204ioctl-v4-1-d5e90b56112b@linux.ibm.com?part=1

Re: [PATCH v4] s390/diag204: Introduce diag204 rework

[Heiko Carstens]

On Tue, Oct 27, 2026 at 11:40:25AM +0100, Mete Durlu wrote:
> +static struct diag204_cache g_cache;

Before you send yet another version: what is "g_cache"? That seems to
be a horrible name for a global variable. What does that even stand
for? Please change the name to something more descriptive.

> +/*
> + * diag204_store() provides users data retrieved from the diag204 facility.
> + * Since diag204 is an expensive call to make, it is limited to a call per
> + * second for now.
> + * diag204_store() first checks if diag204 cache is present and valid. If the
> + * cache is valid, it copies the contents to the users buffer.
> + * If the cache is expired, it first updates the cache and then proceeds with
> + * the same copy operation.
> + *
> + * @buf: Users have to manage their own buffers by using
> + * diag204_allocate_buffer() and diag204_free_buffer().
> + *
> + * @allow_busy: If diag204 supports busy indication facility(bif), users can
> + * specify diag204 to  return BUSY instead of blocking until data becomes ready.
> + *
> + * @allow_busy_wait: If users allow diag204 to use bif, they would also get
> + * the option to specify an interruptible wait behavior by allowing busy wait.
> + * This enables users to interrupt diag204 waiting period via signals.
> + */

Is that AI generated text? That doesn't read like normal kernel documentation.
And since this looks close to kernel-doc format you could also make that
kernel-doc. But make it shorter and don't describe the implementation details.

> + * diag204_init() has to find out, which type of diagnose 204 implementation
> + * we have on the machine. Currently there are three possible scanarios:
> + *   - subcode 4   + simple data format (only one page)
> + *   - subcode 4-6 + extended data format
> + *   - subcode 4-7 + extended data format
> + *
> + * Subcode 5 is used to retrieve the size of the data, provided by subcodes
> + * 6 and 7. Subcode 7 basically has the same function as subcode 6. In addition
> + * to subcode 6 it provides also information about secondary cpus.
> + * In order to get as much information as possible, we first try
> + * subcode 7, then 6 and if both fail, we use subcode 4.
> + */

This looks like it was copied from the old code: please fix the documentation
convert to use imperative mood when you move it.

Re: [PATCH v4] s390/diag204: Introduce diag204 rework

[Mete Durlu]

On 27/10/2026 12:32, Heiko Carstens wrote:
> On Tue, Oct 27, 2026 at 11:40:25AM +0100, Mete Durlu wrote:
>> +static struct diag204_cache g_cache;
> 
> Before you send yet another version: what is "g_cache"? That seems to
> be a horrible name for a global variable. What does that even stand
> for? Please change the name to something more descriptive.

Just global cache, but you are right there is nothing to associate
it with diag204. I'll replace it.

>> +/*
>> + * diag204_store() provides users data retrieved from the diag204 facility.
>> + * Since diag204 is an expensive call to make, it is limited to a call per
>> + * second for now.
>> + * diag204_store() first checks if diag204 cache is present and valid. If the
>> + * cache is valid, it copies the contents to the users buffer.
>> + * If the cache is expired, it first updates the cache and then proceeds with
>> + * the same copy operation.
>> + *
>> + * @buf: Users have to manage their own buffers by using
>> + * diag204_allocate_buffer() and diag204_free_buffer().
>> + *
>> + * @allow_busy: If diag204 supports busy indication facility(bif), users can
>> + * specify diag204 to  return BUSY instead of blocking until data becomes ready.
>> + *
>> + * @allow_busy_wait: If users allow diag204 to use bif, they would also get
>> + * the option to specify an interruptible wait behavior by allowing busy wait.
>> + * This enables users to interrupt diag204 waiting period via signals.
>> + */
> 
> Is that AI generated text? That doesn't read like normal kernel documentation.

No, AI would probably make a better job out of it :D

> And since this looks close to kernel-doc format you could also make that
> kernel-doc. But make it shorter and don't describe the implementation details.

I wrote this without knowing kernel-doc format tbh. When I first
started writing this series there was also a patch series in
discussion targeting s390/uv. I saw the function comments there
and wanted to have something similar here too.

I will just properly read kernel-doc format before making changes.

>> + * diag204_init() has to find out, which type of diagnose 204 implementation
>> + * we have on the machine. Currently there are three possible scanarios:
>> + *   - subcode 4   + simple data format (only one page)
>> + *   - subcode 4-6 + extended data format
>> + *   - subcode 4-7 + extended data format
>> + *
>> + * Subcode 5 is used to retrieve the size of the data, provided by subcodes
>> + * 6 and 7. Subcode 7 basically has the same function as subcode 6. In addition
>> + * to subcode 6 it provides also information about secondary cpus.
>> + * In order to get as much information as possible, we first try
>> + * subcode 7, then 6 and if both fail, we use subcode 4.
>> + */
> 
> This looks like it was copied from the old code: please fix the documentation
> convert to use imperative mood when you move it.

Will do! Should it retain the details about which subcodes and
data formats can exist together?

Thank you!

Re: [PATCH v4] s390/diag204: Introduce diag204 rework

[Heiko Carstens]

On Tue, Oct 27, 2026 at 12:53:22PM +0100, Mete Durlu wrote:
> > > + * diag204_init() has to find out, which type of diagnose 204 implementation
> > > + * we have on the machine. Currently there are three possible scanarios:
> > > + *   - subcode 4   + simple data format (only one page)
> > > + *   - subcode 4-6 + extended data format
> > > + *   - subcode 4-7 + extended data format
> > > + *
> > > + * Subcode 5 is used to retrieve the size of the data, provided by subcodes
> > > + * 6 and 7. Subcode 7 basically has the same function as subcode 6. In addition
> > > + * to subcode 6 it provides also information about secondary cpus.
> > > + * In order to get as much information as possible, we first try
> > > + * subcode 7, then 6 and if both fail, we use subcode 4.
> > > + */
> > 
> > This looks like it was copied from the old code: please fix the documentation
> > convert to use imperative mood when you move it.
> 
> Will do! Should it retain the details about which subcodes and
> data formats can exist together?

Sure, I don't see a reason to remove information.

FWIW, it would be helpful if you would reply-to-all, and not only the list :)