diff mbox

[02/15] ARM: Add page table and page defines needed by KVM

Message ID 20120915153443.21241.37958.stgit@ubuntu (mailing list archive)
State New, archived
Headers show

Commit Message

Christoffer Dall Sept. 15, 2012, 3:34 p.m. UTC 
KVM uses the stage-2 page tables and the Hyp page table format,
so let's define the fields we need to access in KVM.

We use pgprot_guest to indicate stage-2 entries.

Christoffer Dall <c.dall@virtualopensystems.com>
---
 arch/arm/include/asm/pgtable-3level.h |   13 +++++++++++++
 arch/arm/include/asm/pgtable.h        |    5 +++++
 arch/arm/mm/mmu.c                     |    3 +++
 3 files changed, 21 insertions(+)

Comments

Will Deacon Sept. 18, 2012, 12:47 p.m. UTC | #1 
On Sat, Sep 15, 2012 at 04:34:43PM +0100, Christoffer Dall wrote:
> KVM uses the stage-2 page tables and the Hyp page table format,
> so let's define the fields we need to access in KVM.
> 
> We use pgprot_guest to indicate stage-2 entries.
> 
> Christoffer Dall <c.dall@virtualopensystems.com>
> ---
>  arch/arm/include/asm/pgtable-3level.h |   13 +++++++++++++
>  arch/arm/include/asm/pgtable.h        |    5 +++++
>  arch/arm/mm/mmu.c                     |    3 +++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/arch/arm/include/asm/pgtable-3level.h b/arch/arm/include/asm/pgtable-3level.h
> index b249035..7351eee 100644
> --- a/arch/arm/include/asm/pgtable-3level.h
> +++ b/arch/arm/include/asm/pgtable-3level.h
> @@ -102,11 +102,24 @@
>   */
>  #define L_PGD_SWAPPER		(_AT(pgdval_t, 1) << 55)	/* swapper_pg_dir entry */
>  
> +/*
> + * 2-nd stage PTE definitions for LPAE.
> + */

Minor nit: 2nd

> +#define L_PTE2_SHARED		L_PTE_SHARED
> +#define L_PTE2_READ		(_AT(pteval_t, 1) << 6)	/* HAP[0] */
> +#define L_PTE2_WRITE		(_AT(pteval_t, 1) << 7)	/* HAP[1] */

This is actually HAP[2:1], not HAP[1:0]. Also, can you follow what we do for
stage 1 translation and name these RDONLY and WRONLY (do you even use
that?).

> +#define L_PTE2_NORM_WB		(_AT(pteval_t, 3) << 4)	/* MemAttr[3:2] */
> +#define L_PTE2_INNER_WB		(_AT(pteval_t, 3) << 2)	/* MemAttr[1:0] */

Another minor nit: PTE2 looks awful. Maybe L_PTE_HYP_* instead?

>  #ifndef __ASSEMBLY__
>  
>  #define pud_none(pud)		(!pud_val(pud))
>  #define pud_bad(pud)		(!(pud_val(pud) & 2))
>  #define pud_present(pud)	(pud_val(pud))
> +#define pmd_table(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
> +						 PMD_TYPE_TABLE)
> +#define pmd_sect(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
> +						 PMD_TYPE_SECT)
>  
>  #define pud_clear(pudp)			\
>  	do {				\
> diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
> index 41dc31f..c422f62 100644
> --- a/arch/arm/include/asm/pgtable.h
> +++ b/arch/arm/include/asm/pgtable.h
> @@ -70,6 +70,7 @@ extern void __pgd_error(const char *file, int line, pgd_t);
>  
>  extern pgprot_t		pgprot_user;
>  extern pgprot_t		pgprot_kernel;
> +extern pgprot_t		pgprot_guest;
>  
>  #define _MOD_PROT(p, b)	__pgprot(pgprot_val(p) | (b))
>  
> @@ -82,6 +83,10 @@ extern pgprot_t		pgprot_kernel;
>  #define PAGE_READONLY_EXEC	_MOD_PROT(pgprot_user, L_PTE_USER | L_PTE_RDONLY)
>  #define PAGE_KERNEL		_MOD_PROT(pgprot_kernel, L_PTE_XN)
>  #define PAGE_KERNEL_EXEC	pgprot_kernel
> +#define PAGE_HYP		_MOD_PROT(pgprot_kernel, L_PTE_USER)

Just define L_PTE_HYP to L_PTE_USER, otherwise that's confusing.

> +#define PAGE_KVM_GUEST		_MOD_PROT(pgprot_guest, L_PTE2_READ | \
> +					  L_PTE2_NORM_WB | L_PTE2_INNER_WB | \
> +					  L_PTE2_SHARED)

