Re: [PATCH 4/7] x86: Add SSSE3 optimized chacha20

[Noah Goldstein <goldstein.w.n@gmail.com>]

On Thu, Apr 14, 2022 at 12:19 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 14/04/2022 14:10, Noah Goldstein wrote:
> > On Thu, Apr 14, 2022 at 12:03 PM Adhemerval Zanella
> > <adhemerval.zanella@linaro.org> wrote:
> >>
> >>
> >>
> >> On 13/04/2022 20:12, Noah Goldstein wrote:
> >>> On Wed, Apr 13, 2022 at 1:27 PM Adhemerval Zanella via Libc-alpha
> >>> <libc-alpha@sourceware.org> wrote:
> >>>>
> >>>> +
> >>>> +       /* eax zeroed by round loop. */
> >>>> +       leave;
> >>>> +       cfi_adjust_cfa_offset(-8)
> >>>> +       cfi_def_cfa_register(%rsp);
> >>>> +       ret;
> >>>> +       int3;
> >>> why int3?
> >>
> >> It was originally added on libgcrypt by 11ade08efbfbc36dbf3571f1026946269950bc40,
> >> as a straight-line speculation hardening.  It is was is emitted by clang 14 and
> >> gcc 12 with -mharden-sls=return.
> >>
> >> I am not sure if we need that kind of hardening, but I would prefer to the first
> >> version be in sync with libgcrypt as much as possible so the future optimizations
> >> would be simpler to keep localized to glibc (if libgcrypt does not want to
> >> backport it).
> >
> > Okay, can keep for now. Any thoughts on changing it to sse2?
> >
>
> No strong feeling, I used the ssse3 one because it is readily available from
> libgcrypt.

I think the only ssse3 is `pshufb` so you can just replace the optimized
rotates with the shift rotates and that will make it sse2 (unless I'm missing
an instruction).

Also can you add the proper .text section here as well (or .sse2 or .ssse3)

>
> >
> >>
> >>>> +END (__chacha20_ssse3_blocks8)
> >>>> diff --git a/sysdeps/x86_64/chacha20_arch.h b/sysdeps/x86_64/chacha20_arch.h
> >>>> new file mode 100644
> >>>> index 0000000000..37a4fdfb1f
> >>>> --- /dev/null
> >>>> +++ b/sysdeps/x86_64/chacha20_arch.h
> >>>> @@ -0,0 +1,42 @@
> >>>> +/* Chacha20 implementation, used on arc4random.
> >>>> +   Copyright (C) 2022 Free Software Foundation, Inc.
> >>>> +   This file is part of the GNU C Library.
> >>>> +
> >>>> +   The GNU C Library is free software; you can redistribute it and/or
> >>>> +   modify it under the terms of the GNU Lesser General Public
> >>>> +   License as published by the Free Software Foundation; either
> >>>> +   version 2.1 of the License, or (at your option) any later version.
> >>>> +
> >>>> +   The GNU C Library is distributed in the hope that it will be useful,
> >>>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >>>> +   Lesser General Public License for more details.
> >>>> +
> >>>> +   You should have received a copy of the GNU Lesser General Public
> >>>> +   License along with the GNU C Library; if not, see
> >>>> +   <http://www.gnu.org/licenses/>.  */
> >>>> +
> >>>> +#include <ldsodefs.h>
> >>>> +#include <cpu-features.h>
> >>>> +#include <sys/param.h>
> >>>> +
> >>>> +unsigned int __chacha20_ssse3_blocks8 (uint32_t *state, uint8_t *dst,
> >>>> +                                      const uint8_t *src, size_t nblks);
> >>>> +
> >>>> +static inline void
> >>>> +chacha20_crypt (struct chacha20_state *state, uint8_t *dst, const uint8_t *src,
> >>>> +               size_t bytes)
> >>>> +{
> >>>> +  if (CPU_FEATURE_USABLE_P (cpu_features, SSSE3) && bytes >= CHACHA20_BLOCK_SIZE * 4)
> >>>
> >>> Can we make this an ifunc?
> >>
> >> I though about it, but if you check on arc4random implementation the
> >> chacha20_crypt is called for the whole internal buf once it is exhausted.
> >> Assuming a 1 cycle per byte (as indicated by bench-slope libgrcypt on
> >> my machine), it will be at least 1k cycles to encrypt each block.  I
> >> am not sure if setting up an internal PLT call to save a couple of cycles
> >> on a internal function will really show anything significant here (assuming
> >> that the PLT call won't add more overhead in fact).
> >>
> >> Besides that the code boilerplate to setup the internal ifunc is also
> >> way more complex.
> >
> > Okay for now as long as open to changing later (not that we will but that
> > this isn't locking us into the decision).
>
> For sure, if iFUNC does help on this case the change should be simple for
> the generic code.  The boilerplate is for the x86_64 bits in facts (to
> setup the iFUNC resolver, Makefile, etc.).