[PATCH v4 0/9] arm/mem_access: Walk guest page tables in SW if mem_access is active

[PATCH v4 0/9] 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 changes the introduced data-structures and defines to simplify code.
Please note that this patch series is based on Julien Grall's patch series
"xen/arm: Move LPAE definition in a separate header"[1].

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

Cheers,
~Sergej

[0] https://github.com/sergej-proskurin/xen (branch arm-gpt-walk-v4)
[1] https://lists.xen.org/archives/html/xen-devel/2017-06/msg02095.html

Sergej Proskurin (9):
  arm/mem_access: Add (TCR_|TTBCR_)* defines
  arm/mem_access: Add defines supporting PTs with varying page sizes
  arm/mem_access: Add short-descriptor pte typedefs
  arm/mem_access: Introduce GV2M_EXEC permission
  arm/mem_access: Extend BIT-operations to unsigned long long
  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        | 635 +++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/mem_access.c        |  31 +-
 xen/include/asm-arm/bitops.h     |   3 +-
 xen/include/asm-arm/guest_walk.h |  35 +++
 xen/include/asm-arm/lpae.h       |  67 +++++
 xen/include/asm-arm/page.h       |   1 +
 xen/include/asm-arm/processor.h  |  69 ++++-
 xen/include/asm-arm/short-desc.h | 108 +++++++
 9 files changed, 944 insertions(+), 6 deletions(-)
 create mode 100644 xen/arch/arm/guest_walk.c
 create mode 100644 xen/include/asm-arm/guest_walk.h
 create mode 100644 xen/include/asm-arm/short-desc.h

-- 
2.12.2


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

[PATCH v4 1/9] 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.

v4: Cosmetic changes.
---
 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..863a569432 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,9 +190,50 @@
 #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)
+
+/* Note that the fields TCR_EL2.TBI(0|1) exist only if HCR_EL2.E2H==1. */
+#define TCR_EL1_TBI0    (_AC(0x1,ULL)<<37)
+#define TCR_EL1_TBI1    (_AC(0x1,ULL)<<38)
 
 #ifdef CONFIG_ARM_64
 
-- 
2.12.2


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

[PATCH v4 2/9] 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.

v4: Replace existing macros with ones that generate static inline
    helpers as to ease the readability of the code.

    Move the introduced code into lpae.h
---
 xen/include/asm-arm/lpae.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
index 6fbf7c606c..2913428e96 100644
--- a/xen/include/asm-arm/lpae.h
+++ b/xen/include/asm-arm/lpae.h
@@ -151,6 +151,73 @@ static inline bool lpae_is_superpage(lpae_t pte, unsigned int level)
     return (level < 3) && lpae_mapping(pte);
 }
 
