virtio-balloon: make it spec compliant

QEMU implemented the configuration

	VIRTIO_BALLOON_F_REPORTING && ! VIRTIO_BALLOON_F_FREE_PAGE_HINT

incorrectly: it then uses vq3 for reporting, spec says it is always 4.

This is masked by a corresponding bug in driver:
add a work around as I'm going to try and fix the driver bug.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
  drivers/virtio/virtio_balloon.c | 19 +++++++++++++++++--
  1 file changed, 17 insertions(+), 2 deletions(-)

drivers/virtio/virtio_balloon.c +10 -2

index 9a61febbd2f7..7dc3fcd56238 100644
@@ -597,8 +597,23 @@ static int init_vqs(struct virtio_balloon *vb)
 
  	err = virtio_find_vqs(vb->vdev, VIRTIO_BALLOON_VQ_MAX, vqs,
  			      callbacks, names, NULL);
-	if (err)
-		return err;
+	if (err) {
+		/*
+		 * Try to work around QEMU bug which since 2020 confused vq numbers
+		 * when VIRTIO_BALLOON_F_REPORTING but not
+		 * VIRTIO_BALLOON_F_FREE_PAGE_HINT are offered.
+		 */
+		if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING) &&
+		    !virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
+			names[VIRTIO_BALLOON_VQ_FREE_PAGE] = "reporting_vq";
+			callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = balloon_ack;

QEMU implemented the configuration

        VIRTIO_BALLOON_F_REPORTING && ! VIRTIO_BALLOON_F_FREE_PAGE_HINT

incorrectly: it then uses vq3 for reporting, spec says it is always 4.

This is masked by a corresponding bug in driver:
add a work around as I'm going to try and fix the driver bug.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/virtio/virtio_balloon.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

drivers/virtio/virtio_balloon.c +17 -3

index 9a61febbd2f7..7dc3fcd56238 100644
@@ -597,8 +597,23 @@ static int init_vqs(struct virtio_balloon *vb)
 
        err = virtio_find_vqs(vb->vdev, VIRTIO_BALLOON_VQ_MAX, vqs,
                              callbacks, names, NULL);
-       if (err)
-               return err;
+       if (err) {
+               /*
+                * Try to work around QEMU bug which since 2020 confused vq numbers
+                * when VIRTIO_BALLOON_F_REPORTING but not
+                * VIRTIO_BALLOON_F_FREE_PAGE_HINT are offered.
+                */
+               if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING) &&
+                   !virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
+                       names[VIRTIO_BALLOON_VQ_FREE_PAGE] = "reporting_vq";
+                       callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = balloon_ack;
+                       err = virtio_find_vqs(vb->vdev,
+                                             VIRTIO_BALLOON_VQ_REPORTING, vqs, callbacks, names, NULL);
+               }
+
+               if (err)
+                       return err;
+       }
 
        vb->inflate_vq = vqs[VIRTIO_BALLOON_VQ_INFLATE];
        vb->deflate_vq = vqs[VIRTIO_BALLOON_VQ_DEFLATE];
--

MST

Quoted 44 lines · Michael S. Tsirkin

QEMU implemented the configuration

        VIRTIO_BALLOON_F_REPORTING && ! VIRTIO_BALLOON_F_FREE_PAGE_HINT

incorrectly: it then uses vq3 for reporting, spec says it is always 4.

This is masked by a corresponding bug in driver:
add a work around as I'm going to try and fix the driver bug.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/virtio/virtio_balloon.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

drivers/virtio/virtio_balloon.c +17 -3

index 9a61febbd2f7..7dc3fcd56238 100644
@@ -597,8 +597,23 @@ static int init_vqs(struct virtio_balloon *vb)
 
        err = virtio_find_vqs(vb->vdev, VIRTIO_BALLOON_VQ_MAX, vqs,
                              callbacks, names, NULL);
