Re: [PATCH v1 4/5] hw/vfio/ap: Storing event information for an AP configuration change event

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

On 1/7/25 19:43, 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                 | 37 ++++++++++++++++++++++++++++++++++++
>   include/hw/s390x/ap-bridge.h | 17 +++++++++++++++++
>   2 files changed, 54 insertions(+)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 508c6eed7a..9d1e18f100 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -94,6 +94,43 @@ static void vfio_ap_cfg_chg_notifier_handler(void *opaque)
>       css_generate_css_crws(0);
>   }
>   
> +int ap_chsc_sei_nt0_get_event(void *res)
> +{
> +    APConfigChgEvent *cfg_chg_event = QTAILQ_FIRST(&cfg_chg_events);
> +    ChscSeiNt0Res *nt0_res  = (ChscSeiNt0Res *)res;
> +    memset(nt0_res, 0, sizeof(*nt0_res));

please put the memset after the variables.

> +    int rc = 1;

'rc' is not very useful. Why not simply return 0 or 1 ?

> +
> +    if (cfg_chg_event) {

I would reverse the logic and return early.

      if (! cfg_chg_event) {
          return 1;
      }


> +        QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
> +        free(cfg_chg_event);

g_free()

> +
> +        /*
> +         * 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 (!QTAILQ_EMPTY(&cfg_chg_events)) {
> +            nt0_res->flags |= PENDING_EVENT_INFO_BITMASK;
> +        }
> +
> +        nt0_res->length = sizeof(ChscSeiNt0Res);
> +        nt0_res->code = 1;
> +        nt0_res->nt = 0;
> +        nt0_res->rs = 5;
> +        nt0_res->cc = 3;

The above values are cryptic. Can we have some define possibly ?

> +        rc = 0;
> +    }
> +
> +    return rc;
> +}
> +
> +int ap_chsc_sei_nt0_have_event(void)
> +{
> +    return !QTAILQ_EMPTY(&cfg_chg_events);
> +}

Why not use this routine in ap_chsc_sei_nt0_get_event() too ?



Thanks,

C.


>   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..c9beec3db4 100644
> --- a/include/hw/s390x/ap-bridge.h
> +++ b/include/hw/s390x/ap-bridge.h
> @@ -16,4 +16,21 @@
>   
>   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;
> +
> +int ap_chsc_sei_nt0_get_event(void *res);
> +
> +int ap_chsc_sei_nt0_have_event(void);
> +
>   #endif