+/*
+ * The ARMv8 architecture supports pages with different sizes (4K, 16K, and
+ * 64K). To enable guest page table walks for various configurations, the
+ * following helpers enable walking the guest's translation table with varying
+ * page size granularities.
+ */
+
+#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(gva, gran)     GUEST_TABLE_OFFSET((gva >> third_shift(gran)), gran)
+#define second_guest_table_offset(gva, gran)    GUEST_TABLE_OFFSET((gva >> second_shift(gran)), gran)
+#define first_guest_table_offset(gva, gran)     GUEST_TABLE_OFFSET((gva >> first_shift(gran)), gran)
+#define zeroeth_guest_table_offset(gva, gran)   GUEST_TABLE_OFFSET((gva >> zeroeth_shift(gran)), gran)
+
+#define GUEST_TABLE_OFFSET_HELPERS(gran)                                \
+static inline vaddr_t third_guest_table_offset_##gran##K(vaddr_t gva)   \
+{                                                                       \
+    return third_guest_table_offset(gva, gran##K);                      \
+}                                                                       \
+                                                                        \
+static inline vaddr_t second_guest_table_offset_##gran##K(vaddr_t gva)  \
+{                                                                       \
+    return second_guest_table_offset(gva, gran##K);                     \
+}                                                                       \
+                                                                        \
+static inline vaddr_t first_guest_table_offset_##gran##K(vaddr_t gva)   \
+{                                                                       \
+    return first_guest_table_offset(gva, gran##K);                      \
+}                                                                       \
+                                                                        \
+static inline vaddr_t zeroeth_guest_table_offset_##gran##K(vaddr_t gva) \
+{                                                                       \
+    if ( gran == 64 )                                                   \
+        return 0;                                                       \
+    else                                                                \
+        return zeroeth_guest_table_offset((paddr_t)gva, gran##K);       \
+}                                                                       \
+
+GUEST_TABLE_OFFSET_HELPERS(4);
+#ifdef CONFIG_ARM_64
+GUEST_TABLE_OFFSET_HELPERS(16);
+GUEST_TABLE_OFFSET_HELPERS(64);
+#endif
+
 #endif /* __ASSEMBLY__ */
 
 /*
-- 
2.12.2


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

[PATCH v4 3/9] 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.

v4: Move short-descriptor pte typedefs out of page.h into short-desc.h.

    Change the type unsigned int to bool of every bitfield in
    short-descriptor related data-structures that holds only one bit.

    Change the typedef names from pte_sd_* to short_desc_*.
---
 xen/include/asm-arm/short-desc.h | 108 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 108 insertions(+)
 create mode 100644 xen/include/asm-arm/short-desc.h

diff --git a/xen/include/asm-arm/short-desc.h b/xen/include/asm-arm/short-desc.h
new file mode 100644
index 0000000000..6e2ab09d24
--- /dev/null
+++ b/xen/include/asm-arm/short-desc.h
@@ -0,0 +1,108 @@
+#ifndef __ARM_SHORT_DESC_H__
+#define __ARM_SHORT_DESC_H__
+
+/*
+ * Comprises bits of the level 1 short-descriptor format representing
+ * a section.
+ */
+typedef struct __packed {
+    bool pxn:1;                 /* Privileged Execute Never */
+    bool sec:1;                 /* == 1 if section or supersection */
+    bool b:1;                   /* Bufferable */
+    bool c:1;                   /* Cacheable */
+    bool xn:1;                  /* Execute Never */
+    unsigned int dom:4;         /* Domain field */
+    bool impl:1;                /* Implementation defined */
+    unsigned int ap:2;          /* AP[1:0] */
+    unsigned int tex:3;         /* TEX[2:0] */
+    bool ro:1;                  /* AP[2] */
+    bool s:1;                   /* Shareable */
+    bool ng:1;                  /* Non-global */
+    bool supersec:1;            /* Must be 0 for sections */
+    bool ns:1;                  /* Non-secure */
+    unsigned int base:12;       /* Section base address */
+} short_desc_l1_sec_t;
+
+/*
+ * Comprises bits of the level 1 short-descriptor format representing
+ * a supersection.
+ */
+typedef struct __packed {
+    bool pxn:1;                 /* Privileged Execute Never */
+    bool sec:1;                 /* == 1 if section or supersection */
+    bool b:1;                   /* Bufferable */
+    bool c:1;                   /* Cacheable */
+    bool xn:1;                  /* Execute Never */
+    unsigned int extbase2:4;    /* Extended base address, PA[39:36] */
+    bool impl:1;                /* Implementation defined */
+    unsigned int ap:2;          /* AP[1:0] */
+    unsigned int tex:3;         /* TEX[2:0] */
+    bool ro:1;                  /* AP[2] */
+    bool s:1;                   /* Shareable */
+    bool ng:1;                  /* Non-global */
+    bool supersec:1;            /* Must be 0 for sections */
+    bool ns:1;                  /* Non-secure */
+    unsigned int extbase1:4;    /* Extended base address, PA[35:32] */
+    unsigned int base:8;        /* Supersection base address */
+} short_desc_l1_supersec_t;
+
+/*
+ * Comprises bits of the level 2 short-descriptor format representing
+ * a small page.
+ */
+typedef struct __packed {
+    bool xn:1;                  /* Execute Never */
+    bool page:1;                /* ==1 if small page */
+    bool b:1;                   /* Bufferable */
+    bool c:1;                   /* Cacheable */
+    unsigned int ap:2;          /* AP[1:0] */
+    unsigned int tex:3;         /* TEX[2:0] */
+    bool ro:1;                  /* AP[2] */
+    bool s:1;                   /* Shareable */
+    bool ng:1;                  /* Non-global */
+    unsigned int base:20;       /* Small page base address */
+} short_desc_l2_page_t;
+
+/*
+ * Comprises bits of the level 2 short-descriptor format representing
+ * a large page.
+ */
+typedef struct __packed {
+    bool lpage:1;               /* ==1 if large page */
+    bool page:1;                /* ==0 if large page */
+    bool b:1;                   /* Bufferable */
+    bool c:1;                   /* Cacheable */
+    unsigned int ap:2;          /* AP[1:0] */
+    unsigned int sbz:3;         /* Should be zero */
+    bool ro:1;                  /* AP[2] */
+    bool s:1;                   /* Shareable */
+    bool ng:1;                  /* Non-global */
+    unsigned int tex:3;         /* TEX[2:0] */
+    bool xn:1;                  /* Execute Never */
+    unsigned int base:16;       /* Large page base address */
+} short_desc_l2_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 */
+} short_desc_walk_t;
+
+/*
+ * Represents page table entries adhering to the short-descriptor translation
+ * table format.
+ */
+typedef union {
+    uint32_t bits;
+    short_desc_walk_t walk;
+    short_desc_l1_sec_t sec;
+    short_desc_l1_supersec_t supersec;
+    short_desc_l2_page_t pg;
+    short_desc_l2_lpage_t lpg;
+} short_desc_t;
+
+#endif /* __ARM_SHORT_DESC_H__ */
-- 
2.12.2


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

[PATCH v4 4/9] 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>
Acked-by: Julien Grall <julien.grall@arm.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
---
 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 cef2f28914..b8d641bfaf 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -90,6 +90,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

[PATCH v4 5/9] arm/mem_access: Extend BIT-operations to unsigned long long

[Sergej Proskurin]

We extend the BIT macro to using values of unsigned long long as to
enable setting bits of 64-bit registers on AArch32.  In addition, this
commit adds a define holding the register width of 64 bit double-word
registers. This define simplifies 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>
---
v4: We reused the previous commit with the msg "arm/mem_access: Add
    defines holding the width of 32/64bit regs" from v3, as we can reuse
    the already existing define BITS_PER_WORD.
---
 xen/include/asm-arm/bitops.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/include/asm-arm/bitops.h b/xen/include/asm-arm/bitops.h
index bda889841b..c3486d497c 100644
--- a/xen/include/asm-arm/bitops.h
+++ b/xen/include/asm-arm/bitops.h
@@ -21,7 +21,8 @@
 #define __clear_bit(n,p)          clear_bit(n,p)
 
 #define BITS_PER_WORD           32
-#define BIT(nr)                 (1UL << (nr))
+#define BITS_PER_DOUBLE_WORD    64
+#define BIT(nr)                 (1ULL << (nr))
 #define BIT_MASK(nr)            (1UL << ((nr) % BITS_PER_WORD))
 #define BIT_WORD(nr)            ((nr) / BITS_PER_WORD)
 #define BITS_PER_BYTE           8
-- 
2.12.2


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

[PATCH v4 6/9] 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 function guest_walk_tables.

v4: Change the function parameter of type "struct p2m_domain *" to
    "struct vcpu *" in the functions guest_walk_(sd|ld) as well.
---
 xen/arch/arm/Makefile            |  1 +
 xen/arch/arm/guest_walk.c        | 91 ++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/guest_walk.h | 19 +++++++++
 3 files changed, 111 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..b8bb553a6e
--- /dev/null
+++ b/xen/arch/arm/guest_walk.c
@@ -0,0 +1,91 @@
+/*
+ * 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(const struct vcpu *v,
+                         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(const struct vcpu *v,
+                         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);
+    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(v->domain) )
+    {
+        if ( !(tcr & TTBCR_EAE) )
+            return guest_walk_sd(v, gva, ipa, perms);
+    }
+
+    return guest_walk_ld(v, 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

[PATCH v4 7/9] 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.

v4: Move functionality responsible for determining the configured IPA
    output-size into a separate function get_ipa_output_size. In this
    function, we remove the previously used switch statement, which was
    responsible for distinguishing between different IPA output-sizes.
    Instead, we retrieve the information from the introduced ipa_sizes
    array.

    Remove the defines GRANULE_SIZE_INDEX_* and TTBR0_VALID from
    guest_walk.h. Instead, introduce the enums granule_size_index
    active_ttbr directly inside of guest_walk.c so that the associated
    fields don't get exported.

    Adapt the function to the new parameter of type "struct vcpu *".

    Remove support for 52bit IPA output-sizes entirely from this commit.

    Use lpae_* helpers instead of p2m_* helpers.

    Cosmetic fixes & Additional comments.
---
 xen/arch/arm/guest_walk.c | 385 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 383 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index b8bb553a6e..c37c595157 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
@@ -33,6 +35,142 @@ static int guest_walk_sd(const struct vcpu *v,
 }
 
 /*
+ * Get the IPA output_size (configured in TCR_EL1) that shall be used for the
+ * long-descriptor based translation table walk.
+ */
+static unsigned int get_ipa_output_size(struct domain *d, register_t tcr)
+{
+    unsigned int output_size;
+    uint64_t ips;
+
+    static const uint64_t ipa_sizes[7] = {
+        TCR_EL1_IPS_32_BIT_VAL,
+        TCR_EL1_IPS_36_BIT_VAL,
+        TCR_EL1_IPS_40_BIT_VAL,
+        TCR_EL1_IPS_42_BIT_VAL,
+        TCR_EL1_IPS_44_BIT_VAL,
+        TCR_EL1_IPS_48_BIT_VAL,
+        TCR_EL1_IPS_52_BIT_VAL
+    };
+
+    if ( is_64bit_domain(d) )
+    {
+        /* Get the intermediate physical address size. */
+        ips = (tcr & TCR_EL1_IPS_MASK) >> TCR_EL1_IPS_SHIFT;
+
+        /*
+         * Return an error on reserved IPA output-sizes and if the IPA
+         * output-size is 52bit.
+         *
+         * XXX: 52 bit output_size is not supported yet.
+         */
+        if ( ips > TCR_EL1_IPS_48_BIT )
+            return -EFAULT;
+
+        output_size = ipa_sizes[ips];
+    }
+    else
+        output_size = TCR_EL1_IPS_40_BIT_VAL;
+
+    return output_size;
+}
+
+/* Normalized page granule size indices. */
+enum granule_size_index {
+    GRANULE_SIZE_INDEX_4K,
+    GRANULE_SIZE_INDEX_16K,
+    GRANULE_SIZE_INDEX_64K
+};
+
+/* Represent whether TTBR0 or TTBR1 is active. */
+enum active_ttbr {
+    TTBR0_ACTIVE,
+    TTBR1_ACTIVE
+};
+
+/*
+ * 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, enum active_ttbr ttbrx)
+{
+    bool disabled;
+
+    if ( ttbrx == TTBR0_ACTIVE )
+    {
+        /* 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;
+}
+
+/*
+ * 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;
+    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;
+    }
+
+    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
  * that the domain is running on the currently active vCPU. To walk the guest's
@@ -43,8 +181,251 @@ static int guest_walk_ld(const struct vcpu *v,
                          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;
+    paddr_t mask;
+    lpae_t pte, *table;
+    struct page_info *page;
+    register_t tcr = READ_SYSREG(TCR_EL1);
+    struct domain *d = v->domain;
+
+    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);
+
+    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 = BITS_PER_DOUBLE_WORD - t0_sz;
+
+            /* Get TTBR0 and configured page granularity. */
+            disabled = get_ttbr_and_gran_64bit(&ttbr, &gran, tcr, TTBR0_ACTIVE);
+        }
+        else
+        {
+            input_size = BITS_PER_DOUBLE_WORD - t1_sz;
+
+            /* Get TTBR1 and configured page granularity. */
+            disabled = get_ttbr_and_gran_64bit(&ttbr, &gran, tcr, TTBR1_ACTIVE);
+        }
+
+        /*
+         * 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
+    {
+        /* 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 << BITS_PER_WORD) - 1) &
+               ~((1ULL << (BITS_PER_WORD - t0_sz)) - 1);
+
+        if ( t0_sz == 0 || !(gva & mask) )
+        {
+            input_size = BITS_PER_WORD - 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 << BITS_PER_WORD) - 1) &
+               ~((1ULL << (BITS_PER_WORD - t1_sz)) - 1);
+
+        if ( ((t1_sz == 0) && !ttbr) || (t1_sz && (gva & mask) == mask) )
+        {
+            input_size = BITS_PER_WORD - 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));
+
+    /* Get the IPA output_size. */
+    output_size = get_ipa_output_size(d, tcr);
+
+    /* 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;
+
+        /*
+         * 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;
+
+        /*
+         * 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 || !lpae_valid(pte) || lpae_is_superpage(pte, level) )
+            break;
+
+        /*
+         * Temporarily store permissions of the table descriptor as they are
+         * inherited by page table attributes (ARM DDI 0487B.a J1-5928).
+         */
+        xn_table |= pte.pt.xnt;             /* Execute-Never */
+        ro_table |= pte.pt.apt & BIT(1);    /* Read-Only */
+
+        page = get_page_from_gfn(d, pte.walk.base, NULL, P2M_ALLOC);
+
+        if ( !page )
+            return -EFAULT;
+
+        table = __map_domain_page(page);
+    }
+
+    /*
+     * According to to ARM DDI 0487B.a J1-5927, we return an error if the found
+     * PTE is invalid or holds a reserved entry (PTE<1:0> == x0)) or if the PTE
+     * maps a memory block at level 3 (PTE<1:0> == 01).
+     */
+    if ( !lpae_valid(pte) || ((level == 3) && lpae_mapping(pte)) )
+        return -EFAULT;
+
+    *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,
-- 
2.12.2


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

[PATCH v4 8/9] 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.

v4: Adjusted the names of short-descriptor data-types.

    Adapt the function to the new parameter of type "struct vcpu *".

    Cosmetic fixes.
---
 xen/arch/arm/guest_walk.c        | 167 ++++++++++++++++++++++++++++++++++++++-
 xen/include/asm-arm/guest_walk.h |  16 ++++
 2 files changed, 181 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index c37c595157..9cc167af49 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -18,6 +18,7 @@
 #include <xen/sched.h>
 #include <xen/domain_page.h>
 #include <asm/guest_walk.h>
+#include <asm/short-desc.h>
 
 /*
  * The function guest_walk_sd translates a given GVA into an IPA using the
@@ -30,8 +31,170 @@ static int guest_walk_sd(const struct vcpu *v,
                          vaddr_t gva, paddr_t *ipa,
                          unsigned int *perms)
 {
-    /* Not implemented yet. */
-    return -EFAULT;
+    bool disabled = true;
+    int64_t ttbr;
+    paddr_t mask, paddr;
+    short_desc_t pte, *table;
+    struct page_info *page;
+    register_t ttbcr = READ_SYSREG(TCR_EL1);
+    unsigned int level = 0, n = ttbcr & TTBCR_N_MASK;
+    struct domain *d = v->domain;
+
+    const paddr_t offsets[2] = {
+        ((paddr_t)(gva >> 20) & ((1ULL << (12 - n)) - 1)),
+        ((paddr_t)(gva >> 12) & ((1ULL << 8) - 1))
+    };
+
+    mask = ((1ULL << BITS_PER_WORD) - 1) &
+           ~((1ULL << (BITS_PER_WORD - 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 = (short_desc_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 = (short_desc_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;
 }
 
 /*
diff --git a/xen/include/asm-arm/guest_walk.h b/xen/include/asm-arm/guest_walk.h
index 4ed8476e08..d0bed0c7a2 100644
--- a/xen/include/asm-arm/guest_walk.h
+++ b/xen/include/asm-arm/guest_walk.h
@@ -1,6 +1,22 @@
 #ifndef _XEN_GUEST_WALK_H
 #define _XEN_GUEST_WALK_H
 
+/* 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

[PATCH v4 9/9] 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.

v4: Comment why we need to fail if the permission flags that are
    requested by the caller do not satisfy the mapped page.

    Cosmetic fix that simplifies the if-statement checking for the
    GV2M_WRITE permission.
---
 xen/arch/arm/mem_access.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
index bcf49f5c15..9133ac8f03 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,35 @@ 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;
+
+        /*
+         * Check permissions that are assumed by the caller. For instance in
+         * case of guestcopy, the caller assumes that the translated page can
+         * be accessed with requested permissions. If this is not the case, we
+         * should fail.
+         *
+         * Please note that we do not check for the GV2M_EXEC permission. Yet,
+         * since the hardware-based translation through gva_to_ipa does not
+         * test for execute permissions this check can be left out.
+         */
+        if ( (flag & GV2M_WRITE) && !(perms & GV2M_WRITE) )
+            goto err;
+    }
 
     gfn = gaddr_to_gfn(ipa);
 
-- 
2.12.2


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

Re: [PATCH v4 9/9] arm/mem_access: Walk the guest's pt in software

[Tamas K Lengyel]

On Tue, Jun 20, 2017 at 2:33 PM, 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>

Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

> ---
> 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.
>
> v4: Comment why we need to fail if the permission flags that are
>     requested by the caller do not satisfy the mapped page.
>
>     Cosmetic fix that simplifies the if-statement checking for the
>     GV2M_WRITE permission.
> ---
>  xen/arch/arm/mem_access.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
> index bcf49f5c15..9133ac8f03 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,35 @@ 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.
> +             */

If you end up sending another round of the series, I would prefer to
see this comment before the if statement (but I wouldn't hold up the
series over that).

> +            goto err;
> +

Thanks,
Tamas

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

Re: [PATCH v4 9/9] arm/mem_access: Walk the guest's pt in software

[Sergej Proskurin]

Hi Tamas,

[...]

>> +        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.
>> +             */
> 
> If you end up sending another round of the series, I would prefer to
> see this comment before the if statement (but I wouldn't hold up the
> series over that).
> 
>> +            goto err;
>> +

Alright, will do. Thanks!

Cheers,
~Sergej

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

Re: [PATCH v4 1/9] arm/mem_access: Add (TCR_|TTBCR_)* defines

[Julien Grall]

Hi Sergej,

On 20/06/17 21:33, Sergej Proskurin wrote:
> 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.
>
> v4: Cosmetic changes.
> ---
>  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..863a569432 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)

Based on your comment above, I don't understand why you are shifting by 
0 here.

> +
> +#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,9 +190,50 @@
>  #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)

We are trying to avoid code clean-up and addition in the same patch. I 
am OK with that patch, but you need to mention it in the commit message.

> +/* 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)
> +
> +/* Note that the fields TCR_EL2.TBI(0|1) exist only if HCR_EL2.E2H==1. */
> +#define TCR_EL1_TBI0    (_AC(0x1,ULL)<<37)
> +#define TCR_EL1_TBI1    (_AC(0x1,ULL)<<38)
>
>  #ifdef CONFIG_ARM_64
>
>

Cheers,

-- 
Julien Grall

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

Re: [PATCH v4 2/9] arm/mem_access: Add defines supporting PTs with varying page sizes

[Julien Grall]

Hi Sergej,

On 20/06/17 21:33, 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.
>
> v4: Replace existing macros with ones that generate static inline
>     helpers as to ease the readability of the code.
>
>     Move the introduced code into lpae.h
> ---
>  xen/include/asm-arm/lpae.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
>
> diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
> index 6fbf7c606c..2913428e96 100644
> --- a/xen/include/asm-arm/lpae.h
> +++ b/xen/include/asm-arm/lpae.h
> @@ -151,6 +151,73 @@ static inline bool lpae_is_superpage(lpae_t pte, unsigned int level)
>      return (level < 3) && lpae_mapping(pte);
>  }
>
> +/*
> + * The ARMv8 architecture supports pages with different sizes (4K, 16K, and
> + * 64K). To enable guest page table walks for various configurations, the
> + * following helpers enable walking the guest's translation table with varying
> + * page size granularities.
> + */
> +
> +#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)

We already define PAGE_SHIFT_* in xen/iommu.h. You probably want to 
consolidate them in a single common place (maybe xen/paging.h or 
xen/lib.h?).

> +
> +#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(gva, gran)     GUEST_TABLE_OFFSET((gva >> third_shift(gran)), gran)
> +#define second_guest_table_offset(gva, gran)    GUEST_TABLE_OFFSET((gva >> second_shift(gran)), gran)
> +#define first_guest_table_offset(gva, gran)     GUEST_TABLE_OFFSET((gva >> first_shift(gran)), gran)
> +#define zeroeth_guest_table_offset(gva, gran)   GUEST_TABLE_OFFSET((gva >> zeroeth_shift(gran)), gran)

I don't think we should expose those macros and AFAICT you only use them 
once. So I would prefer if you move open-code them in the helpers below.

> +
> +#define GUEST_TABLE_OFFSET_HELPERS(gran)                                \
> +static inline vaddr_t third_guest_table_offset_##gran##K(vaddr_t gva)   \
> +{                                                                       \
> +    return third_guest_table_offset(gva, gran##K);                      \
> +}                                                                       \
> +                                                                        \
> +static inline vaddr_t second_guest_table_offset_##gran##K(vaddr_t gva)  \
> +{                                                                       \
> +    return second_guest_table_offset(gva, gran##K);                     \
> +}                                                                       \
> +                                                                        \
> +static inline vaddr_t first_guest_table_offset_##gran##K(vaddr_t gva)   \
> +{                                                                       \
> +    return first_guest_table_offset(gva, gran##K);                      \
> +}                                                                       \
> +                                                                        \
> +static inline vaddr_t zeroeth_guest_table_offset_##gran##K(vaddr_t gva) \
> +{                                                                       \
> +    if ( gran == 64 )                                                   \

Please add a comment saying 64K does not have zeroeth page-table.

> +        return 0;                                                       \
> +    else                                                                \
> +        return zeroeth_guest_table_offset((paddr_t)gva, gran##K);       \
> +}                                                                       \
> +
> +GUEST_TABLE_OFFSET_HELPERS(4);
> +#ifdef CONFIG_ARM_64
> +GUEST_TABLE_OFFSET_HELPERS(16);
> +GUEST_TABLE_OFFSET_HELPERS(64);
> +#endif

Please undef both GUEST_TABLE_OFFSET and GUEST_TABLE_OFFSET_HELPERS.

> +
>  #endif /* __ASSEMBLY__ */
>
>  /*
>

Cheers,

-- 
Julien Grall

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

Re: [PATCH v4 5/9] arm/mem_access: Extend BIT-operations to unsigned long long

[Julien Grall]

Hi Sergej,

On 20/06/17 21:33, Sergej Proskurin wrote:
> We extend the BIT macro to using values of unsigned long long as to
> enable setting bits of 64-bit registers on AArch32.  In addition, this
> commit adds a define holding the register width of 64 bit double-word
> registers. This define simplifies 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>
> ---
> v4: We reused the previous commit with the msg "arm/mem_access: Add
>     defines holding the width of 32/64bit regs" from v3, as we can reuse
>     the already existing define BITS_PER_WORD.
> ---
>  xen/include/asm-arm/bitops.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/include/asm-arm/bitops.h b/xen/include/asm-arm/bitops.h
> index bda889841b..c3486d497c 100644
> --- a/xen/include/asm-arm/bitops.h
> +++ b/xen/include/asm-arm/bitops.h
> @@ -21,7 +21,8 @@
>  #define __clear_bit(n,p)          clear_bit(n,p)
>
>  #define BITS_PER_WORD           32
> -#define BIT(nr)                 (1UL << (nr))
> +#define BITS_PER_DOUBLE_WORD    64
> +#define BIT(nr)                 (1ULL << (nr))

This macro is part of a set (BIT, BIT_MASK, BIT_WORD) that work on 
unsigned long. Now you are suggesting to upgrade BIT to support unsigned 
long long. This will confuse more than one user and break this set.

I would much prefer if you introduce BIT_ULL as Linux does. At the same 
time BITS_PER_DOUBLE_WORD should be renamed BITS_PER_LONG_LONG.

>  #define BIT_MASK(nr)            (1UL << ((nr) % BITS_PER_WORD))
>  #define BIT_WORD(nr)            ((nr) / BITS_PER_WORD)
>  #define BITS_PER_BYTE           8

Cheers,

-- 
Julien Grall

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

Re: [PATCH v4 6/9] arm/mem_access: Add software guest-page-table walk

[Julien Grall]

Hi Sergej,

On 20/06/17 21:33, Sergej Proskurin wrote:
> +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);
> +    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(v->domain) )
> +    {
> +        if ( !(tcr & TTBCR_EAE) )

NIT: you can merge the two if to above one level of indentation. I.e:

if ( is_32bit_domain(v->domain) && (!tcr & TTBCR_EAE) )
   return guest_walk_ld(...);
else
   return guest_walk_ld(...);

With that change:

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: [PATCH v4 6/9] arm/mem_access: Add software guest-page-table walk

[Sergej Proskurin]

Hi Julien,


On 06/22/2017 01:16 PM, Julien Grall wrote:
> Hi Sergej,
>
> On 20/06/17 21:33, Sergej Proskurin wrote:
>> +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);
>> +    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(v->domain) )
>> +    {
>> +        if ( !(tcr & TTBCR_EAE) )
>
> NIT: you can merge the two if to above one level of indentation. I.e:
>
> if ( is_32bit_domain(v->domain) && (!tcr & TTBCR_EAE) )
>   return guest_walk_ld(...);
> else
>   return guest_walk_ld(...);
>
> With that change:
>
> Acked-by: Julien Grall <julien.grall@arm.com> 

I will do that right away, thanks.

Cheers,
~Sergej


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

Re: [PATCH v4 7/9] arm/mem_access: Add long-descriptor based gpt

[Julien Grall]

Hi Sergej,

On 20/06/17 21:33, 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.
>
> 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.
>
> v4: Move functionality responsible for determining the configured IPA
>     output-size into a separate function get_ipa_output_size. In this
>     function, we remove the previously used switch statement, which was
>     responsible for distinguishing between different IPA output-sizes.
>     Instead, we retrieve the information from the introduced ipa_sizes
>     array.
>
>     Remove the defines GRANULE_SIZE_INDEX_* and TTBR0_VALID from
>     guest_walk.h. Instead, introduce the enums granule_size_index
>     active_ttbr directly inside of guest_walk.c so that the associated
>     fields don't get exported.
>
>     Adapt the function to the new parameter of type "struct vcpu *".
>
>     Remove support for 52bit IPA output-sizes entirely from this commit.
>
>     Use lpae_* helpers instead of p2m_* helpers.
>
>     Cosmetic fixes & Additional comments.
> ---
>  xen/arch/arm/guest_walk.c | 385 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 383 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index b8bb553a6e..c37c595157 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>

Please order the include the alphabetic order.

>
>  /*
>   * The function guest_walk_sd translates a given GVA into an IPA using the
> @@ -33,6 +35,142 @@ static int guest_walk_sd(const struct vcpu *v,
>  }
>
>  /*
> + * Get the IPA output_size (configured in TCR_EL1) that shall be used for the
> + * long-descriptor based translation table walk.
> + */
> +static unsigned int get_ipa_output_size(struct domain *d, register_t tcr)
> +{
> +    unsigned int output_size;
> +    uint64_t ips;
> +
> +    static const uint64_t ipa_sizes[7] = {
> +        TCR_EL1_IPS_32_BIT_VAL,
> +        TCR_EL1_IPS_36_BIT_VAL,
> +        TCR_EL1_IPS_40_BIT_VAL,
> +        TCR_EL1_IPS_42_BIT_VAL,
> +        TCR_EL1_IPS_44_BIT_VAL,
> +        TCR_EL1_IPS_48_BIT_VAL,
> +        TCR_EL1_IPS_52_BIT_VAL
> +    };
> +
> +    if ( is_64bit_domain(d) )
> +    {
> +        /* Get the intermediate physical address size. */
> +        ips = (tcr & TCR_EL1_IPS_MASK) >> TCR_EL1_IPS_SHIFT;
> +
> +        /*
> +         * Return an error on reserved IPA output-sizes and if the IPA
> +         * output-size is 52bit.
> +         *
> +         * XXX: 52 bit output_size is not supported yet.
> +         */
> +        if ( ips > TCR_EL1_IPS_48_BIT )
> +            return -EFAULT;
> +
> +        output_size = ipa_sizes[ips];
> +    }
> +    else
> +        output_size = TCR_EL1_IPS_40_BIT_VAL;
> +
> +    return output_size;
> +}
> +
> +/* Normalized page granule size indices. */
> +enum granule_size_index {
> +    GRANULE_SIZE_INDEX_4K,
> +    GRANULE_SIZE_INDEX_16K,
> +    GRANULE_SIZE_INDEX_64K
> +};
> +
> +/* Represent whether TTBR0 or TTBR1 is active. */
> +enum active_ttbr {
> +    TTBR0_ACTIVE,
> +    TTBR1_ACTIVE
> +};
> +
> +/*
> + * 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, enum active_ttbr ttbrx)
> +{
> +    bool disabled;
> +
> +    if ( ttbrx == TTBR0_ACTIVE )
> +    {
> +        /* 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:

Please explain why you use 4K by 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;

Please explain why you use 4K by default.

> +        }
> +
> +        /* 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;
> +}
> +
> +/*
> + * 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;
> +    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;
> +    }
> +
> +    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
>   * that the domain is running on the currently active vCPU. To walk the guest's
> @@ -43,8 +181,251 @@ static int guest_walk_ld(const struct vcpu *v,
>                           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;
> +    paddr_t mask;
> +    lpae_t pte, *table;
> +    struct page_info *page;
> +    register_t tcr = READ_SYSREG(TCR_EL1);
> +    struct domain *d = v->domain;
> +
> +    const vaddr_t offsets[4][3] = {

It would have been more readable if you had inverted the index (i.e 
granularity then level). It would also allow to use macro:

#define OFFSET(gva, gran)
{
	zeroeth_guest_table_offset_##gran(gva),
	first_guest_table_offset_##gran(gva),
	second_guest_table_offset_##gran(gva),
	third_guest_table_offset_##gran(gva),
}

const vaddr_t offsets[3][4] = {
	OFFSET(gva, 4K),
	OFFSET(gva, 16K),
	OFFSET(gva, 64K),
};

#undef OFFSET

> +        {
> +#ifdef CONFIG_ARM_64

Again, I find those #ifdef pointless, more that you don't use them for 
the masks below... and you will always compute them for 32-bit domain on 
AArch64 Xen.

> +            zeroeth_guest_table_offset_4K(gva),
> +            zeroeth_guest_table_offset_16K(gva),
> +            0, /* There is no zeroeth lookup level with a 64K granule size. */

Please use zeroeth_guest_table_offset_64K(gva) as it does already the 
job for you.

> +#endif
> +        },
> +        {
> +            first_guest_table_offset_4K(gva),
> +#ifdef CONFIG_ARM_64

Ditto

> +            first_guest_table_offset_16K(gva),
> +            first_guest_table_offset_64K(gva),
> +#endif
> +        },
> +        {
> +            second_guest_table_offset_4K(gva),
> +#ifdef CONFIG_ARM_64

Ditto

> +            second_guest_table_offset_16K(gva),
> +            second_guest_table_offset_64K(gva),
> +#endif
> +        },
> +        {
> +            third_guest_table_offset_4K(gva),
> +#ifdef CONFIG_ARM_64

Ditto

> +            third_guest_table_offset_16K(gva),
> +            third_guest_table_offset_64K(gva),
> +#endif
> +        }
> +    };
> +
> +    static const paddr_t masks[4][3] = {

Similar here for the array.

> +        {
> +            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);
> +
> +    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 = BITS_PER_DOUBLE_WORD - t0_sz;

I don't think using BITS_PER_DOUBLE_WORD is correct here. The 
pseudo-code is explicitly saying 64 and does not mention anything about 
double word.

I agree that bit shift should be using define when it is related to a 
register. But I totally disagree on replacing every single number by 
macro when it is not possibility to find a suitable name.

> +
> +            /* Get TTBR0 and configured page granularity. */
> +            disabled = get_ttbr_and_gran_64bit(&ttbr, &gran, tcr, TTBR0_ACTIVE);
> +        }
> +        else
> +        {
> +            input_size = BITS_PER_DOUBLE_WORD - t1_sz;

Ditto.

> +
> +            /* Get TTBR1 and configured page granularity. */
> +            disabled = get_ttbr_and_gran_64bit(&ttbr, &gran, tcr, TTBR1_ACTIVE);
> +        }
> +
> +        /*
> +         * 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
> +    {
> +        /* 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 << BITS_PER_WORD) - 1) &

Same remark as BITS_PER_DOUBLE_WORD and it stands for all the usage of 
BITS_PER_WORD and BITS_PER_DOUBLE_WORD within this patch.

> +               ~((1ULL << (BITS_PER_WORD - t0_sz)) - 1);
> +
> +        if ( t0_sz == 0 || !(gva & mask) )
> +        {
> +            input_size = BITS_PER_WORD - 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 << BITS_PER_WORD) - 1) &
> +               ~((1ULL << (BITS_PER_WORD - t1_sz)) - 1);

Please use GENMASK here plase.

> +
> +        if ( ((t1_sz == 0) && !ttbr) || (t1_sz && (gva & mask) == mask) )
> +        {
> +            input_size = BITS_PER_WORD - 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));
> +
> +    /* Get the IPA output_size. */
> +    output_size = get_ipa_output_size(d, tcr);
> +
> +    /* 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);

This use this construction in two place with a check after. It would 
make sense to have an helper for it.

> +
> +        if ( (output_size < TCR_EL1_IPS_48_BIT_VAL) &&
> +             (pfn_to_paddr(pte.walk.base) & mask) )
> +            return -EFAULT;
> +
> +        /*
> +         * 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;
> +
> +        /*
> +         * 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 || !lpae_valid(pte) || lpae_is_superpage(pte, level) )
> +            break;
> +
> +        /*
> +         * Temporarily store permissions of the table descriptor as they are
> +         * inherited by page table attributes (ARM DDI 0487B.a J1-5928).
> +         */
> +        xn_table |= pte.pt.xnt;             /* Execute-Never */
> +        ro_table |= pte.pt.apt & BIT(1);    /* Read-Only */
> +
> +        page = get_page_from_gfn(d, pte.walk.base, NULL, P2M_ALLOC);
> +
> +        if ( !page )
> +            return -EFAULT;
> +
> +        table = __map_domain_page(page);
> +    }
> +
> +    /*
> +     * According to to ARM DDI 0487B.a J1-5927, we return an error if the found
> +     * PTE is invalid or holds a reserved entry (PTE<1:0> == x0)) or if the PTE
> +     * maps a memory block at level 3 (PTE<1:0> == 01).
> +     */
> +    if ( !lpae_valid(pte) || ((level == 3) && lpae_mapping(pte)) )

If you look at the comment on top of lpae_mapping, it should only be 
used for L0..L2 ptes. So why are you using it on L3 ptes?

> +        return -EFAULT;
> +
> +    *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,
>

Cheers,

-- 
Julien Grall

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

Re: [PATCH v4 8/9] arm/mem_access: Add short-descriptor based gpt

[Julien Grall]

Hi Sergej,

On 20/06/17 21:33, Sergej Proskurin wrote:
> 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.
>
> v4: Adjusted the names of short-descriptor data-types.
>
>     Adapt the function to the new parameter of type "struct vcpu *".
>
>     Cosmetic fixes.
> ---
>  xen/arch/arm/guest_walk.c        | 167 ++++++++++++++++++++++++++++++++++++++-
>  xen/include/asm-arm/guest_walk.h |  16 ++++
>  2 files changed, 181 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index c37c595157..9cc167af49 100644
> --- a/xen/arch/arm/guest_walk.c
> +++ b/xen/arch/arm/guest_walk.c
> @@ -18,6 +18,7 @@
>  #include <xen/sched.h>
>  #include <xen/domain_page.h>
>  #include <asm/guest_walk.h>
> +#include <asm/short-desc.h>
>
>  /*
>   * The function guest_walk_sd translates a given GVA into an IPA using the
> @@ -30,8 +31,170 @@ static int guest_walk_sd(const struct vcpu *v,
>                           vaddr_t gva, paddr_t *ipa,
>                           unsigned int *perms)
>  {
> -    /* Not implemented yet. */
> -    return -EFAULT;
> +    bool disabled = true;
> +    int64_t ttbr;

TTBR is a register. It cannot be signed.

> +    paddr_t mask, paddr;
> +    short_desc_t pte, *table;
> +    struct page_info *page;
> +    register_t ttbcr = READ_SYSREG(TCR_EL1);
> +    unsigned int level = 0, n = ttbcr & TTBCR_N_MASK;
> +    struct domain *d = v->domain;
> +
> +    const paddr_t offsets[2] = {

Why are you using paddr_t here?

Looking at the code, I see very limited point of having the offsets 
array as you don't use a loop and also use each offset in a single place.

> +        ((paddr_t)(gva >> 20) & ((1ULL << (12 - n)) - 1)),

Why the cast? After gva is a virtual address not physical one.

Also, this code is a bit cryptic to read. Can we have some documentation 
at least?

Furthermore, it would be clearer if you first mask then shift. As it 
helps to understand the code.

If you use GENMASK (or GENMASK_ULL if you don't extend GENMASK), this 
will make everything more obvious:

(gva & GENMASK(31 - n, 20)) >> 20

> +        ((paddr_t)(gva >> 12) & ((1ULL << 8) - 1))

(gva & GENMASK(20, 12)) >> 12

> +    };
> +
> +    mask = ((1ULL << BITS_PER_WORD) - 1) &
> +           ~((1ULL << (BITS_PER_WORD - n)) - 1);

Same remark as on the previous patch for BITS_PER_WORD + you could use 
GENMASK.

> +
> +    if ( n == 0 || !(gva & mask) )
> +    {
> +        /* Use TTBR0 for GVA to IPA translation. */
> +        ttbr = READ_SYSREG64(TTBR0_EL1);

Looking at the documentation. For short-descriptor, the register will be 
32-bit. Whilst I can understand why you use READ_SYSREG64, you should at 
least document it. You also probably want to have ttbr uint32_t in that 
case.

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

Ditto.

> +
> +        /* 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;

mask = GENMASK(31, 14);

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

GENMASK(12, 14 - n);

> +        table = (short_desc_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];

This looks quite complex for just reading a pte. I think it would be 
worth to leverage the vgic_guest_access_memory for that (same in LPAE). 
It would also add safety if the offsets the table is mistakenly computed 
(from the current code, I can't convince myself the offset will always 
be right).

> +
> +    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 = (short_desc_t *)((unsigned long)table | ((pte.walk.base & 0x3) << 10));
> +
> +        pte = table[offsets[level]];

Ditto.

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

As perms is modified in few places over the code. I would prefer if you 
first reset *perms at suitable value in guest_walk_tables and the use |= 
everywhere.

This would avoid any mistake in the future where the permission is not 
reported correctly.

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

See above.

> +        }
> +
> +        /* 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;

It is quite strange that above you use plain PAGE_SHIFT_4K which is not 
really related to short-descriptor (you define it in lpae.h) and here 
you use short-descriptor name. Please try to stay consistent.

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

My suggestion about perms would also avoid undefined permission if the 
region is read-only as none of the callers today will initialize perms.

> +        if ( !pte.sec.xn )
> +            *perms |= GV2M_EXEC;
> +    }
> +
> +    return 0;
>  }
>
>  /*
> diff --git a/xen/include/asm-arm/guest_walk.h b/xen/include/asm-arm/guest_walk.h
> index 4ed8476e08..d0bed0c7a2 100644
> --- a/xen/include/asm-arm/guest_walk.h
> +++ b/xen/include/asm-arm/guest_walk.h
> @@ -1,6 +1,22 @@
>  #ifndef _XEN_GUEST_WALK_H
>  #define _XEN_GUEST_WALK_H
>
> +/* 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)

I think those one belongs to short-desc.h.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v4 7/9] arm/mem_access: Add long-descriptor based gpt

[Julien Grall]

Hi Sergej,

On 20/06/17 21:33, Sergej Proskurin wrote:
> +    /*
> +     * 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;

See patch #8 about the problem with the permissions.

> +    if ( !pte.pt.xn && !xn_table )
> +        *perms |= GV2M_EXEC;
> +
> +    return 0;
>  }
>
>  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: [PATCH v4 7/9] arm/mem_access: Add long-descriptor based gpt

[Sergej Proskurin]

Hi Julien,

[...]

>> +static bool get_ttbr_and_gran_64bit(uint64_t *ttbr, unsigned int *gran,
>> +                                    register_t tcr, enum active_ttbr
>> ttbrx)
>> +{
>> +    bool disabled;
>> +
>> +    if ( ttbrx == TTBR0_ACTIVE )
>> +    {
>> +        /* 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:
> 
> Please explain why you use 4K by default.
> 

Fair question. According to ARM DDI 0487B.a D7-2487, if the
TCR_EL1.{TG0|TG1} holds a reserved value, it is implementation defined
how the value is interpreted by the underlying hardware. My
implementation in v4 strongly followed the pseudo-code in ARM DDI
0487B.a, which suggested to use fall back to 4K by default.

However, agree with you at this point. My next patch series explicitly
checks if 4K has to be set or not and returns an error on reserved
values as we cannot be know how the hardware behaves on reserved values.

[...]

>> +
>> +    /*
>> +     * According to to ARM DDI 0487B.a J1-5927, we return an error if
>> the found
>> +     * PTE is invalid or holds a reserved entry (PTE<1:0> == x0)) or
>> if the PTE
>> +     * maps a memory block at level 3 (PTE<1:0> == 01).
>> +     */
>> +    if ( !lpae_valid(pte) || ((level == 3) && lpae_mapping(pte)) )
> 
> If you look at the comment on top of lpae_mapping, it should only be
> used for L0..L2 ptes. So why are you using it on L3 ptes?
> 

Yes, I saw the comment. Yet, I would like to check exactly for this
mapping. If the mapping would as in the check above, it would be an
error, which is treated accordingly. In v3, you have suggested to look
at the the lpae_is_superpage macro, which, in my opinion, is not the
right construct to use at this point. If you should not like this check,
I could fall back to my previous implementation:

if ( !p2m_valid(pte) || ((level == 3) && !p2m_table(pte)) )
    return -EFAULT;


Thank you,
~Sergej

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

Re: [PATCH v4 7/9] arm/mem_access: Add long-descriptor based gpt

[Julien Grall]

On 23/06/17 15:23, Sergej Proskurin wrote:
> Hi Julien,
>
> [...]
>
>>> +static bool get_ttbr_and_gran_64bit(uint64_t *ttbr, unsigned int *gran,
>>> +                                    register_t tcr, enum active_ttbr
>>> ttbrx)
>>> +{
>>> +    bool disabled;
>>> +
>>> +    if ( ttbrx == TTBR0_ACTIVE )
>>> +    {
>>> +        /* 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:
>>
>> Please explain why you use 4K by default.
>>
>
> Fair question. According to ARM DDI 0487B.a D7-2487, if the
> TCR_EL1.{TG0|TG1} holds a reserved value, it is implementation defined
> how the value is interpreted by the underlying hardware. My
> implementation in v4 strongly followed the pseudo-code in ARM DDI
> 0487B.a, which suggested to use fall back to 4K by default.
>
> However, agree with you at this point. My next patch series explicitly
> checks if 4K has to be set or not and returns an error on reserved
> values as we cannot be know how the hardware behaves on reserved values.

No. You should not return an error here as you would not be compliant 
with the ARM ARM. I just asked to add a comment explain why you choose 
4K, even if it says "We follow the pseudo-code".

>
> [...]
>
>>> +
>>> +    /*
>>> +     * According to to ARM DDI 0487B.a J1-5927, we return an error if
>>> the found
>>> +     * PTE is invalid or holds a reserved entry (PTE<1:0> == x0)) or
>>> if the PTE
>>> +     * maps a memory block at level 3 (PTE<1:0> == 01).
>>> +     */
>>> +    if ( !lpae_valid(pte) || ((level == 3) && lpae_mapping(pte)) )
>>
>> If you look at the comment on top of lpae_mapping, it should only be
>> used for L0..L2 ptes. So why are you using it on L3 ptes?
>>
>
> Yes, I saw the comment. Yet, I would like to check exactly for this
> mapping. If the mapping would as in the check above, it would be an
> error, which is treated accordingly. In v3, you have suggested to look
> at the the lpae_is_superpage macro, which, in my opinion, is not the
> right construct to use at this point. If you should not like this check,
> I could fall back to my previous implementation:
>
> if ( !p2m_valid(pte) || ((level == 3) && !p2m_table(pte)) )
>     return -EFAULT;

If you look at the ARM ARM (D4.3.1 and D4.3.2 in DDI0487B.a)
	* level 0,1,2 will have bit 1 set for table and cleared for mapping.
	* level 3 will have bit 1 set for mapping

If you use p2m_table to check it the bit is set, then it mislead 
completely the user as this entry is not a table at all.

In any, this is totally wrong to use p2m_table and p2m_mapping here as 
it would not report correctly the value. So please don't use an helper 
that does not make sense here. You should just open-code it or introduce 
a helper (such as lpae_page with appropriate) to properly check the bit.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v4 8/9] arm/mem_access: Add short-descriptor based gpt

[Sergej Proskurin]

Hi Julien,

[...]

> 
> Looking at the code, I see very limited point of having the offsets
> array as you don't use a loop and also use each offset in a single place.
> 
>> +        ((paddr_t)(gva >> 20) & ((1ULL << (12 - n)) - 1)),
> 
Don't you think it is more readable to have the GVA offsets at one
place. Also, the functionality is in this way similar to the one shown
in guest_walk_ld. In my opinion, it is more intuitive to have both
functions work in a similar way. I suggest keeping the array, however
using GENMASK to generate it (as you mentioned in your comments below).

const vaddr_t offsets[2] = {
    (gva & GENMASK((31 - n), 20)) >> 20,
    (gva & GENMASK(19, 12)) >> 12,
};

> 
> Furthermore, it would be clearer if you first mask then shift. As it
> helps to understand the code.
> 
> If you use GENMASK (or GENMASK_ULL if you don't extend GENMASK), this
> will make everything more obvious:
> 

[...]


>> +
>> +    /*
>> +     * 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];
> 
> This looks quite complex for just reading a pte. I think it would be
> worth to leverage the vgic_guest_access_memory for that (same in LPAE).
> It would also add safety if the offsets the table is mistakenly computed
> (from the current code, I can't convince myself the offset will always
> be right).

As far as I understand, we would still need to use the same offsets even
if we used vgic_access_guest_memory. Also, the only significant
difference between my implementation and vgic_access_guest_memory is
that gic_access_guest_memory checks whether we write over page
boundaries, which is quite hard to achieve as the offsets are limited in
number so that they don't cross page boundaries.

Yet, if you insist, I will try to incorporate vgic_access_guest_memory
into my implementation.

Thanks,
~Sergej



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

Re: [PATCH v4 8/9] arm/mem_access: Add short-descriptor based gpt

[Julien Grall]

On 06/23/2017 08:09 PM, Sergej Proskurin wrote:
> Hi Julien,

Hi Sergej,

> 
> [...]
> 
>>
>> Looking at the code, I see very limited point of having the offsets
>> array as you don't use a loop and also use each offset in a single place.
>>
>>> +        ((paddr_t)(gva >> 20) & ((1ULL << (12 - n)) - 1)),
>>
> Don't you think it is more readable to have the GVA offsets at one
> place. Also, the functionality is in this way similar to the one shown
> in guest_walk_ld. In my opinion, it is more intuitive to have both
> functions work in a similar way. I suggest keeping the array, however
> using GENMASK to generate it (as you mentioned in your comments below).

I disagree here. The way to lookup short-descriptor and long-descriptor 
are very different, for instance you don't have a loop as each level 
handle the offset differently. See the way you play will the first level 
table offsets.

+    paddr = (ttbr & ~mask) | (offsets[level] << 2);

[...]

+    mask = ((1ULL << 10) - 1);
+    pte = table[offsets[level] & mask];
+

If you find a way to avoiding playing with the offsets another time, 
then maybe it is worth it.

> 
> const vaddr_t offsets[2] = {
>      (gva & GENMASK((31 - n), 20)) >> 20,
>      (gva & GENMASK(19, 12)) >> 12,
> };
> 
>>
>> Furthermore, it would be clearer if you first mask then shift. As it
>> helps to understand the code.
>>
>> If you use GENMASK (or GENMASK_ULL if you don't extend GENMASK), this
>> will make everything more obvious:
>>
> 
> [...]
> 
> 
>>> +
>>> +    /*
>>> +     * 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];
>>
>> This looks quite complex for just reading a pte. I think it would be
>> worth to leverage the vgic_guest_access_memory for that (same in LPAE).
>> It would also add safety if the offsets the table is mistakenly computed
>> (from the current code, I can't convince myself the offset will always
>> be right).
> 
> As far as I understand, we would still need to use the same offsets even
> if we used vgic_access_guest_memory. Also, the only significant
> difference between my implementation and vgic_access_guest_memory is
> that gic_access_guest_memory checks whether we write over page
> boundaries, which is quite hard to achieve as the offsets are limited in
> number so that they don't cross page boundaries.

+        /* Make sure that we consider the bits ttbr<12:14-n> if n > 2. */
+        mask = ((1ULL << 12) - 1) & ~((1ULL << (14 - n)) - 1);
+        table = (short_desc_t *)((unsigned long)table | (unsigned 
long)(ttbr & mask));

[...]

+    mask = ((1ULL << 10) - 1);
+    pte = table[offsets[level] & mask];

Are you able to prove me this will never cross a page boundary? 
Personally, when I read that I cannot convince myself that this will 
never cross happen. This is guest memory mapped, so if there is any 
coding error, you may access unmapped memory and then DOS Xen. Not very 
nice :).

We have a function that read/write into guest memory with all safety 
check associated to it. We should take advantage of any helpers that 
will mitigate any potential miscalculations as you would just the wrong 
IPA. It is better than a DOS and also avoid open-coding so it is much 
easier to update any change in the way we access the guest memory.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v4 8/9] arm/mem_access: Add short-descriptor based gpt

[Sergej Proskurin]

Hi Julien,

[...]
 
>>>> +
>>>> +    /*
>>>> +     * 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];
>>>
>>> This looks quite complex for just reading a pte. I think it would be
>>> worth to leverage the vgic_guest_access_memory for that (same in LPAE).
>>> It would also add safety if the offsets the table is mistakenly
>>> computed
>>> (from the current code, I can't convince myself the offset will always
>>> be right).
>>
>> As far as I understand, we would still need to use the same offsets even
>> if we used vgic_access_guest_memory. Also, the only significant
>> difference between my implementation and vgic_access_guest_memory is
>> that gic_access_guest_memory checks whether we write over page
>> boundaries, which is quite hard to achieve as the offsets are limited in
>> number so that they don't cross page boundaries.
>
> +        /* Make sure that we consider the bits ttbr<12:14-n> if n >
> 2. */
> +        mask = ((1ULL << 12) - 1) & ~((1ULL << (14 - n)) - 1);
> +        table = (short_desc_t *)((unsigned long)table | (unsigned
> long)(ttbr & mask));
>
> [...]
>
> +    mask = ((1ULL << 10) - 1);
> +    pte = table[offsets[level] & mask];
>
> Are you able to prove me this will never cross a page boundary?
> Personally, when I read that I cannot convince myself that this will
> never cross happen. This is guest memory mapped, so if there is any
> coding error, you may access unmapped memory and then DOS Xen. Not
> very nice :).
>
> We have a function that read/write into guest memory with all safety
> check associated to it. We should take advantage of any helpers that
> will mitigate any potential miscalculations as you would just the
> wrong IPA. It is better than a DOS and also avoid open-coding so it is
> much easier to update any change in the way we access the guest memory.

Fair enough. I agree, this would indeed make things easier as we would
need to incorporate potential changes to guest memory access logic only
in one place. I will incorporate your suggestions into my code before my
next patch submission. Thank you.

Cheers,
~Sergej


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

Re: [PATCH v4 3/9] arm/mem_access: Add short-descriptor pte typedefs

[Julien Grall]

Hi Sergekj,

On 06/20/2017 09:33 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>

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: [PATCH v4 3/9] arm/mem_access: Add short-descriptor pte typedefs

[Julien Grall]

On 07/04/2017 05:22 PM, Julien Grall wrote:
> Hi Sergekj,
> 
> On 06/20/2017 09:33 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>
> 
> Acked-by: Julien Grall <julien.grall@arm.com>

Hmmm, I commented on the wrong version. I will look at v5. Sorry for the 
noise.

Cheers,

-- 
Julien Grall

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