Re: [PATCH v3 1/6] xen/arm: Skip initializing the BSS section when it is empty

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Thu, Oct 10, 2024 at 6:29 PM Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:
>
> Hi Frediano,
>
> Appreciate your feedback.
>
> On 10/10/2024 15:43, Frediano Ziglio wrote:
> > On Thu, Oct 10, 2024 at 3:05 PM Ayan Kumar Halder
> > <ayan.kumar.halder@xxxxxxx> wrote:
> >> If the BSS section is empty, then the function can just return.
> >>
> >> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> >> ---
> >> Changes from :-
> >>
> >> v1..v2 - New patch introduced in v3.
> >>
> >>   xen/arch/arm/arm64/head.S | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> >> index 14c3720d80..72c7b24498 100644
> >> --- a/xen/arch/arm/arm64/head.S
> >> +++ b/xen/arch/arm/arm64/head.S
> >> @@ -346,6 +346,8 @@ FUNC_LOCAL(zero_bss)
> >>           PRINT("- Zero BSS -\r\n")
> >>           ldr   x0, =__bss_start       /* x0 := vaddr(__bss_start) */
> >>           ldr   x1, =__bss_end         /* x1 := vaddr(__bss_end)   */
> >> +        cmp   x1, x0
> >> +        beq   skip_bss
> >>
> >>   1:      str   xzr, [x0], #8
> >>           cmp   x0, x1
> > Why not just transforming the "do while" loop into a "while" one and
> > just jump to cmp?
> >
> > Something like (not tested)
> >
> > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > index 14c3720d80..987f243578 100644
> > --- a/xen/arch/arm/arm64/head.S
> > +++ b/xen/arch/arm/arm64/head.S
> > @@ -346,9 +346,10 @@ FUNC_LOCAL(zero_bss)
> >          PRINT("- Zero BSS -\r\n")
> >          ldr   x0, =__bss_start       /* x0 := vaddr(__bss_start) */
> >          ldr   x1, =__bss_end         /* x1 := vaddr(__bss_end)   */
> > +        b     2f
> >
> > 1:      str   xzr, [x0], #8
> > -        cmp   x0, x1
> > +2:      cmp   x0, x1
> >          b.lo  1b
>
> I am not really sure if this implementation is better than the previous
> one. The drawback of my implementation is that we have an extra 'cmp'
> instruction. The drawback of this implementation is that we need an
> extra label and there is an un-conditional branch after ldr (readability
> is difficult). May be I am biased. :)
>
> How does this look ?
>
> FUNC_LOCAL(zero_bss)
>          /* Zero BSS only when requested */
>          cbnz  x26, skip_bss
>
>          PRINT("- Zero BSS -\r\n")
>          ldr   x0, =__bss_start       /* x0 := vaddr(__bss_start) */
>          ldr   x1, =__bss_end         /* x1 := vaddr(__bss_end) */
> 1:     cmp   x1, x0
>          beq   skip_bss
>
>          str   xzr, [x0], #8
>          b     1b
>
> skip_bss:
>          ret
> END(zero_bss)
>
> - Ayan
>

Not strong about. Your first version was fine too. I suppose just
personal preferences, being not a hard path if you think first version
was more readable fine with it. Between your 2 versions, I prefer the
first. It has fewer instructions in the loop. Probably I've seen too
much compiler generated code (jumping to the condition at the end is
common there) that for me, it got more readable.

Frediano