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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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