[PATCH v3 0/3] s390: Diag204 Rework

[PATCH v3 0/3] s390: Diag204 Rework

[Mete Durlu]

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 can cause
  performance issues and also data integrity problems. Rate limiting is
  required.
* Diag204 requires a prep phase to utilize properly which means code
  duplication for each user.
* Diag204's current interface in debugfs may not be easily reachable
  as debugfs is seen as a security vulnarability by some distributions.
  The new IOCTL diag device should offer alternartive for diag204.

This is also a prerequisite for utilizing diag204 data in hiperdispatch.
There are plans to incorporate utilization data for making smarter
decisions.

Patches
========================================================================
Patch 1-2:
This patch is split into two, first half is introducing the new diag204
implementation and the second half is replacing the exisiting users.
This is purely for easier review. Before accepted I expect first two
patches to be squashed.

Patch 1:
The first patch introduces a caching mechanism inspired by Sumanth
Korikkar's diag324 implementation. Data buffer gets cached and cache
access is serialized via mutexes. If unused for a defined period diag204
data cache gets freed by a worker after a delay, that way it is
guaranteed that if an LPAR has no diag204 users, no memory is consumed.

Patch 2:
The second patch adapts the existing diag204 users to the new
interface.

Patch 3:
The third patch brings diag204 functionality to the new diag misc device.
Misc device gets two new magic values for diag204, one to fetch the data
size and the other is to actually get the diag data.
The uapi now has a new struct diag204_request for requesting the data.
Struct holds a pointer address to the userspace buffer and two behavior
altering flags; allow_busy and allow_busy_loop. Impact of said flags
are explained in Considerations section below.

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 should also 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 is proposed.
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.

IOCTL Interface
========================================================================
A similar IOCTL interface to existing diag calls is implemented.
Two new ioctl definitions along with a new data struct is added to the
uapi. DIAG204_GET_LEN allows userspace to acquire required buffer
length and DIAG204_GET_BUF fetches the diag204 data.
DIAG_204_GET_SUBCODE enables users from userspace to figure out which
diag204 subcode is being used to fetch data. Using the subcode users can
determine which data types need to be mapped to the diag204 buffer.
When issuing an IOCTL call with DIAG204_GET_BUF users have to utilize
the new diag204_request struct to specify the allocated buffer address
along with the allow_busy an allow_busy_wait flags for diag204 store
operation.

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.

* With the new IOCTL interface, userspace is defaulted to the
  largest diag204 data request subcode available(from subcodes 4, 6 and
  7 always the largest available is used as it returns more data).
  While this aligns with the existing behavior, question is, would
  there be any value in allowing users to specify which subcode to use?
  Although this also means more complicated implementation.

* A new addition with the IOCTL interface are the allow_busy and
  allow_busy_wait toggles. For sure there is benefit in allowing diag204
  to return BUSY instead of waiting for longer durations when it is not
  ready at the moment. allow_busy_wait adds an interruptable polling
  loop around that, with that mechanism diag204 users from userspace can
  wait for diag204 to return from its BUSY state within the same system
  call. Obviously using only a single system call is better but diag204
  users are not performance critical atm and the same polling can be
  implemented from userspace side. This can help reduce some complexity
  on the kernel code.

---
---
base-commit: a804c5742bff224d4cb4d25dd383554d598be32b

Mete Durlu (3):
      s390/diag204: Introduce diag204 rework
      s390/diag204: Replace diag204 users with new implementation
      s390/diag: Add diag204 IOCTL to diag misc device

 arch/s390/hypfs/hypfs_diag.c       | 140 ++-------------
 arch/s390/hypfs/hypfs_diag.h       |   4 +-
 arch/s390/hypfs/hypfs_diag_fs.c    |   9 +-
 arch/s390/include/asm/diag.h       |   8 +-
 arch/s390/include/uapi/asm/diag.h  |   9 +
 arch/s390/kernel/diag/Makefile     |   2 +-
 arch/s390/kernel/diag/diag204.c    | 343 +++++++++++++++++++++++++++++++++++++
 arch/s390/kernel/diag/diag_ioctl.h |   4 +
 arch/s390/kernel/diag/diag_misc.c  |   9 +
 arch/s390/kernel/sthyi.c           |  30 ++--
 10 files changed, 401 insertions(+), 157 deletions(-)

[PATCH v3 1/3] 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.