-       if (err)
-               return err;
+       if (err) {
+               /*
+                * Try to work around QEMU bug which since 2020 confused vq numbers
+                * when VIRTIO_BALLOON_F_REPORTING but not
+                * VIRTIO_BALLOON_F_FREE_PAGE_HINT are offered.
+                */
+               if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING) &&
+                   !virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
+                       names[VIRTIO_BALLOON_VQ_FREE_PAGE] = "reporting_vq";
+                       callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = balloon_ack;
+                       err = virtio_find_vqs(vb->vdev,
+                                             VIRTIO_BALLOON_VQ_REPORTING, vqs, callbacks, names, NULL);
+               }
+
+               if (err)
+                       return err;
+       }
 
        vb->inflate_vq = vqs[VIRTIO_BALLOON_VQ_INFLATE];
        vb->deflate_vq = vqs[VIRTIO_BALLOON_VQ_DEFLATE];
--

Quoted 2 lines

MST

Acked-by Jason Wang <[email protected]>

Do we need a spec to say this is something that needs to be considered by the driver?

Thanks

Quoted 50 lines · Jason Wang

Quoted 44 lines · Michael S. Tsirkin

QEMU implemented the configuration

        VIRTIO_BALLOON_F_REPORTING && ! VIRTIO_BALLOON_F_FREE_PAGE_HINT

incorrectly: it then uses vq3 for reporting, spec says it is always 4.

This is masked by a corresponding bug in driver:
add a work around as I'm going to try and fix the driver bug.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/virtio/virtio_balloon.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

drivers/virtio/virtio_balloon.c +17 -3

index 9a61febbd2f7..7dc3fcd56238 100644
@@ -597,8 +597,23 @@ static int init_vqs(struct virtio_balloon *vb)
 
        err = virtio_find_vqs(vb->vdev, VIRTIO_BALLOON_VQ_MAX, vqs,
                              callbacks, names, NULL);
-       if (err)
-               return err;
+       if (err) {
+               /*
+                * Try to work around QEMU bug which since 2020 confused vq numbers
+                * when VIRTIO_BALLOON_F_REPORTING but not
+                * VIRTIO_BALLOON_F_FREE_PAGE_HINT are offered.
+                */
+               if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING) &&
+                   !virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
+                       names[VIRTIO_BALLOON_VQ_FREE_PAGE] = "reporting_vq";
+                       callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = balloon_ack;
+                       err = virtio_find_vqs(vb->vdev,
+                                             VIRTIO_BALLOON_VQ_REPORTING, vqs, callbacks, names, NULL);
+               }
+
+               if (err)
+                       return err;
+       }
 
        vb->inflate_vq = vqs[VIRTIO_BALLOON_VQ_INFLATE];
        vb->deflate_vq = vqs[VIRTIO_BALLOON_VQ_DEFLATE];
--

Quoted 2 lines

MST

Acked-by Jason Wang <[email protected]>

Quoted 4 lines

Do we need a spec to say this is something that needs to be considered by the driver?

Thanks

I'd say it's a temporary situation that we won't need to bother about in several years.

--
MST

Quoted 57 lines · Michael S. Tsirkin

Quoted 54 lines · Jason Wang

Quoted 44 lines · Michael S. Tsirkin

QEMU implemented the configuration

        VIRTIO_BALLOON_F_REPORTING && ! VIRTIO_BALLOON_F_FREE_PAGE_HINT

incorrectly: it then uses vq3 for reporting, spec says it is always 4.

This is masked by a corresponding bug in driver:
add a work around as I'm going to try and fix the driver bug.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/virtio/virtio_balloon.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

drivers/virtio/virtio_balloon.c +17 -3

index 9a61febbd2f7..7dc3fcd56238 100644
@@ -597,8 +597,23 @@ static int init_vqs(struct virtio_balloon *vb)
 
        err = virtio_find_vqs(vb->vdev, VIRTIO_BALLOON_VQ_MAX, vqs,
                              callbacks, names, NULL);
