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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2023-03-21 at 12:27 +0100, Jan Beulich wrote:
> 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.
Sure.
> It may also want to be __initdata, depending on further planned uses.
I am going to use it only for initial page table initialization.
At least for now.
> 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?
Yes, should it be __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.
Your guessing is correct. In this case addresses of _start and _end
will be calculated in a PC-relative way.
So I'll add a comment.

> 
> 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).
Sure.

Thanks for the comments.
I'll fix that in the new version of the
patch.

~ Oleksii