[for-4.8][PATCH v2 00/23] xen/arm: Rework the P2M code to follow break-before-make sequence

[for-4.8][PATCH v2 00/23] xen/arm: Rework the P2M code to follow break-before-make sequence

[Julien Grall]

Hello all,

The ARM architecture mandates the use of a break-before-make sequence when
changing translation entries if the page table is shared between multiple
CPUs whenever a valid entry is replaced by another valid entry (see D4.7.1
in ARM DDI 0487A.j for more details).

The current P2M code does not respect this sequence and may result to
break coherency on some processors.

Adapting the current implementation to use break-before-make sequence would
imply some code duplication and more TLBs invalidations than necessary.
For instance, if we are replacing a 4KB page and the current mapping in
the P2M is using a 1GB superpage, the following steps will happen:
    1) Shatter the 1GB superpage into a series of 2MB superpages
    2) Shatter the 2MB superpage into a series of 4KB superpages
    3) Replace the 4KB page

As the current implementation is shattering while descending and install
the mapping before continuing to the next level, Xen would need to issue 3
TLB invalidation instructions which is clearly inefficient.

Furthermore, all the operations which modify the page table are using the
same skeleton. It is more complicated to maintain different code paths than
having a generic function that set an entry and take care of the break-before-
make sequence.

The new implementation is based on the x86 EPT one which, I think, fits
quite well for the break-before-make sequence whilst keeping the code
simple.

For all the changes see in each patch.

I have provided a branch based on upstream here:
git://xenbits.xen.org/people/julieng/xen-unstable.git branch p2m-v2

Comments are welcome.

Yours sincerely,

Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Shanker Donthineni <shankerd@codeaurora.org>
Cc: Dirk Behme <dirk.behme@de.bosch.com>
Cc: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Julien Grall (23):
  xen/arm: do_trap_instr_abort_guest: Move the IPA computation out of
    the switch
  xen/arm: p2m: Store in p2m_domain whether we need to clean the entry
  xen/arm: p2m: Rename parameter in p2m_{remove,write}_pte...
  xen/arm: p2m: Use typesafe gfn in p2m_mem_access_radix_set
  xen/arm: p2m: Add a back pointer to domain in p2m_domain
  xen/arm: traps: Move MMIO emulation code in a separate helper
  xen/arm: traps: Check the P2M before injecting a data/instruction
    abort
  xen/arm: p2m: Invalidate the TLBs when write unlocking the p2m
  xen/arm: p2m: Change the type of level_shifts from paddr_t to uint8_t
  xen/arm: p2m: Move the lookup helpers at the top of the file
  xen/arm: p2m: Introduce p2m_get_root_pointer and use it in
    __p2m_lookup
  xen/arm: p2m: Introduce p2m_get_entry and use it to implement
    __p2m_lookup
  xen/arm: p2m: Replace all usage of __p2m_lookup with p2m_get_entry
  xen/arm: p2m: Re-implement p2m_cache_flush using p2m_get_entry
  xen/arm: p2m: Make p2m_{valid,table,mapping} helpers inline
  xen/arm: p2m: Introduce a helper to check if an entry is a superpage
  xen/arm: p2m: Introduce p2m_set_entry and __p2m_set_entry
  xen/arm: p2m: Re-implement relinquish_p2m_mapping using
    p2m_{get,set}_entry
  xen/arm: p2m: Re-implement p2m_remove_using using p2m_set_entry
  xen/arm: p2m: Re-implement p2m_insert_mapping using p2m_set_entry
  xen/arm: p2m: Re-implement p2m_set_mem_access using
    p2m_{set,get}_entry
  xen/arm: p2m: Do not handle shattering in p2m_create_table
  xen/arm: p2m: Export p2m_*_lock helpers

 xen/arch/arm/domain.c      |    8 +-
 xen/arch/arm/p2m.c         | 1316 ++++++++++++++++++++++----------------------
 xen/arch/arm/traps.c       |  126 +++--
 xen/include/asm-arm/p2m.h  |   63 +++
 xen/include/asm-arm/page.h |    8 +
 5 files changed, 828 insertions(+), 693 deletions(-)

-- 
1.9.1


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

[for-4.8][PATCH v2 01/23] xen/arm: do_trap_instr_abort_guest: Move the IPA computation out of the switch

[Julien Grall]

A follow-up patch will add more case to the switch that will require the
IPA. So move the computation out of the switch.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v2:
        - Add Stefano's acked-by
---
 xen/arch/arm/traps.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 39a05fd..a5a5384 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2404,35 +2404,35 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
     int rc;
     register_t gva = READ_SYSREG(FAR_EL2);
     uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK;
+    paddr_t gpa;
+
+    if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
+        gpa = get_faulting_ipa(gva);
+    else
+    {
+        /*
+         * Flush the TLB to make sure the DTLB is clear before
+         * doing GVA->IPA translation. If we got here because of
+         * an entry only present in the ITLB, this translation may
+         * still be inaccurate.
+         */
+        flush_tlb_local();
+
+        rc = gva_to_ipa(gva, &gpa, GV2M_READ);
+        if ( rc == -EFAULT )
+            return; /* Try again */
+    }
 
     switch ( fsc )
     {
     case FSC_FLT_PERM:
     {
-        paddr_t gpa;
         const struct npfec npfec = {
             .insn_fetch = 1,
             .gla_valid = 1,
             .kind = hsr.iabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
         };
 
-        if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
-            gpa = get_faulting_ipa(gva);
-        else
-        {
-            /*
-             * Flush the TLB to make sure the DTLB is clear before
-             * doing GVA->IPA translation. If we got here because of
-             * an entry only present in the ITLB, this translation may
-             * still be inaccurate.
-             */
-            flush_tlb_local();
-
-            rc = gva_to_ipa(gva, &gpa, GV2M_READ);
-            if ( rc == -EFAULT )
-                return; /* Try again */
-        }
-
         rc = p2m_mem_access_check(gpa, gva, npfec);
 
         /* Trap was triggered by mem_access, work here is done */
-- 
1.9.1


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

[for-4.8][PATCH v2 02/23] xen/arm: p2m: Store in p2m_domain whether we need to clean the entry

[Julien Grall]

Each entry in the page table has to be cleaned when the IOMMU does not
support coherent walk. Rather than querying every time the page table is
updated, it is possible to do it only once when the p2m is initialized.

This is because this value can never change, Xen would be in big trouble
otherwise.

With this change, the initialization of the IOMMU for a given domain has
to be done earlier in order to know whether the page table entries need
to be cleaned. It is fine to move the call earlier because it has no
dependency.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v2:
        - Fix typoes in the commit message
        - Add Stefano's reviewed-by
        - Use bool instead of bool_t
---
 xen/arch/arm/domain.c     |  8 +++++---
 xen/arch/arm/p2m.c        | 47 ++++++++++++++++++++++-------------------------
 xen/include/asm-arm/p2m.h |  3 +++
 3 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 20bb2ba..48f04c8 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -555,6 +555,11 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
         return 0;
 
     ASSERT(config != NULL);
+
+    /* p2m_init relies on some value initialized by the IOMMU subsystem */
+    if ( (rc = iommu_domain_init(d)) != 0 )
+        goto fail;
+
     if ( (rc = p2m_init(d)) != 0 )
         goto fail;
 
@@ -637,9 +642,6 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) )
         goto fail;
 
-    if ( (rc = iommu_domain_init(d)) != 0 )
-        goto fail;
-
     return 0;
 
 fail:
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index b648a9d..f482cfd 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -416,7 +416,7 @@ static inline void p2m_remove_pte(lpae_t *p, bool_t flush_cache)
  * level_shift is the number of bits at the level we want to create.
  */
 static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry,
-                            int level_shift, bool_t flush_cache)
+                            int level_shift)
 {
     struct page_info *page;
     lpae_t *p;
@@ -466,7 +466,7 @@ static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry,
     else
         clear_page(p);
 
-    if ( flush_cache )
+    if ( p2m->clean_pte )
         clean_dcache_va_range(p, PAGE_SIZE);
 
     unmap_domain_page(p);
@@ -478,7 +478,7 @@ static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry,
     pte = mfn_to_p2m_entry(_mfn(page_to_mfn(page)), p2m_invalid,
                            p2m->default_access);
 
-    p2m_write_pte(entry, pte, flush_cache);
+    p2m_write_pte(entry, pte, p2m->clean_pte);
 
     return 0;
 }
@@ -661,12 +661,10 @@ static const paddr_t level_shifts[] =
 
 static int p2m_shatter_page(struct p2m_domain *p2m,
                             lpae_t *entry,
-                            unsigned int level,
-                            bool_t flush_cache)
+                            unsigned int level)
 {
     const paddr_t level_shift = level_shifts[level];
-    int rc = p2m_create_table(p2m, entry,
-                              level_shift - PAGE_SHIFT, flush_cache);
+    int rc = p2m_create_table(p2m, entry, level_shift - PAGE_SHIFT);
 
     if ( !rc )
     {
@@ -688,7 +686,6 @@ static int p2m_shatter_page(struct p2m_domain *p2m,
 static int apply_one_level(struct domain *d,
                            lpae_t *entry,
                            unsigned int level,
-                           bool_t flush_cache,
                            enum p2m_operation op,
                            paddr_t start_gpaddr,
                            paddr_t end_gpaddr,
@@ -727,7 +724,7 @@ static int apply_one_level(struct domain *d,
             if ( level < 3 )
                 pte.p2m.table = 0; /* Superpage entry */
 
-            p2m_write_pte(entry, pte, flush_cache);
+            p2m_write_pte(entry, pte, p2m->clean_pte);
 
             *flush |= p2m_valid(orig_pte);
 
@@ -762,7 +759,7 @@ static int apply_one_level(struct domain *d,
             /* Not present -> create table entry and descend */
             if ( !p2m_valid(orig_pte) )
             {
-                rc = p2m_create_table(p2m, entry, 0, flush_cache);
+                rc = p2m_create_table(p2m, entry, 0);
                 if ( rc < 0 )
                     return rc;
                 return P2M_ONE_DESCEND;
@@ -772,7 +769,7 @@ static int apply_one_level(struct domain *d,
             if ( p2m_mapping(orig_pte) )
             {
                 *flush = true;
-                rc = p2m_shatter_page(p2m, entry, level, flush_cache);
+                rc = p2m_shatter_page(p2m, entry, level);
                 if ( rc < 0 )
                     return rc;
             } /* else: an existing table mapping -> descend */
@@ -809,7 +806,7 @@ static int apply_one_level(struct domain *d,
                  * and descend.
                  */
                 *flush = true;
-                rc = p2m_shatter_page(p2m, entry, level, flush_cache);
+                rc = p2m_shatter_page(p2m, entry, level);
                 if ( rc < 0 )
                     return rc;
 
@@ -835,7 +832,7 @@ static int apply_one_level(struct domain *d,
 
         *flush = true;
 
-        p2m_remove_pte(entry, flush_cache);
+        p2m_remove_pte(entry, p2m->clean_pte);
         p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), p2m_access_rwx);
 
         *addr += level_size;
@@ -894,7 +891,7 @@ static int apply_one_level(struct domain *d,
             /* Shatter large pages as we descend */
             if ( p2m_mapping(orig_pte) )
             {
-                rc = p2m_shatter_page(p2m, entry, level, flush_cache);
+                rc = p2m_shatter_page(p2m, entry, level);
                 if ( rc < 0 )
                     return rc;
             } /* else: an existing table mapping -> descend */
@@ -912,7 +909,7 @@ static int apply_one_level(struct domain *d,
                     return rc;
 
                 p2m_set_permission(&pte, pte.p2m.type, a);
-                p2m_write_pte(entry, pte, flush_cache);
+                p2m_write_pte(entry, pte, p2m->clean_pte);
             }
 
             *addr += level_size;
@@ -962,17 +959,9 @@ static int apply_p2m_changes(struct domain *d,
     const unsigned int preempt_count_limit = (op == MEMACCESS) ? 1 : 0x2000;
     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
-     * reached the memory
-     */
-    flush_pt = iommu_enabled && !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
-
     p2m_write_lock(p2m);
 
     /* Static mapping. P2M_ROOT_PAGES > 1 are handled below */
@@ -1078,7 +1067,7 @@ static int apply_p2m_changes(struct domain *d,
             lpae_t old_entry = *entry;
 
             ret = apply_one_level(d, entry,
-                                  level, flush_pt, op,
+                                  level, op,
                                   start_gpaddr, end_gpaddr,
                                   &addr, &maddr, &flush,
                                   t, a);
@@ -1135,7 +1124,7 @@ static int apply_p2m_changes(struct domain *d,
 
                 page_list_del(pg, &p2m->pages);
 
-                p2m_remove_pte(entry, flush_pt);
+                p2m_remove_pte(entry, p2m->clean_pte);
 
                 p2m->stats.mappings[level - 1]--;
                 update_reference_mapping(pages[level - 1], old_entry, *entry);
@@ -1407,6 +1396,14 @@ int p2m_init(struct domain *d)
     p2m->mem_access_enabled = false;
     radix_tree_init(&p2m->mem_access_settings);
 
+    /*
+     * Some IOMMUs don't support coherent PT walk. When the p2m is
+     * shared with the CPU, Xen has to make sure that the PT changes have
+     * reached the memory
+     */
+    p2m->clean_pte = iommu_enabled &&
+        !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
+
     rc = p2m_alloc_table(d);
 
     return rc;
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 53c4d78..b9269e4 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -48,6 +48,9 @@ struct p2m_domain {
      * decrease. */
     gfn_t lowest_mapped_gfn;
 
+    /* Indicate if it is required to clean the cache when writing an entry */
+    bool clean_pte;
+
     /* Gather some statistics for information purposes only */
     struct {
         /* Number of mappings at each p2m tree level */
-- 
1.9.1


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

[for-4.8][PATCH v2 03/23] xen/arm: p2m: Rename parameter in p2m_{remove, write}_pte...

[Julien Grall]

to make clear of the usage. I.e it is used to inform whether Xen needs
to clean the entry after writing in the page table.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>

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

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index f482cfd..5a1d45b 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -390,19 +390,19 @@ static lpae_t mfn_to_p2m_entry(mfn_t mfn, p2m_type_t t, p2m_access_t a)
     return e;
 }
 
-static inline void p2m_write_pte(lpae_t *p, lpae_t pte, bool_t flush_cache)
+static inline void p2m_write_pte(lpae_t *p, lpae_t pte, bool clean_pte)
 {
     write_pte(p, pte);
-    if ( flush_cache )
+    if ( clean_pte )
         clean_dcache(*p);
 }
 
-static inline void p2m_remove_pte(lpae_t *p, bool_t flush_cache)
+static inline void p2m_remove_pte(lpae_t *p, bool clean_pte)
 {
     lpae_t pte;
 
     memset(&pte, 0x00, sizeof(pte));
-    p2m_write_pte(p, pte, flush_cache);
+    p2m_write_pte(p, pte, clean_pte);
 }
 
 /*
-- 
1.9.1


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

[for-4.8][PATCH v2 04/23] xen/arm: p2m: Use typesafe gfn in p2m_mem_access_radix_set

[Julien Grall]

p2m_mem_access_radix_set is expecting a gfn in a parameter. Rename the
parameter 'pfn' to 'gfn' to match its content and use the typesafe gfn
to avoid possible misusage.

Also rename the parameter to gfn to match its content.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v2:
        - Add Stefano's reviewed-by
---
 xen/arch/arm/p2m.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 5a1d45b..950a607 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -550,7 +550,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
     return 0;
 }
 
-static int p2m_mem_access_radix_set(struct p2m_domain *p2m, unsigned long pfn,
+static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn,
                                     p2m_access_t a)
 {
     int rc;
@@ -560,18 +560,18 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, unsigned long pfn,
 
     if ( p2m_access_rwx == a )
     {
-        radix_tree_delete(&p2m->mem_access_settings, pfn);
+        radix_tree_delete(&p2m->mem_access_settings, gfn_x(gfn));
         return 0;
     }
 
-    rc = radix_tree_insert(&p2m->mem_access_settings, pfn,
+    rc = radix_tree_insert(&p2m->mem_access_settings, gfn_x(gfn),
                            radix_tree_int_to_ptr(a));
     if ( rc == -EEXIST )
     {
         /* If a setting already exists, change it to the new one */
         radix_tree_replace_slot(
             radix_tree_lookup_slot(
-                &p2m->mem_access_settings, pfn),
+                &p2m->mem_access_settings, gfn_x(gfn)),
             radix_tree_int_to_ptr(a));
         rc = 0;
     }
@@ -715,7 +715,7 @@ static int apply_one_level(struct domain *d,
             */
              (level == 3 || (!p2m_table(orig_pte) && !p2m->mem_access_enabled)) )
         {
-            rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a);
+            rc = p2m_mem_access_radix_set(p2m, _gfn(paddr_to_pfn(*addr)), a);
             if ( rc < 0 )
                 return rc;
 
@@ -833,7 +833,8 @@ static int apply_one_level(struct domain *d,
         *flush = true;
 
         p2m_remove_pte(entry, p2m->clean_pte);
-        p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), p2m_access_rwx);
+        p2m_mem_access_radix_set(p2m, _gfn(paddr_to_pfn(*addr)),
+                                 p2m_access_rwx);
 
         *addr += level_size;
         *maddr += level_size;
@@ -904,7 +905,8 @@ static int apply_one_level(struct domain *d,
 
             if ( p2m_valid(pte) )
             {
-                rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a);
+                rc = p2m_mem_access_radix_set(p2m, _gfn(paddr_to_pfn(*addr)),
+                                              a);
                 if ( rc < 0 )
                     return rc;
 
-- 
1.9.1


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

[for-4.8][PATCH v2 05/23] xen/arm: p2m: Add a back pointer to domain in p2m_domain

[Julien Grall]

The back pointer will be usefult later to get the domain when we only
have the p2m in hand.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/p2m.c        | 1 +
 xen/include/asm-arm/p2m.h | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 950a607..5cf136f 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1391,6 +1391,7 @@ int p2m_init(struct domain *d)
     if ( rc != 0 )
         return rc;
 
+    p2m->domain = d;
     p2m->max_mapped_gfn = _gfn(0);
     p2m->lowest_mapped_gfn = _gfn(ULONG_MAX);
 
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index b9269e4..b27a3a1 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -81,6 +81,9 @@ struct p2m_domain {
      * enough available bits to store this information.
      */
     struct radix_tree_root mem_access_settings;
+
+    /* back pointer to domain */
+    struct domain *domain;
 };
 
 /*
-- 
1.9.1


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

[for-4.8][PATCH v2 06/23] xen/arm: traps: Move MMIO emulation code in a separate helper

[Julien Grall]

Currently, a stage-2 fault translation will likely access an emulated
region. All the checks are pre-sanitity check for MMIO emulation.

A follow-up patch will handle a new case that could lead to a stage-2
translation. To improve the clarity of the code and the changes, the
current implementation is move in a separate helper.

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

---
    Changes in v2:
        - Keep the break in FSC_FLT_TRANS
        - Use bool instead of bool_t
---
 xen/arch/arm/traps.c | 57 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 24 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index a5a5384..76e4152 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2445,6 +2445,38 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
     inject_iabt_exception(regs, gva, hsr.len);
 }
 
+static bool try_handle_mmio(struct cpu_user_regs *regs,
+                            mmio_info_t *info)
+{
+    const struct hsr_dabt dabt = info->dabt;
+    int rc;
+
+    /* stage-1 page table should never live in an emulated MMIO region */
+    if ( dabt.s1ptw )
+        return false;
+
+    /* All the instructions used on emulated MMIO region should be valid */
+    if ( !dabt.valid )
+        return false;
+
+    /*
+     * Erratum 766422: Thumb store translation fault to Hypervisor may
+     * not have correct HSR Rt value.
+     */
+    if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
+         dabt.write )
+    {
+        rc = decode_instruction(regs, &info->dabt);
+        if ( rc )
+        {
+            gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
+            return false;
+        }
+    }
+
+    return !!handle_mmio(info);
+}
+
 static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
                                      const union hsr hsr)
 {
@@ -2488,29 +2520,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
         break;
     }
     case FSC_FLT_TRANS:
-        if ( dabt.s1ptw )
-            goto bad_data_abort;
-
-        /* XXX: Decode the instruction if ISS is not valid */
-        if ( !dabt.valid )
-            goto bad_data_abort;
-
-        /*
-         * Erratum 766422: Thumb store translation fault to Hypervisor may
-         * not have correct HSR Rt value.
-         */
-        if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
-             dabt.write )
-        {
-            rc = decode_instruction(regs, &info.dabt);
-            if ( rc )
-            {
-                gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
-                goto bad_data_abort;
-            }
-        }
-
-        if ( handle_mmio(&info) )
+        if ( try_handle_mmio(regs, &info) )
         {
             advance_pc(regs, hsr);
             return;
@@ -2521,7 +2531,6 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
                 hsr.bits, dabt.dfsc);
     }
 
-bad_data_abort:
     gdprintk(XENLOG_DEBUG, "HSR=0x%x pc=%#"PRIregister" gva=%#"PRIvaddr
              " gpa=%#"PRIpaddr"\n", hsr.bits, regs->pc, info.gva, info.gpa);
     inject_dabt_exception(regs, info.gva, hsr.len);
-- 
1.9.1


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

[for-4.8][PATCH v2 07/23] xen/arm: traps: Check the P2M before injecting a data/instruction abort

[Julien Grall]

A data/instruction abort may have occurred if another CPU was playing
with the stage-2 page table when following the break-before-make
sequence (see D4.7.1 in ARM DDI 0487A.j). Rather than injecting directly
the fault to the guest, we need to check whether the mapping exists. If
it exists, return to the guest to replay the instruction.

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

---
    Changes in v2:
        - Remove spurious change
        - Fix typoes
---
 xen/arch/arm/traps.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 76e4152..d73d29a 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2405,6 +2405,7 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
     register_t gva = READ_SYSREG(FAR_EL2);
     uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK;
     paddr_t gpa;
+    mfn_t mfn;
 
     if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
         gpa = get_faulting_ipa(gva);
@@ -2418,6 +2419,11 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
          */
         flush_tlb_local();
 
+        /*
+         * We may not be able to translate because someone is
+         * playing with the Stage-2 page table of the domain.
+         * Return to the guest.
+         */
         rc = gva_to_ipa(gva, &gpa, GV2M_READ);
         if ( rc == -EFAULT )
             return; /* Try again */
@@ -2438,8 +2444,17 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
         /* Trap was triggered by mem_access, work here is done */
         if ( !rc )
             return;
+        break;
     }
-    break;
+    case FSC_FLT_TRANS:
+        /*
+         * The PT walk may have failed because someone was playing
+         * with the Stage-2 page table. Walk the Stage-2 PT to check
+         * if the entry exists. If it's the case, return to the guest
+         */
+        mfn = p2m_lookup(current->domain, _gfn(paddr_to_pfn(gpa)), NULL);
+        if ( !mfn_eq(mfn, INVALID_MFN) )
+            return;
     }
 
     inject_iabt_exception(regs, gva, hsr.len);
@@ -2484,6 +2499,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
     int rc;
     mmio_info_t info;
     uint8_t fsc = hsr.dabt.dfsc & ~FSC_LL_MASK;
+    mfn_t mfn;
 
     info.dabt = dabt;
 #ifdef CONFIG_ARM_32
@@ -2497,6 +2513,11 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
     else
     {
         rc = gva_to_ipa(info.gva, &info.gpa, GV2M_READ);
+        /*
+         * We may not be able to translate because someone is
+         * playing with the Stage-2 page table of the domain.
+         * Return to the guest.
+         */
         if ( rc == -EFAULT )
             return; /* Try again */
     }
@@ -2520,11 +2541,25 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
         break;
     }
     case FSC_FLT_TRANS:
+        /*
+         * Attempt first to emulate the MMIO as the data abort will
+         * likely happen in an emulated region.
+         */
         if ( try_handle_mmio(regs, &info) )
         {
             advance_pc(regs, hsr);
             return;
         }
+
+        /*
+         * The PT walk may have failed because someone was playing
+         * with the Stage-2 page table. Walk the Stage-2 PT to check
+         * if the entry exists. If it's the case, return to the guest
+         */
+        mfn = p2m_lookup(current->domain, _gfn(paddr_to_pfn(info.gpa)), NULL);
+        if ( !mfn_eq(mfn, INVALID_MFN) )
+            return;
+
         break;
     default:
         gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n",
-- 
1.9.1


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

[for-4.8][PATCH v2 08/23] xen/arm: p2m: Invalidate the TLBs when write unlocking the p2m

[Julien Grall]

Sometimes the invalidation of the TLBs can be deferred until the p2m is
unlocked. This is for instance the case when multiple mappings are
removed. In other case, such as shattering a superpage, an immediate
flush is required.

Keep track whether a flush is needed directly in the p2m_domain structure
to allow serializing multiple changes. The TLBs will be invalidated when
write unlocking the p2m if necessary.

Also a new helper, p2m_flush_sync, has been introduced to force a
synchronous TLB invalidation.

Finally, replace the call to p2m_flush_tlb by p2m_flush_tlb_sync in
apply_p2m_changes.

Note this patch is not useful today, however follow-up patches will make
advantage of it.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v2:
        - Add Stefano's reviewed-by
---
 xen/arch/arm/p2m.c        | 33 ++++++++++++++++++++++++++++++++-
 xen/include/asm-arm/p2m.h | 11 +++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 5cf136f..b53d4cf 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -52,8 +52,21 @@ static inline void p2m_write_lock(struct p2m_domain *p2m)
     write_lock(&p2m->lock);
 }
 
+static void p2m_flush_tlb(struct p2m_domain *p2m);
+
 static inline void p2m_write_unlock(struct p2m_domain *p2m)
 {
+    if ( p2m->need_flush )
+    {
+        p2m->need_flush = false;
+        /*
+         * The final flush is done with the P2M write lock taken to
+         * to avoid someone else modify the P2M before the TLB
+         * invalidation has completed.
+         */
+        p2m_flush_tlb(p2m);
+    }
+
     write_unlock(&p2m->lock);
 }
 
@@ -72,6 +85,11 @@ static inline int p2m_is_locked(struct p2m_domain *p2m)
     return rw_is_locked(&p2m->lock);
 }
 
+static inline int p2m_is_write_locked(struct p2m_domain *p2m)
+{
+    return rw_is_write_locked(&p2m->lock);
+}
+
 void p2m_dump_info(struct domain *d)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
@@ -165,6 +183,19 @@ static void p2m_flush_tlb(struct p2m_domain *p2m)
 }
 
 /*
+ * Force a synchronous P2M TLB flush.
+ *
+ * Must be called with the p2m lock held.
+ */
+static void p2m_flush_tlb_sync(struct p2m_domain *p2m)
+{
+    ASSERT(p2m_is_write_locked(p2m));
+
+    p2m_flush_tlb(p2m);
+    p2m->need_flush = false;
+}
+
+/*
  * Lookup the MFN corresponding to a domain's GFN.
  *
  * There are no processor functions to do a stage 2 only lookup therefore we
@@ -1153,7 +1184,7 @@ static int apply_p2m_changes(struct domain *d,
 out:
     if ( flush )
     {
-        p2m_flush_tlb(&d->arch.p2m);
+        p2m_flush_tlb_sync(&d->arch.p2m);
         ret = iommu_iotlb_flush(d, gfn_x(sgfn), nr);
         if ( !rc )
             rc = ret;
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index b27a3a1..156df5e 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -51,6 +51,17 @@ struct p2m_domain {
     /* Indicate if it is required to clean the cache when writing an entry */
     bool clean_pte;
 
