[PATCH v1 0/5] Report vfio-ap configuration changes

[PATCH v1 0/5] Report vfio-ap configuration changes

[Rorie Reyes]

This patch series creates and registers a handler that is called when
userspace is notified by the kernel that a guest's AP configuration has
changed. The handler in turn notifies the guest that its AP configuration
has changed. This allows the guest to immediately respond to AP
configuration changes rather than relying on polling or some other
inefficient mechanism for detecting config changes.

Rorie Reyes (5):
  linux-headers: NOTFORMERGE - placeholder uapi updates for AP config
    change
  hw/vfio/ap: notification handler for AP config changed event
  hw/vfio/ap: store object indicating AP config changed in a queue
  hw/vfio/ap: Storing event information for an AP configuration change
    event
  s390: implementing CHSC SEI for AP config change

 hw/vfio/ap.c                 | 74 ++++++++++++++++++++++++++++++++++++
 include/hw/s390x/ap-bridge.h | 17 +++++++++
 linux-headers/linux/vfio.h   |  1 +
 target/s390x/ioinst.c        | 11 +++++-
 4 files changed, 101 insertions(+), 2 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH v1 1/5] linux-headers: NOTFORMERGE - placeholder uapi updates for AP config change

[Rorie Reyes]

This patch adds enumeration constant VFIO_AP_CFG_CHG_IRQ_INDEX to specify
an IRQ index for signaling that a change has been made to the guest's AP
configuration. This is a placeholder for QEMU patches that use this value
since it is a linux-headers update which includes changes that aren't
merged into the kernel. Linux-headers patches should be generated using
scripts/update-linux-headers.sh.

Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
---
 linux-headers/linux/vfio.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index 1b5e254d6a..d0426b5ec0 100644
--- a/linux-headers/linux/vfio.h
+++ b/linux-headers/linux/vfio.h
@@ -671,6 +671,7 @@ enum {
  */
 enum {
 	VFIO_AP_REQ_IRQ_INDEX,
+	VFIO_AP_CFG_CHG_IRQ_INDEX,
 	VFIO_AP_NUM_IRQS
 };
 
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v1 1/5] linux-headers: NOTFORMERGE - placeholder uapi updates for AP config change

[Cédric Le Goater]

Hello Rorie,

On 1/7/25 19:43, Rorie Reyes wrote:
> This patch adds enumeration constant VFIO_AP_CFG_CHG_IRQ_INDEX to specify
> an IRQ index for signaling that a change has been made to the guest's AP
> configuration. This is a placeholder for QEMU patches that use this value
> since it is a linux-headers update which includes changes that aren't
> merged into the kernel.

Is there an upstream proposal for this change that we could look at ?
It is nice to mention related series in the cover letter.

Thanks,

C.



