[PATCH 1/2] virito-balloon: process all in sgs for free_page_vq

[PATCH 1/2] virito-balloon: process all in sgs for free_page_vq

[Jason Wang]

We only process the first in sg which may lead to the bitmap of the
pages belongs to following sgs were not cleared. This may result more
pages to be migrated. Fixing this by process all in sgs for
free_page_vq.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/virtio-balloon.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index c6962fcbfe..17de2558cb 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -510,6 +510,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtQueue *vq = dev->free_page_vq;
     bool ret = true;
+    int i;
 
     while (dev->block_iothread) {
         qemu_cond_wait(&dev->free_page_cond, &dev->free_page_lock);
@@ -544,8 +545,10 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
     }
 
     if (elem->in_num && dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
-        qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
-                                  elem->in_sg[0].iov_len);
+        for (i = 0; i < elem->in_num; i++) {
+            qemu_guest_free_page_hint(elem->in_sg[i].iov_base,
+                                      elem->in_sg[i].iov_len);
+        }
     }
 
 out:
-- 
2.25.1

[PATCH 2/2] virtio-balloon: correct used length

[Jason Wang]

Spec said:

"and len the total of bytes written into the buffer."

For inflateq, deflateq and statsq, we don't process in_sg so the used
length should be zero. For free_page_vq, since the pages could be
changed in the destination, we should make all pages used for safety.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/virtio-balloon.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 17de2558cb..fb4426ac0c 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -231,7 +231,7 @@ static void balloon_stats_poll_cb(void *opaque)
         return;
     }
 
-    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
+    virtqueue_push(s->svq, s->stats_vq_elem, 0);
     virtio_notify(vdev, s->svq);
     g_free(s->stats_vq_elem);
     s->stats_vq_elem = NULL;
@@ -438,7 +438,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
             memory_region_unref(section.mr);
         }
 
-        virtqueue_push(vq, elem, offset);
+        virtqueue_push(vq, elem, 0);
         virtio_notify(vdev, vq);
         g_free(elem);
         virtio_balloon_pbp_free(&pbp);
@@ -510,6 +510,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtQueue *vq = dev->free_page_vq;
     bool ret = true;
+    size_t used = 0;
     int i;
 
     while (dev->block_iothread) {
@@ -548,11 +549,12 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
         for (i = 0; i < elem->in_num; i++) {
             qemu_guest_free_page_hint(elem->in_sg[i].iov_base,
                                       elem->in_sg[i].iov_len);
+            used += elem->in_sg[i].iov_len;
         }
     }
 
 out:
-    virtqueue_push(vq, elem, 1);
+    virtqueue_push(vq, elem, used);
     g_free(elem);
     return ret;
 }
-- 
2.25.1

Re: [PATCH 1/2] virito-balloon: process all in sgs for free_page_vq

[David Hildenbrand]

On 25.11.21 03:20, Jason Wang wrote:
> We only process the first in sg which may lead to the bitmap of the
> pages belongs to following sgs were not cleared. This may result more
> pages to be migrated. Fixing this by process all in sgs for
> free_page_vq.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/virtio/virtio-balloon.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index c6962fcbfe..17de2558cb 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -510,6 +510,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VirtQueue *vq = dev->free_page_vq;
>      bool ret = true;
> +    int i;
>  
>      while (dev->block_iothread) {
>          qemu_cond_wait(&dev->free_page_cond, &dev->free_page_lock);
> @@ -544,8 +545,10 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
>      }
>  
>      if (elem->in_num && dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
> -        qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
> -                                  elem->in_sg[0].iov_len);
> +        for (i = 0; i < elem->in_num; i++) {
> +            qemu_guest_free_page_hint(elem->in_sg[i].iov_base,
> +                                      elem->in_sg[i].iov_len);
> +        }
>      }
>  
>  out:
> 

Yes, but:

1. Linux never used more than one
2. QEMU never consumed more than one

The spec states:

"(b) The driver maps a series of pages and adds them to the free_page_vq
as individual scatter-gather input buffer entries."

However, the spec was written by someone else (Alex) as the code was
(Wei). The code was there first.

I don't particularly care what to adjust (code or spec). However, to me
it feels more like the spec is slightly wrong and it was intended like
the code is by the original code author.

But then again, I don't particularly care :)

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/2] virito-balloon: process all in sgs for free_page_vq

[Philippe Mathieu-Daudé]

