On Wed, Apr 15, 2015 at 3:48 PM, Ian Campbell <ian.campb...@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. > > >
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
