[v4,01/14] ARM: Add page table and page defines needed by KVM

Commit Message

Christoffer Dall Nov. 29, 2012, 3:57 p.m. UTC

On Mon, Nov 19, 2012 at 9:14 AM, Will Deacon <[e-mail]> wrote:
> On Sat, Nov 10, 2012 at 03:42:17PM +0000, Christoffer Dall wrote:
>> KVM uses the stage-2 page tables and the Hyp page table format,
>> so we define the fields and page protection flags needed by KVM.
>>
>> The nomenclature is this:
>>  - page_hyp:        PL2 code/data mappings
>>  - page_hyp_device: PL2 device mappings (vgic access)
>>  - page_s2:         Stage-2 code/data page mappings
>>  - page_s2_device:  Stage-2 device mappings (vgic access)
>>
>> Reviewed-by: Marcelo Tosatti <[e-mail]>
>> Christoffer Dall <[e-mail]>
>
> [...]
>
>> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
>> index 941dfb9..087d949 100644
>> --- a/arch/arm/mm/mmu.c
>> +++ b/arch/arm/mm/mmu.c
>> @@ -57,43 +57,61 @@ 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_hyp_device;
>> +pgprot_t pgprot_s2;
>> +pgprot_t pgprot_s2_device;
>>
>>  EXPORT_SYMBOL(pgprot_user);
>>  EXPORT_SYMBOL(pgprot_kernel);
>> +EXPORT_SYMBOL(pgprot_hyp_device);
>> +EXPORT_SYMBOL(pgprot_s2);
>> +EXPORT_SYMBOL(pgprot_s2_device);
>
> Do we still need these?
>

the exports, nope.

>>  struct cachepolicy {
>>       const char      policy[16];
>>       unsigned int    cr_mask;
>>       pmdval_t        pmd;
>>       pteval_t        pte;
>> +     pteval_t        pte_s2;
>>  };
>>
>> +#ifdef CONFIG_ARM_LPAE
>> +#define s2_policy(policy)    policy
>> +#else
>> +#define s2_policy(policy)    0
>> +#endif
>
> Put the macro in pgtable-{2,3}level.h?
>

I think that's weird, defining something far away from where it's used
will only make it harder to read, pgtable.h is not included in this
file, and there are other #ifdef CONFIG_ARM_LPAE in that file.

Here's the fix from above:



-Christoffer

Comments

Will Deacon Nov. 30, 2012, 11:46 a.m. UTC | #1

On Thu, Nov 29, 2012 at 03:57:00PM +0000, Christoffer Dall wrote:
> On Mon, Nov 19, 2012 at 9:14 AM, Will Deacon <[e-mail]> wrote:
> > On Sat, Nov 10, 2012 at 03:42:17PM +0000, Christoffer Dall wrote:
> >>
> >> +#ifdef CONFIG_ARM_LPAE
> >> +#define s2_policy(policy)    policy
> >> +#else
> >> +#define s2_policy(policy)    0
> >> +#endif
> >
> > Put the macro in pgtable-{2,3}level.h?
> >
> 
> I think that's weird, defining something far away from where it's used
> will only make it harder to read, pgtable.h is not included in this
> file, and there are other #ifdef CONFIG_ARM_LPAE in that file.


Of course pgtable.h is included in this file -- we have direct references to
L_PTE_MT_UNCACHED, for example, so by your logic we should inline all of
that too!

Yes, there are other CONFIG_ARM_LPAE checks in this file, but only where
there's a piece of code that is not applicable one way or the other. For
data definitions, it's really easy to fix in the headers so please do it
there instead.

Will

Christoffer Dall Nov. 30, 2012, 3:54 p.m. UTC | #2

On Fri, Nov 30, 2012 at 6:46 AM, Will Deacon <[e-mail]> wrote:
> On Thu, Nov 29, 2012 at 03:57:00PM +0000, Christoffer Dall wrote:
>> On Mon, Nov 19, 2012 at 9:14 AM, Will Deacon <[e-mail]> wrote:
>> > On Sat, Nov 10, 2012 at 03:42:17PM +0000, Christoffer Dall wrote:
>> >>
>> >> +#ifdef CONFIG_ARM_LPAE
>> >> +#define s2_policy(policy)    policy
>> >> +#else
>> >> +#define s2_policy(policy)    0
>> >> +#endif
>> >
>> > Put the macro in pgtable-{2,3}level.h?
>> >
>>
>> I think that's weird, defining something far away from where it's used
>> will only make it harder to read, pgtable.h is not included in this
>> file, and there are other #ifdef CONFIG_ARM_LPAE in that file.
>
>
> Of course pgtable.h is included in this file -- we have direct references to
> L_PTE_MT_UNCACHED, for example, so by your logic we should inline all of
> that too!

no, not at all!

>
> Yes, there are other CONFIG_ARM_LPAE checks in this file, but only where
> there's a piece of code that is not applicable one way or the other. For
> data definitions, it's really easy to fix in the headers so please do it
> there instead.
>
I don't see that macro as a data definition, it's a way to avoid
compile error when those defines are not there, and I don't see where
that would ever be used outside this file, so moving it just makes you
go hunting for the definition elsewhere.

Perhaps there's an idea behind these files stating that it's
universally better to have things in a header file, but I don't quite
see why?

(I can definitely move it, and I don't want to fight over it, I'm
merely trying to understand why it would make the code cleaner/better
to maintain/etc.)

-Christoffer

Patch

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 087d949..46ca62b 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -63,9 +63,6 @@  pgprot_t pgprot_s2_device;

 EXPORT_SYMBOL(pgprot_user);
 EXPORT_SYMBOL(pgprot_kernel);
-EXPORT_SYMBOL(pgprot_hyp_device);
-EXPORT_SYMBOL(pgprot_s2);
-EXPORT_SYMBOL(pgprot_s2_device);

 struct cachepolicy {
 	const char	policy[16];