| Message ID | 20120915153443.21241.37958.stgit@ubuntu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;