From: "Jason A. Donenfeld" Subject: Re: [PATCH net-next v6 07/23] zinc: ChaCha20 ARM and ARM64 implementations Date: Sat, 29 Sep 2018 04:20:24 +0200 Message-ID: References: <20180925145622.29959-1-Jason@zx2c4.com> <20180925145622.29959-8-Jason@zx2c4.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: LKML , Netdev , Linux Crypto Mailing List , David Miller , Greg Kroah-Hartman , Samuel Neves , Andrew Lutomirski , Jean-Philippe Aumasson , Russell King - ARM Linux , linux-arm-kernel@lists.infradead.org To: Ard Biesheuvel Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-crypto.vger.kernel.org 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