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

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 12/16/2013 11:27 PM, Mukesh Rathor wrote:
> On Mon, 16 Dec 2013 08:40:41 +0000
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>
> > > > > On 14.12.13 at 03:48, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> > > > > 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@xxxxxxxxxx>
> ---
>   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.
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.

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

Cheers,

--
Julien Grall

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