[PATCH v2 00/15] xen/arm: P2M clean-up fixes

[PATCH v2 00/15] xen/arm: P2M clean-up fixes

[Julien Grall]

Hello all,

This patch series contains a bunch of clean-up and fixes for the P2M code on
ARM. The major changes are:
    - Deduce the memory attributes from the p2m type
    - Switch to read-write lock to improve performance
    - Simplify the TLB flush for a give p2m

For all the changes see in each patch.

I have provided a branch with all the patches applied on my repo:
git://xenbits.xen.org/people/julieng/xen-unstable.git branch p2m-cleanup-v2.

Yours sincerely,

Julien Grall (15):
  xen/arm: p2m: Use the typesafe MFN in mfn_to_p2m_entry
  xen/arm: p2m: Use a whitelist rather than blacklist in
    get_page_from_gfn
  xen/arm: p2m: Differentiate cacheable vs non-cacheable MMIO
  xen/arm: p2m: Find the memory attributes based on the p2m type
  xen/arm: p2m: Remove unnecessary locking
  xen/arm: p2m: Introduce p2m_{read,write}_{,un}lock helpers
  xen/arm: p2m: Switch the p2m lock from spinlock to rwlock
  xen/arm: Don't call p2m_alloc_table from arch_domain_create
  xen/arm: p2m: Move the vttbr field from arch_domain to p2m_domain
  xen/arm: p2m: Don't need to restore the state for an idle vCPU.
  xen/arm: p2m: Rework the context switch to another VTTBR in
    flush_tlb_domain
  xen/arm: p2m: Inline p2m_load_VTTBR into p2m_restore_state
  xen/arm: Don't export flush_tlb_domain
  xen/arm: p2m: Replace flush_tlb_domain by p2m_flush_tlb
  xen/arm: p2m: Pass the p2m in parameter rather the domain when it is
    possible

 xen/arch/arm/domain.c          |   3 -
 xen/arch/arm/p2m.c             | 194 ++++++++++++++++++++---------------------
 xen/arch/arm/traps.c           |   2 +-
 xen/include/asm-arm/domain.h   |   1 -
 xen/include/asm-arm/flushtlb.h |   3 -
 xen/include/asm-arm/p2m.h      |  25 +++---
 6 files changed, 113 insertions(+), 115 deletions(-)

-- 
1.9.1


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

[PATCH v2 01/15] xen/arm: p2m: Use the typesafe MFN in mfn_to_p2m_entry

[Julien Grall]

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

---
    Changes in v2:
        - Compute the mfn at every loop rather than incrementing
        - Add Stefano's reviewed-by
---
 xen/arch/arm/p2m.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index d82349c..851b110 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -324,7 +324,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a)
     }
 }
 
-static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
+static lpae_t mfn_to_p2m_entry(mfn_t mfn, unsigned int mattr,
                                p2m_type_t t, p2m_access_t a)
 {
     /*
@@ -358,9 +358,9 @@ static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
 
     p2m_set_permission(&e, t, a);
 
-    ASSERT(!(pfn_to_paddr(mfn) & ~PADDR_MASK));
+    ASSERT(!(pfn_to_paddr(mfn_x(mfn)) & ~PADDR_MASK));
 
-    e.p2m.base = mfn;
+    e.p2m.base = mfn_x(mfn);
 
     return e;
 }
@@ -411,7 +411,7 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
     if ( splitting )
     {
         p2m_type_t t = entry->p2m.type;
-        unsigned long base_pfn = entry->p2m.base;
+        mfn_t mfn = _mfn(entry->p2m.base);
         int i;
 
         /*
@@ -420,7 +420,7 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
          */
          for ( i=0 ; i < LPAE_ENTRIES; i++ )
          {
-             pte = mfn_to_p2m_entry(base_pfn + (i<<(level_shift-LPAE_SHIFT)),
+             pte = mfn_to_p2m_entry(mfn_add(mfn, i << (level_shift - LPAE_SHIFT)),
                                     MATTR_MEM, t, p2m->default_access);
 
              /*
@@ -443,7 +443,7 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
 
     unmap_domain_page(p);
 
-    pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, p2m_invalid,
+    pte = mfn_to_p2m_entry(_mfn(page_to_mfn(page)), MATTR_MEM, p2m_invalid,
                            p2m->default_access);
 
     p2m_write_pte(entry, pte, flush_cache);
@@ -693,7 +693,7 @@ static int apply_one_level(struct domain *d,
                 return rc;
 
             /* New mapping is superpage aligned, make it */
-            pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT, mattr, t, a);
+            pte = mfn_to_p2m_entry(_mfn(*maddr >> PAGE_SHIFT), mattr, t, a);
             if ( level < 3 )
                 pte.p2m.table = 0; /* Superpage entry */
 
-- 
1.9.1


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

[PATCH v2 02/15] xen/arm: p2m: Use a whitelist rather than blacklist in get_page_from_gfn

[Julien Grall]

Currently, the check in get_page_from_gfn is using a blacklist. This is
very fragile because we may forgot to update the check when a new p2m
type is added.

To avoid any possible issue, use a whitelist. All type backed by a RAM
page can could potential be valid. The check is borrowed from x86.

Note with this change, it is not possible anymore to retrieve a page when
the p2m type is p2m_iommu_map_*. This is fine because they are special
mappings for direct mapping workaround and the associated GFN should be
used at all by callers of get_page_from_gfn.

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

---
    Changes in v2:
        - Update the commit message about iommu_mappings
---
 xen/include/asm-arm/p2m.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 3091c04..78d37ab 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -104,9 +104,16 @@ typedef enum {
 #define P2M_RAM_TYPES (p2m_to_mask(p2m_ram_rw) |        \
                        p2m_to_mask(p2m_ram_ro))
 
+/* Grant mapping types, which map to a real frame in another VM */
+#define P2M_GRANT_TYPES (p2m_to_mask(p2m_grant_map_rw) |  \
+                         p2m_to_mask(p2m_grant_map_ro))
+
 /* Useful predicates */
 #define p2m_is_ram(_t) (p2m_to_mask(_t) & P2M_RAM_TYPES)
 #define p2m_is_foreign(_t) (p2m_to_mask(_t) & p2m_to_mask(p2m_map_foreign))
+#define p2m_is_any_ram(_t) (p2m_to_mask(_t) &                   \
+                            (P2M_RAM_TYPES | P2M_GRANT_TYPES |  \
+                             p2m_to_mask(p2m_map_foreign)))
 
 static inline
 void p2m_mem_access_emulate_check(struct vcpu *v,
@@ -224,7 +231,7 @@ static inline struct page_info *get_page_from_gfn(
     if (t)
         *t = p2mt;
 
-    if ( p2mt == p2m_invalid || p2mt == p2m_mmio_direct )
+    if ( !p2m_is_any_ram(p2mt) )
         return NULL;
 
     if ( !mfn_valid(mfn) )
-- 
1.9.1


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

[PATCH v2 03/15] xen/arm: p2m: Differentiate cacheable vs non-cacheable MMIO

[Julien Grall]

Currently, the p2m type p2m_mmio_direct is used to map in stage-2
cacheable MMIO (via map_regions_rw_cache) and non-cacheable one (via
map_mmio_regions). The p2m code is relying on the caller to give the
correct memory attribute.

In a follow-up patch, the p2m code will rely on the p2m type to find the
correct memory attribute. In preparation of this, introduce
p2m_mmio_direct_nc and p2m_mimo_direct_c to differentiate the
cacheability of the MMIO.

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        | 7 ++++---
 xen/include/asm-arm/p2m.h | 3 ++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 851b110..cffb12e 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -272,7 +272,8 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a)
     case p2m_iommu_map_rw:
     case p2m_map_foreign:
     case p2m_grant_map_rw:
-    case p2m_mmio_direct:
+    case p2m_mmio_direct_nc:
+    case p2m_mmio_direct_c:
         e->p2m.xn = 1;
         e->p2m.write = 1;
         break;
@@ -1194,7 +1195,7 @@ int map_regions_rw_cache(struct domain *d,
                          mfn_t mfn)
 {
     return p2m_insert_mapping(d, gfn, nr, mfn,
-                              MATTR_MEM, p2m_mmio_direct);
+                              MATTR_MEM, p2m_mmio_direct_c);
 }
 
 int unmap_regions_rw_cache(struct domain *d,
@@ -1211,7 +1212,7 @@ int map_mmio_regions(struct domain *d,
                      mfn_t mfn)
 {
     return p2m_insert_mapping(d, start_gfn, nr, mfn,
-                              MATTR_DEV, p2m_mmio_direct);
+                              MATTR_DEV, p2m_mmio_direct_nc);
 }
 
 int unmap_mmio_regions(struct domain *d,
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 78d37ab..20a220ea 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -87,7 +87,8 @@ typedef enum {
     p2m_invalid = 0,    /* Nothing mapped here */
     p2m_ram_rw,         /* Normal read/write guest RAM */
     p2m_ram_ro,         /* Read-only; writes are silently dropped */
-    p2m_mmio_direct,    /* Read/write mapping of genuine MMIO area */
+    p2m_mmio_direct_nc, /* Read/write mapping of genuine MMIO area non-cacheable */
+    p2m_mmio_direct_c,  /* Read/write mapping of genuine MMIO area cacheable */
     p2m_map_foreign,    /* Ram pages from foreign domain */
     p2m_grant_map_rw,   /* Read/write grant mapping */
     p2m_grant_map_ro,   /* Read-only grant mapping */
-- 
1.9.1


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

[PATCH v2 04/15] xen/arm: p2m: Find the memory attributes based on the p2m type

[Julien Grall]

Currently, mfn_to_p2m_entry is relying on the caller to provide the
correct memory attribute and will deduce the sharability based on it.

Some of the callers, such as p2m_create_table, are using same memory
attribute regardless the underlying p2m type. For instance, this will
lead to use change the memory attribute from MATTR_DEV to MATTR_MEM when
a MMIO superpage is shattered.

Furthermore, it makes more difficult to support different shareability
with the same memory attribute.

All the memory attributes could be deduced via the p2m type. This will
simplify the code by dropping one parameter.

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

---
    I am not sure whether p2m_direct_mmio_c (cacheable MMIO) should use
    the outer-shareability or inner-shareability. Any opinions?

    Changes in v2:
        - Forgot to add my signed-off-by
        - Add Stefano's reviewed-by
---
 xen/arch/arm/p2m.c | 55 ++++++++++++++++++++++++------------------------------
 1 file changed, 24 insertions(+), 31 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index cffb12e..08f3f17 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -325,8 +325,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a)
     }
 }
 
