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

[Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>]

Tim Deegan <tim@xxxxxxx> writes:

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

The only problem I see is the fact that get_gfn_type_access() is
x86-specific. Actually, I don't see why we can't have
get_gfn_type_access() for ARM: it locks the whole p2m with gfn_lock
(which is just p2m_lock() on x86) and then resolves the gfn. I'm not
sure what should be the right approach for this series: make this
hypercall x86-specific (temporary before we have all the required bits
in ARM) or try to make something up for ARM...

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

True. This second loops searches for a continuous region to preserve the
order of mappings (when possible) but as it advances the gmfn it doesn't
bring any performance penalty. I was under an impression it makes this
code easier to read but if you think it doesn't I won't object.

Thanks,

-- 
  Vitaly

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