Re: [Xen-devel] [PATCH 7/8] xen/arm: Set foreign page type to p2m_map_foreign

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2013-12-05 at 15:42 +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>
> ---
>  xen/arch/arm/mm.c |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 960c872..bf383a7 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -977,6 +977,7 @@ static int xenmem_add_to_physmap_one(
>  {
>      unsigned long mfn = 0;
>      int rc;
> +    p2m_type_t t = p2m_ram_rw;

Can we set this explicitly on a per-mapspace basis please, so the
compiler will complain about an uninitialised variable if we forget to
add the type for a new map space, instead of silently creating writable
ram mappings.

>  
>      switch ( space )
>      {
> @@ -1019,8 +1020,8 @@ static int xenmem_add_to_physmap_one(
>          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;
> @@ -1032,15 +1033,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.

"on the foreign" (and maybe "domain's" or just "foreign page"

> +         * Refcount will be release in XENMEM_remove_from_physmap */

"will be released".

The refcount will also be removed by p2m_teardown. That probably needs
changing to do an actual walk of the p2m tearing things down, which is a
pain.

Stefano's now obsolete dma pinning series had a patch which added a
fairly generic walker in it which might be reusable.

The walk might need to support continuations though.

I wonder if this ref ought to be taken in create_p2m_entries for all
entries and not just foreign ones, and then released in the appropriate
places.

> +        page = get_page_from_gfn(od, idx, NULL, P2M_ALLOC);
> +        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;
> @@ -1051,7 +1055,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