On 11/25/21 03:20, Jason Wang wrote:
> We only process the first in sg which may lead to the bitmap of the
> pages belongs to following sgs were not cleared. This may result more
> pages to be migrated. Fixing this by process all in sgs for
> free_page_vq.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/virtio/virtio-balloon.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Typo "virtio" in subject.

Re: [PATCH 1/2] virito-balloon: process all in sgs for free_page_vq

[Michael S. Tsirkin]

On Thu, Nov 25, 2021 at 09:28:59AM +0100, David Hildenbrand wrote:
> On 25.11.21 03:20, Jason Wang wrote:
> > We only process the first in sg which may lead to the bitmap of the
> > pages belongs to following sgs were not cleared. This may result more
> > pages to be migrated. Fixing this by process all in sgs for
> > free_page_vq.
> > 
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  hw/virtio/virtio-balloon.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index c6962fcbfe..17de2558cb 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -510,6 +510,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >      VirtQueue *vq = dev->free_page_vq;
> >      bool ret = true;
> > +    int i;
> >  
> >      while (dev->block_iothread) {
> >          qemu_cond_wait(&dev->free_page_cond, &dev->free_page_lock);
> > @@ -544,8 +545,10 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
> >      }
> >  
> >      if (elem->in_num && dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
> > -        qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
> > -                                  elem->in_sg[0].iov_len);
> > +        for (i = 0; i < elem->in_num; i++) {
> > +            qemu_guest_free_page_hint(elem->in_sg[i].iov_base,
> > +                                      elem->in_sg[i].iov_len);
> > +        }
> >      }
> >  
> >  out:
> > 
> 
> Yes, but:
> 
> 1. Linux never used more than one
> 2. QEMU never consumed more than one
> 
> The spec states:
> 
> "(b) The driver maps a series of pages and adds them to the free_page_vq
> as individual scatter-gather input buffer entries."
> 
> However, the spec was written by someone else (Alex) as the code was
> (Wei). The code was there first.
> 
> I don't particularly care what to adjust (code or spec). However, to me
> it feels more like the spec is slightly wrong and it was intended like
> the code is by the original code author.
> 
> But then again, I don't particularly care :)

Original QEMU side code had several bugs so, that's another one.
Given nothing too bad happens if guest submits too many S/Gs,
and given the spec also has a general chapter suggesting devices
are flexible in accepting a single buffer split to multiple S/Gs,
I'm inclined to accept the patch.

> -- 
> Thanks,
> 
> David / dhildenb

Re: [PATCH 1/2] virito-balloon: process all in sgs for free_page_vq

[Michael S. Tsirkin]

On Thu, Nov 25, 2021 at 09:34:32AM +0100, Philippe Mathieu-Daudé wrote:
> On 11/25/21 03:20, Jason Wang wrote:
> > We only process the first in sg which may lead to the bitmap of the
> > pages belongs to following sgs were not cleared. This may result more
> > pages to be migrated. Fixing this by process all in sgs for
> > free_page_vq.
> > 
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  hw/virtio/virtio-balloon.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> Typo "virtio" in subject.

Yes, it's an annoyingly common typo.  If using vim, I suggest:

ab virito virtio

in your vimrc.

-- 
MST

Re: [PATCH 1/2] virito-balloon: process all in sgs for free_page_vq

[David Hildenbrand]

On 25.11.21 17:09, Michael S. Tsirkin wrote:
> On Thu, Nov 25, 2021 at 09:28:59AM +0100, David Hildenbrand wrote:
>> On 25.11.21 03:20, Jason Wang wrote:
>>> We only process the first in sg which may lead to the bitmap of the
>>> pages belongs to following sgs were not cleared. This may result more
>>> pages to be migrated. Fixing this by process all in sgs for
>>> free_page_vq.
>>>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> ---
>>>  hw/virtio/virtio-balloon.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>>> index c6962fcbfe..17de2558cb 100644
>>> --- a/hw/virtio/virtio-balloon.c
>>> +++ b/hw/virtio/virtio-balloon.c
>>> @@ -510,6 +510,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
>>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>>      VirtQueue *vq = dev->free_page_vq;
>>>      bool ret = true;
>>> +    int i;
>>>  
>>>      while (dev->block_iothread) {
>>>          qemu_cond_wait(&dev->free_page_cond, &dev->free_page_lock);
>>> @@ -544,8 +545,10 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
>>>      }
>>>  
>>>      if (elem->in_num && dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
>>> -        qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
>>> -                                  elem->in_sg[0].iov_len);
>>> +        for (i = 0; i < elem->in_num; i++) {
>>> +            qemu_guest_free_page_hint(elem->in_sg[i].iov_base,
>>> +                                      elem->in_sg[i].iov_len);
>>> +        }
>>>      }
>>>  
>>>  out:
>>>
>>
>> Yes, but:
>>
>> 1. Linux never used more than one
>> 2. QEMU never consumed more than one
>>
>> The spec states:
>>
>> "(b) The driver maps a series of pages and adds them to the free_page_vq
>> as individual scatter-gather input buffer entries."
>>
>> However, the spec was written by someone else (Alex) as the code was
>> (Wei). The code was there first.
>>
>> I don't particularly care what to adjust (code or spec). However, to me
>> it feels more like the spec is slightly wrong and it was intended like
>> the code is by the original code author.
>>
>> But then again, I don't particularly care :)
> 
> Original QEMU side code had several bugs so, that's another one.
> Given nothing too bad happens if guest submits too many S/Gs,
> and given the spec also has a general chapter suggesting devices
> are flexible in accepting a single buffer split to multiple S/Gs,
> I'm inclined to accept the patch.

