Re: [Xen-devel] [PATCH v4 07/39] arm/p2m: Move hostp2m init/teardown to individual functions

[Julien Grall]

Hi Sergej,

On 30/08/17 19:32, Sergej Proskurin wrote:

This commit pulls out generic init/teardown functionality out of
"p2m_init" and "p2m_teardown" into "p2m_init_one", "p2m_teardown_one",
and "p2m_flush_table" functions.  This allows our future implementation
to reuse existing code for the initialization/teardown of altp2m views.


You likely want to expand the commit message here to explain:
1) Why you export the functions
2) How come you split p2m_teardown is now also flushing the tables...

Very likely 2) should be a separate patch as it is an addition of the 
code and not a split.




Signed-off-by: Sergej Proskurin 
---
Cc: Stefano Stabellini 
Cc: Julien Grall 
---
v2: Added the function p2m_flush_table to the previous version.

v3: Removed struct vttbr.

 Moved define INVALID_VTTBR to p2m.h.

 Exported function prototypes of "p2m_flush_table", "p2m_init_one",
 and "p2m_teardown_one" in p2m.h.

 Extended the function "p2m_flush_table" by additionally resetting
 the fields lowest_mapped_gfn and max_mapped_gfn.

 Added a "p2m_flush_tlb" call in "p2m_flush_table". On altp2m reset
 in function "altp2m_reset", it is important to flush the TLBs after
 clearing the root table pages and before clearing the intermediate
 altp2m page tables to prevent illegal access to stalled TLB entries
 on currently active VCPUs.

 Added a check checking whether p2m->root is NULL in p2m_flush_table.

 Renamed the function "p2m_free_one" to "p2m_teardown_one".

 Removed resetting p2m->vttbr in "p2m_teardown_one", as it the p2m
 will be destroyed afterwards.

 Moved call to "p2m_alloc_table" back to "p2m_init_one".

 Moved the introduction of the type p2m_class_t out of this patch.

 Moved the backpointer to the struct domain out of the struct
 p2m_domain.

v4: Replaced the former use of clear_and_clean_page in p2m_flush_table
 by a routine that invalidates every p2m entry atomically. This
 avoids inconsistencies on CPUs that continue to use the views that
 are to be flushed (e.g., see altp2m_reset).

 Removed unnecessary initializations in the functions "p2m_init_one"
 and "p2m_teardown_one".

 Removed the define INVALID_VTTBR as it is not used any more.

 Cosmetic fixes.
---
  xen/arch/arm/p2m.c| 74 +++
  xen/include/asm-arm/p2m.h |  9 ++
  2 files changed, 78 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 5e86368010..3a1a38e7af 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1203,27 +1203,65 @@ static void p2m_free_vmid(struct domain *d)
  spin_unlock(_alloc_lock);
  }
  
-void p2m_teardown(struct domain *d)

+/* Reset this p2m table to be empty. */
+void p2m_flush_table(struct p2m_domain *p2m)
  {
-struct p2m_domain *p2m = p2m_get_hostp2m(d);
  struct page_info *pg;
+unsigned int i, j;
+lpae_t *table;
+
+if ( p2m->root )
+{
+/* Clear all concatenated root level pages. */
+for ( i = 0; i < P2M_ROOT_PAGES; i++ )
+{
+table = __map_domain_page(p2m->root + i);
+
+for ( j = 0; j < LPAE_ENTRIES; j++ )
+{
+lpae_t *entry = table + j;
+
+/*
+ * Individual altp2m views can be flushed, whilst altp2m is
+ * active. To avoid inconsistencies on CPUs that continue to
+ * use the views to be flushed (e.g., see altp2m_reset), we
+ * must remove every p2m entry atomically.


When I read this comment, I wonder how this is safe to clear without any 
locking? At least to prevent multiple instance to modify the P2M at the 
same time. If you expect the caller to do it for you, then an 
ASSERT(...) is necessary at the beginning of the function.


Likely you want this function do the locking.

Also that comment is more suitable on top of the for loop rather than here.


+ */
+p2m_remove_pte(entry, p2m->clean_pte);
+}
+}


You still haven't address my comment regarding the overhead you 
introduce whilst tearing down a P2M (e.g when the domain is destroyed).



+}
+
+/*
+ * Flush TLBs before releasing remaining intermediate p2m page tables to
+ * prevent illegal access to stalled TLB entries.
+ */
+p2m_flush_tlb(p2m);


Again, this is called by p2m_flush_table where the P2M may not have been 
allocated because the initialization failed. So trying to flush TLB may 
lead to a panic in Xen (the vttbr is invalid).


Furthermore we already flush the TLBs when creating the domain (see 
p2m_alloc_table). So you add yet another overhead.


  
+/* Free the rest of the trie pages back to the paging pool. */

  while ( (pg = page_list_remove_head(>pages)) )
  free_domheap_page(pg);
  
+

[Xen-devel] [PATCH v4 07/39] arm/p2m: Move hostp2m init/teardown to individual functions

[Sergej Proskurin]

This commit pulls out generic init/teardown functionality out of
"p2m_init" and "p2m_teardown" into "p2m_init_one", "p2m_teardown_one",
and "p2m_flush_table" functions.  This allows our future implementation
to reuse existing code for the initialization/teardown of altp2m views.

Signed-off-by: Sergej Proskurin 
---
Cc: Stefano Stabellini 
Cc: Julien Grall 
---
v2: Added the function p2m_flush_table to the previous version.

v3: Removed struct vttbr.

Moved define INVALID_VTTBR to p2m.h.

Exported function prototypes of "p2m_flush_table", "p2m_init_one",
and "p2m_teardown_one" in p2m.h.

Extended the function "p2m_flush_table" by additionally resetting
the fields lowest_mapped_gfn and max_mapped_gfn.

Added a "p2m_flush_tlb" call in "p2m_flush_table". On altp2m reset
in function "altp2m_reset", it is important to flush the TLBs after
clearing the root table pages and before clearing the intermediate
altp2m page tables to prevent illegal access to stalled TLB entries
on currently active VCPUs.

Added a check checking whether p2m->root is NULL in p2m_flush_table.

Renamed the function "p2m_free_one" to "p2m_teardown_one".

Removed resetting p2m->vttbr in "p2m_teardown_one", as it the p2m
will be destroyed afterwards.

Moved call to "p2m_alloc_table" back to "p2m_init_one".

Moved the introduction of the type p2m_class_t out of this patch.

Moved the backpointer to the struct domain out of the struct
p2m_domain.

v4: Replaced the former use of clear_and_clean_page in p2m_flush_table
by a routine that invalidates every p2m entry atomically. This
avoids inconsistencies on CPUs that continue to use the views that
are to be flushed (e.g., see altp2m_reset).

Removed unnecessary initializations in the functions "p2m_init_one"
and "p2m_teardown_one".

Removed the define INVALID_VTTBR as it is not used any more.

Cosmetic fixes.
---
 xen/arch/arm/p2m.c| 74 +++
 xen/include/asm-arm/p2m.h |  9 ++
 2 files changed, 78 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 5e86368010..3a1a38e7af 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1203,27 +1203,65 @@ static void p2m_free_vmid(struct domain *d)
 spin_unlock(_alloc_lock);
 }
 