+    /*
+     * P2M updates may required TLBs to be flushed (invalidated).
+     *
+     * Flushes may be deferred by setting 'need_flush' and then flushing
+     * when the p2m write lock is released.
+     *
+     * If an immediate flush is required (e.g, if a super page is
+     * shattered), call p2m_tlb_flush_sync().
+     */
+    bool need_flush;
+
     /* Gather some statistics for information purposes only */
     struct {
         /* Number of mappings at each p2m tree level */
-- 
1.9.1


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

[for-4.8][PATCH v2 09/23] xen/arm: p2m: Change the type of level_shifts from paddr_t to uint8_t

[Julien Grall]

The level shift can be encoded with 8-bit. So it is not necessary to
use paddr_t (i.e 64-bit).

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

---
    Changes in v2:
        - Use uint8_t rather than unsigned int
        - Replaced paddr_t by uint8_t in p2m_shatter_page
---
 xen/arch/arm/p2m.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index b53d4cf..b4f75e8 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -687,14 +687,14 @@ static const paddr_t level_sizes[] =
     { ZEROETH_SIZE, FIRST_SIZE, SECOND_SIZE, THIRD_SIZE };
 static const paddr_t level_masks[] =
     { ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK };
-static const paddr_t level_shifts[] =
+static const uint8_t level_shifts[] =
     { ZEROETH_SHIFT, FIRST_SHIFT, SECOND_SHIFT, THIRD_SHIFT };
 
 static int p2m_shatter_page(struct p2m_domain *p2m,
                             lpae_t *entry,
                             unsigned int level)
 {
-    const paddr_t level_shift = level_shifts[level];
+    const uint8_t level_shift = level_shifts[level];
     int rc = p2m_create_table(p2m, entry, level_shift - PAGE_SHIFT);
 
     if ( !rc )
-- 
1.9.1


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

[for-4.8][PATCH v2 10/23] xen/arm: p2m: Move the lookup helpers at the top of the file

[Julien Grall]

This will be used later in functions that will be defined earlier in the
file.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/p2m.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index b4f75e8..413780b 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -29,6 +29,14 @@ static unsigned int __read_mostly p2m_root_level;
 
 unsigned int __read_mostly p2m_ipa_bits;
 
+/* Helpers to lookup the properties of each level */
+static const paddr_t level_sizes[] =
+    { ZEROETH_SIZE, FIRST_SIZE, SECOND_SIZE, THIRD_SIZE };
+static const paddr_t level_masks[] =
+    { ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK };
+static const uint8_t level_shifts[] =
+    { ZEROETH_SHIFT, FIRST_SHIFT, SECOND_SHIFT, THIRD_SHIFT };
+
 static bool_t p2m_valid(lpae_t pte)
 {
     return pte.p2m.valid;
@@ -682,14 +690,6 @@ static bool_t is_mapping_aligned(const paddr_t start_gpaddr,
 #define P2M_ONE_PROGRESS_NOP   0x1
 #define P2M_ONE_PROGRESS       0x10
 
-/* Helpers to lookup the properties of each level */
-static const paddr_t level_sizes[] =
-    { ZEROETH_SIZE, FIRST_SIZE, SECOND_SIZE, THIRD_SIZE };
-static const paddr_t level_masks[] =
-    { ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK };
-static const uint8_t level_shifts[] =
-    { ZEROETH_SHIFT, FIRST_SHIFT, SECOND_SHIFT, THIRD_SHIFT };
-
 static int p2m_shatter_page(struct p2m_domain *p2m,
                             lpae_t *entry,
                             unsigned int level)
-- 
1.9.1


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

[for-4.8][PATCH v2 11/23] xen/arm: p2m: Introduce p2m_get_root_pointer and use it in __p2m_lookup

[Julien Grall]

Mapping the root table is always done the same way. To avoid duplicating
the code in a later patch, move the code in a separate helper.

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

---
    Changes in v2:
        - Use level_orders rather than level_shifts - PAGE_SHIFT
        - Move the definition of level_orders in this patch
            * use uint8_t rather than unsigned
            * define *_ORDER in term of *_SHIFT
---
 xen/arch/arm/p2m.c         | 55 +++++++++++++++++++++++++++++++---------------
 xen/include/asm-arm/page.h |  4 ++++
 2 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 413780b..b2a29ad 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -36,6 +36,8 @@ static const paddr_t level_masks[] =
     { ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK };
 static const uint8_t level_shifts[] =
     { ZEROETH_SHIFT, FIRST_SHIFT, SECOND_SHIFT, THIRD_SHIFT };
+static const uint8_t level_orders[] =
+    { ZEROETH_ORDER, FIRST_ORDER, SECOND_ORDER, THIRD_ORDER };
 
 static bool_t p2m_valid(lpae_t pte)
 {
@@ -204,6 +206,37 @@ static void p2m_flush_tlb_sync(struct p2m_domain *p2m)
 }
 
 /*
+ * Find and map the root page table. The caller is responsible for
+ * unmapping the table.
+ *
+ * The function will return NULL if the offset of the root table is
+ * invalid.
+ */
+static lpae_t *p2m_get_root_pointer(struct p2m_domain *p2m,
+                                    gfn_t gfn)
+{
+    unsigned int root_table;
+
+    if ( P2M_ROOT_PAGES == 1 )
+        return __map_domain_page(p2m->root);
+
+    /*
+     * Concatenated root-level tables. The table number will be the
+     * offset at the previous level. It is not possible to
+     * concatenate a level-0 root.
+     */
+    ASSERT(P2M_ROOT_LEVEL > 0);
+
+    root_table = gfn_x(gfn) >> (level_orders[P2M_ROOT_LEVEL - 1]);
+    root_table &= LPAE_ENTRY_MASK;
+
+    if ( root_table >= P2M_ROOT_PAGES )
+        return NULL;
+
+    return __map_domain_page(p2m->root + root_table);
+}
+
+/*
  * Lookup the MFN corresponding to a domain's GFN.
  *
  * There are no processor functions to do a stage 2 only lookup therefore we
@@ -226,7 +259,7 @@ static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
     mfn_t mfn = INVALID_MFN;
     paddr_t mask = 0;
     p2m_type_t _t;
-    unsigned int level, root_table;
+    unsigned int level;
 
     ASSERT(p2m_is_locked(p2m));
     BUILD_BUG_ON(THIRD_MASK != PAGE_MASK);
@@ -236,22 +269,9 @@ static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
 
     *t = p2m_invalid;
 
-    if ( P2M_ROOT_PAGES > 1 )
-    {
-        /*
-         * Concatenated root-level tables. The table number will be
-         * the offset at the previous level. It is not possible to
-         * concatenate a level-0 root.
-         */
-        ASSERT(P2M_ROOT_LEVEL > 0);
-        root_table = offsets[P2M_ROOT_LEVEL - 1];
-        if ( root_table >= P2M_ROOT_PAGES )
-            goto err;
-    }
-    else
-        root_table = 0;
-
-    map = __map_domain_page(p2m->root + root_table);
+    map = p2m_get_root_pointer(p2m, gfn);
+    if ( !map )
+        return INVALID_MFN;
 
     ASSERT(P2M_ROOT_LEVEL < 4);
 
@@ -286,7 +306,6 @@ static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
         *t = pte.p2m.type;
     }
 
-err:
     return mfn;
 }
 
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 05d9f82..a43b0fa 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -457,15 +457,19 @@ static inline int gva_to_ipa(vaddr_t va, paddr_t *paddr, unsigned int flags)
 #define LPAE_ENTRY_MASK (LPAE_ENTRIES - 1)
 
 #define THIRD_SHIFT    (PAGE_SHIFT)
+#define THIRD_ORDER    (THIRD_SHIFT - PAGE_SHIFT)
 #define THIRD_SIZE     ((paddr_t)1 << THIRD_SHIFT)
 #define THIRD_MASK     (~(THIRD_SIZE - 1))
 #define SECOND_SHIFT   (THIRD_SHIFT + LPAE_SHIFT)
+#define SECOND_ORDER   (SECOND_SHIFT - PAGE_SHIFT)
 #define SECOND_SIZE    ((paddr_t)1 << SECOND_SHIFT)
 #define SECOND_MASK    (~(SECOND_SIZE - 1))
 #define FIRST_SHIFT    (SECOND_SHIFT + LPAE_SHIFT)
+#define FIRST_ORDER    (FIRST_SHIFT - PAGE_SHIFT)
 #define FIRST_SIZE     ((paddr_t)1 << FIRST_SHIFT)
 #define FIRST_MASK     (~(FIRST_SIZE - 1))
 #define ZEROETH_SHIFT  (FIRST_SHIFT + LPAE_SHIFT)
+#define ZEROETH_ORDER  (ZEROETH_SHIFT - PAGE_SHIFT)
 #define ZEROETH_SIZE   ((paddr_t)1 << ZEROETH_SHIFT)
 #define ZEROETH_MASK   (~(ZEROETH_SIZE - 1))
 
-- 
1.9.1


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

[for-4.8][PATCH v2 12/23] xen/arm: p2m: Introduce p2m_get_entry and use it to implement __p2m_lookup

[Julien Grall]

Currently, for a given GFN, the function __p2m_lookup will only return
the associated MFN and the p2m type of the mapping.

In some case we need the order of the mapping and the memaccess
permission. Rather than providing a separate function for this purpose,
it is better to implement a generic function to return all the
information.

To avoid passing dummy parameter, a caller that does not need a
specific information can use NULL instead.

The list of the informations retrieved is based on the x86 version. All
of them will be used in follow-up patches.

It might have been possible to extend __p2m_lookup, however I choose to
reimplement it from scratch to allow sharing some helpers with the
function that will update the P2M (will be added in a follow-up patch).

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

---
    Changes in v2:
        - Export p2m_get_entry
        - Fix the computation of the order when there is no mapping
        - Use level_orders rather than level_shifts - PAGE_SHIFT
        - Update documentation
        - Fix typoes
        - The definition of level_orders has been moved in an earlier
        patch
---
 xen/arch/arm/p2m.c        | 188 +++++++++++++++++++++++++++++++++++-----------
 xen/include/asm-arm/p2m.h |   8 ++
 2 files changed, 154 insertions(+), 42 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index b2a29ad..6e56b97 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -238,28 +238,104 @@ static lpae_t *p2m_get_root_pointer(struct p2m_domain *p2m,
 
 /*
  * Lookup the MFN corresponding to a domain's GFN.
+ * Lookup mem access in the ratrix tree.
+ * The entries associated to the GFN is considered valid.
+ */
+static p2m_access_t p2m_mem_access_radix_get(struct p2m_domain *p2m, gfn_t gfn)
+{
+    void *ptr;
+
+    if ( !p2m->mem_access_enabled )
+        return p2m->default_access;
+
+    ptr = radix_tree_lookup(&p2m->mem_access_settings, gfn_x(gfn));
+    if ( !ptr )
+        return p2m_access_rwx;
+    else
+        return radix_tree_ptr_to_int(ptr);
+}
+
+#define GUEST_TABLE_MAP_FAILED 0
+#define GUEST_TABLE_SUPER_PAGE 1
+#define GUEST_TABLE_NORMAL_PAGE 2
+
+static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry,
+                            int level_shift);
+
+/*
+ * Take the currently mapped table, find the corresponding GFN entry,
+ * and map the next table, if available. The previous table will be
+ * unmapped if the next level was mapped (e.g GUEST_TABLE_NORMAL_PAGE
+ * returned).
  *
- * There are no processor functions to do a stage 2 only lookup therefore we
- * do a a software walk.
+ * The read_only parameters indicates whether intermediate tables should
+ * be allocated when not present.
+ *
+ * Return values:
+ *  GUEST_TABLE_MAP_FAILED: Either read_only was set and the entry
+ *  was empty, or allocating a new page failed.
+ *  GUEST_TABLE_NORMAL_PAGE: next level mapped normally
+ *  GUEST_TABLE_SUPER_PAGE: The next entry points to a superpage.
  */
-static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
+static int p2m_next_level(struct p2m_domain *p2m, bool read_only,
+                          lpae_t **table, unsigned int offset)
 {
-    struct p2m_domain *p2m = &d->arch.p2m;
-    const paddr_t paddr = pfn_to_paddr(gfn_x(gfn));
-    const unsigned int offsets[4] = {
-        zeroeth_table_offset(paddr),
-        first_table_offset(paddr),
-        second_table_offset(paddr),
-        third_table_offset(paddr)
-    };
-    const paddr_t masks[4] = {
-        ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK
-    };
-    lpae_t pte, *map;
+    lpae_t *entry;
+    int ret;
+    mfn_t mfn;
+
+    entry = *table + offset;
+
+    if ( !p2m_valid(*entry) )
+    {
+        if ( read_only )
+            return GUEST_TABLE_MAP_FAILED;
+
+        ret = p2m_create_table(p2m, entry, /* not used */ ~0);
+        if ( ret )
+            return GUEST_TABLE_MAP_FAILED;
+    }
+
+    /* The function p2m_next_level is never called at the 3rd level */
+    if ( p2m_mapping(*entry) )
+        return GUEST_TABLE_SUPER_PAGE;
+
+    mfn = _mfn(entry->p2m.base);
+
+    unmap_domain_page(*table);
+    *table = map_domain_page(mfn);
+
+    return GUEST_TABLE_NORMAL_PAGE;
+}
+
+/*
+ * Get the details of a given gfn.
+ *
+ * If the entry is present, the associated MFN will be returned and the
+ * access and type filled up. The page_order will correspond to the
+ * order of the mapping in the page table (i.e it could be a superpage).
+ *
+ * If the entry is not present, INVALID_MFN will be returned and the
+ * page_order will be set according to the order of the invalid range.
+ */
+mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
+                    p2m_type_t *t, p2m_access_t *a,
+                    unsigned int *page_order)
+{
+    paddr_t addr = pfn_to_paddr(gfn_x(gfn));
+    unsigned int level = 0;
+    lpae_t entry, *table;
+    int rc;
     mfn_t mfn = INVALID_MFN;
-    paddr_t mask = 0;
     p2m_type_t _t;
-    unsigned int level;
+
+    /* Convenience aliases */
+    const unsigned int offsets[4] = {
+        zeroeth_table_offset(addr),
+        first_table_offset(addr),
+        second_table_offset(addr),
+        third_table_offset(addr)
+    };
 
     ASSERT(p2m_is_locked(p2m));
     BUILD_BUG_ON(THIRD_MASK != PAGE_MASK);
@@ -269,46 +345,74 @@ static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
 
     *t = p2m_invalid;
 
-    map = p2m_get_root_pointer(p2m, gfn);
-    if ( !map )
-        return INVALID_MFN;
-
-    ASSERT(P2M_ROOT_LEVEL < 4);
+    /* XXX: Check if the mapping is lower than the mapped gfn */
 
-    for ( level = P2M_ROOT_LEVEL ; level < 4 ; level++ )
+    /* This gfn is higher than the highest the p2m map currently holds */
+    if ( gfn_x(gfn) > gfn_x(p2m->max_mapped_gfn) )
     {
-        mask = masks[level];
+        for ( level = P2M_ROOT_LEVEL; level < 3; level++ )
+            if ( (gfn_x(gfn) & (level_masks[level] >> PAGE_SHIFT)) >
+                 gfn_x(p2m->max_mapped_gfn) )
+                break;
 
-        pte = map[offsets[level]];
+        goto out;
+    }
 
-        if ( level == 3 && !p2m_table(pte) )
-            /* Invalid, clobber the pte */
-            pte.bits = 0;
-        if ( level == 3 || !p2m_table(pte) )
-            /* Done */
-            break;
+    table = p2m_get_root_pointer(p2m, gfn);
 
-        ASSERT(level < 3);
+    /*
+     * the table should always be non-NULL because the gfn is below
+     * p2m->max_mapped_gfn and the root table pages are always present.
+     */
+    BUG_ON(table == NULL);
 
-        /* Map for next level */
-        unmap_domain_page(map);
-        map = map_domain_page(_mfn(pte.p2m.base));
+    for ( level = P2M_ROOT_LEVEL; level < 3; level++ )
+    {
+        rc = p2m_next_level(p2m, true, &table, offsets[level]);
+        if ( rc == GUEST_TABLE_MAP_FAILED )
+            goto out_unmap;
+        else if ( rc != GUEST_TABLE_NORMAL_PAGE )
+            break;
     }
 
-    unmap_domain_page(map);
+    entry = table[offsets[level]];
 
-    if ( p2m_valid(pte) )
+    if ( p2m_valid(entry) )
     {
-        ASSERT(mask);
-        ASSERT(pte.p2m.type != p2m_invalid);
-        mfn = _mfn(paddr_to_pfn((pte.bits & PADDR_MASK & mask) |
-                                (paddr & ~mask)));
-        *t = pte.p2m.type;
+        *t = entry.p2m.type;
+
+        if ( a )
+            *a = p2m_mem_access_radix_get(p2m, gfn);
+
+        mfn = _mfn(entry.p2m.base);
+        /*
+         * The entry may point to a superpage. Find the MFN associated
+         * to the GFN.
+         */
+        mfn = mfn_add(mfn, gfn_x(gfn) & ((1UL << level_orders[level]) - 1));
     }
 
+out_unmap:
+    unmap_domain_page(table);
+
+out:
+    if ( page_order )
+        *page_order = level_orders[level];
+
     return mfn;
 }
 
