[U-Boot] [PATCH v4] socfpga: Add socfpga preloader signing to mkimage

[Charles Manning]

Hello Wolfgang

I have some further observations to my last email...

Your input would be vastly appreciated.

Please see below.


On Tue, Feb 25, 2014 at 8:18 AM, Charles Manning <cdhmanning at gmail.com>wrote:

> Hello Wolfgang
> On Monday 24 February 2014 19:48:36 Wolfgang Denk wrote:
> > Dear Charles,
> >
> > In message <1393194939-29786-1-git-send-email-cdhmanning at gmail.com> you
> wrote:
> > > Like many platforms, the Altera socfpga platform requires that the
> > > preloader be "signed" in a certain way or the built-in boot ROM will
> > > not boot the code.
> > >
> > > This change automatically creates an appropriately signed preloader
> > > from an SPL image.
> > >
> > > Signed-off-by: Charles Manning <cdhmanning at gmail.com>
> > > ---
> > >
> > > Changes for v3:
> > >  - Fix some coding style issues.
> > >  - Move from a standalone tool to the mkimgae framework.
> > >
> > > Changes for v4:
> > >  - Fix more coding style issues.
> > >  - Fix typos in Makefile.
> > >  - Rebase on master (previous version was not on master, but on a
> > >    working socfpga branch).
> >
> > There may be perfectly valid reasons why you might decide to ingore a
> > review comments - sch comments may be wrong, too, after all.  But in
> > such a case it is always a good idea to provide feedback to the
> > reviewer why you decided not to follow his advice.  Otherwise he might
> > think you just missed or ignored the comment.
>
> I am sorry, I must have missed some of the comments. I have intended to
> implement them all, except one by Gerhard.
>
> >
> > And this is what is happeneing (again) in your patch...
> >
> > > diff --git a/spl/Makefile b/spl/Makefile
> > > index bf98024..90faaa6 100644
> > > --- a/spl/Makefile
> > > +++ b/spl/Makefile
> > > @@ -181,6 +181,14 @@ $(objtree)/SPL : $(obj)/u-boot-spl.bin
> > >
> > >  ALL-y      += $(obj)/$(SPL_BIN).bin
> > >
> > > +$(OBJTREE)/socfpga-signed-preloader.bin: $(obj)/u-boot-spl.bin
> > > +   $(OBJTREE)/tools/mkimage -T socfpgaimage -d $< $@
> > > +
> > > +
> >
> > One blank line is sufficient.
>
> Ok, a blank line. I can delete that.
>
> >
> > I asked before:  "socfpga-signed-preloader.bin" is a terribly long
> > name.  Can we find a better one?
> >
> > > +ifdef CONFIG_SOCFPGA
> > > +ALL-y += $(OBJTREE)/socfpga-signed-preloader.bin
> > > +endif
> >
> > I asked before: Can we not use "ALL-$(CONFIG_SOCFPGA)" and avoid the
> > ifdef ?
>

> I am sorry, I had developed this code in a different (older) branch where
> socfpga actually works. It is broken in master.
>
> This I shall fix.
>

I can certainly avoid the ifdef, but there is already another one three
lines down
for the SAMSUNG case:

 ifdef CONFIG_SOCFPGA
ALL-y += $(OBJTREE)/socfpga-signed-preloader.bin
endif

ifdef CONFIG_SAMSUNG
ALL-y    += $(obj)/$(BOARD)-spl.bin
endif

It seems odd to me that you would want different conventions in the same
Makefile,
but if that is what you want then that is what you will get.



>
> >
> > > + * Reference doc
> > > http://www.altera.com.cn/literature/hb/cyclone-v/cv_5400A.pdf + * Note
> > > this doc is not entirely accurate.
> >
> > I aseked before: Would you care to explain the errors in the document
> > that might cause problems to the reader?
> >
> > Noting that something contains errors without mentioning what these
> > are is really not helpful.
>
> Ok, I shall mention the errors pertinent to the code. Other errors I shall
> ignore.
>
> >
> > > + * This uses the CRC32 calc out of the well known Apple
> > > + * crc32.c code. CRC32 algorithms do not produce the same results.
> > > + * We need this one. Sorry about the coade bloat.
> >
> > Both Gerhard and me asked before: Why exactly do we need another
> > implementation of CRC32.  We already have some - why cannot we use
> > these?
>
> Well I would have thought that obvious from the comment I put in the code,
> but
> email is an imperfect communications medium, so I shall explain in further
> detail here.
>
> As I am sure you are aware, there are many different crc32 calculations.
> Indeed Wikipedia lists 5 and there are most likely others.
>
> Even when they use the same polynomial, they give differences when you
> stuff
> the bits through the shift register lsb-first or msb-first.
>
> Now from what I see looking through the u-boot lib/ directory u-boot
> provides
> just one crc32 version - Adler (and a bit flipped version thereof for use
> by
> jffs2). If there are others I did not see them.
>
> Now as I expect you are aware, the purpose of a signer is to massage the
> code
> so that it is in a form that the boot ROM accepts. One of those criteria is
> that the crc in the code matches the crc the boot ROM is expecting. If not,
> the boot ROM refuses to execute the code.
>
> It would be nice to use the Adler code. Indeed this is my favourite crc.
> However, this is not what the boot ROM wants.
>
> The boot ROM does not enter into rational discussions, so we must,
> unfortunately, bow to its whims. If it wants a different CRC calculation
> then
> we must supply it.
>
> I did much experimentation to find the crc that worked. I tried the zlib
> crc -
> no luck.  I tried different many things until I found something that
> worked.
>

The actual table values I am using are also found in lib/bzlib_crctable.c

This is, however, not exposed but is a private thing within bzlib.

The best choice would possibly be to expose this with a new crc function,
but that means dragging bzlib (or parts thereof) into mkimage or at least
putting "hooks" into bzlib that were never intended to be there.

The alternative is to maybe just add a new alt_crc.c to lib.


>
> >
> > > + * Copyright for the CRC code:
> > > + *
> > > + * COPYRIGHT (C) 1986 Gary S. Brown.  You may use this program, or
> > > + *  code or tables extracted from it, as desired without restriction.
> >
> > I asked before:  Please provide _exact_ reference. See
> >
> http://www.denx.de/wiki/view/U-Boot/Patches#Attributing_Code_Copyrights_Sig
> >n for instructions how to do this.
>
> As it was, I had attributed this incorrectly. This is a file I generated
> myself, though I had used this as a reference during my research. The
> values
> will look the same as some other code floating out there, but that is
> because
> that is the way things have to be.
>
> I shall fix this wen I make another file.
>
>
> >
> > I also commented before: If you really need this copied code (i. e.
> > you canot use one of the already existing implementations of CRC32 -
> > which I seriously doubt), then better move this into a separate file,
> > and assign a separate license ID tag for it.
>
> You say "already exiting implementations". I see only one: lib/crc32.c.
> Perhaps I am not looking in the right place?
>
> I shall split this code out and call it alt_crc32 and put it in lib with
> one
> of those proxy files in tools.
>
> >
> >
> > I also asked this before: I cannot find a license ID tag in your new
> > code. Please add.
>
> Ok.
>
>
> >
> > > +/* To work in with the mkimage framework, we do some ugly stuff...
> > > + *
> > > + * First we prepend a fake header big enough to make the file 64k.
> > > + * When set_header is called, we fix this up by moving the image
> > > + * around in the buffer.
> > > + */
> >
> > Also asked before: Incorrect multiline comment style. Please fix
> > globally.
>
> Ok.
>
> I am sorry I missed some of your comments earlier.
>
> Regards
>
> Charles
>