On Thu, Apr 28, 2016 at 4:36 AM, George Dunlap <george.dun...@citrix.com> wrote:
> On 27/04/16 16:48, Tamas K Lengyel wrote: > > Don't propagate altp2m changes from ept_set_entry for memshare as > memshare > > already has the lock. We call altp2m propagate changes once memshare > > successfully finishes. Allow the hostp2m entries to be of type > > p2m_ram_shared when applying mem_access. Also, do not trigger PoD for > hostp2m > > when setting altp2m mem_access to be in-line with non-altp2m mem_access > path. > > > > Signed-off-by: Tamas K Lengyel <ta...@tklengyel.com> > > --- > > Cc: George Dunlap <george.dun...@eu.citrix.com> > > Cc: Keir Fraser <k...@xen.org> > > Cc: Jan Beulich <jbeul...@suse.com> > > Cc: Andrew Cooper <andrew.coop...@citrix.com> > > Cc: Jun Nakajima <jun.nakaj...@intel.com> > > Cc: Kevin Tian <kevin.t...@intel.com> > > > > v2: Cosmetic fixes and addressing PoD in the commit message > > --- > > xen/arch/x86/mm/mem_sharing.c | 10 +++++++++- > > xen/arch/x86/mm/p2m-ept.c | 2 +- > > xen/arch/x86/mm/p2m.c | 5 ++--- > > 3 files changed, 12 insertions(+), 5 deletions(-) > > > > diff --git a/xen/arch/x86/mm/mem_sharing.c > b/xen/arch/x86/mm/mem_sharing.c > > index a522423..6693661 100644 > > --- a/xen/arch/x86/mm/mem_sharing.c > > +++ b/xen/arch/x86/mm/mem_sharing.c > > @@ -35,6 +35,7 @@ > > #include <asm/p2m.h> > > #include <asm/atomic.h> > > #include <asm/event.h> > > +#include <asm/altp2m.h> > > #include <xsm/xsm.h> > > > > #include "mm-locks.h" > > @@ -926,11 +927,12 @@ int mem_sharing_share_pages(struct domain *sd, > unsigned long sgfn, shr_handle_t > > int ret = -EINVAL; > > mfn_t smfn, cmfn; > > p2m_type_t smfn_type, cmfn_type; > > + p2m_access_t cmfn_a; > > struct two_gfns tg; > > struct rmap_iterator ri; > > > > get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn, > > - cd, cgfn, &cmfn_type, NULL, &cmfn, > > + cd, cgfn, &cmfn_type, &cmfn_a, &cmfn, > > 0, &tg); > > > > /* This tricky business is to avoid two callers deadlocking if > > @@ -1026,6 +1028,12 @@ int mem_sharing_share_pages(struct domain *sd, > unsigned long sgfn, shr_handle_t > > /* We managed to free a domain page. */ > > atomic_dec(&nr_shared_mfns); > > atomic_inc(&nr_saved_mfns); > > + > > + /* Save the change to the altp2m tables as well if active */ > > + if ( altp2m_active(cd) ) > > + p2m_altp2m_propagate_change(cd, _gfn(cgfn), smfn, PAGE_ORDER_4K, > > + p2m_ram_shared, cmfn_a); > > + > > ret = 0; > > > > err_out: > > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c > > index 1ed5b47..ff84242 100644 > > --- a/xen/arch/x86/mm/p2m-ept.c > > +++ b/xen/arch/x86/mm/p2m-ept.c > > @@ -847,7 +847,7 @@ out: > > if ( is_epte_present(&old_entry) ) > > ept_free_entry(p2m, &old_entry, target); > > > > - if ( rc == 0 && p2m_is_hostp2m(p2m) ) > > + if ( rc == 0 && p2m_is_hostp2m(p2m) && p2mt != p2m_ram_shared ) > > p2m_altp2m_propagate_change(d, _gfn(gfn), mfn, order, p2mt, > p2ma); > > Sorry, just double-checking here: This is the right thing to do because > the only code that is *setting* a page p2m_ram_shared should be the > sharing code, which (after this patch) should be propagating the change > itself? > The problem is with the mm-lock really. The mem_sharing paths already apply mem_sharing_page_lock when changing the type to p2m_ram_shared, which interferes with altp2m_list_lock. It would be fine to propagate the changes to the altp2m tables otherwise if we could make the locks not interfere, but that would require more changes then this patch. > > I'm just conscious that this introduces an invariant that would be easy > to accidentally break. > > At the moment the only place that calls set_entry() with p2m_ram_shared > is in p2m.c:set_shared_p2m_entry(), which is called from > mem_sharing.c:__mem_sharing_unshare_page() and > mem_sharing.c:mem_sharing_share_pages(). You've handled the second case > by adding the if() clause at the end of the operation; and it looks like > the first case is handled because the shared page will be overwritten > by the p2m_change_type_one(). > Right, I don't really see much point in setting the pages shared (again) right before unsharing overwrites it so there is no altp2m propagation there.. Then p2m_change_type_one is outside the mem_sharing_page_lock so there wasn't a need for an extra altp2m check. > > I think at very least adding a comment here saying something like the > following would be helpful: > > "Memsharing is responsible for updating altp2ms after the sharing > operation is complete" > > And perhaps a note before the p2m_change_type_one() in > __mem_sharing_unshare_page() saying something like: > > "This will take care of updating the altp2m mappings." > > What do you think? > That sounds all good to me. The mem_sharing code is undocumented enough that IMHO adding more comments would help with making it more maintainable going forward =) Thanks, Tamas
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
