[PATCH] xen/p2m: Remove np2m-specific filter from generic p2m_flush_table

[PATCH] xen/p2m: Remove np2m-specific filter from generic p2m_flush_table

[George Dunlap]

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);
+    }
 }
 
 struct p2m_domain *
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen/p2m: Remove np2m-specific filter from generic p2m_flush_table

[Tamas K Lengyel]

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
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen/p2m: Remove np2m-specific filter from generic p2m_flush_table

[Matt Leinhos]

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
>>

-- 
Matt Leinhos
Star Lab
https://starlab.io/

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen/p2m: Remove np2m-specific filter from generic p2m_flush_table

[George Dunlap]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen/p2m: Remove np2m-specific filter from generic p2m_flush_table

[Jan Beulich]

>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen/p2m: Remove np2m-specific filter from generic p2m_flush_table

[George Dunlap]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen/p2m: Remove np2m-specific filter from generic p2m_flush_table

[Tamas K Lengyel]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen/p2m: Remove np2m-specific filter from generic p2m_flush_table

[Tim Deegan]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] xen/p2m: Remove np2m-specific filter from generic p2m_flush_table

[George Dunlap]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel