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

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

On Mon, 16 Dec 2013 15:27:22 -0800
Mukesh Rathor <mukesh.rathor@oracle.com> wrote:

> On Mon, 16 Dec 2013 08:40:41 +0000
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
> > >>> On 14.12.13 at 03:48, Mukesh Rathor <mukesh.rathor@oracle.com>
> > >>> wrote:
> > >> Also, Jan may have an opinion about whether a teardown operation
> > >> that has to walk each p2m entry would have to be made
> > >> preemptible.  I'm not sure where we draw the line on such things.
> > > 
> > > Since at present teardown cleanup of foreign is not really that
> > > important as its only applicable to dom0, let me submit another
> > > patch for it on Mon with few ideas. That would also keep this
> > > patch size reasonable, and keep you from having to look at the
> > > same code over and over. 
> > > 
> > > So, please take a look at the version below with above two fixes.
> > > If you approve it, i can resubmit the entire series rebased to
> > > latest with your ack on Monday, and the series can go in while we
> > > resolve the p2m teardown.
> > 
> > Going through the patch again, I'm not seeing any loop being
> > added. Am I missing something here?
> 
> Yes. Since the destruction of p2m leaking foreign pages only applies
> to control domain being destroyed, i don't think it is that critical
> that part get into 4.4. So, I'm submitting a separate patch for it,
> like said above.
> 
> > > --- 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,25 @@ static inline bool_t is_epte_valid(ept_entry_t
> > > *e) return (e->epte != 0 && e->sa_p2mt != p2m_invalid);
> > >  }
> > >  
> > > +static inline void write_ept_entry(ept_entry_t *entryptr,
> > > ept_entry_t *new)
> > 
> > So why do you drop the "atomic_" prefix here?
> 
> To distinguish it from the older atomic_* macro which did nothing but
> atomically write the entry. But if it helps get your approval, I added
> atomic prefix. 
> 
> > Also the second parameter could be "const"...
> 
> Ok.
> 
> Final version below:

Please ignore this, sending V7 with changes to this again after 
comments from Julien.

thanks
mukesh