-static lpae_t mfn_to_p2m_entry(mfn_t mfn, unsigned int mattr,
-                               p2m_type_t t, p2m_access_t a)
+static lpae_t mfn_to_p2m_entry(mfn_t mfn, p2m_type_t t, p2m_access_t a)
 {
     /*
      * sh, xn and write bit will be defined in the following switches
@@ -335,7 +334,6 @@ static lpae_t mfn_to_p2m_entry(mfn_t mfn, unsigned int mattr,
     lpae_t e = (lpae_t) {
         .p2m.af = 1,
         .p2m.read = 1,
-        .p2m.mattr = mattr,
         .p2m.table = 1,
         .p2m.valid = 1,
         .p2m.type = t,
@@ -343,18 +341,21 @@ static lpae_t mfn_to_p2m_entry(mfn_t mfn, unsigned int mattr,
 
     BUILD_BUG_ON(p2m_max_real_type > (1 << 4));
 
-    switch (mattr)
+    switch ( t )
     {
-    case MATTR_MEM:
-        e.p2m.sh = LPAE_SH_INNER;
+    case p2m_mmio_direct_nc:
+        e.p2m.mattr = MATTR_DEV;
+        e.p2m.sh = LPAE_SH_OUTER;
         break;
 
-    case MATTR_DEV:
+    case p2m_mmio_direct_c:
+        e.p2m.mattr = MATTR_MEM;
         e.p2m.sh = LPAE_SH_OUTER;
         break;
+
     default:
-        BUG();
-        break;
+        e.p2m.mattr = MATTR_MEM;
+        e.p2m.sh = LPAE_SH_INNER;
     }
 
     p2m_set_permission(&e, t, a);
@@ -422,7 +423,7 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
          for ( i=0 ; i < LPAE_ENTRIES; i++ )
          {
              pte = mfn_to_p2m_entry(mfn_add(mfn, i << (level_shift - LPAE_SHIFT)),
-                                    MATTR_MEM, t, p2m->default_access);
+                                    t, p2m->default_access);
 
              /*
               * First and second level super pages set p2m.table = 0, but
@@ -444,7 +445,7 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
 
     unmap_domain_page(p);
 
-    pte = mfn_to_p2m_entry(_mfn(page_to_mfn(page)), MATTR_MEM, p2m_invalid,
+    pte = mfn_to_p2m_entry(_mfn(page_to_mfn(page)), p2m_invalid,
                            p2m->default_access);
 
     p2m_write_pte(entry, pte, flush_cache);
@@ -665,7 +666,6 @@ static int apply_one_level(struct domain *d,
                            paddr_t *addr,
                            paddr_t *maddr,
                            bool_t *flush,
-                           int mattr,
                            p2m_type_t t,
                            p2m_access_t a)
 {
@@ -694,7 +694,7 @@ static int apply_one_level(struct domain *d,
                 return rc;
 
             /* New mapping is superpage aligned, make it */
-            pte = mfn_to_p2m_entry(_mfn(*maddr >> PAGE_SHIFT), mattr, t, a);
+            pte = mfn_to_p2m_entry(_mfn(*maddr >> PAGE_SHIFT), t, a);
             if ( level < 3 )
                 pte.p2m.table = 0; /* Superpage entry */
 
@@ -914,7 +914,6 @@ static int apply_p2m_changes(struct domain *d,
                      gfn_t sgfn,
                      unsigned long nr,
                      mfn_t smfn,
-                     int mattr,
                      uint32_t mask,
                      p2m_type_t t,
                      p2m_access_t a)
@@ -1053,7 +1052,7 @@ static int apply_p2m_changes(struct domain *d,
                                   level, flush_pt, op,
                                   start_gpaddr, end_gpaddr,
                                   &addr, &maddr, &flush,
-                                  mattr, t, a);
+                                  t, a);
             if ( ret < 0 ) { rc = ret ; goto out; }
             count += ret;
 
