[PATCH 00/15] xen/arm: Bunch of clean-up/improvement

[PATCH 00/15] xen/arm: Bunch of clean-up/improvement

[Julien Grall]

Hi all,

This is patch series is a bunch of clean-up/improvement I collected while
working on the P2M and trap subsystems.

Cheers,

Julien Grall (15):
  xen/arm: cpregs: Allow HSR_CPREG* to receive more than 1 parameter
  xen/arm: cpregs: Fix typo in the documentation of TTBCR
  xen/arm: Introduce helpers to clear/flags flags in HCR_EL2
  xen/arm: p2m: Reduce the locking section in get_page_from_gva
  xen/arm: p2m: Limit call to mem access code use in get_page_from_gva
  xen/arm: Rework lpae_mapping
  xen/arm: Rework lpae_table
  xen/arm: Rename lpae_valid to lpae_is_valid
  xen/arm: guest_walk: Use lpae_is_mapping to simplify the code
  xen/arm: Introduce helpers to get/set an MFN from/to an LPAE entry
  xen/arm: Allow lpae_is_{table, mapping} helpers to work on invalid
    entry
  xen/arm: p2m: Rename ret to mfn in p2m_lookup
  xen/arm: p2m: Introduce a new variable removing_mapping in
    __p2m_set_entry
  xen/arm: guest_walk_tables: Switch the return to bool
  xen/arm: traps: Move the implementation of GUEST_BUG_ON in traps.h

 xen/arch/arm/guest_walk.c        |  54 +++++++++----------
 xen/arch/arm/mem_access.c        |   2 +-
 xen/arch/arm/mm.c                |  18 +++----
 xen/arch/arm/p2m.c               | 110 ++++++++++++++++++++++++---------------
 xen/arch/arm/traps.c             |  27 +---------
 xen/include/asm-arm/cpregs.h     |   6 +--
 xen/include/asm-arm/guest_walk.h |   8 +--
 xen/include/asm-arm/lpae.h       |  27 +++++-----
 xen/include/asm-arm/processor.h  |  18 +++++++
 xen/include/asm-arm/traps.h      |  24 +++++++++
 10 files changed, 168 insertions(+), 126 deletions(-)

-- 
2.11.0


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

[PATCH 01/15] xen/arm: cpregs: Allow HSR_CPREG* to receive more than 1 parameter

[Julien Grall]

At the moment, HSR_CPREG is expected to receive only the co-processor
register name in parameter. Because the name is actually a define, this
may have been expanded by a previous macro.

Rather than imposing the use of _HSR_CPREG* in such cases, allow
HSR_CPREG to receive more than 1 parameter.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/include/asm-arm/cpregs.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
index 8db65d5e2a..4c74e8161b 100644
--- a/xen/include/asm-arm/cpregs.h
+++ b/xen/include/asm-arm/cpregs.h
@@ -47,8 +47,8 @@
     ((__HSR_CPREG_##op1) << HSR_CP64_OP1_SHIFT)
 
 /* Encode a register as per HSR ISS pattern */
-#define HSR_CPREG32(X) _HSR_CPREG32(X)
-#define HSR_CPREG64(X) _HSR_CPREG64(X)
+#define HSR_CPREG32(X...) _HSR_CPREG32(X)
+#define HSR_CPREG64(X...) _HSR_CPREG64(X)
 
 /*
  * Order registers by Coprocessor-> CRn-> Opcode 1-> CRm-> Opcode 2
-- 
2.11.0


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

[PATCH 02/15] xen/arm: cpregs: Fix typo in the documentation of TTBCR

[Julien Grall]

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/include/asm-arm/cpregs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
index 4c74e8161b..07e5791983 100644
--- a/xen/include/asm-arm/cpregs.h
+++ b/xen/include/asm-arm/cpregs.h
@@ -141,7 +141,7 @@
 #define HSTR            p15,4,c1,c1,3   /* Hyp. System Trap Register */
 
 /* CP15 CR2: Translation Table Base and Control Registers */
-#define TTBCR           p15,0,c2,c0,2   /* Translatation Table Base Control Register */
+#define TTBCR           p15,0,c2,c0,2   /* Translation Table Base Control Register */
 #define TTBR0           p15,0,c2        /* Translation Table Base Reg. 0 */
 #define TTBR1           p15,1,c2        /* Translation Table Base Reg. 1 */
 #define HTTBR           p15,4,c2        /* Hyp. Translation Table Base Register */
-- 
2.11.0


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

[PATCH 03/15] xen/arm: Introduce helpers to clear/flags flags in HCR_EL2

[Julien Grall]

A couple of places in the code will need to clear/set flags in HCR_EL2
for a given vCPU and then replicate into the hardware. Introduce
helpers and replace open-coded version.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---

I haven't find a good place for those new helpers so stick to
processor.h at the moment. This require to use macro rather than inline
helpers given that processor.h is usually the root of all headers.
---
 xen/arch/arm/traps.c            |  3 +--
 xen/include/asm-arm/processor.h | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 9ae64ae6fc..d1bf69b245 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -681,8 +681,7 @@ static void inject_vabt_exception(struct cpu_user_regs *regs)
         break;
     }
 
-    current->arch.hcr_el2 |= HCR_VA;
-    WRITE_SYSREG(current->arch.hcr_el2, HCR_EL2);
+    vcpu_hcr_set_flags(current, HCR_VA);
 }
 
 /*
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 222a02dd99..7e695c2418 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -843,6 +843,24 @@ void abort_guest_exit_end(void);
                                  : : : "memory");                 \
     } while (0)
 
+/*
+ * Clear/Set flags in HCR_EL2 for a given vCPU. It only supports the current
+ * vCPU for now.
+ */
+#define vcpu_hcr_clear_flags(v, flags)              \
+    do {                                            \
+        ASSERT((v) == current);                     \
+        (v)->arch.hcr_el2 &= ~(flags);              \
+        WRITE_SYSREG((v)->arch.hcr_el2, HCR_EL2);   \
+    } while (0)
+
+#define vcpu_hcr_set_flags(v, flags)                \
+    do {                                            \
+        ASSERT((v) == current);                     \
+        (v)->arch.hcr_el2 |= (flags);               \
+        WRITE_SYSREG((v)->arch.hcr_el2, HCR_EL2);   \
+    } while (0)
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ASM_ARM_PROCESSOR_H */
 /*
-- 
2.11.0


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

[PATCH 04/15] xen/arm: p2m: Reduce the locking section in get_page_from_gva

[Julien Grall]

The p2m lock is only necessary to prevent gvirt_to_maddr failing when
break-before-make sequence is used in the P2M update concurrently on
another pCPU. So reduce the locking section.

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

---

    Note a newline has been dropped to keep the block together.
---
 xen/arch/arm/p2m.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 14791388ad..5ca7ffe41b 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1415,9 +1415,13 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
     if ( v != current )
         return NULL;
 
+    /*
+     * The lock is here to protect us against the break-before-make
+     * sequence used when updating the entry.
+     */
     p2m_read_lock(p2m);
-
     par = gvirt_to_maddr(va, &maddr, flags);
+    p2m_read_unlock(p2m);
 
     if ( par )
     {
@@ -1445,8 +1449,6 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
     }
 
 err:
-    p2m_read_unlock(p2m);
-
     if ( !page && p2m->mem_access_enabled )
         page = p2m_mem_access_check_and_get_page(va, flags, v);
 
-- 
2.11.0


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

[PATCH 05/15] xen/arm: p2m: Limit call to mem access code use in get_page_from_gva

[Julien Grall]

Mem access has only an impact on the hardware translation between a
guest virtual address and the machine physical address. So it is not
necessary to fallback to memaccess for all the other case (e.g when it
is not possible to acquire the page behind the MFN).

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

---

Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Tamas K Lengyel <tamas@tklengyel.com>
---
 xen/arch/arm/p2m.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 5ca7ffe41b..ebf74760fa 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1425,17 +1425,24 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
 
     if ( par )
     {
+        /*
+         * When memaccess is enabled, the translation GVA to MADDR may
+         * have failed because of a permission fault.
+         */
+        if ( p2m->mem_access_enabled )
+            return p2m_mem_access_check_and_get_page(va, flags, v);
+
         dprintk(XENLOG_G_DEBUG,
                 "%pv: gvirt_to_maddr failed va=%#"PRIvaddr" flags=0x%lx par=%#"PRIx64"\n",
                 v, va, flags, par);
-        goto err;
+        return NULL;
     }
 
     if ( !mfn_valid(maddr_to_mfn(maddr)) )
     {
         dprintk(XENLOG_G_DEBUG, "%pv: Invalid MFN %#"PRI_mfn"\n",
                 v, mfn_x(maddr_to_mfn(maddr)));
-        goto err;
+        return NULL;
     }
 
     page = mfn_to_page(maddr_to_mfn(maddr));
@@ -1445,13 +1452,9 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
     {
         dprintk(XENLOG_G_DEBUG, "%pv: Failing to acquire the MFN %#"PRI_mfn"\n",
                 v, mfn_x(maddr_to_mfn(maddr)));
-        page = NULL;
+        return NULL;
     }
 
-err:
-    if ( !page && p2m->mem_access_enabled )
-        page = p2m_mem_access_check_and_get_page(va, flags, v);
-
     return page;
 }
 
-- 
2.11.0


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

[PATCH 06/15] xen/arm: Rework lpae_mapping

[Julien Grall]

Currently, lpae_mapping can only work on entry from any level other than
3. Make it work with any level by extending the prototype to pass the
level.

