On Fri, 2023-05-12 at 09:45 +0200, Jan Beulich wrote:
> On 11.05.2023 19:09, Oleksii Kurochko wrote:
> > bss clear cycle requires proper alignment of __bss_start.
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with two remarks, though:
>
> While probably not very important yet for RISC-V (until there is at
> least enough functionality to, say, boot Dom0), you may want to get
> used to add Fixes: tags in cases like this one.
Got it.
>
> > --- a/xen/arch/riscv/xen.lds.S
> > +++ b/xen/arch/riscv/xen.lds.S
> > @@ -137,6 +137,7 @@ SECTIONS
> > __init_end = .;
> >
> > .bss : { /* BSS */
> > + . = ALIGN(POINTER_ALIGN);
> > __bss_start = .;
> > *(.bss.stack_aligned)
> > . = ALIGN(PAGE_SIZE);
>
> While independent of the change here, this ALIGN() visible in context
> is unnecessary, afaict. ALIGN() generally only makes sense when
> there's a linker-script-defined symbol right afterwards. Taking the
> case here, any contributions to .bss.page_aligned have to specify
> proper alignment themselves anyway (or else they'd be dependent upon
> linking order). Just like there's (correctly) no ALIGN(STACK_SIZE)
> ahead of *(.bss.stack_aligned).
It make sense.
>
> The change here might be a good opportunity to drop that ALIGN() at
> the same time. So long as you (and the maintainers) agree, I guess
> the adjustment could easily be made while committing.
I would agree with this. Thanks.
~ Oleksii