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

[Jan Beulich <jbeulich@xxxxxxxx>]

First of all - please don't drop Cc-s when replying. I'm restoring
xen-devel@ here at least.

On 10.05.2023 10:51, Rafaël Kooi wrote:
> On 10/05/2023 10:03, Jan Beulich wrote:> 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.
>>
> 
> I don't mind explaining it further, but with the efi documentation for
> it existing on xenbits, should I just refer to that?

You may of course refer to existing documentation. Iirc that doesn't
cover any compression aspects, though.

>>> --- 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.
>>
> 
> The ZSTD_flush will have to stay, as that is how the decompressor will
> start streaming decompression. The difference will be that the book
> keeping will be "global" (to the translation unit).

But this bookkeeping should be entirely in zstd code (i.e. presumably
unzstd.c).

>>>       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.
> 
> I've used the term "somehow" because I don't know why decompression
> works when Xen loads the kernel from the EFI file system. I assume the
> kernel still gets unpacked by Xen, right? Or does the kernel unpack
> itself?

The handling of Dom0 kernel decompression ought to be entirely independent
of EFI vs legacy. Unless I'm wrong with that (mis-remembering), you
mentioning EFI is potentially misleading. And yes, at least on x86 the
kernel is decompressed by Xen (by peeking into the supplied bzImage). The
difference between a plain bzImage and a "unified EFI binary" is what you
will want to outline in the description (and at least mention in the
comment). What I'm wondering is whether there simply is an issue with size
determination when the kernel is taken from the .kernel section.

> When I present the v2 of this patch, do I add you as a reviewer? Or will
> that be done by the merger?

I'm afraid I don't understand the question. You will continue to Cc
respective maintainers, which will include me. In case you refer to a
Reviewed-by: tag - you can only add such tags once they were offered to
you by the respective person. For this specific one it doesn't mean "an
earlier version of this was looked at by <person>" but "this is deemed
okay by <person>".

Jan