Re: [Xen-devel] [PATCH v2 09/10] xen/arm: Set foreign page type to p2m_map_foreign

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> Xen needs to know that the current page belongs to another domain. Also take
> the refcount to this page.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> 
> ---
>     Changes in v2:
>         - Even if gcc is buggy (see 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501)
>         define p2m type per mapspace to let the compiler warns about 
> unitialized
>         values.
> ---
>  xen/arch/arm/mm.c |   24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index ba51f6e..b08ffa0 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -993,6 +993,7 @@ static int xenmem_add_to_physmap_one(
>  {
>      unsigned long mfn = 0;
>      int rc;
> +    p2m_type_t t;
>  
>      switch ( space )
>      {
> @@ -1025,18 +1026,22 @@ static int xenmem_add_to_physmap_one(
>          
>          d->arch.grant_table_gpfn[idx] = gpfn;
>  
> +        t = p2m_ram_rw;
> +
>          spin_unlock(&d->grant_table->lock);
>          break;
>      case XENMAPSPACE_shared_info:
> -        if ( idx == 0 )
> -            mfn = virt_to_mfn(d->shared_info);
> -        else
> +        if ( idx != 0 )
>              return -EINVAL;
> +
> +        mfn = virt_to_mfn(d->shared_info);
> +        t = p2m_ram_rw;
> +
>          break;
>      case XENMAPSPACE_gmfn_foreign:
>      {
> -        paddr_t maddr;
>          struct domain *od;
> +        struct page_info *page;
>          od = rcu_lock_domain_by_any_id(foreign_domid);
>          if ( od == NULL )
>              return -ESRCH;
> @@ -1048,15 +1053,18 @@ static int xenmem_add_to_physmap_one(
>              return rc;
>          }
>  
> -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
> -        if ( maddr == INVALID_PADDR )
> +        /* Take refcount to the foreign domain page.

"refcount on...". Or "Take a reference to..."

> +         * Refcount will be release in XENMEM_remove_from_physmap */

and a few other places...

Like with the unmap it might be best to push this reference manipulation
down into the eventual function which makes the mapping. That would keep
this stuff much more localised.

> +        page = get_page_from_gfn(od, idx, NULL, P2M_ALLOC);

This made me wonder what happens if the guest drops its mapping while
the foreign one is around. At least the page won't be freed until the
foreign mapping goes away, but it won't be what the tools think it is.

I think this is probably OK, or at least I'm not sure right now what we
could do about it.

> +        if ( !page )
>          {
>              dump_p2m_lookup(od, pfn_to_paddr(idx));
>              rcu_unlock_domain(od);
>              return -EINVAL;
>          }
>  
> -        mfn = maddr >> PAGE_SHIFT;
> +        mfn = page_to_mfn(page);
> +        t = p2m_map_foreign;
>  
>          rcu_unlock_domain(od);
>          break;
> @@ -1067,7 +1075,7 @@ static int xenmem_add_to_physmap_one(
>      }
>  
>      /* Map at new location. */
> -    rc = guest_physmap_add_page(d, gpfn, mfn, 0);
> +    rc = guest_physmap_add_entry(d, gpfn, mfn, 0, t);
>  
>      return rc;
>  }



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