[PATCH v3 0/4] xen/arm: p2m: Add support to remove empty translation table

[PATCH v3 0/4] xen/arm: p2m: Add support to remove empty translation table

[Julien Grall]

Hello,

The main purpose of this patch series is to allow creation of superpage when
it has been previously shattered.

The first patch is not related to the main purpose of this series but fix a
latent bug I've found while looking at the p2m code.

For all the changes see in each patch.

Sincerely yours,

Julien Grall (4):
  xen/arm: p2m: Flush for every exit paths in apply_p2m_changes
  xen/arm: p2m: Store the page for each mapping
  xen/arm: p2m: Introduce a helper to remove an entry in the page table
  xen/arm: p2m: Remove translation table when it's empty

 xen/arch/arm/p2m.c       | 86 +++++++++++++++++++++++++++++++++++++++++++++---
 xen/include/asm-arm/mm.h |  6 ++++
 2 files changed, 87 insertions(+), 5 deletions(-)

-- 
2.1.4

[PATCH v3 1/4] xen/arm: p2m: Flush for every exit paths in apply_p2m_changes

[Julien Grall]

Currently, the TLB is not flushed if an error occured while updating the
stage-2 p2m. However, the TLB will contain stale mappings for any entry
updated so far.

To avoid a such situation, flush on every exit path when the variable
"flush" is set.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---

    Changes in v2:
        - Add Ian's Acked-by
        - Fix typoes
---
 xen/arch/arm/p2m.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index e396c40..f910cab 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1010,7 +1010,7 @@ static int apply_p2m_changes(struct domain *d,
                 if ( (egfn - sgfn) > progress && !(progress & mask) )
                 {
                     rc = progress;
-                    goto tlbflush;
+                    goto out;
                 }
                 break;
             }
