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

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3139 bytes --]

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


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]