[PATCH] arm64: KVM: Add bits for specifying memory type in stage2 PTE

[PATCH] arm64: KVM: Add bits for specifying memory type in stage2 PTE

[Anup Patel]

We cannot use existing stage1 PTE_ATTRINDX() macro for specifying
stage2 memory type because stage1 ATTRINDX = PTE[4:2] and stage2
MEMATTR = PTE[5:2].

This patch adds bit definetions for specifying device, noncacheable,
writethrough, and writeback memory types in stage2 PTE and also uses
it in PAGE_S2 and PAGE_S2_DEVICE.

Signed-off-by: Anup Patel <anup.patel@linaro.org>
Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
---
 arch/arm64/include/asm/pgtable-hwdef.h |    4 ++++
 arch/arm64/include/asm/pgtable.h       |    6 ++++--
 arch/arm64/mm/mmu.c                    |    9 +++++++++
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/pgtable-hwdef.h b/arch/arm64/include/asm/pgtable-hwdef.h
index c49cd61..555babb 100644
--- a/arch/arm64/include/asm/pgtable-hwdef.h
+++ b/arch/arm64/include/asm/pgtable-hwdef.h
@@ -73,6 +73,10 @@
  */
 #define PTE_S2_RDONLY		 (_AT(pteval_t, 1) << 6)   /* HAP[1]   */
 #define PTE_S2_RDWR		 (_AT(pteval_t, 3) << 6)   /* HAP[2:1] */
+#define PTE_S2_MT_DEVICE	 (_AT(pteval_t, 0x0) << 2) /* MemAttr[3:0] */
+#define PTE_S2_MT_UNCACHED	 (_AT(pteval_t, 0x5) << 2) /* MemAttr[3:0] */
+#define PTE_S2_MT_WRITETHROUGH	 (_AT(pteval_t, 0xa) << 2) /* MemAttr[3:0] */
+#define PTE_S2_MT_WRITEBACK	 (_AT(pteval_t, 0xf) << 2) /* MemAttr[3:0] */
 
 /*
  * EL2/HYP PTE/PMD definitions
diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index 43ce724..3003fd0 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -60,6 +60,8 @@ extern void __pgd_error(const char *file, int line, unsigned long val);
 #define _PAGE_DEFAULT		PTE_TYPE_PAGE | PTE_AF
 
 extern pgprot_t pgprot_default;
+extern pgprot_t pgprot_s2;
+extern pgprot_t pgprot_s2_device;
 
 #define __pgprot_modify(prot,mask,bits) \
 	__pgprot((pgprot_val(prot) & ~(mask)) | (bits))
@@ -79,8 +81,8 @@ extern pgprot_t pgprot_default;
 #define PAGE_HYP		_MOD_PROT(pgprot_default, PTE_HYP)
 #define PAGE_HYP_DEVICE		_MOD_PROT(__pgprot(PROT_DEVICE_nGnRE), PTE_HYP)
 
-#define PAGE_S2			_MOD_PROT(pgprot_default, PTE_S2_RDONLY)
-#define PAGE_S2_DEVICE		__pgprot_modify(__pgprot(PROT_DEVICE_nGnRE), PTE_PXN, PTE_S2_RDWR)
+#define PAGE_S2			_MOD_PROT(pgprot_s2, PTE_USER | PTE_S2_RDONLY)
+#define PAGE_S2_DEVICE		_MOD_PROT(pgprot_s2_device, PTE_USER | PTE_S2_RDWR)
 
 #define __PAGE_NONE		__pgprot(((_PAGE_DEFAULT) & ~PTE_TYPE_MASK) | PTE_PROT_NONE)
 #define __PAGE_SHARED		__pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_UXN)
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 70b8cd4..ef26978 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -46,6 +46,12 @@ EXPORT_SYMBOL(empty_zero_page);
 pgprot_t pgprot_default;
 EXPORT_SYMBOL(pgprot_default);
 
+pgprot_t pgprot_s2;
+EXPORT_SYMBOL(pgprot_s2);
+
+pgprot_t pgprot_s2_device;
+EXPORT_SYMBOL(pgprot_s2_device);
+
 static pmdval_t prot_sect_kernel;
 
 struct cachepolicy {
@@ -147,6 +153,9 @@ static void __init init_mem_pgprot(void)
 	}
 
 	pgprot_default = __pgprot(PTE_TYPE_PAGE | PTE_AF | default_pgprot);
+
+	pgprot_s2 = __pgprot(PTE_TYPE_PAGE | PTE_AF | PTE_SHARED | PTE_S2_MT_WRITEBACK);
+	pgprot_s2_device = __pgprot(PTE_TYPE_PAGE | PTE_AF | PTE_S2_MT_DEVICE);
 }
 
 pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
-- 
1.7.9.5

[PATCH] arm64: KVM: Add bits for specifying memory type in stage2 PTE

[Marc Zyngier]

[Looping in Catalin, as we just had an interesting discussion about this]

Hi Anup,

On 09/05/13 06:53, Anup Patel wrote:
> We cannot use existing stage1 PTE_ATTRINDX() macro for specifying
> stage2 memory type because stage1 ATTRINDX = PTE[4:2] and stage2
> MEMATTR = PTE[5:2].

You're right, this is a bug. But...

> This patch adds bit definetions for specifying device, noncacheable,
> writethrough, and writeback memory types in stage2 PTE and also uses
> it in PAGE_S2 and PAGE_S2_DEVICE.

... I have the feeling we don't need most of this:

Stage-2 memory attributes can only restrict what the guest puts in its
Stage-1. Which means that unless we really need to play some ugly tricks
on the guest (which we don't), it is probably best to leave everything
as Normal, Outer Write-Back, Inner Write-Back.

So I think the right fix is to get rid of of PAGE_S2_DEVICE, use PAGE_S2
for everything, and set MemAttr[3:0] to 0b1111.

I'll try that and cook a separate patch if it looks good enough (it is
likely to impact the 32bit code as well...).

> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> ---
>  arch/arm64/include/asm/pgtable-hwdef.h |    4 ++++
>  arch/arm64/include/asm/pgtable.h       |    6 ++++--
>  arch/arm64/mm/mmu.c                    |    9 +++++++++
>  3 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/pgtable-hwdef.h b/arch/arm64/include/asm/pgtable-hwdef.h
> index c49cd61..555babb 100644
> --- a/arch/arm64/include/asm/pgtable-hwdef.h
> +++ b/arch/arm64/include/asm/pgtable-hwdef.h
> @@ -73,6 +73,10 @@
>   */
>  #define PTE_S2_RDONLY		 (_AT(pteval_t, 1) << 6)   /* HAP[1]   */
>  #define PTE_S2_RDWR		 (_AT(pteval_t, 3) << 6)   /* HAP[2:1] */
> +#define PTE_S2_MT_DEVICE	 (_AT(pteval_t, 0x0) << 2) /* MemAttr[3:0] */
> +#define PTE_S2_MT_UNCACHED	 (_AT(pteval_t, 0x5) << 2) /* MemAttr[3:0] */
> +#define PTE_S2_MT_WRITETHROUGH	 (_AT(pteval_t, 0xa) << 2) /* MemAttr[3:0] */
> +#define PTE_S2_MT_WRITEBACK	 (_AT(pteval_t, 0xf) << 2) /* MemAttr[3:0] */
>  
>  /*
>   * EL2/HYP PTE/PMD definitions
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 43ce724..3003fd0 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -60,6 +60,8 @@ extern void __pgd_error(const char *file, int line, unsigned long val);
>  #define _PAGE_DEFAULT		PTE_TYPE_PAGE | PTE_AF
>  
>  extern pgprot_t pgprot_default;
> +extern pgprot_t pgprot_s2;
> +extern pgprot_t pgprot_s2_device;
>  
>  #define __pgprot_modify(prot,mask,bits) \
>  	__pgprot((pgprot_val(prot) & ~(mask)) | (bits))
> @@ -79,8 +81,8 @@ extern pgprot_t pgprot_default;
>  #define PAGE_HYP		_MOD_PROT(pgprot_default, PTE_HYP)
>  #define PAGE_HYP_DEVICE		_MOD_PROT(__pgprot(PROT_DEVICE_nGnRE), PTE_HYP)
>  
> -#define PAGE_S2			_MOD_PROT(pgprot_default, PTE_S2_RDONLY)
> -#define PAGE_S2_DEVICE		__pgprot_modify(__pgprot(PROT_DEVICE_nGnRE), PTE_PXN, PTE_S2_RDWR)
> +#define PAGE_S2			_MOD_PROT(pgprot_s2, PTE_USER | PTE_S2_RDONLY)
> +#define PAGE_S2_DEVICE		_MOD_PROT(pgprot_s2_device, PTE_USER | PTE_S2_RDWR)

Beware, you are reintroducing a bug I fixed a while ago (Stage-2 has no
USER bit).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] arm64: KVM: Add bits for specifying memory type in stage2 PTE

[Anup Patel]

On Thu, May 9, 2013 at 3:02 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> [Looping in Catalin, as we just had an interesting discussion about this]
>
> Hi Anup,
>
> On 09/05/13 06:53, Anup Patel wrote:
>> We cannot use existing stage1 PTE_ATTRINDX() macro for specifying
>> stage2 memory type because stage1 ATTRINDX = PTE[4:2] and stage2
>> MEMATTR = PTE[5:2].
>
> You're right, this is a bug. But...
>
>> This patch adds bit definetions for specifying device, noncacheable,
>> writethrough, and writeback memory types in stage2 PTE and also uses
>> it in PAGE_S2 and PAGE_S2_DEVICE.
>
> ... I have the feeling we don't need most of this:
>
> Stage-2 memory attributes can only restrict what the guest puts in its
> Stage-1. Which means that unless we really need to play some ugly tricks
> on the guest (which we don't), it is probably best to leave everything
> as Normal, Outer Write-Back, Inner Write-Back.
>
> So I think the right fix is to get rid of of PAGE_S2_DEVICE, use PAGE_S2
> for everything, and set MemAttr[3:0] to 0b1111.

IMHO, its good to have separate PAGE_S2_DEVICE because using this we
are enforcing strongly-ordered and non-cacheable memory for devices directly
accessed by guest (such as GIC CPU interface).

But ...

Having just PAGE_S2 will also work assuming that guest always uses
correct memory type for devices (specifically GIC CPU interface).

The most important thing on real HW is to have MemAttr = 0xF for proper
guest access to RAM.

>
> I'll try that and cook a separate patch if it looks good enough (it is
> likely to impact the 32bit code as well...).

Yes, 32bit code also has PAGE_S2_DEVICE macro.

I am fine with your suggestion. Go ahead with your patch.

>
>> Signed-off-by: Anup Patel <anup.patel@linaro.org>
>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
>> ---
>>  arch/arm64/include/asm/pgtable-hwdef.h |    4 ++++
>>  arch/arm64/include/asm/pgtable.h       |    6 ++++--
>>  arch/arm64/mm/mmu.c                    |    9 +++++++++
>>  3 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/pgtable-hwdef.h b/arch/arm64/include/asm/pgtable-hwdef.h
>> index c49cd61..555babb 100644
>> --- a/arch/arm64/include/asm/pgtable-hwdef.h
>> +++ b/arch/arm64/include/asm/pgtable-hwdef.h
>> @@ -73,6 +73,10 @@
>>   */
>>  #define PTE_S2_RDONLY                 (_AT(pteval_t, 1) << 6)   /* HAP[1]   */
>>  #define PTE_S2_RDWR           (_AT(pteval_t, 3) << 6)   /* HAP[2:1] */
>> +#define PTE_S2_MT_DEVICE      (_AT(pteval_t, 0x0) << 2) /* MemAttr[3:0] */
>> +#define PTE_S2_MT_UNCACHED    (_AT(pteval_t, 0x5) << 2) /* MemAttr[3:0] */
>> +#define PTE_S2_MT_WRITETHROUGH        (_AT(pteval_t, 0xa) << 2) /* MemAttr[3:0] */
>> +#define PTE_S2_MT_WRITEBACK   (_AT(pteval_t, 0xf) << 2) /* MemAttr[3:0] */
>>
>>  /*
>>   * EL2/HYP PTE/PMD definitions
>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
>> index 43ce724..3003fd0 100644
>> --- a/arch/arm64/include/asm/pgtable.h
>> +++ b/arch/arm64/include/asm/pgtable.h
>> @@ -60,6 +60,8 @@ extern void __pgd_error(const char *file, int line, unsigned long val);
>>  #define _PAGE_DEFAULT                PTE_TYPE_PAGE | PTE_AF
>>
>>  extern pgprot_t pgprot_default;
>> +extern pgprot_t pgprot_s2;
>> +extern pgprot_t pgprot_s2_device;
>>
>>  #define __pgprot_modify(prot,mask,bits) \
>>       __pgprot((pgprot_val(prot) & ~(mask)) | (bits))
>> @@ -79,8 +81,8 @@ extern pgprot_t pgprot_default;
>>  #define PAGE_HYP             _MOD_PROT(pgprot_default, PTE_HYP)
>>  #define PAGE_HYP_DEVICE              _MOD_PROT(__pgprot(PROT_DEVICE_nGnRE), PTE_HYP)
>>
>> -#define PAGE_S2                      _MOD_PROT(pgprot_default, PTE_S2_RDONLY)
>> -#define PAGE_S2_DEVICE               __pgprot_modify(__pgprot(PROT_DEVICE_nGnRE), PTE_PXN, PTE_S2_RDWR)
>> +#define PAGE_S2                      _MOD_PROT(pgprot_s2, PTE_USER | PTE_S2_RDONLY)
>> +#define PAGE_S2_DEVICE               _MOD_PROT(pgprot_s2_device, PTE_USER | PTE_S2_RDWR)
>
> Beware, you are reintroducing a bug I fixed a while ago (Stage-2 has no
> USER bit).

My mistake. Thanks for pointing.

>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...
>
>
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

Regards,
Anup

[PATCH] arm64: KVM: Add bits for specifying memory type in stage2 PTE

[Marc Zyngier]

On 09/05/13 11:03, Anup Patel wrote:
> On Thu, May 9, 2013 at 3:02 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> [Looping in Catalin, as we just had an interesting discussion about this]
>>
>> Hi Anup,
>>
>> On 09/05/13 06:53, Anup Patel wrote:
>>> We cannot use existing stage1 PTE_ATTRINDX() macro for specifying
>>> stage2 memory type because stage1 ATTRINDX = PTE[4:2] and stage2
>>> MEMATTR = PTE[5:2].
>>
>> You're right, this is a bug. But...
>>
>>> This patch adds bit definetions for specifying device, noncacheable,
>>> writethrough, and writeback memory types in stage2 PTE and also uses
>>> it in PAGE_S2 and PAGE_S2_DEVICE.
>>
>> ... I have the feeling we don't need most of this:
>>
>> Stage-2 memory attributes can only restrict what the guest puts in its
>> Stage-1. Which means that unless we really need to play some ugly tricks
>> on the guest (which we don't), it is probably best to leave everything
>> as Normal, Outer Write-Back, Inner Write-Back.
>>
>> So I think the right fix is to get rid of of PAGE_S2_DEVICE, use PAGE_S2
>> for everything, and set MemAttr[3:0] to 0b1111.
> 
> IMHO, its good to have separate PAGE_S2_DEVICE because using this we
> are enforcing strongly-ordered and non-cacheable memory for devices directly
> accessed by guest (such as GIC CPU interface).
> 
> But ...
> 
> Having just PAGE_S2 will also work assuming that guest always uses
> correct memory type for devices (specifically GIC CPU interface).

That's the idea. If the guest screws up, we're not going to fix it up.

> The most important thing on real HW is to have MemAttr = 0xF for proper
> guest access to RAM.

Definitely.

>>
>> I'll try that and cook a separate patch if it looks good enough (it is
>> likely to impact the 32bit code as well...).
> 
> Yes, 32bit code also has PAGE_S2_DEVICE macro.
> 
> I am fine with your suggestion. Go ahead with your patch.

Just booted a v7 guest without PAGE_S2_device, and it seems happy
enough. I'll fix v8 accordingly and push the result out.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...