[Xen-devel] [PATCH V5 2/3] x86/mm: allocate logdirty_ranges for altp2ms

2018-11-11 Thread Razvan Cojocaru 
This patch is a pre-requisite for the one fixing VGA logdirty
freezes when using altp2m. It only concerns itself with the
ranges allocation / deallocation / initialization part. While
touching the code, I've switched global_logdirty from bool_t
to bool.

Signed-off-by: Razvan Cojocaru 

---
CC: George Dunlap 
CC: Jan Beulich 
CC: Andrew Cooper 
CC: Wei Liu 

---
Changes since V4:
 - Always call p2m_free_logdirty() in p2m_free_one() (previously
   the call was gated on hap_enabled(p2m->domain) && cpu_has_vmx).
---
 xen/arch/x86/mm/p2m.c | 74 ---
 xen/include/asm-x86/p2m.h |  2 +-
 2 files changed, 58 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 42b9ef4..69536c1 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -59,6 +59,28 @@ static void p2m_nestedp2m_init(struct p2m_domain *p2m)
 #endif
 }
 
+static int p2m_init_logdirty(struct p2m_domain *p2m)
+{
+if ( p2m->logdirty_ranges )
+return 0;
+
+p2m->logdirty_ranges = rangeset_new(p2m->domain, "log-dirty",
+RANGESETF_prettyprint_hex);
+if ( !p2m->logdirty_ranges )
+return -ENOMEM;
+
+return 0;
+}
+
+static void p2m_free_logdirty(struct p2m_domain *p2m)
+{
+if ( !p2m->logdirty_ranges )
+return;
+
+rangeset_destroy(p2m->logdirty_ranges);
+p2m->logdirty_ranges = NULL;
+}
+
 /* Init the datastructures for later use by the p2m code */
 static int p2m_initialise(struct domain *d, struct p2m_domain *p2m)
 {
@@ -107,6 +129,7 @@ free_p2m:
 
 static void p2m_free_one(struct p2m_domain *p2m)
 {
+p2m_free_logdirty(p2m);
 if ( hap_enabled(p2m->domain) && cpu_has_vmx )
 ept_p2m_uninit(p2m);
 free_cpumask_var(p2m->dirty_cpumask);
@@ -116,19 +139,19 @@ static void p2m_free_one(struct p2m_domain *p2m)
 static int p2m_init_hostp2m(struct domain *d)
 {
 struct p2m_domain *p2m = p2m_init_one(d);
+int rc;
 
-if ( p2m )
-{
-p2m->logdirty_ranges = rangeset_new(d, "log-dirty",
-RANGESETF_prettyprint_hex);
-if ( p2m->logdirty_ranges )
-{
-d->arch.p2m = p2m;
-return 0;
-}
+if ( !p2m )
+return -ENOMEM;
+
+rc = p2m_init_logdirty(p2m);
+
+if ( !rc )
+d->arch.p2m = p2m;
+else
 p2m_free_one(p2m);
-}
-return -ENOMEM;
+
+return rc;
 }
 
 static void p2m_teardown_hostp2m(struct domain *d)
@@ -138,7 +161,6 @@ static void p2m_teardown_hostp2m(struct domain *d)
 
 if ( p2m )
 {
-rangeset_destroy(p2m->logdirty_ranges);
 p2m_free_one(p2m);
 d->arch.p2m = NULL;
 }
@@ -2279,6 +2301,18 @@ void p2m_flush_altp2m(struct domain *d)
 altp2m_list_unlock(d);
 }
 
+static int p2m_init_altp2m_logdirty(struct p2m_domain *p2m)
+{
+struct p2m_domain *hostp2m = p2m_get_hostp2m(p2m->domain);
+int rc = p2m_init_logdirty(p2m);
+
+if ( rc )
+return rc;
+
+/* The following is really just a rangeset copy. */
+return rangeset_merge(p2m->logdirty_ranges, hostp2m->logdirty_ranges);
+}
+
 int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
 {
 int rc = -EINVAL;
@@ -2290,8 +2324,9 @@ 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_ept(d, idx);
-rc = 0;
+rc = p2m_init_altp2m_logdirty(d->arch.altp2m_p2m[idx]);
+if ( !rc )
+p2m_init_altp2m_ept(d, idx);
 }
 
 altp2m_list_unlock(d);
@@ -2310,9 +2345,13 @@ 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_ept(d, i);
-*idx = i;
-rc = 0;
+rc = p2m_init_altp2m_logdirty(d->arch.altp2m_p2m[i]);
+
+if ( !rc )
+{
+p2m_init_altp2m_ept(d, i);
+*idx = i;
+}
 
 break;
 }
@@ -2341,6 +2380,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned 
int idx)
 {
 p2m_flush_table(d->arch.altp2m_p2m[idx]);
 /* Uninit and reinit ept to force TLB shootdown */
+p2m_free_logdirty(d->arch.altp2m_p2m[idx]);
 ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
 ept_p2m_init(d->arch.altp2m_p2m[idx]);
 d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index ac33f50..c7f5710 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -222,7 +222,7 @@ struct p2m_domain {
 struct rangeset   *logdirty_ranges;
 
 /* Host p2m: Global log-dirty mode enabled for the domain. */
-bool_t global_logdirty;
+bool   global_logdirty;
 
 /* Host p2m: when this flag is set, don't flush all the nested-p2m 
  * tables

Re: [Xen-devel] [PATCH V5 2/3] x86/mm: allocate logdirty_ranges for altp2ms

2018-11-13 Thread George Dunlap 
On 11/11/18 2:07 PM, Razvan Cojocaru wrote:
> This patch is a pre-requisite for the one fixing VGA logdirty
> freezes when using altp2m. It only concerns itself with the
> ranges allocation / deallocation / initialization part. While
> touching the code, I've switched global_logdirty from bool_t
> to bool.
> 
> Signed-off-by: Razvan Cojocaru 

I've convinced myself that this patch is probably correct now, and as a
result I've had a chance to look a bit at the resulting code.  Which
means, unfortunately, that I'm going to be a bit annoying and ask more
questions that I didn't ask last time.

> 
> ---
> CC: George Dunlap 
> CC: Jan Beulich 
> CC: Andrew Cooper 
> CC: Wei Liu 
> 
> ---
> Changes since V4:
>  - Always call p2m_free_logdirty() in p2m_free_one() (previously
>the call was gated on hap_enabled(p2m->domain) && cpu_has_vmx).
> ---
>  xen/arch/x86/mm/p2m.c | 74 
> ---
>  xen/include/asm-x86/p2m.h |  2 +-
>  2 files changed, 58 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 42b9ef4..69536c1 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -59,6 +59,28 @@ static void p2m_nestedp2m_init(struct p2m_domain *p2m)
>  #endif
>  }
>  
> +static int p2m_init_logdirty(struct p2m_domain *p2m)
> +{
> +if ( p2m->logdirty_ranges )
> +return 0;
> +
> +p2m->logdirty_ranges = rangeset_new(p2m->domain, "log-dirty",
> +RANGESETF_prettyprint_hex);
> +if ( !p2m->logdirty_ranges )
> +return -ENOMEM;
> +
> +return 0;
> +}
> +
> +static void p2m_free_logdirty(struct p2m_domain *p2m)
> +{
> +if ( !p2m->logdirty_ranges )
> +return;
> +
> +rangeset_destroy(p2m->logdirty_ranges);
> +p2m->logdirty_ranges = NULL;
> +}
> +
>  /* Init the datastructures for later use by the p2m code */
>  static int p2m_initialise(struct domain *d, struct p2m_domain *p2m)
>  {
> @@ -107,6 +129,7 @@ free_p2m:
>  
>  static void p2m_free_one(struct p2m_domain *p2m)
>  {
> +p2m_free_logdirty(p2m);
>  if ( hap_enabled(p2m->domain) && cpu_has_vmx )
>  ept_p2m_uninit(p2m);
>  free_cpumask_var(p2m->dirty_cpumask);
> @@ -116,19 +139,19 @@ static void p2m_free_one(struct p2m_domain *p2m)
>  static int p2m_init_hostp2m(struct domain *d)
>  {
>  struct p2m_domain *p2m = p2m_init_one(d);
> +int rc;
>  
> -if ( p2m )
> -{
> -p2m->logdirty_ranges = rangeset_new(d, "log-dirty",
> -RANGESETF_prettyprint_hex);
> -if ( p2m->logdirty_ranges )
> -{
> -d->arch.p2m = p2m;
> -return 0;
> -}
> +if ( !p2m )
> +return -ENOMEM;
> +
> +rc = p2m_init_logdirty(p2m);
> +
> +if ( !rc )
> +d->arch.p2m = p2m;
> +else
>  p2m_free_one(p2m);
> -}
> -return -ENOMEM;
> +
> +return rc;
>  }
>  
>  static void p2m_teardown_hostp2m(struct domain *d)
> @@ -138,7 +161,6 @@ static void p2m_teardown_hostp2m(struct domain *d)
>  
>  if ( p2m )
>  {
> -rangeset_destroy(p2m->logdirty_ranges);
>  p2m_free_one(p2m);
>  d->arch.p2m = NULL;
>  }
> @@ -2279,6 +2301,18 @@ void p2m_flush_altp2m(struct domain *d)
>  altp2m_list_unlock(d);
>  }

I think everything above here could usefully be in its own patch; it
would make it easier to verify that there were no functional changes in
the refactoring.

> +static int p2m_init_altp2m_logdirty(struct p2m_domain *p2m)
> +{
> +struct p2m_domain *hostp2m = p2m_get_hostp2m(p2m->domain);
> +int rc = p2m_init_logdirty(p2m);
> +
> +if ( rc )
> +return rc;
> +
> +/* The following is really just a rangeset copy. */
> +return rangeset_merge(p2m->logdirty_ranges, hostp2m->logdirty_ranges);
> +}
> +
>  int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
>  {
>  int rc = -EINVAL;
> @@ -2290,8 +2324,9 @@ 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_ept(d, idx);
> -rc = 0;
> +rc = p2m_init_altp2m_logdirty(d->arch.altp2m_p2m[idx]);
> +if ( !rc )
> +p2m_init_altp2m_ept(d, idx);
>  }
>  
>  altp2m_list_unlock(d);
> @@ -2310,9 +2345,13 @@ 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_ept(d, i);
> -*idx = i;
> -rc = 0;
> +rc = p2m_init_altp2m_logdirty(d->arch.altp2m_p2m[i]);
> +
> +if ( !rc )
> +{
> +p2m_init_altp2m_ept(d, i);
> +*idx = i;
> +}

It looks like there's a 1-1 correspondence between
p2m_init_altp2m_logdirty() succeeding and calling p2m_inti_altp2m_ept().
 Would it make sense to combine them into the same function, maybe

Re: [Xen-devel] [PATCH V5 2/3] x86/mm: allocate logdirty_ranges for altp2ms

2018-11-13 Thread Razvan Cojocaru 
On 11/13/18 7:57 PM, George Dunlap wrote:
> On 11/11/18 2:07 PM, Razvan Cojocaru wrote:
>> This patch is a pre-requisite for the one fixing VGA logdirty
>> freezes when using altp2m. It only concerns itself with the
>> ranges allocation / deallocation / initialization part. While
>> touching the code, I've switched global_logdirty from bool_t
>> to bool.
>>
>> Signed-off-by: Razvan Cojocaru 
> 
> I've convinced myself that this patch is probably correct now, and as a
> result I've had a chance to look a bit at the resulting code.  Which
> means, unfortunately, that I'm going to be a bit annoying and ask more
> questions that I didn't ask last time.

Thanks for the review, and please ask away. :)

>> ---
>> CC: George Dunlap 
>> CC: Jan Beulich 
>> CC: Andrew Cooper 
>> CC: Wei Liu 
>>
>> ---
>> Changes since V4:
>>  - Always call p2m_free_logdirty() in p2m_free_one() (previously
>>the call was gated on hap_enabled(p2m->domain) && cpu_has_vmx).
>> ---
>>  xen/arch/x86/mm/p2m.c | 74 
>> ---
>>  xen/include/asm-x86/p2m.h |  2 +-
>>  2 files changed, 58 insertions(+), 18 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index 42b9ef4..69536c1 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -59,6 +59,28 @@ static void p2m_nestedp2m_init(struct p2m_domain *p2m)
>>  #endif
>>  }
>>  
>> +static int p2m_init_logdirty(struct p2m_domain *p2m)
>> +{
>> +if ( p2m->logdirty_ranges )
>> +return 0;
>> +
>> +p2m->logdirty_ranges = rangeset_new(p2m->domain, "log-dirty",
>> +RANGESETF_prettyprint_hex);
>> +if ( !p2m->logdirty_ranges )
>> +return -ENOMEM;
>> +
>> +return 0;
>> +}
>> +
>> +static void p2m_free_logdirty(struct p2m_domain *p2m)
>> +{
>> +if ( !p2m->logdirty_ranges )
>> +return;
>> +
>> +rangeset_destroy(p2m->logdirty_ranges);
>> +p2m->logdirty_ranges = NULL;
>> +}
>> +
>>  /* Init the datastructures for later use by the p2m code */
>>  static int p2m_initialise(struct domain *d, struct p2m_domain *p2m)
>>  {
>> @@ -107,6 +129,7 @@ free_p2m:
>>  
>>  static void p2m_free_one(struct p2m_domain *p2m)
>>  {
>> +p2m_free_logdirty(p2m);
>>  if ( hap_enabled(p2m->domain) && cpu_has_vmx )
>>  ept_p2m_uninit(p2m);
>>  free_cpumask_var(p2m->dirty_cpumask);
>> @@ -116,19 +139,19 @@ static void p2m_free_one(struct p2m_domain *p2m)
>>  static int p2m_init_hostp2m(struct domain *d)
>>  {
>>  struct p2m_domain *p2m = p2m_init_one(d);
>> +int rc;
>>  
>> -if ( p2m )
>> -{
>> -p2m->logdirty_ranges = rangeset_new(d, "log-dirty",
>> -RANGESETF_prettyprint_hex);
>> -if ( p2m->logdirty_ranges )
>> -{
>> -d->arch.p2m = p2m;
>> -return 0;
>> -}
>> +if ( !p2m )
>> +return -ENOMEM;
>> +
>> +rc = p2m_init_logdirty(p2m);
>> +
>> +if ( !rc )
>> +d->arch.p2m = p2m;
>> +else
>>  p2m_free_one(p2m);
>> -}
>> -return -ENOMEM;
>> +
>> +return rc;
>>  }
>>  
>>  static void p2m_teardown_hostp2m(struct domain *d)
>> @@ -138,7 +161,6 @@ static void p2m_teardown_hostp2m(struct domain *d)
>>  
>>  if ( p2m )
>>  {
>> -rangeset_destroy(p2m->logdirty_ranges);
>>  p2m_free_one(p2m);
>>  d->arch.p2m = NULL;
>>  }
>> @@ -2279,6 +2301,18 @@ void p2m_flush_altp2m(struct domain *d)
>>  altp2m_list_unlock(d);
>>  }
> 
> I think everything above here could usefully be in its own patch; it
> would make it easier to verify that there were no functional changes in
> the refactoring.

Right, I'll split this patch then.

>> +static int p2m_init_altp2m_logdirty(struct p2m_domain *p2m)
>> +{
>> +struct p2m_domain *hostp2m = p2m_get_hostp2m(p2m->domain);
>> +int rc = p2m_init_logdirty(p2m);
>> +
>> +if ( rc )
>> +return rc;
>> +
>> +/* The following is really just a rangeset copy. */
>> +return rangeset_merge(p2m->logdirty_ranges, hostp2m->logdirty_ranges);
>> +}
>> +
>>  int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
>>  {
>>  int rc = -EINVAL;
>> @@ -2290,8 +2324,9 @@ 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_ept(d, idx);
>> -rc = 0;
>> +rc = p2m_init_altp2m_logdirty(d->arch.altp2m_p2m[idx]);
>> +if ( !rc )
>> +p2m_init_altp2m_ept(d, idx);
>>  }
>>  
>>  altp2m_list_unlock(d);
>> @@ -2310,9 +2345,13 @@ 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_ept(d, i);
>> -*idx = i;
>> -rc = 0;
>> +rc = p2m_init_altp2m_logdirty(d->arch.altp2m_p2m[i]);
>> +
>> +if ( !rc )

Re: [Xen-devel] [PATCH V5 2/3] x86/mm: allocate logdirty_ranges for altp2ms

2018-11-13 Thread Razvan Cojocaru 
On 11/13/18 8:43 PM, Razvan Cojocaru wrote:
> On 11/13/18 7:57 PM, George Dunlap wrote:
>> On 11/11/18 2:07 PM, Razvan Cojocaru wrote:
>> At the moment we essentially have two "init" states:
>> * After domain creation; altp2m structures allocated, but no rangesets, & c
>> * After being enabled for the first time: rangesets mirroring hostp2m,
>> p2m_init_altp2m_ept() initialization done
>>
>> Is there any particular reason we allocate the p2m structures on domain
>> creation, but not logdirty range structures?  It seems like allocating
>> altp2m structures on-demand, rather than at domain creation time, might
>> make a lot of the reasoning here simpler.
> 
> I assume that this question is not addressed to me, since I'm not able
> to answer it - I can only assume that having less heap used has been
> preferred.

Actually I now realize that you're asking why the hostp2m rangeset is
created via paging_domain_init() in arch_domain_create() (so immediately
on domain creation) while I'm allocating the altp2m rangesets on altp2m
init.

I'm doing that to save memory, since we can have MAX_ALTP2M altp2ms
(which is currently 10), and only two active altp2ms - that means that I
would allocate 10 rangesets and only use two. In fact we're currently
only using 2 altp2ms and the hostp2m for our #VE work. That saves the
space required for 8 rangesets. If that's not much, or if you think that
the benefits of allocating them early outweigh the costs we can switch
to allocating them on domain creation, like the hostp2m, and perhaps
always keeping them in sync.


Thanks,
Razvan

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

Re: [Xen-devel] [PATCH V5 2/3] x86/mm: allocate logdirty_ranges for altp2ms

2018-11-14 Thread Jan Beulich 
>>> On 13.11.18 at 19:57,  wrote:
> On 11/13/18 8:43 PM, Razvan Cojocaru wrote:
>> On 11/13/18 7:57 PM, George Dunlap wrote:
>>> Is there any particular reason we allocate the p2m structures on domain
>>> creation, but not logdirty range structures?  It seems like allocating
>>> altp2m structures on-demand, rather than at domain creation time, might
>>> make a lot of the reasoning here simpler.
>> 
>> I assume that this question is not addressed to me, since I'm not able
>> to answer it - I can only assume that having less heap used has been
>> preferred.
> 
> Actually I now realize that you're asking why the hostp2m rangeset is
> created via paging_domain_init() in arch_domain_create() (so immediately
> on domain creation) while I'm allocating the altp2m rangesets on altp2m
> init.
> 
> I'm doing that to save memory, since we can have MAX_ALTP2M altp2ms
> (which is currently 10), and only two active altp2ms - that means that I
> would allocate 10 rangesets and only use two. In fact we're currently
> only using 2 altp2ms and the hostp2m for our #VE work. That saves the
> space required for 8 rangesets. If that's not much, or if you think that
> the benefits of allocating them early outweigh the costs we can switch
> to allocating them on domain creation, like the hostp2m, and perhaps
> always keeping them in sync.

I think George's question had the opposite goal: Rather than just
allocating the rangesets on demand, why don't the entire altp2m
structures get allocated on demand?

Jan



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

Re: [Xen-devel] [PATCH V5 2/3] x86/mm: allocate logdirty_ranges for altp2ms

2018-11-14 Thread George Dunlap 
On 11/13/18 6:43 PM, Razvan Cojocaru wrote:
> On 11/13/18 7:57 PM, George Dunlap wrote:
>> On 11/11/18 2:07 PM, Razvan Cojocaru wrote:
[snip]
>> I think everything above here could usefully be in its own patch; it
>> would make it easier to verify that there were no functional changes in
>> the refactoring.
> 
> Right, I'll split this patch then.

Thanks.

>>> @@ -2341,6 +2380,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, 
>>> unsigned int idx)
>>>  {
>>>  p2m_flush_table(d->arch.altp2m_p2m[idx]);
>>>  /* Uninit and reinit ept to force TLB shootdown */
>>> +p2m_free_logdirty(d->arch.altp2m_p2m[idx]);
>>>  ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
>>>  ept_p2m_init(d->arch.altp2m_p2m[idx]);
>>>  d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
>>
>> (In case I forget: Also, this is called without holding the appropriate
>> p2m lock. )
> 
> Could you please provide more details? I have assumed that at the point
> of calling a function called p2m_destroy_altp2m_by_id() it should be
> safe to tear the altp2m down without further precaution.

Are you absolutely positive that at this point there's no way anywhere
else in Xen might be doing something with this p2m struct?

If so, then 1) there should be a comment there explaining why that's the
case, and 2) ideally we should refactor p2m_flush_table such that we can
call what's now p2m_flush_table_locked() without the lock.

