Hi,
At 16:42 -0500 on 08 Nov (1320770546), Andres Lagar-Cavilla wrote:
> For translated domains, critical sections demarcated by a
> get_gfn/put_gfn pair will hold an additional ref on the
> underlying mfn.
Remind me what this gets us again? Is it just belt-and-braces on top of
the locking at the p2m layer?
It should be possible to do this without the extra argument - for
example, the bottom-level function that actually changes a p2m entry
must hold the p2m exclusion lock for that entry, or the master p2m lock,
right? In either case it knows whether it has a ref on the mfn.
I suppose having these locks be recursive makes that a problem, but in
that case you have another problem -- how many mfn refs need to be
moved?
Cheers,
Tim (confused).
> This requires keeping tabs on special manipulations that
> change said mfn:
> - physmap remove page
> - sharing
> - steal page
>
> Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
>
> diff -r 6203a0549d8a -r 4b684fa74636 xen/arch/x86/mm.c
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4128,7 +4128,7 @@ static int replace_grant_p2m_mapping(
> type, mfn_x(old_mfn), frame);
> return GNTST_general_error;
> }
> - guest_physmap_remove_page(d, gfn, frame, 0);
> + guest_physmap_remove_page(d, gfn, frame, 0, HOLDING_GFN);
>
> put_gfn(d, gfn);
> return GNTST_okay;
> @@ -4248,8 +4248,10 @@ int donate_page(
> }
>
> int steal_page(
> - struct domain *d, struct page_info *page, unsigned int memflags)
> + struct domain *d, struct page_info *page, unsigned int memflags,
> + int holding_gfn)
> {
> + unsigned count;
> unsigned long x, y;
> bool_t drop_dom_ref = 0;
>
> @@ -4261,11 +4263,14 @@ int steal_page(
> /*
> * We require there is just one reference (PGC_allocated). We temporarily
> * drop this reference now so that we can safely swizzle the owner.
> + * If, however, this is invoked by a caller holding the p2m entry, there
> + * will be another reference.
> */
> + count = (holding_gfn) ? 1 : 2;
> y = page->count_info;
> do {
> x = y;
> - if ( (x & (PGC_count_mask|PGC_allocated)) != (1 | PGC_allocated) )
> + if ( (x & (PGC_count_mask|PGC_allocated)) != (count | PGC_allocated)
> )
> goto fail;
> y = cmpxchg(&page->count_info, x, x & ~PGC_count_mask);
> } while ( y != x );
> @@ -4276,7 +4281,7 @@ int steal_page(
> do {
> x = y;
> BUG_ON((x & (PGC_count_mask|PGC_allocated)) != PGC_allocated);
> - } while ( (y = cmpxchg(&page->count_info, x, x | 1)) != x );
> + } while ( (y = cmpxchg(&page->count_info, x, x | count)) != x );
>
> /* Unlink from original owner. */
> if ( !(memflags & MEMF_no_refcount) && !--d->tot_pages )
> @@ -4779,7 +4784,8 @@ long arch_memory_op(int op, XEN_GUEST_HA
> {
> if ( is_xen_heap_mfn(prev_mfn) )
> /* Xen heap frames are simply unhooked from this phys slot.
> */
> - guest_physmap_remove_page(d, xatp.gpfn, prev_mfn, 0);
> + guest_physmap_remove_page(d, xatp.gpfn, prev_mfn, 0,
> + HOLDING_GFN);
> else
> /* Normal domain memory is freed, to avoid leaking memory. */
> guest_remove_page(d, xatp.gpfn);
> @@ -4791,7 +4797,7 @@ long arch_memory_op(int op, XEN_GUEST_HA
> gpfn = get_gpfn_from_mfn(mfn);
> ASSERT( gpfn != SHARED_M2P_ENTRY );
> if ( gpfn != INVALID_M2P_ENTRY )
> - guest_physmap_remove_page(d, gpfn, mfn, 0);
> + guest_physmap_remove_page(d, gpfn, mfn, 0, (gpfn == gfn));
>
> /* Map at new location. */
> rc = guest_physmap_add_page(d, xatp.gpfn, mfn, 0);
> diff -r 6203a0549d8a -r 4b684fa74636 xen/arch/x86/mm/mem_sharing.c
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -561,7 +561,7 @@ int mem_sharing_share_pages(shr_handle_t
> list_del(&gfn->list);
> d = get_domain_by_id(gfn->domain);
> BUG_ON(!d);
> - BUG_ON(set_shared_p2m_entry(d, gfn->gfn, se->mfn) == 0);
> + BUG_ON(set_shared_p2m_entry(d, gfn->gfn, se->mfn, 0) == 0);
> put_domain(d);
> list_add(&gfn->list, &se->gfns);
> put_page_and_type(cpage);
> @@ -670,14 +670,9 @@ gfn_found:
> unmap_domain_page(s);
> unmap_domain_page(t);
>
> - /* NOTE: set_shared_p2m_entry will switch the underlying mfn. If
> - * we do get_page withing get_gfn, the correct sequence here
> - * should be
> - get_page(page);
> - put_page(old_page);
> - * so that the ref to the old page is dropped, and a ref to
> - * the new page is obtained to later be dropped in put_gfn */
> - BUG_ON(set_shared_p2m_entry(d, gfn, page_to_mfn(page)) == 0);
> + /* NOTE: set_shared_p2m_entry will swap the underlying mfn and the refs.
> + * It is safe to call put_gfn as usual after this. */
> + BUG_ON(set_shared_p2m_entry(d, gfn, page_to_mfn(page), HOLDING_GFN) ==
> 0);
> put_page_and_type(old_page);
>
> private_page_found:
> diff -r 6203a0549d8a -r 4b684fa74636 xen/arch/x86/mm/p2m.c
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -182,6 +182,13 @@ mfn_t gfn_to_mfn_type_p2m(struct p2m_dom
> }
> #endif
>
> + /* Do a get page to get one additional ref on the mfn */
> + if ( mfn_valid(mfn) )
> + {
> + struct page_info *pg = mfn_to_page(mfn);
> + BUG_ON( !page_get_owner_and_reference(pg) );
> + }
> +
> return mfn;
> }
>
> @@ -195,6 +202,21 @@ void put_gfn(struct domain *d, unsigned
>
> ASSERT(p2m_locked_by_me(p2m));
>
> + {
> + p2m_access_t a;
> + p2m_type_t t;
> + mfn_t mfn = p2m->get_entry(p2m, gfn, &t, &a,
> + p2m_query, NULL);
> +
> + if ( mfn_valid(mfn) )
> + {
> +#ifdef __x86_64__
> + if (likely( !(p2m_is_broken(t)) ))
> +#endif
> + put_page(mfn_to_page(mfn));
> + }
> + }
> +
> p2m_unlock(p2m);
> }
>
> @@ -416,6 +438,28 @@ void p2m_final_teardown(struct domain *d
> p2m_teardown_nestedp2m(d);
> }
>
> +/* If the caller has done get_gfn on this entry, then it has a ref on the
> + * old mfn. Swap the refs so put_gfn puts the appropriate ref */
> +static inline void __p2m_swap_entry_refs(struct p2m_domain *p2m,
> + unsigned long gfn, mfn_t mfn)
> +{
> + p2m_type_t t;
> + p2m_access_t a;
> + mfn_t omfn;
> +
> + if ( !paging_mode_translate(p2m->domain) )
> + return;
> +
> + ASSERT(gfn_locked_by_me(p2m, gfn));
> +
> + omfn = p2m->get_entry(p2m, gfn, &t, &a, p2m_query, NULL);
> +
> + if ( mfn_valid(mfn) )
> + BUG_ON( !page_get_owner_and_reference(mfn_to_page(mfn)) );
> +
> + if ( mfn_valid(omfn) )
> + put_page(mfn_to_page(omfn));
> +}
>
> static void
> p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn,
> @@ -451,11 +495,14 @@ p2m_remove_page(struct p2m_domain *p2m,
>
> void
> guest_physmap_remove_page(struct domain *d, unsigned long gfn,
> - unsigned long mfn, unsigned int page_order)
> + unsigned long mfn, unsigned int page_order,
> + int holding_gfn)
> {
> struct p2m_domain *p2m = p2m_get_hostp2m(d);
> p2m_lock(p2m);
> audit_p2m(p2m, 1);
> + if (holding_gfn)
> + __p2m_swap_entry_refs(p2m, gfn, _mfn(INVALID_MFN));
> p2m_remove_page(p2m, gfn, mfn, page_order);
> audit_p2m(p2m, 1);
> p2m_unlock(p2m);
> @@ -713,7 +760,8 @@ out:
> }
>
> int
> -set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
> +set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
> + int holding_gfn)
> {
> struct p2m_domain *p2m = p2m_get_hostp2m(d);
> int rc = 0;
> @@ -730,6 +778,10 @@ set_shared_p2m_entry(struct domain *d, u
> * sharable first */
> ASSERT(p2m_is_shared(ot));
> ASSERT(mfn_valid(omfn));
> +
> + if ( holding_gfn )
> + __p2m_swap_entry_refs(p2m, gfn, mfn);
> +
> /* XXX: M2P translations have to be handled properly for shared pages */
> set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
>
> diff -r 6203a0549d8a -r 4b684fa74636 xen/common/grant_table.c
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1497,7 +1497,7 @@ gnttab_transfer(
> goto copyback;
> }
>
> - if ( steal_page(d, page, 0) < 0 )
> + if ( steal_page(d, page, 0, HOLDING_GFN) < 0 )
> {
> put_gfn(d, gop.mfn);
> gop.status = GNTST_bad_page;
> @@ -1505,7 +1505,7 @@ gnttab_transfer(
> }
>
> #ifndef __ia64__ /* IA64 implicitly replaces the old page in steal_page(). */
> - guest_physmap_remove_page(d, gop.mfn, mfn, 0);
> + guest_physmap_remove_page(d, gop.mfn, mfn, 0, HOLDING_GFN);
> #endif
> flush_tlb_mask(d->domain_dirty_cpumask);
>
> diff -r 6203a0549d8a -r 4b684fa74636 xen/common/memory.c
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -165,7 +165,7 @@ int guest_remove_page(struct domain *d,
> mfn = mfn_x(get_gfn(d, gmfn, &p2mt));
> if ( unlikely(p2m_is_paging(p2mt)) )
> {
> - guest_physmap_remove_page(d, gmfn, mfn, 0);
> + guest_physmap_remove_page(d, gmfn, mfn, 0, HOLDING_GFN);
> p2m_mem_paging_drop_page(d, gmfn);
> put_gfn(d, gmfn);
> return 1;
> @@ -188,7 +188,7 @@ int guest_remove_page(struct domain *d,
> if(p2m_is_shared(p2mt))
> {
> put_page_and_type(page);
> - guest_physmap_remove_page(d, gmfn, mfn, 0);
> + guest_physmap_remove_page(d, gmfn, mfn, 0, HOLDING_GFN);
> put_gfn(d, gmfn);
> return 1;
> }
> @@ -207,7 +207,7 @@ int guest_remove_page(struct domain *d,
> if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
> put_page(page);
>
> - guest_physmap_remove_page(d, gmfn, mfn, 0);
> + guest_physmap_remove_page(d, gmfn, mfn, 0, HOLDING_GFN);
> put_gfn(d, gmfn);
>
> put_page(page);
> @@ -387,7 +387,7 @@ static long memory_exchange(XEN_GUEST_HA
>
> page = mfn_to_page(mfn);
>
> - if ( unlikely(steal_page(d, page, MEMF_no_refcount)) )
> + if ( unlikely(steal_page(d, page, MEMF_no_refcount,
> HOLDING_GFN)) )
> {
> put_gfn(d, gmfn + k);
> rc = -EINVAL;
> @@ -427,7 +427,7 @@ static long memory_exchange(XEN_GUEST_HA
> gfn = mfn_to_gmfn(d, mfn);
> /* Pages were unshared above */
> BUG_ON(SHARED_M2P(gfn));
> - guest_physmap_remove_page(d, gfn, mfn, 0);
> + guest_physmap_remove_page(d, gfn, mfn, 0, 0);
> put_page(page);
> }
>
> diff -r 6203a0549d8a -r 4b684fa74636 xen/include/asm-x86/mm.h
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -578,7 +578,8 @@ int compat_arch_memory_op(int op, XEN_GU
> int compat_subarch_memory_op(int op, XEN_GUEST_HANDLE(void));
>
> int steal_page(
> - struct domain *d, struct page_info *page, unsigned int memflags);
> + struct domain *d, struct page_info *page, unsigned int memflags,
> + int holding_gfn);
> int donate_page(
> struct domain *d, struct page_info *page, unsigned int memflags);
> int page_make_sharable(struct domain *d,
> diff -r 6203a0549d8a -r 4b684fa74636 xen/include/asm-x86/p2m.h
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -353,6 +353,10 @@ static inline unsigned long get_gfn_unty
> /* Will release the p2m_lock and put the page behind this mapping. */
> void put_gfn(struct domain *d, unsigned long gfn);
>
> +/* Operations that change the underlying mfn in a p2m entry need to be
> + * told whether the caller is holding the current gfn */
> +#define HOLDING_GFN 1
> +
> /* The intent is to have the caller not worry about put_gfn. They apply to
> * very specific situations: debug printk's, dumps during a domain crash,
> * or to peek at a p2m entry/type. Caller is not holding the p2m entry
> @@ -410,7 +414,8 @@ static inline int guest_physmap_add_page
> /* Remove a page from a domain's p2m table */
> void guest_physmap_remove_page(struct domain *d,
> unsigned long gfn,
> - unsigned long mfn, unsigned int page_order);
> + unsigned long mfn, unsigned int page_order,
> + int holding_gfn);
>
> /* Set a p2m range as populate-on-demand */
> int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long
> gfn,
> @@ -471,7 +476,8 @@ p2m_pod_offline_or_broken_replace(struct
>
> #ifdef __x86_64__
> /* Modify p2m table for shared gfn */
> -int set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
> +int set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
> + int holding_gfn);
>
> /* Check if a nominated gfn is valid to be paged out */
> int p2m_mem_paging_nominate(struct domain *d, unsigned long gfn);
> diff -r 6203a0549d8a -r 4b684fa74636 xen/include/xen/paging.h
> --- a/xen/include/xen/paging.h
> +++ b/xen/include/xen/paging.h
> @@ -21,7 +21,7 @@
> #define paging_mode_translate(d) (0)
> #define paging_mode_external(d) (0)
> #define guest_physmap_add_page(d, p, m, o) (0)
> -#define guest_physmap_remove_page(d, p, m, o) ((void)0)
> +#define guest_physmap_remove_page(d, p, m, o, h) ((void)0)
>
> #endif
>
> diff -r 6203a0549d8a -r 4b684fa74636 xen/include/xen/tmem_xen.h
> --- a/xen/include/xen/tmem_xen.h
> +++ b/xen/include/xen/tmem_xen.h
> @@ -198,7 +198,7 @@ static inline void _tmh_free_page_thispo
> struct domain *d = page_get_owner(pi);
>
> ASSERT(IS_VALID_PAGE(pi));
> - if ( (d == NULL) || steal_page(d,pi,0) == 0 )
> + if ( (d == NULL) || steal_page(d,pi,0,0) == 0 )
> tmh_page_list_put(pi);
> else
> {
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home