| Message ID | 134bf758ecd93deffc6623605a8c020a17f64be8.1677249688.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/riscv: init bss section | expand |
On 24.02.2023 15:48, Oleksii Kurochko wrote: > --- a/xen/arch/riscv/setup.c > +++ b/xen/arch/riscv/setup.c > @@ -24,6 +24,18 @@ static void test_macros_from_bug_h(void) > early_printk("WARN is most likely working\n"); > } > > +static void __init init_bss(void) > +{ > + extern char __bss_start; > + extern char __bss_end; Better use [] and then perhaps omit the & operators further down. However, I thought we have a compiler warning option in use which precludes extern declarations which aren't at file scope. Even if I'm misremembering, perhaps better to move them. > + char *bss = &__bss_start; > + > + while ( bss < &__bss_end ) { > + *bss = 0; > + bss++; > + } > +} If you're sure you can defer this until being in C code, why not use memset()? Jan
On 24.02.2023 15:56, Jan Beulich wrote: > On 24.02.2023 15:48, Oleksii Kurochko wrote: >> + char *bss = &__bss_start; >> + >> + while ( bss < &__bss_end ) { >> + *bss = 0; >> + bss++; >> + } >> +} > > If you're sure you can defer this until being in C code, why not use > memset()? Oh, otherwise: Nit (style) - brace placement. Jan
On Fri, 2023-02-24 at 15:56 +0100, Jan Beulich wrote: > On 24.02.2023 15:48, Oleksii Kurochko wrote: > > --- a/xen/arch/riscv/setup.c > > +++ b/xen/arch/riscv/setup.c > > @@ -24,6 +24,18 @@ static void test_macros_from_bug_h(void) > > early_printk("WARN is most likely working\n"); > > } > > > > +static void __init init_bss(void) > > +{ > > + extern char __bss_start; > > + extern char __bss_end; > > Better use [] and then perhaps omit the & operators further down. > However, I thought we have a compiler warning option in use which > precludes extern declarations which aren't at file scope. Even if > I'm misremembering, perhaps better to move them. Thanks. I will update the code then to use []. > > > + char *bss = &__bss_start; > > + > > + while ( bss < &__bss_end ) { > > + *bss = 0; > > + bss++; > > + } > > +} > > If you're sure you can defer this until being in C code, why not use > memset()? I had an issue with from <xen/string.h> #ifndef __HAVE_ARCH_MEMSET #define memset(s, c, n) __builtin_memset(s, c, n) #endif but there is no issue any more so I think I can use memset(). > > Jan ~ Oleksii
On 24/02/2023 2:48 pm, Oleksii Kurochko wrote: > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > xen/arch/riscv/setup.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c > index 154bf3a0bc..593bb471a4 100644 > --- a/xen/arch/riscv/setup.c > +++ b/xen/arch/riscv/setup.c > @@ -24,6 +24,18 @@ static void test_macros_from_bug_h(void) > early_printk("WARN is most likely working\n"); > } > > +static void __init init_bss(void) > +{ > + extern char __bss_start; > + extern char __bss_end; > + char *bss = &__bss_start; > + > + while ( bss < &__bss_end ) { > + *bss = 0; > + bss++; > + } > +} > + > void __init noreturn start_xen(void) > { > /* > @@ -38,6 +50,8 @@ void __init noreturn start_xen(void) > > asm volatile( "mv %0, a1" : "=r" (dtb_base) ); > > + init_bss(); > + > early_printk("Hello from C env\n"); > > trap_init(); Zeroing the BSS needs to one of the earliest thing you do. It really does need to be before entering C, and needs to be as close to the start of head.S as you can reasonably make it. I'd put it even before loading sp in start. Even like this, there are various things the compiler might do behind your back which expect a) the BSS to already be zeroed, and b) not change value unexpectedly. Also, note: arch/riscv/xen.lds.S-143- . = ALIGN(POINTER_ALIGN); arch/riscv/xen.lds.S:144: __bss_end = .; The POINTER_ALIGN there is specifically so you can depend on __bss_{start,end} being suitably aligned to use a register-width store, rather than using byte stores, which in 64bit means you've got 8x fewer iterations. ~Andrew
On Fri, 2023-02-24 at 16:55 +0000, Andrew Cooper wrote: > On 24/02/2023 2:48 pm, Oleksii Kurochko wrote: > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > --- > > xen/arch/riscv/setup.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c > > index 154bf3a0bc..593bb471a4 100644 > > --- a/xen/arch/riscv/setup.c > > +++ b/xen/arch/riscv/setup.c > > @@ -24,6 +24,18 @@ static void test_macros_from_bug_h(void) > > early_printk("WARN is most likely working\n"); > > } > > > > +static void __init init_bss(void) > > +{ > > + extern char __bss_start; > > + extern char __bss_end; > > + char *bss = &__bss_start; > > + > > + while ( bss < &__bss_end ) { > > + *bss = 0; > > + bss++; > > + } > > +} > > + > > void __init noreturn start_xen(void) > > { > > /* > > @@ -38,6 +50,8 @@ void __init noreturn start_xen(void) > > > > asm volatile( "mv %0, a1" : "=r" (dtb_base) ); > > > > + init_bss(); > > + > > early_printk("Hello from C env\n"); > > > > trap_init(); > > Zeroing the BSS needs to one of the earliest thing you do. It really > does need to be before entering C, and needs to be as close to the > start > of head.S as you can reasonably make it. > > I'd put it even before loading sp in start. > > Even like this, there are various things the compiler might do behind > your back which expect a) the BSS to already be zeroed, and b) not > change value unexpectedly. > > > Also, note: > > arch/riscv/xen.lds.S-143- . = ALIGN(POINTER_ALIGN); > arch/riscv/xen.lds.S:144: __bss_end = .; > > The POINTER_ALIGN there is specifically so you can depend on > __bss_{start,end} being suitably aligned to use a register-width > store, > rather than using byte stores, which in 64bit means you've got 8x > fewer > iterations. Thanks for the comments. I'll take them into account in the next version of the patch. > > ~Andrew ~ Oleksii
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c index 154bf3a0bc..593bb471a4 100644 --- a/xen/arch/riscv/setup.c +++ b/xen/arch/riscv/setup.c @@ -24,6 +24,18 @@ static void test_macros_from_bug_h(void) early_printk("WARN is most likely working\n"); } +static void __init init_bss(void) +{ + extern char __bss_start; + extern char __bss_end; + char *bss = &__bss_start; + + while ( bss < &__bss_end ) { + *bss = 0; + bss++; + } +} + void __init noreturn start_xen(void) { /* @@ -38,6 +50,8 @@ void __init noreturn start_xen(void) asm volatile( "mv %0, a1" : "=r" (dtb_base) ); + init_bss(); + early_printk("Hello from C env\n"); trap_init();
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- xen/arch/riscv/setup.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)