[RFC PATCH 1/3] arm64: head.S: replace early literals with constant immediates
Ard Biesheuvel
ard.biesheuvel at linaro.org
Tue Mar 17 00:01:10 PDT 2015
On 16 March 2015 at 18:14, Mark Rutland <mark.rutland at arm.com> wrote:
> Hi Ard,
>
> On Mon, Mar 16, 2015 at 03:23:41PM +0000, Ard Biesheuvel wrote:
>> In preparation of making the kernel relocatable, replace literal
>> symbol references with immediate constants. This is necessary, as
>> the literals will not be populated with meaningful values until
>> after the relocation code has executed.
>
> The majority of these changes look like nice cleanups/simplifications
> regardless of whether the kernel is relocatable, so I like most of the
> patch.
>
OK. I can clean it up and add it to the head.S spring cleaning series.
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>
>> ---
>> arch/arm64/kernel/efi-entry.S | 2 +-
>> arch/arm64/kernel/head.S | 36 +++++++++++++++---------------------
>> 2 files changed, 16 insertions(+), 22 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S
>> index 8ce9b0577442..f78e6a1de825 100644
>> --- a/arch/arm64/kernel/efi-entry.S
>> +++ b/arch/arm64/kernel/efi-entry.S
>> @@ -61,7 +61,7 @@ ENTRY(efi_stub_entry)
>> */
>> mov x20, x0 // DTB address
>> ldr x0, [sp, #16] // relocated _text address
>> - ldr x21, =stext_offset
>> + movz x21, #:abs_g0:stext_offset
>
> This looks a little scary. Why do we need to use a movz with a special
> relocation rather than a simple mov? As far as I can see mov has
> sufficient range.
>
stext_offset is an external symbol supplied by the linker, so you need
a relocation one way or the other, and
plain mov doesn't have those.
>> add x21, x0, x21
>>
>> /*
>> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
>> index 9c856f2aa7a5..1ea3cd2aba34 100644
>> --- a/arch/arm64/kernel/head.S
>> +++ b/arch/arm64/kernel/head.S
>> @@ -46,11 +46,9 @@
>> #error TEXT_OFFSET must be less than 2MB
>> #endif
>>
>> - .macro pgtbl, ttb0, ttb1, virt_to_phys
>> - ldr \ttb1, =swapper_pg_dir
>> - ldr \ttb0, =idmap_pg_dir
>> - add \ttb1, \ttb1, \virt_to_phys
>> - add \ttb0, \ttb0, \virt_to_phys
>> + .macro pgtbl, ttb0, ttb1
>> + adrp \ttb1, swapper_pg_dir
>> + adrp \ttb0, idmap_pg_dir
>> .endm
>
> I reckon we should just kill pgtbl and use adrp inline in both of the
> callsites. That way we can also get rid of the comment in each case,
> beacuse the assembly itself will document which register gets which
> table address.
>
Agreed.
>> #ifdef CONFIG_ARM64_64K_PAGES
>> @@ -63,7 +61,7 @@
>> #define TABLE_SHIFT PUD_SHIFT
>> #endif
>>
>> -#define KERNEL_START KERNEL_RAM_VADDR
>> +#define KERNEL_START _text
>
> We can kill the KERNEL_RAM_VADDR definition too, I believe. It's only
> used here.
>
I am using it in __calc_phys_offset, but there it is probably better
to opencode it as (PAGE_OFFSET + TEXT_OFFSET) for clarity.
>> #define KERNEL_END _end
>>
>> /*
>> @@ -322,7 +320,7 @@ ENDPROC(stext)
>> * been enabled
>> */
>> __create_page_tables:
>> - pgtbl x25, x26, x28 // idmap_pg_dir and swapper_pg_dir addresses
>> + pgtbl x25, x26 // idmap_pg_dir and swapper_pg_dir addresses
>
> Here we could just have:
>
> adrp x25, idmap_pg_dir
> adrp x26, swapper_pg_dir
>
>> mov x27, lr
>>
>> /*
>> @@ -351,8 +349,7 @@ __create_page_tables:
>> * Create the identity mapping.
>> */
>> mov x0, x25 // idmap_pg_dir
>> - ldr x3, =KERNEL_START
>> - add x3, x3, x28 // __pa(KERNEL_START)
>> + adr_l x3, KERNEL_START // __pa(KERNEL_START)
>>
>> #ifndef CONFIG_ARM64_VA_BITS_48
>> #define EXTRA_SHIFT (PGDIR_SHIFT + PAGE_SHIFT - 3)
>> @@ -391,9 +388,8 @@ __create_page_tables:
>> #endif
>>
>> create_pgd_entry x0, x3, x5, x6
>> - ldr x6, =KERNEL_END
>> mov x5, x3 // __pa(KERNEL_START)
>> - add x6, x6, x28 // __pa(KERNEL_END)
>> + adr_l x6, KERNEL_END // __pa(KERNEL_END)
>> create_block_map x0, x7, x3, x5, x6
>>
>> /*
>> @@ -402,8 +398,10 @@ __create_page_tables:
>> mov x0, x26 // swapper_pg_dir
>> mov x5, #PAGE_OFFSET
>> create_pgd_entry x0, x5, x3, x6
>> - ldr x6, =KERNEL_END
>> + adr_l x6, KERNEL_END
>> mov x3, x24 // phys offset
>> + sub x6, x6, x3 // kernel memsize
>> + add x6, x6, x5 // __va(KERNEL_END)
>
> I'm not sure on this; it makes it consistent with the rest w.r.t. using
> adr* but it's now a little harder to read :/
>
Yes, but 'ldr x6, =XX' is going to return 0 until the relocation code
has executed.
>> create_block_map x0, x7, x3, x5, x6
>>
>> /*
>> @@ -538,8 +536,7 @@ ENDPROC(el2_setup)
>> * in x20. See arch/arm64/include/asm/virt.h for more info.
>> */
>> ENTRY(set_cpu_boot_mode_flag)
>> - ldr x1, =__boot_cpu_mode // Compute __boot_cpu_mode
>> - add x1, x1, x28
>> + adr_l x1, __boot_cpu_mode // Compute __boot_cpu_mode
>
> The comment seems redundant now (and arguably was before). I think it's
> a distraction we can drop.
>
ok
>> cmp w20, #BOOT_CPU_MODE_EL2
>> b.ne 1f
>> add x1, x1, #4
>> @@ -598,7 +595,7 @@ ENTRY(secondary_startup)
>> /*
>> * Common entry point for secondary CPUs.
>> */
>> - pgtbl x25, x26, x28 // x25=TTBR0, x26=TTBR1
>> + pgtbl x25, x26 // x25=TTBR0, x26=TTBR1
>
> As mentioned above, I think this is clearer as:
>
> adrp x25, idmap_pg_dir
> adrp x26, swapper_pg_dir
>
>> bl __cpu_setup // initialise processor
>>
>> ldr x21, =secondary_data
>> @@ -655,17 +652,14 @@ ENDPROC(__turn_mmu_on)
>> * Calculate the start of physical memory.
>> */
>> __calc_phys_offset:
>> - adr x0, 1f
>> - ldp x1, x2, [x0]
>> + adrp x0, KERNEL_START // __pa(KERNEL_START)
>> + ldr x1, =KERNEL_RAM_VADDR // __va(KERNEL_START)
>> + mov x2, PAGE_OFFSET
>
> Missing '#' on the PAGE_OFFSET immediate.
>
yep
>> sub x28, x0, x1 // x28 = PHYS_OFFSET - PAGE_OFFSET
>> add x24, x2, x28 // x24 = PHYS_OFFSET
>> ret
>> ENDPROC(__calc_phys_offset)
>>
>> - .align 3
>> -1: .quad .
>> - .quad PAGE_OFFSET
>> -
>> /*
>> * Exception handling. Something went wrong and we can't proceed. We ought to
>> * tell the user, but since we don't have any guarantee that we're even
>> --
>> 1.8.3.2
>>
>>
More information about the linux-arm-kernel
mailing list