[PATCH Altp2m cleanup 2/3 v12 0/3] altp2m cleanup

[PATCH Altp2m cleanup 2/3 v12 0/3] altp2m cleanup

[Paul Lai]

The altp2m clean work is motivated by the following URLs:
   https://lists.xenproject.org/archives/html/xen-devel/2015-07/msg04454.html

Most of the work has been:
Lots of white space, indentation, and other coding style preference
corrections.
Lots of moving altp2m functions to the altp2m file.
Lots of moving ept functions to the ept file.
Fixing memory leaks in the hap_enable() memory allocation failures.


Paul Lai (3):
  Move altp2m specific functions to altp2m files.
  Altp2m cleanup: cleaning up partial memory allocations in
    hap_enable().
  Moving ept code to ept specific files.

 xen/arch/x86/mm/altp2m.c          | 55 +++++++++++++++++++++++++++++++++++
 xen/arch/x86/mm/hap/hap.c         | 61 ++++++++++++++++-----------------------
 xen/arch/x86/mm/p2m-ept.c         | 39 +++++++++++++++++++++++++
 xen/arch/x86/mm/p2m.c             | 43 ++-------------------------
 xen/include/asm-x86/altp2m.h      |  4 ++-
 xen/include/asm-x86/hvm/vmx/vmx.h |  3 ++
 xen/include/asm-x86/p2m.h         |  9 ++----
 7 files changed, 130 insertions(+), 84 deletions(-)

--
since v11

Fixed bug(s) in the memory leak error paths of hap_enable().
Consolidated the memory error path into a single code path for better
maintainability.
Created seperate patch for the memory error cleanup in hap_enable() for easier
backportability.

since v10

Removed the check for d == 0 at the head of altp2m_domain_init() per request.

Fixing code style issues missed in v10.  It was apparent that our eyes
are failing to see code style issues, so we started using some vim scripts
to assist.  Here's one for spaces after parens:
  pclai@pclaidev:/rhel-home/pclai/p2m_new/xen/output$ cat ~/vim_xen
  :highlight ForIfLParenNoSpace ctermbg=red guibg=red
  :highlight ForIfRParenNoSpace ctermbg=red guibg=red
  :match ForIfLParenNoSpace /['for''if'] ([a-zA-Z0-9]/
  :2match ForIfRParenNoSpace /['for''if'] .*[a-zA-Z0-9-+]+[)]$/

Here's another to catch dangling left curly braces:
  pclai@pclaidev:/rhel-home/pclai/p2m_new/xen/output$ cat ~/vim_xen_curly
  :highlight RParenCurly ctermbg=red guibg=red
  :match RParenCurly /) *[{]$/

We specifically unraval p2m_alloc_table() and hap_set_allocation() by
hand instead of calling p2m_teardown().
-- 
2.7.4


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

[PATCH Altp2m cleanup 2/3 v12 1/3] Move altp2m specific functions to altp2m files.

[Paul Lai]

Moving altp2m domain startup and teardown into altp2m_domain_init()
and altp2m_domain_teardown() respectively.
Moving hvm_altp2m_supported() check into functions that use it
for better readability.
Got rid of stray blanks after open paren after function names.
Defining _XEN_ASM_X86_P2M_H instead of _XEN_P2M_H for
xen/include/asm-x86/p2m.h.

Signed-off-by: Paul Lai <paul.c.lai@intel.com>
---
 xen/arch/x86/mm/altp2m.c     | 55 ++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/mm/hap/hap.c    | 14 +----------
 xen/include/asm-x86/altp2m.h |  4 +++-
 3 files changed, 59 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c
index 930bdc2..317c8f7 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>
 
@@ -66,6 +67,60 @@ altp2m_vcpu_destroy(struct vcpu *v)
 }
 
 /*
+ *  allocate and initialize memory for altp2m portion of domain
+ *
+ *  returns < 0 on error
+ *  returns 0 on no operation & success
+ */
+int
+altp2m_domain_init(struct domain *d)
+{
+    int rc;
+    unsigned int i;
+
+    if ( !hvm_altp2m_supported() )
+        return 0;
+
+    /* Init alternate p2m data. */
+    if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
+        return -ENOMEM;
+
+    for ( i = 0; i < MAX_EPTP; i++ )
+        d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
+
+    for ( i = 0; i < MAX_ALTP2M; i++ )
+    {
+        rc = p2m_alloc_table(d->arch.altp2m_p2m[i]);
+        if ( rc != 0 )
+        {
+            altp2m_domain_teardown(d);
+            return rc;
+        }
+    }
+
+    d->arch.altp2m_active = 0;
+
+    return rc;
+}
+
+void
+altp2m_domain_teardown(struct domain *d)
+{
+    unsigned int i;
+
+    if ( !hvm_altp2m_supported() )
+        return;
+
+    d->arch.altp2m_active = 0;
+
+    for ( i = 0; i < MAX_ALTP2M; i++ )
+        p2m_teardown(d->arch.altp2m_p2m[i]);
+
+    free_xenheap_page(d->arch.altp2m_eptp);
+    d->arch.altp2m_eptp = NULL;
+}
+
+/*
  * Local variables:
  * mode: C
  * c-file-style: "BSD"
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 3218fa2..7fe6f83 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -533,19 +533,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]);
-    }
+    altp2m_domain_teardown(d);
 
     /* Destroy nestedp2m's first */
     for (i = 0; i < MAX_NESTEDP2M; i++) {
diff --git a/xen/include/asm-x86/altp2m.h b/xen/include/asm-x86/altp2m.h
index 64c7618..0090c89 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 */
 
@@ -38,4 +37,7 @@ static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v)
     return vcpu_altp2m(v).p2midx;
 }
 
+int altp2m_domain_init(struct domain *d);
+void altp2m_domain_teardown(struct domain *d);
+
 #endif /* __ASM_X86_ALTP2M_H */
-- 
2.7.4


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

[PATCH Altp2m cleanup 2/3 v12 2/3] Altp2m cleanup: cleaning up partial memory allocations in hap_enable().

[Paul Lai]

In hap_enable(), clean up memory leaks upon failure of altp2m_domain_init().
Comment the memory error handling to help match allocs with cleanups.
Consolidate the memory error handing into single code path.
---
 xen/arch/x86/mm/hap/hap.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 7fe6f83..e63ffb0 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>
@@ -488,43 +489,44 @@ int hap_enable(struct domain *d, u32 mode)
     /* allocate P2m table */
     if ( mode & PG_translate )
     {
+        /* p2m_alloc_table() #1 */
         rv = p2m_alloc_table(p2m_get_hostp2m(d));
         if ( rv != 0 )
             goto out;
     }
 
     for (i = 0; i < MAX_NESTEDP2M; i++) {
+        /* nested p2m_alloc_table() #2 */
         rv = p2m_alloc_table(d->arch.nested_p2m[i]);
         if ( rv != 0 )
            goto out;
     }
 
-    if ( hvm_altp2m_supported() )
-    {
-        /* Init alternate p2m data */
-        if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
-        {
-            rv = -ENOMEM;
-            goto out;
-        }
+    if ( (rv = altp2m_domain_init(d)) < 0 )
+        goto out;
 
-        for ( i = 0; i < MAX_EPTP; i++ )
-            d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
+    /* Now let other users see the new mode */
+    d->arch.paging.mode = mode | PG_HAP_enable;
 
-        for ( i = 0; i < MAX_ALTP2M; i++ )
-        {
-            rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
-            if ( rv != 0 )
-               goto out;
-        }
+    domain_unpause(d);
+    return rv;
 
-        d->arch.altp2m_active = 0;
+ out:
+    /* Unravel the partial nested p2m_alloc_table() #2 above */
+    for ( i = 0; i < MAX_NESTEDP2M; i++ )
+    {
+        p2m_teardown(d->arch.nested_p2m[i]);
     }
 
-    /* Now let other users see the new mode */
-    d->arch.paging.mode = mode | PG_HAP_enable;
+    /* Unravel p2m_alloc_table() #1 above */
+    p2m_teardown(p2m_get_hostp2m(d));
+
+    /* Unravel the hap_set_allocation(d, 256, NULL) above */
+    paging_lock(d);
+    hap_set_allocation(d, 0, NULL);
+    ASSERT(d->arch.paging.hap.p2m_pages == 0);
+    paging_unlock(d);
 
- out:
     domain_unpause(d);
     return rv;
 }
