* [PATCH 1/3] s390/diag204: Introduce diag204 rework
2026-10-21 10:28 [PATCH 0/3] s390: Diag204 Rework Mete Durlu
@ 2026-10-21 10:28 ` Mete Durlu
2026-10-21 10:52 ` sashiko-bot
2026-10-21 10:28 ` [PATCH 2/3] s390/diag204: Replace diag204 users with new implementation Mete Durlu
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Mete Durlu @ 2026-10-21 10:28 UTC (permalink / raw)
To: Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, Sumanth Korikkar
Cc: linux-s390, 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 | 339 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 346 insertions(+)
diff --git a/arch/s390/include/asm/diag.h b/arch/s390/include/asm/diag.h
index 8db8db3b1018..fe817a7c290d 100644
--- a/arch/s390/include/asm/diag.h
+++ b/arch/s390/include/asm/diag.h
@@ -337,6 +337,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..8e1909e6caf9
--- /dev/null
+++ b/arch/s390/kernel/diag/diag204.c
@@ -0,0 +1,339 @@
+// 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. 2025
+ */
+
+#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;
+};
+
+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(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);
+rearm:
+ diag204_schedule_work();
+unlock:
+ mutex_unlock(&diag204_mutex);
+}
+
+static int diag204_cache_new(void *buf, unsigned long subcode)
+{
+ int rc;
+
+ lockdep_assert_held(&diag204_mutex);
+ /*
+ * Don't allow busy wait or busy looping when g_cache.buf has
+ * no data yet. We first need a valid data cache before we
+ * can do updates with busy looping.
+ */
+ rc = diag204(subcode, g_info.pages, buf);
+ if (rc < 0)
+ return rc;
+ diag204_set_cache(buf);
+ return rc;
+}
+
+static int diag204_cache_replace(void *buf, unsigned long subcode, int allow_busy_wait)
+{
+ int rc;
+
+ lockdep_assert_held(&diag204_mutex);
+ if (!allow_busy_wait) {
+ rc = diag204(subcode, g_info.pages, buf);
+ } else {
+ /*
+ * Unlock before BUSY loop so updater can sleep, we rely on
+ * g_cache.updating flag here for synchronising with diag204
+ * work function and other diag204 callers.
+ */
+ mutex_unlock(&diag204_mutex);
+ do {
+ rc = diag204(subcode, g_info.pages, buf);
+ if (rc >= 0)
+ break;
+ if (signal_pending(current)) {
+ mutex_lock(&diag204_mutex);
+ return -ERESTARTSYS;
+ }
+ schedule_timeout_interruptible(DIAG204_BUSY_WAIT);
+ } while (rc == -EBUSY);
+ mutex_lock(&diag204_mutex);
+ }
+ if (rc < 0)
+ return rc;
+ diag204_set_cache(buf);
+ return rc;
+}
+
+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);
+ if (!READ_ONCE(g_cache.buf))
+ rc = diag204_cache_new(buf, subcode);
+ else
+ rc = diag204_cache_replace(buf, subcode, allow_busy_wait);
+ WRITE_ONCE(g_cache.updating, false);
+ if (rc < 0)
+ diag204_free_buffer(buf);
+ 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 (!buf)
+ return -ENOMEM;
+ rc = 0;
+ if (allow_busy && !allow_busy_wait) {
+ mutex_lock(&diag204_mutex);
+ /*
+ * Drop the lock if an update is ongoing, user allows to return
+ * BUSY without waiting
+ */
+ if (READ_ONCE(g_cache.updating)) {
+ mutex_unlock(&diag204_mutex);
+ diag204_schedule_work();
+ return -EBUSY;
+ }
+ } else {
+ /* Wait for the ongoing update to end */
+ while (1) {
+ mutex_lock(&diag204_mutex);
+ if (!READ_ONCE(g_cache.updating))
+ break;
+ mutex_unlock(&diag204_mutex);
+ if (signal_pending(current))
+ return -ERESTARTSYS;
+ schedule_timeout_interruptible(DIAG204_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);
+ diag204_schedule_work();
+ 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;
+ }
+ } else {
+ /* 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] s390/diag204: Introduce diag204 rework
2026-10-21 10:28 ` [PATCH 1/3] s390/diag204: Introduce diag204 rework Mete Durlu
@ 2026-10-21 10:52 ` sashiko-bot
0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-10-21 10:52 UTC (permalink / raw)
To: Mete Durlu
Cc: linux-s390, Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] s390/diag204: premature -EBUSY in diag204_update_cache() when cache is empty
- [Medium] s390/diag204: wrong break check on error in diag204_cache_replace()
- [Medium] s390/diag204: missing subcode 4 fallback in diag204_probe()
- [Medium] s390/diag204: unexpected -ERESTARTSYS in diag204_store() when allow_busy_wait is 0
- [Low] s390/diag204: redundant work rearm after cache free in diag204_work_fn()
--
commit d2828ceeab655eb5035c6aab276ad1fae954baa3
Author: Mete Durlu <meted@linux.ibm.com>
s390/diag204: Introduce diag204 rework
This commit introduces a new diag204 implementation in
arch/s390/kernel/diag/diag204.c that caches diag204 data for up to one
second to rate-limit expensive hypervisor calls and frees the cache buffer
via a delayed work item after a period of inactivity.
> diff --git a/arch/s390/kernel/diag/diag204.c b/arch/s390/kernel/diag/diag204.c
> new file mode 100644
> index 0000000000000..8e1909e6caf90
> --- /dev/null
> +++ b/arch/s390/kernel/diag/diag204.c
[ ... ]
> +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);
> +rearm:
> + diag204_schedule_work();
> +unlock:
> + mutex_unlock(&diag204_mutex);
> +}
[Severity: Low]
In diag204_work_fn(), when the cache has expired and is not being updated,
diag204_set_cache(NULL) frees g_cache.buf and clears the pointer, but then
execution falls through to the rearm label and calls diag204_schedule_work().
Should this jump to unlock after diag204_set_cache(NULL) instead of
scheduling another delayed work run 10 seconds later when the cache is
already empty?
> +
> +static int diag204_cache_new(void *buf, unsigned long subcode)
> +{
> + int rc;
> +
> + lockdep_assert_held(&diag204_mutex);
> + /*
> + * Don't allow busy wait or busy looping when g_cache.buf has
> + * no data yet. We first need a valid data cache before we
> + * can do updates with busy looping.
> + */
> + rc = diag204(subcode, g_info.pages, buf);
> + if (rc < 0)
> + return rc;
> + diag204_set_cache(buf);
> + return rc;
> +}
> +
> +static int diag204_cache_replace(void *buf, unsigned long subcode, int allow_busy_wait)
> +{
> + int rc;
> +
> + lockdep_assert_held(&diag204_mutex);
> + if (!allow_busy_wait) {
> + rc = diag204(subcode, g_info.pages, buf);
> + } else {
> + /*
> + * Unlock before BUSY loop so updater can sleep, we rely on
> + * g_cache.updating flag here for synchronising with diag204
> + * work function and other diag204 callers.
> + */
> + mutex_unlock(&diag204_mutex);
> + do {
> + rc = diag204(subcode, g_info.pages, buf);
> + if (rc >= 0)
> + break;
> + if (signal_pending(current)) {
> + mutex_lock(&diag204_mutex);
> + return -ERESTARTSYS;
> + }
> + schedule_timeout_interruptible(DIAG204_BUSY_WAIT);
> + } while (rc == -EBUSY);
[Severity: Medium]
In diag204_cache_replace(), when allow_busy_wait is set, the retry loop only
breaks early on rc >= 0.
If diag204() fails with a negative error other than -EBUSY (such as
-EOPNOTSUPP or -EINVAL), won't this loop either overwrite the error with
-ERESTARTSYS if a signal is pending, or sleep unnecessarily for
DIAG204_BUSY_WAIT before checking the loop condition? Should the break
condition check for rc != -EBUSY instead?
> + mutex_lock(&diag204_mutex);
> + }
> + if (rc < 0)
> + return rc;
> + diag204_set_cache(buf);
> + return rc;
> +}
> +
> +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);
> + if (!READ_ONCE(g_cache.buf))
> + rc = diag204_cache_new(buf, subcode);
> + else
> + rc = diag204_cache_replace(buf, subcode, allow_busy_wait);
[Severity: Medium]
In diag204_update_cache(), DIAG204_BIF_BIT is set in subcode when
g_info.has_bif && allow_busy is true, before checking whether g_cache.buf is
NULL.
When g_cache.buf is NULL (on the initial call to diag204_store() or after
diag204_work_fn() has freed the expired cache) and a caller passes
allow_busy = 1, diag204_cache_new() is called with DIAG204_BIF_BIT set in
subcode.
The comment in diag204_cache_new() notes that busy waiting or busy looping
should not be used when g_cache.buf has no data yet, and diag204_cache_new()
does not loop on -EBUSY. If the hypervisor is busy, won't diag204() return
-EBUSY immediately instead of blocking to populate the initial cache (or
performing the requested busy wait)?
> + WRITE_ONCE(g_cache.updating, false);
> + if (rc < 0)
> + diag204_free_buffer(buf);
> + 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;
> + if (allow_busy && !allow_busy_wait) {
> + mutex_lock(&diag204_mutex);
> + /*
> + * Drop the lock if an update is ongoing, user allows to return
> + * BUSY without waiting
> + */
> + if (READ_ONCE(g_cache.updating)) {
> + mutex_unlock(&diag204_mutex);
> + diag204_schedule_work();
> + return -EBUSY;
> + }
> + } else {
> + /* Wait for the ongoing update to end */
> + while (1) {
> + mutex_lock(&diag204_mutex);
> + if (!READ_ONCE(g_cache.updating))
> + break;
> + mutex_unlock(&diag204_mutex);
> + if (signal_pending(current))
> + return -ERESTARTSYS;
> + schedule_timeout_interruptible(DIAG204_BUSY_WAIT);
> + }
> + }
[Severity: Medium]
Can diag204_store() unexpectedly return -ERESTARTSYS when called with
allow_busy = 0 and allow_busy_wait = 0?
If one thread calls diag204_store() with allow_busy = 1 and
allow_busy_wait = 1 while g_cache.buf is non-NULL and expired,
diag204_update_cache() -> diag204_cache_replace() sets g_cache.updating = true
and drops diag204_mutex while sleeping in its -EBUSY loop.
If a second thread concurrently calls diag204_store(buf, 0, 0), the
allow_busy && !allow_busy_wait check is false, so it enters the else branch
above, sees g_cache.updating == true, drops diag204_mutex, and returns
-ERESTARTSYS if a signal is pending on current, even though the caller did not
request an interruptible wait.
In the follow-up commit "s390/diag204: Replace diag204 users with new
implementation", callers such as hypfs_diag_create_files() (via
hypfs_write_iter() after hypfs_delete_tree() has already removed the hypfs
directory tree, and via hypfs_fill_super()) and diag204_get_data() in
arch/s390/kernel/sthyi.c (via sthyi_fill(), including KVM's handle_sthyi()
intercept handler) call diag204_store() with allow_busy = 0 and
allow_busy_wait = 0 and do not expect -ERESTARTSYS, which can leave the hypfs
tree deleted or abort STHYI emulation after SIE has already advanced the guest
PSW.
[ ... ]
> +/*
> + * 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;
> + }
> + } else {
> + /* 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;
[Severity: Medium]
In diag204_init() -> diag204_probe(), the fallback to DIAG204_INFO_SIMPLE with
DIAG204_SUBC_STIB4 is inside the else branch of if (pages > 0).
If the DIAG204_SUBC_RSI call returns pages > 0, but both DIAG204_SUBC_STIB7
and DIAG204_SUBC_STIB6 fail, won't diag204_probe() skip the DIAG204_SUBC_STIB4
fallback and fail with -EOPNOTSUPP, contrary to the function comment and the
previous probe logic in arch/s390/hypfs/hypfs_diag.c?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-diag204ioctl-v1-0-25645b97bf4e@linux.ibm.com?part=1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] s390/diag204: Replace diag204 users with new implementation
2026-10-21 10:28 [PATCH 0/3] s390: Diag204 Rework Mete Durlu
2026-10-21 10:28 ` [PATCH 1/3] s390/diag204: Introduce diag204 rework Mete Durlu
@ 2026-10-21 10:28 ` Mete Durlu
2026-10-21 11:09 ` sashiko-bot
2026-10-21 10:28 ` [PATCH 3/3] s390/diag: Add diag204 IOCTL to diag misc device Mete Durlu
2026-10-22 7:04 ` [PATCH 0/3] s390: Diag204 Rework Mete Durlu
3 siblings, 1 reply; 10+ messages in thread
From: Mete Durlu @ 2026-10-21 10:28 UTC (permalink / raw)
To: Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, Sumanth Korikkar
Cc: linux-s390, 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 | 28 +++-----
6 files changed, 27 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..13b5a2a4729d 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, 0, 0);
+ 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 fe817a7c290d..9b5f5c02dbf3 100644
--- a/arch/s390/include/asm/diag.h
+++ b/arch/s390/include/asm/diag.h
@@ -122,7 +122,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..5d9228039ce7 100644
--- a/arch/s390/kernel/sthyi.c
+++ b/arch/s390/kernel/sthyi.c
@@ -305,29 +305,17 @@ 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);
+ 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 +450,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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/3] s390/diag204: Replace diag204 users with new implementation
2026-10-21 10:28 ` [PATCH 2/3] s390/diag204: Replace diag204 users with new implementation Mete Durlu
@ 2026-10-21 11:09 ` sashiko-bot
0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-10-21 11:09 UTC (permalink / raw)
To: Mete Durlu
Cc: linux-s390, Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] s390/sthyi: missing format and subcode checks in diag204_get_data()
- [Medium] s390/diag204: broken busy-wait in dbfs_d204_create() on empty cache
--
commit c339eee9c087cee65444e6cc2f4556d0002e4640
Author: Mete Durlu <meted@linux.ibm.com>
s390/diag204: Replace diag204 users with new implementation
This commit converts the hypfs and sthyi callers of diagnose 204 to use
the new centralized diag204 buffer allocation, probing, and caching
interface, and enables building diag204.o in arch/s390/kernel/diag/.
> 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
[ ... ]
> @@ -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;
> }
[Severity: Medium]
Previously, dbfs_d204_create() in arch/s390/hypfs/hypfs_diag.c and
hypfs_diag_create_files() in arch/s390/hypfs/hypfs_diag_fs.c called the
local diag204_store(), which set DIAG204_BIF_BIT (when supported) and
looped with schedule_timeout_interruptible(DIAG204_BUSY_WAIT) while
diag204() returned -EBUSY.
With this change, dbfs_d204_create() calls diag204_store(d204->buf, 1, 1).
When a user reads the s390_hypfs/diag_204 debugfs file while g_cache.buf
is null (such as on the first read after boot, or after diag204_work_fn()
expires and frees the cache), dbfs_d204_create() -> diag204_store() ->
diag204_update_cache() runs:
arch/s390/kernel/diag/diag204.c:diag204_update_cache() {
...
subcode = g_info.type | g_info.subcode;
if (g_info.has_bif && allow_busy)
subcode |= DIAG204_BIF_BIT;
WRITE_ONCE(g_cache.updating, true);
if (!READ_ONCE(g_cache.buf))
rc = diag204_cache_new(buf, subcode);
else
rc = diag204_cache_replace(buf, subcode, allow_busy_wait);
...
}
Because DIAG204_BIF_BIT is already set in subcode before calling
diag204_cache_new(), diag204() returns -EBUSY immediately if the
hypervisor is busy, and diag204_cache_new() returns that error without
performing the allow_busy_wait retry loop:
arch/s390/kernel/diag/diag204.c:diag204_cache_new() {
...
rc = diag204(subcode, g_info.pages, buf);
if (rc < 0)
return rc;
diag204_set_cache(buf);
return rc;
}
Additionally, hypfs_diag_create_files() in arch/s390/hypfs/hypfs_diag_fs.c
now calls diag204_store(buffer, 0, 0), which drops DIAG204_BIF_BIT and
blocks uninterruptibly in the diagnose instruction instead of sleeping
interruptibly.
Should diag204_update_cache() avoid setting DIAG204_BIF_BIT when
g_cache.buf is null (or retry on -EBUSY when allow_busy_wait is set), and
should hypfs_diag_create_files() also pass allow_busy = 1 and
allow_busy_wait = 1?
[ ... ]
> diff --git a/arch/s390/kernel/sthyi.c b/arch/s390/kernel/sthyi.c
> index 5eae2e25997ae..5d9228039ce77 100644
> --- a/arch/s390/kernel/sthyi.c
> +++ b/arch/s390/kernel/sthyi.c
> @@ -305,29 +305,17 @@ 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);
> + 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: Critical]
Prior to this change, diag204_get_data() in arch/s390/kernel/sthyi.c
explicitly requested DIAG204_INFO_EXT and DIAG204_SUBC_STIB7.
In arch/s390/kernel/diag/diag204.c, diag204_probe() falls back to
DIAG204_SUBC_STIB6 or DIAG204_INFO_SIMPLE (DIAG204_SUBC_STIB4 with
pages = 1) when DIAG204_SUBC_STIB7 or DIAG204_INFO_EXT is not supported:
arch/s390/kernel/diag/diag204.c:diag204_probe() {
...
if (diag204(DIAG204_SUBC_STIB6 | type, pages, buf) >= 0) {
subcode = DIAG204_SUBC_STIB6;
goto out;
}
} else {
/* 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;
}
}
...
}
In those fallback cases g_info.pages is still greater than 0, so
diag204_is_available() returns true. However, when s390_sthyi or
handle_sthyi() calls sthyi_fill() -> sthyi_update_cache() -> fill_dst() ->
fill_diag(), fill_diag() and lpar_cpu_inf() in arch/s390/kernel/sthyi.c
unconditionally cast diag204_buf to the extended format structures
(struct diag204_x_info_blk_hdr, struct diag204_x_part_block, and
struct diag204_x_phys_block):
arch/s390/kernel/sthyi.c:fill_diag() {
...
ti_hdr = diag204_buf;
part_block = diag204_buf + sizeof(*ti_hdr);
for (i = 0; i < ti_hdr->npar; i++) {
this_lpar = (void *)part_block - diag204_buf == ti_hdr->this_part;
part_block = lpar_cpu_inf(&lpar_inf, this_lpar, diag224_buf,
part_block);
}
phys_block = (struct diag204_x_phys_block *)part_block;
part_block = diag204_buf + ti_hdr->this_part;
...
}
What happens on a system without facility 74 where diag204_probe() fell
back to DIAG204_INFO_SIMPLE or DIAG204_SUBC_STIB6? With
DIAG204_INFO_SIMPLE, diag204_allocate_buffer() allocates a single page
containing simple-format data, so fill_diag() and lpar_cpu_inf() read
fields at extended-format offsets and step part_block past the end of the
1-page diag204_buf allocation. With DIAG204_SUBC_STIB6, secondary CPUs are
omitted instead of failing with -EOPNOTSUPP.
Should diag204_get_data() check that diag204_get_info_type() ==
DIAG204_INFO_EXT and diag204_get_subcode() == DIAG204_SUBC_STIB7?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-diag204ioctl-v1-0-25645b97bf4e@linux.ibm.com?part=2
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] s390/diag: Add diag204 IOCTL to diag misc device
2026-10-21 10:28 [PATCH 0/3] s390: Diag204 Rework Mete Durlu
2026-10-21 10:28 ` [PATCH 1/3] s390/diag204: Introduce diag204 rework Mete Durlu
2026-10-21 10:28 ` [PATCH 2/3] s390/diag204: Replace diag204 users with new implementation Mete Durlu
@ 2026-10-21 10:28 ` Mete Durlu
2026-10-21 11:13 ` sashiko-bot
2026-10-21 14:01 ` Hendrik Brueckner
2026-10-22 7:04 ` [PATCH 0/3] s390: Diag204 Rework Mete Durlu
3 siblings, 2 replies; 10+ messages in thread
From: Mete Durlu @ 2026-10-21 10:28 UTC (permalink / raw)
To: Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, Sumanth Korikkar
Cc: linux-s390, 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 | 8 +++++++
arch/s390/kernel/diag/diag204.c | 43 ++++++++++++++++++++++++++++++++++++++
arch/s390/kernel/diag/diag_ioctl.h | 3 +++
arch/s390/kernel/diag/diag_misc.c | 10 +++++++++
4 files changed, 64 insertions(+)
diff --git a/arch/s390/include/uapi/asm/diag.h b/arch/s390/include/uapi/asm/diag.h
index b7e6ccb4ff6e..985667b2917a 100644
--- a/arch/s390/include/uapi/asm/diag.h
+++ b/arch/s390/include/uapi/asm/diag.h
@@ -22,11 +22,19 @@ struct diag310_memtop {
__u64 nesting_lvl;
};
+struct diag204_request {
+ __u64 address;
+ int allow_busy;
+ int 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_LEN _IOR(DIAG_MAGIC_STR, 0x7c, size_t)
+#define DIAG204_GET_BUF _IOWR(DIAG_MAGIC_STR, 0x7d, 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 8e1909e6caf9..967b6e0bc667 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 */
@@ -268,6 +270,47 @@ int diag204_store(void *buf, int allow_busy, int allow_busy_wait)
return rc;
}
+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());
+ if (rc < 0)
+ rc = -EFAULT;
+ diag204_free_buffer(buf);
+ 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:
diff --git a/arch/s390/kernel/diag/diag_ioctl.h b/arch/s390/kernel/diag/diag_ioctl.h
index 7080be946785..7718b57ffe3e 100644
--- a/arch/s390/kernel/diag/diag_ioctl.h
+++ b/arch/s390/kernel/diag/diag_ioctl.h
@@ -4,6 +4,9 @@
#include <linux/types.h>
+long diag204_len(unsigned long arg);
+long diag204_buf(unsigned long arg);
+
long diag324_pibbuf(unsigned long arg);
long diag324_piblen(unsigned long arg);
diff --git a/arch/s390/kernel/diag/diag_misc.c b/arch/s390/kernel/diag/diag_misc.c
index efffe02ea02e..ac263baf8d08 100644
--- a/arch/s390/kernel/diag/diag_misc.c
+++ b/arch/s390/kernel/diag/diag_misc.c
@@ -35,6 +35,16 @@ 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_LEN:
+ rc = -EPERM;
+ if (capable(CAP_SYS_ADMIN))
+ rc = diag204_len(arg);
+ break;
+ case DIAG204_GET_BUF:
+ rc = -EPERM;
+ if (capable(CAP_SYS_ADMIN))
+ rc = diag204_buf(arg);
+ break;
default:
rc = -ENOIOCTLCMD;
break;
--
2.56.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 3/3] s390/diag: Add diag204 IOCTL to diag misc device
2026-10-21 10:28 ` [PATCH 3/3] s390/diag: Add diag204 IOCTL to diag misc device Mete Durlu
@ 2026-10-21 11:13 ` sashiko-bot
2026-10-21 14:01 ` Hendrik Brueckner
1 sibling, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-10-21 11:13 UTC (permalink / raw)
To: Mete Durlu
Cc: linux-s390, Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] s390/diag: broken copy_to_user() error check in diag204_buf()
--
commit bc2db0b15c4152f85ed3a637d54a7ba9099ebe1e
Author: Mete Durlu <meted@linux.ibm.com>
s390/diag: Add diag204 IOCTL to diag misc device
This commit adds the DIAG204_GET_LEN and DIAG204_GET_BUF ioctl commands
to the /dev/diag misc device to allow userspace with CAP_SYS_ADMIN to
query and retrieve diag204 data.
> diff --git a/arch/s390/kernel/diag/diag204.c b/arch/s390/kernel/diag/diag204.c
> index 8e1909e6caf90..967b6e0bc667a 100644
> --- a/arch/s390/kernel/diag/diag204.c
> +++ b/arch/s390/kernel/diag/diag204.c
[ ... ]
> +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());
> + if (rc < 0)
> + rc = -EFAULT;
[Severity: Medium]
Since copy_to_user() returns the positive number of uncopied bytes on a
fault rather than a negative error code, will this check miss faults when
copying to address in diag204_buf() via diag_ioctl() (DIAG204_GET_BUF) and
return a positive byte count instead of -EFAULT?
Should this check if (rc) (or if (copy_to_user(...))) instead?
> + diag204_free_buffer(buf);
> + return rc;
> +}
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-diag204ioctl-v1-0-25645b97bf4e@linux.ibm.com?part=3
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 3/3] s390/diag: Add diag204 IOCTL to diag misc device
2026-10-21 10:28 ` [PATCH 3/3] s390/diag: Add diag204 IOCTL to diag misc device Mete Durlu
2026-10-21 11:13 ` sashiko-bot
@ 2026-10-21 14:01 ` Hendrik Brueckner
2026-10-21 14:12 ` Mete Durlu
1 sibling, 1 reply; 10+ messages in thread
From: Hendrik Brueckner @ 2026-10-21 14:01 UTC (permalink / raw)
To: Mete Durlu
Cc: Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, Sumanth Korikkar,
linux-s390
On Wed, Oct 21, 2026 at 12:28:10PM +0200, Mete Durlu wrote:
> 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 | 8 +++++++
> arch/s390/kernel/diag/diag204.c | 43 ++++++++++++++++++++++++++++++++++++++
> arch/s390/kernel/diag/diag_ioctl.h | 3 +++
> arch/s390/kernel/diag/diag_misc.c | 10 +++++++++
> 4 files changed, 64 insertions(+)
[...]
> index efffe02ea02e..ac263baf8d08 100644
> --- a/arch/s390/kernel/diag/diag_misc.c
> +++ b/arch/s390/kernel/diag/diag_misc.c
> @@ -35,6 +35,16 @@ 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_LEN:
> + rc = -EPERM;
> + if (capable(CAP_SYS_ADMIN))
> + rc = diag204_len(arg);
> + break;
> + case DIAG204_GET_BUF:
> + rc = -EPERM;
> + if (capable(CAP_SYS_ADMIN))
Why do you restrict this to sys_admin cap? I think that access control could
be better managed through mode and ownership of the /dev/diag device.
Assuming that I like to run a service collecting information from diag204
would require to run with sys_admin capability (almost root authority).
On a /dev/diag, allowing root and group to access the device would allow
a service to run with regular and respective group privileges to access it
(w/o cap sys admin).
We should revisit the device .mode = 0444 setting and prefer using
0440 to prohibit other users and restricting access to root/group ownership
only.
> + rc = diag204_buf(arg);
> + break;
> default:
> rc = -ENOIOCTLCMD;
> break;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] s390/diag: Add diag204 IOCTL to diag misc device
2026-10-21 14:01 ` Hendrik Brueckner
@ 2026-10-21 14:12 ` Mete Durlu
0 siblings, 0 replies; 10+ messages in thread
From: Mete Durlu @ 2026-10-21 14:12 UTC (permalink / raw)
To: Hendrik Brueckner
Cc: Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, Sumanth Korikkar,
linux-s390
On 21/10/2026 16:01, Hendrik Brueckner wrote:
> On Wed, Oct 21, 2026 at 12:28:10PM +0200, Mete Durlu wrote:
>> 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 | 8 +++++++
>> arch/s390/kernel/diag/diag204.c | 43 ++++++++++++++++++++++++++++++++++++++
>> arch/s390/kernel/diag/diag_ioctl.h | 3 +++
>> arch/s390/kernel/diag/diag_misc.c | 10 +++++++++
>> 4 files changed, 64 insertions(+)
>
> [...]
>
>> index efffe02ea02e..ac263baf8d08 100644
>> --- a/arch/s390/kernel/diag/diag_misc.c
>> +++ b/arch/s390/kernel/diag/diag_misc.c
>> @@ -35,6 +35,16 @@ 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_LEN:
>> + rc = -EPERM;
>> + if (capable(CAP_SYS_ADMIN))
>> + rc = diag204_len(arg);
>> + break;
>> + case DIAG204_GET_BUF:
>> + rc = -EPERM;
>> + if (capable(CAP_SYS_ADMIN))
>
> Why do you restrict this to sys_admin cap? I think that access control could
> be better managed through mode and ownership of the /dev/diag device.
I don't know about other diag codes and I wanted to just preserve the
current authorization level for diag204. If you think the /dev/diag
ownership is a better solution, I wouldn't go against it.
> Assuming that I like to run a service collecting information from diag204
> would require to run with sys_admin capability (almost root authority).
>
> On a /dev/diag, allowing root and group to access the device would allow
> a service to run with regular and respective group privileges to access it
> (w/o cap sys admin).
>
> We should revisit the device .mode = 0444 setting and prefer using
> 0440 to prohibit other users and restricting access to root/group ownership
> only.
>
Sounds like a separate one liner patch involving just
arch/s390/kernel/diag/diag_misc.c
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] s390: Diag204 Rework
2026-10-21 10:28 [PATCH 0/3] s390: Diag204 Rework Mete Durlu
` (2 preceding siblings ...)
2026-10-21 10:28 ` [PATCH 3/3] s390/diag: Add diag204 IOCTL to diag misc device Mete Durlu
@ 2026-10-22 7:04 ` Mete Durlu
3 siblings, 0 replies; 10+ messages in thread
From: Mete Durlu @ 2026-10-22 7:04 UTC (permalink / raw)
To: Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, Sumanth Korikkar
Cc: linux-s390
On 21/10/2026 12:28, Mete Durlu wrote:
[...snip...]
> 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.
>
> 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)
>
> ---
> base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
>
> 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 | 8 +
> arch/s390/kernel/diag/Makefile | 2 +-
> arch/s390/kernel/diag/diag204.c | 382 +++++++++++++++++++++++++++++++++++++
> arch/s390/kernel/diag/diag_ioctl.h | 3 +
> arch/s390/kernel/diag/diag_misc.c | 10 +
> arch/s390/kernel/sthyi.c | 28 +--
> 10 files changed, 437 insertions(+), 157 deletions(-)
I will submit a new version soon with Sashiko findings addressed.
I kindly ask to hold any comments until then.
Thank you!
^ permalink raw reply [flat|nested] 10+ messages in thread