Re: [PATCH v7 3/5] xen/riscv: align __bss_start

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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@xxxxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 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