Yeah, as I said, I don't particularly care. It's certainly an "easy change".

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

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/2] virtio-balloon: correct used length

[Michael S. Tsirkin]

On Thu, Nov 25, 2021 at 10:20:46AM +0800, Jason Wang wrote:
> Spec said:
> 
> "and len the total of bytes written into the buffer."
> 
> For inflateq, deflateq and statsq, we don't process in_sg so the used
> length should be zero. For free_page_vq, since the pages could be
> changed in the destination, we should make all pages used for safety.

Yea, about that, I know I said it, but I was wrong, sorry.

Spec says this:

	\field{len} is particularly useful
	for drivers using untrusted buffers: if a driver does not know exactly
	how much has been written by the device, the driver would have to zero
	the buffer in advance to ensure no data leakage occurs.

	For example, a network driver may hand a received buffer directly to
	an unprivileged userspace application.  If the network device has not
	overwritten the bytes which were in that buffer, this could leak the
	contents of freed memory from other processes to the application.


In other words, device must guarantee that used length was
written into. Since we don't know that, we really should
write 0 there, and the fact we don't is a spec violation.


> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/virtio/virtio-balloon.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 17de2558cb..fb4426ac0c 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -231,7 +231,7 @@ static void balloon_stats_poll_cb(void *opaque)
>          return;
>      }
>  
> -    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> +    virtqueue_push(s->svq, s->stats_vq_elem, 0);
>      virtio_notify(vdev, s->svq);
>      g_free(s->stats_vq_elem);
>      s->stats_vq_elem = NULL;
> @@ -438,7 +438,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>              memory_region_unref(section.mr);
>          }
>  
> -        virtqueue_push(vq, elem, offset);
> +        virtqueue_push(vq, elem, 0);
>          virtio_notify(vdev, vq);
>          g_free(elem);
>          virtio_balloon_pbp_free(&pbp);
> @@ -510,6 +510,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VirtQueue *vq = dev->free_page_vq;
>      bool ret = true;
> +    size_t used = 0;
>      int i;
>  
>      while (dev->block_iothread) {
> @@ -548,11 +549,12 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
>          for (i = 0; i < elem->in_num; i++) {
>              qemu_guest_free_page_hint(elem->in_sg[i].iov_base,
>                                        elem->in_sg[i].iov_len);
> +            used += elem->in_sg[i].iov_len;
>          }
>      }
>  
>  out:
> -    virtqueue_push(vq, elem, 1);
> +    virtqueue_push(vq, elem, used);
>      g_free(elem);
>      return ret;
>  }
> -- 
> 2.25.1

RE: [PATCH 1/2] virito-balloon: process all in sgs for free_page_vq

[Wang, Wei W]

