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

[Atish Patra]

On Wed, Dec 22, 2021 at 9:32 PM Anup Patel <anup at brainfault.org> wrote:
>
> +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?
>

Personally, I think it should be otherwise. As OpenSBI v1.0 is a major release,
we should do our best not to break any user (even if they are not
using libsbi correctly).
The impact on OpenSBI firmware users (only those who use "-Os")is very minimal.
We can always come up with a better fix for this after v1.0.

> 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
> > >>
> >



-- 
Regards,
Atish