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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.05.14 at 03:55, <mukesh.rathor@xxxxxxxxxx> wrote:
> @@ -4584,6 +4584,9 @@ int xenmem_add_to_physmap_one(
>              page = mfn_to_page(mfn);
>              break;
>          }
> +        case XENMAPSPACE_gmfn_foreign:
> +            rc = p2m_add_foreign(d, idx, gpfn, foreign_domid);
> +            return rc;

Any reason not to simply "return p2m_add_foreign();"?

> +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(!p2m_is_foreign(new.sa_p2mt));
> +        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));
> +        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;

I'm afraid this "same_mfn" handling is correct only if sa_p2mt also
does not change.

> @@ -287,14 +288,20 @@ struct page_info *get_page_from_gfn_p2m(
>          /* Fast path: look up and get out */
>          p2m_read_lock(p2m);
>          mfn = __get_gfn_type_access(p2m, gfn, t, a, 0, NULL, 0);
> -        if ( (p2m_is_ram(*t) || p2m_is_grant(*t))
> -             && mfn_valid(mfn)
> +        if ( p2m_is_any_ram(*t) && mfn_valid(mfn)
>               && !((q & P2M_UNSHARE) && p2m_is_shared(*t)) )
>          {
>              page = mfn_to_page(mfn);
> -            if ( !get_page(page, d)
> -                 /* Page could be shared */
> -                 && !get_page(page, dom_cow) )
> +            if ( p2m_is_foreign(*t) )

I think here (and possible elsewhere) use of unlikely() is desirable.

> @@ -444,6 +451,10 @@ int p2m_alloc_table(struct p2m_domain *p2m)
>      return rc;
>  }
>  
> +/*
> + * pvh fixme: when adding support for pvh non-hardware domains, this path 
> must
> + * cleanup any foreign p2m types (release refcnts on them).
> + */

And I wonder whether you shouldn't enforce this by disallowing non-
hardware domains to create foreign mappings.

> +int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
> +                    unsigned long gpfn, domid_t foreigndom)
> +{
> +    p2m_type_t p2mt, p2mt_prev;
> +    unsigned long prev_mfn, mfn;
> +    struct page_info *page;
> +    int rc = -EINVAL;
> +    struct domain *fdom = NULL;

Two pointless initializers.

Jan


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