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

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

[Rorie Reyes]

Changelog:
v11:
- reverted return type to int for 'ap_chsc_sei_nt0_get_event'
   - files reflected are 'ap.c', 'ap-bridge'h, and 'ap-stub.c'
- using defined variables to represent return 0 and 1 to reflect logical sense
- update documentation for 'ap_chsc_sei_nt0_get_event' to reflect return types

v10:
- Added reviewed by for patch 4 by Tony and Cedric
   - review needed for patch 2 and 3
- changed 'cfg_chg_events' to a static variable
- locked 'cfg_chg_events' using QemuMutex
- removed ';' at the end of the definition for NT0
- returning bools for 'ap_chsc_sei_nt0_get_event' and
'ap_chsc_sei_nt0_have_event'
- updated the header file that contains 'ap_chsc_sei_nt0_get_event' and
'ap_chsc_sei_nt0_have_event' to a bool function
- added documentation explaining the returning bool functions
- whitespace clean up

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           | 21 ++++++++
 hw/s390x/meson.build         |  1 +
 hw/vfio/ap.c                 | 96 ++++++++++++++++++++++++++++++++++++
 include/hw/s390x/ap-bridge.h | 39 +++++++++++++++
 target/s390x/ioinst.c        | 11 ++++-
 6 files changed, 167 insertions(+), 2 deletions(-)
 create mode 100644 hw/s390x/ap-stub.c

-- 
2.48.1

[RFC PATCH v11 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 785c0a0197..93c74ebedb 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;
@@ -137,6 +155,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;
@@ -176,6 +197,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:
@@ -188,6 +218,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 v11 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 93c74ebedb..fc435f5c5b 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;
+
+static 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 v11 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                 | 53 ++++++++++++++++++++++++++++++++++++
 include/hw/s390x/ap-bridge.h | 39 ++++++++++++++++++++++++++
 2 files changed, 92 insertions(+)

diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index fc435f5c5b..97a42a575a 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -10,6 +10,7 @@
  * directory.
  */
 
+#include <stdbool.h>
 #include "qemu/osdep.h"
 #include CONFIG_DEVICES /* CONFIG_IOMMUFD */
 #include <linux/vfio.h>
@@ -48,6 +49,8 @@ typedef struct APConfigChgEvent {
 static QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
     QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
 
+static QemuMutex cfg_chg_events_lock;
+
 OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
 
 static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
@@ -96,6 +99,49 @@ 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;
+
+    qemu_mutex_lock(&cfg_chg_events_lock);
+
+    if (!ap_chsc_sei_nt0_have_event()) {
+        qemu_mutex_unlock(&cfg_chg_events_lock);
+        return EVENT_INFORMATION_NOT_STORED;
+    }
+
+    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
+    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
+
+    qemu_mutex_unlock(&cfg_chg_events_lock);
+
+    memset(nt0_res, 0, sizeof(*nt0_res));
+    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 EVENT_INFORMATION_STORED;
+}
+
+bool 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)
 {
@@ -192,6 +238,13 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
     VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev);
     VFIODevice *vbasedev = &vapdev->vdev;
 
+    static bool lock_initialized;
+
+    if (!lock_initialized) {
+        qemu_mutex_init(&cfg_chg_events_lock);
+        lock_initialized = true;
+    }
+
     if (!vfio_device_get_name(vbasedev, errp)) {
         return;
     }
diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h
index 470e439a98..7efc52928d 100644
--- a/include/hw/s390x/ap-bridge.h
+++ b/include/hw/s390x/ap-bridge.h
@@ -16,4 +16,43 @@
 
 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
+
+#define EVENT_INFORMATION_NOT_STORED 1
+#define EVENT_INFORMATION_STORED     0
+
+/**
+ * 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:
+ *   EVENT_INFORMATION_STORED - An event was available and written to @res
+ *   EVENT_INFORMATION_NOT_STORED - No event was available
+ */
+int ap_chsc_sei_nt0_get_event(void *res);
+
+bool ap_chsc_sei_nt0_have_event(void);
+
 #endif
-- 
2.48.1

[RFC PATCH v11 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>
Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
---
 MAINTAINERS           |  1 +
 hw/s390x/ap-stub.c    | 21 +++++++++++++++++++++
 hw/s390x/meson.build  |  1 +
 target/s390x/ioinst.c | 11 +++++++++--
 4 files changed, 32 insertions(+), 2 deletions(-)
 create mode 100644 hw/s390x/ap-stub.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7060cf49b9..444523a35e 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..001fe5f8b0
--- /dev/null
+++ b/hw/s390x/ap-stub.c
@@ -0,0 +1,21 @@
+/*
+ * 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
+ */
+
+#include "qemu/osdep.h"
+#include "hw/s390x/ap-bridge.h"
+
+int ap_chsc_sei_nt0_get_event(void *res)
+{
+    return EVENT_INFORMATION_NOT_STORED;
+}
+
+bool ap_chsc_sei_nt0_have_event(void)
+{
+    return false;
+}
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 v11 3/4] hw/vfio/ap: Storing event information for an AP configuration change event

[Cédric Le Goater]