@@ -1162,7 +1161,7 @@ out:
          * mapping.
          */
         apply_p2m_changes(d, REMOVE, sgfn, gfn - gfn_x(sgfn), smfn,
-                          mattr, 0, p2m_invalid, d->arch.p2m.default_access);
+                          0, p2m_invalid, d->arch.p2m.default_access);
     }
 
     return rc;
@@ -1172,10 +1171,10 @@ static inline int p2m_insert_mapping(struct domain *d,
                                      gfn_t start_gfn,
                                      unsigned long nr,
                                      mfn_t mfn,
-                                     int mattr, p2m_type_t t)
+                                     p2m_type_t t)
 {
     return apply_p2m_changes(d, INSERT, start_gfn, nr, mfn,
-                             mattr, 0, t, d->arch.p2m.default_access);
+                             0, t, d->arch.p2m.default_access);
 }
 
 static inline int p2m_remove_mapping(struct domain *d,
@@ -1185,8 +1184,7 @@ static inline int p2m_remove_mapping(struct domain *d,
 {
     return apply_p2m_changes(d, REMOVE, start_gfn, nr, mfn,
                              /* arguments below not used when removing mapping */
-                             MATTR_MEM, 0, p2m_invalid,
-                             d->arch.p2m.default_access);
+                             0, p2m_invalid, d->arch.p2m.default_access);
 }
 
 int map_regions_rw_cache(struct domain *d,
@@ -1194,8 +1192,7 @@ int map_regions_rw_cache(struct domain *d,
                          unsigned long nr,
                          mfn_t mfn)
 {
-    return p2m_insert_mapping(d, gfn, nr, mfn,
-                              MATTR_MEM, p2m_mmio_direct_c);
+    return p2m_insert_mapping(d, gfn, nr, mfn, p2m_mmio_direct_c);
 }
 
 int unmap_regions_rw_cache(struct domain *d,
@@ -1211,8 +1208,7 @@ int map_mmio_regions(struct domain *d,
                      unsigned long nr,
                      mfn_t mfn)
 {
-    return p2m_insert_mapping(d, start_gfn, nr, mfn,
-                              MATTR_DEV, p2m_mmio_direct_nc);
+    return p2m_insert_mapping(d, start_gfn, nr, mfn, p2m_mmio_direct_nc);
 }
 
 int unmap_mmio_regions(struct domain *d,
@@ -1250,8 +1246,7 @@ int guest_physmap_add_entry(struct domain *d,
                             unsigned long page_order,
                             p2m_type_t t)
 {
-    return p2m_insert_mapping(d, gfn, (1 << page_order), mfn,
-                              MATTR_MEM, t);
+    return p2m_insert_mapping(d, gfn, (1 << page_order), mfn, t);
 }
 
 void guest_physmap_remove_page(struct domain *d,
@@ -1411,7 +1406,7 @@ int relinquish_p2m_mapping(struct domain *d)
     nr = gfn_x(p2m->max_mapped_gfn) - gfn_x(p2m->lowest_mapped_gfn);
 
     return apply_p2m_changes(d, RELINQUISH, p2m->lowest_mapped_gfn, nr,
-                             INVALID_MFN, MATTR_MEM, 0, p2m_invalid,
+                             INVALID_MFN, 0, p2m_invalid,
                              d->arch.p2m.default_access);
 }
 
@@ -1424,8 +1419,7 @@ int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr)
     end = gfn_min(end, p2m->max_mapped_gfn);
 
     return apply_p2m_changes(d, CACHEFLUSH, start, nr, INVALID_MFN,
-                             MATTR_MEM, 0, p2m_invalid,
-                             d->arch.p2m.default_access);
+                             0, p2m_invalid, d->arch.p2m.default_access);
 }
 
 mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn)
@@ -1826,8 +1820,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
     }
 
     rc = apply_p2m_changes(d, MEMACCESS, gfn_add(gfn, start),
-                           (nr - start), INVALID_MFN,
-                           MATTR_MEM, mask, 0, a);
+                           (nr - start), INVALID_MFN, mask, 0, a);
     if ( rc < 0 )
         return rc;
     else if ( rc > 0 )
-- 
1.9.1


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

[PATCH v2 05/15] xen/arm: p2m: Remove unnecessary locking

[Julien Grall]

The p2m is not yet in use when p2m_init and p2m_allocate_table are
called. Furthermore the p2m is not used anymore when p2m_teardown is
called. So taking the p2m lock is not necessary.

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 | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 08f3f17..bcccaa4 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1266,8 +1266,6 @@ int p2m_alloc_table(struct domain *d)
     if ( page == NULL )
         return -ENOMEM;
 
-    spin_lock(&p2m->lock);
-
     /* Clear both first level pages */
     for ( i = 0; i < P2M_ROOT_PAGES; i++ )
         clear_and_clean_page(page + i);
@@ -1283,8 +1281,6 @@ int p2m_alloc_table(struct domain *d)
      */
     flush_tlb_domain(d);
 
-    spin_unlock(&p2m->lock);
-
     return 0;
 }
 
@@ -1349,8 +1345,6 @@ void p2m_teardown(struct domain *d)
     struct p2m_domain *p2m = &d->arch.p2m;
     struct page_info *pg;
 
-    spin_lock(&p2m->lock);
-
     while ( (pg = page_list_remove_head(&p2m->pages)) )
         free_domheap_page(pg);
 
@@ -1362,8 +1356,6 @@ void p2m_teardown(struct domain *d)
     p2m_free_vmid(d);
 
     radix_tree_destroy(&p2m->mem_access_settings, NULL);
