[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi Sergej,

On 20/06/17 21:33, Sergej Proskurin wrote:
The ARMv8 architecture supports pages with different (4K, 16K, and 64K) sizes.
To enable guest page table walks for various configurations, this commit
extends the defines and helpers of the current implementation.

Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
---
Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Cc: Julien Grall <julien.grall@xxxxxxx>
---
v3: Eliminate redundant macro definitions by introducing generic macros.

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

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

diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
index 6fbf7c606c..2913428e96 100644
--- a/xen/include/asm-arm/lpae.h
+++ b/xen/include/asm-arm/lpae.h
@@ -151,6 +151,73 @@ static inline bool lpae_is_superpage(lpae_t pte, unsigned 
int level)
     return (level < 3) && lpae_mapping(pte);
 }

+/*
+ * The ARMv8 architecture supports pages with different sizes (4K, 16K, and
+ * 64K). To enable guest page table walks for various configurations, the
+ * following helpers enable walking the guest's translation table with varying
+ * page size granularities.
+ */
+
+#define LPAE_SHIFT_4K           (9)
+#define LPAE_SHIFT_16K          (11)
+#define LPAE_SHIFT_64K          (13)
+
+#define lpae_entries(gran)      (_AC(1,U) << LPAE_SHIFT_##gran)
+#define lpae_entry_mask(gran)   (lpae_entries(gran) - 1)
+
+#define PAGE_SHIFT_4K           (12)
+#define PAGE_SHIFT_16K          (14)
+#define PAGE_SHIFT_64K          (16)

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

+
+#define third_shift(gran)       (PAGE_SHIFT_##gran)
+#define third_size(gran)        ((paddr_t)1 << third_shift(gran))
+
+#define second_shift(gran)      (third_shift(gran) + LPAE_SHIFT_##gran)
+#define second_size(gran)       ((paddr_t)1 << second_shift(gran))
+
+#define first_shift(gran)       (second_shift(gran) + LPAE_SHIFT_##gran)
+#define first_size(gran)        ((paddr_t)1 << first_shift(gran))
+
+/* Note that there is no zeroeth lookup level with a 64K granule size. */
+#define zeroeth_shift(gran)     (first_shift(gran) + LPAE_SHIFT_##gran)
+#define zeroeth_size(gran)      ((paddr_t)1 << zeroeth_shift(gran))
+
+#define GUEST_TABLE_OFFSET(offs, gran)          ((paddr_t)(offs) & 
lpae_entry_mask(gran))
+#define third_guest_table_offset(gva, gran)     GUEST_TABLE_OFFSET((gva >> 
third_shift(gran)), gran)
+#define second_guest_table_offset(gva, gran)    GUEST_TABLE_OFFSET((gva >> 
second_shift(gran)), gran)
+#define first_guest_table_offset(gva, gran)     GUEST_TABLE_OFFSET((gva >> 
first_shift(gran)), gran)
+#define zeroeth_guest_table_offset(gva, gran)   GUEST_TABLE_OFFSET((gva >> 
zeroeth_shift(gran)), gran)

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

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

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

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

Please undef both GUEST_TABLE_OFFSET and GUEST_TABLE_OFFSET_HELPERS.

+
 #endif /* __ASSEMBLY__ */

 /*


Cheers,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.