+/*
+ * Lookup the MFN corresponding to a domain's GFN.
+ *
+ * There are no processor functions to do a stage 2 only lookup therefore we
+ * do a a software walk.
+ */
+static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
+{
+    return p2m_get_entry(&d->arch.p2m, gfn, t, NULL, NULL);
+}
+
 mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
 {
     mfn_t ret;
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 156df5e..6fe6a37 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -179,6 +179,14 @@ void p2m_dump_info(struct domain *d);
 /* Look up the MFN corresponding to a domain's GFN. */
 mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
 
+/*
+ * Get details of a given gfn.
+ * The P2M lock should be taken by the caller.
+ */
+mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
+                    p2m_type_t *t, p2m_access_t *a,
+                    unsigned int *page_order);
+
 /* Clean & invalidate caches corresponding to a region of guest address space */
 int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr);
 
-- 
1.9.1


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

[for-4.8][PATCH v2 13/23] xen/arm: p2m: Replace all usage of __p2m_lookup with p2m_get_entry

[Julien Grall]

__p2m_lookup is just a wrapper to p2m_get_entry.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>

---
    It might be possible to rework the memaccess code to take advantage
    of all the parameters. I will defer this to the memaccess folks.

    Changes in v2:
        - Add Stefano's acked-by
---
 xen/arch/arm/p2m.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 6e56b97..ddee258 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -402,24 +402,13 @@ out:
     return mfn;
 }
 
-/*
- * Lookup the MFN corresponding to a domain's GFN.
- *
- * There are no processor functions to do a stage 2 only lookup therefore we
- * do a a software walk.
- */
-static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
-{
-    return p2m_get_entry(&d->arch.p2m, gfn, t, NULL, NULL);
-}
-
 mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
 {
     mfn_t ret;
     struct p2m_domain *p2m = &d->arch.p2m;
 
     p2m_read_lock(p2m);
-    ret = __p2m_lookup(d, gfn, t);
+    ret = p2m_get_entry(p2m, gfn, t, NULL, NULL);
     p2m_read_unlock(p2m);
 
     return ret;
@@ -691,7 +680,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
          * No setting was found in the Radix tree. Check if the
          * entry exists in the page-tables.
          */
-        mfn_t mfn = __p2m_lookup(d, gfn, NULL);
+        mfn_t mfn = p2m_get_entry(p2m, gfn, NULL, NULL, NULL);
 
         if ( mfn_eq(mfn, INVALID_MFN) )
             return -ESRCH;
@@ -1611,6 +1600,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag)
     xenmem_access_t xma;
     p2m_type_t t;
     struct page_info *page = NULL;
+    struct p2m_domain *p2m = &current->domain->arch.p2m;
 
     rc = gva_to_ipa(gva, &ipa, flag);
     if ( rc < 0 )
@@ -1671,7 +1661,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag)
      * We had a mem_access permission limiting the access, but the page type
      * could also be limiting, so we need to check that as well.
      */
-    mfn = __p2m_lookup(current->domain, gfn, &t);
+    mfn = p2m_get_entry(p2m, gfn, &t, NULL, NULL);
     if ( mfn_eq(mfn, INVALID_MFN) )
         goto err;
 
-- 
1.9.1


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

[for-4.8][PATCH v2 14/23] xen/arm: p2m: Re-implement p2m_cache_flush using p2m_get_entry

[Julien Grall]

The function p2m_cache_flush can be re-implemented using the generic
function p2m_get_entry by iterating over the range and using the mapping
order given by the callee.

As the current implementation, no preemption is implemented, although
the comment in the current code claimed it. As the function is called by
a DOMCTL with a region of 1GB maximum, I think the preemption can be
left unimplemented for now.

Finally drop the operation CACHEFLUSH in apply_one_level as nobody is
using it anymore. Note that the function could have been dropped in one
go at the end, however I find easier to drop the operations one by one
avoiding a big deletion in the patch that convert the last operation.

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

---
    The loop pattern will be very similar for the reliquish function.
    It might be possible to extract it in a separate function.

    Changes in v2:
        - Introduce and use gfn_next_boundary
        - Flush all the mapping in a superpage rather than page by page.
        - Update doc
---
 xen/arch/arm/p2m.c | 83 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 50 insertions(+), 33 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index ddee258..fa58f1a 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -62,6 +62,22 @@ static inline void p2m_write_lock(struct p2m_domain *p2m)
     write_lock(&p2m->lock);
 }
 
+/*
+ * Return the start of the next mapping based on the order of the
+ * current one.
+ */
+static inline gfn_t gfn_next_boundary(gfn_t gfn, unsigned int order)
+{
+    /*
+     * The order corresponds to the order of the mapping (or invalid
+     * range) in the page table. So we need to align the GFN before
+     * incrementing.
+     */
+    gfn = _gfn(gfn_x(gfn) & ~((1UL << order) - 1));
+
+    return gfn_add(gfn, 1UL << order);
+}
+
 static void p2m_flush_tlb(struct p2m_domain *p2m);
 
 static inline void p2m_write_unlock(struct p2m_domain *p2m)
@@ -734,7 +750,6 @@ enum p2m_operation {
     INSERT,
     REMOVE,
     RELINQUISH,
-    CACHEFLUSH,
     MEMACCESS,
 };
 
@@ -993,36 +1008,6 @@ static int apply_one_level(struct domain *d,
          */
         return P2M_ONE_PROGRESS;
 
-    case CACHEFLUSH:
-        if ( !p2m_valid(orig_pte) )
-        {
-            *addr = (*addr + level_size) & level_mask;
-            return P2M_ONE_PROGRESS_NOP;
-        }
-
-        if ( level < 3 && p2m_table(orig_pte) )
-            return P2M_ONE_DESCEND;
-
-        /*
-         * could flush up to the next superpage boundary, but would
-         * need to be careful about preemption, so just do one 4K page
-         * now and return P2M_ONE_PROGRESS{,_NOP} so that the caller will
-         * continue to loop over the rest of the range.
-         */
-        if ( p2m_is_ram(orig_pte.p2m.type) )
-        {
-            unsigned long offset = paddr_to_pfn(*addr & ~level_mask);
-            flush_page_to_ram(orig_pte.p2m.base + offset);
-
-            *addr += PAGE_SIZE;
-            return P2M_ONE_PROGRESS;
-        }
-        else
-        {
-            *addr += PAGE_SIZE;
-            return P2M_ONE_PROGRESS_NOP;
-        }
-
     case MEMACCESS:
         if ( level < 3 )
         {
@@ -1571,12 +1556,44 @@ int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
     gfn_t end = gfn_add(start, nr);
+    gfn_t next_gfn;
+    p2m_type_t t;
+    unsigned int order;
 
     start = gfn_max(start, p2m->lowest_mapped_gfn);
     end = gfn_min(end, p2m->max_mapped_gfn);
 
-    return apply_p2m_changes(d, CACHEFLUSH, start, nr, INVALID_MFN,
-                             0, p2m_invalid, d->arch.p2m.default_access);
+    /*
+     * The operation cache flush will invalidate the RAM assigned to the
+     * guest in a given range. It will not modify the page table and
+     * flushing the cache whilst the page is used by another CPU is
+     * fine. So using read-lock is fine here.
+     */
+    p2m_read_lock(p2m);
+
+    for ( ; gfn_x(start) < gfn_x(end); start = next_gfn )
+    {
+        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order);
+
+        next_gfn = gfn_next_boundary(start, order);
+
+        /* Skip hole and non-RAM page */
+        if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_ram(t) )
+            continue;
+
+        /* XXX: Implement preemption */
+        while ( gfn_x(start) < gfn_x(next_gfn) )
+        {
+            flush_page_to_ram(mfn_x(mfn));
+
+            start = gfn_add(start, 1);
+            mfn = mfn_add(mfn, 1);
+        }
+    }
+
+    p2m_read_unlock(p2m);
+
+    return 0;
 }
 
 mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn)
-- 
1.9.1


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

[for-4.8][PATCH v2 15/23] xen/arm: p2m: Make p2m_{valid, table, mapping} helpers inline

[Julien Grall]

Those helpers are very small and often used. Let know the compiler they
can be inlined.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

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

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index fa58f1a..9f0f896 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -39,7 +39,7 @@ static const uint8_t level_shifts[] =
 static const uint8_t level_orders[] =
     { ZEROETH_ORDER, FIRST_ORDER, SECOND_ORDER, THIRD_ORDER };
 
-static bool_t p2m_valid(lpae_t pte)
+static inline bool_t p2m_valid(lpae_t pte)
 {
     return pte.p2m.valid;
 }
@@ -48,11 +48,11 @@ static bool_t p2m_valid(lpae_t pte)
  * the table bit and therefore these would return the opposite to what
  * you would expect.
  */
-static bool_t p2m_table(lpae_t pte)
+static inline bool_t p2m_table(lpae_t pte)
 {
     return p2m_valid(pte) && pte.p2m.table;
 }
-static bool_t p2m_mapping(lpae_t pte)
+static inline bool_t p2m_mapping(lpae_t pte)
 {
     return p2m_valid(pte) && !pte.p2m.table;
 }
-- 
1.9.1


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

[for-4.8][PATCH v2 16/23] xen/arm: p2m: Introduce a helper to check if an entry is a superpage

[Julien Grall]

Use the level and the entry to know whether an entry is a superpage.
A superpage can only happen below level 3.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v2:
        - Use bool instead of bool_t
        - Add Stefano's reviewed-by
---
 xen/arch/arm/p2m.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 9f0f896..3ca2e90 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -57,6 +57,11 @@ static inline bool_t p2m_mapping(lpae_t pte)
     return p2m_valid(pte) && !pte.p2m.table;
 }
 
+static inline bool p2m_is_superpage(lpae_t pte, unsigned int level)
+{
+    return (level < 3) && p2m_mapping(pte);
+}
+
 static inline void p2m_write_lock(struct p2m_domain *p2m)
 {
     write_lock(&p2m->lock);
-- 
1.9.1


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

[for-4.8][PATCH v2 17/23] xen/arm: p2m: Introduce p2m_set_entry and __p2m_set_entry

[Julien Grall]

The ARM architecture mandates to use of a break-before-make sequence
when changing translation entries if the page table is shared between
multiple CPUs whenever a valid entry is replaced by another valid entry
(see D4.7.1 in ARM DDI 0487A.j for more details).

The break-before-make sequence can be divided in the following steps:
    1) Invalidate the old entry in the page table
    2) Issue a TLB invalidation instruction for the address associated
    to this entry
    3) Write the new entry

The current P2M code implemented in apply_one_level does not respect
this sequence and may result to break coherency on some processors.

Adapting the current implementation to use the break-before-make
sequence would imply some code duplication and more TLBs invalidation
than necessary. For instance, if we are replacing a 4KB page and the
current mapping in the P2M is using a 1GB superpage, the following steps
will happen:
    1) Shatter the 1GB superpage into a series of 2MB superpages
    2) Shatter the 2MB superpage into a series of 4KB pages
    3) Replace the 4KB page

As the current implementation is shattering while descending and install
the mapping, Xen would need to issue 3 TLB invalidation instructions
which is clearly inefficient.

Furthermore, all the operations which modify the page table are using
the same skeleton. It is more complicated to maintain different code paths
than having a generic function that set an entry and take care of the
break-before-make sequence.

The new implementation is based on the x86 EPT one which, I think,
fits quite well for the break-before-make sequence whilst keeping
the code simple.

The main function of the new implementation is __p2m_set_entry. It will
only work on mapping that are aligned to a block entry in the page table
(i.e 1GB, 2MB, 4KB when using a 4KB granularity).

Another function, p2m_set_entry, is provided to break down is region
into mapping that is aligned to a block entry or 4KB when memaccess is
enabled.

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

---
    Changes in v2:
        - fix typoes in the commit message
        - don't use the default access when shattering the superpage
        - remove duplicated comment
        - export p2m_set_entry
        - s/todo/nr/
        - iommu flush
        - update the stats when removing/adding a mapping
        - update doc
        - fix p2m_split_superpage
        - don't try to allocate intermediate page table when removing an
        entry
        - the TLB flush is not necessary when only permission are
        changed (e.g memaccess).
---
 xen/arch/arm/p2m.c         | 374 +++++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/p2m.h  |  11 ++
 xen/include/asm-arm/page.h |   4 +
 3 files changed, 389 insertions(+)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 3ca2e90..5f7aef0 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -783,6 +783,380 @@ static void p2m_put_l3_page(const lpae_t pte)
     }
 }
 
+/* Free lpae sub-tree behind an entry */
+static void p2m_free_entry(struct p2m_domain *p2m,
+                           lpae_t entry, unsigned int level)
+{
+    unsigned int i;
+    lpae_t *table;
+    mfn_t mfn;
+
+    /* Nothing to do if the entry is invalid. */
+    if ( !p2m_valid(entry) )
+        return;
+
+    /* Nothing to do but updating the states if the entry is a super-page. */
+    if ( p2m_is_superpage(entry, level) )
+    {
+        p2m->stats.mappings[level]--;
+        return;
+    }
+
+    if ( level == 3 )
+    {
+        p2m->stats.mappings[level]--;
+        p2m_put_l3_page(entry);
+        return;
+    }
+
+    table = map_domain_page(_mfn(entry.p2m.base));
+    for ( i = 0; i < LPAE_ENTRIES; i++ )
+        p2m_free_entry(p2m, *(table + i), level + 1);
+
+    unmap_domain_page(table);
+
+    /*
+     * Make sure all the references in the TLB have been removed before
+     * freing the intermediate page table.
+     * XXX: Should we defer the free of the page table to avoid the
+     * flush?
+     */
+    if ( p2m->need_flush )
+        p2m_flush_tlb_sync(p2m);
+
+    mfn = _mfn(entry.p2m.base);
+    ASSERT(mfn_valid(mfn_x(mfn)));
+
+    free_domheap_page(mfn_to_page(mfn_x(mfn)));
+}
+
+static bool p2m_split_superpage(struct p2m_domain *p2m, lpae_t *entry,
+                                unsigned int level, unsigned int target,
+                                const unsigned int *offsets)
+{
+    struct page_info *page;
+    unsigned int i;
+    lpae_t pte, *table;
+    bool rv = true;
+
+    /* Convenience aliases */
+    mfn_t mfn = _mfn(entry->p2m.base);
+    unsigned int next_level = level + 1;
+    unsigned int level_order = level_orders[next_level];
+
+    /*
+     * This should only be called with target != level and the entry is
+     * a superpage.
+     */
+    ASSERT(level < target);
+    ASSERT(p2m_is_superpage(*entry, level));
+
+    page = alloc_domheap_page(NULL, 0);
+    if ( !page )
+        return false;
+
+    page_list_add(page, &p2m->pages);
+    table = __map_domain_page(page);
+
+    /*
+     * We are either splitting a first level 1G page into 512 second level
+     * 2M pages, or a second level 2M page into 512 third level 4K pages.
+     */
+    for ( i = 0; i < LPAE_ENTRIES; i++ )
+    {
+        lpae_t *new_entry = table + i;
+
+        /*
+         * Use the content of the superpage entry and override
+         * the necessary fields. So the correct permission are kept.
+         */
+        pte = *entry;
+        pte.p2m.base = mfn_x(mfn_add(mfn, i << level_order));
+
+        /*
+         * First and second level pages set p2m.table = 0, but third
+         * level entries set p2m.table = 1.
+         */
+        pte.p2m.table = (next_level == 3);
+
+        write_pte(new_entry, pte);
+    }
+
+    /* Update stats */
+    p2m->stats.shattered[level]++;
+    p2m->stats.mappings[level]--;
+    p2m->stats.mappings[next_level] += LPAE_ENTRIES;
+
+    /*
+     * Shatter superpage in the page to the level we want to make the
+     * changes.
+     * This is done outside the loop to avoid checking the offset to
+     * know whether the entry should be shattered for every entry.
+     */
+    if ( next_level != target )
+        rv = p2m_split_superpage(p2m, table + offsets[next_level],
+                                 level + 1, target, offsets);
+
+    if ( p2m->clean_pte )
+        clean_dcache_va_range(table, PAGE_SIZE);
+
+    unmap_domain_page(table);
+
+    pte = mfn_to_p2m_entry(_mfn(page_to_mfn(page)), p2m_invalid,
+                           p2m->default_access);
+
+    /*
+     * Even if we failed, we should install the newly allocated LPAE
+     * entry. The caller will be in charge to free the sub-tree.
+     */
+    p2m_write_pte(entry, pte, p2m->clean_pte);
+
+    return rv;
+}
+
+/*
+ * Insert an entry in the p2m. This should be called with a mapping
+ * equal to a page/superpage (4K, 2M, 1G).
+ */
+static int __p2m_set_entry(struct p2m_domain *p2m,
+                           gfn_t sgfn,
+                           unsigned int page_order,
+                           mfn_t smfn,
+                           p2m_type_t t,
+                           p2m_access_t a)
+{
+    paddr_t addr = pfn_to_paddr(gfn_x(sgfn));
+    unsigned int level = 0;
+    unsigned int target = 3 - (page_order / LPAE_SHIFT);
+    lpae_t *entry, *table, orig_pte;
+    int rc;
+
+    /* Convenience aliases */
+    const unsigned int offsets[4] = {
+        zeroeth_table_offset(addr),
+        first_table_offset(addr),
+        second_table_offset(addr),
+        third_table_offset(addr)
+    };
+
+    ASSERT(p2m_is_write_locked(p2m));
+
+    /*
+     * Check if the level target is valid: we only support
+     * 4K - 2M - 1G mapping.
+     */
+    ASSERT(target > 0 && target <= 3);
+
+    table = p2m_get_root_pointer(p2m, sgfn);
+    if ( !table )
+        return -EINVAL;
+
+    for ( level = P2M_ROOT_LEVEL; level < target; level++ )
+    {
+        /*
+         * Don't try to allocate intermediate page table if the mapping
+         * is about to be removed (i.e mfn == INVALID_MFN).
+         */
+        rc = p2m_next_level(p2m, mfn_eq(smfn, INVALID_MFN),
+                            &table, offsets[level]);
+        if ( rc == GUEST_TABLE_MAP_FAILED )
+        {
+            /*
+             * We are here because p2m_next_level has failed to map
+             * the intermediate page table (e.g the table does not exist
+             * and they p2m tree is read-only). It is a valid case
+             * when removing a mapping as it may not exist in the
+             * page table. In this case, just ignore it.
+             */
+            rc = mfn_eq(smfn, INVALID_MFN) ? 0 : -ENOENT;
+            goto out;
+        }
+        else if ( rc != GUEST_TABLE_NORMAL_PAGE )
+            break;
+    }
+
+    entry = table + offsets[level];
+
+    /*
+     * If we are here with level < target, we must be at a leaf node,
+     * and we need to break up the superpage.
+     */
+    if ( level < target )
+    {
+        /* We need to split the original page. */
+        lpae_t split_pte = *entry;
+
+        ASSERT(p2m_is_superpage(*entry, level));
+
+        if ( !p2m_split_superpage(p2m, &split_pte, level, target, offsets) )
+        {
+            /*
+             * The current super-page is still in-place, so re-increment
+             * the stats.
+             */
+            p2m->stats.mappings[level]++;
+
+            /* Free the allocated sub-tree */
+            p2m_free_entry(p2m, split_pte, level);
+
+            rc = -ENOMEM;
+            goto out;
+        }
+
+        /*
+         * Follow the break-before-sequence to update the entry.
+         * For more details see (D4.7.1 in ARM DDI 0487A.j).
+         */
+        p2m_remove_pte(entry, p2m->clean_pte);
+        p2m_flush_tlb_sync(p2m);
+
+        p2m_write_pte(entry, split_pte, p2m->clean_pte);
+
+        /* then move to the level we want to make real changes */
+        for ( ; level < target; level++ )
+        {
+            rc = p2m_next_level(p2m, true, &table, offsets[level]);
+
+            /*
+             * The entry should be found and either be a table
+             * or a superpage if level 3 is not targeted
+             */
+            ASSERT(rc == GUEST_TABLE_NORMAL_PAGE ||
+                   (rc == GUEST_TABLE_SUPER_PAGE && target < 3));
+        }
+
+        entry = table + offsets[level];
+    }
+
+    /*
+     * We should always be there with the correct level because
+     * all the intermediate tables have been installed if necessary.
+     */
+    ASSERT(level == target);
+
+    orig_pte = *entry;
+
+    /*
+     * The radix-tree can only work on 4KB. This is only used when
+     * memaccess is enabled.
+     */
+    ASSERT(!p2m->mem_access_enabled || page_order == 0);
+    /*
+     * The access type should always be p2m_access_rwx when the mapping
+     * is removed.
+     */
+    ASSERT(!mfn_eq(INVALID_MFN, smfn) || (a == p2m_access_rwx));
+    /*
+     * Update the mem access permission before update the P2M. So we
+     * don't have to revert the mapping if it has failed.
+     */
+    rc = p2m_mem_access_radix_set(p2m, sgfn, a);
+    if ( rc )
+        goto out;
+
+    /*
+     * Always remove the entry in order to follow the break-before-make
+     * sequence when updating the translation table (D4.7.1 in ARM DDI
+     * 0487A.j).
+     */
+    if ( p2m_valid(orig_pte) )
+        p2m_remove_pte(entry, p2m->clean_pte);
+
+    if ( mfn_eq(smfn, INVALID_MFN) )
+        /* Flush can be deferred if the entry is removed */
+        p2m->need_flush |= !!p2m_valid(orig_pte);
+    else
+    {
+        lpae_t pte = mfn_to_p2m_entry(smfn, t, a);
+
+        if ( level < 3 )
+            pte.p2m.table = 0; /* Superpage entry */
+
+        /*
+         * It is necessary to flush the TLB before writing the new entry
+         * to keep coherency when the previous entry was valid.
+         *
+         * Although, it could be defered when only the permissions are
+         * changed (e.g in case of memaccess).
+         */
+        if ( p2m_valid(orig_pte) )
+        {
+            if ( likely(!p2m->mem_access_enabled) ||
+                 P2M_CLEAR_PERM(pte) != P2M_CLEAR_PERM(orig_pte) )
+                p2m_flush_tlb_sync(p2m);
+            else
+                p2m->need_flush = true;
+        }
+        else /* new mapping */
+            p2m->stats.mappings[level]++;
+
+        p2m_write_pte(entry, pte, p2m->clean_pte);
+
+        p2m->max_mapped_gfn = gfn_max(p2m->max_mapped_gfn,
+                                      gfn_add(sgfn, 1 << page_order));
+        p2m->lowest_mapped_gfn = gfn_min(p2m->lowest_mapped_gfn, sgfn);
+    }
+
+    /*
+     * Free the entry only if the original pte was valid and the base
+     * is different (to avoid freeing when permission is changed).
+     */
+    if ( p2m_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base )
+        p2m_free_entry(p2m, orig_pte, level);
+
+    if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) || p2m_valid(*entry)) )
+        rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << page_order);
+    else
+        rc = 0;
+
+out:
+    unmap_domain_page(table);
+
+    return rc;
+}
+
+int p2m_set_entry(struct p2m_domain *p2m,
+                  gfn_t sgfn,
+                  unsigned long nr,
+                  mfn_t smfn,
+                  p2m_type_t t,
+                  p2m_access_t a)
+{
+    int rc = 0;
+
+    while ( nr )
+    {
+        /*
+         * XXX: Support superpage mappings if nr is not aligned to a
+         * superpage size.
+         */
+        unsigned long mask = gfn_x(sgfn) | mfn_x(smfn) | nr;
+        unsigned long order;
+
+        /* Always map 4k by 4k when memaccess is enabled */
+        if ( unlikely(p2m->mem_access_enabled) )
+            order = THIRD_ORDER;
+        else if ( !(mask & ((1UL << FIRST_ORDER) - 1)) )
+            order = FIRST_ORDER;
+        else if ( !(mask & ((1UL << SECOND_ORDER) - 1)) )
+            order = SECOND_ORDER;
+        else
+            order = THIRD_ORDER;
+
+        rc = __p2m_set_entry(p2m, sgfn, order, smfn, t, a);
+        if ( rc )
+            break;
+
+        sgfn = gfn_add(sgfn, (1 << order));
+        if ( !mfn_eq(smfn, INVALID_MFN) )
+           smfn = mfn_add(smfn, (1 << order));
+
+        nr -= (1 << order);
+    }
+
+    return rc;
+}
+
 /*
  * Returns true if start_gpaddr..end_gpaddr contains at least one
  * suitably aligned level_size mappping of maddr.
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 6fe6a37..cca86ef 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -187,6 +187,17 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
                     p2m_type_t *t, p2m_access_t *a,
                     unsigned int *page_order);
 
+/*
+ * Direct set a p2m entry: only for use by the P2M code.
+ * The P2M write lock should be taken.
+ */
+int p2m_set_entry(struct p2m_domain *p2m,
+                  gfn_t sgfn,
+                  unsigned long nr,
+                  mfn_t smfn,
+                  p2m_type_t t,
+                  p2m_access_t a);
+
 /* Clean & invalidate caches corresponding to a region of guest address space */
 int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr);
 
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index a43b0fa..ba63322 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -166,6 +166,10 @@ typedef struct __packed {
     unsigned long sbz1:5;
 } lpae_p2m_t;
 
