| Message ID | 1485789452-19830-1-git-send-email-george.dunlap@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi George, yeap, this solves old mem_access settings being triggered when I recreate altp2m views. Thanks! On Mon, Jan 30, 2017 at 8:17 AM, George Dunlap <george.dunlap@citrix.com> wrote: > Commit 71bb7304e7a7a35ea6df4b0cedebc35028e4c159 added flushing of > nested p2m tables whenever the host p2m table changed. Unfortunately > in the process, it added a filter to the generic p2m_flush_table() > function so that the p2m would only be flushed if it was being used as > a nested p2m. This meant that the p2m was not being flushed at all > for altp2m callers. > > Instead do the nested p2m filtering in p2m_flush_nestedp2m(). > > NB that this is not a security issue: The only time this codepath is > called is in cases where either nestedp2m or altp2m is enabled, and > neither of them are in security support. > > Reported-by: Matt Leinhos <matt@starlab.io> > Signed-off-by: George Dunlap <george.dunlap@citrix.com> > --- > I've smoke-tested this with nested virt and it seems to work fine. > Matt / Tamas, could you test this with altp2m and see if it fixes your > issue? Tested-by: Tamas K Lengyel <tamas.lengyel@zentific.com> > > > CC: Liang Li <liang.z.li@intel.com> > CC: Yang Zhang <yang.z.zhang@intel.com> > CC: Tim Deegan <tim@xen.org> > CC: Tamas K Lengyel <tamas.lengyel@zentific.com> > CC: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Jan Beulich <jbeulich@suse.com> > CC: Matt Leinhos <matt@starlab.io> > --- > xen/arch/x86/mm/p2m.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index aa627d8..0849c6e 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -2048,12 +2048,6 @@ p2m_flush_table(struct p2m_domain *p2m) > ASSERT(page_list_empty(&p2m->pod.super)); > ASSERT(page_list_empty(&p2m->pod.single)); > > - if ( p2m->np2m_base == P2M_BASE_EADDR ) > - { > - p2m_unlock(p2m); > - return; > - } > - > /* This is no longer a valid nested p2m for any address space */ > p2m->np2m_base = P2M_BASE_EADDR; > > @@ -2088,7 +2082,11 @@ p2m_flush_nestedp2m(struct domain *d) > { > int i; > for ( i = 0; i < MAX_NESTEDP2M; i++ ) > - p2m_flush_table(d->arch.nested_p2m[i]); > + { > + struct p2m_domain *p2m = d->arch.nested_p2m[i]; > + if ( p2m->np2m_base != P2M_BASE_EADDR ) > + p2m_flush_table(p2m); > + } > } > > struct p2m_domain * > -- > 2.1.4 >
George / Tamas, I'm away from my dev machine so I cannot test. Thank you both for working so quickly to resolve this behavior! Best regards, --Matt On 01/30/2017 11:07 AM, Tamas K Lengyel wrote: > Hi George, > yeap, this solves old mem_access settings being triggered when I > recreate altp2m views. Thanks! > > On Mon, Jan 30, 2017 at 8:17 AM, George Dunlap <george.dunlap@citrix.com> wrote: >> Commit 71bb7304e7a7a35ea6df4b0cedebc35028e4c159 added flushing of >> nested p2m tables whenever the host p2m table changed. Unfortunately >> in the process, it added a filter to the generic p2m_flush_table() >> function so that the p2m would only be flushed if it was being used as >> a nested p2m. This meant that the p2m was not being flushed at all >> for altp2m callers. >> >> Instead do the nested p2m filtering in p2m_flush_nestedp2m(). >> >> NB that this is not a security issue: The only time this codepath is >> called is in cases where either nestedp2m or altp2m is enabled, and >> neither of them are in security support. >> >> Reported-by: Matt Leinhos <matt@starlab.io> >> Signed-off-by: George Dunlap <george.dunlap@citrix.com> >> --- >> I've smoke-tested this with nested virt and it seems to work fine. >> Matt / Tamas, could you test this with altp2m and see if it fixes your >> issue? > > Tested-by: Tamas K Lengyel <tamas.lengyel@zentific.com> > >> >> >> CC: Liang Li <liang.z.li@intel.com> >> CC: Yang Zhang <yang.z.zhang@intel.com> >> CC: Tim Deegan <tim@xen.org> >> CC: Tamas K Lengyel <tamas.lengyel@zentific.com> >> CC: Andrew Cooper <andrew.cooper3@citrix.com> >> CC: Jan Beulich <jbeulich@suse.com> >> CC: Matt Leinhos <matt@starlab.io> >> --- >> xen/arch/x86/mm/p2m.c | 12 +++++------- >> 1 file changed, 5 insertions(+), 7 deletions(-) >> >> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c >> index aa627d8..0849c6e 100644 >> --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c >> @@ -2048,12 +2048,6 @@ p2m_flush_table(struct p2m_domain *p2m) >> ASSERT(page_list_empty(&p2m->pod.super)); >> ASSERT(page_list_empty(&p2m->pod.single)); >> >> - if ( p2m->np2m_base == P2M_BASE_EADDR ) >> - { >> - p2m_unlock(p2m); >> - return; >> - } >> - >> /* This is no longer a valid nested p2m for any address space */ >> p2m->np2m_base = P2M_BASE_EADDR; >> >> @@ -2088,7 +2082,11 @@ p2m_flush_nestedp2m(struct domain *d) >> { >> int i; >> for ( i = 0; i < MAX_NESTEDP2M; i++ ) >> - p2m_flush_table(d->arch.nested_p2m[i]); >> + { >> + struct p2m_domain *p2m = d->arch.nested_p2m[i]; >> + if ( p2m->np2m_base != P2M_BASE_EADDR ) >> + p2m_flush_table(p2m); >> + } >> } >> >> struct p2m_domain * >> -- >> 2.1.4 >>
On 30/01/17 17:07, Tamas K Lengyel wrote: > Hi George, > yeap, this solves old mem_access settings being triggered when I > recreate altp2m views. Thanks! > > On Mon, Jan 30, 2017 at 8:17 AM, George Dunlap <george.dunlap@citrix.com> wrote: >> Commit 71bb7304e7a7a35ea6df4b0cedebc35028e4c159 added flushing of >> nested p2m tables whenever the host p2m table changed. Unfortunately >> in the process, it added a filter to the generic p2m_flush_table() >> function so that the p2m would only be flushed if it was being used as >> a nested p2m. This meant that the p2m was not being flushed at all >> for altp2m callers. >> >> Instead do the nested p2m filtering in p2m_flush_nestedp2m(). >> >> NB that this is not a security issue: The only time this codepath is >> called is in cases where either nestedp2m or altp2m is enabled, and >> neither of them are in security support. >> >> Reported-by: Matt Leinhos <matt@starlab.io> >> Signed-off-by: George Dunlap <george.dunlap@citrix.com> >> --- >> I've smoke-tested this with nested virt and it seems to work fine. >> Matt / Tamas, could you test this with altp2m and see if it fixes your >> issue? > > Tested-by: Tamas K Lengyel <tamas.lengyel@zentific.com> Any chance you could add a Reviewed-by to this? Can't check it in without an R-b from a non-maintainer or an Acked-by from an x86 maintainer. :-) -George
>>> On 30.01.17 at 16:17, <george.dunlap@citrix.com> wrote: > Commit 71bb7304e7a7a35ea6df4b0cedebc35028e4c159 added flushing of > nested p2m tables whenever the host p2m table changed. Unfortunately > in the process, it added a filter to the generic p2m_flush_table() > function so that the p2m would only be flushed if it was being used as > a nested p2m. This meant that the p2m was not being flushed at all > for altp2m callers. > > Instead do the nested p2m filtering in p2m_flush_nestedp2m(). > > NB that this is not a security issue: The only time this codepath is > called is in cases where either nestedp2m or altp2m is enabled, and > neither of them are in security support. > > Reported-by: Matt Leinhos <matt@starlab.io> > Signed-off-by: George Dunlap <george.dunlap@citrix.com> > --- > I've smoke-tested this with nested virt and it seems to work fine. > Matt / Tamas, could you test this with altp2m and see if it fixes your > issue? > > > CC: Liang Li <liang.z.li@intel.com> > CC: Yang Zhang <yang.z.zhang@intel.com> > CC: Tim Deegan <tim@xen.org> > CC: Tamas K Lengyel <tamas.lengyel@zentific.com> > CC: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Jan Beulich <jbeulich@suse.com> > CC: Matt Leinhos <matt@starlab.io> > --- > xen/arch/x86/mm/p2m.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index aa627d8..0849c6e 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -2048,12 +2048,6 @@ p2m_flush_table(struct p2m_domain *p2m) > ASSERT(page_list_empty(&p2m->pod.super)); > ASSERT(page_list_empty(&p2m->pod.single)); > > - if ( p2m->np2m_base == P2M_BASE_EADDR ) > - { > - p2m_unlock(p2m); > - return; > - } > - > /* This is no longer a valid nested p2m for any address space */ > p2m->np2m_base = P2M_BASE_EADDR; > > @@ -2088,7 +2082,11 @@ p2m_flush_nestedp2m(struct domain *d) > { > int i; > for ( i = 0; i < MAX_NESTEDP2M; i++ ) > - p2m_flush_table(d->arch.nested_p2m[i]); > + { > + struct p2m_domain *p2m = d->arch.nested_p2m[i]; > + if ( p2m->np2m_base != P2M_BASE_EADDR ) > + p2m_flush_table(p2m); > + } > } So the use of p2m_flush_table() in p2m_get_nestedp2m() is fine as is because the new np2m_base can't be P2M_BASE_EADDR (as a comment there says slightly indirectly). I think this may be worth clarifying in the commit message. What about p2m_flush()'es use of p2m_flush_table() though? There in particular are uses from vvmx.c and hap.c, both of which suggest nested-virt context. Jan
On 31/01/17 10:44, Jan Beulich wrote: >>>> On 30.01.17 at 16:17, <george.dunlap@citrix.com> wrote: >> Commit 71bb7304e7a7a35ea6df4b0cedebc35028e4c159 added flushing of >> nested p2m tables whenever the host p2m table changed. Unfortunately >> in the process, it added a filter to the generic p2m_flush_table() >> function so that the p2m would only be flushed if it was being used as >> a nested p2m. This meant that the p2m was not being flushed at all >> for altp2m callers. >> >> Instead do the nested p2m filtering in p2m_flush_nestedp2m(). >> >> NB that this is not a security issue: The only time this codepath is >> called is in cases where either nestedp2m or altp2m is enabled, and >> neither of them are in security support. >> >> Reported-by: Matt Leinhos <matt@starlab.io> >> Signed-off-by: George Dunlap <george.dunlap@citrix.com> >> --- >> I've smoke-tested this with nested virt and it seems to work fine. >> Matt / Tamas, could you test this with altp2m and see if it fixes your >> issue? >> >> >> CC: Liang Li <liang.z.li@intel.com> >> CC: Yang Zhang <yang.z.zhang@intel.com> >> CC: Tim Deegan <tim@xen.org> >> CC: Tamas K Lengyel <tamas.lengyel@zentific.com> >> CC: Andrew Cooper <andrew.cooper3@citrix.com> >> CC: Jan Beulich <jbeulich@suse.com> >> CC: Matt Leinhos <matt@starlab.io> >> --- >> xen/arch/x86/mm/p2m.c | 12 +++++------- >> 1 file changed, 5 insertions(+), 7 deletions(-) >> >> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c >> index aa627d8..0849c6e 100644 >> --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c >> @@ -2048,12 +2048,6 @@ p2m_flush_table(struct p2m_domain *p2m) >> ASSERT(page_list_empty(&p2m->pod.super)); >> ASSERT(page_list_empty(&p2m->pod.single)); >> >> - if ( p2m->np2m_base == P2M_BASE_EADDR ) >> - { >> - p2m_unlock(p2m); >> - return; >> - } >> - >> /* This is no longer a valid nested p2m for any address space */ >> p2m->np2m_base = P2M_BASE_EADDR; >> >> @@ -2088,7 +2082,11 @@ p2m_flush_nestedp2m(struct domain *d) >> { >> int i; >> for ( i = 0; i < MAX_NESTEDP2M; i++ ) >> - p2m_flush_table(d->arch.nested_p2m[i]); >> + { >> + struct p2m_domain *p2m = d->arch.nested_p2m[i]; >> + if ( p2m->np2m_base != P2M_BASE_EADDR ) >> + p2m_flush_table(p2m); >> + } >> } > > So the use of p2m_flush_table() in p2m_get_nestedp2m() is fine > as is because the new np2m_base can't be P2M_BASE_EADDR (as > a comment there says slightly indirectly). I think this may be worth > clarifying in the commit message. > > What about p2m_flush()'es use of p2m_flush_table() though? > There in particular are uses from vvmx.c and hap.c, both of which > suggest nested-virt context. I think the "filter" is only an optimization: If it's not there you'll just end up "clearing" an already clear table. That's the way things were before 71bb730. We could add an nestedp2m-specific wrapper function do to the test instead, and then have all nestedp2m-specific callers call it. Might be a worthwhile change. -George
On Tue, Jan 31, 2017 at 3:24 AM, George Dunlap <george.dunlap@citrix.com> wrote: > On 30/01/17 17:07, Tamas K Lengyel wrote: >> Hi George, >> yeap, this solves old mem_access settings being triggered when I >> recreate altp2m views. Thanks! >> >> On Mon, Jan 30, 2017 at 8:17 AM, George Dunlap <george.dunlap@citrix.com> wrote: >>> Commit 71bb7304e7a7a35ea6df4b0cedebc35028e4c159 added flushing of >>> nested p2m tables whenever the host p2m table changed. Unfortunately >>> in the process, it added a filter to the generic p2m_flush_table() >>> function so that the p2m would only be flushed if it was being used as >>> a nested p2m. This meant that the p2m was not being flushed at all >>> for altp2m callers. >>> >>> Instead do the nested p2m filtering in p2m_flush_nestedp2m(). >>> >>> NB that this is not a security issue: The only time this codepath is >>> called is in cases where either nestedp2m or altp2m is enabled, and >>> neither of them are in security support. >>> >>> Reported-by: Matt Leinhos <matt@starlab.io> >>> Signed-off-by: George Dunlap <george.dunlap@citrix.com> >>> --- >>> I've smoke-tested this with nested virt and it seems to work fine. >>> Matt / Tamas, could you test this with altp2m and see if it fixes your >>> issue? >> >> Tested-by: Tamas K Lengyel <tamas.lengyel@zentific.com> > > Any chance you could add a Reviewed-by to this? Can't check it in > without an R-b from a non-maintainer or an Acked-by from an x86 > maintainer. :-) > In light of the discussion with Jan I would feel better if a R-b came from him or Tim. I'm not that familiar with the nested code base.. Tamas
At 03:44 -0700 on 31 Jan (1485834259), Jan Beulich wrote: > >>> On 30.01.17 at 16:17, <george.dunlap@citrix.com> wrote: > > Commit 71bb7304e7a7a35ea6df4b0cedebc35028e4c159 added flushing of > > nested p2m tables whenever the host p2m table changed. Unfortunately > > in the process, it added a filter to the generic p2m_flush_table() > > function so that the p2m would only be flushed if it was being used as > > a nested p2m. This meant that the p2m was not being flushed at all > > for altp2m callers. > > > > Instead do the nested p2m filtering in p2m_flush_nestedp2m(). I think it would be better to fix the test in p2m_flush_table() so it understands altp2m's use of tables. That way we won't have to deal with filtering at other call sites, as Jan points out. Also, this: > > for ( i = 0; i < MAX_NESTEDP2M; i++ ) > > - p2m_flush_table(d->arch.nested_p2m[i]); > > + { > > + struct p2m_domain *p2m = d->arch.nested_p2m[i]; > > + if ( p2m->np2m_base != P2M_BASE_EADDR ) > > + p2m_flush_table(p2m); > > + } moves the check of np2m_base outside the lock. That might be OK but it's definitely a bit subtle. Cheers, Tim.
On 08/02/17 10:02, Tim Deegan wrote: > At 03:44 -0700 on 31 Jan (1485834259), Jan Beulich wrote: >>>>> On 30.01.17 at 16:17, <george.dunlap@citrix.com> wrote: >>> Commit 71bb7304e7a7a35ea6df4b0cedebc35028e4c159 added flushing of >>> nested p2m tables whenever the host p2m table changed. Unfortunately >>> in the process, it added a filter to the generic p2m_flush_table() >>> function so that the p2m would only be flushed if it was being used as >>> a nested p2m. This meant that the p2m was not being flushed at all >>> for altp2m callers. >>> >>> Instead do the nested p2m filtering in p2m_flush_nestedp2m(). > > I think it would be better to fix the test in p2m_flush_table() so it > understands altp2m's use of tables. That way we won't have to deal > with filtering at other call sites, as Jan points out. Also, this: > >>> for ( i = 0; i < MAX_NESTEDP2M; i++ ) >>> - p2m_flush_table(d->arch.nested_p2m[i]); >>> + { >>> + struct p2m_domain *p2m = d->arch.nested_p2m[i]; >>> + if ( p2m->np2m_base != P2M_BASE_EADDR ) >>> + p2m_flush_table(p2m); >>> + } > > moves the check of np2m_base outside the lock. That might be OK but > it's definitely a bit subtle. Hrm, yes that's not so great. v2 coming up. -George
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index aa627d8..0849c6e 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -2048,12 +2048,6 @@ p2m_flush_table(struct p2m_domain *p2m) ASSERT(page_list_empty(&p2m->pod.super)); ASSERT(page_list_empty(&p2m->pod.single)); - if ( p2m->np2m_base == P2M_BASE_EADDR ) - { - p2m_unlock(p2m); - return; - } - /* This is no longer a valid nested p2m for any address space */ p2m->np2m_base = P2M_BASE_EADDR; @@ -2088,7 +2082,11 @@ p2m_flush_nestedp2m(struct domain *d) { int i; for ( i = 0; i < MAX_NESTEDP2M; i++ ) - p2m_flush_table(d->arch.nested_p2m[i]); + { + struct p2m_domain *p2m = d->arch.nested_p2m[i]; + if ( p2m->np2m_base != P2M_BASE_EADDR ) + p2m_flush_table(p2m); + } } struct p2m_domain *
Commit 71bb7304e7a7a35ea6df4b0cedebc35028e4c159 added flushing of nested p2m tables whenever the host p2m table changed. Unfortunately in the process, it added a filter to the generic p2m_flush_table() function so that the p2m would only be flushed if it was being used as a nested p2m. This meant that the p2m was not being flushed at all for altp2m callers. Instead do the nested p2m filtering in p2m_flush_nestedp2m(). NB that this is not a security issue: The only time this codepath is called is in cases where either nestedp2m or altp2m is enabled, and neither of them are in security support. Reported-by: Matt Leinhos <matt@starlab.io> Signed-off-by: George Dunlap <george.dunlap@citrix.com> --- I've smoke-tested this with nested virt and it seems to work fine. Matt / Tamas, could you test this with altp2m and see if it fixes your issue? CC: Liang Li <liang.z.li@intel.com> CC: Yang Zhang <yang.z.zhang@intel.com> CC: Tim Deegan <tim@xen.org> CC: Tamas K Lengyel <tamas.lengyel@zentific.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Jan Beulich <jbeulich@suse.com> CC: Matt Leinhos <matt@starlab.io> --- xen/arch/x86/mm/p2m.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)