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,
[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.
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@lists.cs.columbia.edu https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
Regards, Anup
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.
linaro-kernel@lists.linaro.org
-
Anup Patel -
Anup Patel -
Marc Zyngier