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 17:39 +0000, Julien Grall wrote:
> 
> On 12/05/2013 04:54 PM, Ian Campbell wrote:
> > On Thu, 2013-12-05 at 16:41 +0000, Julien Grall wrote:
> >
> >>>>
> >>>>        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?
> >
> > I was afraid you'd say that...
> > /scrobbles
> >
> > "xen/arm: introduce a generic p2m walker and use it in p2m_lookup". v6
> > is msgid
> > <1380298560-29352-2-git-send-email-stefano.stabellini@xxxxxxxxxxxxx>. I
> > don't recall if that was the last one though.
> 
> Actually the generic walker will not be usefull for p2m_teardown. It 
> seems to be an extension of p2m_lookup, so will only search a specific gfn.

Ah, sorry, my memory must have been faulty.

> >>> 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?
> >
> > There's a nice symmetry to all p2m entries taking a ref count, and it
> > makes the teardown a bit simpler since you can just drop the ref every
> > time without worrying about the type.
> 
> If I'm not mistaken, even x86 code doesn't have 2 ref for each page. Or 
> I didn't see where the ref is taken/release ...

I seem to remember Tim saying this was some weird historical reason.
(maybe something to do with shadow page tables, or type counts rather
than ref counts?)

> In any case, walk the whole p2m for looking for present page seems a bit 
> tough and slow to release memory for a domain.

Yes, for a large domain it might take a while, which is why it would
need to use hypercall continuations. Luckily the page table itself
stores your progress if you clear the entries as you go, so that should
be reasonably easy to arrange, take a look at x86's
domain_relinquish_resources and how it uses d->arch.relmem to track what
phase of the teardown it is at.

> I'm wondering if we can have a list of foreign page, and browse it when 
> the domain is destroyed.

The problem is that struct page_info only has one list head in it and it
is used for the domain's list of pages.

Ian.



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