[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [V8 PATCH 5/8] pvh dom0: Add and remove foreign pages
On Mon, 24 Mar 2014 09:26:58 +0000 "Jan Beulich" <JBeulich@xxxxxxxx> wrote: > >>> On 22.03.14 at 02:39, <mukesh.rathor@xxxxxxxxxx> wrote: > > +static inline void atomic_write_ept_entry(ept_entry_t *entryptr, > > + const ept_entry_t *new) > > +{ > > + unsigned long oldmfn = 0; > > + > > + if ( p2m_is_foreign(new->sa_p2mt) ) > > + { > > + struct page_info *page; > > + struct domain *fdom; > > + > > + ASSERT(mfn_valid(new->mfn)); > > + page = mfn_to_page(new->mfn); > > + fdom = page_get_owner(page); > > + get_page(page, fdom); > > I'm afraid the checking here is too weak, or at least inconsistent > (considering the ASSERT()): mfn_valid() isn't a sufficient condition > for page_get_owner() to return non-NULL or get_page() to > succeed. If all callers are supposed to guarantee this, then further > ASSERT()s should be added here. If not, error checking is going to Correct, callers pretty much guarantee that. There are stringent checks done in p2m_add_foreign. How about: ASSERT(mfn_valid(new->mfn)); page = mfn_to_page(new->mfn); fdom = page_get_owner(page); ASSERT(fdom); rc = get_page(page, fdom); ASSERT(rc == 0); > be necessary (and possibly the ASSERT() then also needs to be > converted to an error check). > > I also wonder whether page_get_owner_and_reference() wouldn't > be more appropriate to be used here. Could. get_page() does an extra check (owner == domain) for free. But either way. Tim, preference? > > @@ -275,14 +276,19 @@ struct page_info *get_page_from_gfn_p2m( > > /* Fast path: look up and get out */ > > p2m_read_lock(p2m); > > mfn = __get_gfn_type_access(p2m, gfn, t, a, 0, NULL, 0); > > - if ( (p2m_is_ram(*t) || p2m_is_grant(*t)) > > + if ( (p2m_is_ram(*t) || p2m_is_grant(*t) || > > p2m_is_foreign(*t) ) > > Would this perhaps better become something like p2m_is_any_ram()? yes. p2m_remove_page has similar (inverted) check. will do. > (I'm in fact surprised this is only needed in exactly one place.) Looks like it could be added to set_mmio_p2m_entry to make sure mmio is not mapped at foreign mapping, and perhaps guest_physmap_add_entry. Other places, shadow and p2m-pt, has no support at present. > > > +int p2m_add_foreign(struct domain *tdom, unsigned long fgfn, > > + unsigned long gpfn, domid_t fdid) > > +{ > > + int rc = -ESRCH; > > + p2m_type_t p2mt, p2mt_prev; > > + unsigned long prev_mfn, mfn; > > + struct page_info *page = NULL; > > + struct domain *fdom = NULL; > > + > > + /* xentrace mapping pages from xen heap are owned by DOMID_XEN > > */ > > + if ( (fdid == DOMID_XEN && (fdom = rcu_lock_domain(dom_xen)) > > == NULL) || > > + (fdid != DOMID_XEN && (fdom = > > rcu_lock_domain_by_id(fdid)) == NULL) ) > > + goto out; > > Didn't you mean to call get_pg_owner() here, at once taking care of > DOMID_IO too? Also I don't think the reference to xentrace is really > useful here - DOMID_XEN owned pages certainly exist elsewhere too. IIRC, I think I didn't because it will let you get away with DOMID_SELF. I could just check for it before calling get_pg_owner: if ( fdid == DOMID_SELF ) goto out with -EINVAL; else if ( (fdom = get_pg_owner(fdid)) == NULL ) goto out with -ESRCH; .. what do you think? > Of course the question is whether for the purposes you have here > DOMID_XEN/DOMID_IO owned pages are actually validly making it > here. Yes. xentrace running on pvh dom0 wants to map xen heap pages. You mentioned there are possibly other users. I'm not familiar with DOMID_IO use case at this point. thanks for your time. Mukesh _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |