Re: [V8 PATCH 3/8] pvh dom0: Introduce p2m_map_foreign

[George Dunlap <george.dunlap@eu.citrix.com>]

On 03/22/2014 01:39 AM, Mukesh Rathor wrote:
> In this patch, a new type p2m_map_foreign is introduced for pages
> that toolstack on an auto translated dom0 or a control domain maps
> from foreign domains that its creating or supporting during it's
> run time.
>
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> Acked-by: Tim Deegan <tim@xen.org>

Overall looks good, just a couple of comments:

> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index c0ddef0..7050f6a 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -492,7 +492,7 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn,
>           for ( i = 0; i < (1UL << page_order); i++ )
>           {
>               mfn_return = p2m->get_entry(p2m, gfn + i, &t, &a, 0, NULL);
> -            if ( !p2m_is_grant(t) && !p2m_is_shared(t) )
> +            if ( !p2m_is_grant(t) && !p2m_is_shared(t) && !p2m_is_foreign(t) )

What's the unifying charateristic of these three types?  At some point 
we should come up with a name for it, and make a function like 
"p2m_is_ram()", which returns true for all three of them.  (Not required 
for acceptance.)

> @@ -751,16 +750,31 @@ set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
>           set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
>       }
>   
> -    P2M_DEBUG("set mmio %lx %lx\n", gfn, mfn_x(mfn));
> -    rc = set_p2m_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2m_mmio_direct, p2m->default_access);
> +    P2M_DEBUG("set %d %lx %lx\n", gfn_p2mt, gfn, mfn_x(mfn));
> +    rc = set_p2m_entry(p2m, gfn, mfn, PAGE_ORDER_4K, gfn_p2mt,
> +                       p2m->default_access);
>       gfn_unlock(p2m, gfn, 0);
>       if ( 0 == rc )
>           gdprintk(XENLOG_ERR,
> -            "set_mmio_p2m_entry: set_p2m_entry failed! mfn=%08lx\n",
> +            "%s: set_p2m_entry failed! mfn=%08lx\n", __func__,
>               mfn_x(get_gfn_query_unlocked(p2m->domain, gfn, &ot)));
>       return rc;
>   }
>   
> +/* Set foreign mfn in the given guest's p2m table.
> + * Returns: True for success. */
> +static int __attribute__((unused))
> +set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)

Why "unused"?  It appears that tag remains even at the end of the series.

  -George