Re: [Xen-devel] [PATCH v2 Altp2m cleanup v3 3/3] Making altp2m struct dynamically allocated.

2016-09-05 Thread Jan Beulich 
>>> On 02.09.16 at 19:53,  wrote:
> [PAUL] in line

Please configure your mail client to use proper quoting.

> From: Jan Beulich [mailto:jbeul...@suse.com] 
> Sent: Friday, September 2, 2016 6:47 AM
 On 19.08.16 at 19:22,  wrote:
>> Ravi Sahita's dynamically allocated altp2m structs
> 
> I think I've asked before: With this and ...
> 
>> Signed-off-by: Paul Lai 
>> Reviewed-by: Ravi Sahita 
> 
> ... this - who's the actual author?
> 
> [PAUL] Ravi is the actual author.  I thought the commit message would have 
> been clear :(

The commit message by itself is clear, but it contradicts the absence
of a From: tag, may also contradict the S-o-b one (albeit it's not
impossible for someone to sign off on someone else's work), and it's
certainly bogus for him to give a R-b for his own patch.

Jan


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

Re: [Xen-devel] [PATCH v2 Altp2m cleanup v3 3/3] Making altp2m struct dynamically allocated.

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

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

Subject: Re: [PATCH v2 Altp2m cleanup v3 3/3] Making altp2m struct dynamically 
allocated.

>>> On 19.08.16 at 19:22,  wrote:
> Ravi Sahita's dynamically allocated altp2m structs

I think I've asked before: With this and ...

> Signed-off-by: Paul Lai 
> Reviewed-by: Ravi Sahita 

... this - who's the actual author?

[PAUL] Ravi is the actual author.  I thought the commit message would have been 
clear :(

> @@ -5279,11 +5279,11 @@ static int do_altp2m_op(
>  break;
>  }
>  
> -ostate = d->arch.altp2m_active;
> -d->arch.altp2m_active = !!a.u.domain_state.state;
> +ostate = altp2m_active(d);
> +set_altp2m_active(d, !!a.u.domain_state.state);

The !! shouldn't be needed anymore.

> --- a/xen/arch/x86/mm/altp2m.c
> +++ b/xen/arch/x86/mm/altp2m.c
> @@ -73,23 +73,23 @@ hvm_altp2m_init( struct domain *d)
>  unsigned int i = 0;
>  
>  /* Init alternate p2m data. */
> -if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> +if ( (d->arch.altp2m->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);
> +d->arch.altp2m->altp2m_eptp[i] = mfn_x(INVALID_MFN);
>  
>  for ( i = 0; i < MAX_ALTP2M; i++ )
>  {
> -rc = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> +rc = p2m_alloc_table(d->arch.altp2m->altp2m_p2m[i]);
>  if ( rc != 0 )
> goto out;
>  }
>  
> -d->arch.altp2m_active = 0;
> +set_altp2m_active(d, 0);

"false" please (also elsewhere).

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -193,12 +193,15 @@ static void p2m_teardown_altp2m(struct domain 
> *d)
>  
>  for ( i = 0; i < MAX_ALTP2M; i++ )
>  {
> -if ( !d->arch.altp2m_p2m[i] )
> +if ( !d->arch.altp2m->altp2m_p2m[i] )
>  continue;
> -p2m = d->arch.altp2m_p2m[i];
> +p2m = d->arch.altp2m->altp2m_p2m[i];
>  p2m_free_one(p2m);
> -d->arch.altp2m_p2m[i] = NULL;
> +d->arch.altp2m->altp2m_p2m[i] = NULL;
>  }
> +
> +if ( d->arch.altp2m )
> +xfree(d->arch.altp2m);

Two problems here: First, xfree() happily deals with NULL being passed. But 
then, such a NULL check clearly should not come _after_ the pointer did already 
get dereferenced. I.e. first of all you need to clarify for yourself whether 
the function can be called with the pointer being NULL.

[PAUL] great catch

> @@ -206,10 +209,14 @@ static int p2m_init_altp2m(struct domain *d)

And then, considering this is the last patch in the series - how come these two 
functions are still in this source file?

> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -338,6 +338,13 @@ struct p2m_domain {
>  };
>  };
>  
> +struct altp2m_domain {
> +bool_t altp2m_active;
> +struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
> +mm_lock_t altp2m_list_lock;
> +uint64_t *altp2m_eptp;
> +};

None of the altp2m_ prefixes here are really useful for anything afaics.

Jan


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

Re: [Xen-devel] [PATCH v2 Altp2m cleanup v3 3/3] Making altp2m struct dynamically allocated.

2016-09-02 Thread Jan Beulich 
>>> On 19.08.16 at 19:22,  wrote:
> Ravi Sahita's dynamically allocated altp2m structs

I think I've asked before: With this and ...

> Signed-off-by: Paul Lai 
> Reviewed-by: Ravi Sahita 

... this - who's the actual author?

> @@ -5279,11 +5279,11 @@ static int do_altp2m_op(
>  break;
>  }
>  
> -ostate = d->arch.altp2m_active;
> -d->arch.altp2m_active = !!a.u.domain_state.state;
> +ostate = altp2m_active(d);
> +set_altp2m_active(d, !!a.u.domain_state.state);

The !! shouldn't be needed anymore.

> --- a/xen/arch/x86/mm/altp2m.c
> +++ b/xen/arch/x86/mm/altp2m.c
> @@ -73,23 +73,23 @@ hvm_altp2m_init( struct domain *d)
>  unsigned int i = 0;
>  
>  /* Init alternate p2m data. */
> -if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> +if ( (d->arch.altp2m->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);
> +d->arch.altp2m->altp2m_eptp[i] = mfn_x(INVALID_MFN);
>  
>  for ( i = 0; i < MAX_ALTP2M; i++ )
>  {
> -rc = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> +rc = p2m_alloc_table(d->arch.altp2m->altp2m_p2m[i]);
>  if ( rc != 0 )
> goto out;
>  }
>  
> -d->arch.altp2m_active = 0;
> +set_altp2m_active(d, 0);

"false" please (also elsewhere).

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -193,12 +193,15 @@ static void p2m_teardown_altp2m(struct domain *d)
>  
>  for ( i = 0; i < MAX_ALTP2M; i++ )
>  {
> -if ( !d->arch.altp2m_p2m[i] )
> +if ( !d->arch.altp2m->altp2m_p2m[i] )
>  continue;
> -p2m = d->arch.altp2m_p2m[i];
> +p2m = d->arch.altp2m->altp2m_p2m[i];
>  p2m_free_one(p2m);
> -d->arch.altp2m_p2m[i] = NULL;
> +d->arch.altp2m->altp2m_p2m[i] = NULL;
>  }
> +
> +if ( d->arch.altp2m )
> +xfree(d->arch.altp2m);

Two problems here: First, xfree() happily deals with NULL being
passed. But then, such a NULL check clearly should not come
_after_ the pointer did already get dereferenced. I.e. first of all
you need to clarify for yourself whether the function can be called
with the pointer being NULL.

> @@ -206,10 +209,14 @@ static int p2m_init_altp2m(struct domain *d)

And then, considering this is the last patch in the series - how come
these two functions are still in this source file?

> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -338,6 +338,13 @@ struct p2m_domain {
>  };
>  };
>  
> +struct altp2m_domain {
> +bool_t altp2m_active;
> +struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
> +mm_lock_t altp2m_list_lock;
> +uint64_t *altp2m_eptp;
> +};

None of the altp2m_ prefixes here are really useful for anything afaics.

Jan


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

[Xen-devel] [PATCH v2 Altp2m cleanup v3 3/3] Making altp2m struct dynamically allocated.

2016-08-19 Thread Paul Lai 
Ravi Sahita's dynamically allocated altp2m structs

Signed-off-by: Paul Lai 
Reviewed-by: Ravi Sahita 
---
 xen/arch/x86/hvm/hvm.c   |  8 +++---
 xen/arch/x86/hvm/vmx/vmx.c   |  2 +-
 xen/arch/x86/mm/altp2m.c | 18 ++---
 xen/arch/x86/mm/mm-locks.h   |  4 +--
 xen/arch/x86/mm/p2m-ept.c| 10 
 xen/arch/x86/mm/p2m.c| 61 
 xen/common/monitor.c |  1 +
 xen/include/asm-x86/altp2m.h |  7 -
 xen/include/asm-x86/domain.h |  6 ++---
 xen/include/asm-x86/p2m.h|  9 ++-
 10 files changed, 72 insertions(+), 54 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 1995fa4..115d8e7 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5245,7 +5245,7 @@ static int do_altp2m_op(
 
 if ( (a.cmd != HVMOP_altp2m_get_domain_state) &&
  (a.cmd != HVMOP_altp2m_set_domain_state) &&
- !d->arch.altp2m_active )
+ !altp2m_active(d) )
 {
 rc = -EOPNOTSUPP;
 goto out;
@@ -5279,11 +5279,11 @@ static int do_altp2m_op(
 break;
 }
 
-ostate = d->arch.altp2m_active;
-d->arch.altp2m_active = !!a.u.domain_state.state;
+ostate = altp2m_active(d);
+set_altp2m_active(d, !!a.u.domain_state.state);
 
 /* If the alternate p2m state has changed, handle appropriately */
-if ( d->arch.altp2m_active != ostate &&
+if ( altp2m_active(d) != ostate &&
  (ostate || !(rc = p2m_init_altp2m_by_id(d, 0))) )
 {
 for_each_vcpu( d, v )
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 3d330b6..e7c8d04 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2018,7 +2018,7 @@ static void vmx_vcpu_update_vmfunc_ve(struct vcpu *v)
 {
 v->arch.hvm_vmx.secondary_exec_control |= mask;
 __vmwrite(VM_FUNCTION_CONTROL, VMX_VMFUNC_EPTP_SWITCHING);
-__vmwrite(EPTP_LIST_ADDR, virt_to_maddr(d->arch.altp2m_eptp));
+__vmwrite(EPTP_LIST_ADDR, virt_to_maddr(d->arch.altp2m->altp2m_eptp));
 
 if ( cpu_has_vmx_virt_exceptions )
 {
diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c
index cc9b0fc..eec9ffc 100644
--- a/xen/arch/x86/mm/altp2m.c
+++ b/xen/arch/x86/mm/altp2m.c
@@ -73,23 +73,23 @@ hvm_altp2m_init( struct domain *d)
 unsigned int i = 0;
 
 /* Init alternate p2m data. */
-if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
+if ( (d->arch.altp2m->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);
+d->arch.altp2m->altp2m_eptp[i] = mfn_x(INVALID_MFN);
 
 for ( i = 0; i < MAX_ALTP2M; i++ )
 {
-rc = p2m_alloc_table(d->arch.altp2m_p2m[i]);
+rc = p2m_alloc_table(d->arch.altp2m->altp2m_p2m[i]);
 if ( rc != 0 )
goto out;
 }
 
-d->arch.altp2m_active = 0;
+set_altp2m_active(d, 0);
  out:
 return rc;
 }
@@ -98,16 +98,16 @@ void
 hvm_altp2m_teardown( struct domain *d)
 {
 unsigned int i = 0;
-d->arch.altp2m_active = 0;
+set_altp2m_active(d, 0);
 
-if ( d->arch.altp2m_eptp )
+if ( d->arch.altp2m->altp2m_eptp )
 {
-free_xenheap_page(d->arch.altp2m_eptp);
-d->arch.altp2m_eptp = NULL;
+free_xenheap_page(d->arch.altp2m->altp2m_eptp);
+d->arch.altp2m->altp2m_eptp = NULL;
 }
 
 for ( i = 0; i < MAX_ALTP2M; i++ )
-p2m_teardown(d->arch.altp2m_p2m[i]);
+p2m_teardown(d->arch.altp2m->altp2m_p2m[i]);
 }
 
 /*
diff --git a/xen/arch/x86/mm/mm-locks.h b/xen/arch/x86/mm/mm-locks.h
index 086c8bb..4d17b0a 100644
--- a/xen/arch/x86/mm/mm-locks.h
+++ b/xen/arch/x86/mm/mm-locks.h
@@ -251,8 +251,8 @@ declare_mm_rwlock(p2m);
  */
 
 declare_mm_lock(altp2mlist)
-#define altp2m_list_lock(d)   mm_lock(altp2mlist, &(d)->arch.altp2m_list_lock)
-#define altp2m_list_unlock(d) mm_unlock(&(d)->arch.altp2m_list_lock)
+#define altp2m_list_lock(d)   mm_lock(altp2mlist, 
&(d)->arch.altp2m->altp2m_list_lock)
+#define altp2m_list_unlock(d) mm_unlock(&(d)->arch.altp2m->altp2m_list_lock)
 
 /* P2M lock (per-altp2m-table)
  *
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 8247a02..74b2754 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1331,14 +1331,14 @@ void setup_ept_dump(void)
 
 void p2m_init_altp2m_ept_helper( struct domain *d, unsigned int i)
 {
-struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
+struct p2m_domain *p2m = d->arch.altp2m->altp2m_p2m[i];
 struct ept_data *ept;
 
 p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
-p2m->max_remapped_gfn = 0;
+p2m->max_remapped_gfn = gfn_x(_gfn(0UL));
 ept = >ept;
 ept->asr = pagetable_get_pfn(p2m_get_pagetable(p2m));
-d->arch.altp2m_eptp[i] =