On 22.11.2024 10:33, Frediano Ziglio wrote: > Move some assembly code to C. > > Signed-off-by: Frediano Ziglio <frediano.zig...@cloud.com> > --- > 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
