Re: crypto: x86/crct10dif-pcl - cleanup and optimizations

Re: crypto: x86/crct10dif-pcl - cleanup and optimizations

[Sedat Dilek]

Hi,

while digging through a ClangBuiltLinux issue when linking with LLD
linker on x86-64 I checked the settings for...

.rodata.cst16 and .rodata.cst32

...in crypto tree and fell over this change in...

commit "crypto: x86/crct10dif-pcl - cleanup and optimizations":

-.section .rodata.cst16.SHUF_MASK, "aM", @progbits, 16
+.section .rodata.cst32.byteshift_table, "aM", @progbits, 32
.align 16

Is that a typo?
I would have expected...
.rodata.cst32.XXX -> .align 32
or
rodata.cst16.XXX -> .align 16

But I might be wrong as I am no expert for crypto and x86/asm.

Thanks in advance.

Regards,
- Sedat -

[1] https://github.com/ClangBuiltLinux/linux/issues/431
[2] https://bugs.llvm.org/show_bug.cgi?id=42289
[3] https://git.kernel.org/linus/0974037fc55c

Re: crypto: x86/crct10dif-pcl - cleanup and optimizations

[Nick Desaulniers]

On Mon, Jun 17, 2019 at 6:35 AM Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
> Hi,
>
> while digging through a ClangBuiltLinux issue when linking with LLD
> linker on x86-64 I checked the settings for...
>
> .rodata.cst16 and .rodata.cst32
>
> ...in crypto tree and fell over this change in...
>
> commit "crypto: x86/crct10dif-pcl - cleanup and optimizations":
>
> -.section .rodata.cst16.SHUF_MASK, "aM", @progbits, 16
> +.section .rodata.cst32.byteshift_table, "aM", @progbits, 32
> .align 16
>
> Is that a typo?
> I would have expected...
> .rodata.cst32.XXX -> .align 32
> or
> rodata.cst16.XXX -> .align 16
>
> But I might be wrong as I am no expert for crypto and x86/asm.
>
> Thanks in advance.
>
> Regards,
> - Sedat -
>
> [1] https://github.com/ClangBuiltLinux/linux/issues/431
> [2] https://bugs.llvm.org/show_bug.cgi?id=42289

> [3] https://git.kernel.org/linus/0974037fc55c

