Re: [PATCH] arm64/sve: fix genksyms generation

[Alex BennÃe]

Arnd Bergmann <arnd@xxxxxxxx> writes:

> On Mon, Jun 17, 2019 at 1:26 PM Will Deacon <will.deacon@xxxxxxx> wrote:
>>
>> Hi Arnd,
>>
>> On Mon, Jun 17, 2019 at 12:42:11PM +0200, Arnd Bergmann wrote:
>> > genksyms does not understand __uint128_t, so we get a build failure
>> > in the fpsimd module when it cannot export a symbol right:
>>
>> The fpsimd code is builtin, so which module is actually failing? My
>> allmodconfig build succeeds, so I must be missing something.
>
> It happened for me on randconfig builds, you can find one such configuration
> at https://pastebin.com/cU8iQ4ta now. I was building this with clang
> rather than gcc, which may affect the issue, but I assumed not.
>
>> > WARNING: EXPORT symbol "kernel_neon_begin" [vmlinux] version generation failed, symbol will not be versioned.
>> > /home/arnd/cross/x86_64/gcc-8.1.0-nolibc/aarch64-linux/bin/aarch64-linux-ld: arch/arm64/kernel/fpsimd.o: relocation R_AARCH64_ABS32 against `__crc_kernel_neon_begin' can not be used when making a shared object
>> > arch/arm64/kernel/fpsimd.o:(.data+0x0): dangerous relocation: unsupported relocation
>> > arch/arm64/kernel/fpsimd.o:(".discard.addressable"+0x0): dangerous relocation: unsupported relocation
>> > arch/arm64/kernel/fpsimd.o:(".discard.addressable"+0x8): dangerous relocation: unsupported relocation
>> >
>> > We could teach genksyms about the type, but it's easier to just
>> > work around it by defining that type locally in a way that genksyms
>> > understands.
>> >
>> > Fixes: 41040cf7c5f0 ("arm64/sve: Fix missing SVE/FPSIMD endianness conversions")
>>
>> I can't see which part of that patch causes the problem, so I'm a bit wary
>> of the fix. We've been using __uint128_t for a while now, and I see there's
>> one in the x86 kvm code as well, so it would be nice to understand what's
>> happening here so that we can avoid running into it in future as well.
>
> The problem is only in files that export a symbol. This is also the
> case in arch/x86/kernel/kvm.c, but it may be lucky because the
> type only appears /after/ the last export in that file.
>
>> > Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
>> > ---
>> >  arch/arm64/kernel/fpsimd.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
>> > index 07f238ef47ae..2aba07cccf50 100644
>> > --- a/arch/arm64/kernel/fpsimd.c
>> > +++ b/arch/arm64/kernel/fpsimd.c
>> > @@ -400,6 +400,9 @@ static int __init sve_sysctl_init(void) { return 0; }
>> >  #define ZREG(sve_state, vq, n) ((char *)(sve_state) +                \
>> >       (SVE_SIG_ZREG_OFFSET(vq, n) - SVE_SIG_REGS_OFFSET))
>> >
>> > +#ifdef __GENKSYMS__
>> > +typedef __u64 __uint128_t[2];
>> > +#endif
>>
>> I suspect I need to figure out what genksyms is doing, but I'm nervous
>> about exposing this as an array type without understanding whether or
>> not that has consequences for its operation.
>
> The entire point is genksyms is to ensure that types of exported symbols
> are compatible. To do this, it has a limited parser for C source code that
> understands the basic types (char, int, long, _Bool, etc) and how to
> aggregate them into structs and function arguments. This process has
> always been fragile, and it clearly breaks when it fails to understand a
> particular type.

Shouldn't the solution for this be to fix genksyms to be less fragile
and more understanding? The code base doesn't seem to be full of these
sorts of ifdef workarounds.

>
>           Arnd


--
Alex BennÃe