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

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

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)