[v1] Report vfio-ap configuration changes

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

Posted by Rorie Reyes 1 year, 1 month ago

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

Posted by Anthony Krowiak 1 year ago



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

Posted by Rorie Reyes 1 year ago

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

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

Posted by Anthony Krowiak 1 year ago



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

Posted by Cédric Le Goater 1 year ago

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;