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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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@xxxxxxxxxx> 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).

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

Ian.


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