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

[Tim Deegan <tim@xxxxxxx>]

Hi,

At 18:45 -0700 on 01 May (1398966318), Mukesh Rathor wrote:
> On Thu, 1 May 2014 18:19:08 +0200
> Tim Deegan <tim@xxxxxxx> wrote:
> > 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... 

Thanks.

> > 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. 

Well, if new.sp is set it's not non-leaf.  Though I suppose maybe the
test should be for _valid_ + superpage + foreign.

> 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.

Yes, otherwise the ASSERT would be bogus. :)  But it will be useful at
least as documentation if someone adds such a path.

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

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel