[PATCH-for-4.1,v4,6/7] virtio-balloon: Use temporary PBP only

We still have multiple issues in the current code
- The PBP is not freed during unrealize()
- The PBP is not reset on device resets: After a reset, the PBP is stale.
- We are not indicating VIRTIO_BALLOON_F_MUST_TELL_HOST, therefore
  guests (esp. legacy guests) will reuse pages without deflating,
  turning the PBP stale. Adding that would require compat handling.

Instead, let's use the PBP only temporarily, when processing one bulk of
inflation requests. This will keep guest_page_size > 4k working (with
Linux guests). There is nothing to do for deflation requests anymore.
The pbp is only used for a limited amount of time.

Fixes: ed48c59875b6 ("virtio-balloon: Safely handle BALLOON_PAGE_SIZE <
                     host page size")
Cc: qemu-stable@nongnu.org #v4.0.0
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/virtio/virtio-balloon.c         | 21 +++++++++------------
 include/hw/virtio/virtio-balloon.h |  3 ---
 2 files changed, 9 insertions(+), 15 deletions(-)

On Thu, Jul 25, 2019 at 01:36:37PM +0200, David Hildenbrand wrote:
> We still have multiple issues in the current code
> - The PBP is not freed during unrealize()
> - The PBP is not reset on device resets: After a reset, the PBP is stale.
> - We are not indicating VIRTIO_BALLOON_F_MUST_TELL_HOST, therefore
>   guests (esp. legacy guests) will reuse pages without deflating,
>   turning the PBP stale. Adding that would require compat handling.
> 
> Instead, let's use the PBP only temporarily, when processing one bulk of
> inflation requests. This will keep guest_page_size > 4k working (with
> Linux guests). There is nothing to do for deflation requests anymore.
> The pbp is only used for a limited amount of time.
> 
> Fixes: ed48c59875b6 ("virtio-balloon: Safely handle BALLOON_PAGE_SIZE <
>                      host page size")
> Cc: qemu-stable@nongnu.org #v4.0.0
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Acked-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/virtio/virtio-balloon.c         | 21 +++++++++------------
>  include/hw/virtio/virtio-balloon.h |  3 ---
>  2 files changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 40d493a31a..a6282d58d4 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -34,11 +34,11 @@
>  
>  #define BALLOON_PAGE_SIZE  (1 << VIRTIO_BALLOON_PFN_SHIFT)
>  
> -struct PartiallyBalloonedPage {
> +typedef struct PartiallyBalloonedPage {
>      ram_addr_t base_gpa;
>      long subpages;
>      unsigned long *bitmap;
> -};
> +} PartiallyBalloonedPage;
>  
>  static void virtio_balloon_pbp_free(PartiallyBalloonedPage *pbp)
>  {
> @@ -68,11 +68,11 @@ static bool virtio_balloon_pbp_matches(PartiallyBalloonedPage *pbp,
>  }
>  
>  static void balloon_inflate_page(VirtIOBalloon *balloon,
> -                                 MemoryRegion *mr, hwaddr mr_offset)
> +                                 MemoryRegion *mr, hwaddr mr_offset,
> +                                 PartiallyBalloonedPage **pbp)
>  {
>      void *addr = memory_region_get_ram_ptr(mr) + mr_offset;
>      ram_addr_t rb_offset, rb_aligned_offset, base_gpa;
> -    PartiallyBalloonedPage **pbp = &balloon->pbp;
>      RAMBlock *rb;
>      size_t rb_page_size;
>      int subpages;
> @@ -149,12 +149,6 @@ static void balloon_deflate_page(VirtIOBalloon *balloon,
>      rb = qemu_ram_block_from_host(addr, false, &rb_offset);
>      rb_page_size = qemu_ram_pagesize(rb);
>  
> -    if (balloon->pbp) {
> -        /* Let's play safe and always reset the pbp on deflation requests. */
> -        virtio_balloon_pbp_free(balloon->pbp);
> -        balloon->pbp = NULL;
> -    }
> -
>      host_addr = (void *)((uintptr_t)addr & ~(rb_page_size - 1));
>  
>      /* When a page is deflated, we hint the whole host page it lives
> @@ -336,6 +330,7 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v,
>  static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>  {
>      VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
> +    PartiallyBalloonedPage *pbp = NULL;
>      VirtQueueElement *elem;
>      MemoryRegionSection section;
>  
> @@ -344,7 +339,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>          uint32_t pfn;
>          elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
>          if (!elem) {
> -            return;
> +            break;
>          }
>  
>          while (iov_to_buf(elem->out_sg, elem->out_num, offset, &pfn, 4) == 4) {
> @@ -373,7 +368,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>              if (!qemu_balloon_is_inhibited()) {
>                  if (vq == s->ivq) {
>                      balloon_inflate_page(s, section.mr,
> -                                         section.offset_within_region);
> +                                         section.offset_within_region, &pbp);
>                  } else if (vq == s->dvq) {
>                      balloon_deflate_page(s, section.mr, section.offset_within_region);
>                  } else {
> @@ -387,6 +382,8 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>          virtio_notify(vdev, vq);
>          g_free(elem);
>      }
> +
> +    virtio_balloon_pbp_free(pbp);
>  }
>  
>  static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)

Oops this is not early enough.
We must not track pbp across elements: once we push an element
guest can reuse the page.

I propose changing virtio_balloon_pbp_free to get
PartiallyBalloonedPage** and set it to NULL.

Call that within the loop after g_free(elem);




> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> index 5a99293a45..d1c968d237 100644
> --- a/include/hw/virtio/virtio-balloon.h
> +++ b/include/hw/virtio/virtio-balloon.h
> @@ -33,8 +33,6 @@ typedef struct virtio_balloon_stat_modern {
>         uint64_t val;
>  } VirtIOBalloonStatModern;
>  
> -typedef struct PartiallyBalloonedPage PartiallyBalloonedPage;
> -
>  enum virtio_balloon_free_page_report_status {
>      FREE_PAGE_REPORT_S_STOP = 0,
>      FREE_PAGE_REPORT_S_REQUESTED = 1,
> @@ -70,7 +68,6 @@ typedef struct VirtIOBalloon {
>      int64_t stats_last_update;
>      int64_t stats_poll_interval;
>      uint32_t host_features;
> -    PartiallyBalloonedPage *pbp;
>  
>      bool qemu_4_0_config_size;
>  } VirtIOBalloon;
> -- 
> 2.21.0

On 25.07.19 13:53, Michael S. Tsirkin wrote:
> On Thu, Jul 25, 2019 at 01:36:37PM +0200, David Hildenbrand wrote:
>> We still have multiple issues in the current code
>> - The PBP is not freed during unrealize()
>> - The PBP is not reset on device resets: After a reset, the PBP is stale.
>> - We are not indicating VIRTIO_BALLOON_F_MUST_TELL_HOST, therefore
>>   guests (esp. legacy guests) will reuse pages without deflating,
>>   turning the PBP stale. Adding that would require compat handling.
>>
>> Instead, let's use the PBP only temporarily, when processing one bulk of
>> inflation requests. This will keep guest_page_size > 4k working (with
>> Linux guests). There is nothing to do for deflation requests anymore.
>> The pbp is only used for a limited amount of time.
>>
>> Fixes: ed48c59875b6 ("virtio-balloon: Safely handle BALLOON_PAGE_SIZE <
>>                      host page size")
>> Cc: qemu-stable@nongnu.org #v4.0.0
>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>> Acked-by: David Gibson <david@gibson.dropbear.id.au>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/virtio/virtio-balloon.c         | 21 +++++++++------------
>>  include/hw/virtio/virtio-balloon.h |  3 ---
>>  2 files changed, 9 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>> index 40d493a31a..a6282d58d4 100644
>> --- a/hw/virtio/virtio-balloon.c
>> +++ b/hw/virtio/virtio-balloon.c
>> @@ -34,11 +34,11 @@
>>  
>>  #define BALLOON_PAGE_SIZE  (1 << VIRTIO_BALLOON_PFN_SHIFT)
>>  
>> -struct PartiallyBalloonedPage {
>> +typedef struct PartiallyBalloonedPage {
>>      ram_addr_t base_gpa;
>>      long subpages;
>>      unsigned long *bitmap;
>> -};
>> +} PartiallyBalloonedPage;
>>  
>>  static void virtio_balloon_pbp_free(PartiallyBalloonedPage *pbp)
>>  {
>> @@ -68,11 +68,11 @@ static bool virtio_balloon_pbp_matches(PartiallyBalloonedPage *pbp,
>>  }
>>  
>>  static void balloon_inflate_page(VirtIOBalloon *balloon,
>> -                                 MemoryRegion *mr, hwaddr mr_offset)
>> +                                 MemoryRegion *mr, hwaddr mr_offset,
>> +                                 PartiallyBalloonedPage **pbp)
>>  {
>>      void *addr = memory_region_get_ram_ptr(mr) + mr_offset;
>>      ram_addr_t rb_offset, rb_aligned_offset, base_gpa;
>> -    PartiallyBalloonedPage **pbp = &balloon->pbp;
>>      RAMBlock *rb;
>>      size_t rb_page_size;
>>      int subpages;
>> @@ -149,12 +149,6 @@ static void balloon_deflate_page(VirtIOBalloon *balloon,
>>      rb = qemu_ram_block_from_host(addr, false, &rb_offset);
>>      rb_page_size = qemu_ram_pagesize(rb);
>>  
>> -    if (balloon->pbp) {
>> -        /* Let's play safe and always reset the pbp on deflation requests. */
>> -        virtio_balloon_pbp_free(balloon->pbp);
>> -        balloon->pbp = NULL;
>> -    }
>> -
>>      host_addr = (void *)((uintptr_t)addr & ~(rb_page_size - 1));
>>  
>>      /* When a page is deflated, we hint the whole host page it lives
>> @@ -336,6 +330,7 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v,
>>  static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>>  {
>>      VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
>> +    PartiallyBalloonedPage *pbp = NULL;
>>      VirtQueueElement *elem;
>>      MemoryRegionSection section;
>>  
>> @@ -344,7 +339,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>>          uint32_t pfn;
>>          elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
>>          if (!elem) {
>> -            return;
>> +            break;
>>          }
>>  
>>          while (iov_to_buf(elem->out_sg, elem->out_num, offset, &pfn, 4) == 4) {
>> @@ -373,7 +368,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>>              if (!qemu_balloon_is_inhibited()) {
>>                  if (vq == s->ivq) {
>>                      balloon_inflate_page(s, section.mr,
>> -                                         section.offset_within_region);
>> +                                         section.offset_within_region, &pbp);
>>                  } else if (vq == s->dvq) {
>>                      balloon_deflate_page(s, section.mr, section.offset_within_region);
>>                  } else {
>> @@ -387,6 +382,8 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>>          virtio_notify(vdev, vq);
>>          g_free(elem);
>>      }
>> +
>> +    virtio_balloon_pbp_free(pbp);
>>  }
>>  
>>  static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
> 
> Oops this is not early enough.
> We must not track pbp across elements: once we push an element
> guest can reuse the page.
> 

Right, it has to go into the loop.