| Message ID | 1386258131-755-8-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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@linaro.org> > --- > 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; > }
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@linaro.org> >> --- >> 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?
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@eu.citrix.com>. I don't recall if that was the last one though. > > > 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. In some sense (and I'm not sure it does make sense) you could consider there to be one reference for owning the page and a second for mapping it. I don't think a domain can own a page which isn't mapped in its p2m, because decrease reservation frees the page, although the 1:1 ballooning workaround has some aspect of that to it. Ian.
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@eu.citrix.com>. 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. >> >>> 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 ... In any case, walk the whole p2m for looking for present page seems a bit tough and slow to release memory for a domain. I'm wondering if we can have a list of foreign page, and browse it when the domain is destroyed.
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@eu.citrix.com>. 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.
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@linaro.org> >> --- >> 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. Actually, GCC compiler won't complain if t is not set in one of the case. If have tried this following dummy function on several version of GCC (the most recent is 4.8.2) and they don't emit any warning. ================================================================= int f (int a) { int b; switch (a) { case 1: b = 1; break; case 2: /* b is unset */ break; } return b; } ==================================================================== I have also tried clang, and I effectively get an error. But we don't yet support clang for Xen ARM port. I have found a bug report opened in 2004 for this kind of bug, but never fixed... (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501).
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; 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. + * Refcount will be release in XENMEM_remove_from_physmap */ + 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 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@linaro.org> --- xen/arch/arm/mm.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)