Re: [PATCH v2 04/12] x86/altp2m: basic data structures and support routines.

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 22/06/15 19:56, Ed White wrote:
> Add the basic data structures needed to support alternate p2m's and
> the functions to initialise them and tear them down.
>
> Although Intel hardware can handle 512 EPTP's per hardware thread
> concurrently, only 10 per domain are supported in this patch for
> performance reasons.
>
> The iterator in hap_enable() does need to handle 512, so that is now
> uint16_t.
>
> This change also splits the p2m lock into one lock type for altp2m's
> and another type for all other p2m's. The purpose of this is to place
> the altp2m list lock between the types, so the list lock can be
> acquired whilst holding the host p2m lock.
>
> Signed-off-by: Ed White <edmund.h.white@intel.com>
> ---
>  xen/arch/x86/hvm/Makefile           |   2 +
>  xen/arch/x86/hvm/altp2mhvm.c        |  82 ++++++++++++++++++++++++++++
>  xen/arch/x86/hvm/hvm.c              |  21 ++++++++
>  xen/arch/x86/mm/hap/hap.c           |  31 ++++++++++-
>  xen/arch/x86/mm/mm-locks.h          |  33 +++++++++++-
>  xen/arch/x86/mm/p2m.c               | 103 ++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/domain.h        |  10 ++++
>  xen/include/asm-x86/hvm/altp2mhvm.h |  38 +++++++++++++
>  xen/include/asm-x86/hvm/hvm.h       |  17 ++++++
>  xen/include/asm-x86/hvm/vcpu.h      |   9 ++++
>  xen/include/asm-x86/p2m.h           |  30 ++++++++++-
>  11 files changed, 372 insertions(+), 4 deletions(-)
>  create mode 100644 xen/arch/x86/hvm/altp2mhvm.c
>  create mode 100644 xen/include/asm-x86/hvm/altp2mhvm.h
>
> diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
> index 69af47f..da4475d 100644
> --- a/xen/arch/x86/hvm/Makefile
> +++ b/xen/arch/x86/hvm/Makefile
> @@ -1,6 +1,7 @@
>  subdir-y += svm
>  subdir-y += vmx
>  
> +obj-y += altp2mhvm.o

This file is already in a directory named hvm.  I would name it simply
altp2m.c

Similarly, altp2mhvm in function names seems redundant, where altp2m
would suffice.  We will never be in a position of offering altp2m to PV
domains.

>  obj-y += asid.o
>  obj-y += emulate.o
>  obj-y += event.o
> @@ -24,3 +25,4 @@ obj-y += vmsi.o
>  obj-y += vpic.o
>  obj-y += vpt.o
>  obj-y += vpmu.o
> +

Spurious whitespace change.

> diff --git a/xen/arch/x86/hvm/altp2mhvm.c b/xen/arch/x86/hvm/altp2mhvm.c
> new file mode 100644
> index 0000000..802fe5b
> --- /dev/null
> +++ b/xen/arch/x86/hvm/altp2mhvm.c
> @@ -0,0 +1,82 @@
> +/*
> + * Alternate p2m HVM
> + * Copyright (c) 2014, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
> + * Place - Suite 330, Boston, MA 02111-1307 USA.
> + */
> +
> +#include <asm/hvm/support.h>
> +#include <asm/hvm/hvm.h>
> +#include <asm/p2m.h>
> +#include <asm/hvm/altp2mhvm.h>
> +
> +void
> +altp2mhvm_vcpu_reset(struct vcpu *v)
> +{
> +    struct altp2mvcpu *av = &vcpu_altp2mhvm(v);
> +
> +    av->p2midx = INVALID_ALTP2M;
> +    av->veinfo_gfn = 0;
> +
> +    if ( hvm_funcs.ahvm_vcpu_reset )
> +        hvm_funcs.ahvm_vcpu_reset(v);
> +}
> +
> +int
> +altp2mhvm_vcpu_initialise(struct vcpu *v)
> +{
> +    int rc = -EOPNOTSUPP;
> +
> +    if ( v != current )
> +        vcpu_pause(v);
> +
> +    if ( !hvm_funcs.ahvm_vcpu_initialise ||
> +         (hvm_funcs.ahvm_vcpu_initialise(v) == 0) )
> +    {
> +        rc = 0;
> +        altp2mhvm_vcpu_reset(v);
> +        vcpu_altp2mhvm(v).p2midx = 0;
> +        atomic_inc(&p2m_get_altp2m(v)->active_vcpus);
> +
> +        ahvm_vcpu_update_eptp(v);
> +    }
> +
> +    if ( v != current )
> +        vcpu_unpause(v);
> +
> +    return rc;
> +}
> +
> +void
> +altp2mhvm_vcpu_destroy(struct vcpu *v)
> +{
> +    struct p2m_domain *p2m;
> +
> +    if ( v != current )
> +        vcpu_pause(v);
> +
> +    if ( hvm_funcs.ahvm_vcpu_destroy )
> +        hvm_funcs.ahvm_vcpu_destroy(v);
> +
> +    if ( (p2m = p2m_get_altp2m(v)) )
> +        atomic_dec(&p2m->active_vcpus);
> +
> +    altp2mhvm_vcpu_reset(v);
> +
> +    ahvm_vcpu_update_eptp(v);
> +    ahvm_vcpu_update_vmfunc_ve(v);
> +
> +    if ( v != current )
> +        vcpu_unpause(v);
> +}

