[PATCH 2/4] arm64: fixmap: move translation tables to dedicated region

[Ard Biesheuvel]

On 26 March 2015 at 02:28, Mark Rutland <mark.rutland at arm.com> wrote:
> Hi Ard,
>
> On Mon, Mar 23, 2015 at 03:36:54PM +0000, Ard Biesheuvel wrote:
>> This moves the fixmap translation tables to a dedicated region
>> in the linker map. This is needed for a split kernel text from
>> the linear mapping, to ensure that the contents of the tables
>> rely on a single translation regime.
>
> I'm not sure what you mean by this. Could you elaborate?
>
> What problem would we have if we didn't have this section, and how does this
> section solve that?
>

The pgd manipulation code is riddled with va/pa and pa/va
translations, and uses both statically and dynamically allocated
pages. Untangling all of that is not so easy, and it is simpler just
to refer to those regions through the linear mapping exclusively.

> Regardless, I have some comments on the implementation below.
>
>> Also make the population of the translation levels conditional,
>> so that the kernel text can share some levels of translation if
>> needed.
>
> I guess you mean shared with the tables for the text mapping?
>
> Please word this to be explicit w.r.t. what is shared between whom, and when.
>

Yes. Currently, the address space is split down the middle, so fixmap
and linear always live in regions that are disjoint all the way up to
different root pgdir entries. Once that changes, the fixmap code needs
to be prepared for any of the levels it needs to populated having
already been populated.

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>
>> ---
>>  arch/arm64/include/asm/linkage.h |  2 ++
>>  arch/arm64/kernel/vmlinux.lds.S  | 15 ++++++++++-----
>>  arch/arm64/mm/mmu.c              | 15 +++++++++------
>>  3 files changed, 21 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h
>> index 636c1bced7d4..dc9354de6f52 100644
>> --- a/arch/arm64/include/asm/linkage.h
>> +++ b/arch/arm64/include/asm/linkage.h
>> @@ -4,4 +4,6 @@
>>  #define __ALIGN              .align 4
>>  #define __ALIGN_STR  ".align 4"
>>
>> +#define __pgdir              __attribute__((__section__(".pgdir")))
>
> It would be nice for this to also provide page alignment, like
> __page_aligned_bss that this replaces uses of.
>

I thought it wasn't necessary, because we allocate a full page for the
highest level, but we should probably not rely on that.

>> +
>>  #endif
>> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
>> index 434ef407ef0f..d3885043f0b7 100644
>> --- a/arch/arm64/kernel/vmlinux.lds.S
>> +++ b/arch/arm64/kernel/vmlinux.lds.S
>> @@ -149,11 +149,16 @@ SECTIONS
>>
>>       BSS_SECTION(0, 0, 0)
>>
>> -     . = ALIGN(PAGE_SIZE);
>> -     idmap_pg_dir = .;
>> -     . += IDMAP_DIR_SIZE;
>> -     swapper_pg_dir = .;
>> -     . += SWAPPER_DIR_SIZE;
>> +     .pgdir : {
>> +             . = ALIGN(PAGE_SIZE);
>> +             __pgdir_start = .;
>> +             idmap_pg_dir = .;
>> +             . += IDMAP_DIR_SIZE;
>> +             *(.pgdir)
>> +             swapper_pg_dir = .;
>> +             . += SWAPPER_DIR_SIZE;
>> +             __pgdir_stop = .;
>
> Nit: __pgdir_end would be consitent with our other vmlinux.lds.S symbols.
>

Right

>> +     }
>>
>>       _end = .;
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 43496748e3d9..bb3ce41130f3 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -549,12 +549,12 @@ void vmemmap_free(unsigned long start, unsigned long end)
>>  }
>>  #endif       /* CONFIG_SPARSEMEM_VMEMMAP */
>>
>> -static pte_t bm_pte[PTRS_PER_PTE] __page_aligned_bss;
>> +static pte_t bm_pte[PTRS_PER_PTE] __pgdir;
>>  #if CONFIG_ARM64_PGTABLE_LEVELS > 2
>> -static pmd_t bm_pmd[PTRS_PER_PMD] __page_aligned_bss;
>> +static pmd_t bm_pmd[PTRS_PER_PMD] __pgdir;
>>  #endif
>>  #if CONFIG_ARM64_PGTABLE_LEVELS > 3
>> -static pud_t bm_pud[PTRS_PER_PUD] __page_aligned_bss;
>> +static pud_t bm_pud[PTRS_PER_PUD] __pgdir;
>>  #endif
>
> These used to be zeroed (by the bss init code in head.S), but now they won't
> be, and as they live after all the initialized data they could contain garbage
> if we're unlucky. So I suspect this is broken.
>

Actually, they are in this case, as the init code zeroes from the
beginning of idmap to the end of swapper :-)
But that is something that should be more explicit I guess

I was wondering if we should worry about doing the zeroing with the
caches off, which is not needed for fixmap

> If we zero these before use then that's fine. We could define the swapper and
> idmap similarly in this C file for consistency (we zero those in head.S). That
> would bring all the page table allocations into the same file, and .pgdir could
> be simpler:
>
>         .pgdir {
>                 . = ALIGN(PAGE_SIZE);
>                 __pgdir_start = .;
>                 *(.pgdir)
>                 __pgdir_end = .;
>         }
>
>>
>>  static inline pud_t * fixmap_pud(unsigned long addr)
>> @@ -592,11 +592,14 @@ void __init early_fixmap_init(void)
>>       unsigned long addr = FIXADDR_START;
>>
>>       pgd = pgd_offset_k(addr);
>> -     pgd_populate(&init_mm, pgd, bm_pud);
>> +     if (pgd_none(*pgd))
>> +             pgd_populate(&init_mm, pgd, bm_pud);
>>       pud = pud_offset(pgd, addr);
>> -     pud_populate(&init_mm, pud, bm_pmd);
>> +     if (pud_none(*pud))
>> +             pud_populate(&init_mm, pud, bm_pmd);
>>       pmd = pmd_offset(pud, addr);
>> -     pmd_populate_kernel(&init_mm, pmd, bm_pte);
>> +     if (pmd_none(*pmd))
>> +             pmd_populate_kernel(&init_mm, pmd, bm_pte);
>
> It's a bit confusing to have this change in the same patch as the other
> changes, given that this shouldn't be required yet. It might make more sense
> for this to live in the patch moving the text mapping.
>


ok