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

[Mauricio Faria de Oliveira <mfo@igalia.com>]

Thanks for reviewing.

On 2026-05-26 06:51, Juergen Gross wrote:
> 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".

Ack; replaced.

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

Good point; indeed, that is review material. I'll change it for now.

>> (*) 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@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202605140922.q7IlUv7o-lkp@intel.com/
>> Signed-off-by: Mauricio Faria de Oliveira <mfo@igalia.com>
> 
> With (at least) my first remark addressed:
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>

Sent v4 with both remarks above addressed and your new R-b tags; thanks!

> 
> 
> Juergen

-- 
Mauricio