-       if (err)
-               return err;
+       if (err) {
+               /*
+                * Try to work around QEMU bug which since 2020 confused vq numbers
+                * when VIRTIO_BALLOON_F_REPORTING but not
+                * VIRTIO_BALLOON_F_FREE_PAGE_HINT are offered.
+                */
+               if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING) &&
+                   !virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
+                       names[VIRTIO_BALLOON_VQ_FREE_PAGE] = "reporting_vq";
+                       callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = balloon_ack;
+                       err = virtio_find_vqs(vb->vdev,
+                                             VIRTIO_BALLOON_VQ_REPORTING, vqs, callbacks, names, NULL);
+               }
+
+               if (err)
+                       return err;
+       }
 
        vb->inflate_vq = vqs[VIRTIO_BALLOON_VQ_INFLATE];
        vb->deflate_vq = vqs[VIRTIO_BALLOON_VQ_DEFLATE];
--

Quoted 2 lines

MST

Acked-by Jason Wang <[email protected]>

Do we need a spec to say this is something that needs to be considered by the driver?

Thanks

Quoted 2 lines

I'd say it's a temporary situation that we won't need to bother about in several years.

I mean, should a newly-written virtio-balloon driver care about this? If not, it means it can't work for several Qemu versions.

Thanks

Quoted 4 lines

--
MST

virtio balloon communicates to the core that in some configurations vq #s are non-contiguous by setting name pointer to NULL.

Unfortunately, core then turned around and just made them contiguous again. Result is that driver is out of spec.

Implement what the API was supposed to do
in the 1st place. Compatibility with buggy hypervisors is handled inside virtio-balloon, which is the only driver making use of this facility, so far.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

virtio balloon communicates to the core that in some configurations vq #s are non-contiguous by setting name pointer to NULL.

Unfortunately, core then turned around and just made them contiguous again. Result is that driver is out of spec.

Implement what the API was supposed to do
in the 1st place. Compatibility with buggy hypervisors is handled inside virtio-balloon, which is the only driver making use of this facility, so far.

Signed-off-by Michael S. Tsirkin <[email protected]>

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 qemu virtio pci driver is *also* out of spec.

Quoted 5 lines · 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 qemu virtio pci driver is *also* out of spec.

I have to ask the obvious: maybe the spec is wrong and we have to refine that?

Quoted 7 lines · David Hildenbrand

Quoted 5 lines · 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 qemu virtio pci driver is *also* out of spec.

Quoted 2 lines

I have to ask the obvious: maybe the spec is wrong and we have to refine that?

Well having vq function shift depending on features is certainly messy ...

How do we know no one implemented the spec as written though?

Quoted 8 lines · Michael S. Tsirkin

Quoted 7 lines · David Hildenbrand

Quoted 5 lines · 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 qemu virtio pci driver is *also* out of spec.

Quoted 5 lines

Quoted 2 lines

I have to ask the obvious: maybe the spec is wrong and we have to refine that?

Well having vq function shift depending on features is certainly messy ...

Right, but that's how all of this started from the beginning.

Quoted 1 line

How do we know no one implemented the spec as written though?

I understand that concern, IIUC it would imply that:

a) In case of a hypervisor, we never ran with a Linux guest
b) In case of a guest, we never ran under QEMU

It's certainly possible, although I would assume that most other implementation candidates (e.g., cloud-hypervisor) would have complained by now about Linux issues.

What's your experience: if someone would actually implement it according to the spec, would they watch out on the virtio mailing lists for changes (or even be able to vote) and would be able to comment that adjusting the spec to the real first implementation is wrong?

Quoted 4 lines

I understand that concern, IIUC it would imply that:

a) In case of a hypervisor, we never ran with a Linux guest
b) In case of a guest, we never ran under QEMU

Or maybe VIRTIO_BALLOON_F_FREE_PAGE_HINT is set.

Quoted 3 lines

It's certainly possible, although I would assume that most other implementation candidates (e.g., cloud-hypervisor) would have complained by now about Linux issues.

They either set VIRTIO_BALLOON_F_FREE_PAGE_HINT or followed linux bug to work around.

Quoted 4 lines

What's your experience: if someone would actually implement it according to the spec, would they watch out on the virtio mailing lists for changes (or even be able to vote) and would be able to comment that adjusting the spec to the real first implementation is wrong?

Unfortunately my experience is that it's not that likely :(

Whatever we do, we need to take existing setups into account.

How would we do it in the spec without breaking working setups? I guess we could say that both behaviours are legal. That would still mean we need the qemu and linux patches, right?