At the same time, rename the function to lpae_is_mapping so naming stay
consistent accross lpae_* helpers.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/p2m.c         | 12 +++++++-----
 xen/include/asm-arm/lpae.h | 13 +++++++++----
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index ebf74760fa..72a84a33fd 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -241,7 +241,8 @@ static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry);
  *  GUEST_TABLE_SUPER_PAGE: The next entry points to a superpage.
  */
 static int p2m_next_level(struct p2m_domain *p2m, bool read_only,
-                          lpae_t **table, unsigned int offset)
+                          unsigned int level, lpae_t **table,
+                          unsigned int offset)
 {
     lpae_t *entry;
     int ret;
@@ -260,7 +261,8 @@ static int p2m_next_level(struct p2m_domain *p2m, bool read_only,
     }
 
     /* The function p2m_next_level is never called at the 3rd level */
-    if ( lpae_mapping(*entry) )
+    ASSERT(level < 3);
+    if ( lpae_is_mapping(*entry, level) )
         return GUEST_TABLE_SUPER_PAGE;
 
     mfn = _mfn(entry->p2m.base);
@@ -331,7 +333,7 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
 
     for ( level = P2M_ROOT_LEVEL; level < 3; level++ )
     {
-        rc = p2m_next_level(p2m, true, &table, offsets[level]);
+        rc = p2m_next_level(p2m, true, level, &table, offsets[level]);
         if ( rc == GUEST_TABLE_MAP_FAILED )
             goto out_unmap;
         else if ( rc != GUEST_TABLE_NORMAL_PAGE )
@@ -804,7 +806,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
          * is about to be removed (i.e mfn == INVALID_MFN).
          */
         rc = p2m_next_level(p2m, mfn_eq(smfn, INVALID_MFN),
-                            &table, offsets[level]);
+                            level, &table, offsets[level]);
         if ( rc == GUEST_TABLE_MAP_FAILED )
         {
             /*
@@ -861,7 +863,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
         /* then move to the level we want to make real changes */
         for ( ; level < target; level++ )
         {
-            rc = p2m_next_level(p2m, true, &table, offsets[level]);
+            rc = p2m_next_level(p2m, true, level, &table, offsets[level]);
 
             /*
              * The entry should be found and either be a table
diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
index b30853e79d..4cf188ff82 100644
--- a/xen/include/asm-arm/lpae.h
+++ b/xen/include/asm-arm/lpae.h
@@ -134,7 +134,7 @@ static inline bool lpae_valid(lpae_t pte)
 }
 
 /*
- * These two can only be used on L0..L2 ptes because L3 mappings set
+ * This one can only be used on L0..L2 ptes because L3 mappings set
  * the table bit and therefore these would return the opposite to what
  * you would expect.
  */
@@ -143,14 +143,19 @@ static inline bool lpae_table(lpae_t pte)
     return lpae_valid(pte) && pte.walk.table;
 }
 
-static inline bool lpae_mapping(lpae_t pte)
+static inline bool lpae_is_mapping(lpae_t pte, unsigned int level)
 {
-    return lpae_valid(pte) && !pte.walk.table;
+    if ( !lpae_valid(pte) )
+        return false;
+    else if ( level == 3 )
+        return pte.walk.table;
+    else
+        return !pte.walk.table;
 }
 
 static inline bool lpae_is_superpage(lpae_t pte, unsigned int level)
 {
-    return (level < 3) && lpae_mapping(pte);
+    return (level < 3) && lpae_is_mapping(pte, level);
 }
 
 static inline bool lpae_is_page(lpae_t pte, unsigned int level)
-- 
2.11.0


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

[PATCH 07/15] xen/arm: Rework lpae_table

[Julien Grall]

Currently, lpae_table can only work on entry from any level other than
3. Make it work with any level by extending the prototype to pass the
level.

At the same time, rename the function to lpae_is_mapping so naming stay
consistent accross all lpae_* helpers.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c          | 2 +-
 xen/include/asm-arm/lpae.h | 9 ++-------
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index d234c46e41..b7f2dabd05 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -996,7 +996,7 @@ static int create_xen_entries(enum xenmap_operation op,
     for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
     {
         entry = &xen_second[second_linear_offset(addr)];
-        if ( !lpae_table(*entry) )
+        if ( !lpae_is_table(*entry, 2) )
         {
             rc = create_xen_table(entry);
             if ( rc < 0 ) {
diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
index 4cf188ff82..c803569c2d 100644
--- a/xen/include/asm-arm/lpae.h
+++ b/xen/include/asm-arm/lpae.h
@@ -133,14 +133,9 @@ static inline bool lpae_valid(lpae_t pte)
     return pte.walk.valid;
 }
 
-/*
- * This one can only be used on L0..L2 ptes because L3 mappings set
- * the table bit and therefore these would return the opposite to what
- * you would expect.
- */
-static inline bool lpae_table(lpae_t pte)
+static inline bool lpae_is_table(lpae_t pte, unsigned int level)
 {
-    return lpae_valid(pte) && pte.walk.table;
+    return (level < 3) && lpae_valid(pte) && pte.walk.table;
 }
 
 static inline bool lpae_is_mapping(lpae_t pte, unsigned int level)
-- 
2.11.0


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

[PATCH 08/15] xen/arm: Rename lpae_valid to lpae_is_valid

[Julien Grall]

This will help to keep the naming consistent accross all lpae helpers.

No functional change intended.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/guest_walk.c  |  2 +-
 xen/arch/arm/mm.c          |  6 +++---
 xen/arch/arm/p2m.c         | 20 ++++++++++----------
 xen/include/asm-arm/lpae.h |  8 ++++----
 4 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index 4d1ea0cdc1..a7c7e05603 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -546,7 +546,7 @@ static int guest_walk_ld(const struct vcpu *v,
          * - The PTE is not valid.
          * - If (level < 3) and the PTE is valid, we found a block descriptor.
          */
-        if ( level == 3 || !lpae_valid(pte) || lpae_is_superpage(pte, level) )
+        if ( level == 3 || !lpae_is_valid(pte) || lpae_is_superpage(pte, level) )
             break;
 
         /*
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index b7f2dabd05..de9b965d2f 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1005,7 +1005,7 @@ static int create_xen_entries(enum xenmap_operation op,
             }
         }
 
-        BUG_ON(!lpae_valid(*entry));
+        BUG_ON(!lpae_is_valid(*entry));
 
         third = __mfn_to_virt(entry->pt.base);
         entry = &third[third_table_offset(addr)];
@@ -1013,7 +1013,7 @@ static int create_xen_entries(enum xenmap_operation op,
         switch ( op ) {
             case INSERT:
             case RESERVE:
-                if ( lpae_valid(*entry) )
+                if ( lpae_is_valid(*entry) )
                 {
                     printk("%s: trying to replace an existing mapping addr=%lx mfn=%"PRI_mfn"\n",
                            __func__, addr, mfn_x(mfn));
@@ -1030,7 +1030,7 @@ static int create_xen_entries(enum xenmap_operation op,
                 break;
             case MODIFY:
             case REMOVE:
-                if ( !lpae_valid(*entry) )
+                if ( !lpae_is_valid(*entry) )
                 {
                     printk("%s: trying to %s a non-existing mapping addr=%lx\n",
                            __func__, op == REMOVE ? "remove" : "modify", addr);
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 72a84a33fd..a80ac301c5 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -250,7 +250,7 @@ static int p2m_next_level(struct p2m_domain *p2m, bool read_only,
 
     entry = *table + offset;
 
-    if ( !lpae_valid(*entry) )
+    if ( !lpae_is_valid(*entry) )
     {
         if ( read_only )
             return GUEST_TABLE_MAP_FAILED;
@@ -342,7 +342,7 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
 
     entry = table[offsets[level]];
 
-    if ( lpae_valid(entry) )
+    if ( lpae_is_valid(entry) )
     {
         *t = entry.p2m.type;
 
@@ -546,7 +546,7 @@ static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry)
     lpae_t *p;
     lpae_t pte;
 
-    ASSERT(!lpae_valid(*entry));
+    ASSERT(!lpae_is_valid(*entry));
 
     page = alloc_domheap_page(NULL, 0);
     if ( page == NULL )
@@ -610,7 +610,7 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn,
  */
 static void p2m_put_l3_page(const lpae_t pte)
 {
-    ASSERT(lpae_valid(pte));
+    ASSERT(lpae_is_valid(pte));
 
     /*
      * TODO: Handle other p2m types
@@ -638,7 +638,7 @@ static void p2m_free_entry(struct p2m_domain *p2m,
     struct page_info *pg;
 
     /* Nothing to do if the entry is invalid. */
-    if ( !lpae_valid(entry) )
+    if ( !lpae_is_valid(entry) )
         return;
 
     /* Nothing to do but updating the stats if the entry is a super-page. */
@@ -908,12 +908,12 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
      * sequence when updating the translation table (D4.7.1 in ARM DDI
      * 0487A.j).
      */
-    if ( lpae_valid(orig_pte) )
+    if ( lpae_is_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 |= !!lpae_valid(orig_pte);
+        p2m->need_flush |= !!lpae_is_valid(orig_pte);
     else
     {
         lpae_t pte = mfn_to_p2m_entry(smfn, t, a);
@@ -928,7 +928,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
          * Although, it could be defered when only the permissions are
          * changed (e.g in case of memaccess).
          */
-        if ( lpae_valid(orig_pte) )
+        if ( lpae_is_valid(orig_pte) )
         {
             if ( likely(!p2m->mem_access_enabled) ||
                  P2M_CLEAR_PERM(pte) != P2M_CLEAR_PERM(orig_pte) )
@@ -950,11 +950,11 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
      * Free the entry only if the original pte was valid and the base
      * is different (to avoid freeing when permission is changed).
      */
-    if ( lpae_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base )
+    if ( lpae_is_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base )
         p2m_free_entry(p2m, orig_pte, level);
 
     if ( need_iommu(p2m->domain) &&
-         (lpae_valid(orig_pte) || lpae_valid(*entry)) )
+         (lpae_is_valid(orig_pte) || lpae_is_valid(*entry)) )
         rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << page_order);
     else
         rc = 0;
diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
index c803569c2d..1d86020d07 100644
--- a/xen/include/asm-arm/lpae.h
+++ b/xen/include/asm-arm/lpae.h
@@ -128,19 +128,19 @@ typedef union {
     lpae_walk_t walk;
 } lpae_t;
 
-static inline bool lpae_valid(lpae_t pte)
+static inline bool lpae_is_valid(lpae_t pte)
 {
     return pte.walk.valid;
 }
 
 static inline bool lpae_is_table(lpae_t pte, unsigned int level)
 {
-    return (level < 3) && lpae_valid(pte) && pte.walk.table;
+    return (level < 3) && lpae_is_valid(pte) && pte.walk.table;
 }
 
 static inline bool lpae_is_mapping(lpae_t pte, unsigned int level)
 {
-    if ( !lpae_valid(pte) )
+    if ( !lpae_is_valid(pte) )
         return false;
     else if ( level == 3 )
         return pte.walk.table;
@@ -155,7 +155,7 @@ static inline bool lpae_is_superpage(lpae_t pte, unsigned int level)
 
 static inline bool lpae_is_page(lpae_t pte, unsigned int level)
 {
-    return (level == 3) && lpae_valid(pte) && pte.walk.table;
+    return (level == 3) && lpae_is_valid(pte) && pte.walk.table;
 }
 
 /*
-- 
2.11.0


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

[PATCH 09/15] xen/arm: guest_walk: Use lpae_is_mapping to simplify the code

[Julien Grall]

!lpae_is_page(pte, level) && !lpae_is_superpage(pte, level) is
equivalent to !lpae_is_mapping(pte, level).

At the same time drop lpae_is_page(pte, level) that is now unused.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/guest_walk.c  | 2 +-
 xen/include/asm-arm/lpae.h | 5 -----
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index a7c7e05603..e3e21bdad3 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -566,7 +566,7 @@ static int guest_walk_ld(const struct vcpu *v,
      * PTE is invalid or holds a reserved entry (PTE<1:0> == x0)) or if the PTE
      * maps a memory block at level 3 (PTE<1:0> == 01).
      */
-    if ( !lpae_is_page(pte, level) && !lpae_is_superpage(pte, level) )
+    if ( !lpae_is_mapping(pte, level) )
         return -EFAULT;
 
     /* Make sure that the lower bits of the PTE's base address are zero. */
diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
index 1d86020d07..15595cd35c 100644
--- a/xen/include/asm-arm/lpae.h
+++ b/xen/include/asm-arm/lpae.h
@@ -153,11 +153,6 @@ static inline bool lpae_is_superpage(lpae_t pte, unsigned int level)
     return (level < 3) && lpae_is_mapping(pte, level);
 }
 
-static inline bool lpae_is_page(lpae_t pte, unsigned int level)
-{
-    return (level == 3) && lpae_is_valid(pte) && pte.walk.table;
-}
-
 /*
  * AArch64 supports pages with different sizes (4K, 16K, and 64K). To enable
  * page table walks for various configurations, the following helpers enable
-- 
2.11.0


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

[PATCH 10/15] xen/arm: Introduce helpers to get/set an MFN from/to an LPAE entry

[Julien Grall]

The new helpers make easier to read the code by abstracting the way to
set/get an MFN from/to an LPAE entry. The helpers is using "walk" as the
bits are common for accross different LPAE stage.

At the same time, use the new helpers to replace the various open-coding
place.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c          | 10 +++++-----
 xen/arch/arm/p2m.c         | 19 ++++++++++---------
 xen/include/asm-arm/lpae.h |  3 +++
 3 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index de9b965d2f..e3dafe5fd7 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -238,7 +238,7 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
 
         /* For next iteration */
         unmap_domain_page(mapping);
-        mapping = map_domain_page(_mfn(pte.walk.base));
+        mapping = map_domain_page(lpae_to_mfn(pte));
     }
 
     unmap_domain_page(mapping);
@@ -323,7 +323,7 @@ static inline lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned attr)
 
     ASSERT(!(mfn_to_maddr(mfn) & ~PADDR_MASK));
 
-    e.pt.base = mfn_x(mfn);
+    lpae_set_mfn(e, mfn);
 
     return e;
 }
@@ -490,7 +490,7 @@ mfn_t domain_page_map_to_mfn(const void *ptr)
     ASSERT(slot >= 0 && slot < DOMHEAP_ENTRIES);
     ASSERT(map[slot].pt.avail != 0);
 
-    return _mfn(map[slot].pt.base + offset);
+    return mfn_add(lpae_to_mfn(map[slot]), offset);
 }
 #endif
 
@@ -851,7 +851,7 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
             /* mfn_to_virt is not valid on the 1st 1st mfn, since it
              * is not within the xenheap. */
             first = slot == xenheap_first_first_slot ?
-                xenheap_first_first : __mfn_to_virt(p->pt.base);
+                xenheap_first_first : mfn_to_virt(lpae_to_mfn(*p));
         }
         else if ( xenheap_first_first_slot == -1)
         {
@@ -1007,7 +1007,7 @@ static int create_xen_entries(enum xenmap_operation op,
 
         BUG_ON(!lpae_is_valid(*entry));
 
-        third = __mfn_to_virt(entry->pt.base);
+        third = mfn_to_virt(lpae_to_mfn(*entry));
         entry = &third[third_table_offset(addr)];
 
         switch ( op ) {
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index a80ac301c5..ec3fdcb554 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -265,7 +265,7 @@ static int p2m_next_level(struct p2m_domain *p2m, bool read_only,
     if ( lpae_is_mapping(*entry, level) )
         return GUEST_TABLE_SUPER_PAGE;
 
-    mfn = _mfn(entry->p2m.base);
+    mfn = lpae_to_mfn(*entry);
 
     unmap_domain_page(*table);
     *table = map_domain_page(mfn);
@@ -349,7 +349,7 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
         if ( a )
             *a = p2m_mem_access_radix_get(p2m, gfn);
 
-        mfn = _mfn(entry.p2m.base);
+        mfn = lpae_to_mfn(entry);
         /*
          * The entry may point to a superpage. Find the MFN associated
          * to the GFN.
@@ -519,7 +519,7 @@ static lpae_t mfn_to_p2m_entry(mfn_t mfn, p2m_type_t t, p2m_access_t a)
 
     ASSERT(!(mfn_to_maddr(mfn) & ~PADDR_MASK));
 
-    e.p2m.base = mfn_x(mfn);
+    lpae_set_mfn(e, mfn);
 
     return e;
 }
@@ -621,7 +621,7 @@ static void p2m_put_l3_page(const lpae_t pte)
      */
     if ( p2m_is_foreign(pte.p2m.type) )
     {
-        mfn_t mfn = _mfn(pte.p2m.base);
+        mfn_t mfn = lpae_to_mfn(pte);
 
         ASSERT(mfn_valid(mfn));
         put_page(mfn_to_page(mfn));
@@ -655,7 +655,7 @@ static void p2m_free_entry(struct p2m_domain *p2m,
         return;
     }
 
-    table = map_domain_page(_mfn(entry.p2m.base));
+    table = map_domain_page(lpae_to_mfn(entry));
     for ( i = 0; i < LPAE_ENTRIES; i++ )
         p2m_free_entry(p2m, *(table + i), level + 1);
 
@@ -669,7 +669,7 @@ static void p2m_free_entry(struct p2m_domain *p2m,
      */
     p2m_tlb_flush_sync(p2m);
 
-    mfn = _mfn(entry.p2m.base);
+    mfn = lpae_to_mfn(entry);
     ASSERT(mfn_valid(mfn));
 
     pg = mfn_to_page(mfn);
@@ -688,7 +688,7 @@ static bool p2m_split_superpage(struct p2m_domain *p2m, lpae_t *entry,
     bool rv = true;
 
     /* Convenience aliases */
-    mfn_t mfn = _mfn(entry->p2m.base);
+    mfn_t mfn = lpae_to_mfn(*entry);
     unsigned int next_level = level + 1;
     unsigned int level_order = level_orders[next_level];
 
@@ -719,7 +719,7 @@ static bool p2m_split_superpage(struct p2m_domain *p2m, lpae_t *entry,
          * the necessary fields. So the correct permission are kept.
          */
         pte = *entry;
-        pte.p2m.base = mfn_x(mfn_add(mfn, i << level_order));
+        lpae_set_mfn(pte, mfn_add(mfn, i << level_order));
 
         /*
          * First and second level pages set p2m.table = 0, but third
@@ -950,7 +950,8 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
      * Free the entry only if the original pte was valid and the base
      * is different (to avoid freeing when permission is changed).
      */
-    if ( lpae_is_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base )
+    if ( lpae_is_valid(orig_pte) &&
+         !mfn_eq(lpae_to_mfn(*entry), lpae_to_mfn(orig_pte)) )
         p2m_free_entry(p2m, orig_pte, level);
 
     if ( need_iommu(p2m->domain) &&
diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
index 15595cd35c..05c87a8f48 100644
--- a/xen/include/asm-arm/lpae.h
+++ b/xen/include/asm-arm/lpae.h
@@ -153,6 +153,9 @@ static inline bool lpae_is_superpage(lpae_t pte, unsigned int level)
     return (level < 3) && lpae_is_mapping(pte, level);
 }
 
+#define lpae_to_mfn(pte)    (_mfn((pte).walk.base))
+#define lpae_set_mfn(pte, mfn)  ((pte).walk.base = mfn_x(mfn))
+
 /*
  * AArch64 supports pages with different sizes (4K, 16K, and 64K). To enable
  * page table walks for various configurations, the following helpers enable
-- 
2.11.0


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

[PATCH 11/15] xen/arm: Allow lpae_is_{table, mapping} helpers to work on invalid entry

[Julien Grall]

Currently, lpae_is_{table, mapping} helpers will always return false on
entry with the valid bit unset. However, it would be useful to have them
operating on any entry. For instance to store information in advance but
still request a fault.

With that change, the p2m is now providing an overlay for *_is_{table,
mapping} that will check the valid bit of the entry.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/guest_walk.c  |  2 +-
 xen/arch/arm/mm.c          |  2 +-
 xen/arch/arm/p2m.c         | 22 ++++++++++++++++++----
 xen/include/asm-arm/lpae.h | 11 +++++++----
 4 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index e3e21bdad3..4a1b4cf2c8 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -566,7 +566,7 @@ static int guest_walk_ld(const struct vcpu *v,
      * PTE is invalid or holds a reserved entry (PTE<1:0> == x0)) or if the PTE
      * maps a memory block at level 3 (PTE<1:0> == 01).
      */
-    if ( !lpae_is_mapping(pte, level) )
+    if ( !lpae_is_valid(pte) || !lpae_is_mapping(pte, level) )
         return -EFAULT;
 
     /* Make sure that the lower bits of the PTE's base address are zero. */
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index e3dafe5fd7..52e57fef2f 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -996,7 +996,7 @@ static int create_xen_entries(enum xenmap_operation op,
     for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
     {
         entry = &xen_second[second_linear_offset(addr)];
-        if ( !lpae_is_table(*entry, 2) )
+        if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
         {
             rc = create_xen_table(entry);
             if ( rc < 0 ) {
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index ec3fdcb554..07925a1be4 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -219,6 +219,20 @@ static p2m_access_t p2m_mem_access_radix_get(struct p2m_domain *p2m, gfn_t gfn)
         return radix_tree_ptr_to_int(ptr);
 }
 
+/*
+ * lpae_is_* helpers don't check whether the valid bit is set in the
+ * PTE. Provide our own overlay to check the valid bit.
+ */
+static inline bool p2m_is_mapping(lpae_t pte, unsigned int level)
+{
+    return lpae_is_valid(pte) && lpae_is_mapping(pte, level);
+}
+
+static inline bool p2m_is_superpage(lpae_t pte, unsigned int level)
+{
+    return lpae_is_valid(pte) && lpae_is_superpage(pte, level);
+}
+
 #define GUEST_TABLE_MAP_FAILED 0
 #define GUEST_TABLE_SUPER_PAGE 1
 #define GUEST_TABLE_NORMAL_PAGE 2
@@ -262,7 +276,7 @@ static int p2m_next_level(struct p2m_domain *p2m, bool read_only,
 
     /* The function p2m_next_level is never called at the 3rd level */
     ASSERT(level < 3);
-    if ( lpae_is_mapping(*entry, level) )
+    if ( p2m_is_mapping(*entry, level) )
         return GUEST_TABLE_SUPER_PAGE;
 
     mfn = lpae_to_mfn(*entry);
@@ -642,7 +656,7 @@ static void p2m_free_entry(struct p2m_domain *p2m,
         return;
 
     /* Nothing to do but updating the stats if the entry is a super-page. */
-    if ( lpae_is_superpage(entry, level) )
+    if ( p2m_is_superpage(entry, level) )
     {
         p2m->stats.mappings[level]--;
         return;
@@ -697,7 +711,7 @@ static bool p2m_split_superpage(struct p2m_domain *p2m, lpae_t *entry,
      * a superpage.
      */
     ASSERT(level < target);
-    ASSERT(lpae_is_superpage(*entry, level));
+    ASSERT(p2m_is_superpage(*entry, level));
 
     page = alloc_domheap_page(NULL, 0);
     if ( !page )
@@ -834,7 +848,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
         /* We need to split the original page. */
         lpae_t split_pte = *entry;
 
-        ASSERT(lpae_is_superpage(*entry, level));
+        ASSERT(p2m_is_superpage(*entry, level));
 
         if ( !p2m_split_superpage(p2m, &split_pte, level, target, offsets) )
         {
diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
index 05c87a8f48..88f30fc917 100644
--- a/xen/include/asm-arm/lpae.h
+++ b/xen/include/asm-arm/lpae.h
@@ -133,16 +133,19 @@ static inline bool lpae_is_valid(lpae_t pte)
     return pte.walk.valid;
 }
 
+/*
+ * lpae_is_* don't check the valid bit. This gives an opportunity for the
+ * callers to operate on the entry even if they are not valid. For
+ * instance to store information in advance.
+ */
 static inline bool lpae_is_table(lpae_t pte, unsigned int level)
 {
-    return (level < 3) && lpae_is_valid(pte) && pte.walk.table;
+    return (level < 3) && pte.walk.table;
 }
 
 static inline bool lpae_is_mapping(lpae_t pte, unsigned int level)
 {
-    if ( !lpae_is_valid(pte) )
-        return false;
-    else if ( level == 3 )
+    if ( level == 3 )
         return pte.walk.table;
     else
         return !pte.walk.table;
-- 
2.11.0


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

[PATCH 12/15] xen/arm: p2m: Rename ret to mfn in p2m_lookup

[Julien Grall]

Comestic change to make clearer what is the return ('ret' is a bit
too generic).

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 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 07925a1be4..66d58fabd7 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -383,14 +383,14 @@ out:
 
 mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
 {
-    mfn_t ret;
+    mfn_t mfn;
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
 
     p2m_read_lock(p2m);
-    ret = p2m_get_entry(p2m, gfn, t, NULL, NULL);
+    mfn = p2m_get_entry(p2m, gfn, t, NULL, NULL);
     p2m_read_unlock(p2m);
 
-    return ret;
+    return mfn;
 }
 
 int guest_physmap_mark_populate_on_demand(struct domain *d,
-- 
2.11.0


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

[PATCH 13/15] xen/arm: p2m: Introduce a new variable removing_mapping in __p2m_set_entry

[Julien Grall]

This is making the code slightly easier to understand.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/p2m.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 66d58fabd7..e826f57842 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -792,6 +792,8 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
     unsigned int target = 3 - (page_order / LPAE_SHIFT);
     lpae_t *entry, *table, orig_pte;
     int rc;
+    /* A mapping is removed if the MFN is invalid. */
+    bool removing_mapping = mfn_eq(smfn, INVALID_MFN);
 
     /* Convenience aliases */
     const unsigned int offsets[4] = {
@@ -817,9 +819,9 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
     {
         /*
          * Don't try to allocate intermediate page table if the mapping
-         * is about to be removed (i.e mfn == INVALID_MFN).
+         * is about to be removed.
          */
-        rc = p2m_next_level(p2m, mfn_eq(smfn, INVALID_MFN),
+        rc = p2m_next_level(p2m, removing_mapping,
                             level, &table, offsets[level]);
         if ( rc == GUEST_TABLE_MAP_FAILED )
         {
@@ -830,7 +832,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
              * 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;
+            rc = removing_mapping ?  0 : -ENOENT;
             goto out;
         }
         else if ( rc != GUEST_TABLE_NORMAL_PAGE )
@@ -925,7 +927,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
     if ( lpae_is_valid(orig_pte) )
         p2m_remove_pte(entry, p2m->clean_pte);
 
-    if ( mfn_eq(smfn, INVALID_MFN) )
+    if ( removing_mapping )
         /* Flush can be deferred if the entry is removed */
         p2m->need_flush |= !!lpae_is_valid(orig_pte);
     else
-- 
2.11.0


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

[PATCH 14/15] xen/arm: guest_walk_tables: Switch the return to bool

[Julien Grall]

At the moment, guest_walk_tables can either return 0, -EFAULT, -EINVAL.
The use of the last 2 are not clearly defined and used inconsistently in
the code. The current only caller does not care about the return
value and the value of it seems very limited (no way to differentiate
between the 15ish error paths).

So switch to bool to simplify the return and make the developper life a
bit easier.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/guest_walk.c        | 50 ++++++++++++++++++++--------------------
 xen/arch/arm/mem_access.c        |  2 +-
 xen/include/asm-arm/guest_walk.h |  8 +++----
 3 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index 4a1b4cf2c8..7db7a7321b 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -28,9 +28,9 @@
  * page table on a different vCPU, the following registers would need to be
  * loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and SCTLR_EL1.
  */
-static int guest_walk_sd(const struct vcpu *v,
-                         vaddr_t gva, paddr_t *ipa,
-                         unsigned int *perms)
+static bool guest_walk_sd(const struct vcpu *v,
+                          vaddr_t gva, paddr_t *ipa,
+                          unsigned int *perms)
 {
     int ret;
     bool disabled = true;
@@ -79,7 +79,7 @@ static int guest_walk_sd(const struct vcpu *v,
     }
 
     if ( disabled )
-        return -EFAULT;
+        return false;
 
     /*
      * The address of the L1 descriptor for the initial lookup has the
@@ -97,12 +97,12 @@ static int guest_walk_sd(const struct vcpu *v,
     /* Access the guest's memory to read only one PTE. */
     ret = access_guest_memory_by_ipa(d, paddr, &pte, sizeof(short_desc_t), false);
     if ( ret )
-        return -EINVAL;
+        return false;
 
     switch ( pte.walk.dt )
     {
     case L1DESC_INVALID:
-        return -EFAULT;
+        return false;
 
     case L1DESC_PAGE_TABLE:
         /*
@@ -122,10 +122,10 @@ static int guest_walk_sd(const struct vcpu *v,
         /* Access the guest's memory to read only one PTE. */
         ret = access_guest_memory_by_ipa(d, paddr, &pte, sizeof(short_desc_t), false);
         if ( ret )
-            return -EINVAL;
+            return false;
 
         if ( pte.walk.dt == L2DESC_INVALID )
-            return -EFAULT;
+            return false;
 
         if ( pte.pg.page ) /* Small page. */
         {
@@ -175,7 +175,7 @@ static int guest_walk_sd(const struct vcpu *v,
             *perms |= GV2M_EXEC;
     }
 
-    return 0;
+    return true;
 }
 
 /*
@@ -355,9 +355,9 @@ static bool check_base_size(unsigned int output_size, uint64_t base)
  * page table on a different vCPU, the following registers would need to be
  * loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and SCTLR_EL1.
  */
-static int guest_walk_ld(const struct vcpu *v,
-                         vaddr_t gva, paddr_t *ipa,
-                         unsigned int *perms)
+static bool guest_walk_ld(const struct vcpu *v,
+                          vaddr_t gva, paddr_t *ipa,
+                          unsigned int *perms)
 {
     int ret;
     bool disabled = true;
@@ -442,7 +442,7 @@ static int guest_walk_ld(const struct vcpu *v,
          */
         if ( (input_size > TCR_EL1_IPS_48_BIT_VAL) ||
              (input_size < TCR_EL1_IPS_MIN_VAL) )
-            return -EFAULT;
+            return false;
     }
     else
     {
@@ -487,7 +487,7 @@ static int guest_walk_ld(const struct vcpu *v,
     }
 
     if ( disabled )
-        return -EFAULT;
+        return false;
 
     /*
      * The starting level is the number of strides (grainsizes[gran] - 3)
@@ -498,12 +498,12 @@ static int guest_walk_ld(const struct vcpu *v,
     /* Get the IPA output_size. */
     ret = get_ipa_output_size(d, tcr, &output_size);
     if ( ret )
-        return -EFAULT;
+        return false;
 
     /* Make sure the base address does not exceed its configured size. */
     ret = check_base_size(output_size, ttbr);
     if ( !ret )
-        return -EFAULT;
+        return false;
 
     /*
      * Compute the base address of the first level translation table that is
@@ -523,12 +523,12 @@ static int guest_walk_ld(const struct vcpu *v,
         /* Access the guest's memory to read only one PTE. */
         ret = access_guest_memory_by_ipa(d, paddr, &pte, sizeof(lpae_t), false);
         if ( ret )
-            return -EFAULT;
+            return false;
 
         /* Make sure the base address does not exceed its configured size. */
         ret = check_base_size(output_size, pfn_to_paddr(pte.walk.base));
         if ( !ret )
-            return -EFAULT;
+            return false;
 
         /*
          * If page granularity is 64K, make sure the address is aligned
@@ -537,7 +537,7 @@ static int guest_walk_ld(const struct vcpu *v,
         if ( (output_size < TCR_EL1_IPS_52_BIT_VAL) &&
              (gran == GRANULE_SIZE_INDEX_64K) &&
              (pte.walk.base & 0xf) )
-            return -EFAULT;
+            return false;
 
         /*
          * Break if one of the following conditions is true:
@@ -567,7 +567,7 @@ static int guest_walk_ld(const struct vcpu *v,
      * maps a memory block at level 3 (PTE<1:0> == 01).
      */
     if ( !lpae_is_valid(pte) || !lpae_is_mapping(pte, level) )
-        return -EFAULT;
+        return false;
 
     /* Make sure that the lower bits of the PTE's base address are zero. */
     mask = GENMASK_ULL(47, grainsizes[gran]);
@@ -583,11 +583,11 @@ static int guest_walk_ld(const struct vcpu *v,
     if ( !pte.pt.xn && !xn_table )
         *perms |= GV2M_EXEC;
 
-    return 0;
+    return true;
 }
 
-int guest_walk_tables(const struct vcpu *v, vaddr_t gva,
-                      paddr_t *ipa, unsigned int *perms)
+bool guest_walk_tables(const struct vcpu *v, vaddr_t gva,
+                       paddr_t *ipa, unsigned int *perms)
 {
     uint32_t sctlr = READ_SYSREG(SCTLR_EL1);
     register_t tcr = READ_SYSREG(TCR_EL1);
@@ -595,7 +595,7 @@ int guest_walk_tables(const struct vcpu *v, vaddr_t gva,
 
     /* We assume that the domain is running on the currently active domain. */
     if ( v != current )
-        return -EFAULT;
+        return false;
 
     /* Allow perms to be NULL. */
     perms = perms ?: &_perms;
@@ -619,7 +619,7 @@ int guest_walk_tables(const struct vcpu *v, vaddr_t gva,
         /* Memory can be accessed without any restrictions. */
         *perms = GV2M_READ|GV2M_WRITE|GV2M_EXEC;
 
-        return 0;
+        return true;
     }
 
     if ( is_32bit_domain(v->domain) && !(tcr & TTBCR_EAE) )
diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
index ae2686ffa2..57ec7872bb 100644
--- a/xen/arch/arm/mem_access.c
+++ b/xen/arch/arm/mem_access.c
@@ -125,7 +125,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
          * The software gva to ipa translation can still fail, e.g., if the gva
          * is not mapped.
          */
-        if ( guest_walk_tables(v, gva, &ipa, &perms) < 0 )
+        if ( !guest_walk_tables(v, gva, &ipa, &perms) )
             return NULL;
 
         /*
diff --git a/xen/include/asm-arm/guest_walk.h b/xen/include/asm-arm/guest_walk.h
index 4ed8476e08..8768ac9894 100644
--- a/xen/include/asm-arm/guest_walk.h
+++ b/xen/include/asm-arm/guest_walk.h
@@ -2,10 +2,10 @@
 #define _XEN_GUEST_WALK_H
 
 /* Walk the guest's page tables in software. */
-int guest_walk_tables(const struct vcpu *v,
-                      vaddr_t gva,
-                      paddr_t *ipa,
-                      unsigned int *perms);
+bool guest_walk_tables(const struct vcpu *v,
+                       vaddr_t gva,
+                       paddr_t *ipa,
+                       unsigned int *perms);
 
 #endif /* _XEN_GUEST_WALK_H */
 
-- 
2.11.0


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

[PATCH 15/15] xen/arm: traps: Move the implementation of GUEST_BUG_ON in traps.h

[Julien Grall]

GUEST_BUG_ON may be used in other files doing guest emulation.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/traps.c        | 24 ------------------------
 xen/include/asm-arm/traps.h | 24 ++++++++++++++++++++++++
 2 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index d1bf69b245..6751e4d754 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -68,30 +68,6 @@ static inline void check_stack_alignment_constraints(void) {
 #endif
 }
 
-/*
- * GUEST_BUG_ON is intended for checking that the guest state has not been
- * corrupted in hardware and/or that the hardware behaves as we
- * believe it should (i.e. that certain traps can only occur when the
- * guest is in a particular mode).
- *
- * The intention is to limit the damage such h/w bugs (or spec
- * misunderstandings) can do by turning them into Denial of Service
- * attacks instead of e.g. information leaks or privilege escalations.
- *
- * GUEST_BUG_ON *MUST* *NOT* be used to check for guest controllable state!
- *
- * Compared with regular BUG_ON it dumps the guest vcpu state instead
- * of Xen's state.
- */
-#define guest_bug_on_failed(p)                          \
-do {                                                    \
-    show_execution_state(guest_cpu_user_regs());        \
-    panic("Guest Bug: %pv: '%s', line %d, file %s\n",   \
-          current, p, __LINE__, __FILE__);              \
-} while (0)
-#define GUEST_BUG_ON(p) \
-    do { if ( unlikely(p) ) guest_bug_on_failed(#p); } while (0)
-
 #ifdef CONFIG_ARM_32
 static int debug_stack_lines = 20;
 #define stack_words_per_line 8
diff --git a/xen/include/asm-arm/traps.h b/xen/include/asm-arm/traps.h
index 70b52d1d16..0acf7de67d 100644
--- a/xen/include/asm-arm/traps.h
+++ b/xen/include/asm-arm/traps.h
@@ -9,6 +9,30 @@
 # include <asm/arm64/traps.h>
 #endif
 
+/*
+ * GUEST_BUG_ON is intended for checking that the guest state has not been
+ * corrupted in hardware and/or that the hardware behaves as we
+ * believe it should (i.e. that certain traps can only occur when the
+ * guest is in a particular mode).
+ *
+ * The intention is to limit the damage such h/w bugs (or spec
+ * misunderstandings) can do by turning them into Denial of Service
+ * attacks instead of e.g. information leaks or privilege escalations.
+ *
+ * GUEST_BUG_ON *MUST* *NOT* be used to check for guest controllable state!
+ *
+ * Compared with regular BUG_ON it dumps the guest vcpu state instead
+ * of Xen's state.
+ */
+#define guest_bug_on_failed(p)                          \
+do {                                                    \
+    show_execution_state(guest_cpu_user_regs());        \
+    panic("Guest Bug: %pv: '%s', line %d, file %s\n",   \
+          current, p, __LINE__, __FILE__);              \
+} while (0)
+#define GUEST_BUG_ON(p) \
+    do { if ( unlikely(p) ) guest_bug_on_failed(#p); } while (0)
+
 int check_conditional_instr(struct cpu_user_regs *regs, const union hsr hsr);
 
 void advance_pc(struct cpu_user_regs *regs, const union hsr hsr);
-- 
2.11.0


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

Re: [PATCH 05/15] xen/arm: p2m: Limit call to mem access code use in get_page_from_gva

[Razvan Cojocaru]

On 07/16/2018 08:27 PM, Julien Grall wrote:
> Mem access has only an impact on the hardware translation between a
> guest virtual address and the machine physical address. So it is not
> necessary to fallback to memaccess for all the other case (e.g when it
> is not possible to acquire the page behind the MFN).
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


Thanks,
Razvan

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

Re: [PATCH 00/15] xen/arm: Bunch of clean-up/improvement

[Julien Grall]

Ping?

Cheers,

On 16/07/18 18:26, Julien Grall wrote:
> Hi all,
> 
> This is patch series is a bunch of clean-up/improvement I collected while
> working on the P2M and trap subsystems.
> 
> Cheers,
> 
> Julien Grall (15):
>    xen/arm: cpregs: Allow HSR_CPREG* to receive more than 1 parameter
>    xen/arm: cpregs: Fix typo in the documentation of TTBCR
>    xen/arm: Introduce helpers to clear/flags flags in HCR_EL2
>    xen/arm: p2m: Reduce the locking section in get_page_from_gva
>    xen/arm: p2m: Limit call to mem access code use in get_page_from_gva
>    xen/arm: Rework lpae_mapping
>    xen/arm: Rework lpae_table
>    xen/arm: Rename lpae_valid to lpae_is_valid
>    xen/arm: guest_walk: Use lpae_is_mapping to simplify the code
>    xen/arm: Introduce helpers to get/set an MFN from/to an LPAE entry
>    xen/arm: Allow lpae_is_{table, mapping} helpers to work on invalid
>      entry
>    xen/arm: p2m: Rename ret to mfn in p2m_lookup
>    xen/arm: p2m: Introduce a new variable removing_mapping in
>      __p2m_set_entry
>    xen/arm: guest_walk_tables: Switch the return to bool
>    xen/arm: traps: Move the implementation of GUEST_BUG_ON in traps.h
> 
>   xen/arch/arm/guest_walk.c        |  54 +++++++++----------
>   xen/arch/arm/mem_access.c        |   2 +-
>   xen/arch/arm/mm.c                |  18 +++----
>   xen/arch/arm/p2m.c               | 110 ++++++++++++++++++++++++---------------
>   xen/arch/arm/traps.c             |  27 +---------
>   xen/include/asm-arm/cpregs.h     |   6 +--
>   xen/include/asm-arm/guest_walk.h |   8 +--
>   xen/include/asm-arm/lpae.h       |  27 +++++-----
>   xen/include/asm-arm/processor.h  |  18 +++++++
>   xen/include/asm-arm/traps.h      |  24 +++++++++
>   10 files changed, 168 insertions(+), 126 deletions(-)
> 

-- 
Julien Grall

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

Re: [PATCH 02/15] xen/arm: cpregs: Fix typo in the documentation of TTBCR

[Stefano Stabellini]

On Mon, 16 Jul 2018, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

> ---
>  xen/include/asm-arm/cpregs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
> index 4c74e8161b..07e5791983 100644
> --- a/xen/include/asm-arm/cpregs.h
> +++ b/xen/include/asm-arm/cpregs.h
> @@ -141,7 +141,7 @@
>  #define HSTR            p15,4,c1,c1,3   /* Hyp. System Trap Register */
>  
>  /* CP15 CR2: Translation Table Base and Control Registers */
> -#define TTBCR           p15,0,c2,c0,2   /* Translatation Table Base Control Register */
> +#define TTBCR           p15,0,c2,c0,2   /* Translation Table Base Control Register */
>  #define TTBR0           p15,0,c2        /* Translation Table Base Reg. 0 */
>  #define TTBR1           p15,1,c2        /* Translation Table Base Reg. 1 */
>  #define HTTBR           p15,4,c2        /* Hyp. Translation Table Base Register */
> -- 
> 2.11.0
> 

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

Re: [PATCH 03/15] xen/arm: Introduce helpers to clear/flags flags in HCR_EL2

[Stefano Stabellini]

On Mon, 16 Jul 2018, Julien Grall wrote:
> A couple of places in the code will need to clear/set flags in HCR_EL2
> for a given vCPU and then replicate into the hardware. Introduce
> helpers and replace open-coded version.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

The macros look good, but I grepped for them in your series and there
are no more callers. What places are you referring to that they will
need them?


> ---
> 
> I haven't find a good place for those new helpers so stick to
> processor.h at the moment. This require to use macro rather than inline
> helpers given that processor.h is usually the root of all headers.
> ---
>  xen/arch/arm/traps.c            |  3 +--
>  xen/include/asm-arm/processor.h | 18 ++++++++++++++++++
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 9ae64ae6fc..d1bf69b245 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -681,8 +681,7 @@ static void inject_vabt_exception(struct cpu_user_regs *regs)
>          break;
>      }
>  
> -    current->arch.hcr_el2 |= HCR_VA;
> -    WRITE_SYSREG(current->arch.hcr_el2, HCR_EL2);
> +    vcpu_hcr_set_flags(current, HCR_VA);
>  }
>  
>  /*
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 222a02dd99..7e695c2418 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -843,6 +843,24 @@ void abort_guest_exit_end(void);
>                                   : : : "memory");                 \
>      } while (0)
>  
> +/*
> + * Clear/Set flags in HCR_EL2 for a given vCPU. It only supports the current
> + * vCPU for now.
> + */
> +#define vcpu_hcr_clear_flags(v, flags)              \
> +    do {                                            \
> +        ASSERT((v) == current);                     \
> +        (v)->arch.hcr_el2 &= ~(flags);              \
> +        WRITE_SYSREG((v)->arch.hcr_el2, HCR_EL2);   \
> +    } while (0)
> +
> +#define vcpu_hcr_set_flags(v, flags)                \
> +    do {                                            \
> +        ASSERT((v) == current);                     \
> +        (v)->arch.hcr_el2 |= (flags);               \
> +        WRITE_SYSREG((v)->arch.hcr_el2, HCR_EL2);   \
> +    } while (0)
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* __ASM_ARM_PROCESSOR_H */
>  /*
> -- 
> 2.11.0
> 

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

Re: [PATCH 04/15] xen/arm: p2m: Reduce the locking section in get_page_from_gva

[Stefano Stabellini]

On Mon, 16 Jul 2018, Julien Grall wrote:
> The p2m lock is only necessary to prevent gvirt_to_maddr failing when
> break-before-make sequence is used in the P2M update concurrently on
> another pCPU. So reduce the locking section.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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

> ---
> 
>     Note a newline has been dropped to keep the block together.
> ---
>  xen/arch/arm/p2m.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 14791388ad..5ca7ffe41b 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1415,9 +1415,13 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
>      if ( v != current )
>          return NULL;
>  
> +    /*
> +     * The lock is here to protect us against the break-before-make
> +     * sequence used when updating the entry.
> +     */
>      p2m_read_lock(p2m);
> -
>      par = gvirt_to_maddr(va, &maddr, flags);
> +    p2m_read_unlock(p2m);
>  
>      if ( par )
>      {
> @@ -1445,8 +1449,6 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
>      }
>  
>  err:
> -    p2m_read_unlock(p2m);
> -
>      if ( !page && p2m->mem_access_enabled )
>          page = p2m_mem_access_check_and_get_page(va, flags, v);
>  
> -- 
> 2.11.0
> 

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

Re: [PATCH 05/15] xen/arm: p2m: Limit call to mem access code use in get_page_from_gva

[Stefano Stabellini]

On Mon, 16 Jul 2018, Julien Grall wrote:
> Mem access has only an impact on the hardware translation between a
> guest virtual address and the machine physical address. So it is not
> necessary to fallback to memaccess for all the other case (e.g when it
> is not possible to acquire the page behind the MFN).
> 
> 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>
> ---
>  xen/arch/arm/p2m.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 5ca7ffe41b..ebf74760fa 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1425,17 +1425,24 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
>  
>      if ( par )
>      {
> +        /*
> +         * When memaccess is enabled, the translation GVA to MADDR may
> +         * have failed because of a permission fault.
> +         */
> +        if ( p2m->mem_access_enabled )
> +            return p2m_mem_access_check_and_get_page(va, flags, v);
> +
>          dprintk(XENLOG_G_DEBUG,
>                  "%pv: gvirt_to_maddr failed va=%#"PRIvaddr" flags=0x%lx par=%#"PRIx64"\n",
>                  v, va, flags, par);
> -        goto err;
> +        return NULL;
>      }
>  
>      if ( !mfn_valid(maddr_to_mfn(maddr)) )
>      {
>          dprintk(XENLOG_G_DEBUG, "%pv: Invalid MFN %#"PRI_mfn"\n",
>                  v, mfn_x(maddr_to_mfn(maddr)));
> -        goto err;
> +        return NULL;
>      }
>  
>      page = mfn_to_page(maddr_to_mfn(maddr));
> @@ -1445,13 +1452,9 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
>      {
>          dprintk(XENLOG_G_DEBUG, "%pv: Failing to acquire the MFN %#"PRI_mfn"\n",
>                  v, mfn_x(maddr_to_mfn(maddr)));
> -        page = NULL;
> +        return NULL;
>      }
>  
> -err:
> -    if ( !page && p2m->mem_access_enabled )
> -        page = p2m_mem_access_check_and_get_page(va, flags, v);
> -
>      return page;
>  }
>  
> -- 
> 2.11.0
> 

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

Re: [PATCH 06/15] xen/arm: Rework lpae_mapping

[Stefano Stabellini]

On Mon, 16 Jul 2018, Julien Grall wrote:
> Currently, lpae_mapping can only work on entry from any level other than
> 3. Make it work with any level by extending the prototype to pass the
> level.
> 
> At the same time, rename the function to lpae_is_mapping so naming stay
> consistent accross lpae_* helpers.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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

> ---
>  xen/arch/arm/p2m.c         | 12 +++++++-----
>  xen/include/asm-arm/lpae.h | 13 +++++++++----
>  2 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index ebf74760fa..72a84a33fd 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -241,7 +241,8 @@ static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry);
>   *  GUEST_TABLE_SUPER_PAGE: The next entry points to a superpage.
>   */
>  static int p2m_next_level(struct p2m_domain *p2m, bool read_only,
> -                          lpae_t **table, unsigned int offset)
> +                          unsigned int level, lpae_t **table,
> +                          unsigned int offset)
>  {
>      lpae_t *entry;
>      int ret;
> @@ -260,7 +261,8 @@ static int p2m_next_level(struct p2m_domain *p2m, bool read_only,
>      }
>  
>      /* The function p2m_next_level is never called at the 3rd level */
> -    if ( lpae_mapping(*entry) )
> +    ASSERT(level < 3);
> +    if ( lpae_is_mapping(*entry, level) )
>          return GUEST_TABLE_SUPER_PAGE;
>  
>      mfn = _mfn(entry->p2m.base);
> @@ -331,7 +333,7 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>  
>      for ( level = P2M_ROOT_LEVEL; level < 3; level++ )
>      {
> -        rc = p2m_next_level(p2m, true, &table, offsets[level]);
> +        rc = p2m_next_level(p2m, true, level, &table, offsets[level]);
>          if ( rc == GUEST_TABLE_MAP_FAILED )
>              goto out_unmap;
>          else if ( rc != GUEST_TABLE_NORMAL_PAGE )
> @@ -804,7 +806,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>           * is about to be removed (i.e mfn == INVALID_MFN).
>           */
>          rc = p2m_next_level(p2m, mfn_eq(smfn, INVALID_MFN),
> -                            &table, offsets[level]);
> +                            level, &table, offsets[level]);
>          if ( rc == GUEST_TABLE_MAP_FAILED )
>          {
>              /*
> @@ -861,7 +863,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>          /* then move to the level we want to make real changes */
>          for ( ; level < target; level++ )
>          {
> -            rc = p2m_next_level(p2m, true, &table, offsets[level]);
> +            rc = p2m_next_level(p2m, true, level, &table, offsets[level]);
>  
>              /*
>               * The entry should be found and either be a table
> diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
> index b30853e79d..4cf188ff82 100644
> --- a/xen/include/asm-arm/lpae.h
> +++ b/xen/include/asm-arm/lpae.h
> @@ -134,7 +134,7 @@ static inline bool lpae_valid(lpae_t pte)
>  }
>  
>  /*
> - * These two can only be used on L0..L2 ptes because L3 mappings set
> + * This one can only be used on L0..L2 ptes because L3 mappings set
>   * the table bit and therefore these would return the opposite to what
>   * you would expect.
>   */
> @@ -143,14 +143,19 @@ static inline bool lpae_table(lpae_t pte)
>      return lpae_valid(pte) && pte.walk.table;
>  }
>  
> -static inline bool lpae_mapping(lpae_t pte)
> +static inline bool lpae_is_mapping(lpae_t pte, unsigned int level)
>  {
> -    return lpae_valid(pte) && !pte.walk.table;
> +    if ( !lpae_valid(pte) )
> +        return false;
> +    else if ( level == 3 )
> +        return pte.walk.table;
> +    else
> +        return !pte.walk.table;
>  }
>  
>  static inline bool lpae_is_superpage(lpae_t pte, unsigned int level)
>  {
> -    return (level < 3) && lpae_mapping(pte);
> +    return (level < 3) && lpae_is_mapping(pte, level);
>  }
>  
>  static inline bool lpae_is_page(lpae_t pte, unsigned int level)
> -- 
> 2.11.0
> 

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

Re: [PATCH 07/15] xen/arm: Rework lpae_table

[Stefano Stabellini]

On Mon, 16 Jul 2018, Julien Grall wrote:
> Currently, lpae_table can only work on entry from any level other than
> 3. Make it work with any level by extending the prototype to pass the
> level.
> 
> At the same time, rename the function to lpae_is_mapping so naming stay
> consistent accross all lpae_* helpers.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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

> ---
>  xen/arch/arm/mm.c          | 2 +-
>  xen/include/asm-arm/lpae.h | 9 ++-------
>  2 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index d234c46e41..b7f2dabd05 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -996,7 +996,7 @@ static int create_xen_entries(enum xenmap_operation op,
>      for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
>      {
>          entry = &xen_second[second_linear_offset(addr)];
> -        if ( !lpae_table(*entry) )
> +        if ( !lpae_is_table(*entry, 2) )
>          {
>              rc = create_xen_table(entry);
>              if ( rc < 0 ) {
> diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
> index 4cf188ff82..c803569c2d 100644
> --- a/xen/include/asm-arm/lpae.h
> +++ b/xen/include/asm-arm/lpae.h
> @@ -133,14 +133,9 @@ static inline bool lpae_valid(lpae_t pte)
>      return pte.walk.valid;
>  }
>  
> -/*
> - * This one can only be used on L0..L2 ptes because L3 mappings set
> - * the table bit and therefore these would return the opposite to what
> - * you would expect.
> - */
> -static inline bool lpae_table(lpae_t pte)
> +static inline bool lpae_is_table(lpae_t pte, unsigned int level)
>  {
> -    return lpae_valid(pte) && pte.walk.table;
> +    return (level < 3) && lpae_valid(pte) && pte.walk.table;
>  }
>  
>  static inline bool lpae_is_mapping(lpae_t pte, unsigned int level)

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

Re: [PATCH 08/15] xen/arm: Rename lpae_valid to lpae_is_valid

[Stefano Stabellini]

On Mon, 16 Jul 2018, Julien Grall wrote:
> This will help to keep the naming consistent accross all lpae helpers.
> 
> No functional change intended.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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

> ---
>  xen/arch/arm/guest_walk.c  |  2 +-
>  xen/arch/arm/mm.c          |  6 +++---
>  xen/arch/arm/p2m.c         | 20 ++++++++++----------
>  xen/include/asm-arm/lpae.h |  8 ++++----
>  4 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index 4d1ea0cdc1..a7c7e05603 100644
> --- a/xen/arch/arm/guest_walk.c
> +++ b/xen/arch/arm/guest_walk.c
> @@ -546,7 +546,7 @@ static int guest_walk_ld(const struct vcpu *v,
>           * - The PTE is not valid.
>           * - If (level < 3) and the PTE is valid, we found a block descriptor.
>           */
> -        if ( level == 3 || !lpae_valid(pte) || lpae_is_superpage(pte, level) )
> +        if ( level == 3 || !lpae_is_valid(pte) || lpae_is_superpage(pte, level) )
>              break;
>  
>          /*
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index b7f2dabd05..de9b965d2f 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1005,7 +1005,7 @@ static int create_xen_entries(enum xenmap_operation op,
>              }
>          }
>  
> -        BUG_ON(!lpae_valid(*entry));
> +        BUG_ON(!lpae_is_valid(*entry));
>  
>          third = __mfn_to_virt(entry->pt.base);
>          entry = &third[third_table_offset(addr)];
> @@ -1013,7 +1013,7 @@ static int create_xen_entries(enum xenmap_operation op,
>          switch ( op ) {
>              case INSERT:
>              case RESERVE:
> -                if ( lpae_valid(*entry) )
> +                if ( lpae_is_valid(*entry) )
>                  {
>                      printk("%s: trying to replace an existing mapping addr=%lx mfn=%"PRI_mfn"\n",
>                             __func__, addr, mfn_x(mfn));
> @@ -1030,7 +1030,7 @@ static int create_xen_entries(enum xenmap_operation op,
>                  break;
>              case MODIFY:
>              case REMOVE:
> -                if ( !lpae_valid(*entry) )
> +                if ( !lpae_is_valid(*entry) )
>                  {
>                      printk("%s: trying to %s a non-existing mapping addr=%lx\n",
>                             __func__, op == REMOVE ? "remove" : "modify", addr);
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 72a84a33fd..a80ac301c5 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -250,7 +250,7 @@ static int p2m_next_level(struct p2m_domain *p2m, bool read_only,
>  
>      entry = *table + offset;
>  
> -    if ( !lpae_valid(*entry) )
> +    if ( !lpae_is_valid(*entry) )
>      {
>          if ( read_only )
>              return GUEST_TABLE_MAP_FAILED;
> @@ -342,7 +342,7 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>  
>      entry = table[offsets[level]];
>  
> -    if ( lpae_valid(entry) )
> +    if ( lpae_is_valid(entry) )
>      {
>          *t = entry.p2m.type;
>  
> @@ -546,7 +546,7 @@ static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry)
>      lpae_t *p;
>      lpae_t pte;
>  
> -    ASSERT(!lpae_valid(*entry));
> +    ASSERT(!lpae_is_valid(*entry));
>  
>      page = alloc_domheap_page(NULL, 0);
>      if ( page == NULL )
> @@ -610,7 +610,7 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn,
>   */
>  static void p2m_put_l3_page(const lpae_t pte)
>  {
> -    ASSERT(lpae_valid(pte));
> +    ASSERT(lpae_is_valid(pte));
>  
>      /*
>       * TODO: Handle other p2m types
> @@ -638,7 +638,7 @@ static void p2m_free_entry(struct p2m_domain *p2m,
>      struct page_info *pg;
>  
>      /* Nothing to do if the entry is invalid. */
> -    if ( !lpae_valid(entry) )
> +    if ( !lpae_is_valid(entry) )
>          return;
>  
>      /* Nothing to do but updating the stats if the entry is a super-page. */
> @@ -908,12 +908,12 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>       * sequence when updating the translation table (D4.7.1 in ARM DDI
>       * 0487A.j).
>       */
> -    if ( lpae_valid(orig_pte) )
> +    if ( lpae_is_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 |= !!lpae_valid(orig_pte);
> +        p2m->need_flush |= !!lpae_is_valid(orig_pte);
>      else
>      {
>          lpae_t pte = mfn_to_p2m_entry(smfn, t, a);
> @@ -928,7 +928,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>           * Although, it could be defered when only the permissions are
>           * changed (e.g in case of memaccess).
>           */
> -        if ( lpae_valid(orig_pte) )
> +        if ( lpae_is_valid(orig_pte) )
>          {
>              if ( likely(!p2m->mem_access_enabled) ||
>                   P2M_CLEAR_PERM(pte) != P2M_CLEAR_PERM(orig_pte) )
> @@ -950,11 +950,11 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>       * Free the entry only if the original pte was valid and the base
>       * is different (to avoid freeing when permission is changed).
>       */
> -    if ( lpae_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base )
> +    if ( lpae_is_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base )
>          p2m_free_entry(p2m, orig_pte, level);
>  
>      if ( need_iommu(p2m->domain) &&
> -         (lpae_valid(orig_pte) || lpae_valid(*entry)) )
> +         (lpae_is_valid(orig_pte) || lpae_is_valid(*entry)) )
>          rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << page_order);
>      else
>          rc = 0;
> diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
> index c803569c2d..1d86020d07 100644
> --- a/xen/include/asm-arm/lpae.h
> +++ b/xen/include/asm-arm/lpae.h
> @@ -128,19 +128,19 @@ typedef union {
>      lpae_walk_t walk;
>  } lpae_t;
>  
> -static inline bool lpae_valid(lpae_t pte)
> +static inline bool lpae_is_valid(lpae_t pte)
>  {
>      return pte.walk.valid;
>  }
>  
>  static inline bool lpae_is_table(lpae_t pte, unsigned int level)
>  {
> -    return (level < 3) && lpae_valid(pte) && pte.walk.table;
> +    return (level < 3) && lpae_is_valid(pte) && pte.walk.table;
>  }
>  
>  static inline bool lpae_is_mapping(lpae_t pte, unsigned int level)
>  {
> -    if ( !lpae_valid(pte) )
> +    if ( !lpae_is_valid(pte) )
>          return false;
>      else if ( level == 3 )
>          return pte.walk.table;
> @@ -155,7 +155,7 @@ static inline bool lpae_is_superpage(lpae_t pte, unsigned int level)
>  
>  static inline bool lpae_is_page(lpae_t pte, unsigned int level)
>  {
> -    return (level == 3) && lpae_valid(pte) && pte.walk.table;
> +    return (level == 3) && lpae_is_valid(pte) && pte.walk.table;
>  }
>  
>  /*
> -- 
> 2.11.0
> 

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

Re: [PATCH 09/15] xen/arm: guest_walk: Use lpae_is_mapping to simplify the code

[Stefano Stabellini]

On Mon, 16 Jul 2018, Julien Grall wrote:
> !lpae_is_page(pte, level) && !lpae_is_superpage(pte, level) is
> equivalent to !lpae_is_mapping(pte, level).
> 
> At the same time drop lpae_is_page(pte, level) that is now unused.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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

> ---
>  xen/arch/arm/guest_walk.c  | 2 +-
>  xen/include/asm-arm/lpae.h | 5 -----
>  2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index a7c7e05603..e3e21bdad3 100644
> --- a/xen/arch/arm/guest_walk.c
> +++ b/xen/arch/arm/guest_walk.c
> @@ -566,7 +566,7 @@ static int guest_walk_ld(const struct vcpu *v,
>       * PTE is invalid or holds a reserved entry (PTE<1:0> == x0)) or if the PTE
>       * maps a memory block at level 3 (PTE<1:0> == 01).
>       */
> -    if ( !lpae_is_page(pte, level) && !lpae_is_superpage(pte, level) )
> +    if ( !lpae_is_mapping(pte, level) )
>          return -EFAULT;
>  
>      /* Make sure that the lower bits of the PTE's base address are zero. */
> diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
> index 1d86020d07..15595cd35c 100644
> --- a/xen/include/asm-arm/lpae.h
> +++ b/xen/include/asm-arm/lpae.h
> @@ -153,11 +153,6 @@ static inline bool lpae_is_superpage(lpae_t pte, unsigned int level)
>      return (level < 3) && lpae_is_mapping(pte, level);
>  }
>  
> -static inline bool lpae_is_page(lpae_t pte, unsigned int level)
> -{
> -    return (level == 3) && lpae_is_valid(pte) && pte.walk.table;
> -}
> -
>  /*
>   * AArch64 supports pages with different sizes (4K, 16K, and 64K). To enable
>   * page table walks for various configurations, the following helpers enable
> -- 
> 2.11.0
> 

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

Re: [PATCH 03/15] xen/arm: Introduce helpers to clear/flags flags in HCR_EL2

[Julien Grall]

Hi Stefano,

On 08/14/2018 09:46 PM, Stefano Stabellini wrote:
> On Mon, 16 Jul 2018, Julien Grall wrote:
>> A couple of places in the code will need to clear/set flags in HCR_EL2
>> for a given vCPU and then replicate into the hardware. Introduce
>> helpers and replace open-coded version.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> The macros look good, but I grepped for them in your series and there
> are no more callers. What places are you referring to that they will
> need them?

I split some of my work in 2 part to reduce the size of the series. This 
will be used in another series where updating HCR for trapping 
TTBR/SCTLR* will be dynamic.

Although in general, this is an improvement compare to the current code 
as it makes clear what needs to be updated when modifying HCR.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 10/15] xen/arm: Introduce helpers to get/set an MFN from/to an LPAE entry

[Stefano Stabellini]

On Mon, 16 Jul 2018, Julien Grall wrote:
> The new helpers make easier to read the code by abstracting the way to
                      ^ it

> set/get an MFN from/to an LPAE entry. The helpers is using "walk" as the
                                                    ^are

> bits are common for accross different LPAE stage.
                  ^ across                   ^ stages


> At the same time, use the new helpers to replace the various open-coding
> place.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/mm.c          | 10 +++++-----
>  xen/arch/arm/p2m.c         | 19 ++++++++++---------
>  xen/include/asm-arm/lpae.h |  3 +++
>  3 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index de9b965d2f..e3dafe5fd7 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -238,7 +238,7 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
>  
>          /* For next iteration */
>          unmap_domain_page(mapping);
> -        mapping = map_domain_page(_mfn(pte.walk.base));
> +        mapping = map_domain_page(lpae_to_mfn(pte));
>      }
>  
>      unmap_domain_page(mapping);
> @@ -323,7 +323,7 @@ static inline lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned attr)
>  
>      ASSERT(!(mfn_to_maddr(mfn) & ~PADDR_MASK));
>  
> -    e.pt.base = mfn_x(mfn);
> +    lpae_set_mfn(e, mfn);
>  
>      return e;
>  }
> @@ -490,7 +490,7 @@ mfn_t domain_page_map_to_mfn(const void *ptr)
>      ASSERT(slot >= 0 && slot < DOMHEAP_ENTRIES);
>      ASSERT(map[slot].pt.avail != 0);
>  
> -    return _mfn(map[slot].pt.base + offset);
> +    return mfn_add(lpae_to_mfn(map[slot]), offset);
>  }
>  #endif
>  
> @@ -851,7 +851,7 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
>              /* mfn_to_virt is not valid on the 1st 1st mfn, since it
>               * is not within the xenheap. */
>              first = slot == xenheap_first_first_slot ?
> -                xenheap_first_first : __mfn_to_virt(p->pt.base);
> +                xenheap_first_first : mfn_to_virt(lpae_to_mfn(*p));
>          }
>          else if ( xenheap_first_first_slot == -1)
>          {
> @@ -1007,7 +1007,7 @@ static int create_xen_entries(enum xenmap_operation op,
>  
>          BUG_ON(!lpae_is_valid(*entry));
>  
> -        third = __mfn_to_virt(entry->pt.base);
> +        third = mfn_to_virt(lpae_to_mfn(*entry));
>          entry = &third[third_table_offset(addr)];
>  
>          switch ( op ) {
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index a80ac301c5..ec3fdcb554 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -265,7 +265,7 @@ static int p2m_next_level(struct p2m_domain *p2m, bool read_only,
>      if ( lpae_is_mapping(*entry, level) )
>          return GUEST_TABLE_SUPER_PAGE;
>  
> -    mfn = _mfn(entry->p2m.base);
> +    mfn = lpae_to_mfn(*entry);
>  
>      unmap_domain_page(*table);
>      *table = map_domain_page(mfn);
> @@ -349,7 +349,7 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>          if ( a )
>              *a = p2m_mem_access_radix_get(p2m, gfn);
>  
> -        mfn = _mfn(entry.p2m.base);
> +        mfn = lpae_to_mfn(entry);
>          /*
>           * The entry may point to a superpage. Find the MFN associated
>           * to the GFN.
> @@ -519,7 +519,7 @@ static lpae_t mfn_to_p2m_entry(mfn_t mfn, p2m_type_t t, p2m_access_t a)
>  
>      ASSERT(!(mfn_to_maddr(mfn) & ~PADDR_MASK));
>  
> -    e.p2m.base = mfn_x(mfn);
> +    lpae_set_mfn(e, mfn);
>  
>      return e;
>  }
> @@ -621,7 +621,7 @@ static void p2m_put_l3_page(const lpae_t pte)
>       */
>      if ( p2m_is_foreign(pte.p2m.type) )
>      {
> -        mfn_t mfn = _mfn(pte.p2m.base);
> +        mfn_t mfn = lpae_to_mfn(pte);
>  
>          ASSERT(mfn_valid(mfn));
>          put_page(mfn_to_page(mfn));
> @@ -655,7 +655,7 @@ static void p2m_free_entry(struct p2m_domain *p2m,
>          return;
>      }
>  
> -    table = map_domain_page(_mfn(entry.p2m.base));
> +    table = map_domain_page(lpae_to_mfn(entry));
>      for ( i = 0; i < LPAE_ENTRIES; i++ )
>          p2m_free_entry(p2m, *(table + i), level + 1);
>  
> @@ -669,7 +669,7 @@ static void p2m_free_entry(struct p2m_domain *p2m,
>       */
>      p2m_tlb_flush_sync(p2m);
>  
> -    mfn = _mfn(entry.p2m.base);
> +    mfn = lpae_to_mfn(entry);
>      ASSERT(mfn_valid(mfn));
>  
>      pg = mfn_to_page(mfn);
> @@ -688,7 +688,7 @@ static bool p2m_split_superpage(struct p2m_domain *p2m, lpae_t *entry,
>      bool rv = true;
>  
>      /* Convenience aliases */
> -    mfn_t mfn = _mfn(entry->p2m.base);
> +    mfn_t mfn = lpae_to_mfn(*entry);
>      unsigned int next_level = level + 1;
>      unsigned int level_order = level_orders[next_level];
>  
> @@ -719,7 +719,7 @@ static bool p2m_split_superpage(struct p2m_domain *p2m, lpae_t *entry,
>           * the necessary fields. So the correct permission are kept.
>           */
>          pte = *entry;
> -        pte.p2m.base = mfn_x(mfn_add(mfn, i << level_order));
> +        lpae_set_mfn(pte, mfn_add(mfn, i << level_order));
>  
>          /*
>           * First and second level pages set p2m.table = 0, but third
> @@ -950,7 +950,8 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>       * Free the entry only if the original pte was valid and the base
>       * is different (to avoid freeing when permission is changed).
>       */
> -    if ( lpae_is_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base )
> +    if ( lpae_is_valid(orig_pte) &&
> +         !mfn_eq(lpae_to_mfn(*entry), lpae_to_mfn(orig_pte)) )
>          p2m_free_entry(p2m, orig_pte, level);
>  
>      if ( need_iommu(p2m->domain) &&
> diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
> index 15595cd35c..05c87a8f48 100644
> --- a/xen/include/asm-arm/lpae.h
> +++ b/xen/include/asm-arm/lpae.h
> @@ -153,6 +153,9 @@ static inline bool lpae_is_superpage(lpae_t pte, unsigned int level)
>      return (level < 3) && lpae_is_mapping(pte, level);
>  }
>  
> +#define lpae_to_mfn(pte)    (_mfn((pte).walk.base))

I would call it lpae_get_mfn, so that they become lpae_get/set_mfn.
The changes look correct in this patch.


> +#define lpae_set_mfn(pte, mfn)  ((pte).walk.base = mfn_x(mfn))
> +
>  /*
>   * AArch64 supports pages with different sizes (4K, 16K, and 64K). To enable
>   * page table walks for various configurations, the following helpers enable

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

Re: [PATCH 11/15] xen/arm: Allow lpae_is_{table, mapping} helpers to work on invalid entry

[Stefano Stabellini]

On Mon, 16 Jul 2018, Julien Grall wrote:
> Currently, lpae_is_{table, mapping} helpers will always return false on
> entry with the valid bit unset. However, it would be useful to have them
  ^entries

> operating on any entry. For instance to store information in advance but
> still request a fault.
> 
> With that change, the p2m is now providing an overlay for *_is_{table,
> mapping} that will check the valid bit of the entry.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> ---
>  xen/arch/arm/guest_walk.c  |  2 +-
>  xen/arch/arm/mm.c          |  2 +-
>  xen/arch/arm/p2m.c         | 22 ++++++++++++++++++----
>  xen/include/asm-arm/lpae.h | 11 +++++++----
>  4 files changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index e3e21bdad3..4a1b4cf2c8 100644
> --- a/xen/arch/arm/guest_walk.c
> +++ b/xen/arch/arm/guest_walk.c
> @@ -566,7 +566,7 @@ static int guest_walk_ld(const struct vcpu *v,
>       * PTE is invalid or holds a reserved entry (PTE<1:0> == x0)) or if the PTE
>       * maps a memory block at level 3 (PTE<1:0> == 01).
>       */
> -    if ( !lpae_is_mapping(pte, level) )
> +    if ( !lpae_is_valid(pte) || !lpae_is_mapping(pte, level) )
>          return -EFAULT;
>  
>      /* Make sure that the lower bits of the PTE's base address are zero. */
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index e3dafe5fd7..52e57fef2f 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -996,7 +996,7 @@ static int create_xen_entries(enum xenmap_operation op,
>      for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
>      {
>          entry = &xen_second[second_linear_offset(addr)];
> -        if ( !lpae_is_table(*entry, 2) )
> +        if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
>          {
>              rc = create_xen_table(entry);
>              if ( rc < 0 ) {
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index ec3fdcb554..07925a1be4 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -219,6 +219,20 @@ static p2m_access_t p2m_mem_access_radix_get(struct p2m_domain *p2m, gfn_t gfn)
>          return radix_tree_ptr_to_int(ptr);
>  }
>  
> +/*
> + * lpae_is_* helpers don't check whether the valid bit is set in the
> + * PTE. Provide our own overlay to check the valid bit.
> + */
> +static inline bool p2m_is_mapping(lpae_t pte, unsigned int level)
> +{
> +    return lpae_is_valid(pte) && lpae_is_mapping(pte, level);
> +}
> +
> +static inline bool p2m_is_superpage(lpae_t pte, unsigned int level)
> +{
> +    return lpae_is_valid(pte) && lpae_is_superpage(pte, level);
> +}

I like the idea, but it would be clearer to me if they were called
lpae_is_valid_mapping and lpae_is_valid_superpage respectively. What do
you think?

Also, we might as well move them to lpae.h and use them from mm.c and
guest_walk.c as appropriate?


>  #define GUEST_TABLE_MAP_FAILED 0
>  #define GUEST_TABLE_SUPER_PAGE 1
>  #define GUEST_TABLE_NORMAL_PAGE 2
> @@ -262,7 +276,7 @@ static int p2m_next_level(struct p2m_domain *p2m, bool read_only,
>  
>      /* The function p2m_next_level is never called at the 3rd level */
>      ASSERT(level < 3);
> -    if ( lpae_is_mapping(*entry, level) )
> +    if ( p2m_is_mapping(*entry, level) )
>          return GUEST_TABLE_SUPER_PAGE;
>  
>      mfn = lpae_to_mfn(*entry);
> @@ -642,7 +656,7 @@ static void p2m_free_entry(struct p2m_domain *p2m,
>          return;
>  
>      /* Nothing to do but updating the stats if the entry is a super-page. */
> -    if ( lpae_is_superpage(entry, level) )
> +    if ( p2m_is_superpage(entry, level) )
>      {
>          p2m->stats.mappings[level]--;
>          return;
> @@ -697,7 +711,7 @@ static bool p2m_split_superpage(struct p2m_domain *p2m, lpae_t *entry,
>       * a superpage.
>       */
>      ASSERT(level < target);
> -    ASSERT(lpae_is_superpage(*entry, level));
> +    ASSERT(p2m_is_superpage(*entry, level));
>  
>      page = alloc_domheap_page(NULL, 0);
>      if ( !page )
> @@ -834,7 +848,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>          /* We need to split the original page. */
>          lpae_t split_pte = *entry;
>  
> -        ASSERT(lpae_is_superpage(*entry, level));
> +        ASSERT(p2m_is_superpage(*entry, level));
>  
>          if ( !p2m_split_superpage(p2m, &split_pte, level, target, offsets) )
>          {
> diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
> index 05c87a8f48..88f30fc917 100644
> --- a/xen/include/asm-arm/lpae.h
> +++ b/xen/include/asm-arm/lpae.h
> @@ -133,16 +133,19 @@ static inline bool lpae_is_valid(lpae_t pte)
>      return pte.walk.valid;
>  }
>  
> +/*
> + * lpae_is_* don't check the valid bit. This gives an opportunity for the
> + * callers to operate on the entry even if they are not valid. For
> + * instance to store information in advance.
> + */
>  static inline bool lpae_is_table(lpae_t pte, unsigned int level)
>  {
> -    return (level < 3) && lpae_is_valid(pte) && pte.walk.table;
> +    return (level < 3) && pte.walk.table;
>  }
>  
>  static inline bool lpae_is_mapping(lpae_t pte, unsigned int level)
>  {
> -    if ( !lpae_is_valid(pte) )
> -        return false;
> -    else if ( level == 3 )
> +    if ( level == 3 )
>          return pte.walk.table;
>      else
>          return !pte.walk.table;
> -- 
> 2.11.0
> 

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

Re: [PATCH 12/15] xen/arm: p2m: Rename ret to mfn in p2m_lookup

[Stefano Stabellini]

On Mon, 16 Jul 2018, Julien Grall wrote:
> Comestic change to make clearer what is the return ('ret' is a bit
> too generic).
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

> ---
>  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 07925a1be4..66d58fabd7 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -383,14 +383,14 @@ out:
>  
>  mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
>  {
> -    mfn_t ret;
> +    mfn_t mfn;
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>  
>      p2m_read_lock(p2m);
> -    ret = p2m_get_entry(p2m, gfn, t, NULL, NULL);
> +    mfn = p2m_get_entry(p2m, gfn, t, NULL, NULL);
>      p2m_read_unlock(p2m);
>  
> -    return ret;
> +    return mfn;
>  }
>  
>  int guest_physmap_mark_populate_on_demand(struct domain *d,
> -- 
> 2.11.0
> 

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

Re: [PATCH 13/15] xen/arm: p2m: Introduce a new variable removing_mapping in __p2m_set_entry

[Stefano Stabellini]

On Mon, 16 Jul 2018, Julien Grall wrote:
> This is making the code slightly easier to understand.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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


> ---
>  xen/arch/arm/p2m.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 66d58fabd7..e826f57842 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -792,6 +792,8 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>      unsigned int target = 3 - (page_order / LPAE_SHIFT);
>      lpae_t *entry, *table, orig_pte;
>      int rc;
> +    /* A mapping is removed if the MFN is invalid. */
> +    bool removing_mapping = mfn_eq(smfn, INVALID_MFN);
>  
>      /* Convenience aliases */
>      const unsigned int offsets[4] = {
> @@ -817,9 +819,9 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>      {
>          /*
>           * Don't try to allocate intermediate page table if the mapping
> -         * is about to be removed (i.e mfn == INVALID_MFN).
> +         * is about to be removed.
>           */
> -        rc = p2m_next_level(p2m, mfn_eq(smfn, INVALID_MFN),
> +        rc = p2m_next_level(p2m, removing_mapping,
>                              level, &table, offsets[level]);
>          if ( rc == GUEST_TABLE_MAP_FAILED )
>          {
> @@ -830,7 +832,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>               * 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;
> +            rc = removing_mapping ?  0 : -ENOENT;
>              goto out;
>          }
>          else if ( rc != GUEST_TABLE_NORMAL_PAGE )
> @@ -925,7 +927,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>      if ( lpae_is_valid(orig_pte) )
>          p2m_remove_pte(entry, p2m->clean_pte);
>  
> -    if ( mfn_eq(smfn, INVALID_MFN) )
> +    if ( removing_mapping )
>          /* Flush can be deferred if the entry is removed */
>          p2m->need_flush |= !!lpae_is_valid(orig_pte);
>      else
> -- 
> 2.11.0
> 

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

Re: [PATCH 14/15] xen/arm: guest_walk_tables: Switch the return to bool

[Stefano Stabellini]

On Mon, 16 Jul 2018, Julien Grall wrote:
> At the moment, guest_walk_tables can either return 0, -EFAULT, -EINVAL.
> The use of the last 2 are not clearly defined and used inconsistently in
> the code. The current only caller does not care about the return
> value and the value of it seems very limited (no way to differentiate
> between the 15ish error paths).
> 
> So switch to bool to simplify the return and make the developper life a
                                                           ^ developer


> bit easier.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Aside from the minor typo:

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


> ---
>  xen/arch/arm/guest_walk.c        | 50 ++++++++++++++++++++--------------------
>  xen/arch/arm/mem_access.c        |  2 +-
>  xen/include/asm-arm/guest_walk.h |  8 +++----
>  3 files changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index 4a1b4cf2c8..7db7a7321b 100644
> --- a/xen/arch/arm/guest_walk.c
> +++ b/xen/arch/arm/guest_walk.c
> @@ -28,9 +28,9 @@
>   * page table on a different vCPU, the following registers would need to be
>   * loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and SCTLR_EL1.
>   */
> -static int guest_walk_sd(const struct vcpu *v,
> -                         vaddr_t gva, paddr_t *ipa,
> -                         unsigned int *perms)
> +static bool guest_walk_sd(const struct vcpu *v,
> +                          vaddr_t gva, paddr_t *ipa,
> +                          unsigned int *perms)
>  {
>      int ret;
>      bool disabled = true;
> @@ -79,7 +79,7 @@ static int guest_walk_sd(const struct vcpu *v,
>      }
>  
>      if ( disabled )
> -        return -EFAULT;
> +        return false;
>  
>      /*
>       * The address of the L1 descriptor for the initial lookup has the
> @@ -97,12 +97,12 @@ static int guest_walk_sd(const struct vcpu *v,
>      /* Access the guest's memory to read only one PTE. */
>      ret = access_guest_memory_by_ipa(d, paddr, &pte, sizeof(short_desc_t), false);
>      if ( ret )
> -        return -EINVAL;
> +        return false;
>  
>      switch ( pte.walk.dt )
>      {
>      case L1DESC_INVALID:
> -        return -EFAULT;
> +        return false;
>  
>      case L1DESC_PAGE_TABLE:
>          /*
> @@ -122,10 +122,10 @@ static int guest_walk_sd(const struct vcpu *v,
>          /* Access the guest's memory to read only one PTE. */
>          ret = access_guest_memory_by_ipa(d, paddr, &pte, sizeof(short_desc_t), false);
>          if ( ret )
> -            return -EINVAL;
> +            return false;
>  
>          if ( pte.walk.dt == L2DESC_INVALID )
> -            return -EFAULT;
> +            return false;
>  
>          if ( pte.pg.page ) /* Small page. */
>          {
> @@ -175,7 +175,7 @@ static int guest_walk_sd(const struct vcpu *v,
>              *perms |= GV2M_EXEC;
>      }
>  
> -    return 0;
> +    return true;
>  }
>  
>  /*
> @@ -355,9 +355,9 @@ static bool check_base_size(unsigned int output_size, uint64_t base)
>   * page table on a different vCPU, the following registers would need to be
>   * loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and SCTLR_EL1.
>   */
> -static int guest_walk_ld(const struct vcpu *v,
> -                         vaddr_t gva, paddr_t *ipa,
> -                         unsigned int *perms)
> +static bool guest_walk_ld(const struct vcpu *v,
> +                          vaddr_t gva, paddr_t *ipa,
> +                          unsigned int *perms)
>  {
>      int ret;
>      bool disabled = true;
> @@ -442,7 +442,7 @@ static int guest_walk_ld(const struct vcpu *v,
>           */
>          if ( (input_size > TCR_EL1_IPS_48_BIT_VAL) ||
>               (input_size < TCR_EL1_IPS_MIN_VAL) )
> -            return -EFAULT;
> +            return false;
>      }
>      else
>      {
> @@ -487,7 +487,7 @@ static int guest_walk_ld(const struct vcpu *v,
>      }
>  
>      if ( disabled )
> -        return -EFAULT;
> +        return false;
>  
>      /*
>       * The starting level is the number of strides (grainsizes[gran] - 3)
> @@ -498,12 +498,12 @@ static int guest_walk_ld(const struct vcpu *v,
>      /* Get the IPA output_size. */
>      ret = get_ipa_output_size(d, tcr, &output_size);
>      if ( ret )
> -        return -EFAULT;
> +        return false;
>  
>      /* Make sure the base address does not exceed its configured size. */
>      ret = check_base_size(output_size, ttbr);
>      if ( !ret )
> -        return -EFAULT;
> +        return false;
>  
>      /*
>       * Compute the base address of the first level translation table that is
> @@ -523,12 +523,12 @@ static int guest_walk_ld(const struct vcpu *v,
>          /* Access the guest's memory to read only one PTE. */
>          ret = access_guest_memory_by_ipa(d, paddr, &pte, sizeof(lpae_t), false);
>          if ( ret )
> -            return -EFAULT;
> +            return false;
>  
>          /* Make sure the base address does not exceed its configured size. */
>          ret = check_base_size(output_size, pfn_to_paddr(pte.walk.base));
>          if ( !ret )
> -            return -EFAULT;
> +            return false;
>  
>          /*
>           * If page granularity is 64K, make sure the address is aligned
> @@ -537,7 +537,7 @@ static int guest_walk_ld(const struct vcpu *v,
>          if ( (output_size < TCR_EL1_IPS_52_BIT_VAL) &&
>               (gran == GRANULE_SIZE_INDEX_64K) &&
>               (pte.walk.base & 0xf) )
> -            return -EFAULT;
> +            return false;
>  
>          /*
>           * Break if one of the following conditions is true:
> @@ -567,7 +567,7 @@ static int guest_walk_ld(const struct vcpu *v,
>       * maps a memory block at level 3 (PTE<1:0> == 01).
>       */
>      if ( !lpae_is_valid(pte) || !lpae_is_mapping(pte, level) )
> -        return -EFAULT;
> +        return false;
>  
>      /* Make sure that the lower bits of the PTE's base address are zero. */
>      mask = GENMASK_ULL(47, grainsizes[gran]);
> @@ -583,11 +583,11 @@ static int guest_walk_ld(const struct vcpu *v,
>      if ( !pte.pt.xn && !xn_table )
>          *perms |= GV2M_EXEC;
>  
> -    return 0;
> +    return true;
>  }
>  
> -int guest_walk_tables(const struct vcpu *v, vaddr_t gva,
> -                      paddr_t *ipa, unsigned int *perms)
> +bool guest_walk_tables(const struct vcpu *v, vaddr_t gva,
> +                       paddr_t *ipa, unsigned int *perms)
>  {
>      uint32_t sctlr = READ_SYSREG(SCTLR_EL1);
>      register_t tcr = READ_SYSREG(TCR_EL1);
> @@ -595,7 +595,7 @@ int guest_walk_tables(const struct vcpu *v, vaddr_t gva,
>  
>      /* We assume that the domain is running on the currently active domain. */
>      if ( v != current )
> -        return -EFAULT;
> +        return false;
>  
>      /* Allow perms to be NULL. */
>      perms = perms ?: &_perms;
> @@ -619,7 +619,7 @@ int guest_walk_tables(const struct vcpu *v, vaddr_t gva,
>          /* Memory can be accessed without any restrictions. */
>          *perms = GV2M_READ|GV2M_WRITE|GV2M_EXEC;
>  
> -        return 0;
> +        return true;
>      }
>  
>      if ( is_32bit_domain(v->domain) && !(tcr & TTBCR_EAE) )
> diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
> index ae2686ffa2..57ec7872bb 100644
> --- a/xen/arch/arm/mem_access.c
> +++ b/xen/arch/arm/mem_access.c
> @@ -125,7 +125,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
>           * The software gva to ipa translation can still fail, e.g., if the gva
>           * is not mapped.
>           */
> -        if ( guest_walk_tables(v, gva, &ipa, &perms) < 0 )
> +        if ( !guest_walk_tables(v, gva, &ipa, &perms) )
>              return NULL;
>  
>          /*
> diff --git a/xen/include/asm-arm/guest_walk.h b/xen/include/asm-arm/guest_walk.h
> index 4ed8476e08..8768ac9894 100644
> --- a/xen/include/asm-arm/guest_walk.h
> +++ b/xen/include/asm-arm/guest_walk.h
> @@ -2,10 +2,10 @@
>  #define _XEN_GUEST_WALK_H
>  
>  /* Walk the guest's page tables in software. */
> -int guest_walk_tables(const struct vcpu *v,
> -                      vaddr_t gva,
> -                      paddr_t *ipa,
> -                      unsigned int *perms);
> +bool guest_walk_tables(const struct vcpu *v,
> +                       vaddr_t gva,
> +                       paddr_t *ipa,
> +                       unsigned int *perms);
>  
>  #endif /* _XEN_GUEST_WALK_H */
>  
> -- 
> 2.11.0
> 

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

Re: [PATCH 15/15] xen/arm: traps: Move the implementation of GUEST_BUG_ON in traps.h

[Stefano Stabellini]

On Mon, 16 Jul 2018, Julien Grall wrote:
> GUEST_BUG_ON may be used in other files doing guest emulation.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Given that GUEST_BUG_ON is not actually used in any other files in this
patch series, I assume you are referring to one of your future series?


> ---
>  xen/arch/arm/traps.c        | 24 ------------------------
>  xen/include/asm-arm/traps.h | 24 ++++++++++++++++++++++++
>  2 files changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index d1bf69b245..6751e4d754 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -68,30 +68,6 @@ static inline void check_stack_alignment_constraints(void) {
>  #endif
>  }
>  
> -/*
> - * GUEST_BUG_ON is intended for checking that the guest state has not been
> - * corrupted in hardware and/or that the hardware behaves as we
> - * believe it should (i.e. that certain traps can only occur when the
> - * guest is in a particular mode).
> - *
> - * The intention is to limit the damage such h/w bugs (or spec
> - * misunderstandings) can do by turning them into Denial of Service
> - * attacks instead of e.g. information leaks or privilege escalations.
> - *
> - * GUEST_BUG_ON *MUST* *NOT* be used to check for guest controllable state!
> - *
> - * Compared with regular BUG_ON it dumps the guest vcpu state instead
> - * of Xen's state.
> - */
> -#define guest_bug_on_failed(p)                          \
> -do {                                                    \
> -    show_execution_state(guest_cpu_user_regs());        \
> -    panic("Guest Bug: %pv: '%s', line %d, file %s\n",   \
> -          current, p, __LINE__, __FILE__);              \
> -} while (0)
> -#define GUEST_BUG_ON(p) \
> -    do { if ( unlikely(p) ) guest_bug_on_failed(#p); } while (0)
> -
>  #ifdef CONFIG_ARM_32
>  static int debug_stack_lines = 20;
>  #define stack_words_per_line 8
> diff --git a/xen/include/asm-arm/traps.h b/xen/include/asm-arm/traps.h
> index 70b52d1d16..0acf7de67d 100644
> --- a/xen/include/asm-arm/traps.h
> +++ b/xen/include/asm-arm/traps.h
> @@ -9,6 +9,30 @@
>  # include <asm/arm64/traps.h>
>  #endif
>  
> +/*
> + * GUEST_BUG_ON is intended for checking that the guest state has not been
> + * corrupted in hardware and/or that the hardware behaves as we
> + * believe it should (i.e. that certain traps can only occur when the
> + * guest is in a particular mode).
> + *
> + * The intention is to limit the damage such h/w bugs (or spec
> + * misunderstandings) can do by turning them into Denial of Service
> + * attacks instead of e.g. information leaks or privilege escalations.
> + *
> + * GUEST_BUG_ON *MUST* *NOT* be used to check for guest controllable state!
> + *
> + * Compared with regular BUG_ON it dumps the guest vcpu state instead
> + * of Xen's state.
> + */
> +#define guest_bug_on_failed(p)                          \
> +do {                                                    \
> +    show_execution_state(guest_cpu_user_regs());        \
> +    panic("Guest Bug: %pv: '%s', line %d, file %s\n",   \
> +          current, p, __LINE__, __FILE__);              \
> +} while (0)
> +#define GUEST_BUG_ON(p) \
> +    do { if ( unlikely(p) ) guest_bug_on_failed(#p); } while (0)
> +
>  int check_conditional_instr(struct cpu_user_regs *regs, const union hsr hsr);
>  
>  void advance_pc(struct cpu_user_regs *regs, const union hsr hsr);
> -- 
> 2.11.0
> 

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

Re: [PATCH 01/15] xen/arm: cpregs: Allow HSR_CPREG* to receive more than 1 parameter

[Stefano Stabellini]

On Mon, 16 Jul 2018, Julien Grall wrote:
> At the moment, HSR_CPREG is expected to receive only the co-processor
> register name in parameter. Because the name is actually a define, this
> may have been expanded by a previous macro.
> 
> Rather than imposing the use of _HSR_CPREG* in such cases, allow
> HSR_CPREG to receive more than 1 parameter.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

> ---
>  xen/include/asm-arm/cpregs.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
> index 8db65d5e2a..4c74e8161b 100644
> --- a/xen/include/asm-arm/cpregs.h
> +++ b/xen/include/asm-arm/cpregs.h
> @@ -47,8 +47,8 @@
>      ((__HSR_CPREG_##op1) << HSR_CP64_OP1_SHIFT)
>  
>  /* Encode a register as per HSR ISS pattern */
> -#define HSR_CPREG32(X) _HSR_CPREG32(X)
> -#define HSR_CPREG64(X) _HSR_CPREG64(X)
> +#define HSR_CPREG32(X...) _HSR_CPREG32(X)
> +#define HSR_CPREG64(X...) _HSR_CPREG64(X)
>  
>  /*
>   * Order registers by Coprocessor-> CRn-> Opcode 1-> CRm-> Opcode 2
> -- 
> 2.11.0
> 

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

Re: [PATCH 03/15] xen/arm: Introduce helpers to clear/flags flags in HCR_EL2

[Stefano Stabellini]

On Tue, 14 Aug 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 08/14/2018 09:46 PM, Stefano Stabellini wrote:
> > On Mon, 16 Jul 2018, Julien Grall wrote:
> > > A couple of places in the code will need to clear/set flags in HCR_EL2
> > > for a given vCPU and then replicate into the hardware. Introduce
> > > helpers and replace open-coded version.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > 
> > The macros look good, but I grepped for them in your series and there
> > are no more callers. What places are you referring to that they will
> > need them?
> 
> I split some of my work in 2 part to reduce the size of the series. This will
> be used in another series where updating HCR for trapping TTBR/SCTLR* will be
> dynamic.
> 
> Although in general, this is an improvement compare to the current code as it
> makes clear what needs to be updated when modifying HCR.

Yes, it is, but normally I would only introduce vcpu_hcr_set_flags in
this series, because vcpu_hcr_clear_flags is left completely unused?

I also don't mind introducing it anyway if you have a series coming.

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

Re: [PATCH 11/15] xen/arm: Allow lpae_is_{table, mapping} helpers to work on invalid entry

[Julien Grall]

Hi Stefano,

On 08/14/2018 10:33 PM, Stefano Stabellini wrote:
> On Mon, 16 Jul 2018, Julien Grall wrote:
>> Currently, lpae_is_{table, mapping} helpers will always return false on
>> entry with the valid bit unset. However, it would be useful to have them
>    ^entries
> 
>> operating on any entry. For instance to store information in advance but
>> still request a fault.
>>
>> With that change, the p2m is now providing an overlay for *_is_{table,
>> mapping} that will check the valid bit of the entry.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>   xen/arch/arm/guest_walk.c  |  2 +-
>>   xen/arch/arm/mm.c          |  2 +-
>>   xen/arch/arm/p2m.c         | 22 ++++++++++++++++++----
>>   xen/include/asm-arm/lpae.h | 11 +++++++----
>>   4 files changed, 27 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
>> index e3e21bdad3..4a1b4cf2c8 100644
>> --- a/xen/arch/arm/guest_walk.c
>> +++ b/xen/arch/arm/guest_walk.c
>> @@ -566,7 +566,7 @@ static int guest_walk_ld(const struct vcpu *v,
>>        * PTE is invalid or holds a reserved entry (PTE<1:0> == x0)) or if the PTE
>>        * maps a memory block at level 3 (PTE<1:0> == 01).
>>        */
>> -    if ( !lpae_is_mapping(pte, level) )
>> +    if ( !lpae_is_valid(pte) || !lpae_is_mapping(pte, level) )
>>           return -EFAULT;
>>   
>>       /* Make sure that the lower bits of the PTE's base address are zero. */
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index e3dafe5fd7..52e57fef2f 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -996,7 +996,7 @@ static int create_xen_entries(enum xenmap_operation op,
>>       for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
>>       {
>>           entry = &xen_second[second_linear_offset(addr)];
>> -        if ( !lpae_is_table(*entry, 2) )
>> +        if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
>>           {
>>               rc = create_xen_table(entry);
>>               if ( rc < 0 ) {
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index ec3fdcb554..07925a1be4 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -219,6 +219,20 @@ static p2m_access_t p2m_mem_access_radix_get(struct p2m_domain *p2m, gfn_t gfn)
>>           return radix_tree_ptr_to_int(ptr);
>>   }
>>   
>> +/*
>> + * lpae_is_* helpers don't check whether the valid bit is set in the
>> + * PTE. Provide our own overlay to check the valid bit.
>> + */
>> +static inline bool p2m_is_mapping(lpae_t pte, unsigned int level)
>> +{
>> +    return lpae_is_valid(pte) && lpae_is_mapping(pte, level);
>> +}
>> +
>> +static inline bool p2m_is_superpage(lpae_t pte, unsigned int level)
>> +{
>> +    return lpae_is_valid(pte) && lpae_is_superpage(pte, level);
>> +}
> 
> I like the idea, but it would be clearer to me if they were called
> lpae_is_valid_mapping and lpae_is_valid_superpage respectively. What do
> you think?
>  > Also, we might as well move them to lpae.h and use them from mm.c and
> guest_walk.c as appropriate?

lpae.h is not suitable because as I said in the commit message each 
page-table (stage-1, stage-2) may have a different view of what "valid" 
means.

At the moment, that view is the same but it is not going to stay long 
like that. Hence the reason of prefixing with p2m_. They are specific to 
the p2m. This is giving us some more liberty in the future while making 
the lpae code a bit more generic.

In guest_walk.c there are only one user, so the introduction of an 
helper is quite limited there.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 03/15] xen/arm: Introduce helpers to clear/flags flags in HCR_EL2

[Julien Grall]

Hi Stefano,

On 08/14/2018 10:49 PM, Stefano Stabellini wrote:
> On Tue, 14 Aug 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 08/14/2018 09:46 PM, Stefano Stabellini wrote:
>>> On Mon, 16 Jul 2018, Julien Grall wrote:
>>>> A couple of places in the code will need to clear/set flags in HCR_EL2
>>>> for a given vCPU and then replicate into the hardware. Introduce
>>>> helpers and replace open-coded version.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>
>>> The macros look good, but I grepped for them in your series and there
>>> are no more callers. What places are you referring to that they will
>>> need them?
>>
>> I split some of my work in 2 part to reduce the size of the series. This will
>> be used in another series where updating HCR for trapping TTBR/SCTLR* will be
>> dynamic.
>>
>> Although in general, this is an improvement compare to the current code as it
>> makes clear what needs to be updated when modifying HCR.
> 
> Yes, it is, but normally I would only introduce vcpu_hcr_set_flags in
> this series, because vcpu_hcr_clear_flags is left completely unused?

Well, it makes sense to keep the pair together. I am happy to move that 
patch in the other if you prefer.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 03/15] xen/arm: Introduce helpers to clear/flags flags in HCR_EL2

[Stefano Stabellini]

On Tue, 14 Aug 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 08/14/2018 10:49 PM, Stefano Stabellini wrote:
> > On Tue, 14 Aug 2018, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 08/14/2018 09:46 PM, Stefano Stabellini wrote:
> > > > On Mon, 16 Jul 2018, Julien Grall wrote:
> > > > > A couple of places in the code will need to clear/set flags in HCR_EL2
> > > > > for a given vCPU and then replicate into the hardware. Introduce
> > > > > helpers and replace open-coded version.
> > > > > 
> > > > > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > > > 
> > > > The macros look good, but I grepped for them in your series and there
> > > > are no more callers. What places are you referring to that they will
> > > > need them?
> > > 
> > > I split some of my work in 2 part to reduce the size of the series. This
> > > will
> > > be used in another series where updating HCR for trapping TTBR/SCTLR* will
> > > be
> > > dynamic.
> > > 
> > > Although in general, this is an improvement compare to the current code as
> > > it
> > > makes clear what needs to be updated when modifying HCR.
> > 
> > Yes, it is, but normally I would only introduce vcpu_hcr_set_flags in
> > this series, because vcpu_hcr_clear_flags is left completely unused?
> 
> Well, it makes sense to keep the pair together. I am happy to move that patch
> in the other if you prefer.

Sounds good. When you do that, you can directly add my reviewed-by.

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

Re: [PATCH 11/15] xen/arm: Allow lpae_is_{table, mapping} helpers to work on invalid entry

[Stefano Stabellini]

On Tue, 14 Aug 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 08/14/2018 10:33 PM, Stefano Stabellini wrote:
> > On Mon, 16 Jul 2018, Julien Grall wrote:
> > > Currently, lpae_is_{table, mapping} helpers will always return false on
> > > entry with the valid bit unset. However, it would be useful to have them
> >    ^entries
> > 
> > > operating on any entry. For instance to store information in advance but
> > > still request a fault.
> > > 
> > > With that change, the p2m is now providing an overlay for *_is_{table,
> > > mapping} that will check the valid bit of the entry.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > 
> > > ---
> > >   xen/arch/arm/guest_walk.c  |  2 +-
> > >   xen/arch/arm/mm.c          |  2 +-
> > >   xen/arch/arm/p2m.c         | 22 ++++++++++++++++++----
> > >   xen/include/asm-arm/lpae.h | 11 +++++++----
> > >   4 files changed, 27 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> > > index e3e21bdad3..4a1b4cf2c8 100644
> > > --- a/xen/arch/arm/guest_walk.c
> > > +++ b/xen/arch/arm/guest_walk.c
> > > @@ -566,7 +566,7 @@ static int guest_walk_ld(const struct vcpu *v,
> > >        * PTE is invalid or holds a reserved entry (PTE<1:0> == x0)) or if
> > > the PTE
> > >        * maps a memory block at level 3 (PTE<1:0> == 01).
> > >        */
> > > -    if ( !lpae_is_mapping(pte, level) )
> > > +    if ( !lpae_is_valid(pte) || !lpae_is_mapping(pte, level) )
> > >           return -EFAULT;
> > >         /* Make sure that the lower bits of the PTE's base address are
> > > zero. */
> > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > index e3dafe5fd7..52e57fef2f 100644
> > > --- a/xen/arch/arm/mm.c
> > > +++ b/xen/arch/arm/mm.c
> > > @@ -996,7 +996,7 @@ static int create_xen_entries(enum xenmap_operation
> > > op,
> > >       for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
> > >       {
> > >           entry = &xen_second[second_linear_offset(addr)];
> > > -        if ( !lpae_is_table(*entry, 2) )
> > > +        if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
> > >           {
> > >               rc = create_xen_table(entry);
> > >               if ( rc < 0 ) {
> > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > index ec3fdcb554..07925a1be4 100644
> > > --- a/xen/arch/arm/p2m.c
> > > +++ b/xen/arch/arm/p2m.c
> > > @@ -219,6 +219,20 @@ static p2m_access_t p2m_mem_access_radix_get(struct
> > > p2m_domain *p2m, gfn_t gfn)
> > >           return radix_tree_ptr_to_int(ptr);
> > >   }
> > >   +/*
> > > + * lpae_is_* helpers don't check whether the valid bit is set in the
> > > + * PTE. Provide our own overlay to check the valid bit.
> > > + */
> > > +static inline bool p2m_is_mapping(lpae_t pte, unsigned int level)
> > > +{
> > > +    return lpae_is_valid(pte) && lpae_is_mapping(pte, level);
> > > +}
> > > +
> > > +static inline bool p2m_is_superpage(lpae_t pte, unsigned int level)
> > > +{
> > > +    return lpae_is_valid(pte) && lpae_is_superpage(pte, level);
> > > +}
> > 
> > I like the idea, but it would be clearer to me if they were called
> > lpae_is_valid_mapping and lpae_is_valid_superpage respectively. What do
> > you think?
> >  > Also, we might as well move them to lpae.h and use them from mm.c and
> > guest_walk.c as appropriate?
> 
> lpae.h is not suitable because as I said in the commit message each page-table
> (stage-1, stage-2) may have a different view of what "valid" means.
> 
> At the moment, that view is the same but it is not going to stay long like
> that. Hence the reason of prefixing with p2m_. They are specific to the p2m.
> This is giving us some more liberty in the future while making the lpae code a
> bit more generic.
> 
> In guest_walk.c there are only one user, so the introduction of an helper is
> quite limited there.

I see, so by "p2m_is_mapping" you meant specifically
"stage2_is_mapping", right? That makes sense now.

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

Re: [PATCH 11/15] xen/arm: Allow lpae_is_{table, mapping} helpers to work on invalid entry

[Julien Grall]

Hi,

On 08/15/2018 08:13 PM, Stefano Stabellini wrote:
> On Tue, 14 Aug 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 08/14/2018 10:33 PM, Stefano Stabellini wrote:
>>> On Mon, 16 Jul 2018, Julien Grall wrote:
>>>> Currently, lpae_is_{table, mapping} helpers will always return false on
>>>> entry with the valid bit unset. However, it would be useful to have them
>>>     ^entries
>>>
>>>> operating on any entry. For instance to store information in advance but
>>>> still request a fault.
>>>>
>>>> With that change, the p2m is now providing an overlay for *_is_{table,
>>>> mapping} that will check the valid bit of the entry.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>
>>>> ---
>>>>    xen/arch/arm/guest_walk.c  |  2 +-
>>>>    xen/arch/arm/mm.c          |  2 +-
>>>>    xen/arch/arm/p2m.c         | 22 ++++++++++++++++++----
>>>>    xen/include/asm-arm/lpae.h | 11 +++++++----
>>>>    4 files changed, 27 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
>>>> index e3e21bdad3..4a1b4cf2c8 100644
>>>> --- a/xen/arch/arm/guest_walk.c
>>>> +++ b/xen/arch/arm/guest_walk.c
>>>> @@ -566,7 +566,7 @@ static int guest_walk_ld(const struct vcpu *v,
>>>>         * PTE is invalid or holds a reserved entry (PTE<1:0> == x0)) or if
>>>> the PTE
>>>>         * maps a memory block at level 3 (PTE<1:0> == 01).
>>>>         */
>>>> -    if ( !lpae_is_mapping(pte, level) )
>>>> +    if ( !lpae_is_valid(pte) || !lpae_is_mapping(pte, level) )
>>>>            return -EFAULT;
>>>>          /* Make sure that the lower bits of the PTE's base address are
>>>> zero. */
>>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>>>> index e3dafe5fd7..52e57fef2f 100644
>>>> --- a/xen/arch/arm/mm.c
>>>> +++ b/xen/arch/arm/mm.c
>>>> @@ -996,7 +996,7 @@ static int create_xen_entries(enum xenmap_operation
>>>> op,
>>>>        for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
>>>>        {
>>>>            entry = &xen_second[second_linear_offset(addr)];
>>>> -        if ( !lpae_is_table(*entry, 2) )
>>>> +        if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
>>>>            {
>>>>                rc = create_xen_table(entry);
>>>>                if ( rc < 0 ) {
>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>> index ec3fdcb554..07925a1be4 100644
>>>> --- a/xen/arch/arm/p2m.c
>>>> +++ b/xen/arch/arm/p2m.c
>>>> @@ -219,6 +219,20 @@ static p2m_access_t p2m_mem_access_radix_get(struct
>>>> p2m_domain *p2m, gfn_t gfn)
>>>>            return radix_tree_ptr_to_int(ptr);
>>>>    }
>>>>    +/*
>>>> + * lpae_is_* helpers don't check whether the valid bit is set in the
>>>> + * PTE. Provide our own overlay to check the valid bit.
>>>> + */
>>>> +static inline bool p2m_is_mapping(lpae_t pte, unsigned int level)
>>>> +{
>>>> +    return lpae_is_valid(pte) && lpae_is_mapping(pte, level);
>>>> +}
>>>> +
>>>> +static inline bool p2m_is_superpage(lpae_t pte, unsigned int level)
>>>> +{
>>>> +    return lpae_is_valid(pte) && lpae_is_superpage(pte, level);
>>>> +}
>>>
>>> I like the idea, but it would be clearer to me if they were called
>>> lpae_is_valid_mapping and lpae_is_valid_superpage respectively. What do
>>> you think?
>>>   > Also, we might as well move them to lpae.h and use them from mm.c and
>>> guest_walk.c as appropriate?
>>
>> lpae.h is not suitable because as I said in the commit message each page-table
>> (stage-1, stage-2) may have a different view of what "valid" means.
>>
>> At the moment, that view is the same but it is not going to stay long like
>> that. Hence the reason of prefixing with p2m_. They are specific to the p2m.
>> This is giving us some more liberty in the future while making the lpae code a
>> bit more generic.
>>
>> In guest_walk.c there are only one user, so the introduction of an helper is
>> quite limited there.
> 
> I see, so by "p2m_is_mapping" you meant specifically
> "stage2_is_mapping", right? That makes sense now.

Yes. We use the term "p2m" everywhere in Xen to refer to stage-2 
page-tables (see lpae_p2m_t). So it makes sense to prefix the helpers 
with "p2m_" here.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 15/15] xen/arm: traps: Move the implementation of GUEST_BUG_ON in traps.h

[Julien Grall]

Hi Stefano,

On 08/14/2018 10:43 PM, Stefano Stabellini wrote:
> On Mon, 16 Jul 2018, Julien Grall wrote:
>> GUEST_BUG_ON may be used in other files doing guest emulation.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Given that GUEST_BUG_ON is not actually used in any other files in this
> patch series, I assume you are referring to one of your future series?

It is going to be used later. However, I don't really refer to any 
series in particular. It is just that this macros could be helpful in 
any emulation code to catch what we think is a invalid architectural 
behavior.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 15/15] xen/arm: traps: Move the implementation of GUEST_BUG_ON in traps.h

[Stefano Stabellini]

On Thu, 16 Aug 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 08/14/2018 10:43 PM, Stefano Stabellini wrote:
> > On Mon, 16 Jul 2018, Julien Grall wrote:
> > > GUEST_BUG_ON may be used in other files doing guest emulation.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > 
> > Given that GUEST_BUG_ON is not actually used in any other files in this
> > patch series, I assume you are referring to one of your future series?
> 
> It is going to be used later. However, I don't really refer to any series in
> particular. It is just that this macros could be helpful in any emulation code
> to catch what we think is a invalid architectural behavior.

It is only that a concrete use-case helps. The idea of moving
GUEST_BUG_ON is good, but where to? For instance, wouldn't it be better
to move it to guest_access.h? I am just trying to point to an existing
header which is more widely used across the emulators (vpl011,
vgic-v3-its.c, hvm.c, ...)

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

Re: [PATCH 15/15] xen/arm: traps: Move the implementation of GUEST_BUG_ON in traps.h

[Julien Grall]

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

Hi Stefano,

On Thu, 16 Aug 2018, 19:17 Stefano Stabellini, <sstabellini@kernel.org>
wrote:

> On Thu, 16 Aug 2018, Julien Grall wrote:
> > Hi Stefano,
> >
> > On 08/14/2018 10:43 PM, Stefano Stabellini wrote:
> > > On Mon, 16 Jul 2018, Julien Grall wrote:
> > > > GUEST_BUG_ON may be used in other files doing guest emulation.
> > > >
> > > > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > >
> > > Given that GUEST_BUG_ON is not actually used in any other files in this
> > > patch series, I assume you are referring to one of your future series?
> >
> > It is going to be used later. However, I don't really refer to any
> series in
> > particular. It is just that this macros could be helpful in any
> emulation code
> > to catch what we think is a invalid architectural behavior.
>
> It is only that a concrete use-case helps. The idea of moving
> GUEST_BUG_ON is good, but where to? For instance, wouldn't it be better
> to move it to guest_access.h? I am just trying to point to an existing
> header which is more widely used across the emulators (vpl011,
> vgic-v3-its.c, hvm.c, ...)
>

Definitely not in guest_access.h or any wider in includes. If you look at
the implementation and documentation, it will crash the hypervisor because
the goal is to catch hardware bug or misunderstanding of the spec. This is
not meant to be used in emulation. We should not crash the guest/hypervisor
in bad behavior but inject an exception.

So trap.h is the best places for it as hardware trap are now split accros
multiples files.

Cheers,

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

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

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

Re: [PATCH 15/15] xen/arm: traps: Move the implementation of GUEST_BUG_ON in traps.h

[Stefano Stabellini]

On Mon, 20 Aug 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On Thu, 16 Aug 2018, 19:17 Stefano Stabellini, <sstabellini@kernel.org> wrote:
>       On Thu, 16 Aug 2018, Julien Grall wrote:
>       > Hi Stefano,
>       >
>       > On 08/14/2018 10:43 PM, Stefano Stabellini wrote:
>       > > On Mon, 16 Jul 2018, Julien Grall wrote:
>       > > > GUEST_BUG_ON may be used in other files doing guest emulation.
>       > > >
>       > > > Signed-off-by: Julien Grall <julien.grall@linaro.org>
>       > >
>       > > Given that GUEST_BUG_ON is not actually used in any other files in this
>       > > patch series, I assume you are referring to one of your future series?
>       >
>       > It is going to be used later. However, I don't really refer to any series in
>       > particular. It is just that this macros could be helpful in any emulation code
>       > to catch what we think is a invalid architectural behavior.
> 
>       It is only that a concrete use-case helps. The idea of moving
>       GUEST_BUG_ON is good, but where to? For instance, wouldn't it be better
>       to move it to guest_access.h? I am just trying to point to an existing
>       header which is more widely used across the emulators (vpl011,
>       vgic-v3-its.c, hvm.c, ...)
> 
> 
> Definitely not in guest_access.h or any wider in includes. If you look at the implementation and documentation, it will crash the
> hypervisor because the goal is to catch hardware bug or misunderstanding of the spec. This is not meant to be used in emulation.
> We should not crash the guest/hypervisor in bad behavior but inject an exception.
> 
> So trap.h is the best places for it as hardware trap are now split accros multiples files.

OK

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

Re: [PATCH 00/15] xen/arm: Bunch of clean-up/improvement

[Julien Grall]

Hi,

On 16/07/18 18:26, Julien Grall wrote:
> Julien Grall (15):
>    xen/arm: cpregs: Allow HSR_CPREG* to receive more than 1 parameter
>    xen/arm: cpregs: Fix typo in the documentation of TTBCR
>    xen/arm: p2m: Reduce the locking section in get_page_from_gva
>    xen/arm: p2m: Limit call to mem access code use in get_page_from_gva
>    xen/arm: Rework lpae_mapping
>    xen/arm: Rework lpae_table
>    xen/arm: Rename lpae_valid to lpae_is_valid
>    xen/arm: guest_walk: Use lpae_is_mapping to simplify the code
>    xen/arm: p2m: Rename ret to mfn in p2m_lookup
>    xen/arm: p2m: Introduce a new variable removing_mapping in
>      __p2m_set_entry
I have committed those 10 patches. The rest requires some rework and 
therefore a respin.

Cheers,

-- 
Julien Grall

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