Please put an variable block at the bottom of the file.

/*
 * Local variables:
 * mode: C
 * c-file-style: "BSD"
 * c-basic-offset: 4
 * tab-width: 4
 * indent-tabs-mode: nil
 * End:
 */

> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index af68d44..d75c12d 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -58,6 +58,7 @@
>  #include <asm/hvm/cacheattr.h>
>  #include <asm/hvm/trace.h>
>  #include <asm/hvm/nestedhvm.h>
> +#include <asm/hvm/altp2mhvm.h>
>  #include <asm/hvm/event.h>
>  #include <asm/mtrr.h>
>  #include <asm/apic.h>
> @@ -2373,6 +2374,7 @@ void hvm_vcpu_destroy(struct vcpu *v)
>  {
>      hvm_all_ioreq_servers_remove_vcpu(v->domain, v);
>  
> +    altp2mhvm_vcpu_destroy(v);
>      nestedhvm_vcpu_destroy(v);
>  
>      free_compat_arg_xlat(v);
> @@ -6486,6 +6488,25 @@ bool_t hvm_altp2m_supported()
>      return hvm_funcs.altp2m_supported;
>  }
>  
> +void ahvm_vcpu_update_eptp(struct vcpu *v)
> +{
> +    if (hvm_funcs.ahvm_vcpu_update_eptp)
> +        hvm_funcs.ahvm_vcpu_update_eptp(v);
> +}
> +
> +void ahvm_vcpu_update_vmfunc_ve(struct vcpu *v)
> +{
> +    if (hvm_funcs.ahvm_vcpu_update_vmfunc_ve)
> +        hvm_funcs.ahvm_vcpu_update_vmfunc_ve(v);
> +}
> +
> +bool_t ahvm_vcpu_emulate_ve(struct vcpu *v)
> +{
> +    if (hvm_funcs.ahvm_vcpu_emulate_ve)
> +        return hvm_funcs.ahvm_vcpu_emulate_ve(v);
> +    return 0;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index d0d3f1e..202aa42 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -459,7 +459,7 @@ void hap_domain_init(struct domain *d)
>  int hap_enable(struct domain *d, u32 mode)
>  {
>      unsigned int old_pages;
> -    uint8_t i;
> +    uint16_t i;
>      int rv = 0;
>  
>      domain_pause(d);
> @@ -498,6 +498,24 @@ int hap_enable(struct domain *d, u32 mode)
>             goto out;
>      }
>  
> +    /* Init alternate p2m data */
> +    if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )

Please use alloc_domheap_page() and map_domain_page_global() so the
allocation is accounted against the domain.