@@ -1096,15 +1096,13 @@ static int apply_p2m_changes(struct domain *d,
 
     rc = 0;
 
-tlbflush:
+out:
     if ( flush )
     {
         flush_tlb_domain(d);
         iommu_iotlb_flush(d, sgfn, egfn - sgfn);
     }
 
-out:
-
     if ( rc < 0 && ( op == INSERT || op == ALLOCATE ) &&
          addr != start_gpaddr )
     {
-- 
2.1.4

[PATCH v3 2/4] xen/arm: p2m: Store the page for each mapping

[Julien Grall]

The page will be use later for reference counting. So we need a quick
access to the page associated to the mapping.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---

    Changes in v2:
        - Add Ian's acked-by
---
 xen/arch/arm/p2m.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index f910cab..f28ae3f 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -942,6 +942,7 @@ static int apply_p2m_changes(struct domain *d,
     int rc, ret;
     struct p2m_domain *p2m = &d->arch.p2m;
     lpae_t *mappings[4] = { NULL, NULL, NULL, NULL };
+    struct page_info *pages[4] = { NULL, NULL, NULL, NULL };
     paddr_t addr, orig_maddr = maddr;
     unsigned int level = 0;
     unsigned int cur_root_table = ~0;
@@ -964,7 +965,10 @@ static int apply_p2m_changes(struct domain *d,
 
     /* Static mapping. P2M_ROOT_PAGES > 1 are handled below */
     if ( P2M_ROOT_PAGES == 1 )
+    {
         mappings[P2M_ROOT_LEVEL] = __map_domain_page(p2m->root);
+        pages[P2M_ROOT_LEVEL] = p2m->root;
+    }
 
     addr = start_gpaddr;
     while ( addr < end_gpaddr )
@@ -1047,6 +1051,7 @@ static int apply_p2m_changes(struct domain *d,
                     unmap_domain_page(mappings[P2M_ROOT_LEVEL]);
                 mappings[P2M_ROOT_LEVEL] =
                     __map_domain_page(p2m->root + root_table);
+                pages[P2M_ROOT_LEVEL] = p2m->root + root_table;
                 cur_root_table = root_table;
                 /* Any mapping further down is now invalid */
                 for ( i = P2M_ROOT_LEVEL; i < 4; i++ )
@@ -1079,6 +1084,7 @@ static int apply_p2m_changes(struct domain *d,
                 if ( mappings[level+1] )
                     unmap_domain_page(mappings[level+1]);
                 mappings[level+1] = map_domain_page(_mfn(entry->p2m.base));
+                pages[level+1] = mfn_to_page(entry->p2m.base);
                 cur_offset[level] = offset;
                 /* Any mapping further down is now invalid */
                 for ( i = level+1; i < 4; i++ )
-- 
2.1.4

[PATCH v3 3/4] xen/arm: p2m: Introduce a helper to remove an entry in the page table

[Julien Grall]

Factorize the code to remove an entry in p2m_remove_pte so we can re-use
it later.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---

    Changes in v2:
        - Add Ian's acked-by
        - Fix typoes
---
 xen/arch/arm/p2m.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index f28ae3f..ae0acf0 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -367,6 +367,14 @@ static inline void p2m_write_pte(lpae_t *p, lpae_t pte, bool_t flush_cache)
         clean_dcache(*p);
 }
 
+static inline void p2m_remove_pte(lpae_t *p, bool_t flush_cache)
+{
+    lpae_t pte;
+
+    memset(&pte, 0x00, sizeof(pte));
+    p2m_write_pte(p, pte, flush_cache);
+}
+
 /*
  * Allocate a new page table page and hook it in via the given entry.
  * apply_one_level relies on this returning 0 on success
@@ -839,8 +847,7 @@ static int apply_one_level(struct domain *d,
 
         *flush = true;
 
-        memset(&pte, 0x00, sizeof(pte));
-        p2m_write_pte(entry, pte, flush_cache);
+        p2m_remove_pte(entry, flush_cache);
         p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), p2m_access_rwx);
 
         *addr += level_size;
-- 
2.1.4

[PATCH v3 4/4] xen/arm: p2m: Remove translation table when it's empty

[Julien Grall]

Currently, the translation table is left in place even if no entries is
inuse. Because of how the p2m code has been implemented, replacing a
translation table by a block (i.e superpage) is not supported. Therefore,
any mapping of a superpage size will be split in smaller chunks making
the translation less efficient.

Replacing a table by a block when a new mapping is added would be too
complicated because it requires to check if all the upper levels are not
inuse and free them if necessary.

Instead, we will remove the empty translation table when the mapping are
removed. To avoid going through all the table checking if no entry is
inuse, a counter representing the number of entry currently inuse is
kept per table translation and updated when an entry change state (i.e
valid <-> invalid).

As Xen allocates a page for each translation table, it's possible to
store the counter in the struct page_info. A new field p2m_refcount as
been introduced in the inuse union for this purpose. This is fine as the
page is only used by the P2M code and nobody touch the other field of
the union type_info.

For record, type_info has not been used because it would require
more work to use it properly as Xen on ARM doesn't yet have the concept
of type.

Once Xen has finished to remove a mapping and all the reference to each
translation table has been updated, the level will be lookup backward to
check if we need first need to free an unused translation table at an
higher level and then the lower levels. This will allow to propagate the
number of reference and free multiple translation table at different level
in one go.

Signed-off-by: Julien Grall <julien.grall@citrix.com>

---
    Changes in v3:
        - Fix indentation in the code again. I hope it's good know.
        - Fix reference counting when the entry became valid

    Changes in v2:
        - Introduce a new field p2m_refcount in the inuse union
        - Update the commit message to explain why we didn't use
        type_info
        - Fix indentation in the code
        - Explain what protect the page in update_reference_mapping
---
 xen/arch/arm/p2m.c       | 65 ++++++++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/mm.h |  6 +++++
 2 files changed, 71 insertions(+)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index ae0acf0..2190908 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -427,6 +427,8 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
 
              write_pte(&p[i], pte);
          }
+
+         page->u.inuse.p2m_refcount = LPAE_ENTRIES;
     }
     else
         clear_page(p);
@@ -936,6 +938,20 @@ static int apply_one_level(struct domain *d,
     BUG(); /* Should never get here */
 }
 
+/*
+ * The page is only used by the P2M code which is protected by the p2m->lock.
+ * So we can avoid to use atomic helpers.
+ */
+static void update_reference_mapping(struct page_info *page,
+                                     lpae_t old_entry,
+                                     lpae_t new_entry)
+{
+    if ( p2m_valid(old_entry) && !p2m_valid(new_entry) )
+        page->u.inuse.p2m_refcount--;
+    else if ( !p2m_valid(old_entry) && p2m_valid(new_entry) )
+        page->u.inuse.p2m_refcount++;
+}
+
 static int apply_p2m_changes(struct domain *d,
                      enum p2m_operation op,
                      paddr_t start_gpaddr,
@@ -961,6 +977,8 @@ static int apply_p2m_changes(struct domain *d,
     const bool_t preempt = !is_idle_vcpu(current);
     bool_t flush = false;
     bool_t flush_pt;
+    PAGE_LIST_HEAD(free_pages);
+    struct page_info *pg;
 
     /* Some IOMMU don't support coherent PT walk. When the p2m is
      * shared with the CPU, Xen has to make sure that the PT changes have
@@ -1070,6 +1088,7 @@ static int apply_p2m_changes(struct domain *d,
         {
             unsigned offset = offsets[level];
             lpae_t *entry = &mappings[level][offset];
+            lpae_t old_entry = *entry;
 
             ret = apply_one_level(d, entry,
                                   level, flush_pt, op,
@@ -1078,6 +1097,10 @@ static int apply_p2m_changes(struct domain *d,
                                   mattr, t, a);
             if ( ret < 0 ) { rc = ret ; goto out; }
             count += ret;
+
+            if ( ret != P2M_ONE_PROGRESS_NOP )
+                update_reference_mapping(pages[level], old_entry, *entry);
+
             /* L3 had better have done something! We cannot descend any further */
             BUG_ON(level == 3 && ret == P2M_ONE_DESCEND);
             if ( ret != P2M_ONE_DESCEND ) break;
@@ -1099,6 +1122,45 @@ static int apply_p2m_changes(struct domain *d,
             }
             /* else: next level already valid */
         }
+
+        BUG_ON(level > 3);
+
+        if ( op == REMOVE )
+        {
+            for ( ; level > P2M_ROOT_LEVEL; level-- )
+            {
+                lpae_t old_entry;
+                lpae_t *entry;
+                unsigned int offset;
+
+                pg = pages[level];
+
+                /*
+                 * No need to try the previous level if the current one
+                 * still contains some mappings.
+                 */
+                if ( pg->u.inuse.p2m_refcount )
+                    break;
+
+                offset = offsets[level - 1];
+                entry = &mappings[level - 1][offset];
+                old_entry = *entry;
+
+                page_list_del(pg, &p2m->pages);
+
+                p2m_remove_pte(entry, flush_pt);
+
+                p2m->stats.mappings[level - 1]--;
+                update_reference_mapping(pages[level - 1], old_entry, *entry);
+
+                /*
+                 * We can't free the page now because it may be present
+                 * in the guest TLB. Queue it and free it after the TLB
+                 * has been flushed.
+                 */
+                page_list_add(pg, &free_pages);
+            }
+        }
     }
 
     if ( op == ALLOCATE || op == INSERT )
@@ -1116,6 +1178,9 @@ out:
         iommu_iotlb_flush(d, sgfn, egfn - sgfn);
     }
 
+    while ( (pg = page_list_remove_head(&free_pages)) )
+        free_domheap_page(pg);
+
     if ( rc < 0 && ( op == INSERT || op == ALLOCATE ) &&
          addr != start_gpaddr )
     {
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index a95082e..1427163 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -33,6 +33,12 @@ struct page_info
         struct {
             /* Type reference count and various PGT_xxx flags and fields. */
             unsigned long type_info;
+            /*
+             * Reference count for page table used in the P2M code.
+             * The counter is protected by the p2m->lock of the
+             * associated domain.
+             */
+            unsigned long p2m_refcount;
         } inuse;
         /* Page is on a free list: ((count_info & PGC_count_mask) == 0). */
         struct {
-- 
2.1.4

Re: [PATCH v3 4/4] xen/arm: p2m: Remove translation table when it's empty

[Ian Campbell]

On Tue, 2015-12-01 at 17:52 +0000, Julien Grall wrote:
> Currently, the translation table is left in place even if no entries is

"are in use" (in fact s/inuse/in use/ throughout.

> inuse. Because of how the p2m code has been implemented, replacing a
> translation table by a block (i.e superpage) is not supported. Therefore,
> any mapping of a superpage size will be split in smaller chunks making
      
      remapping ? To make it clear it doesn't apply to an initial mapping?

> the translation less efficient.
> 
> Replacing a table by a block when a new mapping is added would be too
> complicated because it requires to check if all the upper levels are not

                         requires us to check

> inuse and free them if necessary.
> 
> Instead, we will remove the empty translation table when the mapping are

                                                           mappings    are

> removed. To avoid going through all the table checking if no entry is
> inuse, a counter representing the number of entry currently inuse is
> kept per table translation and updated when an entry change state (i.e

                                                       changes

> valid <-> invalid).
> 
> As Xen allocates a page for each translation table, it's possible to
> store the counter in the struct page_info. A new field p2m_refcount as

                                                                      has

> been introduced in the inuse union for this purpose. This is fine as the
> page is only used by the P2M code and nobody touch the other field of

                                                touches

> the union type_info.
> 
> For record, type_info has not been used because it would require

  For the record,

> more work to use it properly as Xen on ARM doesn't yet have the concept
> of type.
> 
> Once Xen has finished to remove a mapping and all the reference to each

               finished removing  a mapping...          references

> translation table has been updated, the level will be lookup backward to

                    have                                 looked at backwards?

I'm not sure about that second one, maybe the "the higher levels will be
looked at in turn to check..." is better? Hrm, no that doesn't really work
with the rest of the paragraph.

    Once Xen has finished to remove a mapping and all the references to each
    translation table have been updated, then the higher levels will be
    processed and freed as needed. This will allow to propagate the number
    of references and free multiple translation table at different level in
    one go.

?

> check if we need first need to free an unused translation table at an
> higher level and then the lower levels. This will allow to propagate the
> number of reference and free multiple translation table at different
> level
> in one go.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

The code looks good, I can fixup the commit log if we can agree what to
write for the second and final comments above (hopefully the rest are just
uncontroversial grammar fixes).

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 4/4] xen/arm: p2m: Remove translation table when it's empty

[Julien Grall]

Hi Ian,

On 15/12/15 10:25, Ian Campbell wrote:
> On Tue, 2015-12-01 at 17:52 +0000, Julien Grall wrote:
>> Currently, the translation table is left in place even if no entries is
> 
> "are in use" (in fact s/inuse/in use/ throughout.
> 
>> inuse. Because of how the p2m code has been implemented, replacing a
>> translation table by a block (i.e superpage) is not supported. Therefore,
>> any mapping of a superpage size will be split in smaller chunks making
>       
>       remapping ? To make it clear it doesn't apply to an initial mapping?

remapping is good here.

> 
>> the translation less efficient.
>>
>> Replacing a table by a block when a new mapping is added would be too
>> complicated because it requires to check if all the upper levels are not
> 
>                          requires us to check
> 
>> inuse and free them if necessary.
>>
>> Instead, we will remove the empty translation table when the mapping are
> 
>                                                            mappings    are
> 
>> removed. To avoid going through all the table checking if no entry is
>> inuse, a counter representing the number of entry currently inuse is
>> kept per table translation and updated when an entry change state (i.e
> 
>                                                        changes
> 
>> valid <-> invalid).
>>
>> As Xen allocates a page for each translation table, it's possible to
>> store the counter in the struct page_info. A new field p2m_refcount as
> 
>                                                                       has
> 
>> been introduced in the inuse union for this purpose. This is fine as the
>> page is only used by the P2M code and nobody touch the other field of
> 
>                                                 touches
> 
>> the union type_info.
>>
>> For record, type_info has not been used because it would require
> 
>   For the record,
> 
>> more work to use it properly as Xen on ARM doesn't yet have the concept
>> of type.
>>
>> Once Xen has finished to remove a mapping and all the reference to each
> 
>                finished removing  a mapping...          references
> 
>> translation table has been updated, the level will be lookup backward to
> 
>                     have                                 looked at backwards?
> 
> I'm not sure about that second one, maybe the "the higher levels will be
> looked at in turn to check..." is better? Hrm, no that doesn't really work
> with the rest of the paragraph.
> 
>     Once Xen has finished to remove a mapping and all the references to each
>     translation table have been updated, then the higher levels will be
>     processed and freed as needed. This will allow to propagate the number
>     of references and free multiple translation table at different level in
>     one go.
> 
> ?

This is clearer than what I wrote. I'm happy if you replace it.

>> check if we need first need to free an unused translation table at an
>> higher level and then the lower levels. This will allow to propagate the
>> number of reference and free multiple translation table at different
>> level
>> in one go.
>>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> The code looks good, I can fixup the commit log if we can agree what to
> write for the second and final comments above (hopefully the rest are just
> uncontroversial grammar fixes).

I agree with all the comments. Thank you for fixing it.

Regards,

-- 
Julien Grall

Re: [PATCH v3 0/4] xen/arm: p2m: Add support to remove empty translation table

[Ian Campbell]

On Tue, 2015-12-01 at 17:52 +0000, Julien Grall wrote:
> 
> Julien Grall (4):
>   xen/arm: p2m: Flush for every exit paths in apply_p2m_changes
>   xen/arm: p2m: Store the page for each mapping
>   xen/arm: p2m: Introduce a helper to remove an entry in the page table
>   xen/arm: p2m: Remove translation table when it's empty

All applied with the edits to the commit message of #4 as discussed.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel