[PATCH Altp2m cleanup 2/3 v10 0/2] Cleaning up altp2m code

[PATCH Altp2m cleanup 2/3 v10 0/2] Cleaning up altp2m code

[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.

Paul Lai (2):
  Move altp2m specific functions to altp2m files.
  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().

 xen/arch/x86/mm/altp2m.c          | 58 +++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/mm/hap/hap.c         | 40 ++++++++-------------------
 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, 119 insertions(+), 77 deletions(-)

-- 
2.7.4


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

[PATCH Altp2m cleanup 2/3 v10 1/2] 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.
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>
---
v10
Per request:
        fix coding style of "if (rc) {" 
---
 xen/arch/x86/mm/altp2m.c          | 58 +++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/mm/hap/hap.c         | 40 ++++++++-------------------
 xen/include/asm-x86/altp2m.h      |  4 ++-
 xen/include/asm-x86/hvm/vmx/vmx.h |  3 ++
 xen/include/asm-x86/p2m.h         |  9 ++----
 5 files changed, 78 insertions(+), 36 deletions(-)

diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c
index 930bdc2..0b99d5d 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,63 @@ 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 ( 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 )
+        {
+            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..4612871 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>
@@ -493,32 +494,25 @@ int hap_enable(struct domain *d, u32 mode)
             goto out;
     }
 
-    for (i = 0; i < MAX_NESTEDP2M; i++) {
+    for (i = 0; i < MAX_NESTEDP2M; i++)
+    {
         rv = p2m_alloc_table(d->arch.nested_p2m[i]);
         if ( rv != 0 )
            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 )
+        for (i = 0; i < MAX_NESTEDP2M; i++)
         {
-            rv = -ENOMEM;
-            goto out;
+            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 +527,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 */
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 v10 1/2] Move altp2m specific functions to altp2m files.

[Jan Beulich]

>>> On 12.10.16 at 01:40, <paul.c.lai@intel.com> wrote:
> @@ -66,6 +67,63 @@ 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 ( d == NULL )
> +        return 0;

I'm sorry for not having paid attention before, but why do you add
this check?

> @@ -493,32 +494,25 @@ int hap_enable(struct domain *d, u32 mode)
>              goto out;
>      }
>  
> -    for (i = 0; i < MAX_NESTEDP2M; i++) {
> +    for (i = 0; i < MAX_NESTEDP2M; i++)
> +    {

If you correct coding style issues, then please deal with all of them
at once. Albeit this code is unrelated to altp2m, so maybe you'd
better leave it alone in this patch.

>          rv = p2m_alloc_table(d->arch.nested_p2m[i]);
>          if ( rv != 0 )
>             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 )
> +        for (i = 0; i < MAX_NESTEDP2M; i++)

As said on v8: Coding style (you've moved the opening brace, but
you didn't insert blanks).

>          {
> -            rv = -ENOMEM;
> -            goto out;
> +            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;
>      }

Repeating my v8 comment: "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." While it looks like the final part of that may
have been taken care of, may I once again remind you that it is a
waste of everyone's time unless _all_ comments given on a prior
version get addressed either verbally or in the next submitted
version? In particular (but please don't again take this as the only
thing needing changing/explaining) the call to hap_teardown()
looks unmotivated. Don't you mean to just undo the earlier
hap_set_allocation()? And then - did you check that cleanup is
(a) necessary here in the first place and (b) is now complete? I
ask because e.g. the nested p2m allocation loop doesn't appear
to be doing any cleanup either. It looks like this is wrong too,
but if you fix it you need to (1) confirm for yourself the change
is needed and (2) reason about the change in the commit message.
And it may well be that this again would better not be done here,
but in a prerequisite (and thus backportable) patch.

Jan


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

[PATCH Altp2m cleanup 2/3 v10 2/2] 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().

[Paul Lai]

Signed-off-by: Paul Lai <paul.c.lai@intel.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v10
Added Reviewed-by stamp
---
 xen/arch/x86/mm/p2m-ept.c | 39 +++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/mm/p2m.c     | 43 ++-----------------------------------------
 2 files changed, 41 insertions(+), 41 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 9526fff..9fecbc9 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;
 
-- 
2.7.4


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

Re: [PATCH Altp2m cleanup 2/3 v10 2/2] 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().

[Jan Beulich]

>>> On 12.10.16 at 01:40, <paul.c.lai@intel.com> wrote:
> Signed-off-by: Paul Lai <paul.c.lai@intel.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> v10
> Added Reviewed-by stamp

Same thing here - please address _all_ review comments. On v9
Konrad did say "someting is off with your title. I think you need to
add an extra newline." As it stands the patch title is definitely too
long.

Jan


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