[PATCH Altp2m cleanup 2/3 v11 0/2] clean up altp2m code

[PATCH Altp2m cleanup 2/3 v11 0/2] clean 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.

 xen/arch/x86/mm/altp2m.c          | 55 +++++++++++++++++++++++++++++++++++++++
 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, 117 insertions(+), 76 deletions(-)

--
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 v11 1/2] Move altp2m specific functions to altp2m files.

[Paul Lai]

Moving altp2m domain startup and teardown into altp2m_domain_init()
and altp2m_domain_teardown() respectively.
In hap_enable(), upon partial success of altp2m_domain_init(), unravel
the partial p2m_alloc_table() and unravel hap_set_allocation().
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>
---
v11
Per request....

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() during partial success of
altp2m_domain_init().
---
 xen/arch/x86/mm/altp2m.c          | 55 +++++++++++++++++++++++++++++++++++++++
 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, 76 insertions(+), 35 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..8826e77 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,21 @@ 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 )
+        /* Unravel the partial p2m_alloc_table() above */
+        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;
-        }
+        /* 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);
 
-        d->arch.altp2m_active = 0;
+        goto out;
     }
 
     /* Now let other users see the new mode */
@@ -533,19 +529,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 v11 1/2] Move altp2m specific functions to altp2m files.

[Jan Beulich]

>>> On 19.10.16 at 22:32, <paul.c.lai@intel.com> wrote:
> @@ -499,26 +500,21 @@ 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 )
> +        /* Unravel the partial p2m_alloc_table() above */
> +        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;
> -        }
> +        /* 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);
>  
> -        d->arch.altp2m_active = 0;
> +        goto out;
>      }
>  
>      /* Now let other users see the new mode */

As said before, I don't think this is the right way to approach this.
For one, this error handling fix - if it is really needed, proof of
which is sadly _still_ missing from the patch description - should
be a separate patch (to allow backporting). And then it should be
complete - right now you undo one of the earlier p2m_alloc_table()
instances, but not the other, and you adjust only the altp2m error
paths, but neither the PG_translate nor the nested ones. And if
you deal with the whole issue at once (again - provided this needs
fixing in the first place, i.e. the cleanup doesn't occur by other,
implicit means), it will likely become clear that central error handling
produces easier to read/maintain code than repeated instances of
partial cleanup. The question really is whether one of the
existing teardown functions can't fulfill this purpose (or can't be
easily made do so).

> --- 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);

The movement of p2m_find_altp2m_by_eptp()'s declaration
appears to belong into patch 2. And the addition of
p2m_init_altp2m_ept()'s one very certainly has to go there.

Jan

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

[PATCH Altp2m cleanup 2/3 v11 2/2] 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>
---
v11
Fixing length of title that was reqested in v9, but missed in v10.
---
 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