Re: [Xen-devel] [PATCH RFC 1/2] xen/pvh: take the p2m lock when doing logdirty ops from HVM domains

[Tim Deegan <tim@xxxxxxx>]

At 12:03 +0200 on 16 Oct (1413457382), Roger Pau Monné wrote:
> El 16/10/14 a les 11.20, Tim Deegan ha escrit:
> > Ah, I see.  This is the _caller_'s p2m lock but the _target_'s paging
> > lock.  Holding the caller's p2m lock to unstick this seems a bit of a
> > strange answer -- the paging op might be quite a long one.  And having
> > all vcpus take their own p2m locks before remote paging locks (and
> > probably other MM locks too operations) is going to be quite messy.
> >
> > I guess the tricky path is clear_user_hvm, writing the bitmap back to
> > the caller's memory.  I wonder whether we could use unlocked p2m
> > lookups for that.  Probably not, because what if the caller's memory is
> > PoD, etc?
> 
> Yes, the functions that need the caller p2m lock is
> clear_user_hvm/copy_to_user_hvm. If I'm not mistaken we explicitly
> stated that PVH is not going to use PoD, but since we are there we can
> try to fix this function so it can work with pure HVM domains that can
> indeed use PoD.
> 
> > Getting hold of all the bitmap pages before taking the lock would be
> > messy too.
> >
> > So this may end up being the least bad answer but I'd like to see if
> > we can think of something better first.
> 
> I'm certainly open to ideas, this was the naive way I've found to fixing it.

Sorry for the delay getting back to this -- I'm aware that I
discouraged this patch without suggesting an alternative!

I've looked through the hypervisor for callers of copy_to_guest() and
similar functions and they're almost all either:
 - at the head or tail of a hypercall; or
 - in code which doesn't touch any other domain (e.g. ctxt switch).

The only interesting cases are:
 - paging_log_dirty_op(), which as you pointed out writes the bitmap
   back to the caller while still holding the target's paging lock.
 - shadow_track_dirty_vram(), likewise.
 - hap_track_dirty_vram(), which allocates a local buffer to build
   the bitmap in and then writes it back after dropping the lock
   (and so should be OK).
 - XEN_DOMCTL_getmemlist, which has a similar issue with
   d->page_alloc_lock, for which reason it's already restricted to
   non-paging-mode-external guests (and so also OK).

shadow_track_dirty_vram() can probably be made to behave like
hap_track_dirty_vram() -- at the moment it needs the lock because it's
copying from a shared master bitmap, but it could either copy it twice
(ugh) or do some RCU-like thing to allow the copies to happen outside
the paging lock.

That leaves paging_log_dirty_op().  The inner loops of that function
are already set up to be preempted for softirqs, &c.  I wonder could
we arrange for it to map the first N pages of bitmap before taking any
locks, and then drop the locks after that many pages, and unmap them
again.

It wouldn't even need to actually set up a hypercall continuation if 
!hypercall_preempt_check(); it can simply map a new batch and carry
on.

Does that sounds plausible?  If so, then we can leave the lock
constraints as they are, which would make me very happy. :)

Cheers,

Tim.

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