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

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

On Thu, 1 May 2014 18:19:08 +0200
Tim Deegan <tim@xen.org> wrote:

> Hi,
> 
> Nearly there, except that the teardown code is now gone, with nothing
> replacing it.  Ideally we'd have some logic to find active EPT L1
> tables and undo the foreign refcounts.  If not that, then:
>  - a hard-coded check, in p2m_add_foreign I suppose, that only allows
>    foreign mappings to be added to a p2m for the hardware domain,
>    with a pvh fixme comment explaining why; and
>  - another pvh fixme comment in the p2m teardown code to say that
>    once non-hardware domains can have foreign mappings there will
>    have to be some cleanup code to handle them.

Added fixmes... 

> Also, in the inner refcounting logic:
> 
> 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.  I suppose I can add it, it would be  easy
to find it anyways....  BTW, there is no path allowing 
superpage for foreign at present.

> > +        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?

thanks,
Mukesh