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

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 12/05/2013 04:34 PM, Ian Campbell wrote:
> 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.
Will do.

> >       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.
Do you have a link to this patch?

> 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.
If I'm not mistaken, Xen already takes a ref when the page is allocated 
for the domain. Why would we need to take another ref for these pages?
--
Julien Grall

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