> I think you're saying that I should p2m_lock(d->arch.altp2m_p2m[idx])
> just for the duration of the p2m_free_logdirty() call?

The same argument really goes for ept_p2m_uninit/init -- uninit actually
frees a data structure; if anyone else may be using that, you'll run
into a use-after-free bug.  (Although that really needs to be changed as
well -- freeing and re-allocating a structure just to set all the bits
is ridiculous.)

If we need locking, then I'd grab the p2m lock before p2m_flush_table()
(calling p2m_flush_table_locked() instead), and release it after the
ept_p2m_init().

I realize you didn't write this code, and so I'm not holding you
responsible for all the changes I mentioned above.  But if we're going
to add the p2m_free_logdirty() call, we do need to either grab the lock
or add a comment explaining why it's not necessary; we might as well fix
it properly at the same time.

p2m_flush_table() already grabs and releases the lock; so grabbing the
lock over all four calls won't add any more overhead (or risk of
deadlock) than what we already have.

>> I'm a bit suspicious of long strings of these sorts of functions in the
>> middle of another function.  It turns out that there are three copies of
>> this sequence of function calls (p2m_flush_table -> ept_p2m_uninit ->
>> ept_p2m_init):
>>
>> * Here (p2m_destroy_altp2m_id), when the user asks for the alt2m index
>> to be destroyed
>>
>> * In p2m_flush_altp2m(), which is called when altp2m is disabled for a
>> domain
>>
>> * In p2m_reset_altp2m(), which is called when an entry in the hostp2m is
>> set to INVALID_MFN.
>>
>> Presumably in p2m_reset_altp2m() we don't want to call
>> p2m_free_logdirty(), as the altp2m is still active and we want to keep
>> the logdirty ranges around.  But in p2m_flush_altp2m(), I'm pretty sure
>> we do want to discard them: when altp2m is enabled again,
>> p2m_init_logdirty() will return early, leaving the old rangesets in
>> place; if the hostp2m rangesets have changed between the time altp2m was
>> disabled and enabled again, the rangeset_merge() may have incorrect results.
> 
> I'll call p2m_free_logdirty() in p2m_flush_altp2m() as well.

