| Message ID | 1461258632-3330-1-git-send-email-tamas@tklengyel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Sadly I only have little nitpicks. Feel free to ignore them. > 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) ) Missing space. > + { > + p2m_access_t a; Newline. > + 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); > > rc = -ESRCH; > - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) > + if ( !mfn_valid(mfn) || (t != p2m_ram_rw && t != p2m_ram_shared) ) Would it be easier to read if there were more of ()? > return rc; > > /* If this is a superpage, copy that first */ > @@ -1760,7 +1759,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, > } > > return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, > - (current->domain != d)); > + (current->domain != d)); That looks like a cleanup? Perhaps a different patch?
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 <tamas@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.dunlap@eu.citrix.com> > Cc: Keir Fraser <keir@xen.org> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Jun Nakajima <jun.nakajima@intel.com> > Cc: Kevin Tian <kevin.tian@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? -George
On Wed, Apr 27, 2016 at 9:01 AM, George Dunlap <george.dunlap@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 <tamas@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.dunlap@eu.citrix.com> > > Cc: Keir Fraser <keir@xen.org> > > Cc: Jan Beulich <jbeulich@suse.com> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > Cc: Jun Nakajima <jun.nakajima@intel.com> > > Cc: Kevin Tian <kevin.tian@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
On 27/04/16 16:18, Tamas K Lengyel wrote: > On Wed, Apr 27, 2016 at 9:01 AM, George Dunlap <george.dunlap@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 <tamas@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.dunlap@eu.citrix.com> >>> Cc: Keir Fraser <keir@xen.org> >>> Cc: Jan Beulich <jbeulich@suse.com> >>> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >>> Cc: Jun Nakajima <jun.nakajima@intel.com> >>> Cc: Kevin Tian <kevin.tian@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
On Wed, Apr 27, 2016 at 9:31 AM, George Dunlap <george.dunlap@citrix.com> wrote: > On 27/04/16 16:18, Tamas K Lengyel wrote: > > On Wed, Apr 27, 2016 at 9:01 AM, George Dunlap <george.dunlap@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 <tamas@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.dunlap@eu.citrix.com> > >>> Cc: Keir Fraser <keir@xen.org> > >>> Cc: Jan Beulich <jbeulich@suse.com> > >>> Cc: Andrew Cooper <andrew.cooper3@citrix.com> > >>> Cc: Jun Nakajima <jun.nakajima@intel.com> > >>> Cc: Kevin Tian <kevin.tian@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. > Sure, I just rather do that in a separate patch and for now have the mem_access paths behaving the same way before doing that adjustment. Tamas
On 27/04/16 16:37, Tamas K Lengyel wrote: > On Wed, Apr 27, 2016 at 9:31 AM, George Dunlap <george.dunlap@citrix.com> > wrote: > >> On 27/04/16 16:18, Tamas K Lengyel wrote: >>> On Wed, Apr 27, 2016 at 9:01 AM, George Dunlap <george.dunlap@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 <tamas@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.dunlap@eu.citrix.com> >>>>> Cc: Keir Fraser <keir@xen.org> >>>>> Cc: Jan Beulich <jbeulich@suse.com> >>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >>>>> Cc: Jun Nakajima <jun.nakajima@intel.com> >>>>> Cc: Kevin Tian <kevin.tian@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. >> > > Sure, I just rather do that in a separate patch and for now have the > mem_access paths behaving the same way before doing that adjustment. Ok, in that case please add a few sentences in the changelog addressing the change. Thanks, -George
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); rc = -ESRCH; - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) + if ( !mfn_valid(mfn) || (t != p2m_ram_rw && t != p2m_ram_shared) ) return rc; /* If this is a superpage, copy that first */ @@ -1760,7 +1759,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, } return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, - (current->domain != d)); + (current->domain != d)); } /*
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 <tamas@tklengyel.com> --- Cc: George Dunlap <george.dunlap@eu.citrix.com> Cc: Keir Fraser <keir@xen.org> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Jun Nakajima <jun.nakajima@intel.com> Cc: Kevin Tian <kevin.tian@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(-)