| Message ID | 2e9018989c628a519aadeae150786efe5e8054ab.1683824347.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | enable MMU for RISC-V | expand |
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. > --- 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). 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. Jan
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
diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S index fe475d096d..f9d89b69b9 100644 --- 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);
bss clear cycle requires proper alignment of __bss_start. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V7: * the patch was introduced in the current patch series. --- xen/arch/riscv/xen.lds.S | 1 + 1 file changed, 1 insertion(+)