[RFC PATCH v2 0/8] arm/mem_access: Walk guest page tables in SW if mem_access is active

[RFC PATCH v2 0/8] arm/mem_access: Walk guest page tables in SW if mem_access is active

[Sergej Proskurin]

Hi all,

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

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

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

Cheers,
~Sergej

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

Sergej Proskurin (8):
  arm/mem_access: Add (TCR_|TTBCR_)* defines
  arm/mem_access: Add defines holding the width of 32/64bit regs
  arm/mem_access: Add defines supporting PTs with varying page sizes
  arm/mem_access: Add short-descriptor pte typedefs
  arm/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/mem_access.c       |  20 +-
 xen/arch/arm/p2m.c              | 446 ++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/p2m.h       |   6 +
 xen/include/asm-arm/page.h      |  86 ++++++++
 xen/include/asm-arm/processor.h |  53 +++++
 5 files changed, 610 insertions(+), 1 deletion(-)

-- 
2.12.2


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

[RFC PATCH v2 1/8] arm/mem_access: Add (TCR_|TTBCR_)* defines

[Sergej Proskurin]

This commit adds (TCR_|TTBCR_)* defines to simplify access to the respective
register contents.

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.
---
 xen/include/asm-arm/processor.h | 49 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 855ded1b07..c095dad7e9 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -94,6 +94,9 @@
 #define TTBCR_N_2KB  _AC(0x03,U)
 #define TTBCR_N_1KB  _AC(0x04,U)
 
+#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)
@@ -155,6 +158,19 @@
 /* TCR: Stage 1 Translation Control */
 
 #define TCR_T0SZ(x)     ((x)<<0)
+#define TCR_T0SZ_SHIFT  (0)
+#define TCR_T1SZ_SHIFT  (16)
+
+/*
+ * According to ARM DDI 0487A.g, TCR_EL1.{T0SZ,T1SZ} (Aarch64, Section D7-2021)
+ * comprises 6 bits and TTBCR.{T0SZ,T1SZ} (Aarch32, Section G6-4624) comprises
+ * 3 bits following another 3 bits for RES0. Thus, the mask for both registers
+ * should be 0x3f.
+ */
+#define TCR_SZ_MASK     (_AC(0x3f,UL)<<0)
+
+#define TCR_EPD0        (_AC(0x1,UL)<<7)
+#define TCR_EPD1        (_AC(0x1,UL)<<23)
 
 #define TCR_IRGN0_NC    (_AC(0x0,UL)<<8)
 #define TCR_IRGN0_WBWA  (_AC(0x1,UL)<<8)
@@ -173,11 +189,35 @@
 #define TCR_TG0_4K      (_AC(0x0,UL)<<14)
 #define TCR_TG0_64K     (_AC(0x1,UL)<<14)
 #define TCR_TG0_16K     (_AC(0x2,UL)<<14)
+#define TCR_TG0_MASK    (_AC(0x3,UL)<<14)
+#define TCR_TG0_SHIFT   (14)
+
+#define TCR_TG1_16K     (_AC(0x1,UL)<<30)
+#define TCR_TG1_4K      (_AC(0x2,UL)<<30)
+#define TCR_TG1_64K     (_AC(0x3,UL)<<30)
+#define TCR_TG1_MASK    (_AC(0x3,UL)<<30)
+#define TCR_TG1_SHIFT   (30)
+
+#define TCR_IPS_32_BIT  (_AC(0x0,ULL)<<32)
+#define TCR_IPS_36_BIT  (_AC(0x1,ULL)<<32)
+#define TCR_IPS_40_BIT  (_AC(0x2,ULL)<<32)
+#define TCR_IPS_42_BIT  (_AC(0x3,ULL)<<32)
+#define TCR_IPS_44_BIT  (_AC(0x4,ULL)<<32)
+#define TCR_IPS_48_BIT  (_AC(0x5,ULL)<<32)
+#define TCR_IPS_MASK    (_AC(0x7,ULL)<<32)
+#define TCR_IPS_SHIFT   (32)
+
+#define TCR_TB_31       (31)
 
 #ifdef CONFIG_ARM_64
 
 #define TCR_PS(x)       ((x)<<16)
 #define TCR_TBI         (_AC(0x1,UL)<<20)
+#define TCR_TBI0        (_AC(0x1,UL)<<37)
+#define TCR_TBI1        (_AC(0x1,UL)<<38)
+
+#define TCR_TB_63       (63)
+#define TCR_TB_55       (55)
 
 #define TCR_RES1        (_AC(1,UL)<<31|_AC(1,UL)<<23)
 
@@ -187,6 +227,15 @@
 
 #endif
 
+#define IPS_MIN         (25)
+#define IPS_MAX         (48)
+#define IPS_32_BIT      (32)
+#define IPS_36_BIT      (36)
+#define IPS_40_BIT      (40)
+#define IPS_42_BIT      (42)
+#define IPS_44_BIT      (44)
+#define IPS_48_BIT      (48)
+
 /* VTCR: Stage 2 Translation Control */
 
 #define VTCR_T0SZ(x)    ((x)<<0)
-- 
2.12.2


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

Re: [RFC PATCH v2 1/8] arm/mem_access: Add (TCR_|TTBCR_)* defines

[Julien Grall]

Hi Sergej,

On 06/01/2017 04:18 PM, Sergej Proskurin wrote:
> This commit adds (TCR_|TTBCR_)* defines to simplify access to the respective
> register contents.
> 
> 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.
> ---
>   xen/include/asm-arm/processor.h | 49 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 49 insertions(+)
> 
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 855ded1b07..c095dad7e9 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -94,6 +94,9 @@
>   #define TTBCR_N_2KB  _AC(0x03,U)
>   #define TTBCR_N_1KB  _AC(0x04,U)
>   
> +#define TTBCR_PD0       (_AC(1,U)<<4)
> +#define TTBCR_PD1       (_AC(1,U)<<5)

Looking at it, it is not clear whether the apply when EAE is set or not. 
In fact, they only apply when EAE is not set. This should at least be 
clear in a comment and potentially in the name.

> +
>   /* SCTLR System Control Register. */
>   /* HSCTLR is a subset of this. */
>   #define SCTLR_TE        (_AC(1,U)<<30)
> @@ -155,6 +158,19 @@
>   /* TCR: Stage 1 Translation Control */
>   
>   #define TCR_T0SZ(x)     ((x)<<0)

Please replace the hardcode 0 by TCR_T0SZ_SHIFT at the same time (and 
mention it in the commit message).

> +#define TCR_T0SZ_SHIFT  (0)
> +#define TCR_T1SZ_SHIFT  (16)
> +
> +/*
> + * According to ARM DDI 0487A.g, TCR_EL1.{T0SZ,T1SZ} (Aarch64, Section D7-2021)
NIT: s/Aarch64/AArch64/

Also, 0487A.g is a 2 years old spec, there was quite a few revisions 
since then. Please download and quote the latest spec (i.e 0487B.a).


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

NIT: s/Aarch32/AArch64/

> + * 3 bits following another 3 bits for RES0. Thus, the mask for both registers
> + * should be 0x3f.
> + */
> +#define TCR_SZ_MASK     (_AC(0x3f,UL)<<0)
> +
> +#define TCR_EPD0        (_AC(0x1,UL)<<7)
> +#define TCR_EPD1        (_AC(0x1,UL)<<23)
>   
>   #define TCR_IRGN0_NC    (_AC(0x0,UL)<<8)
>   #define TCR_IRGN0_WBWA  (_AC(0x1,UL)<<8)
> @@ -173,11 +189,35 @@
>   #define TCR_TG0_4K      (_AC(0x0,UL)<<14)
>   #define TCR_TG0_64K     (_AC(0x1,UL)<<14)
>   #define TCR_TG0_16K     (_AC(0x2,UL)<<14)
> +#define TCR_TG0_MASK    (_AC(0x3,UL)<<14)
> +#define TCR_TG0_SHIFT   (14)
> +
> +#define TCR_TG1_16K     (_AC(0x1,UL)<<30)
> +#define TCR_TG1_4K      (_AC(0x2,UL)<<30)
> +#define TCR_TG1_64K     (_AC(0x3,UL)<<30)
> +#define TCR_TG1_MASK    (_AC(0x3,UL)<<30)
> +#define TCR_TG1_SHIFT   (30)
> +
> +#define TCR_IPS_32_BIT  (_AC(0x0,ULL)<<32)
> +#define TCR_IPS_36_BIT  (_AC(0x1,ULL)<<32)
> +#define TCR_IPS_40_BIT  (_AC(0x2,ULL)<<32)
> +#define TCR_IPS_42_BIT  (_AC(0x3,ULL)<<32)
> +#define TCR_IPS_44_BIT  (_AC(0x4,ULL)<<32)
> +#define TCR_IPS_48_BIT  (_AC(0x5,ULL)<<32)
> +#define TCR_IPS_MASK    (_AC(0x7,ULL)<<32)
> +#define TCR_IPS_SHIFT   (32)

The fields TG1 and IPS does not exist for AArch32. You should probably 
mention it at least in a comment.

Also, a lot of the new defines you add are for TCR_EL1 and not TCR_EL2.
Please make the distinction in the name to avoid misusing them.

> +
> +#define TCR_TB_31       (31)
>   
>   #ifdef CONFIG_ARM_64
>   
>   #define TCR_PS(x)       ((x)<<16)
>   #define TCR_TBI         (_AC(0x1,UL)<<20)
> +#define TCR_TBI0        (_AC(0x1,UL)<<37)
> +#define TCR_TBI1        (_AC(0x1,UL)<<38)

Those fields don't exist in TCR_EL2.

> +
> +#define TCR_TB_63       (63)
> +#define TCR_TB_55       (55)

Same here.

>   
>   #define TCR_RES1        (_AC(1,UL)<<31|_AC(1,UL)<<23)
>   
> @@ -187,6 +227,15 @@
>   
>   #endif
>   
> +#define IPS_MIN         (25)
> +#define IPS_MAX         (48)
> +#define IPS_32_BIT      (32)
> +#define IPS_36_BIT      (36)
> +#define IPS_40_BIT      (40)
> +#define IPS_42_BIT      (42)
> +#define IPS_44_BIT      (44)
> +#define IPS_48_BIT      (48)

What is it for? Which register?

> +
>   /* VTCR: Stage 2 Translation Control */
>   
>   #define VTCR_T0SZ(x)    ((x)<<0)
> 

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH v2 1/8] arm/mem_access: Add (TCR_|TTBCR_)* defines

[Sergej Proskurin]

Hi Julien,

[...]


>
> Also, a lot of the new defines you add are for TCR_EL1 and not TCR_EL2.
> Please make the distinction in the name to avoid misusing them.
>
>> +
>> +#define TCR_TB_31       (31)
>>     #ifdef CONFIG_ARM_64
>>     #define TCR_PS(x)       ((x)<<16)
>>   #define TCR_TBI         (_AC(0x1,UL)<<20)
>> +#define TCR_TBI0        (_AC(0x1,UL)<<37)
>> +#define TCR_TBI1        (_AC(0x1,UL)<<38)
>
> Those fields don't exist in TCR_EL2.

This is not entirely correct. All of the introduced fields are also
available in TCR_EL2, however, only if HCR_EL2.E2H==1. I will comment
that appropriately. Do you think that we should use nevertheless
different names for the introduced defines?


[...]

Cheers,
~Sergej


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

Re: [RFC PATCH v2 1/8] arm/mem_access: Add (TCR_|TTBCR_)* defines

[Julien Grall]

On 07/06/17 15:56, Sergej Proskurin wrote:
> Hi Julien,
>
> [...]
>
>
>>
>> Also, a lot of the new defines you add are for TCR_EL1 and not TCR_EL2.
>> Please make the distinction in the name to avoid misusing them.
>>
>>> +
>>> +#define TCR_TB_31       (31)
>>>     #ifdef CONFIG_ARM_64
>>>     #define TCR_PS(x)       ((x)<<16)
>>>   #define TCR_TBI         (_AC(0x1,UL)<<20)
>>> +#define TCR_TBI0        (_AC(0x1,UL)<<37)
>>> +#define TCR_TBI1        (_AC(0x1,UL)<<38)
>>
>> Those fields don't exist in TCR_EL2.
>
> This is not entirely correct. All of the introduced fields are also
> available in TCR_EL2, however, only if HCR_EL2.E2H==1. I will comment
> that appropriately. Do you think that we should use nevertheless
> different names for the introduced defines?


This was added by ARMv8.1. HCR_EL2.E2H = 1 as is to run Host Operating 
System in EL2 (e.g for Type-2 hypervisor). This will very never be used 
by Xen.

In any case, the naming gives the impression it also exists when 
HCR_EL2.E2H = 0.

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH v2 1/8] arm/mem_access: Add (TCR_|TTBCR_)* defines

[Julien Grall]

On 07/06/17 16:07, Julien Grall wrote:
>
>
> On 07/06/17 15:56, Sergej Proskurin wrote:
>> Hi Julien,
>>
>> [...]
>>
>>
>>>
>>> Also, a lot of the new defines you add are for TCR_EL1 and not TCR_EL2.
>>> Please make the distinction in the name to avoid misusing them.
>>>
>>>> +
>>>> +#define TCR_TB_31       (31)
>>>>     #ifdef CONFIG_ARM_64
>>>>     #define TCR_PS(x)       ((x)<<16)
>>>>   #define TCR_TBI         (_AC(0x1,UL)<<20)
>>>> +#define TCR_TBI0        (_AC(0x1,UL)<<37)
>>>> +#define TCR_TBI1        (_AC(0x1,UL)<<38)
>>>
>>> Those fields don't exist in TCR_EL2.
>>
>> This is not entirely correct. All of the introduced fields are also
>> available in TCR_EL2, however, only if HCR_EL2.E2H==1. I will comment
>> that appropriately. Do you think that we should use nevertheless
>> different names for the introduced defines?
>
>
> This was added by ARMv8.1. HCR_EL2.E2H = 1 as is to run Host Operating
> System in EL2 (e.g for Type-2 hypervisor). This will very never be used
> by Xen.
>
> In any case, the naming gives the impression it also exists when
> HCR_EL2.E2H = 0.

