Re: [PATCH v2 2/2] xen/arm: Add support for booting gzip compressed uImages

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 02/02/2023 12:01, Julien Grall wrote:
> 
> 
> Hi Michal,
> 
> On 02/02/2023 08:49, Michal Orzel wrote:
>> @@ -265,11 +284,14 @@ static __init int kernel_decompress(struct bootmodule 
>> *mod)
>>   #define IH_ARCH_ARM             2       /* ARM          */
>>   #define IH_ARCH_ARM64           22      /* ARM64        */
>>
>> +/* uImage Compression Types */
>> +#define IH_COMP_GZIP            1
>> +
>>   /*
>>    * Check if the image is a uImage and setup kernel_info
>>    */
>>   static int __init kernel_uimage_probe(struct kernel_info *info,
>> -                                      paddr_t addr, paddr_t size)
>> +                                      struct bootmodule *mod)
>>   {
>>       struct {
>>           __be32 magic;   /* Image Header Magic Number */
>> @@ -287,6 +309,8 @@ static int __init kernel_uimage_probe(struct kernel_info 
>> *info,
>>       } uimage;
>>
>>       uint32_t len;
>> +    paddr_t addr = mod->start;
>> +    paddr_t size = mod->size;
>>
>>       if ( size < sizeof(uimage) )
>>           return -EINVAL;
> 
> Shouldn't we return -ENOENT here?
Frankly speaking, I do not want to fall through in such a case.
If a kernel size is less than 64B, something is wrong, isn't it?
I am not sure if Xen would handle a kernel whose size is less than a page.

I do not like the whole falling through in kernel_probe even in case of obvious 
violations.
But this is something not related to this series so I added to my TODO to 
properly handle
the return types from kernel_probe path. If you really think, we should return 
-ENOENT here,
then ok (although I do not like it). Could this be done on commit if you insist 
on that?

> 
> The rest look good to me.
Thanks,

> 
> Cheers,
> 
> --
> Julien Grall

~Michal