Re: [Xen-devel] [V6 PATCH 6/7] pvh dom0: Add and remove foreign pages

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.12.13 at 03:54, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> 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


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