[RFC PATCH v3 00/10] arm/mem_access: Walk guest page tables in SW if mem_access is active

[RFC PATCH v3 00/10] arm/mem_access: Walk guest page tables in SW if mem_access is active

[Sergej Proskurin]

Hi all,

The function p2m_mem_access_check_and_get_page is called from the function
get_page_from_gva if mem_access is active and the hardware-aided translation of
the given guest virtual address (gva) into machine address fails. That is, if
the stage-2 translation tables constrain access to the guests's page tables,
hardware-assisted translation will fail. The idea of the function
p2m_mem_access_check_and_get_page is thus to translate the given gva and check
the requested access rights in software. However, as the current implementation
of p2m_mem_access_check_and_get_page makes use of the hardware-aided gva to ipa
translation, the translation might also fail because of reasons stated above
and will become equally relevant for the altp2m implementation on ARM.  As
such, we provide a software guest translation table walk to address the above
mentioned issue. We submit this patch series as an RFC to discuss the
appropriate location for the code and further functionality required to fix the
above concerns.

The current version of the implementation supports translation of both the
short-descriptor as well as the long-descriptor translation table format on
ARMv7 and ARMv8 (AArch32/AArch64).

This revised version incorporates the comments of the previous patch series and
mainly adds additional data-structures and defines to simplify code
readability. Also, this patch series exposes p2m_* macros to access certain PTE
properties in a more readable way. Apart from that, please note that since the
individual functions responsible for walking the guest page tables must support
various details of both AArch32 and AArch64, a simplification of seemingly
identical parts in the affected functions has not always been possible.

The following patch series can be found on Github[0].

Cheers,
~Sergej

[0] https://github.com/sergej-proskurin/xen (branch arm-gpt-walk-v3)

Sergej Proskurin (10):
  arm/mem_access: Add (TCR_|TTBCR_)* defines
  arm/mem_access: Add defines holding the width of 32/64bit regs
  arm/mem_access: Add defines supporting PTs with varying page sizes
  arm/mem_access: Add short-descriptor pte typedefs
  arm/p2m: Make PTE helpers publicly available
  arm/mem_access: Introduce GV2M_EXEC permission
  arm/mem_access: Add software guest-page-table walk
  arm/mem_access: Add long-descriptor based gpt
  arm/mem_access: Add short-descriptor based gpt
  arm/mem_access: Walk the guest's pt in software

 xen/arch/arm/Makefile            |   1 +
 xen/arch/arm/guest_walk.c        | 646 +++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/mem_access.c        |  21 +-
 xen/arch/arm/p2m.c               |  23 --
 xen/include/asm-arm/guest_walk.h |  44 +++
 xen/include/asm-arm/p2m.h        |  27 ++
 xen/include/asm-arm/page.h       | 150 +++++++++
 xen/include/asm-arm/processor.h  |  73 ++++-
 8 files changed, 957 insertions(+), 28 deletions(-)
 create mode 100644 xen/arch/arm/guest_walk.c
 create mode 100644 xen/include/asm-arm/guest_walk.h

-- 
2.12.2


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

[RFC PATCH v3 01/10] arm/mem_access: Add (TCR_|TTBCR_)* defines

[Sergej Proskurin]

This commit adds (TCR_|TTBCR_)* defines to simplify access to the
respective register contents. At the same time, we adjust the macro
TCR_T0SZ by using the newly introduced TCR_T0SZ_SHIFT instead of the
hardcoded value.

Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
---
v2: Define TCR_SZ_MASK in a way so that it can be also applied to 32-bit guests
    using the long-descriptor translation table format.

    Extend the previous commit by further defines allowing a simplified access
    to the registers TCR_EL1 and TTBCR.

v3: Replace the hardcoded value 0 in the TCR_T0SZ macro with the newly
    introduced TCR_T0SZ_SHIFT. Also, replace the hardcoded value 14 in
    the TCR_TG0_* macros with the introduced TCR_TG0_SHIFT.

    Comment when to apply the defines TTBCR_PD(0|1), according to ARM
    DDI 0487B-a and ARM DDI 0406C-b.

    Remove TCR_TB_* defines.

    Comment when certain TCR_EL2 register fields can be applied.
---
 xen/include/asm-arm/processor.h | 69 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 65 insertions(+), 4 deletions(-)

diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 855ded1b07..13f172a20f 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -94,6 +94,13 @@
 #define TTBCR_N_2KB  _AC(0x03,U)
 #define TTBCR_N_1KB  _AC(0x04,U)
 
+/*
+ * TTBCR_PD(0|1) can be applied only if LPAE is disabled, i.e., TTBCR.EAE==0
+ * (ARM DDI 0487B-a G6-5203 and ARM DDI 0406C-b B4-1722).
+ */
+#define TTBCR_PD0       (_AC(1,U)<<4)
+#define TTBCR_PD1       (_AC(1,U)<<5)
+
 /* SCTLR System Control Register. */
 /* HSCTLR is a subset of this. */
 #define SCTLR_TE        (_AC(1,U)<<30)
@@ -154,7 +161,20 @@
 
 /* TCR: Stage 1 Translation Control */
 
-#define TCR_T0SZ(x)     ((x)<<0)
+#define TCR_T0SZ_SHIFT  (0)
+#define TCR_T1SZ_SHIFT  (16)
+#define TCR_T0SZ(x)     ((x)<<TCR_T0SZ_SHIFT)
+
+/*
+ * According to ARM DDI 0487B-a, TCR_EL1.{T0SZ,T1SZ} (AArch64, Section D7-2480)
+ * comprises 6 bits and TTBCR.{T0SZ,T1SZ} (AArch32, Section G6-5204) comprises
+ * 3 bits following another 3 bits for RES0. Thus, the mask for both registers
+ * should be 0x3f.
+ */
+#define TCR_SZ_MASK     (_AC(0x3f,UL)<<0)
+
+#define TCR_EPD0        (_AC(0x1,UL)<<7)
+#define TCR_EPD1        (_AC(0x1,UL)<<23)
 
 #define TCR_IRGN0_NC    (_AC(0x0,UL)<<8)
 #define TCR_IRGN0_WBWA  (_AC(0x1,UL)<<8)
@@ -170,15 +190,56 @@
 #define TCR_SH0_OS      (_AC(0x2,UL)<<12)
 #define TCR_SH0_IS      (_AC(0x3,UL)<<12)
 
-#define TCR_TG0_4K      (_AC(0x0,UL)<<14)
-#define TCR_TG0_64K     (_AC(0x1,UL)<<14)
-#define TCR_TG0_16K     (_AC(0x2,UL)<<14)
+/* Note that the fields TCR_EL1.{TG0,TG1} are not available on AArch32. */
+#define TCR_TG0_SHIFT   (14)
+#define TCR_TG0_MASK    (_AC(0x3,UL)<<TCR_TG0_SHIFT)
+#define TCR_TG0_4K      (_AC(0x0,UL)<<TCR_TG0_SHIFT)
+#define TCR_TG0_64K     (_AC(0x1,UL)<<TCR_TG0_SHIFT)
+#define TCR_TG0_16K     (_AC(0x2,UL)<<TCR_TG0_SHIFT)
+
+/* Note that the field TCR_EL2.TG1 exists only if HCR_EL2.E2H==1. */
+#define TCR_EL1_TG1_SHIFT   (30)
+#define TCR_EL1_TG1_MASK    (_AC(0x3,UL)<<TCR_EL1_TG1_SHIFT)
+#define TCR_EL1_TG1_16K     (_AC(0x1,UL)<<TCR_EL1_TG1_SHIFT)
+#define TCR_EL1_TG1_4K      (_AC(0x2,UL)<<TCR_EL1_TG1_SHIFT)
+#define TCR_EL1_TG1_64K     (_AC(0x3,UL)<<TCR_EL1_TG1_SHIFT)
+
+/*
+ * Note that the field TCR_EL1.IPS is not available on AArch32. Also, the field
+ * TCR_EL2.IPS exists only if HCR_EL2.E2H==1.
+ */
+#define TCR_EL1_IPS_SHIFT   (32)
+#define TCR_EL1_IPS_MASK    (_AC(0x7,ULL)<<TCR_EL1_IPS_SHIFT)
+#define TCR_EL1_IPS_32_BIT  (_AC(0x0,ULL)<<TCR_EL1_IPS_SHIFT)
+#define TCR_EL1_IPS_36_BIT  (_AC(0x1,ULL)<<TCR_EL1_IPS_SHIFT)
+#define TCR_EL1_IPS_40_BIT  (_AC(0x2,ULL)<<TCR_EL1_IPS_SHIFT)
+#define TCR_EL1_IPS_42_BIT  (_AC(0x3,ULL)<<TCR_EL1_IPS_SHIFT)
+#define TCR_EL1_IPS_44_BIT  (_AC(0x4,ULL)<<TCR_EL1_IPS_SHIFT)
+#define TCR_EL1_IPS_48_BIT  (_AC(0x5,ULL)<<TCR_EL1_IPS_SHIFT)
+#define TCR_EL1_IPS_52_BIT  (_AC(0x6,ULL)<<TCR_EL1_IPS_SHIFT)
+
+/*
+ * The following values correspond to the bit masks represented by
+ * TCR_EL1_IPS_XX_BIT defines.
+ */
+#define TCR_EL1_IPS_32_BIT_VAL  (32)
+#define TCR_EL1_IPS_36_BIT_VAL  (36)
+#define TCR_EL1_IPS_40_BIT_VAL  (40)
+#define TCR_EL1_IPS_42_BIT_VAL  (42)
+#define TCR_EL1_IPS_44_BIT_VAL  (44)
+#define TCR_EL1_IPS_48_BIT_VAL  (48)
+#define TCR_EL1_IPS_52_BIT_VAL  (52)
+#define TCR_EL1_IPS_MIN_VAL     (25)
 
 #ifdef CONFIG_ARM_64
 
 #define TCR_PS(x)       ((x)<<16)
 #define TCR_TBI         (_AC(0x1,UL)<<20)
 
+/* Note that the fields TCR_EL2.TBI(0|1) exist only if HCR_EL2.E2H==1. */
+#define TCR_EL1_TBI0    (_AC(0x1,UL)<<37)
+#define TCR_EL1_TBI1    (_AC(0x1,UL)<<38)
+
 #define TCR_RES1        (_AC(1,UL)<<31|_AC(1,UL)<<23)
 
 #else
-- 
2.12.2


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

[RFC PATCH v3 02/10] arm/mem_access: Add defines holding the width of 32/64bit regs

[Sergej Proskurin]

This commit adds two defines holding the register width of 32 bit and 64 bit
registers. These defines simplify using the associated constants in the
following commits.

Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
---
 xen/include/asm-arm/processor.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 13f172a20f..653dc139e4 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -338,6 +338,10 @@
 #define MM64_VMID_16_BITS_SUPPORT   0x2
 #endif
 
+/* Register width */
+#define REGISTER_WIDTH_64_BIT       (64)
+#define REGISTER_WIDTH_32_BIT       (32)
+
 #ifndef __ASSEMBLY__
 
 struct cpuinfo_arm {
-- 
2.12.2


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

[RFC PATCH v3 03/10] arm/mem_access: Add defines supporting PTs with varying page sizes

[Sergej Proskurin]

The ARMv8 architecture supports pages with different (4K, 16K, and 64K) sizes.
To enable guest page table walks for various configurations, this commit
extends the defines and helpers of the current implementation.

Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
---
v3: Eliminate redundant macro definitions by introducing generic macros.
---
 xen/include/asm-arm/page.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 497b4c86ad..e2e4b597a5 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -500,6 +500,51 @@ static inline int gva_to_ipa(vaddr_t va, paddr_t *paddr, unsigned int flags)
 
 #define PAGE_ALIGN(x) (((x) + PAGE_SIZE - 1) & PAGE_MASK)
 
+#define LPAE_SHIFT_4K           (9)
+#define LPAE_SHIFT_16K          (11)
+#define LPAE_SHIFT_64K          (13)
+
+#define lpae_entries(gran)      (_AC(1,U) << LPAE_SHIFT_##gran)
+#define lpae_entry_mask(gran)   (lpae_entries(gran) - 1)
+
+#define PAGE_SHIFT_4K           (12)
+#define PAGE_SHIFT_16K          (14)
+#define PAGE_SHIFT_64K          (16)
+
+#define third_shift(gran)       (PAGE_SHIFT_##gran)
+#define third_size(gran)        ((paddr_t)1 << third_shift(gran))
+
+#define second_shift(gran)      (third_shift(gran) + LPAE_SHIFT_##gran)
+#define second_size(gran)       ((paddr_t)1 << second_shift(gran))
+
+#define first_shift(gran)       (second_shift(gran) + LPAE_SHIFT_##gran)
+#define first_size(gran)        ((paddr_t)1 << first_shift(gran))
+
+/* Note that there is no zeroeth lookup level with a 64K granule size. */
+#define zeroeth_shift(gran)     (first_shift(gran) + LPAE_SHIFT_##gran)
+#define zeroeth_size(gran)      ((paddr_t)1 << zeroeth_shift(gran))
+
+#define GUEST_TABLE_OFFSET(offs, gran)          ((paddr_t)(offs) & lpae_entry_mask(gran))
+#define third_guest_table_offset(va, gran)      GUEST_TABLE_OFFSET((va >> third_shift(gran)), gran)
+#define second_guest_table_offset(va, gran)     GUEST_TABLE_OFFSET((va >> second_shift(gran)), gran)
+#define first_guest_table_offset(va, gran)      GUEST_TABLE_OFFSET((va >> first_shift(gran)), gran)
+#define zeroeth_guest_table_offset(va, gran)    GUEST_TABLE_OFFSET((va >> zeroeth_shift(gran)), gran)
+
+#define third_guest_table_offset_4k(va)         third_guest_table_offset(va, 4K)
+#define third_guest_table_offset_16k(va)        third_guest_table_offset(va, 16K)
+#define third_guest_table_offset_64k(va)        third_guest_table_offset(va, 64K)
+
+#define second_guest_table_offset_4k(va)        second_guest_table_offset(va, 4K)
+#define second_guest_table_offset_16k(va)       second_guest_table_offset(va, 16K)
+#define second_guest_table_offset_64k(va)       second_guest_table_offset(va, 64K)
+
+#define first_guest_table_offset_4k(va)         first_guest_table_offset(va, 4K)
+#define first_guest_table_offset_16k(va)        first_guest_table_offset(va, 16K)
+#define first_guest_table_offset_64k(va)        first_guest_table_offset(va, 64K)
+
+#define zeroeth_guest_table_offset_4k(va)       zeroeth_guest_table_offset(va, 4K)
+#define zeroeth_guest_table_offset_16k(va)      zeroeth_guest_table_offset(va, 16K)
+
 #endif /* __ARM_PAGE_H__ */
 
 /*
-- 
2.12.2


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

[RFC PATCH v3 04/10] arm/mem_access: Add short-descriptor pte typedefs

[Sergej Proskurin]

The current implementation does not provide appropriate types for
short-descriptor translation table entries. As such, this commit adds new
types, which simplify managing the respective translation table entries.

Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
---
v3: Add more short-descriptor related pte typedefs that will be used by
    the following commits.
---
 xen/include/asm-arm/page.h | 104 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 104 insertions(+)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index e2e4b597a5..7a4aa64144 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -205,6 +205,110 @@ typedef union {
     lpae_walk_t walk;
 } lpae_t;
 
+/*
+ *  Comprises bits of the level 1 short-descriptor format representing
+ *  a section.
+ */
+typedef struct __packed {
+    unsigned int pxn:1;         /* Privileged Execute Never */
+    unsigned int sec:1;         /* == 1 if section or supersection */
+    unsigned int b:1;           /* Bufferable */
+    unsigned int c:1;           /* Cacheable */
+    unsigned int xn:1;          /* Execute Never */
+    unsigned int dom:4;         /* Domain field */
+    unsigned int impl:1;        /* Implementation defined */
+    unsigned int ap:2;          /* AP[1:0] */
+    unsigned int tex:3;         /* TEX[2:0] */
+    unsigned int ro:1;          /* AP[2] */
+    unsigned int s:1;           /* Shareable */
+    unsigned int ng:1;          /* Non-global */
+    unsigned int supersec:1;    /* Must be 0 for sections */
+    unsigned int ns:1;          /* Non-secure */
+    unsigned int base:12;       /* Section base address */
+} pte_sd_l1desc_sec_t;
+
+/*
+ *  Comprises bits of the level 1 short-descriptor format representing
+ *  a supersection.
+ */
+typedef struct __packed {
+    unsigned int pxn:1;         /* Privileged Execute Never */
+    unsigned int sec:1;         /* == 1 if section or supersection */
+    unsigned int b:1;           /* Bufferable */
+    unsigned int c:1;           /* Cacheable */
+    unsigned int xn:1;          /* Execute Never */
+    unsigned int extbase2:4;    /* Extended base address, PA[39:36] */
+    unsigned int impl:1;        /* Implementation defined */
+    unsigned int ap:2;          /* AP[1:0] */
+    unsigned int tex:3;         /* TEX[2:0] */
+    unsigned int ro:1;          /* AP[2] */
+    unsigned int s:1;           /* Shareable */
+    unsigned int ng:1;          /* Non-global */
+    unsigned int supersec:1;    /* Must be 0 for sections */
+    unsigned int ns:1;          /* Non-secure */
+    unsigned int extbase1:4;    /* Extended base address, PA[35:32] */
+    unsigned int base:8;        /* Supersection base address */
+} pte_sd_l1desc_supersec_t;
+
+/*
+ *  Comprises bits of the level 2 short-descriptor format representing
+ *  a small page.
+ */
+typedef struct __packed {
+    unsigned int xn:1;          /* Execute Never */
+    unsigned int page:1;        /* ==1 if small page */
+    unsigned int b:1;           /* Bufferable */
+    unsigned int c:1;           /* Cacheable */
+    unsigned int ap:2;          /* AP[1:0] */
+    unsigned int tex:3;         /* TEX[2:0] */
+    unsigned int ro:1;          /* AP[2] */
+    unsigned int s:1;           /* Shareable */
+    unsigned int ng:1;          /* Non-global */
+    unsigned int base:20;       /* Small page base address */
+} pte_sd_l2desc_page_t;
+
+/*
+ *  Comprises bits of the level 2 short-descriptor format representing
+ *  a large page.
+ */
+typedef struct __packed {
+    unsigned int lpage:1;       /* ==1 if large page */
+    unsigned int page:1;        /* ==0 if large page */
+    unsigned int b:1;           /* Bufferable */
+    unsigned int c:1;           /* Cacheable */
+    unsigned int ap:2;          /* AP[1:0] */
+    unsigned int sbz:3;         /* Should be zero */
+    unsigned int ro:1;          /* AP[2] */
+    unsigned int s:1;           /* Shareable */
+    unsigned int ng:1;          /* Non-global */
+    unsigned int tex:3;         /* TEX[2:0] */
+    unsigned int xn:1;          /* Execute Never */
+    unsigned int base:16;       /* Large page base address */
+} pte_sd_l2desc_lpage_t;
+
+/*
+ * Comprises the bits required to walk page tables adhering to the
+ * short-descriptor translation table format.
+ */
+typedef struct __packed {
+    unsigned int dt:2;          /* Descriptor type */
+    unsigned int pad1:8;
+    unsigned int base:22;       /* Base address of block or next table */
+} pte_sd_walk_t;
+
+/*
+ * Represents page table entries adhering to the short-descriptor translation
+ * table format.
+ */
+typedef union {
+    uint32_t bits;
+    pte_sd_walk_t walk;
+    pte_sd_l1desc_sec_t sec;
+    pte_sd_l1desc_supersec_t supersec;
+    pte_sd_l2desc_page_t pg;
+    pte_sd_l2desc_lpage_t lpg;
+} pte_sd_t;
+
 /* Standard entry type that we'll use to build Xen's own pagetables.
  * We put the same permissions at every level, because they're ignored
  * by the walker in non-leaf entries. */
-- 
2.12.2


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

[RFC PATCH v3 05/10] arm/p2m: Make PTE helpers publicly available

[Sergej Proskurin]

In this commit we make the p2m_* helpers, which access PTE properties in
a simplified way, publicly available. This is due to the fact that the
helpers will be used in guest_walk.c in one of the following commits.

Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
---
Cc:
---
 xen/arch/arm/p2m.c        | 23 -----------------------
 xen/include/asm-arm/p2m.h | 27 +++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 23 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index b7bbea1d81..eecbcdf870 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -46,29 +46,6 @@ static const paddr_t level_masks[] =
 static const uint8_t level_orders[] =
     { ZEROETH_ORDER, FIRST_ORDER, SECOND_ORDER, THIRD_ORDER };
 
-static inline bool_t p2m_valid(lpae_t pte)
-{
-    return pte.p2m.valid;
-}
-/*
- * These two 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_t p2m_table(lpae_t pte)
-{
-    return p2m_valid(pte) && pte.p2m.table;
-}
-static inline bool_t p2m_mapping(lpae_t pte)
-{
-    return p2m_valid(pte) && !pte.p2m.table;
-}
-
-static inline bool p2m_is_superpage(lpae_t pte, unsigned int level)
-{
-    return (level < 3) && p2m_mapping(pte);
-}
-
 static void p2m_flush_tlb(struct p2m_domain *p2m);
 
 /* Unlock the flush and do a P2M TLB flush if necessary */
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 18c57f936e..8053f2a0cf 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -198,6 +198,33 @@ static inline int p2m_is_write_locked(struct p2m_domain *p2m)
     return rw_is_write_locked(&p2m->lock);
 }
 
+/*
+ * Helpers to lookup properties of ptes.
+ */
+
+static inline bool_t p2m_valid(lpae_t pte)
+{
+    return pte.p2m.valid;
+}
+/*
+ * These two 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_t p2m_table(lpae_t pte)
+{
+    return p2m_valid(pte) && pte.p2m.table;
+}
+static inline bool_t p2m_mapping(lpae_t pte)
+{
+    return p2m_valid(pte) && !pte.p2m.table;
+}
+
+static inline bool p2m_is_superpage(lpae_t pte, unsigned int level)
+{
+    return (level < 3) && p2m_mapping(pte);
+}
+
 /* Look up the MFN corresponding to a domain's GFN. */
 mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
 
-- 
2.12.2


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

[RFC PATCH v3 06/10] arm/mem_access: Introduce GV2M_EXEC permission

[Sergej Proskurin]

We extend the current implementation by an additional permission,
GV2M_EXEC, which will be used to describe execute permissions of PTE's
as part of our guest translation table walk implementation.

Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
---
Cc:
---
 xen/include/asm-arm/page.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 7a4aa64144..5bdb2e463e 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -89,6 +89,7 @@
 /* Flags for get_page_from_gva, gvirt_to_maddr etc */
 #define GV2M_READ  (0u<<0)
 #define GV2M_WRITE (1u<<0)
+#define GV2M_EXEC  (1u<<1)
 
 #ifndef __ASSEMBLY__
 
-- 
2.12.2


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

[RFC PATCH v3 07/10] arm/mem_access: Add software guest-page-table walk

[Sergej Proskurin]

The function p2m_mem_access_check_and_get_page in mem_access.c
translates a gva to an ipa by means of the hardware functionality of the
ARM architecture. This is implemented in the function gva_to_ipa. If
mem_access is active, hardware-based gva to ipa translation might fail,
as gva_to_ipa uses the guest's translation tables, access to which might
be restricted by the active VTTBR. To address this issue, in this commit
we add a software-based guest-page-table walk, which will be used by the
function p2m_mem_access_check_and_get_page perform the gva to ipa
translation in software in one of the following commits.

Note: The introduced function guest_walk_tables assumes that the domain,
the gva of which is to be translated, is running on the currently active
vCPU. To walk the guest's page tables on a different vCPU, the following
registers would need to be loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and
SCTLR_EL1.

Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
---
v2: Rename p2m_gva_to_ipa to p2m_walk_gpt and move it to p2m.c.

    Move the functionality responsible for walking long-descriptor based
    translation tables out of the function p2m_walk_gpt. Also move out
    the long-descriptor based translation out of this commit.

    Change function parameters in order to return access access rights
    to a requested gva.

    Cosmetic fixes.

v3: Rename the introduced functions to guest_walk_(tables|sd|ld) and
    move the implementation to guest_copy.(c|h).

    Set permissions in guest_walk_tables also if the MMU is disabled.

    Change the function parameter of type "struct p2m_domain *" to
    "struct vcpu *" in the functions guest_walk_(tables|sd|ld).
---
 xen/arch/arm/Makefile            |  1 +
 xen/arch/arm/guest_walk.c        | 92 ++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/guest_walk.h | 19 +++++++++
 3 files changed, 112 insertions(+)
 create mode 100644 xen/arch/arm/guest_walk.c
 create mode 100644 xen/include/asm-arm/guest_walk.h

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 49e1fb2f84..282d2c2949 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_HAS_GICV3) += gic-v3.o
 obj-$(CONFIG_HAS_ITS) += gic-v3-its.o
 obj-$(CONFIG_HAS_ITS) += gic-v3-lpi.o
 obj-y += guestcopy.o
+obj-y += guest_walk.o
 obj-y += hvm.o
 obj-y += io.o
 obj-y += irq.o
diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
new file mode 100644
index 0000000000..f2f3533665
--- /dev/null
+++ b/xen/arch/arm/guest_walk.c
@@ -0,0 +1,92 @@
+/*
+ * Guest page table walk
+ * Copyright (c) 2017 Sergej Proskurin <proskurin@sec.in.tum.de>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/sched.h>
+
+/*
+ * The function guest_walk_sd translates a given GVA into an IPA using the
+ * short-descriptor translation table format in software. This function assumes
+ * that the domain is running on the currently active vCPU. To walk the guest's
+ * 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(struct domain *d,
+                         vaddr_t gva, paddr_t *ipa,
+                         unsigned int *perms)
+{
+    /* Not implemented yet. */
+    return -EFAULT;
+}
+
+/*
+ * The function guest_walk_ld translates a given GVA into an IPA using the
+ * long-descriptor translation table format in software. This function assumes
+ * that the domain is running on the currently active vCPU. To walk the guest's
+ * 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(struct domain *d,
+                         vaddr_t gva, paddr_t *ipa,
+                         unsigned int *perms)
+{
+    /* Not implemented yet. */
+    return -EFAULT;
+}
+
+int 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);
+    struct domain *d = v->domain;
+    unsigned int _perms = GV2M_READ;
+
+    /* We assume that the domain is running on the currently active domain. */
+    if ( v != current )
+        return -EFAULT;
+
+    /* Allow perms to be NULL. */
+    perms = perms ?: &_perms;
+
+    /* If the MMU is disabled, there is no need to translate the gva. */
+    if ( !(sctlr & SCTLR_M) )
+    {
+        *ipa = gva;
+
+        /* Memory can be accessed without any restrictions. */
+        *perms = GV2M_READ|GV2M_WRITE|GV2M_EXEC;
+
+        return 0;
+    }
+
+    if ( is_32bit_domain(d) )
+    {
+        if ( !(tcr & TTBCR_EAE) )
+            return guest_walk_sd(d, gva, ipa, perms);
+    }
+
+    return guest_walk_ld(d, gva, ipa, perms);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-arm/guest_walk.h b/xen/include/asm-arm/guest_walk.h
new file mode 100644
index 0000000000..4ed8476e08
--- /dev/null
+++ b/xen/include/asm-arm/guest_walk.h
@@ -0,0 +1,19 @@
+#ifndef _XEN_GUEST_WALK_H
+#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);
+
+#endif /* _XEN_GUEST_WALK_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.12.2


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