On 5/23/25 18:03, 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                 | 53 ++++++++++++++++++++++++++++++++++++
>   include/hw/s390x/ap-bridge.h | 39 ++++++++++++++++++++++++++
>   2 files changed, 92 insertions(+)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index fc435f5c5b..97a42a575a 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -10,6 +10,7 @@
>    * directory.
>    */
>   
> +#include <stdbool.h>
>   #include "qemu/osdep.h"
>   #include CONFIG_DEVICES /* CONFIG_IOMMUFD */
>   #include <linux/vfio.h>
> @@ -48,6 +49,8 @@ typedef struct APConfigChgEvent {
>   static QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
>       QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
>   
> +static QemuMutex cfg_chg_events_lock;
> +
>   OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
>   
>   static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
> @@ -96,6 +99,49 @@ 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;
> +
> +    qemu_mutex_lock(&cfg_chg_events_lock);

please consider using WITH_QEMU_LOCK_GUARD()

> +    if (!ap_chsc_sei_nt0_have_event()) {
> +        qemu_mutex_unlock(&cfg_chg_events_lock);
> +        return EVENT_INFORMATION_NOT_STORED;
> +    }
> +
> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
> +
> +    qemu_mutex_unlock(&cfg_chg_events_lock);
> +
> +    memset(nt0_res, 0, sizeof(*nt0_res));
> +    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 EVENT_INFORMATION_STORED;
> +}
> +
> +bool ap_chsc_sei_nt0_have_event(void)

hmm, no locking ?

> +{
> +    return !QTAILQ_EMPTY(&cfg_chg_events);
> +}
> +
>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>                                             unsigned int irq, Error **errp)
>   {
> @@ -192,6 +238,13 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>       VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev);
>       VFIODevice *vbasedev = &vapdev->vdev;
>   
> +    static bool lock_initialized;
> +
> +    if (!lock_initialized) {
> +        qemu_mutex_init(&cfg_chg_events_lock);
> +        lock_initialized = true;
> +    }

this could be replaced with a constructor routine. See hyperv.


Thanks,

C.



>       if (!vfio_device_get_name(vbasedev, errp)) {
>           return;
>       }
> diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h
> index 470e439a98..7efc52928d 100644
> --- a/include/hw/s390x/ap-bridge.h
> +++ b/include/hw/s390x/ap-bridge.h
> @@ -16,4 +16,43 @@
>   
>   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
> +
> +#define EVENT_INFORMATION_NOT_STORED 1
> +#define EVENT_INFORMATION_STORED     0
> +
> +/**
> + * 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:
> + *   EVENT_INFORMATION_STORED - An event was available and written to @res
> + *   EVENT_INFORMATION_NOT_STORED - No event was available
> + */
> +int ap_chsc_sei_nt0_get_event(void *res);
> +
> +bool ap_chsc_sei_nt0_have_event(void);
> +>   #endif

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

[Rorie Reyes]

Hey Cedric,

You mentioned the following in my v9 patches

"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. "

Does this still apply down bellow?

On 5/26/25 4:40 AM, Cédric Le Goater wrote:
> On 5/23/25 18:03, 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                 | 53 ++++++++++++++++++++++++++++++++++++
>>   include/hw/s390x/ap-bridge.h | 39 ++++++++++++++++++++++++++
>>   2 files changed, 92 insertions(+)
>>
>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>> index fc435f5c5b..97a42a575a 100644
>> --- a/hw/vfio/ap.c
>> +++ b/hw/vfio/ap.c
>> @@ -10,6 +10,7 @@
>>    * directory.
>>    */
>>   +#include <stdbool.h>
>>   #include "qemu/osdep.h"
>>   #include CONFIG_DEVICES /* CONFIG_IOMMUFD */
>>   #include <linux/vfio.h>
>> @@ -48,6 +49,8 @@ typedef struct APConfigChgEvent {
>>   static QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
>>       QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
>>   +static QemuMutex cfg_chg_events_lock;
>> +
>>   OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
>>     static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>> @@ -96,6 +99,49 @@ 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;
>> +
>> +    qemu_mutex_lock(&cfg_chg_events_lock);
>
> please consider using WITH_QEMU_LOCK_GUARD()
>
See note above about bql_locked
>> +    if (!ap_chsc_sei_nt0_have_event()) {
>> +        qemu_mutex_unlock(&cfg_chg_events_lock);
>> +        return EVENT_INFORMATION_NOT_STORED;
>> +    }
>> +
>> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
>> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
>> +
>> +    qemu_mutex_unlock(&cfg_chg_events_lock);
>> +
>> +    memset(nt0_res, 0, sizeof(*nt0_res));
>> +    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 EVENT_INFORMATION_STORED;
>> +}
>> +
>> +bool ap_chsc_sei_nt0_have_event(void)
>
> hmm, no locking ?
>
See not above for bql_locked
>> +{
>> +    return !QTAILQ_EMPTY(&cfg_chg_events);
>> +}
>> +
>>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>>                                             unsigned int irq, Error 
>> **errp)
>>   {
>> @@ -192,6 +238,13 @@ static void vfio_ap_realize(DeviceState *dev, 
>> Error **errp)
>>       VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev);
>>       VFIODevice *vbasedev = &vapdev->vdev;
>>   +    static bool lock_initialized;
>> +
>> +    if (!lock_initialized) {
>> +        qemu_mutex_init(&cfg_chg_events_lock);
>> +        lock_initialized = true;
>> +    }
>
> this could be replaced with a constructor routine. See hyperv.
>
>
> Thanks,
>
> C.
>
Noted
>
>
>>       if (!vfio_device_get_name(vbasedev, errp)) {
>>           return;
>>       }
>> diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h
>> index 470e439a98..7efc52928d 100644
>> --- a/include/hw/s390x/ap-bridge.h
>> +++ b/include/hw/s390x/ap-bridge.h
>> @@ -16,4 +16,43 @@
>>     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
>> +
>> +#define EVENT_INFORMATION_NOT_STORED 1
>> +#define EVENT_INFORMATION_STORED     0
>> +
>> +/**
>> + * 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:
>> + *   EVENT_INFORMATION_STORED - An event was available and written 
>> to @res
>> + *   EVENT_INFORMATION_NOT_STORED - No event was available
>> + */
>> +int ap_chsc_sei_nt0_get_event(void *res);
>> +
>> +bool ap_chsc_sei_nt0_have_event(void);
>> +>   #endif
>

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

