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

[Rorie Reyes <rreyes@linux.ibm.com>]

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
>