[PATCH 0/2] virtio-balloon: make it spec compliant

[PATCH 0/2] 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 qemu virtio pci driver
is *also* out of spec.

To fix:
1. add vq4 as per spec
2. to help out the buggy qemu driver, if finding vqs fail,
try with vq3 as reporting.

Fixes: b0c504f15471 ("virtio-balloon: add support for providing free page reports to host")
Cc: "Alexander Duyck" <alexander.h.duyck@linux.intel.com>
Reported-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Michael S. Tsirkin (2):
  virtio_balloon: add work around for out of spec QEMU
  virtio: fix vq # for balloon

 arch/um/drivers/virtio_uml.c           |  4 ++--
 drivers/remoteproc/remoteproc_virtio.c |  4 ++--
 drivers/s390/virtio/virtio_ccw.c       |  4 ++--
 drivers/virtio/virtio_balloon.c        | 19 +++++++++++++++++--
 drivers/virtio/virtio_mmio.c           |  4 ++--
 drivers/virtio/virtio_pci_common.c     |  8 ++++----
 drivers/virtio/virtio_vdpa.c           |  4 ++--
 7 files changed, 31 insertions(+), 16 deletions(-)

-- 
MST

[PATCH 1/2] virtio_balloon: add work around for out of spec QEMU

[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 <mst@redhat.com>
---
 drivers/virtio/virtio_balloon.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 9a61febbd2f7..7dc3fcd56238 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -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

[PATCH 2/2] virtio: fix vq # when vq skipped

[Michael S. Tsirkin]

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 <mst@redhat.com>
---
 arch/um/drivers/virtio_uml.c           | 4 ++--
 drivers/remoteproc/remoteproc_virtio.c | 4 ++--
 drivers/s390/virtio/virtio_ccw.c       | 4 ++--
 drivers/virtio/virtio_mmio.c           | 4 ++--
 drivers/virtio/virtio_pci_common.c     | 8 ++++----
 drivers/virtio/virtio_vdpa.c           | 4 ++--
 6 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/arch/um/drivers/virtio_uml.c b/arch/um/drivers/virtio_uml.c
index 77faa2cf3a13..d65346cd340e 100644
--- a/arch/um/drivers/virtio_uml.c
+++ b/arch/um/drivers/virtio_uml.c
@@ -1019,7 +1019,7 @@ static int vu_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 		       struct irq_affinity *desc)
 {
 	struct virtio_uml_device *vu_dev = to_virtio_uml_device(vdev);
-	int i, queue_idx = 0, rc;
+	int i, rc;
 	struct virtqueue *vq;
 
 	/* not supported for now */
@@ -1036,7 +1036,7 @@ static int vu_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 			continue;
 		}
 