-
-    spin_unlock(&p2m->lock);
 }
 
 int p2m_init(struct domain *d)
@@ -1374,12 +1366,11 @@ int p2m_init(struct domain *d)
     spin_lock_init(&p2m->lock);
     INIT_PAGE_LIST_HEAD(&p2m->pages);
 
-    spin_lock(&p2m->lock);
     p2m->vmid = INVALID_VMID;
 
     rc = p2m_alloc_vmid(d);
     if ( rc != 0 )
-        goto err;
+        return rc;
 
     d->arch.vttbr = 0;
 
@@ -1392,9 +1383,6 @@ int p2m_init(struct domain *d)
     p2m->mem_access_enabled = false;
     radix_tree_init(&p2m->mem_access_settings);
 
-err:
-    spin_unlock(&p2m->lock);
-
     return rc;
 }
 
-- 
1.9.1


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

[PATCH v2 06/15] xen/arm: p2m: Introduce p2m_{read, write}_{, un}lock helpers

[Julien Grall]

Some functions in the p2m code do not require to modify the P2M code.
Document it by introducing separate helpers to lock the p2m.

This patch does not change the lock. This will be done in a subsequent
patch.

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 | 49 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index bcccaa4..5c67090 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -47,11 +47,36 @@ static bool_t p2m_mapping(lpae_t pte)
     return p2m_valid(pte) && !pte.p2m.table;
 }
 
+static inline void p2m_write_lock(struct p2m_domain *p2m)
+{
+    spin_lock(&p2m->lock);
+}
+
+static inline void p2m_write_unlock(struct p2m_domain *p2m)
+{
+    spin_unlock(&p2m->lock);
+}
+
+static inline void p2m_read_lock(struct p2m_domain *p2m)
+{
+    spin_lock(&p2m->lock);
+}
+
+static inline void p2m_read_unlock(struct p2m_domain *p2m)
+{
+    spin_unlock(&p2m->lock);
+}
+
+static inline int p2m_is_locked(struct p2m_domain *p2m)
+{
+    return spin_is_locked(&p2m->lock);
+}
+
 void p2m_dump_info(struct domain *d)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
 
-    spin_lock(&p2m->lock);
+    p2m_read_lock(p2m);
     printk("p2m mappings for domain %d (vmid %d):\n",
            d->domain_id, p2m->vmid);
     BUG_ON(p2m->stats.mappings[0] || p2m->stats.shattered[0]);
@@ -60,7 +85,7 @@ void p2m_dump_info(struct domain *d)
     printk("  2M mappings: %ld (shattered %ld)\n",
            p2m->stats.mappings[2], p2m->stats.shattered[2]);
     printk("  4K mappings: %ld\n", p2m->stats.mappings[3]);
-    spin_unlock(&p2m->lock);
+    p2m_read_unlock(p2m);
 }
 
 void memory_type_changed(struct domain *d)
@@ -166,7 +191,7 @@ static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
     p2m_type_t _t;
     unsigned int level, root_table;
 
-    ASSERT(spin_is_locked(&p2m->lock));
+    ASSERT(p2m_is_locked(p2m));
     BUILD_BUG_ON(THIRD_MASK != PAGE_MASK);
 
     /* Allow t to be NULL */
@@ -233,9 +258,9 @@ mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
     mfn_t ret;
     struct p2m_domain *p2m = &d->arch.p2m;
 
-    spin_lock(&p2m->lock);
+    p2m_read_lock(p2m);
     ret = __p2m_lookup(d, gfn, t);
-    spin_unlock(&p2m->lock);
+    p2m_read_unlock(p2m);
 
     return ret;
 }
@@ -475,7 +500,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
 #undef ACCESS
     };
 
-    ASSERT(spin_is_locked(&p2m->lock));
+    ASSERT(p2m_is_locked(p2m));
 
     /* If no setting was ever set, just return rwx. */
     if ( !p2m->mem_access_enabled )
@@ -944,7 +969,7 @@ static int apply_p2m_changes(struct domain *d,
      */
     flush_pt = iommu_enabled && !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
 
-    spin_lock(&p2m->lock);
+    p2m_write_lock(p2m);
 
     /* Static mapping. P2M_ROOT_PAGES > 1 are handled below */
     if ( P2M_ROOT_PAGES == 1 )
@@ -1148,7 +1173,7 @@ out:
             unmap_domain_page(mappings[level]);
     }
 
-    spin_unlock(&p2m->lock);
+    p2m_write_unlock(p2m);
 
     if ( rc < 0 && ( op == INSERT ) &&
          addr != start_gpaddr )
@@ -1529,7 +1554,7 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
     if ( v != current )
         return NULL;
 
-    spin_lock(&p2m->lock);
+    p2m_read_lock(p2m);
 
     rc = gvirt_to_maddr(va, &maddr, flags);
 
@@ -1549,7 +1574,7 @@ err:
     if ( !page && p2m->mem_access_enabled )
         page = p2m_mem_access_check_and_get_page(va, flags);
 
-    spin_unlock(&p2m->lock);
+    p2m_read_unlock(p2m);
 
     return page;
 }
@@ -1823,9 +1848,9 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn,
     int ret;
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
 
-    spin_lock(&p2m->lock);
+    p2m_read_lock(p2m);
     ret = __p2m_get_mem_access(d, gfn, access);
-    spin_unlock(&p2m->lock);
+    p2m_read_unlock(p2m);
 
     return ret;
 }
-- 
1.9.1


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

[PATCH v2 07/15] xen/arm: p2m: Switch the p2m lock from spinlock to rwlock

[Julien Grall]

P2M reads do not require to be serialized. This will add contention
when PV drivers are using multi-queue because parallel grant
map/unmaps/copies will happen on DomU's p2m.

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

---
    I have not done benchark to verify the performance, however a rwlock
    is always an improvement compare to a spinlock when most of the
    access only read data.

    It might be possible to convert the rwlock to a per-cpu rwlock which
    show some improvement on x86.

    Changes in v2:
        - Add Stefano's reviewed-by
---
 xen/arch/arm/p2m.c        | 12 ++++++------
 xen/include/asm-arm/p2m.h |  3 ++-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 5c67090..9ba8904 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -49,27 +49,27 @@ static bool_t p2m_mapping(lpae_t pte)
 
 static inline void p2m_write_lock(struct p2m_domain *p2m)
 {
-    spin_lock(&p2m->lock);
+    write_lock(&p2m->lock);
 }
 
 static inline void p2m_write_unlock(struct p2m_domain *p2m)
 {
-    spin_unlock(&p2m->lock);
+    write_unlock(&p2m->lock);
 }
 
 static inline void p2m_read_lock(struct p2m_domain *p2m)
 {
-    spin_lock(&p2m->lock);
+    read_lock(&p2m->lock);
 }
 
 static inline void p2m_read_unlock(struct p2m_domain *p2m)
 {
-    spin_unlock(&p2m->lock);
+    read_unlock(&p2m->lock);
 }
 
 static inline int p2m_is_locked(struct p2m_domain *p2m)
 {
-    return spin_is_locked(&p2m->lock);
+    return rw_is_locked(&p2m->lock);
 }
 
 void p2m_dump_info(struct domain *d)
