[ Cc: to Masahiro for the tools/ "vs" lib/ build support part ]
On Mon, Mar 10, 2014 at 16:04 +1300, Charles Manning wrote:
On Sunday 09 March 2014 05:51:23 Gerhard Sittig wrote:
On Thu, Mar 06, 2014 at 15:40 +1300, Charles Manning wrote:
--- /dev/null +++ b/lib/bzlib_crc32.c @@ -0,0 +1,26 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@gmail.com
- SPDX-License-Identifier: GPL-2.0+
- This provides a CRC-32 using the tables in bzlib_crctable.c
- */
+#include <bzlib_crc32.h> +#include "bzlib_private.h"
+uint32_t bzlib_crc32(uint32_t crc, const void *_buf, int length) +{
- const uint8_t *buf = _buf;
- crc ^= ~0;
- while (length--) {
crc = (crc << 8) ^ BZ2_crc32Table[((crc >> 24) ^ *buf) & 0xff];buf++;- }
- crc ^= ~0;
- return crc;
+}
Since you already include the private bzlib header for the BZ2_crc32Table[] declaration, you might as well use the BZ_INITIALISE_CRC(), BZ_FINALISE_CRC(), and BZ_UPDATE_CRC() macros declared from there (right next to the table's declaration), instead of re-inventing how the CRC gets calculated.
If you think that makes it more clear I shall do that. I don't think though that it really is any clearer.
Admittedly your wrapper is thin. Yet it duplicates what the bzlib header already provides. And does so in different phrases (i.e. textually does not match the original, so probably gets missed upon searches).
It's more a matter of consistency to re-use the code as well as the table of the bzip2 implementation, instead of re-inventing both or using parts of it only.
You probably should determine whether you want to provide one routine to do the complete calculate over a given byte stream, without any "previous" CRC value -- this is what your current use case is. Or whether you want to provide three primitives to initialize, update, and finalize a CRC (which is not used yet). Or whether you want to provide the three primitives plus one convenience wrapper.
I try to use a single "familiar" function, like crc32() does.
You can start with 0, then use it with the results.
eg
crc = crc32(0, first_buff,...); crc = crc32(crc, second_bufer,...);
This only works "by coincidence" because the final XOR and the initial value happen to be complimentary and only by chance the value of zero "works". So it's not a general pattern, but a specific feature of this very implementation.
Given that your current application only determines CRC values for complete buffers and nothing is done piecemeal, I'd suggest to provide the one convenience wrapper routine which does not take a previous partial result.
Should other use cases later require incremental calculation, they can introduce and use the three primitives to open, iterate and close the calculation. This better reflects what's happening. Or am I being overly picky?
--- a/tools/Makefile +++ b/tools/Makefile @@ -70,6 +70,8 @@ RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := rsa-sign.o # common objs for dumpimage and mkimage dumpimage-mkimage-objs := aisimage.o \ $(FIT_SIG_OBJS-y) \
bzlib_crc32.o \bzlib_crctable.o \ crc32.o \ default_image.o \ fit_image.o \@@ -85,6 +87,7 @@ dumpimage-mkimage-objs := aisimage.o \ os_support.o \ pblimage.o \ sha1.o \
socfpgaimage.o \ ublimage.o \ $(LIBFDT_OBJS) \ $(RSA_OBJS-y)diff --git a/tools/bzlib_crc32.c b/tools/bzlib_crc32.c new file mode 100644 index 0000000..b7f7aa5 --- /dev/null +++ b/tools/bzlib_crc32.c @@ -0,0 +1 @@ +#include "../lib/bzlib_crc32.c" diff --git a/tools/bzlib_crctable.c b/tools/bzlib_crctable.c new file mode 100644 index 0000000..53d38ef --- /dev/null +++ b/tools/bzlib_crctable.c @@ -0,0 +1 @@ +#include "../bzlib_crctable.c" diff --git a/tools/bzlib_private.h b/tools/bzlib_private.h new file mode 100644 index 0000000..cfb74be --- /dev/null +++ b/tools/bzlib_private.h @@ -0,0 +1 @@ +#include "lib/bzlib_private.h"
Now this is incredibly ugly. You are duplicating lib/ stuff in tools/ and build it there as well? Or not at all build it in lib/ where the code actually resides?
It's only a side note that #include for "*.c" is problematic as some compilers do semi-intelligent stuff when the filename does not end in ".h" and then break compilation, so this hack should neither get spread nor suggested for use.
I absolutely agree this is very, very, ugly.
I was just following the current way of doing things. This is how crc32.c, sha1.c, md5.c, fdt.c and many others are handled.
I expect that this is worth a refactorisation effort, but I think that should be a separate exercise from doing this signer.
Is there any (real, technical) reason why the bzip stuff (the CRC-32 calculation that has been made available separately) cannot get built and used as a library, and the tools/ application just gets linked against it as one would expect?
From my limited understanding, lib/ is not built for the host. It is only built for the target. That is why we have all these ugly C files: crc32.c, sha1.c,... in tools/.
With Kbuild, is the '#include "../otherdir/source.c" trick still needed? Would a list of object files with relative path specs work, or can object files in one output directory result from compile units taken out of several source directories?
Can the tools/ build for the host environment use a library that gets built from lib/ sources?
That an existing implementation uses similar hacks need not mean that it must be OK, or cannot get improved. :)
--- /dev/null +++ b/tools/socfpgaimage.c @@ -0,0 +1,255 @@ [ ... ]
- "Header" is a structure of the following format.
- this is positioned at 0x40.
- Endian is LSB.
- Offset Length Usage
- 0x40 4 Validation word 0x31305341
- 0x44 1 Version (whatever, zero is fine)
- 0x45 1 Flags (unused, zero is fine)
- 0x46 2 Length (in units of u32, including the end
checksum). + * 0x48 2 Zero
- 0x4A 2 Checksum over the heder. NB Not CRC32
This is wrong (and is not what the Handbook Appendix says). The length is 32bits in width. It just happens that in little endian representation and with a maximum of 60KB of payload data, the upper 16bits naturally remain zero. Still the documentation and implementation should work with a 32bit length.
The implementation surely only has to work with the ROM. There is no point in it working for arbitrary cases.
Still it's a deviation from the documentation and obfuscates what's happening. Think of the next person to read and manipulate the code, and find a "32bits length spec" in a 16bit field. That's unexpected, and not necessary. (or is it?)
I still feel that the "length" spec is both, a length in units of 32bit words, as well as a 32bit value. This is what Altera documents in Appendix A.
I agree that the term "checksum" might be confusing, because it's a mere sum of the preceeding bytes, and "by coincidence" the sum is used to check plausibility of the header. I wouldn't either know how to best put this into a short comment. From personal experience I can tell that "sum" alone would have helped a lot.
What is a checksum but a sum that is used for checking? The term checksum on its own does not imply any particular algorithm. If you think sum is better, I shall use that.
Well, the term "checksum" usually is associated with CRCs, in contrast to mere sums. At least that's what triggered in my head. But I'm happy to learn how others feel about it. I may be wrong, and am happy to improve where possible.
Background information: The SPL is supposed to be up to 60KB in size (the upper 4KB are for the ROM loader's internal use, and for data shared among the preloader and the bootloader). The SPL memory image has a gap at 0x40 (after the vectors, and before the "useful code") where the preloader header/signature gets put into. After the signature application, a CRC-32 is calculated over the complete data and gets appended to the data (the length spec in the header includes the CRC location). Then the image gets padded to full 64KB and gets duplicated four times (to increase robustness by means of redundancy, assuming boot media might have read errors).
Is 60K a hard limitation for SPL? I know that people are considering SPLs of larger size. Yeah... I know... what happened to 4k bootloaders?
The 60K value is neither a limitation in the U-Boot project nor specific to the SPL approach. It's the limit of the SoC's ROM loader (OCRAM minus reserved data area).
The above "the SPL is supposed to be up to 60KB in size" is a specific statement for the Altera SoCFPGA preloader case. I thought you should have recognized that number, having worked on the preloader for some time. :)
Your submission's motivation is to initially stamp a preloader that's the fresh result of a compilation. I can see that it would be useful and desirable to apply the tool to existing binaries as well, i.e. throw a 64KB binary at the tool to re-create the signature.
I am confused by what you are saying here. There is only one point to this: sign a preloader so that a SocFPGA boot ROM will use it. There are no other use cases that make any sense.
Or put another way... would I run the omap signer on an arbitrary binary? No. Same argument here.
By "signing existing binaries" do you mean existing preloaders? If so, people can to that with mkimage -T socfpgaimage ... (once this addition is in place).
Upon re-consideration the idea turned out to be not of much benefit. I said that, can't tell whether I should have gone and removed all remarks about it instead. Just forget I said something. :)
- Note that the CRC used here is **not** the zlib/Adler crc32. It is
the + * CRC-32 used in bzip2, ethernet and elsewhere.
If you want to be most specific, cite the parameters of the CRC-32 used here. See the above comments and http://opensource.apple.com/source/CommonCrypto/CommonCrypto-60049/libcn/cr c32-bzip2.c and maybe http://www.ross.net/crc/download/crc_v3.txt
BTW I'm still under the impression that zlib uses the same polynomial as well as identical initial values and final XOR masks (see lib/crc32.c). So where is the difference between the zlib and the bzlib CRC-32 parameters?
$ perl -e 'use Compress::Zlib; printf "%x\n", crc32("123456789");' cbf43926
From my understanding zlib crc32() == Adler.
Please compare the tables in lib/crc32.c and lib/bzlib_crc32.c. They are not the same.
I have run many tests.
I do believe that they are not the same. Yet available data suggests that both use identical polynomial, although resulting in different values for the same input. So something else must be different. That's what I said. No conflict here. I wasn't saying that zlib must be used, I was wondering why there would be a difference.
I considered the "serializer" accessors of previous patch iterations most appropriate (except for their names), and feel this approach with a struct is inferior (especially because it doesn't cover the CRC later). But others need not agree.
I too prefer the older serialisers but was told I could not use my own. Yes, there are some in target space: put_aligned_le16() etc, but from my understanding it is unhealthy to use target space functions from host space (mkimage etc).
There may be responses of different severity, there are several grades from questions to recommendations to strong suggestions to total NAKs.
I understood that you were asked to look into whether there is something you can re-use, while experience suggests that there should be something along those lines.
Still I guess that the suggestion was not a hard and fast rule. Investigating and telling "does not apply" is as valid a response to these requests as changing the submission -- if there is necessity or reason. We need not shoehorn code into a specific approach which does not apply or does not fit the specific purpose. This kind of feedback is mostly about trying to blend in and remaining aware of when something is a workaround (at least I got that much). Code should not only work, but it's as important that it's readable.
+/*
- The header checksum is just a very simple checksum over
- the header area.
- There is still a crc32 over the whole lot.
- */
+static uint16_t hdr_checksum(const uint8_t *buf, int len) +{
- uint16_t ret = 0;
- int i;
- for (i = 0; i < len; i++) {
ret += (((uint16_t) *buf) & 0xff);buf++;- }
- return ret;
+}
What is the typecast good for? Why are there still whitespace issues? Can you either fix them or tell why this is OK? I.e. can you _somehow_ respond to feedback you receive? Ignoring review comments makes you recive less feedback later.
Ok, I will do away with the cast. It does not serve any function.
I am unsure what whitespace issue do you mean? Does checkpatch not catch things like this?
You see the multiple empty lines after the function, don't you? Even if not strictly required, you might want to use "--strict" with checkpatch as well. It may sound picky, but this kind of stuff is not only cosmetics, but in addition suggests how much attention is put to details, and how much care is taken in iterations of a submission.
The usual C language style comments apply: Please don't mix declarations and instructions.
What do you mean by "declaration and instructions"? Do you mean the initial value mixing?
You can eliminate variables. Use names which better reflect what is happening. Something like this is more idiomatic:
static uint16_t header_sum(const uint8_t *buf, int len) { uint16_t sum;
sum = 0; while (len-- > 0) { sum += *buf++; } return sum; }
Ok I will do that.
Maybe this helps: "uint16_t sum;" is a declaration. "sum = 0;" is an instruction. "uint16_t sum = 0;" is a declaration that is combined with an instruction. It's useful to a certain amount, but can turn into obfuscation when used excessively. It saves almost nothing (just typing the variable name once), but can be costly in terms of bugs and maintenance. In the specific case above, pre-setting the sum and adding the items unnecessarily is several lines apart with other declarations between them. It's easy to miss that there is more code involved than one might perceive at first glance. The effect gets stronger with code that is not as trivial.
+/* Sign the buffer and return the signed buffer size */ +static int sign_buffer(uint8_t *buf,
uint8_t version, uint8_t flags,int len, int pad_64k)+{
- uint32_t calc_crc;
- /* Align the length up */
- len = (len + 3) & (~3);
Is there nothing you can re-use? If not can you introduce something generic to roundup() for user space tools to use?
Sure I define something for reuse...
Can you suggest where I might put it?
Can you rephrase the condition such that it becomes evident you are doing ROUNDUP(len, sizeof(uint32_t)) and avoid the magic "3" that has no such meaning for readers?
U-Boot apparently has a roundup() macro, both in the bootloader as well as in user space (mxsimage.c uses it). Did you look it up? `git grep -i roundup`
- if (!pad_64k)
return len + 4;- return PADDED_SIZE;
+}
+static void socfpgaimage_print_header(const void *ptr) +{
- if (verify_buffer(ptr) == 0)
printf("Looks like a sane SOCFPGA preloader\n");- else
printf("Not a sane SOCFPGA preloader\n");+}
Would it be useful to print a little more information than "it's a preloader" without any further details? I'm not familiar with the context this routine gets called in, what do users expect?
It gets called at the end of making an image.
Ok, I will print out some numbers too. They are probably of little use to anyone, but they look "technical" :-).
At least the size might be nice to have. Checksums less so. It's a judgement call. Haven't checked what other formats do (except having seen U-Boot's native format several hundred times). Just had to ask.
That the print routine gets called at the end of image creation does not mean that it's the only use, BTW. Somebody may want to run -l interactively or from tools/scripts.
virtually yours Gerhard Sittig