I was more thinking of refactoring those two to share the same code, and
potentially having p2m_reset_altp2m() share the same code as well.  The
reason you missed the p2m_flush_altp2m() there was because of the code
duplication.

Or alternately...

>>> Is there any particular reason we allocate the p2m structures on domain
>>> creation, but not logdirty range structures?  It seems like allocating
>>> altp2m structures on-demand, rather than at domain creation time, might
>>> make a lot of the reasoning here simpler.
>>
>> I assume that this question is not addressed to me, since I'm not able
>> to answer it - I can only assume that having less heap used has been
>> preferred.

I'm asking you because you've recently been going through this code, and
probably have at least as much familiarity with it as I do.  I can't
immediately see any reason to allocate them at domain creation time.
Maybe you can and maybe you can't, but I won't know until I ask. :-)

> Actually I now realize that you're asking why the hostp2m rangeset is
> created via paging_domain_init() in arch_domain_create() (so immediately
> on domain creation) while I'm allocating the altp2m rangesets on altp2m
> init.
>
> I'm doing that to save memory, since we can have MAX_ALTP2M altp2ms
> (which is currently 10), and only two active altp2ms - that means that I
> would alloc

Re: [Xen-devel] [PATCH V5 2/3] x86/mm: allocate logdirty_ranges for altp2ms

