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

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

On Mon, 05 May 2014 11:56:05 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 02.05.14 at 03:55, <mukesh.rathor@oracle.com> wrote:
> > @@ -4584,6 +4584,9 @@ int xenmem_add_to_physmap_one(
> >              page = mfn_to_page(mfn);
> >              break;
> >          }
> > +        case XENMAPSPACE_gmfn_foreign:
> > +            rc = p2m_add_foreign(d, idx, gpfn, foreign_domid);
> > +            return rc;
> 
> Any reason not to simply "return p2m_add_foreign();"?

Could, rc allows quick adding of printk for debugging. But, 
I will change it.

> > +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(!p2m_is_foreign(new.sa_p2mt));
> > +        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));
> > +        if ( fdom == NULL )
> > +            return -ESRCH;
> > +
> > +        /* get refcount on the page */
> > +        if ( !get_page(mfn_to_page(new.mfn), fdom) )
> > +            return -EBUSY;
> > +    }
> > +
> > +    if ( unlikely(p2m_is_foreign(entryptr->sa_p2mt)) && !same_mfn )
> > +        oldmfn = entryptr->mfn;
> 
> I'm afraid this "same_mfn" handling is correct only if sa_p2mt also
> does not change.

Yup, my bad. Will fix it, thanks.

> > @@ -287,14 +288,20 @@ 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))
> > -             && mfn_valid(mfn)
> > +        if ( p2m_is_any_ram(*t) && mfn_valid(mfn)
> >               && !((q & P2M_UNSHARE) && p2m_is_shared(*t)) )
> >          {
> >              page = mfn_to_page(mfn);
> > -            if ( !get_page(page, d)
> > -                 /* Page could be shared */
> > -                 && !get_page(page, dom_cow) )
> > +            if ( p2m_is_foreign(*t) )
> 
> I think here (and possible elsewhere) use of unlikely() is desirable.

Ok.

> > @@ -444,6 +451,10 @@ int p2m_alloc_table(struct p2m_domain *p2m)
> >      return rc;
> >  }
> >  
> > +/*
> > + * pvh fixme: when adding support for pvh non-hardware domains,
> > this path must
> > + * cleanup any foreign p2m types (release refcnts on them).
> > + */
> 
> And I wonder whether you shouldn't enforce this by disallowing non-
> hardware domains to create foreign mappings.

Hmm... Tim wanted the enforcement. That will ensure the cleanup is
implemented without falling thru the cracks.

thanks
Mukesh