diff mbox series

[v3,5/5] qapi: introduce CONFIG_READ event

Message ID 20240329183758.3360733-6-vsementsov@yandex-team.ru
State New
Headers show
Series vhost-user-blk: live resize additional APIs | expand

Commit Message

Vladimir Sementsov-Ogievskiy March 29, 2024, 6:37 p.m. UTC 
Send a new event when guest reads virtio-pci config after
virtio_notify_config() call.

That's useful to check that guest fetched modified config, for example
after resizing disk backend.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/virtio/virtio-pci.c |  9 +++++++++
 include/monitor/qdev.h |  2 ++
 monitor/monitor.c      |  1 +
 qapi/qdev.json         | 33 +++++++++++++++++++++++++++++++++
 stubs/qdev.c           |  6 ++++++
 system/qdev-monitor.c  |  6 ++++++
 6 files changed, 57 insertions(+)

Comments

Markus Armbruster April 24, 2024, 12:11 p.m. UTC | #1 
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> Send a new event when guest reads virtio-pci config after
> virtio_notify_config() call.
>
> That's useful to check that guest fetched modified config, for example
> after resizing disk backend.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  hw/virtio/virtio-pci.c |  9 +++++++++
>  include/monitor/qdev.h |  2 ++
>  monitor/monitor.c      |  1 +
>  qapi/qdev.json         | 33 +++++++++++++++++++++++++++++++++
>  stubs/qdev.c           |  6 ++++++
>  system/qdev-monitor.c  |  6 ++++++
>  6 files changed, 57 insertions(+)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 92afbae71c..c0c158dae2 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -23,6 +23,7 @@
>  #include "hw/boards.h"
>  #include "hw/virtio/virtio.h"
>  #include "migration/qemu-file-types.h"
> +#include "monitor/qdev.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pci_bus.h"
>  #include "hw/qdev-properties.h"
> @@ -530,6 +531,10 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
>      }
>      addr -= config;
>  
> +    if (vdev->generation > 0) {
> +        qdev_virtio_config_read_event(DEVICE(proxy));
> +    }
> +
>      switch (size) {
>      case 1:
>          val = virtio_config_readb(vdev, addr);
> @@ -1884,6 +1889,10 @@ static uint64_t virtio_pci_device_read(void *opaque, hwaddr addr,
>          return UINT64_MAX;
>      }
>  
> +    if (vdev->generation > 0) {
> +        qdev_virtio_config_read_event(DEVICE(proxy));
> +    }
> +
>      switch (size) {
>      case 1:
>          val = virtio_config_modern_readb(vdev, addr);
> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
> index 1d57bf6577..fc9a834dca 100644
> --- a/include/monitor/qdev.h
> +++ b/include/monitor/qdev.h
> @@ -36,4 +36,6 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>   */
>  const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
>  
> +void qdev_virtio_config_read_event(DeviceState *dev);
> +
>  #endif
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 01ede1babd..5b06146503 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -316,6 +316,7 @@ static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
>      [QAPI_EVENT_VSERPORT_CHANGE]   = { 1000 * SCALE_MS },
>      [QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE] = { 1000 * SCALE_MS },
>      [QAPI_EVENT_HV_BALLOON_STATUS_REPORT] = { 1000 * SCALE_MS },
> +    [QAPI_EVENT_VIRTIO_CONFIG_READ] = { 300 * SCALE_MS },

All the other rate-limited events use 1s.  Why 0.3s for this one?

>  };
>  
>  /*
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index e8be79c3d5..29a4f47360 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -182,3 +182,36 @@
>  { 'command': 'device-sync-config',
>    'features': [ 'unstable' ],
>    'data': {'id': 'str'} }
> +
> +##
> +# @VIRTIO_CONFIG_READ:
> +#
> +# Emitted whenever guest reads virtio device configuration after
> +# configuration change.

Is it emitted whenever the guest reads, or only when it reads after a
configuration change?

> +#
> +# The event may be used in pair with device-sync-config. It shows
> +# that guest has re-read updated configuration. It doesn't
> +# guarantee that guest successfully handled it and updated the
> +# view of the device for the user, but still it's a kind of
> +# success indicator.

The event is virtio-only.  device-sync-config isn't.  Why?

> +#
> +# @device: device name
> +#
> +# @path: device path
> +#
> +# Features:
> +#
> +# @unstable: The event is experimental.
> +#

Missing:

   # Note: This event is rate-limited.
   #

> +# Since: 9.1
> +#
> +# Example:
> +#
> +#     <- { "event": "VIRTIO_CONFIG_READ",
> +#          "data": { "device": "virtio-net-pci-0",
> +#                    "path": "/machine/peripheral/virtio-net-pci-0" },
> +#          "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> +##
> +{ 'event': 'VIRTIO_CONFIG_READ',
> +  'features': [ 'unstable' ],
> +  'data': { '*device': 'str', 'path': 'str' } }
> diff --git a/stubs/qdev.c b/stubs/qdev.c
> index 6869f6f90a..ab6c4afe0b 100644
> --- a/stubs/qdev.c
> +++ b/stubs/qdev.c
> @@ -26,3 +26,9 @@ void qapi_event_send_device_unplug_guest_error(const char *device,
>  {
>      /* Nothing to do. */
>  }
> +
> +void qapi_event_send_virtio_config_read(const char *device,
> +                                        const char *path)
> +{
> +    /* Nothing to do. */
> +}
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index cb35ea0b86..8a2ca77fde 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -26,6 +26,7 @@
>  #include "sysemu/runstate.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-qdev.h"
> +#include "qapi/qapi-events-qdev.h"
>  #include "qapi/qmp/dispatch.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qerror.h"
> @@ -1206,3 +1207,8 @@ bool qmp_command_available(const QmpCommand *cmd, Error **errp)
>      }
>      return true;
>  }
> +
> +void qdev_virtio_config_read_event(DeviceState *dev)
> +{
> +    qapi_event_send_virtio_config_read(dev->id, dev->canonical_path);
> +}