Signed-off-by: Mete Durlu <meted@linux.ibm.com>
---
 arch/s390/include/asm/diag.h    |   7 +
 arch/s390/kernel/diag/diag204.c | 293 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 300 insertions(+)

diff --git a/arch/s390/include/asm/diag.h b/arch/s390/include/asm/diag.h
index 1416ee1ad323..8669cfa039d3 100644
--- a/arch/s390/include/asm/diag.h
+++ b/arch/s390/include/asm/diag.h
@@ -336,6 +336,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/diag204.c b/arch/s390/kernel/diag/diag204.c
new file mode 100644
index 000000000000..f447bfaf756b
--- /dev/null
+++ b/arch/s390/kernel/diag/diag204.c
@@ -0,0 +1,293 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Request CPU utilization information from underlying diag 0x204
+ * facility. Diag 0x204 provides a variety of utilization values.
+ *
+ * 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 <asm/diag.h>
+
+struct diag204_info {
+	int pages;			/* number of pages for diag204 data */
+	enum diag204_format type;	/* used diag204 data format */
+	enum diag204_sc subcode;	/* used diag204 subcode for store */
+	bool has_bif;
+};
+
+struct diag204_cache {
+	ktime_t timestamp;
+	void *buf;			/* page aligned diag204 buffer */
+	bool updating;			/* ongoing cache update indicator */
+};
+
+static struct diag204_cache g_cache;
+static struct diag204_info g_info;
+static DEFINE_MUTEX(diag204_mutex);
+
+#define DIAG204_CACHE_EXPIRY (NSEC_PER_SEC)
+#define DIAG204_WORK_DELAY (HZ * 10)
+#define DIAG204_BUSY_WAIT (HZ / 10)
+
+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_info.pages > 0;
+}
+
+enum diag204_format diag204_get_info_type(void)
+{
+	return g_info.type;
+}
+
+enum diag204_sc diag204_get_subcode(void)
+{
+	return g_info.subcode;
+}
+
+int diag204_get_buf_size(void)
+{
+	return g_info.pages * PAGE_SIZE;
+}
+
+void diag204_free_buffer(void *buf)
+{
+	if (!buf)
+		return;
+	vfree(buf);
+}
+
+/*
+ * 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 423 pages!
+ */
+static void *__diag204_allocate_buffer(int pages)
+{
+	void *buf;
+
+	buf = __vmalloc_node(array_size(pages, PAGE_SIZE), PAGE_SIZE,
+			     GFP_KERNEL | __GFP_ZERO, NUMA_NO_NODE,
+			     __builtin_return_address(0));
+	return buf;
+}
+
+void *diag204_allocate_buffer(void)
+{
+	return __diag204_allocate_buffer(g_info.pages);
+}
+
+static void diag204_schedule_work(void)
+{
+	mod_delayed_work(system_dfl_wq, &diag204_work, DIAG204_WORK_DELAY);
+}
+
+static int diag204_is_cache_valid(unsigned long duration_ns)
+{
+	ktime_t expire;
+
+	lockdep_assert_held(&diag204_mutex);
+	if (!READ_ONCE(g_cache.buf))
+		return 0;
+	expire = ktime_add_ns(READ_ONCE(g_cache.timestamp), duration_ns);
+	if (ktime_after(ktime_get(), expire))
+		return 0;
+	return 1;
+}
+
+static void diag204_set_cache(void *buf)
+{
+	diag204_free_buffer(READ_ONCE(g_cache.buf));
+	WRITE_ONCE(g_cache.buf, buf);
+	if (!buf)
+		WRITE_ONCE(g_cache.timestamp, 0);
+	else
+		WRITE_ONCE(g_cache.timestamp, ktime_get());
+}
+
+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(DIAG204_CACHE_EXPIRY))
+		goto rearm;
+	if (READ_ONCE(g_cache.updating))
+		goto rearm;
+	diag204_set_cache(NULL);
+	goto unlock;
+rearm:
+	diag204_schedule_work();
+unlock:
+	mutex_unlock(&diag204_mutex);
+}
+
+static int diag204_update_cache(int allow_busy, int allow_busy_wait)
+{
+	unsigned long subcode;
+	void *buf;
+	int rc;
+
+	lockdep_assert_held(&diag204_mutex);
+	buf = diag204_allocate_buffer();
+	if (!buf)
+		return -ENOMEM;
+	subcode = g_info.type | g_info.subcode;
+	if (g_info.has_bif && allow_busy)
+		subcode |= DIAG204_BIF_BIT;
+	WRITE_ONCE(g_cache.updating, true);
+	mutex_unlock(&diag204_mutex);
+	do {
+		rc = diag204(subcode, g_info.pages, buf);
+		if (rc != -EBUSY || !allow_busy_wait)
+			break;
+		if (signal_pending(current)) {
+			rc = -ERESTARTSYS;
+			break;
+		}
+		schedule_timeout_interruptible(DIAG204_BUSY_WAIT);
+	} while (allow_busy_wait);
+	mutex_lock(&diag204_mutex);
+	if (rc >= 0) {
+		diag204_set_cache(buf);
+		diag204_schedule_work();
+	} 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. To circumvent this limitation a caching mechanism is used.
+ *
+ * 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 utilize it and 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 is useful since it gives 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 (allow_busy && !g_info.has_bif)
+		return -EINVAL;
+	if (!buf)
+		return -ENOMEM;
+	rc = 0;
+	do {
+		mutex_lock(&diag204_mutex);
+		/* 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;
+		if (signal_pending(current))
+			return -ERESTARTSYS;
+		schedule_timeout_interruptible(DIAG204_BUSY_WAIT);
+	} while (1);
+	if (!diag204_is_cache_valid(DIAG204_CACHE_EXPIRY))
+		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);
+	return rc;
+}
+
+/*
+ * 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)
+{
+	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_info.pages = pages;
+	g_info.subcode = subcode;
+	g_info.type = type;
+	g_info.has_bif = diag204_has_bif();
+fail_store:
+	vfree(buf);
+fail_alloc:
+	return rc;
+}
+
+static int __init diag204_init(void)
+{
+	return diag204_probe();
+}
+device_initcall(diag204_init);

-- 
2.56.0

[PATCH v3 2/3] s390/diag204: Replace diag204 users with new implementation

[Mete Durlu]

Replace the existing diag204 users in kernelspace with the new diag204
interface.

Signed-off-by: Mete Durlu <meted@linux.ibm.com>
---
 arch/s390/hypfs/hypfs_diag.c    | 140 ++++------------------------------------
 arch/s390/hypfs/hypfs_diag.h    |   4 +-
 arch/s390/hypfs/hypfs_diag_fs.c |   9 ++-
 arch/s390/include/asm/diag.h    |   1 -
 arch/s390/kernel/diag/Makefile  |   2 +-
 arch/s390/kernel/sthyi.c        |  30 +++------
 6 files changed, 29 insertions(+), 157 deletions(-)

diff --git a/arch/s390/hypfs/hypfs_diag.c b/arch/s390/hypfs/hypfs_diag.c
index 08777f57bd24..c3eebe4667fa 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,20 @@ 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();
+	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 +88,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 +105,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 8669cfa039d3..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 {
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/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;
 }
 

-- 
2.56.0

[PATCH v3 3/3] s390/diag: Add diag204 IOCTL to diag misc device

[Mete Durlu]

Add IOCTL implementation necessary to fetch diag204 data from the new
diag misc device under /dev.

Signed-off-by: Mete Durlu <meted@linux.ibm.com>
---
 arch/s390/include/uapi/asm/diag.h  |  9 +++++++
 arch/s390/kernel/diag/diag204.c    | 50 ++++++++++++++++++++++++++++++++++++++
 arch/s390/kernel/diag/diag_ioctl.h |  4 +++
 arch/s390/kernel/diag/diag_misc.c  |  9 +++++++
 4 files changed, 72 insertions(+)

diff --git a/arch/s390/include/uapi/asm/diag.h b/arch/s390/include/uapi/asm/diag.h
index b7e6ccb4ff6e..e3d09a450a70 100644
--- a/arch/s390/include/uapi/asm/diag.h
+++ b/arch/s390/include/uapi/asm/diag.h
@@ -22,11 +22,20 @@ struct diag310_memtop {
 	__u64 nesting_lvl;
 };
 
+struct diag204_request {
+	__u64 address;
+	__u32 allow_busy;
+	__u32 allow_busy_wait;
+};
+
 /* Diag ioctl definitions */
 #define DIAG324_GET_PIBBUF	_IOWR(DIAG_MAGIC_STR, 0x77, struct diag324_pib)
 #define DIAG324_GET_PIBLEN	_IOR(DIAG_MAGIC_STR, 0x78, size_t)
 #define DIAG310_GET_STRIDE	_IOR(DIAG_MAGIC_STR, 0x79, size_t)
 #define DIAG310_GET_MEMTOPLEN	_IOWR(DIAG_MAGIC_STR, 0x7a, size_t)
 #define DIAG310_GET_MEMTOPBUF	_IOWR(DIAG_MAGIC_STR, 0x7b, struct diag310_memtop)
+#define DIAG204_GET_SUBCODE	_IOR(DIAG_MAGIC_STR, 0x7c, size_t)
+#define DIAG204_GET_LEN		_IOR(DIAG_MAGIC_STR, 0x7d, size_t)
+#define DIAG204_GET_BUF		_IOR(DIAG_MAGIC_STR, 0x7e, struct diag204_request)
 
 #endif /* __S390_UAPI_ASM_DIAG_H */
diff --git a/arch/s390/kernel/diag/diag204.c b/arch/s390/kernel/diag/diag204.c
index f447bfaf756b..237303aa44d6 100644
--- a/arch/s390/kernel/diag/diag204.c
+++ b/arch/s390/kernel/diag/diag204.c
@@ -14,6 +14,8 @@
 #include <linux/vmalloc.h>
 
 #include <asm/diag.h>
+#include <uapi/asm/diag.h>
+#include "diag_ioctl.h"
 
 struct diag204_info {
 	int pages;			/* number of pages for diag204 data */
@@ -222,6 +224,54 @@ int diag204_store(void *buf, int allow_busy, int allow_busy_wait)
 	return rc;
 }
 
+long diag204_subcode(unsigned long arg)
+{
+	if (!diag204_is_available())
+		return -EOPNOTSUPP;
+	if (put_user(diag204_get_subcode(), (size_t __user *)arg))
+		return -EFAULT;
+	return 0;
+}
+
+long diag204_len(unsigned long arg)
+{
+	if (!diag204_is_available())
+		return -EOPNOTSUPP;
+	if (put_user(diag204_get_buf_size(), (size_t __user *)arg))
+		return -EFAULT;
+	return 0;
+}
+
+long diag204_buf(unsigned long arg)
+{
+	struct diag204_request __user *udata;
+	int rc, allow_busy, allow_busy_wait;
+	u64 address;
+	void *buf;
+
+	if (!diag204_is_available())
+		return -EOPNOTSUPP;
+	udata = (struct diag204_request __user *)arg;
+	if (get_user(address, &udata->address))
+		return -EFAULT;
+	if (get_user(allow_busy, &udata->allow_busy))
+		return -EFAULT;
+	if (get_user(allow_busy_wait, &udata->allow_busy_wait))
+		return -EFAULT;
+	buf = diag204_allocate_buffer();
+	if (!buf)
+		return -ENOMEM;
+	rc = diag204_store(buf, allow_busy, allow_busy_wait);
+	if (rc < 0) {
+		diag204_free_buffer(buf);
+		return rc;
+	}
+	rc = copy_to_user((void __user *)address, buf,
+			  diag204_get_buf_size());
+	diag204_free_buffer(buf);
+	return rc ? -EFAULT : 0;
+}
+
 /*
  * diag204_probe() has to find out, which type of diagnose 204 implementation
  * we have on our machine. Currently there are three possible scanarios:
diff --git a/arch/s390/kernel/diag/diag_ioctl.h b/arch/s390/kernel/diag/diag_ioctl.h
index 7080be946785..68241c67451a 100644
--- a/arch/s390/kernel/diag/diag_ioctl.h
+++ b/arch/s390/kernel/diag/diag_ioctl.h
@@ -11,4 +11,8 @@ long diag310_memtop_stride(unsigned long arg);
 long diag310_memtop_len(unsigned long arg);
 long diag310_memtop_buf(unsigned long arg);
 
+long diag204_subcode(unsigned long arg);
+long diag204_len(unsigned long arg);
+long diag204_buf(unsigned long arg);
+
 #endif /* _DIAG_IOCTL_H */
diff --git a/arch/s390/kernel/diag/diag_misc.c b/arch/s390/kernel/diag/diag_misc.c
index efffe02ea02e..ea7ee9ed805a 100644
--- a/arch/s390/kernel/diag/diag_misc.c
+++ b/arch/s390/kernel/diag/diag_misc.c
@@ -35,6 +35,15 @@ static long diag_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	case DIAG310_GET_MEMTOPBUF:
 		rc = diag310_memtop_buf(arg);
 		break;
+	case DIAG204_GET_SUBCODE:
+		rc = diag204_subcode(arg);
+		break;
+	case DIAG204_GET_LEN:
+		rc = diag204_len(arg);
+		break;
+	case DIAG204_GET_BUF:
+		rc = diag204_buf(arg);
+		break;
 	default:
 		rc = -ENOIOCTLCMD;
 		break;

-- 
2.56.0

Re: [PATCH v3 2/3] s390/diag204: Replace diag204 users with new implementation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] s390/diag204: diag204_store() calls fail with -EINVAL without BIF support
--

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

s390/diag204: Replace diag204 users with new implementation

This commit converts existing kernelspace diagnose 204 users in hypfs and
sthyi to use the new centralized diag204 interface.

> diff --git a/arch/s390/hypfs/hypfs_diag.c b/arch/s390/hypfs/hypfs_diag.c
> index 08777f57bd244..c3eebe4667faf 100644
> --- a/arch/s390/hypfs/hypfs_diag.c
> +++ b/arch/s390/hypfs/hypfs_diag.c
> @@ -22,139 +22,24 @@

[ ... ]

> -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,20 @@ 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();
> +	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;
>  	}