@@ -1388,7 +1388,7 @@ int p2m_init(struct domain *d)
     struct p2m_domain *p2m = &d->arch.p2m;
     int rc = 0;
 
-    spin_lock_init(&p2m->lock);
+    rwlock_init(&p2m->lock);
     INIT_PAGE_LIST_HEAD(&p2m->pages);
 
     p2m->vmid = INVALID_VMID;
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 20a220ea..abda70c 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -3,6 +3,7 @@
 
 #include <xen/mm.h>
 #include <xen/radix-tree.h>
+#include <xen/rwlock.h>
 #include <public/vm_event.h> /* for vm_event_response_t */
 #include <public/memory.h>
 #include <xen/p2m-common.h>
@@ -20,7 +21,7 @@ extern void memory_type_changed(struct domain *);
 /* Per-p2m-table state */
 struct p2m_domain {
     /* Lock that protects updates to the p2m */
-    spinlock_t lock;
+    rwlock_t lock;
 
     /* Pages used to construct the p2m */
     struct page_list_head pages;
-- 
1.9.1


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

[PATCH v2 08/15] xen/arm: Don't call p2m_alloc_table from arch_domain_create

[Julien Grall]

The p2m root table does not need to be allocate separately.

Also remove unnecessary fields initialization as the structure is already
memset to 0 and the fields will be overridden by p2m_alloc_table.

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

---
    Changes in v2:
        - Rebase on the latest staging
        - Fix typo in the commit message
        - Add Stefano's reviewed-by
---
 xen/arch/arm/domain.c     | 3 ---
 xen/arch/arm/p2m.c        | 8 +++-----
 xen/include/asm-arm/p2m.h | 7 -------
 3 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 4e5259b..20bb2ba 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -569,9 +569,6 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     share_xen_page_with_guest(
         virt_to_page(d->shared_info), d, XENSHARE_writable);
 
-    if ( (rc = p2m_alloc_table(d)) != 0 )
-        goto fail;
-
     switch ( config->gic_version )
     {
     case XEN_DOMCTL_CONFIG_GIC_NATIVE:
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 9ba8904..512fd7d 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1281,7 +1281,7 @@ void guest_physmap_remove_page(struct domain *d,
     p2m_remove_mapping(d, gfn, (1 << page_order), mfn);
 }
 
-int p2m_alloc_table(struct domain *d)
+static int p2m_alloc_table(struct domain *d)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
     struct page_info *page;
@@ -1397,10 +1397,6 @@ int p2m_init(struct domain *d)
     if ( rc != 0 )
         return rc;
 
-    d->arch.vttbr = 0;
-
-    p2m->root = NULL;
-
     p2m->max_mapped_gfn = _gfn(0);
     p2m->lowest_mapped_gfn = _gfn(ULONG_MAX);
 
@@ -1408,6 +1404,8 @@ int p2m_init(struct domain *d)
     p2m->mem_access_enabled = false;
     radix_tree_init(&p2m->mem_access_settings);
 
+    rc = p2m_alloc_table(d);
+
     return rc;
 }
 
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index abda70c..ce28e8a 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -149,13 +149,6 @@ void p2m_teardown(struct domain *d);
  */
 int relinquish_p2m_mapping(struct domain *d);
 
-/*
- * Allocate a new p2m table for a domain.
- *
- * Returns 0 for success or -errno.
- */
-int p2m_alloc_table(struct domain *d);
-
 /* Context switch */
 void p2m_save_state(struct vcpu *p);
 void p2m_restore_state(struct vcpu *n);
-- 
1.9.1


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

[PATCH v2 09/15] xen/arm: p2m: Move the vttbr field from arch_domain to p2m_domain

[Julien Grall]

The field vttbr holds the base address of the translation table for
guest. Its value will depends on how the p2m has been initialized and
will only be used by the P2M code.

So move the field from arch_domain to p2m_domain. This will also ease
the implementation of altp2m.

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

---
    Changes in v2:
        - Forgot to add my signed-off-by
        - Fix typo in the commit message
---
 xen/arch/arm/p2m.c           | 11 +++++++----
 xen/arch/arm/traps.c         |  2 +-
 xen/include/asm-arm/domain.h |  1 -
 xen/include/asm-arm/p2m.h    |  3 +++
 4 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 512fd7d..7e524fe 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -107,10 +107,14 @@ void dump_p2m_lookup(struct domain *d, paddr_t addr)
 
 static void p2m_load_VTTBR(struct domain *d)
 {
+    struct p2m_domain *p2m = &d->arch.p2m;
+
     if ( is_idle_domain(d) )
         return;
-    BUG_ON(!d->arch.vttbr);
-    WRITE_SYSREG64(d->arch.vttbr, VTTBR_EL2);
+
+    ASSERT(p2m->vttbr);
+
+    WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2);
     isb(); /* Ensure update is visible */
 }
 
@@ -1297,8 +1301,7 @@ static int p2m_alloc_table(struct domain *d)
 
     p2m->root = page;
 
-    d->arch.vttbr = page_to_maddr(p2m->root)
-        | ((uint64_t)p2m->vmid&0xff)<<48;
+    p2m->vttbr = page_to_maddr(p2m->root) | ((uint64_t)p2m->vmid & 0xff) << 48;
 
     /*
      * Make sure that all TLBs corresponding to the new VMID are flushed
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 2482a20..f509a00 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -880,7 +880,7 @@ void vcpu_show_registers(const struct vcpu *v)
     ctxt.ifsr32_el2 = v->arch.ifsr;
 #endif
 
-    ctxt.vttbr_el2 = v->domain->arch.vttbr;
+    ctxt.vttbr_el2 = v->domain->arch.p2m.vttbr;
 
     _show_registers(&v->arch.cpu_info->guest_cpu_user_regs, &ctxt, 1, v);
 }
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 4e9d8bf..9452fcd 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -48,7 +48,6 @@ struct arch_domain
 
     /* Virtual MMU */
     struct p2m_domain p2m;
