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

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

[-- Attachment #1: Type: text/plain, Size: 5219 bytes --]

Please ignore my comments bellow. I made some foundational mistakes 
going through this process. I should be able to return bools based on 
Cedric's remarks. I flipped the meaning behind returning 1 vs 0 as false 
vs true which is incorrect. Returning 1 is true and returning 0 is 
false. I had those concepts mixed up. I apologize

On 5/22/25 11:05 PM, Rorie Reyes wrote:
>
>
> On 5/22/25 3:02 PM, Anthony Krowiak wrote:
>>
>>
>>
>> On 5/22/25 9:35 AM, Cédric Le Goater wrote:
>>> On 5/12/25 20:02, 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                 | 39 
>>>> ++++++++++++++++++++++++++++++++++++
>>>>   include/hw/s390x/ap-bridge.h | 22 ++++++++++++++++++++
>>>>   2 files changed, 61 insertions(+)
>>>>
>>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>>> index 5ea5dd9cca..4f88f80c54 100644
>>>> --- a/hw/vfio/ap.c
>>>> +++ b/hw/vfio/ap.c
>>>> @@ -96,6 +96,45 @@ 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;
>>>> +
>>>> +    if (!ap_chsc_sei_nt0_have_event()) {
>>>> +        return 1;
>>>> +    }
>>>> +
>>>> +    cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
>>>> +    memset(nt0_res, 0, sizeof(*nt0_res));
>>>> +
>>>> +    QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
>>>> +    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 0;
>>>> +
>>>
>>> extra white line ^
>>>
>>> and returning a bool would make more sense.
>>
>> It may make more sense from a readability standpoint,
>> but if you look at the caller (chsc_sei_nt0_get_event in 
>> target/s390x/ioinst.c),
>> it returns an int, so it can simply return whatever is returned from 
>> this
>> function. On the other hand, it really doesn't make a difference, so if
>> you feel strongly about this, then it can be changed to a bool.
>>
>>
> I think I'd rather keep the returns as is because I've went through 
> several changes that uses a bool return and my code doesn't work as 
> designed. The ap config doesn't update in real time which allows the 
> polling time to take over when notifying the guest. Would it suffice 
> to document the following in ap-bridge.h (the comment bellow)
>>>
>>>> +}
>>>> +
>>>> +int ap_chsc_sei_nt0_have_event(void)
>>>> +{
>>>> +    return !QTAILQ_EMPTY(&cfg_chg_events);
>>>
>>> same here for the bool.
>>
>> same as above
>>
>>>
>>>> +}
>>>> +
>>>>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>>>>                                             unsigned int irq, Error 
>>>> **errp)
>>>>   {
>>>> diff --git a/include/hw/s390x/ap-bridge.h 
>>>> b/include/hw/s390x/ap-bridge.h
>>>> index 470e439a98..f4d838bf99 100644
>>>> --- a/include/hw/s390x/ap-bridge.h
>>>> +++ b/include/hw/s390x/ap-bridge.h
>>>> @@ -16,4 +16,26 @@
>>>>     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;
>>>> +
>>>> +int ap_chsc_sei_nt0_get_event(void *res);
>>>
>>> Documentation would be nice to have since a "return 1" means failure.
>>
>> True.
>>
> Since I want to keep the return as is, will this documentation suffice?
>
>
> /**
> * 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:
> * 0 - An event was available and written to @res
> * 1 - No event was available
> */
>>>
>>>> +
>>>> +int ap_chsc_sei_nt0_have_event(void);
>>>> +>   #endif
>>>
>>>
>>> Thanks,
>>>
>>> C.
>>>
>>>
>>

[-- Attachment #2: Type: text/html, Size: 9222 bytes --]