Which configuration needs the stub?
Vladimir Sementsov-Ogievskiy April 29, 2024, 9:02 a.m. UTC | #2 
On 24.04.24 15:11, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> Send a new event when guest reads virtio-pci config after
>> virtio_notify_config() call.
>>
>> That's useful to check that guest fetched modified config, for example
>> after resizing disk backend.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   hw/virtio/virtio-pci.c |  9 +++++++++
>>   include/monitor/qdev.h |  2 ++
>>   monitor/monitor.c      |  1 +
>>   qapi/qdev.json         | 33 +++++++++++++++++++++++++++++++++
>>   stubs/qdev.c           |  6 ++++++
>>   system/qdev-monitor.c  |  6 ++++++
>>   6 files changed, 57 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 92afbae71c..c0c158dae2 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -23,6 +23,7 @@
>>   #include "hw/boards.h"
>>   #include "hw/virtio/virtio.h"
>>   #include "migration/qemu-file-types.h"
>> +#include "monitor/qdev.h"
>>   #include "hw/pci/pci.h"
>>   #include "hw/pci/pci_bus.h"
>>   #include "hw/qdev-properties.h"
>> @@ -530,6 +531,10 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
>>       }
>>       addr -= config;
>>   
>> +    if (vdev->generation > 0) {
>> +        qdev_virtio_config_read_event(DEVICE(proxy));
>> +    }
>> +
>>       switch (size) {
>>       case 1:
>>           val = virtio_config_readb(vdev, addr);
>> @@ -1884,6 +1889,10 @@ static uint64_t virtio_pci_device_read(void *opaque, hwaddr addr,
>>           return UINT64_MAX;
>>       }
>>   
>> +    if (vdev->generation > 0) {
>> +        qdev_virtio_config_read_event(DEVICE(proxy));
>> +    }
>> +
>>       switch (size) {
>>       case 1:
>>           val = virtio_config_modern_readb(vdev, addr);
>> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
>> index 1d57bf6577..fc9a834dca 100644
>> --- a/include/monitor/qdev.h
>> +++ b/include/monitor/qdev.h
>> @@ -36,4 +36,6 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>>    */
>>   const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
>>   
>> +void qdev_virtio_config_read_event(DeviceState *dev);
>> +
>>   #endif
>> diff --git a/monitor/monitor.c b/monitor/monitor.c
>> index 01ede1babd..5b06146503 100644
>> --- a/monitor/monitor.c
>> +++ b/monitor/monitor.c
>> @@ -316,6 +316,7 @@ static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
>>       [QAPI_EVENT_VSERPORT_CHANGE]   = { 1000 * SCALE_MS },
>>       [QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE] = { 1000 * SCALE_MS },
>>       [QAPI_EVENT_HV_BALLOON_STATUS_REPORT] = { 1000 * SCALE_MS },
>> +    [QAPI_EVENT_VIRTIO_CONFIG_READ] = { 300 * SCALE_MS },
> 
> All the other rate-limited events use 1s.  Why 0.3s for this one?

No actual reason, just seemed to me that 1s is too much. Should be better to keep all limits to be the same, until no concrete reason to break it.

> 
>>   };
>>   
>>   /*
>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>> index e8be79c3d5..29a4f47360 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -182,3 +182,36 @@
>>   { 'command': 'device-sync-config',
>>     'features': [ 'unstable' ],
>>     'data': {'id': 'str'} }
>> +
>> +##
>> +# @VIRTIO_CONFIG_READ:
>> +#
>> +# Emitted whenever guest reads virtio device configuration after
>> +# configuration change.
> 
> Is it emitted whenever the guest reads, or only when it reads after a
> configuration change?

Hmm, it's emitted only when vdev->generation > 0, which generally mean that there was at least one call to virtio_notify_config()... That's not the logic, which could be simply described here.


Actually, now I think that event was a premature improvement. In our final downstream solution only the command device-sync-config is used, not the event. I see that the concept of the event is objectionable, I think, I'll better just drop it in v4.

> 
>> +#
>> +# The event may be used in pair with device-sync-config. It shows
>> +# that guest has re-read updated configuration. It doesn't
>> +# guarantee that guest successfully handled it and updated the
>> +# view of the device for the user, but still it's a kind of
>> +# success indicator.
> 
> The event is virtio-only.  device-sync-config isn't.  Why?
> 
>> +#
>> +# @device: device name
>> +#
>> +# @path: device path
>> +#
>> +# Features:
>> +#
>> +# @unstable: The event is experimental.
>> +#
> 
> Missing:
> 
>     # Note: This event is rate-limited.
>     #
> 
>> +# Since: 9.1
>> +#
>> +# Example:
>> +#
>> +#     <- { "event": "VIRTIO_CONFIG_READ",
>> +#          "data": { "device": "virtio-net-pci-0",
>> +#                    "path": "/machine/peripheral/virtio-net-pci-0" },
>> +#          "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>> +##
>> +{ 'event': 'VIRTIO_CONFIG_READ',
>> +  'features': [ 'unstable' ],
>> +  'data': { '*device': 'str', 'path': 'str' } }
>> diff --git a/stubs/qdev.c b/stubs/qdev.c
>> index 6869f6f90a..ab6c4afe0b 100644
>> --- a/stubs/qdev.c
>> +++ b/stubs/qdev.c
>> @@ -26,3 +26,9 @@ void qapi_event_send_device_unplug_guest_error(const char *device,
>>   {
>>       /* Nothing to do. */
>>   }
>> +
>> +void qapi_event_send_virtio_config_read(const char *device,
>> +                                        const char *path)
>> +{
>> +    /* Nothing to do. */
>> +}
>> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
>> index cb35ea0b86..8a2ca77fde 100644
>> --- a/system/qdev-monitor.c
>> +++ b/system/qdev-monitor.c
>> @@ -26,6 +26,7 @@
>>   #include "sysemu/runstate.h"
>>   #include "qapi/error.h"
>>   #include "qapi/qapi-commands-qdev.h"
>> +#include "qapi/qapi-events-qdev.h"
>>   #include "qapi/qmp/dispatch.h"
>>   #include "qapi/qmp/qdict.h"
>>   #include "qapi/qmp/qerror.h"
>> @@ -1206,3 +1207,8 @@ bool qmp_command_available(const QmpCommand *cmd, Error **errp)
>>       }
>>       return true;
>>   }
>> +
>> +void qdev_virtio_config_read_event(DeviceState *dev)
>> +{
>> +    qapi_event_send_virtio_config_read(dev->id, dev->canonical_path);
>> +}
> 
> Which configuration needs the stub?
>
diff mbox series

