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

[Jessica Clarke]

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?

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