qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] virtio-balloon: only create statsq when VIRTIO_BALLOON_F_STATS_VQ exists
@ 2025-12-11  9:05 Aaron Lo
  2025-12-11 13:14 ` Michael S. Tsirkin
  2025-12-11 14:22 ` Daniel P. Berrangé
  0 siblings, 2 replies; 5+ messages in thread
From: Aaron Lo @ 2025-12-11  9:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: Michael S. Tsirkin, David Hildenbrand, qemu-trivial, Aaron Lo

The VirtIO specification (section 5.5.2) states that the stats queue
is only present if the VIRTIO_BALLOON_F_STATS_VQ feature is
negotiated. QEMU currently creates the statsq unconditionally.

This patch guards statsq creation so it occurs only when the
feature bit is enabled.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3188

Signed-off-by: Aaron Lo <aaronlo0929@gmail.com>
---
 hw/virtio/virtio-balloon.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 02cdd807d7..f5d4d5f60c 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -892,7 +892,10 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
 
     s->ivq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
     s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
-    s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
+
+    if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_STATS_VQ)) {
+        s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
+    }
 
     if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
         s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
@@ -932,7 +935,9 @@ static void virtio_balloon_device_unrealize(DeviceState *dev)
 
     virtio_delete_queue(s->ivq);
     virtio_delete_queue(s->dvq);
-    virtio_delete_queue(s->svq);
+    if (s->svq) {
+        virtio_delete_queue(s->svq);
+    }
     if (s->free_page_vq) {
         virtio_delete_queue(s->free_page_vq);
     }

---
base-commit: 9c23f2a7b0b45277693a14074b1aaa827eecdb92
change-id: 20251211-balloon-check-stats-feature-7ea658e038ce

Best regards,
-- 
Aaron Lo <aaronlo0929@gmail.com>



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] virtio-balloon: only create statsq when VIRTIO_BALLOON_F_STATS_VQ exists
  2025-12-11  9:05 [PATCH] virtio-balloon: only create statsq when VIRTIO_BALLOON_F_STATS_VQ exists Aaron Lo
@ 2025-12-11 13:14 ` Michael S. Tsirkin
  2025-12-11 14:22 ` Daniel P. Berrangé
  1 sibling, 0 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2025-12-11 13:14 UTC (permalink / raw)
  To: Aaron Lo; +Cc: qemu-devel, David Hildenbrand, qemu-trivial

On Thu, Dec 11, 2025 at 03:05:49AM -0600, Aaron Lo wrote:
> The VirtIO specification (section 5.5.2) states that the stats queue
> is only present if the VIRTIO_BALLOON_F_STATS_VQ feature is
> negotiated. QEMU currently creates the statsq unconditionally.
> 
> This patch guards statsq creation so it occurs only when the
> feature bit is enabled.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3188
> 
> Signed-off-by: Aaron Lo <aaronlo0929@gmail.com>


did you test this with linux guests and with
free page hinting?



The issue is that sadly everyone is out of spec here.


We will have to fix the spec.


