Re: [Xen-devel] [PATCH v7 04/10] xen: Introduce XEN_DOMCTL_soft_reset

[Tim Deegan <tim@xxxxxxx>]

Hi,

At 17:25 +0200 on 27 May (1432747540), Vitaly Kuznetsov wrote:
> New operation reassigns all memory pages from source domain to the destination
> domain mapping them at exactly the same GFNs. Pages mapped more than once 
> (e.g.
> grants) are being copied.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> ---
> Changes in v7:
> - is_soft_reset flag added to struct domain to preserve destination domain's
>   paused state across possible hypercall preemption.
> [Jan Beulich]
> - XENMEM_soft_reset -> XEN_DOMCTL_soft_reset
> - domain_soft_reset() returns int
> - no locking for is_dying as it is now proteced by domctl_lock
> - print a warning on !mfn_valid case
> - check PGC_allocated for pages
> - no print on assign_pages failure (it prints error messages on both its 
> failure paths)
> - don't re-read page->count_info, use copy_page flag
> - minor stucturing change
> - pause both source and destination domain while processing the hypercall
> - remove nr_transferred from public interface
> [Tim Deegan]
> - use get_gfn_type_access() in PoD case (x86-only)
> ---
>  xen/common/domain.c         | 186 
> ++++++++++++++++++++++++++++++++++++++++++++
>  xen/common/domctl.c         |  72 +++++++++++++++++
>  xen/include/public/domctl.h |  28 +++++++
>  xen/include/xen/sched.h     |   6 ++
>  4 files changed, 292 insertions(+)
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 7825c56..824f325 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1006,6 +1006,192 @@ int domain_unpause_by_systemcontroller(struct domain 
> *d)
>      return 0;
>  }
>  
> +int domain_soft_reset(struct domain *source_d, struct domain *dest_d,
> +                      xen_pfn_t *gmfn_start)
> +{
> +    int rc = 0;
> +    unsigned long mfn, mfn_new, gmfn, last_gmfn, count;
> +    unsigned int order;
> +    p2m_type_t p2mt;
> +    struct page_info *page, *new_page, *next_page;
> +    int drop_dom_ref, copy_page;
> +
> +    last_gmfn = domain_get_maximum_gpfn(source_d);
> +    gmfn = *gmfn_start;
> +    while ( gmfn <= last_gmfn )
> +    {
> +        page = get_page_from_gfn(source_d, gmfn, &p2mt, 0);

In order to be safe against p2m changes, you should use
get_gfn_type_access() _here_, and put_gfn() when you're finished with the
gfn.  You'll also need to get_page() the returned MFN, of course, to
make sure that it isn't freed before you reassign it.

> +        if ( unlikely(page == NULL) )
> +        {
> +#ifdef CONFIG_X86
> +            struct p2m_domain *p2m = p2m_get_hostp2m(source_d);
> +            p2m_access_t p2ma;
> +            mfn_t mfn_p2m;
> +
> +            order = 0;
> +            mfn_p2m = get_gfn_type_access(p2m, gmfn,
> +                                          &p2mt, &p2ma, 0, &order);
> +            if ( p2m_is_pod(p2mt) )
> +            {
> +                rc = guest_physmap_mark_populate_on_demand(dest_d, gmfn,
> +                                                           order);
> +                if ( unlikely(rc) )
> +                {
> +                    printk(XENLOG_G_ERR "Failed to mark Dom%d's GFN %lx"
> +                           " (order: %d) as PoD\n", source_d->domain_id,
> +                           gmfn, order);
> +                    goto fail;
> +                }
> +            }
> +            put_gfn(source_d, gmfn);
> +            gmfn += 1ul << order;
> +#else
> +            gmfn++;
> +#endif
> +            goto preempt_check;
> +        }
> +
> +        mfn = page_to_mfn(page);
> +        if ( unlikely(!mfn_valid(mfn)) )
> +        {
> +            printk(XENLOG_G_WARNING "Dom%d's GFN %lx points to invalid 
> MFN\n",
> +                   source_d->domain_id, gmfn);
> +            put_page(page);
> +            gmfn++;
> +            goto preempt_check;
> +        }
> +
> +        next_page = page;
> +        count = 0;
> +        copy_page = 0;
> +        while ( next_page && !is_xen_heap_page(next_page) &&
> +                page_to_mfn(next_page) == mfn + count )

What's the purpose of this second loop?  It doesn't seem to be doing
anything that the outer loop couldn't.

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel