Re: [V6 PATCH 6.1/7] pvh dom0: Add and remove foreign pages

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

On Mon, 9 Dec 2013 13:11:49 +0100
Tim Deegan <tim@xen.org> wrote:

> Hi,
> 
> At 18:34 -0800 on 06 Dec (1386351256), Mukesh Rathor wrote:
> > In this patch, a new function, xenmem_add_foreign_to_p2m(), is added
> > to map pages from foreign guest into current dom0 for domU creation.
> > Such pages are typed p2m_map_foreign. Another function
> > xenmem_rem_foreign_from_p2m() is added to remove such pages. Note,
> > in the remove path, we must release the refcount that was taken
> > during the map phase.
> > 
> > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> [...]
> > +/*
> > + * Add frames from foreign domain to target domain's physmap.
> > Similar to
> > + * XENMAPSPACE_gmfn but the frame is foreign being mapped into
> > current,
> > + * and is not removed from foreign domain.
> > + * Usage: libxl on pvh dom0 creating a guest and doing
> > privcmd_ioctl_mmap.
> > + * Side Effect: the mfn for fgfn will be refcounted so it is not
> > lost
> > + *              while mapped here. The refcnt is released in
> > do_memory_op()
> > + *              via XENMEM_remove_from_physmap.
> 
> Is that comment out of date?  AFAICS the put_page() happens...

yup.

> > +/* Note, the refcnt released here is taken in
> > xenmem_add_foreign_to_p2m */ +int
> > xenmem_rem_foreign_from_p2m(struct domain *d, unsigned long gpfn) +{
> > +    unsigned long mfn;
> > +    p2m_type_t p2mt;
> > +    struct domain *foreign_dom;
> > +
> > +    mfn = mfn_x(get_gfn_query(d, gpfn, &p2mt));
> > +    if ( !mfn_valid(mfn) )
> > +    {
> > +        gdprintk(XENLOG_WARNING, "Invalid mfn for gpfn:%lx
> > domid:%d\n",
> > +                 gpfn, d->domain_id);
> > +        return -EINVAL;
> > +    }
> > +
> > +    foreign_dom = page_get_owner(mfn_to_page(mfn));
> > +    ASSERT(d != foreign_dom);
> > +    ASSERT(is_pvh_domain(d));
> > +
> > +    guest_physmap_remove_page(d, gpfn, mfn, 0);
> > +    put_page(mfn_to_page(mfn));
> 
> ...here, and doesn't look safe.  This put_page() is to balance the
> get_page() in xenmem_add_foreign_to_p2m() but (a) you haven't checked
> here that the entry you're removing is actually a foreign one and (b)
> you haven't updated any of the other paths that might clear a p2m
> entry that contained a foreign mapping.

(a)The function is only called for foreign currently, but good idea
to add a check now that this code is in a public function.

(b) right, i missed that. trying to figure places where i'd need to do
that. Looks like i could just do that in p2m_remove_page.

> I think the refcounting will have to be done at the bottom of the
> arch-specific implementation, where the actual p2m entry gets set or
> cleared.

Hmm... in the add path, need to get refcnt before removing previous
mapping, so i do get_page*, then remove prev mapping, then set p2m.

In the remove path, perhaps the put_page() could be moved to p2m_remove_page
thereby benefitting (b) above, but we miss the symmetry with add 
path. So either way... lmk.

thanks
Mukesh