To clarify my point. They should be named with TCR_EL1_TB* as this is 
how it is used in Xen.

Cheers,

-- 
Julien Grall

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

[RFC PATCH v2 2/8] arm/mem_access: Add defines holding the width of 32/64bit regs

[Sergej Proskurin]

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

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

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


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

[RFC PATCH v2 3/8] 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>
---
 xen/include/asm-arm/page.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 4b46e8831c..6222b1d4a2 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -500,6 +500,73 @@ static inline int gva_to_ipa(vaddr_t va, paddr_t *paddr, unsigned int flags)
 
 #define PAGE_ALIGN(x) (((x) + PAGE_SIZE - 1) & PAGE_MASK)
 
+#define LPAE_SHIFT_4K           (9)
+#define LPAE_SHIFT_16K          (11)
+#define LPAE_SHIFT_64K          (13)
+
+#define LPAE_ENTRIES_4K         (_AC(1,U) << LPAE_SHIFT_4K)
+#define LPAE_ENTRIES_16K        (_AC(1,U) << LPAE_SHIFT_16K)
+#define LPAE_ENTRIES_64K        (_AC(1,U) << LPAE_SHIFT_64K)
+
+#define LPAE_ENTRY_MASK_4K      (LPAE_ENTRIES_4K - 1)
+#define LPAE_ENTRY_MASK_16K     (LPAE_ENTRIES_16K - 1)
+#define LPAE_ENTRY_MASK_64K     (LPAE_ENTRIES_64K - 1)
+
+#define PAGE_SHIFT_4K           (12)
+#define PAGE_SHIFT_16K          (14)
+#define PAGE_SHIFT_64K          (16)
+
+#define THIRD_SHIFT_4K          (PAGE_SHIFT_4K)
+#define THIRD_SHIFT_16K         (PAGE_SHIFT_16K)
+#define THIRD_SHIFT_64K         (PAGE_SHIFT_64K)
+
+#define THIRD_SIZE_4K           ((paddr_t)1 << THIRD_SHIFT_4K)
+#define THIRD_SIZE_16K          ((paddr_t)1 << THIRD_SHIFT_16K)
+#define THIRD_SIZE_64K          ((paddr_t)1 << THIRD_SHIFT_64K)
+
+#define SECOND_SHIFT_4K         (THIRD_SHIFT_4K + LPAE_SHIFT_4K)
+#define SECOND_SHIFT_16K        (THIRD_SHIFT_16K + LPAE_SHIFT_16K)
+#define SECOND_SHIFT_64K        (THIRD_SHIFT_64K + LPAE_SHIFT_64K)
+
+#define SECOND_SIZE_4K          ((paddr_t)1 << SECOND_SHIFT_4K)
+#define SECOND_SIZE_16K         ((paddr_t)1 << SECOND_SHIFT_16K)
+#define SECOND_SIZE_64K         ((paddr_t)1 << SECOND_SHIFT_64K)
+
+#define FIRST_SHIFT_4K          (SECOND_SHIFT_4K + LPAE_SHIFT_4K)
+#define FIRST_SHIFT_16K         (SECOND_SHIFT_16K + LPAE_SHIFT_16K)
+#define FIRST_SHIFT_64K         (SECOND_SHIFT_64K + LPAE_SHIFT_64K)
+
+#define FIRST_SIZE_4K           ((paddr_t)1 << FIRST_SHIFT_4K)
+#define FIRST_SIZE_16K          ((paddr_t)1 << FIRST_SHIFT_16K)
+#define FIRST_SIZE_64K          ((paddr_t)1 << FIRST_SHIFT_64K)
+
+#define ZEROETH_SHIFT_4K        (FIRST_SHIFT_4K + LPAE_SHIFT_4K)
+#define ZEROETH_SHIFT_16K       (FIRST_SHIFT_16K + LPAE_SHIFT_16K)
+
+#define ZEROETH_SIZE_4K         ((paddr_t)1 << ZEROETH_SHIFT_4K)
+#define ZEROETH_SIZE_16K        ((paddr_t)1 << ZEROETH_SHIFT_16K)
+
+#define GUEST_TABLE_OFFSET(offs, gran)          ((paddr_t)(offs) & LPAE_ENTRY_MASK_##gran)
+#define third_guest_table_offset(va, gran)      GUEST_TABLE_OFFSET((va >> THIRD_SHIFT_##gran), gran)
+#define second_guest_table_offset(va, gran)     GUEST_TABLE_OFFSET((va >> SECOND_SHIFT_##gran), gran)
+#define first_guest_table_offset(va, gran)      GUEST_TABLE_OFFSET((va >> FIRST_SHIFT_##gran), gran)
+#define zeroeth_guest_table_offset(va, gran)    GUEST_TABLE_OFFSET((va >> ZEROETH_SHIFT_##gran), gran)
+
+#define third_guest_table_offset_4k(va)         third_guest_table_offset(va, 4K)
+#define third_guest_table_offset_16k(va)        third_guest_table_offset(va, 16K)
+#define third_guest_table_offset_64k(va)        third_guest_table_offset(va, 64K)
+
+#define second_guest_table_offset_4k(va)        second_guest_table_offset(va, 4K)
+#define second_guest_table_offset_16k(va)       second_guest_table_offset(va, 16K)
+#define second_guest_table_offset_64k(va)       second_guest_table_offset(va, 64K)
+
+#define first_guest_table_offset_4k(va)         first_guest_table_offset(va, 4K)
+#define first_guest_table_offset_16k(va)        first_guest_table_offset(va, 16K)
+#define first_guest_table_offset_64k(va)        first_guest_table_offset(va, 64K)
+
+#define zeroeth_guest_table_offset_4k(va)       zeroeth_guest_table_offset(va, 4K)
+#define zeroeth_guest_table_offset_16k(va)      zeroeth_guest_table_offset(va, 16K)
+
 #endif /* __ARM_PAGE_H__ */
 
 /*
-- 
2.12.2


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

Re: [RFC PATCH v2 3/8] arm/mem_access: Add defines supporting PTs with varying page sizes

[Julien Grall]

Hi Sergej,

On 06/01/2017 04:18 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>
> ---
>   xen/include/asm-arm/page.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 67 insertions(+)
> 
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 4b46e8831c..6222b1d4a2 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -500,6 +500,73 @@ static inline int gva_to_ipa(vaddr_t va, paddr_t *paddr, unsigned int flags)
>   
>   #define PAGE_ALIGN(x) (((x) + PAGE_SIZE - 1) & PAGE_MASK)
>   
> +#define LPAE_SHIFT_4K           (9)
> +#define LPAE_SHIFT_16K          (11)
> +#define LPAE_SHIFT_64K          (13)
> +
> +#define LPAE_ENTRIES_4K         (_AC(1,U) << LPAE_SHIFT_4K)
> +#define LPAE_ENTRIES_16K        (_AC(1,U) << LPAE_SHIFT_16K)
> +#define LPAE_ENTRIES_64K        (_AC(1,U) << LPAE_SHIFT_64K)

It sounds like to me you can introduce macros to avoid all the 
redundancies as you did below for GUEST_TABLE_OFFSET.

This macro would take a granularity in parameter and create all the 
*_gran define associated to it.

But I am not fully convinced they all need to be defined. You only use 
them once and could easily use LPAE_SHIFT_*K to get what you need in the 
guest page table walker.

Cheers,

-- 
Julien Grall

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

[RFC PATCH v2 4/8] arm/mem_access: Add short-descriptor pte typedefs

[Sergej Proskurin]

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

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

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 6222b1d4a2..5ea97ba95b 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -205,6 +205,25 @@ typedef union {
     lpae_walk_t walk;
 } lpae_t;
 
+/*
+ * Comprises the bits required to walk page tables adhering to the
+ * short-descriptor translation table format.
+ */
+typedef struct __packed {
+    unsigned int dt:2;          /* Descriptor type */
+    unsigned int pad1:8;
+    unsigned int base:22;       /* Base address of block or next table */
+} pte_sd_walk_t;
+
+/*
+ * Represents page table entries adhering to the short-descriptor translation
+ * table format.
+ */
+typedef union {
+    uint32_t bits;
+    pte_sd_walk_t walk;
+} pte_sd_t;
+
 /* Standard entry type that we'll use to build Xen's own pagetables.
  * We put the same permissions at every level, because they're ignored
  * by the walker in non-leaf entries. */
-- 
2.12.2


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

Re: [RFC PATCH v2 4/8] arm/mem_access: Add short-descriptor pte typedefs

[Julien Grall]

Hi Sergej,

On 06/01/2017 04:18 PM, Sergej Proskurin wrote:
> The current implementation does not provide appropriate types for
> short-descriptor translation table entries. As such, this commit adds new
> types, which simplify managing the respective translation table entries.
> 
> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
>   xen/include/asm-arm/page.h | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
> 
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 6222b1d4a2..5ea97ba95b 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -205,6 +205,25 @@ typedef union {
>       lpae_walk_t walk;
>   } lpae_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 */

This is clearly confusing. The base address size varies with the level 
you are currently walking. Without looking at the code, I can guess that 
you would need shift/mask to adapt the base address. This is a call for 
providing structure for each level (there is only 2 anyway).

> +} pte_sd_walk_t;

I am a bit surprised this is the only bits you required for the walking 
as you also need to return the permissions.

Looking at the patch doing the implement in patch #7, there is a lot of 
hardcoding value. This is a call for a better structure definition here.

> +/*
> + * Represents page table entries adhering to the short-descriptor translation
> + * table format.
> + */
> +typedef union {
> +    uint32_t bits;
> +    pte_sd_walk_t walk;
> +} pte_sd_t;
> +
>   /* Standard entry type that we'll use to build Xen's own pagetables.
>    * We put the same permissions at every level, because they're ignored
>    * by the walker in non-leaf entries. */
> 

Cheers,

-- 
Julien Grall

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

[RFC PATCH v2 5/8] 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: This function p2m_walk_gpt 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 table on a different vCPU, the following registers
would need to be loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and SCTLR_EL1.

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

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

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

    Cosmetic fixes.
---
 xen/arch/arm/p2m.c        | 58 +++++++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/p2m.h |  6 +++++
 2 files changed, 64 insertions(+)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 752e948070..0337d83581 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1548,6 +1548,64 @@ void __init setup_virt_paging(void)
 }
 
 /*
+ * The function __p2m_walk_gpt_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 __p2m_walk_gpt_sd(struct p2m_domain *p2m,
+                             vaddr_t gva, paddr_t *ipa,
+                             unsigned int *perm_ro)
+{
+    /* Not implemented yet. */
+    return -EFAULT;
+}
+
+/*
+ * The function __p2m_walk_gpt_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 __p2m_walk_gpt_ld(struct p2m_domain *p2m,
+                             vaddr_t gva, paddr_t *ipa,
+                             unsigned int *perm_ro)
+{
+    /* Not implemented yet. */
+    return -EFAULT;
+}
+
+int p2m_walk_gpt(struct p2m_domain *p2m, vaddr_t gva,
+                 paddr_t *ipa, unsigned int *perm_ro)
+{
+    uint32_t sctlr = READ_SYSREG(SCTLR_EL1);
+    register_t tcr = READ_SYSREG(TCR_EL1);
+#ifdef CONFIG_ARM_64
+    struct domain *d = p2m->domain;
+#endif
+
+    /* If the MMU is disabled, there is no need to translate the gva. */
+    if ( !(sctlr & SCTLR_M) )
+    {
+        *ipa = gva;
+
+        return 0;
+    }
+
+#ifdef CONFIG_ARM_64
+    if ( is_32bit_domain(d) )
+#endif
+    {
+        if ( !(tcr & TTBCR_EAE) )
+            return __p2m_walk_gpt_sd(p2m, gva, ipa, perm_ro);
+    }
+
+    return __p2m_walk_gpt_ld(p2m, gva, ipa, perm_ro);
+}
+
+/*
  * Local variables:
  * mode: C
  * c-file-style: "BSD"
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 18c57f936e..cc9f4bf225 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -264,6 +264,12 @@ void guest_physmap_remove_page(struct domain *d,
 
 mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn);
 
+/* Walk the guest's page tables in software. */
+int p2m_walk_gpt(struct p2m_domain *p2m,
+                 vaddr_t gva,
+                 paddr_t *ipa,
+                 unsigned int *perm_ro);
+
 /*
  * Populate-on-demand
  */
-- 
2.12.2


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

Re: [RFC PATCH v2 5/8] arm/mem_access: Add software guest-page-table walk

[Julien Grall]

Hi Sergej,

On 06/01/2017 04:18 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: This function p2m_walk_gpt 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 table on a different vCPU, the following registers
> would need to be loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and SCTLR_EL1.
> 
> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
> v2: Rename p2m_gva_to_ipa to p2m_walk_gpt and move it to p2m.c.
> 
>      Move the functionality responsible for walking long-descriptor based
>      translation tables out of the function p2m_walk_gpt. Also move out the
>      long-descriptor based translation out of this commit.
> 
>      Change function parameters in order to return access access rights to a
>      requested gva.
> 
>      Cosmetic fixes.
> ---
>   xen/arch/arm/p2m.c        | 58 +++++++++++++++++++++++++++++++++++++++++++++++

I know I suggested to move in p2m.c. Looking at the diff stat, this will 
increase quite a lot p2m.c which is already big.

How about introducing a file guest_walk.c which contain the new functions?

