* [PATCH] altp2m: Allow the hostp2m to be shared
@ 2016-04-21 17:10 Tamas K Lengyel
2016-04-25 20:30 ` Konrad Rzeszutek Wilk
2016-04-27 15:01 ` George Dunlap
0 siblings, 2 replies; 7+ messages in thread
From: Tamas K Lengyel @ 2016-04-21 17:10 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Tamas K Lengyel, Keir Fraser, Jan Beulich,
George Dunlap, Andrew Cooper, Jun Nakajima
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(-)
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));
}
/*
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] altp2m: Allow the hostp2m to be shared
2016-04-21 17:10 [PATCH] altp2m: Allow the hostp2m to be shared Tamas K Lengyel
@ 2016-04-25 20:30 ` Konrad Rzeszutek Wilk
2016-04-27 15:01 ` George Dunlap
1 sibling, 0 replies; 7+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-04-25 20:30 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Kevin Tian, Keir Fraser, Jun Nakajima, George Dunlap,
Andrew Cooper, Jan Beulich, xen-devel
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?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] altp2m: Allow the hostp2m to be shared
2016-04-21 17:10 [PATCH] altp2m: Allow the hostp2m to be shared Tamas K Lengyel
2016-04-25 20:30 ` Konrad Rzeszutek Wilk
@ 2016-04-27 15:01 ` George Dunlap
2016-04-27 15:18 ` Tamas K Lengyel
1 sibling, 1 reply; 7+ messages in thread
From: George Dunlap @ 2016-04-27 15:01 UTC (permalink / raw)
To: Tamas K Lengyel, xen-devel
Cc: Kevin Tian, Keir Fraser, Jan Beulich, George Dunlap,
Andrew Cooper, Jun Nakajima
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] altp2m: Allow the hostp2m to be shared
2016-04-27 15:01 ` George Dunlap
@ 2016-04-27 15:18 ` Tamas K Lengyel
2016-04-27 15:31 ` George Dunlap
0 siblings, 1 reply; 7+ messages in thread
From: Tamas K Lengyel @ 2016-04-27 15:18 UTC (permalink / raw)
To: George Dunlap
Cc: Kevin Tian, Keir Fraser, Jan Beulich, George Dunlap,
Andrew Cooper, Jun Nakajima, Xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 3555 bytes --]
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
[-- Attachment #1.2: Type: text/html, Size: 5065 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] altp2m: Allow the hostp2m to be shared
2016-04-27 15:18 ` Tamas K Lengyel
@ 2016-04-27 15:31 ` George Dunlap
2016-04-27 15:37 ` Tamas K Lengyel
0 siblings, 1 reply; 7+ messages in thread
From: George Dunlap @ 2016-04-27 15:31 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Kevin Tian, Keir Fraser, Jan Beulich, George Dunlap,
Andrew Cooper, Jun Nakajima, Xen-devel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] altp2m: Allow the hostp2m to be shared
2016-04-27 15:31 ` George Dunlap
@ 2016-04-27 15:37 ` Tamas K Lengyel
2016-04-27 15:40 ` George Dunlap
0 siblings, 1 reply; 7+ messages in thread
From: Tamas K Lengyel @ 2016-04-27 15:37 UTC (permalink / raw)
To: George Dunlap
Cc: Kevin Tian, Keir Fraser, Jan Beulich, George Dunlap,
Andrew Cooper, Jun Nakajima, Xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 4491 bytes --]
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
[-- Attachment #1.2: Type: text/html, Size: 6744 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] altp2m: Allow the hostp2m to be shared
2016-04-27 15:37 ` Tamas K Lengyel
@ 2016-04-27 15:40 ` George Dunlap
0 siblings, 0 replies; 7+ messages in thread
From: George Dunlap @ 2016-04-27 15:40 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Kevin Tian, Keir Fraser, Jan Beulich, George Dunlap,
Andrew Cooper, Jun Nakajima, Xen-devel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-04-27 15:40 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-21 17:10 [PATCH] altp2m: Allow the hostp2m to be shared Tamas K Lengyel
2016-04-25 20:30 ` Konrad Rzeszutek Wilk
2016-04-27 15:01 ` George Dunlap
2016-04-27 15:18 ` Tamas K Lengyel
2016-04-27 15:31 ` George Dunlap
2016-04-27 15:37 ` Tamas K Lengyel
2016-04-27 15:40 ` George Dunlap
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).