Re: [PATCH v2 2/3] xen/riscv: initialize .bss section

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2023-03-02 at 14:12 +0000, Andrew Cooper wrote:
> On 02/03/2023 1:23 pm, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes since v1:
> >  * initialization of .bss was moved to head.S
> > ---
> >  xen/arch/riscv/include/asm/asm.h | 4 ++++
> >  xen/arch/riscv/riscv64/head.S    | 9 +++++++++
> >  2 files changed, 13 insertions(+)
> > 
> > diff --git a/xen/arch/riscv/include/asm/asm.h
> > b/xen/arch/riscv/include/asm/asm.h
> > index 6d426ecea7..5208529cb4 100644
> > --- a/xen/arch/riscv/include/asm/asm.h
> > +++ b/xen/arch/riscv/include/asm/asm.h
> > @@ -26,14 +26,18 @@
> >  #if __SIZEOF_POINTER__ == 8
> >  #ifdef __ASSEMBLY__
> >  #define RISCV_PTR              .dword
> > +#define RISCV_SZPTR            8
> >  #else
> >  #define RISCV_PTR              ".dword"
> > +#define RISCV_SZPTR            8
> >  #endif
> >  #elif __SIZEOF_POINTER__ == 4
> >  #ifdef __ASSEMBLY__
> >  #define RISCV_PTR              .word
> > +#define RISCV_SZPTR            4
> >  #else
> >  #define RISCV_PTR              ".word"
> > +#define RISCV_SZPTR            4
> 
> This an extremely verbose way of saying that __SIZEOF_POINTER__ is
> the
> right value to use...
> 
> Just drop the change here.  The code is better without this
> indirection.
> 
> >  #endif
> >  #else
> >  #error "Unexpected __SIZEOF_POINTER__"
> > diff --git a/xen/arch/riscv/riscv64/head.S
> > b/xen/arch/riscv/riscv64/head.S
> > index 851b4691a5..b139976b7a 100644
> > --- a/xen/arch/riscv/riscv64/head.S
> > +++ b/xen/arch/riscv/riscv64/head.S
> > @@ -13,6 +13,15 @@ ENTRY(start)
> >          lla     a6, _dtb_base
> >          REG_S   a1, (a6)
> >  
> 
> /* Clear the BSS */
> 
> The comments (even just oneliners) will become increasingly useful as
> the logic here grows.
> 
> > +        la      a3, __bss_start
> > +        la      a4, __bss_end
> > +        ble     a4, a3, clear_bss_done
> > +clear_bss:
> > +        REG_S   zero, (a3)
> > +        add     a3, a3, RISCV_SZPTR
> > +        blt     a3, a4, clear_bss
> > +clear_bss_done:
> 
> You should use t's here, not a's.  What we are doing here is
> temporary
> and not constructing arguments to a function.  Furthermore we want to
> preserve the a's where possible to avoid spilling the parameters.
> 
> Finally, the symbols should have an .L_ prefix to make the local
> symbols.
> 
> It really doesn't matter now, but will when you're retrofitting elf
> metadata to asm code to make livepatching work.  (I'm doing this on
> x86
> and it would have been easier if people had written the code nicely
> the
> first time around.)
Thanks. I'll update the code.
> 
> ~Andrew