It would be cleaner to separate the cacheability attributes out from here
and into the cache_policies array. Then you just need L_PTE_HYP_RDONLY here.

Will
Catalin Marinas Sept. 18, 2012, 2:06 p.m. UTC | #2 
On 18 September 2012 13:47, Will Deacon <will.deacon@arm.com> wrote:
> On Sat, Sep 15, 2012 at 04:34:43PM +0100, Christoffer Dall wrote:
>> +#define L_PTE2_SHARED                L_PTE_SHARED
>> +#define L_PTE2_READ          (_AT(pteval_t, 1) << 6) /* HAP[0] */
>> +#define L_PTE2_WRITE         (_AT(pteval_t, 1) << 7) /* HAP[1] */
>
> This is actually HAP[2:1], not HAP[1:0]. Also, can you follow what we do for
> stage 1 translation and name these RDONLY and WRONLY (do you even use
> that?).

We can't use RDONLY as this would have value 0 as the HAP attributes
(stage 2 overriding stage 1 translation attributes). Unless you add 4
definitions like NOACCESS, RDONLY, WRONLY and RDWR to cover all the
bit combinations.

>> +#define L_PTE2_NORM_WB               (_AT(pteval_t, 3) << 4) /* MemAttr[3:2] */
>> +#define L_PTE2_INNER_WB              (_AT(pteval_t, 3) << 2) /* MemAttr[1:0] */
>
> Another minor nit: PTE2 looks awful. Maybe L_PTE_HYP_* instead?

L_PTE_HYP may be confused with the Stage 1 Hyp translation which is
different from the guest Stage 2.

But I have another minor nit - just write them in the ascending bit
order as other definitions in this file.
Christoffer Dall Sept. 18, 2012, 3:05 p.m. UTC | #3 
On Tue, Sep 18, 2012 at 10:06 AM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On 18 September 2012 13:47, Will Deacon <will.deacon@arm.com> wrote:
>> On Sat, Sep 15, 2012 at 04:34:43PM +0100, Christoffer Dall wrote:
>>> +#define L_PTE2_SHARED                L_PTE_SHARED
>>> +#define L_PTE2_READ          (_AT(pteval_t, 1) << 6) /* HAP[0] */
>>> +#define L_PTE2_WRITE         (_AT(pteval_t, 1) << 7) /* HAP[1] */
>>
>> This is actually HAP[2:1], not HAP[1:0]. Also, can you follow what we do for
>> stage 1 translation and name these RDONLY and WRONLY (do you even use
>> that?).
>
> We can't use RDONLY as this would have value 0 as the HAP attributes
> (stage 2 overriding stage 1 translation attributes). Unless you add 4
> definitions like NOACCESS, RDONLY, WRONLY and RDWR to cover all the
> bit combinations.
>
>>> +#define L_PTE2_NORM_WB               (_AT(pteval_t, 3) << 4) /* MemAttr[3:2] */
>>> +#define L_PTE2_INNER_WB              (_AT(pteval_t, 3) << 2) /* MemAttr[1:0] */
>>
>> Another minor nit: PTE2 looks awful. Maybe L_PTE_HYP_* instead?
>
> L_PTE_HYP may be confused with the Stage 1 Hyp translation which is
> different from the guest Stage 2.

exactly, it's misleading, how about L_PTE_STAGE2, a little verbose,
but clear...?

>
> But I have another minor nit - just write them in the ascending bit
> order as other definitions in this file.
>

ok, will fix.

Thanks,
-Christoffer
Catalin Marinas Sept. 18, 2012, 3:07 p.m. UTC | #4 
On Tue, Sep 18, 2012 at 04:05:13PM +0100, Christoffer Dall wrote:
> On Tue, Sep 18, 2012 at 10:06 AM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On 18 September 2012 13:47, Will Deacon <will.deacon@arm.com> wrote:
> >> On Sat, Sep 15, 2012 at 04:34:43PM +0100, Christoffer Dall wrote:
> >>> +#define L_PTE2_SHARED                L_PTE_SHARED
> >>> +#define L_PTE2_READ          (_AT(pteval_t, 1) << 6) /* HAP[0] */
> >>> +#define L_PTE2_WRITE         (_AT(pteval_t, 1) << 7) /* HAP[1] */
> >>
> >> This is actually HAP[2:1], not HAP[1:0]. Also, can you follow what we do for
> >> stage 1 translation and name these RDONLY and WRONLY (do you even use
> >> that?).
> >
> > We can't use RDONLY as this would have value 0 as the HAP attributes
> > (stage 2 overriding stage 1 translation attributes). Unless you add 4
> > definitions like NOACCESS, RDONLY, WRONLY and RDWR to cover all the
> > bit combinations.
> >
> >>> +#define L_PTE2_NORM_WB               (_AT(pteval_t, 3) << 4) /* MemAttr[3:2] */
> >>> +#define L_PTE2_INNER_WB              (_AT(pteval_t, 3) << 2) /* MemAttr[1:0] */
> >>
> >> Another minor nit: PTE2 looks awful. Maybe L_PTE_HYP_* instead?
> >
> > L_PTE_HYP may be confused with the Stage 1 Hyp translation which is
> > different from the guest Stage 2.
> 
> exactly, it's misleading, how about L_PTE_STAGE2, a little verbose,
> but clear...?

I don't mind any (apart from L_PTE_HYP_ would be confusing) for stage 2.
You could just use S2 to make it shorter.
Christoffer Dall Sept. 18, 2012, 3:10 p.m. UTC | #5 
On Tue, Sep 18, 2012 at 11:07 AM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Tue, Sep 18, 2012 at 04:05:13PM +0100, Christoffer Dall wrote:
>> On Tue, Sep 18, 2012 at 10:06 AM, Catalin Marinas
>> <catalin.marinas@arm.com> wrote:
>> > On 18 September 2012 13:47, Will Deacon <will.deacon@arm.com> wrote:
>> >> On Sat, Sep 15, 2012 at 04:34:43PM +0100, Christoffer Dall wrote:
>> >>> +#define L_PTE2_SHARED                L_PTE_SHARED
>> >>> +#define L_PTE2_READ          (_AT(pteval_t, 1) << 6) /* HAP[0] */
>> >>> +#define L_PTE2_WRITE         (_AT(pteval_t, 1) << 7) /* HAP[1] */
>> >>
>> >> This is actually HAP[2:1], not HAP[1:0]. Also, can you follow what we do for
>> >> stage 1 translation and name these RDONLY and WRONLY (do you even use
>> >> that?).
>> >
>> > We can't use RDONLY as this would have value 0 as the HAP attributes
>> > (stage 2 overriding stage 1 translation attributes). Unless you add 4
>> > definitions like NOACCESS, RDONLY, WRONLY and RDWR to cover all the
>> > bit combinations.
>> >
>> >>> +#define L_PTE2_NORM_WB               (_AT(pteval_t, 3) << 4) /* MemAttr[3:2] */
>> >>> +#define L_PTE2_INNER_WB              (_AT(pteval_t, 3) << 2) /* MemAttr[1:0] */
>> >>
>> >> Another minor nit: PTE2 looks awful. Maybe L_PTE_HYP_* instead?
>> >
>> > L_PTE_HYP may be confused with the Stage 1 Hyp translation which is
>> > different from the guest Stage 2.
>>
>> exactly, it's misleading, how about L_PTE_STAGE2, a little verbose,
>> but clear...?
>
> I don't mind any (apart from L_PTE_HYP_ would be confusing) for stage 2.
> You could just use S2 to make it shorter.
>
I'm good with that, done.

-Christoffer
diff mbox

Patch