On Friday, November 26, 2021 12:11 AM, David Hildenbrand wrote:
> On 25.11.21 17:09, Michael S. Tsirkin wrote:
> > On Thu, Nov 25, 2021 at 09:28:59AM +0100, David Hildenbrand wrote:
> >> On 25.11.21 03:20, Jason Wang wrote:
> >>> We only process the first in sg which may lead to the bitmap of the
> >>> pages belongs to following sgs were not cleared. This may result
> >>> more pages to be migrated. Fixing this by process all in sgs for
> >>> free_page_vq.
> >>>
> >>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>> ---
> >>>  hw/virtio/virtio-balloon.c | 7 +++++--
> >>>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> >>> index c6962fcbfe..17de2558cb 100644
> >>> --- a/hw/virtio/virtio-balloon.c
> >>> +++ b/hw/virtio/virtio-balloon.c
> >>> @@ -510,6 +510,7 @@ static bool get_free_page_hints(VirtIOBalloon
> *dev)
> >>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >>>      VirtQueue *vq = dev->free_page_vq;
> >>>      bool ret = true;
> >>> +    int i;
> >>>
> >>>      while (dev->block_iothread) {
> >>>          qemu_cond_wait(&dev->free_page_cond,
> &dev->free_page_lock);
> >>> @@ -544,8 +545,10 @@ static bool get_free_page_hints(VirtIOBalloon
> *dev)
> >>>      }
> >>>
> >>>      if (elem->in_num && dev->free_page_hint_status ==
> FREE_PAGE_HINT_S_START) {
> >>> -        qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
> >>> -                                  elem->in_sg[0].iov_len);
> >>> +        for (i = 0; i < elem->in_num; i++) {
> >>> +            qemu_guest_free_page_hint(elem->in_sg[i].iov_base,
> >>> +                                      elem->in_sg[i].iov_len);
> >>> +        }
> >>>      }
> >>>
> >>>  out:
> >>>
> >>
> >> Yes, but:
> >>
> >> 1. Linux never used more than one
> >> 2. QEMU never consumed more than one

Yes, it works based on the fact that Linux only sends one hint each time.

> >>
> >> The spec states:
> >>
> >> "(b) The driver maps a series of pages and adds them to the
> >> free_page_vq as individual scatter-gather input buffer entries."
> >>
> >> However, the spec was written by someone else (Alex) as the code was
> >> (Wei). The code was there first.
> >>
> >> I don't particularly care what to adjust (code or spec). However, to
> >> me it feels more like the spec is slightly wrong and it was intended
> >> like the code is by the original code author.
> >>
> >> But then again, I don't particularly care :)
> >
> > Original QEMU side code had several bugs so, that's another one.
> > Given nothing too bad happens if guest submits too many S/Gs, and
> > given the spec also has a general chapter suggesting devices are
> > flexible in accepting a single buffer split to multiple S/Gs, I'm
> > inclined to accept the patch.
> 
> Yeah, as I said, I don't particularly care. It's certainly an "easy change".
> 
> Acked-by: David Hildenbrand <david@redhat.com>
> 

Don’t object the change.
Just in case something unexpected, it would be better if someone could help do a test.

Thanks,
Wei

Re: [PATCH 1/2] virito-balloon: process all in sgs for free_page_vq

[Michael S. Tsirkin]