+/* Permission mask: xn, write, read */
+#define P2M_PERM_MASK (0x00400000000000C0ULL)
+#define P2M_CLEAR_PERM(pte) ((pte).bits & ~P2M_PERM_MASK)
+
 /*
  * Walk is the common bits of p2m and pt entries which are needed to
  * simply walk the table (e.g. for debug).
-- 
1.9.1


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

[for-4.8][PATCH v2 18/23] xen/arm: p2m: Re-implement relinquish_p2m_mapping using p2m_{get, set}_entry

[Julien Grall]

The function relinquish_p2m_mapping can be re-implemented using
p2m_{get,set}_entry by iterating over the range mapped and using the
mapping order given by the callee.

Given that the preemption was chosen arbitrarily, it is now done on every
512 iterations. Meaning that Xen may check more often if the function is
preempted when there are no mappings.

Finally drop the operation RELINQUISH in apply_* as nobody is using it
anymore. Note that the functions could have been dropped in one go at
the end, however I find easier to drop the operations one by one
avoiding a big deletion in the patch that remove the last operation.

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

---
    Changes in v2:
        - Erase entry one by one as I have not time so far to check
        whether it is possible to avoid removing entry in the p2m.
        - Use gfn_next_boundary
---
 xen/arch/arm/p2m.c | 77 +++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 59 insertions(+), 18 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 5f7aef0..ce09c4c 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -754,7 +754,6 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn,
 enum p2m_operation {
     INSERT,
     REMOVE,
-    RELINQUISH,
     MEMACCESS,
 };
 
@@ -1318,7 +1317,6 @@ static int apply_one_level(struct domain *d,
 
         break;
 
-    case RELINQUISH:
     case REMOVE:
         if ( !p2m_valid(orig_pte) )
         {
@@ -1502,17 +1500,6 @@ static int apply_p2m_changes(struct domain *d,
         {
             switch ( op )
             {
-            case RELINQUISH:
-                /*
-                 * Arbitrarily, preempt every 512 operations or 8192 nops.
-                 * 512*P2M_ONE_PROGRESS == 8192*P2M_ONE_PROGRESS_NOP == 0x2000
-                 * This is set in preempt_count_limit.
-                 *
-                 */
-                p2m->lowest_mapped_gfn = _gfn(addr >> PAGE_SHIFT);
-                rc = -ERESTART;
-                goto out;
-
             case MEMACCESS:
             {
                 /*
@@ -1919,16 +1906,70 @@ int p2m_init(struct domain *d)
     return rc;
 }
 
+/*
+ * The function will go through the p2m and remove page reference when it
+ * is required. The mapping will be removed from the p2m.
+ *
+ * XXX: See whether the mapping can be left intact in the p2m.
+ */
 int relinquish_p2m_mapping(struct domain *d)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
-    unsigned long nr;
+    unsigned long count = 0;
+    p2m_type_t t;
+    int rc = 0;
+    unsigned int order;
+
+    /* Convenience alias */
+    gfn_t start = p2m->lowest_mapped_gfn;
+    gfn_t end = p2m->max_mapped_gfn;
+
+    p2m_write_lock(p2m);
+
+    for ( ; gfn_x(start) < gfn_x(end);
+          start = gfn_next_boundary(start, order) )
+    {
+        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order);
+
+        count++;
+        /*
+         * Arbitrarily preempt every 512 iterations.
+         */
+        if ( !(count % 512) && hypercall_preempt_check() )
+        {
+            rc = -ERESTART;
+            break;
+        }
 
-    nr = gfn_x(p2m->max_mapped_gfn) - gfn_x(p2m->lowest_mapped_gfn);
+        /*
+         * p2m_set_entry will take care of removing reference on page
+         * when it is necessary and removing the mapping in the p2m.
+         */
+        if ( !mfn_eq(mfn, INVALID_MFN) )
+        {
+            /*
+             * For valid mapping, the start will always be aligned as
+             * entry will be removed whilst relinquishing.
+             */
+            rc = __p2m_set_entry(p2m, start, order, INVALID_MFN,
+                                 p2m_invalid, p2m_access_rwx);
+            if ( unlikely(rc) )
+            {
+                printk(XENLOG_G_ERR "Unable to remove mapping gfn=%#"PRI_gfn" order=%u from the p2m of domain %d\n", gfn_x(start), order, d->domain_id);
+                break;
+            }
+        }
+    }
 
-    return apply_p2m_changes(d, RELINQUISH, p2m->lowest_mapped_gfn, nr,
-                             INVALID_MFN, 0, p2m_invalid,
-                             d->arch.p2m.default_access);
+    /*
+     * Update lowest_mapped_gfn so on the next call we still start where
+     * we stopped.
+     */
+    p2m->lowest_mapped_gfn = start;
+
+    p2m_write_unlock(p2m);
+
+    return rc;
 }
 
 int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr)
-- 
1.9.1


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

[for-4.8][PATCH v2 19/23] xen/arm: p2m: Re-implement p2m_remove_using using p2m_set_entry

[Julien Grall]

The function p2m_insert_mapping can be re-implemented using the generic
function p2m_set_entry.

Also drop the operation REMOVE in apply_* as nobody is using it anymore.
Note that the functions could have been dropped in one go at the end,
however I find easier to drop the operations one by one avoiding a big
deletion in the patch that converts the last operation.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabelini <sstabellini@kernel.org>

---
    Changes in v2:
        - Add Stefano's reviewed-by
---
 xen/arch/arm/p2m.c | 125 ++++++-----------------------------------------------
 1 file changed, 13 insertions(+), 112 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index ce09c4c..6c9a6b2 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -753,7 +753,6 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn,
 
 enum p2m_operation {
     INSERT,
-    REMOVE,
     MEMACCESS,
 };
 
@@ -1232,7 +1231,6 @@ static int apply_one_level(struct domain *d,
                            p2m_access_t a)
 {
     const paddr_t level_size = level_sizes[level];
-    const paddr_t level_mask = level_masks[level];
 
     struct p2m_domain *p2m = &d->arch.p2m;
     lpae_t pte;
@@ -1317,74 +1315,6 @@ static int apply_one_level(struct domain *d,
 
         break;
 
-    case REMOVE:
-        if ( !p2m_valid(orig_pte) )
-        {
-            /* Progress up to next boundary */
-            *addr = (*addr + level_size) & level_mask;
-            *maddr = (*maddr + level_size) & level_mask;
-            return P2M_ONE_PROGRESS_NOP;
-        }
-
-        if ( level < 3 )
-        {
-            if ( p2m_table(orig_pte) )
-                return P2M_ONE_DESCEND;
-
-            if ( op == REMOVE &&
-                 !is_mapping_aligned(*addr, end_gpaddr,
-                                     0, /* maddr doesn't matter for remove */
-                                     level_size) )
-            {
-                /*
-                 * Removing a mapping from the middle of a superpage. Shatter
-                 * and descend.
-                 */
-                *flush = true;
-                rc = p2m_shatter_page(p2m, entry, level);
-                if ( rc < 0 )
-                    return rc;
-
-                return P2M_ONE_DESCEND;
-            }
-        }
-
-        /*
-         * Ensure that the guest address addr currently being
-         * handled (that is in the range given as argument to
-         * this function) is actually mapped to the corresponding
-         * machine address in the specified range. maddr here is
-         * the machine address given to the function, while
-         * orig_pte.p2m.base is the machine frame number actually
-         * mapped to the guest address: check if the two correspond.
-         */
-         if ( op == REMOVE &&
-              pfn_to_paddr(orig_pte.p2m.base) != *maddr )
-             printk(XENLOG_G_WARNING
-                    "p2m_remove dom%d: mapping at %"PRIpaddr" is of maddr %"PRIpaddr" not %"PRIpaddr" as expected\n",
-                    d->domain_id, *addr, pfn_to_paddr(orig_pte.p2m.base),
-                    *maddr);
-
-        *flush = true;
-
-        p2m_remove_pte(entry, p2m->clean_pte);
-        p2m_mem_access_radix_set(p2m, _gfn(paddr_to_pfn(*addr)),
-                                 p2m_access_rwx);
-
-        *addr += level_size;
-        *maddr += level_size;
-
-        p2m->stats.mappings[level]--;
-
-        if ( level == 3 )
-            p2m_put_l3_page(orig_pte);
-
-        /*
-         * This is still a single pte write, no matter the level, so no need to
-         * scale.
-         */
-        return P2M_ONE_PROGRESS;
-
     case MEMACCESS:
         if ( level < 3 )
         {
@@ -1596,43 +1526,6 @@ static int apply_p2m_changes(struct domain *d,
         }
 
         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, p2m->clean_pte);
-
-                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 == INSERT )
@@ -1674,8 +1567,10 @@ out:
          * addr keeps the address of the end of the last successfully-inserted
          * mapping.
          */
-        apply_p2m_changes(d, REMOVE, sgfn, gfn - gfn_x(sgfn), smfn,
-                          0, p2m_invalid, d->arch.p2m.default_access);
+        p2m_write_lock(p2m);
+        p2m_set_entry(p2m, sgfn, gfn - gfn_x(sgfn), INVALID_MFN,
+                      p2m_invalid, p2m_access_rwx);
+        p2m_write_unlock(p2m);
     }
 
     return rc;
@@ -1696,9 +1591,15 @@ static inline int p2m_remove_mapping(struct domain *d,
                                      unsigned long nr,
                                      mfn_t mfn)
 {
-    return apply_p2m_changes(d, REMOVE, start_gfn, nr, mfn,
-                             /* arguments below not used when removing mapping */
-                             0, p2m_invalid, d->arch.p2m.default_access);
+    struct p2m_domain *p2m = &d->arch.p2m;
+    int rc;
+
+    p2m_write_lock(p2m);
+    rc = p2m_set_entry(p2m, start_gfn, nr, INVALID_MFN,
+                       p2m_invalid, p2m_access_rwx);
+    p2m_write_unlock(p2m);
+
+    return rc;
 }
 
 int map_regions_rw_cache(struct domain *d,
-- 
1.9.1


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

[for-4.8][PATCH v2 20/23] xen/arm: p2m: Re-implement p2m_insert_mapping using p2m_set_entry

[Julien Grall]

The function p2m_insert_mapping can be re-implemented using the generic
function p2m_set_entry.

Note that the mapping is not reverted anymore if Xen fails to insert a
mapping. This was added to ensure the MMIO are not kept half-mapped
in case of failure and to follow the x86 counterpart. This was removed
on the x86 part by commit c3c756bd "x86/p2m: use large pages for MMIO
mappings" and I think we should let the caller taking care of it.

Finally drop the operation INSERT in apply_* as nobody is using it
anymore. Note that the functions could have been dropped in one go at the
end, however I find easier to drop the operations one by one avoiding a
big deletion in the patch that convert the last operation.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---
    Changes in v2:
        - Drop todo about safety checks (similar as x86) as we are not
        mandate to protect a guest from his own dumbess as long as it
        does not impact Xen internal reference counting (e.g foreign).
        - Add Stefano's Reviewed-by
        - Fix typo
---
 xen/arch/arm/p2m.c | 143 +++--------------------------------------------------
 1 file changed, 8 insertions(+), 135 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 6c9a6b2..734923b 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -752,7 +752,6 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn,
 }
 
 enum p2m_operation {
-    INSERT,
     MEMACCESS,
 };
 
@@ -1155,41 +1154,6 @@ int p2m_set_entry(struct p2m_domain *p2m,
     return rc;
 }
 
-/*
- * Returns true if start_gpaddr..end_gpaddr contains at least one
- * suitably aligned level_size mappping of maddr.
- *
- * So long as the range is large enough the end_gpaddr need not be
- * aligned (callers should create one superpage mapping based on this
- * result and then call this again on the new range, eventually the
- * slop at the end will cause this function to return false).
- */
-static bool_t is_mapping_aligned(const paddr_t start_gpaddr,
-                                 const paddr_t end_gpaddr,
-                                 const paddr_t maddr,
-                                 const paddr_t level_size)
-{
-    const paddr_t level_mask = level_size - 1;
-
-    /* No hardware superpages at level 0 */
-    if ( level_size == ZEROETH_SIZE )
-        return false;
-
-    /*
-     * A range smaller than the size of a superpage at this level
-     * cannot be superpage aligned.
-     */
-    if ( ( end_gpaddr - start_gpaddr ) < level_size - 1 )
-        return false;
-
-    /* Both the gpaddr and maddr must be aligned */
-    if ( start_gpaddr & level_mask )
-        return false;
-    if ( maddr & level_mask )
-        return false;
-    return true;
-}
-
 #define P2M_ONE_DESCEND        0
 #define P2M_ONE_PROGRESS_NOP   0x1
 #define P2M_ONE_PROGRESS       0x10