diff --git a/arch/arm/include/asm/pgtable-3level.h b/arch/arm/include/asm/pgtable-3level.h
index b249035..7351eee 100644
--- a/arch/arm/include/asm/pgtable-3level.h
+++ b/arch/arm/include/asm/pgtable-3level.h
@@ -102,11 +102,24 @@ 
  */
 #define L_PGD_SWAPPER		(_AT(pgdval_t, 1) << 55)	/* swapper_pg_dir entry */
 
+/*
+ * 2-nd stage PTE definitions for LPAE.
+ */
+#define L_PTE2_SHARED		L_PTE_SHARED
+#define L_PTE2_READ		(_AT(pteval_t, 1) << 6)	/* HAP[0] */
+#define L_PTE2_WRITE		(_AT(pteval_t, 1) << 7)	/* HAP[1] */
+#define L_PTE2_NORM_WB		(_AT(pteval_t, 3) << 4)	/* MemAttr[3:2] */
+#define L_PTE2_INNER_WB		(_AT(pteval_t, 3) << 2)	/* MemAttr[1:0] */
+
 #ifndef __ASSEMBLY__
 
 #define pud_none(pud)		(!pud_val(pud))
 #define pud_bad(pud)		(!(pud_val(pud) & 2))
 #define pud_present(pud)	(pud_val(pud))
+#define pmd_table(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
+						 PMD_TYPE_TABLE)
+#define pmd_sect(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
+						 PMD_TYPE_SECT)
 
 #define pud_clear(pudp)			\
 	do {				\
diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
index 41dc31f..c422f62 100644
--- a/arch/arm/include/asm/pgtable.h
+++ b/arch/arm/include/asm/pgtable.h
@@ -70,6 +70,7 @@  extern void __pgd_error(const char *file, int line, pgd_t);
 
 extern pgprot_t		pgprot_user;
 extern pgprot_t		pgprot_kernel;
+extern pgprot_t		pgprot_guest;
 
 #define _MOD_PROT(p, b)	__pgprot(pgprot_val(p) | (b))
 
@@ -82,6 +83,10 @@  extern pgprot_t		pgprot_kernel;
 #define PAGE_READONLY_EXEC	_MOD_PROT(pgprot_user, L_PTE_USER | L_PTE_RDONLY)
 #define PAGE_KERNEL		_MOD_PROT(pgprot_kernel, L_PTE_XN)
 #define PAGE_KERNEL_EXEC	pgprot_kernel
+#define PAGE_HYP		_MOD_PROT(pgprot_kernel, L_PTE_USER)
+#define PAGE_KVM_GUEST		_MOD_PROT(pgprot_guest, L_PTE2_READ | \
+					  L_PTE2_NORM_WB | L_PTE2_INNER_WB | \
+					  L_PTE2_SHARED)
 
 #define __PAGE_NONE		__pgprot(_L_PTE_DEFAULT | L_PTE_RDONLY | L_PTE_XN)
 #define __PAGE_SHARED		__pgprot(_L_PTE_DEFAULT | L_PTE_USER | L_PTE_XN)
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 76bf4f5..a153fd4 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -56,9 +56,11 @@  static unsigned int cachepolicy __initdata = CPOLICY_WRITEBACK;
 static unsigned int ecc_mask __initdata = 0;
 pgprot_t pgprot_user;
 pgprot_t pgprot_kernel;
+pgprot_t pgprot_guest;
 
 EXPORT_SYMBOL(pgprot_user);
 EXPORT_SYMBOL(pgprot_kernel);
+EXPORT_SYMBOL(pgprot_guest);
 
 struct cachepolicy {
 	const char	policy[16];
@@ -514,6 +516,7 @@  static void __init build_mem_type_table(void)
 	pgprot_user   = __pgprot(L_PTE_PRESENT | L_PTE_YOUNG | user_pgprot);
 	pgprot_kernel = __pgprot(L_PTE_PRESENT | L_PTE_YOUNG |
 				 L_PTE_DIRTY | kern_pgprot);
+	pgprot_guest  = __pgprot(L_PTE_PRESENT | L_PTE_YOUNG);
 
 	mem_types[MT_LOW_VECTORS].prot_l1 |= ecc_mask;
 	mem_types[MT_HIGH_VECTORS].prot_l1 |= ecc_mask;