[Cédric Le Goater]

On 6/3/25 14:58, Rorie Reyes wrote:
> Hey Cedric,
> 
> You mentioned the following in my v9 patches
> 
> "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. "
> 
> Does this still apply down bellow?

Anthony replied :

   https://lore.kernel.org/qemu-devel/ed2a2aa3-68a7-480c-a6a4-a8219af12d7b@linux.ibm.com/

Thanks,

C.

> 
> On 5/26/25 4:40 AM, Cédric Le Goater wrote:
>> On 5/23/25 18:03, 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                 | 53 ++++++++++++++++++++++++++++++++++++
>>>   include/hw/s390x/ap-bridge.h | 39 ++++++++++++++++++++++++++
>>>   2 files changed, 92 insertions(+)
>>>
>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>> index fc435f5c5b..97a42a575a 100644
>>> --- a/hw/vfio/ap.c
>>> +++ b/hw/vfio/ap.c
>>> @@ -10,6 +10,7 @@
>>>    * directory.
>>>    */
>>>   +#include <stdbool.h>
>>>   #include "qemu/osdep.h"
>>>   #include CONFIG_DEVICES /* CONFIG_IOMMUFD */
>>>   #include <linux/vfio.h>
>>> @@ -48,6 +49,8 @@ typedef struct APConfigChgEvent {
>>>   static QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
>>>       QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
>>>   +static QemuMutex cfg_chg_events_lock;
>>> +
>>>   OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
>>>     static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>>> @@ -96,6 +99,49 @@ 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;
>>> +
>>> +    qemu_mutex_lock(&cfg_chg_events_lock);
>>
>> please consider using WITH_QEMU_LOCK_GUARD()
>>
> See note above about bql_locked
>>> +    if (!ap_chsc_sei_nt0_have_event()) {
>>> +        qemu_mutex_unlock(&cfg_chg_events_lock);
>>> +        return EVENT_INFORMATION_NOT_STORED;
>>> +    }
>>> +
>>> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
>>> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
>>> +
>>> +    qemu_mutex_unlock(&cfg_chg_events_lock);
>>> +
>>> +    memset(nt0_res, 0, sizeof(*nt0_res));
>>> +    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 EVENT_INFORMATION_STORED;
>>> +}
>>> +
>>> +bool ap_chsc_sei_nt0_have_event(void)
>>
>> hmm, no locking ?
>>
> See not above for bql_locked
>>> +{
>>> +    return !QTAILQ_EMPTY(&cfg_chg_events);
>>> +}
>>> +
>>>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>>>                                             unsigned int irq, Error **errp)
>>>   {
>>> @@ -192,6 +238,13 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>>>       VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev);
>>>       VFIODevice *vbasedev = &vapdev->vdev;
>>>   +    static bool lock_initialized;
>>> +
>>> +    if (!lock_initialized) {
>>> +        qemu_mutex_init(&cfg_chg_events_lock);
>>> +        lock_initialized = true;
>>> +    }
>>
>> this could be replaced with a constructor routine. See hyperv.
>>
>>
>> Thanks,
>>
>> C.
>>
> Noted
>>
>>
>>>       if (!vfio_device_get_name(vbasedev, errp)) {
>>>           return;
>>>       }
>>> diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h
>>> index 470e439a98..7efc52928d 100644
>>> --- a/include/hw/s390x/ap-bridge.h
>>> +++ b/include/hw/s390x/ap-bridge.h
>>> @@ -16,4 +16,43 @@
>>>     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
>>> +
>>> +#define EVENT_INFORMATION_NOT_STORED 1
>>> +#define EVENT_INFORMATION_STORED     0
>>> +
>>> +/**
>>> + * 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:
>>> + *   EVENT_INFORMATION_STORED - An event was available and written to @res
>>> + *   EVENT_INFORMATION_NOT_STORED - No event was available
>>> + */
>>> +int ap_chsc_sei_nt0_get_event(void *res);
>>> +
>>> +bool ap_chsc_sei_nt0_have_event(void);
>>> +>   #endif
>>
> 

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

[Rorie Reyes]

On 6/3/25 10:21 AM, Cédric Le Goater wrote:
> On 6/3/25 14:58, Rorie Reyes wrote:
>> Hey Cedric,
>>
>> You mentioned the following in my v9 patches
>>
>> "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. "
>>
>> Does this still apply down bellow?
>
> Anthony replied :
>
> https://lore.kernel.org/qemu-devel/ed2a2aa3-68a7-480c-a6a4-a8219af12d7b@linux.ibm.com/
>
> Thanks,
>
> C.
>
So we'll still use WITH_QEMU_LOCK_GUARD?
>>
>> On 5/26/25 4:40 AM, Cédric Le Goater wrote:
>>> On 5/23/25 18:03, 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                 | 53 
>>>> ++++++++++++++++++++++++++++++++++++
>>>>   include/hw/s390x/ap-bridge.h | 39 ++++++++++++++++++++++++++
>>>>   2 files changed, 92 insertions(+)
>>>>
>>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>>> index fc435f5c5b..97a42a575a 100644
>>>> --- a/hw/vfio/ap.c
>>>> +++ b/hw/vfio/ap.c
>>>> @@ -10,6 +10,7 @@
>>>>    * directory.
>>>>    */
>>>>   +#include <stdbool.h>
>>>>   #include "qemu/osdep.h"
>>>>   #include CONFIG_DEVICES /* CONFIG_IOMMUFD */
>>>>   #include <linux/vfio.h>
>>>> @@ -48,6 +49,8 @@ typedef struct APConfigChgEvent {
>>>>   static QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
>>>>       QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
>>>>   +static QemuMutex cfg_chg_events_lock;
>>>> +
>>>>   OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
>>>>     static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>>>> @@ -96,6 +99,49 @@ 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;
>>>> +
>>>> +    qemu_mutex_lock(&cfg_chg_events_lock);
>>>
>>> please consider using WITH_QEMU_LOCK_GUARD()
>>>
>> See note above about bql_locked
>>>> +    if (!ap_chsc_sei_nt0_have_event()) {
>>>> +        qemu_mutex_unlock(&cfg_chg_events_lock);
>>>> +        return EVENT_INFORMATION_NOT_STORED;
>>>> +    }
>>>> +
>>>> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
>>>> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
>>>> +
>>>> +    qemu_mutex_unlock(&cfg_chg_events_lock);
>>>> +
>>>> +    memset(nt0_res, 0, sizeof(*nt0_res));
>>>> +    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 EVENT_INFORMATION_STORED;
>>>> +}
>>>> +
>>>> +bool ap_chsc_sei_nt0_have_event(void)
>>>
>>> hmm, no locking ?
>>>
>> See not above for bql_locked
>>>> +{
>>>> +    return !QTAILQ_EMPTY(&cfg_chg_events);
>>>> +}
>>>> +
>>>>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>>>>                                             unsigned int irq, Error 
>>>> **errp)
>>>>   {
>>>> @@ -192,6 +238,13 @@ static void vfio_ap_realize(DeviceState *dev, 
>>>> Error **errp)
>>>>       VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev);
>>>>       VFIODevice *vbasedev = &vapdev->vdev;
>>>>   +    static bool lock_initialized;
>>>> +
>>>> +    if (!lock_initialized) {
>>>> +        qemu_mutex_init(&cfg_chg_events_lock);
>>>> +        lock_initialized = true;
>>>> +    }
>>>
>>> this could be replaced with a constructor routine. See hyperv.
>>>
>>>
>>> Thanks,
>>>
>>> C.
>>>
>> Noted
>>>
>>>
>>>>       if (!vfio_device_get_name(vbasedev, errp)) {
>>>>           return;
>>>>       }
>>>> diff --git a/include/hw/s390x/ap-bridge.h 
>>>> b/include/hw/s390x/ap-bridge.h
>>>> index 470e439a98..7efc52928d 100644
>>>> --- a/include/hw/s390x/ap-bridge.h
>>>> +++ b/include/hw/s390x/ap-bridge.h
>>>> @@ -16,4 +16,43 @@
>>>>     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
>>>> +
>>>> +#define EVENT_INFORMATION_NOT_STORED 1
>>>> +#define EVENT_INFORMATION_STORED     0
>>>> +
>>>> +/**
>>>> + * 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:
>>>> + *   EVENT_INFORMATION_STORED - An event was available and written 
>>>> to @res
>>>> + *   EVENT_INFORMATION_NOT_STORED - No event was available
>>>> + */
>>>> +int ap_chsc_sei_nt0_get_event(void *res);
>>>> +
>>>> +bool ap_chsc_sei_nt0_have_event(void);
>>>> +>   #endif
>>>
>>
>

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

[Cédric Le Goater]

On 6/3/25 20:01, Rorie Reyes wrote:
> 
> On 6/3/25 10:21 AM, Cédric Le Goater wrote:
>> On 6/3/25 14:58, Rorie Reyes wrote:
>>> Hey Cedric,
>>>
>>> You mentioned the following in my v9 patches
>>>
>>> "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. "
>>>
>>> Does this still apply down bellow?
>>
>> Anthony replied :
>>
>> https://lore.kernel.org/qemu-devel/ed2a2aa3-68a7-480c-a6a4-a8219af12d7b@linux.ibm.com/
>>
>> Thanks,
>>
>> C.
>>
> So we'll still use WITH_QEMU_LOCK_GUARD?

If a lock is needed to protect the list, then ap_chsc_sei_nt0_have_event()
should lock/unlock too. WITH_QEMU_LOCK_GUARD() is just a pratical way to
do so.


Thanks,

C.



