Re: [PATCH v8 11/28] x86/asm/head: annotate data appropriatelly

[Borislav Petkov <bp@alien8.de>]

> Subject: Re: [PATCH v8 11/28] x86/asm/head: annotate data appropriatelly

appropriately

On Thu, Aug 08, 2019 at 12:38:37PM +0200, Jiri Slaby wrote:
> Use the new SYM_DATA, SYM_DATA_START, and SYM_DATA_END in both 32 and 64
> bit heads.  In the 64-bit version, define also
> SYM_DATA_START_PAGE_ALIGNED locally using the new SYM_START. It is used
> in the code instead of NEXT_PAGE() which was defined in this file and
> has been using the obsolete macro GLOBAL().
> 
> Now, the data in the 64-bit object file look sane:
> Value   Size Type    Bind   Vis      Ndx Name
>   0000  4096 OBJECT  GLOBAL DEFAULT   15 init_level4_pgt
>   1000  4096 OBJECT  GLOBAL DEFAULT   15 level3_kernel_pgt
>   2000  2048 OBJECT  GLOBAL DEFAULT   15 level2_kernel_pgt
>   3000  4096 OBJECT  GLOBAL DEFAULT   15 level2_fixmap_pgt
>   4000  4096 OBJECT  GLOBAL DEFAULT   15 level1_fixmap_pgt
>   5000     2 OBJECT  GLOBAL DEFAULT   15 early_gdt_descr
>   5002     8 OBJECT  LOCAL  DEFAULT   15 early_gdt_descr_base
>   500a     8 OBJECT  GLOBAL DEFAULT   15 phys_base
>   0000     8 OBJECT  GLOBAL DEFAULT   17 initial_code
>   0008     8 OBJECT  GLOBAL DEFAULT   17 initial_gs
>   0010     8 OBJECT  GLOBAL DEFAULT   17 initial_stack
>   0000     4 OBJECT  GLOBAL DEFAULT   19 early_recursion_flag
>   1000  4096 OBJECT  GLOBAL DEFAULT   19 early_level4_pgt
>   2000 0x40000 OBJECT  GLOBAL DEFAULT   19 early_dynamic_pgts
>   0000  4096 OBJECT  GLOBAL DEFAULT   22 empty_zero_page
> 
> All have correct size and type.

Nice.

> 
> Note, that we can now see that it might be worth pushing
> early_recursion_flag after early_dynamic_pgts -- we are wasting almost
> 4K of .init.data.

Yes, please do in a separate patch which can even go separately. I get
here:

---
Disassembly of section .init.data:

ffffffff82684000 <early_recursion_flag>:
        ...

ffffffff82685000 <early_top_pgt>:
        ...

ffffffff82686000 <early_dynamic_pgts>:
        ...

ffffffff826c6000 <next_early_pgt>:
        ...

ffffffff826c6020 <initcall_level_names>:
---


vs


---
Disassembly of section .init.data:

ffffffff82684000 <early_top_pgt>:
        ...

ffffffff82685000 <early_dynamic_pgts>:
        ...

ffffffff826c5000 <early_recursion_flag>:
ffffffff826c5000:       00 00                   add    %al,(%rax)
        ...

ffffffff826c5004 <next_early_pgt>:
        ...

ffffffff826c5020 <initcall_level_names>:
---

That's exactly 4K saved.


> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> ---
>  arch/x86/kernel/head_32.S | 29 ++++++++-------
>  arch/x86/kernel/head_64.S | 78 +++++++++++++++++++++------------------
>  2 files changed, 58 insertions(+), 49 deletions(-)
> 
> diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
> index 0bae769b7b59..2d5390d84467 100644
> --- a/arch/x86/kernel/head_32.S
> +++ b/arch/x86/kernel/head_32.S
> @@ -502,8 +502,7 @@ ENDPROC(early_ignore_irq)
>  
>  __INITDATA
>  	.align 4
> -GLOBAL(early_recursion_flag)
> -	.long 0
> +SYM_DATA(early_recursion_flag, .long 0)
>  
>  __REFDATA
>  	.align 4
> @@ -551,7 +550,7 @@ EXPORT_SYMBOL(empty_zero_page)
>  __PAGE_ALIGNED_DATA
>  	/* Page-aligned for the benefit of paravirt? */
>  	.align PGD_ALIGN
> -ENTRY(initial_page_table)
> +SYM_DATA_START(initial_page_table)
>  	.long	pa(initial_pg_pmd+PGD_IDENT_ATTR),0	/* low identity map */
>  # if KPMDS == 3
>  	.long	pa(initial_pg_pmd+PGD_IDENT_ATTR),0
> @@ -569,17 +568,18 @@ ENTRY(initial_page_table)
>  #  error "Kernel PMDs should be 1, 2 or 3"
>  # endif
>  	.align PAGE_SIZE		/* needs to be page-sized too */
> +SYM_DATA_END(initial_page_table)
>  #endif
>  
>  .data
>  .balign 4
> -ENTRY(initial_stack)
> -	/*
> -	 * The SIZEOF_PTREGS gap is a convention which helps the in-kernel
> -	 * unwinder reliably detect the end of the stack.
> -	 */
> -	.long init_thread_union + THREAD_SIZE - SIZEOF_PTREGS - \
> -	      TOP_OF_KERNEL_STACK_PADDING;
> +/*
> + * The SIZEOF_PTREGS gap is a convention which helps the in-kernel unwinder
> + * reliably detect the end of the stack.
> + */
> +SYM_DATA(initial_stack,
> +		.long init_thread_union + THREAD_SIZE -
> +		SIZEOF_PTREGS - TOP_OF_KERNEL_STACK_PADDING)
>  
>  __INITRODATA
>  int_msg:
> @@ -600,22 +600,25 @@ int_msg:
>  	ALIGN
>  # early boot GDT descriptor (must use 1:1 address mapping)
>  	.word 0				# 32 bit align gdt_desc.address
> -boot_gdt_descr:
> +SYM_DATA_START(boot_gdt_descr)
>  	.word __BOOT_DS+7
>  	.long boot_gdt - __PAGE_OFFSET
> +SYM_DATA_END(boot_gdt_descr)

So there's one "globl boot_gdt_descr" above already and this turns into:

 .data
.globl boot_gdt_descr
^^^^^^^^^^^^^^^^^^^^^

 .align 4,0x90
 # early boot GDT descriptor (must use 1:1 address mapping)
 .word 0 # 32 bit align gdt_desc.address
.globl boot_gdt_descr ; ; boot_gdt_descr:
^^^^^^^^^^^^^^^^^^^^^

I guess you can remove the above one.

Also, this can be made a local symbol too.

>  # boot GDT descriptor (later on used by CPU#0):
>  	.word 0				# 32 bit align gdt_desc.address
> -ENTRY(early_gdt_descr)
> +SYM_DATA_START(early_gdt_descr)
>  	.word GDT_ENTRIES*8-1
>  	.long gdt_page			/* Overwritten for secondary CPUs */
> +SYM_DATA_END(early_gdt_descr)
>  
>  /*
>   * The boot_gdt must mirror the equivalent in setup.S and is
>   * used only for booting.
>   */
>  	.align L1_CACHE_BYTES
> -ENTRY(boot_gdt)
> +SYM_DATA_START(boot_gdt)
>  	.fill GDT_ENTRY_BOOT_CS,8,0
>  	.quad 0x00cf9a000000ffff	/* kernel 4GB code at 0x00000000 */
>  	.quad 0x00cf92000000ffff	/* kernel 4GB data at 0x00000000 */
> +SYM_DATA_END(boot_gdt)
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 6fedcda37634..6661c76a2049 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -260,16 +260,14 @@ END(start_cpu0)
>  	/* Both SMP bootup and ACPI suspend change these variables */
>  	__REFDATA
>  	.balign	8
> -	GLOBAL(initial_code)
> -	.quad	x86_64_start_kernel
> -	GLOBAL(initial_gs)
> -	.quad	INIT_PER_CPU_VAR(fixed_percpu_data)
> -	GLOBAL(initial_stack)
> -	/*
> -	 * The SIZEOF_PTREGS gap is a convention which helps the in-kernel
> -	 * unwinder reliably detect the end of the stack.
> -	 */
> -	.quad  init_thread_union + THREAD_SIZE - SIZEOF_PTREGS
> +SYM_DATA(initial_code,	.quad x86_64_start_kernel)
> +SYM_DATA(initial_gs,	.quad INIT_PER_CPU_VAR(fixed_percpu_data))

<---- newline here.

> +/*
> + * The SIZEOF_PTREGS gap is a convention which helps the in-kernel unwinder
> + * reliably detect the end of the stack.
> + */
> +SYM_DATA(initial_stack,
> +		.quad init_thread_union + THREAD_SIZE - SIZEOF_PTREGS)

No need to break that line.

...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.