Re: [V4 PATCH 6/7] pvh dom0: Add and remove foreign pages

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Wed, 04 Dec 2013 17:00:57 +0000
Julien Grall <julien.grall@linaro.org> wrote:

> On 12/04/2013 01:05 AM, Mukesh Rathor wrote:
> > On Wed, 04 Dec 2013 00:00:54 +0000
> > Julien Grall <julien.grall@linaro.org> wrote:
> > 
......
> refcnt'd
> +         * the page. See also xenmem_add_foreign_to_pmap().
> +         */
> +        page = get_page_from_gfn(d, xrfp.gpfn, &p2mt, P2M_ALLOC);
> +
> +        if ( page || p2m_is_foreign(p2mt) )
>          {
> -            guest_physmap_remove_page(d, xrfp.gpfn,
> page_to_mfn(page), 0);
> -            put_page(page);
> +            if ( page )
> +                mfn = page_to_mfn(page);
> +            else
> +            {
> +                mfn = gmfn_to_mfn(d, xrfp.gpfn);
> +                page = mfn_to_page(mfn);
> +
> +                ASSERT(d != page_get_owner(page));
> +            }
> +
> +            guest_physmap_remove_page(d, xrfp.gpfn, mfn, 0);
> +            if (page)
> +                put_page(page);


Why did you drop the following release of refcnt from here?

+            if ( p2m_is_foreign(p2mt) )
+            {
+                put_page(mfn_to_page(mfn));
+                put_gfn(d, xrfp.gpfn);
+            }


>          }
>          else
>              rc = -ENOENT;
> 
> This change is assuming that:
>   - p2m_is_foreign is defined on ARM (I have a patch for that)

I added #define p2m_is_foreign to 0 in my patch for now.

>   - gmfn_to_mfn is defined on x86 (I don't have a patch, but the code
> should be trivial?)

I'd rather just ifdef CONFIG_X86 like guest_remove_page() does in the
same file for gmfn_to_mfn.

Also, lets not mix any partial arm changes with these x86 changes here.

So, in this patch, I've added p2m_is_foreign set to 0, and changed the
remove path to remain exactly same for arm. So, arm will continue to work
the same way, and no build break. This way, you can add arm change 
incrementally and we can track things nicely.

Please see the changes in the next version.

thanks,
Mukesh