>>>
>>> On 5/26/25 4:40 AM, Cédric Le Goater wrote:
>>>> On 5/23/25 18:03, 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                 | 53 ++++++++++++++++++++++++++++++++++++
>>>>>   include/hw/s390x/ap-bridge.h | 39 ++++++++++++++++++++++++++
>>>>>   2 files changed, 92 insertions(+)
>>>>>
>>>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>>>> index fc435f5c5b..97a42a575a 100644
>>>>> --- a/hw/vfio/ap.c
>>>>> +++ b/hw/vfio/ap.c
>>>>> @@ -10,6 +10,7 @@
>>>>>    * directory.
>>>>>    */
>>>>>   +#include <stdbool.h>
>>>>>   #include "qemu/osdep.h"
>>>>>   #include CONFIG_DEVICES /* CONFIG_IOMMUFD */
>>>>>   #include <linux/vfio.h>
>>>>> @@ -48,6 +49,8 @@ typedef struct APConfigChgEvent {
>>>>>   static QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
>>>>>       QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
>>>>>   +static QemuMutex cfg_chg_events_lock;
>>>>> +
>>>>>   OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
>>>>>     static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>>>>> @@ -96,6 +99,49 @@ 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;
>>>>> +
>>>>> +    qemu_mutex_lock(&cfg_chg_events_lock);
>>>>
>>>> please consider using WITH_QEMU_LOCK_GUARD()
>>>>
>>> See note above about bql_locked
>>>>> +    if (!ap_chsc_sei_nt0_have_event()) {
>>>>> +        qemu_mutex_unlock(&cfg_chg_events_lock);
>>>>> +        return EVENT_INFORMATION_NOT_STORED;
>>>>> +    }
>>>>> +
>>>>> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
>>>>> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
>>>>> +
>>>>> +    qemu_mutex_unlock(&cfg_chg_events_lock);
>>>>> +
>>>>> +    memset(nt0_res, 0, sizeof(*nt0_res));
>>>>> +    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 EVENT_INFORMATION_STORED;
>>>>> +}
>>>>> +
>>>>> +bool ap_chsc_sei_nt0_have_event(void)
>>>>
>>>> hmm, no locking ?
>>>>
>>> See not above for bql_locked
>>>>> +{
>>>>> +    return !QTAILQ_EMPTY(&cfg_chg_events);
>>>>> +}
>>>>> +
>>>>>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>>>>>                                             unsigned int irq, Error **errp)
>>>>>   {
>>>>> @@ -192,6 +238,13 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>>>>>       VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev);
>>>>>       VFIODevice *vbasedev = &vapdev->vdev;
>>>>>   +    static bool lock_initialized;
>>>>> +
>>>>> +    if (!lock_initialized) {
>>>>> +        qemu_mutex_init(&cfg_chg_events_lock);
>>>>> +        lock_initialized = true;
>>>>> +    }
>>>>
>>>> this could be replaced with a constructor routine. See hyperv.
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> C.
>>>>
>>> Noted
>>>>
>>>>
>>>>>       if (!vfio_device_get_name(vbasedev, errp)) {
>>>>>           return;
>>>>>       }
>>>>> diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h
>>>>> index 470e439a98..7efc52928d 100644
>>>>> --- a/include/hw/s390x/ap-bridge.h
>>>>> +++ b/include/hw/s390x/ap-bridge.h
>>>>> @@ -16,4 +16,43 @@
>>>>>     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
>>>>> +
>>>>> +#define EVENT_INFORMATION_NOT_STORED 1
>>>>> +#define EVENT_INFORMATION_STORED     0
>>>>> +
>>>>> +/**
>>>>> + * 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:
>>>>> + *   EVENT_INFORMATION_STORED - An event was available and written to @res
>>>>> + *   EVENT_INFORMATION_NOT_STORED - No event was available
>>>>> + */
>>>>> +int ap_chsc_sei_nt0_get_event(void *res);
>>>>> +
>>>>> +bool ap_chsc_sei_nt0_have_event(void);
>>>>> +>   #endif
>>>>
>>>
>>
> 

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

[Anthony Krowiak]

On 6/3/25 4:30 PM, Cédric Le Goater wrote:
> On 6/3/25 20:01, Rorie Reyes wrote:
>>
>> On 6/3/25 10:21 AM, Cédric Le Goater wrote:
>>> On 6/3/25 14:58, Rorie Reyes wrote:
>>>> Hey Cedric,
>>>>
>>>> You mentioned the following in my v9 patches
>>>>
>>>> "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. "
>>>>
>>>> Does this still apply down bellow?
>>>
>>> Anthony replied :
>>>
>>> https://lore.kernel.org/qemu-devel/ed2a2aa3-68a7-480c-a6a4-a8219af12d7b@linux.ibm.com/ 
>>>
>>>
>>> Thanks,
>>>
>>> C.
>>>
>> So we'll still use WITH_QEMU_LOCK_GUARD?
>
> If a lock is needed to protect the list, then 
> ap_chsc_sei_nt0_have_event()
> should lock/unlock too. WITH_QEMU_LOCK_GUARD() is just a pratical way to
> do so.

Since ap_chsc_sei_nt0_have_event() is a single line that returns
!QTAILQ_EMPTY(&cfg_chg_events), wouldn't it be better to just
use the QEMU_LOCK_GUARD macro which, if I'm not mistaken,
will unlock on the return statement?