-void p2m_teardown(struct domain *d)
+/* Reset this p2m table to be empty. */
+void p2m_flush_table(struct p2m_domain *p2m)
 {
-struct p2m_domain *p2m = p2m_get_hostp2m(d);
 struct page_info *pg;
+unsigned int i, j;
+lpae_t *table;
+
+if ( p2m->root )
+{
+/* Clear all concatenated root level pages. */
+for ( i = 0; i < P2M_ROOT_PAGES; i++ )
+{
+table = __map_domain_page(p2m->root + i);
+
+for ( j = 0; j < LPAE_ENTRIES; j++ )
+{
+lpae_t *entry = table + j;
+
+/*
+ * Individual altp2m views can be flushed, whilst altp2m is
+ * active. To avoid inconsistencies on CPUs that continue to
+ * use the views to be flushed (e.g., see altp2m_reset), we
+ * must remove every p2m entry atomically.
+ */
+p2m_remove_pte(entry, p2m->clean_pte);
+}
+}
+}
+
+/*
+ * Flush TLBs before releasing remaining intermediate p2m page tables to
+ * prevent illegal access to stalled TLB entries.
+ */
+p2m_flush_tlb(p2m);
 
+/* Free the rest of the trie pages back to the paging pool. */
 while ( (pg = page_list_remove_head(>pages)) )
 free_domheap_page(pg);
 
+p2m->lowest_mapped_gfn = INVALID_GFN;
+p2m->max_mapped_gfn = _gfn(0);
+}
+
+void p2m_teardown_one(struct p2m_domain *p2m)
+{
+p2m_flush_table(p2m);
+
 if ( p2m->root )
 free_domheap_pages(p2m->root, P2M_ROOT_ORDER);
 
 p2m->root = NULL;
 
-p2m_free_vmid(d);
+p2m_free_vmid(p2m->domain);
 
 radix_tree_destroy(>mem_access_settings, NULL);
 }
 
-int p2m_init(struct domain *d)
+int p2m_init_one(struct domain *d, struct p2m_domain *p2m)
 {
-struct p2m_domain *p2m = p2m_get_hostp2m(d);
 int rc = 0;
 unsigned int cpu;
 
@@ -1268,6 +1306,32 @@ int p2m_init(struct domain *d)
 return rc;
 }
 
+static void p2m_teardown_hostp2m(struct domain *d)
+{
+struct p2m_domain *p2m = p2m_get_hostp2m(d);
+
+p2m_teardown_one(p2m);
+}
+
+void p2m_teardown(struct domain *d)
+{
+p2m_teardown_hostp2m(d);
+}
+
+static int p2m_init_hostp2m(struct domain *d)
+{
+struct p2m_domain *p2m = p2m_get_hostp2m(d);
+
+p2m->p2m_class = p2m_host;
+
+return p2m_init_one(d, p2m);
+}
+
+int p2m_init(struct domain *d)
+{
+return p2m_init_hostp2m(d);
+}
+
 /*
  * The function will go through the p2m and remove page reference when it
  * is required. The mapping will be removed from the