On Fri, Jul 19, 2019 at 06:01:20PM +0200, David Hildenbrand wrote: > Using the address of a RAMBlock to test for a matching pbp is not really > safe. Instead, let's use the guest physical address of the base page > along with the page size (via the number of subpages). > > While at it, move "struct PartiallyBalloonedPage" to virtio-balloon.h > now (previously most probably avoided to te the ramblock), renaming it. > > Also, let's only dynamically allocating the bitmap. This makes the code > easier to read and maintain - we can reuse bitmap_new(). > > Signed-off-by: David Hildenbrand This mostly looks good, but one thing bothers me.. > --- > hw/virtio/virtio-balloon.c | 52 +++++++++++++++++------------- > include/hw/virtio/virtio-balloon.h | 15 +++++++-- > 2 files changed, 42 insertions(+), 25 deletions(-) > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > index 29cee344b2..8e5f9459c2 100644 > --- a/hw/virtio/virtio-balloon.c > +++ b/hw/virtio/virtio-balloon.c > @@ -34,23 +34,31 @@ > > #define BALLOON_PAGE_SIZE (1 << VIRTIO_BALLOON_PFN_SHIFT) > > -struct PartiallyBalloonedPage { > - RAMBlock *rb; > - ram_addr_t base; > - unsigned long bitmap[]; > -}; > - > static void virtio_balloon_reset_pbp(VirtIOBalloon *balloon) > { > - g_free(balloon->pbp); > - balloon->pbp = NULL; > + balloon->pbp.base_gpa = 0; > + balloon->pbp.subpages = 0; > + g_free(balloon->pbp.bitmap); > + balloon->pbp.bitmap = NULL; > +} > + > +static bool virtio_balloon_pbp_valid(VirtIOBalloon *balloon) > +{ > + return balloon->pbp.bitmap; > +} > + > +static bool virtio_balloon_pbp_matches(VirtIOBalloon *balloon, > + ram_addr_t base_gpa, long subpages) > +{ > + return balloon->pbp.subpages == subpages && > + balloon->pbp.base_gpa == base_gpa; .. so, I'm trying to think what base_gpa matching, but subpages not matching means. I think that can only happen if a pbp is created, then the ramblock it was in is unplugged, then another ramblock is plugged in covering the same address and with a different (host) page size. Which is unlikely but possible, IIUC. However, also possible and marginally less unlikely is to plug a new block of the same page size, in which case this *will* match, but presumably the pbp should still be discarded. > } > > static void balloon_inflate_page(VirtIOBalloon *balloon, > MemoryRegion *mr, hwaddr mr_offset) > { > void *addr = memory_region_get_ram_ptr(mr) + mr_offset; > - ram_addr_t rb_offset, rb_aligned_offset; > + ram_addr_t rb_offset, rb_aligned_offset, base_gpa; > RAMBlock *rb; > size_t rb_page_size; > int subpages; > @@ -81,32 +89,32 @@ static void balloon_inflate_page(VirtIOBalloon *balloon, > > rb_aligned_offset = QEMU_ALIGN_DOWN(rb_offset, rb_page_size); > subpages = rb_page_size / BALLOON_PAGE_SIZE; > + base_gpa = memory_region_get_ram_addr(mr) + mr_offset - > + (rb_offset - rb_aligned_offset); > > - if (balloon->pbp > - && (rb != balloon->pbp->rb > - || rb_aligned_offset != balloon->pbp->base)) { > + if (virtio_balloon_pbp_valid(balloon) && > + !virtio_balloon_pbp_matches(balloon, base_gpa, subpages)) { > /* We've partially ballooned part of a host page, but now > * we're trying to balloon part of a different one. Too hard, > * give up on the old partial page */ > virtio_balloon_reset_pbp(balloon); > } > > - if (!balloon->pbp) { > + if (!virtio_balloon_pbp_valid(balloon)) { > /* Starting on a new host page */ > - size_t bitlen = BITS_TO_LONGS(subpages) * sizeof(unsigned long); > - balloon->pbp = g_malloc0(sizeof(PartiallyBalloonedPage) + bitlen); > - balloon->pbp->rb = rb; > - balloon->pbp->base = rb_aligned_offset; > + balloon->pbp.base_gpa = base_gpa; > + balloon->pbp.subpages = subpages; > + balloon->pbp.bitmap = bitmap_new(subpages); > } > > - set_bit((rb_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE, > - balloon->pbp->bitmap); > + set_bit((rb_offset - rb_aligned_offset) / BALLOON_PAGE_SIZE, > + balloon->pbp.bitmap); > > - if (bitmap_full(balloon->pbp->bitmap, subpages)) { > + if (bitmap_full(balloon->pbp.bitmap, subpages)) { > /* We've accumulated a full host page, we can actually discard > * it now */ > > - ram_block_discard_range(rb, balloon->pbp->base, rb_page_size); > + ram_block_discard_range(rb, rb_aligned_offset, rb_page_size); > /* We ignore errors from ram_block_discard_range(), because it > * has already reported them, and failing to discard a balloon > * page is not fatal */ > @@ -130,7 +138,7 @@ 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) { > + if (virtio_balloon_pbp_valid(balloon)) { > /* Let's play safe and always reset the pbp on deflation requests. */ > virtio_balloon_reset_pbp(balloon); > } > diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h > index 5a99293a45..0190d78d71 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, > @@ -42,6 +40,12 @@ enum virtio_balloon_free_page_report_status { > FREE_PAGE_REPORT_S_DONE = 3, > }; > > +typedef struct VirtIOPartiallyBalloonedPage { > + ram_addr_t base_gpa; > + long subpages; > + unsigned long *bitmap; > +} VirtIOPartiallyBalloonedPage; > + > typedef struct VirtIOBalloon { > VirtIODevice parent_obj; > VirtQueue *ivq, *dvq, *svq, *free_page_vq; > @@ -70,7 +74,12 @@ typedef struct VirtIOBalloon { > int64_t stats_last_update; > int64_t stats_poll_interval; > uint32_t host_features; > - PartiallyBalloonedPage *pbp; > + > + /* > + * Information about a partially ballooned page - does not have to be > + * migrated and has to be reset on every device reset. > + */ > + VirtIOPartiallyBalloonedPage 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