>   xen/include/asm-arm/p2m.h |  6 +++++
>   2 files changed, 64 insertions(+)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 752e948070..0337d83581 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1548,6 +1548,64 @@ void __init setup_virt_paging(void)
>   }
>   
>   /*
> + * The function __p2m_walk_gpt_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 __p2m_walk_gpt_sd(struct p2m_domain *p2m,

The __ are not necessary here. You don't use the name p2m_walk_gpt_sd 
anywhere.

> +                             vaddr_t gva, paddr_t *ipa,
> +                             unsigned int *perm_ro)

I am a bit confused with perm_ro. Will you only return 0/1? If so it 
should be a bool.

But we likely want to know more permission such as the execution bit...

> +{
> +    /* Not implemented yet. */
> +    return -EFAULT;
> +}
> +
> +/*
> + * The function __p2m_walk_gpt_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 __p2m_walk_gpt_ld(struct p2m_domain *p2m,

Ditto.

> +                             vaddr_t gva, paddr_t *ipa,
> +                             unsigned int *perm_ro)
> +{
> +    /* Not implemented yet. */
> +    return -EFAULT;
> +}
> +
> +int p2m_walk_gpt(struct p2m_domain *p2m, vaddr_t gva,

So you mix 2 things, p2m and gpt. P2M is used to refer to stage-2 page 
table. Whilst gpt stands I guess for guest page table.

The name gpt is not very used in Xen and would prefer a clearer name 
such as the x86 one "guest_walk_tables".

> +                 paddr_t *ipa, unsigned int *perm_ro)
> +{
> +    uint32_t sctlr = READ_SYSREG(SCTLR_EL1);
> +    register_t tcr = READ_SYSREG(TCR_EL1);
> +#ifdef CONFIG_ARM_64
> +    struct domain *d = p2m->domain;
> +#endif

The only place use *d is in the is_32bit_domain, so no need to add some 
#ifdef and define the variable.

> +
> +    /* If the MMU is disabled, there is no need to translate the gva. */
> +    if ( !(sctlr & SCTLR_M) )
> +    {
> +        *ipa = gva;

Why *perm_ro is not set here?

> +
> +        return 0;
> +    }
> +
> +#ifdef CONFIG_ARM_64
> +    if ( is_32bit_domain(d) )
> +#endif

is_32bit_domain exists for 32-bit Xen. So not need to have this #ifdef.

> +    {
> +        if ( !(tcr & TTBCR_EAE) )
> +            return __p2m_walk_gpt_sd(p2m, gva, ipa, perm_ro);
> +    }
> +
> +    return __p2m_walk_gpt_ld(p2m, gva, ipa, perm_ro);
> +}
> +
> +/*
>    * Local variables:
>    * mode: C
>    * c-file-style: "BSD"
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 18c57f936e..cc9f4bf225 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -264,6 +264,12 @@ void guest_physmap_remove_page(struct domain *d,
>   
>   mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn);
>   
> +/* Walk the guest's page tables in software. */
> +int p2m_walk_gpt(struct p2m_domain *p2m,
> +                 vaddr_t gva,
> +                 paddr_t *ipa,
> +                 unsigned int *perm_ro);
> +
>   /*
>    * Populate-on-demand
>    */
> 

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH v2 5/8] arm/mem_access: Add software guest-page-table walk

[Sergej Proskurin]

Hi Julien,

[...]

> I know I suggested to move in p2m.c. Looking at the diff stat, this
> will increase quite a lot p2m.c which is already big.
>
> How about introducing a file guest_walk.c which contain the new
> functions?
>

No problem at all. I will gladly move the functionality into a separate
file.

>
>> +                             vaddr_t gva, paddr_t *ipa,
>> +                             unsigned int *perm_ro)
>
> I am a bit confused with perm_ro. Will you only return 0/1? If so it
> should be a bool.
>
> But we likely want to know more permission such as the execution bit...
>

Yes, I agree that we should return more permissions back to the caller.
I suggest that we agree on required permissions at this point, as do not
only have the execution bit (!XN) but also we distinguish between the
Privileged XN (PXN) bit in the long-descriptor format, as well as the
access permissions bits (AP[2:x]), which inform which EL/PL is allowed
to access (RWX) the particular memory region.

Or do you think returning an additional execute bit (!XN) would suffice
for now, as we don't really care about execution permissions at
different EL's/PL's at this point?

>
>
> The name gpt is not very used in Xen and would prefer a clearer name
> such as the x86 one "guest_walk_tables".

Sounds good to me. I have changed the names to guest_walk_(tables|sd|ld).

>
>> +                 paddr_t *ipa, unsigned int *perm_ro)
>> +{
>> +    uint32_t sctlr = READ_SYSREG(SCTLR_EL1);
>> +    register_t tcr = READ_SYSREG(TCR_EL1);
>> +#ifdef CONFIG_ARM_64
>> +    struct domain *d = p2m->domain;
>> +#endif
>
> The only place use *d is in the is_32bit_domain, so no need to add
> some #ifdef and define the variable.
>

[...]

>> +
>> +#ifdef CONFIG_ARM_64
>> +    if ( is_32bit_domain(d) )
>> +#endif
>
> is_32bit_domain exists for 32-bit Xen. So not need to have this #ifdef.
>

True. The reason for this #ifdef is that since is_32bit_domain(d)
resolves to a (1) on ARMv7, the compiler complained about the fact that
the variable struct domain *d was not used. To resolve this, I can
simply use p2m->domain at this point and remove the local variable
completely. Alternatively, I could use struct domain *d as a function
parameter instead of struct p2m_domain *p2m. I believe it would be
cleaner to provide the domain instead of the p2m as parameter, as we
don't really need the p2m. What would you prefer?

Cheers,
~Sergej


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

Re: [RFC PATCH v2 5/8] arm/mem_access: Add software guest-page-table walk

[Julien Grall]

On 08/06/2017 13:43, Sergej Proskurin wrote:
> Hi Julien,

Hi Sergej,

>
> [...]
>
>> I know I suggested to move in p2m.c. Looking at the diff stat, this
>> will increase quite a lot p2m.c which is already big.
>>
>> How about introducing a file guest_walk.c which contain the new
>> functions?
>>
>
> No problem at all. I will gladly move the functionality into a separate
> file.
>
>>
>>> +                             vaddr_t gva, paddr_t *ipa,
>>> +                             unsigned int *perm_ro)
>>
>> I am a bit confused with perm_ro. Will you only return 0/1? If so it
>> should be a bool.
>>
>> But we likely want to know more permission such as the execution bit...
>>
>
> Yes, I agree that we should return more permissions back to the caller.
> I suggest that we agree on required permissions at this point, as do not
> only have the execution bit (!XN) but also we distinguish between the
> Privileged XN (PXN) bit in the long-descriptor format, as well as the
> access permissions bits (AP[2:x]), which inform which EL/PL is allowed
> to access (RWX) the particular memory region.
>
> Or do you think returning an additional execute bit (!XN) would suffice
> for now, as we don't really care about execution permissions at
> different EL's/PL's at this point?

I think Read/Write/eXecute should be enough for now. We can add 
additional one later on.

My main point here is not about the permission returned but the 
interface. At the moment, you return a boolean-like value and it would 
be tedious to update the callers. What I would like to see is a set of 
flags that we can easily extend. Something like:

#define PERM_READ  (1 << 0)
#define PERM_WRITE (1 << 1)
#define PERM_EXEC  (1 << 2)

