Re: [Xen-devel] [PATCH v2 Altp2m cleanup v3 2/3] Move altp2m specific functions to altp2m files.

2016-09-05 Thread Jan Beulich 
>>> On 02.09.16 at 19:56,  wrote:
> From: Jan Beulich [mailto:jbeul...@suse.com] 
> Sent: Friday, September 2, 2016 6:31 AM
 On 19.08.16 at 19:22,  wrote:
>> +/* Init alternate p2m data. */
>> +if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
>> +{
>> +rc = -ENOMEM;
>> +goto out;
>> +}
> 
> When the epilogue (after the target label) is just a return statement, 
> please avoid using goto.
> 
> [PAUL] I don't see this code in an epilogue (after the target label).  

I don't understand: The function ends like this

 out:
return rc;
}

What is it that you don't see here? Again, all I'm asking for is that
in a case like this you simply use "return" instead of the rc
assignment and "goto".

Jan


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

Re: [Xen-devel] [PATCH v2 Altp2m cleanup v3 2/3] Move altp2m specific functions to altp2m files.

2016-09-02 Thread Lai, Paul C 
[PAUL] in-line

Ravi -- please comment on swap comment below.

-Original Message-
From: Jan Beulich [mailto:jbeul...@suse.com] 
Sent: Friday, September 2, 2016 6:31 AM
To: Lai, Paul C 
Cc: george.dun...@citrix.com; Sahita, Ravi ; xen-devel 

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

>>> On 19.08.16 at 19:22,  wrote:
> @@ -65,6 +66,50 @@ altp2m_vcpu_destroy(struct vcpu *v)
>  vcpu_unpause(v);
>  }
>  
> +int
> +hvm_altp2m_init( struct domain *d)

Stray blank (more elsewhere). Also I don't think hvm_ is a proper prefix for a 
function placed in this file, i.e. if altp2m_init() is used elsewhere already, 
then altp2m_hvm_init() or perhaps better
altp2m_domain_init() please.

> +{
> +int rc = 0;
> +unsigned int i = 0;

Pointless initializers.

> +/* Init alternate p2m data. */
> +if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> +{
> +rc = -ENOMEM;
> +goto out;
> +}

When the epilogue (after the target label) is just a return statement, please 
avoid using goto.

[PAUL] I don't see this code in an epilogue (after the target label).  

> +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;
> +}

Please avoid the if() - free_xenheap_page() happily deals with a NULL pointer 
passed to it.

> +for ( i = 0; i < MAX_ALTP2M; i++ )
> +p2m_teardown(d->arch.altp2m_p2m[i]);

I realize it's been that way in the original code, but wouldn't swapping the 
two things be both more natural and more robust?

[PAUL] Ravi ?

> @@ -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] = 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;
> -}
> -
> -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);

I wonder whether in both cases the hvm_altp2m_supported() would better also be 
moved into the functions.

It looks like the parts above and below this point, except for header file 
adjustments, are completely independent. Please consider splitting into two 
patches.

> --- 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_helper( struct domain *d, unsigned int i)

Please drop the _helper suffix now that there is _ept.

Jan


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

Re: [Xen-devel] [PATCH v2 Altp2m cleanup v3 2/3] Move altp2m specific functions to altp2m files.

2016-09-02 Thread Jan Beulich 
>>> On 19.08.16 at 19:22,  wrote:
> @@ -65,6 +66,50 @@ altp2m_vcpu_destroy(struct vcpu *v)
>  vcpu_unpause(v);
>  }
>  
> +int
> +hvm_altp2m_init( struct domain *d)

Stray blank (more elsewhere). Also I don't think hvm_ is a proper
prefix for a function placed in this file, i.e. if altp2m_init() is used
elsewhere already, then altp2m_hvm_init() or perhaps better
altp2m_domain_init() please.

> +{
> +int rc = 0;
> +unsigned int i = 0;

Pointless initializers.

> +/* Init alternate p2m data. */
> +if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> +{
> +rc = -ENOMEM;
> +goto out;
> +}

When the epilogue (after the target label) is just a return statement,
please avoid using goto.

> +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;
> +}

Please avoid the if() - free_xenheap_page() happily deals with a
NULL pointer passed to it.

> +for ( i = 0; i < MAX_ALTP2M; i++ )
> +p2m_teardown(d->arch.altp2m_p2m[i]);

I realize it's been that way in the original code, but wouldn't swapping
the two things be both more natural and more robust?

> @@ -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] = 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;
> -}
> -
> -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);

I wonder whether in both cases the hvm_altp2m_supported()
would better also be moved into the functions.

It looks like the parts above and below this point, except for header
file adjustments, are completely independent. Please consider
splitting into two patches.

> --- 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_helper( struct domain *d, unsigned int i)

Please drop the _helper suffix now that there is _ept.

Jan


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

[Xen-devel] [PATCH v2 Altp2m cleanup v3 2/3] Move altp2m specific functions to altp2m files.

2016-08-19 Thread Paul Lai 
Move altp2m specific functions to altp2m files.  This makes the code
a little easier to read.

Also moving ept code to ept specific files as requested in:
  https://lists.xenproject.org/archives/html/xen-devel/2015-07/msg04323.html

Signed-off-by: Paul Lai 
---
 xen/arch/x86/mm/altp2m.c  | 45 +++
 xen/arch/x86/mm/hap/hap.c | 35 +-
 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, 101 insertions(+), 79 deletions(-)

diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c
index 930bdc2..cc9b0fc 100644
--- a/xen/arch/x86/mm/altp2m.c
+++ b/xen/arch/x86/mm/altp2m.c
@@ -17,6 +17,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -65,6 +66,50 @@ altp2m_vcpu_destroy(struct vcpu *v)
 vcpu_unpause(v);
 }
 
+int
+hvm_altp2m_init( struct domain *d)
+{
+int rc = 0;
+unsigned int i = 0;
+
+/* Init alternate p2m data. */
+if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
+{
+rc = -ENOMEM;
+goto out;
+}
+
+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 )
+   goto out;
+}
+
+d->arch.altp2m_active = 0;
+ out:
+return rc;
+}
+
+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 3218fa2..6e1e9a5 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -37,6 +37,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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] = 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;
-}
-
-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 13cab24..8247a02 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_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 = >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 = >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 ff0cce8..7673663 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