Re: [XEN PATCH 2/2] x86/Dom0: Use streaming decompression for ZSTD compressed kernels

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.05.2023 02:18, Rafaël Kooi wrote:
> On Arch Linux kernel decompression will fail when Xen has been unified
> with the kernel and initramfs as a single binary. This change works for
> both streaming and non-streaming ZSTD content.

This could do with better explaining what "unified" means, and how
streaming decompression actually makes a difference.

> --- a/xen/common/decompress.c
> +++ b/xen/common/decompress.c
> @@ -3,11 +3,26 @@
>  #include <xen/string.h>
>  #include <xen/decompress.h>
>  
> +typedef struct _ZSTD_state
> +{
> +    void *write_buf;
> +    unsigned int write_pos;
> +} ZSTD_state;
> +
>  static void __init cf_check error(const char *msg)
>  {
>      printk("%s\n", msg);
>  }
>  
> +static int __init cf_check ZSTD_flush(void *buf, unsigned int pos,
> +                                      void *userptr)
> +{
> +    ZSTD_state *state = (ZSTD_state*)userptr;
> +    memcpy(state->write_buf + state->write_pos, buf, pos);
> +    state->write_pos += pos;
> +    return pos;
> +}

This doesn't really belong here, but will (I expect) go away anyway once
you drop the earlier patch.

> @@ -17,22 +32,32 @@ int __init decompress(void *inbuf, unsigned int len, void 
> *outbuf)
>  #endif
>  
>      if ( len >= 3 && !memcmp(inbuf, "\x42\x5a\x68", 3) )
> -        return bunzip2(inbuf, len, NULL, NULL, outbuf, NULL, error);
> +        return bunzip2(inbuf, len, NULL, NULL, outbuf, NULL, error, NULL);
>  
>      if ( len >= 6 && !memcmp(inbuf, "\3757zXZ", 6) )
> -        return unxz(inbuf, len, NULL, NULL, outbuf, NULL, error);
> +        return unxz(inbuf, len, NULL, NULL, outbuf, NULL, error, NULL);
>  
>      if ( len >= 2 && !memcmp(inbuf, "\135\000", 2) )
> -        return unlzma(inbuf, len, NULL, NULL, outbuf, NULL, error);
> +        return unlzma(inbuf, len, NULL, NULL, outbuf, NULL, error, NULL);
>  
>      if ( len >= 5 && !memcmp(inbuf, "\x89LZO", 5) )
> -        return unlzo(inbuf, len, NULL, NULL, outbuf, NULL, error);
> +        return unlzo(inbuf, len, NULL, NULL, outbuf, NULL, error, NULL);
>  
>      if ( len >= 2 && !memcmp(inbuf, "\x02\x21", 2) )
> -     return unlz4(inbuf, len, NULL, NULL, outbuf, NULL, error);
> +        return unlz4(inbuf, len, NULL, NULL, outbuf, NULL, error, NULL);

This also looks wrong here - if the earlier patch was to be kept, I expect
all these adjustments would have to move there. Otherwise build would be
broken when just the 1st patch is in place.

>      if ( len >= 4 && !memcmp(inbuf, "\x28\xb5\x2f\xfd", 4) )
> -     return unzstd(inbuf, len, NULL, NULL, outbuf, NULL, error);
> +    {
> +        // NOTE (Rafaël): On Arch Linux the kernel is compressed in a way
> +        // that requires streaming ZSTD decompression. Otherwise 
> decompression
> +        // will fail when using a unified EFI binary. Somehow decompression
> +        // works when not using a unified EFI binary, I suspect this is the
> +        // kernel self decompressing. Or there is a code path that I am not
> +        // aware of that takes care of the use case properly.

Along the lines of what I've said for the description, this wants to avoid
terms like "somehow" if at all possible.

We also don't normally put our names in such comments.

Finally please see ./CODING_STYLE for how we expect comments to be
formatted.

Jan