[PATCH v5 00/12] arm/mem_access: Walk guest page tables in SW if mem_access is active

[PATCH v5 00/12] 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.

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.
Also, this patch reuses existing code for accessing the guest's memory through
the function vgic_access_guest_memory. In this way, we outsource
safety-relevant checks associated with accessing the guest's memory to one
place and hence simplify potential modification of such.  Please note that this
patch series is based on the second part of Julien Grall's patch series in [1]:
"xen/arm: Move LPAE definition in a separate header".

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

Cheers,
~Sergej

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


Sergej Proskurin (12):
  arm/mem_access: Add and cleanup (TCR_|TTBCR_)* defines
  arm/mem_access: Move PAGE_SHIFT_* macros to lib.h
  arm/mem_access: Add defines supporting PTs with varying page sizes
  arm/lpae: Introduce lpae_page helper
  arm/mem_access: Add short-descriptor pte typedefs and macros
  arm/mem_access: Introduce GV2M_EXEC permission
  arm/mem_access: Introduce BIT_ULL bit operation
  arm/mem_access: Introduce GENMASK_ULL bit operation
  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        | 614 +++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/mem_access.c        |  31 +-
 xen/include/asm-arm/bitops.h     |   1 +
 xen/include/asm-arm/config.h     |   2 +
 xen/include/asm-arm/guest_walk.h |  19 ++
 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 | 130 +++++++++
 xen/include/xen/bitops.h         |   2 +
 xen/include/xen/iommu.h          |   3 +-
 xen/include/xen/lib.h            |   4 +
 13 files changed, 937 insertions(+), 7 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.13.1


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

[PATCH v5 01/12] arm/mem_access: Add and cleanup (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 macros
TCR_T0SZ and TCR_TG0_* by using the newly introduced TCR_T0SZ_SHIFT and
TCR_TG0_SHIFT instead of the hardcoded values.

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.

v5: Remove the shift by 0 of the TCR_SZ_MASK as it can be applied to
    both TCR_T0SZ and TCR_T1SZ (which reside at different offsets).

    Adjust commit message to make clear that we do not only add but also
    cleanup some TCR_* defines.
---
 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..3dd439de33 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))
+
+#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.13.1


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

[PATCH v5 02/12] arm/mem_access: Move PAGE_SHIFT_* macros to lib.h

[Sergej Proskurin]

The following commits introduce a software guest page table walk
software implementation that supports varying guest page size
granularities. This commit moves already existing PAGE_SHIFT_(4K|64K)
and the new PAGE_SHIFT_16K defines to a common place in xen/lib.h as
to allow the following commits to use the consolidated defines.

Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
---
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/include/xen/iommu.h | 3 +--
 xen/include/xen/lib.h   | 4 ++++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 5803e3f95b..75746e55b0 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -20,6 +20,7 @@
 #define _IOMMU_H_
 
 #include <xen/init.h>
+#include <xen/lib.h>
 #include <xen/spinlock.h>
 #include <xen/pci.h>
 #include <public/hvm/ioreq.h>
@@ -41,12 +42,10 @@ extern unsigned int iommu_dev_iotlb_timeout;
 #define IOMMU_PAGE_MASK(sz) (~(u64)0 << PAGE_SHIFT_##sz)
 #define IOMMU_PAGE_ALIGN(sz, addr)  (((addr) + ~PAGE_MASK_##sz) & PAGE_MASK_##sz)
 
-#define PAGE_SHIFT_4K       (12)
 #define PAGE_SIZE_4K        IOMMU_PAGE_SIZE(4K)
 #define PAGE_MASK_4K        IOMMU_PAGE_MASK(4K)
 #define PAGE_ALIGN_4K(addr) IOMMU_PAGE_ALIGN(4K, addr)
 
-#define PAGE_SHIFT_64K          (16)
 #define PAGE_SIZE_64K           IOMMU_PAGE_SIZE(64K)
 #define PAGE_MASK_64K           IOMMU_PAGE_MASK(64K)
 #define PAGE_ALIGN_64K(addr)    IOMMU_PAGE_ALIGN(64K, addr)
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 995a85a7db..8e2777ac67 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -8,6 +8,10 @@
 #include <xen/string.h>
 #include <asm/bug.h>
 
+#define PAGE_SHIFT_4K           (12)
+#define PAGE_SHIFT_16K          (14)
+#define PAGE_SHIFT_64K          (16)
+
 #define BUG_ON(p)  do { if (unlikely(p)) BUG();  } while (0)
 #define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
 
-- 
2.13.1


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

[PATCH v5 03/12] 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

v5: Remove PAGE_SHIFT_* defines from lpae.h as we import them now from
    the header xen/lib.h.

    Remove *_guest_table_offset macros as to reduce the number of
    exported macros which are only used once. Instead, use the
    associated functionality directly within the
    GUEST_TABLE_OFFSET_HELPERS.

    Add comment in GUEST_TABLE_OFFSET_HELPERS stating that a page table
    with 64K page size granularity does not have a zeroeth lookup level.

    Add #undefs for GUEST_TABLE_OFFSET and GUEST_TABLE_OFFSET_HELPERS.

    Remove CONFIG_ARM_64 #defines.
---
 xen/include/asm-arm/lpae.h | 62 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
index 6fbf7c606c..2f7891ed0b 100644
--- a/xen/include/asm-arm/lpae.h
+++ b/xen/include/asm-arm/lpae.h
@@ -3,6 +3,8 @@
 
 #ifndef __ASSEMBLY__
 
