Re: [PATCH v3 1/4] x86/asm: move inline string functions to <asm/string_inline.h>

[Juergen Gross]

On 20.05.26 23:12, Mauricio Faria de Oliveira wrote:
> In next patch, inline string functions are included from 'boot/string.c'.

Please don't use "In next patch". You can't be sure the patches of a
series are going to be committed all together.

A better alternative wording would be "In a future patch".

> Using the header <asm/string.h> is problematic for a couple of reasons (*)
> (i.e., build errors), which can be addressed, but introduce unnecessary
> complexity and regression risk (beyond these _found_ couple of reasons).
>
> Using a new header <asm/string_inline.h> is simpler and transparent to
> existing users of <asm/string.h>, with less changes to 'boot/string.c'
> and its users (eg 'boot/compressed/string.c' and 'purgatory/purgatory.ro'),
> which minimize regression risk.
>
> No functional change intended.

I'd rather put the footnote below after the "---" line, as it is more
interesting for the reviewer than the consumer of "git log" or "git show".

Note that others might disagree, though.

> (*) Reasons not to include <asm/string.h> in 'boot/string.c':
>
> 1) 'boot/string.c' is built for 16-bit/real mode thus some type and word
> size errors happen when <asm/string.h> include, e.g., <asm/string_64.h>.
>
> This can be addressed with '#ifndef _SETUP' (defined by 'boot/Makefile').
>
> 2) 'boot/string.c' is included by 'boot/compressed/string.c' which is
> the source of 'purgatory/string.o', linked by 'purgatory/purgatory.ro'
> (CONFIG_KEXEC_FILE).
>
> In 64BIT, <asm/string.h> includes <asm/string_64.h>, which references
> __memset() and __memmove() with KCFI_REFERENCE(), ie, __ADDRESSABLE();
> however, 'purgatory/purgatory.ro' is not linked with implementations.
>
> So, CONFIG_KEXEC_FILE and CONFIG_CFI without CONFIG_KASAN hit errors:
>
>    >> ld.lld: error: undefined symbol: __memset
>       >>> referenced by string.c
>       >>>               arch/x86/purgatory/purgatory.ro:\
>                         (__UNIQUE_ID_addressable___memset_0)
>    --
>    >> ld.lld: error: undefined symbol: __memmove
>       >>> referenced by string.c
>       >>>               arch/x86/purgatory/purgatory.ro:\
>                         (__UNIQUE_ID_addressable___memmove_1)
>
> (Note: this is not hit with CONFIG_KASAN because 'boot/compressed/string.c'
> adds aliases __memset()/__memmove() to memset()/memmove() in that case.)
>
> This can be addressed with 'CFLAGS_string.o := -D__DISABLE_EXPORTS' so to
> disable KCFI_REFERENCE() in 'purgatory/Makefile' (it removes CC_FLAGS_CFI
> anyway).
>
> ...
>
> However, since a change in this series would need more changes to address
> errors it causes, it is reasonable to change the series not to cause them,
> by using a separate header with _just_ inline string functions.
>
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Closes: https://lore.kernel.org/oe-kbuild-all/202605140922.q7IlUv7o-lkp@xxxxxxxxx/
> Signed-off-by: Mauricio Faria de Oliveira <mfo@xxxxxxxxxx>

With (at least) my first remark addressed:

Reviewed-by: Juergen Gross <jgross@xxxxxxxx>


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature