[RFC PATCH v9 0/4] Report vfio-ap configuration changes

[RFC PATCH v9 0/4] Report vfio-ap configuration changes

[Rorie Reyes]

Changelog:
v9:
- added SPDX licensing to newly created file 'hw/s390x/ap-stub.c'

v8:
- fixed windows cross-compile build
- moved /hw/vfio/ap-stub.c to /hw/s390x/ap-stub.c
- updated the use of stub file to MAINTAINERS to reflect new location
- removed if_false for 'CONFIG_VFIO_AP' statement from /hw/vfio/meson.build
- added if_false for 'CONFIG_VFIO_AP' to use ap-stub.c in /hw/s390x/meson.build
- all those changes still address '--without-default-devices' issue from v5

v7:
- Dropped initial commit for linux-header file vfio.h since I created two new commits
to address the changes made in v6
- Moved patches 6 and 7 to the beginning of the series after dropping the first patch
   - Because I dropped the initial commit for linux-header file vfio.h, I had to add
VFIO_AP_CFG_CHG_IRQ_INDEX
- Resyncing latest to v6.15-rc3
- Still need Thomas Huth's review of v5 changes for patch 6/6

v6:
- Updating the update-linux-headers script to address kernel commit change 8a14
- Update headers to retrieve uapi information for vfio-ap for update to Linux v6.15-rc1
- Still need Thomas Huth's review of v5 changes for patch 7/7 (see below)

v5:
- configuring using the '--without-default-devices' fails when building the code
- created a stub file for functions ap_chsc_sei_nt0_get_event and ap_chsc_sei_nt0_have_event
- add if_false for 'CONFIG_VFIO_AP' use ap-stub.c in meson.build
- add the use of the stub file to MAINTAINERS since it's a new file

v4:
- allocating cfg_chg_event before inserting into the queue
- calling nt0_have_event in if loop to check if there are any
elemenets in the queue, then calling QTAILQ_FIRST when the check
passes
- moving memset() after the check

v3:
- changes that were made to patch 3/5 should have been made in
patch 2/5

v2:
- removed warnings that weren't needed
- added unregister function
- removed whitelines
- changed variable names for consistency
- removed rc variable and returning 1 or 0 outright
- reversed logics for if statements
- using g_free() instead of free()
- replaced hardcoded numeric values by defining them with #define
in the header

--------------------------------------------------------------------------
This patch series creates and registers a handler that is called when
userspace is notified by the kernel that a guest's AP configuration has
changed. The handler in turn notifies the guest that its AP configuration
has changed. This allows the guest to immediately respond to AP
configuration changes rather than relying on polling or some other
inefficient mechanism for detecting config changes.

Rorie Reyes (4):
  hw/vfio/ap: notification handler for AP config changed event
  hw/vfio/ap: store object indicating AP config changed in a queue
  hw/vfio/ap: Storing event information for an AP configuration change
    event
  s390: implementing CHSC SEI for AP config change

 MAINTAINERS                  |  1 +
 hw/s390x/ap-stub.c           | 25 +++++++++++
 hw/s390x/meson.build         |  1 +
 hw/vfio/ap.c                 | 82 ++++++++++++++++++++++++++++++++++++
 include/hw/s390x/ap-bridge.h | 22 ++++++++++
 target/s390x/ioinst.c        | 11 ++++-
 6 files changed, 140 insertions(+), 2 deletions(-)
 create mode 100644 hw/s390x/ap-stub.c

-- 
2.48.1

[RFC PATCH v9 1/4] hw/vfio/ap: notification handler for AP config changed event

[Rorie Reyes]

Register an event notifier handler to process AP configuration
change events by queuing the event and generating a CRW to let
the guest know its AP configuration has changed

Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
---
 hw/vfio/ap.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index 1207c08d8d..3d0af7a54a 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -18,6 +18,7 @@
 #include "hw/vfio/vfio-device.h"
 #include "system/iommufd.h"
 #include "hw/s390x/ap-device.h"
+#include "hw/s390x/css.h"
 #include "qemu/error-report.h"
 #include "qemu/event_notifier.h"
 #include "qemu/main-loop.h"
@@ -37,6 +38,7 @@ struct VFIOAPDevice {
     APDevice apdev;
     VFIODevice vdev;
     EventNotifier req_notifier;
+    EventNotifier cfg_notifier;
 };
 
 OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
@@ -70,6 +72,18 @@ static void vfio_ap_req_notifier_handler(void *opaque)
     }
 }
 
+static void vfio_ap_cfg_chg_notifier_handler(void *opaque)
+{
+    VFIOAPDevice *vapdev = opaque;
+
+    if (!event_notifier_test_and_clear(&vapdev->cfg_notifier)) {
+        return;
+    }
+
+    css_generate_css_crws(0);
+
+}
+
 static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
                                           unsigned int irq, Error **errp)
 {
@@ -85,6 +99,10 @@ static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
         notifier = &vapdev->req_notifier;
         fd_read = vfio_ap_req_notifier_handler;
         break;
+    case VFIO_AP_CFG_CHG_IRQ_INDEX:
+        notifier = &vapdev->cfg_notifier;
+        fd_read = vfio_ap_cfg_chg_notifier_handler;
+        break;
     default:
         error_setg(errp, "vfio: Unsupported device irq(%d)", irq);
         return false;
@@ -136,6 +154,9 @@ static void vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev,
     case VFIO_AP_REQ_IRQ_INDEX:
         notifier = &vapdev->req_notifier;
         break;
+    case VFIO_AP_CFG_CHG_IRQ_INDEX:
+        notifier = &vapdev->cfg_notifier;
+        break;
     default:
         error_report("vfio: Unsupported device irq(%d)", irq);
         return;
@@ -175,6 +196,15 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
         warn_report_err(err);
     }
 
+    if (!vfio_ap_register_irq_notifier(vapdev, VFIO_AP_CFG_CHG_IRQ_INDEX, &err))
+    {
+        /*
+         * Report this error, but do not make it a failing condition.
+         * Lack of this IRQ in the host does not prevent normal operation.
+         */
+        warn_report_err(err);
+    }
+
     return;
 
 error:
@@ -187,6 +217,7 @@ static void vfio_ap_unrealize(DeviceState *dev)
     VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev);
 
     vfio_ap_unregister_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX);
+    vfio_ap_unregister_irq_notifier(vapdev, VFIO_AP_CFG_CHG_IRQ_INDEX);
     vfio_device_detach(&vapdev->vdev);
     g_free(vapdev->vdev.name);
 }
-- 
2.48.1

[RFC PATCH v9 2/4] hw/vfio/ap: store object indicating AP config changed in a queue

[Rorie Reyes]

Creates an object indicating that an AP configuration change event
has been received and stores it in a queue. These objects will later
be used to store event information for an AP configuration change
when the CHSC instruction is intercepted.

Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
---
 hw/vfio/ap.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index 3d0af7a54a..5ea5dd9cca 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -41,6 +41,13 @@ struct VFIOAPDevice {
     EventNotifier cfg_notifier;
 };
 
+typedef struct APConfigChgEvent {
+    QTAILQ_ENTRY(APConfigChgEvent) next;
+} APConfigChgEvent;
+
+QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
+    QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
+
 OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
 
 static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
@@ -74,12 +81,17 @@ static void vfio_ap_req_notifier_handler(void *opaque)
 
 static void vfio_ap_cfg_chg_notifier_handler(void *opaque)
 {
+    APConfigChgEvent *cfg_chg_event;
     VFIOAPDevice *vapdev = opaque;
 
     if (!event_notifier_test_and_clear(&vapdev->cfg_notifier)) {
         return;
     }
 
+    cfg_chg_event = g_new0(APConfigChgEvent, 1);
+
+    QTAILQ_INSERT_TAIL(&cfg_chg_events, cfg_chg_event, next);
+
     css_generate_css_crws(0);
 
 }
-- 
2.48.1

[RFC PATCH v9 3/4] hw/vfio/ap: Storing event information for an AP configuration change event

[Rorie Reyes]

These functions can be invoked by the function that handles interception
of the CHSC SEI instruction for requests indicating the accessibility of
one or more adjunct processors has changed.

Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
---
 hw/vfio/ap.c                 | 39 ++++++++++++++++++++++++++++++++++++
 include/hw/s390x/ap-bridge.h | 22 ++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index 5ea5dd9cca..4f88f80c54 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -96,6 +96,45 @@ static void vfio_ap_cfg_chg_notifier_handler(void *opaque)
 
 }
 