[RFC PATCH v3 08/10] arm/mem_access: Add long-descriptor based gpt

[Sergej Proskurin]

This commit adds functionality to walk the guest's page tables using the
long-descriptor translation table format for both ARMv7 and ARMv8.
Similar to the hardware architecture, the implementation supports
different page granularities (4K, 16K, and 64K). The implementation is
based on ARM DDI 0487B-a J1-5922, J1-5999, and ARM DDI 0406C-b B3-1510.

Note that the current implementation lacks support for Large VA/PA on
ARMv8.2 architectures (LVA/LPA, 52-bit virtual and physical address
sizes). The associated location in the code is marked appropriately.

Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
---
v2: Use TCR_SZ_MASK instead of TTBCR_SZ_MASK for ARM 32-bit guests using
    the long-descriptor translation table format.

    Cosmetic fixes.

v3: Move the implementation to ./xen/arch/arm/guest_copy.c.

    Remove the array strides and declare the array grainsizes as static
    const instead of just const to reduce the function stack overhead.

    Move parts of the funtion guest_walk_ld into the static functions
    get_ttbr_and_gran_64bit and get_top_bit to reduce complexity.

    Use the macro BIT(x) instead of (1UL << x).

    Add more comments && Cosmetic fixes.
---
 xen/arch/arm/guest_walk.c        | 397 ++++++++++++++++++++++++++++++++++++++-
 xen/include/asm-arm/guest_walk.h |   9 +
 2 files changed, 404 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index f2f3533665..90bcc218ec 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -16,6 +16,8 @@
  */
 
 #include <xen/sched.h>
