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

[David Hildenbrand <david@redhat.com>]

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.

-- 

Thanks,

David / dhildenb