Re: [Qemu-devel] [PATCH v4 2/4] migration: API to clear bits of guest free pages from the dirty bitmap

[Michael S. Tsirkin]

On Thu, Mar 15, 2018 at 06:52:41PM +0800, Wei Wang wrote:
> On 03/15/2018 02:11 AM, Dr. David Alan Gilbert wrote:
> > * Wei Wang (address@hidden) wrote:
> > > This patch adds an API to clear bits corresponding to guest free pages
> > > from the dirty bitmap. Spilt the free page block if it crosses the QEMU
> > > RAMBlock boundary.
> > > 
> > > Signed-off-by: Wei Wang <address@hidden>
> > > CC: Dr. David Alan Gilbert <address@hidden>
> > > CC: Juan Quintela <address@hidden>
> > > CC: Michael S. Tsirkin <address@hidden>
> > > ---
> > >   include/migration/misc.h |  2 ++
> > >   migration/ram.c          | 21 +++++++++++++++++++++
> > >   2 files changed, 23 insertions(+)
> > > 
> > > diff --git a/include/migration/misc.h b/include/migration/misc.h
> > > index 77fd4f5..fae1acf 100644
> > > --- a/include/migration/misc.h
> > > +++ b/include/migration/misc.h
> > > @@ -14,11 +14,13 @@
> > >   #ifndef MIGRATION_MISC_H
> > >   #define MIGRATION_MISC_H
> > > +#include "exec/cpu-common.h"
> > >   #include "qemu/notify.h"
> > >   /* migration/ram.c */
> > >   void ram_mig_init(void);
> > > +void qemu_guest_free_page_hint(void *addr, size_t len);
> > >   /* migration/block.c */
> > > diff --git a/migration/ram.c b/migration/ram.c
> > > index 5e33e5c..e172798 100644
> > > --- a/migration/ram.c
> > > +++ b/migration/ram.c
> > > @@ -2189,6 +2189,27 @@ static int ram_init_all(RAMState **rsp)
> > >       return 0;
> > >   }
> > This could do with some comments
> 
> OK, I'll add some.
> 
> > 
> > > +void qemu_guest_free_page_hint(void *addr, size_t len)
> > > +{
> > > +    RAMBlock *block;
> > > +    ram_addr_t offset;
> > > +    size_t used_len, start, npages;
> >  From your use I think the addr and len are coming raw from the guest;
> > so we need to take some care.
> > 
> 
> Actually the "addr" here has been the host address that corresponds to the
> guest free page. It's from elem->in_sg[0].iov_base.
> 
> > 
> > > +        if (unlikely(offset + len > block->used_length)) {
> > I think to make that overflow safe, that should be:
> >    if (len > (block->used_length - offset)) {
> > 
> > But we'll need another test before it, because qemu_ram_block_from_host
> > seems to check max_length not used_length, so we need to check
> > for offset > block->used_length first
> 
> OK, how about adding an assert above, like this:
> 
> block = qemu_ram_block_from_host(addr, false, &offset);
> assert (offset  < block->used_length );
> if (!block)
>     ...
> 
> The address corresponds to a guest free page, which means it should be
> within used_length. If not, something weird happens, I think we'd better to
> assert it in that case.
> 
> Best,
> Wei

What if memory has been removed by hotunplug after guest sent the
free page notification?

This seems to actually be likely to happen as memory being unplugged
would typically be mostly free.

-- 
MST