+ Peter, Fangrui (who have looked at this, and started looking into
this from LLD's perspective)

In fact, looking closer at that diff, the section in question
previously had 32b alignment.  Eric, was that change intentional?  It
seems funny to have a 32b entity size but a 16b alignment.

PDF page 81 / printed page 67 of this doc:
https://web.eecs.umich.edu/~prabal/teaching/resources/eecs373/Assembler.pdf
says:

"The linker may remove duplicates within sections with the
same name, same entity size and same flags. "

So for us, LLD is NOT merging these sections due to differing
alignments, which is producing warnings when loading such kernel
modules that link against these object files.
-- 
Thanks,
~Nick Desaulniers

Re: crypto: x86/crct10dif-pcl - cleanup and optimizations

[Nick Desaulniers]

On Mon, Jun 17, 2019 at 11:06 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
> In fact, looking closer at that diff, the section in question
> previously had 32b alignment.  Eric, was that change intentional?  It
> seems funny to have a 32b entity size but a 16b alignment.

32B & 16B
-- 
Thanks,
~Nick Desaulniers

Re: crypto: x86/crct10dif-pcl - cleanup and optimizations

[Eric Biggers]

On Mon, Jun 17, 2019 at 11:06:21AM -0700, Nick Desaulniers wrote:
> On Mon, Jun 17, 2019 at 6:35 AM Sedat Dilek <sedat.dilek@gmail.com> wrote:
> >
> > Hi,
> >
> > while digging through a ClangBuiltLinux issue when linking with LLD
> > linker on x86-64 I checked the settings for...
> >
> > .rodata.cst16 and .rodata.cst32
> >
> > ...in crypto tree and fell over this change in...
> >
> > commit "crypto: x86/crct10dif-pcl - cleanup and optimizations":
> >
> > -.section .rodata.cst16.SHUF_MASK, "aM", @progbits, 16
> > +.section .rodata.cst32.byteshift_table, "aM", @progbits, 32
> > .align 16
> >
> > Is that a typo?
> > I would have expected...
> > .rodata.cst32.XXX -> .align 32
> > or
> > rodata.cst16.XXX -> .align 16
> >
> > But I might be wrong as I am no expert for crypto and x86/asm.
> >
> > Thanks in advance.
> >
> > Regards,
> > - Sedat -
> >
> > [1] https://github.com/ClangBuiltLinux/linux/issues/431
> > [2] https://bugs.llvm.org/show_bug.cgi?id=42289
> 
> > [3] https://git.kernel.org/linus/0974037fc55c
> 
> + Peter, Fangrui (who have looked at this, and started looking into
> this from LLD's perspective)
> 
> In fact, looking closer at that diff, the section in question
> previously had 32b alignment.  Eric, was that change intentional?  It
> seems funny to have a 32b entity size but a 16b alignment.
> 
> PDF page 81 / printed page 67 of this doc:
> https://web.eecs.umich.edu/~prabal/teaching/resources/eecs373/Assembler.pdf
> says:
> 
> "The linker may remove duplicates within sections with the
> same name, same entity size and same flags. "
> 
> So for us, LLD is NOT merging these sections due to differing
> alignments, which is producing warnings when loading such kernel
> modules that link against these object files.
> -- 
> Thanks,
> ~Nick Desaulniers

It was an intentional change since actually no alignment is required for this.
It's an array of 32 bytes and the code loads 16 bytes starting from some byte
offset in the range 1...16, so it has to use movdqu anyway.

There's no problem with changing it back to 32, but I don't fully understand
what's going on here.  Where is it documented how alignment specifiers interact
with the section merging?  Also, there are already other mergeable sections in
the x86 crypto code with alignment smaller than their entity size, e.g.

	.rodata.cst16.aegis128_const
	.rodata.cst16.aegis128l_const
	.rodata.cst16.aegis256_const
	.rodata.cst16.morus640_const
	.rodata.cst256.K256

Do those need to be changed too?

- Eric

Re: crypto: x86/crct10dif-pcl - cleanup and optimizations

[Sedat Dilek]

[-- Attachment #1: Type: text/plain, Size: 4246 bytes --]

On Mon, Jun 17, 2019 at 8:23 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Mon, Jun 17, 2019 at 11:06:21AM -0700, Nick Desaulniers wrote:
> > On Mon, Jun 17, 2019 at 6:35 AM Sedat Dilek <sedat.dilek@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > while digging through a ClangBuiltLinux issue when linking with LLD
> > > linker on x86-64 I checked the settings for...
> > >
> > > .rodata.cst16 and .rodata.cst32
> > >
> > > ...in crypto tree and fell over this change in...
> > >
> > > commit "crypto: x86/crct10dif-pcl - cleanup and optimizations":
> > >
> > > -.section .rodata.cst16.SHUF_MASK, "aM", @progbits, 16
> > > +.section .rodata.cst32.byteshift_table, "aM", @progbits, 32
> > > .align 16
> > >
> > > Is that a typo?
> > > I would have expected...
> > > .rodata.cst32.XXX -> .align 32
> > > or
> > > rodata.cst16.XXX -> .align 16
> > >
> > > But I might be wrong as I am no expert for crypto and x86/asm.
> > >
> > > Thanks in advance.
> > >
> > > Regards,
> > > - Sedat -
> > >
> > > [1] https://github.com/ClangBuiltLinux/linux/issues/431
> > > [2] https://bugs.llvm.org/show_bug.cgi?id=42289
> >
> > > [3] https://git.kernel.org/linus/0974037fc55c
> >
> > + Peter, Fangrui (who have looked at this, and started looking into
> > this from LLD's perspective)
> >
> > In fact, looking closer at that diff, the section in question
> > previously had 32b alignment.  Eric, was that change intentional?  It
> > seems funny to have a 32b entity size but a 16b alignment.
> >
> > PDF page 81 / printed page 67 of this doc:
> > https://web.eecs.umich.edu/~prabal/teaching/resources/eecs373/Assembler.pdf
> > says:
> >
> > "The linker may remove duplicates within sections with the
> > same name, same entity size and same flags. "
> >
> > So for us, LLD is NOT merging these sections due to differing
> > alignments, which is producing warnings when loading such kernel
> > modules that link against these object files.
> > --
> > Thanks,
> > ~Nick Desaulniers
>
> It was an intentional change since actually no alignment is required for this.
> It's an array of 32 bytes and the code loads 16 bytes starting from some byte
> offset in the range 1...16, so it has to use movdqu anyway.
>
> There's no problem with changing it back to 32, but I don't fully understand
> what's going on here.  Where is it documented how alignment specifiers interact
> with the section merging?  Also, there are already other mergeable sections in
> the x86 crypto code with alignment smaller than their entity size, e.g.
>
>         .rodata.cst16.aegis128_const
>         .rodata.cst16.aegis128l_const
>         .rodata.cst16.aegis256_const
>         .rodata.cst16.morus640_const
>         .rodata.cst256.K256
>
> Do those need to be changed too?
>

Hi Eric, Hi Nick,

I am building Linux v5.1.16 with a new llvm-toolchain including the fix for LLD:

"[ELF] Allow placing SHF_MERGE sections with different alignments into
the same MergeSyntheticSection"

[ Alignment=16 before my patch ]

$ cd arch/x86/crypto/
$ for o in $(ls *.o) ; do echo [ $o ] ; readelf -WS $o | grep
rodata\.cst32 ; done

[ crct10dif-pcl-asm_64.o ]
  [ 9] .rodata.cst32.byteshift_table PROGBITS        0000000000000000
0004e0 000020 20  AM  0   0 16

[ crct10dif-pclmul.o ]
  [ 9] .rodata.cst32.byteshift_table PROGBITS        0000000000000000
000b40 000020 20  AM  0   0 16

[ Alignment=32 after my patch ]

[ crct10dif-pcl-asm_64.o ]
  [ 9] .rodata.cst32.byteshift_table PROGBITS        0000000000000000
0004e0 000020 20  AM  0   0 32

[ crct10dif-pclmul.o ]
  [ 9] .rodata.cst32.byteshift_table PROGBITS        0000000000000000
000b40 000020 20  AM  0   0 32

I am still building the Linux-kernel but first checks in [3] looks good.

I can send out a separate patch if you like for the issue I have reported.

I can not say much to ...

>         .rodata.cst16.aegis128_const
>         .rodata.cst16.aegis128l_const
>         .rodata.cst16.aegis256_const
>         .rodata.cst16.morus640_const
>         .rodata.cst256.K256

... as I am not a Linker or Linux/x86/crypto specialist.

Thanks.

Regards,
- Sedat -

[1] https://bugs.llvm.org/show_bug.cgi?id=42289#c7
[2] https://reviews.llvm.org/D63432
[3] https://github.com/ClangBuiltLinux/linux/issues/431#issuecomment-508132852

[-- Attachment #2: 0001-crypto-x86-crct10dif-pcl-Fix-alignment-size-in-.roda.patch --]
[-- Type: text/x-patch, Size: 974 bytes --]

From 3e6b8ff93b100b8c140a0f693b3f2f77dd7e740b Mon Sep 17 00:00:00 2001
From: Sedat Dilek <sedat.dilek@credativ.de>
Date: Tue, 18 Jun 2019 09:32:38 +0200
Subject: [PATCH] crypto: x86/crct10dif-pcl: Fix alignment size in
 .rodata.cst32 section

---
 arch/x86/crypto/crct10dif-pcl-asm_64.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/crypto/crct10dif-pcl-asm_64.S b/arch/x86/crypto/crct10dif-pcl-asm_64.S
index 3d873e67749d..480fcd07e973 100644
--- a/arch/x86/crypto/crct10dif-pcl-asm_64.S
+++ b/arch/x86/crypto/crct10dif-pcl-asm_64.S
@@ -322,7 +322,7 @@ ENDPROC(crc_t10dif_pcl)
 	.octa	0x000102030405060708090A0B0C0D0E0F
 
 .section	.rodata.cst32.byteshift_table, "aM", @progbits, 32
-.align 16
+.align 32
 # For 1 <= len <= 15, the 16-byte vector beginning at &byteshift_table[16 - len]
 # is the index vector to shift left by 'len' bytes, and is also {0x80, ...,
 # 0x80} XOR the index vector to shift right by '16 - len' bytes.
-- 
2.20.1

Re: crypto: x86/crct10dif-pcl - cleanup and optimizations

[Eric Biggers]

Hi Sedat,

On Wed, Jul 03, 2019 at 05:16:40PM +0200, Sedat Dilek wrote:
> 
> Hi Eric, Hi Nick,
> 
> I am building Linux v5.1.16 with a new llvm-toolchain including the fix for LLD:
> 
> "[ELF] Allow placing SHF_MERGE sections with different alignments into
> the same MergeSyntheticSection"
> 
> [ Alignment=16 before my patch ]
> 
> $ cd arch/x86/crypto/
> $ for o in $(ls *.o) ; do echo [ $o ] ; readelf -WS $o | grep
> rodata\.cst32 ; done
> 
> [ crct10dif-pcl-asm_64.o ]
>   [ 9] .rodata.cst32.byteshift_table PROGBITS        0000000000000000
> 0004e0 000020 20  AM  0   0 16
> 
> [ crct10dif-pclmul.o ]
>   [ 9] .rodata.cst32.byteshift_table PROGBITS        0000000000000000
> 000b40 000020 20  AM  0   0 16
> 
> [ Alignment=32 after my patch ]
> 
> [ crct10dif-pcl-asm_64.o ]
>   [ 9] .rodata.cst32.byteshift_table PROGBITS        0000000000000000
> 0004e0 000020 20  AM  0   0 32
> 
> [ crct10dif-pclmul.o ]
>   [ 9] .rodata.cst32.byteshift_table PROGBITS        0000000000000000
> 000b40 000020 20  AM  0   0 32
> 
> I am still building the Linux-kernel but first checks in [3] looks good.
> 
> I can send out a separate patch if you like for the issue I have reported.

Sorry, I am still confused.  Are you saying that something still needs to be
fixed in the kernel code, and if so, why?  To reiterate, the byteshift_table
doesn't actually *need* any particular alignment.  Would it avoid the confusion
if I changed it to no alignment?  Or is there some section merging related
reason it actually needs to be 32?

> 
> I can not say much to ...
> 
> >         .rodata.cst16.aegis128_const
> >         .rodata.cst16.aegis128l_const
> >         .rodata.cst16.aegis256_const
> >         .rodata.cst16.morus640_const
> >         .rodata.cst256.K256
> 
> ... as I am not a Linker or Linux/x86/crypto specialist.

Well those all seem to be the same issue; the needed alignment isn't the same as
the entity size.  So if the crct10dif one needs to be fixed, these need to be
too.  Am I missing something?

- Eric

Re: crypto: x86/crct10dif-pcl - cleanup and optimizations

[Nick Desaulniers]

On Wed, Jul 3, 2019 at 9:15 AM Eric Biggers <ebiggers@kernel.org> wrote:
> Sorry, I am still confused.  Are you saying that something still needs to be
> fixed in the kernel code, and if so, why?  To reiterate, the byteshift_table
> doesn't actually *need* any particular alignment.  Would it avoid the confusion
> if I changed it to no alignment?  Or is there some section merging related
> reason it actually needs to be 32?

Looks like the section merging of similarly named sections of
differing alignment in LLD just got reverted:
https://bugs.llvm.org/show_bug.cgi?id=42289#c8
I wasn't able to find any documentation that said alignment must match
entity size, but if there's not a functional reason for them to differ
then it seems like LLD need not even support such a particularly
non-common case.

-- 
Thanks,
~Nick Desaulniers

Re: crypto: x86/crct10dif-pcl - cleanup and optimizations

[Sedat Dilek]

On Wed, Jul 3, 2019 at 8:52 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Wed, Jul 3, 2019 at 9:15 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > Sorry, I am still confused.  Are you saying that something still needs to be
> > fixed in the kernel code, and if so, why?  To reiterate, the byteshift_table
> > doesn't actually *need* any particular alignment.  Would it avoid the confusion
> > if I changed it to no alignment?  Or is there some section merging related
> > reason it actually needs to be 32?
>
> Looks like the section merging of similarly named sections of
> differing alignment in LLD just got reverted:
> https://bugs.llvm.org/show_bug.cgi?id=42289#c8
> I wasn't able to find any documentation that said alignment must match
> entity size, but if there's not a functional reason for them to differ
> then it seems like LLD need not even support such a particularly
> non-common case.
>

My primary goal was to get rid of these sysfs warnings when building
with clang-9 and linking with lld-9 (details see [0]).
"clang-9" and "lld-9" are snapshot versions (details see [1]).

- Sedat -

[0] https://github.com/ClangBuiltLinux/linux/issues/431
[1] https://github.com/ClangBuiltLinux/linux/issues/431#issuecomment-508354865

Re: crypto: x86/crct10dif-pcl - cleanup and optimizations

[Nick Desaulniers]

On Wed, Jul 3, 2019 at 11:52 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Wed, Jul 3, 2019 at 9:15 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > Sorry, I am still confused.  Are you saying that something still needs to be
> > fixed in the kernel code, and if so, why?  To reiterate, the byteshift_table
> > doesn't actually *need* any particular alignment.  Would it avoid the confusion
> > if I changed it to no alignment?  Or is there some section merging related
> > reason it actually needs to be 32?
>
> Looks like the section merging of similarly named sections of
> differing alignment in LLD just got reverted:
> https://bugs.llvm.org/show_bug.cgi?id=42289#c8

And re-fixed in:
https://reviews.llvm.org/rL365139
Seems like special care was needed for SHF_STRINGS.
-- 
Thanks,
~Nick Desaulniers