> ---
>  hw/virtio/virtio-balloon.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 02cdd807d7..f5d4d5f60c 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -892,7 +892,10 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>  
>      s->ivq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
>      s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
> -    s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
> +
> +    if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_STATS_VQ)) {
> +        s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
> +    }
>  
>      if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>          s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
> @@ -932,7 +935,9 @@ static void virtio_balloon_device_unrealize(DeviceState *dev)
>  
>      virtio_delete_queue(s->ivq);
>      virtio_delete_queue(s->dvq);
> -    virtio_delete_queue(s->svq);
> +    if (s->svq) {
> +        virtio_delete_queue(s->svq);
> +    }
>      if (s->free_page_vq) {
>          virtio_delete_queue(s->free_page_vq);
>      }
> 
> ---
> base-commit: 9c23f2a7b0b45277693a14074b1aaa827eecdb92
> change-id: 20251211-balloon-check-stats-feature-7ea658e038ce
> 
> Best regards,
> -- 
> Aaron Lo <aaronlo0929@gmail.com>



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] virtio-balloon: only create statsq when VIRTIO_BALLOON_F_STATS_VQ exists
  2025-12-11  9:05 [PATCH] virtio-balloon: only create statsq when VIRTIO_BALLOON_F_STATS_VQ exists Aaron Lo
  2025-12-11 13:14 ` Michael S. Tsirkin
@ 2025-12-11 14:22 ` Daniel P. Berrangé
  2025-12-11 20:15   ` Aaron Lo
  1 sibling, 1 reply; 5+ messages in thread
From: Daniel P. Berrangé @ 2025-12-11 14:22 UTC (permalink / raw)
  To: Aaron Lo; +Cc: qemu-devel, Michael S. Tsirkin, David Hildenbrand, qemu-trivial

On Thu, Dec 11, 2025 at 03:05:49AM -0600, Aaron Lo wrote:
> The VirtIO specification (section 5.5.2) states that the stats queue
> is only present if the VIRTIO_BALLOON_F_STATS_VQ feature is
> negotiated. QEMU currently creates the statsq unconditionally.
> 
> This patch guards statsq creation so it occurs only when the
> feature bit is enabled.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3188
> 
> Signed-off-by: Aaron Lo <aaronlo0929@gmail.com>
> ---
>  hw/virtio/virtio-balloon.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 02cdd807d7..f5d4d5f60c 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -892,7 +892,10 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>  
>      s->ivq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
>      s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
> -    s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
> +
> +    if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_STATS_VQ)) {
> +        s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
> +    }

This seems like a change that is liable to break live migration
state compatibility, as IIUC the queues are encoded in the state ?

>  
>      if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>          s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
> @@ -932,7 +935,9 @@ static void virtio_balloon_device_unrealize(DeviceState *dev)
>  
>      virtio_delete_queue(s->ivq);
>      virtio_delete_queue(s->dvq);
> -    virtio_delete_queue(s->svq);
> +    if (s->svq) {
> +        virtio_delete_queue(s->svq);
> +    }
>      if (s->free_page_vq) {
>          virtio_delete_queue(s->free_page_vq);
>      }
> 
> ---
> base-commit: 9c23f2a7b0b45277693a14074b1aaa827eecdb92
> change-id: 20251211-balloon-check-stats-feature-7ea658e038ce
> 
> Best regards,
> -- 
> Aaron Lo <aaronlo0929@gmail.com>
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] virtio-balloon: only create statsq when VIRTIO_BALLOON_F_STATS_VQ exists
  2025-12-11 14:22 ` Daniel P. Berrangé
@ 2025-12-11 20:15   ` Aaron Lo
  2025-12-11 22:18     ` Michael S. Tsirkin
  0 siblings, 1 reply; 5+ messages in thread
From: Aaron Lo @ 2025-12-11 20:15 UTC (permalink / raw)
  To: "Daniel P. Berrangé"
  Cc: qemu-devel, Michael S. Tsirkin, David Hildenbrand, qemu-trivial

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

Hi Daniel and Michael,

Thanks for the quick feedback.

Given the migration concerns and issues with the spec itself, I think it’ll be best to drop the code changes for now.

Thanks again for the guidance,
Aaron

> On Dec 11, 2025, at 8:22 AM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> On Thu, Dec 11, 2025 at 03:05:49AM -0600, Aaron Lo wrote:
>> The VirtIO specification (section 5.5.2) states that the stats queue
>> is only present if the VIRTIO_BALLOON_F_STATS_VQ feature is
>> negotiated. QEMU currently creates the statsq unconditionally.
>> 
>> This patch guards statsq creation so it occurs only when the
>> feature bit is enabled.
>> 
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3188
>> 
>> Signed-off-by: Aaron Lo <aaronlo0929@gmail.com>
>> ---
>> hw/virtio/virtio-balloon.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>> index 02cdd807d7..f5d4d5f60c 100644
>> --- a/hw/virtio/virtio-balloon.c
>> +++ b/hw/virtio/virtio-balloon.c
>> @@ -892,7 +892,10 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>> 
>>     s->ivq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
>>     s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
>> -    s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
>> +
>> +    if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_STATS_VQ)) {
>> +        s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
>> +    }
> 
> This seems like a change that is liable to break live migration
> state compatibility, as IIUC the queues are encoded in the state ?
> 
>> 
>>     if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>>         s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
>> @@ -932,7 +935,9 @@ static void virtio_balloon_device_unrealize(DeviceState *dev)
>> 
>>     virtio_delete_queue(s->ivq);
>>     virtio_delete_queue(s->dvq);
>> -    virtio_delete_queue(s->svq);
>> +    if (s->svq) {
>> +        virtio_delete_queue(s->svq);
>> +    }
>>     if (s->free_page_vq) {
>>         virtio_delete_queue(s->free_page_vq);
>>     }
>> 
>> ---
>> base-commit: 9c23f2a7b0b45277693a14074b1aaa827eecdb92
>> change-id: 20251211-balloon-check-stats-feature-7ea658e038ce
>> 
>> Best regards,
>> -- 
>> Aaron Lo <aaronlo0929@gmail.com>
>> 
>> 
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com <https://berrange.com/>      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org <https://libvirt.org/>         -o-            https://fstop138.berrange.com <https://fstop138.berrange.com/> :|
> |: https://entangle-photo.org <https://entangle-photo.org/>    -o-    https://www.instagram.com/dberrange :|


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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] virtio-balloon: only create statsq when VIRTIO_BALLOON_F_STATS_VQ exists
  2025-12-11 20:15   ` Aaron Lo
@ 2025-12-11 22:18     ` Michael S. Tsirkin
  0 siblings, 0 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2025-12-11 22:18 UTC (permalink / raw)
  To: Aaron Lo
  Cc: "Daniel P. Berrangé", qemu-devel, David Hildenbrand,
	qemu-trivial