@@ -1241,80 +1205,6 @@ static int apply_one_level(struct domain *d,
 
     switch ( op )
     {
-    case INSERT:
-        if ( is_mapping_aligned(*addr, end_gpaddr, *maddr, level_size) &&
-           /*
-            * We do not handle replacing an existing table with a superpage
-            * or when mem_access is in use.
-            */
-             (level == 3 || (!p2m_table(orig_pte) && !p2m->mem_access_enabled)) )
-        {
-            rc = p2m_mem_access_radix_set(p2m, _gfn(paddr_to_pfn(*addr)), a);
-            if ( rc < 0 )
-                return rc;
-
-            /* New mapping is superpage aligned, make it */
-            pte = mfn_to_p2m_entry(_mfn(*maddr >> PAGE_SHIFT), t, a);
-            if ( level < 3 )
-                pte.p2m.table = 0; /* Superpage entry */
-
-            p2m_write_pte(entry, pte, p2m->clean_pte);
-
-            *flush |= p2m_valid(orig_pte);
-
-            *addr += level_size;
-            *maddr += level_size;
-
-            if ( p2m_valid(orig_pte) )
-            {
-                /*
-                 * We can't currently get here for an existing table
-                 * mapping, since we don't handle replacing an
-                 * existing table with a superpage. If we did we would
-                 * need to handle freeing (and accounting) for the bit
-                 * of the p2m tree which we would be about to lop off.
-                 */
-                BUG_ON(level < 3 && p2m_table(orig_pte));
-                if ( level == 3 )
-                    p2m_put_l3_page(orig_pte);
-            }
-            else /* New mapping */
-                p2m->stats.mappings[level]++;
-
-            return P2M_ONE_PROGRESS;
-        }
-        else
-        {
-            /* New mapping is not superpage aligned, create a new table entry */
-
-            /* L3 is always suitably aligned for mapping (handled, above) */
-            BUG_ON(level == 3);
-
-            /* Not present -> create table entry and descend */
-            if ( !p2m_valid(orig_pte) )
-            {
-                rc = p2m_create_table(p2m, entry, 0);
-                if ( rc < 0 )
-                    return rc;
-                return P2M_ONE_DESCEND;
-            }
-
-            /* Existing superpage mapping -> shatter and descend */
-            if ( p2m_mapping(orig_pte) )
-            {
-                *flush = true;
-                rc = p2m_shatter_page(p2m, entry, level);
-                if ( rc < 0 )
-                    return rc;
-            } /* else: an existing table mapping -> descend */
-
-            BUG_ON(!p2m_table(*entry));
-
-            return P2M_ONE_DESCEND;
-        }
-
-        break;
-
     case MEMACCESS:
         if ( level < 3 )
         {
@@ -1528,13 +1418,6 @@ static int apply_p2m_changes(struct domain *d,
         BUG_ON(level > 3);
     }
 
-    if ( op == INSERT )
-    {
-        p2m->max_mapped_gfn = gfn_max(p2m->max_mapped_gfn,
-                                      gfn_add(sgfn, nr));
-        p2m->lowest_mapped_gfn = gfn_min(p2m->lowest_mapped_gfn, sgfn);
-    }
-
     rc = 0;
 
 out:
@@ -1557,22 +1440,6 @@ out:
 
     p2m_write_unlock(p2m);
 
-    if ( rc < 0 && ( op == INSERT ) &&
-         addr != start_gpaddr )
-    {
-        unsigned long gfn = paddr_to_pfn(addr);
-
-        BUG_ON(addr == end_gpaddr);
-        /*
-         * addr keeps the address of the end of the last successfully-inserted
-         * mapping.
-         */
-        p2m_write_lock(p2m);
-        p2m_set_entry(p2m, sgfn, gfn - gfn_x(sgfn), INVALID_MFN,
-                      p2m_invalid, p2m_access_rwx);
-        p2m_write_unlock(p2m);
-    }
-
     return rc;
 }
 
@@ -1582,8 +1449,14 @@ static inline int p2m_insert_mapping(struct domain *d,
                                      mfn_t mfn,
                                      p2m_type_t t)
 {
-    return apply_p2m_changes(d, INSERT, start_gfn, nr, mfn,
-                             0, t, d->arch.p2m.default_access);
+    struct p2m_domain *p2m = &d->arch.p2m;
+    int rc;
+
+    p2m_write_lock(p2m);
+    rc = p2m_set_entry(p2m, start_gfn, nr, mfn, t, p2m->default_access);
+    p2m_write_unlock(p2m);
+
+    return rc;
 }
 
 static inline int p2m_remove_mapping(struct domain *d,
-- 
1.9.1


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

[for-4.8][PATCH v2 21/23] xen/arm: p2m: Re-implement p2m_set_mem_access using p2m_{set, get}_entry

[Julien Grall]

The function p2m_set_mem_access can be re-implemented using the generic
functions p2m_get_entry and __p2m_set_entry.

Also the function apply_p2m_changes is dropped completely as it is not
used anymore.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Tamas K Lengyel <tamas@tklengyel.com>

---
    Changes in v2:
        - Remove level_shifts as it is not used anymore
        - Fix multiple bugs in the code
        - Use gfn_next_boundary
        - Drop the paragraph about performance issue as
        Break-Before-Make is not required when only the permission are
        changed.
---
 xen/arch/arm/p2m.c | 327 +++++------------------------------------------------
 1 file changed, 29 insertions(+), 298 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 734923b..aa740c2 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -34,8 +34,6 @@ static const paddr_t level_sizes[] =
     { ZEROETH_SIZE, FIRST_SIZE, SECOND_SIZE, THIRD_SIZE };
 static const paddr_t level_masks[] =
     { ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK };
-static const uint8_t level_shifts[] =
-    { ZEROETH_SHIFT, FIRST_SHIFT, SECOND_SHIFT, THIRD_SHIFT };
 static const uint8_t level_orders[] =
     { ZEROETH_ORDER, FIRST_ORDER, SECOND_ORDER, THIRD_ORDER };
 
@@ -1154,295 +1152,6 @@ int p2m_set_entry(struct p2m_domain *p2m,
     return rc;
 }
 
-#define P2M_ONE_DESCEND        0
-#define P2M_ONE_PROGRESS_NOP   0x1
-#define P2M_ONE_PROGRESS       0x10
-
-static int p2m_shatter_page(struct p2m_domain *p2m,
-                            lpae_t *entry,
-                            unsigned int level)
-{
-    const uint8_t level_shift = level_shifts[level];
-    int rc = p2m_create_table(p2m, entry, level_shift - PAGE_SHIFT);
-
-    if ( !rc )
-    {
-        p2m->stats.shattered[level]++;
-        p2m->stats.mappings[level]--;
-        p2m->stats.mappings[level+1] += LPAE_ENTRIES;
-    }
-
-    return rc;
-}
-
-/*
- * 0   == (P2M_ONE_DESCEND) continue to descend the tree
- * +ve == (P2M_ONE_PROGRESS_*) handled at this level, continue, flush,
- *        entry, addr and maddr updated.  Return value is an
- *        indication of the amount of work done (for preemption).
- * -ve == (-Exxx) error.
- */
-static int apply_one_level(struct domain *d,
-                           lpae_t *entry,
-                           unsigned int level,
-                           enum p2m_operation op,
-                           paddr_t start_gpaddr,
-                           paddr_t end_gpaddr,
-                           paddr_t *addr,
-                           paddr_t *maddr,
-                           bool_t *flush,
-                           p2m_type_t t,
-                           p2m_access_t a)
-{
-    const paddr_t level_size = level_sizes[level];
-
-    struct p2m_domain *p2m = &d->arch.p2m;
-    lpae_t pte;
-    const lpae_t orig_pte = *entry;
-    int rc;
-
-    BUG_ON(level > 3);
-
-    switch ( op )
-    {
-    case MEMACCESS:
-        if ( level < 3 )
-        {
-            if ( !p2m_valid(orig_pte) )
-            {
-                *addr += level_size;
-                return P2M_ONE_PROGRESS_NOP;
-            }
-
-            /* Shatter large pages as we descend */
-            if ( p2m_mapping(orig_pte) )
-            {
-                rc = p2m_shatter_page(p2m, entry, level);
-                if ( rc < 0 )
-                    return rc;
-            } /* else: an existing table mapping -> descend */
-
-            return P2M_ONE_DESCEND;
-        }
-        else
-        {
-            pte = orig_pte;
-
-            if ( p2m_valid(pte) )
-            {
-                rc = p2m_mem_access_radix_set(p2m, _gfn(paddr_to_pfn(*addr)),
-                                              a);
-                if ( rc < 0 )
-                    return rc;
-
-                p2m_set_permission(&pte, pte.p2m.type, a);
-                p2m_write_pte(entry, pte, p2m->clean_pte);
-            }
-
-            *addr += level_size;
-            *flush = true;
-            return P2M_ONE_PROGRESS;
-        }
-    }
-
-    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,
-                     gfn_t sgfn,
-                     unsigned long nr,
-                     mfn_t smfn,
-                     uint32_t mask,
-                     p2m_type_t t,
-                     p2m_access_t a)
-{
-    paddr_t start_gpaddr = pfn_to_paddr(gfn_x(sgfn));
-    paddr_t end_gpaddr = pfn_to_paddr(gfn_x(sgfn) + nr);
-    paddr_t maddr = pfn_to_paddr(mfn_x(smfn));
-    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;
-    unsigned int level = 0;
-    unsigned int cur_root_table = ~0;
-    unsigned int cur_offset[4] = { ~0, ~0, ~0, ~0 };
-    unsigned int count = 0;
-    const unsigned int preempt_count_limit = (op == MEMACCESS) ? 1 : 0x2000;
-    const bool_t preempt = !is_idle_vcpu(current);
-    bool_t flush = false;
-    PAGE_LIST_HEAD(free_pages);
-    struct page_info *pg;
-
-    p2m_write_lock(p2m);
-
-    /* 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 )
-    {
-        int root_table;
-        const unsigned int offsets[4] = {
-            zeroeth_table_offset(addr),
-            first_table_offset(addr),
-            second_table_offset(addr),
-            third_table_offset(addr)
-        };
-
-        /*
-         * Check if current iteration should be possibly preempted.
-         * Since count is initialised to 0 above we are guaranteed to
-         * always make at least one pass as long as preempt_count_limit is
-         * initialized with a value >= 1.
-         */
-        if ( preempt && count >= preempt_count_limit
-             && hypercall_preempt_check() )
-        {
-            switch ( op )
-            {
-            case MEMACCESS:
-            {
-                /*
-                 * Preempt setting mem_access permissions as required by XSA-89,
-                 * if it's not the last iteration.
-                 */
-                uint32_t progress = paddr_to_pfn(addr) - gfn_x(sgfn) + 1;
-
-                if ( nr > progress && !(progress & mask) )
-                {
-                    rc = progress;
-                    goto out;
-                }
-                break;
-            }
-
-            default:
-                break;
-            };
-
-            /*
-             * Reset current iteration counter.
-             */
-            count = 0;
-        }
-
-        if ( P2M_ROOT_PAGES > 1 )
-        {
-            int i;
-            /*
-             * Concatenated root-level tables. The table number will be the
-             * offset at the previous level. It is not possible to concatenate
-             * a level-0 root.
-             */
-            ASSERT(P2M_ROOT_LEVEL > 0);
-            root_table = offsets[P2M_ROOT_LEVEL - 1];
-            if ( root_table >= P2M_ROOT_PAGES )
-            {
-                rc = -EINVAL;
-                goto out;
-            }
-
-            if ( cur_root_table != root_table )
-            {
-                if ( mappings[P2M_ROOT_LEVEL] )
-                    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++ )
-                    cur_offset[i] = ~0;
-            }
-        }
-
-        for ( level = P2M_ROOT_LEVEL; level < 4; level++ )
-        {
-            unsigned offset = offsets[level];
-            lpae_t *entry = &mappings[level][offset];
-            lpae_t old_entry = *entry;
-
-            ret = apply_one_level(d, entry,
-                                  level, op,
-                                  start_gpaddr, end_gpaddr,
-                                  &addr, &maddr, &flush,
-                                  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;
-
-            BUG_ON(!p2m_valid(*entry));
-
-            if ( cur_offset[level] != offset )
-            {
-                /* Update mapping for next level */
-                int i;
-                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++ )
-                    cur_offset[i] = ~0;
-            }
-            /* else: next level already valid */
-        }
-
-        BUG_ON(level > 3);
-    }
-
-    rc = 0;
-
-out:
-    if ( flush )
-    {
-        p2m_flush_tlb_sync(&d->arch.p2m);
-        ret = iommu_iotlb_flush(d, gfn_x(sgfn), nr);
-        if ( !rc )
-            rc = ret;
-    }
-
-    while ( (pg = page_list_remove_head(&free_pages)) )
-        free_domheap_page(pg);
-
-    for ( level = P2M_ROOT_LEVEL; level < 4; level ++ )
-    {
-        if ( mappings[level] )
-            unmap_domain_page(mappings[level]);
-    }
-
-    p2m_write_unlock(p2m);
-
-    return rc;
-}
-
 static inline int p2m_insert_mapping(struct domain *d,
                                      gfn_t start_gfn,
                                      unsigned long nr,
@@ -2139,6 +1848,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     p2m_access_t a;
+    unsigned int order;
     long rc = 0;
 
     static const p2m_access_t memaccess[] = {
@@ -2181,14 +1891,35 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
         return 0;
     }
 
-    rc = apply_p2m_changes(d, MEMACCESS, gfn_add(gfn, start),
-                           (nr - start), INVALID_MFN, mask, 0, a);
-    if ( rc < 0 )
-        return rc;
-    else if ( rc > 0 )
-        return start + rc;
+    p2m_write_lock(p2m);
 
-    return 0;
+    for ( gfn = gfn_add(gfn, start); nr > start;
+          gfn = gfn_next_boundary(gfn, order) )
+    {
+        p2m_type_t t;
+        mfn_t mfn = p2m_get_entry(p2m, gfn, &t, NULL, &order);
+
+
+        if ( !mfn_eq(mfn, INVALID_MFN) )
+        {
+            order = 0;
+            rc = __p2m_set_entry(p2m, gfn, 0, mfn, t, a);
+            if ( rc )
+                break;
+        }
+
+        start += gfn_x(gfn_next_boundary(gfn, order)) - gfn_x(gfn);
+        /* Check for continuation if it is not the last iteration */
+        if ( nr > start && !(start & mask) && hypercall_preempt_check() )
+        {
+            rc = start;
+            break;
+        }
+    }
+
+    p2m_write_unlock(p2m);
+
+    return rc;
 }
 
 int p2m_get_mem_access(struct domain *d, gfn_t gfn,
-- 
1.9.1


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

[for-4.8][PATCH v2 22/23] xen/arm: p2m: Do not handle shattering in p2m_create_table

[Julien Grall]

The helper p2m_create_table is only called to create a brand new table.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewd-by: Stefano Stabellini <sstabelini@kernel.org>

---
    Changes in v2:
        - Add Stefano's reviewed-by
---
 xen/arch/arm/p2m.c | 56 ++++++------------------------------------------------
 1 file changed, 6 insertions(+), 50 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index aa740c2..c1dac09 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -278,8 +278,7 @@ static p2m_access_t p2m_mem_access_radix_get(struct p2m_domain *p2m, gfn_t gfn)
 #define GUEST_TABLE_SUPER_PAGE 1
 #define GUEST_TABLE_NORMAL_PAGE 2
 
-static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry,
-                            int level_shift);
+static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry);
 
 /*
  * Take the currently mapped table, find the corresponding GFN entry,
@@ -310,7 +309,7 @@ static int p2m_next_level(struct p2m_domain *p2m, bool read_only,
         if ( read_only )
             return GUEST_TABLE_MAP_FAILED;
 
-        ret = p2m_create_table(p2m, entry, /* not used */ ~0);
+        ret = p2m_create_table(p2m, entry);
         if ( ret )
             return GUEST_TABLE_MAP_FAILED;
     }
@@ -575,25 +574,14 @@ static inline void p2m_remove_pte(lpae_t *p, bool clean_pte)
     p2m_write_pte(p, pte, clean_pte);
 }
 
-/*
- * Allocate a new page table page and hook it in via the given entry.
- * apply_one_level relies on this returning 0 on success
- * and -ve on failure.
- *
- * If the existing entry is present then it must be a mapping and not
- * a table and it will be shattered into the next level down.
- *
- * level_shift is the number of bits at the level we want to create.
- */
-static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry,
-                            int level_shift)
+/* Allocate a new page table page and hook it in via the given entry. */
+static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry)
 {
     struct page_info *page;
     lpae_t *p;
     lpae_t pte;
-    int splitting = p2m_valid(*entry);
 
-    BUG_ON(p2m_table(*entry));
+    ASSERT(!p2m_valid(*entry));
 
     page = alloc_domheap_page(NULL, 0);
     if ( page == NULL )
@@ -602,39 +590,7 @@ static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry,
     page_list_add(page, &p2m->pages);
 
     p = __map_domain_page(page);
-    if ( splitting )
-    {
-        mfn_t mfn = _mfn(entry->p2m.base);
-        int i;
-
-        /*
-         * We are either splitting a first level 1G page into 512 second level
-         * 2M pages, or a second level 2M page into 512 third level 4K pages.
-         */
-         for ( i=0 ; i < LPAE_ENTRIES; i++ )
-         {
-             /*
-              * Use the content of the superpage entry and override
-              * the necessary fields. So the correct permissions are
-              * kept.
-              */
-             pte = *entry;
-             pte.p2m.base = mfn_x(mfn_add(mfn,
-                                          i << (level_shift - LPAE_SHIFT)));
-
-             /*
-              * First and second level super pages set p2m.table = 0, but
-              * third level entries set table = 1.
-              */
-             pte.p2m.table = !(level_shift - LPAE_SHIFT);
-
-             write_pte(&p[i], pte);
-         }
-
-         page->u.inuse.p2m_refcount = LPAE_ENTRIES;
-    }
-    else
-        clear_page(p);
+    clear_page(p);
 
     if ( p2m->clean_pte )
         clean_dcache_va_range(p, PAGE_SIZE);
-- 
1.9.1


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

[for-4.8][PATCH v2 23/23] xen/arm: p2m: Export p2m_*_lock helpers

[Julien Grall]

Earlier patches exported the p2m interface (p2m_get_entry and
p2m_set_entry) to allow splitting xen/arch/arm/p2m.c. Those functions
require the callers to lock the p2m, so we need to export p2m_*_lock
helpers.

All helpers but p2m_write_unlock but p2m_write_unlock are moved in
xen/include/asm-arm/p2m.h to allow inlining. The helpers
p2m_write_unlock is kept in p2m.c because it depends on a static
function.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/p2m.c        | 28 ++--------------------------
 xen/include/asm-arm/p2m.h | 27 +++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index c1dac09..fa08e06 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -60,11 +60,6 @@ static inline bool p2m_is_superpage(lpae_t pte, unsigned int level)
     return (level < 3) && p2m_mapping(pte);
 }
 
