Re: [PATCH v5 2/7] xen/riscv: initialize boot_info structure

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.03.2023 15:39, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -1,12 +1,16 @@
>  #include <xen/compile.h>
>  #include <xen/init.h>
> +#include <xen/kernel.h>
>  
> +#include <asm/boot-info.h>
>  #include <asm/early_printk.h>
>  
>  /* Xen stack for bringing up the first CPU. */
>  unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
>      __aligned(STACK_SIZE);
>  
> +struct boot_info boot_info = { (unsigned long)&_start, (unsigned long)&_end, 
> 0UL, 0UL };

You add no declaration in a header, in which case this wants to be static.
It may also want to be __initdata, depending on further planned uses. I
would also like to suggest using C99 dedicated initializers and in any
event omit the 0UL initializer values (as that's what the compiler will
use anyway). Yet even then I think the line is still too long and hence
needs wrapping.

> @@ -15,11 +19,19 @@ unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
>   */
>  int dummy_bss __attribute__((unused));
>  
> +static void fill_boot_info(void)

__init?

> +{
> +    boot_info.load_start = (unsigned long)_start;
> +    boot_info.load_end   = (unsigned long)_end;
> +}

I'd like to understand how this is intended to work: _start and _end
are known at compile time, and their value won't change (unless you
process relocations alongside switching from 1:1 mapping to some
other virtual memory layout). Therefore - why can't these be put in
the initializer as well? Guessing that the values derived here are
different because of being calculated in a PC-relative way, I think
this really needs a comment. If, of course, this can be relied upon
in the first place.

Also please be consistent about the use of unary & when taking the
address of arrays (or functions). Personally I prefer the & to be
omitted in such cases, but what I consider relevant is that there
be no mix (in new code at least).

Jan