Re: [PATCH V14 3/7] xen/arm: Allow hypervisor access to mem_access protected pages

[Tim Deegan <tim@xen.org>]

At 16:45 +0100 on 15 Apr (1429116345), Ian Campbell wrote:
> On Wed, 2015-04-15 at 17:36 +0200, Tamas K Lengyel wrote:
> > 
> > 
> > On Wed, Apr 15, 2015 at 3:48 PM, Ian Campbell
> > <ian.campbell@citrix.com> wrote:
> >         On Thu, 2015-03-26 at 23:05 +0100, Tamas K Lengyel wrote:
> >         > @@ -1209,6 +1306,10 @@ struct page_info
> >         *get_page_from_gva(struct domain *d, vaddr_t va,
> >         >
> >         >  err:
> >         >      spin_unlock(&p2m->lock);
> >         > +
> >         > +    if ( !page && p2m->mem_access_enabled )
> >         > +        page = p2m_mem_access_check_and_get_page(va,
> >         flags);
> >         
> >         Is this safe/correct to do without continuing to hold the p2m
> >         lock?
> >         
> >         It seems like the result of gva_to_ipa in the new function
> >         perhaps ought
> >         to be? Not sure about the p2m_get_mem_access (or does it have
> >         its own
> >         lock? Should it?)
> > 
> > 
> > p2m_get_mem_access does lock p2m->lock before it queries the radix
> > tree. There is another p2m_lookup in p2m_mem_access_check_and_get_page
> > which also does its own locking.
> 
> Understood, but my concern was whether each of those needs to see a
> consistent p2m, or whether we can tolerate it changing and giving a
> different result as we progress through the options.
> 
> >         The case I'm thinking about is something else (grant ops etc)
> >         changing
> >         the p2m between the first check in get_page_from_gva and this
> >         one. Worst
> >         case would be spurious results from a race, which perhaps are
> >         tolerable?
> > 
> > 
> > I'm not sure. Taking and releasing the lock doesn't seem very
> > efficient for sure and I guess there could be some race conditions
> > there.. Changing it however would require an extra flag to be sent to
> > p2m_get_mem_access and p2m_lookup to forgo their own locking because
> > the caller already holds the lock. It shouldn't be too drastic of a
> > change, but any thoughts on it?
> 
> Taking p2m_lookup as an example I think the usual approach would be for
> __p2m_lookup become an unlocked version and for p2m_lookup become a
> simple wrapper which takes the lock.
> 
> __p2m_lookup could presumably be static, at least to start with.
> 
> Other options would be to push the locking into all the callers
> (probably not nice) or to go the x86 route and essentially have a
> lock/ref on the pte entry itself and use p2m_get/put_pte instead of
> p2m_lookup (at least, that's my limited understanding).

Yep.  The lock is nominally per-entry but in fact a single lock over
the whole p2m.  And the preferred interface is get_gfn()/put_gfn().

> I think x86 does it that way mainly for page sharing and friends.

Yes, but it adds a measure of sanity to any state-machine-style p2m
changes.  The alternative is to use CAS operations for every p2m
update, with appropriate unwinding in each caller if they lose the
race.

Cheers,

Tim.