We probably want to make them a bit less generic or re-use something 
already existing (I haven't checked what we currently have).

>>
>>
>> The name gpt is not very used in Xen and would prefer a clearer name
>> such as the x86 one "guest_walk_tables".
>
> Sounds good to me. I have changed the names to guest_walk_(tables|sd|ld).
>
>>
>>> +                 paddr_t *ipa, unsigned int *perm_ro)
>>> +{
>>> +    uint32_t sctlr = READ_SYSREG(SCTLR_EL1);
>>> +    register_t tcr = READ_SYSREG(TCR_EL1);
>>> +#ifdef CONFIG_ARM_64
>>> +    struct domain *d = p2m->domain;
>>> +#endif
>>
>> The only place use *d is in the is_32bit_domain, so no need to add
>> some #ifdef and define the variable.
>>
>
> [...]
>
>>> +
>>> +#ifdef CONFIG_ARM_64
>>> +    if ( is_32bit_domain(d) )
>>> +#endif
>>
>> is_32bit_domain exists for 32-bit Xen. So not need to have this #ifdef.
>>
>
> True. The reason for this #ifdef is that since is_32bit_domain(d)
> resolves to a (1) on ARMv7, the compiler complained about the fact that
> the variable struct domain *d was not used. To resolve this, I can
> simply use p2m->domain at this point and remove the local variable
> completely. Alternatively, I could use struct domain *d as a function
> parameter instead of struct p2m_domain *p2m. I believe it would be
> cleaner to provide the domain instead of the p2m as parameter, as we
> don't really need the p2m. What would you prefer?

Technically this should be a vCPU as the guest page-table may be 
different on each vCPU. So I would prefer to use a vCPU here.

Also, it would allow us to make sure the function has been called with 
vCPU == current (i.e an ASSERT).

Cheers,

-- 
Julien Grall

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

[RFC PATCH v2 6/8] 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 0487A-g J11-5608, J11-5679, and ARM DDI 0406C-b
B3-1510.

Note that the current implementation lacks support for 32-bit EL0
running on top of 64-bit EL1. 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.
---
 xen/arch/arm/p2m.c | 271 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 269 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 0337d83581..ea3be6f050 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1573,8 +1573,275 @@ static int __p2m_walk_gpt_ld(struct p2m_domain *p2m,
                              vaddr_t gva, paddr_t *ipa,
                              unsigned int *perm_ro)
 {
-    /* Not implemented yet. */
-    return -EFAULT;
+    int t0_sz, t1_sz, disabled = 1;
+    unsigned int level, gran;
+    unsigned int topbit = 0, input_size = 0, output_size;
+    uint64_t ttbr = 0, ips;
+    paddr_t mask;
+    lpae_t pte, *table;
+    struct page_info *page;
+    register_t tcr = READ_SYSREG(TCR_EL1);
+    struct domain *d = p2m->domain;
+
+    const vaddr_t offsets[4][3] = {
+        {
+#ifdef CONFIG_ARM_64
+            zeroeth_guest_table_offset_4k(gva),
+            zeroeth_guest_table_offset_16k(gva),
+            0, /* There is no zeroeth lookup level with a 64K granule size. */
+#endif
+        },
+        {
+            first_guest_table_offset_4k(gva),
+#ifdef CONFIG_ARM_64
+            first_guest_table_offset_16k(gva),
+            first_guest_table_offset_64k(gva),
+#endif
+        },
+        {
+            second_guest_table_offset_4k(gva),
+#ifdef CONFIG_ARM_64
+            second_guest_table_offset_16k(gva),
+            second_guest_table_offset_64k(gva),
+#endif
+        },
+        {
+            third_guest_table_offset_4k(gva),
+#ifdef CONFIG_ARM_64
+            third_guest_table_offset_16k(gva),
+            third_guest_table_offset_64k(gva),
+#endif
+        }
+    };
+
+    const paddr_t masks[4][3] = {
+        {
+            ZEROETH_SIZE_4K - 1,
+            ZEROETH_SIZE_16K - 1,
+            0 /* There is no zeroeth lookup level with a 64K granule size. */
+        },
+        {
+            FIRST_SIZE_4K - 1,
+            FIRST_SIZE_16K - 1,
+            FIRST_SIZE_64K - 1
+        },
+        {
+            SECOND_SIZE_4K - 1,
+            SECOND_SIZE_16K - 1,
+            SECOND_SIZE_64K - 1
+        },
+        {
+            THIRD_SIZE_4K - 1,
+            THIRD_SIZE_16K - 1,
+            THIRD_SIZE_64K - 1
+        }
+    };
+
+    const unsigned int grainsizes[3] = {
+        PAGE_SHIFT_4K,
+        PAGE_SHIFT_16K,
+        PAGE_SHIFT_64K
+    };
+
+    const unsigned int strides[3] = {
+        LPAE_SHIFT_4K,
+        LPAE_SHIFT_16K,
+        LPAE_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, according to "AddrTop" pseudocode
+     * implementation in ARM DDI 0487A-g J11-5739.
+     *
+     * XXX: We do not check whether the 64bit domain uses a 32-bit EL0. In this
+     * case, we need to set topbit to 31, as well.
+     */
+    if ( is_32bit_domain(d) )
+        topbit = TCR_TB_31;
+#ifdef CONFIG_ARM_64
+    else if ( is_64bit_domain(d) )
+    {
+        if ( ((gva & (1UL << TCR_TB_55)) && (tcr & TCR_TBI1)) ||
+             (!(gva & (1UL << TCR_TB_55)) && (tcr & TCR_TBI0)) )
+            topbit = TCR_TB_55;
+        else
+            topbit = TCR_TB_63;
+    }
+#endif
+
+#ifdef CONFIG_ARM_64
+    if ( is_64bit_domain(d) )
+    {
+        if ( (gva & (1UL << topbit)) == 0 )
+        {
+            input_size = REGISTER_WIDTH_64_BIT - t0_sz;
+
+            if ( input_size > IPS_MAX )
+                /* We limit the input_size to be max 48 bit. */
+                input_size = IPS_MAX;
+            else if ( input_size < IPS_MIN )
+                /* We limit the input_size to be max 25 bit. */
+                input_size = IPS_MIN;
+
+            /* Normalize granule size. */
+            switch ( tcr & TCR_TG0_MASK ) {
+            case TCR_TG0_16K:
+                gran = 1;
+                break;
+            case TCR_TG0_64K:
+                gran = 2;
+                break;
+            default:
+                gran = 0;
+            }
+
+            /* 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 ) ? 1 : 0;
+        }
+        else
+        {
+            input_size = REGISTER_WIDTH_64_BIT - t1_sz;
+
+            if ( input_size > IPS_MAX )
+                /* We limit the input_size to be max 48 bit. */
+                input_size = IPS_MAX;
+            else if ( input_size < IPS_MIN )
+                /* We limit the input_size to be max 25 bit. */
+                input_size = IPS_MIN;
+
+            /* Normalize granule size. */
+            switch ( tcr & TCR_TG1_MASK ) {
+            case TCR_TG1_16K:
+                gran = 1;
+                break;
+            case TCR_TG1_64K:
+                gran = 2;
+                break;
+            default:
+                gran = 0;
+            }
+
+            /* 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 ) ? 1 : 0;
+        }
+    }
+    else
+#endif
+    {
+        /* Granule size of Aarch32 or ARMv7 architectures is always 4K, indexed by 0. */
+        gran = 0;
+
+        mask = ((1ULL << REGISTER_WIDTH_32_BIT) - 1) &
+               ~((1ULL << (REGISTER_WIDTH_32_BIT - t0_sz)) - 1);
+
+        if ( t0_sz == 0 || !(gva & mask) )
+        {
+            input_size = REGISTER_WIDTH_32_BIT - t0_sz;
+
+            /* Use TTBR0 for GVA to IPA translation. */
+            ttbr = READ_SYSREG64(TTBR0_EL1);
+
+            /* If TCR.EPD0 is set, translations using TTBR0 are disabled. */
+            disabled = ( tcr & TCR_EPD0 ) ? 1 : 0;
+        }
+
+        mask = ((1ULL << REGISTER_WIDTH_32_BIT) - 1) &
+               ~((1ULL << (REGISTER_WIDTH_32_BIT - t1_sz)) - 1);
+
+        if ( ((t1_sz == 0) && !ttbr) || (t1_sz && (gva & mask) == mask) )
+        {
+            input_size = REGISTER_WIDTH_32_BIT - t1_sz;
+
+            /* Use TTBR1 for GVA to IPA translation. */
+            ttbr = READ_SYSREG64(TTBR1_EL1);
+
+            /* If TCR.EPD1 is set, translations using TTBR1 are disabled. */
+            disabled = ( tcr & TCR_EPD1 ) ? 1 : 0;
+        }
+    }
+
+    if ( disabled )
+        return -EFAULT;
+
+    level = 4 - DIV_ROUND_UP((input_size - grainsizes[gran]), strides[gran]);
+
+    /* XXX: We do not consider 32bit EL0 running on Aarch64, yet. */
+    if ( is_64bit_domain(d) )
+    {
+        /* Get the intermediate physical address size. */
+        ips = (tcr & TCR_IPS_MASK) >> TCR_IPS_SHIFT;
+
+        switch (ips) {
+        case TCR_IPS_32_BIT:
+            output_size = IPS_32_BIT;
+            break;
+        case TCR_IPS_36_BIT:
+            output_size = IPS_36_BIT;
+            break;
+        case TCR_IPS_40_BIT:
+            output_size = IPS_40_BIT;
+            break;
+        case TCR_IPS_42_BIT:
+            output_size = IPS_42_BIT;
+            break;
+        case TCR_IPS_44_BIT:
+            output_size = IPS_44_BIT;
+            break;
+        case TCR_IPS_48_BIT:
+        default:
+            output_size = IPS_48_BIT;
+        }
+    }
+    else
+        output_size = IPS_40_BIT;
+
+    /* Make sure the base address does not exceed its configured size. */
+    mask = ((1ULL << IPS_48_BIT) - 1) & ~((1ULL << output_size) - 1);
+    if ( output_size != IPS_48_BIT && (ttbr & mask) )
+        return -EFAULT;
+
+    mask = ((1ULL << output_size) - 1);
+    page = get_page_from_gfn(d, paddr_to_pfn(ttbr & mask), NULL, P2M_ALLOC);
+    if ( !page )
+        return -EFAULT;
+
+    table = __map_domain_page(page);
+
+    for ( ; ; level++ )
+    {
+        pte = table[offsets[level][gran]];
+
+        unmap_domain_page(table);
+        put_page(page);
+
+        if ( level == 3 || !pte.walk.valid || !pte.walk.table )
+            break;
+
+        page = get_page_from_gfn(d, pte.walk.base, NULL, P2M_ALLOC);
+        if ( !page )
+            return -EFAULT;
+
+        table = __map_domain_page(page);
+    }
+
+    if ( !pte.walk.valid || ((level == 3) & !pte.walk.table) )
+        return -EFAULT;
+
+    *ipa = pfn_to_paddr(pte.walk.base) | (gva & masks[level][gran]);
+
+    *perm_ro = pte.pt.ro;
+
+    /* Return the entire pte so that the caller can check flags by herself. */
+    return 0;
 }
 
 int p2m_walk_gpt(struct p2m_domain *p2m, vaddr_t gva,
-- 
2.12.2


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

Re: [RFC PATCH v2 6/8] arm/mem_access: Add long-descriptor based gpt

[Julien Grall]

Hi Sergej,

On 06/01/2017 04:18 PM, Sergej Proskurin wrote:
> This commit adds functionality to walk the guest's page tables using the
> long-descriptor translation table format for both ARMv7 and ARMv8.
> Similar to the hardware architecture, the implementation supports
> different page granularities (4K, 16K, and 64K). The implementation is
> based on ARM DDI 0487A-g J11-5608, J11-5679, and ARM DDI 0406C-b
> B3-1510.

Please use the most recent ARM ARM.

> 
> Note that the current implementation lacks support for 32-bit EL0
> running on top of 64-bit EL1. 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.
> ---
>   xen/arch/arm/p2m.c | 271 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 269 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 0337d83581..ea3be6f050 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1573,8 +1573,275 @@ static int __p2m_walk_gpt_ld(struct p2m_domain *p2m,
>                                vaddr_t gva, paddr_t *ipa,
>                                unsigned int *perm_ro)
>   {
> -    /* Not implemented yet. */
> -    return -EFAULT;
> +    int t0_sz, t1_sz, disabled = 1;

Looking at the way you use t0_sz and t1_sz, they will never be < 0 so I 
thin should be unsigned.

Also, I think disabled is always 0 or 1. If that is true, please use 
bool/true/false.

> +    unsigned int level, gran;
> +    unsigned int topbit = 0, input_size = 0, output_size;
> +    uint64_t ttbr = 0, ips;
> +    paddr_t mask;
> +    lpae_t pte, *table;
> +    struct page_info *page;
> +    register_t tcr = READ_SYSREG(TCR_EL1);
> +    struct domain *d = p2m->domain;
> +
> +    const vaddr_t offsets[4][3] = {
> +        {
> +#ifdef CONFIG_ARM_64
> +            zeroeth_guest_table_offset_4k(gva),
> +            zeroeth_guest_table_offset_16k(gva),
> +            0, /* There is no zeroeth lookup level with a 64K granule size. */
> +#endif
> +        },
> +        {
> +            first_guest_table_offset_4k(gva),
> +#ifdef CONFIG_ARM_64
> +            first_guest_table_offset_16k(gva),
> +            first_guest_table_offset_64k(gva),
> +#endif
> +        },
> +        {
> +            second_guest_table_offset_4k(gva),
> +#ifdef CONFIG_ARM_64
> +            second_guest_table_offset_16k(gva),
> +            second_guest_table_offset_64k(gva),
> +#endif
> +        },
> +        {
> +            third_guest_table_offset_4k(gva),
> +#ifdef CONFIG_ARM_64
> +            third_guest_table_offset_16k(gva),
> +            third_guest_table_offset_64k(gva),
> +#endif
> +        }
> +    };
> +
> +    const paddr_t masks[4][3] = {
> +        {
> +            ZEROETH_SIZE_4K - 1,
> +            ZEROETH_SIZE_16K - 1,
> +            0 /* There is no zeroeth lookup level with a 64K granule size. */
> +        },
> +        {
> +            FIRST_SIZE_4K - 1,
> +            FIRST_SIZE_16K - 1,
> +            FIRST_SIZE_64K - 1
> +        },
> +        {
> +            SECOND_SIZE_4K - 1,
> +            SECOND_SIZE_16K - 1,
> +            SECOND_SIZE_64K - 1
> +        },
> +        {
> +            THIRD_SIZE_4K - 1,
> +            THIRD_SIZE_16K - 1,
> +            THIRD_SIZE_64K - 1
> +        }
> +    };

Please define them as static. It is not necessary to have them on the 
stack everytime.

> +
> +    const unsigned int grainsizes[3] = {
> +        PAGE_SHIFT_4K,
> +        PAGE_SHIFT_16K,
> +        PAGE_SHIFT_64K > +    };

Ditto.

> +
> +    const unsigned int strides[3] = {
> +        LPAE_SHIFT_4K,
> +        LPAE_SHIFT_16K,
> +        LPAE_SHIFT_64K
> +    };

Ditto. Also, the stride can be found from the page shift. So I am not 
convinced you need that.

> +
> +    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, according to "AddrTop" pseudocode

That's a call to have a separate helper for AddrTop rather than 
open-coding in this function.

I think this function could be split in multiple part, making easier the 
review. For instance, you have a lot of "if is_*bit_domain".

> +     * implementation in ARM DDI 0487A-g J11-5739.
> +     *
> +     * XXX: We do not check whether the 64bit domain uses a 32-bit EL0. In this
> +     * case, we need to set topbit to 31, as well.
I think checking 32-bit EL0 is straigh-forward enough to get it done 
now. Have a look at psr_most_is_32bit.

> +     */
> +    if ( is_32bit_domain(d) )
> +        topbit = TCR_TB_31;
> +#ifdef CONFIG_ARM_64
> +    else if ( is_64bit_domain(d) )
> +    {
> +        if ( ((gva & (1UL << TCR_TB_55)) && (tcr & TCR_TBI1)) ||

Please use BIT(...) instead of (1UL << ...).

> +             (!(gva & (1UL << TCR_TB_55)) && (tcr & TCR_TBI0)) )

Ditto.

> +            topbit = TCR_TB_55;

This is really confusing. You define TCR_TB_* to * but it is not even 
part of the register TCR_. TBH, I don't think they hence the code and 
would just hardcoded the 55 here. Afterall it is in the name :).

> +        else
> +            topbit = TCR_TB_63

Ditto.

> +    }
> +#endif
> +
> +#ifdef CONFIG_ARM_64
> +    if ( is_64bit_domain(d) )
> +    {

Likely a comment is missing here to explain what you are doing below. My 
understand is you are selecting TTBR*_EL1. And looking at the code, this 
could be abstracted as both branches are nearly the same.

> +        if ( (gva & (1UL << topbit)) == 0 )

BIT(...)

> +        {
> +            input_size = REGISTER_WIDTH_64_BIT - t0_sz;
> +
> +            if ( input_size > IPS_MAX )
> +                /* We limit the input_size to be max 48 bit. */
> +                input_size = IPS_MAX;
> +            else if ( input_size < IPS_MIN )
> +                /* We limit the input_size to be max 25 bit. */
> +                input_size = IPS_MIN;

like this could be simplified by using min/max. But I think we should 
bail out here. Likely something in the page table is wrong and ignoring 
is the worst thing to do.

For instance ARMv8.2 has extended the input size to 52 bits. It would be 
difficult to catch what is missing because of that, not mentioning that 
the only caller today will be memaccess that is not enabled by default.

> +
> +            /* Normalize granule size. */

I think 0, 1, 2 is more confusing to read. It would be better to use 
directly TCR_TG0_*.

> +            switch ( tcr & TCR_TG0_MASK ) {

Coding style, the brace should be on a newline.

> +            case TCR_TG0_16K:
> +                gran = 1;
> +                break;
> +            case TCR_TG0_64K:
> +                gran = 2;
> +                break;
> +            default:
> +                gran = 0;
> +            } > +
> +            /* 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 ) ? 1 : 0;

disabled = !!(tcr & TCR_EPD0);

or if you are using bool as requested above:

disabled =  tcr & TCR_EPD0;

> +        }
> +        else
> +        {
> +            input_size = REGISTER_WIDTH_64_BIT - t1_sz;
> +
> +            if ( input_size > IPS_MAX )
> +                /* We limit the input_size to be max 48 bit. */
> +                input_size = IPS_MAX;
> +            else if ( input_size < IPS_MIN )
> +                /* We limit the input_size to be max 25 bit. */
> +                input_size = IPS_MIN;
> +
> +            /* Normalize granule size. */
> +            switch ( tcr & TCR_TG1_MASK ) {

Coding style.

> +            case TCR_TG1_16K:
If you shift your tcr by TCR_TG1_SHIFT then all this code can become 
generic. Avoiding duplication, reviewing twice similar code and 
potential bug.

> +                gran = 1;
> +                break;
> +            case TCR_TG1_64K:
> +                gran = 2;
> +                break;
> +            default:
> +                gran = 0;
> +            }
> +
> +            /* 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 ) ? 1 : 0;

Same as above.

> +        }
> +    }
> +    else
> +#endif
> +    {
> +        /* Granule size of Aarch32 or ARMv7 architectures is always 4K, indexed by 0. */

NIT: It is not necessarily to mention ARMv7. ARMv7 is always AArch32.

Also s/Aarch32/Aarch32/

> +        gran = 0;
> +
> +        mask = ((1ULL << REGISTER_WIDTH_32_BIT) - 1) &
> +               ~((1ULL << (REGISTER_WIDTH_32_BIT - t0_sz)) - 1);

I don't understand this mask. Why do you need it?

> +
> +        if ( t0_sz == 0 || !(gva & mask) )
> +        {
> +            input_size = REGISTER_WIDTH_32_BIT - t0_sz;
> +
> +            /* Use TTBR0 for GVA to IPA translation. */
> +            ttbr = READ_SYSREG64(TTBR0_EL1);
> +
> +            /* If TCR.EPD0 is set, translations using TTBR0 are disabled. */
> +            disabled = ( tcr & TCR_EPD0 ) ? 1 : 0;

As for the AArch64, there is a call to abstract here.

> +        }
> +
> +        mask = ((1ULL << REGISTER_WIDTH_32_BIT) - 1) &
> +               ~((1ULL << (REGISTER_WIDTH_32_BIT - t1_sz)) - 1);
> +
> +        if ( ((t1_sz == 0) && !ttbr) || (t1_sz && (gva & mask) == mask) )
> +        {
> +            input_size = REGISTER_WIDTH_32_BIT - t1_sz;
> +
> +            /* Use TTBR1 for GVA to IPA translation. */
> +            ttbr = READ_SYSREG64(TTBR1_EL1);
> +
> +            /* If TCR.EPD1 is set, translations using TTBR1 are disabled. */
> +            disabled = ( tcr & TCR_EPD1 ) ? 1 : 0;

Ditto.

> +        }
> +    }
> +
> +    if ( disabled )
> +        return -EFAULT;
> +
> +    level = 4 - DIV_ROUND_UP((input_size - grainsizes[gran]), strides[gran]);

It took me a bit to understand this. The comment in the ARM ARM 
pseudo-code would be useful to keep it here.

> +
> +    /* XXX: We do not consider 32bit EL0 running on Aarch64, yet. */

See my comment above about 32bit EL0 support.

> +    if ( is_64bit_domain(d) )
> +    {
> +        /* Get the intermediate physical address size. */
> +        ips = (tcr & TCR_IPS_MASK) >> TCR_IPS_SHIFT;
> +
> +        switch (ips) {
> +        case TCR_IPS_32_BIT:
> +            output_size = IPS_32_BIT;
> +            break;
> +        case TCR_IPS_36_BIT:
> +            output_size = IPS_36_BIT;
> +            break;
> +        case TCR_IPS_40_BIT:
> +            output_size = IPS_40_BIT;
> +            break;
> +        case TCR_IPS_42_BIT:
> +            output_size = IPS_42_BIT;
> +            break;
> +        case TCR_IPS_44_BIT:
> +            output_size = IPS_44_BIT;
> +            break;
> +        case TCR_IPS_48_BIT:
> +        default:
> +            output_size = IPS_48_BIT;
> +        }
> +    }
> +    else
> +        output_size = IPS_40_BIT;
> +
> +    /* Make sure the base address does not exceed its configured size. */
> +    mask = ((1ULL << IPS_48_BIT) - 1) & ~((1ULL << output_size) - 1);
> +    if ( output_size != IPS_48_BIT && (ttbr & mask) )
> +        return -EFAULT;
> +
> +    mask = ((1ULL << output_size) - 1);
> +    page = get_page_from_gfn(d, paddr_to_pfn(ttbr & mask), NULL, P2M_ALLOC);
> +    if ( !page )
> +        return -EFAULT;
> +
> +    table = __map_domain_page(page);
> +
> +    for ( ; ; level++ )
> +    {
> +        pte = table[offsets[level][gran]];
> +
> +        unmap_domain_page(table);
> +        put_page(page);
> +
> +        if ( level == 3 || !pte.walk.valid || !pte.walk.table )

Please comment this line.

> +            break;
> +
> +        page = get_page_from_gfn(d, pte.walk.base, NULL, P2M_ALLOC);

It is a bit confusing. You care about the output_size for the when 
getting the TBBR but not the rest of the tables. Is it normal?

> +        if ( !page )
> +            return -EFAULT;
> +
> +        table = __map_domain_page(page);
> +    }
> +
> +    if ( !pte.walk.valid || ((level == 3) & !pte.walk.table) )

There is a call to leverage the p2m_table/p2m_valid helpers here.

> +        return -EFAULT;
> +
> +    *ipa = pfn_to_paddr(pte.walk.base) | (gva & masks[level][gran]);
> +
> +    *perm_ro = pte.pt.ro;
> +
> +    /* Return the entire pte so that the caller can check flags by herself. */

Hmmm? You don't return the entire pte but the only whether the page is 
writable or readable-only.

In general we want to have more information back such as execute-never 
bit...

> +    return 0;
>   }
>   
>   int p2m_walk_gpt(struct p2m_domain *p2m, vaddr_t gva,
> 

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH v2 6/8] arm/mem_access: Add long-descriptor based gpt

[Sergej Proskurin]

Hi Julien,

[...]

>
>> +        {
>> +            input_size = REGISTER_WIDTH_64_BIT - t0_sz;
>> +
>> +            if ( input_size > IPS_MAX )
>> +                /* We limit the input_size to be max 48 bit. */
>> +                input_size = IPS_MAX;
>> +            else if ( input_size < IPS_MIN )
>> +                /* We limit the input_size to be max 25 bit. */
>> +                input_size = IPS_MIN;
>
> like this could be simplified by using min/max. But I think we should
> bail out here. Likely something in the page table is wrong and
> ignoring is the worst thing to do.
>
> For instance ARMv8.2 has extended the input size to 52 bits. It would
> be difficult to catch what is missing because of that, not mentioning
> that the only caller today will be memaccess that is not enabled by
> default.
>

Agreed.

>> +
>> +            /* Normalize granule size. */
>
> I think 0, 1, 2 is more confusing to read. It would be better to use
> directly TCR_TG0_*.
>

I agree, however the ARM architecture uses different granularity
encodigs for TG0 and TG1. That is the values for (tcr & TCR_TG0_MASK) >>
TCR_TG0_SHIFT are different for the same granularity (e.g. shifted
TCR_TG0_4K == 0x0 vs. TCR_EL1_TG1_4K == 0x2).


Because of this, we won't be able to use TCR_TG0_* and TCR_TG1_*
directly. It would be probably easier to read/review the code if a part
of this functionality would be in a separate function (e.g.
get_granularity()), though. I will see what I can do at this point.

>> +            switch ( tcr & TCR_TG0_MASK ) {
>> +            case TCR_TG0_16K:
>> +                gran = 1;
>> +                break;
>> +            case TCR_TG0_64K:
>> +                gran = 2;
>> +                break;
>> +            default:
>> +                gran = 0;
>> +            } > +
>> +            /* 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 ) ? 1 : 0;
>> +        }
>> +        else
>> +        {
>> +            input_size = REGISTER_WIDTH_64_BIT - t1_sz;
>> +
>> +            if ( input_size > IPS_MAX )
>> +                /* We limit the input_size to be max 48 bit. */
>> +                input_size = IPS_MAX;
>> +            else if ( input_size < IPS_MIN )
>> +                /* We limit the input_size to be max 25 bit. */
>> +                input_size = IPS_MIN;
>> +
>> +            /* Normalize granule size. */
>> +            switch ( tcr & TCR_TG1_MASK ) {
>> +            case TCR_TG1_16K:
> If you shift your tcr by TCR_TG1_SHIFT then all this code can become
> generic. Avoiding duplication, reviewing twice similar code and
> potential bug.

Please see my comment above.

[...]

Cheers,
~Sergej


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

Re: [RFC PATCH v2 6/8] arm/mem_access: Add long-descriptor based gpt

[Julien Grall]

Hi Sergej,

On 09/06/17 12:50, Sergej Proskurin wrote:
>>> +
>>> +            /* Normalize granule size. */
>>
>> I think 0, 1, 2 is more confusing to read. It would be better to use
>> directly TCR_TG0_*.
>>
>
> I agree, however the ARM architecture uses different granularity
> encodigs for TG0 and TG1. That is the values for (tcr & TCR_TG0_MASK) >>
> TCR_TG0_SHIFT are different for the same granularity (e.g. shifted
> TCR_TG0_4K == 0x0 vs. TCR_EL1_TG1_4K == 0x2).
>
>
> Because of this, we won't be able to use TCR_TG0_* and TCR_TG1_*
> directly. It would be probably easier to read/review the code if a part
> of this functionality would be in a separate function (e.g.
> get_granularity()), though. I will see what I can do at this point.

You are right sorry. However, I still think open-code 0,1,2 is not 
intuitive. You should introduce proper define for that.

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH v2 6/8] arm/mem_access: Add long-descriptor based gpt

[Sergej Proskurin]

Hi Julien,


>
>> +
>> +    const unsigned int strides[3] = {
>> +        LPAE_SHIFT_4K,
>> +        LPAE_SHIFT_16K,
>> +        LPAE_SHIFT_64K
>> +    };
>
> Also, the stride can be found from the page shift. So I am not
> convinced you need that.

Sure, but don't you think it is cleaner doing it that way, than
subtracting a value from PAGE_SHIFT_* although we have already a
suitable define for that? Apart from that, we need to consider different
strides for different granularities, which would make it harder to
read/review if we don't use an array of strides at this point. For
instance, see the following formula to compute the starting level of the
translation tables:

level = 4 - DIV_ROUND_UP((input_size - grainsizes[gran]), strides[gran]);

>
>> +     * implementation in ARM DDI 0487A-g J11-5739.
>> +     *
>> +     * XXX: We do not check whether the 64bit domain uses a 32-bit
>> EL0. In this
>> +     * case, we need to set topbit to 31, as well.
> I think checking 32-bit EL0 is straigh-forward enough to get it done
> now. Have a look at psr_most_is_32bit.

Apparently, I have misunderstood the comment in the AddrTop pseudo-code
implementation. So, we don't need to check whether EL0 is running in
AArch32 as long as EL1 runs in AArch64. In this way, the GVA's of EL0
would be simply zero-extended to 64 bits (i.e., according to DDI 0487B-a
J1-6066, as far as I understand, we would use the AArch64 translation
regime for AArch64 EL1 and AArch32 EL0). It makes sense as EL1 would
otherwise need to be translated differently (i.e. EL0 using the AArch32
translation and EL1 using the AArch64 format).

Cheers,
~Sergej


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

Re: [RFC PATCH v2 6/8] arm/mem_access: Add long-descriptor based gpt

[Julien Grall]

On 12/06/17 11:12, Sergej Proskurin wrote:
> Hi Julien,

Hello Sergej,

>>
>>> +
>>> +    const unsigned int strides[3] = {
>>> +        LPAE_SHIFT_4K,
>>> +        LPAE_SHIFT_16K,
>>> +        LPAE_SHIFT_64K
>>> +    };
>>
>> Also, the stride can be found from the page shift. So I am not
>> convinced you need that.
>
> Sure, but don't you think it is cleaner doing it that way, than
> subtracting a value from PAGE_SHIFT_* although we have already a
> suitable define for that? Apart from that, we need to consider different
> strides for different granularities, which would make it harder to
> read/review if we don't use an array of strides at this point. For
> instance, see the following formula to compute the starting level of the
> translation tables:
>
> level = 4 - DIV_ROUND_UP((input_size - grainsizes[gran]), strides[gran]);

Looking at AArch64.TranslationTableWalk the stride is basically:

stride = grainsize - 3; // Log2(page size / 8 bytes)

So I still don't see how this would make the code less readable. This 
would also avoid to introduce yet another array on the stack (though it 
should really have been static) for limited purpose.

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH v2 6/8] arm/mem_access: Add long-descriptor based gpt

[Sergej Proskurin]

Hi Julien,


On 06/12/2017 12:44 PM, Julien Grall wrote:
>
>
> On 12/06/17 11:12, Sergej Proskurin wrote:
>> Hi Julien,
>
> Hello Sergej,
>
>>>
>>>> +
>>>> +    const unsigned int strides[3] = {
>>>> +        LPAE_SHIFT_4K,
>>>> +        LPAE_SHIFT_16K,
>>>> +        LPAE_SHIFT_64K
>>>> +    };
>>>
>>> Also, the stride can be found from the page shift. So I am not
>>> convinced you need that.
>>
>> Sure, but don't you think it is cleaner doing it that way, than
>> subtracting a value from PAGE_SHIFT_* although we have already a
>> suitable define for that? Apart from that, we need to consider different
>> strides for different granularities, which would make it harder to
>> read/review if we don't use an array of strides at this point. For
>> instance, see the following formula to compute the starting level of the
>> translation tables:
>>
>> level = 4 - DIV_ROUND_UP((input_size - grainsizes[gran]),
>> strides[gran]);
>
> Looking at AArch64.TranslationTableWalk the stride is basically:
>
> stride = grainsize - 3; // Log2(page size / 8 bytes)
>
> So I still don't see how this would make the code less readable. This
> would also avoid to introduce yet another array on the stack (though
> it should really have been static) for limited purpose

Yeap, I have already made both of the arrays static. However, removing
the strides array completely is absolutely fine by me too.

Cheers,
~Sergej

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

[RFC PATCH v2 7/8] 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 0487A-g G4-4189 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>
---
 xen/arch/arm/p2m.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 123 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index ea3be6f050..fa112b873c 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1558,8 +1558,129 @@ static int __p2m_walk_gpt_sd(struct p2m_domain *p2m,
                              vaddr_t gva, paddr_t *ipa,
                              unsigned int *perm_ro)
 {
-    /* Not implemented yet. */
-    return -EFAULT;
+    int disabled = 1;
+    int32_t ttbr;
+    paddr_t mask;
+    pte_sd_t pte, *table;
+    struct page_info *page;
+    register_t ttbcr = READ_SYSREG(TCR_EL1);
+    unsigned int level = 0, n = ttbcr & TTBCR_N_MASK;
+    struct domain *d = p2m->domain;
+
+    const paddr_t offsets[2] = {
+        ((paddr_t)(gva >> 20) & ((1ULL << (12 - n)) - 1)),
+        ((paddr_t)(gva >> 12) & ((1ULL << 8) - 1))
+    };
+
+    /* TODO: Do the same (31 bit) with LPAE code!! */
+    mask = ((1ULL << REGISTER_WIDTH_32_BIT) - 1) &
+           ~((1ULL << (REGISTER_WIDTH_32_BIT - n)) - 1);
+
+    if ( n == 0 || !(gva & mask) )
+    {
+        /* Use TTBR0 for GVA to IPA translation. */
+        ttbr = READ_SYSREG64(TTBR0_EL1);
+
+        /* If TTBCR.PD0 is set, translations using TTBR0 are disabled. */
+        disabled = ( ttbcr & TTBCR_PD0 ) ? 1 : 0;
+    }
+    else
+    {
+        /* Use TTBR1 for GVA to IPA translation. */
+        ttbr = READ_SYSREG64(TTBR1_EL1);
+
+        /* If TTBCR.PD1 is set, translations using TTBR1 are disabled. */
+        disabled = ( ttbcr & TTBCR_PD1 ) ? 1 : 0;
+    }
+
+    if ( disabled )
+        return -EFAULT;
+
+    mask = (1ULL << (14 - n)) - 1;
+    page = get_page_from_gfn(d, paddr_to_pfn(ttbr & ~mask), NULL, P2M_ALLOC);
+    if ( !page )
+        return -EFAULT;
+
+    /*
+     * XXX: The 2nd level lookup table might comprise 4 concatenated 4K
+     * pages.  Check how to map concatenated tables at once.
+     */
+    table = __map_domain_page(page);
+
+    /* Consider offset if n > 2. */
+    if ( n > 2 )
+        table = (pte_sd_t *)((unsigned long)table | (unsigned long)(ttbr & mask));
+
+    pte = table[offsets[level]];
+
+    unmap_domain_page(table);
+    put_page(page);
+
+    switch ( pte.walk.dt ) {
+    case 0: /* Invalid mapping. */
+        return -EFAULT;
+
+    case 1: /* Large or small page. */
+        level++;
+
+        page = get_page_from_gfn(d, (pte.walk.base >> 2), NULL, P2M_ALLOC);
+        if ( !page )
+            return -EFAULT;
+
+        table = __map_domain_page(page);
+        table = (pte_sd_t *)((unsigned long)table | ((pte.walk.base & 0x3) << 10));
+
+        pte = table[offsets[level]];
+
+        unmap_domain_page(table);
+        put_page(page);
+
+        if ( pte.walk.dt == 0 )
+            break;
+
+        if ( pte.walk.dt & 0x2 ) /* Small page. */
+        {
+            mask = (1ULL << PAGE_SHIFT_4K) - 1;
+            *ipa = (pte.bits & ~mask) | (gva & mask);
+        }
+        else /* Large page. */
+        {
+            mask = (1ULL << PAGE_SHIFT_64K) - 1;
+            *ipa = (pte.bits & ~mask) | (gva & mask);
+        }
+
+        /* Set access permissions[2:0]. */
+        *perm_ro = (pte.bits & 0x200) >> 9;
+
+        break;
+
+    case 2: /* Section. */
+    case 3: /* Section or Supersection. */
+        if ( !(pte.bits & (1ULL << 18)) ) /* Section */
+        {
+            mask = (1ULL << 20) - 1;
+            *ipa = (pte.bits & ~mask) | (gva & mask);
+        }
+        else /* Supersection */
+        {
+            mask = (1ULL << 24) - 1;
+            *ipa = (pte.bits & ~mask) | (gva & mask);
+
+            mask = ((1ULL << 24) - 1) & ~((1ULL << 20) - 1);
+            *ipa |= (pte.bits & mask) << 32;
+
+            mask = ((1ULL << 9) - 1) & ~((1ULL << 5) - 1);
+            *ipa |= (pte.bits & mask) << 36;
+        }
+
+        /* Set access permission[2]. */
+        *perm_ro = (pte.bits & 0x8000) >> 15;
+    }
+
+    if ( pte.walk.dt == 0 )
+        return -EFAULT;
+
+    return 0;
 }
 
 /*
-- 
2.12.2


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

Re: [RFC PATCH v2 7/8] arm/mem_access: Add short-descriptor based gpt

[Julien Grall]

Hi Sergej,

On 06/01/2017 04:18 PM, 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 0487A-g G4-4189 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>
> ---
>   xen/arch/arm/p2m.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 123 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index ea3be6f050..fa112b873c 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1558,8 +1558,129 @@ static int __p2m_walk_gpt_sd(struct p2m_domain *p2m,
>                                vaddr_t gva, paddr_t *ipa,
>                                unsigned int *perm_ro)
>   {
> -    /* Not implemented yet. */
> -    return -EFAULT;
> +    int disabled = 1;

This should be bool as you use 0/1.

> +    int32_t ttbr;

Likely you want to use uint64_t here.

> +    paddr_t mask;
> +    pte_sd_t pte, *table;
> +    struct page_info *page;
> +    register_t ttbcr = READ_SYSREG(TCR_EL1);
> +    unsigned int level = 0, n = ttbcr & TTBCR_N_MASK;
> +    struct domain *d = p2m->domain;
> +
> +    const paddr_t offsets[2] = {
> +        ((paddr_t)(gva >> 20) & ((1ULL << (12 - n)) - 1)),
> +        ((paddr_t)(gva >> 12) & ((1ULL << 8) - 1))
> +    };
> +
> +    /* TODO: Do the same (31 bit) with LPAE code!! */
> +    mask = ((1ULL << REGISTER_WIDTH_32_BIT) - 1) &
> +           ~((1ULL << (REGISTER_WIDTH_32_BIT - n)) - 1);
> +
> +    if ( n == 0 || !(gva & mask) )
> +    {
> +        /* Use TTBR0 for GVA to IPA translation. */
> +        ttbr = READ_SYSREG64(TTBR0_EL1);
> +
> +        /* If TTBCR.PD0 is set, translations using TTBR0 are disabled. */
> +        disabled = ( ttbcr & TTBCR_PD0 ) ? 1 : 0
disable = ttbcr & TTBCR_PD0;

> +    }
> +    else
> +    {
> +        /* Use TTBR1 for GVA to IPA translation. */
> +        ttbr = READ_SYSREG64(TTBR1_EL1);
> +
> +        /* If TTBCR.PD1 is set, translations using TTBR1 are disabled. */
> +        disabled = ( ttbcr & TTBCR_PD1 ) ? 1 : 0;

Ditto.

> +    }
> +
> +    if ( disabled )
> +        return -EFAULT;
> +
> +    mask = (1ULL << (14 - n)) - 1;

Please explain the 14 here;

> +    page = get_page_from_gfn(d, paddr_to_pfn(ttbr & ~mask), NULL, P2M_ALLOC);
> +    if ( !page )
> +        return -EFAULT;
> +
> +    /*
> +     * XXX: The 2nd level lookup table might comprise 4 concatenated 4K
> +     * pages.  Check how to map concatenated tables at once.
> +     */

You will not be able to map concatenated tables at once because they may 
not be contiguous in guest memory. Though you could use vmap.

But in this case, I would only look for the page used and only mapped 
that one.

> +    table = __map_domain_page(page);
> +
> +    /* Consider offset if n > 2. */
> +    if ( n > 2 )
> +        table = (pte_sd_t *)((unsigned long)table | (unsigned long)(ttbr & mask));
What are you trying to achieve here?

> +
> +    pte = table[offsets[level]];
> +
> +    unmap_domain_page(table);
> +    put_page(page);
> +
> +    switch ( pte.walk.dt ) {

Coding style.

> +    case 0: /* Invalid mapping. */
> +        return -EFAULT;
> +
> +    case 1: /* Large or small page. */

Hmmm, dt == 1 means page table. Not small/large.

> +        level++;
> +
> +        page = get_page_from_gfn(d, (pte.walk.base >> 2), NULL, P2M_ALLOC);
> +        if ( !page )
> +            return -EFAULT;
> +
> +        table = __map_domain_page(page);
> +        table = (pte_sd_t *)((unsigned long)table | ((pte.walk.base & 0x3) << 10));

Same as above. What are you trying to achieve here? Also I am quite 
confuse with the pte.walk.base & 0x3.

> +
> +        pte = table[offsets[level]];
> +
> +        unmap_domain_page(table);
> +        put_page(page);
> +
> +        if ( pte.walk.dt == 0 )

Please avoid hardcode value when possible. When I read 0 here I don't 
know what it means.

> +            break;
> +
> +        if ( pte.walk.dt & 0x2 ) /* Small page. */

Please avoid hardcode value.

> +        {
> +            mask = (1ULL << PAGE_SHIFT_4K) - 1 > +            *ipa = (pte.bits & ~mask) | (gva & mask);

You really don't need to duplicate that line and...

> +        }
> +        else /* Large page. */
> +        {
> +            mask = (1ULL << PAGE_SHIFT_64K) - 1;
> +            *ipa = (pte.bits & ~mask) | (gva & mask);

... and that one.

> +        }
> +
> +        /* Set access permissions[2:0]. */
> +        *perm_ro = (pte.bits & 0x200) >> 9;

No hardcoding value please. And looking at the LPAE version, you are 
only setting one bit there but 2 bits here. How the caller will no what 
to do?

> +
> +        break;
> +
> +    case 2: /* Section. */
> +    case 3: /* Section or Supersection. */

Both 2 and 3 may point to Section or Supersection.

> +        if ( !(pte.bits & (1ULL << 18)) ) /* Section */

Please don't hardcode 18.

> +        {
> +            mask = (1ULL << 20) - 1;

Same here.

> +            *ipa = (pte.bits & ~mask) | (gva & mask);
> +        }
> +        else /* Supersection */
> +        {
> +            mask = (1ULL << 24) - 1;

Same here.

> +            *ipa = (pte.bits & ~mask) | (gva & mask);
> +
> +            mask = ((1ULL << 24) - 1) & ~((1ULL << 20) - 1);

Same here.

> +            *ipa |= (pte.bits & mask) << 32;
> +
> +            mask = ((1ULL << 9) - 1) & ~((1ULL << 5) - 1);

Same here.

> +            *ipa |= (pte.bits & mask) << 36;

I don't understand why you introduce a pte_sd_walk structure in a way 
that you cannot take easily advantage. It would be better to rework it 
for your purpose.

> +        }
> +
> +        /* Set access permission[2]. */
> +        *perm_ro = (pte.bits & 0x8000) >> 15;

No hardcoding value please. And here you set one bit but 2 bits above....

> +    }
> +
> +    if ( pte.walk.dt == 0 )
> +        return -EFAULT;

Don't you already handle it in the switch?

> +
> +    return 0;
>   }
>   
>   /*
> 

Cheers,

-- 
Julien Grall

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

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

[Sergej Proskurin]

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

Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
---
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
---
v2: Check the returned access rights after walking the guest's page tables in
    the function p2m_mem_access_check_and_get_page.
---
 xen/arch/arm/mem_access.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

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


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

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

[Sergej Proskurin]

This commit adds (TCR_|TTBCR_)* defines to simplify access to the respective
register contents.

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.
---
 xen/include/asm-arm/processor.h | 49 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 855ded1b07..c095dad7e9 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -94,6 +94,9 @@
 #define TTBCR_N_2KB  _AC(0x03,U)
 #define TTBCR_N_1KB  _AC(0x04,U)
 
+#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)
@@ -155,6 +158,19 @@
 /* TCR: Stage 1 Translation Control */
 
 #define TCR_T0SZ(x)     ((x)<<0)
+#define TCR_T0SZ_SHIFT  (0)
+#define TCR_T1SZ_SHIFT  (16)
+
+/*
+ * According to ARM DDI 0487A.g, TCR_EL1.{T0SZ,T1SZ} (Aarch64, Section D7-2021)
+ * comprises 6 bits and TTBCR.{T0SZ,T1SZ} (Aarch32, Section G6-4624) comprises
+ * 3 bits following another 3 bits for RES0. Thus, the mask for both registers
+ * should be 0x3f.
+ */
+#define TCR_SZ_MASK     (_AC(0x3f,UL)<<0)
+
+#define TCR_EPD0        (_AC(0x1,UL)<<7)
+#define TCR_EPD1        (_AC(0x1,UL)<<23)
 
 #define TCR_IRGN0_NC    (_AC(0x0,UL)<<8)
 #define TCR_IRGN0_WBWA  (_AC(0x1,UL)<<8)
@@ -173,11 +189,35 @@
 #define TCR_TG0_4K      (_AC(0x0,UL)<<14)
 #define TCR_TG0_64K     (_AC(0x1,UL)<<14)
 #define TCR_TG0_16K     (_AC(0x2,UL)<<14)
+#define TCR_TG0_MASK    (_AC(0x3,UL)<<14)
+#define TCR_TG0_SHIFT   (14)
+
+#define TCR_TG1_16K     (_AC(0x1,UL)<<30)
+#define TCR_TG1_4K      (_AC(0x2,UL)<<30)
+#define TCR_TG1_64K     (_AC(0x3,UL)<<30)
+#define TCR_TG1_MASK    (_AC(0x3,UL)<<30)
+#define TCR_TG1_SHIFT   (30)
+
+#define TCR_IPS_32_BIT  (_AC(0x0,ULL)<<32)
+#define TCR_IPS_36_BIT  (_AC(0x1,ULL)<<32)
+#define TCR_IPS_40_BIT  (_AC(0x2,ULL)<<32)
+#define TCR_IPS_42_BIT  (_AC(0x3,ULL)<<32)
+#define TCR_IPS_44_BIT  (_AC(0x4,ULL)<<32)
+#define TCR_IPS_48_BIT  (_AC(0x5,ULL)<<32)
+#define TCR_IPS_MASK    (_AC(0x7,ULL)<<32)
+#define TCR_IPS_SHIFT   (32)
+
+#define TCR_TB_31       (31)
 
 #ifdef CONFIG_ARM_64
 
 #define TCR_PS(x)       ((x)<<16)
 #define TCR_TBI         (_AC(0x1,UL)<<20)
+#define TCR_TBI0        (_AC(0x1,UL)<<37)
+#define TCR_TBI1        (_AC(0x1,UL)<<38)
+
+#define TCR_TB_63       (63)
+#define TCR_TB_55       (55)
 
 #define TCR_RES1        (_AC(1,UL)<<31|_AC(1,UL)<<23)
 
@@ -187,6 +227,15 @@
 
 #endif
 
+#define IPS_MIN         (25)
+#define IPS_MAX         (48)
+#define IPS_32_BIT      (32)
+#define IPS_36_BIT      (36)
+#define IPS_40_BIT      (40)
+#define IPS_42_BIT      (42)
+#define IPS_44_BIT      (44)
+#define IPS_48_BIT      (48)
+
 /* VTCR: Stage 2 Translation Control */
 
 #define VTCR_T0SZ(x)    ((x)<<0)
-- 
2.12.2


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

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

[Julien Grall]

I have two series on my inbox with the same patches. I assume it is a 
mistake and only reviewed the "RFC PATCH v2" version.

Let me know if it is not right.

Cheers,

On 06/01/2017 04:18 PM, Sergej Proskurin wrote:
> This commit adds (TCR_|TTBCR_)* defines to simplify access to the respective
> register contents.
> 
> 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.
> ---
>   xen/include/asm-arm/processor.h | 49 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 49 insertions(+)
> 
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 855ded1b07..c095dad7e9 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -94,6 +94,9 @@
>   #define TTBCR_N_2KB  _AC(0x03,U)
>   #define TTBCR_N_1KB  _AC(0x04,U)
>   
> +#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)
> @@ -155,6 +158,19 @@
>   /* TCR: Stage 1 Translation Control */
>   
>   #define TCR_T0SZ(x)     ((x)<<0)
> +#define TCR_T0SZ_SHIFT  (0)
> +#define TCR_T1SZ_SHIFT  (16)
> +
> +/*
> + * According to ARM DDI 0487A.g, TCR_EL1.{T0SZ,T1SZ} (Aarch64, Section D7-2021)
> + * comprises 6 bits and TTBCR.{T0SZ,T1SZ} (Aarch32, Section G6-4624) comprises
> + * 3 bits following another 3 bits for RES0. Thus, the mask for both registers
> + * should be 0x3f.
> + */
> +#define TCR_SZ_MASK     (_AC(0x3f,UL)<<0)
> +
> +#define TCR_EPD0        (_AC(0x1,UL)<<7)
> +#define TCR_EPD1        (_AC(0x1,UL)<<23)
>   
>   #define TCR_IRGN0_NC    (_AC(0x0,UL)<<8)
>   #define TCR_IRGN0_WBWA  (_AC(0x1,UL)<<8)
> @@ -173,11 +189,35 @@
>   #define TCR_TG0_4K      (_AC(0x0,UL)<<14)
>   #define TCR_TG0_64K     (_AC(0x1,UL)<<14)
>   #define TCR_TG0_16K     (_AC(0x2,UL)<<14)
> +#define TCR_TG0_MASK    (_AC(0x3,UL)<<14)
> +#define TCR_TG0_SHIFT   (14)
> +
> +#define TCR_TG1_16K     (_AC(0x1,UL)<<30)
> +#define TCR_TG1_4K      (_AC(0x2,UL)<<30)
> +#define TCR_TG1_64K     (_AC(0x3,UL)<<30)
> +#define TCR_TG1_MASK    (_AC(0x3,UL)<<30)
> +#define TCR_TG1_SHIFT   (30)
> +
> +#define TCR_IPS_32_BIT  (_AC(0x0,ULL)<<32)
> +#define TCR_IPS_36_BIT  (_AC(0x1,ULL)<<32)
> +#define TCR_IPS_40_BIT  (_AC(0x2,ULL)<<32)
> +#define TCR_IPS_42_BIT  (_AC(0x3,ULL)<<32)
> +#define TCR_IPS_44_BIT  (_AC(0x4,ULL)<<32)
> +#define TCR_IPS_48_BIT  (_AC(0x5,ULL)<<32)
> +#define TCR_IPS_MASK    (_AC(0x7,ULL)<<32)
> +#define TCR_IPS_SHIFT   (32)
> +
> +#define TCR_TB_31       (31)
>   
>   #ifdef CONFIG_ARM_64
>   
>   #define TCR_PS(x)       ((x)<<16)
>   #define TCR_TBI         (_AC(0x1,UL)<<20)
> +#define TCR_TBI0        (_AC(0x1,UL)<<37)
> +#define TCR_TBI1        (_AC(0x1,UL)<<38)
> +
> +#define TCR_TB_63       (63)
> +#define TCR_TB_55       (55)
>   
>   #define TCR_RES1        (_AC(1,UL)<<31|_AC(1,UL)<<23)
>   
> @@ -187,6 +227,15 @@
>   
>   #endif
>   
> +#define IPS_MIN         (25)
> +#define IPS_MAX         (48)
> +#define IPS_32_BIT      (32)
> +#define IPS_36_BIT      (36)
> +#define IPS_40_BIT      (40)
> +#define IPS_42_BIT      (42)
> +#define IPS_44_BIT      (44)
> +#define IPS_48_BIT      (48)
> +
>   /* VTCR: Stage 2 Translation Control */
>   
>   #define VTCR_T0SZ(x)    ((x)<<0)
> 

-- 
Julien Grall

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

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

[Sergej Proskurin]

Hi Julien,


On 06/02/2017 05:13 PM, Julien Grall wrote:
> I have two series on my inbox with the same patches. I assume it is a
> mistake and only reviewed the "RFC PATCH v2" version.
> 
> Let me know if it is not right.


there was a mistake during the submission process so that the same patch
series has been submitted twice. So there is no need for reviewing the
second series.

Thank you very much for your review of the first series, I will try to
address your comments as soon as I can.

Cheers,
~Sergej

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

[PATCH 2/8] arm/mem_access: Add defines holding the width of 32/64bit regs

[Sergej Proskurin]

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

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

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


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

[PATCH 3/8] 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>
---
 xen/include/asm-arm/page.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 4b46e8831c..6222b1d4a2 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -500,6 +500,73 @@ static inline int gva_to_ipa(vaddr_t va, paddr_t *paddr, unsigned int flags)
 
 #define PAGE_ALIGN(x) (((x) + PAGE_SIZE - 1) & PAGE_MASK)
 
+#define LPAE_SHIFT_4K           (9)
+#define LPAE_SHIFT_16K          (11)
+#define LPAE_SHIFT_64K          (13)
+
+#define LPAE_ENTRIES_4K         (_AC(1,U) << LPAE_SHIFT_4K)
+#define LPAE_ENTRIES_16K        (_AC(1,U) << LPAE_SHIFT_16K)
+#define LPAE_ENTRIES_64K        (_AC(1,U) << LPAE_SHIFT_64K)
+
+#define LPAE_ENTRY_MASK_4K      (LPAE_ENTRIES_4K - 1)
+#define LPAE_ENTRY_MASK_16K     (LPAE_ENTRIES_16K - 1)
+#define LPAE_ENTRY_MASK_64K     (LPAE_ENTRIES_64K - 1)
+
+#define PAGE_SHIFT_4K           (12)
+#define PAGE_SHIFT_16K          (14)
+#define PAGE_SHIFT_64K          (16)
+
+#define THIRD_SHIFT_4K          (PAGE_SHIFT_4K)
+#define THIRD_SHIFT_16K         (PAGE_SHIFT_16K)
+#define THIRD_SHIFT_64K         (PAGE_SHIFT_64K)
+
+#define THIRD_SIZE_4K           ((paddr_t)1 << THIRD_SHIFT_4K)
+#define THIRD_SIZE_16K          ((paddr_t)1 << THIRD_SHIFT_16K)
+#define THIRD_SIZE_64K          ((paddr_t)1 << THIRD_SHIFT_64K)
+
+#define SECOND_SHIFT_4K         (THIRD_SHIFT_4K + LPAE_SHIFT_4K)
+#define SECOND_SHIFT_16K        (THIRD_SHIFT_16K + LPAE_SHIFT_16K)
+#define SECOND_SHIFT_64K        (THIRD_SHIFT_64K + LPAE_SHIFT_64K)
+
+#define SECOND_SIZE_4K          ((paddr_t)1 << SECOND_SHIFT_4K)
+#define SECOND_SIZE_16K         ((paddr_t)1 << SECOND_SHIFT_16K)
+#define SECOND_SIZE_64K         ((paddr_t)1 << SECOND_SHIFT_64K)
+
+#define FIRST_SHIFT_4K          (SECOND_SHIFT_4K + LPAE_SHIFT_4K)
+#define FIRST_SHIFT_16K         (SECOND_SHIFT_16K + LPAE_SHIFT_16K)
+#define FIRST_SHIFT_64K         (SECOND_SHIFT_64K + LPAE_SHIFT_64K)
+
+#define FIRST_SIZE_4K           ((paddr_t)1 << FIRST_SHIFT_4K)
+#define FIRST_SIZE_16K          ((paddr_t)1 << FIRST_SHIFT_16K)
+#define FIRST_SIZE_64K          ((paddr_t)1 << FIRST_SHIFT_64K)
+
+#define ZEROETH_SHIFT_4K        (FIRST_SHIFT_4K + LPAE_SHIFT_4K)
+#define ZEROETH_SHIFT_16K       (FIRST_SHIFT_16K + LPAE_SHIFT_16K)
+
+#define ZEROETH_SIZE_4K         ((paddr_t)1 << ZEROETH_SHIFT_4K)
+#define ZEROETH_SIZE_16K        ((paddr_t)1 << ZEROETH_SHIFT_16K)
+
+#define GUEST_TABLE_OFFSET(offs, gran)          ((paddr_t)(offs) & LPAE_ENTRY_MASK_##gran)
+#define third_guest_table_offset(va, gran)      GUEST_TABLE_OFFSET((va >> THIRD_SHIFT_##gran), gran)
+#define second_guest_table_offset(va, gran)     GUEST_TABLE_OFFSET((va >> SECOND_SHIFT_##gran), gran)
+#define first_guest_table_offset(va, gran)      GUEST_TABLE_OFFSET((va >> FIRST_SHIFT_##gran), gran)
+#define zeroeth_guest_table_offset(va, gran)    GUEST_TABLE_OFFSET((va >> ZEROETH_SHIFT_##gran), gran)
+
+#define third_guest_table_offset_4k(va)         third_guest_table_offset(va, 4K)
+#define third_guest_table_offset_16k(va)        third_guest_table_offset(va, 16K)
+#define third_guest_table_offset_64k(va)        third_guest_table_offset(va, 64K)
+
+#define second_guest_table_offset_4k(va)        second_guest_table_offset(va, 4K)
+#define second_guest_table_offset_16k(va)       second_guest_table_offset(va, 16K)
+#define second_guest_table_offset_64k(va)       second_guest_table_offset(va, 64K)
+
+#define first_guest_table_offset_4k(va)         first_guest_table_offset(va, 4K)
+#define first_guest_table_offset_16k(va)        first_guest_table_offset(va, 16K)
+#define first_guest_table_offset_64k(va)        first_guest_table_offset(va, 64K)
+
+#define zeroeth_guest_table_offset_4k(va)       zeroeth_guest_table_offset(va, 4K)
+#define zeroeth_guest_table_offset_16k(va)      zeroeth_guest_table_offset(va, 16K)
+
 #endif /* __ARM_PAGE_H__ */
 
 /*
-- 
2.12.2


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

[PATCH 4/8] arm/mem_access: Add short-descriptor pte typedefs

[Sergej Proskurin]

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

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

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 6222b1d4a2..5ea97ba95b 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -205,6 +205,25 @@ typedef union {
     lpae_walk_t walk;
 } lpae_t;
 
+/*
+ * Comprises the bits required to walk page tables adhering to the
+ * short-descriptor translation table format.
+ */
+typedef struct __packed {
+    unsigned int dt:2;          /* Descriptor type */
+    unsigned int pad1:8;
+    unsigned int base:22;       /* Base address of block or next table */
+} pte_sd_walk_t;
+
+/*
+ * Represents page table entries adhering to the short-descriptor translation
+ * table format.
+ */
+typedef union {
+    uint32_t bits;
+    pte_sd_walk_t walk;
+} pte_sd_t;
+
 /* Standard entry type that we'll use to build Xen's own pagetables.
  * We put the same permissions at every level, because they're ignored
  * by the walker in non-leaf entries. */
-- 
2.12.2


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

[PATCH 5/8] 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: This function p2m_walk_gpt 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 table on a different vCPU, the following registers
would need to be loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and SCTLR_EL1.

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

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

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

    Cosmetic fixes.
---
 xen/arch/arm/p2m.c        | 58 +++++++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/p2m.h |  6 +++++
 2 files changed, 64 insertions(+)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 752e948070..0337d83581 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1548,6 +1548,64 @@ void __init setup_virt_paging(void)
 }
 
 /*
+ * The function __p2m_walk_gpt_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 __p2m_walk_gpt_sd(struct p2m_domain *p2m,
+                             vaddr_t gva, paddr_t *ipa,
+                             unsigned int *perm_ro)
+{
+    /* Not implemented yet. */
+    return -EFAULT;
+}
+
+/*
+ * The function __p2m_walk_gpt_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 __p2m_walk_gpt_ld(struct p2m_domain *p2m,
+                             vaddr_t gva, paddr_t *ipa,
+                             unsigned int *perm_ro)
+{
+    /* Not implemented yet. */
+    return -EFAULT;
+}
+
+int p2m_walk_gpt(struct p2m_domain *p2m, vaddr_t gva,
+                 paddr_t *ipa, unsigned int *perm_ro)
+{
+    uint32_t sctlr = READ_SYSREG(SCTLR_EL1);
+    register_t tcr = READ_SYSREG(TCR_EL1);
+#ifdef CONFIG_ARM_64
+    struct domain *d = p2m->domain;
+#endif
+
+    /* If the MMU is disabled, there is no need to translate the gva. */
+    if ( !(sctlr & SCTLR_M) )
+    {
+        *ipa = gva;
+
+        return 0;
+    }
+
+#ifdef CONFIG_ARM_64
+    if ( is_32bit_domain(d) )
+#endif
+    {
+        if ( !(tcr & TTBCR_EAE) )
+            return __p2m_walk_gpt_sd(p2m, gva, ipa, perm_ro);
+    }
+
+    return __p2m_walk_gpt_ld(p2m, gva, ipa, perm_ro);
+}
+
+/*
  * Local variables:
  * mode: C
  * c-file-style: "BSD"
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 18c57f936e..cc9f4bf225 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -264,6 +264,12 @@ void guest_physmap_remove_page(struct domain *d,
 
 mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn);
 
+/* Walk the guest's page tables in software. */
+int p2m_walk_gpt(struct p2m_domain *p2m,
+                 vaddr_t gva,
+                 paddr_t *ipa,
+                 unsigned int *perm_ro);
+
 /*
  * Populate-on-demand
  */
