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 <[email protected]>
> > ---
> > 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
