Re: [PATCH v2 0/3] ARM: NEON based fast(er) AES in CBC/CTR/XTS modes
Hi Ard, On Thu, Oct 03, 2013 at 10:59:23PM +0100, Ard Biesheuvel wrote: > Note to reviewers: > Reviewing the file aesbs-core.S may be a bit overwhelming, so if there are any > questions or concerns, please refer the file bsaes-armv7.pl which can be found > at the link below. This is the original Perl script that gets called by > OpenSSL's build system during their build to generate the .S file on the fly. > [In the case of OpenSSL, this is used in some cases to target different > assemblers or ABIs]. This arrangement is not suitable (or required) for the > kernel, so I have taken the generated .S file instead. > > http://git.openssl.org/gitweb/?p=openssl.git;a=commit;h=6f6a6130 > > This series still depends on commit a62b01cd (crypto: create generic version > of > ablk_helper) which I omitted this time but which can be found in the cryptodev > tree or in linux-next. Why do you consider it unsuitable to ship the perl script with the kernel? Perl 5 is already documented as a build dependency in Documentation/Changes and I'd *much* rather the .S file was generated rather than shipped and hacked. That amount of opaque assembly code for something like crypto feels really dangerous from both a review and a maintenance perspective. Will -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] ARM: NEON based fast(er) AES in CBC/CTR/XTS modes
On 4 October 2013 19:48, Will Deacon wrote: > On Thu, Oct 03, 2013 at 10:59:23PM +0100, Ard Biesheuvel wrote: >> Note to reviewers: >> Reviewing the file aesbs-core.S may be a bit overwhelming, so if there are >> any >> questions or concerns, please refer the file bsaes-armv7.pl which can be >> found >> at the link below. This is the original Perl script that gets called by >> OpenSSL's build system during their build to generate the .S file on the fly. >> [In the case of OpenSSL, this is used in some cases to target different >> assemblers or ABIs]. This arrangement is not suitable (or required) for the >> kernel, so I have taken the generated .S file instead. >> >> http://git.openssl.org/gitweb/?p=openssl.git;a=commit;h=6f6a6130 >> >> This series still depends on commit a62b01cd (crypto: create generic version >> of >> ablk_helper) which I omitted this time but which can be found in the >> cryptodev >> tree or in linux-next. > > Why do you consider it unsuitable to ship the perl script with the kernel? > Perl 5 is already documented as a build dependency in Documentation/Changes > and I'd *much* rather the .S file was generated rather than shipped and > hacked. That amount of opaque assembly code for something like crypto feels > really > dangerous from both a review and a maintenance perspective. > First of all, please note that the whole point of working so closely with the OpenSSL maintainer on this is that the version I am presenting here is the verbatim output of the Perl script that lives in the OpenSSL tree. So just shipped, not shipped and hacked. Personally, I would much prefer merging the .pl file as well, I just thought (and I did poll some people informally) that this is not something most people are happy about. If I am wrong about this, than I am quite happy to respin so the .S is generated on the fly. -- Ard. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] ARM: NEON based fast(er) AES in CBC/CTR/XTS modes
On Fri, Oct 04, 2013 at 08:04:50PM +0200, Ard Biesheuvel wrote: > First of all, please note that the whole point of working so closely > with the OpenSSL maintainer on this is that the version I am > presenting here is the verbatim output of the Perl script that lives > in the OpenSSL tree. So just shipped, not shipped and hacked. > > Personally, I would much prefer merging the .pl file as well, I just > thought (and I did poll some people informally) that this is not > something most people are happy about. If I am wrong about this, than > I am quite happy to respin so the .S is generated on the fly. While it is desirable to keep the dependencies on external tools to a minimum, as perl is already on the list of required dependencies, there is no problem with including a script. Also, remember that the GPL says: "The source code for a work means the preferred form of the work for making modifications to it." So here's the question: is the assembly code the perferred form to make modifications? From what you're saying above, the answer to that seems to be no. Now, I'm not going to throw toys out of the pram and say that this is a hard requirement, but just take a moment to think about how we treat vendors who don't do this, instead supplying "non-preferred" forms of source code, and think about whether there's double standards here. Now, while I can imagine that people have an ideological objection to perl (using comments like "write only code" etc) that's not a good enough excuse to avoid including the perferred form for future modification. Now, we have mechanisms in the kernel build where we can include a prepared source which can be used to lessen the burden on the toolset required to build the kernel. So, including both the perl script and the pre-generated assembly is entirely acceptable. This tends to nullify the excuse that "we don't want to add additional tool burden to kbuild" argument. So, what I'd strongly recommend is that we add both the pre-generated assembly with a _shipped suffix, the perl script, and include a rule like this: quiet_cmd_perl = PERL$@ cmd_perl = perl $< > $@ $(src)/blahblah.S_shipped: $(src)/myperlscript $(call cmd,perl) and that should end up running "myperlscript" whenever it has a date stamp newer than the _shipped file, or if that file is missing. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] ARM: NEON based fast(er) AES in CBC/CTR/XTS modes
On Fri, 4 Oct 2013, Will Deacon wrote: > Hi Ard, > > On Thu, Oct 03, 2013 at 10:59:23PM +0100, Ard Biesheuvel wrote: > > Note to reviewers: > > Reviewing the file aesbs-core.S may be a bit overwhelming, so if there are > > any > > questions or concerns, please refer the file bsaes-armv7.pl which can be > > found > > at the link below. This is the original Perl script that gets called by > > OpenSSL's build system during their build to generate the .S file on the > > fly. > > [In the case of OpenSSL, this is used in some cases to target different > > assemblers or ABIs]. This arrangement is not suitable (or required) for the > > kernel, so I have taken the generated .S file instead. > > > > http://git.openssl.org/gitweb/?p=openssl.git;a=commit;h=6f6a6130 > > > > This series still depends on commit a62b01cd (crypto: create generic > > version of > > ablk_helper) which I omitted this time but which can be found in the > > cryptodev > > tree or in linux-next. > > Why do you consider it unsuitable to ship the perl script with the kernel? > Perl 5 is already documented as a build dependency in Documentation/Changes Do you have an example of something that does require perl to build the kernel on ARM? I was under the impression that people try to avoid it as much as possible in general. I'm personally sitting on the fence between effectively adding a new kernel build dependencies or carrying the output of the perl script. But if the kernel build does already require perl in practice then this might tip the balance. Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] ARM: NEON based fast(er) AES in CBC/CTR/XTS modes
On Fri, 4 Oct 2013, Russell King - ARM Linux wrote: > Also, remember that the GPL says: > > "The source code for a work means the preferred form of the work for > making modifications to it." > > So here's the question: is the assembly code the perferred form to make > modifications? From what you're saying above, the answer to that seems > to be no. Now, I'm not going to throw toys out of the pram and say > that this is a hard requirement, but just take a moment to think about > how we treat vendors who don't do this, instead supplying "non-preferred" > forms of source code, and think about whether there's double standards > here. > > Now, while I can imagine that people have an ideological objection to > perl (using comments like "write only code" etc) that's not a good > enough excuse to avoid including the perferred form for future > modification. > > Now, we have mechanisms in the kernel build where we can include a > prepared source which can be used to lessen the burden on the toolset > required to build the kernel. So, including both the perl script and > the pre-generated assembly is entirely acceptable. This tends to > nullify the excuse that "we don't want to add additional tool burden > to kbuild" argument. Those are very good arguments. And I agree with your recommendation as well. Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] ARM: NEON based fast(er) AES in CBC/CTR/XTS modes
On 4 October 2013 20:34, Nicolas Pitre wrote: > On Fri, 4 Oct 2013, Will Deacon wrote: [...] >> >> Why do you consider it unsuitable to ship the perl script with the kernel? >> Perl 5 is already documented as a build dependency in Documentation/Changes > > Do you have an example of something that does require perl to build the > kernel on ARM? I was under the impression that people try to avoid it > as much as possible in general. > > I'm personally sitting on the fence between effectively adding a new > kernel build dependencies or carrying the output of the perl script. > But if the kernel build does already require perl in practice then this > might tip the balance. > I like Russell's suggestion the most, in fact. In this case, the build time requirement for Perl effectively gets suspended until you start making modifications to the perl script, and the relation between the .S and the .pl files is made explicit by the make rule. Should I put the cmd_perl rule in scripts/Makefile.build ? Or can I just keep it under arch/arm/crypto ? -- Ard. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] ARM: NEON based fast(er) AES in CBC/CTR/XTS modes
On Fri, Oct 04, 2013 at 02:34:01PM -0400, Nicolas Pitre wrote: > Do you have an example of something that does require perl to build the > kernel on ARM? I was under the impression that people try to avoid it > as much as possible in general. > > I'm personally sitting on the fence between effectively adding a new > kernel build dependencies or carrying the output of the perl script. > But if the kernel build does already require perl in practice then this > might tip the balance. That is really not a concern with the modern kbuild. It has supported having shipped versions of generated files included in the source tree for years. So, we can have the perl version included (the preferred form for editing) while avoiding the issue of requiring everyone to have perl. In other words, if you want to build a kernel, you don't need perl for this. If you want to edit the "preferred" form, then you do need perl so that the "preferred" form can be turned into assembly to update the shipped version. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] ARM: NEON based fast(er) AES in CBC/CTR/XTS modes
On Fri, Oct 04, 2013 at 08:41:35PM +0200, Ard Biesheuvel wrote: > On 4 October 2013 20:34, Nicolas Pitre wrote: > > On Fri, 4 Oct 2013, Will Deacon wrote: > [...] > >> > >> Why do you consider it unsuitable to ship the perl script with the kernel? > >> Perl 5 is already documented as a build dependency in Documentation/Changes > > > > Do you have an example of something that does require perl to build the > > kernel on ARM? I was under the impression that people try to avoid it > > as much as possible in general. > > > > I'm personally sitting on the fence between effectively adding a new > > kernel build dependencies or carrying the output of the perl script. > > But if the kernel build does already require perl in practice then this > > might tip the balance. > > > > I like Russell's suggestion the most, in fact. In this case, the build > time requirement for Perl effectively gets suspended until you start > making modifications to the perl script, and the relation between the > .S and the .pl files is made explicit by the make rule. > > Should I put the cmd_perl rule in scripts/Makefile.build ? Or can I > just keep it under arch/arm/crypto ? Just running through the Makefiles, it seems we have a fair amount of stuff already using perl in various ways. So I wouldn't worry too much about where it's placed. It's probably something that should eventually end up in scripts/ at _some_ point. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] ARM: NEON based fast(er) AES in CBC/CTR/XTS modes
On Fri, 4 Oct 2013, Ard Biesheuvel wrote: > On 4 October 2013 20:34, Nicolas Pitre wrote: > > On Fri, 4 Oct 2013, Will Deacon wrote: > [...] > >> > >> Why do you consider it unsuitable to ship the perl script with the kernel? > >> Perl 5 is already documented as a build dependency in Documentation/Changes > > > > Do you have an example of something that does require perl to build the > > kernel on ARM? I was under the impression that people try to avoid it > > as much as possible in general. > > > > I'm personally sitting on the fence between effectively adding a new > > kernel build dependencies or carrying the output of the perl script. > > But if the kernel build does already require perl in practice then this > > might tip the balance. > > > > I like Russell's suggestion the most, in fact. Me too. > In this case, the build time requirement for Perl effectively gets > suspended until you start making modifications to the perl script, and > the relation between the .S and the .pl files is made explicit by the > make rule. > > Should I put the cmd_perl rule in scripts/Makefile.build ? Or can I > just keep it under arch/arm/crypto ? To avoid possible gag reactions from the wider community I'd suggest you keep it local for now. Unless there are already such perl usage elsewhere in the tree in which case abstracting it to scripts/Makefile.build first is recommended. Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] ARM: NEON based fast(er) AES in CBC/CTR/XTS modes
On Fri, 4 Oct 2013, Russell King - ARM Linux wrote: > On Fri, Oct 04, 2013 at 08:41:35PM +0200, Ard Biesheuvel wrote: > > On 4 October 2013 20:34, Nicolas Pitre wrote: > > > On Fri, 4 Oct 2013, Will Deacon wrote: > > [...] > > >> > > >> Why do you consider it unsuitable to ship the perl script with the > > >> kernel? > > >> Perl 5 is already documented as a build dependency in > > >> Documentation/Changes > > > > > > Do you have an example of something that does require perl to build the > > > kernel on ARM? I was under the impression that people try to avoid it > > > as much as possible in general. > > > > > > I'm personally sitting on the fence between effectively adding a new > > > kernel build dependencies or carrying the output of the perl script. > > > But if the kernel build does already require perl in practice then this > > > might tip the balance. > > > > > > > I like Russell's suggestion the most, in fact. In this case, the build > > time requirement for Perl effectively gets suspended until you start > > making modifications to the perl script, and the relation between the > > .S and the .pl files is made explicit by the make rule. > > > > Should I put the cmd_perl rule in scripts/Makefile.build ? Or can I > > just keep it under arch/arm/crypto ? > > Just running through the Makefiles, it seems we have a fair amount > of stuff already using perl in various ways. So I wouldn't worry too > much about where it's placed. It's probably something that should > eventually end up in scripts/ at _some_ point. BTW, Russell's opinion has precedence over what I just said. Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