Patch

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 92afbae71c..c0c158dae2 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -23,6 +23,7 @@ 
 #include "hw/boards.h"
 #include "hw/virtio/virtio.h"
 #include "migration/qemu-file-types.h"
+#include "monitor/qdev.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_bus.h"
 #include "hw/qdev-properties.h"
@@ -530,6 +531,10 @@  static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
     }
     addr -= config;
 
+    if (vdev->generation > 0) {
+        qdev_virtio_config_read_event(DEVICE(proxy));
+    }
+
     switch (size) {
     case 1:
         val = virtio_config_readb(vdev, addr);
@@ -1884,6 +1889,10 @@  static uint64_t virtio_pci_device_read(void *opaque, hwaddr addr,
         return UINT64_MAX;
     }
 
+    if (vdev->generation > 0) {
+        qdev_virtio_config_read_event(DEVICE(proxy));
+    }
+
     switch (size) {
     case 1:
         val = virtio_config_modern_readb(vdev, addr);
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 1d57bf6577..fc9a834dca 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -36,4 +36,6 @@  DeviceState *qdev_device_add_from_qdict(const QDict *opts,
  */
 const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
 
+void qdev_virtio_config_read_event(DeviceState *dev);
+
 #endif
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 01ede1babd..5b06146503 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -316,6 +316,7 @@  static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
     [QAPI_EVENT_VSERPORT_CHANGE]   = { 1000 * SCALE_MS },
     [QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE] = { 1000 * SCALE_MS },
     [QAPI_EVENT_HV_BALLOON_STATUS_REPORT] = { 1000 * SCALE_MS },
+    [QAPI_EVENT_VIRTIO_CONFIG_READ] = { 300 * SCALE_MS },
 };
 
 /*
diff --git a/qapi/qdev.json b/qapi/qdev.json
index e8be79c3d5..29a4f47360 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -182,3 +182,36 @@ 
 { 'command': 'device-sync-config',
   'features': [ 'unstable' ],
   'data': {'id': 'str'} }
+
+##
+# @VIRTIO_CONFIG_READ:
+#
+# Emitted whenever guest reads virtio device configuration after
+# configuration change.
+#
+# The event may be used in pair with device-sync-config. It shows
+# that guest has re-read updated configuration. It doesn't
+# guarantee that guest successfully handled it and updated the
+# view of the device for the user, but still it's a kind of
+# success indicator.
+#
+# @device: device name
+#
+# @path: device path
+#
+# Features:
+#
+# @unstable: The event is experimental.
+#
+# Since: 9.1
+#
+# Example:
+#
+#     <- { "event": "VIRTIO_CONFIG_READ",
+#          "data": { "device": "virtio-net-pci-0",
+#                    "path": "/machine/peripheral/virtio-net-pci-0" },
+#          "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
+##
+{ 'event': 'VIRTIO_CONFIG_READ',
+  'features': [ 'unstable' ],
+  'data': { '*device': 'str', 'path': 'str' } }
diff --git a/stubs/qdev.c b/stubs/qdev.c
index 6869f6f90a..ab6c4afe0b 100644
--- a/stubs/qdev.c
+++ b/stubs/qdev.c
@@ -26,3 +26,9 @@  void qapi_event_send_device_unplug_guest_error(const char *device,
 {
     /* Nothing to do. */
 }
+
+void qapi_event_send_virtio_config_read(const char *device,
+                                        const char *path)
+{
+    /* Nothing to do. */
+}
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index cb35ea0b86..8a2ca77fde 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -26,6 +26,7 @@ 
 #include "sysemu/runstate.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-qdev.h"
+#include "qapi/qapi-events-qdev.h"
 #include "qapi/qmp/dispatch.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
@@ -1206,3 +1207,8 @@  bool qmp_command_available(const QmpCommand *cmd, Error **errp)
     }
     return true;
 }
+
+void qdev_virtio_config_read_event(DeviceState *dev)
+{
+    qapi_event_send_virtio_config_read(dev->id, dev->canonical_path);
+}