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

[Tim Deegan <tim@xxxxxxx>]

At 12:58 +0200 on 14 May (1431608313), Roger Pau Monné wrote:
> 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@xxxxxxxxxx>
> >
> > 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.

Oh, so we do.  No need for the extra check here, then.  But I'd still
prefer restarting the whole function to having a second restart point.

Cheers,

Tim.

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