On Fri, Nov 26, 2021 at 01:21:46AM +0000, Wang, Wei W wrote:
> On Friday, November 26, 2021 12:11 AM, David Hildenbrand wrote:
> > On 25.11.21 17:09, Michael S. Tsirkin wrote:
> > > On Thu, Nov 25, 2021 at 09:28:59AM +0100, David Hildenbrand wrote:
> > >> On 25.11.21 03:20, Jason Wang wrote:
> > >>> We only process the first in sg which may lead to the bitmap of the
> > >>> pages belongs to following sgs were not cleared. This may result
> > >>> more pages to be migrated. Fixing this by process all in sgs for
> > >>> free_page_vq.
> > >>>
> > >>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > >>> ---
> > >>>  hw/virtio/virtio-balloon.c | 7 +++++--
> > >>>  1 file changed, 5 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > >>> index c6962fcbfe..17de2558cb 100644
> > >>> --- a/hw/virtio/virtio-balloon.c
> > >>> +++ b/hw/virtio/virtio-balloon.c
> > >>> @@ -510,6 +510,7 @@ static bool get_free_page_hints(VirtIOBalloon
> > *dev)
> > >>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > >>>      VirtQueue *vq = dev->free_page_vq;
> > >>>      bool ret = true;
> > >>> +    int i;
> > >>>
> > >>>      while (dev->block_iothread) {
> > >>>          qemu_cond_wait(&dev->free_page_cond,
> > &dev->free_page_lock);
> > >>> @@ -544,8 +545,10 @@ static bool get_free_page_hints(VirtIOBalloon
> > *dev)
> > >>>      }
> > >>>
> > >>>      if (elem->in_num && dev->free_page_hint_status ==
> > FREE_PAGE_HINT_S_START) {
> > >>> -        qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
> > >>> -                                  elem->in_sg[0].iov_len);
> > >>> +        for (i = 0; i < elem->in_num; i++) {
> > >>> +            qemu_guest_free_page_hint(elem->in_sg[i].iov_base,
> > >>> +                                      elem->in_sg[i].iov_len);
> > >>> +        }
> > >>>      }
> > >>>
> > >>>  out:
> > >>>
> > >>
> > >> Yes, but:
> > >>
> > >> 1. Linux never used more than one
> > >> 2. QEMU never consumed more than one
> 
> Yes, it works based on the fact that Linux only sends one hint each time.
> 
> > >>
> > >> The spec states:
> > >>
> > >> "(b) The driver maps a series of pages and adds them to the
> > >> free_page_vq as individual scatter-gather input buffer entries."
> > >>
> > >> However, the spec was written by someone else (Alex) as the code was
> > >> (Wei). The code was there first.
> > >>
> > >> I don't particularly care what to adjust (code or spec). However, to
> > >> me it feels more like the spec is slightly wrong and it was intended
> > >> like the code is by the original code author.
> > >>
> > >> But then again, I don't particularly care :)
> > >
> > > Original QEMU side code had several bugs so, that's another one.
> > > Given nothing too bad happens if guest submits too many S/Gs, and
> > > given the spec also has a general chapter suggesting devices are
> > > flexible in accepting a single buffer split to multiple S/Gs, I'm
> > > inclined to accept the patch.
> > 
> > Yeah, as I said, I don't particularly care. It's certainly an "easy change".
> > 
> > Acked-by: David Hildenbrand <david@redhat.com>
> > 
> 
> Don’t object the change.
> Just in case something unexpected, it would be better if someone could help do a test.
> 
> Thanks,
> Wei

Yes, the setup you used to test the original patches will do fine ...

-- 
MST

Re: [PATCH 1/2] virito-balloon: process all in sgs for free_page_vq

[Jason Wang]

On Fri, Nov 26, 2021 at 9:21 AM Wang, Wei W <wei.w.wang@intel.com> wrote:
>
> On Friday, November 26, 2021 12:11 AM, David Hildenbrand wrote:
> > On 25.11.21 17:09, Michael S. Tsirkin wrote:
> > > On Thu, Nov 25, 2021 at 09:28:59AM +0100, David Hildenbrand wrote:
> > >> On 25.11.21 03:20, Jason Wang wrote:
> > >>> We only process the first in sg which may lead to the bitmap of the
> > >>> pages belongs to following sgs were not cleared. This may result
> > >>> more pages to be migrated. Fixing this by process all in sgs for
> > >>> free_page_vq.
> > >>>
> > >>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > >>> ---
> > >>>  hw/virtio/virtio-balloon.c | 7 +++++--
> > >>>  1 file changed, 5 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > >>> index c6962fcbfe..17de2558cb 100644
> > >>> --- a/hw/virtio/virtio-balloon.c
> > >>> +++ b/hw/virtio/virtio-balloon.c
> > >>> @@ -510,6 +510,7 @@ static bool get_free_page_hints(VirtIOBalloon
> > *dev)
> > >>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > >>>      VirtQueue *vq = dev->free_page_vq;
> > >>>      bool ret = true;
> > >>> +    int i;
> > >>>
> > >>>      while (dev->block_iothread) {
> > >>>          qemu_cond_wait(&dev->free_page_cond,
> > &dev->free_page_lock);
> > >>> @@ -544,8 +545,10 @@ static bool get_free_page_hints(VirtIOBalloon
> > *dev)
> > >>>      }
> > >>>
> > >>>      if (elem->in_num && dev->free_page_hint_status ==
> > FREE_PAGE_HINT_S_START) {
> > >>> -        qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
> > >>> -                                  elem->in_sg[0].iov_len);
> > >>> +        for (i = 0; i < elem->in_num; i++) {
> > >>> +            qemu_guest_free_page_hint(elem->in_sg[i].iov_base,
> > >>> +                                      elem->in_sg[i].iov_len);
> > >>> +        }
> > >>>      }
> > >>>
> > >>>  out:
> > >>>
> > >>
> > >> Yes, but:
> > >>
> > >> 1. Linux never used more than one
> > >> 2. QEMU never consumed more than one
>
> Yes, it works based on the fact that Linux only sends one hint each time.
>
> > >>
> > >> The spec states:
> > >>
> > >> "(b) The driver maps a series of pages and adds them to the
> > >> free_page_vq as individual scatter-gather input buffer entries."
> > >>
> > >> However, the spec was written by someone else (Alex) as the code was
> > >> (Wei). The code was there first.
> > >>
> > >> I don't particularly care what to adjust (code or spec). However, to
> > >> me it feels more like the spec is slightly wrong and it was intended
> > >> like the code is by the original code author.
> > >>
> > >> But then again, I don't particularly care :)
> > >
> > > Original QEMU side code had several bugs so, that's another one.
> > > Given nothing too bad happens if guest submits too many S/Gs, and
> > > given the spec also has a general chapter suggesting devices are
> > > flexible in accepting a single buffer split to multiple S/Gs, I'm
> > > inclined to accept the patch.

