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

[Ard Biesheuvel]

On Mon, 17 Jun 2019 at 18:13, Will Deacon <will.deacon@xxxxxxx> wrote:
>
> On Mon, Jun 17, 2019 at 02:21:46PM +0200, Arnd Bergmann wrote:
> > On Mon, Jun 17, 2019 at 1:26 PM Will Deacon <will.deacon@xxxxxxx> wrote:
> > > 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.
>
> Hmm, I've failed to reproduce the issue with that config and either GCC
> (7.1.1 and 8.3.0) or Clang (a flavour of 9.0.0 from a few months ago).
>
> > > > 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.
>
> Ok, but the patch that appears to cause this problem doesn't change the
> type of anything we're exporting. The symbol in your log is
> "kernel_neon_begin" which is:
>
>         void kernel_neon_begin(void);
>
> so I'm still fairly confused about the problem. In fact, even if I create
> a silly:
>
>         void will_kernel_neon_begin(__uint128_t);
>
> function, then somehow I see it being processed:
>
>         __crc_will_kernel_neon_begin = 0x5401d250;
>
> Is there some way that your passing '-w' to genksyms?
>

The problem is not about the types we're *exporting*. Genksyms just
gives up halfway through the file, as soon as it encounters something
it doesn't like, and any symbol that hasn't been encountered yet at
that point will not have a crc generated for it.