+int ap_chsc_sei_nt0_get_event(void *res)
+{
+    ChscSeiNt0Res *nt0_res  = (ChscSeiNt0Res *)res;
+    APConfigChgEvent *cfg_chg_event;
+
+    if (!ap_chsc_sei_nt0_have_event()) {
+        return 1;
+    }
+
+    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
+    memset(nt0_res, 0, sizeof(*nt0_res));
+
+    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
+    g_free(cfg_chg_event);
+
+    /*
+     * If there are any AP configuration change events in the queue,
+     * indicate to the caller that there is pending event info in
+     * the response block
+     */
+    if (ap_chsc_sei_nt0_have_event()) {
+        nt0_res->flags |= PENDING_EVENT_INFO_BITMASK;
+    }
+
+    nt0_res->length = sizeof(ChscSeiNt0Res);
+    nt0_res->code = NT0_RES_RESPONSE_CODE;
+    nt0_res->nt = NT0_RES_NT_DEFAULT;
+    nt0_res->rs = NT0_RES_RS_AP_CHANGE;
+    nt0_res->cc = NT0_RES_CC_AP_CHANGE;
+
+    return 0;
+
+}
+
+int ap_chsc_sei_nt0_have_event(void)
+{
+    return !QTAILQ_EMPTY(&cfg_chg_events);
+}
+
 static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
                                           unsigned int irq, Error **errp)
 {
diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h
index 470e439a98..f4d838bf99 100644
--- a/include/hw/s390x/ap-bridge.h
+++ b/include/hw/s390x/ap-bridge.h
@@ -16,4 +16,26 @@
 
 void s390_init_ap(void);
 
+typedef struct ChscSeiNt0Res {
+    uint16_t length;
+    uint16_t code;
+    uint8_t reserved1;
+    uint16_t reserved2;
+    uint8_t nt;
+#define PENDING_EVENT_INFO_BITMASK 0x80;
+    uint8_t flags;
+    uint8_t reserved3;
+    uint8_t rs;
+    uint8_t cc;
+} QEMU_PACKED ChscSeiNt0Res;
+
+#define NT0_RES_RESPONSE_CODE 1;
+#define NT0_RES_NT_DEFAULT    0;
+#define NT0_RES_RS_AP_CHANGE  5;
+#define NT0_RES_CC_AP_CHANGE  3;
+
+int ap_chsc_sei_nt0_get_event(void *res);
+
+int ap_chsc_sei_nt0_have_event(void);
+
 #endif
-- 
2.48.1

[RFC PATCH v9 4/4] s390: implementing CHSC SEI for AP config change

[Rorie Reyes]

Handle interception of the CHSC SEI instruction for requests
indicating the guest's AP configuration has changed.

If configuring --without-default-devices, hw/s390x/ap-stub.c
was created to handle such circumstance. Also added the
following to hw/s390x/meson.build if CONFIG_VFIO_AP is
false, it will use the stub file.

Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
---
 MAINTAINERS           |  1 +
 hw/s390x/ap-stub.c    | 25 +++++++++++++++++++++++++
 hw/s390x/meson.build  |  1 +
 target/s390x/ioinst.c | 11 +++++++++--
 4 files changed, 36 insertions(+), 2 deletions(-)
 create mode 100644 hw/s390x/ap-stub.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 23174b4ca7..070c746c69 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -112,6 +112,7 @@ F: hw/intc/s390_flic.c
 F: hw/intc/s390_flic_kvm.c
 F: hw/s390x/
 F: hw/vfio/ap.c
+F: hw/s390x/ap-stub.c
 F: hw/vfio/ccw.c
 F: hw/watchdog/wdt_diag288.c
 F: include/hw/s390x/
diff --git a/hw/s390x/ap-stub.c b/hw/s390x/ap-stub.c
new file mode 100644
index 0000000000..e2dacff959
--- /dev/null
+++ b/hw/s390x/ap-stub.c
@@ -0,0 +1,25 @@
+/*
+ * VFIO based AP matrix device assignment
+ *
+ * Copyright 2025 IBM Corp.
+ * Author(s): Rorie Reyes <rreyes@linux.ibm.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/s390x/ap-bridge.h"
+
+int ap_chsc_sei_nt0_get_event(void *res)
+{
+    return 0;
+}
+
+int ap_chsc_sei_nt0_have_event(void)
+{
+    return 0;
+}
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index 3bbebfd817..99cbcbd7d6 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -33,6 +33,7 @@ s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files(
 ))
 s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-ccw.c'))
 s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-vfio.c'))
+s390x_ss.add(when: 'CONFIG_VFIO_AP', if_false: files('ap-stub.c'))
 
 virtio_ss = ss.source_set()
 virtio_ss.add(files('virtio-ccw.c'))
diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
index fe62ba5b06..2320dd4c12 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -18,6 +18,7 @@
 #include "trace.h"
 #include "hw/s390x/s390-pci-bus.h"
 #include "target/s390x/kvm/pv.h"
+#include "hw/s390x/ap-bridge.h"
 
 /* All I/O instructions but chsc use the s format */
 static uint64_t get_address_from_regs(CPUS390XState *env, uint32_t ipb,
@@ -574,13 +575,19 @@ out:
 
 static int chsc_sei_nt0_get_event(void *res)
 {
-    /* no events yet */
+    if (s390_has_feat(S390_FEAT_AP)) {
+        return ap_chsc_sei_nt0_get_event(res);
+    }
+
     return 1;
 }
 
 static int chsc_sei_nt0_have_event(void)
 {
-    /* no events yet */
+    if (s390_has_feat(S390_FEAT_AP)) {
+        return ap_chsc_sei_nt0_have_event();
+    }
+
     return 0;
 }
 
-- 
2.48.1

Re: [RFC PATCH v9 4/4] s390: implementing CHSC SEI for AP config change

[Cédric Le Goater]

On 5/12/25 20:02, Rorie Reyes wrote:
> Handle interception of the CHSC SEI instruction for requests
> indicating the guest's AP configuration has changed.
> 
> If configuring --without-default-devices, hw/s390x/ap-stub.c
> was created to handle such circumstance. Also added the
> following to hw/s390x/meson.build if CONFIG_VFIO_AP is
> false, it will use the stub file.
> 
> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
> ---
>   MAINTAINERS           |  1 +
>   hw/s390x/ap-stub.c    | 25 +++++++++++++++++++++++++
>   hw/s390x/meson.build  |  1 +
>   target/s390x/ioinst.c | 11 +++++++++--
>   4 files changed, 36 insertions(+), 2 deletions(-)
>   create mode 100644 hw/s390x/ap-stub.c



Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> diff --git a/MAINTAINERS b/MAINTAINERS
> index 23174b4ca7..070c746c69 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -112,6 +112,7 @@ F: hw/intc/s390_flic.c
>   F: hw/intc/s390_flic_kvm.c
>   F: hw/s390x/
>   F: hw/vfio/ap.c
> +F: hw/s390x/ap-stub.c
>   F: hw/vfio/ccw.c
>   F: hw/watchdog/wdt_diag288.c
>   F: include/hw/s390x/
> diff --git a/hw/s390x/ap-stub.c b/hw/s390x/ap-stub.c
> new file mode 100644
> index 0000000000..e2dacff959
> --- /dev/null
> +++ b/hw/s390x/ap-stub.c
> @@ -0,0 +1,25 @@
> +/*
> + * VFIO based AP matrix device assignment
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Rorie Reyes <rreyes@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/s390x/ap-bridge.h"
> +
> +int ap_chsc_sei_nt0_get_event(void *res)
> +{
> +    return 0;
> +}
> +
> +int ap_chsc_sei_nt0_have_event(void)
> +{
> +    return 0;
> +}
> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> index 3bbebfd817..99cbcbd7d6 100644
> --- a/hw/s390x/meson.build
> +++ b/hw/s390x/meson.build
> @@ -33,6 +33,7 @@ s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files(
>   ))
>   s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-ccw.c'))
>   s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-vfio.c'))
> +s390x_ss.add(when: 'CONFIG_VFIO_AP', if_false: files('ap-stub.c'))
>   
>   virtio_ss = ss.source_set()
>   virtio_ss.add(files('virtio-ccw.c'))
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index fe62ba5b06..2320dd4c12 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -18,6 +18,7 @@
>   #include "trace.h"
>   #include "hw/s390x/s390-pci-bus.h"
>   #include "target/s390x/kvm/pv.h"
> +#include "hw/s390x/ap-bridge.h"
>   
>   /* All I/O instructions but chsc use the s format */
>   static uint64_t get_address_from_regs(CPUS390XState *env, uint32_t ipb,
> @@ -574,13 +575,19 @@ out:
>   
>   static int chsc_sei_nt0_get_event(void *res)
>   {
> -    /* no events yet */
> +    if (s390_has_feat(S390_FEAT_AP)) {
> +        return ap_chsc_sei_nt0_get_event(res);
> +    }
> +
>       return 1;
>   }
>   
>   static int chsc_sei_nt0_have_event(void)
>   {
> -    /* no events yet */
> +    if (s390_has_feat(S390_FEAT_AP)) {
> +        return ap_chsc_sei_nt0_have_event();
> +    }
> +
>       return 0;
>   }
>   

Re: [RFC PATCH v9 2/4] hw/vfio/ap: store object indicating AP config changed in a queue

[Anthony Krowiak]

On 5/12/25 2:02 PM, Rorie Reyes wrote:
> Creates an object indicating that an AP configuration change event
> has been received and stores it in a queue. These objects will later
> be used to store event information for an AP configuration change
> when the CHSC instruction is intercepted.
>
> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>

Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>

> ---
>   hw/vfio/ap.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
>
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 3d0af7a54a..5ea5dd9cca 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -41,6 +41,13 @@ struct VFIOAPDevice {
>       EventNotifier cfg_notifier;
>   };
>   
> +typedef struct APConfigChgEvent {
> +    QTAILQ_ENTRY(APConfigChgEvent) next;
> +} APConfigChgEvent;
> +
> +QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
> +    QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
> +
>   OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
>   
>   static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
> @@ -74,12 +81,17 @@ static void vfio_ap_req_notifier_handler(void *opaque)
>   
>   static void vfio_ap_cfg_chg_notifier_handler(void *opaque)
>   {
> +    APConfigChgEvent *cfg_chg_event;
>       VFIOAPDevice *vapdev = opaque;
>   
>       if (!event_notifier_test_and_clear(&vapdev->cfg_notifier)) {
>           return;
>       }
>   
> +    cfg_chg_event = g_new0(APConfigChgEvent, 1);
> +
> +    QTAILQ_INSERT_TAIL(&cfg_chg_events, cfg_chg_event, next);
> +
>       css_generate_css_crws(0);
>   
>   }

Re: [RFC PATCH v9 3/4] hw/vfio/ap: Storing event information for an AP configuration change event

[Anthony Krowiak]

On 5/12/25 2:02 PM, Rorie Reyes wrote:
> These functions can be invoked by the function that handles interception
> of the CHSC SEI instruction for requests indicating the accessibility of
> one or more adjunct processors has changed.
>
> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>

Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>

> ---
>   hw/vfio/ap.c                 | 39 ++++++++++++++++++++++++++++++++++++
>   include/hw/s390x/ap-bridge.h | 22 ++++++++++++++++++++
>   2 files changed, 61 insertions(+)
>
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 5ea5dd9cca..4f88f80c54 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -96,6 +96,45 @@ static void vfio_ap_cfg_chg_notifier_handler(void *opaque)
>   
>   }
>   
> +int ap_chsc_sei_nt0_get_event(void *res)
> +{
> +    ChscSeiNt0Res *nt0_res  = (ChscSeiNt0Res *)res;
> +    APConfigChgEvent *cfg_chg_event;
> +
> +    if (!ap_chsc_sei_nt0_have_event()) {
> +        return 1;
> +    }
> +
> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
> +    memset(nt0_res, 0, sizeof(*nt0_res));
> +
> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
> +    g_free(cfg_chg_event);
> +
> +    /*
> +     * If there are any AP configuration change events in the queue,
> +     * indicate to the caller that there is pending event info in
> +     * the response block
> +     */
> +    if (ap_chsc_sei_nt0_have_event()) {
> +        nt0_res->flags |= PENDING_EVENT_INFO_BITMASK;
> +    }
> +
> +    nt0_res->length = sizeof(ChscSeiNt0Res);
> +    nt0_res->code = NT0_RES_RESPONSE_CODE;
> +    nt0_res->nt = NT0_RES_NT_DEFAULT;
> +    nt0_res->rs = NT0_RES_RS_AP_CHANGE;
> +    nt0_res->cc = NT0_RES_CC_AP_CHANGE;
> +
> +    return 0;
> +
> +}
> +
> +int ap_chsc_sei_nt0_have_event(void)
> +{
> +    return !QTAILQ_EMPTY(&cfg_chg_events);
> +}
> +
>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>                                             unsigned int irq, Error **errp)
>   {
> diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h
> index 470e439a98..f4d838bf99 100644
> --- a/include/hw/s390x/ap-bridge.h
> +++ b/include/hw/s390x/ap-bridge.h
> @@ -16,4 +16,26 @@
>   
>   void s390_init_ap(void);
>   
> +typedef struct ChscSeiNt0Res {
> +    uint16_t length;
> +    uint16_t code;
> +    uint8_t reserved1;
> +    uint16_t reserved2;
> +    uint8_t nt;
> +#define PENDING_EVENT_INFO_BITMASK 0x80;
> +    uint8_t flags;
> +    uint8_t reserved3;
> +    uint8_t rs;
> +    uint8_t cc;
> +} QEMU_PACKED ChscSeiNt0Res;
> +
> +#define NT0_RES_RESPONSE_CODE 1;
> +#define NT0_RES_NT_DEFAULT    0;
> +#define NT0_RES_RS_AP_CHANGE  5;
> +#define NT0_RES_CC_AP_CHANGE  3;
> +
> +int ap_chsc_sei_nt0_get_event(void *res);
> +
> +int ap_chsc_sei_nt0_have_event(void);
> +
>   #endif

Re: [RFC PATCH v9 4/4] s390: implementing CHSC SEI for AP config change

[Anthony Krowiak]

On 5/12/25 2:02 PM, Rorie Reyes wrote:
> Handle interception of the CHSC SEI instruction for requests
> indicating the guest's AP configuration has changed.
>
> If configuring --without-default-devices, hw/s390x/ap-stub.c
> was created to handle such circumstance. Also added the
> following to hw/s390x/meson.build if CONFIG_VFIO_AP is
> false, it will use the stub file.
>
> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>

Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>

> ---
>   MAINTAINERS           |  1 +
>   hw/s390x/ap-stub.c    | 25 +++++++++++++++++++++++++
>   hw/s390x/meson.build  |  1 +
>   target/s390x/ioinst.c | 11 +++++++++--
>   4 files changed, 36 insertions(+), 2 deletions(-)
>   create mode 100644 hw/s390x/ap-stub.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 23174b4ca7..070c746c69 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -112,6 +112,7 @@ F: hw/intc/s390_flic.c
>   F: hw/intc/s390_flic_kvm.c
>   F: hw/s390x/
>   F: hw/vfio/ap.c
> +F: hw/s390x/ap-stub.c
>   F: hw/vfio/ccw.c
>   F: hw/watchdog/wdt_diag288.c
>   F: include/hw/s390x/
> diff --git a/hw/s390x/ap-stub.c b/hw/s390x/ap-stub.c
> new file mode 100644
> index 0000000000..e2dacff959
> --- /dev/null
> +++ b/hw/s390x/ap-stub.c
> @@ -0,0 +1,25 @@
> +/*
> + * VFIO based AP matrix device assignment
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Rorie Reyes <rreyes@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/s390x/ap-bridge.h"
> +
> +int ap_chsc_sei_nt0_get_event(void *res)
> +{
> +    return 0;
> +}
> +
> +int ap_chsc_sei_nt0_have_event(void)
> +{
> +    return 0;
> +}
> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> index 3bbebfd817..99cbcbd7d6 100644
> --- a/hw/s390x/meson.build
> +++ b/hw/s390x/meson.build
> @@ -33,6 +33,7 @@ s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files(
>   ))
>   s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-ccw.c'))
>   s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-vfio.c'))
> +s390x_ss.add(when: 'CONFIG_VFIO_AP', if_false: files('ap-stub.c'))
>   
>   virtio_ss = ss.source_set()
>   virtio_ss.add(files('virtio-ccw.c'))
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index fe62ba5b06..2320dd4c12 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -18,6 +18,7 @@
>   #include "trace.h"
>   #include "hw/s390x/s390-pci-bus.h"
>   #include "target/s390x/kvm/pv.h"
> +#include "hw/s390x/ap-bridge.h"
>   
>   /* All I/O instructions but chsc use the s format */
>   static uint64_t get_address_from_regs(CPUS390XState *env, uint32_t ipb,
> @@ -574,13 +575,19 @@ out:
>   
>   static int chsc_sei_nt0_get_event(void *res)
>   {
> -    /* no events yet */
> +    if (s390_has_feat(S390_FEAT_AP)) {
> +        return ap_chsc_sei_nt0_get_event(res);
> +    }
> +
>       return 1;
>   }
>   
>   static int chsc_sei_nt0_have_event(void)
>   {
> -    /* no events yet */
> +    if (s390_has_feat(S390_FEAT_AP)) {
> +        return ap_chsc_sei_nt0_have_event();
> +    }
> +
>       return 0;
>   }
>   

Re: [RFC PATCH v9 0/4] Report vfio-ap configuration changes

[Cédric Le Goater]

On 5/12/25 20:02, Rorie Reyes wrote:
> Changelog:
> v9:
> - added SPDX licensing to newly created file 'hw/s390x/ap-stub.c'
> 
> v8:
> - fixed windows cross-compile build
> - moved /hw/vfio/ap-stub.c to /hw/s390x/ap-stub.c
> - updated the use of stub file to MAINTAINERS to reflect new location
> - removed if_false for 'CONFIG_VFIO_AP' statement from /hw/vfio/meson.build
> - added if_false for 'CONFIG_VFIO_AP' to use ap-stub.c in /hw/s390x/meson.build
> - all those changes still address '--without-default-devices' issue from v5
> 
> v7:
> - Dropped initial commit for linux-header file vfio.h since I created two new commits
> to address the changes made in v6
> - Moved patches 6 and 7 to the beginning of the series after dropping the first patch
>     - Because I dropped the initial commit for linux-header file vfio.h, I had to add
> VFIO_AP_CFG_CHG_IRQ_INDEX
> - Resyncing latest to v6.15-rc3
> - Still need Thomas Huth's review of v5 changes for patch 6/6
> 
> v6:
> - Updating the update-linux-headers script to address kernel commit change 8a14
> - Update headers to retrieve uapi information for vfio-ap for update to Linux v6.15-rc1
> - Still need Thomas Huth's review of v5 changes for patch 7/7 (see below)
> 
> v5:
> - configuring using the '--without-default-devices' fails when building the code
> - created a stub file for functions ap_chsc_sei_nt0_get_event and ap_chsc_sei_nt0_have_event
> - add if_false for 'CONFIG_VFIO_AP' use ap-stub.c in meson.build
> - add the use of the stub file to MAINTAINERS since it's a new file
> 
> v4:
> - allocating cfg_chg_event before inserting into the queue
> - calling nt0_have_event in if loop to check if there are any
> elemenets in the queue, then calling QTAILQ_FIRST when the check
> passes
> - moving memset() after the check
> 
> v3:
> - changes that were made to patch 3/5 should have been made in
> patch 2/5
> 
> v2:
> - removed warnings that weren't needed
> - added unregister function
> - removed whitelines
> - changed variable names for consistency
> - removed rc variable and returning 1 or 0 outright
> - reversed logics for if statements
> - using g_free() instead of free()
> - replaced hardcoded numeric values by defining them with #define
> in the header
> 
> --------------------------------------------------------------------------
> This patch series creates and registers a handler that is called when
> userspace is notified by the kernel that a guest's AP configuration has
> changed. The handler in turn notifies the guest that its AP configuration
> has changed. This allows the guest to immediately respond to AP
> configuration changes rather than relying on polling or some other
> inefficient mechanism for detecting config changes.
> 
> Rorie Reyes (4):
>    hw/vfio/ap: notification handler for AP config changed event
>    hw/vfio/ap: store object indicating AP config changed in a queue
>    hw/vfio/ap: Storing event information for an AP configuration change
>      event
>    s390: implementing CHSC SEI for AP config change
> 
>   MAINTAINERS                  |  1 +
>   hw/s390x/ap-stub.c           | 25 +++++++++++
>   hw/s390x/meson.build         |  1 +
>   hw/vfio/ap.c                 | 82 ++++++++++++++++++++++++++++++++++++
>   include/hw/s390x/ap-bridge.h | 22 ++++++++++
>   target/s390x/ioinst.c        | 11 ++++-
>   6 files changed, 140 insertions(+), 2 deletions(-)
>   create mode 100644 hw/s390x/ap-stub.c
> 

Applied to vfio-next.

Thanks,

C.

Re: [RFC PATCH v9 4/4] s390: implementing CHSC SEI for AP config change

[Cédric Le Goater]

On 5/13/25 13:38, Anthony Krowiak wrote:
> 
> 
> 
> On 5/12/25 2:02 PM, Rorie Reyes wrote:
>> Handle interception of the CHSC SEI instruction for requests
>> indicating the guest's AP configuration has changed.
>>
>> If configuring --without-default-devices, hw/s390x/ap-stub.c
>> was created to handle such circumstance. Also added the
>> following to hw/s390x/meson.build if CONFIG_VFIO_AP is
>> false, it will use the stub file.
>>
>> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
> 
> Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> 
>> ---
>>   MAINTAINERS           |  1 +
>>   hw/s390x/ap-stub.c    | 25 +++++++++++++++++++++++++
>>   hw/s390x/meson.build  |  1 +
>>   target/s390x/ioinst.c | 11 +++++++++--
>>   4 files changed, 36 insertions(+), 2 deletions(-)
>>   create mode 100644 hw/s390x/ap-stub.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 23174b4ca7..070c746c69 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -112,6 +112,7 @@ F: hw/intc/s390_flic.c
>>   F: hw/intc/s390_flic_kvm.c
>>   F: hw/s390x/
>>   F: hw/vfio/ap.c
>> +F: hw/s390x/ap-stub.c
>>   F: hw/vfio/ccw.c
>>   F: hw/watchdog/wdt_diag288.c
>>   F: include/hw/s390x/
>> diff --git a/hw/s390x/ap-stub.c b/hw/s390x/ap-stub.c
>> new file mode 100644
>> index 0000000000..e2dacff959
>> --- /dev/null
>> +++ b/hw/s390x/ap-stub.c
>> @@ -0,0 +1,25 @@
>> +/*
>> + * VFIO based AP matrix device assignment
>> + *
>> + * Copyright 2025 IBM Corp.
>> + * Author(s): Rorie Reyes <rreyes@linux.ibm.com>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>> + * your option) any later version. See the COPYING file in the top-level
>> + * directory.

FYI, I dropped the redundent license boilerplate. See [1].

C.

[1] https://github.com/legoater/qemu/commit/8db3dbac401c56da6e865dcba1304f305555c22d

>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/s390x/ap-bridge.h"
>> +
>> +int ap_chsc_sei_nt0_get_event(void *res)
>> +{
>> +    return 0;
>> +}
>> +
>> +int ap_chsc_sei_nt0_have_event(void)
>> +{
>> +    return 0;
>> +}
>> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
>> index 3bbebfd817..99cbcbd7d6 100644
>> --- a/hw/s390x/meson.build
>> +++ b/hw/s390x/meson.build
>> @@ -33,6 +33,7 @@ s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files(
>>   ))
>>   s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-ccw.c'))
>>   s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-vfio.c'))
>> +s390x_ss.add(when: 'CONFIG_VFIO_AP', if_false: files('ap-stub.c'))
>>   virtio_ss = ss.source_set()
>>   virtio_ss.add(files('virtio-ccw.c'))
>> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
>> index fe62ba5b06..2320dd4c12 100644
>> --- a/target/s390x/ioinst.c
>> +++ b/target/s390x/ioinst.c
>> @@ -18,6 +18,7 @@
>>   #include "trace.h"
>>   #include "hw/s390x/s390-pci-bus.h"
>>   #include "target/s390x/kvm/pv.h"
>> +#include "hw/s390x/ap-bridge.h"
>>   /* All I/O instructions but chsc use the s format */
>>   static uint64_t get_address_from_regs(CPUS390XState *env, uint32_t ipb,
>> @@ -574,13 +575,19 @@ out:
>>   static int chsc_sei_nt0_get_event(void *res)
>>   {
>> -    /* no events yet */
>> +    if (s390_has_feat(S390_FEAT_AP)) {
>> +        return ap_chsc_sei_nt0_get_event(res);
>> +    }
>> +
>>       return 1;
>>   }
>>   static int chsc_sei_nt0_have_event(void)
>>   {
>> -    /* no events yet */
>> +    if (s390_has_feat(S390_FEAT_AP)) {
>> +        return ap_chsc_sei_nt0_have_event();
>> +    }
>> +
>>       return 0;
>>   }
> 

Re: [RFC PATCH v9 4/4] s390: implementing CHSC SEI for AP config change

[Rorie Reyes]

>>> diff --git a/hw/s390x/ap-stub.c b/hw/s390x/ap-stub.c
>>> new file mode 100644
>>> index 0000000000..e2dacff959
>>> --- /dev/null
>>> +++ b/hw/s390x/ap-stub.c
>>> @@ -0,0 +1,25 @@
>>> +/*
>>> + * VFIO based AP matrix device assignment
>>> + *
>>> + * Copyright 2025 IBM Corp.
>>> + * Author(s): Rorie Reyes <rreyes@linux.ibm.com>
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 
>>> or (at
>>> + * your option) any later version. See the COPYING file in the 
>>> top-level
>>> + * directory.
>
> FYI, I dropped the redundent license boilerplate. See [1].
>
> C.
>
> [1] 
> https://github.com/legoater/qemu/commit/8db3dbac401c56da6e865dcba1304f305555c22d
>
No problem! Thank you Cedric

Re: [RFC PATCH v9 2/4] hw/vfio/ap: store object indicating AP config changed in a queue

[Cédric Le Goater]

On 5/12/25 20:02, Rorie Reyes wrote:
> Creates an object indicating that an AP configuration change event
> has been received and stores it in a queue. These objects will later
> be used to store event information for an AP configuration change
> when the CHSC instruction is intercepted.
> 
> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
> ---
>   hw/vfio/ap.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 3d0af7a54a..5ea5dd9cca 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -41,6 +41,13 @@ struct VFIOAPDevice {
>       EventNotifier cfg_notifier;
>   };
>   
> +typedef struct APConfigChgEvent {
> +    QTAILQ_ENTRY(APConfigChgEvent) next;
> +} APConfigChgEvent;
> +
> +QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
> +    QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
> +

Could cfg_chg_events be static ?


Thanks,

C.



>   OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
>   
>   static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
> @@ -74,12 +81,17 @@ static void vfio_ap_req_notifier_handler(void *opaque)
>   
>   static void vfio_ap_cfg_chg_notifier_handler(void *opaque)
>   {
> +    APConfigChgEvent *cfg_chg_event;
>       VFIOAPDevice *vapdev = opaque;
>   
>       if (!event_notifier_test_and_clear(&vapdev->cfg_notifier)) {
>           return;
>       }
>   
> +    cfg_chg_event = g_new0(APConfigChgEvent, 1);
> +
> +    QTAILQ_INSERT_TAIL(&cfg_chg_events, cfg_chg_event, next);
> +
>       css_generate_css_crws(0);
>   
>   }

Re: [RFC PATCH v9 3/4] hw/vfio/ap: Storing event information for an AP configuration change event

[Cédric Le Goater]

On 5/12/25 20:02, Rorie Reyes wrote:
> These functions can be invoked by the function that handles interception
> of the CHSC SEI instruction for requests indicating the accessibility of
> one or more adjunct processors has changed.
> 
> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
> ---
>   hw/vfio/ap.c                 | 39 ++++++++++++++++++++++++++++++++++++
>   include/hw/s390x/ap-bridge.h | 22 ++++++++++++++++++++
>   2 files changed, 61 insertions(+)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 5ea5dd9cca..4f88f80c54 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -96,6 +96,45 @@ static void vfio_ap_cfg_chg_notifier_handler(void *opaque)
>   
>   }
>   
> +int ap_chsc_sei_nt0_get_event(void *res)
> +{
> +    ChscSeiNt0Res *nt0_res  = (ChscSeiNt0Res *)res;
> +    APConfigChgEvent *cfg_chg_event;
> +
> +    if (!ap_chsc_sei_nt0_have_event()) {
> +        return 1;
> +    }
> +
> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
> +    memset(nt0_res, 0, sizeof(*nt0_res));
> +
> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);

btw, I don't know if this was discussed. Are we OK to manipulate the
'cfg_chg_events' construct withou locking ?

> +    g_free(cfg_chg_event);
> +
> +    /*
> +     * If there are any AP configuration change events in the queue,
> +     * indicate to the caller that there is pending event info in
> +     * the response block
> +     */
> +    if (ap_chsc_sei_nt0_have_event()) {
> +        nt0_res->flags |= PENDING_EVENT_INFO_BITMASK;
> +    }
> +
> +    nt0_res->length = sizeof(ChscSeiNt0Res);
> +    nt0_res->code = NT0_RES_RESPONSE_CODE;
> +    nt0_res->nt = NT0_RES_NT_DEFAULT;
> +    nt0_res->rs = NT0_RES_RS_AP_CHANGE;
> +    nt0_res->cc = NT0_RES_CC_AP_CHANGE;
> +
> +    return 0;
> +
> +}
> +
> +int ap_chsc_sei_nt0_have_event(void)
> +{
> +    return !QTAILQ_EMPTY(&cfg_chg_events);
> +}
> +
>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>                                             unsigned int irq, Error **errp)
>   {
> diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h
> index 470e439a98..f4d838bf99 100644
> --- a/include/hw/s390x/ap-bridge.h
> +++ b/include/hw/s390x/ap-bridge.h
> @@ -16,4 +16,26 @@
>   
>   void s390_init_ap(void);
>   
> +typedef struct ChscSeiNt0Res {
> +    uint16_t length;
> +    uint16_t code;
> +    uint8_t reserved1;
> +    uint16_t reserved2;
> +    uint8_t nt;
> +#define PENDING_EVENT_INFO_BITMASK 0x80;
> +    uint8_t flags;
> +    uint8_t reserved3;
> +    uint8_t rs;
> +    uint8_t cc;
> +} QEMU_PACKED ChscSeiNt0Res;
> +
> +#define NT0_RES_RESPONSE_CODE 1;
> +#define NT0_RES_NT_DEFAULT    0;
> +#define NT0_RES_RS_AP_CHANGE  5;
> +#define NT0_RES_CC_AP_CHANGE  3;



please drop the ending ';'


Thanks,

C.


> +int ap_chsc_sei_nt0_get_event(void *res);
> +
> +int ap_chsc_sei_nt0_have_event(void);
> +
>   #endif

Re: [RFC PATCH v9 3/4] hw/vfio/ap: Storing event information for an AP configuration change event

[Cédric Le Goater]

On 5/12/25 20:02, Rorie Reyes wrote:
> These functions can be invoked by the function that handles interception
> of the CHSC SEI instruction for requests indicating the accessibility of
> one or more adjunct processors has changed.
> 
> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
> ---
>   hw/vfio/ap.c                 | 39 ++++++++++++++++++++++++++++++++++++
>   include/hw/s390x/ap-bridge.h | 22 ++++++++++++++++++++
>   2 files changed, 61 insertions(+)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 5ea5dd9cca..4f88f80c54 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -96,6 +96,45 @@ static void vfio_ap_cfg_chg_notifier_handler(void *opaque)
>   
>   }
>   
> +int ap_chsc_sei_nt0_get_event(void *res)
> +{
> +    ChscSeiNt0Res *nt0_res  = (ChscSeiNt0Res *)res;
> +    APConfigChgEvent *cfg_chg_event;
> +
> +    if (!ap_chsc_sei_nt0_have_event()) {
> +        return 1;
> +    }
> +
> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
> +    memset(nt0_res, 0, sizeof(*nt0_res));
> +
> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
> +    g_free(cfg_chg_event);
> +
> +    /*
> +     * If there are any AP configuration change events in the queue,
> +     * indicate to the caller that there is pending event info in
> +     * the response block
> +     */
> +    if (ap_chsc_sei_nt0_have_event()) {
> +        nt0_res->flags |= PENDING_EVENT_INFO_BITMASK;
> +    }
> +
> +    nt0_res->length = sizeof(ChscSeiNt0Res);
> +    nt0_res->code = NT0_RES_RESPONSE_CODE;
> +    nt0_res->nt = NT0_RES_NT_DEFAULT;
> +    nt0_res->rs = NT0_RES_RS_AP_CHANGE;
> +    nt0_res->cc = NT0_RES_CC_AP_CHANGE;
> +
> +    return 0;
> +

extra white line ^

and returning a bool would make more sense.

> +}
> +
> +int ap_chsc_sei_nt0_have_event(void)
> +{
> +    return !QTAILQ_EMPTY(&cfg_chg_events);

same here for the bool.

> +}
> +
>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>                                             unsigned int irq, Error **errp)
>   {
> diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h
> index 470e439a98..f4d838bf99 100644
> --- a/include/hw/s390x/ap-bridge.h
> +++ b/include/hw/s390x/ap-bridge.h
> @@ -16,4 +16,26 @@
>   
>   void s390_init_ap(void);
>   
> +typedef struct ChscSeiNt0Res {
> +    uint16_t length;
> +    uint16_t code;
> +    uint8_t reserved1;
> +    uint16_t reserved2;
> +    uint8_t nt;
> +#define PENDING_EVENT_INFO_BITMASK 0x80;
> +    uint8_t flags;
> +    uint8_t reserved3;
> +    uint8_t rs;
> +    uint8_t cc;
> +} QEMU_PACKED ChscSeiNt0Res;
> +
> +#define NT0_RES_RESPONSE_CODE 1;
> +#define NT0_RES_NT_DEFAULT    0;
> +#define NT0_RES_RS_AP_CHANGE  5;
> +#define NT0_RES_CC_AP_CHANGE  3;
> +
> +int ap_chsc_sei_nt0_get_event(void *res);

Documentation would be nice to have since a "return 1" means failure.

> +
> +int ap_chsc_sei_nt0_have_event(void);
> +>   #endif


Thanks,

C.

Re: [RFC PATCH v9 2/4] hw/vfio/ap: store object indicating AP config changed in a queue

[Rorie Reyes]

On 5/22/25 9:27 AM, Cédric Le Goater wrote:
> On 5/12/25 20:02, Rorie Reyes wrote:
>> Creates an object indicating that an AP configuration change event
>> has been received and stores it in a queue. These objects will later
>> be used to store event information for an AP configuration change
>> when the CHSC instruction is intercepted.
>>
>> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
>> ---
>>   hw/vfio/ap.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>> index 3d0af7a54a..5ea5dd9cca 100644
>> --- a/hw/vfio/ap.c
>> +++ b/hw/vfio/ap.c
>> @@ -41,6 +41,13 @@ struct VFIOAPDevice {
>>       EventNotifier cfg_notifier;
>>   };
>>   +typedef struct APConfigChgEvent {
>> +    QTAILQ_ENTRY(APConfigChgEvent) next;
>> +} APConfigChgEvent;
>> +
>> +QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
>> +    QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
>> +
>
> Could cfg_chg_events be static ?
>
>
> Thanks,
>
> C.
>
You make a good point, Cedric. I'm only using this variable in ap.c. 
Having it as a static would prevent it from being modified anywhere else

Re: [RFC PATCH v9 2/4] hw/vfio/ap: store object indicating AP config changed in a queue

[Cédric Le Goater]

On 5/22/25 16:28, Rorie Reyes wrote:
> 
> On 5/22/25 9:27 AM, Cédric Le Goater wrote:
>> On 5/12/25 20:02, Rorie Reyes wrote:
>>> Creates an object indicating that an AP configuration change event
>>> has been received and stores it in a queue. These objects will later
>>> be used to store event information for an AP configuration change
>>> when the CHSC instruction is intercepted.
>>>
>>> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
>>> ---
>>>   hw/vfio/ap.c | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>> index 3d0af7a54a..5ea5dd9cca 100644
>>> --- a/hw/vfio/ap.c
>>> +++ b/hw/vfio/ap.c
>>> @@ -41,6 +41,13 @@ struct VFIOAPDevice {
>>>       EventNotifier cfg_notifier;
>>>   };
>>>   +typedef struct APConfigChgEvent {
>>> +    QTAILQ_ENTRY(APConfigChgEvent) next;
>>> +} APConfigChgEvent;
>>> +
>>> +QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
>>> +    QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
>>> +
>>
>> Could cfg_chg_events be static ?
>>
>>
>> Thanks,
>>
>> C.
>>
> You make a good point, Cedric. I'm only using this variable in ap.c. Having it as a static would prevent it from being modified anywhere else
> 

Good. Would you have time to send a v10 ?


Thanks,

C.

Re: [RFC PATCH v9 2/4] hw/vfio/ap: store object indicating AP config changed in a queue

[Rorie Reyes]

On 5/22/25 11:36 AM, Cédric Le Goater wrote:
> On 5/22/25 16:28, Rorie Reyes wrote:
>>
>> On 5/22/25 9:27 AM, Cédric Le Goater wrote:
>>> On 5/12/25 20:02, Rorie Reyes wrote:
>>>> Creates an object indicating that an AP configuration change event
>>>> has been received and stores it in a queue. These objects will later
>>>> be used to store event information for an AP configuration change
>>>> when the CHSC instruction is intercepted.
>>>>
>>>> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
>>>> ---
>>>>   hw/vfio/ap.c | 12 ++++++++++++
>>>>   1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>>> index 3d0af7a54a..5ea5dd9cca 100644
>>>> --- a/hw/vfio/ap.c
>>>> +++ b/hw/vfio/ap.c
>>>> @@ -41,6 +41,13 @@ struct VFIOAPDevice {
>>>>       EventNotifier cfg_notifier;
>>>>   };
>>>>   +typedef struct APConfigChgEvent {
>>>> +    QTAILQ_ENTRY(APConfigChgEvent) next;
>>>> +} APConfigChgEvent;
>>>> +
>>>> +QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
>>>> +    QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
>>>> +
>>>
>>> Could cfg_chg_events be static ?
>>>
>>>
>>> Thanks,
>>>
>>> C.
>>>
>> You make a good point, Cedric. I'm only using this variable in ap.c. 
>> Having it as a static would prevent it from being modified anywhere else
>>
>
> Good. Would you have time to send a v10 ?
>
>
> Thanks,
>
> C.
>
>
I should be able to send a v10 by today. I'm currently working on the 
other two comments you left for patch 3

Re: [RFC PATCH v9 3/4] hw/vfio/ap: Storing event information for an AP configuration change event

[Rorie Reyes]

On 5/22/25 9:30 AM, Cédric Le Goater wrote:
> On 5/12/25 20:02, Rorie Reyes wrote:
>> These functions can be invoked by the function that handles interception
>> of the CHSC SEI instruction for requests indicating the accessibility of
>> one or more adjunct processors has changed.
>>
>> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
>> ---
>>   hw/vfio/ap.c                 | 39 ++++++++++++++++++++++++++++++++++++
>>   include/hw/s390x/ap-bridge.h | 22 ++++++++++++++++++++
>>   2 files changed, 61 insertions(+)
>>
>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>> index 5ea5dd9cca..4f88f80c54 100644
>> --- a/hw/vfio/ap.c
>> +++ b/hw/vfio/ap.c
>> @@ -96,6 +96,45 @@ static void vfio_ap_cfg_chg_notifier_handler(void 
>> *opaque)
>>     }
>>   +int ap_chsc_sei_nt0_get_event(void *res)
>> +{
>> +    ChscSeiNt0Res *nt0_res  = (ChscSeiNt0Res *)res;
>> +    APConfigChgEvent *cfg_chg_event;
>> +
>> +    if (!ap_chsc_sei_nt0_have_event()) {
>> +        return 1;
>> +    }
>> +
>> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
>> +    memset(nt0_res, 0, sizeof(*nt0_res));
>> +
>> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
>
> btw, I don't know if this was discussed. Are we OK to manipulate the
> 'cfg_chg_events' construct withou locking ?
>
I don't think it was discussed. Since I made it static, should we think 
about locking the construct? If so, would using 'static QemuMutex 
cfg_chg_events_lock;' to declare it and use 'qemu_mutex_lock()' and 
'qemu_mutex_unlock()' to lock and unlock when needed? Tony, do you have 
any thoughts on this process?

>> diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h
>> index 470e439a98..f4d838bf99 100644
>> --- a/include/hw/s390x/ap-bridge.h
>> +++ b/include/hw/s390x/ap-bridge.h
>> @@ -16,4 +16,26 @@
>>     void s390_init_ap(void);
>>   +typedef struct ChscSeiNt0Res {
>> +    uint16_t length;
>> +    uint16_t code;
>> +    uint8_t reserved1;
>> +    uint16_t reserved2;
>> +    uint8_t nt;
>> +#define PENDING_EVENT_INFO_BITMASK 0x80;
>> +    uint8_t flags;
>> +    uint8_t reserved3;
>> +    uint8_t rs;
>> +    uint8_t cc;
>> +} QEMU_PACKED ChscSeiNt0Res;
>> +
>> +#define NT0_RES_RESPONSE_CODE 1;
>> +#define NT0_RES_NT_DEFAULT    0;
>> +#define NT0_RES_RS_AP_CHANGE  5;
>> +#define NT0_RES_CC_AP_CHANGE  3;
>
>
>
> please drop the ending ';'
>
>
> Thanks,
>
> C.
>
Will drop in the next version

Re: [RFC PATCH v9 3/4] hw/vfio/ap: Storing event information for an AP configuration change event

[Anthony Krowiak]

On 5/22/25 9:30 AM, Cédric Le Goater wrote:
> On 5/12/25 20:02, Rorie Reyes wrote:
>> These functions can be invoked by the function that handles interception
>> of the CHSC SEI instruction for requests indicating the accessibility of
>> one or more adjunct processors has changed.
>>
>> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
>> ---
>>   hw/vfio/ap.c                 | 39 ++++++++++++++++++++++++++++++++++++
>>   include/hw/s390x/ap-bridge.h | 22 ++++++++++++++++++++
>>   2 files changed, 61 insertions(+)
>>
>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>> index 5ea5dd9cca..4f88f80c54 100644
>> --- a/hw/vfio/ap.c
>> +++ b/hw/vfio/ap.c
>> @@ -96,6 +96,45 @@ static void vfio_ap_cfg_chg_notifier_handler(void 
>> *opaque)
>>     }
>>   +int ap_chsc_sei_nt0_get_event(void *res)
>> +{
>> +    ChscSeiNt0Res *nt0_res  = (ChscSeiNt0Res *)res;
>> +    APConfigChgEvent *cfg_chg_event;
>> +
>> +    if (!ap_chsc_sei_nt0_have_event()) {
>> +        return 1;
>> +    }
>> +
>> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
>> +    memset(nt0_res, 0, sizeof(*nt0_res));
>> +
>> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
>
> btw, I don't know if this was discussed. Are we OK to manipulate the
> 'cfg_chg_events' construct withou locking ?

This has never been discussed, but it's an interesting question. The
ap_chsc_sei_nt0_get_event and ap_chsc_sei_nt0_have_event functions
are called as a result of a SIE exit to handle interception of a
CHSC SEI instruction. Handling of the intercepted instructions is
done under the Big QEMU Lock (see kvm_arch_handle_exit in 
target/s390x/kvm/kvm.c),
so presumably no other processes will get access to these functions
until the instruction is handled.

On the other hand, the vfio_cfg_chg_notifier_handler function that 
handles the eventfd
indicating the guest's AP configuration has been changed by the host 
device driver
adds  APConfigChgEvent objects to this queue. If, however, you think 
about the flow,
when the notifier handler gets called to handle an AP config changed 
event, it
queues a channel request word (CRW) indicating there is an SEI pending. 
Consequently,
the ap_chsc_sei_nt0_get_event and ap_chsc_sei_nt0_have_event functions 
will get called
only after the guest receives the CRW event and executes the CHSC SEI 
instruction. Since
the Big QEMU Lock is taken when the CHSC SE instruction is intercepted, 
it can not proceed
until whatever the holding process releases it; so for that flow, it 
seems highly likely if not
impossible for conflict given the event will always be added to the 
queue before an attempt
can be made to retrieve it.

Having gone through this dissertation, I don't see how it can hurt to 
lock the queue when
it is being accessed and would certainly make things bullet proof. What 
is your opinion?

>
>> +    g_free(cfg_chg_event);
>> +
>> +    /*
>> +     * If there are any AP configuration change events in the queue,
>> +     * indicate to the caller that there is pending event info in
>> +     * the response block
>> +     */
>> +    if (ap_chsc_sei_nt0_have_event()) {
>> +        nt0_res->flags |= PENDING_EVENT_INFO_BITMASK;
>> +    }
>> +
>> +    nt0_res->length = sizeof(ChscSeiNt0Res);
>> +    nt0_res->code = NT0_RES_RESPONSE_CODE;
>> +    nt0_res->nt = NT0_RES_NT_DEFAULT;
>> +    nt0_res->rs = NT0_RES_RS_AP_CHANGE;
>> +    nt0_res->cc = NT0_RES_CC_AP_CHANGE;
>> +
>> +    return 0;
>> +
>> +}
>> +
>> +int ap_chsc_sei_nt0_have_event(void)
>> +{
>> +    return !QTAILQ_EMPTY(&cfg_chg_events);
>> +}
>> +
>>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>>                                             unsigned int irq, Error 
>> **errp)
>>   {
>> diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h
>> index 470e439a98..f4d838bf99 100644
>> --- a/include/hw/s390x/ap-bridge.h
>> +++ b/include/hw/s390x/ap-bridge.h
>> @@ -16,4 +16,26 @@
>>     void s390_init_ap(void);
>>   +typedef struct ChscSeiNt0Res {
>> +    uint16_t length;
>> +    uint16_t code;
>> +    uint8_t reserved1;
>> +    uint16_t reserved2;
>> +    uint8_t nt;
>> +#define PENDING_EVENT_INFO_BITMASK 0x80;
>> +    uint8_t flags;
>> +    uint8_t reserved3;
>> +    uint8_t rs;
>> +    uint8_t cc;
>> +} QEMU_PACKED ChscSeiNt0Res;
>> +
>> +#define NT0_RES_RESPONSE_CODE 1;
>> +#define NT0_RES_NT_DEFAULT    0;
>> +#define NT0_RES_RS_AP_CHANGE  5;
>> +#define NT0_RES_CC_AP_CHANGE  3;
>
>
>
> please drop the ending ';'
>
>
> Thanks,
>
> C.
>
>
>> +int ap_chsc_sei_nt0_get_event(void *res);
>> +
>> +int ap_chsc_sei_nt0_have_event(void);
>> +
>>   #endif
>

Re: [RFC PATCH v9 3/4] hw/vfio/ap: Storing event information for an AP configuration change event

[Anthony Krowiak]

On 5/22/25 9:35 AM, Cédric Le Goater wrote:
> On 5/12/25 20:02, Rorie Reyes wrote:
>> These functions can be invoked by the function that handles interception
>> of the CHSC SEI instruction for requests indicating the accessibility of
>> one or more adjunct processors has changed.
>>
>> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
>> ---
>>   hw/vfio/ap.c                 | 39 ++++++++++++++++++++++++++++++++++++
>>   include/hw/s390x/ap-bridge.h | 22 ++++++++++++++++++++
>>   2 files changed, 61 insertions(+)
>>
>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>> index 5ea5dd9cca..4f88f80c54 100644
>> --- a/hw/vfio/ap.c
>> +++ b/hw/vfio/ap.c
>> @@ -96,6 +96,45 @@ static void vfio_ap_cfg_chg_notifier_handler(void 
>> *opaque)
>>     }
>>   +int ap_chsc_sei_nt0_get_event(void *res)
>> +{
>> +    ChscSeiNt0Res *nt0_res  = (ChscSeiNt0Res *)res;
>> +    APConfigChgEvent *cfg_chg_event;
>> +
>> +    if (!ap_chsc_sei_nt0_have_event()) {
>> +        return 1;
>> +    }
>> +
>> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
>> +    memset(nt0_res, 0, sizeof(*nt0_res));
>> +
>> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
>> +    g_free(cfg_chg_event);
>> +
>> +    /*
>> +     * If there are any AP configuration change events in the queue,
>> +     * indicate to the caller that there is pending event info in
>> +     * the response block
>> +     */
>> +    if (ap_chsc_sei_nt0_have_event()) {
>> +        nt0_res->flags |= PENDING_EVENT_INFO_BITMASK;
>> +    }
>> +
>> +    nt0_res->length = sizeof(ChscSeiNt0Res);
>> +    nt0_res->code = NT0_RES_RESPONSE_CODE;
>> +    nt0_res->nt = NT0_RES_NT_DEFAULT;
>> +    nt0_res->rs = NT0_RES_RS_AP_CHANGE;
>> +    nt0_res->cc = NT0_RES_CC_AP_CHANGE;
>> +
>> +    return 0;
>> +
>
> extra white line ^
>
> and returning a bool would make more sense.

It may make more sense from a readability standpoint,
but if you look at the caller (chsc_sei_nt0_get_event in 
target/s390x/ioinst.c),
it returns an int, so it can simply return whatever is returned from this
function. On the other hand, it really doesn't make a difference, so if
you feel strongly about this, then it can be changed to a bool.


>
>> +}
>> +
>> +int ap_chsc_sei_nt0_have_event(void)
>> +{
>> +    return !QTAILQ_EMPTY(&cfg_chg_events);
>
> same here for the bool.

same as above

>
>> +}
>> +
>>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>>                                             unsigned int irq, Error 
>> **errp)
>>   {
>> diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h
>> index 470e439a98..f4d838bf99 100644
>> --- a/include/hw/s390x/ap-bridge.h
>> +++ b/include/hw/s390x/ap-bridge.h
>> @@ -16,4 +16,26 @@
>>     void s390_init_ap(void);
>>   +typedef struct ChscSeiNt0Res {
>> +    uint16_t length;
>> +    uint16_t code;
>> +    uint8_t reserved1;
>> +    uint16_t reserved2;
>> +    uint8_t nt;
>> +#define PENDING_EVENT_INFO_BITMASK 0x80;
>> +    uint8_t flags;
>> +    uint8_t reserved3;
>> +    uint8_t rs;
>> +    uint8_t cc;
>> +} QEMU_PACKED ChscSeiNt0Res;
>> +
>> +#define NT0_RES_RESPONSE_CODE 1;
>> +#define NT0_RES_NT_DEFAULT    0;
>> +#define NT0_RES_RS_AP_CHANGE  5;
>> +#define NT0_RES_CC_AP_CHANGE  3;
>> +
>> +int ap_chsc_sei_nt0_get_event(void *res);
>
> Documentation would be nice to have since a "return 1" means failure.

True.

>
>> +
>> +int ap_chsc_sei_nt0_have_event(void);
>> +>   #endif
>
>
> Thanks,
>
> C.
>
>

Re: [RFC PATCH v9 3/4] hw/vfio/ap: Storing event information for an AP configuration change event

[Anthony Krowiak]

On 5/22/25 1:17 PM, Rorie Reyes wrote:
>
> On 5/22/25 9:30 AM, Cédric Le Goater wrote:
>> On 5/12/25 20:02, Rorie Reyes wrote:
>>> These functions can be invoked by the function that handles 
>>> interception
>>> of the CHSC SEI instruction for requests indicating the 
>>> accessibility of
>>> one or more adjunct processors has changed.
>>>
>>> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
>>> ---
>>>   hw/vfio/ap.c                 | 39 
>>> ++++++++++++++++++++++++++++++++++++
>>>   include/hw/s390x/ap-bridge.h | 22 ++++++++++++++++++++
>>>   2 files changed, 61 insertions(+)
>>>
>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>> index 5ea5dd9cca..4f88f80c54 100644
>>> --- a/hw/vfio/ap.c
>>> +++ b/hw/vfio/ap.c
>>> @@ -96,6 +96,45 @@ static void vfio_ap_cfg_chg_notifier_handler(void 
>>> *opaque)
>>>     }
>>>   +int ap_chsc_sei_nt0_get_event(void *res)
>>> +{
>>> +    ChscSeiNt0Res *nt0_res  = (ChscSeiNt0Res *)res;
>>> +    APConfigChgEvent *cfg_chg_event;
>>> +
>>> +    if (!ap_chsc_sei_nt0_have_event()) {
>>> +        return 1;
>>> +    }
>>> +
>>> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
>>> +    memset(nt0_res, 0, sizeof(*nt0_res));
>>> +
>>> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
>>
>> btw, I don't know if this was discussed. Are we OK to manipulate the
>> 'cfg_chg_events' construct withou locking ?
>>
> I don't think it was discussed. Since I made it static, should we 
> think about locking the construct? If so, would using 'static 
> QemuMutex cfg_chg_events_lock;' to declare it and use 
> 'qemu_mutex_lock()' and 'qemu_mutex_unlock()' to lock and unlock when 
> needed? Tony, do you have any thoughts on this process?

See my response to Cedric. If you decide to go with locking, I don't 
have a problem with using QemuMutex.

>
>>> diff --git a/include/hw/s390x/ap-bridge.h 
>>> b/include/hw/s390x/ap-bridge.h
>>> index 470e439a98..f4d838bf99 100644
>>> --- a/include/hw/s390x/ap-bridge.h
>>> +++ b/include/hw/s390x/ap-bridge.h
>>> @@ -16,4 +16,26 @@
>>>     void s390_init_ap(void);
>>>   +typedef struct ChscSeiNt0Res {
>>> +    uint16_t length;
>>> +    uint16_t code;
>>> +    uint8_t reserved1;
>>> +    uint16_t reserved2;
>>> +    uint8_t nt;
>>> +#define PENDING_EVENT_INFO_BITMASK 0x80;
>>> +    uint8_t flags;
>>> +    uint8_t reserved3;
>>> +    uint8_t rs;
>>> +    uint8_t cc;
>>> +} QEMU_PACKED ChscSeiNt0Res;
>>> +
>>> +#define NT0_RES_RESPONSE_CODE 1;
>>> +#define NT0_RES_NT_DEFAULT    0;
>>> +#define NT0_RES_RS_AP_CHANGE  5;
>>> +#define NT0_RES_CC_AP_CHANGE  3;
>>
>>
>>
>> please drop the ending ';'
>>
>>
>> Thanks,
>>
>> C.
>>
> Will drop in the next version

Re: [RFC PATCH v9 2/4] hw/vfio/ap: store object indicating AP config changed in a queue

[Rorie Reyes]

On 5/22/25 11:36 AM, Cédric Le Goater wrote:
> On 5/22/25 16:28, Rorie Reyes wrote:
>>
>> On 5/22/25 9:27 AM, Cédric Le Goater wrote:
>>> On 5/12/25 20:02, Rorie Reyes wrote:
>>>> Creates an object indicating that an AP configuration change event
>>>> has been received and stores it in a queue. These objects will later
>>>> be used to store event information for an AP configuration change
>>>> when the CHSC instruction is intercepted.
>>>>
>>>> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
>>>> ---
>>>>   hw/vfio/ap.c | 12 ++++++++++++
>>>>   1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>>> index 3d0af7a54a..5ea5dd9cca 100644
>>>> --- a/hw/vfio/ap.c
>>>> +++ b/hw/vfio/ap.c
>>>> @@ -41,6 +41,13 @@ struct VFIOAPDevice {
>>>>       EventNotifier cfg_notifier;
>>>>   };
>>>>   +typedef struct APConfigChgEvent {
>>>> +    QTAILQ_ENTRY(APConfigChgEvent) next;
>>>> +} APConfigChgEvent;
>>>> +
>>>> +QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
>>>> +    QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
>>>> +
>>>
>>> Could cfg_chg_events be static ?
>>>
>>>
>>> Thanks,
>>>
>>> C.
>>>
>> You make a good point, Cedric. I'm only using this variable in ap.c. 
>> Having it as a static would prevent it from being modified anywhere else
>>
>
> Good. Would you have time to send a v10 ?
>
>
> Thanks,
>
> C.
>
Hey Cedric, I thought I'd be able to send out a v10 earlier but hit some 
bugs when working on the patch 3 comments. I'm working on it now and 
I'll try to get it out ASAP. Thanks

Re: [RFC PATCH v9 3/4] hw/vfio/ap: Storing event information for an AP configuration change event

[Rorie Reyes]

[-- Attachment #1: Type: text/plain, Size: 4665 bytes --]


On 5/22/25 3:02 PM, Anthony Krowiak wrote:
>
>
>
> On 5/22/25 9:35 AM, Cédric Le Goater wrote:
>> On 5/12/25 20:02, Rorie Reyes wrote:
>>> These functions can be invoked by the function that handles 
>>> interception
>>> of the CHSC SEI instruction for requests indicating the 
>>> accessibility of
>>> one or more adjunct processors has changed.
>>>
>>> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
>>> ---
>>>   hw/vfio/ap.c                 | 39 
>>> ++++++++++++++++++++++++++++++++++++
>>>   include/hw/s390x/ap-bridge.h | 22 ++++++++++++++++++++
>>>   2 files changed, 61 insertions(+)
>>>
>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>> index 5ea5dd9cca..4f88f80c54 100644
>>> --- a/hw/vfio/ap.c
>>> +++ b/hw/vfio/ap.c
>>> @@ -96,6 +96,45 @@ static void vfio_ap_cfg_chg_notifier_handler(void 
>>> *opaque)
>>>     }
>>>   +int ap_chsc_sei_nt0_get_event(void *res)
>>> +{
>>> +    ChscSeiNt0Res *nt0_res  = (ChscSeiNt0Res *)res;
>>> +    APConfigChgEvent *cfg_chg_event;
>>> +
>>> +    if (!ap_chsc_sei_nt0_have_event()) {
>>> +        return 1;
>>> +    }
>>> +
>>> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
>>> +    memset(nt0_res, 0, sizeof(*nt0_res));
>>> +
>>> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
>>> +    g_free(cfg_chg_event);
>>> +
>>> +    /*
>>> +     * If there are any AP configuration change events in the queue,
>>> +     * indicate to the caller that there is pending event info in
>>> +     * the response block
>>> +     */
>>> +    if (ap_chsc_sei_nt0_have_event()) {
>>> +        nt0_res->flags |= PENDING_EVENT_INFO_BITMASK;
>>> +    }
>>> +
>>> +    nt0_res->length = sizeof(ChscSeiNt0Res);
>>> +    nt0_res->code = NT0_RES_RESPONSE_CODE;
>>> +    nt0_res->nt = NT0_RES_NT_DEFAULT;
>>> +    nt0_res->rs = NT0_RES_RS_AP_CHANGE;
>>> +    nt0_res->cc = NT0_RES_CC_AP_CHANGE;
>>> +
>>> +    return 0;
>>> +
>>
>> extra white line ^
>>
>> and returning a bool would make more sense.
>
> It may make more sense from a readability standpoint,
> but if you look at the caller (chsc_sei_nt0_get_event in 
> target/s390x/ioinst.c),
> it returns an int, so it can simply return whatever is returned from this
> function. On the other hand, it really doesn't make a difference, so if
> you feel strongly about this, then it can be changed to a bool.
>
>
I think I'd rather keep the returns as is because I've went through 
several changes that uses a bool return and my code doesn't work as 
designed. The ap config doesn't update in real time which allows the 
polling time to take over when notifying the guest. Would it suffice to 
document the following in ap-bridge.h (the comment bellow)
>>
>>> +}
>>> +
>>> +int ap_chsc_sei_nt0_have_event(void)
>>> +{
>>> +    return !QTAILQ_EMPTY(&cfg_chg_events);
>>
>> same here for the bool.
>
> same as above
>
>>
>>> +}
>>> +
>>>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>>>                                             unsigned int irq, Error 
>>> **errp)
>>>   {
>>> diff --git a/include/hw/s390x/ap-bridge.h 
>>> b/include/hw/s390x/ap-bridge.h
>>> index 470e439a98..f4d838bf99 100644
>>> --- a/include/hw/s390x/ap-bridge.h
>>> +++ b/include/hw/s390x/ap-bridge.h
>>> @@ -16,4 +16,26 @@
>>>     void s390_init_ap(void);
>>>   +typedef struct ChscSeiNt0Res {
>>> +    uint16_t length;
>>> +    uint16_t code;
>>> +    uint8_t reserved1;
>>> +    uint16_t reserved2;
>>> +    uint8_t nt;
>>> +#define PENDING_EVENT_INFO_BITMASK 0x80;
>>> +    uint8_t flags;
>>> +    uint8_t reserved3;
>>> +    uint8_t rs;
>>> +    uint8_t cc;
>>> +} QEMU_PACKED ChscSeiNt0Res;
>>> +
>>> +#define NT0_RES_RESPONSE_CODE 1;
>>> +#define NT0_RES_NT_DEFAULT    0;
>>> +#define NT0_RES_RS_AP_CHANGE  5;
>>> +#define NT0_RES_CC_AP_CHANGE  3;
>>> +
>>> +int ap_chsc_sei_nt0_get_event(void *res);
>>
>> Documentation would be nice to have since a "return 1" means failure.
>
> True.
>
Since I want to keep the return as is, will this documentation suffice?


/**
* ap_chsc_sei_nt0_get_event - Retrieve the next pending AP config
* change event
* @res:Pointer to a ChscSeiNt0Res struct to be filled with event
* data
*
* This function checks for any pending AP config change events and,
* if present, populates the provided response structure with the
* appropriate SEI NT0 fields.
*
* Return:
* 0 - An event was available and written to @res
* 1 - No event was available
*/

>>
>>> +
>>> +int ap_chsc_sei_nt0_have_event(void);
>>> +>   #endif
>>
>>
>> Thanks,
>>
>> C.
>>
>>
>

[-- Attachment #2: Type: text/html, Size: 9111 bytes --]

Re: [RFC PATCH v9 3/4] hw/vfio/ap: Storing event information for an AP configuration change event

[Rorie Reyes]

[-- Attachment #1: Type: text/plain, Size: 5219 bytes --]

Please ignore my comments bellow. I made some foundational mistakes 
going through this process. I should be able to return bools based on 
Cedric's remarks. I flipped the meaning behind returning 1 vs 0 as false 
vs true which is incorrect. Returning 1 is true and returning 0 is 
false. I had those concepts mixed up. I apologize

On 5/22/25 11:05 PM, Rorie Reyes wrote:
>
>
> On 5/22/25 3:02 PM, Anthony Krowiak wrote:
>>
>>
>>
>> On 5/22/25 9:35 AM, Cédric Le Goater wrote:
>>> On 5/12/25 20:02, Rorie Reyes wrote:
>>>> These functions can be invoked by the function that handles 
>>>> interception
>>>> of the CHSC SEI instruction for requests indicating the 
>>>> accessibility of
>>>> one or more adjunct processors has changed.
>>>>
>>>> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
>>>> ---
>>>>   hw/vfio/ap.c                 | 39 
>>>> ++++++++++++++++++++++++++++++++++++
>>>>   include/hw/s390x/ap-bridge.h | 22 ++++++++++++++++++++
>>>>   2 files changed, 61 insertions(+)
>>>>
>>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>>> index 5ea5dd9cca..4f88f80c54 100644
>>>> --- a/hw/vfio/ap.c
>>>> +++ b/hw/vfio/ap.c
>>>> @@ -96,6 +96,45 @@ static void 
>>>> vfio_ap_cfg_chg_notifier_handler(void *opaque)
>>>>     }
>>>>   +int ap_chsc_sei_nt0_get_event(void *res)
>>>> +{
>>>> +    ChscSeiNt0Res *nt0_res  = (ChscSeiNt0Res *)res;
>>>> +    APConfigChgEvent *cfg_chg_event;
>>>> +
>>>> +    if (!ap_chsc_sei_nt0_have_event()) {
>>>> +        return 1;
>>>> +    }
>>>> +
>>>> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
>>>> +    memset(nt0_res, 0, sizeof(*nt0_res));
>>>> +
>>>> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
>>>> +    g_free(cfg_chg_event);
>>>> +
>>>> +    /*
>>>> +     * If there are any AP configuration change events in the queue,
>>>> +     * indicate to the caller that there is pending event info in
>>>> +     * the response block
>>>> +     */
>>>> +    if (ap_chsc_sei_nt0_have_event()) {
>>>> +        nt0_res->flags |= PENDING_EVENT_INFO_BITMASK;
>>>> +    }
>>>> +
>>>> +    nt0_res->length = sizeof(ChscSeiNt0Res);
>>>> +    nt0_res->code = NT0_RES_RESPONSE_CODE;
>>>> +    nt0_res->nt = NT0_RES_NT_DEFAULT;
>>>> +    nt0_res->rs = NT0_RES_RS_AP_CHANGE;
>>>> +    nt0_res->cc = NT0_RES_CC_AP_CHANGE;
>>>> +
>>>> +    return 0;
>>>> +
>>>
>>> extra white line ^
>>>
>>> and returning a bool would make more sense.
>>
>> It may make more sense from a readability standpoint,
>> but if you look at the caller (chsc_sei_nt0_get_event in 
>> target/s390x/ioinst.c),
>> it returns an int, so it can simply return whatever is returned from 
>> this
>> function. On the other hand, it really doesn't make a difference, so if
>> you feel strongly about this, then it can be changed to a bool.
>>
>>
> I think I'd rather keep the returns as is because I've went through 
> several changes that uses a bool return and my code doesn't work as 
> designed. The ap config doesn't update in real time which allows the 
> polling time to take over when notifying the guest. Would it suffice 
> to document the following in ap-bridge.h (the comment bellow)
>>>
>>>> +}
>>>> +
>>>> +int ap_chsc_sei_nt0_have_event(void)
>>>> +{
>>>> +    return !QTAILQ_EMPTY(&cfg_chg_events);
>>>
>>> same here for the bool.
>>
>> same as above
>>
>>>
>>>> +}
>>>> +
>>>>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>>>>                                             unsigned int irq, Error 
>>>> **errp)
>>>>   {
>>>> diff --git a/include/hw/s390x/ap-bridge.h 
>>>> b/include/hw/s390x/ap-bridge.h
>>>> index 470e439a98..f4d838bf99 100644
>>>> --- a/include/hw/s390x/ap-bridge.h
>>>> +++ b/include/hw/s390x/ap-bridge.h
>>>> @@ -16,4 +16,26 @@
>>>>     void s390_init_ap(void);
>>>>   +typedef struct ChscSeiNt0Res {
>>>> +    uint16_t length;
>>>> +    uint16_t code;
>>>> +    uint8_t reserved1;
>>>> +    uint16_t reserved2;
>>>> +    uint8_t nt;
>>>> +#define PENDING_EVENT_INFO_BITMASK 0x80;
>>>> +    uint8_t flags;
>>>> +    uint8_t reserved3;
>>>> +    uint8_t rs;
>>>> +    uint8_t cc;
>>>> +} QEMU_PACKED ChscSeiNt0Res;
>>>> +
>>>> +#define NT0_RES_RESPONSE_CODE 1;
>>>> +#define NT0_RES_NT_DEFAULT    0;
>>>> +#define NT0_RES_RS_AP_CHANGE  5;
>>>> +#define NT0_RES_CC_AP_CHANGE  3;
>>>> +
>>>> +int ap_chsc_sei_nt0_get_event(void *res);
>>>
>>> Documentation would be nice to have since a "return 1" means failure.
>>
>> True.
>>
> Since I want to keep the return as is, will this documentation suffice?
>
>
> /**
> * ap_chsc_sei_nt0_get_event - Retrieve the next pending AP config
> * change event
> * @res:Pointer to a ChscSeiNt0Res struct to be filled with event
> * data
> *
> * This function checks for any pending AP config change events and,
> * if present, populates the provided response structure with the
> * appropriate SEI NT0 fields.
> *
> * Return:
> * 0 - An event was available and written to @res
> * 1 - No event was available
> */
>>>
>>>> +
>>>> +int ap_chsc_sei_nt0_have_event(void);
>>>> +>   #endif
>>>
>>>
>>> Thanks,
>>>
>>> C.
>>>
>>>
>>

[-- Attachment #2: Type: text/html, Size: 9222 bytes --]

Re: [RFC PATCH v9 3/4] hw/vfio/ap: Storing event information for an AP configuration change event

[Cédric Le Goater]

On 5/22/25 20:55, Anthony Krowiak wrote:
> 
> 
> 
> On 5/22/25 9:30 AM, Cédric Le Goater wrote:
>> On 5/12/25 20:02, Rorie Reyes wrote:
>>> These functions can be invoked by the function that handles interception
>>> of the CHSC SEI instruction for requests indicating the accessibility of
>>> one or more adjunct processors has changed.
>>>
>>> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
>>> ---
>>>   hw/vfio/ap.c                 | 39 ++++++++++++++++++++++++++++++++++++
>>>   include/hw/s390x/ap-bridge.h | 22 ++++++++++++++++++++
>>>   2 files changed, 61 insertions(+)
>>>
>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>> index 5ea5dd9cca..4f88f80c54 100644
>>> --- a/hw/vfio/ap.c
>>> +++ b/hw/vfio/ap.c
>>> @@ -96,6 +96,45 @@ static void vfio_ap_cfg_chg_notifier_handler(void *opaque)
>>>     }
>>>   +int ap_chsc_sei_nt0_get_event(void *res)
>>> +{
>>> +    ChscSeiNt0Res *nt0_res  = (ChscSeiNt0Res *)res;
>>> +    APConfigChgEvent *cfg_chg_event;
>>> +
>>> +    if (!ap_chsc_sei_nt0_have_event()) {
>>> +        return 1;
>>> +    }
>>> +
>>> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
>>> +    memset(nt0_res, 0, sizeof(*nt0_res));
>>> +
>>> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
>>
>> btw, I don't know if this was discussed. Are we OK to manipulate the
>> 'cfg_chg_events' construct withou locking ?
> 
> This has never been discussed, but it's an interesting question. The
> ap_chsc_sei_nt0_get_event and ap_chsc_sei_nt0_have_event functions
> are called as a result of a SIE exit to handle interception of a
> CHSC SEI instruction. Handling of the intercepted instructions is
> done under the Big QEMU Lock (see kvm_arch_handle_exit in target/s390x/kvm/kvm.c),
> so presumably no other processes will get access to these functions
> until the instruction is handled.
> 
> On the other hand, the vfio_cfg_chg_notifier_handler function that handles the eventfd
> indicating the guest's AP configuration has been changed by the host device driver
> adds  APConfigChgEvent objects to this queue. If, however, you think about the flow,
> when the notifier handler gets called to handle an AP config changed event, it
> queues a channel request word (CRW) indicating there is an SEI pending. Consequently,
> the ap_chsc_sei_nt0_get_event and ap_chsc_sei_nt0_have_event functions will get called
> only after the guest receives the CRW event and executes the CHSC SEI instruction. Since
> the Big QEMU Lock is taken when the CHSC SE instruction is intercepted, it can not proceed
> until whatever the holding process releases it; so for that flow, it seems highly likely if not
> impossible for conflict given the event will always be added to the queue before an attempt
> can be made to retrieve it.
> 
> Having gone through this dissertation, I don't see how it can hurt to lock the queue when
> it is being accessed and would certainly make things bullet proof. What is your opinion?

In that case, let's keep it simple (no mutex) and add a assert(bql_locked())
statement where we think the bql should be protecting access to shared
resources.

Thanks,

C.

Re: [RFC PATCH v9 3/4] hw/vfio/ap: Storing event information for an AP configuration change event

[Anthony Krowiak]

On 5/26/25 4:43 AM, Cédric Le Goater wrote:
> On 5/22/25 20:55, Anthony Krowiak wrote:
>>
>>
>>
>> On 5/22/25 9:30 AM, Cédric Le Goater wrote:
>>> On 5/12/25 20:02, Rorie Reyes wrote:
>>>> These functions can be invoked by the function that handles 
>>>> interception
>>>> of the CHSC SEI instruction for requests indicating the 
>>>> accessibility of
>>>> one or more adjunct processors has changed.
>>>>
>>>> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
>>>> ---
>>>>   hw/vfio/ap.c                 | 39 
>>>> ++++++++++++++++++++++++++++++++++++
>>>>   include/hw/s390x/ap-bridge.h | 22 ++++++++++++++++++++
>>>>   2 files changed, 61 insertions(+)
>>>>
>>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>>> index 5ea5dd9cca..4f88f80c54 100644
>>>> --- a/hw/vfio/ap.c
>>>> +++ b/hw/vfio/ap.c
>>>> @@ -96,6 +96,45 @@ static void 
>>>> vfio_ap_cfg_chg_notifier_handler(void *opaque)
>>>>     }
>>>>   +int ap_chsc_sei_nt0_get_event(void *res)
>>>> +{
>>>> +    ChscSeiNt0Res *nt0_res  = (ChscSeiNt0Res *)res;
>>>> +    APConfigChgEvent *cfg_chg_event;
>>>> +
>>>> +    if (!ap_chsc_sei_nt0_have_event()) {
>>>> +        return 1;
>>>> +    }
>>>> +
>>>> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
>>>> +    memset(nt0_res, 0, sizeof(*nt0_res));
>>>> +
>>>> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
>>>
>>> btw, I don't know if this was discussed. Are we OK to manipulate the
>>> 'cfg_chg_events' construct withou locking ?
>>
>> This has never been discussed, but it's an interesting question. The
>> ap_chsc_sei_nt0_get_event and ap_chsc_sei_nt0_have_event functions
>> are called as a result of a SIE exit to handle interception of a
>> CHSC SEI instruction. Handling of the intercepted instructions is
>> done under the Big QEMU Lock (see kvm_arch_handle_exit in 
>> target/s390x/kvm/kvm.c),
>> so presumably no other processes will get access to these functions
>> until the instruction is handled.
>>
>> On the other hand, the vfio_cfg_chg_notifier_handler function that 
>> handles the eventfd
>> indicating the guest's AP configuration has been changed by the host 
>> device driver
>> adds  APConfigChgEvent objects to this queue. If, however, you think 
>> about the flow,
>> when the notifier handler gets called to handle an AP config changed 
>> event, it
>> queues a channel request word (CRW) indicating there is an SEI 
>> pending. Consequently,
>> the ap_chsc_sei_nt0_get_event and ap_chsc_sei_nt0_have_event 
>> functions will get called
>> only after the guest receives the CRW event and executes the CHSC SEI 
>> instruction. Since
>> the Big QEMU Lock is taken when the CHSC SE instruction is 
>> intercepted, it can not proceed
>> until whatever the holding process releases it; so for that flow, it 
>> seems highly likely if not
>> impossible for conflict given the event will always be added to the 
>> queue before an attempt
>> can be made to retrieve it.
>>
>> Having gone through this dissertation, I don't see how it can hurt to 
>> lock the queue when
>> it is being accessed and would certainly make things bullet proof. 
>> What is your opinion?
>
> In that case, let's keep it simple (no mutex) and add a 
> assert(bql_locked())
> statement where we think the bql should be protecting access to shared

I don't think we want to do that in the vfio_cfg_chg_notifier_handler 
function because
that function is called when the host device driver signals an eventfd 
to notify
userspace that the guest's AP configuration has been changed and I don't 
think the
Big QEMU Lock is taken during that process.

> resources.
>
> Thanks,
>
> C.
>