Re: [Xen-devel] [V13 PATCH 1/2] pvh dom0: Add and remove foreign pages

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.05.14 at 01:51, <mukesh.rathor@xxxxxxxxxx> wrote:
> +static int atomic_write_ept_entry(ept_entry_t *entryptr, ept_entry_t new,
> +                                  int level)
> +{
> +    int rc = 0;
> +    unsigned long oldmfn = INVALID_MFN;
> +    bool_t skip_foreign = (new.mfn == entryptr->mfn &&
> +                           new.sa_p2mt == entryptr->sa_p2mt);
> +
> +    if ( level )
> +    {
> +        ASSERT(!is_epte_superpage(&new) || !p2m_is_foreign(new.sa_p2mt));
> +        write_atomic(&entryptr->epte, new.epte);
> +        goto out;
> +    }
> +
> +    if ( unlikely(p2m_is_foreign(new.sa_p2mt)) && !skip_foreign )
> +    {
> +        struct domain *fdom;
> +
> +        rc = -EINVAL;
> +        if ( !mfn_valid(new.mfn) )
> +            goto out;
> +
> +        rc = -ESRCH;
> +        fdom = page_get_owner(mfn_to_page(new.mfn));
> +        if ( fdom == NULL )
> +            goto out;
> +
> +        /* get refcount on the page */
> +        rc = -EBUSY;
> +        if ( !get_page(mfn_to_page(new.mfn), fdom) )
> +            goto out;
> +    }
> +
> +    if ( unlikely(p2m_is_foreign(entryptr->sa_p2mt)) && !skip_foreign )
> +        oldmfn = entryptr->mfn;
> +
> +    write_atomic(&entryptr->epte, new.epte);
> +
> +    if ( unlikely(oldmfn != INVALID_MFN) )
> +        put_page(mfn_to_page(oldmfn));
> +
> +    rc = 0;
> +
> + out:
> +    if ( rc )
> +        gdprintk(XENLOG_ERR, "epte o:%"PRIx64" n:%"PRIx64" rc:%d\n",
> +                 entryptr->epte, new.epte, rc);
> +    return rc;
> +}

There's still no sign of any use of is_epte_present() here, and also no
mention anywhere that taking refcounts even for inaccessible entries
is correct. I think this is actually okay, but the policy (refcount taken
even for inaccessible pages) should be spelled out somewhere.

> @@ -688,10 +748,10 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long 
> gfn, mfn_t mfn,
>          ept_p2m_type_to_flags(&new_entry, p2mt, p2ma);
>      }
>  
> -    atomic_write_ept_entry(ept_entry, new_entry);
> +    rc = atomic_write_ept_entry(ept_entry, new_entry, target);

To me it would seem cleaner to clear old_entry here right away, so
there's no confusion about it needing freeing on eventual new error
paths getting added in the future.

Jan


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