[PATCH v4 0/5] Add support for CRC64 generation in linker

[Ulf Samuelsson]

Den 2023-02-21 kl. 13:02, skrev Nick Clifton:
> Hi Ulf,
>
>> Fourth patchset.
>> Get rid of binaries in testsuite.
>
> Thanks.
>
> I do wonder if it would be possible to simplify some of the extensions
> to the linker script syntax, in order to make it easier to use and to
> allow for future extensions.  For example:
>
>   DIGEST "algorithm" ( start_symbol,  end_symbol )

Yes, I am open to suggestions.

>
> So "algorithm" could be "CRC" or "CRC32" or "CRC64" or "CRC64-WE" or
> "CRC TABLE" or any of the other algorithms/features that you want to
> support[1].  And more could be added in the future without having to
> update the linker script syntax.
>
CRC TABLE is different from the other, since it generates a CRC table
and the others generate the checksum.

> If the algorithm needs extra data apart from the start and end symbols
> then this could be included in the name, eg:
>
>   DIGEST "CRC64-POLY-0xDEADBEEFDEADBEEF" (ecc_start , ecc_end)

I think it make the code a little more complex and thus higher cost of 
maintenance.

Then you have to parse the name in the code, and it seems easier to let 
'bison' do the job.

Then you would get.


DIGEST "<algo>"        (ecc_start , ecc_end)

               if (get_crc_algo($2, &size, &polynom)) {

                     lang_add_crc_syndrome (false, size, polynome);

               } else if (get_sha_algo(&size)) {

                     lang_add_sha(size);

               }

Supported algo = { CRC64-ECMA, CRC64-ISO, CRC64-WE, CRC32 } right now.

DIGEST  POLY (0xDEADBEEFDEADBEEF, <size>)   (ecc_start , ecc_end)

                     lang_add_crc64_syndrome (false, $6, $3);

DIGEST  POLYI (0xDEADBEEFDEADBEEF, <size>)   (ecc_start , ecc_end)

                     lang_add_crc64_syndrome (true, $6, $3);

DIGEST TABLE

		  lang_add_assignment (exp_assign (CRC64_TABLE, exp_nameop (NAME,"."), false));
		  lang_add_table (); /* A generic version requires that the table is after the DIGEST */

> I would also suggest that functionality should not be restricted to
> just the .text section, but that it should work for any section. (It
> is reasonable to restrict it to only working inside a single section
> however).

I am not as well educated on different sections in an image except as 
you guys,
so I thought restricting things to the ".text" section should work.

The question is how to know which section to parse.

Is there a way to tell which section a symbol is located in?

Then no directive is needed.

You just get the section of CRC_ADDRESS.

Otherwise, there needs to be a way of specifying the section.


>
>> An algorithm typically needs
>> <type> *init_<algo>_tab( <type> poly )
>> <type> calc_<algo>_inv( const unsigned char *input_str, size_t 
>> num_bytes );
>> <type> calc_<algo>    ( const unsigned char *input_str, size_t 
>> num_bytes );
>> void lang_add_<algo>_syndrome(bool invert, bfd_vma poly)
>> void lang_add_<algo>_table(void)
>
> Since there are going to be several different versions of these 
> functions,
> and possibly more to come in the future, I think that it would be best to
> put them into a new file. eg lddigest.c  This might also make it 
> easier for
> other linkers to make use of the code, should they decide to implement a
> similar feature.
>
Yes, that could be done.

>
> I am going to post some individual reviews of the patches as well, but I
> wanted to give my overall impressions here.
>
The patches need additional testing, so this is for review of the 
concept only.
I checked that the tables generated by CRC64 TABLE and CRC32 TABLE
are the same.

One bug I found is the crc32_invert variable which should be crc_invert,
the same as used for crc64.


> Cheers
>   Nick
>
> [1] Well except for TIMESTAMP and DEBUG ON/OFF which are valid new
> features in their own rights.
>
I find them quite useful.
I have one more addition which I would like to add, and that is knowledge
about flash sectors.

I would like to tell the linker ALIGN to the end of the current flash 
sector.
Those are chip dependent, so the linker needs to know the flash organisation
of the specific chip for this to work.

=============================

BTW, I did some calculation of code size.

The initial patch had 350 lines of code, where about 100 are the core 
CRC64 routines.
The CRC calculation is low complexity code which fetches the text section
and calculates the CRC over an array of characters.

The ielftool application which can insert a CRC into an ELF file is 
11-12,000 lines of C code
that needs to understand the ELF format and all sectors generated.
If someone "invents" a new sector, or otherwise changes the ELF file, 
then the tool needs to be updated.
That is difficult to support.

This is the case for ALL file formats and all variants.

If there is just one such tool per file format, then with a dozen file 
formats,
there is a need to maintain 130,000-150,000 lines of code.

Now, since this tool is not standardized, multiple companies need to 
maintain their own
version, so these 350 lines (or whatever it will end up in) saves 
millions of lines of code worldwide.

Best Regards
Ulf Samuelsson