> +    {
> +        rv = -ENOMEM;
> +        goto out;
> +    }
> +
> +    for (i = 0; i < MAX_EPTP; i++)
> +        d->arch.altp2m_eptp[i] = ~0ul;
> +
> +    for (i = 0; i < MAX_ALTP2M; i++) {
> +        rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> +        if ( rv != 0 )
> +           goto out;
> +    }
> +
> +    d->arch.altp2m_active = 0;
> +
>      /* Now let other users see the new mode */
>      d->arch.paging.mode = mode | PG_HAP_enable;
>  
> @@ -510,6 +528,17 @@ void hap_final_teardown(struct domain *d)
>  {
>      uint8_t i;
>  
> +    d->arch.altp2m_active = 0;
> +
> +    if ( d->arch.altp2m_eptp ) {
> +        free_xenheap_page(d->arch.altp2m_eptp);
> +        d->arch.altp2m_eptp = NULL;
> +    }
> +
> +    for (i = 0; i < MAX_ALTP2M; i++) {
> +        p2m_teardown(d->arch.altp2m_p2m[i]);
> +    }
> +
>      /* Destroy nestedp2m's first */
>      for (i = 0; i < MAX_NESTEDP2M; i++) {
>          p2m_teardown(d->arch.nested_p2m[i]);
> diff --git a/xen/arch/x86/mm/mm-locks.h b/xen/arch/x86/mm/mm-locks.h
> index b4f035e..954b345 100644
> --- a/xen/arch/x86/mm/mm-locks.h
> +++ b/xen/arch/x86/mm/mm-locks.h
> @@ -217,7 +217,7 @@ declare_mm_lock(nestedp2m)
>  #define nestedp2m_lock(d)   mm_lock(nestedp2m, &(d)->arch.nested_p2m_lock)
>  #define nestedp2m_unlock(d) mm_unlock(&(d)->arch.nested_p2m_lock)
>  
> -/* P2M lock (per-p2m-table)
> +/* P2M lock (per-non-alt-p2m-table)
>   *
>   * This protects all queries and updates to the p2m table.
>   * Queries may be made under the read lock but all modifications
> @@ -228,7 +228,36 @@ declare_mm_lock(nestedp2m)
>   */
>  
>  declare_mm_rwlock(p2m);

Normally, we expect the declarations to be in the nesting order, and
sofar, this is the case.  Please leave a comment here explaining why the
p2m lock declaration is now removed from the rest of its implementation.

> -#define p2m_lock(p)           mm_write_lock(p2m, &(p)->lock);
> +
> +/* Alternate P2M list lock (per-domain)
> + *
> + * A per-domain lock that protects the list of alternate p2m's.
> + * Any operation that walks the list needs to acquire this lock.
> + * Additionally, before destroying an alternate p2m all VCPU's
> + * in the target domain must be paused.  */
> +
> +declare_mm_lock(altp2mlist)
> +#define altp2m_lock(d)   mm_lock(altp2mlist, &(d)->arch.altp2m_lock)
> +#define altp2m_unlock(d) mm_unlock(&(d)->arch.altp2m_lock)
> +
> +/* P2M lock (per-altp2m-table)
> + *
> + * This protects all queries and updates to the p2m table.
> + * Queries may be made under the read lock but all modifications
> + * need the main (write) lock.
> + *
> + * The write lock is recursive as it is common for a code path to look
> + * up a gfn and later mutate it.
> + */
> +
> +declare_mm_rwlock(altp2m);
> +#define p2m_lock(p)                         \
> +{                                           \
> +    if ( p2m_is_altp2m(p) )                 \
> +        mm_write_lock(altp2m, &(p)->lock);  \
> +    else                                    \
> +        mm_write_lock(p2m, &(p)->lock);     \
> +}
>  #define p2m_unlock(p)         mm_write_unlock(&(p)->lock);
>  #define gfn_lock(p,g,o)       p2m_lock(p)
>  #define gfn_unlock(p,g,o)     p2m_unlock(p)
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 1fd1194..87b4b75 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -35,6 +35,7 @@
>  #include <asm/hvm/vmx/vmx.h> /* ept_p2m_init() */
>  #include <asm/mem_sharing.h>
>  #include <asm/hvm/nestedhvm.h>
> +#include <asm/hvm/altp2mhvm.h>
>  #include <asm/hvm/svm/amd-iommu-proto.h>
>  #include <xsm/xsm.h>
>  
> @@ -183,6 +184,45 @@ static void p2m_teardown_nestedp2m(struct domain *d)
>      }
>  }
>  
> +static void p2m_teardown_altp2m(struct domain *d);

You can avoid this forward declaration by moving the implementation of
p2m_teardown_altp2m() up here.

> +
> +static int p2m_init_altp2m(struct domain *d)
> +{
> +    uint8_t i;
> +    struct p2m_domain *p2m;
> +
> +    mm_lock_init(&d->arch.altp2m_lock);
> +    for (i = 0; i < MAX_ALTP2M; i++)
> +    {
> +        d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d);
> +        if ( p2m == NULL )
> +        {
> +            p2m_teardown_altp2m(d);
> +            return -ENOMEM;
> +        }
> +        p2m->p2m_class = p2m_alternate;
> +        p2m->access_required = 1;
> +        _atomic_set(&p2m->active_vcpus, 0);
> +    }
> +
> +    return 0;
> +}
> +
> +static void p2m_teardown_altp2m(struct domain *d)
> +{
> +    uint8_t i;
> +    struct p2m_domain *p2m;
> +
> +    for (i = 0; i < MAX_ALTP2M; i++)
> +    {
> +        if ( !d->arch.altp2m_p2m[i] )
> +            continue;
> +        p2m = d->arch.altp2m_p2m[i];
> +        p2m_free_one(p2m);
> +        d->arch.altp2m_p2m[i] = NULL;
> +    }
> +}
> +
>  int p2m_init(struct domain *d)
>  {
>      int rc;
> @@ -196,7 +236,14 @@ int p2m_init(struct domain *d)
>       * (p2m_init runs too early for HVM_PARAM_* options) */
>      rc = p2m_init_nestedp2m(d);
>      if ( rc )
> +    {
>          p2m_teardown_hostp2m(d);
> +        return rc;
> +    }
> +
> +    rc = p2m_init_altp2m(d);
> +    if ( rc )
> +        p2m_teardown_altp2m(d);
>  
>      return rc;
>  }
> @@ -1920,6 +1967,62 @@ int unmap_mmio_regions(struct domain *d,
>      return err;
>  }
>  
> +bool_t p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp, unsigned long *idx)

Wouldn't it be better to return index directly, using INVALID_ALTP2M as
a sentinel, rather than returning idx via pointer?

~Andrew