* [PATCH] xen/riscv: init bss section
@ 2023-02-24 14:48 Oleksii Kurochko
2023-02-24 14:56 ` Jan Beulich
2023-02-24 16:55 ` Andrew Cooper
0 siblings, 2 replies; 6+ messages in thread
From: Oleksii Kurochko @ 2023-02-24 14:48 UTC (permalink / raw
To: xen-devel
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Oleksii Kurochko, Bob Eshleman, Alistair Francis, Connor Davis
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();
--
2.39.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/riscv: init bss section
2023-02-24 14:48 [PATCH] xen/riscv: init bss section Oleksii Kurochko
@ 2023-02-24 14:56 ` Jan Beulich
2023-02-24 14:57 ` Jan Beulich
2023-02-24 16:43 ` Oleksii
2023-02-24 16:55 ` Andrew Cooper
1 sibling, 2 replies; 6+ messages in thread
From: Jan Beulich @ 2023-02-24 14:56 UTC (permalink / raw
To: Oleksii Kurochko
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida, Bob Eshleman,
Alistair Francis, Connor Davis, xen-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/riscv: init bss section
2023-02-24 14:56 ` Jan Beulich
@ 2023-02-24 14:57 ` Jan Beulich
2023-02-24 16:43 ` Oleksii
1 sibling, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2023-02-24 14:57 UTC (permalink / raw
To: Oleksii Kurochko
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida, Bob Eshleman,
Alistair Francis, Connor Davis, xen-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/riscv: init bss section
2023-02-24 14:56 ` Jan Beulich
2023-02-24 14:57 ` Jan Beulich
@ 2023-02-24 16:43 ` Oleksii
1 sibling, 0 replies; 6+ messages in thread
From: Oleksii @ 2023-02-24 16:43 UTC (permalink / raw
To: Jan Beulich
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida, Bob Eshleman,
Alistair Francis, Connor Davis, xen-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/riscv: init bss section
2023-02-24 14:48 [PATCH] xen/riscv: init bss section Oleksii Kurochko
2023-02-24 14:56 ` Jan Beulich
@ 2023-02-24 16:55 ` Andrew Cooper
2023-02-27 10:06 ` Oleksii
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2023-02-24 16:55 UTC (permalink / raw
To: Oleksii Kurochko, xen-devel
Cc: Stefano Stabellini, Gianluca Guida, Bob Eshleman,
Alistair Francis, Connor Davis
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/riscv: init bss section
2023-02-24 16:55 ` Andrew Cooper
@ 2023-02-27 10:06 ` Oleksii
0 siblings, 0 replies; 6+ messages in thread
From: Oleksii @ 2023-02-27 10:06 UTC (permalink / raw
To: Andrew Cooper, xen-devel
Cc: Stefano Stabellini, Gianluca Guida, Bob Eshleman,
Alistair Francis, Connor Davis
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-02-27 10:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-24 14:48 [PATCH] xen/riscv: init bss section Oleksii Kurochko
2023-02-24 14:56 ` Jan Beulich
2023-02-24 14:57 ` Jan Beulich
2023-02-24 16:43 ` Oleksii
2023-02-24 16:55 ` Andrew Cooper
2023-02-27 10:06 ` Oleksii
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).