>
>
> Thanks,
>
> C.
>
>
>
>>>>
>>>> On 5/26/25 4:40 AM, Cédric Le Goater wrote:
>>>>> On 5/23/25 18:03, 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                 | 53 
>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>   include/hw/s390x/ap-bridge.h | 39 ++++++++++++++++++++++++++
>>>>>>   2 files changed, 92 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>>>>> index fc435f5c5b..97a42a575a 100644
>>>>>> --- a/hw/vfio/ap.c
>>>>>> +++ b/hw/vfio/ap.c
>>>>>> @@ -10,6 +10,7 @@
>>>>>>    * directory.
>>>>>>    */
>>>>>>   +#include <stdbool.h>
>>>>>>   #include "qemu/osdep.h"
>>>>>>   #include CONFIG_DEVICES /* CONFIG_IOMMUFD */
>>>>>>   #include <linux/vfio.h>
>>>>>> @@ -48,6 +49,8 @@ typedef struct APConfigChgEvent {
>>>>>>   static QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
>>>>>>       QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
>>>>>>   +static QemuMutex cfg_chg_events_lock;
>>>>>> +
>>>>>>   OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
>>>>>>     static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>>>>>> @@ -96,6 +99,49 @@ 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;
>>>>>> +
>>>>>> +    qemu_mutex_lock(&cfg_chg_events_lock);
>>>>>
>>>>> please consider using WITH_QEMU_LOCK_GUARD()
>>>>>
>>>> See note above about bql_locked
>>>>>> +    if (!ap_chsc_sei_nt0_have_event()) {
>>>>>> +        qemu_mutex_unlock(&cfg_chg_events_lock);
>>>>>> +        return EVENT_INFORMATION_NOT_STORED;
>>>>>> +    }
>>>>>> +
>>>>>> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
>>>>>> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
>>>>>> +
>>>>>> +    qemu_mutex_unlock(&cfg_chg_events_lock);
>>>>>> +
>>>>>> +    memset(nt0_res, 0, sizeof(*nt0_res));
>>>>>> +    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 EVENT_INFORMATION_STORED;
>>>>>> +}
>>>>>> +
>>>>>> +bool ap_chsc_sei_nt0_have_event(void)
>>>>>
>>>>> hmm, no locking ?
>>>>>
>>>> See not above for bql_locked
>>>>>> +{
>>>>>> +    return !QTAILQ_EMPTY(&cfg_chg_events);
>>>>>> +}
>>>>>> +
>>>>>>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>>>>>>                                             unsigned int irq, 
>>>>>> Error **errp)
>>>>>>   {
>>>>>> @@ -192,6 +238,13 @@ static void vfio_ap_realize(DeviceState 
>>>>>> *dev, Error **errp)
>>>>>>       VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev);
>>>>>>       VFIODevice *vbasedev = &vapdev->vdev;
>>>>>>   +    static bool lock_initialized;
>>>>>> +
>>>>>> +    if (!lock_initialized) {
>>>>>> +        qemu_mutex_init(&cfg_chg_events_lock);
>>>>>> +        lock_initialized = true;
>>>>>> +    }
>>>>>
>>>>> this could be replaced with a constructor routine. See hyperv.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> C.
>>>>>
>>>> Noted
>>>>>
>>>>>
>>>>>>       if (!vfio_device_get_name(vbasedev, errp)) {
>>>>>>           return;
>>>>>>       }
>>>>>> diff --git a/include/hw/s390x/ap-bridge.h 
>>>>>> b/include/hw/s390x/ap-bridge.h
>>>>>> index 470e439a98..7efc52928d 100644
>>>>>> --- a/include/hw/s390x/ap-bridge.h
>>>>>> +++ b/include/hw/s390x/ap-bridge.h
>>>>>> @@ -16,4 +16,43 @@
>>>>>>     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
>>>>>> +
>>>>>> +#define EVENT_INFORMATION_NOT_STORED 1
>>>>>> +#define EVENT_INFORMATION_STORED     0
>>>>>> +
>>>>>> +/**
>>>>>> + * 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:
>>>>>> + *   EVENT_INFORMATION_STORED - An event was available and 
>>>>>> written to @res
>>>>>> + *   EVENT_INFORMATION_NOT_STORED - No event was available
>>>>>> + */
>>>>>> +int ap_chsc_sei_nt0_get_event(void *res);
>>>>>> +
>>>>>> +bool ap_chsc_sei_nt0_have_event(void);
>>>>>> +>   #endif
>>>>>
>>>>
>>>
>>
>

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

[Rorie Reyes]

On 6/4/25 9:47 AM, Anthony Krowiak wrote:
>
>
>
> On 6/3/25 4:30 PM, Cédric Le Goater wrote:
>> On 6/3/25 20:01, Rorie Reyes wrote:
>>>
>>> On 6/3/25 10:21 AM, Cédric Le Goater wrote:
>>>> On 6/3/25 14:58, Rorie Reyes wrote:
>>>>> Hey Cedric,
>>>>>
>>>>> You mentioned the following in my v9 patches
>>>>>
>>>>> "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. "
>>>>>
>>>>> Does this still apply down bellow?
>>>>
>>>> Anthony replied :
>>>>
>>>> https://lore.kernel.org/qemu-devel/ed2a2aa3-68a7-480c-a6a4-a8219af12d7b@linux.ibm.com/ 
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> C.
>>>>
>>> So we'll still use WITH_QEMU_LOCK_GUARD?
>>
>> If a lock is needed to protect the list, then 
>> ap_chsc_sei_nt0_have_event()
>> should lock/unlock too. WITH_QEMU_LOCK_GUARD() is just a pratical way to
>> do so.
>
> Since ap_chsc_sei_nt0_have_event() is a single line that returns
> !QTAILQ_EMPTY(&cfg_chg_events), wouldn't it be better to just
> use the QEMU_LOCK_GUARD macro which, if I'm not mistaken,
> will unlock on the return statement?
>>
>>
>> Thanks,
>>
>> C.
>>
>>
>>
>>>>>
>>>>> On 5/26/25 4:40 AM, Cédric Le Goater wrote:
>>>>>> On 5/23/25 18:03, 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                 | 53 
>>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>>   include/hw/s390x/ap-bridge.h | 39 ++++++++++++++++++++++++++
>>>>>>>   2 files changed, 92 insertions(+)
>>>>>>>
>>>>>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>>>>>> index fc435f5c5b..97a42a575a 100644
>>>>>>> --- a/hw/vfio/ap.c
>>>>>>> +++ b/hw/vfio/ap.c
>>>>>>> @@ -10,6 +10,7 @@
>>>>>>>    * directory.
>>>>>>>    */
>>>>>>>   +#include <stdbool.h>
>>>>>>>   #include "qemu/osdep.h"
>>>>>>>   #include CONFIG_DEVICES /* CONFIG_IOMMUFD */
>>>>>>>   #include <linux/vfio.h>
>>>>>>> @@ -48,6 +49,8 @@ typedef struct APConfigChgEvent {
>>>>>>>   static QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
>>>>>>>       QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
>>>>>>>   +static QemuMutex cfg_chg_events_lock;
>>>>>>> +
>>>>>>>   OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
>>>>>>>     static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>>>>>>> @@ -96,6 +99,49 @@ 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;
>>>>>>> +
>>>>>>> +    qemu_mutex_lock(&cfg_chg_events_lock);
>>>>>>
>>>>>> please consider using WITH_QEMU_LOCK_GUARD()
>>>>>>
>>>>> See note above about bql_locked
>>>>>>> +    if (!ap_chsc_sei_nt0_have_event()) {
>>>>>>> +        qemu_mutex_unlock(&cfg_chg_events_lock);
>>>>>>> +        return EVENT_INFORMATION_NOT_STORED;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
>>>>>>> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
>>>>>>> +
>>>>>>> +    qemu_mutex_unlock(&cfg_chg_events_lock);
>>>>>>> +
>>>>>>> +    memset(nt0_res, 0, sizeof(*nt0_res));
>>>>>>> +    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 EVENT_INFORMATION_STORED;
>>>>>>> +}
>>>>>>> +
>>>>>>> +bool ap_chsc_sei_nt0_have_event(void)
>>>>>>
>>>>>> hmm, no locking ?
>>>>>>
How important do we need to lock this? When I lock this method my guest 
freezes every time. But when I only lock the get event, my code 
continues to work as designed
>>>>> See not above for bql_locked
>>>>>>> +{
>>>>>>> +    return !QTAILQ_EMPTY(&cfg_chg_events);
>>>>>>> +}
>>>>>>> +
>>>>>>>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>>>>>>>                                             unsigned int irq, 
>>>>>>> Error **errp)

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

