Re: [Xen-devel] [PATCH] x86/p2m: Stop other vcpus using a nested p2m before clearing it

[George Dunlap]

On 08/02/17 18:06, George Dunlap wrote:
> On 08/02/17 13:20, Andrew Cooper wrote:
>> On 08/02/17 13:13, Jan Beulich wrote:
>> On 07.02.17 at 19:48,  wrote:
 Until the IPI has completed, other processors might be running on this 
 nested
 p2m object.  clear_domain_page() does not guarantee to make 8-byte atomic
 updates, which means that a pagewalk on a remote processor might encounter 
 a
 partial update.

 This is currently safe as other issues prevents a nested p2m ever being 
 shared
 between two cpus (although this is contrary to the original plan).

 Setting p2m->np2m_base to P2M_BASE_EADDR before the IPI ensures that the 
 IPI'd
 processors won't continue to use the flushed mappings.

 While modifying this function, remove all the trailing whitespace and tweak
 style in the affected areas.

 Signed-off-by: Andrew Cooper 
>>> Reviewed-by: Jan Beulich 
>>> but ...
>>>
 @@ -1633,19 +1635,21 @@ p2m_flush_table(struct p2m_domain *p2m)
  
  /* This is no longer a valid nested p2m for any address space */
  p2m->np2m_base = P2M_BASE_EADDR;
 -
 -/* Zap the top level of the trie */
 -mfn = pagetable_get_mfn(p2m_get_pagetable(p2m));
 -clear_domain_page(mfn);
  
  /* Make sure nobody else is using this p2m table */
  nestedhvm_vmcx_flushtlb(p2m);
  
 +/* Zap the top level of the trie */
>>> s/trie/tree/ here, as you touch it anyway?
>>
>> Trie here refers to the datastructure https://en.wikipedia.org/wiki/Trie
>> which is the structure implemented by processor pagetables.  It is more
>> specific than just calling them trees.
> 
> Never heard that before, but we seem to already have at least six
> instances in the hypervisor (and an ocaml file called 'trie.mli'), so I
> guess there's precedent. :-)
> 
> Reviewed-by: George Dunlap 
> 
> I'll check this one in.

Or maybe I won't. :-)

 -George


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

Re: [Xen-devel] [PATCH] x86/p2m: Stop other vcpus using a nested p2m before clearing it

[George Dunlap]

On 08/02/17 13:20, Andrew Cooper wrote:
> On 08/02/17 13:13, Jan Beulich wrote:
> On 07.02.17 at 19:48,  wrote:
>>> Until the IPI has completed, other processors might be running on this 
>>> nested
>>> p2m object.  clear_domain_page() does not guarantee to make 8-byte atomic
>>> updates, which means that a pagewalk on a remote processor might encounter a
>>> partial update.
>>>
>>> This is currently safe as other issues prevents a nested p2m ever being 
>>> shared
>>> between two cpus (although this is contrary to the original plan).
>>>
>>> Setting p2m->np2m_base to P2M_BASE_EADDR before the IPI ensures that the 
>>> IPI'd
>>> processors won't continue to use the flushed mappings.
>>>
>>> While modifying this function, remove all the trailing whitespace and tweak
>>> style in the affected areas.
>>>
>>> Signed-off-by: Andrew Cooper 
>> Reviewed-by: Jan Beulich 
>> but ...
>>
>>> @@ -1633,19 +1635,21 @@ p2m_flush_table(struct p2m_domain *p2m)
>>>  
>>>  /* This is no longer a valid nested p2m for any address space */
>>>  p2m->np2m_base = P2M_BASE_EADDR;
>>> -
>>> -/* Zap the top level of the trie */
>>> -mfn = pagetable_get_mfn(p2m_get_pagetable(p2m));
>>> -clear_domain_page(mfn);
>>>  
>>>  /* Make sure nobody else is using this p2m table */
>>>  nestedhvm_vmcx_flushtlb(p2m);
>>>  
>>> +/* Zap the top level of the trie */
>> s/trie/tree/ here, as you touch it anyway?
> 
> Trie here refers to the datastructure https://en.wikipedia.org/wiki/Trie
> which is the structure implemented by processor pagetables.  It is more
> specific than just calling them trees.

Never heard that before, but we seem to already have at least six
instances in the hypervisor (and an ocaml file called 'trie.mli'), so I
guess there's precedent. :-)

Reviewed-by: George Dunlap 

I'll check this one in.

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

Re: [Xen-devel] [PATCH] x86/p2m: Stop other vcpus using a nested p2m before clearing it

[Andrew Cooper]

On 08/02/17 13:13, Jan Beulich wrote:
 On 07.02.17 at 19:48,  wrote:
>> Until the IPI has completed, other processors might be running on this nested
>> p2m object.  clear_domain_page() does not guarantee to make 8-byte atomic
>> updates, which means that a pagewalk on a remote processor might encounter a
>> partial update.
>>
>> This is currently safe as other issues prevents a nested p2m ever being 
>> shared
>> between two cpus (although this is contrary to the original plan).
>>
>> Setting p2m->np2m_base to P2M_BASE_EADDR before the IPI ensures that the 
>> IPI'd
>> processors won't continue to use the flushed mappings.
>>
>> While modifying this function, remove all the trailing whitespace and tweak
>> style in the affected areas.
>>
>> Signed-off-by: Andrew Cooper 
> Reviewed-by: Jan Beulich 
> but ...
>
>> @@ -1633,19 +1635,21 @@ p2m_flush_table(struct p2m_domain *p2m)
>>  
>>  /* This is no longer a valid nested p2m for any address space */
>>  p2m->np2m_base = P2M_BASE_EADDR;
>> -
>> -/* Zap the top level of the trie */
>> -mfn = pagetable_get_mfn(p2m_get_pagetable(p2m));
>> -clear_domain_page(mfn);
>>  
>>  /* Make sure nobody else is using this p2m table */
>>  nestedhvm_vmcx_flushtlb(p2m);
>>  
>> +/* Zap the top level of the trie */
> s/trie/tree/ here, as you touch it anyway?

Trie here refers to the datastructure https://en.wikipedia.org/wiki/Trie
which is the structure implemented by processor pagetables.  It is more
specific than just calling them trees.

~Andrew

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

Re: [Xen-devel] [PATCH] x86/p2m: Stop other vcpus using a nested p2m before clearing it

[Jan Beulich]

>>> On 07.02.17 at 19:48,  wrote:
> Until the IPI has completed, other processors might be running on this nested
> p2m object.  clear_domain_page() does not guarantee to make 8-byte atomic
> updates, which means that a pagewalk on a remote processor might encounter a
> partial update.
> 
> This is currently safe as other issues prevents a nested p2m ever being shared
> between two cpus (although this is contrary to the original plan).
> 
> Setting p2m->np2m_base to P2M_BASE_EADDR before the IPI ensures that the IPI'd
> processors won't continue to use the flushed mappings.
> 
> While modifying this function, remove all the trailing whitespace and tweak
> style in the affected areas.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Jan Beulich 
but ...

> @@ -1633,19 +1635,21 @@ p2m_flush_table(struct p2m_domain *p2m)
>  
>  /* This is no longer a valid nested p2m for any address space */
>  p2m->np2m_base = P2M_BASE_EADDR;
> -
> -/* Zap the top level of the trie */
> -mfn = pagetable_get_mfn(p2m_get_pagetable(p2m));
> -clear_domain_page(mfn);
>  
>  /* Make sure nobody else is using this p2m table */
>  nestedhvm_vmcx_flushtlb(p2m);
>  
> +/* Zap the top level of the trie */

s/trie/tree/ here, as you touch it anyway?

Jan


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

Re: [Xen-devel] [PATCH] x86/p2m: Stop other vcpus using a nested p2m before clearing it

[Tim Deegan]

At 18:48 + on 07 Feb (1486493293), Andrew Cooper wrote:
> Until the IPI has completed, other processors might be running on this nested
> p2m object.  clear_domain_page() does not guarantee to make 8-byte atomic
> updates, which means that a pagewalk on a remote processor might encounter a
> partial update.
> 
> This is currently safe as other issues prevents a nested p2m ever being shared
> between two cpus (although this is contrary to the original plan).
> 
> Setting p2m->np2m_base to P2M_BASE_EADDR before the IPI ensures that the IPI'd
> processors won't continue to use the flushed mappings.
> 
> While modifying this function, remove all the trailing whitespace and tweak
> style in the affected areas.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Tim Deegan 

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

[Xen-devel] [PATCH] x86/p2m: Stop other vcpus using a nested p2m before clearing it

[Andrew Cooper]

Until the IPI has completed, other processors might be running on this nested
p2m object.  clear_domain_page() does not guarantee to make 8-byte atomic
updates, which means that a pagewalk on a remote processor might encounter a
partial update.

This is currently safe as other issues prevents a nested p2m ever being shared
between two cpus (although this is contrary to the original plan).

Setting p2m->np2m_base to P2M_BASE_EADDR before the IPI ensures that the IPI'd
processors won't continue to use the flushed mappings.

While modifying this function, remove all the trailing whitespace and tweak
style in the affected areas.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Tim Deegan 
CC: George Dunlap 
---
 xen/arch/x86/mm/p2m.c | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 7675e9c..c5b1642 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1618,8 +1618,10 @@ p2m_flush_table(struct p2m_domain *p2m)
 
 p2m_lock(p2m);
 
-/* "Host" p2m tables can have shared entries  that need a bit more 
- * care when discarding them */
+/*
+ * "Host" p2m tables can have shared entries  that need a bit more care
+ * when discarding them.
+ */
 ASSERT(!p2m_is_hostp2m(p2m));
 /* Nested p2m's do not do pod, hence the asserts (and no pod lock)*/
 ASSERT(page_list_empty(>pod.super));
@@ -1633,19 +1635,21 @@ p2m_flush_table(struct p2m_domain *p2m)
 
 /* This is no longer a valid nested p2m for any address space */
 p2m->np2m_base = P2M_BASE_EADDR;
-
-/* Zap the top level of the trie */
-mfn = pagetable_get_mfn(p2m_get_pagetable(p2m));
-clear_domain_page(mfn);
 
 /* Make sure nobody else is using this p2m table */
 nestedhvm_vmcx_flushtlb(p2m);
 
+/* Zap the top level of the trie */
+mfn = pagetable_get_mfn(p2m_get_pagetable(p2m));
+clear_domain_page(mfn);
+
 /* Free the rest of the trie pages back to the paging pool */
 top = mfn_to_page(mfn);
 while ( (pg = page_list_remove_head(>pages)) )
-if ( pg != top ) 
+{
+if ( pg != top )
 d->arch.paging.free_page(d, pg);
+}
 page_list_add(top, >pages);
 
 p2m_unlock(p2m);
-- 
2.1.4


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