Re: [PATCH RFC/WIPv2 1/6] Introduce XENMEM_transfer operation

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 24/09/14 15:20, Vitaly Kuznetsov wrote:
> New operation reassigns pages from one domain to the other mapping them
> at exactly the same GFNs in the destination domain. Pages mapped more
> than once (e.g. granted pages) are being copied.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  xen/common/memory.c         | 178 ++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/public/memory.h |  32 +++++++-
>  2 files changed, 209 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 2e3225d..653e117 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -578,6 +578,180 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>      return rc;
>  }
>  
> +static long memory_transfer(XEN_GUEST_HANDLE_PARAM(xen_memory_transfer_t) arg)
> +{
> +    long rc = 0;
> +    struct xen_memory_transfer trans;
> +    struct domain *source_d, *dest_d;
> +    unsigned long mfn, gmfn, last_gmfn;
> +    p2m_type_t p2mt;
> +    struct page_info *page, *new_page;
> +    char *sp, *dp;
> +    int copying;
> +
> +    if ( copy_from_guest(&trans, arg, 1) )
> +        return -EFAULT;
> +
> +    source_d = rcu_lock_domain_by_any_id(trans.source_domid);
> +    if ( source_d == NULL )
> +    {
> +        rc = -ESRCH;
> +        goto fail_early;
> +    }
> +
> +    if ( source_d->is_dying )
> +    {
> +        rc = -EINVAL;
> +        rcu_unlock_domain(source_d);
> +        goto fail_early;
> +    }
> +
> +    dest_d = rcu_lock_domain_by_any_id(trans.dest_domid);
> +    if ( dest_d == NULL )
> +    {
> +        rc = -ESRCH;
> +        rcu_unlock_domain(source_d);
> +        goto fail_early;
> +    }
> +
> +    if ( dest_d->is_dying )
> +    {
> +        rc = -EINVAL;
> +        goto fail;
> +    }
> +
> +    last_gmfn = trans.gmfn_start + trans.gmfn_count;
> +    for ( gmfn = trans.gmfn_start; gmfn < last_gmfn; gmfn++ )
> +    {
> +        page = get_page_from_gfn(source_d, gmfn, &p2mt, 0);
> +        if ( !page )
> +        {
> +            continue;
> +        }
> +
> +        mfn = page_to_mfn(page);
> +        if ( !mfn_valid(mfn) )
> +        {
> +            put_page(page);
> +            continue;
> +        }
> +
> +        copying = 0;
> +
> +        if ( is_xen_heap_mfn(mfn) )
> +        {
> +            put_page(page);
> +            continue;
> +        }
> +
> +        /* Page table always worth copying */
> +        if ( (page->u.inuse.type_info & PGT_l4_page_table) ||
> +             (page->u.inuse.type_info & PGT_l3_page_table) ||
> +             (page->u.inuse.type_info & PGT_l2_page_table) ||
> +             (page->u.inuse.type_info & PGT_l1_page_table) )
> +            copying = 1;

How can copying pagetables like this ever work?  You will end up with an
L4 belonging to the new domain pointing to L3's owned by the old domain.

Even if you change the ownership of the pages pointed to by the L1's, as
soon as the old domain is torn down, the new domains pagetables will be
freed heap pages.

~Andrew