Re: [PATCH v2 4/4] x86/boot: Use external symbols from cmdline_parse_early

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.11.2024 10:33, Frediano Ziglio wrote:
> Move some assembly code to C.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
> ---
>  xen/arch/x86/boot/build32.lds.S         |  1 +
>  xen/arch/x86/boot/cmdline.c             | 14 ++++++++++++--
>  xen/arch/x86/boot/head.S                |  9 +--------
>  xen/arch/x86/boot/trampoline.S          |  2 +-
>  xen/arch/x86/include/asm/setup.h        |  2 ++
>  xen/arch/x86/include/boot/xen/cpumask.h |  1 +
>  xen/arch/x86/include/boot/xen/string.h  | 10 ++++++++++
>  7 files changed, 28 insertions(+), 11 deletions(-)
>  create mode 100644 xen/arch/x86/include/boot/xen/cpumask.h
>  create mode 100644 xen/arch/x86/include/boot/xen/string.h

Again the diffstat doesn't really suggest this is a win. As an upside
I can see that the argument passing to the function is somewhat ugly
when done from assembly, especially when the function needs new
parameters added or ones removed / changed. The downside is that now
you're switching to dealing with globals, which generally seems less
desirable.

> --- a/xen/arch/x86/include/asm/setup.h
> +++ b/xen/arch/x86/include/asm/setup.h
> @@ -16,6 +16,8 @@ extern uint64_t boot_tsc_stamp;
>  extern void *stack_start;
>  extern unsigned int multiboot_ptr;
>  
> +struct domain;
> +
>  void early_cpu_init(bool verbose);
>  void early_time_init(void);
>  

While I think I can see why this would be needed, personally I think
such forward decls belong either immediately past all #include-s or
immediately ahead of where they are first needed.

> --- /dev/null
> +++ b/xen/arch/x86/include/boot/xen/cpumask.h
> @@ -0,0 +1 @@
> +/* Empty. */

Are there perhaps better ways to deal with whatever needs dealing with
(which sadly isn't obvious and also isn't mentioned anywhere)? At a
guess, asm/numa.h may be where the problem is, yet then setup.h
includes that just to get a decl of nodeid_t afaics. As we're meaning
to split headers into two or perhaps even three parts anyway (to allow
reducing dependency chains), maybe we should do so here and introduce
e.g. asm/types/numa.h?

Jan