-    uint64_t vttbr;
 
     struct hvm_domain hvm_domain;
     gfn_t *grant_table_gfn;
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index ce28e8a..53c4d78 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -32,6 +32,9 @@ struct p2m_domain {
     /* Current VMID in use */
     uint8_t vmid;
 
+    /* Current Translation Table Base Register for the p2m */
+    uint64_t vttbr;
+
     /*
      * Highest guest frame that's ever been mapped in the p2m
      * Only takes into account ram and foreign mapping
-- 
1.9.1


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

[PATCH v2 10/15] xen/arm: p2m: Don't need to restore the state for an idle vCPU.

[Julien Grall]

The function p2m_restore_state could be called with an idle vCPU in
arguments (when called by construct_dom0). However, we will never return
to EL0/EL1 in this case, so it is not necessary to restore the p2m
registers.

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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 7e524fe..aff5906 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -127,6 +127,9 @@ void p2m_restore_state(struct vcpu *n)
 {
     register_t hcr;
 
+    if ( is_idle_vcpu(n) )
+        return;
+
     hcr = READ_SYSREG(HCR_EL2);
     WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2);
     isb();
-- 
1.9.1


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

[PATCH v2 11/15] xen/arm: p2m: Rework the context switch to another VTTBR in flush_tlb_domain

[Julien Grall]

The current implementation of flush_tlb_domain is relying on the domain
to have a single p2m. With the upcoming feature altp2m, a single domain
may have different p2m. So we would need to switch to the correct p2m in
order to flush the TLBs.

Rather than checking whether the domain is not the current domain, check
whether the VTTBR is different. The resulting assembly code is much
smaller: from 38 instructions (+ 2 functions call) to 22 instructions.

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

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index aff5906..7ee0171 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -151,24 +151,28 @@ void p2m_restore_state(struct vcpu *n)
 
 void flush_tlb_domain(struct domain *d)
 {
+    struct p2m_domain *p2m = &d->arch.p2m;
     unsigned long flags = 0;
+    uint64_t ovttbr;
 
     /*
-     * Update the VTTBR if necessary with the domain d. In this case,
-     * it's only necessary to flush TLBs on every CPUs with the current VMID
-     * (our domain).
+     * ARM only provides an instruction to flush TLBs for the current
+     * VMID. So switch to the VTTBR of a given P2M if different.
      */
-    if ( d != current->domain )
+    ovttbr = READ_SYSREG64(VTTBR_EL2);
+    if ( ovttbr != p2m->vttbr )
     {
         local_irq_save(flags);
-        p2m_load_VTTBR(d);
+        WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2);
+        isb();
     }
 
     flush_tlb();
 
-    if ( d != current->domain )
+    if ( ovttbr != READ_SYSREG64(VTTBR_EL2) )
     {
-        p2m_load_VTTBR(current->domain);
+        WRITE_SYSREG64(ovttbr, VTTBR_EL2);
+        isb();
         local_irq_restore(flags);
     }
 }
-- 
1.9.1


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

[PATCH v2 12/15] xen/arm: p2m: Inline p2m_load_VTTBR into p2m_restore_state

[Julien Grall]

p2m_restore_state is the last caller of p2m_load_VTTBR and already check
if the vCPU does not belong to the idle domain.

Note that it is likely possible to remove some isb in the function
p2m_restore_state, however this is not the purpose of this patch. So the
numerous isb have been left.

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, 2 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 7ee0171..6a9767c 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -105,19 +105,6 @@ void dump_p2m_lookup(struct domain *d, paddr_t addr)
                  P2M_ROOT_LEVEL, P2M_ROOT_PAGES);
 }
 
-static void p2m_load_VTTBR(struct domain *d)
-{
-    struct p2m_domain *p2m = &d->arch.p2m;
-
-    if ( is_idle_domain(d) )
-        return;
-
-    ASSERT(p2m->vttbr);
-
-    WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2);
-    isb(); /* Ensure update is visible */
-}
-
 void p2m_save_state(struct vcpu *p)
 {
     p->arch.sctlr = READ_SYSREG(SCTLR_EL1);
@@ -126,6 +113,7 @@ void p2m_save_state(struct vcpu *p)
 void p2m_restore_state(struct vcpu *n)
 {
     register_t hcr;
+    struct p2m_domain *p2m = &n->domain->arch.p2m;
 
     if ( is_idle_vcpu(n) )
         return;
@@ -134,7 +122,7 @@ void p2m_restore_state(struct vcpu *n)
     WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2);
     isb();
 
-    p2m_load_VTTBR(n->domain);
+    WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2);
     isb();
 
     if ( is_32bit_domain(n->domain) )
-- 
1.9.1


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

[PATCH v2 13/15] xen/arm: Don't export flush_tlb_domain

[Julien Grall]

The function flush_tlb_domain is not used outside of the file where it
has been declared.

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             | 2 +-
 xen/include/asm-arm/flushtlb.h | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 6a9767c..bda9b97 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -137,7 +137,7 @@ void p2m_restore_state(struct vcpu *n)
     isb();
 }
 
-void flush_tlb_domain(struct domain *d)
+static void flush_tlb_domain(struct domain *d)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
     unsigned long flags = 0;
diff --git a/xen/include/asm-arm/flushtlb.h b/xen/include/asm-arm/flushtlb.h
index c986b3f..329fbb4 100644
--- a/xen/include/asm-arm/flushtlb.h
+++ b/xen/include/asm-arm/flushtlb.h
@@ -25,9 +25,6 @@ do {                                                                    \
 /* Flush specified CPUs' TLBs */
 void flush_tlb_mask(const cpumask_t *mask);
 
-/* Flush CPU's TLBs for the specified domain */
-void flush_tlb_domain(struct domain *d);
-
 #endif /* __ASM_ARM_FLUSHTLB_H__ */
 /*
  * Local variables:
-- 
1.9.1


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

[PATCH v2 14/15] xen/arm: p2m: Replace flush_tlb_domain by p2m_flush_tlb

[Julien Grall]

The function to flush the TLBs for a given p2m does not need to know about
the domain. So pass directly the p2m in parameter.

At the same time rename the function to p2m_flush_tlb to match the
parameter change.

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 | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index bda9b97..97a3a2b 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -137,9 +137,8 @@ void p2m_restore_state(struct vcpu *n)
     isb();
 }
 
-static void flush_tlb_domain(struct domain *d)
+static void p2m_flush_tlb(struct p2m_domain *p2m)
 {
-    struct p2m_domain *p2m = &d->arch.p2m;
     unsigned long flags = 0;
     uint64_t ovttbr;
 
@@ -1157,7 +1156,7 @@ static int apply_p2m_changes(struct domain *d,
 out:
     if ( flush )
     {
-        flush_tlb_domain(d);
+        p2m_flush_tlb(&d->arch.p2m);
         ret = iommu_iotlb_flush(d, gfn_x(sgfn), nr);
         if ( !rc )
             rc = ret;
@@ -1302,7 +1301,7 @@ static int p2m_alloc_table(struct domain *d)
      * Make sure that all TLBs corresponding to the new VMID are flushed
      * before using it
      */
-    flush_tlb_domain(d);
+    p2m_flush_tlb(p2m);
 
     return 0;
 }