Yes, and it's probably too late to change the spec even if we want to change.

> >
> > Yeah, as I said, I don't particularly care. It's certainly an "easy change".
> >
> > Acked-by: David Hildenbrand <david@redhat.com>
> >

Thanks

>
> Don’t object the change.
> Just in case something unexpected, it would be better if someone could help do a test.

I've tested the code with migration before sending the patches, I see
the hint works fine.

Thanks

>
> Thanks,
> Wei

RE: [PATCH 1/2] virito-balloon: process all in sgs for free_page_vq

[Wang, Wei W]

On Friday, November 26, 2021 10:31 AM, Jason Wang wrote:
> 
> I've tested the code with migration before sending the patches, I see the hint
> works fine.
> 

That's great (assume you saw great reduction in the migration time as well).
Reviewed-by: Wei Wang <wei.w.wang@intel.com>

Thanks,
Wei

Re: [PATCH 1/2] virito-balloon: process all in sgs for free_page_vq

[Jason Wang]

On Fri, Nov 26, 2021 at 12:10 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Nov 25, 2021 at 09:34:32AM +0100, Philippe Mathieu-Daudé wrote:
> > On 11/25/21 03:20, Jason Wang wrote:
> > > We only process the first in sg which may lead to the bitmap of the
> > > pages belongs to following sgs were not cleared. This may result more
> > > pages to be migrated. Fixing this by process all in sgs for
> > > free_page_vq.
> > >
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  hw/virtio/virtio-balloon.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > Typo "virtio" in subject.
>
> Yes, it's an annoyingly common typo.  If using vim, I suggest:
>
> ab virito virtio
>
> in your vimrc.

Right, actually I'm using flyspell with emacs. I will add a dedicated
detection like this if it's possible.

Will fix it.

Thanks

>
> --
> MST
>

Re: [PATCH 1/2] virito-balloon: process all in sgs for free_page_vq

[Jason Wang]

On Fri, Nov 26, 2021 at 10:40 AM Wang, Wei W <wei.w.wang@intel.com> wrote:
>
> On Friday, November 26, 2021 10:31 AM, Jason Wang wrote:
> >
> > I've tested the code with migration before sending the patches, I see the hint
> > works fine.
> >
>
> That's great (assume you saw great reduction in the migration time as well).
> Reviewed-by: Wei Wang <wei.w.wang@intel.com>

I don't measure that. But it should be sufficient to see the hint
considering we don't modify any logic at dirty bitmap layer.

Thanks

>
> Thanks,
> Wei

Re: [PATCH 2/2] virtio-balloon: correct used length

[Jason Wang]

On Fri, Nov 26, 2021 at 12:14 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Nov 25, 2021 at 10:20:46AM +0800, Jason Wang wrote:
> > Spec said:
> >
> > "and len the total of bytes written into the buffer."
> >
> > For inflateq, deflateq and statsq, we don't process in_sg so the used
> > length should be zero. For free_page_vq, since the pages could be
> > changed in the destination, we should make all pages used for safety.
>
> Yea, about that, I know I said it, but I was wrong, sorry.
>
> Spec says this:
>
>         \field{len} is particularly useful
>         for drivers using untrusted buffers: if a driver does not know exactly
>         how much has been written by the device, the driver would have to zero
>         the buffer in advance to ensure no data leakage occurs.
>
>         For example, a network driver may hand a received buffer directly to
>         an unprivileged userspace application.  If the network device has not
>         overwritten the bytes which were in that buffer, this could leak the
>         contents of freed memory from other processes to the application.
>
>
> In other words, device must guarantee that used length was
> written into. Since we don't know that, we really should
> write 0 there, and the fact we don't is a spec violation.

