On 27/04/16 16:18, Tamas K Lengyel wrote: > On Wed, Apr 27, 2016 at 9:01 AM, George Dunlap <george.dun...@citrix.com> > wrote: > >> On 21/04/16 18:10, 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. Also, allow the hostp2m entries to be of type >>> p2m_ram_shared. >>> >>> Signed-off-by: Tamas K Lengyel <ta...@tklengyel.com> >> >> Sorry for the delay in reviewing -- trying to get my head back around >> the altp2m code. On the whole looks reasonable, but one question... >> >>> --- >>> 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> >>> --- >>> xen/arch/x86/mm/mem_sharing.c | 11 +++++++++++ >>> xen/arch/x86/mm/p2m-ept.c | 2 +- >>> xen/arch/x86/mm/p2m.c | 7 +++---- >>> 3 files changed, 15 insertions(+), 5 deletions(-) >>> >>> diff --git a/xen/arch/x86/mm/mem_sharing.c >> b/xen/arch/x86/mm/mem_sharing.c >>> index a522423..d5b4b2d 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" >>> @@ -1026,6 +1027,16 @@ 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); >>> + >>> + if( altp2m_active(cd) ) >>> + { >>> + p2m_access_t a; >>> + struct p2m_domain *p2m = p2m_get_hostp2m(cd); >>> + p2m->get_entry(p2m, cgfn, NULL, &a, 0, NULL, NULL); >>> + p2m_altp2m_propagate_change(cd, _gfn(cgfn), smfn, PAGE_ORDER_4K, >>> + p2m_ram_shared, a); >>> + } >>> + >>> ret = 0; >>> >>> err_out: >>> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c >>> index 3cb6868..1ac3018 100644 >>> --- a/xen/arch/x86/mm/p2m-ept.c >>> +++ b/xen/arch/x86/mm/p2m-ept.c >>> @@ -846,7 +846,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); >>> >>> return rc; >>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c >>> index b3fce1b..d2aebf7 100644 >>> --- a/xen/arch/x86/mm/p2m.c >>> +++ b/xen/arch/x86/mm/p2m.c >>> @@ -1739,11 +1739,10 @@ int p2m_set_altp2m_mem_access(struct domain *d, >> struct p2m_domain *hp2m, >>> /* Check host p2m if no valid entry in alternate */ >>> if ( !mfn_valid(mfn) ) >>> { >>> - mfn = hp2m->get_entry(hp2m, gfn_l, &t, &old_a, >>> - P2M_ALLOC | P2M_UNSHARE, &page_order, >> NULL); >>> + mfn = hp2m->get_entry(hp2m, gfn_l, &t, &old_a, 0, &page_order, >> NULL); >> >> Why are you getting rid of P2M_ALLOC here? What happens if the hp2m >> entry is populate-on-demand? >> > > There is a check further down here that only allows p2m_ram_rw and > p2m_ram_shared.
So what P2M_ALLOC means is, "If this is entry is PoD, then please populate it so I get a ram page." So the only way you can get a p2m_populate_on_demand type returned is if you remove this flag. Leave it and (assuming there's enough ram to go around), you'll always get p2m_ram_rw. :-) > On the non-altp2m path mem_access doesn't request P2M_ALLOC > either (but doesn't check the type), so I would say mem_access is not > compatible with PoD. Off the top of my head I can't see a reason why they couldn't co-exist in principle, if you added P2M_ALLOC in a few key places. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel