| Message ID | 495bd1d7fc2becdd48bd00253c079971e2231e99.1677762812.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Do basic initialization things | expand |
On 02/03/2023 1:23 pm, Oleksii Kurochko wrote: > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > 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
On 02.03.2023 14:23, Oleksii Kurochko wrote: > --- 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) > > + la a3, __bss_start > + la a4, __bss_end > + ble a4, a3, clear_bss_done While it may be that .bss is indeed empty right now, even short term it won't be, and never will. I'd drop this conditional (and in particular the label), inserting a transient item into .bss for the time being. As soon as your patch introducing page tables has landed, there will be multiple pages worth of .bss. Also are this and ... > +clear_bss: > + REG_S zero, (a3) > + add a3, a3, RISCV_SZPTR > + blt a3, a4, clear_bss ... this branch actually the correct ones? I'd expect the unsigned flavors to be used when comparing addresses. It may not matter here and/or right now, but it'll set a bad precedent unless you expect to only ever work on addresses which have the sign bit clear. Jan > +clear_bss_done: > + > la sp, cpu0_boot_stack > li t0, STACK_SIZE > add sp, sp, t0
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@gmail.com> > > --- > > 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
On Thu, 2023-03-02 at 15:22 +0100, Jan Beulich wrote: > On 02.03.2023 14:23, Oleksii Kurochko wrote: > > --- 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) > > > > + la a3, __bss_start > > + la a4, __bss_end > > + ble a4, a3, clear_bss_done > > While it may be that .bss is indeed empty right now, even short term > it won't be, and never will. I'd drop this conditional (and in > particular the label), inserting a transient item into .bss for the > time being. As soon as your patch introducing page tables has landed, > there will be multiple pages worth of .bss. If I understand you correctly you suggested declare some variable: int dummy_bss __attribute__((unused)); Then .bss won't be zero: $ riscv64-linux-gnu-objdump -x xen/xen-syms | grep -i dummy_bss 0000000080205000 g O .bss 0000000000000004 .hidden dummy_bss And when page tables will be ready it will be needed to remove dummy_bss. Another one option is to update linker script ( looks better then previous one ): --- a/xen/arch/riscv/xen.lds.S +++ b/xen/arch/riscv/xen.lds.S @@ -140,6 +140,7 @@ SECTIONS . = ALIGN(SMP_CACHE_BYTES); __per_cpu_data_end = .; *(.bss .bss.*) + . = . + 1; . = ALIGN(POINTER_ALIGN); __bss_end = .; } :text If one of the options is fine then to be honest I am not sure that I understand why it is better than have 3 instructions which will be unnecessary when first bss variable will be introduced. And actually the same will be with item in bss, it will become unnecessary when something from bss will be introduced. I am OK with one of the mentioned above options but still would like to understand what are advantages. > > Also are this and ... > > > +clear_bss: > > + REG_S zero, (a3) > > + add a3, a3, RISCV_SZPTR > > + blt a3, a4, clear_bss > > ... this branch actually the correct ones? I'd expect the unsigned > flavors to be used when comparing addresses. It may not matter here > and/or right now, but it'll set a bad precedent unless you expect > to only ever work on addresses which have the sign bit clear. I'll change blt to bltu. ~ Oleksii
On 02.03.2023 16:55, Oleksii wrote: > On Thu, 2023-03-02 at 15:22 +0100, Jan Beulich wrote: >> On 02.03.2023 14:23, Oleksii Kurochko wrote: >>> --- 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) >>> >>> + la a3, __bss_start >>> + la a4, __bss_end >>> + ble a4, a3, clear_bss_done >> >> While it may be that .bss is indeed empty right now, even short term >> it won't be, and never will. I'd drop this conditional (and in >> particular the label), inserting a transient item into .bss for the >> time being. As soon as your patch introducing page tables has landed, >> there will be multiple pages worth of .bss. > If I understand you correctly you suggested declare some variable: > int dummy_bss __attribute__((unused)); > > Then .bss won't be zero: > $ riscv64-linux-gnu-objdump -x xen/xen-syms | grep -i dummy_bss > 0000000080205000 g O .bss 0000000000000004 .hidden dummy_bss > > And when page tables will be ready it will be needed to remove > dummy_bss. > > Another one option is to update linker script ( looks better then > previous one ): > --- a/xen/arch/riscv/xen.lds.S > +++ b/xen/arch/riscv/xen.lds.S > @@ -140,6 +140,7 @@ SECTIONS > . = ALIGN(SMP_CACHE_BYTES); > __per_cpu_data_end = .; > *(.bss .bss.*) > + . = . + 1; > . = ALIGN(POINTER_ALIGN); > __bss_end = .; > } :text Right, I did think of this as an alternative solution as well. Either is fine with me. > If one of the options is fine then to be honest I am not sure that I > understand why it is better than have 3 instructions which will be > unnecessary when first bss variable will be introduced. And actually > the same will be with item in bss, it will become unnecessary when > something from bss will be introduced. > > I am OK with one of the mentioned above options but still would like > to understand what are advantages. You could also remove the branch and the label once .bss is no longer empty. It'll just raise needless questions if that's left in long term. Plus - I'm not a maintainer, I'm only voicing suggestions ... Jan
On 02/03/2023 3:55 pm, Oleksii wrote: > On Thu, 2023-03-02 at 15:22 +0100, Jan Beulich wrote: >> On 02.03.2023 14:23, Oleksii Kurochko wrote: >>> --- 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) >>> >>> + la a3, __bss_start >>> + la a4, __bss_end >>> + ble a4, a3, clear_bss_done >> While it may be that .bss is indeed empty right now, even short term >> it won't be, and never will. I'd drop this conditional (and in >> particular the label), inserting a transient item into .bss for the >> time being. As soon as your patch introducing page tables has landed, >> there will be multiple pages worth of .bss. > If I understand you correctly you suggested declare some variable: > int dummy_bss __attribute__((unused)); > > Then .bss won't be zero: > $ riscv64-linux-gnu-objdump -x xen/xen-syms | grep -i dummy_bss > 0000000080205000 g O .bss 0000000000000004 .hidden dummy_bss > > And when page tables will be ready it will be needed to remove > dummy_bss. Well - to be deleted when the first real bss user appears, but yes - that will probably be the pagetable series. > > Another one option is to update linker script ( looks better then > previous one ): > --- a/xen/arch/riscv/xen.lds.S > +++ b/xen/arch/riscv/xen.lds.S > @@ -140,6 +140,7 @@ SECTIONS > . = ALIGN(SMP_CACHE_BYTES); > __per_cpu_data_end = .; > *(.bss .bss.*) > + . = . + 1; > . = ALIGN(POINTER_ALIGN); > __bss_end = .; > } :text > > If one of the options is fine then to be honest I am not sure that I > understand why it is better than have 3 instructions which will be > unnecessary when first bss variable will be introduced. And actually > the same will be with item in bss, it will become unnecessary when > something from bss will be introduced. > > I am OK with one of the mentioned above options but still would like > to understand what are advantages. A one-line delete in a C file deletion is most obviously-safe of the 3 options to be performed at some later date, when we've started forgetting the specific details in this patch. >> Also are this and ... >> >>> +clear_bss: >>> + REG_S zero, (a3) >>> + add a3, a3, RISCV_SZPTR >>> + blt a3, a4, clear_bss >> ... this branch actually the correct ones? I'd expect the unsigned >> flavors to be used when comparing addresses. It may not matter here >> and/or right now, but it'll set a bad precedent unless you expect >> to only ever work on addresses which have the sign bit clear. > I'll change blt to bltu. This should indeed an unsigned compare. It doesn't explode in practice because paging is disabled and RISC-V's MAXPHYADDR is 56 bits so doesn't set the sign bit. ~Andrew
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 #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) + 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: + la sp, cpu0_boot_stack li t0, STACK_SIZE add sp, sp, t0
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- 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(+)