Re: [PATCH v3 2/2] x86/head64: Replace pointer fixups with RIP_RELATIVE_ADDR()

[Tom Lendacky]

On 1/30/24 16:08, Kevin Loughlin wrote:
> Now that we have RIP_RELATIVE_ADDR(), we can replace the "fixup_*()"
> family of functions in head64.c with the new macro for cleaner code.

If this series is purely for backporting, this specific patch isn't 
needed, right? Since this all works today with clang?

Thanks,
Tom

> Signed-off-by: Kevin Loughlin <kevinloughlin@xxxxxxxxxx>
> ---
>   arch/x86/kernel/head64.c | 63 +++++++++++++++-------------------------
>   1 file changed, 24 insertions(+), 39 deletions(-)
>
> diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
> index d159239997f4..e782149eefc4 100644
> --- a/arch/x86/kernel/head64.c
> +++ b/arch/x86/kernel/head64.c
> @@ -85,23 +85,8 @@ static struct desc_ptr startup_gdt_descr __initdata = {
>   	.address = 0,
>   };
>   
> -static void __head *fixup_pointer(void *ptr, unsigned long physaddr)
> -{
> -	return ptr - (void *)_text + (void *)physaddr;
> -}
> -
> -static unsigned long __head *fixup_long(void *ptr, unsigned long physaddr)
> -{
> -	return fixup_pointer(ptr, physaddr);
> -}
> -
>   #ifdef CONFIG_X86_5LEVEL
> -static unsigned int __head *fixup_int(void *ptr, unsigned long physaddr)
> -{
> -	return fixup_pointer(ptr, physaddr);
> -}
> -
> -static bool __head check_la57_support(unsigned long physaddr)
> +static bool __head check_la57_support(void)
>   {
>   	/*
>   	 * 5-level paging is detected and enabled at kernel decompression
> @@ -110,17 +95,17 @@ static bool __head check_la57_support(unsigned long physaddr)
>   	if (!(native_read_cr4() & X86_CR4_LA57))
>   		return false;
>   
> -	*fixup_int(&__pgtable_l5_enabled, physaddr) = 1;
> -	*fixup_int(&pgdir_shift, physaddr) = 48;
> -	*fixup_int(&ptrs_per_p4d, physaddr) = 512;
> -	*fixup_long(&page_offset_base, physaddr) = __PAGE_OFFSET_BASE_L5;
> -	*fixup_long(&vmalloc_base, physaddr) = __VMALLOC_BASE_L5;
> -	*fixup_long(&vmemmap_base, physaddr) = __VMEMMAP_BASE_L5;
> +	*((unsigned int *)RIP_RELATIVE_ADDR(__pgtable_l5_enabled)) = 1;
> +	*((unsigned int *)RIP_RELATIVE_ADDR(pgdir_shift)) = 48;
> +	*((unsigned int *)RIP_RELATIVE_ADDR(ptrs_per_p4d)) = 512;
> +	*((unsigned long *)RIP_RELATIVE_ADDR(page_offset_base)) = __PAGE_OFFSET_BASE_L5;
> +	*((unsigned long *)RIP_RELATIVE_ADDR(vmalloc_base)) = __VMALLOC_BASE_L5;
> +	*((unsigned long *)RIP_RELATIVE_ADDR(vmemmap_base)) = __VMEMMAP_BASE_L5;
>   
>   	return true;
>   }
>   #else
> -static bool __head check_la57_support(unsigned long physaddr)
> +static bool __head check_la57_support(void)
>   {
>   	return false;
>   }
> @@ -175,8 +160,8 @@ static unsigned long __head sme_postprocess_startup(struct boot_params *bp, pmdv
>    * Code in __startup_64() can be relocated during execution, but the compiler
>    * doesn't have to generate PC-relative relocations when accessing globals from
>    * that function. Clang actually does not generate them, which leads to
> - * boot-time crashes. To work around this problem, every global pointer must
> - * be adjusted using fixup_pointer() or RIP_RELATIVE_ADDR().
> + * boot-time crashes. To work around this problem, every global variable access
> + * must be adjusted using RIP_RELATIVE_ADDR().
>    */
>   unsigned long __head __startup_64(unsigned long physaddr,
>   				  struct boot_params *bp)
> @@ -194,7 +179,7 @@ unsigned long __head __startup_64(unsigned long physaddr,
>   	int i;
>   	unsigned int *next_pgt_ptr;
>   
> -	la57 = check_la57_support(physaddr);
> +	la57 = check_la57_support();
>   
>   	/* Is the address too large? */
>   	if (physaddr >> MAX_PHYSMEM_BITS)
> @@ -215,7 +200,7 @@ unsigned long __head __startup_64(unsigned long physaddr,
>   
>   	/* Fixup the physical addresses in the page table */
>   
> -	pgd = fixup_pointer(early_top_pgt, physaddr);
> +	pgd = RIP_RELATIVE_ADDR(early_top_pgt);
>   	p = pgd + pgd_index(__START_KERNEL_map);
>   	if (la57)
>   		*p = (unsigned long)level4_kernel_pgt;
> @@ -224,15 +209,15 @@ unsigned long __head __startup_64(unsigned long physaddr,
>   	*p += _PAGE_TABLE_NOENC - __START_KERNEL_map + load_delta;
>   
>   	if (la57) {
> -		p4d = fixup_pointer(level4_kernel_pgt, physaddr);
> +		p4d = RIP_RELATIVE_ADDR(level4_kernel_pgt);
>   		p4d[511] += load_delta;
>   	}
>   
> -	pud = fixup_pointer(level3_kernel_pgt, physaddr);
> +	pud = RIP_RELATIVE_ADDR(level3_kernel_pgt);
>   	pud[510] += load_delta;
>   	pud[511] += load_delta;
>   
> -	pmd = fixup_pointer(level2_fixmap_pgt, physaddr);
> +	pmd = RIP_RELATIVE_ADDR(level2_fixmap_pgt);
>   	for (i = FIXMAP_PMD_TOP; i > FIXMAP_PMD_TOP - FIXMAP_PMD_NUM; i--)
>   		pmd[i] += load_delta;
>   
> @@ -243,8 +228,8 @@ unsigned long __head __startup_64(unsigned long physaddr,
>   	 * it avoids problems around wraparound.
>   	 */
>   
> -	next_pgt_ptr = fixup_pointer(&next_early_pgt, physaddr);
> -	early_dynamic_pgts_ptr = fixup_pointer(early_dynamic_pgts, physaddr);
> +	early_dynamic_pgts_ptr = RIP_RELATIVE_ADDR(early_dynamic_pgts);
> +	next_pgt_ptr = RIP_RELATIVE_ADDR(next_early_pgt);
>   	pud = (pudval_t *) early_dynamic_pgts_ptr[(*next_pgt_ptr)++];
>   	pmd = (pmdval_t *) early_dynamic_pgts_ptr[(*next_pgt_ptr)++];
>   
> @@ -272,7 +257,7 @@ unsigned long __head __startup_64(unsigned long physaddr,
>   
>   	pmd_entry = __PAGE_KERNEL_LARGE_EXEC & ~_PAGE_GLOBAL;
>   	/* Filter out unsupported __PAGE_KERNEL_* bits: */
> -	mask_ptr = fixup_pointer(&__supported_pte_mask, physaddr);
> +	mask_ptr = RIP_RELATIVE_ADDR(__supported_pte_mask);
>   	pmd_entry &= *mask_ptr;
>   	pmd_entry += sme_me_mask_fixed_up;
>   	pmd_entry +=  physaddr;
> @@ -299,7 +284,7 @@ unsigned long __head __startup_64(unsigned long physaddr,
>   	 * error, causing the BIOS to halt the system.
>   	 */
>   
> -	pmd = fixup_pointer(level2_kernel_pgt, physaddr);
> +	pmd = RIP_RELATIVE_ADDR(level2_kernel_pgt);
>   
>   	/* invalidate pages before the kernel image */
>   	for (i = 0; i < pmd_index((unsigned long)_text); i++)
> @@ -318,7 +303,7 @@ unsigned long __head __startup_64(unsigned long physaddr,
>   	 * Fixup phys_base - remove the memory encryption mask to obtain
>   	 * the true physical address.
>   	 */
> -	*fixup_long(&phys_base, physaddr) += load_delta - sme_me_mask_fixed_up;
> +	*((unsigned long *)RIP_RELATIVE_ADDR(phys_base)) += load_delta - sme_me_mask_fixed_up;
>   
>   	return sme_postprocess_startup(bp, pmd);
>   }
> @@ -594,15 +579,15 @@ static void set_bringup_idt_handler(gate_desc *idt, int n, void *handler)
>   /* This runs while still in the direct mapping */
>   static void __head startup_64_load_idt(unsigned long physbase)
>   {
> -	struct desc_ptr *desc = fixup_pointer(&bringup_idt_descr, physbase);
> -	gate_desc *idt = fixup_pointer(bringup_idt_table, physbase);
> +	struct desc_ptr *desc = RIP_RELATIVE_ADDR(bringup_idt_descr);
> +	gate_desc *idt = RIP_RELATIVE_ADDR(bringup_idt_table);
>   
>   
>   	if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)) {
>   		void *handler;
>   
>   		/* VMM Communication Exception */
> -		handler = fixup_pointer(vc_no_ghcb, physbase);
> +		handler = RIP_RELATIVE_ADDR(vc_no_ghcb);
>   		set_bringup_idt_handler(idt, X86_TRAP_VC, handler);
>   	}
>   
> @@ -629,7 +614,7 @@ void early_setup_idt(void)
>   void __head startup_64_setup_env(unsigned long physbase)
>   {
>   	/* Load GDT */
> -	startup_gdt_descr.address = (unsigned long)fixup_pointer(startup_gdt, physbase);
> +	startup_gdt_descr.address = (unsigned long)RIP_RELATIVE_ADDR(startup_gdt);
>   	native_load_gdt(&startup_gdt_descr);
>   
>   	/* New GDT is live - reload data segment registers */