[Anthony Krowiak]

On 6/5/25 1:57 PM, Rorie Reyes wrote:
>
> On 6/4/25 9:47 AM, Anthony Krowiak wrote:
>>
>>
>>
>> On 6/3/25 4:30 PM, Cédric Le Goater wrote:
>>> On 6/3/25 20:01, Rorie Reyes wrote:
>>>>
>>>> On 6/3/25 10:21 AM, Cédric Le Goater wrote:
>>>>> On 6/3/25 14:58, Rorie Reyes wrote:
>>>>>> Hey Cedric,
>>>>>>
>>>>>> You mentioned the following in my v9 patches
>>>>>>
>>>>>> "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. "
>>>>>>
>>>>>> Does this still apply down bellow?
>>>>>
>>>>> Anthony replied :
>>>>>
>>>>> https://lore.kernel.org/qemu-devel/ed2a2aa3-68a7-480c-a6a4-a8219af12d7b@linux.ibm.com/ 
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> C.
>>>>>
>>>> So we'll still use WITH_QEMU_LOCK_GUARD?
>>>
>>> If a lock is needed to protect the list, then 
>>> ap_chsc_sei_nt0_have_event()
>>> should lock/unlock too. WITH_QEMU_LOCK_GUARD() is just a pratical 
>>> way to
>>> do so.
>>
>> Since ap_chsc_sei_nt0_have_event() is a single line that returns
>> !QTAILQ_EMPTY(&cfg_chg_events), wouldn't it be better to just
>> use the QEMU_LOCK_GUARD macro which, if I'm not mistaken,
>> will unlock on the return statement?
>>>
>>>
>>> Thanks,
>>>
>>> C.
>>>
>>>
>>>
>>>>>>
>>>>>> On 5/26/25 4:40 AM, Cédric Le Goater wrote:
>>>>>>> On 5/23/25 18:03, 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                 | 53 
>>>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>>>   include/hw/s390x/ap-bridge.h | 39 ++++++++++++++++++++++++++
>>>>>>>>   2 files changed, 92 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>>>>>>> index fc435f5c5b..97a42a575a 100644
>>>>>>>> --- a/hw/vfio/ap.c
>>>>>>>> +++ b/hw/vfio/ap.c
>>>>>>>> @@ -10,6 +10,7 @@
>>>>>>>>    * directory.
>>>>>>>>    */
>>>>>>>>   +#include <stdbool.h>
>>>>>>>>   #include "qemu/osdep.h"
>>>>>>>>   #include CONFIG_DEVICES /* CONFIG_IOMMUFD */
>>>>>>>>   #include <linux/vfio.h>
>>>>>>>> @@ -48,6 +49,8 @@ typedef struct APConfigChgEvent {
>>>>>>>>   static QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
>>>>>>>>       QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
>>>>>>>>   +static QemuMutex cfg_chg_events_lock;
>>>>>>>> +
>>>>>>>>   OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
>>>>>>>>     static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>>>>>>>> @@ -96,6 +99,49 @@ 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;
>>>>>>>> +
>>>>>>>> +    qemu_mutex_lock(&cfg_chg_events_lock);
>>>>>>>
>>>>>>> please consider using WITH_QEMU_LOCK_GUARD()
>>>>>>>
>>>>>> See note above about bql_locked
>>>>>>>> +    if (!ap_chsc_sei_nt0_have_event()) {
>>>>>>>> + qemu_mutex_unlock(&cfg_chg_events_lock);
>>>>>>>> +        return EVENT_INFORMATION_NOT_STORED;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
>>>>>>>> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
>>>>>>>> +
>>>>>>>> +    qemu_mutex_unlock(&cfg_chg_events_lock);
>>>>>>>> +
>>>>>>>> +    memset(nt0_res, 0, sizeof(*nt0_res));
>>>>>>>> +    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 EVENT_INFORMATION_STORED;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +bool ap_chsc_sei_nt0_have_event(void)
>>>>>>>
>>>>>>> hmm, no locking ?
>>>>>>>
> How important do we need to lock this? When I lock this method my 
> guest freezes every time. But when I only lock the get event, my code 
> continues to work as designed

Try locking in both functions, but instead of calling the have event 
function from the get event function, use
!QTAILQ_EMPTY(&cfg_chg_events) in the get event function. If you are 
holding the lock when you call the have event function, you will be 
locking the same lock twice and it will hang when you lock the second 
time because the lock is already held.

>>>>>> See not above for bql_locked
>>>>>>>> +{
>>>>>>>> +    return !QTAILQ_EMPTY(&cfg_chg_events);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>>>>>>>>                                             unsigned int irq, 
>>>>>>>> Error **errp)

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

[Rorie Reyes]

On 6/6/25 7:48 AM, Anthony Krowiak wrote:
>
>
>
> On 6/5/25 1:57 PM, Rorie Reyes wrote:
>>
>> On 6/4/25 9:47 AM, Anthony Krowiak wrote:
>>>
>>>
>>>
>>> On 6/3/25 4:30 PM, Cédric Le Goater wrote:
>>>> On 6/3/25 20:01, Rorie Reyes wrote:
>>>>>
>>>>> On 6/3/25 10:21 AM, Cédric Le Goater wrote:
>>>>>> On 6/3/25 14:58, Rorie Reyes wrote:
>>>>>>> Hey Cedric,
>>>>>>>
>>>>>>> You mentioned the following in my v9 patches
>>>>>>>
>>>>>>> "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. "
>>>>>>>
>>>>>>> Does this still apply down bellow?
>>>>>>
>>>>>> Anthony replied :
>>>>>>
>>>>>> https://lore.kernel.org/qemu-devel/ed2a2aa3-68a7-480c-a6a4-a8219af12d7b@linux.ibm.com/ 
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> C.
>>>>>>
>>>>> So we'll still use WITH_QEMU_LOCK_GUARD?
>>>>
>>>> If a lock is needed to protect the list, then 
>>>> ap_chsc_sei_nt0_have_event()
>>>> should lock/unlock too. WITH_QEMU_LOCK_GUARD() is just a pratical 
>>>> way to
>>>> do so.
>>>
>>> Since ap_chsc_sei_nt0_have_event() is a single line that returns
>>> !QTAILQ_EMPTY(&cfg_chg_events), wouldn't it be better to just
>>> use the QEMU_LOCK_GUARD macro which, if I'm not mistaken,
>>> will unlock on the return statement?
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> C.
>>>>
>>>>
>>>>
>>>>>>>
>>>>>>> On 5/26/25 4:40 AM, Cédric Le Goater wrote:
>>>>>>>> On 5/23/25 18:03, 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                 | 53 
>>>>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>>>>   include/hw/s390x/ap-bridge.h | 39 ++++++++++++++++++++++++++
>>>>>>>>>   2 files changed, 92 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>>>>>>>> index fc435f5c5b..97a42a575a 100644
>>>>>>>>> --- a/hw/vfio/ap.c
>>>>>>>>> +++ b/hw/vfio/ap.c
>>>>>>>>> @@ -10,6 +10,7 @@
>>>>>>>>>    * directory.
>>>>>>>>>    */
>>>>>>>>>   +#include <stdbool.h>
>>>>>>>>>   #include "qemu/osdep.h"
>>>>>>>>>   #include CONFIG_DEVICES /* CONFIG_IOMMUFD */
>>>>>>>>>   #include <linux/vfio.h>
>>>>>>>>> @@ -48,6 +49,8 @@ typedef struct APConfigChgEvent {
>>>>>>>>>   static QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
>>>>>>>>>       QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
>>>>>>>>>   +static QemuMutex cfg_chg_events_lock;
>>>>>>>>> +
>>>>>>>>>   OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
>>>>>>>>>     static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>>>>>>>>> @@ -96,6 +99,49 @@ 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;
>>>>>>>>> +
>>>>>>>>> +    qemu_mutex_lock(&cfg_chg_events_lock);
>>>>>>>>
>>>>>>>> please consider using WITH_QEMU_LOCK_GUARD()
>>>>>>>>
>>>>>>> See note above about bql_locked
>>>>>>>>> +    if (!ap_chsc_sei_nt0_have_event()) {
>>>>>>>>> + qemu_mutex_unlock(&cfg_chg_events_lock);
>>>>>>>>> +        return EVENT_INFORMATION_NOT_STORED;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
>>>>>>>>> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
>>>>>>>>> +
>>>>>>>>> +    qemu_mutex_unlock(&cfg_chg_events_lock);
>>>>>>>>> +
>>>>>>>>> +    memset(nt0_res, 0, sizeof(*nt0_res));
>>>>>>>>> +    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 EVENT_INFORMATION_STORED;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +bool ap_chsc_sei_nt0_have_event(void)
>>>>>>>>
>>>>>>>> hmm, no locking ?
>>>>>>>>
>> How important do we need to lock this? When I lock this method my 
>> guest freezes every time. But when I only lock the get event, my code 
>> continues to work as designed
>
> Try locking in both functions, but instead of calling the have event 
> function from the get event function, use
> !QTAILQ_EMPTY(&cfg_chg_events) in the get event function. If you are 
> holding the lock when you call the have event function, you will be 
> locking the same lock twice and it will hang when you lock the second 
> time because the lock is already held.
>
Let me give that a shot. Thanks Tony
>>>>>>> See not above for bql_locked
>>>>>>>>> +{
>>>>>>>>> +    return !QTAILQ_EMPTY(&cfg_chg_events);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>>>>>>>>> unsigned int irq, Error **errp)
>