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

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

On Mon, 16 Dec 2013 23:44:37 +0000
Julien Grall <julien.grall@linaro.org> wrote:

> 
> 
> On 12/16/2013 11:27 PM, Mukesh Rathor 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:
> >
> > thanks
> > Mukesh
> > ---------------------
> >
> > In this patch, a new function, p2m_add_foreign(), is added
> > to map pages from foreign guest into current dom0 for domU creation.
> > Such pages are typed p2m_map_foreign. Another function
> > p2m_remove_foreign() is added to remove such pages. Note, it is the
> > nature of such pages that a refcnt is held during their stay in the
> > p2m. The refcnt is added and released in the low level ept code for
> > convenience. The cleanup of foreign pages from p2m upon it's
> > destruction, is submitted subsequently under a separate patch.
> >
> > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> > ---
> >   xen/arch/x86/mm.c         |   23 +++++++---
> >   xen/arch/x86/mm/p2m-ept.c |   30 +++++++++++---
> >   xen/arch/x86/mm/p2m-pt.c  |    9 ++++-
> >   xen/arch/x86/mm/p2m.c     |   96
> > +++++++++++++++++++++++++++++++++++++++++++++
> > xen/common/memory.c       |   12 +++++- xen/include/asm-arm/p2m.h
> > |    8 +++- xen/include/asm-x86/p2m.h |    7 +++
> >   7 files changed, 169 insertions(+), 16 deletions(-)
> 
> Following the discussion we had on ARM thread (see 
> https://patches.linaro.org/22361/), the approach is to remove
> specific patch for p2m foreign on common code. So get_page_from_gfn
> must handle reference on foreign mapping.

Fine, I'll just cleanup and move that to guest_physmap.... it's getting
a bit frustrating now.....

> The code is pretty simple on ARM, see:
> https://patches.linaro.org/22536/ and I don't see why this kind of
> modification can't go on x86 part.

It can, but previously it was not deemed necessary. 

> Also, can you remove all ARM specific code in this patch?

Not until your stuff is checked in.