From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Jan Beulich" <JBeulich@suse.com>
Subject: Re: [V6 PATCH 6/7] pvh dom0: Add and remove foreign
 pages
Date: Fri, 06 Dec 2013 11:46:35 +0000
Message-ID: <52A1C72B020000780010AD68@nat28.tlf.novell.com>
References: <1386297524-15483-1-git-send-email-mukesh.rathor@oracle.com>
	<1386297524-15483-7-git-send-email-mukesh.rathor@oracle.com>
	<20131205185424.5986673b@mantra.us.oracle.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta14.messagelabs.com ([193.109.254.103])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <JBeulich@suse.com>) id 1Votrj-0003ie-01
	for xen-devel@lists.xenproject.org; Fri, 06 Dec 2013 11:46:39 +0000
In-Reply-To: <20131205185424.5986673b@mantra.us.oracle.com>
Content-Disposition: inline
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Mukesh Rathor <mukesh.rathor@oracle.com>
Cc: george.dunlap@eu.citrix.com, xen-devel <xen-devel@lists.xenproject.org>, keir.xen@gmail.com, tim@xen.org
List-Id: xen-devel@lists.xenproject.org

>>> On 06.12.13 at 03:54, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> @@ -693,11 +695,42 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>              return rc;
>          }
>  
> +        /*
> +         * If autotranslate guest, (eg pvh), the gfn could be mapped to a mfn
> +         * from foreign domain by the user space tool during domain creation.
> +         * We need to check for that, free it up from the p2m, and release
> +         * refcnt on it. In such a case, page would be NULL and the following
> +         * call would not have refcnt'd the page.
> +         * See also xenmem_add_foreign_to_p2m().
> +         */
>          page = get_page_from_gfn(d, xrfp.gpfn, NULL, P2M_ALLOC);
>          if ( page )
> +            mfn = page_to_mfn(page);
> +#ifdef CONFIG_X86

I take this to mean that the code is okay for ARM now. But such a
conditional here needs explanation in a code comment, or putting
into something that's generic (i.e. "else if ()") but currently happening
to be always false for ARM.

> +        else
>          {
> -            guest_physmap_remove_page(d, xrfp.gpfn, page_to_mfn(page), 0);
> -            put_page(page);
> +            mfn = mfn_x(get_gfn_query(d, xrfp.gpfn, &p2mt));
> +            if ( p2m_is_foreign(p2mt) )
> +            {
> +                struct domain *foreign_dom;
> +
> +                foreign_dom = page_get_owner(mfn_to_page(mfn));
> +                ASSERT(is_pvh_domain(d));
> +                ASSERT(d != foreign_dom);
> +            }
> +        }
> +#endif
> +        if ( page || p2m_is_foreign(p2mt) )
> +        {
> +            guest_physmap_remove_page(d, xrfp.gpfn, mfn, 0);
> +            if ( page )
> +                put_page(page);
> +
> +            if ( p2m_is_foreign(p2mt) )
> +            {
> +                put_page(mfn_to_page(mfn));
> +                put_gfn(d, xrfp.gpfn);
> +            }

The code as it stands gives the impression that there could be two
put_page() invocations in a single run here. Based on the comment
above I assume this should never be the case though. That would
be nice to be documented via a suitable ASSERT(), or it could be
made more obvious by doing something like

        if ( page || p2m_is_foreign(p2mt) )
        {
            guest_physmap_remove_page(d, xrfp.gpfn, mfn, 0);
            if ( p2m_is_foreign(p2mt) )
            {
                page = mfn_to_page(mfn);
                put_gfn(d, xrfp.gpfn);
            }
            put_page(page);
        }

Jan