2018-11-14 Thread Razvan Cojocaru 
On 11/14/18 1:58 PM, George Dunlap wrote:
> On 11/13/18 6:43 PM, Razvan Cojocaru wrote:
>> On 11/13/18 7:57 PM, George Dunlap wrote:
>>> On 11/11/18 2:07 PM, Razvan Cojocaru wrote:
 @@ -2341,6 +2380,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, 
 unsigned int idx)
  {
  p2m_flush_table(d->arch.altp2m_p2m[idx]);
  /* Uninit and reinit ept to force TLB shootdown */
 +p2m_free_logdirty(d->arch.altp2m_p2m[idx]);
  ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
  ept_p2m_init(d->arch.altp2m_p2m[idx]);
  d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
>>>
>>> (In case I forget: Also, this is called without holding the appropriate
>>> p2m lock. )
>>
>> Could you please provide more details? I have assumed that at the point
>> of calling a function called p2m_destroy_altp2m_by_id() it should be
>> safe to tear the altp2m down without further precaution.
> 
> Are you absolutely positive that at this point there's no way anywhere
> else in Xen might be doing something with this p2m struct?
> 
> If so, then 1) there should be a comment there explaining why that's the
> case, and 2) ideally we should refactor p2m_flush_table such that we can
> call what's now p2m_flush_table_locked() without the lock.