> Linux-headers patches should be generated using
> scripts/update-linux-headers.sh.
> 
> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
> ---
>   linux-headers/linux/vfio.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
> index 1b5e254d6a..d0426b5ec0 100644
> --- a/linux-headers/linux/vfio.h
> +++ b/linux-headers/linux/vfio.h
> @@ -671,6 +671,7 @@ enum {
>    */
>   enum {
>   	VFIO_AP_REQ_IRQ_INDEX,
> +	VFIO_AP_CFG_CHG_IRQ_INDEX,
>   	VFIO_AP_NUM_IRQS
>   };
>   

Re: [PATCH v1 1/5] linux-headers: NOTFORMERGE - placeholder uapi updates for AP config change

[Rorie Reyes]

On 1/8/25 2:29 AM, Cédric Le Goater wrote:
> Hello Rorie,
>
> On 1/7/25 19:43, Rorie Reyes wrote:
>> This patch adds enumeration constant VFIO_AP_CFG_CHG_IRQ_INDEX to 
>> specify
>> an IRQ index for signaling that a change has been made to the guest's AP
>> configuration. This is a placeholder for QEMU patches that use this 
>> value
>> since it is a linux-headers update which includes changes that aren't
>> merged into the kernel.
>
> Is there an upstream proposal for this change that we could look at ?
> It is nice to mention related series in the cover letter.
>
> Thanks,
>
> C.
>
>
Hey Cedric,

There is no upstream proposal for this. It is an s390 internal line item.

Thanks,

Rorie

Re: [PATCH v1 1/5] linux-headers: NOTFORMERGE - placeholder uapi updates for AP config change

[Eric Farman]

On Tue, 2025-01-14 at 13:51 -0500, Rorie Reyes wrote:
> On 1/8/25 2:29 AM, Cédric Le Goater wrote:
> > Hello Rorie,
> > 
> > On 1/7/25 19:43, Rorie Reyes wrote:
> > > This patch adds enumeration constant VFIO_AP_CFG_CHG_IRQ_INDEX to 
> > > specify
> > > an IRQ index for signaling that a change has been made to the guest's AP
> > > configuration. This is a placeholder for QEMU patches that use this 
> > > value
> > > since it is a linux-headers update which includes changes that aren't
> > > merged into the kernel.
> > 
> > Is there an upstream proposal for this change that we could look at ?
> > It is nice to mention related series in the cover letter.
> > 
> > Thanks,
> > 
> > C.
> > 
> > 
> Hey Cedric,
> 
> There is no upstream proposal for this. It is an s390 internal line item.

Rorie,

I think Cedric was looking for the matching kernel code you proposed, which this QEMU series depends
on:

https://lore.kernel.org/r/20250107183645.90082-1-rreyes@linux.ibm.com/

Thanks,
Eric

Re: [PATCH v1 1/5] linux-headers: NOTFORMERGE - placeholder uapi updates for AP config change

[Cédric Le Goater]

On 1/14/25 20:55, Eric Farman wrote:
> On Tue, 2025-01-14 at 13:51 -0500, Rorie Reyes wrote:
>> On 1/8/25 2:29 AM, Cédric Le Goater wrote:
>>> Hello Rorie,
>>>
>>> On 1/7/25 19:43, Rorie Reyes wrote:
>>>> This patch adds enumeration constant VFIO_AP_CFG_CHG_IRQ_INDEX to
>>>> specify
>>>> an IRQ index for signaling that a change has been made to the guest's AP
>>>> configuration. This is a placeholder for QEMU patches that use this
>>>> value
>>>> since it is a linux-headers update which includes changes that aren't
>>>> merged into the kernel.
>>>
>>> Is there an upstream proposal for this change that we could look at ?
>>> It is nice to mention related series in the cover letter.
>>>
>>> Thanks,
>>>
>>> C.
>>>
>>>
>> Hey Cedric,
>>
>> There is no upstream proposal for this. It is an s390 internal line item.
> 
> Rorie,
> 
> I think Cedric was looking for the matching kernel code you proposed, which this QEMU series depends
> on:
> 
> https://lore.kernel.org/r/20250107183645.90082-1-rreyes@linux.ibm.com/

There is a v2 in the works, right? is that 6.14 material ?

Thanks,

C.


> 
> Thanks,
> Eric
> 

Re: [PATCH v1 1/5] linux-headers: NOTFORMERGE - placeholder uapi updates for AP config change

[Eric Farman]

On Fri, 2025-01-24 at 09:49 +0100, Cédric Le Goater wrote:
> On 1/14/25 20:55, Eric Farman wrote:
> > On Tue, 2025-01-14 at 13:51 -0500, Rorie Reyes wrote:
> > > On 1/8/25 2:29 AM, Cédric Le Goater wrote:
> > > > Hello Rorie,
> > > > 
> > > > On 1/7/25 19:43, Rorie Reyes wrote:
> > > > > This patch adds enumeration constant VFIO_AP_CFG_CHG_IRQ_INDEX to
> > > > > specify
> > > > > an IRQ index for signaling that a change has been made to the guest's AP
> > > > > configuration. This is a placeholder for QEMU patches that use this
> > > > > value
> > > > > since it is a linux-headers update which includes changes that aren't
> > > > > merged into the kernel.
> > > > 
> > > > Is there an upstream proposal for this change that we could look at ?
> > > > It is nice to mention related series in the cover letter.
> > > > 
> > > > Thanks,
> > > > 
> > > > C.
> > > > 
> > > > 
> > > Hey Cedric,
> > > 
> > > There is no upstream proposal for this. It is an s390 internal line item.
> > 
> > Rorie,
> > 
> > I think Cedric was looking for the matching kernel code you proposed, which this QEMU series depends
> > on:
> > 
> > https://lore.kernel.org/r/20250107183645.90082-1-rreyes@linux.ibm.com/
> 
> There is a v2 in the works, right? is that 6.14 material ?

Yes, but will probably miss the current merge window. :(

Thanks,
Eric

[PATCH v1 2/5] hw/vfio/ap: notification handler for AP config changed event

[Rorie Reyes]

Register an event notifier handler to process AP configuration
change events by queuing the event and generating a CRW to let
the guest know its AP configuration has changed

Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
Tested-by: Anthony Krowiak <akrowiak@linux.ibm.com>
---
 hw/vfio/ap.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index 30b08ad375..533cadb2dd 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -18,6 +18,7 @@
 #include "hw/vfio/vfio-common.h"
 #include "system/iommufd.h"
 #include "hw/s390x/ap-device.h"
+#include "hw/s390x/css.h"
 #include "qemu/error-report.h"
 #include "qemu/event_notifier.h"
 #include "qemu/main-loop.h"
@@ -37,6 +38,7 @@ struct VFIOAPDevice {
     APDevice apdev;
     VFIODevice vdev;
     EventNotifier req_notifier;
+    EventNotifier cfg_notifier;
 };
 
 OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
@@ -70,6 +72,18 @@ static void vfio_ap_req_notifier_handler(void *opaque)
     }
 }
 