On Thu, Dec 11, 2025 at 02:15:58PM -0600, Aaron Lo wrote:
> Hi Daniel and Michael,
> 
> Thanks for the quick feedback.
> 
> Given the migration concerns and issues with the spec itself, I think it’ll be
> best to drop the code changes for now.
> 
> Thanks again for the guidance,
> Aaron

You can do the spec changes though ;)

> 
>     On Dec 11, 2025, at 8:22 AM, Daniel P. Berrangé <berrange@redhat.com>
>     wrote:
> 
>     On Thu, Dec 11, 2025 at 03:05:49AM -0600, Aaron Lo wrote:
> 
>         The VirtIO specification (section 5.5.2) states that the stats queue
>         is only present if the VIRTIO_BALLOON_F_STATS_VQ feature is
>         negotiated. QEMU currently creates the statsq unconditionally.
> 
>         This patch guards statsq creation so it occurs only when the
>         feature bit is enabled.
> 
>         Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3188
> 
>         Signed-off-by: Aaron Lo <aaronlo0929@gmail.com>
>         ---
>         hw/virtio/virtio-balloon.c | 9 +++++++--
>         1 file changed, 7 insertions(+), 2 deletions(-)
> 
>         diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>         index 02cdd807d7..f5d4d5f60c 100644
>         --- a/hw/virtio/virtio-balloon.c
>         +++ b/hw/virtio/virtio-balloon.c
>         @@ -892,7 +892,10 @@ static void virtio_balloon_device_realize
>         (DeviceState *dev, Error **errp)
> 
>             s->ivq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
>             s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
>         -    s->svq = virtio_add_queue(vdev, 128,
>         virtio_balloon_receive_stats);
>         +
>         +    if (virtio_has_feature(s->host_features,
>         VIRTIO_BALLOON_F_STATS_VQ)) {
>         +        s->svq = virtio_add_queue(vdev, 128,
>         virtio_balloon_receive_stats);
>         +    }
> 
> 
>     This seems like a change that is liable to break live migration
>     state compatibility, as IIUC the queues are encoded in the state ?
> 
> 
> 
>             if (virtio_has_feature(s->host_features,
>         VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>                 s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
>         @@ -932,7 +935,9 @@ static void virtio_balloon_device_unrealize
>         (DeviceState *dev)
> 
>             virtio_delete_queue(s->ivq);
>             virtio_delete_queue(s->dvq);
>         -    virtio_delete_queue(s->svq);
>         +    if (s->svq) {
>         +        virtio_delete_queue(s->svq);
>         +    }
>             if (s->free_page_vq) {
>                 virtio_delete_queue(s->free_page_vq);
>             }
> 
>         ---
>         base-commit: 9c23f2a7b0b45277693a14074b1aaa827eecdb92
>         change-id: 20251211-balloon-check-stats-feature-7ea658e038ce
> 
>         Best regards,
>         -- 
>         Aaron Lo <aaronlo0929@gmail.com>
> 
> 
> 
> 
>     With regards,
>     Daniel
>     -- 
>     |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange
>      :|
>     |: https://libvirt.org         -o-            https://fstop138.berrange.com
>      :|
>     |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange
>      :|
> 
> 



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2025-12-11 22:19 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-11  9:05 [PATCH] virtio-balloon: only create statsq when VIRTIO_BALLOON_F_STATS_VQ exists Aaron Lo
2025-12-11 13:14 ` Michael S. Tsirkin
2025-12-11 14:22 ` Daniel P. Berrangé
2025-12-11 20:15   ` Aaron Lo
2025-12-11 22:18     ` Michael S. Tsirkin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).