Re: [V10 PATCH 3/4] pvh dom0: Add and remove foreign pages

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

On Fri, 2 May 2014 10:55:55 +0200
Tim Deegan <tim@xen.org> wrote:

> Hi,
> 
> At 18:45 -0700 on 01 May (1398966318), Mukesh Rathor wrote:
> > On Thu, 1 May 2014 18:19:08 +0200
> > Tim Deegan <tim@xen.org> wrote:
.....
> > > 
> > > At 18:06 -0700 on 29 Apr (1398791207), Mukesh Rathor wrote:
> > > > diff --git a/xen/arch/x86/mm/p2m-ept.c
> > > > b/xen/arch/x86/mm/p2m-ept.c index c0bfc50..11474e8 100644
> > > > --- a/xen/arch/x86/mm/p2m-ept.c
> > > > +++ b/xen/arch/x86/mm/p2m-ept.c
> > > > @@ -36,8 +36,6 @@
> > > >  
> > > >  #define
> > > > atomic_read_ept_entry(__pepte)                              \
> > > > ( (ept_entry_t) { .epte = read_atomic(&(__pepte)->epte) } )
> > > > -#define atomic_write_ept_entry(__pepte,
> > > > __epte)                     \
> > > > -    write_atomic(&(__pepte)->epte, (__epte).epte)
> > > >  
> > > >  #define is_epte_present(ept_entry)      ((ept_entry)->epte &
> > > > 0x7) #define is_epte_superpage(ept_entry)    ((ept_entry)->sp)
> > > > @@ -46,6 +44,46 @@ static inline bool_t
> > > > is_epte_valid(ept_entry_t *e) return (e->epte != 0 &&
> > > > e->sa_p2mt != p2m_invalid); }
> > > >  
> > > > +/* returns : 0 for success, -errno otherwise */
> > > > +static int atomic_write_ept_entry(ept_entry_t *entryptr,
> > > > ept_entry_t new,
> > > > +                                  int level)
> > > > +{
> > > > +    bool_t same_mfn = (new.mfn == entryptr->mfn);
> > > > +    unsigned long oldmfn = INVALID_MFN;
> > > > +
> > > > +    if ( level )
> > > > +    {
> > > 
> > > ASSERT(!(new.sp && p2m_is_foreign(new.sa_p2mt))) here?
> > 
> > Yeah, I debated adding p2m_is_foreign ASSERT, but didn't because
> > iirc Jan had said he wanted to use up the non-leaf bits for
> > something else in future. 
> 
> Well, if new.sp is set it's not non-leaf.  Though I suppose maybe the
> test should be for _valid_ + superpage + foreign.

Ok, so looks like:

    ASSERT(new.sa_p2mt == p2m_invalid || 
           !(new.sp && p2m_is_foreign(new.sa_p2mt)));

should do it?

...

> > > > +        write_atomic(&entryptr->epte, new.epte);
> > > > +        return 0;
> > > > +    }
> > > > +
> > > > +    if ( unlikely(p2m_is_foreign(new.sa_p2mt)) && !same_mfn )
> > > > +    {
> > > > +        struct domain *fdom;
> > > > +
> > > > +        if ( !mfn_valid(new.mfn) )
> > > > +            return -EINVAL;
> > > > +
> > > > +        fdom = page_get_owner(mfn_to_page(new.mfn));
> > > 
> > > This needs a matching put_pg_owner() once you're done with it.
> > 
> > Doing page_get_owner, not get_pg_owner here, so don't think so,
> > right?
> 
> Oh yes -- sorry, I got confused between this and the case where you do
> use get_pg_owner().  But actually on this subject ISTR objecting to
> exporting get_pg_owner() from mm.c before.  Oh yes, here it is:
> 
> http://lists.xenproject.org/archives/html/xen-devel/2014-01/msg02612.html
> 
> So can you use rcu_lock_live_remote_domain_by_id() here instead?


In p2m_add_foreign() we already get the rcu lock, still do it here? 

And, btw, that function, p2m_add_foreign(), went from 

+    if ( (fdid == DOMID_XEN && (fdom = rcu_lock_domain(dom_xen)) == NULL) ||
+         (fdid != DOMID_XEN && (fdom = rcu_lock_domain_by_id(fdid)) == NULL) )
+        goto out;

to 

+    fdom = get_pg_owner(foreigndom);

(causing get_pg_owner to be exported) at Jan's comment/suggestion in v8.

thanks,
mukesh