On Mon, Jul 22, 2019 at 03:41:08PM +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-sta...@nongnu.org #v4.0.0
> Suggested-by: Michael S. Tsirkin <m...@redhat.com>
> Signed-off-by: David Hildenbrand <da...@redhat.com>
Ah, nice idea.
Acked-by: David Gibson <da...@gibson.dropbear.id.au>
> ---
> 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)
> 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;
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
