diff mbox series

x86/boot: Introduce boot-helpers.h

Message ID 20241118171809.2447714-1-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series x86/boot: Introduce boot-helpers.h | expand

Commit Message

Andrew Cooper Nov. 18, 2024, 5:18 p.m. UTC 
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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Frediano Ziglio <frediano.ziglio@cloud.com>
CC: Stefano Stabellini <sstabellini@kernel.org>

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

Comments

Jan Beulich Nov. 19, 2024, 8:19 a.m. UTC | #1 
On 18.11.2024 18:18, Andrew Cooper 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@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> --- /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.
> + */

How certain are you/we that what is said in the 2nd paragraph will remain
as a pattern?

Also, nit: s/provide/provided/?

Jan
Frediano Ziglio Nov. 19, 2024, 9:11 a.m. UTC | #2 
On Mon, Nov 18, 2024 at 5:18 PM Andrew Cooper <andrew.cooper3@citrix.com> 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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Frediano Ziglio <frediano.ziglio@cloud.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
>
> 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
Andrew Cooper Nov. 19, 2024, 1:14 p.m. UTC | #3 
On 19/11/2024 8:19 am, Jan Beulich wrote:
> On 18.11.2024 18:18, Andrew Cooper 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@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>> --- /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.
>> + */
> How certain are you/we that what is said in the 2nd paragraph will remain
> as a pattern?

Unsure, but there needs to be an explanation of why reloc_trampoline32()
isn't here.

It's not clear how we'd even safely get 32bit declarations here.  They
would really want to be nocall, so the rest of Xen can't use them
accidentally.

The comment is easy enough to change if the circumstances change.

> Also, nit: s/provide/provided/?

Oops yes, will fix.

~Andrew
diff mbox series

Patch

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
+
+void reloc_trampoline64(void);
+
+#endif /* X86_BOOT_HELPERS_H */