Re: [Xen-devel] [PATCH v3 05/13] x86/altp2m: basic data structures and support routines.

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/07/15 19:09, 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@xxxxxxxxx>

Only some style issues.  Otherwise, Reviewed-by: Andrew Cooper
<andrew.cooper3@xxxxxxxxxx>

> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 6faf3f4..f21d34d 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -6502,6 +6504,25 @@ enum hvm_intblk nhvm_interrupt_blocked(struct vcpu *v)
>      return hvm_funcs.nhvm_intr_blocked(v);
>  }
>  
> +void ap2m_vcpu_update_eptp(struct vcpu *v)
> +{
> +    if (hvm_funcs.ap2m_vcpu_update_eptp)

spaces inside brackets

> +        hvm_funcs.ap2m_vcpu_update_eptp(v);
> +}
> +
> +void ap2m_vcpu_update_vmfunc_ve(struct vcpu *v)
> +{
> +    if (hvm_funcs.ap2m_vcpu_update_vmfunc_ve)

spaces inside brackets

> +        hvm_funcs.ap2m_vcpu_update_vmfunc_ve(v);
> +}
> +
> +bool_t ap2m_vcpu_emulate_ve(struct vcpu *v)
> +{
> +    if (hvm_funcs.ap2m_vcpu_emulate_ve)

spaces inside brackets

> +        return hvm_funcs.ap2m_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..c00282c 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 )
> +    {
> +        rv = -ENOMEM;
> +        goto out;
> +    }
> +
> +    for (i = 0; i < MAX_EPTP; i++)
> +        d->arch.altp2m_eptp[i] = INVALID_MFN;
> +
> +    for (i = 0; i < MAX_ALTP2M; i++) {

braces

> +        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 ) {

braces

> +        free_xenheap_page(d->arch.altp2m_eptp);
> +        d->arch.altp2m_eptp = NULL;
> +    }
> +
> +    for (i = 0; i < MAX_ALTP2M; i++) {

braces

> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 1fd1194..58d4951 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/altp2m.h>
>  #include <asm/hvm/svm/amd-iommu-proto.h>
>  #include <xsm/xsm.h>
>  
> @@ -183,6 +184,43 @@ static void p2m_teardown_nestedp2m(struct domain *d)
>      }
>  }
>  
> +static void p2m_teardown_altp2m(struct domain *d)
> +{
> +    uint8_t i;

A plain unsigned int here would suffice.  It also looks curios as you
use uint16 for the same iteration elsewhere.

> +    struct p2m_domain *p2m;
> +
> +    for (i = 0; i < MAX_ALTP2M; i++)

spaces inside brackets

> +    {
> +        if ( !d->arch.altp2m_p2m[i] )
> +            continue;
> +        p2m = d->arch.altp2m_p2m[i];
> +        p2m_free_one(p2m);
> +        d->arch.altp2m_p2m[i] = NULL;
> +    }
> +}
> +
> +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++)

spaces inside brackets

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