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

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

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