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

[Tim Deegan <tim@xen.org>]

Hi,

At 18:44 -0800 on 12 Dec (1386870289), Mukesh Rathor wrote:
> 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.

This looks much better, thank you.  I'm afraid it's still not ready to
be committed though:

> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index 08d1d72..3b9f764 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,26 @@ 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)
> +{
> +    if ( p2m_is_foreign(entryptr->sa_p2mt) )
> +        put_page(mfn_to_page(entryptr->mfn));

The usual idiom for such things is to take the new ref first, then
drop the old one.  That way you don't have to worry about temporarily
dropping a ref to zero (yes, I know the caller ought to have a ref too
but it's better to be safe than sorry)...

> +    if ( p2m_is_foreign(new->sa_p2mt) )
> +    {
> +        struct page_info *page;
> +        struct domain *fdom;
> +
> +        ASSERT(mfn_valid(new->mfn));
> +        ASSERT(new->mfn != entryptr->mfn);

...and you won't need this assertion.

> +        page = mfn_to_page(new->mfn);
> +        fdom = page_get_owner(page);
> +        get_page(page, fdom);
> +    }
> +    write_atomic(&entryptr->epte, new->epte);
> +}

Also, the non-EPT p2m code needs to have _something_ for this, even if
it's just an error return on all p2m_foreign mappings (and a warning
to console, and some more PVH fixme comments...)

> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 0659ef1..29f7b23 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -479,6 +479,8 @@ void p2m_teardown(struct p2m_domain *p2m)
>              /* Does not fail with ENOMEM given the DESTROY flag */
>              BUG_ON(mem_sharing_unshare_page(d, gfn, MEM_SHARING_DESTROY_GFN));
>          }
> +        if ( p2m_is_foreign(t) )
> +            put_page(mfn_to_page(mfn));

That's not going to work. Just above here there's an early exit if the
domain has no shared pages.

This loop should go away anyway, since as it says in the comment above
it's a noop after the earlier teardown (and if there is work to do, a
foreach(0..max) loop is definitely the wrong thing to do).  So I'm
afraid you can't hook your teardown operation here.  You'll have to
walk the actual p2m pages.

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.

Tim.