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

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

On Wed, 11 Dec 2013 15:29:03 +0100
Tim Deegan <tim@xen.org> wrote:

> At 17:47 -0800 on 10 Dec (1386694075), Mukesh Rathor wrote:
> > On Wed, 11 Dec 2013 01:35:08 +0000
> > Julien Grall <julien.grall@linaro.org> wrote:
> > 
> > > >       unsigned long mfn = gmfn_to_mfn(d, gfn);
> > > >
> > > > -    ASSERT(t == NULL);
> > > > +    ASSERT(*t == INT_MAX);
> > > 
> > > There is various place where get_page_from_gfn where t == NULL.
> > > With this solution it will segfault every time.
> > > 
> > > I would do something like that:
> > >    if (*t)
> > >      t = INT_MAX;
> > 
> > here's updated:
> > ------------
> > 
> > In this patch, a new function, p2m_add_foreign(), is added
> > to map pages from foreign guest into current dom0 for domU creation.
> > Such pages are typed p2m_map_foreign. Another function
> > p2m_remove_foreign() is added to remove such pages. Note, in
> > the remove path, we must release the refcount that was taken during
> > the map phase. This is done in p2m_remove_page, which also addresses
> > releasing of refcnt when the domain is destroyed.
> 
> Did you test that?  I don't think it can be true.

Yes. In this version, I had added code to p2m_remove_page() to do that.

> Maybe I wasn't clear last time: this refcount is effectively held by
> the presence of a foreign mapping in a p2m entry.  AFAICT the only
> properly safe way to make sure that broken guest/tools behaviour can't
> mess up Xen's internal refcounting is to have the ref be taken and
> dropped at the time that the entry itelf is written/replaced, e.g.
> ept_set_entry() (or maybe atomic_write_ept_entry()) on EPT and
> paging_write_p2m_entry() on NPT/shadow.

Ah, I was fixated on thinking only p2m_add_foreign was ever gonna
add p2m foreign. Hmm... a bit worried with all the p2m locking in p2m
path and me doing get_page* in ept_set_entry().... But, may be we'll be
ok. Looking at the code to refresh all the locking in my brain....

> Trying to find all the higher-level operations that might cause
> foreign mappings to be inserted/removed is going to be difficult and
> fragile.

Yeah, i found that out staring at the code.

> You'll also need to handle domain teardown, which right now just frees
> all the memory holding the p2m tables (see p2m_teardown()).  That will
> need somehow to check those tables for valid foreign mappings and DTRT
> about them.

Ok, I was thinking since this is dom0 if p2m is tearing down, nothing 
to worry about.  But, with control domains, and all that, we'd need to 
take care of the teardown path. So, I'll fix it.

I'll have another version out hopefully tomorrow, with
get_page* and put_page* in ept path, and p2m_teardown fixed up, and all
tested. I'm thinking something along the lines of:

ept_set_entry():
   ...
   if (p2mt == foreign)
   {
       page = mfn_to_page(mfn);
       fdom = page_get_owner(page);
       get_page(page, fdom);
   }
   table = map_domain_page(pagetable_get_pfn(p2m_get_pagetable(p2m)));
   .....


thanks a lot,
Mukesh