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

[Anthony Krowiak <akrowiak@linux.ibm.com>]

On 5/23/25 12:47 AM, 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 | 36 ++++++++++++++++++++++++
>   2 files changed, 89 insertions(+)
>
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index fc435f5c5b..a6b4514606 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)
>   
>   }
>   
> +bool 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 true;
> +    }
> +
> +    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 false;
> +}

The boolean return values in the function above do not make logical sense.
What they are effectively saying is that event information has been stored
in the response when there is no event information to store (i.e., the event
queue is empty), and that event information has not been stored if the
response has been filled with event information.

After looking at the calling code in target/s390x/ionst.c, apparently 
the meaning of
the int value originally returned from the above function was not in 
fact intended to
be a boolean value. It looks like the caller uses this value to indicate 
whether the
response code should be set to 0x0001 (this function returns 0) or 
0x0005 (this
function returns 1); so, the boolean values returned match what is 
expected by
the caller. I think this is why your original pass at returning a 
boolean caused your
patch to fail; because you did what made logical sense in this function.

I think the calling code is very convoluted to say the least, so maybe 
what makes sense
here is to continue to return an int and use #define to document the 
return value; for
example:

#define EVENT_INFORMATION_STORED           0
#define EVENT_INFORMATION_NOT_STORED 1

It would probably make a great deal of sense to refactor the calling 
code, but that
could potentially open up a large can of worms, so at least this makes 
sense from
the perspective of reading this code.

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

It's probably fine to return boolean from the above function because it
makes sense even from the caller's perspective.

> +
>   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..ae31532f6e 100644
> --- a/include/hw/s390x/ap-bridge.h
> +++ b/include/hw/s390x/ap-bridge.h
> @@ -16,4 +16,40 @@
>   
>   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
> +
> +/**
> + * 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:
> + *   false - An event was available and written to @res
> + *   true - No event was available
> + */
> +bool ap_chsc_sei_nt0_get_event(void *res);
> +
> +bool ap_chsc_sei_nt0_have_event(void);
> +
>   #endif