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

[Julien Grall <julien@xxxxxxx>]

On 02/02/2023 11:36, Michal Orzel wrote:
> Hi Julien,
Hi,

> On 02/02/2023 12:23, Julien Grall wrote:
> > On 02/02/2023 11:12, Michal Orzel wrote:
> > > Hi Julien,
> > Hi Michal,
> >
> > > 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 agree something is likely wrong but this should not be the job of
> > kernel_uimage_probe() to enforce this for everyone.
> >
> > To give a concrete example, let's imagine we decide to re-order the call
> > so kernel_uimage_probe() happens *after* an new header A than would
> > require 128 bytes (the number is made up).
> >
> > It would be wrong for A to return -EINVAL because the other protocol may
> > require a smaller size. The same goes here even at least for consistency.
> >
> > So if you really want to enforce a minimum size, then such check should
> > be in the caller. Then each probe could return -ENOENT if the size is
> > too small.
> >
> > > I am not sure if Xen would handle a kernel whose size is less than a page.
> > I don't see any reason why it would not be.
> >
> > > 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?
> > See above for an alternative proposal. Depending on the version we
> > settle on I can do it on commit (but this is not going to happen today
> > as OSSTEst is still blocked).
> Ok, lets stick to your original suggestion with s/-EINVAL/-ENOENT/
It looks like I didn't gave my reviewed-by. So:

Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>

I will change it on commit. As we discussed on IRC, even if there is no 
arm32 testing in OSSTest, I expect this to only impact boot and 
therefore should be caught by the Gitlab CI.
You already done some testing [1], so I will commit it now.

> and I will come up with something for a future patch as this will require more 
> changes
> to make it generic. I also do not like at all the fact that we are not 
> informing the user about the error code
> when calling panic from construct_{dom0,domU}...
I saw you posted it. I will add it in my queue of patches to review.

Cheers,

[1] 
https://gitlab.com/xen-project/people/morzel/xen-orzelmichal/-/pipelines/770984105

--
Julien Grall