| Message ID | 1486574546-10818-1-git-send-email-george.dunlap@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
At 17:22 +0000 on 08 Feb (1486574546), George Dunlap wrote: > Commit 71bb7304e7a7a35ea6df4b0cedebc35028e4c159 added flushing of > nested p2m tables whenever the host p2m table changed. Unfortunately > in the process, it added a filter to 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. > > Only check np2m_base if p2m_class is set to p2m_nested. > > 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> > CC: Tamas K Lengyel <tamas.lengyel@zentific.com> > --- > xen/arch/x86/mm/p2m.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index 6548e9f..0af2ec1 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -1627,7 +1627,9 @@ 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 ) > + /* No need to flush if it's already empty */ > + if ( p2m->p2m_class == p2m_nested && > + p2m->np2m_base == P2M_BASE_EADDR ) Looks like p2m_is_nestedp2m(p2m) is the usual idiom. Either way: Reviewed-by: Tim Deegan <tim@xen.org>
On 08/02/17 17:29, Tim Deegan wrote: > At 17:22 +0000 on 08 Feb (1486574546), George Dunlap wrote: >> Commit 71bb7304e7a7a35ea6df4b0cedebc35028e4c159 added flushing of >> nested p2m tables whenever the host p2m table changed. Unfortunately >> in the process, it added a filter to 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. >> >> Only check np2m_base if p2m_class is set to p2m_nested. >> >> 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> >> CC: Tamas K Lengyel <tamas.lengyel@zentific.com> >> --- >> xen/arch/x86/mm/p2m.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c >> index 6548e9f..0af2ec1 100644 >> --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c >> @@ -1627,7 +1627,9 @@ 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 ) >> + /* No need to flush if it's already empty */ >> + if ( p2m->p2m_class == p2m_nested && >> + p2m->np2m_base == P2M_BASE_EADDR ) > > Looks like p2m_is_nestedp2m(p2m) is the usual idiom. Either way: > > Reviewed-by: Tim Deegan <tim@xen.org> Right -- I'll change it to the usual idiom and check it in after I've gotten a test report (or waited a reasonable amount of time). -George
On Wed, Feb 8, 2017 at 11:12 AM, George Dunlap <george.dunlap@citrix.com> wrote: > On 08/02/17 17:29, Tim Deegan wrote: > > At 17:22 +0000 on 08 Feb (1486574546), George Dunlap wrote: > >> Commit 71bb7304e7a7a35ea6df4b0cedebc35028e4c159 added flushing of > >> nested p2m tables whenever the host p2m table changed. Unfortunately > >> in the process, it added a filter to 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. > >> > >> Only check np2m_base if p2m_class is set to p2m_nested. > >> > >> 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> > >> CC: Tamas K Lengyel <tamas.lengyel@zentific.com> > >> --- > >> xen/arch/x86/mm/p2m.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > >> index 6548e9f..0af2ec1 100644 > >> --- a/xen/arch/x86/mm/p2m.c > >> +++ b/xen/arch/x86/mm/p2m.c > >> @@ -1627,7 +1627,9 @@ 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 ) > >> + /* No need to flush if it's already empty */ > >> + if ( p2m->p2m_class == p2m_nested && > >> + p2m->np2m_base == P2M_BASE_EADDR ) > > > > Looks like p2m_is_nestedp2m(p2m) is the usual idiom. Either way: > > > > Reviewed-by: Tim Deegan <tim@xen.org> > > Right -- I'll change it to the usual idiom and check it in after I've > gotten a test report (or waited a reasonable amount of time). Tested-by: Tamas K Lengyel <tamas@tklengyel.com>
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 6548e9f..0af2ec1 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1627,7 +1627,9 @@ 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 ) + /* No need to flush if it's already empty */ + if ( p2m->p2m_class == p2m_nested && + p2m->np2m_base == P2M_BASE_EADDR ) { p2m_unlock(p2m); return;
Commit 71bb7304e7a7a35ea6df4b0cedebc35028e4c159 added flushing of nested p2m tables whenever the host p2m table changed. Unfortunately in the process, it added a filter to 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. Only check np2m_base if p2m_class is set to p2m_nested. 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> --- CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Jan Beulich <jbeulich@suse.com> CC: Tim Deegan <tim@xen.org> CC: Tamas K Lengyel <tamas.lengyel@zentific.com> --- xen/arch/x86/mm/p2m.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)