[PATCH RFC] virtio-balloon: make it spec compliant

[PATCH RFC] virtio-balloon: make it spec compliant

[Michael S. Tsirkin]

Currently, if VIRTIO_BALLOON_F_FREE_PAGE_HINT is off but
VIRTIO_BALLOON_F_REPORTING is on, then the reporting vq
gets number 3 while spec says it's number 4.
It happens to work because the linux virtio pci driver
is *also* out of spec.

To fix:
1. add vq4 as per spec
2. to help out the buggy Linux driver, in the above configuration,
   also create vq3, and handle it exactly as we do vq4.

I think that some clever hack is doable to address the issue
for existing machine types (which would get it in user's hands
sooner), but I'm not 100% sure what, exactly.

This is a simpler, straight-forward approach.

Reported-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

I don't think I'll stop here, I want to fix exiting machine types,
but sending this here for comparison.
I'll send a Linux patch later.


 include/hw/virtio/virtio-balloon.h | 1 +
 hw/core/machine.c                  | 1 +
 hw/virtio/virtio-balloon.c         | 5 ++++-
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index 5139cf8ab6..d4426404ed 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -70,6 +70,7 @@ struct VirtIOBalloon {
     uint32_t host_features;
 
     bool qemu_4_0_config_size;
+    bool reporting_vq_is_vq3;
     uint32_t poison_val;
 };
 
diff --git a/hw/core/machine.c b/hw/core/machine.c
index f4cba6496c..353a143b2b 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -39,6 +39,7 @@ GlobalProperty hw_compat_9_0[] = {
     {"scsi-disk-base", "migrate-emulated-scsi-request", "false" },
     {"vfio-pci", "skip-vsc-check", "false" },
     { "virtio-pci", "x-pcie-pm-no-soft-reset", "off" },
+    { "virtio-balloon-device", "x-reporting-vq-is-vq3", "true" },
 };
 const size_t hw_compat_9_0_len = G_N_ELEMENTS(hw_compat_9_0);
 
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 50da34d6cc..4712bee521 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -892,7 +892,8 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
 
     if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
         /* Work around Linux driver which is buggy in this configuration */
-        if (!virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
+        if (!s->reporting_vq_is_vq3 &&
+            !virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
             s->free_page_vq = virtio_add_queue(vdev, 32,
                                                virtio_balloon_handle_report);
         }
@@ -1021,6 +1022,8 @@ static Property virtio_balloon_properties[] = {
      */
     DEFINE_PROP_BOOL("qemu-4-0-config-size", VirtIOBalloon,
                      qemu_4_0_config_size, false),
+    DEFINE_PROP_BOOL("x-reporting-vq-is-vq3", VirtIOBalloon,
+                     reporting_vq_is_vq3, false),
     DEFINE_PROP_LINK("iothread", VirtIOBalloon, iothread, TYPE_IOTHREAD,
                      IOThread *),
     DEFINE_PROP_END_OF_LIST(),
-- 
MST

Re: [PATCH RFC] virtio-balloon: make it spec compliant

[David Hildenbrand]

On 04.07.24 23:30, Michael S. Tsirkin wrote:
> Currently, if VIRTIO_BALLOON_F_FREE_PAGE_HINT is off but
> VIRTIO_BALLOON_F_REPORTING is on, then the reporting vq
> gets number 3 while spec says it's number 4.
> It happens to work because the linux virtio pci driver
> is *also* out of spec.
> 
> To fix:
> 1. add vq4 as per spec
> 2. to help out the buggy Linux driver, in the above configuration,
>     also create vq3, and handle it exactly as we do vq4.
> 
> I think that some clever hack is doable to address the issue
> for existing machine types (which would get it in user's hands
> sooner), but I'm not 100% sure what, exactly.
> 
> This is a simpler, straight-forward approach.
> 
> Reported-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> I don't think I'll stop here, I want to fix exiting machine types,
> but sending this here for comparison.
> I'll send a Linux patch later.

The downside is that new machine types will stop working with mainline 
Linux / major distros in that feature combination, right?

What's the approach that you are thinking of?

-- 
Cheers,

David / dhildenb

Re: [PATCH RFC] virtio-balloon: make it spec compliant

[Michael S. Tsirkin]

On Wed, Jul 10, 2024 at 05:15:18AM +0200, David Hildenbrand wrote:
> On 04.07.24 23:30, Michael S. Tsirkin wrote:
> > Currently, if VIRTIO_BALLOON_F_FREE_PAGE_HINT is off but
> > VIRTIO_BALLOON_F_REPORTING is on, then the reporting vq
> > gets number 3 while spec says it's number 4.
> > It happens to work because the linux virtio pci driver
> > is *also* out of spec.
> > 
> > To fix:
> > 1. add vq4 as per spec
> > 2. to help out the buggy Linux driver, in the above configuration,
> >     also create vq3, and handle it exactly as we do vq4.
> > 
> > I think that some clever hack is doable to address the issue
> > for existing machine types (which would get it in user's hands
> > sooner), but I'm not 100% sure what, exactly.
> > 
> > This is a simpler, straight-forward approach.
> > 
> > Reported-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > 
> > I don't think I'll stop here, I want to fix exiting machine types,
> > but sending this here for comparison.
> > I'll send a Linux patch later.
> 
> The downside is that new machine types will stop working with mainline Linux
> / major distros in that feature combination, right?

The should keep working, that's why I put in part 2. above.

> What's the approach that you are thinking of?
> 
> -- 
> Cheers,
> 
> David / dhildenb