+#include <xen/domain_page.h>
+#include <asm/guest_walk.h>
 
 /*
  * The function guest_walk_sd translates a given GVA into an IPA using the
@@ -32,6 +34,92 @@ static int guest_walk_sd(struct domain *d,
     return -EFAULT;
 }
 
+#ifdef CONFIG_ARM_64
+/*
+ * Select the TTBR(0|1)_EL1 that will be used for address translation using the
+ * long-descriptor translation table format and return the page granularity
+ * that is used by the selected TTBR.
+ */
+static bool get_ttbr_and_gran_64bit(uint64_t *ttbr, unsigned int *gran,
+                                    register_t tcr, bool ttbrx)
+{
+    bool disabled;
+
+    if ( ttbrx == TTBR0_VALID )
+    {
+        /* Normalize granule size. */
+        switch ( tcr & TCR_TG0_MASK )
+        {
+        case TCR_TG0_16K:
+            *gran = GRANULE_SIZE_INDEX_16K;
+            break;
+        case TCR_TG0_64K:
+            *gran = GRANULE_SIZE_INDEX_64K;
+            break;
+        default:
+            *gran = GRANULE_SIZE_INDEX_4K;
+        }
+
+        /* Use TTBR0 for GVA to IPA translation. */
+        *ttbr = READ_SYSREG64(TTBR0_EL1);
+
+        /* If TCR.EPD0 is set, translations using TTBR0 are disabled. */
+        disabled = tcr & TCR_EPD0;
+    }
+    else
+    {
+        /* Normalize granule size. */
+        switch ( tcr & TCR_EL1_TG1_MASK )
+        {
+        case TCR_EL1_TG1_16K:
+            *gran = GRANULE_SIZE_INDEX_16K;
+            break;
+        case TCR_EL1_TG1_64K:
+            *gran = GRANULE_SIZE_INDEX_64K;
+            break;
+        default:
+            *gran = GRANULE_SIZE_INDEX_4K;
+        }
+
+        /* Use TTBR1 for GVA to IPA translation. */
+        *ttbr = READ_SYSREG64(TTBR1_EL1);
+
+        /* If TCR.EPD1 is set, translations using TTBR1 are disabled. */
+        disabled = tcr & TCR_EPD1;
+    }
+
+    return disabled;
+}
+#endif
+
+/*
+ * Get the MSB number of the GVA, according to "AddrTop" pseudocode
+ * implementation in ARM DDI 0487B-a J1-6066.
+ */
+static unsigned int get_top_bit(struct domain *d, vaddr_t gva, register_t tcr)
+{
+    unsigned int topbit;
+
+    /*
+     * IF EL1 is using AArch64 then addresses from EL0 using AArch32 are
+     * zero-extended to 64 bits (ARM DDI 0487B-a J1-6066).
+     */
+    if ( is_32bit_domain(d) )
+        topbit = 31;
+#ifdef CONFIG_ARM_64
+    else if ( is_64bit_domain(d) )
+    {
+        if ( ((gva & BIT(55)) && (tcr & TCR_EL1_TBI1)) ||
+             (!(gva & BIT(55)) && (tcr & TCR_EL1_TBI0)) )
+            topbit = 55;
+        else
+            topbit = 63;
+    }
+#endif
+
+    return topbit;
+}
+
 /*
  * The function guest_walk_ld translates a given GVA into an IPA using the
  * long-descriptor translation table format in software. This function assumes
@@ -43,8 +131,313 @@ static int guest_walk_ld(struct domain *d,
                          vaddr_t gva, paddr_t *ipa,
                          unsigned int *perms)
 {
-    /* Not implemented yet. */
-    return -EFAULT;
+    bool disabled = true;
+    bool ro_table = false, xn_table = false;
+    unsigned int t0_sz, t1_sz;
+    unsigned int level, gran;
+    unsigned int topbit = 0, input_size = 0, output_size;
+    uint64_t ttbr = 0, ips;
+    paddr_t mask;
+    lpae_t pte, *table;
+    struct page_info *page;
+    register_t tcr = READ_SYSREG(TCR_EL1);
+
+    const vaddr_t offsets[4][3] = {
+        {
+#ifdef CONFIG_ARM_64
+            zeroeth_guest_table_offset_4k(gva),
+            zeroeth_guest_table_offset_16k(gva),
+            0, /* There is no zeroeth lookup level with a 64K granule size. */
+#endif
+        },
+        {
+            first_guest_table_offset_4k(gva),
+#ifdef CONFIG_ARM_64
+            first_guest_table_offset_16k(gva),
+            first_guest_table_offset_64k(gva),
+#endif
+        },
+        {
+            second_guest_table_offset_4k(gva),
+#ifdef CONFIG_ARM_64
+            second_guest_table_offset_16k(gva),
+            second_guest_table_offset_64k(gva),
+#endif
+        },
+        {
+            third_guest_table_offset_4k(gva),
+#ifdef CONFIG_ARM_64
+            third_guest_table_offset_16k(gva),
+            third_guest_table_offset_64k(gva),
+#endif
+        }
+    };
+
+    static const paddr_t masks[4][3] = {
+        {
+            zeroeth_size(4K) - 1,
+            zeroeth_size(16K) - 1,
+            0 /* There is no zeroeth lookup level with a 64K granule size. */
+        },
+        {
+            first_size(4K) - 1,
+            first_size(16K) - 1,
+            first_size(64K) - 1
+        },
+        {
+            second_size(4K) - 1,
+            second_size(16K) - 1,
+            second_size(64K) - 1
+        },
+        {
+            third_size(4K) - 1,
+            third_size(16K) - 1,
+            third_size(64K) - 1
+        }
+    };
+
+    static const unsigned int grainsizes[3] = {
+        PAGE_SHIFT_4K,
+        PAGE_SHIFT_16K,
+        PAGE_SHIFT_64K
+    };
+
+    t0_sz = (tcr >> TCR_T0SZ_SHIFT) & TCR_SZ_MASK;
+    t1_sz = (tcr >> TCR_T1SZ_SHIFT) & TCR_SZ_MASK;
+
+    /* Get the MSB number of the GVA. */
+    topbit = get_top_bit(d, gva, tcr);
+
+#ifdef CONFIG_ARM_64
+    if ( is_64bit_domain(d) )
+    {
+        /* Select the TTBR(0|1)_EL1 that will be used for address translation. */
+
+        if ( (gva & BIT(topbit)) == 0 )
+        {
+            input_size = REGISTER_WIDTH_64_BIT - t0_sz;
+
+            /* Get TTBR0 and configured page granularity. */
+            disabled = get_ttbr_and_gran_64bit(&ttbr, &gran, tcr, TTBR0_VALID);
+        }
+        else
+        {
+            input_size = REGISTER_WIDTH_64_BIT - t1_sz;
+
+            /* Get TTBR1 and configured page granularity. */
+            disabled = get_ttbr_and_gran_64bit(&ttbr, &gran, tcr, TTBR1_VALID);
+        }
+
+        /*
+         * The current implementation supports intermediate physical address
+         * sizes (IPS) up to 48 bit.
+         *
+         * XXX: Determine whether the IPS_MAX_VAL is 48 or 52 in software.
+         */
+        if ( (input_size > TCR_EL1_IPS_48_BIT_VAL) ||
+             (input_size < TCR_EL1_IPS_MIN_VAL) )
+            return -EFAULT;
+    }
+    else
+#endif
+    {
+        /* Granule size of AArch32 architectures is always 4K. */
+        gran = GRANULE_SIZE_INDEX_4K;
+
+        /* Select the TTBR(0|1)_EL1 that will be used for address translation. */
+
+        /*
+         * Check if the bits <31:32-t0_sz> of the GVA are set to 0 (DDI 0487B-a
+         * J1-5999). If so, TTBR0 shall be used for address translation.
+         */
+        mask = ((1ULL << REGISTER_WIDTH_32_BIT) - 1) &
+               ~((1ULL << (REGISTER_WIDTH_32_BIT - t0_sz)) - 1);
+
+        if ( t0_sz == 0 || !(gva & mask) )
+        {
+            input_size = REGISTER_WIDTH_32_BIT - t0_sz;
+
+            /* Use TTBR0 for GVA to IPA translation. */
+            ttbr = READ_SYSREG64(TTBR0_EL1);
+
+            /* If TCR.EPD0 is set, translations using TTBR0 are disabled. */
+            disabled = tcr & TCR_EPD0;
+        }
+
+        /*
+         * Check if the bits <31:32-t1_sz> of the GVA are set to 1 (DDI 0487B-a
+         * J1-6000). If so, TTBR1 shall be used for address translation.
+         */
+        mask = ((1ULL << REGISTER_WIDTH_32_BIT) - 1) &
+               ~((1ULL << (REGISTER_WIDTH_32_BIT - t1_sz)) - 1);
+
+        if ( ((t1_sz == 0) && !ttbr) || (t1_sz && (gva & mask) == mask) )
+        {
+            input_size = REGISTER_WIDTH_32_BIT - t1_sz;
+
+            /* Use TTBR1 for GVA to IPA translation. */
+            ttbr = READ_SYSREG64(TTBR1_EL1);
+
+            /* If TCR.EPD1 is set, translations using TTBR1 are disabled. */
+            disabled = tcr & TCR_EPD1;
+        }
+    }
+
+    if ( disabled )
+        return -EFAULT;
+
+    /*
+     * The starting level is the number of strides (grainsizes[gran] - 3)
+     * needed to consume the input address (DDI 0487B-a J1-5924).
+     */
+    level = 4 - DIV_ROUND_UP((input_size - grainsizes[gran]), (grainsizes[gran] - 3));
+
+    if ( is_64bit_domain(d) )
+    {
+        /* Get the intermediate physical address size. */
+        ips = (tcr & TCR_EL1_IPS_MASK) >> TCR_EL1_IPS_SHIFT;
+
+        switch (ips)
+        {
+        case TCR_EL1_IPS_32_BIT:
+            output_size = TCR_EL1_IPS_32_BIT_VAL;
+            break;
+        case TCR_EL1_IPS_36_BIT:
+            output_size = TCR_EL1_IPS_36_BIT_VAL;
+            break;
+        case TCR_EL1_IPS_40_BIT:
+            output_size = TCR_EL1_IPS_40_BIT_VAL;
+            break;
+        case TCR_EL1_IPS_42_BIT:
+            output_size = TCR_EL1_IPS_42_BIT_VAL;
+            break;
+        case TCR_EL1_IPS_44_BIT:
+            output_size = TCR_EL1_IPS_44_BIT_VAL;
+            break;
+        case TCR_EL1_IPS_48_BIT:
+            output_size = TCR_EL1_IPS_48_BIT_VAL;
+            break;
+        case TCR_EL1_IPS_52_BIT:
+            /* XXX: 52 bit output_size is not supported yet. */
+            return -EFAULT;
+        default:
+            output_size = TCR_EL1_IPS_48_BIT_VAL;
+        }
+    }
+    else
+        output_size = TCR_EL1_IPS_40_BIT_VAL;
+
+    /* Make sure the base address does not exceed its configured size. */
+    mask = ((1ULL << TCR_EL1_IPS_48_BIT_VAL) - 1) & ~((1ULL << output_size) - 1);
+    if ( output_size < TCR_EL1_IPS_48_BIT_VAL && (ttbr & mask) )
+        return -EFAULT;
+
+    mask = ((1ULL << output_size) - 1);
+    page = get_page_from_gfn(d, paddr_to_pfn(ttbr & mask), NULL, P2M_ALLOC);
+    if ( !page )
+        return -EFAULT;
+
+    table = __map_domain_page(page);
+
+    for ( ; ; level++ )
+    {
+        pte = table[offsets[level][gran]];
+
+        unmap_domain_page(table);
+        put_page(page);
+
+        /* Make sure the base address does not exceed its configured size. */
+        mask = ((1ULL << TCR_EL1_IPS_48_BIT_VAL) - 1) &
+               ~((1ULL << output_size) - 1);
+
+        if ( (output_size < TCR_EL1_IPS_48_BIT_VAL) &&
+             (pfn_to_paddr(pte.walk.base) & mask) )
+            return -EFAULT;
+
+#ifdef CONFIG_ARM_64
+        /*
+         * If page granularity is 64K, make sure the address is aligned
+         * appropriately.
+         */
+        if ( (output_size < TCR_EL1_IPS_52_BIT_VAL) &&
+             (gran == GRANULE_SIZE_INDEX_64K) &&
+             (pte.walk.base & 0xf) )
+            return -EFAULT;
+#endif
+
+        /*
+         * Break if one of the following conditions are true:
+         *
+         * - We have found the PTE holding the IPA (level == 3).
+         * - The PTE is not valid.
+         * - If (level < 3) and the PTE is valid, we found a block descriptor.
+         */
+        if ( level == 3 || !p2m_valid(pte) || !p2m_table(pte) )
+            break;
+
+        /*
+         * Temporarily store permissions of the table descriptor as they are
+         * inherited by page table attributes (ARM DDI 0487B-a J1-5928).
+         */
+        xn_table = xn_table | !!(pte.pt.xnt);          /* Execute-Never */
+        ro_table = ro_table | !!(pte.pt.apt & BIT(1)); /* Read-Only */
+
+#ifdef CONFIG_ARM_64
+        if ( output_size == TCR_EL1_IPS_52_BIT_VAL )
+        {
+            unsigned long gfn;
+
+            /*
+             * The GFN must be rearranged according to the following format of
+             * the PTE bits [pte<15:12>:pte<47:16>:0000].
+             */
+            gfn = ((unsigned long)(pte.walk.base & 0xf) << 36) |
+                  (pte.walk.base & ~0xf);
+
+            page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
+        }
+        else
+#endif
+            page = get_page_from_gfn(d, pte.walk.base, NULL, P2M_ALLOC);
+
+        if ( !page )
+            return -EFAULT;
+
+        table = __map_domain_page(page);
+    }
+
+    if ( !p2m_valid(pte) || ((level == 3) && !p2m_table(pte)) )
+        return -EFAULT;
+
+#ifdef CONFIG_ARM_64
+    if ( output_size == TCR_EL1_IPS_52_BIT_VAL )
+    {
+        unsigned long gfn;
+
+        /*
+         * The GFN must be rearranged according to the following format of the
+         * PTE bits [pte<15:12>:pte<47:16>:0000].
+         */
+        gfn = ((unsigned long)(pte.walk.base & 0xf) << 36) |
+              (pte.walk.base & ~0xf);
+
+        *ipa = pfn_to_paddr(gfn) | (gva & masks[level][gran]);
+    }
+    else
+#endif
+        *ipa = pfn_to_paddr(pte.walk.base) | (gva & masks[level][gran]);
+
+    /*
+     * Set permissions so that the caller can check the flags by herself. Note
+     * that stage 1 translations also inherit attributes from the tables
+     * (ARM DDI 0487B-a J1-5928).
+     */
+    if ( !pte.pt.ro && !ro_table )
+        *perms = GV2M_WRITE;
+    if ( !pte.pt.xn && !xn_table )
+        *perms |= GV2M_EXEC;
+
+    return 0;
 }
 
 int guest_walk_tables(const struct vcpu *v, vaddr_t gva,
diff --git a/xen/include/asm-arm/guest_walk.h b/xen/include/asm-arm/guest_walk.h
index 4ed8476e08..ce01f0fa08 100644
--- a/xen/include/asm-arm/guest_walk.h
+++ b/xen/include/asm-arm/guest_walk.h
@@ -1,6 +1,15 @@
 #ifndef _XEN_GUEST_WALK_H
 #define _XEN_GUEST_WALK_H
 
+/* Normalized page granule size indices. */
+#define GRANULE_SIZE_INDEX_4K               (0)
+#define GRANULE_SIZE_INDEX_16K              (1)
+#define GRANULE_SIZE_INDEX_64K              (2)
+
+/* Represent whether TTBR0 or TTBR1 is valid. */
+#define TTBR0_VALID                         (0)
+#define TTBR1_VALID                         (1)
+
 /* Walk the guest's page tables in software. */
 int guest_walk_tables(const struct vcpu *v,
                       vaddr_t gva,
-- 
2.12.2


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

[RFC PATCH v3 09/10] arm/mem_access: Add short-descriptor based gpt

[Sergej Proskurin]

This commit adds functionality to walk the guest's page tables using the
short-descriptor translation table format for both ARMv7 and ARMv8. The
implementation is based on ARM DDI 0487B-a J1-6002 and ARM DDI 0406C-b
B3-1506.

Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
---
v3: Move the implementation to ./xen/arch/arm/guest_copy.c.

    Use defines instead of hardcoded values.

    Cosmetic fixes & Added more coments.
---
 xen/arch/arm/guest_walk.c        | 165 ++++++++++++++++++++++++++++++++++++++-
 xen/include/asm-arm/guest_walk.h |  16 ++++
 2 files changed, 179 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index 90bcc218ec..7a61341533 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -30,8 +30,169 @@ static int guest_walk_sd(struct domain *d,
                          vaddr_t gva, paddr_t *ipa,
                          unsigned int *perms)
 {
-    /* Not implemented yet. */
-    return -EFAULT;
+    bool disabled = true;
+    int64_t ttbr;
+    paddr_t mask, paddr;
+    pte_sd_t pte, *table;
+    struct page_info *page;
+    register_t ttbcr = READ_SYSREG(TCR_EL1);
+    unsigned int level = 0, n = ttbcr & TTBCR_N_MASK;
+
+    const paddr_t offsets[2] = {
+        ((paddr_t)(gva >> 20) & ((1ULL << (12 - n)) - 1)),
+        ((paddr_t)(gva >> 12) & ((1ULL << 8) - 1))
+    };
+
+    mask = ((1ULL << REGISTER_WIDTH_32_BIT) - 1) &
+           ~((1ULL << (REGISTER_WIDTH_32_BIT - n)) - 1);
+
+    if ( n == 0 || !(gva & mask) )
+    {
+        /* Use TTBR0 for GVA to IPA translation. */
+        ttbr = READ_SYSREG64(TTBR0_EL1);
+
+        /* If TTBCR.PD0 is set, translations using TTBR0 are disabled. */
+        disabled = ttbcr & TTBCR_PD0;
+    }
+    else
+    {
+        /* Use TTBR1 for GVA to IPA translation. */
+        ttbr = READ_SYSREG64(TTBR1_EL1);
+
+        /* If TTBCR.PD1 is set, translations using TTBR1 are disabled. */
+        disabled = ttbcr & TTBCR_PD1;
+
+        /*
+         * TTBR1 translation always works like n==0 TTBR0 translation (ARM DDI
+         * 0487B-a J1-6003).
+         */
+        n = 0;
+    }
+
+    if ( disabled )
+        return -EFAULT;
+
+    /*
+     * The address of the descriptor for the initial lookup has the following
+     * format: [ttbr<31:14-n>:gva<31-n:20>:00] (ARM DDI 0487B-a J1-6003). In
+     * this way, the first lookup level might comprise up to four consecutive
+     * pages. To avoid mapping all of the pages, we simply map the page that is
+     * needed by the first level translation by incorporating up to 2 MSBs of
+     * the GVA.
+     */
+    mask = (1ULL << (14 - n)) - 1;
+    paddr = (ttbr & ~mask) | (offsets[level] << 2);
+
+    page = get_page_from_gfn(d, paddr_to_pfn(paddr), NULL, P2M_ALLOC);
+    if ( !page )
+        return -EFAULT;
+
+    table = __map_domain_page(page);
+
+    /*
+     * Consider that the first level address translation does not need to be
+     * page-aligned if n > 2.
+     */
+    if ( n > 2 )
+    {
+        /* Make sure that we consider the bits ttbr<12:14-n> if n > 2. */
+        mask = ((1ULL << 12) - 1) & ~((1ULL << (14 - n)) - 1);
+        table = (pte_sd_t *)((unsigned long)table | (unsigned long)(ttbr & mask));
+    }
+
+    /*
+     * As we have considered up to 2 MSBs of the GVA for mapping the first
+     * level translation table, we need to make sure that we limit the table
+     * offset that is is indexed by GVA<31-n:20> to max 10 bits to avoid
+     * exceeding the page size limit.
+     */
+    mask = ((1ULL << 10) - 1);
+    pte = table[offsets[level] & mask];
+
+    unmap_domain_page(table);
+    put_page(page);
+
+    switch ( pte.walk.dt )
+    {
+    case L1DESC_INVALID:
+        return -EFAULT;
+
+    case L1DESC_PAGE_TABLE:
+        level++;
+
+        page = get_page_from_gfn(d, (pte.walk.base >> 2), NULL, P2M_ALLOC);
+        if ( !page )
+            return -EFAULT;
+
+        table = __map_domain_page(page);
+        /*
+         * The second level translation table is addressed by PTE<31:10>. Hence
+         * it does not need to be page aligned. Make sure that we also consider
+         * the bits PTE<11:10>.
+         */
+        table = (pte_sd_t *)((unsigned long)table | ((pte.walk.base & 0x3) << 10));
+
+        pte = table[offsets[level]];
+
+        unmap_domain_page(table);
+        put_page(page);
+
+        if ( pte.walk.dt == L2DESC_INVALID )
+            return -EFAULT;
+
+        if ( pte.pg.page ) /* Small page. */
+        {
+            mask = (1ULL << PAGE_SHIFT_4K) - 1;
+
+            *ipa = (pte.pg.base << PAGE_SHIFT_4K) | (gva & mask);
+
+            /* Set execute permissions associated with the small page. */
+            if ( !pte.pg.xn )
+                *perms = GV2M_EXEC;
+        }
+        else /* Large page. */
+        {
+            mask = (1ULL << PAGE_SHIFT_64K) - 1;
+
+            *ipa = (pte.lpg.base << PAGE_SHIFT_64K) | (gva & mask);
+
+            /* Set execute permissions associated with the large page. */
+            if ( !pte.lpg.xn )
+                *perms = GV2M_EXEC;
+        }
+
+        /* Set permissions so that the caller can check the flags by herself. */
+        if ( !pte.pg.ro )
+            *perms |= GV2M_WRITE;
+
+        break;
+
+    case L1DESC_SECTION:
+    case L1DESC_SECTION_PXN:
+        if ( !pte.sec.supersec ) /* Section */
+        {
+            mask = (1ULL << L1DESC_SECTION_SHIFT) - 1;
+
+            *ipa = (pte.sec.base << L1DESC_SECTION_SHIFT) | (gva & mask);
+        }
+        else /* Supersection */
+        {
+            mask = (1ULL << L1DESC_SUPERSECTION_SHIFT) - 1;
+
+            *ipa = gva & mask;
+            *ipa |= (paddr_t)(pte.supersec.base) << L1DESC_SUPERSECTION_SHIFT;
+            *ipa |= (paddr_t)(pte.supersec.extbase1) << L1DESC_SUPERSECTION_EXT_BASE1_SHIFT;
+            *ipa |= (paddr_t)(pte.supersec.extbase2) << L1DESC_SUPERSECTION_EXT_BASE2_SHIFT;
+        }
+
+        /* Set permissions so that the caller can check the flags by herself. */
+        if ( !pte.sec.ro )
+            *perms = GV2M_WRITE;
+        if ( !pte.sec.xn )
+            *perms |= GV2M_EXEC;
+    }
+
+    return 0;
 }
 
 #ifdef CONFIG_ARM_64
diff --git a/xen/include/asm-arm/guest_walk.h b/xen/include/asm-arm/guest_walk.h
index ce01f0fa08..f7269be8d7 100644
--- a/xen/include/asm-arm/guest_walk.h
+++ b/xen/include/asm-arm/guest_walk.h
@@ -10,6 +10,22 @@
 #define TTBR0_VALID                         (0)
 #define TTBR1_VALID                         (1)
 
+/* First level translation table descriptor types used by the AArch32
+ * short-descriptor translation table format. */
+#define L1DESC_INVALID                      (0)
+#define L1DESC_PAGE_TABLE                   (1)
+#define L1DESC_SECTION                      (2)
+#define L1DESC_SECTION_PXN                  (3)
+
+/* Defines for section and supersection shifts. */
+#define L1DESC_SECTION_SHIFT                (20)
+#define L1DESC_SUPERSECTION_SHIFT           (24)
+#define L1DESC_SUPERSECTION_EXT_BASE1_SHIFT (32)
+#define L1DESC_SUPERSECTION_EXT_BASE2_SHIFT (36)
+
+/* Second level translation table descriptor types. */
+#define L2DESC_INVALID                      (0)
+
 /* Walk the guest's page tables in software. */
 int guest_walk_tables(const struct vcpu *v,
                       vaddr_t gva,
-- 
2.12.2


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

[RFC PATCH v3 10/10] arm/mem_access: Walk the guest's pt in software

[Sergej Proskurin]

In this commit, we make use of the gpt walk functionality introduced in
the previous commits. If mem_access is active, hardware-based gva to ipa
translation might fail, as gva_to_ipa uses the guest's translation
tables, access to which might be restricted by the active VTTBR. To
side-step potential translation errors in the function
p2m_mem_access_check_and_get_page due to restricted memory (e.g. to the
guest's page tables themselves), we walk the guest's page tables in
software.

Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
---
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
---
v2: Check the returned access rights after walking the guest's page tables in
    the function p2m_mem_access_check_and_get_page.

v3: Adapt Function names and parameter.
---
 xen/arch/arm/mem_access.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
index 04b1506b00..acb5539bb6 100644
--- a/xen/arch/arm/mem_access.c
+++ b/xen/arch/arm/mem_access.c
@@ -22,6 +22,7 @@
 #include <xen/vm_event.h>
 #include <public/vm_event.h>
 #include <asm/event.h>
+#include <asm/guest_walk.h>
 
 static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
                                 xenmem_access_t *access)
@@ -101,6 +102,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
                                   const struct vcpu *v)
 {
     long rc;
+    unsigned int perms;
     paddr_t ipa;
     gfn_t gfn;
     mfn_t mfn;
@@ -110,8 +112,25 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
     struct p2m_domain *p2m = &v->domain->arch.p2m;
 
     rc = gva_to_ipa(gva, &ipa, flag);
+
+    /*
+     * In case mem_access is active, hardware-based gva_to_ipa translation
+     * might fail. Since gva_to_ipa uses the guest's translation tables, access
+     * to which might be restricted by the active VTTBR, we perform a gva to
+     * ipa translation in software.
+     */
     if ( rc < 0 )
-        goto err;
+    {
+        if ( guest_walk_tables(v, gva, &ipa, &perms) < 0 )
+            /*
+             * The software gva to ipa translation can still fail, e.g., if the
+             * gva is not mapped.
+             */
+            goto err;
+
+        if ( ((flag & GV2M_WRITE) == GV2M_WRITE) && !(perms & GV2M_WRITE) )
+            goto err;
+    }
 
     gfn = _gfn(paddr_to_pfn(ipa));
 
-- 
2.12.2


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

Re: [RFC PATCH v3 04/10] arm/mem_access: Add short-descriptor pte typedefs

[Andrew Cooper]

On 15/06/17 12:05, Sergej Proskurin wrote:
> The current implementation does not provide appropriate types for
> short-descriptor translation table entries. As such, this commit adds new
> types, which simplify managing the respective translation table entries.
>
> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
> v3: Add more short-descriptor related pte typedefs that will be used by
>     the following commits.
> ---
>  xen/include/asm-arm/page.h | 104 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 104 insertions(+)
>
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index e2e4b597a5..7a4aa64144 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -205,6 +205,110 @@ typedef union {
>      lpae_walk_t walk;
>  } lpae_t;
>  
> +/*
> + *  Comprises bits of the level 1 short-descriptor format representing
> + *  a section.
> + */
> +typedef struct __packed {
> +    unsigned int pxn:1;         /* Privileged Execute Never */

(I'm not an ARM maintainer, but) can I recommend using bool bitfields
for boolean fields like this.

There's no difference when using the structure for reading data, but it
helps reduce subtle bugs such as

foo.pxn = (flags & NO_EXECUTE);

when using the structures to create a new value, or modify existing ones.

~Andrew

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

Re: [RFC PATCH v3 10/10] arm/mem_access: Walk the guest's pt in software

[Tamas K Lengyel]

On Thu, Jun 15, 2017 at 5:05 AM, Sergej Proskurin
<proskurin@sec.in.tum.de> wrote:
> In this commit, we make use of the gpt walk functionality introduced in
> the previous commits. If mem_access is active, hardware-based gva to ipa
> translation might fail, as gva_to_ipa uses the guest's translation
> tables, access to which might be restricted by the active VTTBR. To
> side-step potential translation errors in the function
> p2m_mem_access_check_and_get_page due to restricted memory (e.g. to the
> guest's page tables themselves), we walk the guest's page tables in
> software.
>
> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
> ---
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Tamas K Lengyel <tamas@tklengyel.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
> v2: Check the returned access rights after walking the guest's page tables in
>     the function p2m_mem_access_check_and_get_page.
>
> v3: Adapt Function names and parameter.
> ---
>  xen/arch/arm/mem_access.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
> index 04b1506b00..acb5539bb6 100644
> --- a/xen/arch/arm/mem_access.c
> +++ b/xen/arch/arm/mem_access.c
> @@ -22,6 +22,7 @@
>  #include <xen/vm_event.h>
>  #include <public/vm_event.h>
>  #include <asm/event.h>
> +#include <asm/guest_walk.h>
>
>  static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
>                                  xenmem_access_t *access)
> @@ -101,6 +102,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
>                                    const struct vcpu *v)
>  {
>      long rc;
> +    unsigned int perms;
>      paddr_t ipa;
>      gfn_t gfn;
>      mfn_t mfn;
> @@ -110,8 +112,25 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
>      struct p2m_domain *p2m = &v->domain->arch.p2m;
>
>      rc = gva_to_ipa(gva, &ipa, flag);
> +
> +    /*
> +     * In case mem_access is active, hardware-based gva_to_ipa translation
> +     * might fail. Since gva_to_ipa uses the guest's translation tables, access
> +     * to which might be restricted by the active VTTBR, we perform a gva to
> +     * ipa translation in software.
> +     */
>      if ( rc < 0 )
> -        goto err;
> +    {
> +        if ( guest_walk_tables(v, gva, &ipa, &perms) < 0 )
> +            /*
> +             * The software gva to ipa translation can still fail, e.g., if the
> +             * gva is not mapped.
> +             */
> +            goto err;
> +
> +        if ( ((flag & GV2M_WRITE) == GV2M_WRITE) && !(perms & GV2M_WRITE) )

Wouldn't it be enough to do (flag & GV2M_WRITE) without the following
comparison? Also, a comment explaining why this is an error-condition
would be nice.

> +            goto err;
> +    }
>
>      gfn = _gfn(paddr_to_pfn(ipa));
>
> --
> 2.12.2

Thanks,
Tamas

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

Re: [RFC PATCH v3 03/10] arm/mem_access: Add defines supporting PTs with varying page sizes

[Julien Grall]

Hi Sergej,

On 06/15/2017 12:05 PM, Sergej Proskurin wrote:
> The ARMv8 architecture supports pages with different (4K, 16K, and 64K) sizes.
> To enable guest page table walks for various configurations, this commit
> extends the defines and helpers of the current implementation.
> 
> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
> v3: Eliminate redundant macro definitions by introducing generic macros.
> ---
>   xen/include/asm-arm/page.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 45 insertions(+)
> 
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 497b4c86ad..e2e4b597a5 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -500,6 +500,51 @@ static inline int gva_to_ipa(vaddr_t va, paddr_t *paddr, unsigned int flags)
>   
>   #define PAGE_ALIGN(x) (((x) + PAGE_SIZE - 1) & PAGE_MASK)
>   
> +#define LPAE_SHIFT_4K           (9)
> +#define LPAE_SHIFT_16K          (11)
> +#define LPAE_SHIFT_64K          (13)
> +
> +#define lpae_entries(gran)      (_AC(1,U) << LPAE_SHIFT_##gran)
> +#define lpae_entry_mask(gran)   (lpae_entries(gran) - 1)
> +
> +#define PAGE_SHIFT_4K           (12)
> +#define PAGE_SHIFT_16K          (14)
> +#define PAGE_SHIFT_64K          (16)
> +
> +#define third_shift(gran)       (PAGE_SHIFT_##gran)
> +#define third_size(gran)        ((paddr_t)1 << third_shift(gran))
> +
> +#define second_shift(gran)      (third_shift(gran) + LPAE_SHIFT_##gran)
> +#define second_size(gran)       ((paddr_t)1 << second_shift(gran))
> +
> +#define first_shift(gran)       (second_shift(gran) + LPAE_SHIFT_##gran)
> +#define first_size(gran)        ((paddr_t)1 << first_shift(gran))
> +
> +/* Note that there is no zeroeth lookup level with a 64K granule size. */
> +#define zeroeth_shift(gran)     (first_shift(gran) + LPAE_SHIFT_##gran)
> +#define zeroeth_size(gran)      ((paddr_t)1 << zeroeth_shift(gran))
> +
> +#define GUEST_TABLE_OFFSET(offs, gran)          ((paddr_t)(offs) & lpae_entry_mask(gran))
> +#define third_guest_table_offset(va, gran)      GUEST_TABLE_OFFSET((va >> third_shift(gran)), gran)
> +#define second_guest_table_offset(va, gran)     GUEST_TABLE_OFFSET((va >> second_shift(gran)), gran)
> +#define first_guest_table_offset(va, gran)      GUEST_TABLE_OFFSET((va >> first_shift(gran)), gran)
> +#define zeroeth_guest_table_offset(va, gran)    GUEST_TABLE_OFFSET((va >> zeroeth_shift(gran)), gran)
> +
> +#define third_guest_table_offset_4k(va)         third_guest_table_offset(va, 4K)
> +#define third_guest_table_offset_16k(va)        third_guest_table_offset(va, 16K)
> +#define third_guest_table_offset_64k(va)        third_guest_table_offset(va, 64K)
> +
> +#define second_guest_table_offset_4k(va)        second_guest_table_offset(va, 4K)
> +#define second_guest_table_offset_16k(va)       second_guest_table_offset(va, 16K)
> +#define second_guest_table_offset_64k(va)       second_guest_table_offset(va, 64K)
> +
> +#define first_guest_table_offset_4k(va)         first_guest_table_offset(va, 4K)
> +#define first_guest_table_offset_16k(va)        first_guest_table_offset(va, 16K)
> +#define first_guest_table_offset_64k(va)        first_guest_table_offset(va, 64K)
> +
> +#define zeroeth_guest_table_offset_4k(va)       zeroeth_guest_table_offset(va, 4K)
> +#define zeroeth_guest_table_offset_16k(va)      zeroeth_guest_table_offset(va, 16K)

So this is really confusing to group by level rather than granularity.

Also, I still think you can make this more generic by introducing macros 
that will generate helpers (see VGIC_REG_HELPERS).

This would require to use static inline function, but at least it would 
avoid to duplicate some much code.

> +
>   #endif /* __ARM_PAGE_H__ */
>   
>   /*
> 

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH v3 04/10] arm/mem_access: Add short-descriptor pte typedefs

[Julien Grall]

Hi Andrew,

On 06/15/2017 01:03 PM, Andrew Cooper wrote:
> On 15/06/17 12:05, Sergej Proskurin wrote:
>> The current implementation does not provide appropriate types for
>> short-descriptor translation table entries. As such, this commit adds new
>> types, which simplify managing the respective translation table entries.
>>
>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>> ---
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>> ---
>> v3: Add more short-descriptor related pte typedefs that will be used by
>>      the following commits.
>> ---
>>   xen/include/asm-arm/page.h | 104 +++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 104 insertions(+)
>>
>> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
>> index e2e4b597a5..7a4aa64144 100644
>> --- a/xen/include/asm-arm/page.h
>> +++ b/xen/include/asm-arm/page.h
>> @@ -205,6 +205,110 @@ typedef union {
>>       lpae_walk_t walk;
>>   } lpae_t;
>>   
>> +/*
>> + *  Comprises bits of the level 1 short-descriptor format representing
>> + *  a section.
>> + */
>> +typedef struct __packed {
>> +    unsigned int pxn:1;         /* Privileged Execute Never */
> 
> (I'm not an ARM maintainer, but) can I recommend using bool bitfields
> for boolean fields like this.

I was not aware it was possible to do boolean fields. I am all for it.

> 
> There's no difference when using the structure for reading data, but it
> helps reduce subtle bugs such as
> 
> foo.pxn = (flags & NO_EXECUTE);
> 
> when using the structures to create a new value, or modify existing ones.
> 
> ~Andrew
> 

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH v3 04/10] arm/mem_access: Add short-descriptor pte typedefs

[Andrew Cooper]

On 15/06/17 20:44, Julien Grall wrote:
> Hi Andrew,
>
> On 06/15/2017 01:03 PM, Andrew Cooper wrote:
>> On 15/06/17 12:05, Sergej Proskurin wrote:
>>> The current implementation does not provide appropriate types for
>>> short-descriptor translation table entries. As such, this commit
>>> adds new
>>> types, which simplify managing the respective translation table
>>> entries.
>>>
>>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>>> ---
>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>> Cc: Julien Grall <julien.grall@arm.com>
>>> ---
>>> v3: Add more short-descriptor related pte typedefs that will be used by
>>>      the following commits.
>>> ---
>>>   xen/include/asm-arm/page.h | 104
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 104 insertions(+)
>>>
>>> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
>>> index e2e4b597a5..7a4aa64144 100644
>>> --- a/xen/include/asm-arm/page.h
>>> +++ b/xen/include/asm-arm/page.h
>>> @@ -205,6 +205,110 @@ typedef union {
>>>       lpae_walk_t walk;
>>>   } lpae_t;
>>>   +/*
>>> + *  Comprises bits of the level 1 short-descriptor format representing
>>> + *  a section.
>>> + */
>>> +typedef struct __packed {
>>> +    unsigned int pxn:1;         /* Privileged Execute Never */
>>
>> (I'm not an ARM maintainer, but) can I recommend using bool bitfields
>> for boolean fields like this.
>
> I was not aware it was possible to do boolean fields. I am all for it.

There isn't a good example in xen yet, but
http://xenbits.xen.org/gitweb/?p=xtf.git;a=commitdiff;h=f099211f2ebdadf61ae6416559220d69b788cd2b
is the XTF work I'm basing some imminent Xen improvements on.

~Andrew

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

Re: [RFC PATCH v3 05/10] arm/p2m: Make PTE helpers publicly available

[Julien Grall]

Hi Sergej,

On 06/15/2017 12:05 PM, Sergej Proskurin wrote:
> In this commit we make the p2m_* helpers, which access PTE properties in
> a simplified way, publicly available. This is due to the fact that the
> helpers will be used in guest_walk.c in one of the following commits.
> 
> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
> ---
> Cc:
> ---
>   xen/arch/arm/p2m.c        | 23 -----------------------
>   xen/include/asm-arm/p2m.h | 27 +++++++++++++++++++++++++++
>   2 files changed, 27 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index b7bbea1d81..eecbcdf870 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -46,29 +46,6 @@ static const paddr_t level_masks[] =
>   static const uint8_t level_orders[] =
>       { ZEROETH_ORDER, FIRST_ORDER, SECOND_ORDER, THIRD_ORDER };
>   
> -static inline bool_t p2m_valid(lpae_t pte)
> -{
> -    return pte.p2m.valid;
> -}
> -/*
> - * These two 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_t p2m_table(lpae_t pte)
> -{
> -    return p2m_valid(pte) && pte.p2m.table;
> -}
> -static inline bool_t p2m_mapping(lpae_t pte)
> -{
> -    return p2m_valid(pte) && !pte.p2m.table;
> -}
> -
> -static inline bool p2m_is_superpage(lpae_t pte, unsigned int level)
> -{
> -    return (level < 3) && p2m_mapping(pte);
> -}
> -
>   static void p2m_flush_tlb(struct p2m_domain *p2m);
>   
>   /* Unlock the flush and do a P2M TLB flush if necessary */
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 18c57f936e..8053f2a0cf 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -198,6 +198,33 @@ static inline int p2m_is_write_locked(struct p2m_domain *p2m)
>       return rw_is_write_locked(&p2m->lock);
>   }
>   
> +/*
> + * Helpers to lookup properties of ptes.
> + */
> +
> +static inline bool_t p2m_valid(lpae_t pte)

The name implies they should only be used for stage-2 page table. But 
you are using for other place such as for stage-1 page table.

This means that they are in the wrong header (they should go in page.h).

I would actually start to split page.h with lpae.h and 
short-descriptor.h to avoid mixing two type of page table in the same 
header. Note that I would be happy if you keep lpae in page.h for the 
time being. But short-descriptor should definitely go in a separate file.

I would also rename the helpers you move to lpae_*.

> +{
> +    return pte.p2m.valid;

As you plan to re-use for other things than stage-2 page-table, you use 
pte.walk rather than pte.p2m.

All those comments applies for the rest of the helpers.

> +}
> +/*
> + * These two 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_t p2m_table(lpae_t pte)
> +{
> +    return p2m_valid(pte) && pte.p2m.table;
> +}
> +static inline bool_t p2m_mapping(lpae_t pte)
> +{
> +    return p2m_valid(pte) && !pte.p2m.table;
> +}
> +
> +static inline bool p2m_is_superpage(lpae_t pte, unsigned int level)
> +{
> +    return (level < 3) && p2m_mapping(pte);
> +}
> +
>   /* Look up the MFN corresponding to a domain's GFN. */
>   mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
>   
> 

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH v3 04/10] arm/mem_access: Add short-descriptor pte typedefs

[Julien Grall]

Hi Sergej,

On 06/15/2017 12:05 PM, Sergej Proskurin wrote:
> The current implementation does not provide appropriate types for
> short-descriptor translation table entries. As such, this commit adds new
> types, which simplify managing the respective translation table entries.
> 
> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
> v3: Add more short-descriptor related pte typedefs that will be used by
>      the following commits.
> ---
>   xen/include/asm-arm/page.h | 104 +++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 104 insertions(+)

page.h is already big and I don't like the idea of mixing lpae and short 
descriptor in the same header. Hence the latter is only used for guest 
page table walk.

As mentioned in a follow-up patch, I would like to start moving out LPAE 
code and short descriptor code in separate header.

I am happy if you only move short-descriptor for now.

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH v3 06/10] arm/mem_access: Introduce GV2M_EXEC permission

[Julien Grall]

Hi Sergej,

On 06/15/2017 12:05 PM, Sergej Proskurin wrote:
> We extend the current implementation by an additional permission,
> GV2M_EXEC, which will be used to describe execute permissions of PTE's
> as part of our guest translation table walk implementation.
> 
> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>

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

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH v3 04/10] arm/mem_access: Add short-descriptor pte typedefs

[Sergej Proskurin]

Hi Andrew,

On 06/15/2017 09:49 PM, Andrew Cooper wrote:
> On 15/06/17 20:44, Julien Grall wrote:
>> Hi Andrew,
>>
>> On 06/15/2017 01:03 PM, Andrew Cooper wrote:
>>> On 15/06/17 12:05, Sergej Proskurin wrote:
>>>> The current implementation does not provide appropriate types for
>>>> short-descriptor translation table entries. As such, this commit
>>>> adds new
>>>> types, which simplify managing the respective translation table
>>>> entries.
>>>>
>>>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>>>> ---
>>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>>> Cc: Julien Grall <julien.grall@arm.com>
>>>> ---
>>>> v3: Add more short-descriptor related pte typedefs that will be used by
>>>>      the following commits.
>>>> ---
>>>>   xen/include/asm-arm/page.h | 104
>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>>   1 file changed, 104 insertions(+)
>>>>
>>>> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
>>>> index e2e4b597a5..7a4aa64144 100644
>>>> --- a/xen/include/asm-arm/page.h
>>>> +++ b/xen/include/asm-arm/page.h
>>>> @@ -205,6 +205,110 @@ typedef union {
>>>>       lpae_walk_t walk;
>>>>   } lpae_t;
>>>>   +/*
>>>> + *  Comprises bits of the level 1 short-descriptor format representing
>>>> + *  a section.
>>>> + */
>>>> +typedef struct __packed {
>>>> +    unsigned int pxn:1;         /* Privileged Execute Never */
>>>
>>> (I'm not an ARM maintainer, but) can I recommend using bool bitfields
>>> for boolean fields like this.
>>
>> I was not aware it was possible to do boolean fields. I am all for it.
> 
> There isn't a good example in xen yet, but
> http://xenbits.xen.org/gitweb/?p=xtf.git;a=commitdiff;h=f099211f2ebdadf61ae6416559220d69b788cd2b
> is the XTF work I'm basing some imminent Xen improvements on.
> 

Thanks for the advice. I did not know that you can use boolean types for
bitfields either. I will fix that right away.

Cheers,
~Sergej

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

Re: [RFC PATCH v3 05/10] arm/p2m: Make PTE helpers publicly available

[Sergej Proskurin]

Hi Julien,

On 06/15/2017 09:53 PM, Julien Grall wrote:
> Hi Sergej,
> 
> On 06/15/2017 12:05 PM, Sergej Proskurin wrote:
>> In this commit we make the p2m_* helpers, which access PTE properties in
>> a simplified way, publicly available. This is due to the fact that the
>> helpers will be used in guest_walk.c in one of the following commits.
>>
>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>> ---
>> Cc:
>> ---
>>   xen/arch/arm/p2m.c        | 23 -----------------------
>>   xen/include/asm-arm/p2m.h | 27 +++++++++++++++++++++++++++
>>   2 files changed, 27 insertions(+), 23 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index b7bbea1d81..eecbcdf870 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -46,29 +46,6 @@ static const paddr_t level_masks[] =
>>   static const uint8_t level_orders[] =
>>       { ZEROETH_ORDER, FIRST_ORDER, SECOND_ORDER, THIRD_ORDER };
>>   -static inline bool_t p2m_valid(lpae_t pte)
>> -{
>> -    return pte.p2m.valid;
>> -}
>> -/*
>> - * These two 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_t p2m_table(lpae_t pte)
>> -{
>> -    return p2m_valid(pte) && pte.p2m.table;
>> -}
>> -static inline bool_t p2m_mapping(lpae_t pte)
>> -{
>> -    return p2m_valid(pte) && !pte.p2m.table;
>> -}
>> -
>> -static inline bool p2m_is_superpage(lpae_t pte, unsigned int level)
>> -{
>> -    return (level < 3) && p2m_mapping(pte);
>> -}
>> -
>>   static void p2m_flush_tlb(struct p2m_domain *p2m);
>>     /* Unlock the flush and do a P2M TLB flush if necessary */
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index 18c57f936e..8053f2a0cf 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -198,6 +198,33 @@ static inline int p2m_is_write_locked(struct
>> p2m_domain *p2m)
>>       return rw_is_write_locked(&p2m->lock);
>>   }
>>   +/*
>> + * Helpers to lookup properties of ptes.
>> + */
>> +
>> +static inline bool_t p2m_valid(lpae_t pte)
> 
> The name implies they should only be used for stage-2 page table. But
> you are using for other place such as for stage-1 page table.
> 
> This means that they are in the wrong header (they should go in page.h).
> 
> I would actually start to split page.h with lpae.h and
> short-descriptor.h to avoid mixing two type of page table in the same
> header. Note that I would be happy if you keep lpae in page.h for the
> time being. But short-descriptor should definitely go in a separate file.
> 
> I would also rename the helpers you move to lpae_*.
> 
>> +{
>> +    return pte.p2m.valid;
> 
> As you plan to re-use for other things than stage-2 page-table, you use
> pte.walk rather than pte.p2m.
> 
> All those comments applies for the rest of the helpers.
> 
>> +}
>> +/*
>> + * These two 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_t p2m_table(lpae_t pte)
>> +{
>> +    return p2m_valid(pte) && pte.p2m.table;
>> +}
>> +static inline bool_t p2m_mapping(lpae_t pte)
>> +{
>> +    return p2m_valid(pte) && !pte.p2m.table;
>> +}
>> +
>> +static inline bool p2m_is_superpage(lpae_t pte, unsigned int level)
>> +{
>> +    return (level < 3) && p2m_mapping(pte);
>> +}
>> +
>>   /* Look up the MFN corresponding to a domain's GFN. */
>>   mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
>>  
> 

Thanks. I have moved the upper helpers to page.h for now and renamed
them to lpae_* helpers as part of my most recent patch version. The
submission will follow soon.

Cheers,
~Sergej

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

Re: [RFC PATCH v3 05/10] arm/p2m: Make PTE helpers publicly available

[Julien Grall]

Hi Sergej,

On 06/16/2017 04:44 PM, Sergej Proskurin wrote:
> Thanks. I have moved the upper helpers to page.h for now and renamed
> them to lpae_* helpers as part of my most recent patch version. The
> submission will follow soon.

They should go in the new file lpae.h (you were CCed on the patch series 
I sent yesterday).

I would also be easier to review if you have 2 patches:
     #1 renaming p2m_* to lpae_* + using walk
     #2 move the helpers in lpae.h

I already have them in my private branch as I need them in other place. 
So I don't mind to send them if it helps you.

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH v3 05/10] arm/p2m: Make PTE helpers publicly available

[Sergej Proskurin]

Hi Julien,

On 06/16/2017 08:23 PM, Julien Grall wrote:
> Hi Sergej,
> 
> On 06/16/2017 04:44 PM, Sergej Proskurin wrote:
>> Thanks. I have moved the upper helpers to page.h for now and renamed
>> them to lpae_* helpers as part of my most recent patch version. The
>> submission will follow soon.
> 
> They should go in the new file lpae.h (you were CCed on the patch series
> I sent yesterday).
> 

Yes, I saw your patch. However, I wasn't sure if I should use your code
without it being acked. So, moving the definitions to lpae.h is not a
problem at all. I will gladly rebase my code on top of your branch (or
simply use your code if you should send it to me).

> I would also be easier to review if you have 2 patches:
>     #1 renaming p2m_* to lpae_* + using walk
>     #2 move the helpers in lpae.h
> 

Yes, this is almost exactly how I did it. Whereas I have (i) moved the
code and (ii) renamed the functions and the affected code regions (that
is their definition + in the code, where the functions were actually
called).

> I already have them in my private branch as I need them in other place.
> So I don't mind to send them if it helps you.
> 

Sure, that would be great. Thank you.

Cheers,
~Sergej

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

Re: [RFC PATCH v3 10/10] arm/mem_access: Walk the guest's pt in software

[Sergej Proskurin]

Hi Tamas,

[...]


>> +
>> +        if ( ((flag & GV2M_WRITE) == GV2M_WRITE) && !(perms & GV2M_WRITE) )
> 
> Wouldn't it be enough to do (flag & GV2M_WRITE) without the following
> comparison? Also, a comment explaining why this is an error-condition
> would be nice.
> 

Yes, you are absolutely correct: (flag & GV2M_WRITE) is already
sufficient. I will adapt the upper if-statement and add a comment in the
next version of my patch series.

Cheers,
~Sergej

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

Re: [RFC PATCH v3 07/10] arm/mem_access: Add software guest-page-table walk

[Julien Grall]

On 15/06/17 12:05, Sergej Proskurin wrote:
> The function p2m_mem_access_check_and_get_page in mem_access.c
> translates a gva to an ipa by means of the hardware functionality of the
> ARM architecture. This is implemented in the function gva_to_ipa. If
> mem_access is active, hardware-based gva to ipa translation might fail,
> as gva_to_ipa uses the guest's translation tables, access to which might
> be restricted by the active VTTBR. To address this issue, in this commit
> we add a software-based guest-page-table walk, which will be used by the
> function p2m_mem_access_check_and_get_page perform the gva to ipa
> translation in software in one of the following commits.
>
> Note: The introduced function guest_walk_tables assumes that the domain,
> the gva of which is to be translated, is running on the currently active
> vCPU. To walk the guest's page tables on a different vCPU, the following
> registers would need to be loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and
> SCTLR_EL1.
>
> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
> v2: Rename p2m_gva_to_ipa to p2m_walk_gpt and move it to p2m.c.
>
>     Move the functionality responsible for walking long-descriptor based
>     translation tables out of the function p2m_walk_gpt. Also move out
>     the long-descriptor based translation out of this commit.
>
>     Change function parameters in order to return access access rights
>     to a requested gva.
>
>     Cosmetic fixes.
>
> v3: Rename the introduced functions to guest_walk_(tables|sd|ld) and
>     move the implementation to guest_copy.(c|h).
>
>     Set permissions in guest_walk_tables also if the MMU is disabled.
>
>     Change the function parameter of type "struct p2m_domain *" to
>     "struct vcpu *" in the functions guest_walk_(tables|sd|ld).
> ---
>  xen/arch/arm/Makefile            |  1 +
>  xen/arch/arm/guest_walk.c        | 92 ++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/guest_walk.h | 19 +++++++++
>  3 files changed, 112 insertions(+)
>  create mode 100644 xen/arch/arm/guest_walk.c
>  create mode 100644 xen/include/asm-arm/guest_walk.h
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 49e1fb2f84..282d2c2949 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_HAS_GICV3) += gic-v3.o
>  obj-$(CONFIG_HAS_ITS) += gic-v3-its.o
>  obj-$(CONFIG_HAS_ITS) += gic-v3-lpi.o
>  obj-y += guestcopy.o
> +obj-y += guest_walk.o
>  obj-y += hvm.o
>  obj-y += io.o
>  obj-y += irq.o
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> new file mode 100644
> index 0000000000..f2f3533665
> --- /dev/null
> +++ b/xen/arch/arm/guest_walk.c
> @@ -0,0 +1,92 @@
> +/*
> + * Guest page table walk
> + * Copyright (c) 2017 Sergej Proskurin <proskurin@sec.in.tum.de>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/sched.h>
> +
> +/*
> + * The function guest_walk_sd translates a given GVA into an IPA using the
> + * short-descriptor translation table format in software. This function assumes
> + * that the domain is running on the currently active vCPU. To walk the guest's
> + * 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(struct domain *d,

As I said in v2, it should be a vcpu and not a domain here  because 
page-table are per-vCPU.

> +                         vaddr_t gva, paddr_t *ipa,
> +                         unsigned int *perms)
> +{
> +    /* Not implemented yet. */
> +    return -EFAULT;
> +}
> +
> +/*
> + * The function guest_walk_ld translates a given GVA into an IPA using the
> + * long-descriptor translation table format in software. This function assumes
> + * that the domain is running on the currently active vCPU. To walk the guest's
> + * 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(struct domain *d,
> +                         vaddr_t gva, paddr_t *ipa,
> +                         unsigned int *perms)

Ditto.

> +{
> +    /* Not implemented yet. */
> +    return -EFAULT;
> +}
> +
> +int 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);
> +    struct domain *d = v->domain;
> +    unsigned int _perms = GV2M_READ;
> +
> +    /* We assume that the domain is running on the currently active domain. */
> +    if ( v != current )
> +        return -EFAULT;
> +
> +    /* Allow perms to be NULL. */
> +    perms = perms ?: &_perms;
> +
> +    /* If the MMU is disabled, there is no need to translate the gva. */
> +    if ( !(sctlr & SCTLR_M) )
> +    {
> +        *ipa = gva;
> +
> +        /* Memory can be accessed without any restrictions. */
> +        *perms = GV2M_READ|GV2M_WRITE|GV2M_EXEC;
> +
> +        return 0;
> +    }
> +
> +    if ( is_32bit_domain(d) )
> +    {
> +        if ( !(tcr & TTBCR_EAE) )
> +            return guest_walk_sd(d, gva, ipa, perms);
> +    }
> +
> +    return guest_walk_ld(d, gva, ipa, perms);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-arm/guest_walk.h b/xen/include/asm-arm/guest_walk.h
> new file mode 100644
> index 0000000000..4ed8476e08
> --- /dev/null
> +++ b/xen/include/asm-arm/guest_walk.h
> @@ -0,0 +1,19 @@
> +#ifndef _XEN_GUEST_WALK_H
> +#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);
> +
> +#endif /* _XEN_GUEST_WALK_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
>

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH v3 08/10] arm/mem_access: Add long-descriptor based gpt

