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. 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.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