-static inline void p2m_write_lock(struct p2m_domain *p2m)
-{
-    write_lock(&p2m->lock);
-}
-
 /*
  * Return the start of the next mapping based on the order of the
  * current one.
@@ -83,7 +78,8 @@ static inline gfn_t gfn_next_boundary(gfn_t gfn, unsigned int order)
 
 static void p2m_flush_tlb(struct p2m_domain *p2m);
 
-static inline void p2m_write_unlock(struct p2m_domain *p2m)
+/* Unlock the flush and do a P2M TLB flush if necessary */
+void p2m_write_unlock(struct p2m_domain *p2m)
 {
     if ( p2m->need_flush )
     {
@@ -99,26 +95,6 @@ static inline void p2m_write_unlock(struct p2m_domain *p2m)
     write_unlock(&p2m->lock);
 }
 
-static inline void p2m_read_lock(struct p2m_domain *p2m)
-{
-    read_lock(&p2m->lock);
-}
-
-static inline void p2m_read_unlock(struct p2m_domain *p2m)
-{
-    read_unlock(&p2m->lock);
-}
-
-static inline int p2m_is_locked(struct p2m_domain *p2m)
-{
-    return rw_is_locked(&p2m->lock);
-}
-
-static inline int p2m_is_write_locked(struct p2m_domain *p2m)
-{
-    return rw_is_write_locked(&p2m->lock);
-}
-
 void p2m_dump_info(struct domain *d)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index cca86ef..1480c5b 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -176,6 +176,33 @@ void p2m_restore_state(struct vcpu *n);
 /* Print debugging/statistial info about a domain's p2m */
 void p2m_dump_info(struct domain *d);
 
+static inline void p2m_write_lock(struct p2m_domain *p2m)
+{
+    write_lock(&p2m->lock);
+}
+
+void p2m_write_unlock(struct p2m_domain *p2m);
+
+static inline void p2m_read_lock(struct p2m_domain *p2m)
+{
+    read_lock(&p2m->lock);
+}
+
+static inline void p2m_read_unlock(struct p2m_domain *p2m)
+{
+    read_unlock(&p2m->lock);
+}
+
+static inline int p2m_is_locked(struct p2m_domain *p2m)
+{
+    return rw_is_locked(&p2m->lock);
+}
+
+static inline int p2m_is_write_locked(struct p2m_domain *p2m)
+{
+    return rw_is_write_locked(&p2m->lock);
+}
+
 /* Look up the MFN corresponding to a domain's GFN. */
 mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
 
-- 
1.9.1


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

Re: [for-4.8][PATCH v2 21/23] xen/arm: p2m: Re-implement p2m_set_mem_access using p2m_{set, get}_entry

[Razvan Cojocaru]

On 09/15/2016 02:28 PM, Julien Grall wrote:
> The function p2m_set_mem_access can be re-implemented using the generic
> functions p2m_get_entry and __p2m_set_entry.
> 
> Also the function apply_p2m_changes is dropped completely as it is not
> used anymore.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Tamas K Lengyel <tamas@tklengyel.com>
> 
> ---
>     Changes in v2:
>         - Remove level_shifts as it is not used anymore
>         - Fix multiple bugs in the code
>         - Use gfn_next_boundary
>         - Drop the paragraph about performance issue as
>         Break-Before-Make is not required when only the permission are
>         changed.
> ---
>  xen/arch/arm/p2m.c | 327 +++++------------------------------------------------
>  1 file changed, 29 insertions(+), 298 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 734923b..aa740c2 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -34,8 +34,6 @@ static const paddr_t level_sizes[] =
>      { ZEROETH_SIZE, FIRST_SIZE, SECOND_SIZE, THIRD_SIZE };
>  static const paddr_t level_masks[] =
>      { ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK };
> -static const uint8_t level_shifts[] =
> -    { ZEROETH_SHIFT, FIRST_SHIFT, SECOND_SHIFT, THIRD_SHIFT };
>  static const uint8_t level_orders[] =
>      { ZEROETH_ORDER, FIRST_ORDER, SECOND_ORDER, THIRD_ORDER };
>  
> @@ -1154,295 +1152,6 @@ int p2m_set_entry(struct p2m_domain *p2m,
>      return rc;
>  }
>  
> -#define P2M_ONE_DESCEND        0
> -#define P2M_ONE_PROGRESS_NOP   0x1
> -#define P2M_ONE_PROGRESS       0x10
> -
> -static int p2m_shatter_page(struct p2m_domain *p2m,
> -                            lpae_t *entry,
> -                            unsigned int level)
> -{
> -    const uint8_t level_shift = level_shifts[level];
> -    int rc = p2m_create_table(p2m, entry, level_shift - PAGE_SHIFT);
> -
> -    if ( !rc )
> -    {
> -        p2m->stats.shattered[level]++;
> -        p2m->stats.mappings[level]--;
> -        p2m->stats.mappings[level+1] += LPAE_ENTRIES;
> -    }
> -
> -    return rc;
> -}
> -
> -/*
> - * 0   == (P2M_ONE_DESCEND) continue to descend the tree
> - * +ve == (P2M_ONE_PROGRESS_*) handled at this level, continue, flush,
> - *        entry, addr and maddr updated.  Return value is an
> - *        indication of the amount of work done (for preemption).
> - * -ve == (-Exxx) error.
> - */
> -static int apply_one_level(struct domain *d,
> -                           lpae_t *entry,
> -                           unsigned int level,
> -                           enum p2m_operation op,
> -                           paddr_t start_gpaddr,
> -                           paddr_t end_gpaddr,
> -                           paddr_t *addr,
> -                           paddr_t *maddr,
> -                           bool_t *flush,
> -                           p2m_type_t t,
> -                           p2m_access_t a)
> -{
> -    const paddr_t level_size = level_sizes[level];
> -
> -    struct p2m_domain *p2m = &d->arch.p2m;
> -    lpae_t pte;
> -    const lpae_t orig_pte = *entry;
> -    int rc;
> -
> -    BUG_ON(level > 3);
> -
> -    switch ( op )
> -    {
> -    case MEMACCESS:
> -        if ( level < 3 )
> -        {
> -            if ( !p2m_valid(orig_pte) )
> -            {
> -                *addr += level_size;
> -                return P2M_ONE_PROGRESS_NOP;
> -            }
> -
> -            /* Shatter large pages as we descend */
> -            if ( p2m_mapping(orig_pte) )
> -            {
> -                rc = p2m_shatter_page(p2m, entry, level);
> -                if ( rc < 0 )
> -                    return rc;
> -            } /* else: an existing table mapping -> descend */
> -
> -            return P2M_ONE_DESCEND;
> -        }
> -        else
> -        {
> -            pte = orig_pte;
> -
> -            if ( p2m_valid(pte) )
> -            {
> -                rc = p2m_mem_access_radix_set(p2m, _gfn(paddr_to_pfn(*addr)),
> -                                              a);
> -                if ( rc < 0 )
> -                    return rc;
> -
> -                p2m_set_permission(&pte, pte.p2m.type, a);
> -                p2m_write_pte(entry, pte, p2m->clean_pte);
> -            }
> -
> -            *addr += level_size;
> -            *flush = true;
> -            return P2M_ONE_PROGRESS;
> -        }
> -    }
> -
> -    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,
> -                     gfn_t sgfn,
> -                     unsigned long nr,
> -                     mfn_t smfn,
> -                     uint32_t mask,
> -                     p2m_type_t t,
> -                     p2m_access_t a)
> -{
> -    paddr_t start_gpaddr = pfn_to_paddr(gfn_x(sgfn));
> -    paddr_t end_gpaddr = pfn_to_paddr(gfn_x(sgfn) + nr);
> -    paddr_t maddr = pfn_to_paddr(mfn_x(smfn));
> -    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;
> -    unsigned int level = 0;
> -    unsigned int cur_root_table = ~0;
> -    unsigned int cur_offset[4] = { ~0, ~0, ~0, ~0 };
> -    unsigned int count = 0;
> -    const unsigned int preempt_count_limit = (op == MEMACCESS) ? 1 : 0x2000;
> -    const bool_t preempt = !is_idle_vcpu(current);
> -    bool_t flush = false;
> -    PAGE_LIST_HEAD(free_pages);
> -    struct page_info *pg;
> -
> -    p2m_write_lock(p2m);
> -
> -    /* 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 )
> -    {
> -        int root_table;
> -        const unsigned int offsets[4] = {
> -            zeroeth_table_offset(addr),
> -            first_table_offset(addr),
> -            second_table_offset(addr),
> -            third_table_offset(addr)
> -        };
> -
> -        /*
> -         * Check if current iteration should be possibly preempted.
> -         * Since count is initialised to 0 above we are guaranteed to
> -         * always make at least one pass as long as preempt_count_limit is
> -         * initialized with a value >= 1.
> -         */
> -        if ( preempt && count >= preempt_count_limit
> -             && hypercall_preempt_check() )
> -        {
> -            switch ( op )
> -            {
> -            case MEMACCESS:
> -            {
> -                /*
> -                 * Preempt setting mem_access permissions as required by XSA-89,
> -                 * if it's not the last iteration.
> -                 */
> -                uint32_t progress = paddr_to_pfn(addr) - gfn_x(sgfn) + 1;
> -
> -                if ( nr > progress && !(progress & mask) )
> -                {
> -                    rc = progress;
> -                    goto out;
> -                }
> -                break;
> -            }
> -
> -            default:
> -                break;
> -            };
> -
> -            /*
> -             * Reset current iteration counter.
> -             */
> -            count = 0;
> -        }
> -
> -        if ( P2M_ROOT_PAGES > 1 )
> -        {
> -            int i;
> -            /*
> -             * Concatenated root-level tables. The table number will be the
> -             * offset at the previous level. It is not possible to concatenate
> -             * a level-0 root.
> -             */
> -            ASSERT(P2M_ROOT_LEVEL > 0);
> -            root_table = offsets[P2M_ROOT_LEVEL - 1];
> -            if ( root_table >= P2M_ROOT_PAGES )
> -            {
> -                rc = -EINVAL;
> -                goto out;
> -            }
> -
> -            if ( cur_root_table != root_table )
> -            {
> -                if ( mappings[P2M_ROOT_LEVEL] )
> -                    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++ )
> -                    cur_offset[i] = ~0;
> -            }
> -        }
> -
> -        for ( level = P2M_ROOT_LEVEL; level < 4; level++ )
> -        {
> -            unsigned offset = offsets[level];
> -            lpae_t *entry = &mappings[level][offset];
> -            lpae_t old_entry = *entry;
> -
> -            ret = apply_one_level(d, entry,
> -                                  level, op,
> -                                  start_gpaddr, end_gpaddr,
> -                                  &addr, &maddr, &flush,
> -                                  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;
> -
> -            BUG_ON(!p2m_valid(*entry));
> -
> -            if ( cur_offset[level] != offset )
> -            {
> -                /* Update mapping for next level */
> -                int i;
> -                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++ )
> -                    cur_offset[i] = ~0;
> -            }
> -            /* else: next level already valid */
> -        }
> -
> -        BUG_ON(level > 3);
> -    }
> -
> -    rc = 0;
> -
> -out:
> -    if ( flush )
> -    {
> -        p2m_flush_tlb_sync(&d->arch.p2m);
> -        ret = iommu_iotlb_flush(d, gfn_x(sgfn), nr);
> -        if ( !rc )
> -            rc = ret;
> -    }
> -
> -    while ( (pg = page_list_remove_head(&free_pages)) )
> -        free_domheap_page(pg);
> -
> -    for ( level = P2M_ROOT_LEVEL; level < 4; level ++ )
> -    {
> -        if ( mappings[level] )
> -            unmap_domain_page(mappings[level]);
> -    }
> -
> -    p2m_write_unlock(p2m);
> -
> -    return rc;
> -}
> -
>  static inline int p2m_insert_mapping(struct domain *d,
>                                       gfn_t start_gfn,
>                                       unsigned long nr,
> @@ -2139,6 +1848,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>  {
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>      p2m_access_t a;
> +    unsigned int order;
>      long rc = 0;
>  
>      static const p2m_access_t memaccess[] = {
> @@ -2181,14 +1891,35 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>          return 0;
>      }
>  
> -    rc = apply_p2m_changes(d, MEMACCESS, gfn_add(gfn, start),
> -                           (nr - start), INVALID_MFN, mask, 0, a);
> -    if ( rc < 0 )
> -        return rc;
> -    else if ( rc > 0 )
> -        return start + rc;
> +    p2m_write_lock(p2m);
>  
> -    return 0;
> +    for ( gfn = gfn_add(gfn, start); nr > start;
> +          gfn = gfn_next_boundary(gfn, order) )
> +    {
> +        p2m_type_t t;
> +        mfn_t mfn = p2m_get_entry(p2m, gfn, &t, NULL, &order);
> +
> +

Extra blank line here.

Other than that (and keeping in mind that I unfortunately can't test
this on ARM), FWIW:

Reviewed-by: Razvan Cojocaru <rcojocaru@bitdefender.com>


Thanks,
Razvan


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

Re: [for-4.8][PATCH v2 22/23] xen/arm: p2m: Do not handle shattering in p2m_create_table

[Julien Grall]

Hi,

On 15/09/2016 12:28, Julien Grall wrote:
> The helper p2m_create_table is only called to create a brand new table.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewd-by: Stefano Stabellini <sstabelini@kernel.org>

I made 2 typoes here. It should be:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Regards,

-- 
Julien Grall

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

Re: [for-4.8][PATCH v2 00/23] xen/arm: Rework the P2M code to follow break-before-make sequence

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 1901 bytes --]

On Thu, Sep 15, 2016 at 5:28 AM, Julien Grall <julien.grall@arm.com> wrote:

> Hello all,
>
> The ARM architecture mandates the use of a break-before-make sequence when
> changing translation entries if the page table is shared between multiple
> CPUs whenever a valid entry is replaced by another valid entry (see D4.7.1
> in ARM DDI 0487A.j for more details).
>
> The current P2M code does not respect this sequence and may result to
> break coherency on some processors.
>
> Adapting the current implementation to use break-before-make sequence would
> imply some code duplication and more TLBs invalidations than necessary.
> For instance, if we are replacing a 4KB page and the current mapping in
> the P2M is using a 1GB superpage, the following steps will happen:
>     1) Shatter the 1GB superpage into a series of 2MB superpages
>     2) Shatter the 2MB superpage into a series of 4KB superpages
>     3) Replace the 4KB page
>
> As the current implementation is shattering while descending and install
> the mapping before continuing to the next level, Xen would need to issue 3
> TLB invalidation instructions which is clearly inefficient.
>
> Furthermore, all the operations which modify the page table are using the
> same skeleton. It is more complicated to maintain different code paths than
> having a generic function that set an entry and take care of the
> break-before-
> make sequence.
>
> The new implementation is based on the x86 EPT one which, I think, fits
> quite well for the break-before-make sequence whilst keeping the code
> simple.
>
> For all the changes see in each patch.
>
> I have provided a branch based on upstream here:
> git://xenbits.xen.org/people/julieng/xen-unstable.git branch p2m-v2
>
>
Tested-by: Tamas K Lengyel <tamas@tklengyel.com>

Works without any issue on both the Cubietruck and the HiKey LeMaker with
the xen-access test-cases.

Cheers,
Tamas

[-- Attachment #1.2: Type: text/html, Size: 2618 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [for-4.8][PATCH v2 05/23] xen/arm: p2m: Add a back pointer to domain in p2m_domain

[Stefano Stabellini]

On Thu, 15 Sep 2016, Julien Grall wrote:
> The back pointer will be usefult later to get the domain when we only
> have the p2m in hand.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/p2m.c        | 1 +
>  xen/include/asm-arm/p2m.h | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 950a607..5cf136f 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1391,6 +1391,7 @@ int p2m_init(struct domain *d)
>      if ( rc != 0 )
>          return rc;
>  
> +    p2m->domain = d;
>      p2m->max_mapped_gfn = _gfn(0);
>      p2m->lowest_mapped_gfn = _gfn(ULONG_MAX);
>  
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index b9269e4..b27a3a1 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -81,6 +81,9 @@ struct p2m_domain {
>       * enough available bits to store this information.
>       */
>      struct radix_tree_root mem_access_settings;
> +
> +    /* back pointer to domain */
> +    struct domain *domain;
>  };
>  
>  /*
> -- 
> 1.9.1
> 

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

Re: [for-4.8][PATCH v2 06/23] xen/arm: traps: Move MMIO emulation code in a separate helper

[Stefano Stabellini]

On Thu, 15 Sep 2016, Julien Grall wrote:
> Currently, a stage-2 fault translation will likely access an emulated
> region. All the checks are pre-sanitity check for MMIO emulation.
> 
> A follow-up patch will handle a new case that could lead to a stage-2
> translation. To improve the clarity of the code and the changes, the
> current implementation is move in a separate helper.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Changes in v2:
>         - Keep the break in FSC_FLT_TRANS
>         - Use bool instead of bool_t
> ---
>  xen/arch/arm/traps.c | 57 ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 33 insertions(+), 24 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index a5a5384..76e4152 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2445,6 +2445,38 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>      inject_iabt_exception(regs, gva, hsr.len);
>  }
>  
> +static bool try_handle_mmio(struct cpu_user_regs *regs,
> +                            mmio_info_t *info)
> +{
> +    const struct hsr_dabt dabt = info->dabt;
> +    int rc;
> +
> +    /* stage-1 page table should never live in an emulated MMIO region */
> +    if ( dabt.s1ptw )
> +        return false;
> +
> +    /* All the instructions used on emulated MMIO region should be valid */
> +    if ( !dabt.valid )
> +        return false;
> +
> +    /*
> +     * Erratum 766422: Thumb store translation fault to Hypervisor may
> +     * not have correct HSR Rt value.
> +     */
> +    if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
> +         dabt.write )
> +    {
> +        rc = decode_instruction(regs, &info->dabt);
> +        if ( rc )
> +        {
> +            gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
> +            return false;
> +        }
> +    }
> +
> +    return !!handle_mmio(info);
> +}
> +
>  static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>                                       const union hsr hsr)
>  {
> @@ -2488,29 +2520,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>          break;
>      }
>      case FSC_FLT_TRANS:
> -        if ( dabt.s1ptw )
> -            goto bad_data_abort;
> -
> -        /* XXX: Decode the instruction if ISS is not valid */
> -        if ( !dabt.valid )
> -            goto bad_data_abort;
> -
> -        /*
> -         * Erratum 766422: Thumb store translation fault to Hypervisor may
> -         * not have correct HSR Rt value.
> -         */
> -        if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
> -             dabt.write )
> -        {
> -            rc = decode_instruction(regs, &info.dabt);
> -            if ( rc )
> -            {
> -                gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
> -                goto bad_data_abort;
> -            }
> -        }
> -
> -        if ( handle_mmio(&info) )
> +        if ( try_handle_mmio(regs, &info) )
>          {
>              advance_pc(regs, hsr);
>              return;
> @@ -2521,7 +2531,6 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>                  hsr.bits, dabt.dfsc);
>      }
>  
> -bad_data_abort:
>      gdprintk(XENLOG_DEBUG, "HSR=0x%x pc=%#"PRIregister" gva=%#"PRIvaddr
>               " gpa=%#"PRIpaddr"\n", hsr.bits, regs->pc, info.gva, info.gpa);
>      inject_dabt_exception(regs, info.gva, hsr.len);
> -- 
> 1.9.1
> 

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

Re: [for-4.8][PATCH v2 07/23] xen/arm: traps: Check the P2M before injecting a data/instruction abort

[Stefano Stabellini]

On Thu, 15 Sep 2016, Julien Grall wrote:
> A data/instruction abort may have occurred if another CPU was playing
> with the stage-2 page table when following the break-before-make
> sequence (see D4.7.1 in ARM DDI 0487A.j). Rather than injecting directly
> the fault to the guest, we need to check whether the mapping exists. If
> it exists, return to the guest to replay the instruction.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Changes in v2:
>         - Remove spurious change
>         - Fix typoes
> ---
>  xen/arch/arm/traps.c | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 76e4152..d73d29a 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2405,6 +2405,7 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>      register_t gva = READ_SYSREG(FAR_EL2);
>      uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK;
>      paddr_t gpa;
> +    mfn_t mfn;
>  
>      if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
>          gpa = get_faulting_ipa(gva);
> @@ -2418,6 +2419,11 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>           */
>          flush_tlb_local();
>  
> +        /*
> +         * We may not be able to translate because someone is
> +         * playing with the Stage-2 page table of the domain.
> +         * Return to the guest.
> +         */
>          rc = gva_to_ipa(gva, &gpa, GV2M_READ);
>          if ( rc == -EFAULT )
>              return; /* Try again */
> @@ -2438,8 +2444,17 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>          /* Trap was triggered by mem_access, work here is done */
>          if ( !rc )
>              return;
> +        break;
>      }
> -    break;
> +    case FSC_FLT_TRANS:
> +        /*
> +         * The PT walk may have failed because someone was playing
> +         * with the Stage-2 page table. Walk the Stage-2 PT to check
> +         * if the entry exists. If it's the case, return to the guest
> +         */
> +        mfn = p2m_lookup(current->domain, _gfn(paddr_to_pfn(gpa)), NULL);
> +        if ( !mfn_eq(mfn, INVALID_MFN) )
> +            return;
>      }
>  
>      inject_iabt_exception(regs, gva, hsr.len);
> @@ -2484,6 +2499,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>      int rc;
>      mmio_info_t info;
>      uint8_t fsc = hsr.dabt.dfsc & ~FSC_LL_MASK;
> +    mfn_t mfn;
>  
>      info.dabt = dabt;
>  #ifdef CONFIG_ARM_32
> @@ -2497,6 +2513,11 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>      else
>      {
>          rc = gva_to_ipa(info.gva, &info.gpa, GV2M_READ);
> +        /*
> +         * We may not be able to translate because someone is
> +         * playing with the Stage-2 page table of the domain.
> +         * Return to the guest.
> +         */
>          if ( rc == -EFAULT )
>              return; /* Try again */
>      }
> @@ -2520,11 +2541,25 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>          break;
>      }
>      case FSC_FLT_TRANS:
> +        /*
> +         * Attempt first to emulate the MMIO as the data abort will
> +         * likely happen in an emulated region.
> +         */
>          if ( try_handle_mmio(regs, &info) )
>          {
>              advance_pc(regs, hsr);
>              return;
>          }
> +
> +        /*
> +         * The PT walk may have failed because someone was playing
> +         * with the Stage-2 page table. Walk the Stage-2 PT to check
> +         * if the entry exists. If it's the case, return to the guest
> +         */
> +        mfn = p2m_lookup(current->domain, _gfn(paddr_to_pfn(info.gpa)), NULL);
> +        if ( !mfn_eq(mfn, INVALID_MFN) )
> +            return;
> +
>          break;
>      default:
>          gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n",
> -- 
> 1.9.1
> 

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

Re: [for-4.8][PATCH v2 09/23] xen/arm: p2m: Change the type of level_shifts from paddr_t to uint8_t

[Stefano Stabellini]

On Thu, 15 Sep 2016, Julien Grall wrote:
> The level shift can be encoded with 8-bit. So it is not necessary to
> use paddr_t (i.e 64-bit).
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Changes in v2:
>         - Use uint8_t rather than unsigned int
>         - Replaced paddr_t by uint8_t in p2m_shatter_page
> ---
>  xen/arch/arm/p2m.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index b53d4cf..b4f75e8 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -687,14 +687,14 @@ static const paddr_t level_sizes[] =
>      { ZEROETH_SIZE, FIRST_SIZE, SECOND_SIZE, THIRD_SIZE };
>  static const paddr_t level_masks[] =
>      { ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK };
> -static const paddr_t level_shifts[] =
> +static const uint8_t level_shifts[] =
>      { ZEROETH_SHIFT, FIRST_SHIFT, SECOND_SHIFT, THIRD_SHIFT };
>  
>  static int p2m_shatter_page(struct p2m_domain *p2m,
>                              lpae_t *entry,
>                              unsigned int level)
>  {
> -    const paddr_t level_shift = level_shifts[level];
> +    const uint8_t level_shift = level_shifts[level];
>      int rc = p2m_create_table(p2m, entry, level_shift - PAGE_SHIFT);
>  
>      if ( !rc )
> -- 
> 1.9.1
> 

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

Re: [for-4.8][PATCH v2 11/23] xen/arm: p2m: Introduce p2m_get_root_pointer and use it in __p2m_lookup

[Stefano Stabellini]

On Thu, 15 Sep 2016, Julien Grall wrote:
> Mapping the root table is always done the same way. To avoid duplicating
> the code in a later patch, move the code in a separate helper.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Changes in v2:
>         - Use level_orders rather than level_shifts - PAGE_SHIFT
>         - Move the definition of level_orders in this patch
>             * use uint8_t rather than unsigned
>             * define *_ORDER in term of *_SHIFT
> ---
>  xen/arch/arm/p2m.c         | 55 +++++++++++++++++++++++++++++++---------------
>  xen/include/asm-arm/page.h |  4 ++++
>  2 files changed, 41 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 413780b..b2a29ad 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -36,6 +36,8 @@ static const paddr_t level_masks[] =
>      { ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK };
>  static const uint8_t level_shifts[] =
>      { ZEROETH_SHIFT, FIRST_SHIFT, SECOND_SHIFT, THIRD_SHIFT };
> +static const uint8_t level_orders[] =
> +    { ZEROETH_ORDER, FIRST_ORDER, SECOND_ORDER, THIRD_ORDER };
>  
>  static bool_t p2m_valid(lpae_t pte)
>  {
> @@ -204,6 +206,37 @@ static void p2m_flush_tlb_sync(struct p2m_domain *p2m)
>  }
>  
>  /*
> + * Find and map the root page table. The caller is responsible for
> + * unmapping the table.
> + *
> + * The function will return NULL if the offset of the root table is
> + * invalid.
> + */
> +static lpae_t *p2m_get_root_pointer(struct p2m_domain *p2m,
> +                                    gfn_t gfn)
> +{
> +    unsigned int root_table;
> +
> +    if ( P2M_ROOT_PAGES == 1 )
> +        return __map_domain_page(p2m->root);
> +
> +    /*
> +     * Concatenated root-level tables. The table number will be the
> +     * offset at the previous level. It is not possible to
> +     * concatenate a level-0 root.
> +     */
> +    ASSERT(P2M_ROOT_LEVEL > 0);
> +
> +    root_table = gfn_x(gfn) >> (level_orders[P2M_ROOT_LEVEL - 1]);
> +    root_table &= LPAE_ENTRY_MASK;
> +
> +    if ( root_table >= P2M_ROOT_PAGES )
> +        return NULL;
> +
> +    return __map_domain_page(p2m->root + root_table);
> +}
> +
> +/*
>   * Lookup the MFN corresponding to a domain's GFN.
>   *
>   * There are no processor functions to do a stage 2 only lookup therefore we
> @@ -226,7 +259,7 @@ static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
>      mfn_t mfn = INVALID_MFN;
>      paddr_t mask = 0;
>      p2m_type_t _t;
> -    unsigned int level, root_table;
> +    unsigned int level;
>  
>      ASSERT(p2m_is_locked(p2m));
>      BUILD_BUG_ON(THIRD_MASK != PAGE_MASK);
> @@ -236,22 +269,9 @@ static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
>  
>      *t = p2m_invalid;
>  
> -    if ( P2M_ROOT_PAGES > 1 )
> -    {
> -        /*
> -         * Concatenated root-level tables. The table number will be
> -         * the offset at the previous level. It is not possible to
> -         * concatenate a level-0 root.
> -         */
> -        ASSERT(P2M_ROOT_LEVEL > 0);
> -        root_table = offsets[P2M_ROOT_LEVEL - 1];
> -        if ( root_table >= P2M_ROOT_PAGES )
> -            goto err;
> -    }
> -    else
> -        root_table = 0;
> -
> -    map = __map_domain_page(p2m->root + root_table);
> +    map = p2m_get_root_pointer(p2m, gfn);
> +    if ( !map )
> +        return INVALID_MFN;
>  
>      ASSERT(P2M_ROOT_LEVEL < 4);
>  
> @@ -286,7 +306,6 @@ static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
>          *t = pte.p2m.type;
>      }
>  
> -err:
>      return mfn;
>  }
>  
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 05d9f82..a43b0fa 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -457,15 +457,19 @@ static inline int gva_to_ipa(vaddr_t va, paddr_t *paddr, unsigned int flags)
>  #define LPAE_ENTRY_MASK (LPAE_ENTRIES - 1)
>  
>  #define THIRD_SHIFT    (PAGE_SHIFT)
> +#define THIRD_ORDER    (THIRD_SHIFT - PAGE_SHIFT)
>  #define THIRD_SIZE     ((paddr_t)1 << THIRD_SHIFT)
>  #define THIRD_MASK     (~(THIRD_SIZE - 1))
>  #define SECOND_SHIFT   (THIRD_SHIFT + LPAE_SHIFT)
> +#define SECOND_ORDER   (SECOND_SHIFT - PAGE_SHIFT)
>  #define SECOND_SIZE    ((paddr_t)1 << SECOND_SHIFT)
>  #define SECOND_MASK    (~(SECOND_SIZE - 1))
>  #define FIRST_SHIFT    (SECOND_SHIFT + LPAE_SHIFT)
> +#define FIRST_ORDER    (FIRST_SHIFT - PAGE_SHIFT)
>  #define FIRST_SIZE     ((paddr_t)1 << FIRST_SHIFT)
>  #define FIRST_MASK     (~(FIRST_SIZE - 1))
>  #define ZEROETH_SHIFT  (FIRST_SHIFT + LPAE_SHIFT)
> +#define ZEROETH_ORDER  (ZEROETH_SHIFT - PAGE_SHIFT)
>  #define ZEROETH_SIZE   ((paddr_t)1 << ZEROETH_SHIFT)
>  #define ZEROETH_MASK   (~(ZEROETH_SIZE - 1))
>  
> -- 
> 1.9.1
> 

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