[Julien Grall]

Hi Sergej,

On 15/06/17 12:05, Sergej Proskurin wrote:
> This commit adds functionality to walk the guest's page tables using the
> long-descriptor translation table format for both ARMv7 and ARMv8.
> Similar to the hardware architecture, the implementation supports
> different page granularities (4K, 16K, and 64K). The implementation is
> based on ARM DDI 0487B-a J1-5922, J1-5999, and ARM DDI 0406C-b B3-1510.

NIT: The spec is 0487B.a and not 0487B-a.

>
> Note that the current implementation lacks support for Large VA/PA on
> ARMv8.2 architectures (LVA/LPA, 52-bit virtual and physical address
> sizes). The associated location in the code is marked appropriately.

I am a bit confused, you are stating the 52-bit is not supported, but 
you seem to implement it below.

I don't want to see a feature half supported as this means rotten code. 
Given that 52-bit support in Xen is not implemented (and would require 
some work), 52-bit guest cannot be supported. So I would prefer that to 
see no code at all. Though I would keep the comment in the commit message.

>
> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
> v2: Use TCR_SZ_MASK instead of TTBCR_SZ_MASK for ARM 32-bit guests using
>     the long-descriptor translation table format.
>
>     Cosmetic fixes.
>
> v3: Move the implementation to ./xen/arch/arm/guest_copy.c.
>
>     Remove the array strides and declare the array grainsizes as static
>     const instead of just const to reduce the function stack overhead.
>
>     Move parts of the funtion guest_walk_ld into the static functions
>     get_ttbr_and_gran_64bit and get_top_bit to reduce complexity.
>
>     Use the macro BIT(x) instead of (1UL << x).
>
>     Add more comments && Cosmetic fixes.
> ---
>  xen/arch/arm/guest_walk.c        | 397 ++++++++++++++++++++++++++++++++++++++-
>  xen/include/asm-arm/guest_walk.h |   9 +
>  2 files changed, 404 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index f2f3533665..90bcc218ec 100644
> --- a/xen/arch/arm/guest_walk.c
> +++ b/xen/arch/arm/guest_walk.c
> @@ -16,6 +16,8 @@
>   */
>
>  #include <xen/sched.h>
> +#include <xen/domain_page.h>
> +#include <asm/guest_walk.h>
>
>  /*
>   * The function guest_walk_sd translates a given GVA into an IPA using the
> @@ -32,6 +34,92 @@ static int guest_walk_sd(struct domain *d,
>      return -EFAULT;
>  }
>
> +#ifdef CONFIG_ARM_64

It is fairly confusing. Sometimes you #ifdef the AArch64 code, sometimes 
not (see below the if is_64bit_domain below). Can you stay consistent 
across the file please.

IHMO, we should really limit the #ifdef CONFIG_ARM_64. This makes more 
difficult to follow the code and has limited benefits if all definitions 
also exist for AArch32 too.

> +/*
> + * Select the TTBR(0|1)_EL1 that will be used for address translation using the
> + * long-descriptor translation table format and return the page granularity
> + * that is used by the selected TTBR.
> + */
> +static bool get_ttbr_and_gran_64bit(uint64_t *ttbr, unsigned int *gran,
> +                                    register_t tcr, bool ttbrx)
> +{
> +    bool disabled;
> +
> +    if ( ttbrx == TTBR0_VALID )
> +    {
> +        /* Normalize granule size. */
> +        switch ( tcr & TCR_TG0_MASK )
> +        {
> +        case TCR_TG0_16K:
> +            *gran = GRANULE_SIZE_INDEX_16K;
> +            break;
> +        case TCR_TG0_64K:
> +            *gran = GRANULE_SIZE_INDEX_64K;
> +            break;
> +        default:
> +            *gran = GRANULE_SIZE_INDEX_4K;
> +        }
> +
> +        /* Use TTBR0 for GVA to IPA translation. */
> +        *ttbr = READ_SYSREG64(TTBR0_EL1);
> +
> +        /* If TCR.EPD0 is set, translations using TTBR0 are disabled. */
> +        disabled = tcr & TCR_EPD0;
> +    }
> +    else
> +    {
> +        /* Normalize granule size. */
> +        switch ( tcr & TCR_EL1_TG1_MASK )
> +        {
> +        case TCR_EL1_TG1_16K:
> +            *gran = GRANULE_SIZE_INDEX_16K;
> +            break;
> +        case TCR_EL1_TG1_64K:
> +            *gran = GRANULE_SIZE_INDEX_64K;
> +            break;
> +        default:
> +            *gran = GRANULE_SIZE_INDEX_4K;
> +        }
> +
> +        /* Use TTBR1 for GVA to IPA translation. */
> +        *ttbr = READ_SYSREG64(TTBR1_EL1);
> +
> +        /* If TCR.EPD1 is set, translations using TTBR1 are disabled. */
> +        disabled = tcr & TCR_EPD1;
> +    }
> +
> +    return disabled;
> +}
> +#endif
> +
> +/*
> + * Get the MSB number of the GVA, according to "AddrTop" pseudocode
> + * implementation in ARM DDI 0487B-a J1-6066.

NIT: The spec is 0487B.a and not 0487B-a.

> + */
> +static unsigned int get_top_bit(struct domain *d, vaddr_t gva, register_t tcr)
> +{
> +    unsigned int topbit;
> +
> +    /*
> +     * IF EL1 is using AArch64 then addresses from EL0 using AArch32 are
> +     * zero-extended to 64 bits (ARM DDI 0487B-a J1-6066).
> +     */
> +    if ( is_32bit_domain(d) )
> +        topbit = 31;
> +#ifdef CONFIG_ARM_64
> +    else if ( is_64bit_domain(d) )
> +    {
> +        if ( ((gva & BIT(55)) && (tcr & TCR_EL1_TBI1)) ||
> +             (!(gva & BIT(55)) && (tcr & TCR_EL1_TBI0)) )
> +            topbit = 55;
> +        else
> +            topbit = 63;
> +    }
> +#endif
> +
> +    return topbit;
> +}
> +
>  /*
>   * The function guest_walk_ld translates a given GVA into an IPA using the
>   * long-descriptor translation table format in software. This function assumes
> @@ -43,8 +131,313 @@ static int guest_walk_ld(struct domain *d,
>                           vaddr_t gva, paddr_t *ipa,
>                           unsigned int *perms)
>  {
> -    /* Not implemented yet. */
> -    return -EFAULT;
> +    bool disabled = true;
> +    bool ro_table = false, xn_table = false;
> +    unsigned int t0_sz, t1_sz;
> +    unsigned int level, gran;
> +    unsigned int topbit = 0, input_size = 0, output_size;
> +    uint64_t ttbr = 0, ips;
> +    paddr_t mask;
> +    lpae_t pte, *table;
> +    struct page_info *page;
> +    register_t tcr = READ_SYSREG(TCR_EL1);
> +
> +    const vaddr_t offsets[4][3] = {
> +        {
> +#ifdef CONFIG_ARM_64
> +            zeroeth_guest_table_offset_4k(gva),
> +            zeroeth_guest_table_offset_16k(gva),
> +            0, /* There is no zeroeth lookup level with a 64K granule size. */
> +#endif
> +        },
> +        {
> +            first_guest_table_offset_4k(gva),
> +#ifdef CONFIG_ARM_64
> +            first_guest_table_offset_16k(gva),
> +            first_guest_table_offset_64k(gva),
> +#endif
> +        },
> +        {
> +            second_guest_table_offset_4k(gva),
> +#ifdef CONFIG_ARM_64
> +            second_guest_table_offset_16k(gva),
> +            second_guest_table_offset_64k(gva),
> +#endif
> +        },
> +        {
> +            third_guest_table_offset_4k(gva),
> +#ifdef CONFIG_ARM_64
> +            third_guest_table_offset_16k(gva),
> +            third_guest_table_offset_64k(gva),
> +#endif
> +        }
> +    };
> +
> +    static const paddr_t masks[4][3] = {
> +        {
> +            zeroeth_size(4K) - 1,
> +            zeroeth_size(16K) - 1,
> +            0 /* There is no zeroeth lookup level with a 64K granule size. */
> +        },
> +        {
> +            first_size(4K) - 1,
> +            first_size(16K) - 1,
> +            first_size(64K) - 1
> +        },
> +        {
> +            second_size(4K) - 1,
> +            second_size(16K) - 1,
> +            second_size(64K) - 1
> +        },
> +        {
> +            third_size(4K) - 1,
> +            third_size(16K) - 1,
> +            third_size(64K) - 1
> +        }
> +    };
> +
> +    static const unsigned int grainsizes[3] = {
> +        PAGE_SHIFT_4K,
> +        PAGE_SHIFT_16K,
> +        PAGE_SHIFT_64K
> +    };
> +
> +    t0_sz = (tcr >> TCR_T0SZ_SHIFT) & TCR_SZ_MASK;
> +    t1_sz = (tcr >> TCR_T1SZ_SHIFT) & TCR_SZ_MASK;
> +
> +    /* Get the MSB number of the GVA. */
> +    topbit = get_top_bit(d, gva, tcr);
> +
> +#ifdef CONFIG_ARM_64
> +    if ( is_64bit_domain(d) )
> +    {
> +        /* Select the TTBR(0|1)_EL1 that will be used for address translation. */
> +
> +        if ( (gva & BIT(topbit)) == 0 )
> +        {
> +            input_size = REGISTER_WIDTH_64_BIT - t0_sz;
> +
> +            /* Get TTBR0 and configured page granularity. */
> +            disabled = get_ttbr_and_gran_64bit(&ttbr, &gran, tcr, TTBR0_VALID);
> +        }
> +        else
> +        {
> +            input_size = REGISTER_WIDTH_64_BIT - t1_sz;
> +
> +            /* Get TTBR1 and configured page granularity. */
> +            disabled = get_ttbr_and_gran_64bit(&ttbr, &gran, tcr, TTBR1_VALID);
> +        }
> +
> +        /*
> +         * The current implementation supports intermediate physical address
> +         * sizes (IPS) up to 48 bit.
> +         *
> +         * XXX: Determine whether the IPS_MAX_VAL is 48 or 52 in software.
> +         */
> +        if ( (input_size > TCR_EL1_IPS_48_BIT_VAL) ||
> +             (input_size < TCR_EL1_IPS_MIN_VAL) )
> +            return -EFAULT;
> +    }
> +    else
> +#endif
> +    {
> +        /* Granule size of AArch32 architectures is always 4K. */
> +        gran = GRANULE_SIZE_INDEX_4K;
> +
> +        /* Select the TTBR(0|1)_EL1 that will be used for address translation. */
> +
> +        /*
> +         * Check if the bits <31:32-t0_sz> of the GVA are set to 0 (DDI 0487B-a
> +         * J1-5999). If so, TTBR0 shall be used for address translation.
> +         */
> +        mask = ((1ULL << REGISTER_WIDTH_32_BIT) - 1) &
> +               ~((1ULL << (REGISTER_WIDTH_32_BIT - t0_sz)) - 1);
> +
> +        if ( t0_sz == 0 || !(gva & mask) )
> +        {
> +            input_size = REGISTER_WIDTH_32_BIT - t0_sz;
> +
> +            /* Use TTBR0 for GVA to IPA translation. */
> +            ttbr = READ_SYSREG64(TTBR0_EL1);
> +
> +            /* If TCR.EPD0 is set, translations using TTBR0 are disabled. */
> +            disabled = tcr & TCR_EPD0;
> +        }
> +
> +        /*
> +         * Check if the bits <31:32-t1_sz> of the GVA are set to 1 (DDI 0487B-a
> +         * J1-6000). If so, TTBR1 shall be used for address translation.
> +         */
> +        mask = ((1ULL << REGISTER_WIDTH_32_BIT) - 1) &
> +               ~((1ULL << (REGISTER_WIDTH_32_BIT - t1_sz)) - 1);
> +
> +        if ( ((t1_sz == 0) && !ttbr) || (t1_sz && (gva & mask) == mask) )
> +        {
> +            input_size = REGISTER_WIDTH_32_BIT - t1_sz;
> +
> +            /* Use TTBR1 for GVA to IPA translation. */
> +            ttbr = READ_SYSREG64(TTBR1_EL1);
> +
> +            /* If TCR.EPD1 is set, translations using TTBR1 are disabled. */
> +            disabled = tcr & TCR_EPD1;
> +        }
> +    }
> +
> +    if ( disabled )
> +        return -EFAULT;
> +
> +    /*
> +     * The starting level is the number of strides (grainsizes[gran] - 3)
> +     * needed to consume the input address (DDI 0487B-a J1-5924).
> +     */
> +    level = 4 - DIV_ROUND_UP((input_size - grainsizes[gran]), (grainsizes[gran] - 3));
> +
> +    if ( is_64bit_domain(d) )

