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

[PATCH 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.

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 an helper to remove an entry in the page table
  xen/arm: p2m: Remove translation table when it's empty

 xen/arch/arm/p2m.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 77 insertions(+), 5 deletions(-)

-- 
2.1.4

[PATCH 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 stall mappings for any entry
updated so far.

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

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 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

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

[Ian Campbell]

On Wed, 2015-11-18 at 15:49 +0000, Julien Grall wrote:
> Currently, the TLB is not flushed if an error occured while updating the
> stage-2 p2m. However, the TLB will contain stall mappings for any entry

"stale".

> updated so far.
> 
> To avoid a such situation, flush on every exit paths when the variable

"path"

> "flush" is set.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH 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>
---
 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

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

[Ian Campbell]

On Wed, 2015-11-18 at 15:49 +0000, Julien Grall wrote:
> 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>

[PATCH 3/4] xen/arm: p2m: Introduce an 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>
---
 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

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

[Ian Campbell]

On Wed, 2015-11-18 at 15:49 +0000, Julien Grall wrote:

Subject: "a helper"

> 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>

[PATCH 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.The field type_info has been
choosen for this purpose as the p2m owns the page and nobody should used
it.

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>
---
 xen/arch/arm/p2m.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index ae0acf0..9e3aced 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.type_info = LPAE_ENTRIES;
     }
     else
         clear_page(p);
@@ -936,6 +938,16 @@ static int apply_one_level(struct domain *d,
     BUG(); /* Should never get here */
 }
 
+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.type_info--;
+    else if ( !p2m_valid(old_entry) && !p2m_valid(new_entry) )
+        page->u.inuse.type_info++;
+}
+
 static int apply_p2m_changes(struct domain *d,
                      enum p2m_operation op,
                      paddr_t start_gpaddr,
@@ -961,6 +973,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 +1084,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 +1093,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 +1118,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.type_info )
+                    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 +1174,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 )
     {
-- 
2.1.4

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

[Ian Campbell]

On Wed, 2015-11-18 at 15:49 +0000, Julien Grall wrote:
> 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.The field type_info has been
> choosen for this purpose as the p2m owns the page and nobody should used

"chosen"

> @@ -936,6 +938,16 @@ static int apply_one_level(struct domain *d,
>      BUG(); /* Should never get here */
>  }
>  
> +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.type_info--;
> +    else if ( !p2m_valid(old_entry) && !p2m_valid(new_entry) )
> +        page->u.inuse.type_info++;
> +}

Is there some suitable locking in place for page here?

I think the argument is that this page is part of the p2m and therefore the
p2m lock is the thing which protects it, and we certainly hold that
everywhere around here.

type_info is not actually a bare integer, it has some flags at the top (see
PGT_*) which are sometimes used (e.g. share_xen_page_with_guest).

I think for consistency we should probably add a PGT_p2m and use that (and
perhaps audit some of the other PGT_* which don't all seem to be completely
obviously not-x86).

Presumably we would then want {get,put}_page_type to actually do something
and to use them.

If we don't want to do that (I'm leaning towards we should, but I might be
convinceable otherwise) then I think we should avoid the use of type_info
as a bare counter, which would imply using another member of the inuse
union (p2m_refcount or some such).

>              if ( ret != P2M_ONE_DESCEND ) break;
> @@ -1099,6 +1118,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-- )
> +            {

Something is up with the indentation here.

> +                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.type_info )
> +                    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);

This made me wonder how the existing pte clear path (which you refactored
into this function) gets away without freeing the page, are we just leaking
it onto the p2m->pages list?

p2m_put_l3_page (at the other call site) only takes care of foreign
mappings, which aren't on that list.

Maybe there is a latent bug here? And if so perhaps the fix involves making
p2m_remove_pte take care of various bits and bobs of the book keeping which
is done here?

> +
> +                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);

You could have used page_list_move instead of del+add, but I can't quite
convince myself it matters.

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

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

[Julien Grall]

Hi Ian,

On 25/11/15 12:40, Ian Campbell wrote:
> On Wed, 2015-11-18 at 15:49 +0000, Julien Grall wrote:
>> 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.The field type_info has been
>> choosen for this purpose as the p2m owns the page and nobody should used
> 
> "chosen"
> 
>> @@ -936,6 +938,16 @@ static int apply_one_level(struct domain *d,
>>      BUG(); /* Should never get here */
>>  }
>>  
>> +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.type_info--;
>> +    else if ( !p2m_valid(old_entry) && !p2m_valid(new_entry) )
>> +        page->u.inuse.type_info++;
>> +}
> 
> Is there some suitable locking in place for page here?
> 
> I think the argument is that this page is part of the p2m and therefore the
> p2m lock is the thing which protects it, and we certainly hold that
> everywhere around here.

Correct. I can add a comment in the code to explain that.

> type_info is not actually a bare integer, it has some flags at the top (see
> PGT_*) which are sometimes used (e.g. share_xen_page_with_guest).
> 
> I think for consistency we should probably add a PGT_p2m and use that (and
> perhaps audit some of the other PGT_* which don't all seem to be completely
> obviously not-x86).
> 
> Presumably we would then want {get,put}_page_type to actually do something
> and to use them.
> 
> If we don't want to do that (I'm leaning towards we should, but I might be
> convinceable otherwise) then I think we should avoid the use of type_info
> as a bare counter, which would imply using another member of the inuse
> union (p2m_refcount or some such).

I think using correctly the field type_count would require lots of faff
on ARM as we don't use it for now.

So I would go with introducing a new member of inuse union.

> 
>>              if ( ret != P2M_ONE_DESCEND ) break;
>> @@ -1099,6 +1118,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-- )
>> +            {
> 
> Something is up with the indentation here.

Hmmm will give a look.

> 
>> +                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.type_info )
>> +                    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);
> 
> This made me wonder how the existing pte clear path (which you refactored
> into this function) gets away without freeing the page, are we just leaking
> it onto the p2m->pages list?

Because the existing pte clear path is only called on the leaf of the
page tables.

The intermediate table will left linked and empty.

> p2m_put_l3_page (at the other call site) only takes care of foreign
> mappings, which aren't on that list.
> 
> Maybe there is a latent bug here? And if so perhaps the fix involves making
> p2m_remove_pte take care of various bits and bobs of the book keeping which
> is done here?
> 
>> +
>> +                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);
> 
> You could have used page_list_move instead of del+add, but I can't quite
> convince myself it matters.

Are you sure? AFAICT, page_list_move move the head of the list from one
variable to another. So all the list is moved.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Mon, 2015-11-30 at 14:26 +0000, Julien Grall wrote:
> +
> > > +                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);
> > 
> > You could have used page_list_move instead of del+add, but I can't
> > quite
> > convince myself it matters.
> 
> Are you sure?

No.

>  AFAICT, page_list_move move the head of the list from one
> variable to another. So all the list is moved.

I think you are probably right.

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