image: Add support for ZSTD decompression

On 08/06/2020 21.01, Robert Marko wrote:
> On Wed, May 20, 2020 at 2:35 PM Tom Rini <trini at konsulko.com> wrote:
>>
>> On Wed, May 20, 2020 at 01:38:01PM +0200, Robert Marko wrote:
>>
>>> Tom,
>>> I have tried various things but CONFIG_IS_ENABLED won't work inside of
>>> switch case.
>>> It works fine outside of if though.
>>
>> OK, thanks, I'll poke things more to make sure what I'm worried about
>> doesn't happen.
> 
> Hi,
> were you able to test the commit?

So I took a look at this since I'm also interested in getting zstd
support in. And the problem is that when building host tools, the
IS_ENABLED / CONFIG_IS_ENABLED logic doesn't make much sense anyway -
one can of course include the kconfig.h header that defines those macros
to avoid the cpp error (one has to be careful, because the headers
included vary wildly depending on USE_HOSTCC), but I don't think it's
meaningful - the host tools should not depend on configuration for any
specific board.

Nor do they currently; the use of ifdef CONFIG_GZIP for example just
means that that part is ignored for the host tools - doing

                uint size = unc_len;

should be a no-op when CONFIG_GZIP=y, but it breaks the build of the
host tools.

That, in turn, means that the fit_check_sign tool (which AFAICT is the
only host tool that actually care about this - we really should build
host tools with the
-ffunction-sections,-fdata-sections,-Wl,--gc-sections flags) currently
"works" in the sense that it does check the signature, but it doesn't
really check if the image(s) can be decompressed using the vendored copy
of the decompression code; hidden in the output is

Unimplemented compression type 5

but for some reason that ends up being ok.

So, for now, I suggest simply surrounding all the CASE_IH_COMP_* (except
NONE) by a #ifndef USE_HOSTCC - I don't think that will change anything
at all for the existing host tools. Then you should be able to use the
proper CONFIG_IS_ENABLED(ZSTD) to guard the IH_COMP_ZSTD case. Also, it
would allow us to change the existing bare ifdefs to be SPL-aware, i.e.
change #ifdef CONFIG_GZIP to #if CONFIG_IS_ENABLED(GZIP) [assuming the
SPL_GZIP symbol actually exists].

Rasmus

On Wed, Jun 10, 2020 at 11:13:37AM +0200, Rasmus Villemoes wrote:
> On 08/06/2020 21.01, Robert Marko wrote:
> > On Wed, May 20, 2020 at 2:35 PM Tom Rini <trini at konsulko.com> wrote:
> >>
> >> On Wed, May 20, 2020 at 01:38:01PM +0200, Robert Marko wrote:
> >>
> >>> Tom,
> >>> I have tried various things but CONFIG_IS_ENABLED won't work inside of
> >>> switch case.
> >>> It works fine outside of if though.
> >>
> >> OK, thanks, I'll poke things more to make sure what I'm worried about
> >> doesn't happen.
> > 
> > Hi,
> > were you able to test the commit?
> 
> So I took a look at this since I'm also interested in getting zstd
> support in. And the problem is that when building host tools, the
> IS_ENABLED / CONFIG_IS_ENABLED logic doesn't make much sense anyway -
> one can of course include the kconfig.h header that defines those macros
> to avoid the cpp error (one has to be careful, because the headers
> included vary wildly depending on USE_HOSTCC), but I don't think it's
> meaningful - the host tools should not depend on configuration for any
> specific board.
> 
> Nor do they currently; the use of ifdef CONFIG_GZIP for example just
> means that that part is ignored for the host tools - doing
> 
> --- a/common/image.c
> +++ b/common/image.c
> @@ -430,12 +430,12 @@ int image_decomp(int comp, ulong load, ulong
> image_start, int type,
>                 else
>                         ret = -ENOSPC;
>                 break;
> -#ifdef CONFIG_GZIP
> +//#ifdef CONFIG_GZIP
>         case IH_COMP_GZIP: {
>                 ret = gunzip(load_buf, unc_len, image_buf, &image_len);
>                 break;
>         }
> -#endif /* CONFIG_GZIP */
> +//#endif /* CONFIG_GZIP */
>  #ifdef CONFIG_BZIP2
>         case IH_COMP_BZIP2: {
>                 uint size = unc_len;
> 
> should be a no-op when CONFIG_GZIP=y, but it breaks the build of the
> host tools.
> 
> That, in turn, means that the fit_check_sign tool (which AFAICT is the
> only host tool that actually care about this - we really should build
> host tools with the
> -ffunction-sections,-fdata-sections,-Wl,--gc-sections flags) currently
> "works" in the sense that it does check the signature, but it doesn't
> really check if the image(s) can be decompressed using the vendored copy
> of the decompression code; hidden in the output is
> 
> Unimplemented compression type 5
> 
> but for some reason that ends up being ok.
> 
> So, for now, I suggest simply surrounding all the CASE_IH_COMP_* (except
> NONE) by a #ifndef USE_HOSTCC - I don't think that will change anything
> at all for the existing host tools. Then you should be able to use the
> proper CONFIG_IS_ENABLED(ZSTD) to guard the IH_COMP_ZSTD case. Also, it
> would allow us to change the existing bare ifdefs to be SPL-aware, i.e.
> change #ifdef CONFIG_GZIP to #if CONFIG_IS_ENABLED(GZIP) [assuming the
> SPL_GZIP symbol actually exists].

Alright, so first thanks for digging here.  Looking over the tools, for
verifying the signature we don't need to decompress the contents as we
are just checking the signature.  For dumpimage it's also OK to dump a
compressed file and leave it to regular tools to decompress.  Do you
want to make a patch to guard the existing section and CONFIG_IS_ENABLED
it or should I and Suggested-by you?  Thanks!