Re: [PATCH v6 4/4] xen: rework paging_log_dirty_op to work with hvm guests

["Roger Pau Monné" <roger.pau@citrix.com>]

El 14/05/15 a les 11.53, Tim Deegan ha escrit:
> At 16:25 +0200 on 11 May (1431361528), Roger Pau Monne wrote:
>> When the caller of paging_log_dirty_op is a hvm guest Xen would choke when
>> trying to copy the dirty bitmap to the guest because the paging lock is
>> already held.
>>
>> Fix this by independently mapping each page of the guest bitmap as needed
>> without the paging lock held.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> This looks mostly correct, but OTOH we now seem to have two nested
> stop-and-retry mechsnisms in one function.  I think this would be
> cleaner if here:
> 
>> +                    if ( pages >> (3 + PAGE_SHIFT) !=
>> +                         index_mapped >> (3 + PAGE_SHIFT) )
>>                      {
>> -                        rv = -EFAULT;
>> -                        goto out;
>> +                        /* We need to map next page */
>> +                        d->arch.paging.preempt.log_dirty.i4 = i4;
>> +                        d->arch.paging.preempt.log_dirty.i3 = i3;
>> +                        d->arch.paging.preempt.log_dirty.i2 = i2;
>> +                        d->arch.paging.preempt.log_dirty.done = pages;
>> +                        paging_unlock(d);
>> +                        unmap_dirty_bitmap(dirty_bitmap, page);
> 
> we set the rest of the preempt state, like so:
> 
>         d->arch.paging.preempt.dom = current->domain;
>         d->arch.paging.preempt.op = sc->op;
> 	resuming = 1;
> 
> and bailed to the very top of the function.  I think we might also
> need a hypercall_preempt_check() here as well, so that this restart
> path doesn't stop us ever reaching the preempt_checks below.

OK, I don't mind adding a preempt check here, although I think we would
hit the ones below anyway as we go scanning the dirty bitmap even if we
have to restart.

Roger.