[PATCH] firmware: Move memcpy/memset mapping to fw_base.S

[Anup Patel]

+Others

On Thu, Dec 23, 2021 at 10:57 AM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
>
> On 23 Dec 2021, at 05:26, Anup Patel <anup at brainfault.org> wrote:
> >
> > On Wed, Dec 22, 2021 at 6:25 PM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
> >>
> >> On 22 Dec 2021, at 12:44, Anup Patel <anup.patel at wdc.com> wrote:
> >>>
> >>> Some of the external firmwares using OpenSBI as library are facing
> >>> issues with the weak memcpy() and memset() aliases in libsbi.a so
> >>> we move these to fw_base.S. This way mapping of implicit memcpy()
> >>> or memset() calls to sbi_memcpy() or sbi_memset() will only be done
> >>> for OpenSBI firmwares.
> >>> (Refer, https://github.com/riscv-software-src/opensbi/issues/234)
> >>
> >> Consumers that want to override the definitions in libsbi should just
> >> make sure -lsbi/libsbi.a appears after the object or library containing
> >> the strong definition.
> >
> > I agree with you. This is definitely a link-time ordering issue for external
> > firmwares. Ideally, external firmware projects should fix this at their end.
> >
> >>
> >> If you really want to support people that can’t get their link order
> >> right (which then hurts people who *do* and want to just use libsbi’s
> >> memcpy/memset) then these should be strong aliases and/or defined in C;
> >> there’s zero reason for it to be in assembly and, as a downstream that
> >> would need to modify this code if written in assembly, hurts
> >> portability. And if for some reason you still insist on assembly then
> >> you should be using tail here, not j, as j is only supported for jumps
> >> within a file, not across files.
> >
> > I am certainly not happy in moving this to fw_base.S but currently
> > OpenSBI firmwares will only use memcpy/memset symbols implicitly
> > when "-Os" compile option is used so by default this change does
> > not impact users of OpenSBI firmwares.
> >
> > At this point, the number of OpenSBI firmware users is much more
> > than the number of libsbi.a users so eventually we have to be more
> > strict with external firmwares otherwise such things will continue to
> > add restrictions on things we can do in libsbi.a.
> >
> > I want to release OpenSBI v1.0 this week so for the time being I will
> > will replace the use of "j" with "tail" and post a v2. Post OpenSBI v1.0,
> > release, I will try to find a better solution (may be add C sources for
> > firmware and move it there).
>
> Is there not an argument to be made that we should break their broken
> build systems *now* rather than post-1.0 when there might be some kind
> of greater expectation about stability?

Honestly, I am fine dropping this patch as well. I will let others share their
views.

Regards,
Anup

>
> Jess
>
> > Regards,
> > Anup
> >
> >>
> >> Jess
> >>
> >>> Signed-off-by: Anup Patel <anup.patel at wdc.com>
> >>> ---
> >>> firmware/fw_base.S   | 14 ++++++++++++++
> >>> lib/sbi/sbi_string.c |  6 ------
> >>> 2 files changed, 14 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/firmware/fw_base.S b/firmware/fw_base.S
> >>> index 1569e60..6197a7f 100644
> >>> --- a/firmware/fw_base.S
> >>> +++ b/firmware/fw_base.S
> >>> @@ -561,6 +561,20 @@ fw_platform_init:
> >>>      add     a0, a1, zero
> >>>      ret
> >>>
> >>> +     /* Map implicit memcpy() added by compiler to sbi_memcpy() */
> >>> +     .section .text
> >>> +     .align 3
> >>> +     .globl memcpy
> >>> +memcpy:
> >>> +     j       sbi_memcpy
> >>> +
> >>> +     /* Map implicit memset() added by compiler to sbi_memset() */
> >>> +     .section .text
> >>> +     .align 3
> >>> +     .globl memset
> >>> +memset:
> >>> +     j       sbi_memset
> >>> +
> >>> .macro        TRAP_SAVE_AND_SETUP_SP_T0
> >>>      /* Swap TP and MSCRATCH */
> >>>      csrrw   tp, CSR_MSCRATCH, tp
> >>> diff --git a/lib/sbi/sbi_string.c b/lib/sbi/sbi_string.c
> >>> index c67c02e..c87bce9 100644
> >>> --- a/lib/sbi/sbi_string.c
> >>> +++ b/lib/sbi/sbi_string.c
> >>> @@ -122,9 +122,6 @@ void *sbi_memset(void *s, int c, size_t count)
> >>>      return s;
> >>> }
> >>>
> >>> -void *memset(void *s, int c, size_t count) \
> >>> -__attribute__((weak, alias("sbi_memset")));
> >>> -
> >>> void *sbi_memcpy(void *dest, const void *src, size_t count)
> >>> {
> >>>      char *temp1       = dest;
> >>> @@ -138,9 +135,6 @@ void *sbi_memcpy(void *dest, const void *src, size_t count)
> >>>      return dest;
> >>> }
> >>>
> >>> -void *memcpy(void *dest, const void *src, size_t count) \
> >>> -__attribute__((weak, alias("sbi_memcpy")));
> >>> -
> >>> void *sbi_memmove(void *dest, const void *src, size_t count)
> >>> {
> >>>      char *temp1       = (char *)dest;
> >>> --
> >>> 2.25.1
> >>>
> >>>
> >>> --
> >>> opensbi mailing list
> >>> opensbi at lists.infradead.org
> >>> http://lists.infradead.org/mailman/listinfo/opensbi
> >>
>