Re: [V8 PATCH 5/8] pvh dom0: Add and remove foreign pages

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Mon, 24 Mar 2014 09:26:58 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 22.03.14 at 02:39, <mukesh.rathor@oracle.com> 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