[ ... ]

> diff --git a/arch/s390/hypfs/hypfs_diag_fs.c b/arch/s390/hypfs/hypfs_diag_fs.c
> index 39621c3260103..2ce9119abe990 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/kernel/sthyi.c b/arch/s390/kernel/sthyi.c
> index 5eae2e25997ae..9adb26588d5b5 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;

[Severity: High]
Does this cause diag204_store() calls in hypfs and sthyi to fail with
-EINVAL on systems without the Diagnose 204 Busy Indication Facility
(BIF)?

In arch/s390/kernel/diag/diag204.c, diag204_store() rejects any call
where allow_busy is set if g_info.has_bif is false:

    if (!allow_busy && allow_busy_wait)
        return -EOPNOTSUPP;
    if (allow_busy && !g_info.has_bif)
        return -EINVAL;

Previously, diag204_store() in arch/s390/hypfs/hypfs_diag.c checked
diag204_has_bif() before setting DIAG204_BIF_BIT, and diag204_get_data()
in arch/s390/kernel/sthyi.c checked both diag204_has_bif() and
diag204_allow_busy. When BIF was not supported, both callers issued
diagnose 204 without DIAG204_BIF_BIT.

With this change, dbfs_d204_create() in arch/s390/hypfs/hypfs_diag.c and
hypfs_diag_create_files() in arch/s390/hypfs/hypfs_diag_fs.c both pass
allow_busy = 1 and allow_busy_wait = 1 unconditionally:

    rc = diag204_store(d204->buf, 1, 1);