+static void vfio_ap_cfg_chg_notifier_handler(void *opaque)
+{
+    VFIOAPDevice *vapdev = opaque;
+
+    if (!event_notifier_test_and_clear(&vapdev->cfg_notifier)) {
+        warn_report("Event notifier not initialized");
+        return;
+    }
+
+    css_generate_css_crws(0);
+}
+
 static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
                                           unsigned int irq, Error **errp)
 {
@@ -85,6 +99,10 @@ static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
         notifier = &vapdev->req_notifier;
         fd_read = vfio_ap_req_notifier_handler;
         break;
+    case VFIO_AP_CFG_CHG_IRQ_INDEX:
+        notifier = &vapdev->cfg_notifier;
+        fd_read = vfio_ap_cfg_chg_notifier_handler;
+        break;
     default:
         error_setg(errp, "vfio: Unsupported device irq(%d)", irq);
         return false;
@@ -175,6 +193,15 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
         warn_report_err(err);
     }
 
+    if (!vfio_ap_register_irq_notifier(vapdev, VFIO_AP_CFG_CHG_IRQ_INDEX, &err))
+    {
+        /*
+         * Report this error, but do not make it a failing condition.
+         * Lack of this IRQ in the host does not prevent normal operation.
+         */
+        warn_report_err(err);
+    }
+
     return;
 
 error:
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v1 2/5] hw/vfio/ap: notification handler for AP config changed event

[Cédric Le Goater]

On 1/7/25 19:43, Rorie Reyes wrote:
> Register an event notifier handler to process AP configuration
> change events by queuing the event and generating a CRW to let
> the guest know its AP configuration has changed
> 
> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
> Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> Tested-by: Anthony Krowiak <akrowiak@linux.ibm.com>

I don't recall a previous version of these changes where these
trailers were added. Was this part of an internal review ?
It is best to restart the process when the patches become public.

Thanks,

C.