+#include <xen/lib.h>
+
 /*
  * WARNING!  Unlike the x86 pagetable code, where l1 is the lowest level and
  * l4 is the root of the trie, the ARM pagetables follow ARM's documentation:
@@ -151,6 +153,66 @@ 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 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 GUEST_TABLE_OFFSET_HELPERS(gran)                                                \
+static inline vaddr_t third_guest_table_offset_##gran##K(vaddr_t gva)                   \
+{                                                                                       \
+    return GUEST_TABLE_OFFSET((gva >> third_shift(gran##K)), gran##K);                  \
+}                                                                                       \
+                                                                                        \
+static inline vaddr_t second_guest_table_offset_##gran##K(vaddr_t gva)                  \
+{                                                                                       \
+    return GUEST_TABLE_OFFSET((gva >> second_shift(gran##K)), gran##K);                 \
+}                                                                                       \
+                                                                                        \
+static inline vaddr_t first_guest_table_offset_##gran##K(vaddr_t gva)                   \
+{                                                                                       \
+    return GUEST_TABLE_OFFSET(((paddr_t)gva >> first_shift(gran##K)), gran##K);         \
+}                                                                                       \
+                                                                                        \
+static inline vaddr_t zeroeth_guest_table_offset_##gran##K(vaddr_t gva)                 \
+{                                                                                       \
+    /* Note that there is no zeroeth lookup level with a 64K granule size. */           \
+    if ( gran == 64 )                                                                   \
+        return 0;                                                                       \
+    else                                                                                \
+        return GUEST_TABLE_OFFSET(((paddr_t)gva >> zeroeth_shift(gran##K)), gran##K);   \
+}                                                                                       \
+
+GUEST_TABLE_OFFSET_HELPERS(4);
+GUEST_TABLE_OFFSET_HELPERS(16);
+GUEST_TABLE_OFFSET_HELPERS(64);
+
+#undef GUEST_TABLE_OFFSET
+#undef GUEST_TABLE_OFFSET_HELPERS
+
 #endif /* __ASSEMBLY__ */
 
 /*
-- 
2.13.1


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

[PATCH v5 04/12] arm/lpae: Introduce lpae_page helper

[Sergej Proskurin]

This commit introduces a new helper that checks whether the target PTE
holds a page mapping or not. This helper will be used as part of the
following commits.

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

diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
index 2f7891ed0b..20565d2c8a 100644
--- a/xen/include/asm-arm/lpae.h
+++ b/xen/include/asm-arm/lpae.h
@@ -153,6 +153,11 @@ static inline bool lpae_is_superpage(lpae_t pte, unsigned int level)
     return (level < 3) && lpae_mapping(pte);
 }
 
+static inline bool lpae_page(lpae_t pte, unsigned int level)
+{
+    return (level == 3) && lpae_valid(pte) && pte.walk.table;
+}
+
 /*
  * The ARMv8 architecture supports pages with different sizes (4K, 16K, and
  * 64K). To enable guest page table walks for various configurations, the
-- 
2.13.1


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

[PATCH v5 05/12] arm/mem_access: Add short-descriptor pte typedefs and macros

[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_*.

v5: Add {L1|L2}DESC_* defines to this commit.
---
 xen/include/asm-arm/short-desc.h | 130 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 130 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..9652a103c4
--- /dev/null
+++ b/xen/include/asm-arm/short-desc.h
@@ -0,0 +1,130 @@
+#ifndef __ARM_SHORT_DESC_H__
+#define __ARM_SHORT_DESC_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)
+
+/* Defines for small (4K) and large page (64K) shifts. */
+#define L2DESC_SMALL_PAGE_SHIFT             (12)
+#define L2DESC_LARGE_PAGE_SHIFT             (16)
+
+/*
+ * 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.13.1


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

[PATCH v5 06/12] 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.13.1


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

[PATCH v5 07/12] arm/mem_access: Introduce BIT_ULL bit operation

[Sergej Proskurin]

We introduce the BIT_ULL 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.

v5: Introduce a new macro BIT_ULL instead of changing the type of the
    macro BIT.

    Remove the define BITS_PER_DOUBLE_WORD.
---
 xen/include/asm-arm/bitops.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/include/asm-arm/bitops.h b/xen/include/asm-arm/bitops.h
index bda889841b..1cbfb9edb2 100644
--- a/xen/include/asm-arm/bitops.h
+++ b/xen/include/asm-arm/bitops.h
@@ -24,6 +24,7 @@
 #define BIT(nr)                 (1UL << (nr))
 #define BIT_MASK(nr)            (1UL << ((nr) % BITS_PER_WORD))
 #define BIT_WORD(nr)            ((nr) / BITS_PER_WORD)
+#define BIT_ULL(nr)             (1ULL << (nr))
 #define BITS_PER_BYTE           8
 
 #define ADDR (*(volatile int *) addr)
-- 
2.13.1


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

[PATCH v5 08/12] arm/mem_access: Introduce GENMASK_ULL bit operation

[Sergej Proskurin]

The current implementation of GENMASK is capable of creating bitmasks of
32-bit values on AArch32 and 64-bit values on AArch64. As we need to
create masks for 64-bit values on AArch32 as well, in this commit we
introduce the GENMASK_ULL bit operation.

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

diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index 5b6f3c985d..7fa412f1b1 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -19,6 +19,8 @@
 #define BITS_PER_LONG (BYTES_PER_LONG << 3)
 #define POINTER_ALIGN BYTES_PER_LONG
 
+#define BITS_PER_LONG_LONG 64
+
 /* xen_ulong_t is always 64 bits */
 #define BITS_PER_XEN_ULONG 64
 
diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
index bd0883ab22..47170c9bfd 100644
--- a/xen/include/xen/bitops.h
+++ b/xen/include/xen/bitops.h
@@ -9,6 +9,8 @@
  */
 #define GENMASK(h, l) \
     (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
+#define GENMASK_ULL(h, l) \
+    (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
 
 /*
  * ffs: find first bit set. This is defined the same way as
-- 
2.13.1


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

[PATCH v5 09/12] 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>
Acked-by: Julien Grall <julien.grall@arm.com>
---
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.

v5: Merge two if-statements in guest_walk_tables to ease readability.

    Set perms to GV2M_READ as to avoid undefined permissions.
---
 xen/arch/arm/Makefile            |  1 +
 xen/arch/arm/guest_walk.c        | 89 ++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/guest_walk.h | 19 +++++++++
 3 files changed, 109 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..0c30dba68f
--- /dev/null
+++ b/xen/arch/arm/guest_walk.c
@@ -0,0 +1,89 @@
+/*
+ * 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;
+
+    /* 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;
+    *perms = GV2M_READ;
+
+    /* 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) && !(tcr & TTBCR_EAE) )
+        return guest_walk_sd(v, gva, ipa, perms);
+    else
+        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.13.1


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

[PATCH v5 10/12] 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.

v5: Make use of the function vgic_access_guest_memory to read page table
    entries in guest memory.

    Invert the indeces of the arrays "offsets" and "masks" and simplify
    readability by using an appropriate macro for the entries.

    Remove remaining CONFIG_ARM_64 #ifdefs.

    Remove the use of the macros BITS_PER_WORD and BITS_PER_DOUBLE_WORD.

    Use GENMASK_ULL instead of manually creating complex masks to ease
    readability.

    Also, create a macro CHECK_BASE_SIZE which simply reduces the code
    size and simplifies readability.

    Make use of the newly introduced lpae_page macro in the if-statement
    to test for invalid/reserved mappings in the L3 PTE.

    Cosmetic fixes and additional comments.
---
 xen/arch/arm/guest_walk.c | 390 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 388 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index 0c30dba68f..1f41fcefe9 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -15,7 +15,9 @@
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <xen/domain_page.h>
 #include <xen/sched.h>
+#include <asm/guest_walk.h>
 
 /*
  * The function guest_walk_sd translates a given GVA into an IPA using the
@@ -33,6 +35,171 @@ 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 int get_ipa_output_size(struct domain *d, register_t tcr,
+                               unsigned int *output_size)
+{
+    unsigned int ips;
+
+    static const unsigned int 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 0;
+}
+
+/* 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. Please note that the TCR.TG0 and TCR.TG1
+ * encodings differ.
+ */
+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:
+            /*
+             * According to ARM DDI 0487B.a D7-2487, if the TCR_EL1.TG0 value
+             * is programmed to either a reserved value, or a size that has not
+             * been implemented, then the hardware will treat the field as if
+             * it has been programmed to an IMPLEMENTATION DEFINED choice.
+             *
+             * This implementation strongly follows the pseudo-code
+             * implementation from ARM DDI 0487B.a J1-5924 which suggests to
+             * fall back to 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:
+            /*
+             * According to ARM DDI 0487B.a D7-2486, if the TCR_EL1.TG1 value
+             * is programmed to either a reserved value, or a size that has not
+             * been implemented, then the hardware will treat the field as if
+             * it has been programmed to an IMPLEMENTATION DEFINED choice.
+             *
+             * This implementation strongly follows the pseudo-code
+             * implementation from ARM DDI 0487B.a J1-5924 which suggests to
+             * fall back to 4K by 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_ULL(55)) && (tcr & TCR_EL1_TBI1)) ||
+             (!(gva & BIT_ULL(55)) && (tcr & TCR_EL1_TBI0)) )
+            topbit = 55;
+        else
+            topbit = 63;
+    }
+
+    return topbit;
+}
+
+/* Make sure the base address does not exceed its configured size. */
+#define CHECK_BASE_SIZE(output_size, base)                                  \
+{                                                                           \
+    paddr_t mask = GENMASK_ULL((TCR_EL1_IPS_48_BIT_VAL - 1), output_size);  \
+    if ( output_size < TCR_EL1_IPS_48_BIT_VAL && (base & mask) )            \
+        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
@@ -43,10 +210,229 @@ static int guest_walk_ld(const struct vcpu *v,
                          vaddr_t gva, paddr_t *ipa,
                          unsigned int *perms)
 {
-    /* Not implemented yet. */
-    return -EFAULT;
+    int ret;
+    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, paddr;
+    lpae_t pte;
+    register_t tcr = READ_SYSREG(TCR_EL1);
+    struct domain *d = v->domain;
+
+#define OFFSETS(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] = {
+        OFFSETS(gva, 4K),
+        OFFSETS(gva, 16K),
+        OFFSETS(gva, 64K)
+    };
+
+#undef OFFSETS
+
+#define MASKS(gran)                         \
+{                                           \
+    zeroeth_size(gran) - 1,                 \
+    first_size(gran) - 1,                   \
+    second_size(gran) - 1,                  \
+    third_size(gran) - 1                    \
+}
+
+    static const paddr_t masks[3][4] = {
+        MASKS(4K),
+        MASKS(16K),
+        MASKS(64K)
+    };
+
+#undef MASKS
+
+    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_ULL(topbit)) == 0 )
+        {
+            input_size = 64 - t0_sz;
+
+            /* Get TTBR0 and configured page granularity. */
+            disabled = get_ttbr_and_gran_64bit(&ttbr, &gran, tcr, TTBR0_ACTIVE);
+        }
+        else
+        {
+            input_size = 64 - 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 = GENMASK_ULL(31, (32 - t0_sz));
+
+        if ( t0_sz == 0 || !(gva & mask) )
+        {
+            input_size = 32 - 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 = GENMASK_ULL(31, (32 - t1_sz));
+
+        if ( ((t1_sz == 0) && !ttbr) || (t1_sz && (gva & mask) == mask) )
+        {
+            input_size = 32 - 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 (ARM DDI 0487B.a J1-5924).
+     */
+    level = 4 - DIV_ROUND_UP((input_size - grainsizes[gran]), (grainsizes[gran] - 3));
+
+    /* Get the IPA output_size. */
+    ret = get_ipa_output_size(d, tcr, &output_size);
+    if ( ret )
+        return -EFAULT;
+
+    /* Make sure the base address does not exceed its configured size. */
+    CHECK_BASE_SIZE(output_size, ttbr);
+
+    /*
+     * Compute the base address of the first level translation table that is
+     * given by TTBRx_EL1 (ARM DDI 0487B.a D4-2024 and J1-5926).
+     */
+    mask = GENMASK_ULL(47, grainsizes[gran]);
+    paddr = (ttbr & mask);
+
+    for ( ; ; level++ )
+    {
+        /*
+         * Add offset given by the GVA to the translation table base address.
+         * Shift the offset by 3 as it is 8-byte aligned.
+         */
+        paddr |= offsets[gran][level] << 3;
+
+        /* Access the guest's memory to read only one PTE. */
+        ret = vgic_access_guest_memory(d, paddr, &pte, sizeof(lpae_t), false);
+        if ( ret )
+            return -EINVAL;
+
+        /* Make sure the base address does not exceed its configured size. */
+        CHECK_BASE_SIZE(output_size, pfn_to_paddr(pte.walk.base));
+
+        /*
+         * 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 is 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 */
+
+        /* Compute the base address of the next level translation table. */
+        mask = GENMASK_ULL(47, grainsizes[gran]);
+        paddr = pfn_to_paddr(pte.walk.base) & mask;
+    }
+
+    /*
+     * 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_page(pte, level)) )
+        return -EFAULT;
+
+    *ipa = pfn_to_paddr(pte.walk.base) | (gva & masks[gran][level]);
+
+    /*
+     * 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;
+}
+
+#undef CHECK_BASE_SIZE
+
 int guest_walk_tables(const struct vcpu *v, vaddr_t gva,
                       paddr_t *ipa, unsigned int *perms)
 {
-- 
2.13.1


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

[PATCH v5 11/12] 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.

v5: Make use of the function vgic_access_guest_memory read page table
    entries in guest memory. At the same time, eliminate the offsets
    array, as there is no need for an array. Instead, we apply the
    associated masks to compute the GVA offsets directly in the code.

    Use GENMASK to compute complex masks to ease code readability.

    Use the type uint32_t for the TTBR register.

    Make use of L2DESC_{SMALL|LARGE}_PAGE_SHIFT instead of
    PAGE_SHIFT_{4K|64K} macros.

    Remove {L1|L2}DESC_* defines from this commit.

    Add comments and cosmetic fixes.
---
 xen/arch/arm/guest_walk.c | 143 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 141 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index 1f41fcefe9..d2bbc28da4 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -18,6 +18,7 @@
 #include <xen/domain_page.h>
 #include <xen/sched.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,146 @@ static int guest_walk_sd(const struct vcpu *v,
                          vaddr_t gva, paddr_t *ipa,
                          unsigned int *perms)
 {
-    /* Not implemented yet. */
-    return -EFAULT;
+    int ret;
+    bool disabled = true;
+    uint32_t ttbr;
+    paddr_t mask, paddr;
+    short_desc_t pte;
+    register_t ttbcr = READ_SYSREG(TCR_EL1);
+    unsigned int level = 0, n = ttbcr & TTBCR_N_MASK;
+    struct domain *d = v->domain;
+
+    mask = GENMASK_ULL(31, (32 - n));
+
+    if ( n == 0 || !(gva & mask) )
+    {
+        /*
+         * Use TTBR0 for GVA to IPA translation.
+         *
+         * Note that on AArch32, the TTBR0_EL1 register is 32-bit wide.
+         * Nevertheless, we have to use the READ_SYSREG64 macro, as it is
+         * required for reading TTBR0_EL1.
+         */
+        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.
+         *
+         * Note that on AArch32, the TTBR1_EL1 register is 32-bit wide.
+         * Nevertheless, we have to use the READ_SYSREG64 macro, as it is
+         * required for reading TTBR0_EL1.
+         */
+        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 L1 descriptor for the initial lookup has the
+     * following format: [ttbr<31:14-n>:gva<31-n:20>:00] (ARM DDI 0487B.a
+     * J1-6003). Note that the following GPA computation already considers that
+     * the first level address translation might comprise up to four
+     * consecutive pages and does not need to be page-aligned if n > 2.
+     */
+    mask = GENMASK(31, (14 - n));
+    paddr = (ttbr & mask);
+
+    mask = GENMASK((31 - n), 20);
+    paddr |= (gva & mask) >> 18;
+
+    /* Access the guest's memory to read only one PTE. */
+    ret = vgic_access_guest_memory(d, paddr, &pte, sizeof(short_desc_t), false);
+    if ( ret )
+        return -EINVAL;
+
+    switch ( pte.walk.dt )
+    {
+    case L1DESC_INVALID:
+        return -EFAULT;
+
+    case L1DESC_PAGE_TABLE:
+        level++;
+
+        /*
+         * The address of the L2 descriptor has the following format:
+         * [l1desc<31:10>:gva<19:12>:00] (ARM DDI 0487B.aJ1-6004). Note that
+         * the following address computation already considers that the second
+         * level translation table does not need to be page aligned.
+         */
+        mask = GENMASK(19, 12);
+        paddr = (pte.walk.base << 10) | ((gva % mask) >> 10);
+
+        /* Access the guest's memory to read only one PTE. */
+        ret = vgic_access_guest_memory(d, paddr, &pte, sizeof(short_desc_t), false);
+        if ( ret )
+            return -EINVAL;
+
+        if ( pte.walk.dt == L2DESC_INVALID )
+            return -EFAULT;
+
+        if ( pte.pg.page ) /* Small page. */
+        {
+            mask = (1ULL << L2DESC_SMALL_PAGE_SHIFT) - 1;
+            *ipa = (pte.pg.base << L2DESC_SMALL_PAGE_SHIFT) | (gva & mask);
+
+            /* Set execute permissions associated with the small page. */
+            if ( !pte.pg.xn )
+                *perms |= GV2M_EXEC;
+        }
+        else /* Large page. */
+        {
+            mask = (1ULL << L2DESC_LARGE_PAGE_SHIFT) - 1;
+            *ipa = (pte.lpg.base << L2DESC_LARGE_PAGE_SHIFT) | (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;
 }
 
 /*
-- 
2.13.1


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

[PATCH v5 12/12] 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>
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.

v5: Move comment to ease code readability.
---
 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..a58611daed 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;
+    {
+        /*
+         * The software gva to ipa translation can still fail, e.g., if the gva
+         * is not mapped.
+         */
+        if ( guest_walk_tables(v, gva, &ipa, &perms) < 0 )
+            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.13.1


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

Re: [PATCH v5 02/12] arm/mem_access: Move PAGE_SHIFT_* macros to lib.h

[Jan Beulich]

>>> Sergej Proskurin <proskurin@sec.in.tum.de> 06/27/17 1:52 PM >>>
>The following commits introduce a software guest page table walk
>software implementation that supports varying guest page size
>granularities. This commit moves already existing PAGE_SHIFT_(4K|64K)
>and the new PAGE_SHIFT_16K defines to a common place in xen/lib.h as
>to allow the following commits to use the consolidated defines.

I don't think the PAGE_SHIFT_* should live far away from the other PAGE_*_*
macros derived from them. I'm also not convinced lib.h is a good place.

Jan


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

Re: [PATCH v5 02/12] arm/mem_access: Move PAGE_SHIFT_* macros to lib.h

[Sergej Proskurin]

Hi Jan,


On 06/27/2017 02:04 PM, Jan Beulich wrote:
>>>> Sergej Proskurin <proskurin@sec.in.tum.de> 06/27/17 1:52 PM >>>
>> The following commits introduce a software guest page table walk
>> software implementation that supports varying guest page size
>> granularities. This commit moves already existing PAGE_SHIFT_(4K|64K)
>> and the new PAGE_SHIFT_16K defines to a common place in xen/lib.h as
>> to allow the following commits to use the consolidated defines.
> I don't think the PAGE_SHIFT_* should live far away from the other PAGE_*_*
> macros derived from them. I'm also not convinced lib.h is a good place.

I agree. I can move related PAGE_*_* from xen/iommu.h together with
PAGE_SIZE_* macros into a common place. As we already define PAGE_*
macros in asm/config.h, I believe it would make sense to extend these by
the upper PAGE_*_* macros. What do you think?

If you believe asm/config.h is a good place for the upper macros, I
would move them and simultaneously (as a separate patch) remove the
IOMMU_PAGE_* defines from xen/iommu.h. AFAICT they act only as helpers
and are not used elsewhere. Instead, I could add similar helpers in
asm/config.h. S.th. like the following:

---

#define PAGE_SIZE_GRAN(sz)          (1UL << PAGE_SHIFT_##sz)
#define PAGE_MASK_GRAN(sz)          (~(PAGE_SIZE_##sz - 1))
#define PAGE_ALIGN_GRAN(sz, addr)   (((addr) + ~PAGE_MASK_##sz) &
PAGE_MASK_##sz)

#define PAGE_SHIFT_4K               (12)
#define PAGE_SIZE_4K                PAGE_SIZE_GRAN(4K)
#define PAGE_MASK_4K                PAGE_MASK_GRAN(4K)
#define PAGE_ALIGN_4K(addr)         PAGE_ALIGN_GRAN(4K, addr)

#define PAGE_SHIFT_16K              (14)
#define PAGE_SIZE_16K               PAGE_SIZE_GRAN(16K)
#define PAGE_MASK_16K               PAGE_MASK_GRAN(16K)
#define PAGE_ALIGN_16K(addr)        PAGE_ALIGN_GRAN(16K, addr)

#define PAGE_SHIFT_64K              (16)
#define PAGE_SIZE_64K               PAGE_SIZE_GRAN(64K)
#define PAGE_MASK_64K               PAGE_MASK_GRAN(64K)
#define PAGE_ALIGN_64K(addr)        PAGE_ALIGN_GRAN(64K, addr)

---

Thanks,
~Sergej


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

Re: [PATCH v5 02/12] arm/mem_access: Move PAGE_SHIFT_* macros to lib.h

[Sergej Proskurin]

Hi Jan,


On 07/03/2017 10:40 AM, Sergej Proskurin wrote:
> Hi Jan,
>
>
> On 06/27/2017 02:04 PM, Jan Beulich wrote:
>>>>> Sergej Proskurin <proskurin@sec.in.tum.de> 06/27/17 1:52 PM >>>
>>> The following commits introduce a software guest page table walk
>>> software implementation that supports varying guest page size
>>> granularities. This commit moves already existing PAGE_SHIFT_(4K|64K)
>>> and the new PAGE_SHIFT_16K defines to a common place in xen/lib.h as
>>> to allow the following commits to use the consolidated defines.
>> I don't think the PAGE_SHIFT_* should live far away from the other PAGE_*_*
>> macros derived from them. I'm also not convinced lib.h is a good place.
> I agree. I can move related PAGE_*_* from xen/iommu.h together with
> PAGE_SIZE_* macros into a common place. As we already define PAGE_*
> macros in asm/config.h, I believe it would make sense to extend these by
> the upper PAGE_*_* macros. What do you think?
>
> If you believe asm/config.h is a good place for the upper macros, I
> would move them and simultaneously (as a separate patch) remove the
> IOMMU_PAGE_* defines from xen/iommu.h. AFAICT they act only as helpers
> and are not used elsewhere. Instead, I could add similar helpers in
> asm/config.h. S.th. like the following:
>
> ---
>
> #define PAGE_SIZE_GRAN(sz)          (1UL << PAGE_SHIFT_##sz)
> #define PAGE_MASK_GRAN(sz)          (~(PAGE_SIZE_##sz - 1))

To prevent potential type width issues with ARMv7, I would reuse the
macro from xen/iommu.h at this point:

PAGE_MASK_GRAN(sz)                     (~(u64)0 << PAGE_SHIFT_##sz)

> #define PAGE_ALIGN_GRAN(sz, addr)   (((addr) + ~PAGE_MASK_##sz) &
> PAGE_MASK_##sz)
>
> #define PAGE_SHIFT_4K               (12)
> #define PAGE_SIZE_4K                PAGE_SIZE_GRAN(4K)
> #define PAGE_MASK_4K                PAGE_MASK_GRAN(4K)
> #define PAGE_ALIGN_4K(addr)         PAGE_ALIGN_GRAN(4K, addr)
>
> #define PAGE_SHIFT_16K              (14)
> #define PAGE_SIZE_16K               PAGE_SIZE_GRAN(16K)
> #define PAGE_MASK_16K               PAGE_MASK_GRAN(16K)
> #define PAGE_ALIGN_16K(addr)        PAGE_ALIGN_GRAN(16K, addr)
>
> #define PAGE_SHIFT_64K              (16)
> #define PAGE_SIZE_64K               PAGE_SIZE_GRAN(64K)
> #define PAGE_MASK_64K               PAGE_MASK_GRAN(64K)
> #define PAGE_ALIGN_64K(addr)        PAGE_ALIGN_GRAN(64K, addr)
>
> ---
>
> Thanks,
> ~Sergej
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

Thanks,
~Sergej


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

Re: [PATCH v5 02/12] arm/mem_access: Move PAGE_SHIFT_* macros to lib.h

[Jan Beulich]

>>> On 03.07.17 at 10:40, <proskurin@sec.in.tum.de> wrote:
> On 06/27/2017 02:04 PM, Jan Beulich wrote:
>>>>> Sergej Proskurin <proskurin@sec.in.tum.de> 06/27/17 1:52 PM >>>
>>> The following commits introduce a software guest page table walk
>>> software implementation that supports varying guest page size
>>> granularities. This commit moves already existing PAGE_SHIFT_(4K|64K)
>>> and the new PAGE_SHIFT_16K defines to a common place in xen/lib.h as
>>> to allow the following commits to use the consolidated defines.
>> I don't think the PAGE_SHIFT_* should live far away from the other PAGE_*_*
>> macros derived from them. I'm also not convinced lib.h is a good place.
> 
> I agree. I can move related PAGE_*_* from xen/iommu.h together with
> PAGE_SIZE_* macros into a common place. As we already define PAGE_*
> macros in asm/config.h, I believe it would make sense to extend these by
> the upper PAGE_*_* macros. What do you think?
> 
> If you believe asm/config.h is a good place for the upper macros,

I don't, no: config.h should represent settings only, while here you
define constants which aren't necessarily properties of the system
the hypervisor is being built for.

Jan


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

Re: [PATCH v5 02/12] arm/mem_access: Move PAGE_SHIFT_* macros to lib.h

[Jan Beulich]

>>> On 03.07.17 at 11:03, <proskurin@sec.in.tum.de> wrote:
> To prevent potential type width issues with ARMv7, I would reuse the
> macro from xen/iommu.h at this point:
> 
> PAGE_MASK_GRAN(sz)                     (~(u64)0 << PAGE_SHIFT_##sz)

Seems reasonable, except
- no new use of u64 please (use uint64_t instead)
- it's questionable whether a 64-bit type here is correct in the
  first place, especially when considering 32-bit architectures

Jan


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

Re: [PATCH v5 02/12] arm/mem_access: Move PAGE_SHIFT_* macros to lib.h

[Sergej Proskurin]

Hi Jan,


On 07/03/2017 11:16 AM, Jan Beulich wrote:
>>>> On 03.07.17 at 11:03, <proskurin@sec.in.tum.de> wrote:
>> To prevent potential type width issues with ARMv7, I would reuse the
>> macro from xen/iommu.h at this point:
>>
>> PAGE_MASK_GRAN(sz)                     (~(u64)0 << PAGE_SHIFT_##sz)
> Seems reasonable, except
> - no new use of u64 please (use uint64_t instead)
> - it's questionable whether a 64-bit type here is correct in the
>   first place, especially when considering 32-bit architectures

As to prevent breaking the currently available implementation making use
of the existing defines in xen/iommu.h, I believe the PAGE_MASK_GRAN(sz)
macro should keep the type of uint64_t. Additonally, the introduced
PAGE_*_GRAN(sz) macros might be applied in the context of the
long-descriptor translation table format with 64bit PTEs for both ARMv8
and ARMv7. Having an unsigned long at this point would limit the mask to
32 bit on ARMv7 and thus not be appropriate in handling the
long-descriptor format on ARMv7. Please let me know if you still think
that using the uint64_t at this point is still questionable.

Apart from that, I discovered that there is no code that currently uses
PAGE_{SIZE,MASK.ALIGN}_64*K. The only 64-bit related macro in use is
PAGE_SHIFT_64K. So I wonder whether we need these defines after all (the
same would apply to 16K as well)?

Thanks you,
~Sergej


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

Re: [PATCH v5 02/12] arm/mem_access: Move PAGE_SHIFT_* macros to lib.h

[Sergej Proskurin]

Hi Jan,


On 07/03/2017 11:13 AM, Jan Beulich wrote:
>>>> On 03.07.17 at 10:40, <proskurin@sec.in.tum.de> wrote:
>> On 06/27/2017 02:04 PM, Jan Beulich wrote:
>>>>>> Sergej Proskurin <proskurin@sec.in.tum.de> 06/27/17 1:52 PM >>>
>>>> The following commits introduce a software guest page table walk
>>>> software implementation that supports varying guest page size
>>>> granularities. This commit moves already existing PAGE_SHIFT_(4K|64K)
>>>> and the new PAGE_SHIFT_16K defines to a common place in xen/lib.h as
>>>> to allow the following commits to use the consolidated defines.
>>> I don't think the PAGE_SHIFT_* should live far away from the other PAGE_*_*
>>> macros derived from them. I'm also not convinced lib.h is a good place.
>> I agree. I can move related PAGE_*_* from xen/iommu.h together with
>> PAGE_SIZE_* macros into a common place. As we already define PAGE_*
>> macros in asm/config.h, I believe it would make sense to extend these by
>> the upper PAGE_*_* macros. What do you think?
>>
>> If you believe asm/config.h is a good place for the upper macros,
> I don't, no: config.h should represent settings only, while here you
> define constants which aren't necessarily properties of the system
> the hypervisor is being built for.
>

Right. Sorry, I additionally forgot that I took the macros away from a
header accessible to other architectures as well.. So asm/config.h is
definitely not the right choice. Alternatively, I thought of
xen/paging.h, however this would generate a cyclic dependency in mm.h.
What do you think about using a new header xen/page.h instead? Unless
you have a better suggestion. Thank you very much in advance.

Cheers,
~Sergej


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

Re: [PATCH v5 02/12] arm/mem_access: Move PAGE_SHIFT_* macros to lib.h

[Jan Beulich]

>>> On 03.07.17 at 15:07, <proskurin@sec.in.tum.de> wrote:
> On 07/03/2017 11:16 AM, Jan Beulich wrote:
>>>>> On 03.07.17 at 11:03, <proskurin@sec.in.tum.de> wrote:
>>> To prevent potential type width issues with ARMv7, I would reuse the
>>> macro from xen/iommu.h at this point:
>>>
>>> PAGE_MASK_GRAN(sz)                     (~(u64)0 << PAGE_SHIFT_##sz)
>> Seems reasonable, except
>> - no new use of u64 please (use uint64_t instead)
>> - it's questionable whether a 64-bit type here is correct in the
>>   first place, especially when considering 32-bit architectures
> 
> As to prevent breaking the currently available implementation making use
> of the existing defines in xen/iommu.h, I believe the PAGE_MASK_GRAN(sz)
> macro should keep the type of uint64_t. Additonally, the introduced
> PAGE_*_GRAN(sz) macros might be applied in the context of the
> long-descriptor translation table format with 64bit PTEs for both ARMv8
> and ARMv7. Having an unsigned long at this point would limit the mask to
> 32 bit on ARMv7 and thus not be appropriate in handling the
> long-descriptor format on ARMv7. Please let me know if you still think
> that using the uint64_t at this point is still questionable.

Yes, I still view it as questionable, as much depends on what contexts
these macros are meant to be used in. But please realize, questionable
!= wrong.

> Apart from that, I discovered that there is no code that currently uses
> PAGE_{SIZE,MASK.ALIGN}_64*K. The only 64-bit related macro in use is
> PAGE_SHIFT_64K. So I wonder whether we need these defines after all (the
> same would apply to 16K as well)?

Yes, I think they should be added / removed / kept in full sets.

Jan


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

Re: [PATCH v5 02/12] arm/mem_access: Move PAGE_SHIFT_* macros to lib.h

[Jan Beulich]

>>> On 03.07.17 at 15:17, <proskurin@sec.in.tum.de> wrote:
> On 07/03/2017 11:13 AM, Jan Beulich wrote:
>>>>> On 03.07.17 at 10:40, <proskurin@sec.in.tum.de> wrote:
>>> On 06/27/2017 02:04 PM, Jan Beulich wrote:
>>>>>>> Sergej Proskurin <proskurin@sec.in.tum.de> 06/27/17 1:52 PM >>>
>>>>> The following commits introduce a software guest page table walk
>>>>> software implementation that supports varying guest page size
>>>>> granularities. This commit moves already existing PAGE_SHIFT_(4K|64K)
>>>>> and the new PAGE_SHIFT_16K defines to a common place in xen/lib.h as
>>>>> to allow the following commits to use the consolidated defines.
>>>> I don't think the PAGE_SHIFT_* should live far away from the other PAGE_*_*
>>>> macros derived from them. I'm also not convinced lib.h is a good place.
>>> I agree. I can move related PAGE_*_* from xen/iommu.h together with
>>> PAGE_SIZE_* macros into a common place. As we already define PAGE_*
>>> macros in asm/config.h, I believe it would make sense to extend these by
>>> the upper PAGE_*_* macros. What do you think?
>>>
>>> If you believe asm/config.h is a good place for the upper macros,
>> I don't, no: config.h should represent settings only, while here you
>> define constants which aren't necessarily properties of the system
>> the hypervisor is being built for.
>>
> 
> Right. Sorry, I additionally forgot that I took the macros away from a
> header accessible to other architectures as well.. So asm/config.h is
> definitely not the right choice. Alternatively, I thought of
> xen/paging.h, however this would generate a cyclic dependency in mm.h.
> What do you think about using a new header xen/page.h instead?

xen/page-sizes.h or xen/page-defs.h? I don't like page.h very much
because the per-arch headers of that name have a certain purpose
already, and as that's connected to actual page sizes in use on a
system I wouldn't want to see other page sizes to appear in a
header with that name. But if others think page.h is a good name, I
could live with it.

Jan


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

Re: [PATCH v5 01/12] arm/mem_access: Add and cleanup (TCR_|TTBCR_)* defines

[Julien Grall]

Hi Sergej,

On 06/27/2017 12:52 PM, Sergej Proskurin wrote:
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 855ded1b07..3dd439de33 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)

NIT D7-2380 is not a section but a page.

> + * comprises 6 bits and TTBCR.{T0SZ,T1SZ} (AArch32, Section G6-5204) comprises

Ditto.

With that:

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 v5 03/12] arm/mem_access: Add defines supporting PTs with varying page sizes

[Julien Grall]

Hi Sergej,

On 06/27/2017 12:52 PM, Sergej Proskurin wrote:
> The ARMv8 architecture supports pages with different (4K, 16K, and 64K) sizes.
> To enable guest page table walks for various configurations, this commit
> extends the defines and helpers of the current implementation.
> 
> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
> v3: Eliminate redundant macro definitions by introducing generic macros.
> 
> 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
> 
> v5: Remove PAGE_SHIFT_* defines from lpae.h as we import them now from
>      the header xen/lib.h.
> 
>      Remove *_guest_table_offset macros as to reduce the number of
>      exported macros which are only used once. Instead, use the
>      associated functionality directly within the
>      GUEST_TABLE_OFFSET_HELPERS.
> 
>      Add comment in GUEST_TABLE_OFFSET_HELPERS stating that a page table
>      with 64K page size granularity does not have a zeroeth lookup level.
> 
>      Add #undefs for GUEST_TABLE_OFFSET and GUEST_TABLE_OFFSET_HELPERS.
> 
>      Remove CONFIG_ARM_64 #defines.
> ---
>   xen/include/asm-arm/lpae.h | 62 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 62 insertions(+)
> 
> diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
> index 6fbf7c606c..2f7891ed0b 100644
> --- a/xen/include/asm-arm/lpae.h
> +++ b/xen/include/asm-arm/lpae.h
> @@ -3,6 +3,8 @@
>   
>   #ifndef __ASSEMBLY__
>   
> +#include <xen/lib.h>
> +
>   /*
>    * WARNING!  Unlike the x86 pagetable code, where l1 is the lowest level and
>    * l4 is the root of the trie, the ARM pagetables follow ARM's documentation:
> @@ -151,6 +153,66 @@ 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 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 GUEST_TABLE_OFFSET_HELPERS(gran)                                                \
> +static inline vaddr_t third_guest_table_offset_##gran##K(vaddr_t gva)                   \

Sorry I haven't spot it before. This is not going to work properly on 
32-bit if you use vaddr_t. Indeed, input for stage-2 page-table (i.e 
IPA) will be 40-bit. But vaddr_t is 32-bit. So you to use paddr_t here 
and in all the helpers below.

> +{                                                                                       \
> +    return GUEST_TABLE_OFFSET((gva >> third_shift(gran##K)), gran##K);                  \
> +}                                                                                       \
> +                                                                                        \
> +static inline vaddr_t second_guest_table_offset_##gran##K(vaddr_t gva)                  \
> +{                                                                                       \
> +    return GUEST_TABLE_OFFSET((gva >> second_shift(gran##K)), gran##K);                 \
> +}                                                                                       \
> +                                                                                        \
> +static inline vaddr_t first_guest_table_offset_##gran##K(vaddr_t gva)                   \
> +{                                                                                       \
> +    return GUEST_TABLE_OFFSET(((paddr_t)gva >> first_shift(gran##K)), gran##K);         \
> +}                                                                                       \
> +                                                                                        \
> +static inline vaddr_t zeroeth_guest_table_offset_##gran##K(vaddr_t gva)                 \
> +{                                                                                       \
> +    /* Note that there is no zeroeth lookup level with a 64K granule size. */           \
> +    if ( gran == 64 )                                                                   \
> +        return 0;                                                                       \
> +    else                                                                                \
> +        return GUEST_TABLE_OFFSET(((paddr_t)gva >> zeroeth_shift(gran##K)), gran##K);   \
> +}                                                                                       \
> +
> +GUEST_TABLE_OFFSET_HELPERS(4);
> +GUEST_TABLE_OFFSET_HELPERS(16);
> +GUEST_TABLE_OFFSET_HELPERS(64);
> +
> +#undef GUEST_TABLE_OFFSET
> +#undef GUEST_TABLE_OFFSET_HELPERS
> +
>   #endif /* __ASSEMBLY__ */
>   
>   /*
> 

-- 
Julien Grall

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

Re: [PATCH v5 04/12] arm/lpae: Introduce lpae_page helper

[Julien Grall]

Hi Sergej,

On 06/27/2017 12:52 PM, Sergej Proskurin wrote:
> This commit introduces a new helper that checks whether the target PTE
> holds a page mapping or not. This helper will be used as part of the
> following commits.
> 
> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>

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

Cheers,

> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
>   xen/include/asm-arm/lpae.h | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
> index 2f7891ed0b..20565d2c8a 100644
> --- a/xen/include/asm-arm/lpae.h
> +++ b/xen/include/asm-arm/lpae.h
> @@ -153,6 +153,11 @@ static inline bool lpae_is_superpage(lpae_t pte, unsigned int level)
>       return (level < 3) && lpae_mapping(pte);
>   }
>   
> +static inline bool lpae_page(lpae_t pte, unsigned int level)
> +{
> +    return (level == 3) && lpae_valid(pte) && pte.walk.table;
> +}
> +
>   /*
>    * The ARMv8 architecture supports pages with different sizes (4K, 16K, and
>    * 64K). To enable guest page table walks for various configurations, the
> 

-- 
Julien Grall

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

Re: [PATCH v5 05/12] arm/mem_access: Add short-descriptor pte typedefs and macros

[Julien Grall]

Hi Sergej,

On 06/27/2017 12:52 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,

> ---
> 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_*.
> 
> v5: Add {L1|L2}DESC_* defines to this commit.
> ---
>   xen/include/asm-arm/short-desc.h | 130 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 130 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..9652a103c4
> --- /dev/null
> +++ b/xen/include/asm-arm/short-desc.h
> @@ -0,0 +1,130 @@
> +#ifndef __ARM_SHORT_DESC_H__
> +#define __ARM_SHORT_DESC_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)
> +
> +/* Defines for small (4K) and large page (64K) shifts. */
> +#define L2DESC_SMALL_PAGE_SHIFT             (12)
> +#define L2DESC_LARGE_PAGE_SHIFT             (16)
> +
> +/*
> + * 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__ */
> 

-- 
Julien Grall

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

Re: [PATCH v5 07/12] arm/mem_access: Introduce BIT_ULL bit operation

[Julien Grall]

Hi Sergej,

On 06/27/2017 12:52 PM, Sergej Proskurin wrote:
> We introduce the BIT_ULL 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>

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

Cheers,

> ---
> 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.
> 
> v5: Introduce a new macro BIT_ULL instead of changing the type of the
>      macro BIT.
> 
>      Remove the define BITS_PER_DOUBLE_WORD.
> ---
>   xen/include/asm-arm/bitops.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/xen/include/asm-arm/bitops.h b/xen/include/asm-arm/bitops.h
> index bda889841b..1cbfb9edb2 100644
> --- a/xen/include/asm-arm/bitops.h
> +++ b/xen/include/asm-arm/bitops.h
> @@ -24,6 +24,7 @@
>   #define BIT(nr)                 (1UL << (nr))
>   #define BIT_MASK(nr)            (1UL << ((nr) % BITS_PER_WORD))
>   #define BIT_WORD(nr)            ((nr) / BITS_PER_WORD)
> +#define BIT_ULL(nr)             (1ULL << (nr))
>   #define BITS_PER_BYTE           8
>   
>   #define ADDR (*(volatile int *) addr)
> 

-- 
Julien Grall

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

Re: [PATCH v5 08/12] arm/mem_access: Introduce GENMASK_ULL bit operation

[Julien Grall]

Hi Sergej,

On 06/27/2017 12:52 PM, Sergej Proskurin wrote:
> The current implementation of GENMASK is capable of creating bitmasks of
> 32-bit values on AArch32 and 64-bit values on AArch64. As we need to
> create masks for 64-bit values on AArch32 as well, in this commit we
> introduce the GENMASK_ULL bit operation.
> 
> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> > ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
>   xen/include/asm-arm/config.h | 2 ++ >   xen/include/xen/bitops.h     | 2 ++

This is common code and the relevant maintainers should have been CCed.

This is not the first time we are trying to introduce GENMASK_ULL. I 
would recommend you to read the following discussion:

https://patchwork.kernel.org/patch/9665869/

Cheers,

>   2 files changed, 4 insertions(+)
> 
> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index 5b6f3c985d..7fa412f1b1 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -19,6 +19,8 @@
>   #define BITS_PER_LONG (BYTES_PER_LONG << 3)
>   #define POINTER_ALIGN BYTES_PER_LONG
>   
> +#define BITS_PER_LONG_LONG 64
> +
>   /* xen_ulong_t is always 64 bits */
>   #define BITS_PER_XEN_ULONG 64
>   
> diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
> index bd0883ab22..47170c9bfd 100644
> --- a/xen/include/xen/bitops.h
> +++ b/xen/include/xen/bitops.h
> @@ -9,6 +9,8 @@
>    */
>   #define GENMASK(h, l) \
>       (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
> +#define GENMASK_ULL(h, l) \
> +    (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
>   
>   /*
>    * ffs: find first bit set. This is defined the same way as
> 

-- 
Julien Grall

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

Re: [PATCH v5 09/12] arm/mem_access: Add software guest-page-table walk

[Julien Grall]

Hi Sergej,

On 06/27/2017 12:52 PM, Sergej Proskurin wrote:
> The function p2m_mem_access_check_and_get_page in mem_access.c
> translates a gva to an ipa by means of the hardware functionality of the
> ARM architecture. This is implemented in the function gva_to_ipa. If
> mem_access is active, hardware-based gva to ipa translation might fail,
> as gva_to_ipa uses the guest's translation tables, access to which might
> be restricted by the active VTTBR. To address this issue, in this commit
> we add a software-based guest-page-table walk, which will be used by the
> function p2m_mem_access_check_and_get_page perform the gva to ipa
> translation in software in one of the following commits.
> 
> Note: The introduced function guest_walk_tables assumes that the domain,
> the gva of which is to be translated, is running on the currently active
> vCPU. To walk the guest's page tables on a different vCPU, the following
> registers would need to be loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and
> SCTLR_EL1.
> 
> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
> Acked-by: Julien Grall <julien.grall@arm.com>
> ---
> 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.
> 
> v5: Merge two if-statements in guest_walk_tables to ease readability.
> 
>      Set perms to GV2M_READ as to avoid undefined permissions.

I would appreciate if you mention that you kept my tag even with the 
change made and asked whether I am happy with it...

In this case, you need a bit more rationale to explain why setting to 
GV2M_READ by default is fine... If it is just a random value, then say it.

Cheers.

-- 
Julien Grall

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

Re: [PATCH v5 10/12] arm/mem_access: Add long-descriptor based gpt

[Julien Grall]

Hi Sergej,

On 06/27/2017 12:52 PM, Sergej Proskurin wrote:
>      Also, create a macro CHECK_BASE_SIZE which simply reduces the code
>      size and simplifies readability.

No, it makes more confusing because you have the return within the 
macro. It is not that bad too have an helper checking the base and do

if ( check_base_size(....) )
   return -EINVAL;

[...]

> +/* Make sure the base address does not exceed its configured size. */
> +#define CHECK_BASE_SIZE(output_size, base)                                  \
> +{                                                                           \
> +    paddr_t mask = GENMASK_ULL((TCR_EL1_IPS_48_BIT_VAL - 1), output_size);  \
> +    if ( output_size < TCR_EL1_IPS_48_BIT_VAL && (base & mask) )            \
> +        return -EFAULT;                                                     \
> +}

See my comment in the changelog about this macro.

[...]

> +    for ( ; ; level++ )
> +    {
> +        /*
> +         * Add offset given by the GVA to the translation table base address.
> +         * Shift the offset by 3 as it is 8-byte aligned.
> +         */
> +        paddr |= offsets[gran][level] << 3;
> +
> +        /* Access the guest's memory to read only one PTE. */
> +        ret = vgic_access_guest_memory(d, paddr, &pte, sizeof(lpae_t), false);

It really doesn't make sense to call a function vgic_* in guest page 
table walk code. I wasn't expected that I needed to explicitly say that 
vgic_access_* should be moved in ARM generic code and be renamed.

[...]

> +    /*
> +     * 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_page(pte, level)) )

NIT: What you want to check here is either the entry is a superpage or a 
page. So the below check would be easier to parse:

if ( !lpae_is_superpage(pte, level) || !lpae_is_page(pte, level) )

> +        return -EFAULT;
> +
> +    *ipa = pfn_to_paddr(pte.walk.base) | (gva & masks[gran][level]);
> +
> +    /*
> +     * 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;
> +}
> +
> +#undef CHECK_BASE_SIZE
> +
>   int guest_walk_tables(const struct vcpu *v, vaddr_t gva,
>                         paddr_t *ipa, unsigned int *perms)
>   {
> 

Cheers,

-- 
Julien Grall

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

Re: [PATCH v5 09/12] arm/mem_access: Add software guest-page-table walk

[Sergej Proskurin]

Hi Julien,

On 07/04/2017 06:58 PM, Julien Grall wrote:
> Hi Sergej,
> 
> On 06/27/2017 12:52 PM, Sergej Proskurin wrote:
>> The function p2m_mem_access_check_and_get_page in mem_access.c
>> translates a gva to an ipa by means of the hardware functionality of the
>> ARM architecture. This is implemented in the function gva_to_ipa. If
>> mem_access is active, hardware-based gva to ipa translation might fail,
>> as gva_to_ipa uses the guest's translation tables, access to which might
>> be restricted by the active VTTBR. To address this issue, in this commit
>> we add a software-based guest-page-table walk, which will be used by the
>> function p2m_mem_access_check_and_get_page perform the gva to ipa
>> translation in software in one of the following commits.
>>
>> Note: The introduced function guest_walk_tables assumes that the domain,
>> the gva of which is to be translated, is running on the currently active
>> vCPU. To walk the guest's page tables on a different vCPU, the following
>> registers would need to be loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and
>> SCTLR_EL1.
>>
>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>> Acked-by: Julien Grall <julien.grall@arm.com>
>> ---
>> 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.
>>
>> v5: Merge two if-statements in guest_walk_tables to ease readability.
>>
>>      Set perms to GV2M_READ as to avoid undefined permissions.
> 
> I would appreciate if you mention that you kept my tag even with the
> change made and asked whether I am happy with it...
> 

Sorry for not mentioning that. Since it was your suggestion right after
you have acked this patch, I thought it would be sufficient to just
state the changes. I will consider that in the future.

> In this case, you need a bit more rationale to explain why setting to
> GV2M_READ by default is fine... If it is just a random value, then say it.
> 

I will add a comment in the change log stating that we set perms to
GV2M_READ (that equals to 0) by default to avoid having potentially
random values returned on errors.

Shall I remove your ack for this patch in the next version?

Thanks,
~Sergej

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

Re: [PATCH v5 08/12] arm/mem_access: Introduce GENMASK_ULL bit operation

[Sergej Proskurin]

Hi Julien,

On 07/04/2017 06:28 PM, Julien Grall wrote:
> Hi Sergej,
> 
> On 06/27/2017 12:52 PM, Sergej Proskurin wrote:
>> The current implementation of GENMASK is capable of creating bitmasks of
>> 32-bit values on AArch32 and 64-bit values on AArch64. As we need to
>> create masks for 64-bit values on AArch32 as well, in this commit we
>> introduce the GENMASK_ULL bit operation.
>>
>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> > ---
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/include/asm-arm/config.h | 2 ++ >   xen/include/xen/bitops.h    
>> | 2 ++
> 
> This is common code and the relevant maintainers should have been CCed.
> 

According to ./scripts/get_maintainer.pl Stefano Stabellini and you were
the only maintainers to put on Cc. I would appreciate it if you would
point out what I am missing. Thank you very much in advance.

> This is not the first time we are trying to introduce GENMASK_ULL. I
> would recommend you to read the following discussion:
> 
> https://patchwork.kernel.org/patch/9665869/
> 

Thank you.

Cheers,
~Sergej

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

Re: [PATCH v5 03/12] arm/mem_access: Add defines supporting PTs with varying page sizes

[Sergej Proskurin]

Hi Julien,

On 07/04/2017 06:15 PM, Julien Grall wrote:
> Hi Sergej,
> 

[...]

>> +
>> +#define GUEST_TABLE_OFFSET(offs, gran)          ((paddr_t)(offs) &
>> lpae_entry_mask(gran))
>> +#define
>> GUEST_TABLE_OFFSET_HELPERS(gran)                                               
>> \
>> +static inline vaddr_t third_guest_table_offset_##gran##K(vaddr_t
>> gva)                   \
> 
> Sorry I haven't spot it before. This is not going to work properly on
> 32-bit if you use vaddr_t. Indeed, input for stage-2 page-table (i.e
> IPA) will be 40-bit. But vaddr_t is 32-bit. So you to use paddr_t here
> and in all the helpers below.
> 

I agree that IPAs won't work properly on AArch32. However, we don't walk
the second stage translation tables with the introduced code (yet?). In
fact, second stage translation walks in software are not supported at
the moment. I understand why you would think in this direction, with
ARM's nested virtualization support coming up, where we might need to
walk the second stage translation tables in sw. Yet, with the current
implementation, we work on on GVAs (not IPAs) and hence the vaddr_t
should not present an issue (except that the now missing CONFIG_ARM_64
#ifdef's in the long-descriptor translation table walk create compile
issues as we need to support both different page granularities and
zeroeth-level offsets which work on gva's > 32bit on AArch64).

If you wish to see the implementation extended in the future to support
walking the 2nd stage address translation, then I will gladly change
vaddr_t to paddr_t.

Cheers,
~Sergej

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

Re: [PATCH v5 08/12] arm/mem_access: Introduce GENMASK_ULL bit operation

[Sergej Proskurin]

Hi Julien,

On 07/04/2017 10:46 PM, Sergej Proskurin wrote:
> Hi Julien,
> 
> On 07/04/2017 06:28 PM, Julien Grall wrote:
>> Hi Sergej,
>>
>> On 06/27/2017 12:52 PM, Sergej Proskurin wrote:
>>> The current implementation of GENMASK is capable of creating bitmasks of
>>> 32-bit values on AArch32 and 64-bit values on AArch64. As we need to
>>> create masks for 64-bit values on AArch32 as well, in this commit we
>>> introduce the GENMASK_ULL bit operation.
>>>
>>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> > ---
>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>> Cc: Julien Grall <julien.grall@arm.com>
>>> ---
>>>   xen/include/asm-arm/config.h | 2 ++ >   xen/include/xen/bitops.h    
>>> | 2 ++
>>
>> This is common code and the relevant maintainers should have been CCed.
>>
> 
> According to ./scripts/get_maintainer.pl Stefano Stabellini and you were
> the only maintainers to put on Cc. I would appreciate it if you would
> point out what I am missing. Thank you very much in advance.
> 
>> This is not the first time we are trying to introduce GENMASK_ULL. I
>> would recommend you to read the following discussion:
>>
>> https://patchwork.kernel.org/patch/9665869/
>>
> 

While I agree with you that GENMASK_ULL reduces potential mistakes by
generating ULL masks manually, I don't think that we have more arguments
for introducing GENMASK_ULL this time than the last two times..

So, anyway, I will gladly put the other maintainers on Cc and retry the
submission one more time (it would be great if you would provide me with
the list of the respective maintainers, as ./scripts/get_maintainer.pl
apparently did not involve all parties). Thank you.

Cheers,
~Sergej

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

Re: [PATCH v5 03/12] arm/mem_access: Add defines supporting PTs with varying page sizes

[Julien Grall]

On 04/07/17 22:33, Sergej Proskurin wrote:
> Hi Julien,

Hi Sergej,

> On 07/04/2017 06:15 PM, Julien Grall wrote:
>> Hi Sergej,
>>
>
> [...]
>
>>> +
>>> +#define GUEST_TABLE_OFFSET(offs, gran)          ((paddr_t)(offs) &
>>> lpae_entry_mask(gran))
>>> +#define
>>> GUEST_TABLE_OFFSET_HELPERS(gran)
>>> \
>>> +static inline vaddr_t third_guest_table_offset_##gran##K(vaddr_t
>>> gva)                   \
>>
>> Sorry I haven't spot it before. This is not going to work properly on
>> 32-bit if you use vaddr_t. Indeed, input for stage-2 page-table (i.e
>> IPA) will be 40-bit. But vaddr_t is 32-bit. So you to use paddr_t here
>> and in all the helpers below.
>>
>
> I agree that IPAs won't work properly on AArch32. However, we don't walk
> the second stage translation tables with the introduced code (yet?). In
> fact, second stage translation walks in software are not supported at
> the moment. I understand why you would think in this direction, with
> ARM's nested virtualization support coming up, where we might need to
> walk the second stage translation tables in sw. Yet, with the current
> implementation, we work on on GVAs (not IPAs) and hence the vaddr_t
> should not present an issue (except that the now missing CONFIG_ARM_64
> #ifdef's in the long-descriptor translation table walk create compile
> issues as we need to support both different page granularities and
> zeroeth-level offsets which work on gva's > 32bit on AArch64).
>
> If you wish to see the implementation extended in the future to support
> walking the 2nd stage address translation, then I will gladly change
> vaddr_t to paddr_t.

Rather than justifying with: "We don't use like that today, so it is 
fine to keep the bug", you should think: "How can it be used in the 
future?". And when I see a cast from vaddr_t to paddr_t in the code, 
then I can directly tell something is wrong.

We already have code to walk stage-2 page-table (see p2m_lookup) and are 
using similar macro a bit everywhere in the p2m code. I was actually 
thinking to make *_table_offset an alias to *_table_offset_4k because 
they are the same. Note I am not asking modify the p2m code...

Even though the stage-2 code does not use those helpers today, I don't 
see any valid reasons to keep a known latent bug in the code. It will 
likely be forgotten and I wish good luck of the developer who will have 
the issue.

BTW, I think you can drop "guest" in the name because those new helps 
are very generic.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v5 03/12] arm/mem_access: Add defines supporting PTs with varying page sizes

[Sergej Proskurin]

Hi Julien,


On 07/05/2017 01:41 PM, Julien Grall wrote:
>
>
> On 04/07/17 22:33, Sergej Proskurin wrote:
>> Hi Julien,
>
> Hi Sergej,
>
>> On 07/04/2017 06:15 PM, Julien Grall wrote:
>>> Hi Sergej,
>>>
>>
>> [...]
>>
>>>> +
>>>> +#define GUEST_TABLE_OFFSET(offs, gran)          ((paddr_t)(offs) &
>>>> lpae_entry_mask(gran))
>>>> +#define
>>>> GUEST_TABLE_OFFSET_HELPERS(gran)
>>>> \
>>>> +static inline vaddr_t third_guest_table_offset_##gran##K(vaddr_t
>>>> gva)                   \
>>>
>>> Sorry I haven't spot it before. This is not going to work properly on
>>> 32-bit if you use vaddr_t. Indeed, input for stage-2 page-table (i.e
>>> IPA) will be 40-bit. But vaddr_t is 32-bit. So you to use paddr_t here
>>> and in all the helpers below.
>>>
>>
>> I agree that IPAs won't work properly on AArch32. However, we don't walk
>> the second stage translation tables with the introduced code (yet?). In
>> fact, second stage translation walks in software are not supported at
>> the moment. I understand why you would think in this direction, with
>> ARM's nested virtualization support coming up, where we might need to
>> walk the second stage translation tables in sw. Yet, with the current
>> implementation, we work on on GVAs (not IPAs) and hence the vaddr_t
>> should not present an issue (except that the now missing CONFIG_ARM_64
>> #ifdef's in the long-descriptor translation table walk create compile
>> issues as we need to support both different page granularities and
>> zeroeth-level offsets which work on gva's > 32bit on AArch64).
>>
>> If you wish to see the implementation extended in the future to support
>> walking the 2nd stage address translation, then I will gladly change
>> vaddr_t to paddr_t.
>
> Rather than justifying with: "We don't use like that today, so it is
> fine to keep the bug", you should think: "How can it be used in the
> future?". And when I see a cast from vaddr_t to paddr_t in the code,
> then I can directly tell something is wrong.
>
> We already have code to walk stage-2 page-table (see p2m_lookup) and
> are using similar macro a bit everywhere in the p2m code. I was
> actually thinking to make *_table_offset an alias to *_table_offset_4k
> because they are the same. Note I am not asking modify the p2m code...
>
> Even though the stage-2 code does not use those helpers today, I don't
> see any valid reasons to keep a known latent bug in the code. It will
> likely be forgotten and I wish good luck of the developer who will
> have the issue.
>
> BTW, I think you can drop "guest" in the name because those new helps
> are very generic.

Please don't understand me wrong. I am absolutely up for the change and
simply wanted to understand the reason behind your suggestion. The type
change will be part of the next version (including the naming
suggestion). Thank you.

Cheers,
~Sergej



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

Re: [PATCH v5 09/12] arm/mem_access: Add software guest-page-table walk

[Julien Grall]

On 04/07/17 21:25, Sergej Proskurin wrote:
> Hi Julien,
>
> On 07/04/2017 06:58 PM, Julien Grall wrote:
>> Hi Sergej,
>>
>> On 06/27/2017 12:52 PM, Sergej Proskurin wrote:
>>> The function p2m_mem_access_check_and_get_page in mem_access.c
>>> translates a gva to an ipa by means of the hardware functionality of the
>>> ARM architecture. This is implemented in the function gva_to_ipa. If
>>> mem_access is active, hardware-based gva to ipa translation might fail,
>>> as gva_to_ipa uses the guest's translation tables, access to which might
>>> be restricted by the active VTTBR. To address this issue, in this commit
>>> we add a software-based guest-page-table walk, which will be used by the
>>> function p2m_mem_access_check_and_get_page perform the gva to ipa
>>> translation in software in one of the following commits.
>>>
>>> Note: The introduced function guest_walk_tables assumes that the domain,
>>> the gva of which is to be translated, is running on the currently active
>>> vCPU. To walk the guest's page tables on a different vCPU, the following
>>> registers would need to be loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and
>>> SCTLR_EL1.
>>>
>>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>>> Acked-by: Julien Grall <julien.grall@arm.com>
>>> ---
>>> 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.
>>>
>>> v5: Merge two if-statements in guest_walk_tables to ease readability.
>>>
>>>      Set perms to GV2M_READ as to avoid undefined permissions.
>>
>> I would appreciate if you mention that you kept my tag even with the
>> change made and asked whether I am happy with it...
>>
>
> Sorry for not mentioning that. Since it was your suggestion right after
> you have acked this patch, I thought it would be sufficient to just
> state the changes. I will consider that in the future.

I suggested to perms, I don't remember suggesting to use GVM_READ.

>
>> In this case, you need a bit more rationale to explain why setting to
>> GV2M_READ by default is fine... If it is just a random value, then say it.
>>
>
> I will add a comment in the change log stating that we set perms to
> GV2M_READ (that equals to 0) by default to avoid having potentially
> random values returned on errors.

Beware that you are re-using GV2M_READ for different purpose. In the 
current code it is used to check whether a guest VA is readable.

Here, you are using both GV2M_* to return the permissions of a mapping. 
What matters is whether it is fine to consider a page will always have 
the read permission.

At the moment, you consider you will always translate the VA -> IPA with 
EL1 permission (see D4-29 in ARM DDI 0487B.a). So the resulting 
permission in stage-1 will always contain read.

I am ok if you make this assumption, but this needs to be explained in a 
comment in the code. So we are not surprised in the future why you 
always set read permission.

>
> Shall I remove your ack for this patch in the next version?

Yes please.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v5 08/12] arm/mem_access: Introduce GENMASK_ULL bit operation

[Julien Grall]

Hi,

On 04/07/17 22:44, Sergej Proskurin wrote:
> Hi Julien,
>
> On 07/04/2017 10:46 PM, Sergej Proskurin wrote:
>> Hi Julien,
>>
>> On 07/04/2017 06:28 PM, Julien Grall wrote:
>>> Hi Sergej,
>>>
>>> On 06/27/2017 12:52 PM, Sergej Proskurin wrote:
>>>> The current implementation of GENMASK is capable of creating bitmasks of
>>>> 32-bit values on AArch32 and 64-bit values on AArch64. As we need to
>>>> create masks for 64-bit values on AArch32 as well, in this commit we
>>>> introduce the GENMASK_ULL bit operation.
>>>>
>>>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> > ---
>>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>>> Cc: Julien Grall <julien.grall@arm.com>
>>>> ---
>>>>   xen/include/asm-arm/config.h | 2 ++ >   xen/include/xen/bitops.h
>>>> | 2 ++
>>>
>>> This is common code and the relevant maintainers should have been CCed.
>>>
>>
>> According to ./scripts/get_maintainer.pl Stefano Stabellini and you were
>> the only maintainers to put on Cc. I would appreciate it if you would
>> point out what I am missing. Thank you very much in advance.
>>
>>> This is not the first time we are trying to introduce GENMASK_ULL. I
>>> would recommend you to read the following discussion:
>>>
>>> https://patchwork.kernel.org/patch/9665869/
>>>
>>
>
> While I agree with you that GENMASK_ULL reduces potential mistakes by
> generating ULL masks manually, I don't think that we have more arguments
> for introducing GENMASK_ULL this time than the last two times..

I do feel GENMASK_ULL is a nice things to have. I am wondering if we 
could put GENMASK_ULL in ARM code if common code does not want it. 
Stefano, any opinion?

> So, anyway, I will gladly put the other maintainers on Cc and retry the
> submission one more time (it would be great if you would provide me with
> the list of the respective maintainers, as ./scripts/get_maintainer.pl
> apparently did not involve all parties). Thank you.

You should avoid to rely blindly on scripts/get_maintainer.pl. It can 
get confused if you have patch modifying arch and common. You can look 
at MAINTAINERS to find the list of maintainers to CC.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v5 09/12] arm/mem_access: Add software guest-page-table walk

[Sergej Proskurin]

Hi Julien,


>
>>
>>> In this case, you need a bit more rationale to explain why setting to
>>> GV2M_READ by default is fine... If it is just a random value, then
>>> say it.
>>>
>>
>> I will add a comment in the change log stating that we set perms to
>> GV2M_READ (that equals to 0) by default to avoid having potentially
>> random values returned on errors.
>
> Beware that you are re-using GV2M_READ for different purpose. In the
> current code it is used to check whether a guest VA is readable.
>
> Here, you are using both GV2M_* to return the permissions of a
> mapping. What matters is whether it is fine to consider a page will
> always have the read permission.
>
> At the moment, you consider you will always translate the VA -> IPA
> with EL1 permission (see D4-29 in ARM DDI 0487B.a). So the resulting
> permission in stage-1 will always contain read.
>
> I am ok if you make this assumption, but this needs to be explained in
> a comment in the code. So we are not surprised in the future why you
> always set read permission.
>

Alright. I will state in a comment in code and change-log that since the
current implementation considers a valid mapping as readable at least by
EL1 because the current implementation does not consider further
attributes responsible for distinguishing between EL0 and EL1. Thank you.

>>
>> Shall I remove your ack for this patch in the next version?
>
> Yes please.
>
> Cheers,
>

Cheers,
~Sergej


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

Re: [PATCH v5 04/12] arm/lpae: Introduce lpae_page helper

[Sergej Proskurin]

Hi Julien,


On 07/04/2017 06:23 PM, Julien Grall wrote:
> Hi Sergej,
>
> On 06/27/2017 12:52 PM, Sergej Proskurin wrote:
>> This commit introduces a new helper that checks whether the target PTE
>> holds a page mapping or not. This helper will be used as part of the
>> following commits.
>>
>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>
> Reviewed-by: Julien Grall <julien.grall@arm.com>
>
> Cheers,
>
>> ---
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/include/asm-arm/lpae.h | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
>> index 2f7891ed0b..20565d2c8a 100644
>> --- a/xen/include/asm-arm/lpae.h
>> +++ b/xen/include/asm-arm/lpae.h
>> @@ -153,6 +153,11 @@ static inline bool lpae_is_superpage(lpae_t pte,
>> unsigned int level)
>>       return (level < 3) && lpae_mapping(pte);
>>   }
>>   +static inline bool lpae_page(lpae_t pte, unsigned int level)
>> +{
>> +    return (level == 3) && lpae_valid(pte) && pte.walk.table;
>> +}
>> +
>>   /*
>>    * The ARMv8 architecture supports pages with different sizes (4K,
>> 16K, and
>>    * 64K). To enable guest page table walks for various
>> configurations, the
>>
>

Would that be ok for you if I changed the name of the helper lpae_page
into lpae_is_page as to be conform with lpae_is_superpage or shall I
remove your Reviewed-by for doing this? Thanks.

Cheers,
~Sergej

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

Re: [PATCH v5 04/12] arm/lpae: Introduce lpae_page helper

[Julien Grall]

On 05/07/17 14:11, Sergej Proskurin wrote:
> Hi Julien,
>
>
> On 07/04/2017 06:23 PM, Julien Grall wrote:
>> Hi Sergej,
>>
>> On 06/27/2017 12:52 PM, Sergej Proskurin wrote:
>>> This commit introduces a new helper that checks whether the target PTE
>>> holds a page mapping or not. This helper will be used as part of the
>>> following commits.
>>>
>>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>>
>> Reviewed-by: Julien Grall <julien.grall@arm.com>
>>
>> Cheers,
>>
>>> ---
>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>> Cc: Julien Grall <julien.grall@arm.com>
>>> ---
>>>   xen/include/asm-arm/lpae.h | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
>>> index 2f7891ed0b..20565d2c8a 100644
>>> --- a/xen/include/asm-arm/lpae.h
>>> +++ b/xen/include/asm-arm/lpae.h
>>> @@ -153,6 +153,11 @@ static inline bool lpae_is_superpage(lpae_t pte,
>>> unsigned int level)
>>>       return (level < 3) && lpae_mapping(pte);
>>>   }
>>>   +static inline bool lpae_page(lpae_t pte, unsigned int level)
>>> +{
>>> +    return (level == 3) && lpae_valid(pte) && pte.walk.table;
>>> +}
>>> +
>>>   /*
>>>    * The ARMv8 architecture supports pages with different sizes (4K,
>>> 16K, and
>>>    * 64K). To enable guest page table walks for various
>>> configurations, the
>>>
>>
>
> Would that be ok for you if I changed the name of the helper lpae_page
> into lpae_is_page as to be conform with lpae_is_superpage or shall I
> remove your Reviewed-by for doing this? Thanks.

I am fine with that.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v5 11/12] arm/mem_access: Add short-descriptor based gpt

[Julien Grall]

Hi Sergej,

On 27/06/17 12:52, 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.
>
> v5: Make use of the function vgic_access_guest_memory read page table
>     entries in guest memory. At the same time, eliminate the offsets
>     array, as there is no need for an array. Instead, we apply the
>     associated masks to compute the GVA offsets directly in the code.
>
>     Use GENMASK to compute complex masks to ease code readability.
>
>     Use the type uint32_t for the TTBR register.
>
>     Make use of L2DESC_{SMALL|LARGE}_PAGE_SHIFT instead of
>     PAGE_SHIFT_{4K|64K} macros.
>
>     Remove {L1|L2}DESC_* defines from this commit.
>
>     Add comments and cosmetic fixes.
> ---
>  xen/arch/arm/guest_walk.c | 143 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 141 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index 1f41fcefe9..d2bbc28da4 100644
> --- a/xen/arch/arm/guest_walk.c
> +++ b/xen/arch/arm/guest_walk.c
> @@ -18,6 +18,7 @@
>  #include <xen/domain_page.h>
>  #include <xen/sched.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,146 @@ static int guest_walk_sd(const struct vcpu *v,
>                           vaddr_t gva, paddr_t *ipa,
>                           unsigned int *perms)
>  {
> -    /* Not implemented yet. */
> -    return -EFAULT;
> +    int ret;
> +    bool disabled = true;
> +    uint32_t ttbr;
> +    paddr_t mask, paddr;
> +    short_desc_t pte;
> +    register_t ttbcr = READ_SYSREG(TCR_EL1);
> +    unsigned int level = 0, n = ttbcr & TTBCR_N_MASK;
> +    struct domain *d = v->domain;
> +
> +    mask = GENMASK_ULL(31, (32 - n));
> +
> +    if ( n == 0 || !(gva & mask) )
> +    {
> +        /*
> +         * Use TTBR0 for GVA to IPA translation.
> +         *
> +         * Note that on AArch32, the TTBR0_EL1 register is 32-bit wide.
> +         * Nevertheless, we have to use the READ_SYSREG64 macro, as it is
> +         * required for reading TTBR0_EL1.
> +         */
> +        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.
> +         *
> +         * Note that on AArch32, the TTBR1_EL1 register is 32-bit wide.
> +         * Nevertheless, we have to use the READ_SYSREG64 macro, as it is
> +         * required for reading TTBR0_EL1.

NIT: s/TTBR0_EL1/TTBR1_EL1/

> +         */
> +        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 L1 descriptor for the initial lookup has the
> +     * following format: [ttbr<31:14-n>:gva<31-n:20>:00] (ARM DDI 0487B.a
> +     * J1-6003). Note that the following GPA computation already considers that
> +     * the first level address translation might comprise up to four
> +     * consecutive pages and does not need to be page-aligned if n > 2.
> +     */
> +    mask = GENMASK(31, (14 - n));
> +    paddr = (ttbr & mask);
> +
> +    mask = GENMASK((31 - n), 20);
> +    paddr |= (gva & mask) >> 18;
> +
> +    /* Access the guest's memory to read only one PTE. */
> +    ret = vgic_access_guest_memory(d, paddr, &pte, sizeof(short_desc_t), false);

See my comment on the LPAE code about vgic_access_guest_memory.

> +    if ( ret )
> +        return -EINVAL;
> +
> +    switch ( pte.walk.dt )
> +    {
> +    case L1DESC_INVALID:
> +        return -EFAULT;
> +
> +    case L1DESC_PAGE_TABLE:
> +        level++;

I am not sure what this variable is used for. You increment it but never 
use the value. Maybe a left-over of a previous patch?

> +
> +        /*
> +         * The address of the L2 descriptor has the following format:
> +         * [l1desc<31:10>:gva<19:12>:00] (ARM DDI 0487B.aJ1-6004). Note that
> +         * the following address computation already considers that the second
> +         * level translation table does not need to be page aligned.
> +         */
> +        mask = GENMASK(19, 12);
> +        paddr = (pte.walk.base << 10) | ((gva % mask) >> 10);

I don't understand the "gva % mask".

> +
> +        /* Access the guest's memory to read only one PTE. */
> +        ret = vgic_access_guest_memory(d, paddr, &pte, sizeof(short_desc_t), false);

Ditto for the vgic_access_guest_memory.

The rest looks good to me.

> +        if ( ret )
> +            return -EINVAL;
> +
> +        if ( pte.walk.dt == L2DESC_INVALID )
> +            return -EFAULT;
> +
> +        if ( pte.pg.page ) /* Small page. */
> +        {
> +            mask = (1ULL << L2DESC_SMALL_PAGE_SHIFT) - 1;
> +            *ipa = (pte.pg.base << L2DESC_SMALL_PAGE_SHIFT) | (gva & mask);
> +
> +            /* Set execute permissions associated with the small page. */
> +            if ( !pte.pg.xn )
> +                *perms |= GV2M_EXEC;
> +        }
> +        else /* Large page. */
> +        {
> +            mask = (1ULL << L2DESC_LARGE_PAGE_SHIFT) - 1;
> +            *ipa = (pte.lpg.base << L2DESC_LARGE_PAGE_SHIFT) | (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;
>  }
>
>  /*
>

-- 
Julien Grall

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

Re: [PATCH v5 10/12] arm/mem_access: Add long-descriptor based gpt

[Sergej Proskurin]

Hi Julien,


[...]

>> +        /* Access the guest's memory to read only one PTE. */
>> +        ret = vgic_access_guest_memory(d, paddr, &pte,
>> sizeof(lpae_t), false);
>
> It really doesn't make sense to call a function vgic_* in guest page
> table walk code. I wasn't expected that I needed to explicitly say
> that vgic_access_* should be moved in ARM generic code and be renamed.

Do you think it would make sense to put the vgic_access_guest_memory
functionality into arch/arm/guestcopy.c? Alternatively I could introduce
a new file, such as guest-memory-access.c.

The same question applies to exporting the functionality in
include/asm-arm/guest_access.h or would you prefer a new header file
(e.g. guest-memory-access.h)?

Concerning the function name, I believe guest_memory_access() would be
appropriate.

Cheers,
~Sergej


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

Re: [PATCH v5 10/12] arm/mem_access: Add long-descriptor based gpt

[Julien Grall]

On 05/07/17 15:37, Sergej Proskurin wrote:
> Hi Julien,

Hi Sergej,

>
> [...]
>
>>> +        /* Access the guest's memory to read only one PTE. */
>>> +        ret = vgic_access_guest_memory(d, paddr, &pte,
>>> sizeof(lpae_t), false);
>>
>> It really doesn't make sense to call a function vgic_* in guest page
>> table walk code. I wasn't expected that I needed to explicitly say
>> that vgic_access_* should be moved in ARM generic code and be renamed.
>
> Do you think it would make sense to put the vgic_access_guest_memory
> functionality into arch/arm/guestcopy.c? Alternatively I could introduce
> a new file, such as guest-memory-access.c.
>
> The same question applies to exporting the functionality in
> include/asm-arm/guest_access.h or would you prefer a new header file
> (e.g. guest-memory-access.h)?

guest_access.h is a good place. It deals with copy memory from/to the guest.

>
> Concerning the function name, I believe guest_memory_access() would be
> appropriate.

I would like to see the word "ipa" in the name to not confuse the 
raw_copy_* using va.

One suggestion would be access_guest_memory_by_ipa or something similar.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v5 10/12] arm/mem_access: Add long-descriptor based gpt

[Sergej Proskurin]

Hi Julien,


On 07/04/2017 07:06 PM, Julien Grall wrote:
> Hi Sergej,
>
>
>> +    /*
>> +     * 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_page(pte, level)) )
>
> NIT: What you want to check here is either the entry is a superpage or
> a page. So the below check would be easier to parse:
>
> if ( !lpae_is_superpage(pte, level) || !lpae_is_page(pte, level) )

Your suggested check is easier to parse, however introduces a bug
because as soon as the pte is not a superpage it will return an -EFAULT
at this point. I have changed the || into an && which makes it fine
again. The next patch series will follow shortly.

Cheers
~Sergej

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

Re: [PATCH v5 10/12] arm/mem_access: Add long-descriptor based gpt

[Julien Grall]

On 06/07/17 12:18, Sergej Proskurin wrote:
> Hi Julien,
>
>
> On 07/04/2017 07:06 PM, Julien Grall wrote:
>> Hi Sergej,
>>
>>
>>> +    /*
>>> +     * 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_page(pte, level)) )
>>
>> NIT: What you want to check here is either the entry is a superpage or
>> a page. So the below check would be easier to parse:
>>
>> if ( !lpae_is_superpage(pte, level) || !lpae_is_page(pte, level) )
>
> Your suggested check is easier to parse, however introduces a bug
> because as soon as the pte is not a superpage it will return an -EFAULT
> at this point. I have changed the || into an && which makes it fine
> again. The next patch series will follow shortly.

Whoops. You are right :).

Cheers,

>
> Cheers
> ~Sergej
>

-- 
Julien Grall

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