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.) ~Andrew
