Re: [PATCH] xen/riscv: init bss section
On Fri, 2023-02-24 at 16:55 +, Andrew Cooper wrote:
> On 24/02/2023 2:48 pm, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko
> > ---
> > 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
Re: [PATCH] xen/riscv: init bss section
On 24/02/2023 2:48 pm, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko
> ---
> 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
Re: [PATCH] xen/riscv: init bss section
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
#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
Re: [PATCH] xen/riscv: init bss section
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
Re: [PATCH] xen/riscv: init bss section
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
[PATCH] xen/riscv: init bss section
Signed-off-by: Oleksii Kurochko
---
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();
--
2.39.0