The problem is, if we write 0, the driver may assume there's no change
on those pages?

Thanks

>
>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  hw/virtio/virtio-balloon.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index 17de2558cb..fb4426ac0c 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -231,7 +231,7 @@ static void balloon_stats_poll_cb(void *opaque)
> >          return;
> >      }
> >
> > -    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> > +    virtqueue_push(s->svq, s->stats_vq_elem, 0);
> >      virtio_notify(vdev, s->svq);
> >      g_free(s->stats_vq_elem);
> >      s->stats_vq_elem = NULL;
> > @@ -438,7 +438,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> >              memory_region_unref(section.mr);
> >          }
> >
> > -        virtqueue_push(vq, elem, offset);
> > +        virtqueue_push(vq, elem, 0);
> >          virtio_notify(vdev, vq);
> >          g_free(elem);
> >          virtio_balloon_pbp_free(&pbp);
> > @@ -510,6 +510,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >      VirtQueue *vq = dev->free_page_vq;
> >      bool ret = true;
> > +    size_t used = 0;
> >      int i;
> >
> >      while (dev->block_iothread) {
> > @@ -548,11 +549,12 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
> >          for (i = 0; i < elem->in_num; i++) {
> >              qemu_guest_free_page_hint(elem->in_sg[i].iov_base,
> >                                        elem->in_sg[i].iov_len);
> > +            used += elem->in_sg[i].iov_len;
> >          }
> >      }
> >
> >  out:
> > -    virtqueue_push(vq, elem, 1);
> > +    virtqueue_push(vq, elem, used);
> >      g_free(elem);
> >      return ret;
> >  }
> > --
> > 2.25.1
>

Re: [PATCH 2/2] virtio-balloon: correct used length

[Michael S. Tsirkin]

On Fri, Nov 26, 2021 at 10:45:43AM +0800, Jason Wang wrote:
> On Fri, Nov 26, 2021 at 12:14 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Nov 25, 2021 at 10:20:46AM +0800, Jason Wang wrote:
> > > Spec said:
> > >
> > > "and len the total of bytes written into the buffer."
> > >
> > > For inflateq, deflateq and statsq, we don't process in_sg so the used
> > > length should be zero. For free_page_vq, since the pages could be
> > > changed in the destination, we should make all pages used for safety.
> >
> > Yea, about that, I know I said it, but I was wrong, sorry.
> >
> > Spec says this:
> >
> >         \field{len} is particularly useful
> >         for drivers using untrusted buffers: if a driver does not know exactly
> >         how much has been written by the device, the driver would have to zero
> >         the buffer in advance to ensure no data leakage occurs.
> >
> >         For example, a network driver may hand a received buffer directly to
> >         an unprivileged userspace application.  If the network device has not
> >         overwritten the bytes which were in that buffer, this could leak the
> >         contents of freed memory from other processes to the application.
> >
> >
> > In other words, device must guarantee that used length was
> > written into. Since we don't know that, we really should
> > write 0 there, and the fact we don't is a spec violation.
> 
> The problem is, if we write 0, the driver may assume there's no change
> on those pages?
> 
> Thanks

No:


The driver MUST NOT make assumptions about data in device-writable buffers
beyond the first \field{len} bytes, and SHOULD ignore this data.



> >
> >
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  hw/virtio/virtio-balloon.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > > index 17de2558cb..fb4426ac0c 100644
> > > --- a/hw/virtio/virtio-balloon.c
> > > +++ b/hw/virtio/virtio-balloon.c
> > > @@ -231,7 +231,7 @@ static void balloon_stats_poll_cb(void *opaque)
> > >          return;
> > >      }
> > >
> > > -    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> > > +    virtqueue_push(s->svq, s->stats_vq_elem, 0);
> > >      virtio_notify(vdev, s->svq);
> > >      g_free(s->stats_vq_elem);
> > >      s->stats_vq_elem = NULL;
> > > @@ -438,7 +438,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> > >              memory_region_unref(section.mr);
> > >          }
> > >
> > > -        virtqueue_push(vq, elem, offset);
> > > +        virtqueue_push(vq, elem, 0);
> > >          virtio_notify(vdev, vq);
> > >          g_free(elem);
> > >          virtio_balloon_pbp_free(&pbp);
> > > @@ -510,6 +510,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
> > >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > >      VirtQueue *vq = dev->free_page_vq;
> > >      bool ret = true;
> > > +    size_t used = 0;
> > >      int i;
> > >
> > >      while (dev->block_iothread) {
> > > @@ -548,11 +549,12 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
> > >          for (i = 0; i < elem->in_num; i++) {
> > >              qemu_guest_free_page_hint(elem->in_sg[i].iov_base,
> > >                                        elem->in_sg[i].iov_len);
> > > +            used += elem->in_sg[i].iov_len;
> > >          }
> > >      }
> > >
> > >  out:
> > > -    virtqueue_push(vq, elem, 1);
> > > +    virtqueue_push(vq, elem, used);
> > >      g_free(elem);
> > >      return ret;
> > >  }
> > > --
> > > 2.25.1
> >

