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
