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

[Andrey Gruzdev]

On 30.11.2020 19:32, Peter Xu wrote:
> 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. :)

Yeah, seems assert is a right thing here :)

> > 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
>
> [...]

Ooops, didn't know that, thanks for info!

> > > > +/**
> > > > + * 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.

Now I also think we'd better use common types - mostly uint64_t 
directly, not to confuse anybody with that specific type descriptions.

> > > > +        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?

Mm, need to think about that..

> > > > +    }
> > > > +
> > > >        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,

Agree.

Thanks,

--
Andrey Gruzdev, Principal Engineer
Virtuozzo GmbH  +7-903-247-6397
                virtuzzo.com