AFAICT the only place p2m_destroy_altp2m_by_id() is ever called is in
arch/x86/hvm/hvm.c's do_altp2m_op() (on HVMOP_altp2m_destroy_p2m), which
is done under domain lock. Is that insufficient?

>> I think you're saying that I should p2m_lock(d->arch.altp2m_p2m[idx])
>> just for the duration of the p2m_free_logdirty() call?
> 
> The same argument really goes for ept_p2m_uninit/init -- uninit actually
> frees a data structure; if anyone else may be using that, you'll run
> into a use-after-free bug.  (Although that really needs to be changed as
> well -- freeing and re-allocating a structure just to set all the bits
> is ridiculous.)
> 
> If we need locking, then I'd grab the p2m lock before p2m_flush_table()
> (calling p2m_flush_table_locked() instead), and release it after the
> ept_p2m_init().
> 
> I realize you didn't write this code, and so I'm not holding you
> responsible for all the changes I mentioned above.  But if we're going
> to add the p2m_free_logdirty() call, we do need to either grab the lock
> or add a comment explaining why it's not necessary; we might as well fix
> it properly at the same time.
> 
> p2m_flush_table() already grabs and releases the lock; so grabbing the
> lock over all four calls won't add any more overhead (or risk of
> deadlock) than what we already have.

Of course, I'll use p2m_flush_table_locked().

>>> I'm a bit suspicious of long strings of these sorts of functions in the
>>> middle of another function.  It turns out that there are three copies of
>>> this sequence of function calls (p2m_flush_table -> ept_p2m_uninit ->
>>> ept_p2m_init):
>>>
>>> * Here (p2m_destroy_altp2m_id), when the user asks for the alt2m index
>>> to be destroyed
>>>
>>> * In p2m_flush_altp2m(), which is called when altp2m is disabled for a
>>> domain
>>>
>>> * In p2m_reset_altp2m(), which is called when an entry in the hostp2m is
>>> set to INVALID_MFN.
>>>
>>> Presumably in p2m_reset_altp2m() we don't want to call
>>> p2m_free_logdirty(), as the altp2m is still active and we want to keep
>>> the logdirty ranges around.  But in p2m_flush_altp2m(), I'm pretty sure
>>> we do want to discard them: when altp2m is enabled again,
>>> p2m_init_logdirty() will return early, leaving the old rangesets in
>>> place; if the hostp2m rangesets have changed between the time altp2m was
>>> disabled and enabled again, the rangeset_merge() may have incorrect results.
>>
>> I'll call p2m_free_logdirty() in p2m_flush_altp2m() as well.
> 
> I was more thinking of refactoring those two to share the same code, and
> potentially having p2m_reset_altp2m() share the same code as well.  The
> reason you missed the p2m_flush_altp2m() there was because of the code
> duplication.

Right, I'll do my best to refactor that then. TBH I'm not a big fan of
that extra verbosity either but thought the least code churn would be
good for reviewing.

> Or alternately...
> 
 Is there any particular reason we allocate the p2m structures on domain
 creation, but not logdirty range structures?  It seems like allocating
 altp2m structures on-demand, rather than at domain creation time, might
 make a lot of the reasoning here simpler.
>>>
>>> I assume that this question is not addressed to me, since I'm not able
>>> to answer it - I can only assume that having less heap used has been
>>> preferred.
> 
> I'm asking you because you've recently been going through this code, and
> probably have at least as much familiarity with it as I do.  I can't
> immediately see any reason to allocate them at domain creation time.
> Maybe you can and maybe you can't, but I won't know until I ask. :-)

I've looked at the code closer today, an

Re: [Xen-devel] [PATCH V5 2/3] x86/mm: allocate logdirty_ranges for altp2ms

2018-11-14 Thread Jan Beulich 
>>> On 14.11.18 at 13:50,  wrote:
> On 11/14/18 1:58 PM, George Dunlap wrote:
>> On 11/13/18 6:43 PM, Razvan Cojocaru wrote:
>>> On 11/13/18 7:57 PM, George Dunlap wrote:
 On 11/11/18 2:07 PM, Razvan Cojocaru wrote:
> @@ -2341,6 +2380,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, 
> unsigned int idx)
>  {
>  p2m_flush_table(d->arch.altp2m_p2m[idx]);
>  /* Uninit and reinit ept to force TLB shootdown */
> +p2m_free_logdirty(d->arch.altp2m_p2m[idx]);
>  ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
>  ept_p2m_init(d->arch.altp2m_p2m[idx]);
>  d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);

 (In case I forget: Also, this is called without holding the appropriate
 p2m lock. )
>>>
>>> Could you please provide more details? I have assumed that at the point
>>> of calling a function called p2m_destroy_altp2m_by_id() it should be
>>> safe to tear the altp2m down without further precaution.
>> 
>> Are you absolutely positive that at this point there's no way anywhere
>> else in Xen might be doing something with this p2m struct?
>> 
>> If so, then 1) there should be a comment there explaining why that's the
>> case, and 2) ideally we should refactor p2m_flush_table such that we can
>> call what's now p2m_flush_table_locked() without the lock.
> 
> AFAICT the only place p2m_destroy_altp2m_by_id() is ever called is in
> arch/x86/hvm/hvm.c's do_altp2m_op() (on HVMOP_altp2m_destroy_p2m), which
> is done under domain lock. Is that insufficient?

Holding the domain lock does not imply nothing can happen to the
domain elsewhere. Only if both parties hold the _same_ lock there
is a guarantee of serialization between both.

> Is there any particular reason we allocate the p2m structures on domain
> creation, but not logdirty range structures?  It seems like allocating
> altp2m structures on-demand, rather than at domain creation time, might
> make a lot of the reasoning here simpler.

 I assume that this question is not addressed to me, since I'm not able
 to answer it - I can only assume that having less heap used has been
 preferred.
>> 
>> I'm asking you because you've recently been going through this code, and
>> probably have at least as much familiarity with it as I do.  I can't
>> immediately see any reason to allocate them at domain creation time.
>> Maybe you can and maybe you can't, but I won't know until I ask. :-)
> 
> I've looked at the code closer today, and there's no reason as far as I
> can tell why we shouldn't allocate altp2ms on-demand. However, changing
> the code is somewhat involved at this point, since there's a lot of:
> 
> 2357 if ( d->arch.altp2m_eptp[idx] != mfn_x(INVALID_MFN) )
> 2358 {
> 2359 p2m = d->arch.altp2m_p2m[idx];
> 2360
> 2361 if ( !_atomic_read(p2m->active_vcpus) )
> 2362 {
> 2363 p2m_flush_table(d->arch.altp2m_p2m[idx]);
> 2364 /* Uninit and reinit ept to force TLB shootdown */
> 2365 ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
> 2366 ept_p2m_init(d->arch.altp2m_p2m[idx]);
> 2367 d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
> 2368 rc = 0;
> 2369 }
> 2370 }
> 
> going on. That is, code checking that d->arch.altp2m_eptp[idx] !=
> mfn_x(INVALID_MFN), and then blindly assuming that p2m will not be NULL
> and is usable.

Wouldn't the implication of George's proposal be that
d->arch.altp2m_eptp[] slots get demand-populated, too?

Jan



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

Re: [Xen-devel] [PATCH V5 2/3] x86/mm: allocate logdirty_ranges for altp2ms

2018-11-14 Thread Razvan Cojocaru 
On 11/14/18 4:00 PM, Jan Beulich wrote:
 On 14.11.18 at 13:50,  wrote:
>> On 11/14/18 1:58 PM, George Dunlap wrote:
>>> On 11/13/18 6:43 PM, Razvan Cojocaru wrote:
 On 11/13/18 7:57 PM, George Dunlap wrote:
> On 11/11/18 2:07 PM, Razvan Cojocaru wrote:
>> @@ -2341,6 +2380,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, 
>> unsigned int idx)
>>  {
>>  p2m_flush_table(d->arch.altp2m_p2m[idx]);
>>  /* Uninit and reinit ept to force TLB shootdown */
>> +p2m_free_logdirty(d->arch.altp2m_p2m[idx]);
>>  ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
>>  ept_p2m_init(d->arch.altp2m_p2m[idx]);
>>  d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
>
> (In case I forget: Also, this is called without holding the appropriate
> p2m lock. )

 Could you please provide more details? I have assumed that at the point
 of calling a function called p2m_destroy_altp2m_by_id() it should be
 safe to tear the altp2m down without further precaution.
>>>
>>> Are you absolutely positive that at this point there's no way anywhere
>>> else in Xen might be doing something with this p2m struct?
>>>
>>> If so, then 1) there should be a comment there explaining why that's the
>>> case, and 2) ideally we should refactor p2m_flush_table such that we can
>>> call what's now p2m_flush_table_locked() without the lock.
>>
>> AFAICT the only place p2m_destroy_altp2m_by_id() is ever called is in
>> arch/x86/hvm/hvm.c's do_altp2m_op() (on HVMOP_altp2m_destroy_p2m), which
>> is done under domain lock. Is that insufficient?
> 
> Holding the domain lock does not imply nothing can happen to the
> domain elsewhere. Only if both parties hold the _same_ lock there
> is a guarantee of serialization between both.

