Re: [PATCH v4 3/6] support UFFD write fault processing in ram_save_iterate()

[Peter Xu]

On Mon, Nov 30, 2020 at 12:14:03AM +0300, Andrey Gruzdev wrote:
> > > +#ifdef CONFIG_LINUX
> > > +/**
> > > + * ram_find_block_by_host_address: find RAM block containing host page
> > > + *
> > > + * Returns pointer to RAMBlock if found, NULL otherwise
> > > + *
> > > + * @rs: current RAM state
> > > + * @page_address: host page address
> > > + */
> > > +static RAMBlock *ram_find_block_by_host_address(RAMState *rs, hwaddr 
> > > page_address)
> > 
> > Reuse qemu_ram_block_from_host() somehow?
> > 
> 
> Seems not very suitable here, since we use rs->last_seen_block to restart
> search..

The search logic is actually still the same, it's just about which block to
start searching (rs->last_seen_block, ram_list.mru_block, or the 1st one).  So
an internal helper to pass in that information would be nice.  Though that'll
require rcu lock taken before hand to keep the ramblock ptr valid.

No worry - we can keep this and work on top too, I think.

[...]

> > > +static RAMBlock *poll_fault_page(RAMState *rs, ram_addr_t *offset)
> > > +{
> > > +    struct uffd_msg uffd_msg;
> > > +    hwaddr page_address;
> > > +    RAMBlock *bs;
> > > +    int res;
> > > +
> > > +    if (!migrate_background_snapshot()) {
> > > +        return NULL;
> > > +    }
> > > +
> > > +    res = uffd_read_events(rs->uffdio_fd, &uffd_msg, 1);
> > > +    if (res <= 0) {
> > > +        return NULL;
> > > +    }
> > > +
> > > +    page_address = uffd_msg.arg.pagefault.address;
> > > +    bs = ram_find_block_by_host_address(rs, page_address);
> > > +    if (!bs) {
> > > +        /* In case we couldn't find respective block, just unprotect 
> > > faulting page. */
> > > +        uffd_protect_memory(rs->uffdio_fd, page_address, 
> > > TARGET_PAGE_SIZE, false);
> > > +        error_report("ram_find_block_by_host_address() failed: 
> > > address=0x%0lx",
> > > +                page_address);
> > 
> > Looks ok to error_report() instead of assert(), but I'll suggest drop the 
> > call
> > to uffd_protect_memory() at least.  The only reason to not use assert() is
> > because we try our best to avoid crashing the vm, however I really doubt
> > whether uffd_protect_memory() is the right thing to do even if it happens - 
> > we
> > may at last try to unprotect some strange pages that we don't even know 
> > where
> > it is...
> > 
> 
> IMHO better to unprotect these strange pages then to leave them protected by
> UFFD.. To avoid getting VM completely in-operational.
> At least we know the page generated wr-fault, maybe due to incorrect
> write-tracking initialization, or RAMBlock somehow has gone. Nevertheless if
> leave the page as is, VM would certainly lock.

Yes makes some senes too.  However it's definitely a severe programming error,
even if the VM is unblocked, it can be in a very weird state...

Maybe we just assert? Then we can see how unlucky we'll be. :)

> 
> Hmm, I wonder about assert(). In QEMU it would do something in release
> builds?

I guess yes, at least for now.  Because osdep.h has this:

/*
 * We have a lot of unaudited code that may fail in strange ways, or
 * even be a security risk during migration, if you disable assertions
 * at compile-time.  You may comment out these safety checks if you
 * absolutely want to disable assertion overhead, but it is not
 * supported upstream so the risk is all yours.  Meanwhile, please
 * submit patches to remove any side-effects inside an assertion, or
 * fixing error handling that should use Error instead of assert.
 */
#ifdef NDEBUG
#error building with NDEBUG is not supported
#endif
#ifdef G_DISABLE_ASSERT
#error building with G_DISABLE_ASSERT is not supported
#endif

[...]

> > > +/**
> > > + * ram_save_host_page_post: ram_save_host_page() post-notifier
> > > + *
> > > + * @rs: current RAM state
> > > + * @pss: page-search-status structure
> > > + * @opaque: opaque context value
> > > + * @res_override: pointer to the return value of ram_save_host_page(),
> > > + *   overwritten in case of an error
> > > + */
> > > +static void ram_save_host_page_post(RAMState *rs, PageSearchStatus *pss,
> > > +        void *opaque, int *res_override)
> > > +{
> > > +    /* Check if page is from UFFD-managed region. */
> > > +    if (pss->block->flags & RAM_UF_WRITEPROTECT) {
> > > +#ifdef CONFIG_LINUX
> > > +        ram_addr_t page_from = (ram_addr_t) opaque;
> > > +        hwaddr page_address = (hwaddr) pss->block->host +
> > > +                              ((hwaddr) page_from << TARGET_PAGE_BITS);
> > 
> > I feel like most new uses of hwaddr is not correct...  As I also commented 
> > in
> > the other patch.  We should save a lot of castings if switched.
> > 
> 
> Need to check. hwaddr is typedef'ed as uint64_t while ram_addr_t as
> uintptr_t. I any case UFFD interface relies on u64 type.

For example, page_from should be a host address, we can use unsigned long, or
uint64_t, but ram_addr_t is not proper, which is only used in ramblock address
space.

I think it's fine that e.g. we use common types like uint64_t directly.

> 
> > > +        hwaddr run_length = (hwaddr) (pss->page - page_from + 1) << 
> > > TARGET_PAGE_BITS;
> > > +        int res;
> > > +
> > > +        /* Flush async buffers before un-protect. */
> > > +        qemu_fflush(rs->f);
> > > +        /* Un-protect memory range. */
> > > +        res = uffd_protect_memory(rs->uffdio_fd, page_address, 
> > > run_length, false);
> > > +        /* We don't want to override existing error from 
> > > ram_save_host_page(). */
> > > +        if (res < 0 && *res_override >= 0) {
> > > +            *res_override = res;
> > 
> > What is this used for?  If res<0, I thought we should just fail the 
> > snapshot.
> > 
> > Meanwhile, res_override points to "pages", and then it'll be rewrite to the
> > errno returned by uffd_protect_memory().  Smells strange.
> > 
> > Can this ever be triggered anyway?
> > 
> 
> Yes, since "pages" is also for error return, if negative. If we have a
> problem with un-protecting, promote the error to the loop in
> ram_find_and_save_block() so it exits early ("pages" guaranteed to be
> non-zero). In outer routines retcode would go to qemu_set_file_error().

Ok I see your point.

> 
> > > +        }
> > > +#else
> > > +        /* Should never happen */
> > > +        qemu_file_set_error(rs->f, -ENOSYS);
> > > +#endif /* CONFIG_LINUX */
> > > +    }
> > > +}
> > > +
> > >   /**
> > >    * ram_find_and_save_block: finds a dirty page and sends it to f
> > >    *
> > > @@ -1779,14 +1908,14 @@ static int ram_find_and_save_block(RAMState *rs, 
> > > bool last_stage)
> > >           return pages;
> > >       }
> > > +    if (!rs->last_seen_block) {
> > > +        rs->last_seen_block = QLIST_FIRST_RCU(&ram_list.blocks);
> > 
> > Why setup the last seen block to be the first if null?
> > 
> 
> Because ram_find_block_by_host_address() relies on rs->last_seen_block.
> Pss is not passed to that routine.

Ok, but it's odd - because that's not the real "last seen block".

So now I'm rethinking maybe we could still reuse qemu_ram_block_from_host() by
providing a new helper to do the search logic as mentioned above, which could
take a pointer of RAMBlock as the 1st ramblock to search.  Then
ram_find_block_by_host_address() should pass in rs->last_seen_block (which
could be NULL - then we'll start with the 1st ramblock), or ram_list.mru_block
for qemu_ram_block_from_host().  That could be a standalone new patch, then we
don't need this slightly strange change.  What do you think?

> 
> > > +    }
> > > +
> > >       pss.block = rs->last_seen_block;
> > >       pss.page = rs->last_page;
> > >       pss.complete_round = false;
> > > -    if (!pss.block) {
> > > -        pss.block = QLIST_FIRST_RCU(&ram_list.blocks);
> > > -    }
> > > -
> > >       do {
> > >           again = true;
> > >           found = get_queued_page(rs, &pss);
> > > @@ -1797,7 +1926,11 @@ static int ram_find_and_save_block(RAMState *rs, 
> > > bool last_stage)
> > >           }
> > >           if (found) {
> > > +            void *opaque;
> > > +
> > > +            ram_save_host_page_pre(rs, &pss, &opaque);
> > >               pages = ram_save_host_page(rs, &pss, last_stage);
> > > +            ram_save_host_page_post(rs, &pss, opaque, &pages);
> > 
> > So the pre/post idea is kind of an overkill to me...
> > 
> > How about we do the unprotect in ram_save_host_page() in the simple way, 
> > like:
> > 
> >    ram_save_host_page()
> >      start_addr = pss->page;
> >      do {
> >        ...
> >        (migrate pages)
> >        ...
> >      } while (...);
> >      if (background_snapshot_enabled()) {
> >        unprotect pages within start_addr to pss->page;
> >      }
> >      ...
> > 
> 
> Personally I prefer not to modify existing code. May be adding to simple
> calls would finally look cleaner?

Let's wait a 3rd opinion from Juan/Dave or others.

Thanks,

-- 
Peter Xu