Re: [PATCH v1 Altp2m cleanup 2/3] Move altp2m specific functions to altp2m files.

[George Dunlap <dunlapg@umich.edu>]

On Tue, Jun 21, 2016 at 5:04 PM, Paul Lai <paul.c.lai@intel.com> wrote:
> Move altp2m specific functions to altp2m files.  This makes the code
> a little easier to read.
>
> Signed-off-by: Paul Lai <paul.c.lai@intel.com>
> ---
>  xen/arch/x86/mm/altp2m.c          | 43 +++++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/mm/hap/hap.c         | 35 +++++--------------------------
>  xen/arch/x86/mm/p2m-ept.c         | 38 ++++++++++++++++++++++++++++++++++
>  xen/arch/x86/mm/p2m.c             | 41 +------------------------------------
>  xen/include/asm-x86/altp2m.h      |  3 ++-
>  xen/include/asm-x86/hvm/vmx/vmx.h |  3 +++
>  xen/include/asm-x86/p2m.h         |  9 +++-----

You forgot to CC' the MM maintainer (me).  :-)

Please see MAINTAINERS file, and/or use get-maintainers.pl in the
future, or your patches risk dropping through the cracks.

(I've put this on my list to review now.)

 -George

>  7 files changed, 95 insertions(+), 77 deletions(-)
>
> diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c
> index 10605c8..1caf6b4 100644
> --- a/xen/arch/x86/mm/altp2m.c
> +++ b/xen/arch/x86/mm/altp2m.c
> @@ -17,6 +17,7 @@
>
>  #include <asm/hvm/support.h>
>  #include <asm/hvm/hvm.h>
> +#include <asm/domain.h>
>  #include <asm/p2m.h>
>  #include <asm/altp2m.h>
>
> @@ -65,6 +66,48 @@ altp2m_vcpu_destroy(struct vcpu *v)
>          vcpu_unpause(v);
>  }
>
> +int
> +hvm_altp2m_init( struct domain *d) {
> +    int rv = 0;
> +    unsigned int i = 0;
> +
> +    /* 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++ )
> +    {
> +        rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> +        if ( rv != 0 )
> +           goto out;
> +    }
> +
> +    d->arch.altp2m_active = 0;
> + out:
> +    return rv;
> +}
> +
> +void
> +hvm_altp2m_teardown( struct domain *d) {
> +    unsigned int i = 0;
> +    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]);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index 9c2cd49..07833b7 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -37,6 +37,7 @@
>  #include <asm/hap.h>
>  #include <asm/paging.h>
>  #include <asm/p2m.h>
> +#include <asm/altp2m.h>
>  #include <asm/domain.h>
>  #include <xen/numa.h>
>  #include <asm/hvm/nestedhvm.h>
> @@ -501,24 +502,9 @@ int hap_enable(struct domain *d, u32 mode)
>
>      if ( hvm_altp2m_supported() )
>      {
> -        /* 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++ )
> -        {
> -            rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> -            if ( rv != 0 )
> -               goto out;
> -        }
> -
> -        d->arch.altp2m_active = 0;
> +        rv = hvm_altp2m_init(d);
> +        if ( rv != 0 )
> +           goto out;
>      }
>
>      /* Now let other users see the new mode */
> @@ -534,18 +520,7 @@ void hap_final_teardown(struct domain *d)
>      unsigned int i;
>
>      if ( hvm_altp2m_supported() )
> -    {
> -        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]);
> -    }
> +        hvm_altp2m_teardown(d);
>
>      /* Destroy nestedp2m's first */
>      for (i = 0; i < MAX_NESTEDP2M; i++) {
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index 7166c71..dff34b1 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1329,6 +1329,44 @@ void setup_ept_dump(void)
>      register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0);
>  }
>
> +void p2m_init_altp2m_helper( struct domain *d, unsigned int i) {
> +    struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
> +    struct ept_data *ept;
> +
> +    p2m->min_remapped_gfn = INVALID_GFN;
> +    p2m->max_remapped_gfn = 0;
> +    ept = &p2m->ept;
> +    ept->asr = pagetable_get_pfn(p2m_get_pagetable(p2m));
> +    d->arch.altp2m_eptp[i] = ept_get_eptp(ept);
> +}
> +
> +unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
> +{
> +    struct p2m_domain *p2m;
> +    struct ept_data *ept;
> +    unsigned int i;
> +
> +    altp2m_list_lock(d);
> +
> +    for ( i = 0; i < MAX_ALTP2M; i++ )
> +    {
> +        if ( d->arch.altp2m_eptp[i] == INVALID_MFN )
> +            continue;
> +
> +        p2m = d->arch.altp2m_p2m[i];
> +        ept = &p2m->ept;
> +
> +        if ( eptp == ept_get_eptp(ept) )
> +            goto out;
> +    }
> +
> +    i = INVALID_ALTP2M;
> +
> + out:
> +    altp2m_list_unlock(d);
> +    return i;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 89462b2..90f2d95 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -196,8 +196,8 @@ static void p2m_teardown_altp2m(struct domain *d)
>          if ( !d->arch.altp2m_p2m[i] )
>              continue;
>          p2m = d->arch.altp2m_p2m[i];
> -        d->arch.altp2m_p2m[i] = NULL;
>          p2m_free_one(p2m);
> +        d->arch.altp2m_p2m[i] = NULL;
>      }
>  }
>
> @@ -2270,33 +2270,6 @@ int unmap_mmio_regions(struct domain *d,
>      return i == nr ? 0 : i ?: ret;
>  }
>
> -unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
> -{
> -    struct p2m_domain *p2m;
> -    struct ept_data *ept;
> -    unsigned int i;
> -
> -    altp2m_list_lock(d);
> -
> -    for ( i = 0; i < MAX_ALTP2M; i++ )
> -    {
> -        if ( d->arch.altp2m_eptp[i] == INVALID_MFN )
> -            continue;
> -
> -        p2m = d->arch.altp2m_p2m[i];
> -        ept = &p2m->ept;
> -
> -        if ( eptp == ept_get_eptp(ept) )
> -            goto out;
> -    }
> -
> -    i = INVALID_ALTP2M;
> -
> - out:
> -    altp2m_list_unlock(d);
> -    return i;
> -}
> -
>  bool_t p2m_switch_vcpu_altp2m_by_id(struct vcpu *v, unsigned int idx)
>  {
>      struct domain *d = v->domain;
> @@ -2402,18 +2375,6 @@ void p2m_flush_altp2m(struct domain *d)
>      altp2m_list_unlock(d);
>  }
>
> -static void p2m_init_altp2m_helper(struct domain *d, unsigned int i)
> -{
> -    struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
> -    struct ept_data *ept;
> -
> -    p2m->min_remapped_gfn = INVALID_GFN;
> -    p2m->max_remapped_gfn = 0;
> -    ept = &p2m->ept;
> -    ept->asr = pagetable_get_pfn(p2m_get_pagetable(p2m));
> -    d->arch.altp2m_eptp[i] = ept_get_eptp(ept);
> -}
> -
>  int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
>  {
>      int rc = -EINVAL;
> diff --git a/xen/include/asm-x86/altp2m.h b/xen/include/asm-x86/altp2m.h
> index 64c7618..7ce047d 100644
> --- a/xen/include/asm-x86/altp2m.h
> +++ b/xen/include/asm-x86/altp2m.h
> @@ -18,7 +18,6 @@
>  #ifndef __ASM_X86_ALTP2M_H
>  #define __ASM_X86_ALTP2M_H
>
> -#include <xen/types.h>
>  #include <xen/sched.h>         /* for struct vcpu, struct domain */
>  #include <asm/hvm/vcpu.h>      /* for vcpu_altp2m */
>
> @@ -37,5 +36,7 @@ static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v)
>  {
>      return vcpu_altp2m(v).p2midx;
>  }
> +int hvm_altp2m_init(struct domain *d);
> +void hvm_altp2m_teardown(struct domain *d);
>
>  #endif /* __ASM_X86_ALTP2M_H */
> diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
> index 359b2a9..98032fb 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -556,6 +556,9 @@ void ept_p2m_uninit(struct p2m_domain *p2m);
>  void ept_walk_table(struct domain *d, unsigned long gfn);
>  bool_t ept_handle_misconfig(uint64_t gpa);
>  void setup_ept_dump(void);
> +void p2m_init_altp2m_helper( struct domain *d, unsigned int i);
> +/* Locate an alternate p2m by its EPTP */
> +unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp);
>
>  void update_guest_eip(void);
>
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 65675a2..d7c8c12 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -23,8 +23,8 @@
>   * along with this program; If not, see <http://www.gnu.org/licenses/>.
>   */
>
> -#ifndef _XEN_P2M_H
> -#define _XEN_P2M_H
> +#ifndef _XEN_ASM_X86_P2M_H
> +#define _XEN_ASM_X86_P2M_H
>
>  #include <xen/config.h>
>  #include <xen/paging.h>
> @@ -779,9 +779,6 @@ static inline struct p2m_domain *p2m_get_altp2m(struct vcpu *v)
>      return v->domain->arch.altp2m_p2m[index];
>  }
>
> -/* Locate an alternate p2m by its EPTP */
> -unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp);
> -
>  /* Switch alternate p2m for a single vcpu */
>  bool_t p2m_switch_vcpu_altp2m_by_id(struct vcpu *v, unsigned int idx);
>
> @@ -843,7 +840,7 @@ static inline unsigned int p2m_get_iommu_flags(p2m_type_t p2mt)
>      return flags;
>  }
>
> -#endif /* _XEN_P2M_H */
> +#endif /* _XEN_ASM_X86_P2M_H */
>
>  /*
>   * Local variables:
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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