Re: [for-4.8][PATCH v2 12/23] xen/arm: p2m: Introduce p2m_get_entry and use it to implement __p2m_lookup

[Stefano Stabellini]

On Thu, 15 Sep 2016, Julien Grall wrote:
> Currently, for a given GFN, the function __p2m_lookup will only return
> the associated MFN and the p2m type of the mapping.
> 
> In some case we need the order of the mapping and the memaccess
> permission. Rather than providing a separate function for this purpose,
> it is better to implement a generic function to return all the
> information.
> 
> To avoid passing dummy parameter, a caller that does not need a
> specific information can use NULL instead.
> 
> The list of the informations retrieved is based on the x86 version. All
> of them will be used in follow-up patches.
> 
> It might have been possible to extend __p2m_lookup, however I choose to
> reimplement it from scratch to allow sharing some helpers with the
> function that will update the P2M (will be added in a follow-up patch).
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Changes in v2:
>         - Export p2m_get_entry
>         - Fix the computation of the order when there is no mapping
>         - Use level_orders rather than level_shifts - PAGE_SHIFT
>         - Update documentation
>         - Fix typoes
>         - The definition of level_orders has been moved in an earlier
>         patch
> ---
>  xen/arch/arm/p2m.c        | 188 +++++++++++++++++++++++++++++++++++-----------
>  xen/include/asm-arm/p2m.h |   8 ++
>  2 files changed, 154 insertions(+), 42 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index b2a29ad..6e56b97 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -238,28 +238,104 @@ static lpae_t *p2m_get_root_pointer(struct p2m_domain *p2m,
>  
>  /*
>   * Lookup the MFN corresponding to a domain's GFN.
> + * Lookup mem access in the ratrix tree.
> + * The entries associated to the GFN is considered valid.
> + */
> +static p2m_access_t p2m_mem_access_radix_get(struct p2m_domain *p2m, gfn_t gfn)
> +{
> +    void *ptr;
> +
> +    if ( !p2m->mem_access_enabled )
> +        return p2m->default_access;
> +
> +    ptr = radix_tree_lookup(&p2m->mem_access_settings, gfn_x(gfn));
> +    if ( !ptr )
> +        return p2m_access_rwx;
> +    else
> +        return radix_tree_ptr_to_int(ptr);
> +}
> +
> +#define GUEST_TABLE_MAP_FAILED 0
> +#define GUEST_TABLE_SUPER_PAGE 1
> +#define GUEST_TABLE_NORMAL_PAGE 2
> +
> +static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry,
> +                            int level_shift);
> +
> +/*
> + * Take the currently mapped table, find the corresponding GFN entry,
> + * and map the next table, if available. The previous table will be
> + * unmapped if the next level was mapped (e.g GUEST_TABLE_NORMAL_PAGE
> + * returned).
>   *
> - * There are no processor functions to do a stage 2 only lookup therefore we
> - * do a a software walk.
> + * The read_only parameters indicates whether intermediate tables should
> + * be allocated when not present.
> + *
> + * Return values:
> + *  GUEST_TABLE_MAP_FAILED: Either read_only was set and the entry
> + *  was empty, or allocating a new page failed.
> + *  GUEST_TABLE_NORMAL_PAGE: next level mapped normally
> + *  GUEST_TABLE_SUPER_PAGE: The next entry points to a superpage.
>   */
> -static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
> +static int p2m_next_level(struct p2m_domain *p2m, bool read_only,
> +                          lpae_t **table, unsigned int offset)
>  {
> -    struct p2m_domain *p2m = &d->arch.p2m;
> -    const paddr_t paddr = pfn_to_paddr(gfn_x(gfn));
> -    const unsigned int offsets[4] = {
> -        zeroeth_table_offset(paddr),
> -        first_table_offset(paddr),
> -        second_table_offset(paddr),
> -        third_table_offset(paddr)
> -    };
> -    const paddr_t masks[4] = {
> -        ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK
> -    };
> -    lpae_t pte, *map;
> +    lpae_t *entry;
> +    int ret;
> +    mfn_t mfn;
> +
> +    entry = *table + offset;
> +
> +    if ( !p2m_valid(*entry) )
> +    {
> +        if ( read_only )
> +            return GUEST_TABLE_MAP_FAILED;
> +
> +        ret = p2m_create_table(p2m, entry, /* not used */ ~0);
> +        if ( ret )
> +            return GUEST_TABLE_MAP_FAILED;
> +    }
> +
> +    /* The function p2m_next_level is never called at the 3rd level */
> +    if ( p2m_mapping(*entry) )
> +        return GUEST_TABLE_SUPER_PAGE;
> +
> +    mfn = _mfn(entry->p2m.base);
> +
> +    unmap_domain_page(*table);
> +    *table = map_domain_page(mfn);
> +
> +    return GUEST_TABLE_NORMAL_PAGE;
> +}
> +
> +/*
> + * Get the details of a given gfn.
> + *
> + * If the entry is present, the associated MFN will be returned and the
> + * access and type filled up. The page_order will correspond to the
> + * order of the mapping in the page table (i.e it could be a superpage).
> + *
> + * If the entry is not present, INVALID_MFN will be returned and the
> + * page_order will be set according to the order of the invalid range.
> + */
> +mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
> +                    p2m_type_t *t, p2m_access_t *a,
> +                    unsigned int *page_order)
> +{
> +    paddr_t addr = pfn_to_paddr(gfn_x(gfn));
> +    unsigned int level = 0;
> +    lpae_t entry, *table;
> +    int rc;
>      mfn_t mfn = INVALID_MFN;
> -    paddr_t mask = 0;
>      p2m_type_t _t;
> -    unsigned int level;
> +
> +    /* Convenience aliases */
> +    const unsigned int offsets[4] = {
> +        zeroeth_table_offset(addr),
> +        first_table_offset(addr),
> +        second_table_offset(addr),
> +        third_table_offset(addr)
> +    };
>  
>      ASSERT(p2m_is_locked(p2m));
>      BUILD_BUG_ON(THIRD_MASK != PAGE_MASK);
> @@ -269,46 +345,74 @@ static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
>  
>      *t = p2m_invalid;
>  
> -    map = p2m_get_root_pointer(p2m, gfn);
> -    if ( !map )
> -        return INVALID_MFN;
> -
> -    ASSERT(P2M_ROOT_LEVEL < 4);
> +    /* XXX: Check if the mapping is lower than the mapped gfn */
>  
> -    for ( level = P2M_ROOT_LEVEL ; level < 4 ; level++ )
> +    /* This gfn is higher than the highest the p2m map currently holds */
> +    if ( gfn_x(gfn) > gfn_x(p2m->max_mapped_gfn) )
>      {
> -        mask = masks[level];
> +        for ( level = P2M_ROOT_LEVEL; level < 3; level++ )
> +            if ( (gfn_x(gfn) & (level_masks[level] >> PAGE_SHIFT)) >
> +                 gfn_x(p2m->max_mapped_gfn) )
> +                break;
>  
> -        pte = map[offsets[level]];
> +        goto out;
> +    }
>  
> -        if ( level == 3 && !p2m_table(pte) )
> -            /* Invalid, clobber the pte */
> -            pte.bits = 0;
> -        if ( level == 3 || !p2m_table(pte) )
> -            /* Done */
> -            break;
> +    table = p2m_get_root_pointer(p2m, gfn);
>  
> -        ASSERT(level < 3);
> +    /*
> +     * the table should always be non-NULL because the gfn is below
> +     * p2m->max_mapped_gfn and the root table pages are always present.
> +     */
> +    BUG_ON(table == NULL);
>  
> -        /* Map for next level */
> -        unmap_domain_page(map);
> -        map = map_domain_page(_mfn(pte.p2m.base));
> +    for ( level = P2M_ROOT_LEVEL; level < 3; level++ )
> +    {
> +        rc = p2m_next_level(p2m, true, &table, offsets[level]);
> +        if ( rc == GUEST_TABLE_MAP_FAILED )
> +            goto out_unmap;
> +        else if ( rc != GUEST_TABLE_NORMAL_PAGE )
> +            break;
>      }
>  
> -    unmap_domain_page(map);
> +    entry = table[offsets[level]];
>  
> -    if ( p2m_valid(pte) )
> +    if ( p2m_valid(entry) )
>      {
> -        ASSERT(mask);
> -        ASSERT(pte.p2m.type != p2m_invalid);
> -        mfn = _mfn(paddr_to_pfn((pte.bits & PADDR_MASK & mask) |
> -                                (paddr & ~mask)));
> -        *t = pte.p2m.type;
> +        *t = entry.p2m.type;
> +
> +        if ( a )
> +            *a = p2m_mem_access_radix_get(p2m, gfn);
> +
> +        mfn = _mfn(entry.p2m.base);
> +        /*
> +         * The entry may point to a superpage. Find the MFN associated
> +         * to the GFN.
> +         */
> +        mfn = mfn_add(mfn, gfn_x(gfn) & ((1UL << level_orders[level]) - 1));
>      }
>  
> +out_unmap:
> +    unmap_domain_page(table);
> +
> +out:
> +    if ( page_order )
> +        *page_order = level_orders[level];
> +
>      return mfn;
>  }
>  
> +/*
> + * Lookup the MFN corresponding to a domain's GFN.
> + *
> + * There are no processor functions to do a stage 2 only lookup therefore we
> + * do a a software walk.
> + */
> +static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
> +{
> +    return p2m_get_entry(&d->arch.p2m, gfn, t, NULL, NULL);
> +}
> +
>  mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
>  {
>      mfn_t ret;
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 156df5e..6fe6a37 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -179,6 +179,14 @@ void p2m_dump_info(struct domain *d);
>  /* Look up the MFN corresponding to a domain's GFN. */
>  mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
>  
> +/*
> + * Get details of a given gfn.
> + * The P2M lock should be taken by the caller.
> + */
> +mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
> +                    p2m_type_t *t, p2m_access_t *a,
> +                    unsigned int *page_order);
> +
>  /* Clean & invalidate caches corresponding to a region of guest address space */
>  int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr);
>  
> -- 
> 1.9.1
> 

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

Re: [for-4.8][PATCH v2 14/23] xen/arm: p2m: Re-implement p2m_cache_flush using p2m_get_entry

[Stefano Stabellini]

On Thu, 15 Sep 2016, Julien Grall wrote:
> The function p2m_cache_flush can be re-implemented using the generic
> function p2m_get_entry by iterating over the range and using the mapping
> order given by the callee.
> 
> As the current implementation, no preemption is implemented, although
> the comment in the current code claimed it. As the function is called by
> a DOMCTL with a region of 1GB maximum, I think the preemption can be
> left unimplemented for now.
> 
> Finally drop the operation CACHEFLUSH in apply_one_level as nobody is
> using it anymore. Note that the function could have been dropped in one
> go at the end, however I find easier to drop the operations one by one
> avoiding a big deletion in the patch that convert the last operation.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     The loop pattern will be very similar for the reliquish function.
>     It might be possible to extract it in a separate function.
> 
>     Changes in v2:
>         - Introduce and use gfn_next_boundary
>         - Flush all the mapping in a superpage rather than page by page.
>         - Update doc
> ---
>  xen/arch/arm/p2m.c | 83 ++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 50 insertions(+), 33 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index ddee258..fa58f1a 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -62,6 +62,22 @@ static inline void p2m_write_lock(struct p2m_domain *p2m)
>      write_lock(&p2m->lock);
>  }
>  
> +/*
> + * Return the start of the next mapping based on the order of the
> + * current one.
> + */
> +static inline gfn_t gfn_next_boundary(gfn_t gfn, unsigned int order)
> +{
> +    /*
> +     * The order corresponds to the order of the mapping (or invalid
> +     * range) in the page table. So we need to align the GFN before
> +     * incrementing.
> +     */
> +    gfn = _gfn(gfn_x(gfn) & ~((1UL << order) - 1));
> +
> +    return gfn_add(gfn, 1UL << order);
> +}
> +
>  static void p2m_flush_tlb(struct p2m_domain *p2m);
>  
>  static inline void p2m_write_unlock(struct p2m_domain *p2m)
> @@ -734,7 +750,6 @@ enum p2m_operation {
>      INSERT,
>      REMOVE,
>      RELINQUISH,
> -    CACHEFLUSH,
>      MEMACCESS,
>  };
>  
> @@ -993,36 +1008,6 @@ static int apply_one_level(struct domain *d,
>           */
>          return P2M_ONE_PROGRESS;
>  
> -    case CACHEFLUSH:
> -        if ( !p2m_valid(orig_pte) )
> -        {
> -            *addr = (*addr + level_size) & level_mask;
> -            return P2M_ONE_PROGRESS_NOP;
> -        }
> -
> -        if ( level < 3 && p2m_table(orig_pte) )
> -            return P2M_ONE_DESCEND;
> -
> -        /*
> -         * could flush up to the next superpage boundary, but would
> -         * need to be careful about preemption, so just do one 4K page
> -         * now and return P2M_ONE_PROGRESS{,_NOP} so that the caller will
> -         * continue to loop over the rest of the range.
> -         */
> -        if ( p2m_is_ram(orig_pte.p2m.type) )
> -        {
> -            unsigned long offset = paddr_to_pfn(*addr & ~level_mask);
> -            flush_page_to_ram(orig_pte.p2m.base + offset);
> -
> -            *addr += PAGE_SIZE;
> -            return P2M_ONE_PROGRESS;
> -        }
> -        else
> -        {
> -            *addr += PAGE_SIZE;
> -            return P2M_ONE_PROGRESS_NOP;
> -        }
> -
>      case MEMACCESS:
>          if ( level < 3 )
>          {
> @@ -1571,12 +1556,44 @@ int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr)
>  {
>      struct p2m_domain *p2m = &d->arch.p2m;
>      gfn_t end = gfn_add(start, nr);
> +    gfn_t next_gfn;
> +    p2m_type_t t;
> +    unsigned int order;
>  
>      start = gfn_max(start, p2m->lowest_mapped_gfn);
>      end = gfn_min(end, p2m->max_mapped_gfn);
>  
> -    return apply_p2m_changes(d, CACHEFLUSH, start, nr, INVALID_MFN,
> -                             0, p2m_invalid, d->arch.p2m.default_access);
> +    /*
> +     * The operation cache flush will invalidate the RAM assigned to the
> +     * guest in a given range. It will not modify the page table and
> +     * flushing the cache whilst the page is used by another CPU is
> +     * fine. So using read-lock is fine here.
> +     */
> +    p2m_read_lock(p2m);
> +
> +    for ( ; gfn_x(start) < gfn_x(end); start = next_gfn )
> +    {
> +        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order);
> +
> +        next_gfn = gfn_next_boundary(start, order);
> +
> +        /* Skip hole and non-RAM page */
> +        if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_ram(t) )
> +            continue;
> +
> +        /* XXX: Implement preemption */
> +        while ( gfn_x(start) < gfn_x(next_gfn) )
> +        {
> +            flush_page_to_ram(mfn_x(mfn));
> +
> +            start = gfn_add(start, 1);
> +            mfn = mfn_add(mfn, 1);
> +        }
> +    }
> +
> +    p2m_read_unlock(p2m);
> +
> +    return 0;
>  }
>  
>  mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn)
> -- 
> 1.9.1
> 

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

Re: [for-4.8][PATCH v2 18/23] xen/arm: p2m: Re-implement relinquish_p2m_mapping using p2m_{get, set}_entry

[Stefano Stabellini]

