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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Tim Deegan <tim@xen.org> writes:

> At 17:26 +0100 on 22 May (1432315574), Jan Beulich wrote:
>> >>> On 22.05.15 at 17:36, <vkuznets@redhat.com> wrote:
>> >>>>> On 13.05.15 at 11:49, <vkuznets@redhat.com> 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.
>
> I don't think that will work.  Given that you have to make this
> preemptable you can't hope to hold the p2m lock for the entire
> operation.
>
> If you want to make sure that the p2m mapping doesn't change
> underfoot, you can use get_gfn()/put_gfn() around each individual
> operation.

Thanks, I see... An additional concern from Jan was (I suppose) about
the safeness (or correctness) of this operation as a whole: even when
both source and destination domains are paused their mappings can be
changed by the control domain (especially having possible preemption in
mind). I'm putting the source domain to the dying state so most
hypercalls will fail (if we're not checking is_dying somewhere we
probably should) but the destination domain is alive. I'm not sure this
adds any additional risk as a misbehaving control domain is always
able to screw mappings.

>  If you use get_gfn_type_access() it will also report
> superpage mappings, so you can drop the loop that attempts
> to detect them in the PoD case.

Thanks, I'll have a look. This one is x86-specific but the whole PoD
case is already x86-specific because of p2m_is_pod().

>
> Incidentally, counting from 0-->max_mapped_gfn is unfortunate (though I
> know this is not the first code to do that).  It would be better
> to introduce an iterator over the p2m itself, either some sort of
> for_each_gfn(dom, callback_fn, callback_arg) or a get_next_gfn(dom,
> base_gfn, &found_gfn) that skips unmapped areas.

I agree. Do you see this as a compulsory part of this series?

>
> Tim.

-- 
  Vitaly