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

[Tim Deegan <tim@xxxxxxx>]

Hi,

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.

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?

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

Cheers,

Tim.

> +        if ( fdom == NULL )
> +            return -ESRCH;
> +
> +        /* get refcount on the page */
> +        if ( !get_page(mfn_to_page(new.mfn), fdom) )
> +            return -EBUSY;
> +    }
> +
> +    if ( unlikely(p2m_is_foreign(entryptr->sa_p2mt)) && !same_mfn )
> +        oldmfn = entryptr->mfn;
> +
> +    write_atomic(&entryptr->epte, new.epte);
> +
> +    if ( unlikely(oldmfn != INVALID_MFN) )
> +        put_page(mfn_to_page(oldmfn));
> +
> +    return 0;
> +}
> +

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