Re: get_gfn_query() locking

[Tim Deegan <tim@xen.org>]

At 10:53 -0400 on 30 Oct (1351594401), Andres Lagar-Cavilla wrote:
> >> And then again, with the p2m lock being recursive these
> >> days, I don't think there's any harm calling the other methods
> >> here with that lock held.
> 
> Is the patch you refer to http://www.gossamer-threads.com/lists/xen/devel/261025 and the hunk in question the following?
> + get_gfn_query(d, pfn, &pt); 
> + p2m_change_type(d, pfn, pt, p2m_ram_broken); 
> + put_gfn(d, pfn); 
> 
> There really is no way to get rid of that p2m lock-protected critical
> section if the domain allows for paging etc. You might want to
> introduce a syntactically cleaner unconditional p2m_change_type
> variant that doesn't cmpxchg with the previous type -- that is
> effectively what goes on here. Should be a tiny amount of refactoring
> and the code will be cleaner, no need for query or put.

I don't think that change-type is even what's wanted here.  You want to
use some more raw form of set_p2m_entry(), since keeping the MFN is not
important.  How about:

 guest_physmap_add_entry(d, pfn, MFN_INVALID, p2m_ram_broken);

?

> > 
> > True, but it wouldn't be safe to call it with the paging lock held.
> > OTOH since we're not seeing any crashes from the lock-ordering
> > constraints maybe we don't do that.
> > 
> > Andres, what do you think?  Can we just drop/amend that comment?
> > 
> 
> If you refer to removing the ordering constraints

Noooooooooooooo! :)

I think we should drop the comment that says it's OK to call
get_gfn_query() with the paging lock held (and audit to check that we
don't do that anywhere).

Tim.