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.

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?

Thanks,

Tamas

The rest of it looked good to me.

Ian.