> ---
>   hw/vfio/ap.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 30b08ad375..533cadb2dd 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -18,6 +18,7 @@
>   #include "hw/vfio/vfio-common.h"
>   #include "system/iommufd.h"
>   #include "hw/s390x/ap-device.h"
> +#include "hw/s390x/css.h"
>   #include "qemu/error-report.h"
>   #include "qemu/event_notifier.h"
>   #include "qemu/main-loop.h"
> @@ -37,6 +38,7 @@ struct VFIOAPDevice {
>       APDevice apdev;
>       VFIODevice vdev;
>       EventNotifier req_notifier;
> +    EventNotifier cfg_notifier;
>   };
>   
>   OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
> @@ -70,6 +72,18 @@ static void vfio_ap_req_notifier_handler(void *opaque)
>       }
>   }
>   
> +static void vfio_ap_cfg_chg_notifier_handler(void *opaque)
> +{
> +    VFIOAPDevice *vapdev = opaque;
> +
> +    if (!event_notifier_test_and_clear(&vapdev->cfg_notifier)) {
> +        warn_report("Event notifier not initialized");
> +        return;
> +    }
> +
> +    css_generate_css_crws(0);
> +}
> +
>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>                                             unsigned int irq, Error **errp)
>   {
> @@ -85,6 +99,10 @@ static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>           notifier = &vapdev->req_notifier;
>           fd_read = vfio_ap_req_notifier_handler;
>           break;
> +    case VFIO_AP_CFG_CHG_IRQ_INDEX:
> +        notifier = &vapdev->cfg_notifier;
> +        fd_read = vfio_ap_cfg_chg_notifier_handler;
> +        break;
>       default:
>           error_setg(errp, "vfio: Unsupported device irq(%d)", irq);
>           return false;
> @@ -175,6 +193,15 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>           warn_report_err(err);
>       }
>   
> +    if (!vfio_ap_register_irq_notifier(vapdev, VFIO_AP_CFG_CHG_IRQ_INDEX, &err))
> +    {
> +        /*
> +         * Report this error, but do not make it a failing condition.
> +         * Lack of this IRQ in the host does not prevent normal operation.
> +         */
> +        warn_report_err(err);
> +    }
> +
>       return;
>   
>   error:

Re: [PATCH v1 2/5] hw/vfio/ap: notification handler for AP config changed event

[Rorie Reyes]

On 1/8/25 2:34 AM, Cédric Le Goater wrote:

> On 1/7/25 19:43, Rorie Reyes wrote:
>> Register an event notifier handler to process AP configuration
>> change events by queuing the event and generating a CRW to let
>> the guest know its AP configuration has changed
>>
>> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
>> Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
>> Tested-by: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> I don't recall a previous version of these changes where these
> trailers were added. Was this part of an internal review ?
> It is best to restart the process when the patches become public.
>
> Thanks,
>
> C.

Hey Cédric,

Yes these patches went through an internal review and Anthony Krowiak is

the primary maintainer for VFIO-AP. This is my first upstream review so

apologize for the confusion.

Thanks,

Rorie

Re: [PATCH v1 2/5] hw/vfio/ap: notification handler for AP config changed event

[Anthony Krowiak]

On 1/7/25 1:43 PM, Rorie Reyes wrote:
> Register an event notifier handler to process AP configuration
> change events by queuing the event and generating a CRW to let
> the guest know its AP configuration has changed
>
> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
> Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> Tested-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> ---
>   hw/vfio/ap.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
>
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 30b08ad375..533cadb2dd 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -18,6 +18,7 @@
>   #include "hw/vfio/vfio-common.h"
>   #include "system/iommufd.h"
>   #include "hw/s390x/ap-device.h"
> +#include "hw/s390x/css.h"
>   #include "qemu/error-report.h"
>   #include "qemu/event_notifier.h"
>   #include "qemu/main-loop.h"
> @@ -37,6 +38,7 @@ struct VFIOAPDevice {
>       APDevice apdev;
>       VFIODevice vdev;
>       EventNotifier req_notifier;
> +    EventNotifier cfg_notifier;
>   };
>   
>   OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
> @@ -70,6 +72,18 @@ static void vfio_ap_req_notifier_handler(void *opaque)
>       }
>   }
>   
> +static void vfio_ap_cfg_chg_notifier_handler(void *opaque)
> +{
> +    VFIOAPDevice *vapdev = opaque;
> +
> +    if (!event_notifier_test_and_clear(&vapdev->cfg_notifier)) {
> +        warn_report("Event notifier not initialized");
> +        return;
> +    }
> +
> +    css_generate_css_crws(0);
> +}
> +
>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>                                             unsigned int irq, Error **errp)
>   {
> @@ -85,6 +99,10 @@ static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>           notifier = &vapdev->req_notifier;
>           fd_read = vfio_ap_req_notifier_handler;
>           break;
> +    case VFIO_AP_CFG_CHG_IRQ_INDEX:
> +        notifier = &vapdev->cfg_notifier;
> +        fd_read = vfio_ap_cfg_chg_notifier_handler;
> +        break;
>       default:
>           error_setg(errp, "vfio: Unsupported device irq(%d)", irq);
>           return false;
> @@ -175,6 +193,15 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>           warn_report_err(err);
>       }

