Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
Hi Ard, On Sat, Sep 29, 2018 at 8:16 AM Ard Biesheuvel wrote: > > You mean to say that since these nobs are def_bool y and are > > essentially "depends on ARM", then I should just straight up use > > CONFIG_ARM? I had thought about this, but figured this would make it > > easier to later make these optional or have other options block them > > need be, or even if the dependencies and requirements for having them > > changes (for example, with UML on x86). I think doing it this way > > gives us some flexibility later on. So if that's a compelling enough > > reason, I'd like to keep those. > > Sure. But probably better to be consistent then, and stop using > CONFIG_ARM directly in your code. Ack. > > The reason it was added was indeed because of: > > https://lists.01.org/pipermail/kbuild-all/2018-September/053114.html > > -- exactly what you suspected, ARCH_RPC. Have a better suggestion than > > !CPU_32v3? > > Yes, you could just add > > asflags-$(CONFIG_CPU_32v3) += -march=armv4 > > with a comment stating that we don't actually support ARMv3 but only > use it as a code generation target for reasons unrelated to the ISA Alright, I'll do exactly that. Though, if the rationale for this has to do only with codegen -- with what the C compiler does -- then shouldn't this be set globally for CONFIG_CPU_32v3? I couldn't find any macros that test against __LINUX_ARM_ARCH__ being 3 in the assembly, so this shouldn't be a problem I don't think. Maybe I'll send a patch. Jason
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
On 29 September 2018 at 04:20, Jason A. Donenfeld wrote: > Hi Ard, > > On Fri, Sep 28, 2018 at 6:02 PM Ard Biesheuvel > wrote: >> Please put comments like this below the --- > > git-notes is nice for this indeed. > >> Are these CONFIG_ symbols defined anywhere at this point? > > Yes, they're introduced in the first zinc commit. There's no git-blame > on git.kernel.org, presumably because it's expensive to compute, but > there is on my personal instance, so this might help: > https://git.zx2c4.com/linux-dev/blame/lib/zinc/Kconfig?h=jd/wireguard > >> In any case, I don't think these is a reason for these, at least not >> on ARM/arm64. The 64-bitness is implied in both cases > > You mean to say that since these nobs are def_bool y and are > essentially "depends on ARM", then I should just straight up use > CONFIG_ARM? I had thought about this, but figured this would make it > easier to later make these optional or have other options block them > need be, or even if the dependencies and requirements for having them > changes (for example, with UML on x86). I think doing it this way > gives us some flexibility later on. So if that's a compelling enough > reason, I'd like to keep those. > Sure. But probably better to be consistent then, and stop using CONFIG_ARM directly in your code. >> and the >> dependency on !CPU_32v3 you introduce (looking at the version of >> Kconfig at the end of the series) seems spurious to me. Was that added >> because of some kbuild robot report? (we don't support ARMv3 in the >> kernel but ARCH_RPC is built in v3 mode because of historical reasons >> while the actual core is a v4) > > I added the !CPU_32v3 in my development tree after posting v6, so good > to hear you're just looking straight at the updated tree. If you see > things jump out in there prior to me posting v7, don't hesitate to let > me know. > > The reason it was added was indeed because of: > https://lists.01.org/pipermail/kbuild-all/2018-September/053114.html > -- exactly what you suspected, ARCH_RPC. Have a better suggestion than > !CPU_32v3? Yes, you could just add asflags-$(CONFIG_CPU_32v3) += -march=armv4 with a comment stating that we don't actually support ARMv3 but only use it as a code generation target for reasons unrelated to the ISA > It seems to me like so long as the kernel has CPU_32v3 as a > thing in any form, I should mark Zinc as not supporting it, since > we'll certainly be at least v4 and up. (Do you guys have any old Acorn > ARM610 boxes sitting around for old time's sake at LinaroHQ? ;-) > AFAIK we only support the StrongARM flavor of RiscPC which is ARMv4. But yes, some people do care deeply about these antiquated platforms, including Russell, and there is no particular to leave this one behind. >> > +#endif >> > + >> >> No need to make asmlinkage declarations conditional > > Yep, addressed in the IS_ENABLED cleanup. > >> >> if (IS_ENABLED()) > > Sorted. > >> >> """ >> if (!IS_ENABLED(CONFIG_ARM)) >>return false; >> >> hchacha20_arm(x, derived_key); >> return true; >> """ >> >> and drop the #ifdefs > > Also sorted. > > Regards, > Jason
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
Hi Ard, On Fri, Sep 28, 2018 at 6:02 PM Ard Biesheuvel wrote: > Please put comments like this below the --- git-notes is nice for this indeed. > Are these CONFIG_ symbols defined anywhere at this point? Yes, they're introduced in the first zinc commit. There's no git-blame on git.kernel.org, presumably because it's expensive to compute, but there is on my personal instance, so this might help: https://git.zx2c4.com/linux-dev/blame/lib/zinc/Kconfig?h=jd/wireguard > In any case, I don't think these is a reason for these, at least not > on ARM/arm64. The 64-bitness is implied in both cases You mean to say that since these nobs are def_bool y and are essentially "depends on ARM", then I should just straight up use CONFIG_ARM? I had thought about this, but figured this would make it easier to later make these optional or have other options block them need be, or even if the dependencies and requirements for having them changes (for example, with UML on x86). I think doing it this way gives us some flexibility later on. So if that's a compelling enough reason, I'd like to keep those. > and the > dependency on !CPU_32v3 you introduce (looking at the version of > Kconfig at the end of the series) seems spurious to me. Was that added > because of some kbuild robot report? (we don't support ARMv3 in the > kernel but ARCH_RPC is built in v3 mode because of historical reasons > while the actual core is a v4) I added the !CPU_32v3 in my development tree after posting v6, so good to hear you're just looking straight at the updated tree. If you see things jump out in there prior to me posting v7, don't hesitate to let me know. The reason it was added was indeed because of: https://lists.01.org/pipermail/kbuild-all/2018-September/053114.html -- exactly what you suspected, ARCH_RPC. Have a better suggestion than !CPU_32v3? It seems to me like so long as the kernel has CPU_32v3 as a thing in any form, I should mark Zinc as not supporting it, since we'll certainly be at least v4 and up. (Do you guys have any old Acorn ARM610 boxes sitting around for old time's sake at LinaroHQ? ;-) > > +#endif > > + > > No need to make asmlinkage declarations conditional Yep, addressed in the IS_ENABLED cleanup. > > if (IS_ENABLED()) Sorted. > > """ > if (!IS_ENABLED(CONFIG_ARM)) >return false; > > hchacha20_arm(x, derived_key); > return true; > """ > > and drop the #ifdefs Also sorted. Regards, Jason
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
On 25 September 2018 at 16:56, Jason A. Donenfeld wrote:
> These wire Andy Polyakov's implementations up to the kernel for ARMv7,8
> NEON, and introduce Eric Biggers' ultra-fast scalar implementation for
> CPUs without NEON or for CPUs with slow NEON (Cortex-A5,7).
>
> This commit does the following:
> - Adds the glue code for the assembly implementations.
> - Renames the ARMv8 code into place, since it can at this point be
> used wholesale.
> - Merges Andy Polyakov's ARMv7 NEON code with Eric Biggers' <=ARMv7
> scalar code.
>
> Commit note: Eric Biggers' scalar code is brand new, and quite possibly
> prematurely added to this commit, and so it may require a bit of revision.
>
Please put comments like this below the ---
> This commit delivers approximately the same or much better performance than
> the existing crypto API's code and has been measured to do as such on:
>
> - ARM1176JZF-S [ARMv6]
> - Cortex-A7[ARMv7]
> - Cortex-A8[ARMv7]
> - Cortex-A9[ARMv7]
> - Cortex-A17 [ARMv7]
> - Cortex-A53 [ARMv8]
> - Cortex-A55 [ARMv8]
> - Cortex-A73 [ARMv8]
> - Cortex-A75 [ARMv8]
>
> Interestingly, Andy Polyakov's scalar code is slower than Eric Biggers',
> but is also significantly shorter. This has the advantage that it does
> not evict other code from L1 cache -- particularly on ARM11 chips -- and
> so in certain circumstances it can actually be faster. However, it wasn't
> found that this had an affect on any code existing in the kernel today.
>
> Signed-off-by: Jason A. Donenfeld
> Co-authored-by: Eric Biggers
> Cc: Samuel Neves
> Cc: Andy Lutomirski
> Cc: Greg KH
> Cc: Jean-Philippe Aumasson
> Cc: Russell King
> Cc: linux-arm-ker...@lists.infradead.org
> ---
> lib/zinc/Makefile | 2 +
> lib/zinc/chacha20/chacha20-arm-glue.h | 88 +++
> ...acha20-arm-cryptogams.S => chacha20-arm.S} | 502 --
> ...20-arm64-cryptogams.S => chacha20-arm64.S} | 0
> lib/zinc/chacha20/chacha20.c | 2 +
> 5 files changed, 556 insertions(+), 38 deletions(-)
> create mode 100644 lib/zinc/chacha20/chacha20-arm-glue.h
> rename lib/zinc/chacha20/{chacha20-arm-cryptogams.S => chacha20-arm.S} (71%)
> rename lib/zinc/chacha20/{chacha20-arm64-cryptogams.S => chacha20-arm64.S}
> (100%)
>
> diff --git a/lib/zinc/Makefile b/lib/zinc/Makefile
> index 223a0816c918..e47f64e12bbd 100644
> --- a/lib/zinc/Makefile
> +++ b/lib/zinc/Makefile
> @@ -4,4 +4,6 @@ ccflags-$(CONFIG_ZINC_DEBUG) += -DDEBUG
>
> zinc_chacha20-y := chacha20/chacha20.o
> zinc_chacha20-$(CONFIG_ZINC_ARCH_X86_64) += chacha20/chacha20-x86_64.o
> +zinc_chacha20-$(CONFIG_ZINC_ARCH_ARM) += chacha20/chacha20-arm.o
> +zinc_chacha20-$(CONFIG_ZINC_ARCH_ARM64) += chacha20/chacha20-arm64.o
Are these CONFIG_ symbols defined anywhere at this point?
In any case, I don't think these is a reason for these, at least not
on ARM/arm64. The 64-bitness is implied in both cases, and the
dependency on !CPU_32v3 you introduce (looking at the version of
Kconfig at the end of the series) seems spurious to me. Was that added
because of some kbuild robot report? (we don't support ARMv3 in the
kernel but ARCH_RPC is built in v3 mode because of historical reasons
while the actual core is a v4)
> obj-$(CONFIG_ZINC_CHACHA20) += zinc_chacha20.o
> diff --git a/lib/zinc/chacha20/chacha20-arm-glue.h
> b/lib/zinc/chacha20/chacha20-arm-glue.h
> new file mode 100644
> index ..86cce851ed02
> --- /dev/null
> +++ b/lib/zinc/chacha20/chacha20-arm-glue.h
> @@ -0,0 +1,88 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/*
> + * Copyright (C) 2015-2018 Jason A. Donenfeld . All Rights
> Reserved.
> + */
> +
> +#include
> +#include
> +#if defined(CONFIG_ARM)
> +#include
> +#include
> +#endif
> +
> +asmlinkage void chacha20_arm(u8 *out, const u8 *in, const size_t len,
> +const u32 key[8], const u32 counter[4]);
> +#if defined(CONFIG_ARM)
> +asmlinkage void hchacha20_arm(const u32 state[16], u32 out[8]);
> +#endif
> +#if defined(CONFIG_KERNEL_MODE_NEON)
> +asmlinkage void chacha20_neon(u8 *out, const u8 *in, const size_t len,
> + const u32 key[8], const u32 counter[4]);
> +#endif
> +
No need to make asmlinkage declarations conditional
> +static bool chacha20_use_neon __ro_after_init;
> +
> +static void __init chacha20_fpu_init(void)
> +{
> +#if defined(CONFIG_ARM64)
> + chacha20_use_neon = elf_hwcap & HWCAP_ASIMD;
> +#elif defined(CONFIG_ARM)
> + switch (read_cpuid_part()) {
> + case ARM_CPU_PART_CORTEX_A7:
> + case ARM_CPU_PART_CORTEX_A5:
> + /* The Cortex-A7 and Cortex-A5 do not perform well with the
> NEON
> +* implementation but do incredibly with the scalar one and
> use
> +* less power.
> +*/
> + break;
> + default:
> + chacha20_use_neon = elf_hwcap & HWCAP_NEON;
> + }
> +
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
On Thu, Sep 27, 2018 at 6:27 PM Andy Lutomirski wrote: > I would add another consideration: if you can get better latency with > negligible overhead (0.1%? 0.05%), then that might make sense too. For > example, it seems plausible that checking need_resched() every few blocks > adds basically no overhead, and the SIMD helpers could do this themselves or > perhaps only ever do a block at a time. > > need_resched() costs a cacheline access, but it’s usually a hot cacheline, > and the actual check is just whether a certain bit in memory is set. Yes you're right, I do plan to check quite often, rather than seldom, for this reason. I've been toying with the idea of instead processing 65k (maximum size of a UDP packet) at a time before checking need_resched(), but armed with the 20µs figure, this isn't remotely possible on most hardware. So I'll stick with the original conservative plan of checking very often, and not making things different from the aspects worked out by the present crypto API in this regard.
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
> On Sep 27, 2018, at 8:19 AM, Jason A. Donenfeld wrote:
>
> Hey again Thomas,
>
>> On Thu, Sep 27, 2018 at 3:26 PM Jason A. Donenfeld wrote:
>>
>> Hi Thomas,
>>
>> I'm trying to optimize this for crypto performance while still taking
>> into account preemption concerns. I'm having a bit of trouble figuring
>> out a way to determine numerically what the upper bounds for this
>> stuff looks like. I'm sure I could pick a pretty sane number that's
>> arguably okay -- and way under the limit -- but I still am interested
>> in determining what that limit actually is. I was hoping there'd be a
>> debugging option called, "warn if preemption is disabled for too
>> long", or something, but I couldn't find anything like that. I'm also
>> not quite sure what the latency limits are, to just compute this with
>> a formula. Essentially what I'm trying to determine is:
>>
>> preempt_disable();
>> asm volatile(".fill N, 1, 0x90;");
>> preempt_enable();
>>
>> What is the maximum value of N for which the above is okay? What
>> technique would you generally use in measuring this?
>>
>> Thanks,
>> Jason
>
> From talking to Peter (now CC'd) on IRC, it sounds like what you're
> mostly interested in is clocktime latency on reasonable hardware, with
> a goal of around ~20µs as a maximum upper bound? I don't expect to get
> anywhere near this value at all, but if you can confirm that's a
> decent ballpark, it would make for some interesting calculations.
>
>
I would add another consideration: if you can get better latency with
negligible overhead (0.1%? 0.05%), then that might make sense too. For example,
it seems plausible that checking need_resched() every few blocks adds basically
no overhead, and the SIMD helpers could do this themselves or perhaps only ever
do a block at a time.
need_resched() costs a cacheline access, but it’s usually a hot cacheline, and
the actual check is just whether a certain bit in memory is set.
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
Hey again Thomas,
On Thu, Sep 27, 2018 at 3:26 PM Jason A. Donenfeld wrote:
>
> Hi Thomas,
>
> I'm trying to optimize this for crypto performance while still taking
> into account preemption concerns. I'm having a bit of trouble figuring
> out a way to determine numerically what the upper bounds for this
> stuff looks like. I'm sure I could pick a pretty sane number that's
> arguably okay -- and way under the limit -- but I still am interested
> in determining what that limit actually is. I was hoping there'd be a
> debugging option called, "warn if preemption is disabled for too
> long", or something, but I couldn't find anything like that. I'm also
> not quite sure what the latency limits are, to just compute this with
> a formula. Essentially what I'm trying to determine is:
>
> preempt_disable();
> asm volatile(".fill N, 1, 0x90;");
> preempt_enable();
>
> What is the maximum value of N for which the above is okay? What
> technique would you generally use in measuring this?
>
> Thanks,
> Jason
>From talking to Peter (now CC'd) on IRC, it sounds like what you're
mostly interested in is clocktime latency on reasonable hardware, with
a goal of around ~20µs as a maximum upper bound? I don't expect to get
anywhere near this value at all, but if you can confirm that's a
decent ballpark, it would make for some interesting calculations.
Regards,
Jason
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
Hi Thomas,
I'm trying to optimize this for crypto performance while still taking
into account preemption concerns. I'm having a bit of trouble figuring
out a way to determine numerically what the upper bounds for this
stuff looks like. I'm sure I could pick a pretty sane number that's
arguably okay -- and way under the limit -- but I still am interested
in determining what that limit actually is. I was hoping there'd be a
debugging option called, "warn if preemption is disabled for too
long", or something, but I couldn't find anything like that. I'm also
not quite sure what the latency limits are, to just compute this with
a formula. Essentially what I'm trying to determine is:
preempt_disable();
asm volatile(".fill N, 1, 0x90;");
preempt_enable();
What is the maximum value of N for which the above is okay? What
technique would you generally use in measuring this?
Thanks,
Jason
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
On Wed, Sep 26, 2018 at 5:52 PM Ard Biesheuvel
wrote:
>
> On Wed, 26 Sep 2018 at 17:50, Jason A. Donenfeld wrote:
> >
> > On Wed, Sep 26, 2018 at 5:45 PM Jason A. Donenfeld wrote:
> > > So what you have in mind is something like calling simd_relax() every
> > > 4096 bytes or so?
> >
> > That was actually pretty easy, putting together both of your suggestions:
> >
> > static inline bool chacha20_arch(struct chacha20_ctx *state, u8 *dst,
> > u8 *src, size_t len,
> > simd_context_t *simd_context)
> > {
> > while (len > PAGE_SIZE) {
> > chacha20_arch(state, dst, src, PAGE_SIZE, simd_context);
> > len -= PAGE_SIZE;
> > src += PAGE_SIZE;
> > dst += PAGE_SIZE;
> > simd_relax(simd_context);
> > }
> > if (IS_ENABLED(CONFIG_KERNEL_MODE_NEON) && chacha20_use_neon &&
> > len >= CHACHA20_BLOCK_SIZE * 3 && simd_use(simd_context))
> > chacha20_neon(dst, src, len, state->key, state->counter);
> > else
> > chacha20_arm(dst, src, len, state->key, state->counter);
> >
> > state->counter[0] += (len + 63) / 64;
> > return true;
> > }
>
> Nice one :-)
>
> This works for me (but perhaps add a comment as well)
As elegant as my quick recursive solution was, gcc produced kind of
bad code from it, as you might expect. So I've implemented this using
a boring old loop that works the way it's supposed to. This is marked
for v7.
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
On Wed, Sep 26, 2018 at 7:37 PM Eric Biggers wrote: > Can you please stop accusing Ard of "filibustering" your patchset? Spending > too > long in non-preemptible code is a real problem even on non-RT systems. > syzkaller has been reporting bugs where the kernel spins too long without any > preemption points, both in crypto-related code and elsewhere in the kernel. > So > we've had to add explicit preemption points to address those, as otherwise > users > can lock up all CPUs for tens of seconds. The issue being discussed here is > basically the same except here preemption is being explicitly disabled via > kernel_neon_begin(), so it becomes a problem even on non-CONFIG_PREEMPT > kernels. The async distraction (re:filibustering) and the preempt concern are two totally different things. I've already posted some code elsewhere in this thread that addresses the preempt issue that looked good to Ard. This will be part of v7.
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
On Wed, Sep 26, 2018 at 05:41:12PM +0200, Jason A. Donenfeld wrote: > On Wed, Sep 26, 2018 at 4:02 PM Ard Biesheuvel > wrote: > > I don't think it makes sense to keep > > it simple now and add the complexity later (and the same concern > > applies to async support btw). > > Ugh, no. I don't want to add needless complexity, period. Zinc is > synchronous, not asynchronous. It provides software implementations. > That's what it does. While many of your reviews have been useful, many > of your comments indicate some desire to change and mold the purpose > and focus of Zinc away from Zinc's intents. Stop that. It's not going > to become a bloated mess of "things Ard wanted and quipped about on > LKML." Things like these only serve to filibuster the patchset > indefinitely. But maybe that's what you'd like all along? Hard to > tell, honestly. So, no, sorry, Zinc isn't gaining an async interface > right now. Can you please stop accusing Ard of "filibustering" your patchset? Spending too long in non-preemptible code is a real problem even on non-RT systems. syzkaller has been reporting bugs where the kernel spins too long without any preemption points, both in crypto-related code and elsewhere in the kernel. So we've had to add explicit preemption points to address those, as otherwise users can lock up all CPUs for tens of seconds. The issue being discussed here is basically the same except here preemption is being explicitly disabled via kernel_neon_begin(), so it becomes a problem even on non-CONFIG_PREEMPT kernels. It's much better to address this problem (which is a regression from the current skcipher API which only maps a page at a time) up front rather than have to rush to patch it after the fact once the syzbot reports start coming in. - Eric
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
> On Sep 26, 2018, at 10:03 AM, Jason A. Donenfeld wrote: > >> On Wed, Sep 26, 2018 at 6:21 PM Andy Lutomirski wrote: >> Are, is what you’re saying that the Zinc chacha20 functions should call >> simd_relax() every n bytes automatically for some reasonable value of n? If >> so, seems sensible, except that some care might be needed to make sure they >> interact with preemption correctly. >> >> What I mean is: the public Zinc entry points should either be callable in an >> atomic context or they should not be. I think this should be checked at >> runtime in an appropriate place with an __might_sleep or similar. Or >> simd_relax should learn to *not* schedule if the result of preempt_enable() >> leaves it atomic. (And the latter needs to be done in a way that works even >> on non-preempt kernels, and I don’t remember whether that’s possible.). And >> this should happen regardless of how many bytes are processed. IOW, calling >> into Zinc should be equally not atomic-safe for 100 bytes and for 10 MB. > > I'm not sure this is actually a problem. Namely: > > preempt_disable(); > kernel_fpu_begin(); > kernel_fpu_end(); > schedule(); <--- bug! > > Calling kernel_fpu_end() disables preemption, but AFAIK, preemption > enabling/disabling is recursive, so kernel_fpu_end's use of > preempt_disable won't actually do anything until the outer preempt > enable is called: > > preempt_disable(); > kernel_fpu_begin(); > kernel_fpu_end(); > preempt_enable(); > schedule(); <--- works! > > Or am I missing some more subtle point? > No, I think you’re right. I was mid-remembering precisely how simd_relax() worked.
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
On Wed, 26 Sep 2018 at 19:03, Jason A. Donenfeld wrote: > > On Wed, Sep 26, 2018 at 6:21 PM Andy Lutomirski wrote: > > Are, is what you’re saying that the Zinc chacha20 functions should call > > simd_relax() every n bytes automatically for some reasonable value of n? > > If so, seems sensible, except that some care might be needed to make sure > > they interact with preemption correctly. > > > > What I mean is: the public Zinc entry points should either be callable in > > an atomic context or they should not be. I think this should be checked at > > runtime in an appropriate place with an __might_sleep or similar. Or > > simd_relax should learn to *not* schedule if the result of preempt_enable() > > leaves it atomic. (And the latter needs to be done in a way that works even > > on non-preempt kernels, and I don’t remember whether that’s possible.). And > > this should happen regardless of how many bytes are processed. IOW, calling > > into Zinc should be equally not atomic-safe for 100 bytes and for 10 MB. > > I'm not sure this is actually a problem. Namely: > > preempt_disable(); > kernel_fpu_begin(); > kernel_fpu_end(); > schedule(); <--- bug! > > Calling kernel_fpu_end() disables preemption, but AFAIK, preemption > enabling/disabling is recursive, so kernel_fpu_end's use of > preempt_disable won't actually do anything until the outer preempt > enable is called: > > preempt_disable(); > kernel_fpu_begin(); > kernel_fpu_end(); > preempt_enable(); > schedule(); <--- works! > > Or am I missing some more subtle point? > No that seems accurate to me.
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
On Wed, Sep 26, 2018 at 6:55 PM Ard Biesheuvel wrote: > Framing it as /needless/ complexity does not help at all. The changes > you are proposing are very useful, but nobody wants two crypto > subsystems with two different maintainers in the kernel, so I would > like to understand where this is going in the future. I am not saying > it should block these patches though. Thanks for clarifying. I understood you to be intending to block the patches until they were converted to an async interface, which is not what Zinc's about. Seeing as you're just curious about future directions, that seems much more tenable. > Also, I have spent a *lot* of time looking at your code, and trying to > make it better, especially for use cases that weren't on your radar to > begin with I am extremely grateful for a good portion of your reviews indeed. As I mentioned earlier, much is very useful. But in other places, I fear you're steering this in a direction I really am hesitant to go. > (e.g., 'pet projects' [your words] Taken out of context. > consideration for other aspects of kernel programming, e.g., > preemption under -rt, stack size constraints, coding style, importing > code from other projects etc. And indeed all of these concerns I've been pretty amenable to, and continue to do so. What I'm commenting on are things outside of these. > - please try to be less dismissive of > feedback first time around, but try to understand why people are > raising these issues Apologies, and duly noted. I'll give you the benefit of the doubt. Jason
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
On Wed, Sep 26, 2018 at 6:21 PM Andy Lutomirski wrote: > Are, is what you’re saying that the Zinc chacha20 functions should call > simd_relax() every n bytes automatically for some reasonable value of n? If > so, seems sensible, except that some care might be needed to make sure they > interact with preemption correctly. > > What I mean is: the public Zinc entry points should either be callable in an > atomic context or they should not be. I think this should be checked at > runtime in an appropriate place with an __might_sleep or similar. Or > simd_relax should learn to *not* schedule if the result of preempt_enable() > leaves it atomic. (And the latter needs to be done in a way that works even > on non-preempt kernels, and I don’t remember whether that’s possible.). And > this should happen regardless of how many bytes are processed. IOW, calling > into Zinc should be equally not atomic-safe for 100 bytes and for 10 MB. I'm not sure this is actually a problem. Namely: preempt_disable(); kernel_fpu_begin(); kernel_fpu_end(); schedule(); <--- bug! Calling kernel_fpu_end() disables preemption, but AFAIK, preemption enabling/disabling is recursive, so kernel_fpu_end's use of preempt_disable won't actually do anything until the outer preempt enable is called: preempt_disable(); kernel_fpu_begin(); kernel_fpu_end(); preempt_enable(); schedule(); <--- works! Or am I missing some more subtle point? Jason
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
On Wed, 26 Sep 2018 at 17:41, Jason A. Donenfeld wrote: > > On Wed, Sep 26, 2018 at 4:02 PM Ard Biesheuvel > wrote: > > I don't think it makes sense to keep > > it simple now and add the complexity later (and the same concern > > applies to async support btw). > > Ugh, no. I don't want to add needless complexity, period. Zinc is > synchronous, not asynchronous. It provides software implementations. > That's what it does. While many of your reviews have been useful, many > of your comments indicate some desire to change and mold the purpose > and focus of Zinc away from Zinc's intents. Stop that. It's not going > to become a bloated mess of "things Ard wanted and quipped about on > LKML." Things like these only serve to filibuster the patchset > indefinitely. But maybe that's what you'd like all along? Hard to > tell, honestly. So, no, sorry, Zinc isn't gaining an async interface > right now. Framing it as /needless/ complexity does not help at all. The changes you are proposing are very useful, but nobody wants two crypto subsystems with two different maintainers in the kernel, so I would like to understand where this is going in the future. I am not saying it should block these patches though. Also, I have spent a *lot* of time looking at your code, and trying to make it better, especially for use cases that weren't on your radar to begin with (e.g., 'pet projects' [your words] like the Cortex-A7 which will be in almost every new 32-bit Android phone). So characterizing my feedback as some kind of sabotage is not very productive either. Contrary to what you seem to think, I am not deeply invested in the crypto API. What I do care about is that the ARM crypto pieces in the kernel are maintained, supported and improved by someone who understands the use cases Linaro's members care about, and is willing to make an effort to gain such understanding if he doesn't. I have no doubt that your involvement in the kernel's crypto subsystem will have a significant positive impact when it comes to code quality, robustness and usability. I'd just like to see a bit more consideration for other aspects of kernel programming, e.g., preemption under -rt, stack size constraints, coding style, importing code from other projects etc. - please try to be less dismissive of feedback first time around, but try to understand why people are raising these issues; I'm sure you will appreciate it when future contributors to zinc will do the same.
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
> On Sep 26, 2018, at 7:02 AM, Ard Biesheuvel wrote: > > (+ Herbert, Thomas) > >> On Wed, 26 Sep 2018 at 15:33, Jason A. Donenfeld wrote: >> >> Hi Ard, >> . > >> And if it becomes one, >> this is something we can address *later*, but certainly there's no use >> of adding additional complexity to the initial patchset to do this >> now. >> > > You are introducing a very useful SIMD abstraction, but it lets code > run with preemption disabled for unbounded amounts of time, and so now > is the time to ensure we get it right. > > Part of the [justified] criticism on the current state of the crypto > API is on its complexity, and so I don't think it makes sense to keep > it simple now and add the complexity later (and the same concern > applies to async support btw). Are, is what you’re saying that the Zinc chacha20 functions should call simd_relax() every n bytes automatically for some reasonable value of n? If so, seems sensible, except that some care might be needed to make sure they interact with preemption correctly. What I mean is: the public Zinc entry points should either be callable in an atomic context or they should not be. I think this should be checked at runtime in an appropriate place with an __might_sleep or similar. Or simd_relax should learn to *not* schedule if the result of preempt_enable() leaves it atomic. (And the latter needs to be done in a way that works even on non-preempt kernels, and I don’t remember whether that’s possible.). And this should happen regardless of how many bytes are processed. IOW, calling into Zinc should be equally not atomic-safe for 100 bytes and for 10 MB. As for async, ISTM a really good WireGuard accelerator would expose a different interface than crypto API supports, and it probably makes sense to wait for such hardware to show up before figuring out how to use it. And no matter what form it takes, I don’t think it should complicate the basic Zinc crypto entry points.
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
On Wed, Sep 26, 2018 at 5:52 PM Ard Biesheuvel wrote: > Nice one :-) > > This works for me (but perhaps add a comment as well) Sure. Just a prototype; it'll be clean for v7.
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
On Wed, 26 Sep 2018 at 17:50, Jason A. Donenfeld wrote:
>
> On Wed, Sep 26, 2018 at 5:45 PM Jason A. Donenfeld wrote:
> > So what you have in mind is something like calling simd_relax() every
> > 4096 bytes or so?
>
> That was actually pretty easy, putting together both of your suggestions:
>
> static inline bool chacha20_arch(struct chacha20_ctx *state, u8 *dst,
> u8 *src, size_t len,
> simd_context_t *simd_context)
> {
> while (len > PAGE_SIZE) {
> chacha20_arch(state, dst, src, PAGE_SIZE, simd_context);
> len -= PAGE_SIZE;
> src += PAGE_SIZE;
> dst += PAGE_SIZE;
> simd_relax(simd_context);
> }
> if (IS_ENABLED(CONFIG_KERNEL_MODE_NEON) && chacha20_use_neon &&
> len >= CHACHA20_BLOCK_SIZE * 3 && simd_use(simd_context))
> chacha20_neon(dst, src, len, state->key, state->counter);
> else
> chacha20_arm(dst, src, len, state->key, state->counter);
>
> state->counter[0] += (len + 63) / 64;
> return true;
> }
Nice one :-)
This works for me (but perhaps add a comment as well)
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
On Wed, Sep 26, 2018 at 5:45 PM Jason A. Donenfeld wrote:
> So what you have in mind is something like calling simd_relax() every
> 4096 bytes or so?
That was actually pretty easy, putting together both of your suggestions:
static inline bool chacha20_arch(struct chacha20_ctx *state, u8 *dst,
u8 *src, size_t len,
simd_context_t *simd_context)
{
while (len > PAGE_SIZE) {
chacha20_arch(state, dst, src, PAGE_SIZE, simd_context);
len -= PAGE_SIZE;
src += PAGE_SIZE;
dst += PAGE_SIZE;
simd_relax(simd_context);
}
if (IS_ENABLED(CONFIG_KERNEL_MODE_NEON) && chacha20_use_neon &&
len >= CHACHA20_BLOCK_SIZE * 3 && simd_use(simd_context))
chacha20_neon(dst, src, len, state->key, state->counter);
else
chacha20_arm(dst, src, len, state->key, state->counter);
state->counter[0] += (len + 63) / 64;
return true;
}
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
On Wed, Sep 26, 2018 at 5:42 PM Ard Biesheuvel wrote: > > On Wed, 26 Sep 2018 at 16:02, Ard Biesheuvel > wrote: > Actually, looking at the code again, the abstraction does appear to be > fine, it is just the chacha20 code that does not make use of it. So what you have in mind is something like calling simd_relax() every 4096 bytes or so?
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
On Wed, 26 Sep 2018 at 16:02, Ard Biesheuvel wrote:
>
> (+ Herbert, Thomas)
>
> On Wed, 26 Sep 2018 at 15:33, Jason A. Donenfeld wrote:
> >
> > Hi Ard,
> >
> > On Wed, Sep 26, 2018 at 10:59 AM Ard Biesheuvel
> > wrote:
> > > > +static inline bool chacha20_arch(struct chacha20_ctx *state, u8 *dst,
> > > > +const u8 *src, size_t len,
> > > > +simd_context_t *simd_context)
> > > > +{
> > > > +#if defined(CONFIG_KERNEL_MODE_NEON)
> > > > + if (chacha20_use_neon && len >= CHACHA20_BLOCK_SIZE * 3 &&
> > > > + simd_use(simd_context))
> > > > + chacha20_neon(dst, src, len, state->key,
> > > > state->counter);
> > > > + else
> > > > +#endif
> > >
> > > Better to use IS_ENABLED() here:
> > >
> > > > + if (IS_ENABLED(CONFIG_KERNEL_MODE_NEON)) &&
> > > > + chacha20_use_neon && len >= CHACHA20_BLOCK_SIZE * 3 &&
> > > > + simd_use(simd_context))
> >
> > Good idea. I'll fix that up.
> >
> > >
> > > Also, this still has unbounded worst case scheduling latency, given
> > > that the outer library function passes its entire input straight into
> > > the NEON routine.
> >
> > The vast majority of crypto routines in arch/*/crypto/ follow this
> > same exact pattern, actually. I realize a few don't -- probably the
> > ones you had a hand in :) -- but I think this is up to the caller to
> > handle.
>
> Anything that uses the scatterwalk API (AEADs and skciphers) will
> handle at most a page at a time. Hashes are different, which is why
> some of them have to handle it explicitly.
>
> > I made a change so that in chacha20poly1305.c, it calls
> > simd_relax after handling each scatter-gather element, so a
> > "construction" will handle this gracefully. But I believe it's up to
> > the caller to decide on what sizes of information it wants to pass to
> > primitives. Put differently, this also hasn't ever been an issue
> > before -- the existing state of the tree indicates this -- and so I
> > don't anticipate this will be a real issue now.
>
> The state of the tree does not capture all relevant context or
> history. The scheduling latency issue was brought up very recently by
> the -rt folks on the mailing lists.
>
> > And if it becomes one,
> > this is something we can address *later*, but certainly there's no use
> > of adding additional complexity to the initial patchset to do this
> > now.
> >
>
> You are introducing a very useful SIMD abstraction, but it lets code
> run with preemption disabled for unbounded amounts of time, and so now
> is the time to ensure we get it right.
>
Actually, looking at the code again, the abstraction does appear to be
fine, it is just the chacha20 code that does not make use of it.
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
On Wed, Sep 26, 2018 at 4:02 PM Ard Biesheuvel wrote: > I don't think it makes sense to keep > it simple now and add the complexity later (and the same concern > applies to async support btw). Ugh, no. I don't want to add needless complexity, period. Zinc is synchronous, not asynchronous. It provides software implementations. That's what it does. While many of your reviews have been useful, many of your comments indicate some desire to change and mold the purpose and focus of Zinc away from Zinc's intents. Stop that. It's not going to become a bloated mess of "things Ard wanted and quipped about on LKML." Things like these only serve to filibuster the patchset indefinitely. But maybe that's what you'd like all along? Hard to tell, honestly. So, no, sorry, Zinc isn't gaining an async interface right now.
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
On Wed, Sep 26, 2018 at 4:36 PM Andrew Lunn wrote: > The wireguard interface claims it is GSO capable. This means the > network stack will pass it big chunks of data and leave it to the > network interface to perform the segmentation into 1500 byte MTU > frames on the wire. I've not looked at how wireguard actually handles > these big chunks. But to get maximum performance, it should try to > keep them whole, just add a header and/or trailer. Will wireguard pass > these big chunks of data to the crypto code? Do we now have 64K blocks > being worked on? Does the latency jump from 4K to 64K? That might be > new, so the existing state of the tree does not help you here. No, it only requests GSO superpackets so that it can group the pieces and encrypt them on the same core. But they're each encrypted separately (broken up immediately after ndo_start_xmit), and so they wind up being ~1420 bytes each to encrypt. I spoke about this at netdev2.2 if you're interested in the architecture; there's a paper: https://www.wireguard.com/papers/wireguard-netdev22.pdf https://www.youtube.com/watch?v=54orFwtQ1XY https://www.wireguard.com/talks/netdev2017-slides.pdf
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
> > Also, this still has unbounded worst case scheduling latency, given > > that the outer library function passes its entire input straight into > > the NEON routine. > > The vast majority of crypto routines in arch/*/crypto/ follow this > same exact pattern, actually. I realize a few don't -- probably the > ones you had a hand in :) -- but I think this is up to the caller to > handle. I made a change so that in chacha20poly1305.c, it calls > simd_relax after handling each scatter-gather element, so a > "construction" will handle this gracefully. But I believe it's up to > the caller to decide on what sizes of information it wants to pass to > primitives. Put differently, this also hasn't ever been an issue > before -- the existing state of the tree indicates this -- and so I > don't anticipate this will be a real issue now. And if it becomes one, > this is something we can address *later*, but certainly there's no use > of adding additional complexity to the initial patchset to do this > now. Hi Jason This is not my area of expertise, so you should verify what i'm say here... My guess is, IPSEC will mostly ask the crypto code to work on 1500 byte full MTU packets and 64 byte TCP ACK packets. Disk encryption i guess works on 4K blocks. So these requests are all quite small, keeping the latency reasonably bounded. The wireguard interface claims it is GSO capable. This means the network stack will pass it big chunks of data and leave it to the network interface to perform the segmentation into 1500 byte MTU frames on the wire. I've not looked at how wireguard actually handles these big chunks. But to get maximum performance, it should try to keep them whole, just add a header and/or trailer. Will wireguard pass these big chunks of data to the crypto code? Do we now have 64K blocks being worked on? Does the latency jump from 4K to 64K? That might be new, so the existing state of the tree does not help you here. Andrew
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
(+ Herbert, Thomas)
On Wed, 26 Sep 2018 at 15:33, Jason A. Donenfeld wrote:
>
> Hi Ard,
>
> On Wed, Sep 26, 2018 at 10:59 AM Ard Biesheuvel
> wrote:
> > > +static inline bool chacha20_arch(struct chacha20_ctx *state, u8 *dst,
> > > +const u8 *src, size_t len,
> > > +simd_context_t *simd_context)
> > > +{
> > > +#if defined(CONFIG_KERNEL_MODE_NEON)
> > > + if (chacha20_use_neon && len >= CHACHA20_BLOCK_SIZE * 3 &&
> > > + simd_use(simd_context))
> > > + chacha20_neon(dst, src, len, state->key, state->counter);
> > > + else
> > > +#endif
> >
> > Better to use IS_ENABLED() here:
> >
> > > + if (IS_ENABLED(CONFIG_KERNEL_MODE_NEON)) &&
> > > + chacha20_use_neon && len >= CHACHA20_BLOCK_SIZE * 3 &&
> > > + simd_use(simd_context))
>
> Good idea. I'll fix that up.
>
> >
> > Also, this still has unbounded worst case scheduling latency, given
> > that the outer library function passes its entire input straight into
> > the NEON routine.
>
> The vast majority of crypto routines in arch/*/crypto/ follow this
> same exact pattern, actually. I realize a few don't -- probably the
> ones you had a hand in :) -- but I think this is up to the caller to
> handle.
Anything that uses the scatterwalk API (AEADs and skciphers) will
handle at most a page at a time. Hashes are different, which is why
some of them have to handle it explicitly.
> I made a change so that in chacha20poly1305.c, it calls
> simd_relax after handling each scatter-gather element, so a
> "construction" will handle this gracefully. But I believe it's up to
> the caller to decide on what sizes of information it wants to pass to
> primitives. Put differently, this also hasn't ever been an issue
> before -- the existing state of the tree indicates this -- and so I
> don't anticipate this will be a real issue now.
The state of the tree does not capture all relevant context or
history. The scheduling latency issue was brought up very recently by
the -rt folks on the mailing lists.
> And if it becomes one,
> this is something we can address *later*, but certainly there's no use
> of adding additional complexity to the initial patchset to do this
> now.
>
You are introducing a very useful SIMD abstraction, but it lets code
run with preemption disabled for unbounded amounts of time, and so now
is the time to ensure we get it right.
Part of the [justified] criticism on the current state of the crypto
API is on its complexity, and so I don't think it makes sense to keep
it simple now and add the complexity later (and the same concern
applies to async support btw).
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
Hi Ard,
On Wed, Sep 26, 2018 at 10:59 AM Ard Biesheuvel
wrote:
> > +static inline bool chacha20_arch(struct chacha20_ctx *state, u8 *dst,
> > +const u8 *src, size_t len,
> > +simd_context_t *simd_context)
> > +{
> > +#if defined(CONFIG_KERNEL_MODE_NEON)
> > + if (chacha20_use_neon && len >= CHACHA20_BLOCK_SIZE * 3 &&
> > + simd_use(simd_context))
> > + chacha20_neon(dst, src, len, state->key, state->counter);
> > + else
> > +#endif
>
> Better to use IS_ENABLED() here:
>
> > + if (IS_ENABLED(CONFIG_KERNEL_MODE_NEON)) &&
> > + chacha20_use_neon && len >= CHACHA20_BLOCK_SIZE * 3 &&
> > + simd_use(simd_context))
Good idea. I'll fix that up.
>
> Also, this still has unbounded worst case scheduling latency, given
> that the outer library function passes its entire input straight into
> the NEON routine.
The vast majority of crypto routines in arch/*/crypto/ follow this
same exact pattern, actually. I realize a few don't -- probably the
ones you had a hand in :) -- but I think this is up to the caller to
handle. I made a change so that in chacha20poly1305.c, it calls
simd_relax after handling each scatter-gather element, so a
"construction" will handle this gracefully. But I believe it's up to
the caller to decide on what sizes of information it wants to pass to
primitives. Put differently, this also hasn't ever been an issue
before -- the existing state of the tree indicates this -- and so I
don't anticipate this will be a real issue now. And if it becomes one,
this is something we can address *later*, but certainly there's no use
of adding additional complexity to the initial patchset to do this
now.
Jason
Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
On Tue, 25 Sep 2018 at 17:00, Jason A. Donenfeld wrote:
>
> These wire Andy Polyakov's implementations up to the kernel for ARMv7,8
> NEON, and introduce Eric Biggers' ultra-fast scalar implementation for
> CPUs without NEON or for CPUs with slow NEON (Cortex-A5,7).
>
> This commit does the following:
> - Adds the glue code for the assembly implementations.
> - Renames the ARMv8 code into place, since it can at this point be
> used wholesale.
> - Merges Andy Polyakov's ARMv7 NEON code with Eric Biggers' <=ARMv7
> scalar code.
>
> Commit note: Eric Biggers' scalar code is brand new, and quite possibly
> prematurely added to this commit, and so it may require a bit of revision.
>
> This commit delivers approximately the same or much better performance than
> the existing crypto API's code and has been measured to do as such on:
>
> - ARM1176JZF-S [ARMv6]
> - Cortex-A7[ARMv7]
> - Cortex-A8[ARMv7]
> - Cortex-A9[ARMv7]
> - Cortex-A17 [ARMv7]
> - Cortex-A53 [ARMv8]
> - Cortex-A55 [ARMv8]
> - Cortex-A73 [ARMv8]
> - Cortex-A75 [ARMv8]
>
> Interestingly, Andy Polyakov's scalar code is slower than Eric Biggers',
> but is also significantly shorter. This has the advantage that it does
> not evict other code from L1 cache -- particularly on ARM11 chips -- and
> so in certain circumstances it can actually be faster. However, it wasn't
> found that this had an affect on any code existing in the kernel today.
>
> Signed-off-by: Jason A. Donenfeld
> Co-authored-by: Eric Biggers
> Cc: Samuel Neves
> Cc: Andy Lutomirski
> Cc: Greg KH
> Cc: Jean-Philippe Aumasson
> Cc: Russell King
> Cc: linux-arm-ker...@lists.infradead.org
> ---
> lib/zinc/Makefile | 2 +
> lib/zinc/chacha20/chacha20-arm-glue.h | 88 +++
> ...acha20-arm-cryptogams.S => chacha20-arm.S} | 502 --
> ...20-arm64-cryptogams.S => chacha20-arm64.S} | 0
> lib/zinc/chacha20/chacha20.c | 2 +
> 5 files changed, 556 insertions(+), 38 deletions(-)
> create mode 100644 lib/zinc/chacha20/chacha20-arm-glue.h
> rename lib/zinc/chacha20/{chacha20-arm-cryptogams.S => chacha20-arm.S} (71%)
> rename lib/zinc/chacha20/{chacha20-arm64-cryptogams.S => chacha20-arm64.S}
> (100%)
>
> diff --git a/lib/zinc/Makefile b/lib/zinc/Makefile
> index 223a0816c918..e47f64e12bbd 100644
> --- a/lib/zinc/Makefile
> +++ b/lib/zinc/Makefile
> @@ -4,4 +4,6 @@ ccflags-$(CONFIG_ZINC_DEBUG) += -DDEBUG
>
> zinc_chacha20-y := chacha20/chacha20.o
> zinc_chacha20-$(CONFIG_ZINC_ARCH_X86_64) += chacha20/chacha20-x86_64.o
> +zinc_chacha20-$(CONFIG_ZINC_ARCH_ARM) += chacha20/chacha20-arm.o
> +zinc_chacha20-$(CONFIG_ZINC_ARCH_ARM64) += chacha20/chacha20-arm64.o
> obj-$(CONFIG_ZINC_CHACHA20) += zinc_chacha20.o
> diff --git a/lib/zinc/chacha20/chacha20-arm-glue.h
> b/lib/zinc/chacha20/chacha20-arm-glue.h
> new file mode 100644
> index ..86cce851ed02
> --- /dev/null
> +++ b/lib/zinc/chacha20/chacha20-arm-glue.h
> @@ -0,0 +1,88 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/*
> + * Copyright (C) 2015-2018 Jason A. Donenfeld . All Rights
> Reserved.
> + */
> +
> +#include
> +#include
> +#if defined(CONFIG_ARM)
> +#include
> +#include
> +#endif
> +
> +asmlinkage void chacha20_arm(u8 *out, const u8 *in, const size_t len,
> +const u32 key[8], const u32 counter[4]);
> +#if defined(CONFIG_ARM)
> +asmlinkage void hchacha20_arm(const u32 state[16], u32 out[8]);
> +#endif
> +#if defined(CONFIG_KERNEL_MODE_NEON)
> +asmlinkage void chacha20_neon(u8 *out, const u8 *in, const size_t len,
> + const u32 key[8], const u32 counter[4]);
> +#endif
> +
> +static bool chacha20_use_neon __ro_after_init;
> +
> +static void __init chacha20_fpu_init(void)
> +{
> +#if defined(CONFIG_ARM64)
> + chacha20_use_neon = elf_hwcap & HWCAP_ASIMD;
> +#elif defined(CONFIG_ARM)
> + switch (read_cpuid_part()) {
> + case ARM_CPU_PART_CORTEX_A7:
> + case ARM_CPU_PART_CORTEX_A5:
> + /* The Cortex-A7 and Cortex-A5 do not perform well with the
> NEON
> +* implementation but do incredibly with the scalar one and
> use
> +* less power.
> +*/
> + break;
> + default:
> + chacha20_use_neon = elf_hwcap & HWCAP_NEON;
> + }
> +#endif
> +}
> +
> +static inline bool chacha20_arch(struct chacha20_ctx *state, u8 *dst,
> +const u8 *src, size_t len,
> +simd_context_t *simd_context)
> +{
> +#if defined(CONFIG_KERNEL_MODE_NEON)
> + if (chacha20_use_neon && len >= CHACHA20_BLOCK_SIZE * 3 &&
> + simd_use(simd_context))
> + chacha20_neon(dst, src, len, state->key, state->counter);
> + else
> +#endif
Better to use IS_ENABLED() here:
> + if (IS_ENABLED(CONFIG_KERNEL_MODE_NEON)) &&
> + chacha20_use_neon && len
[PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations
These wire Andy Polyakov's implementations up to the kernel for ARMv7,8
NEON, and introduce Eric Biggers' ultra-fast scalar implementation for
CPUs without NEON or for CPUs with slow NEON (Cortex-A5,7).
This commit does the following:
- Adds the glue code for the assembly implementations.
- Renames the ARMv8 code into place, since it can at this point be
used wholesale.
- Merges Andy Polyakov's ARMv7 NEON code with Eric Biggers' <=ARMv7
scalar code.
Commit note: Eric Biggers' scalar code is brand new, and quite possibly
prematurely added to this commit, and so it may require a bit of revision.
This commit delivers approximately the same or much better performance than
the existing crypto API's code and has been measured to do as such on:
- ARM1176JZF-S [ARMv6]
- Cortex-A7[ARMv7]
- Cortex-A8[ARMv7]
- Cortex-A9[ARMv7]
- Cortex-A17 [ARMv7]
- Cortex-A53 [ARMv8]
- Cortex-A55 [ARMv8]
- Cortex-A73 [ARMv8]
- Cortex-A75 [ARMv8]
Interestingly, Andy Polyakov's scalar code is slower than Eric Biggers',
but is also significantly shorter. This has the advantage that it does
not evict other code from L1 cache -- particularly on ARM11 chips -- and
so in certain circumstances it can actually be faster. However, it wasn't
found that this had an affect on any code existing in the kernel today.
Signed-off-by: Jason A. Donenfeld
Co-authored-by: Eric Biggers
Cc: Samuel Neves
Cc: Andy Lutomirski
Cc: Greg KH
Cc: Jean-Philippe Aumasson
Cc: Russell King
Cc: linux-arm-ker...@lists.infradead.org
---
lib/zinc/Makefile | 2 +
lib/zinc/chacha20/chacha20-arm-glue.h | 88 +++
...acha20-arm-cryptogams.S => chacha20-arm.S} | 502 --
...20-arm64-cryptogams.S => chacha20-arm64.S} | 0
lib/zinc/chacha20/chacha20.c | 2 +
5 files changed, 556 insertions(+), 38 deletions(-)
create mode 100644 lib/zinc/chacha20/chacha20-arm-glue.h
rename lib/zinc/chacha20/{chacha20-arm-cryptogams.S => chacha20-arm.S} (71%)
rename lib/zinc/chacha20/{chacha20-arm64-cryptogams.S => chacha20-arm64.S}
(100%)
diff --git a/lib/zinc/Makefile b/lib/zinc/Makefile
index 223a0816c918..e47f64e12bbd 100644
--- a/lib/zinc/Makefile
+++ b/lib/zinc/Makefile
@@ -4,4 +4,6 @@ ccflags-$(CONFIG_ZINC_DEBUG) += -DDEBUG
zinc_chacha20-y := chacha20/chacha20.o
zinc_chacha20-$(CONFIG_ZINC_ARCH_X86_64) += chacha20/chacha20-x86_64.o
+zinc_chacha20-$(CONFIG_ZINC_ARCH_ARM) += chacha20/chacha20-arm.o
+zinc_chacha20-$(CONFIG_ZINC_ARCH_ARM64) += chacha20/chacha20-arm64.o
obj-$(CONFIG_ZINC_CHACHA20) += zinc_chacha20.o
diff --git a/lib/zinc/chacha20/chacha20-arm-glue.h
b/lib/zinc/chacha20/chacha20-arm-glue.h
new file mode 100644
index ..86cce851ed02
--- /dev/null
+++ b/lib/zinc/chacha20/chacha20-arm-glue.h
@@ -0,0 +1,88 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+/*
+ * Copyright (C) 2015-2018 Jason A. Donenfeld . All Rights
Reserved.
+ */
+
+#include
+#include
+#if defined(CONFIG_ARM)
+#include
+#include
+#endif
+
+asmlinkage void chacha20_arm(u8 *out, const u8 *in, const size_t len,
+const u32 key[8], const u32 counter[4]);
+#if defined(CONFIG_ARM)
+asmlinkage void hchacha20_arm(const u32 state[16], u32 out[8]);
+#endif
+#if defined(CONFIG_KERNEL_MODE_NEON)
+asmlinkage void chacha20_neon(u8 *out, const u8 *in, const size_t len,
+ const u32 key[8], const u32 counter[4]);
+#endif
+
+static bool chacha20_use_neon __ro_after_init;
+
+static void __init chacha20_fpu_init(void)
+{
+#if defined(CONFIG_ARM64)
+ chacha20_use_neon = elf_hwcap & HWCAP_ASIMD;
+#elif defined(CONFIG_ARM)
+ switch (read_cpuid_part()) {
+ case ARM_CPU_PART_CORTEX_A7:
+ case ARM_CPU_PART_CORTEX_A5:
+ /* The Cortex-A7 and Cortex-A5 do not perform well with the NEON
+* implementation but do incredibly with the scalar one and use
+* less power.
+*/
+ break;
+ default:
+ chacha20_use_neon = elf_hwcap & HWCAP_NEON;
+ }
+#endif
+}
+
+static inline bool chacha20_arch(struct chacha20_ctx *state, u8 *dst,
+const u8 *src, size_t len,
+simd_context_t *simd_context)
+{
+#if defined(CONFIG_KERNEL_MODE_NEON)
+ if (chacha20_use_neon && len >= CHACHA20_BLOCK_SIZE * 3 &&
+ simd_use(simd_context))
+ chacha20_neon(dst, src, len, state->key, state->counter);
+ else
+#endif
+ chacha20_arm(dst, src, len, state->key, state->counter);
+
+ state->counter[0] += (len + 63) / 64;
+ return true;
+}
+
+static inline bool hchacha20_arch(u32 derived_key[CHACHA20_KEY_WORDS],
+ const u8 nonce[HCHACHA20_NONCE_SIZE],
+ const u8 key[HCHACHA20_KEY_SIZE],
+ simd_context_t *simd_context)
+{
+#if def
