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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.01.17 at 16:17, <george.dunlap@xxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> ---
> 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@xxxxxxxxx>
> CC: Yang Zhang <yang.z.zhang@xxxxxxxxx>
> CC: Tim Deegan <tim@xxxxxxx>
> CC: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Matt Leinhos <matt@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel