[PATCH Altp2m cleanup v8] Cleaning up altp2m code

[PATCH Altp2m cleanup v8] Cleaning up altp2m code

[Paul Lai]

Altp2m cleanup work

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.
Lots of function return type corrections (and checking).
Just using 'return' after a if() clause instead of using a goto
if the block is can be a one liner.

Paul Lai (1):
  Move altp2m specific functions to altp2m files.

 xen/arch/x86/mm/altp2m.c          | 57 +++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/mm/hap/hap.c         | 38 +++++++-------------------
 xen/arch/x86/mm/p2m-ept.c         | 39 +++++++++++++++++++++++++++
 xen/arch/x86/mm/p2m.c             | 45 +++----------------------------
 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, 117 insertions(+), 78 deletions(-)

-- 
2.7.4


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

[PATCH Altp2m cleanup v8] Move altp2m specific functions to altp2m files.

[Paul Lai]

This makes the code a little easier to read.
Moving hvm_altp2m_supported() check into functions that use it
for better readability.
Moving ept code to ept specific files as requested in:
  https://lists.xenproject.org/archives/html/xen-devel/2015-07/msg04323.html
Renamed p2m_init_altp2m_helper() to p2m_init_altp2m_ept().
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>
---
v8 of this patch.
No change since v4 since we've just focused on patch #1 in this series.
---
 xen/arch/x86/mm/altp2m.c          | 57 +++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/mm/hap/hap.c         | 38 +++++++-------------------
 xen/arch/x86/mm/p2m-ept.c         | 39 +++++++++++++++++++++++++++
 xen/arch/x86/mm/p2m.c             | 45 +++----------------------------
 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, 117 insertions(+), 78 deletions(-)

diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c
index 930bdc2..62801ae 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,62 @@ altp2m_vcpu_destroy(struct vcpu *v)
 }
 
 /*
+ * altp2m_domain_init(struct domain *d))
+ *  allocate and initialize memory for altp2m portion of domain
+ *
+ *  returns < 0 on error
+ *  returns 0 on no operation (success)
+ *  returns > 0 on success
+ */
+int
+altp2m_domain_init(struct domain *d)
+{
+    int rc;
+    unsigned int i;
+
+    if ( d == NULL)
+        return 0;
+
+    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 )
+           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;
+
+    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
  * c-file-style: "BSD"
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 3218fa2..61c0c42 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>
@@ -499,26 +500,17 @@ int hap_enable(struct domain *d, u32 mode)
            goto out;
     }
 
-    if ( hvm_altp2m_supported() )
+    if ( (rv = altp2m_domain_init(d)) < 0 )
     {
-        /* Init alternate p2m data */
-        if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
-        {
-            rv = -ENOMEM;
-            goto out;
+        for (i = 0; i < MAX_NESTEDP2M; i++) {
+            p2m_teardown(d->arch.nested_p2m[i]);
         }
 
-        for ( i = 0; i < MAX_EPTP; i++ )
-            d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
-
-        for ( i = 0; i < MAX_ALTP2M; i++ )
-        {
-            rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
-            if ( rv != 0 )
-               goto out;
-        }
+        if ( d->arch.paging.hap.total_pages != 0 )
+            hap_teardown(d, NULL);
 
-        d->arch.altp2m_active = 0;
+        p2m_teardown(p2m_get_hostp2m(d));
+        goto out;
     }
 
     /* Now let other users see the new mode */
@@ -533,19 +525,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/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 9526fff..173be24 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -197,8 +197,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;
     }
 }
 
@@ -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/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 */
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 v8] Move altp2m specific functions to altp2m files.

[Jan Beulich]

>>> On 04.10.16 at 20:13, <paul.c.lai@intel.com> wrote:
> v8 of this patch.
> No change since v4 since we've just focused on patch #1 in this series.

Well, not exactly: I did repeatedly mention that comments made
there should be applied to the other parts of the original series.
Anyway...

> @@ -66,6 +67,62 @@ altp2m_vcpu_destroy(struct vcpu *v)
>  }
>  
>  /*
> + * altp2m_domain_init(struct domain *d))

I don't see the point of repeating the name in the comment here. In
any event there's a stray closing parenthesis.

> + *  allocate and initialize memory for altp2m portion of domain
> + *
> + *  returns < 0 on error
> + *  returns 0 on no operation (success)
> + *  returns > 0 on success

I can't spot any case of this.

> + */
> +int
> +altp2m_domain_init(struct domain *d)
> +{
> +    int rc;
> +    unsigned int i;
> +
> +    if ( d == NULL)

Missing blank.

> +        return 0;
> +
> +    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 )
> +           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;
> +
> +    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]);

As said before - I think you want to switch these two steps around,
even if right now their order if benign. This would (a) mirror the
order used during init (read: doing the inverse here) and (b) make
sure code called out of p2m_teardown() could still access the other
data structure if need be.

> @@ -499,26 +500,17 @@ int hap_enable(struct domain *d, u32 mode)
>             goto out;
>      }
>  
> -    if ( hvm_altp2m_supported() )
> +    if ( (rv = altp2m_domain_init(d)) < 0 )
>      {
> -        /* Init alternate p2m data */
> -        if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> -        {
> -            rv = -ENOMEM;
> -            goto out;
> +        for (i = 0; i < MAX_NESTEDP2M; i++) {

Coding style.

> +            p2m_teardown(d->arch.nested_p2m[i]);
>          }
>  
> -        for ( i = 0; i < MAX_EPTP; i++ )
> -            d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
> -
> -        for ( i = 0; i < MAX_ALTP2M; i++ )
> -        {
> -            rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> -            if ( rv != 0 )
> -               goto out;
> -        }
> +        if ( d->arch.paging.hap.total_pages != 0 )
> +            hap_teardown(d, NULL);
>  
> -        d->arch.altp2m_active = 0;
> +        p2m_teardown(p2m_get_hostp2m(d));
> +        goto out;
>      }

The portions of code removed in this hunk went into
altp2m_domain_init(). The additions, however, are unexplained in
the commit message and I'm not convinced they actually properly
deal with the previously missing error cleanup. In particular I don't
see how partial success of altp2m_domain_init() is being dealt with.

Also I continue to be of the opinion that most of the changes below
here could actually be in a separate patch.

> @@ -197,8 +197,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;

I think I've been puzzled by this change before, and I continue to
be. If this is a necessary change for something, it should be
mentioned in the commit message.

Jan

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