Re: [Qemu-devel] [PATCH v1 3/3] virtio-balloon: Rework pbp tracking data

[Michael S. Tsirkin]

On Mon, Jul 22, 2019 at 09:43:43AM +0200, David Hildenbrand wrote:
> On 22.07.19 05:04, David Gibson wrote:
> > 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 <address@hidden>
> > 
> > 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.
> 
> Why should it be discarded? The guest didn't deflate, so if we drop the
> backing page, it works as expected. If the guest deflated, the pbp would
> be discarded.
> 
> Please note that this will not happen in real life (Linux):
> 
> Before we unplug a DIMM, the guest has to offline that memory. Offlining
> means evacuating all pages that are not free (Buddy). Balloon-inflated
> pages are treated like allocated pages, so the balloon pages will have
> to be moved to a different location (inflate new + deflate old). At this
> point, we had a deflate on the page and dropped the pbp.
> 
> If the guest would be reusing memory (after unplug/replug) and leave
> parts of the memory inflated, it would be expected that something goes
> wrong - especially, also the balloon stats would be most probably wrong.
> 
> Using ramblock notifiers, we could discard the pbp in case a new block
> is added/removed, however I am not convinced that this is really needed.

I think I agree here.

> 
> 
> However, there is (yet) another related issue with PBP. In QEMU, we
> don't set
> 
> #define VIRTIO_BALLOON_F_MUST_TELL_HOST 0 /* Tell before reclaiming pages */
> 
> Which means it would right now be valid for the guest to reuse pages
> tracked in the PBP without deflating (although Linux always tells the
> host). This could result in stale PBP data.
> 
> We really should be setting VIRTIO_BALLOON_F_MUST_TELL_HOST.

Ouch. More than that, anything using PBP is already corrupting
guest memory with legacy guests (before 3.0).
Well given it's also corrupting host memory, I'm not sure
whether we should care ...


> -- 
> 
> Thanks,
> 
> David / dhildenb