Re: [PATCH] x86/boot: Introduce boot-helpers.h

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Mon, Nov 18, 2024 at 5:18 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> Eclair complains that neither reloc_trampoline{32,64}() can see their
> declarations.
>
> reloc_trampoline32() needs to become asmlinkage, while reloc_trampoline64()
> needs declaring properly in a way that both efi-boot.h and reloc-trampoline.c
> can see.
>
> Introduce boot-helpers.h for the purpose.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>
> https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1549438881
> ---
>  xen/arch/x86/boot/reloc-trampoline.c    |  4 +++-
>  xen/arch/x86/efi/efi-boot.h             |  4 ++--
>  xen/arch/x86/include/asm/boot-helpers.h | 13 +++++++++++++
>  3 files changed, 18 insertions(+), 3 deletions(-)
>  create mode 100644 xen/arch/x86/include/asm/boot-helpers.h
>
> diff --git a/xen/arch/x86/boot/reloc-trampoline.c 
> b/xen/arch/x86/boot/reloc-trampoline.c
> index d5548eb08f85..e35e7c78aa86 100644
> --- a/xen/arch/x86/boot/reloc-trampoline.c
> +++ b/xen/arch/x86/boot/reloc-trampoline.c
> @@ -2,13 +2,15 @@
>
>  #include <xen/compiler.h>
>  #include <xen/stdint.h>
> +
> +#include <asm/boot-helpers.h>
>  #include <asm/trampoline.h>
>
>  extern const int32_t __trampoline_rel_start[], __trampoline_rel_stop[];
>  extern const int32_t __trampoline_seg_start[], __trampoline_seg_stop[];
>
>  #if defined(__i386__)
> -void reloc_trampoline32(void)
> +void asmlinkage reloc_trampoline32(void)
>  #elif defined (__x86_64__)
>  void reloc_trampoline64(void)
>  #else
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 9d3f2b71447e..1d8902a9a724 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -4,6 +4,8 @@
>   * therefore can define arch specific global variables.
>   */
>  #include <xen/vga.h>
> +
> +#include <asm/boot-helpers.h>
>  #include <asm/e820.h>
>  #include <asm/edd.h>
>  #include <asm/microcode.h>
> @@ -103,8 +105,6 @@ static void __init efi_arch_relocate_image(unsigned long 
> delta)
>      }
>  }
>
> -void reloc_trampoline64(void);
> -
>  static void __init relocate_trampoline(unsigned long phys)
>  {
>      trampoline_phys = phys;
> diff --git a/xen/arch/x86/include/asm/boot-helpers.h 
> b/xen/arch/x86/include/asm/boot-helpers.h
> new file mode 100644
> index 000000000000..166f49b4da01
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/boot-helpers.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Declarations for helper functions compiled for both 32bit and 64bit.
> + *
> + * The 32bit forms are called only from assembly, so no declaration is 
> provide
> + * here.
> + */
> +#ifndef X86_BOOT_HELPERS_H
> +#define X86_BOOT_HELPERS_H

Why not follow the coding style ? IMHO if we don't agree on coding
style we should update it, otherwise we should follow coding style.

Maybe considering this
https://lists.xenproject.org/archives/html/xen-devel/2024-11/msg00658.html
move the file into the boot include directory instead?

> +
> +void reloc_trampoline64(void);
> +
> +#endif /* X86_BOOT_HELPERS_H */

Frediano