-		vqs[i] = vu_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
+		vqs[i] = vu_setup_vq(vdev, i, callbacks[i], names[i],
 				     ctx ? ctx[i] : false);
 		if (IS_ERR(vqs[i])) {
 			rc = PTR_ERR(vqs[i]);
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 25b66b113b69..2d17135abb66 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -187,7 +187,7 @@ static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
 				 const bool * ctx,
 				 struct irq_affinity *desc)
 {
-	int i, ret, queue_idx = 0;
+	int i, ret;
 
 	for (i = 0; i < nvqs; ++i) {
 		if (!names[i]) {
@@ -195,7 +195,7 @@ static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
 			continue;
 		}
 
-		vqs[i] = rp_find_vq(vdev, queue_idx++, callbacks[i], names[i],
+		vqs[i] = rp_find_vq(vdev, i, callbacks[i], names[i],
 				    ctx ? ctx[i] : false);
 		if (IS_ERR(vqs[i])) {
 			ret = PTR_ERR(vqs[i]);
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index d6491fc84e8c..64541b3bb8a2 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -696,7 +696,7 @@ static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 {
 	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
 	dma64_t *indicatorp = NULL;
-	int ret, i, queue_idx = 0;
+	int ret, i;
 	struct ccw1 *ccw;
 	dma32_t indicatorp_dma = 0;
 
@@ -710,7 +710,7 @@ static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 			continue;
 		}
 
-		vqs[i] = virtio_ccw_setup_vq(vdev, queue_idx++, callbacks[i],
+		vqs[i] = virtio_ccw_setup_vq(vdev, i, callbacks[i],
 					     names[i], ctx ? ctx[i] : false,
 					     ccw);
 		if (IS_ERR(vqs[i])) {
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 173596589c71..a3a66a0b7cb1 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -496,7 +496,7 @@ static int vm_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
 {
 	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
 	int irq = platform_get_irq(vm_dev->pdev, 0);
-	int i, err, queue_idx = 0;
+	int i, err;
 
 	if (irq < 0)
 		return irq;
@@ -515,7 +515,7 @@ static int vm_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
 			continue;
 		}
 
-		vqs[i] = vm_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
+		vqs[i] = vm_setup_vq(vdev, i, callbacks[i], names[i],
 				     ctx ? ctx[i] : false);
 		if (IS_ERR(vqs[i])) {
 			vm_del_vqs(vdev);
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index f6b0b00e4599..eeff060cacec 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -292,7 +292,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	u16 msix_vec;
-	int i, err, nvectors, allocated_vectors, queue_idx = 0;
+	int i, err, nvectors, allocated_vectors;
 
 	vp_dev->vqs = kcalloc(nvqs, sizeof(*vp_dev->vqs), GFP_KERNEL);
 	if (!vp_dev->vqs)
@@ -328,7 +328,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
 			msix_vec = allocated_vectors++;
 		else
 			msix_vec = VP_MSIX_VQ_VECTOR;
-		vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
+		vqs[i] = vp_setup_vq(vdev, i, callbacks[i], names[i],
 				     ctx ? ctx[i] : false,
 				     msix_vec);
 		if (IS_ERR(vqs[i])) {
@@ -365,7 +365,7 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned int nvqs,
 		const char * const names[], const bool *ctx)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	int i, err, queue_idx = 0;
+	int i, err;
 
 	vp_dev->vqs = kcalloc(nvqs, sizeof(*vp_dev->vqs), GFP_KERNEL);
 	if (!vp_dev->vqs)
@@ -383,7 +383,7 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned int nvqs,
 			vqs[i] = NULL;
 			continue;
 		}
-		vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
+		vqs[i] = vp_setup_vq(vdev, i, callbacks[i], names[i],
 				     ctx ? ctx[i] : false,
 				     VIRTIO_MSI_NO_VECTOR);
 		if (IS_ERR(vqs[i])) {
diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
index e803db0da307..fe91a5d673dc 100644
--- a/drivers/virtio/virtio_vdpa.c
+++ b/drivers/virtio/virtio_vdpa.c
@@ -370,7 +370,7 @@ static int virtio_vdpa_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
 	struct cpumask *masks;
 	struct vdpa_callback cb;
 	bool has_affinity = desc && ops->set_vq_affinity;
-	int i, err, queue_idx = 0;
+	int i, err;
 
 	if (has_affinity) {
 		masks = create_affinity_masks(nvqs, desc ? desc : &default_affd);
@@ -384,7 +384,7 @@ static int virtio_vdpa_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
 			continue;
 		}
 
-		vqs[i] = virtio_vdpa_setup_vq(vdev, queue_idx++,
+		vqs[i] = virtio_vdpa_setup_vq(vdev, i,
 					      callbacks[i], names[i], ctx ?
 					      ctx[i] : false);
 		if (IS_ERR(vqs[i])) {
-- 
MST

Re: [PATCH 0/2] virtio-balloon: make it spec compliant

[David Hildenbrand]

On 05.07.24 12:08, 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 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?

-- 
Cheers,

David / dhildenb

Re: [PATCH 0/2] virtio-balloon: make it spec compliant

[Michael S. Tsirkin]

On Fri, Jul 05, 2024 at 12:15:30PM +0200, David Hildenbrand wrote:
> On 05.07.24 12:08, 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 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?

Well having vq function shift depending on features is certainly
messy ...
How do we know no one implemented the spec as written though?

-- 
MST

Re: [PATCH 0/2] virtio-balloon: make it spec compliant

[David Hildenbrand]

On 05.07.24 12:19, Michael S. Tsirkin wrote:
> On Fri, Jul 05, 2024 at 12:15:30PM +0200, David Hildenbrand wrote:
>> On 05.07.24 12:08, 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 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?
> 
> Well having vq function shift depending on features is certainly
> messy ...

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

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

-- 
Cheers,

David / dhildenb

Re: [PATCH 0/2] virtio-balloon: make it spec compliant

[Michael S. Tsirkin]

On Fri, Jul 05, 2024 at 01:00:50PM +0200, David Hildenbrand wrote:
> On 05.07.24 12:19, Michael S. Tsirkin wrote:
> > On Fri, Jul 05, 2024 at 12:15:30PM +0200, David Hildenbrand wrote:
> > > On 05.07.24 12:08, 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 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?
> > 
> > Well having vq function shift depending on features is certainly
> > messy ...
> 
> Right, but that's how all of this started from the beginning.
> 
> > 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

Or maybe VIRTIO_BALLOON_F_FREE_PAGE_HINT is set.



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





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

-- 
MST

Re: [PATCH 0/2] virtio-balloon: make it spec compliant

[David Hildenbrand]

Sorry for the late reply!

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

Right, in which case it would be according to the spec.

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

Okay, in the latter case it would be the unofficial way of doing it.

>> 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 :(

That's what I thought, unfortunately.

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

That makes sense to me. Let me take a look at the patches.

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] virtio_balloon: add work around for out of spec QEMU

[David Hildenbrand]

On 05.07.24 12:08, Michael S. Tsirkin wrote:
> 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 <mst@redhat.com>
> ---
>   drivers/virtio/virtio_balloon.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 9a61febbd2f7..7dc3fcd56238 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -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];

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/2] virtio: fix vq # when vq skipped

[David Hildenbrand]

On 05.07.24 12:09, Michael S. Tsirkin wrote:
> 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 <mst@redhat.com>
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] virtio_balloon: add work around for out of spec QEMU

[Jason Wang]

On Fri, Jul 5, 2024 at 6:09 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> 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 <mst@redhat.com>
> ---
>  drivers/virtio/virtio_balloon.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 9a61febbd2f7..7dc3fcd56238 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -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
>

Acked-by: Jason Wang <jasowang@redhat.com>

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

Thanks

Re: [PATCH 2/2] virtio: fix vq # when vq skipped

[Jason Wang]

On Fri, Jul 5, 2024 at 6:09 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> 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 <mst@redhat.com>

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

Re: [PATCH 1/2] virtio_balloon: add work around for out of spec QEMU

[Michael S. Tsirkin]

On Wed, Jul 10, 2024 at 11:23:20AM +0800, Jason Wang wrote:
> On Fri, Jul 5, 2024 at 6:09 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > 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 <mst@redhat.com>
> > ---
> >  drivers/virtio/virtio_balloon.c | 19 +++++++++++++++++--
> >  1 file changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > index 9a61febbd2f7..7dc3fcd56238 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -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
> >
> 
> Acked-by: Jason Wang <jasowang@redhat.com>
> 
> 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

Re: [PATCH 1/2] virtio_balloon: add work around for out of spec QEMU

[Jason Wang]

On Wed, Jul 10, 2024 at 2:16 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Jul 10, 2024 at 11:23:20AM +0800, Jason Wang wrote:
> > On Fri, Jul 5, 2024 at 6:09 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > 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 <mst@redhat.com>
> > > ---
> > >  drivers/virtio/virtio_balloon.c | 19 +++++++++++++++++--
> > >  1 file changed, 17 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > > index 9a61febbd2f7..7dc3fcd56238 100644
> > > --- a/drivers/virtio/virtio_balloon.c
> > > +++ b/drivers/virtio/virtio_balloon.c
> > > @@ -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
> > >
> >
> > Acked-by: Jason Wang <jasowang@redhat.com>
> >
> > 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.

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

>
> --
> MST
>

Re: [PATCH 1/2] virtio_balloon: add work around for out of spec QEMU

[Michael S. Tsirkin]

On Wed, Jul 10, 2024 at 03:37:49PM +0800, Jason Wang wrote:
> On Wed, Jul 10, 2024 at 2:16 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Jul 10, 2024 at 11:23:20AM +0800, Jason Wang wrote:
> > > On Fri, Jul 5, 2024 at 6:09 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > 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 <mst@redhat.com>
> > > > ---
> > > >  drivers/virtio/virtio_balloon.c | 19 +++++++++++++++++--
> > > >  1 file changed, 17 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > > > index 9a61febbd2f7..7dc3fcd56238 100644
> > > > --- a/drivers/virtio/virtio_balloon.c
> > > > +++ b/drivers/virtio/virtio_balloon.c
> > > > @@ -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
> > > >
> > >
> > > Acked-by: Jason Wang <jasowang@redhat.com>
> > >
> > > 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.
> 
> 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

True - I could not find a way to make it work in a way that
would be compatible with old qemu.


> >
> > --
> > MST
> >