-- 
1.9.1


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

[PATCH v2 15/15] xen/arm: p2m: Pass the p2m in parameter rather the domain when it is possible

[Julien Grall]

Some p2m functions do not care about the domain except to get the
associate p2m.

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, 7 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 97a3a2b..40a0b80 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -415,10 +415,9 @@ 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 domain *d, lpae_t *entry,
+static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry,
                             int level_shift, bool_t flush_cache)
 {
-    struct p2m_domain *p2m = &d->arch.p2m;
     struct page_info *page;
     lpae_t *p;
     lpae_t pte;
@@ -652,18 +651,17 @@ static const paddr_t level_masks[] =
 static const paddr_t level_shifts[] =
     { ZEROETH_SHIFT, FIRST_SHIFT, SECOND_SHIFT, THIRD_SHIFT };
 
-static int p2m_shatter_page(struct domain *d,
+static int p2m_shatter_page(struct p2m_domain *p2m,
                             lpae_t *entry,
                             unsigned int level,
                             bool_t flush_cache)
 {
     const paddr_t level_shift = level_shifts[level];
-    int rc = p2m_create_table(d, entry,
+    int rc = p2m_create_table(p2m, entry,
                               level_shift - PAGE_SHIFT, flush_cache);
 
     if ( !rc )
     {
-        struct p2m_domain *p2m = &d->arch.p2m;
         p2m->stats.shattered[level]++;
         p2m->stats.mappings[level]--;
         p2m->stats.mappings[level+1] += LPAE_ENTRIES;
@@ -756,7 +754,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(d, entry, 0, flush_cache);
+                rc = p2m_create_table(p2m, entry, 0, flush_cache);
                 if ( rc < 0 )
                     return rc;
                 return P2M_ONE_DESCEND;
@@ -766,7 +764,7 @@ static int apply_one_level(struct domain *d,
             if ( p2m_mapping(orig_pte) )
             {
                 *flush = true;
-                rc = p2m_shatter_page(d, entry, level, flush_cache);
+                rc = p2m_shatter_page(p2m, entry, level, flush_cache);
                 if ( rc < 0 )
                     return rc;
             } /* else: an existing table mapping -> descend */
@@ -803,7 +801,7 @@ static int apply_one_level(struct domain *d,
                  * and descend.
                  */
                 *flush = true;
-                rc = p2m_shatter_page(d, entry, level, flush_cache);
+                rc = p2m_shatter_page(p2m, entry, level, flush_cache);
                 if ( rc < 0 )
                     return rc;
 
@@ -888,7 +886,7 @@ static int apply_one_level(struct domain *d,
             /* Shatter large pages as we descend */
             if ( p2m_mapping(orig_pte) )
             {
-                rc = p2m_shatter_page(d, entry, level, flush_cache);
+                rc = p2m_shatter_page(p2m, entry, level, flush_cache);
                 if ( rc < 0 )
                     return rc;
             } /* else: an existing table mapping -> descend */
-- 
1.9.1


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

Re: [PATCH v2 02/15] xen/arm: p2m: Use a whitelist rather than blacklist in get_page_from_gfn

[Stefano Stabellini]

On Thu, 28 Jul 2016, Julien Grall wrote:
> Currently, the check in get_page_from_gfn is using a blacklist. This is
> very fragile because we may forgot to update the check when a new p2m
> type is added.
> 
> To avoid any possible issue, use a whitelist. All type backed by a RAM
> page can could potential be valid. The check is borrowed from x86.
> 
> Note with this change, it is not possible anymore to retrieve a page when
> the p2m type is p2m_iommu_map_*. This is fine because they are special
> mappings for direct mapping workaround and the associated GFN should be
> used at all by callers of get_page_from_gfn.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>     Changes in v2:
>         - Update the commit message about iommu_mappings
> ---
>  xen/include/asm-arm/p2m.h | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 3091c04..78d37ab 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -104,9 +104,16 @@ typedef enum {
>  #define P2M_RAM_TYPES (p2m_to_mask(p2m_ram_rw) |        \
>                         p2m_to_mask(p2m_ram_ro))
>  
> +/* Grant mapping types, which map to a real frame in another VM */
> +#define P2M_GRANT_TYPES (p2m_to_mask(p2m_grant_map_rw) |  \
> +                         p2m_to_mask(p2m_grant_map_ro))
> +
>  /* Useful predicates */
>  #define p2m_is_ram(_t) (p2m_to_mask(_t) & P2M_RAM_TYPES)
>  #define p2m_is_foreign(_t) (p2m_to_mask(_t) & p2m_to_mask(p2m_map_foreign))
> +#define p2m_is_any_ram(_t) (p2m_to_mask(_t) &                   \
> +                            (P2M_RAM_TYPES | P2M_GRANT_TYPES |  \
> +                             p2m_to_mask(p2m_map_foreign)))
>  
>  static inline
>  void p2m_mem_access_emulate_check(struct vcpu *v,
> @@ -224,7 +231,7 @@ static inline struct page_info *get_page_from_gfn(
>      if (t)
>          *t = p2mt;
>  
> -    if ( p2mt == p2m_invalid || p2mt == p2m_mmio_direct )
> +    if ( !p2m_is_any_ram(p2mt) )
>          return NULL;
>  
>      if ( !mfn_valid(mfn) )
> -- 
> 1.9.1
> 

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

Re: [PATCH v2 09/15] xen/arm: p2m: Move the vttbr field from arch_domain to p2m_domain

[Stefano Stabellini]

On Thu, 28 Jul 2016, Julien Grall wrote:
> The field vttbr holds the base address of the translation table for
> guest. Its value will depends on how the p2m has been initialized and
> will only be used by the P2M code.
> 
> So move the field from arch_domain to p2m_domain. This will also ease
> the implementation of altp2m.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>     Changes in v2:
>         - Forgot to add my signed-off-by
>         - Fix typo in the commit message
> ---
>  xen/arch/arm/p2m.c           | 11 +++++++----
>  xen/arch/arm/traps.c         |  2 +-
>  xen/include/asm-arm/domain.h |  1 -
>  xen/include/asm-arm/p2m.h    |  3 +++
>  4 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 512fd7d..7e524fe 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -107,10 +107,14 @@ void dump_p2m_lookup(struct domain *d, paddr_t addr)
>  
>  static void p2m_load_VTTBR(struct domain *d)
>  {
> +    struct p2m_domain *p2m = &d->arch.p2m;
> +
>      if ( is_idle_domain(d) )
>          return;
> -    BUG_ON(!d->arch.vttbr);
> -    WRITE_SYSREG64(d->arch.vttbr, VTTBR_EL2);
> +
> +    ASSERT(p2m->vttbr);
> +
> +    WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2);
>      isb(); /* Ensure update is visible */
>  }
>  
> @@ -1297,8 +1301,7 @@ static int p2m_alloc_table(struct domain *d)
>  
>      p2m->root = page;
>  
> -    d->arch.vttbr = page_to_maddr(p2m->root)
> -        | ((uint64_t)p2m->vmid&0xff)<<48;
> +    p2m->vttbr = page_to_maddr(p2m->root) | ((uint64_t)p2m->vmid & 0xff) << 48;
>  
>      /*
>       * Make sure that all TLBs corresponding to the new VMID are flushed
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 2482a20..f509a00 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -880,7 +880,7 @@ void vcpu_show_registers(const struct vcpu *v)
>      ctxt.ifsr32_el2 = v->arch.ifsr;
>  #endif
>  
> -    ctxt.vttbr_el2 = v->domain->arch.vttbr;
> +    ctxt.vttbr_el2 = v->domain->arch.p2m.vttbr;
>  
>      _show_registers(&v->arch.cpu_info->guest_cpu_user_regs, &ctxt, 1, v);
>  }
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 4e9d8bf..9452fcd 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -48,7 +48,6 @@ struct arch_domain
>  
>      /* Virtual MMU */
>      struct p2m_domain p2m;
> -    uint64_t vttbr;
>  
>      struct hvm_domain hvm_domain;
>      gfn_t *grant_table_gfn;
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index ce28e8a..53c4d78 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -32,6 +32,9 @@ struct p2m_domain {
>      /* Current VMID in use */
>      uint8_t vmid;
>  
> +    /* Current Translation Table Base Register for the p2m */
> +    uint64_t vttbr;
> +
>      /*
>       * Highest guest frame that's ever been mapped in the p2m
>       * Only takes into account ram and foreign mapping
> -- 
> 1.9.1
> 

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

Re: [PATCH v2 11/15] xen/arm: p2m: Rework the context switch to another VTTBR in flush_tlb_domain

[Stefano Stabellini]

On Thu, 28 Jul 2016, Julien Grall wrote:
> The current implementation of flush_tlb_domain is relying on the domain
> to have a single p2m. With the upcoming feature altp2m, a single domain
> may have different p2m. So we would need to switch to the correct p2m in
> order to flush the TLBs.
> 
> Rather than checking whether the domain is not the current domain, check
> whether the VTTBR is different. The resulting assembly code is much
> smaller: from 38 instructions (+ 2 functions call) to 22 instructions.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


>  xen/arch/arm/p2m.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index aff5906..7ee0171 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -151,24 +151,28 @@ void p2m_restore_state(struct vcpu *n)
>  
>  void flush_tlb_domain(struct domain *d)
>  {
> +    struct p2m_domain *p2m = &d->arch.p2m;
>      unsigned long flags = 0;
> +    uint64_t ovttbr;
>  
>      /*
> -     * Update the VTTBR if necessary with the domain d. In this case,
> -     * it's only necessary to flush TLBs on every CPUs with the current VMID
> -     * (our domain).
> +     * ARM only provides an instruction to flush TLBs for the current
> +     * VMID. So switch to the VTTBR of a given P2M if different.
>       */
> -    if ( d != current->domain )
> +    ovttbr = READ_SYSREG64(VTTBR_EL2);
> +    if ( ovttbr != p2m->vttbr )
>      {
>          local_irq_save(flags);
> -        p2m_load_VTTBR(d);
> +        WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2);
> +        isb();
>      }
>  
>      flush_tlb();
>  
> -    if ( d != current->domain )
> +    if ( ovttbr != READ_SYSREG64(VTTBR_EL2) )
>      {
> -        p2m_load_VTTBR(current->domain);
> +        WRITE_SYSREG64(ovttbr, VTTBR_EL2);
> +        isb();
>          local_irq_restore(flags);
>      }
>  }
> -- 
> 1.9.1
> 

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

Re: [PATCH v2 00/15] xen/arm: P2M clean-up fixes

[Stefano Stabellini]

Committed, thanks

On Thu, 28 Jul 2016, Julien Grall wrote:
> Hello all,
> 
> This patch series contains a bunch of clean-up and fixes for the P2M code on
> ARM. The major changes are:
>     - Deduce the memory attributes from the p2m type
>     - Switch to read-write lock to improve performance
>     - Simplify the TLB flush for a give p2m
> 
> For all the changes see in each patch.
> 
> I have provided a branch with all the patches applied on my repo:
> git://xenbits.xen.org/people/julieng/xen-unstable.git branch p2m-cleanup-v2.
> 
> Yours sincerely,
> 
> Julien Grall (15):
>   xen/arm: p2m: Use the typesafe MFN in mfn_to_p2m_entry
>   xen/arm: p2m: Use a whitelist rather than blacklist in
>     get_page_from_gfn
>   xen/arm: p2m: Differentiate cacheable vs non-cacheable MMIO
>   xen/arm: p2m: Find the memory attributes based on the p2m type
>   xen/arm: p2m: Remove unnecessary locking
>   xen/arm: p2m: Introduce p2m_{read,write}_{,un}lock helpers
>   xen/arm: p2m: Switch the p2m lock from spinlock to rwlock
>   xen/arm: Don't call p2m_alloc_table from arch_domain_create
>   xen/arm: p2m: Move the vttbr field from arch_domain to p2m_domain
>   xen/arm: p2m: Don't need to restore the state for an idle vCPU.
>   xen/arm: p2m: Rework the context switch to another VTTBR in
>     flush_tlb_domain
>   xen/arm: p2m: Inline p2m_load_VTTBR into p2m_restore_state
>   xen/arm: Don't export flush_tlb_domain
>   xen/arm: p2m: Replace flush_tlb_domain by p2m_flush_tlb
>   xen/arm: p2m: Pass the p2m in parameter rather the domain when it is
>     possible
> 
>  xen/arch/arm/domain.c          |   3 -
>  xen/arch/arm/p2m.c             | 194 ++++++++++++++++++++---------------------
>  xen/arch/arm/traps.c           |   2 +-
>  xen/include/asm-arm/domain.h   |   1 -
>  xen/include/asm-arm/flushtlb.h |   3 -
>  xen/include/asm-arm/p2m.h      |  25 +++---
>  6 files changed, 113 insertions(+), 115 deletions(-)
> 
> -- 
> 1.9.1
> 

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