I missed this in my previous reviews; however, this needs a function to 
unregister the
VFIO_AP_CFG_CHG_IRQ_INDEX notifier.

>   
> +    if (!vfio_ap_register_irq_notifier(vapdev, VFIO_AP_CFG_CHG_IRQ_INDEX, &err))
> +    {
> +        /*
> +         * Report this error, but do not make it a failing condition.
> +         * Lack of this IRQ in the host does not prevent normal operation.
> +         */
> +        warn_report_err(err);
> +    }
> +
>       return;
>   
>   error:

Re: [PATCH v1 2/5] hw/vfio/ap: notification handler for AP config changed event

[Cédric Le Goater]

On 1/7/25 19:43, Rorie Reyes wrote:
> Register an event notifier handler to process AP configuration
> change events by queuing the event and generating a CRW to let
> the guest know its AP configuration has changed
> 
> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
> Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> Tested-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> ---
>   hw/vfio/ap.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 30b08ad375..533cadb2dd 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -18,6 +18,7 @@
>   #include "hw/vfio/vfio-common.h"
>   #include "system/iommufd.h"
>   #include "hw/s390x/ap-device.h"
> +#include "hw/s390x/css.h"
>   #include "qemu/error-report.h"
>   #include "qemu/event_notifier.h"
>   #include "qemu/main-loop.h"
> @@ -37,6 +38,7 @@ struct VFIOAPDevice {
>       APDevice apdev;
>       VFIODevice vdev;
>       EventNotifier req_notifier;
> +    EventNotifier cfg_notifier;
>   };
>   
>   OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
> @@ -70,6 +72,18 @@ static void vfio_ap_req_notifier_handler(void *opaque)
>       }
>   }
>   
> +static void vfio_ap_cfg_chg_notifier_handler(void *opaque)
> +{
> +    VFIOAPDevice *vapdev = opaque;
> +
> +    if (!event_notifier_test_and_clear(&vapdev->cfg_notifier)) {
> +        warn_report("Event notifier not initialized");

I don't think this warning is needed. May be reverse the logic :

     if (event_notifier_test_and_clear(&vapdev->cfg_notifier)) {
         css_generate_css_crws(0);
     }


Thanks,

C.



> +        return;
> +    }
> +
> +    css_generate_css_crws(0);
> +}
> +
>   static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>                                             unsigned int irq, Error **errp)
>   {
> @@ -85,6 +99,10 @@ static bool vfio_ap_register_irq_notifier(VFIOAPDevice *vapdev,
>           notifier = &vapdev->req_notifier;
>           fd_read = vfio_ap_req_notifier_handler;
>           break;
> +    case VFIO_AP_CFG_CHG_IRQ_INDEX:
> +        notifier = &vapdev->cfg_notifier;
> +        fd_read = vfio_ap_cfg_chg_notifier_handler;
> +        break;
>       default:
>           error_setg(errp, "vfio: Unsupported device irq(%d)", irq);
>           return false;
>
> @@ -175,6 +193,15 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>           warn_report_err(err);
>       }
>   
> +    if (!vfio_ap_register_irq_notifier(vapdev, VFIO_AP_CFG_CHG_IRQ_INDEX, &err))
> +    {
> +        /*
> +         * Report this error, but do not make it a failing condition.
> +         * Lack of this IRQ in the host does not prevent normal operation.
> +         */
> +        warn_report_err(err);
> +    }
> +
>       return;
>   
>   error:

[PATCH v1 3/5] hw/vfio/ap: store object indicating AP config changed in a queue

[Rorie Reyes]

Creates an object indicating that an AP configuration change event
has been received and stores it in a queue. These objects will later
be used to store event information for an AP configuration change
when the CHSC instruction is intercepted.

Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
Tested-by: Anthony Krowiak <akrowiak@linux.ibm.com>
---
 hw/vfio/ap.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index 533cadb2dd..508c6eed7a 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -41,6 +41,13 @@ struct VFIOAPDevice {
     EventNotifier cfg_notifier;
 };
 
+typedef struct APConfigChgEvent {
+    QTAILQ_ENTRY(APConfigChgEvent) next;
+} APConfigChgEvent;
+
+QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
+    QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
+
 OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
 
 static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
@@ -76,6 +83,9 @@ static void vfio_ap_cfg_chg_notifier_handler(void *opaque)
 {
     VFIOAPDevice *vapdev = opaque;
 
+    APConfigChgEvent *new_event = g_new0(APConfigChgEvent, 1);
+
+    QTAILQ_INSERT_TAIL(&cfg_chg_events, new_event, next);
     if (!event_notifier_test_and_clear(&vapdev->cfg_notifier)) {
         warn_report("Event notifier not initialized");
         return;
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v1 3/5] hw/vfio/ap: store object indicating AP config changed in a queue

[Cédric Le Goater]

On 1/7/25 19:43, Rorie Reyes wrote:
> Creates an object indicating that an AP configuration change event
> has been received and stores it in a queue. These objects will later
> be used to store event information for an AP configuration change
> when the CHSC instruction is intercepted.
> 
> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
> Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> Tested-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> ---
>   hw/vfio/ap.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 533cadb2dd..508c6eed7a 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -41,6 +41,13 @@ struct VFIOAPDevice {
>       EventNotifier cfg_notifier;
>   };
>   
> +typedef struct APConfigChgEvent {
> +    QTAILQ_ENTRY(APConfigChgEvent) next;
> +} APConfigChgEvent;
> +
> +QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
> +    QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
> +
>   OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
>   
>   static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
> @@ -76,6 +83,9 @@ static void vfio_ap_cfg_chg_notifier_handler(void *opaque)
>   {
>       VFIOAPDevice *vapdev = opaque;
> 

Extra white line ^

> +    APConfigChgEvent *new_event = g_new0(APConfigChgEvent, 1);

minor comment :

I would use the same variable name for APConfigChgEvent in this patch
and following. Easier to read. So, rename 'new_event' to 'cfg_chg_event'
or 'event'.

Thanks,

C.


> +
> +    QTAILQ_INSERT_TAIL(&cfg_chg_events, new_event, next);
>       if (!event_notifier_test_and_clear(&vapdev->cfg_notifier)) {
>           warn_report("Event notifier not initialized");
>           return;

Re: [PATCH v1 3/5] hw/vfio/ap: store object indicating AP config changed in a queue

[Anthony Krowiak]

On 1/7/25 1:43 PM, Rorie Reyes wrote:
> Creates an object indicating that an AP configuration change event
> has been received and stores it in a queue. These objects will later
> be used to store event information for an AP configuration change
> when the CHSC instruction is intercepted.
>
> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
> Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> Tested-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> ---
>   hw/vfio/ap.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 533cadb2dd..508c6eed7a 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -41,6 +41,13 @@ struct VFIOAPDevice {
>       EventNotifier cfg_notifier;
>   };
>   
> +typedef struct APConfigChgEvent {
> +    QTAILQ_ENTRY(APConfigChgEvent) next;
> +} APConfigChgEvent;
> +
> +QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
> +    QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
> +

The two chunks added above should got below the object declaration
below to keep it with the VFIOAPDevice structure to which it refers.

>   OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
>   
>   static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
> @@ -76,6 +83,9 @@ static void vfio_ap_cfg_chg_notifier_handler(void *opaque)
>   {
>       VFIOAPDevice *vapdev = opaque;
>   
> +    APConfigChgEvent *new_event = g_new0(APConfigChgEvent, 1);
> +
> +    QTAILQ_INSERT_TAIL(&cfg_chg_events, new_event, next);
>       if (!event_notifier_test_and_clear(&vapdev->cfg_notifier)) {
>           warn_report("Event notifier not initialized");
>           return;

Re: [PATCH v1 3/5] hw/vfio/ap: store object indicating AP config changed in a queue

[Anthony Krowiak]

On 1/7/25 1:43 PM, Rorie Reyes wrote:
> Creates an object indicating that an AP configuration change event
> has been received and stores it in a queue. These objects will later
> be used to store event information for an AP configuration change
> when the CHSC instruction is intercepted.
>
> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
> Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> Tested-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> ---
>   hw/vfio/ap.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 533cadb2dd..508c6eed7a 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -41,6 +41,13 @@ struct VFIOAPDevice {
>       EventNotifier cfg_notifier;
>   };
>   
> +typedef struct APConfigChgEvent {
> +    QTAILQ_ENTRY(APConfigChgEvent) next;
> +} APConfigChgEvent;
> +
> +QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
> +    QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
> +
>   OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
>   
>   static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
> @@ -76,6 +83,9 @@ static void vfio_ap_cfg_chg_notifier_handler(void *opaque)
>   {
>       VFIOAPDevice *vapdev = opaque;
>   
> +    APConfigChgEvent *new_event = g_new0(APConfigChgEvent, 1);
> +
> +    QTAILQ_INSERT_TAIL(&cfg_chg_events, new_event, next);

I didn't notice this before giving my r-b, but I don't think new_event 
should be
inserted into the queue until the 'event_notifier_test_and_clear' below
passes.

>       if (!event_notifier_test_and_clear(&vapdev->cfg_notifier)) {
>           warn_report("Event notifier not initialized");
>           return;

Re: [PATCH v1 3/5] hw/vfio/ap: store object indicating AP config changed in a queue

[Rorie Reyes]

On 2/3/25 11:50 AM, Anthony Krowiak wrote:
>
>
>
> On 1/7/25 1:43 PM, Rorie Reyes wrote:
>> Creates an object indicating that an AP configuration change event
>> has been received and stores it in a queue. These objects will later
>> be used to store event information for an AP configuration change
>> when the CHSC instruction is intercepted.
>>
>> Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
>> Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
>> Tested-by: Anthony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   hw/vfio/ap.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>> index 533cadb2dd..508c6eed7a 100644
>> --- a/hw/vfio/ap.c
>> +++ b/hw/vfio/ap.c
>> @@ -41,6 +41,13 @@ struct VFIOAPDevice {
>>       EventNotifier cfg_notifier;
>>   };
>>   +typedef struct APConfigChgEvent {
>> +    QTAILQ_ENTRY(APConfigChgEvent) next;
>> +} APConfigChgEvent;
>> +
>> +QTAILQ_HEAD(, APConfigChgEvent) cfg_chg_events =
>> +    QTAILQ_HEAD_INITIALIZER(cfg_chg_events);
>> +
>>   OBJECT_DECLARE_SIMPLE_TYPE(VFIOAPDevice, VFIO_AP_DEVICE)
>>     static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>> @@ -76,6 +83,9 @@ static void vfio_ap_cfg_chg_notifier_handler(void 
>> *opaque)
>>   {
>>       VFIOAPDevice *vapdev = opaque;
>>   +    APConfigChgEvent *new_event = g_new0(APConfigChgEvent, 1);
>> +
>> +    QTAILQ_INSERT_TAIL(&cfg_chg_events, new_event, next);
>
> I didn't notice this before giving my r-b, but I don't think new_event 
> should be
> inserted into the queue until the 'event_notifier_test_and_clear' below
> passes.


Hey Tony,

Can you give me a quick explanation for this suggestion?

>>       if (!event_notifier_test_and_clear(&vapdev->cfg_notifier)) {
>>           warn_report("Event notifier not initialized");
>>           return;
>

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

[Rorie Reyes]

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));
+    int rc = 1;
+
+    if (cfg_chg_event) {
+        QTAILQ_REMOVE(&cfg_chg_events, cfg_chg_event, next);
+        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 (!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;
+
+        rc = 0;
+    }
+
+    return rc;
+}
+
+int ap_chsc_sei_nt0_have_event(void)
+{
+    return !QTAILQ_EMPTY(&cfg_chg_events);
+}
+
 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
-- 
2.39.5 (Apple Git-154)

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

[Cédric Le Goater]

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

[PATCH v1 5/5] s390: implementing CHSC SEI for AP config change

[Rorie Reyes]

Handle interception of the CHSC SEI instruction for requests
indicating the guest's AP configuration has changed.

Signed-off-by: Rorie Reyes <rreyes@linux.ibm.com>
Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
Tested-by: Anthony Krowiak <akrowiak@linux.ibm.com>
---
 target/s390x/ioinst.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
index a944f16c25..f061c6db14 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -17,6 +17,7 @@
 #include "trace.h"
 #include "hw/s390x/s390-pci-bus.h"
 #include "target/s390x/kvm/pv.h"
+#include "hw/s390x/ap-bridge.h"
 
 /* All I/O instructions but chsc use the s format */
 static uint64_t get_address_from_regs(CPUS390XState *env, uint32_t ipb,
@@ -573,13 +574,19 @@ out:
 
 static int chsc_sei_nt0_get_event(void *res)
 {
-    /* no events yet */
+    if (s390_has_feat(S390_FEAT_AP)) {
+        return ap_chsc_sei_nt0_get_event(res);
+    }
+
     return 1;
 }
 
 static int chsc_sei_nt0_have_event(void)
 {
-    /* no events yet */
+    if (s390_has_feat(S390_FEAT_AP)) {
+        return ap_chsc_sei_nt0_have_event();
+    }
+
     return 0;
 }
 
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v1 0/5] Report vfio-ap configuration changes

[Alex Williamson]

On Tue,  7 Jan 2025 13:43:49 -0500
Rorie Reyes <rreyes@linux.ibm.com> wrote:

> This patch series creates and registers a handler that is called when
> userspace is notified by the kernel that a guest's AP configuration has
> changed. The handler in turn notifies the guest that its AP configuration
> has changed. This allows the guest to immediately respond to AP
> configuration changes rather than relying on polling or some other
> inefficient mechanism for detecting config changes.

Why are configuration changes to the device allowed while the device is
in use?

Would a uevent be considered an inefficient mechanism?  Why?

Thanks,
Alex

Re: [PATCH v1 0/5] Report vfio-ap configuration changes

[Rorie Reyes]

On 1/7/25 2:06 PM, Alex Williamson wrote:

> Why are configuration changes to the device allowed while the device is
> in use?
>
> Would a uevent be considered an inefficient mechanism?  Why?
>
> Thanks,
> Alex

I believe a vfio device is typically used to pass through a single I/O 
device, like a VGPU or PCI device. VFIO allows direct access to the 
memory of the underlying device to perform the I/O. Our vfio device does 
not follow that model. It represents a guest's AP configuration, not 
individual AP devices. Granting guest access to the AP devices in the 
configuration is controlled outside of VFIO. The purpose of the mediated 
device is to provide a means for specifying the AP configuration for the 
guest. When the mediated device is attached to the guest, the vfio_ap 
device driver sets the AP configuration assigned to the mdev into the 
control blocks used to start the guest.

We could use the sysfs attributes of the mediated device to do hot 
plug/unplug of devices for the guest. The vfio_ap host device driver 
would perform the hot plug/unplug via an IBM Z mechanism. It was left up 
to the AP bus driver running in the guest to detect these changes via a 
polling mechanism. My patches allow us to notify the AP bus driver 
immediately - via an eventfd to userspace - of those changes which is 
far more efficient than relying on a polling mechanism.

Re: [PATCH v1 0/5] Report vfio-ap configuration changes

[Rorie Reyes]

On 1/7/25 2:06 PM, Alex Williamson wrote:
> On Tue,  7 Jan 2025 13:43:49 -0500
> Rorie Reyes <rreyes@linux.ibm.com> wrote:
>
>> This patch series creates and registers a handler that is called when
>> userspace is notified by the kernel that a guest's AP configuration has
>> changed. The handler in turn notifies the guest that its AP configuration
>> has changed. This allows the guest to immediately respond to AP
>> configuration changes rather than relying on polling or some other
>> inefficient mechanism for detecting config changes.
> Why are configuration changes to the device allowed while the device is
> in use?
>
> Would a uevent be considered an inefficient mechanism?  Why?
>
> Thanks,
> Alex
>
Hey Alex,

Sorry for the long delay, but to answer your question, the VFIO device 
is typically used to pass through a single I/O

device, like a VGPU or PCI device. VFIO allows direct access to the 
memory of the underlying device to perofrm the I/O.

Our VFIO device does not follow that model and it represents a guest's 
AP configuration, not an individual AP device. Granting

guest access to AP devices in the configuration is controlled outside of 
the VFIO. The purpose of the mdev is to provide

a means for specifying the AP configuration for the guest. When the mdev 
is attached to the guest, the vfio_ap device driver

set the AP configuration assigned to the mdev into the control blocks 
used to start the guest.