Re: [PATCH RFC 02/12] x86/np2m: add np2m_flush_eptp()

[George Dunlap <george.dunlap@citrix.com>]

On 07/18/2017 11:34 AM, Sergey Dyasli wrote:
> The new function finds all np2m objects with the specified eptp and
> flushes them. p2m_flush_table_locked() is added in order not to release
> the p2m lock after np2m_base check.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>

This patch looks plausible except for...

> ---
>  xen/arch/x86/mm/p2m.c     | 34 +++++++++++++++++++++++++++++++---
>  xen/include/asm-x86/p2m.h |  2 ++
>  2 files changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index b8c8bba421..bc330d8f52 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1708,15 +1708,14 @@ p2m_getlru_nestedp2m(struct domain *d, struct p2m_domain *p2m)
>      return p2m;
>  }
>  
> -/* Reset this p2m table to be empty */
>  static void
> -p2m_flush_table(struct p2m_domain *p2m)
> +p2m_flush_table_locked(struct p2m_domain *p2m)
>  {
>      struct page_info *top, *pg;
>      struct domain *d = p2m->domain;
>      mfn_t mfn;
>  
> -    p2m_lock(p2m);
> +    ASSERT(p2m_locked_by_me(p2m));
>  
>      /*
>       * "Host" p2m tables can have shared entries &c that need a bit more care
> @@ -1756,6 +1755,14 @@ p2m_flush_table(struct p2m_domain *p2m)
>      p2m_unlock(p2m);

...this.  Why on earth would we unlock this at the end of the function,
instead of letting the caller do it?

If we were to do that, at very least it should be called
"p2m_flush_and_unlock_table()".  But I think we just want to leave the
unlock out, because...

>  }
>  
> +/* Reset this p2m table to be empty */
> +static void
> +p2m_flush_table(struct p2m_domain *p2m)
> +{
> +    p2m_lock(p2m);
> +    p2m_flush_table_locked(p2m);

..this looks wrong -- a balanced lock/unlock is easier to verify, and...

> +}
> +
>  void
>  p2m_flush(struct vcpu *v, struct p2m_domain *p2m)
>  {
> @@ -1773,6 +1780,27 @@ p2m_flush_nestedp2m(struct domain *d)
>          p2m_flush_table(d->arch.nested_p2m[i]);
>  }
>  
> +void np2m_flush_eptp(struct vcpu *v, unsigned long eptp)
> +{
> +    struct domain *d = v->domain;
> +    struct p2m_domain *p2m;
> +    unsigned int i;
> +
> +    eptp &= ~(0xfffull);
> +
> +    nestedp2m_lock(d);
> +    for ( i = 0; i < MAX_NESTEDP2M; i++ )
> +    {
> +        p2m = d->arch.nested_p2m[i];
> +        p2m_lock(p2m);
> +        if ( p2m->np2m_base == eptp )
> +            p2m_flush_table_locked(p2m);
> +        else
> +            p2m_unlock(p2m);

...here we have essentially a pointless 'else'.  Lock/unlock around the
whole conditional would make much more sense.

 -George

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