and:

    rc = diag204_store(buffer, 1, 1);

When g_info.has_bif is false, reading the debugfs diag_204 file or
mounting and updating s390_hypfs will immediately return -EINVAL.

Similarly, in arch/s390/kernel/sthyi.c, diag204_get_data() now passes
diag204_allow_busy directly to diag204_store():

    sthyi_fill()
      -> sthyi_update_cache()
        -> fill_dst()
          -> diag204_get_data(is_diag204_cached(sctns))
            -> diag204_store(diag204_buf, diag204_allow_busy, 0)

On systems without hardware STHYI (!test_facility(74)) and without BIF,
the first sthyi_fill() call succeeds because sthyi_cache.info is zeroed
and is_diag204_cached(sctns) is false. Once fill_diag() populates the
validity bits in sthyi_cache.info and the cache expires after
CACHE_VALID_JIFFIES, subsequent calls to sthyi_fill() pass
diag204_allow_busy = true and fail with -EINVAL because
sthyi_update_cache() only handles -EBUSY.

Should these callers gate allow_busy and allow_busy_wait on BIF support,
or should diag204_store() fall back to a non-BIF store when
!g_info.has_bif?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-diag204ioctl-v3-0-c25e7147736e@linux.ibm.com?part=2

Re: [PATCH v3 1/3] 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: diag204_store() rejects allow_busy when BIF is unsupported
- [Medium] s390/diag204: diag204_store() returns -ERESTARTSYS when allow_busy_wait is 0
--

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

s390/diag204: Introduce diag204 rework

This commit introduces a caching layer for diagnose 204 calls in
arch/s390/kernel/diag/diag204.c to limit hardware invocations to at most
once per second, freeing the cache buffer via delayed work when idle.

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

[ ... ]

> +static int diag204_update_cache(int allow_busy, int allow_busy_wait)
> +{
> +	unsigned long subcode;
> +	void *buf;
> +	int rc;
> +
> +	lockdep_assert_held(&diag204_mutex);
> +	buf = diag204_allocate_buffer();
> +	if (!buf)
> +		return -ENOMEM;
> +	subcode = g_info.type | g_info.subcode;
> +	if (g_info.has_bif && allow_busy)
> +		subcode |= DIAG204_BIF_BIT;

[ ... ]

> +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 (allow_busy && !g_info.has_bif)
> +		return -EINVAL;

[Severity: High]
Should diag204_store() in arch/s390/kernel/diag/diag204.c return -EINVAL when
allow_busy is set and g_info.has_bif is false, or should it fall back to a
regular blocking diagnose 204 call?

In diag204_update_cache(), the code checks both g_info.has_bif and allow_busy
before setting DIAG204_BIF_BIT:

	subcode = g_info.type | g_info.subcode;
	if (g_info.has_bif && allow_busy)
		subcode |= DIAG204_BIF_BIT;

Because diag204_update_cache() is only called from diag204_store(), the early
return of -EINVAL in diag204_store() when allow_busy && !g_info.has_bif makes
the g_info.has_bif check in diag204_update_cache() unreachable when
!g_info.has_bif.

When the follow-up change ("s390/diag204: Replace diag204 users with new
implementation") converts existing callers:

- dbfs_d204_create() in arch/s390/hypfs/hypfs_diag.c and
  hypfs_diag_create_files() in arch/s390/hypfs/hypfs_diag_fs.c both call
  diag204_store(..., 1, 1) unconditionally.
- diag204_get_data() in arch/s390/kernel/sthyi.c calls
  diag204_store(diag204_buf, diag204_allow_busy, 0), where
  diag204_allow_busy is true on every cache refresh after the initial fill.

On systems where !sclp.has_diag204_bif, will this cause hypfs initialization,
debugfs diag_204 reads, and sthyi cache updates to fail with -EINVAL instead
of falling back to a non-busy-indication diag204 call?

> +	if (!buf)
> +		return -ENOMEM;
> +	rc = 0;
> +	do {
> +		mutex_lock(&diag204_mutex);
> +		/* 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;
> +		if (signal_pending(current))
> +			return -ERESTARTSYS;
> +		schedule_timeout_interruptible(DIAG204_BUSY_WAIT);
> +	} while (1);

[Severity: Medium]
Can this wait loop in diag204_store() in arch/s390/kernel/diag/diag204.c
unexpectedly sleep interruptibly and return -ERESTARTSYS when the caller
passes allow_busy = 0 and allow_busy_wait = 0?

If one thread is inside diag204_update_cache() with g_cache.updating set to
true while dropping diag204_mutex, a second thread calling
diag204_store(buf, 0, 0) will see READ_ONCE(g_cache.updating) as true:

	if (allow_busy && !allow_busy_wait)
		return -EBUSY;
	if (signal_pending(current))
		return -ERESTARTSYS;
	schedule_timeout_interruptible(DIAG204_BUSY_WAIT);

Because allow_busy is 0, the first condition is false and the loop checks
signal_pending(current) and returns -ERESTARTSYS even though allow_busy_wait
was not requested.

When the follow-up commits ("s390/diag204: Replace diag204 users with new
implementation" and "s390/diag: Add diag204 IOCTL to diag misc device") are
applied, callers such as diag204_buf() or the guest sthyi path:

handle_sthyi()
  -> sthyi_fill()
    -> sthyi_update_cache()
      -> fill_dst()
        -> diag204_get_data(false)
          -> diag204_store(diag204_buf, 0, 0)

can hit this path when a signal or TIF_NOTIFY_SIGNAL is pending during a
concurrent cache update. In handle_sthyi(), an unexpected -ERESTARTSYS return
from sthyi_fill() when the sthyi cache is not yet populated causes
handle_sthyi() to return before filling the guest buffer or setting the guest
condition code after the guest instruction pointer has already been advanced.

Should the concurrent update wait in diag204_store() avoid interruptible
sleep and -ERESTARTSYS when !allow_busy_wait?

> +	if (!diag204_is_cache_valid(DIAG204_CACHE_EXPIRY))
> +		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);
> +	return rc;
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-diag204ioctl-v3-0-c25e7147736e@linux.ibm.com?part=1

Re: [PATCH v3 3/3] s390/diag: Add diag204 IOCTL to diag misc device

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] s390/diag: diag204_store() fails with -EINVAL when BIF is unavailable
--

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

s390/diag: Add diag204 IOCTL to diag misc device

This commit adds the DIAG204_GET_SUBCODE, DIAG204_GET_LEN, and
DIAG204_GET_BUF ioctls to the /dev/diag misc device to allow userspace to
retrieve diagnose 204 data.

> diff --git a/arch/s390/kernel/diag/diag204.c b/arch/s390/kernel/diag/diag204.c
> index f447bfaf756b4..237303aa44d6f 100644
> --- a/arch/s390/kernel/diag/diag204.c
> +++ b/arch/s390/kernel/diag/diag204.c
[ ... ]
> @@ -222,6 +224,54 @@ int diag204_store(void *buf, int allow_busy, int allow_busy_wait)
>  	return rc;
>  }
>  
[ ... ]
> +long diag204_buf(unsigned long arg)
> +{
> +	struct diag204_request __user *udata;
> +	int rc, allow_busy, allow_busy_wait;
> +	u64 address;
> +	void *buf;
> +
> +	if (!diag204_is_available())
> +		return -EOPNOTSUPP;
> +	udata = (struct diag204_request __user *)arg;
> +	if (get_user(address, &udata->address))
> +		return -EFAULT;
> +	if (get_user(allow_busy, &udata->allow_busy))
> +		return -EFAULT;
> +	if (get_user(allow_busy_wait, &udata->allow_busy_wait))
> +		return -EFAULT;
> +	buf = diag204_allocate_buffer();
> +	if (!buf)
> +		return -ENOMEM;
> +	rc = diag204_store(buf, allow_busy, allow_busy_wait);

[Severity: Medium]
What happens on systems where diagnose 204 is available, but the busy
indication facility (g_info.has_bif) is not supported, when a caller passes a
non-zero allow_busy value?

In arch/s390/kernel/diag/diag204.c, diag204_update_cache() is written to fall
back to a regular diagnose 204 call when g_info.has_bif is false:

    subcode = g_info.type | g_info.subcode;
    if (g_info.has_bif && allow_busy)
            subcode |= DIAG204_BIF_BIT;

However, earlier in the series, diag204_store() in
arch/s390/kernel/diag/diag204.c rejects any call with allow_busy set when
g_info.has_bif is false:

    if (allow_busy && !g_info.has_bif)
            return -EINVAL;

Because arch/s390/include/uapi/asm/diag.h does not provide an ioctl for
userspace to query whether BIF is supported, a userspace caller invoking
ioctl(fd, DIAG204_GET_BUF, &req) -> diag_ioctl() -> diag204_buf() ->
diag204_store() with allow_busy set has no way to know upfront that it must
pass allow_busy = 0 on machines without BIF.

In addition, the in-kernel callers converted earlier in the series pass
allow_busy = 1 unconditionally when BIF is not supported:

- arch/s390/hypfs/hypfs_diag.c: dbfs_d204_create() calls
  diag204_store(d204->buf, 1, 1)
- arch/s390/hypfs/hypfs_diag_fs.c: hypfs_diag_create_files() calls
  diag204_store(buffer, 1, 1)
- arch/s390/kernel/sthyi.c: diag204_get_data() calls
  diag204_store(diag204_buf, diag204_allow_busy, 0) where
  diag204_allow_busy = is_diag204_cached(sctns) is true after the first cache
  fill

Should diag204_store() allow allow_busy when !g_info.has_bif and let
diag204_update_cache() ignore it (or clamp allow_busy and allow_busy_wait to 0
when !g_info.has_bif) instead of returning -EINVAL?

> +	if (rc < 0) {
> +		diag204_free_buffer(buf);
> +		return rc;
> +	}
> +	rc = copy_to_user((void __user *)address, buf,
> +			  diag204_get_buf_size());
> +	diag204_free_buffer(buf);
> +	return rc ? -EFAULT : 0;
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-diag204ioctl-v3-0-c25e7147736e@linux.ibm.com?part=3