Here an example where you don't use #ifdef.

> +    {
> +        /* Get the intermediate physical address size. */
> +        ips = (tcr & TCR_EL1_IPS_MASK) >> TCR_EL1_IPS_SHIFT;
> +
> +        switch (ips)
> +        {
> +        case TCR_EL1_IPS_32_BIT:
> +            output_size = TCR_EL1_IPS_32_BIT_VAL;
> +            break;
> +        case TCR_EL1_IPS_36_BIT:
> +            output_size = TCR_EL1_IPS_36_BIT_VAL;
> +            break;
> +        case TCR_EL1_IPS_40_BIT:
> +            output_size = TCR_EL1_IPS_40_BIT_VAL;
> +            break;
> +        case TCR_EL1_IPS_42_BIT:
> +            output_size = TCR_EL1_IPS_42_BIT_VAL;
> +            break;
> +        case TCR_EL1_IPS_44_BIT:
> +            output_size = TCR_EL1_IPS_44_BIT_VAL;
> +            break;
> +        case TCR_EL1_IPS_48_BIT:
> +            output_size = TCR_EL1_IPS_48_BIT_VAL;
> +            break;
> +        case TCR_EL1_IPS_52_BIT:
> +            /* XXX: 52 bit output_size is not supported yet. */

If you bail out here, you should also bail out for all reserved output size.

> +            return -EFAULT;
> +        default:
> +            output_size = TCR_EL1_IPS_48_BIT_VAL;
> +        }

Looking at this. Why not using an array to store all the values? This 
would avoid a repetitive switch here.

> +    }
> +    else
> +        output_size = TCR_EL1_IPS_40_BIT_VAL;
> +
> +    /* Make sure the base address does not exceed its configured size. */
> +    mask = ((1ULL << TCR_EL1_IPS_48_BIT_VAL) - 1) & ~((1ULL << output_size) - 1);
> +    if ( output_size < TCR_EL1_IPS_48_BIT_VAL && (ttbr & mask) )
> +        return -EFAULT;
> +
> +    mask = ((1ULL << output_size) - 1);
> +    page = get_page_from_gfn(d, paddr_to_pfn(ttbr & mask), NULL, P2M_ALLOC);
> +    if ( !page )
> +        return -EFAULT;
> +
> +    table = __map_domain_page(page);
> +
> +    for ( ; ; level++ )
> +    {
> +        pte = table[offsets[level][gran]];
> +
> +        unmap_domain_page(table);
> +        put_page(page);
> +
> +        /* Make sure the base address does not exceed its configured size. */
> +        mask = ((1ULL << TCR_EL1_IPS_48_BIT_VAL) - 1) &
> +               ~((1ULL << output_size) - 1);
> +
> +        if ( (output_size < TCR_EL1_IPS_48_BIT_VAL) &&
> +             (pfn_to_paddr(pte.walk.base) & mask) )
> +            return -EFAULT;
> +
> +#ifdef CONFIG_ARM_64
> +        /*
> +         * If page granularity is 64K, make sure the address is aligned
> +         * appropriately.
> +         */
> +        if ( (output_size < TCR_EL1_IPS_52_BIT_VAL) &&
> +             (gran == GRANULE_SIZE_INDEX_64K) &&
> +             (pte.walk.base & 0xf) )
> +            return -EFAULT;
> +#endif
> +
> +        /*
> +         * Break if one of the following conditions are true:
> +         *
> +         * - We have found the PTE holding the IPA (level == 3).
> +         * - The PTE is not valid.
> +         * - If (level < 3) and the PTE is valid, we found a block descriptor.
> +         */
> +        if ( level == 3 || !p2m_valid(pte) || !p2m_table(pte) )

Please have a look at lpae_is_superpage(...).

> +            break;
> +
> +        /*
> +         * Temporarily store permissions of the table descriptor as they are
> +         * inherited by page table attributes (ARM DDI 0487B-a J1-5928).
> +         */
> +        xn_table = xn_table | !!(pte.pt.xnt);          /* Execute-Never */

You can simplify with.

xn_table |= !!(pte.pt_xnt);

Potentially you can even drop the !!.


> +        ro_table = ro_table | !!(pte.pt.apt & BIT(1)); /* Read-Only */

Ditto.

> +
> +#ifdef CONFIG_ARM_64
> +        if ( output_size == TCR_EL1_IPS_52_BIT_VAL )
> +        {
> +            unsigned long gfn;
> +
> +            /*
> +             * The GFN must be rearranged according to the following format of
> +             * the PTE bits [pte<15:12>:pte<47:16>:0000].
> +             */
> +            gfn = ((unsigned long)(pte.walk.base & 0xf) << 36) |
> +                  (pte.walk.base & ~0xf);
> +
> +            page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
> +        }
> +        else
> +#endif
> +            page = get_page_from_gfn(d, pte.walk.base, NULL, P2M_ALLOC);
> +
> +        if ( !page )
> +            return -EFAULT;
> +
> +        table = __map_domain_page(page);
> +    }
> +
> +    if ( !p2m_valid(pte) || ((level == 3) && !p2m_table(pte)) )

