Re: [Xen-devel] [PATCH v6 04/10] xen: Introduce XENMEM_soft_reset operation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.05.15 at 17:36, <vkuznets@xxxxxxxxxx> wrote:
>>>>> On 13.05.15 at 11:49, <vkuznets@xxxxxxxxxx> wrote:
>>> +    if ( !source_d->is_dying )
>>> +    {
>>> +        /*
>>> +         * Make sure no allocation/remapping for the source domain is 
>>> ongoing
>>> +         * and set is_dying flag to prevent such actions in future.
>>> +         */
>>> +        spin_lock(&source_d->page_alloc_lock);
>>> +        source_d->is_dying = DOMDYING_locked;
>>
>> Furthermore I don't see how this prevents there being further
>> changes to the GFN <-> MFN mappings for the guest (yet you rely
>> on them to be stable below afaict).
>>
> 
> As you suggested below we can complement that by pausing both source and
> destination domains here. In that case these domains won't be changing
> their mappings by themselves but it would still be possible for the
> hypervisor to change something. We do have !d->is_dying check in many
> places though ... In theory we could have taken the p2m_lock() for both
> domains but I'm not sure all stuff I need here will cope with p2m_lock()
> already being held, this lock is x86-specific and I'm not sure we want
> it in the first place. I'd be very grateful for some additional ideas on
> how to make it safer.

For whether p2m_lock() might be needed here I'd like to defer to
Tim. As to there being changes by the hypervisor - the hypervisor
is a passive entity, i.e. unless asked by some guest to do
something, it won't do anything. Hence the question is whether
other domains (including the control one) could cause any change
here.

Also the destination domain as I understand it should have never
got un-paused before this operation. 

>>> +            if ( unlikely(source_d->tot_pages == 0) )
>>> +                drop_dom_ref = 1;
>>> +            spin_unlock(&source_d->page_alloc_lock);
>>> +            put_page(next_page);
>>> +            if ( drop_dom_ref )
>>> +                put_domain(source_d);
>>> +
>>> +            if ( unlikely(assign_pages(dest_d, next_page, 0, 0)) )
>>> +            {
>>> +                printk(XENLOG_G_INFO "Failed to assign Dom%d's MFN %lx"
>>> +                       " to Dom%d\n", source_d->domain_id, mfn,
>>> +                       dest_d->domain_id);
>>> +                rc = -EFAULT;
>>
>> A better (more distinct) error code should be used here. And I
>> suppose printing the GFN would be more helpful for debugging
>> possible issues than printing the MFN.
> 
> As I can see assign_pages() fails in two cases:
> 1) Over-allocating
> 2) Domain is dying (this is never supposed to happen as we're holding
> the lock).
> So actually both MFN and GFN are a bit irrelevant here.

Well, perhaps. To decide what to print, simply try to understand
what would be helpful to understand the reasons for the failure
without having to instrument the code right away.

Jan


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