-- 
2.7.4


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

[PATCH Altp2m cleanup 2/3 v12 3/3] Moving ept code to ept specific files.

[Paul Lai]

The was requested in:
  https://lists.xenproject.org/archives/html/xen-devel/2015-07/msg04323.html
Renamed p2m_init_altp2m_helper() to p2m_init_altp2m_ept().

Signed-off-by: Paul Lai <paul.c.lai@intel.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/x86/mm/p2m-ept.c         | 39 +++++++++++++++++++++++++++++++++++
 xen/arch/x86/mm/p2m.c             | 43 ++-------------------------------------
 xen/include/asm-x86/hvm/vmx/vmx.h |  3 +++
 xen/include/asm-x86/p2m.h         |  9 +++-----
 4 files changed, 47 insertions(+), 47 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 13cab24..04878f5 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1329,6 +1329,45 @@ void setup_ept_dump(void)
     register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0);
 }
 
+void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
+{
+    struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
+    struct ept_data *ept;
+
+    p2m->min_remapped_gfn = gfn_x(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] == mfn_x(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 6a45185..aa627d8 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2342,33 +2342,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] == mfn_x(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;
@@ -2474,18 +2447,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 = gfn_x(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;
@@ -2497,7 +2458,7 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
 
     if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
     {
-        p2m_init_altp2m_helper(d, idx);
+        p2m_init_altp2m_ept(d, idx);
         rc = 0;
     }
 
@@ -2517,7 +2478,7 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
         if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
             continue;
 
-        p2m_init_altp2m_helper(d, i);
+        p2m_init_altp2m_ept(d, i);
         *idx = i;
         rc = 0;
 
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index 4cdd9b1..9f4c7de 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -561,6 +561,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_ept(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 7035860..0e72880 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>
@@ -784,9 +784,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);
 
@@ -848,7 +845,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:
-- 
2.7.4


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

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

[Jan Beulich]

>>> On 11.11.16 at 00:45, <paul.c.lai@intel.com> wrote:
> @@ -66,6 +67,60 @@ altp2m_vcpu_destroy(struct vcpu *v)
>  }
>  
>  /*
> + *  allocate and initialize memory for altp2m portion of domain
> + *
> + *  returns < 0 on error
> + *  returns 0 on no operation & success
> + */
> +int
> +altp2m_domain_init(struct domain *d)
> +{
> +    int rc;
> +    unsigned int i;
> +
> +    if ( !hvm_altp2m_supported() )
> +        return 0;
> +
> +    /* Init alternate p2m data. */
> +    if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> +        return -ENOMEM;
> +
> +    for ( i = 0; i < MAX_EPTP; i++ )
> +        d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
> +
> +    for ( i = 0; i < MAX_ALTP2M; i++ )
> +    {
> +        rc = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> +        if ( rc != 0 )
> +        {
> +            altp2m_domain_teardown(d);
> +            return rc;
> +        }
> +    }
> +
> +    d->arch.altp2m_active = 0;
> +
> +    return rc;
> +}

This new function has no caller, and hence does not replace any
existing code (rendering the description only partly correct).

Jan


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

Re: [PATCH Altp2m cleanup 2/3 v12 2/3] Altp2m cleanup: cleaning up partial memory allocations in hap_enable().

[Jan Beulich]

>>> On 11.11.16 at 00:45, <paul.c.lai@intel.com> wrote:
> @@ -488,43 +489,44 @@ int hap_enable(struct domain *d, u32 mode)
>      /* allocate P2m table */
>      if ( mode & PG_translate )
>      {
> +        /* p2m_alloc_table() #1 */
>          rv = p2m_alloc_table(p2m_get_hostp2m(d));
>          if ( rv != 0 )
>              goto out;
>      }
>  
>      for (i = 0; i < MAX_NESTEDP2M; i++) {
> +        /* nested p2m_alloc_table() #2 */

I don't consider these comments particularly useful.

>          rv = p2m_alloc_table(d->arch.nested_p2m[i]);
>          if ( rv != 0 )
>             goto out;
>      }
>  
> -    if ( hvm_altp2m_supported() )
> -    {
> -        /* Init alternate p2m data */
> -        if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> -        {
> -            rv = -ENOMEM;
> -            goto out;
> -        }
> +    if ( (rv = altp2m_domain_init(d)) < 0 )
> +        goto out;

Hmm, so you at once switch to the new function here. I think I've
been quite clear on previous rounds that the bug fix should be
first in the series, to aid potential backporting. That said, the
description still does not make clear that there is any issue here
in the first place. I can only re-iterate: Whether there's a leak to
be fixed here depends on how this function and any error
cleanup up the call stack leading here interact. And in order to
determine whether the patch here is a bug fix or mere cleanup
(and maybe unnecessary, due to only making the code bigger
and hence [slightly] harder to read), this is imperative to know.
I think you understand that the decision whether to backport
such a change also depends on knowing whether it fixes an
actual bug.

> -        for ( i = 0; i < MAX_EPTP; i++ )
> -            d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
> +    /* Now let other users see the new mode */
> +    d->arch.paging.mode = mode | PG_HAP_enable;
>  
> -        for ( i = 0; i < MAX_ALTP2M; i++ )
> -        {
> -            rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> -            if ( rv != 0 )
> -               goto out;
> -        }
> +    domain_unpause(d);
> +    return rv;
>  
> -        d->arch.altp2m_active = 0;
> + out:
> +    /* Unravel the partial nested p2m_alloc_table() #2 above */
> +    for ( i = 0; i < MAX_NESTEDP2M; i++ )
> +    {
> +        p2m_teardown(d->arch.nested_p2m[i]);
>      }

Unnecessary braces.

Jan


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

Re: [PATCH Altp2m cleanup 2/3 v12 3/3] Moving ept code to ept specific files.

[George Dunlap]

On 10/11/16 23:45, Paul Lai wrote:
> The was requested in:
>   https://lists.xenproject.org/archives/html/xen-devel/2015-07/msg04323.html
> Renamed p2m_init_altp2m_helper() to p2m_init_altp2m_ept().
> 
> Signed-off-by: Paul Lai <paul.c.lai@intel.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

I think ideally the request reference should go below a '---' at the
bottom of the commit message: It's information to the reviewers, but
it's not really that relevant to archaeologists (and the link is not
unlikely to be broken anyway).

That can be fixed up on check-in, so:

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

> ---
>  xen/arch/x86/mm/p2m-ept.c         | 39 +++++++++++++++++++++++++++++++++++
>  xen/arch/x86/mm/p2m.c             | 43 ++-------------------------------------
>  xen/include/asm-x86/hvm/vmx/vmx.h |  3 +++
>  xen/include/asm-x86/p2m.h         |  9 +++-----
>  4 files changed, 47 insertions(+), 47 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index 13cab24..04878f5 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1329,6 +1329,45 @@ void setup_ept_dump(void)
>      register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0);
>  }
>  
> +void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
> +{
> +    struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
> +    struct ept_data *ept;
> +
> +    p2m->min_remapped_gfn = gfn_x(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] == mfn_x(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 6a45185..aa627d8 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2342,33 +2342,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] == mfn_x(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;
> @@ -2474,18 +2447,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 = gfn_x(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;
> @@ -2497,7 +2458,7 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
>  
>      if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>      {
> -        p2m_init_altp2m_helper(d, idx);
> +        p2m_init_altp2m_ept(d, idx);
>          rc = 0;
>      }
>  
> @@ -2517,7 +2478,7 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
>          if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
>              continue;
>  
> -        p2m_init_altp2m_helper(d, i);
> +        p2m_init_altp2m_ept(d, i);
>          *idx = i;
>          rc = 0;
>  
> diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
> index 4cdd9b1..9f4c7de 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -561,6 +561,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_ept(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 7035860..0e72880 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>
> @@ -784,9 +784,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);
>  
> @@ -848,7 +845,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:
> 


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

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

[George Dunlap]

On 10/11/16 23:45, Paul Lai wrote:
> Moving altp2m domain startup and teardown into altp2m_domain_init()
> and altp2m_domain_teardown() respectively.

You're not "moving" the startup into a function unless the new function
appears *and* the old code disappears.

I think it would be better to have the function introduced in the next
patch, so that it's easier to verify that the removed code and the added
code are doing the same thing.

Everything else looks good to me, thanks.

 -George

> Moving hvm_altp2m_supported() check into functions that use it
> for better readability.
> Got rid of stray blanks after open paren after function names.
> Defining _XEN_ASM_X86_P2M_H instead of _XEN_P2M_H for
> xen/include/asm-x86/p2m.h.
> 
> Signed-off-by: Paul Lai <paul.c.lai@intel.com>
> ---
>  xen/arch/x86/mm/altp2m.c     | 55 ++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/mm/hap/hap.c    | 14 +----------
>  xen/include/asm-x86/altp2m.h |  4 +++-
>  3 files changed, 59 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c
> index 930bdc2..317c8f7 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>
>  
> @@ -66,6 +67,60 @@ altp2m_vcpu_destroy(struct vcpu *v)
>  }
>  
>  /*
> + *  allocate and initialize memory for altp2m portion of domain
> + *
> + *  returns < 0 on error
> + *  returns 0 on no operation & success
> + */
> +int
> +altp2m_domain_init(struct domain *d)
> +{
> +    int rc;
> +    unsigned int i;
> +
> +    if ( !hvm_altp2m_supported() )
> +        return 0;
> +
> +    /* Init alternate p2m data. */
> +    if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> +        return -ENOMEM;
> +
> +    for ( i = 0; i < MAX_EPTP; i++ )
> +        d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
> +
> +    for ( i = 0; i < MAX_ALTP2M; i++ )
> +    {
> +        rc = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> +        if ( rc != 0 )
> +        {
> +            altp2m_domain_teardown(d);
> +            return rc;
> +        }
> +    }
> +
> +    d->arch.altp2m_active = 0;
> +
> +    return rc;
> +}
> +
> +void
> +altp2m_domain_teardown(struct domain *d)
> +{
> +    unsigned int i;
> +
> +    if ( !hvm_altp2m_supported() )
> +        return;
> +
> +    d->arch.altp2m_active = 0;
> +
> +    for ( i = 0; i < MAX_ALTP2M; i++ )
> +        p2m_teardown(d->arch.altp2m_p2m[i]);
> +
> +    free_xenheap_page(d->arch.altp2m_eptp);
> +    d->arch.altp2m_eptp = NULL;
> +}
> +
> +/*
>   * Local variables:
>   * mode: C
>   * c-file-style: "BSD"
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index 3218fa2..7fe6f83 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -533,19 +533,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]);
> -    }
> +    altp2m_domain_teardown(d);
>  
>      /* Destroy nestedp2m's first */
>      for (i = 0; i < MAX_NESTEDP2M; i++) {
> diff --git a/xen/include/asm-x86/altp2m.h b/xen/include/asm-x86/altp2m.h
> index 64c7618..0090c89 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 */
>  
> @@ -38,4 +37,7 @@ static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v)
>      return vcpu_altp2m(v).p2midx;
>  }
>  
> +int altp2m_domain_init(struct domain *d);
> +void altp2m_domain_teardown(struct domain *d);
> +
>  #endif /* __ASM_X86_ALTP2M_H */
> 


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