Please explain at least the second part of the || in comment.

> +        return -EFAULT;
> +
> +#ifdef CONFIG_ARM_64
> +    if ( output_size == TCR_EL1_IPS_52_BIT_VAL )
> +    {
> +        unsigned long gfn;
> +
> +        /*
> +         * The GFN must be rearranged according to the following format of the
> +         * PTE bits [pte<15:12>:pte<47:16>:0000].
> +         */
> +        gfn = ((unsigned long)(pte.walk.base & 0xf) << 36) |
> +              (pte.walk.base & ~0xf);
> +
> +        *ipa = pfn_to_paddr(gfn) | (gva & masks[level][gran]);
> +    }
> +    else
> +#endif
> +        *ipa = pfn_to_paddr(pte.walk.base) | (gva & masks[level][gran]);
> +
> +    /*
> +     * Set permissions so that the caller can check the flags by herself. Note
> +     * that stage 1 translations also inherit attributes from the tables
> +     * (ARM DDI 0487B-a J1-5928).
> +     */
> +    if ( !pte.pt.ro && !ro_table )
> +        *perms = GV2M_WRITE;
> +    if ( !pte.pt.xn && !xn_table )
> +        *perms |= GV2M_EXEC;
> +
> +    return 0;
>  }
>
>  int guest_walk_tables(const struct vcpu *v, vaddr_t gva,
> diff --git a/xen/include/asm-arm/guest_walk.h b/xen/include/asm-arm/guest_walk.h
> index 4ed8476e08..ce01f0fa08 100644
> --- a/xen/include/asm-arm/guest_walk.h
> +++ b/xen/include/asm-arm/guest_walk.h
> @@ -1,6 +1,15 @@
>  #ifndef _XEN_GUEST_WALK_H
>  #define _XEN_GUEST_WALK_H
>
> +/* Normalized page granule size indices. */
> +#define GRANULE_SIZE_INDEX_4K               (0)
> +#define GRANULE_SIZE_INDEX_16K              (1)
> +#define GRANULE_SIZE_INDEX_64K              (2)

Why this is exported? You only use them internally.

> +
> +/* Represent whether TTBR0 or TTBR1 is valid. */
> +#define TTBR0_VALID                         (0)
> +#define TTBR1_VALID                         (1)

Ditto.

> +
>  /* Walk the guest's page tables in software. */
>  int guest_walk_tables(const struct vcpu *v,
>                        vaddr_t gva,
>

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH v3 08/10] arm/mem_access: Add long-descriptor based gpt

[Sergej Proskurin]

Hi Julien,

[...]

On 06/19/2017 02:45 PM, Julien Grall wrote:
> Hi Sergej,
>
>> +/* Normalized page granule size indices. */
>> +#define GRANULE_SIZE_INDEX_4K               (0)
>> +#define GRANULE_SIZE_INDEX_16K              (1)
>> +#define GRANULE_SIZE_INDEX_64K              (2)
>
> Why this is exported? You only use them internally.
>

In v2 you pointed out that it is hard to read hardcoded values. I would
gladly change these changes back as I agree on this point: I also don't
like having exported indices, as well. Or would you like to see these
defines (and the TTBRx_VALID values beyond) directly within the
guest_walk.c?

>> +
>> +/* Represent whether TTBR0 or TTBR1 is valid. */
>> +#define TTBR0_VALID                         (0)
>> +#define TTBR1_VALID                         (1)
>
> Ditto.

Cheers,
~Sergej


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

Re: [RFC PATCH v3 08/10] arm/mem_access: Add long-descriptor based gpt

[Julien Grall]

On 19/06/17 16:43, Sergej Proskurin wrote:
> Hi Julien,
>
> [...]
>
> On 06/19/2017 02:45 PM, Julien Grall wrote:
>> Hi Sergej,
>>
>>> +/* Normalized page granule size indices. */
>>> +#define GRANULE_SIZE_INDEX_4K               (0)
>>> +#define GRANULE_SIZE_INDEX_16K              (1)
>>> +#define GRANULE_SIZE_INDEX_64K              (2)
>>
>> Why this is exported? You only use them internally.
>>
>
> In v2 you pointed out that it is hard to read hardcoded values. I would
> gladly change these changes back as I agree on this point: I also don't
> like having exported indices, as well. Or would you like to see these
> defines (and the TTBRx_VALID values beyond) directly within the
> guest_walk.c?

The latter please + probably using enum rather than define to make them 
safe. But...

>
>>> +
>>> +/* Represent whether TTBR0 or TTBR1 is valid. */
>>> +#define TTBR0_VALID                         (0)
>>> +#define TTBR1_VALID                         (1)

look at that again. You are using bool as the type. So they should be 
true/false. Or if it is an index, they type should be int. Though, the 
enum would make it clearer.

Cheers,

-- 
Julien Grall

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