Right, I was under the impression that for the duration of a HVMOP (or
DOMCTL) nothing moves in the domain.

In that case, we do need the locking as George has suggested.

>> Is there any particular reason we allocate the p2m structures on domain
>> creation, but not logdirty range structures?  It seems like allocating
>> altp2m structures on-demand, rather than at domain creation time, might
>> make a lot of the reasoning here simpler.
>
> I assume that this question is not addressed to me, since I'm not able
> to answer it - I can only assume that having less heap used has been
> preferred.
>>>
>>> I'm asking you because you've recently been going through this code, and
>>> probably have at least as much familiarity with it as I do.  I can't
>>> immediately see any reason to allocate them at domain creation time.
>>> Maybe you can and maybe you can't, but I won't know until I ask. :-)
>>
>> I've looked at the code closer today, and there's no reason as far as I
>> can tell why we shouldn't allocate altp2ms on-demand. However, changing
>> the code is somewhat involved at this point, since there's a lot of:
>>
>> 2357 if ( d->arch.altp2m_eptp[idx] != mfn_x(INVALID_MFN) )
>> 2358 {
>> 2359 p2m = d->arch.altp2m_p2m[idx];
>> 2360
>> 2361 if ( !_atomic_read(p2m->active_vcpus) )
>> 2362 {
>> 2363 p2m_flush_table(d->arch.altp2m_p2m[idx]);
>> 2364 /* Uninit and reinit ept to force TLB shootdown */
>> 2365 ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
>> 2366 ept_p2m_init(d->arch.altp2m_p2m[idx]);
>> 2367 d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
>> 2368 rc = 0;
>> 2369 }
>> 2370 }
>>
>> going on. That is, code checking that d->arch.altp2m_eptp[idx] !=
>> mfn_x(INVALID_MFN), and then blindly assuming that p2m will not be NULL
>> and is usable.
> 
> Wouldn't the implication of George's proposal be that
> d->arch.altp2m_eptp[] slots get demand-populated, too?

Of course, but still we must make sure that that really does happen in
all (corner) cases, and that the two never get out of sync (some
function sets d->arch.altp2m_p2m[idx] to NULL while leaving
d->arch.altp2m_eptp[idx] != mfn_x(INVALID_MFN), for example).

My point was just that this requires quite a bit of testing to make sure
we got it right IMHO. Which we should do, but it's also very important
to get the display problem fixed.


Thanks,
Razvan

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

Re: [Xen-devel] [PATCH V5 2/3] x86/mm: allocate logdirty_ranges for altp2ms

2018-11-14 Thread Jan Beulich 
>>> On 14.11.18 at 15:05,  wrote:
> On 11/14/18 4:00 PM, Jan Beulich wrote:
> On 14.11.18 at 13:50,  wrote:
>>> On 11/14/18 1:58 PM, George Dunlap wrote:
 On 11/13/18 6:43 PM, Razvan Cojocaru wrote:
> On 11/13/18 7:57 PM, George Dunlap wrote:
>> On 11/11/18 2:07 PM, Razvan Cojocaru wrote:
>>> @@ -2341,6 +2380,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, 
>>> unsigned int idx)
>>>  {
>>>  p2m_flush_table(d->arch.altp2m_p2m[idx]);
>>>  /* Uninit and reinit ept to force TLB shootdown */
>>> +p2m_free_logdirty(d->arch.altp2m_p2m[idx]);
>>>  ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
>>>  ept_p2m_init(d->arch.altp2m_p2m[idx]);
>>>  d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
>>
>> (In case I forget: Also, this is called without holding the appropriate
>> p2m lock. )
>
> Could you please provide more details? I have assumed that at the point
> of calling a function called p2m_destroy_altp2m_by_id() it should be
> safe to tear the altp2m down without further precaution.

 Are you absolutely positive that at this point there's no way anywhere
 else in Xen might be doing something with this p2m struct?

 If so, then 1) there should be a comment there explaining why that's the
 case, and 2) ideally we should refactor p2m_flush_table such that we can
 call what's now p2m_flush_table_locked() without the lock.
>>>
>>> AFAICT the only place p2m_destroy_altp2m_by_id() is ever called is in
>>> arch/x86/hvm/hvm.c's do_altp2m_op() (on HVMOP_altp2m_destroy_p2m), which
>>> is done under domain lock. Is that insufficient?
>> 
>> Holding the domain lock does not imply nothing can happen to the
>> domain elsewhere. Only if both parties hold the _same_ lock there
>> is a guarantee of serialization between both.
> 
> Right, I was under the impression that for the duration of a HVMOP (or
> DOMCTL) nothing moves in the domain.

Well, if you need such behavior, you need to pause the domain (as
various domctl-s actually do).

Jan



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