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

["Cédric Le Goater" <clg@redhat.com>]

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