Re: [Xen-devel] [PATCH v4 08/11] xen/arm: Handle remove foreign mapping

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-12-16 at 16:26 +0000, Julien Grall wrote:
> 
> On 12/16/2013 03:40 PM, Ian Campbell wrote:
> > On Mon, 2013-12-16 at 15:34 +0000, Julien Grall wrote:
> >>
> >> On 12/16/2013 11:51 AM, Tim Deegan wrote:
> >>> At 19:37 +0000 on 13 Dec (1386959858), Julien Grall wrote:
> >>>> @@ -693,12 +694,21 @@ long do_memory_op(unsigned long cmd, 
> >>>> XEN_GUEST_HANDLE_PARAM(void) arg)
> >>>>                return rc;
> >>>>            }
> >>>>
> >>>> -        page = get_page_from_gfn(d, xrfp.gpfn, NULL, P2M_ALLOC);
> >>>> +        /*
> >>>> +         * 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.
> >>>> +         */
> >>>> +        page = get_page_from_gfn(d, xrfp.gpfn, &p2mt, P2M_ALLOC);
> >>>>            if ( page )
> >>>>            {
> >>>>                guest_physmap_remove_page(d, xrfp.gpfn, 
> >>>> page_to_mfn(page), 0);
> >>>>                put_page(page);
> >>>>            }
> >>>> +        else if ( p2m_is_foreign(p2mt) )
> >>>> +            rc = p2m_remove_foreign(d, xrfp.gpfn);
> >>>
> >>> This doesn't seem like the right interface -- having special cases
> >>> like this in the callers is something we slipped into in x86 for a lot
> >>> of the paging/sharing code and it's not nice.  I think maybe we can
> >>> just have get_page_from_gfn() DTRT for foreign (and grant) entries.
> >>>
> >>> Also, the comment will have been out of data by the time the x86
> >>> version of this code is finished, as we won't be handling the refcount
> >>> at this level. :)
> >>
> >> I will remove the comment and modify get_page_from_gfn to handle foreign
> >> mapping.
> > 
> > You'll want to coordinate with Mukesh on that latter I think.
> > 
> > Ian.
> > 
> > 
> 
> I have reworked this patch. I get a simpler patch:
> 
> commit aab2e5d2ae7d0fa87c74cae2f22044f87be33f70
> Author: Julien Grall <julien.grall@xxxxxxxxxx>
> Date:   Fri Dec 13 16:51:03 2013 +0000
> 
>     xen/arm: Handle remove foreign mapping
>     
>     Modify get_page_from_gfn to take reference on foreign mapping. This will 
> avoid
>     specific handling in the common code.
>     
>     Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
>     
>     ---
>         Changes in v5:
>             - Remove specific p2m handling in common code
>             - Handle foreign mapping in get_page_from_gfn
>         Changes in v4:
>             - Split patch #6 from dom0 pvh series v6.2 to retrieve only common
>             code.
>             - Rework commit title
>             - Rename xen_rem_foreign_from_p2m in p2m_remove_foreign
>             - Get the mfn from the pte. We are not sure that maddr given in
>             parameters is valid
>         Changes in v3:
>             - Move put_page in create_p2m_entries
>             - Move xenmem_rem_foreign_from_p2m in arch/arm/p2m.c
>         Changes in v2:
>             - Introduce the patch
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 39d8a03..f7bd7e2 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -317,10 +317,21 @@ static int create_p2m_entries(struct domain *d,
>                  break;
>              case REMOVE:
>                  {
> -                    lpae_t pte;
> +                    lpae_t pte = third[third_table_offset(addr)];
> +                    unsigned long mfn;
> +
> +                    maddr = (pte.bits & PADDR_MASK & PAGE_MASK);

I thought we had a macro for this, but apparently not. While looking for
it I spotted that x86 has pte_to_mfn, which sounds like a useful
innovation... (not essential as part of this series though).

> +                    mfn = paddr_to_pfn(maddr);
> +
> +                    /* TODO: Handle other p2m type */
> +                    if ( pte.p2m.valid && p2m_is_foreign(pte.p2m.type) )
> +                    {
> +                        ASSERT(mfn_valid(mfn));

Something somewhere is making sure we don't put foreign MMIO regions
into the p2m, right?

> +                        put_page(mfn_to_page(mfn));
> +                    }
> +
>                      memset(&pte, 0x00, sizeof(pte));
>                      write_pte(&third[third_table_offset(addr)], pte);
> -                    maddr += PAGE_SIZE;
>                  }
>                  break;
>          }
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 0eb07a8..e0b58da 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -122,9 +122,21 @@ static inline struct page_info *get_page_from_gfn(
>      if ( !mfn_valid(mfn) )
>          return NULL;
>      page = mfn_to_page(mfn);
> -    if ( !get_page(page, d) )
> -        return NULL;
> -    return page;
> +
> +    if ( get_page(page, d) )

This isn't noisy (even at debug level) on failure, I thought so?

Might be safer (and TBH more logical) to move it after the foreign
special case.

> +        return page;
> +
> +    /* get_page won't work on foreign mapping because the page doesn't
> +     * belong to the current domain.
> +     */
> +    if ( p2mt == p2m_map_foreign )
> +    {
> +        struct domain *fdom = page_get_owner_and_reference(page);
> +        ASSERT(fdom != NULL);

ASSERT(fdom != d)
?

> +        return page;
> +    }
> +
> +    return NULL;
>  }
>  
>  int get_page_type(struct page_info *page, unsigned long type);
> 



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