On Thu, 15 Sep 2016, Julien Grall wrote:
> The function relinquish_p2m_mapping can be re-implemented using
> p2m_{get,set}_entry by iterating over the range mapped and using the
> mapping order given by the callee.
> 
> Given that the preemption was chosen arbitrarily, it is now done on every
> 512 iterations. Meaning that Xen may check more often if the function is
> preempted when there are no mappings.
> 
> Finally drop the operation RELINQUISH in apply_* as nobody is using it
> anymore. Note that the functions could have been dropped in one go at
> the end, however I find easier to drop the operations one by one
> avoiding a big deletion in the patch that remove the last operation.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Changes in v2:
>         - Erase entry one by one as I have not time so far to check
>         whether it is possible to avoid removing entry in the p2m.
>         - Use gfn_next_boundary
> ---
>  xen/arch/arm/p2m.c | 77 +++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 59 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 5f7aef0..ce09c4c 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -754,7 +754,6 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn,
>  enum p2m_operation {
>      INSERT,
>      REMOVE,
> -    RELINQUISH,
>      MEMACCESS,
>  };
>  
> @@ -1318,7 +1317,6 @@ static int apply_one_level(struct domain *d,
>  
>          break;
>  
> -    case RELINQUISH:
>      case REMOVE:
>          if ( !p2m_valid(orig_pte) )
>          {
> @@ -1502,17 +1500,6 @@ static int apply_p2m_changes(struct domain *d,
>          {
>              switch ( op )
>              {
> -            case RELINQUISH:
> -                /*
> -                 * Arbitrarily, preempt every 512 operations or 8192 nops.
> -                 * 512*P2M_ONE_PROGRESS == 8192*P2M_ONE_PROGRESS_NOP == 0x2000
> -                 * This is set in preempt_count_limit.
> -                 *
> -                 */
> -                p2m->lowest_mapped_gfn = _gfn(addr >> PAGE_SHIFT);
> -                rc = -ERESTART;
> -                goto out;
> -
>              case MEMACCESS:
>              {
>                  /*
> @@ -1919,16 +1906,70 @@ int p2m_init(struct domain *d)
>      return rc;
>  }
>  
> +/*
> + * The function will go through the p2m and remove page reference when it
> + * is required. The mapping will be removed from the p2m.
> + *
> + * XXX: See whether the mapping can be left intact in the p2m.
> + */
>  int relinquish_p2m_mapping(struct domain *d)
>  {
>      struct p2m_domain *p2m = &d->arch.p2m;
> -    unsigned long nr;
> +    unsigned long count = 0;
> +    p2m_type_t t;
> +    int rc = 0;
> +    unsigned int order;
> +
> +    /* Convenience alias */
> +    gfn_t start = p2m->lowest_mapped_gfn;
> +    gfn_t end = p2m->max_mapped_gfn;
> +
> +    p2m_write_lock(p2m);
> +
> +    for ( ; gfn_x(start) < gfn_x(end);
> +          start = gfn_next_boundary(start, order) )
> +    {
> +        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order);
> +
> +        count++;
> +        /*
> +         * Arbitrarily preempt every 512 iterations.
> +         */
> +        if ( !(count % 512) && hypercall_preempt_check() )
> +        {
> +            rc = -ERESTART;
> +            break;
> +        }
>  
> -    nr = gfn_x(p2m->max_mapped_gfn) - gfn_x(p2m->lowest_mapped_gfn);
> +        /*
> +         * p2m_set_entry will take care of removing reference on page
> +         * when it is necessary and removing the mapping in the p2m.
> +         */
> +        if ( !mfn_eq(mfn, INVALID_MFN) )
> +        {
> +            /*
> +             * For valid mapping, the start will always be aligned as
> +             * entry will be removed whilst relinquishing.
> +             */
> +            rc = __p2m_set_entry(p2m, start, order, INVALID_MFN,
> +                                 p2m_invalid, p2m_access_rwx);
> +            if ( unlikely(rc) )
> +            {
> +                printk(XENLOG_G_ERR "Unable to remove mapping gfn=%#"PRI_gfn" order=%u from the p2m of domain %d\n", gfn_x(start), order, d->domain_id);
> +                break;
> +            }
> +        }
> +    }
>  
> -    return apply_p2m_changes(d, RELINQUISH, p2m->lowest_mapped_gfn, nr,
> -                             INVALID_MFN, 0, p2m_invalid,
> -                             d->arch.p2m.default_access);
> +    /*
> +     * Update lowest_mapped_gfn so on the next call we still start where
> +     * we stopped.
> +     */
> +    p2m->lowest_mapped_gfn = start;
> +
> +    p2m_write_unlock(p2m);
> +
> +    return rc;
>  }
>  
>  int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr)
> -- 
> 1.9.1
> 

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

Re: [for-4.8][PATCH v2 17/23] xen/arm: p2m: Introduce p2m_set_entry and __p2m_set_entry

[Stefano Stabellini]

On Thu, 15 Sep 2016, Julien Grall wrote:
> The ARM architecture mandates to use of a break-before-make sequence
> when changing translation entries if the page table is shared between
> multiple CPUs whenever a valid entry is replaced by another valid entry
> (see D4.7.1 in ARM DDI 0487A.j for more details).
> 
> The break-before-make sequence can be divided in the following steps:
>     1) Invalidate the old entry in the page table
>     2) Issue a TLB invalidation instruction for the address associated
>     to this entry
>     3) Write the new entry
> 
> The current P2M code implemented in apply_one_level does not respect
> this sequence and may result to break coherency on some processors.
> 
> Adapting the current implementation to use the break-before-make
> sequence would imply some code duplication and more TLBs invalidation
> than necessary. For instance, if we are replacing a 4KB page and the
> current mapping in the P2M is using a 1GB superpage, the following steps
> will happen:
>     1) Shatter the 1GB superpage into a series of 2MB superpages
>     2) Shatter the 2MB superpage into a series of 4KB pages
>     3) Replace the 4KB page
> 
> As the current implementation is shattering while descending and install
> the mapping, Xen would need to issue 3 TLB invalidation instructions
> which is clearly inefficient.
> 
> Furthermore, all the operations which modify the page table are using
> the same skeleton. It is more complicated to maintain different code paths
> than having a generic function that set an entry and take care of the
> break-before-make sequence.
> 
> The new implementation is based on the x86 EPT one which, I think,
> fits quite well for the break-before-make sequence whilst keeping
> the code simple.
> 
> The main function of the new implementation is __p2m_set_entry. It will
> only work on mapping that are aligned to a block entry in the page table
> (i.e 1GB, 2MB, 4KB when using a 4KB granularity).
> 
> Another function, p2m_set_entry, is provided to break down is region
> into mapping that is aligned to a block entry or 4KB when memaccess is
> enabled.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     Changes in v2:
>         - fix typoes in the commit message
>         - don't use the default access when shattering the superpage
>         - remove duplicated comment
>         - export p2m_set_entry
>         - s/todo/nr/
>         - iommu flush
>         - update the stats when removing/adding a mapping
>         - update doc
>         - fix p2m_split_superpage
>         - don't try to allocate intermediate page table when removing an
>         entry
>         - the TLB flush is not necessary when only permission are
>         changed (e.g memaccess).
> ---
>  xen/arch/arm/p2m.c         | 374 +++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/p2m.h  |  11 ++
>  xen/include/asm-arm/page.h |   4 +
>  3 files changed, 389 insertions(+)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 3ca2e90..5f7aef0 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -783,6 +783,380 @@ static void p2m_put_l3_page(const lpae_t pte)
>      }
>  }
>  
> +/* Free lpae sub-tree behind an entry */
> +static void p2m_free_entry(struct p2m_domain *p2m,
> +                           lpae_t entry, unsigned int level)
> +{
> +    unsigned int i;
> +    lpae_t *table;
> +    mfn_t mfn;
> +
> +    /* Nothing to do if the entry is invalid. */
> +    if ( !p2m_valid(entry) )
> +        return;
> +
> +    /* Nothing to do but updating the states if the entry is a super-page. */
                                           ^ stats

Aside from this small typo, everything else is good:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> +    if ( p2m_is_superpage(entry, level) )
> +    {
> +        p2m->stats.mappings[level]--;
> +        return;
> +    }
> +
> +    if ( level == 3 )
> +    {
> +        p2m->stats.mappings[level]--;
> +        p2m_put_l3_page(entry);
> +        return;
> +    }
> +
> +    table = map_domain_page(_mfn(entry.p2m.base));
> +    for ( i = 0; i < LPAE_ENTRIES; i++ )
> +        p2m_free_entry(p2m, *(table + i), level + 1);
> +
> +    unmap_domain_page(table);
> +
> +    /*
> +     * Make sure all the references in the TLB have been removed before
> +     * freing the intermediate page table.
> +     * XXX: Should we defer the free of the page table to avoid the
> +     * flush?
> +     */
> +    if ( p2m->need_flush )
> +        p2m_flush_tlb_sync(p2m);
> +
> +    mfn = _mfn(entry.p2m.base);
> +    ASSERT(mfn_valid(mfn_x(mfn)));
> +
> +    free_domheap_page(mfn_to_page(mfn_x(mfn)));
> +}
> +
> +static bool p2m_split_superpage(struct p2m_domain *p2m, lpae_t *entry,
> +                                unsigned int level, unsigned int target,
> +                                const unsigned int *offsets)
> +{
> +    struct page_info *page;
> +    unsigned int i;
> +    lpae_t pte, *table;
> +    bool rv = true;
> +
> +    /* Convenience aliases */
> +    mfn_t mfn = _mfn(entry->p2m.base);
> +    unsigned int next_level = level + 1;
> +    unsigned int level_order = level_orders[next_level];
> +
> +    /*
> +     * This should only be called with target != level and the entry is
> +     * a superpage.
> +     */
> +    ASSERT(level < target);
> +    ASSERT(p2m_is_superpage(*entry, level));
> +
> +    page = alloc_domheap_page(NULL, 0);
> +    if ( !page )
> +        return false;
> +
> +    page_list_add(page, &p2m->pages);
> +    table = __map_domain_page(page);
> +
> +    /*
> +     * We are either splitting a first level 1G page into 512 second level
> +     * 2M pages, or a second level 2M page into 512 third level 4K pages.
> +     */
> +    for ( i = 0; i < LPAE_ENTRIES; i++ )
> +    {
> +        lpae_t *new_entry = table + i;
> +
> +        /*
> +         * Use the content of the superpage entry and override
> +         * the necessary fields. So the correct permission are kept.
> +         */
> +        pte = *entry;
> +        pte.p2m.base = mfn_x(mfn_add(mfn, i << level_order));
> +
> +        /*
> +         * First and second level pages set p2m.table = 0, but third
> +         * level entries set p2m.table = 1.
> +         */
> +        pte.p2m.table = (next_level == 3);
> +
> +        write_pte(new_entry, pte);
> +    }
> +
> +    /* Update stats */
> +    p2m->stats.shattered[level]++;
> +    p2m->stats.mappings[level]--;
> +    p2m->stats.mappings[next_level] += LPAE_ENTRIES;
> +
> +    /*
> +     * Shatter superpage in the page to the level we want to make the
> +     * changes.
> +     * This is done outside the loop to avoid checking the offset to
> +     * know whether the entry should be shattered for every entry.
> +     */
> +    if ( next_level != target )
> +        rv = p2m_split_superpage(p2m, table + offsets[next_level],
> +                                 level + 1, target, offsets);
> +
> +    if ( p2m->clean_pte )
> +        clean_dcache_va_range(table, PAGE_SIZE);
> +
> +    unmap_domain_page(table);
> +
> +    pte = mfn_to_p2m_entry(_mfn(page_to_mfn(page)), p2m_invalid,
> +                           p2m->default_access);
> +
> +    /*
> +     * Even if we failed, we should install the newly allocated LPAE
> +     * entry. The caller will be in charge to free the sub-tree.
> +     */
> +    p2m_write_pte(entry, pte, p2m->clean_pte);
> +
> +    return rv;
> +}
> +
> +/*
> + * Insert an entry in the p2m. This should be called with a mapping
> + * equal to a page/superpage (4K, 2M, 1G).
> + */
> +static int __p2m_set_entry(struct p2m_domain *p2m,
> +                           gfn_t sgfn,
> +                           unsigned int page_order,
> +                           mfn_t smfn,
> +                           p2m_type_t t,
> +                           p2m_access_t a)
> +{
> +    paddr_t addr = pfn_to_paddr(gfn_x(sgfn));
> +    unsigned int level = 0;
> +    unsigned int target = 3 - (page_order / LPAE_SHIFT);
> +    lpae_t *entry, *table, orig_pte;
> +    int rc;
> +
> +    /* Convenience aliases */
> +    const unsigned int offsets[4] = {
> +        zeroeth_table_offset(addr),
> +        first_table_offset(addr),
> +        second_table_offset(addr),
> +        third_table_offset(addr)
> +    };
> +
> +    ASSERT(p2m_is_write_locked(p2m));
> +
> +    /*
> +     * Check if the level target is valid: we only support
> +     * 4K - 2M - 1G mapping.
> +     */
> +    ASSERT(target > 0 && target <= 3);
> +
> +    table = p2m_get_root_pointer(p2m, sgfn);
> +    if ( !table )
> +        return -EINVAL;
> +
> +    for ( level = P2M_ROOT_LEVEL; level < target; level++ )
> +    {
> +        /*
> +         * Don't try to allocate intermediate page table if the mapping
> +         * is about to be removed (i.e mfn == INVALID_MFN).
> +         */
> +        rc = p2m_next_level(p2m, mfn_eq(smfn, INVALID_MFN),
> +                            &table, offsets[level]);
> +        if ( rc == GUEST_TABLE_MAP_FAILED )
> +        {
> +            /*
> +             * We are here because p2m_next_level has failed to map
> +             * the intermediate page table (e.g the table does not exist
> +             * and they p2m tree is read-only). It is a valid case
> +             * when removing a mapping as it may not exist in the
> +             * page table. In this case, just ignore it.
> +             */
> +            rc = mfn_eq(smfn, INVALID_MFN) ? 0 : -ENOENT;
> +            goto out;
> +        }
> +        else if ( rc != GUEST_TABLE_NORMAL_PAGE )
> +            break;
> +    }
> +
> +    entry = table + offsets[level];
> +
> +    /*
> +     * If we are here with level < target, we must be at a leaf node,
> +     * and we need to break up the superpage.
> +     */
> +    if ( level < target )
> +    {
> +        /* We need to split the original page. */
> +        lpae_t split_pte = *entry;
> +
> +        ASSERT(p2m_is_superpage(*entry, level));
> +
> +        if ( !p2m_split_superpage(p2m, &split_pte, level, target, offsets) )
> +        {
> +            /*
> +             * The current super-page is still in-place, so re-increment
> +             * the stats.
> +             */
> +            p2m->stats.mappings[level]++;
> +
> +            /* Free the allocated sub-tree */
> +            p2m_free_entry(p2m, split_pte, level);
> +
> +            rc = -ENOMEM;
> +            goto out;
> +        }
> +
> +        /*
> +         * Follow the break-before-sequence to update the entry.
> +         * For more details see (D4.7.1 in ARM DDI 0487A.j).
> +         */
> +        p2m_remove_pte(entry, p2m->clean_pte);
> +        p2m_flush_tlb_sync(p2m);
> +
> +        p2m_write_pte(entry, split_pte, p2m->clean_pte);
> +
> +        /* then move to the level we want to make real changes */
> +        for ( ; level < target; level++ )
> +        {
> +            rc = p2m_next_level(p2m, true, &table, offsets[level]);
> +
> +            /*
> +             * The entry should be found and either be a table
> +             * or a superpage if level 3 is not targeted
> +             */
> +            ASSERT(rc == GUEST_TABLE_NORMAL_PAGE ||
> +                   (rc == GUEST_TABLE_SUPER_PAGE && target < 3));
> +        }
> +
> +        entry = table + offsets[level];
> +    }
> +
> +    /*
> +     * We should always be there with the correct level because
> +     * all the intermediate tables have been installed if necessary.
> +     */
> +    ASSERT(level == target);
> +
> +    orig_pte = *entry;
> +
> +    /*
> +     * The radix-tree can only work on 4KB. This is only used when
> +     * memaccess is enabled.
> +     */
> +    ASSERT(!p2m->mem_access_enabled || page_order == 0);
> +    /*
> +     * The access type should always be p2m_access_rwx when the mapping
> +     * is removed.
> +     */
> +    ASSERT(!mfn_eq(INVALID_MFN, smfn) || (a == p2m_access_rwx));
> +    /*
> +     * Update the mem access permission before update the P2M. So we
> +     * don't have to revert the mapping if it has failed.
> +     */
> +    rc = p2m_mem_access_radix_set(p2m, sgfn, a);
> +    if ( rc )
> +        goto out;
> +
> +    /*
> +     * Always remove the entry in order to follow the break-before-make
> +     * sequence when updating the translation table (D4.7.1 in ARM DDI
> +     * 0487A.j).
> +     */
> +    if ( p2m_valid(orig_pte) )
> +        p2m_remove_pte(entry, p2m->clean_pte);
> +
> +    if ( mfn_eq(smfn, INVALID_MFN) )
> +        /* Flush can be deferred if the entry is removed */
> +        p2m->need_flush |= !!p2m_valid(orig_pte);
> +    else
> +    {
> +        lpae_t pte = mfn_to_p2m_entry(smfn, t, a);
> +
> +        if ( level < 3 )
> +            pte.p2m.table = 0; /* Superpage entry */
> +
> +        /*
> +         * It is necessary to flush the TLB before writing the new entry
> +         * to keep coherency when the previous entry was valid.
> +         *
> +         * Although, it could be defered when only the permissions are
> +         * changed (e.g in case of memaccess).
> +         */
> +        if ( p2m_valid(orig_pte) )
> +        {
> +            if ( likely(!p2m->mem_access_enabled) ||
> +                 P2M_CLEAR_PERM(pte) != P2M_CLEAR_PERM(orig_pte) )
> +                p2m_flush_tlb_sync(p2m);
> +            else
> +                p2m->need_flush = true;
> +        }
> +        else /* new mapping */
> +            p2m->stats.mappings[level]++;
> +
> +        p2m_write_pte(entry, pte, p2m->clean_pte);
> +
> +        p2m->max_mapped_gfn = gfn_max(p2m->max_mapped_gfn,
> +                                      gfn_add(sgfn, 1 << page_order));
> +        p2m->lowest_mapped_gfn = gfn_min(p2m->lowest_mapped_gfn, sgfn);
> +    }
> +
> +    /*
> +     * Free the entry only if the original pte was valid and the base
> +     * is different (to avoid freeing when permission is changed).
> +     */
> +    if ( p2m_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base )
> +        p2m_free_entry(p2m, orig_pte, level);
> +
> +    if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) || p2m_valid(*entry)) )
> +        rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << page_order);
> +    else
> +        rc = 0;
> +
> +out:
> +    unmap_domain_page(table);
> +
> +    return rc;
> +}
> +
> +int p2m_set_entry(struct p2m_domain *p2m,
> +                  gfn_t sgfn,
> +                  unsigned long nr,
> +                  mfn_t smfn,
> +                  p2m_type_t t,
> +                  p2m_access_t a)
> +{
> +    int rc = 0;
> +
> +    while ( nr )
> +    {
> +        /*
> +         * XXX: Support superpage mappings if nr is not aligned to a
> +         * superpage size.
> +         */
> +        unsigned long mask = gfn_x(sgfn) | mfn_x(smfn) | nr;
> +        unsigned long order;
> +
> +        /* Always map 4k by 4k when memaccess is enabled */
> +        if ( unlikely(p2m->mem_access_enabled) )
> +            order = THIRD_ORDER;
> +        else if ( !(mask & ((1UL << FIRST_ORDER) - 1)) )
> +            order = FIRST_ORDER;
> +        else if ( !(mask & ((1UL << SECOND_ORDER) - 1)) )
> +            order = SECOND_ORDER;
> +        else
> +            order = THIRD_ORDER;
> +
> +        rc = __p2m_set_entry(p2m, sgfn, order, smfn, t, a);
> +        if ( rc )
> +            break;
> +
> +        sgfn = gfn_add(sgfn, (1 << order));
> +        if ( !mfn_eq(smfn, INVALID_MFN) )
> +           smfn = mfn_add(smfn, (1 << order));
> +
> +        nr -= (1 << order);
> +    }
> +
> +    return rc;
> +}
> +
>  /*
>   * Returns true if start_gpaddr..end_gpaddr contains at least one
>   * suitably aligned level_size mappping of maddr.
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 6fe6a37..cca86ef 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -187,6 +187,17 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>                      p2m_type_t *t, p2m_access_t *a,
>                      unsigned int *page_order);
>  
> +/*
> + * Direct set a p2m entry: only for use by the P2M code.
> + * The P2M write lock should be taken.
> + */
> +int p2m_set_entry(struct p2m_domain *p2m,
> +                  gfn_t sgfn,
> +                  unsigned long nr,
> +                  mfn_t smfn,
> +                  p2m_type_t t,
> +                  p2m_access_t a);
> +
>  /* Clean & invalidate caches corresponding to a region of guest address space */
>  int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr);
>  
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index a43b0fa..ba63322 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -166,6 +166,10 @@ typedef struct __packed {
>      unsigned long sbz1:5;
>  } lpae_p2m_t;
>  
> +/* Permission mask: xn, write, read */
> +#define P2M_PERM_MASK (0x00400000000000C0ULL)
> +#define P2M_CLEAR_PERM(pte) ((pte).bits & ~P2M_PERM_MASK)
> +
>  /*
>   * Walk is the common bits of p2m and pt entries which are needed to
>   * simply walk the table (e.g. for debug).
> -- 
> 1.9.1
> 

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

Re: [for-4.8][PATCH v2 00/23] xen/arm: Rework the P2M code to follow break-before-make sequence

[Stefano Stabellini]

On Thu, 15 Sep 2016, Julien Grall wrote:
> Hello all,
> 
> The ARM architecture mandates the use of a break-before-make sequence when
> changing translation entries if the page table is shared between multiple
> CPUs whenever a valid entry is replaced by another valid entry (see D4.7.1
> in ARM DDI 0487A.j for more details).
> 
> The current P2M code does not respect this sequence and may result to
> break coherency on some processors.
> 
> Adapting the current implementation to use break-before-make sequence would
> imply some code duplication and more TLBs invalidations than necessary.
> For instance, if we are replacing a 4KB page and the current mapping in
> the P2M is using a 1GB superpage, the following steps will happen:
>     1) Shatter the 1GB superpage into a series of 2MB superpages
>     2) Shatter the 2MB superpage into a series of 4KB superpages
>     3) Replace the 4KB page
> 
> As the current implementation is shattering while descending and install
> the mapping before continuing to the next level, Xen would need to issue 3
> TLB invalidation instructions which is clearly inefficient.
> 
> Furthermore, all the operations which modify the page table are using the
> same skeleton. It is more complicated to maintain different code paths than
> having a generic function that set an entry and take care of the break-before-
> make sequence.
> 
> The new implementation is based on the x86 EPT one which, I think, fits
> quite well for the break-before-make sequence whilst keeping the code
> simple.
> 
> For all the changes see in each patch.
> 
> I have provided a branch based on upstream here:
> git://xenbits.xen.org/people/julieng/xen-unstable.git branch p2m-v2

Committed


> Comments are welcome.
> 
> Yours sincerely,
> 
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Tamas K Lengyel <tamas@tklengyel.com>
> Cc: Shanker Donthineni <shankerd@codeaurora.org>
> Cc: Dirk Behme <dirk.behme@de.bosch.com>
> Cc: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> 
> Julien Grall (23):
>   xen/arm: do_trap_instr_abort_guest: Move the IPA computation out of
>     the switch
>   xen/arm: p2m: Store in p2m_domain whether we need to clean the entry
>   xen/arm: p2m: Rename parameter in p2m_{remove,write}_pte...
>   xen/arm: p2m: Use typesafe gfn in p2m_mem_access_radix_set
>   xen/arm: p2m: Add a back pointer to domain in p2m_domain
>   xen/arm: traps: Move MMIO emulation code in a separate helper
>   xen/arm: traps: Check the P2M before injecting a data/instruction
>     abort
>   xen/arm: p2m: Invalidate the TLBs when write unlocking the p2m
>   xen/arm: p2m: Change the type of level_shifts from paddr_t to uint8_t
>   xen/arm: p2m: Move the lookup helpers at the top of the file
>   xen/arm: p2m: Introduce p2m_get_root_pointer and use it in
>     __p2m_lookup
>   xen/arm: p2m: Introduce p2m_get_entry and use it to implement
>     __p2m_lookup
>   xen/arm: p2m: Replace all usage of __p2m_lookup with p2m_get_entry
>   xen/arm: p2m: Re-implement p2m_cache_flush using p2m_get_entry
>   xen/arm: p2m: Make p2m_{valid,table,mapping} helpers inline
>   xen/arm: p2m: Introduce a helper to check if an entry is a superpage
>   xen/arm: p2m: Introduce p2m_set_entry and __p2m_set_entry
>   xen/arm: p2m: Re-implement relinquish_p2m_mapping using
>     p2m_{get,set}_entry
>   xen/arm: p2m: Re-implement p2m_remove_using using p2m_set_entry
>   xen/arm: p2m: Re-implement p2m_insert_mapping using p2m_set_entry
>   xen/arm: p2m: Re-implement p2m_set_mem_access using
>     p2m_{set,get}_entry
>   xen/arm: p2m: Do not handle shattering in p2m_create_table
>   xen/arm: p2m: Export p2m_*_lock helpers
> 
>  xen/arch/arm/domain.c      |    8 +-
>  xen/arch/arm/p2m.c         | 1316 ++++++++++++++++++++++----------------------
>  xen/arch/arm/traps.c       |  126 +++--
>  xen/include/asm-arm/p2m.h  |   63 +++
>  xen/include/asm-arm/page.h |    8 +
>  5 files changed, 828 insertions(+), 693 deletions(-)
> 
> -- 
> 1.9.1
> 

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