Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V5:
 * the patch was introduced in the current patch series (V5)
---
 xen/arch/riscv/setup.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 0908bdb9f9..36556eb779 100644
--- 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 };
+
 /*  
  * To be sure that .bss isn't zero. It will simplify code of
  * .bss initialization.
@@ -15,11 +19,19 @@ unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
  */
 int dummy_bss __attribute__((unused));
 
+static void fill_boot_info(void)
+{
+    boot_info.load_start = (unsigned long)_start;
+    boot_info.load_end   = (unsigned long)_end;
+}
+
 void __init noreturn start_xen(unsigned long bootcpu_id,
                                unsigned long dtb_paddr)
 {
     early_printk("Hello from C env\n");
 
+    fill_boot_info();
+
     early_printk("All set up\n");
     for ( ;; )
         asm volatile ("wfi");
-- 
2.39.2

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

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