-- 
2.12.2


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

[PATCH 6/8] 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 0487A-g J11-5608, J11-5679, and ARM DDI 0406C-b
B3-1510.

Note that the current implementation lacks support for 32-bit EL0
running on top of 64-bit EL1. 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.
---
 xen/arch/arm/p2m.c | 271 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 269 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 0337d83581..ea3be6f050 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1573,8 +1573,275 @@ static int __p2m_walk_gpt_ld(struct p2m_domain *p2m,
                              vaddr_t gva, paddr_t *ipa,
                              unsigned int *perm_ro)
 {
-    /* Not implemented yet. */
-    return -EFAULT;
+    int t0_sz, t1_sz, disabled = 1;
+    unsigned int level, gran;
+    unsigned int topbit = 0, input_size = 0, output_size;
+    uint64_t ttbr = 0, ips;
+    paddr_t mask;
+    lpae_t pte, *table;
+    struct page_info *page;
+    register_t tcr = READ_SYSREG(TCR_EL1);
+    struct domain *d = p2m->domain;
+
+    const vaddr_t offsets[4][3] = {
+        {
+#ifdef CONFIG_ARM_64
+            zeroeth_guest_table_offset_4k(gva),
+            zeroeth_guest_table_offset_16k(gva),
+            0, /* There is no zeroeth lookup level with a 64K granule size. */
+#endif
+        },
+        {
+            first_guest_table_offset_4k(gva),
+#ifdef CONFIG_ARM_64
+            first_guest_table_offset_16k(gva),
+            first_guest_table_offset_64k(gva),
+#endif
+        },
+        {
+            second_guest_table_offset_4k(gva),
+#ifdef CONFIG_ARM_64
+            second_guest_table_offset_16k(gva),
+            second_guest_table_offset_64k(gva),
+#endif
+        },
+        {
+            third_guest_table_offset_4k(gva),
+#ifdef CONFIG_ARM_64
+            third_guest_table_offset_16k(gva),
+            third_guest_table_offset_64k(gva),
+#endif
+        }
+    };
+
+    const paddr_t masks[4][3] = {
+        {
+            ZEROETH_SIZE_4K - 1,
+            ZEROETH_SIZE_16K - 1,
+            0 /* There is no zeroeth lookup level with a 64K granule size. */
+        },
+        {
+            FIRST_SIZE_4K - 1,
+            FIRST_SIZE_16K - 1,
+            FIRST_SIZE_64K - 1
+        },
+        {
+            SECOND_SIZE_4K - 1,
+            SECOND_SIZE_16K - 1,
+            SECOND_SIZE_64K - 1
+        },
+        {
+            THIRD_SIZE_4K - 1,
+            THIRD_SIZE_16K - 1,
+            THIRD_SIZE_64K - 1
+        }
+    };
+
+    const unsigned int grainsizes[3] = {
+        PAGE_SHIFT_4K,
+        PAGE_SHIFT_16K,
+        PAGE_SHIFT_64K
+    };
+
+    const unsigned int strides[3] = {
+        LPAE_SHIFT_4K,
+        LPAE_SHIFT_16K,
+        LPAE_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, according to "AddrTop" pseudocode
+     * implementation in ARM DDI 0487A-g J11-5739.
+     *
+     * XXX: We do not check whether the 64bit domain uses a 32-bit EL0. In this
+     * case, we need to set topbit to 31, as well.
+     */
+    if ( is_32bit_domain(d) )
+        topbit = TCR_TB_31;
+#ifdef CONFIG_ARM_64
+    else if ( is_64bit_domain(d) )
+    {
+        if ( ((gva & (1UL << TCR_TB_55)) && (tcr & TCR_TBI1)) ||
+             (!(gva & (1UL << TCR_TB_55)) && (tcr & TCR_TBI0)) )
+            topbit = TCR_TB_55;
+        else
+            topbit = TCR_TB_63;
+    }
+#endif
+
+#ifdef CONFIG_ARM_64
+    if ( is_64bit_domain(d) )
+    {
+        if ( (gva & (1UL << topbit)) == 0 )
+        {
+            input_size = REGISTER_WIDTH_64_BIT - t0_sz;
+
+            if ( input_size > IPS_MAX )
+                /* We limit the input_size to be max 48 bit. */
+                input_size = IPS_MAX;
+            else if ( input_size < IPS_MIN )
+                /* We limit the input_size to be max 25 bit. */
+                input_size = IPS_MIN;
+
+            /* Normalize granule size. */
+            switch ( tcr & TCR_TG0_MASK ) {
+            case TCR_TG0_16K:
+                gran = 1;
+                break;
+            case TCR_TG0_64K:
+                gran = 2;
+                break;
+            default:
+                gran = 0;
+            }
+
+            /* 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 ) ? 1 : 0;
+        }
+        else
+        {
+            input_size = REGISTER_WIDTH_64_BIT - t1_sz;
+
+            if ( input_size > IPS_MAX )
+                /* We limit the input_size to be max 48 bit. */
+                input_size = IPS_MAX;
+            else if ( input_size < IPS_MIN )
+                /* We limit the input_size to be max 25 bit. */
+                input_size = IPS_MIN;
+
+            /* Normalize granule size. */
+            switch ( tcr & TCR_TG1_MASK ) {
+            case TCR_TG1_16K:
+                gran = 1;
+                break;
+            case TCR_TG1_64K:
+                gran = 2;
+                break;
+            default:
+                gran = 0;
+            }
+
+            /* 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 ) ? 1 : 0;
+        }
+    }
+    else
+#endif
+    {
+        /* Granule size of Aarch32 or ARMv7 architectures is always 4K, indexed by 0. */
+        gran = 0;
+
+        mask = ((1ULL << REGISTER_WIDTH_32_BIT) - 1) &
+               ~((1ULL << (REGISTER_WIDTH_32_BIT - t0_sz)) - 1);
+
+        if ( t0_sz == 0 || !(gva & mask) )
+        {
+            input_size = REGISTER_WIDTH_32_BIT - t0_sz;
+
+            /* Use TTBR0 for GVA to IPA translation. */
+            ttbr = READ_SYSREG64(TTBR0_EL1);
+
+            /* If TCR.EPD0 is set, translations using TTBR0 are disabled. */
+            disabled = ( tcr & TCR_EPD0 ) ? 1 : 0;
+        }
+
+        mask = ((1ULL << REGISTER_WIDTH_32_BIT) - 1) &
+               ~((1ULL << (REGISTER_WIDTH_32_BIT - t1_sz)) - 1);
+
+        if ( ((t1_sz == 0) && !ttbr) || (t1_sz && (gva & mask) == mask) )
+        {
+            input_size = REGISTER_WIDTH_32_BIT - t1_sz;
+
+            /* Use TTBR1 for GVA to IPA translation. */
+            ttbr = READ_SYSREG64(TTBR1_EL1);
+
+            /* If TCR.EPD1 is set, translations using TTBR1 are disabled. */
+            disabled = ( tcr & TCR_EPD1 ) ? 1 : 0;
+        }
+    }
+
+    if ( disabled )
+        return -EFAULT;
+
+    level = 4 - DIV_ROUND_UP((input_size - grainsizes[gran]), strides[gran]);
+
+    /* XXX: We do not consider 32bit EL0 running on Aarch64, yet. */
+    if ( is_64bit_domain(d) )
+    {
+        /* Get the intermediate physical address size. */
+        ips = (tcr & TCR_IPS_MASK) >> TCR_IPS_SHIFT;
+
+        switch (ips) {
+        case TCR_IPS_32_BIT:
+            output_size = IPS_32_BIT;
+            break;
+        case TCR_IPS_36_BIT:
+            output_size = IPS_36_BIT;
+            break;
+        case TCR_IPS_40_BIT:
+            output_size = IPS_40_BIT;
+            break;
+        case TCR_IPS_42_BIT:
+            output_size = IPS_42_BIT;
+            break;
+        case TCR_IPS_44_BIT:
+            output_size = IPS_44_BIT;
+            break;
+        case TCR_IPS_48_BIT:
+        default:
+            output_size = IPS_48_BIT;
+        }
+    }
+    else
+        output_size = IPS_40_BIT;
+
+    /* Make sure the base address does not exceed its configured size. */
+    mask = ((1ULL << IPS_48_BIT) - 1) & ~((1ULL << output_size) - 1);
+    if ( output_size != IPS_48_BIT && (ttbr & mask) )
+        return -EFAULT;
+
+    mask = ((1ULL << output_size) - 1);
+    page = get_page_from_gfn(d, paddr_to_pfn(ttbr & mask), NULL, P2M_ALLOC);
+    if ( !page )
+        return -EFAULT;
+
+    table = __map_domain_page(page);
+
+    for ( ; ; level++ )
+    {
+        pte = table[offsets[level][gran]];
+
+        unmap_domain_page(table);
+        put_page(page);
+
+        if ( level == 3 || !pte.walk.valid || !pte.walk.table )
+            break;
+
+        page = get_page_from_gfn(d, pte.walk.base, NULL, P2M_ALLOC);
+        if ( !page )
+            return -EFAULT;
+
+        table = __map_domain_page(page);
+    }
+
+    if ( !pte.walk.valid || ((level == 3) & !pte.walk.table) )
+        return -EFAULT;
+
+    *ipa = pfn_to_paddr(pte.walk.base) | (gva & masks[level][gran]);
+
+    *perm_ro = pte.pt.ro;
+
+    /* Return the entire pte so that the caller can check flags by herself. */
+    return 0;
 }
 
 int p2m_walk_gpt(struct p2m_domain *p2m, vaddr_t gva,
-- 
2.12.2


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

[PATCH 7/8] 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 0487A-g G4-4189 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>
---
 xen/arch/arm/p2m.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 123 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index ea3be6f050..fa112b873c 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1558,8 +1558,129 @@ static int __p2m_walk_gpt_sd(struct p2m_domain *p2m,
                              vaddr_t gva, paddr_t *ipa,
                              unsigned int *perm_ro)
 {
-    /* Not implemented yet. */
-    return -EFAULT;
+    int disabled = 1;
+    int32_t ttbr;
+    paddr_t mask;
+    pte_sd_t pte, *table;
+    struct page_info *page;
+    register_t ttbcr = READ_SYSREG(TCR_EL1);
+    unsigned int level = 0, n = ttbcr & TTBCR_N_MASK;
+    struct domain *d = p2m->domain;
+
+    const paddr_t offsets[2] = {
+        ((paddr_t)(gva >> 20) & ((1ULL << (12 - n)) - 1)),
+        ((paddr_t)(gva >> 12) & ((1ULL << 8) - 1))
+    };
+
+    /* TODO: Do the same (31 bit) with LPAE code!! */
+    mask = ((1ULL << REGISTER_WIDTH_32_BIT) - 1) &
+           ~((1ULL << (REGISTER_WIDTH_32_BIT - n)) - 1);
+
+    if ( n == 0 || !(gva & mask) )
+    {
+        /* Use TTBR0 for GVA to IPA translation. */
+        ttbr = READ_SYSREG64(TTBR0_EL1);
+
+        /* If TTBCR.PD0 is set, translations using TTBR0 are disabled. */
+        disabled = ( ttbcr & TTBCR_PD0 ) ? 1 : 0;
+    }
+    else
+    {
+        /* Use TTBR1 for GVA to IPA translation. */
+        ttbr = READ_SYSREG64(TTBR1_EL1);
+
+        /* If TTBCR.PD1 is set, translations using TTBR1 are disabled. */
+        disabled = ( ttbcr & TTBCR_PD1 ) ? 1 : 0;
+    }
+
+    if ( disabled )
+        return -EFAULT;
+
+    mask = (1ULL << (14 - n)) - 1;
+    page = get_page_from_gfn(d, paddr_to_pfn(ttbr & ~mask), NULL, P2M_ALLOC);
+    if ( !page )
+        return -EFAULT;
+
+    /*
+     * XXX: The 2nd level lookup table might comprise 4 concatenated 4K
+     * pages.  Check how to map concatenated tables at once.
+     */
+    table = __map_domain_page(page);
+
+    /* Consider offset if n > 2. */
+    if ( n > 2 )
+        table = (pte_sd_t *)((unsigned long)table | (unsigned long)(ttbr & mask));
+
+    pte = table[offsets[level]];
+
+    unmap_domain_page(table);
+    put_page(page);
+
+    switch ( pte.walk.dt ) {
+    case 0: /* Invalid mapping. */
+        return -EFAULT;
+
+    case 1: /* Large or small page. */
+        level++;
+
+        page = get_page_from_gfn(d, (pte.walk.base >> 2), NULL, P2M_ALLOC);
+        if ( !page )
+            return -EFAULT;
+
+        table = __map_domain_page(page);
+        table = (pte_sd_t *)((unsigned long)table | ((pte.walk.base & 0x3) << 10));
+
+        pte = table[offsets[level]];
+
+        unmap_domain_page(table);
+        put_page(page);
+
+        if ( pte.walk.dt == 0 )
+            break;
+
+        if ( pte.walk.dt & 0x2 ) /* Small page. */
+        {
+            mask = (1ULL << PAGE_SHIFT_4K) - 1;
+            *ipa = (pte.bits & ~mask) | (gva & mask);
+        }
+        else /* Large page. */
+        {
+            mask = (1ULL << PAGE_SHIFT_64K) - 1;
+            *ipa = (pte.bits & ~mask) | (gva & mask);
+        }
+
+        /* Set access permissions[2:0]. */
+        *perm_ro = (pte.bits & 0x200) >> 9;
+
+        break;
+
+    case 2: /* Section. */
+    case 3: /* Section or Supersection. */
+        if ( !(pte.bits & (1ULL << 18)) ) /* Section */
+        {
+            mask = (1ULL << 20) - 1;
+            *ipa = (pte.bits & ~mask) | (gva & mask);
+        }
+        else /* Supersection */
+        {
+            mask = (1ULL << 24) - 1;
+            *ipa = (pte.bits & ~mask) | (gva & mask);
+
+            mask = ((1ULL << 24) - 1) & ~((1ULL << 20) - 1);
+            *ipa |= (pte.bits & mask) << 32;
+
+            mask = ((1ULL << 9) - 1) & ~((1ULL << 5) - 1);
+            *ipa |= (pte.bits & mask) << 36;
+        }
+
+        /* Set access permission[2]. */
+        *perm_ro = (pte.bits & 0x8000) >> 15;
+    }
+
+    if ( pte.walk.dt == 0 )
+        return -EFAULT;
+
+    return 0;
 }
 
 /*
-- 
2.12.2


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

[PATCH 8/8] arm/mem_access: Walk the guest's pt in software

[Sergej Proskurin]

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

Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
---
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
---
v2: Check the returned access rights after walking the guest's page tables in
    the function p2m_mem_access_check_and_get_page.
---
 xen/arch/arm/mem_access.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

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


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