Re: [PATCH 2/2] virtio-balloon: correct used length

[Jason Wang]

On Fri, Nov 26, 2021 at 3:37 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Nov 26, 2021 at 10:45:43AM +0800, Jason Wang wrote:
> > On Fri, Nov 26, 2021 at 12:14 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, Nov 25, 2021 at 10:20:46AM +0800, Jason Wang wrote:
> > > > Spec said:
> > > >
> > > > "and len the total of bytes written into the buffer."
> > > >
> > > > For inflateq, deflateq and statsq, we don't process in_sg so the used
> > > > length should be zero. For free_page_vq, since the pages could be
> > > > changed in the destination, we should make all pages used for safety.
> > >
> > > Yea, about that, I know I said it, but I was wrong, sorry.
> > >
> > > Spec says this:
> > >
> > >         \field{len} is particularly useful
> > >         for drivers using untrusted buffers: if a driver does not know exactly
> > >         how much has been written by the device, the driver would have to zero
> > >         the buffer in advance to ensure no data leakage occurs.
> > >
> > >         For example, a network driver may hand a received buffer directly to
> > >         an unprivileged userspace application.  If the network device has not
> > >         overwritten the bytes which were in that buffer, this could leak the
> > >         contents of freed memory from other processes to the application.
> > >
> > >
> > > In other words, device must guarantee that used length was
> > > written into. Since we don't know that, we really should
> > > write 0 there, and the fact we don't is a spec violation.
> >
> > The problem is, if we write 0, the driver may assume there's no change
> > on those pages?
> >
> > Thanks
>
> No:
>
>
> The driver MUST NOT make assumptions about data in device-writable buffers
> beyond the first \field{len} bytes, and SHOULD ignore this data.

Good to know this. Will fix it in V2.

Thanks

>
>
>
> > >
> > >
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > ---
> > > >  hw/virtio/virtio-balloon.c | 8 +++++---
> > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > > > index 17de2558cb..fb4426ac0c 100644
> > > > --- a/hw/virtio/virtio-balloon.c
> > > > +++ b/hw/virtio/virtio-balloon.c
> > > > @@ -231,7 +231,7 @@ static void balloon_stats_poll_cb(void *opaque)
> > > >          return;
> > > >      }
> > > >
> > > > -    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> > > > +    virtqueue_push(s->svq, s->stats_vq_elem, 0);
> > > >      virtio_notify(vdev, s->svq);
> > > >      g_free(s->stats_vq_elem);
> > > >      s->stats_vq_elem = NULL;
> > > > @@ -438,7 +438,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> > > >              memory_region_unref(section.mr);
> > > >          }
> > > >
> > > > -        virtqueue_push(vq, elem, offset);
> > > > +        virtqueue_push(vq, elem, 0);
> > > >          virtio_notify(vdev, vq);
> > > >          g_free(elem);
> > > >          virtio_balloon_pbp_free(&pbp);
> > > > @@ -510,6 +510,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
> > > >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > > >      VirtQueue *vq = dev->free_page_vq;
> > > >      bool ret = true;
> > > > +    size_t used = 0;
> > > >      int i;
> > > >
> > > >      while (dev->block_iothread) {
> > > > @@ -548,11 +549,12 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
> > > >          for (i = 0; i < elem->in_num; i++) {
> > > >              qemu_guest_free_page_hint(elem->in_sg[i].iov_base,
> > > >                                        elem->in_sg[i].iov_len);
> > > > +            used += elem->in_sg[i].iov_len;
> > > >          }
> > > >      }
> > > >
> > > >  out:
> > > > -    virtqueue_push(vq, elem, 1);
> > > > +    virtqueue_push(vq, elem, used);
> > > >      g_free(elem);
> > > >      return ret;
> > > >  }
> > > > --
> > > > 2.25.1
> > >
>