Re: [PATCH v4 3/4] remoteproc: imx_dsp_rproc: Add remoteproc driver for DSP on i.MX

[Mathieu Poirier]

On Wed, Sep 22, 2021 at 09:35:54AM +0800, Shengjiu Wang wrote:
> Hi Mathieu
> 
> On Fri, Sep 17, 2021 at 11:22 PM Mathieu Poirier
> <mathieu.poirier@xxxxxxxxxx> wrote:
> >
> > On Fri, Sep 17, 2021 at 05:44:44PM +0800, Shengjiu Wang wrote:
> > > On Fri, Sep 17, 2021 at 1:20 PM Shengjiu Wang <shengjiu.wang@xxxxxxxxx> wrote:
> > > >
> > > > On Fri, Sep 17, 2021 at 1:00 AM Mathieu Poirier
> > > > <mathieu.poirier@xxxxxxxxxx> wrote:
> > > > >
> > > > > [...]
> > > > >
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * imx_dsp_rproc_elf_load_segments() - load firmware segments to memory
> > > > > > > > + * @rproc: remote processor which will be booted using these fw segments
> > > > > > > > + * @fw: the ELF firmware image
> > > > > > > > + *
> > > > > > > > + * This function specially checks if memsz is zero or not, otherwise it
> > > > > > > > + * is mostly same as rproc_elf_load_segments().
> > > > > > > > + */
> > > > > > > > +static int imx_dsp_rproc_elf_load_segments(struct rproc *rproc,
> > > > > > > > +                                        const struct firmware *fw)
> > > > > > > > +{
> > > > > > > > +     struct device *dev = &rproc->dev;
> > > > > > > > +     u8 class = fw_elf_get_class(fw);
> > > > > > > > +     u32 elf_phdr_get_size = elf_size_of_phdr(class);
> > > > > > > > +     const u8 *elf_data = fw->data;
> > > > > > > > +     const void *ehdr, *phdr;
> > > > > > > > +     int i, ret = 0;
> > > > > > > > +     u16 phnum;
> > > > > > > > +
> > > > > > > > +     ehdr = elf_data;
> > > > > > > > +     phnum = elf_hdr_get_e_phnum(class, ehdr);
> > > > > > > > +     phdr = elf_data + elf_hdr_get_e_phoff(class, ehdr);
> > > > > > > > +
> > > > > > > > +     /* go through the available ELF segments */
> > > > > > > > +     for (i = 0; i < phnum; i++, phdr += elf_phdr_get_size) {
> > > > > > > > +             u64 da = elf_phdr_get_p_paddr(class, phdr);
> > > > > > > > +             u64 memsz = elf_phdr_get_p_memsz(class, phdr);
> > > > > > > > +             u64 filesz = elf_phdr_get_p_filesz(class, phdr);
> > > > > > > > +             u64 offset = elf_phdr_get_p_offset(class, phdr);
> > > > > > > > +             u32 type = elf_phdr_get_p_type(class, phdr);
> > > > > > > > +             void *ptr;
> > > > > > > > +             bool is_iomem;
> > > > > > > > +
> > > > > > > > +             if (type != PT_LOAD || !memsz)
> > > > > > >
> > > > > > > You did a really good job with adding comments but this part is undocumented...
> > > > > > > If I read this correctly you need to check for !memsz because some part of
> > > > > > > the program segment may have a header but its memsz is zero, in which case it can
> > > > > > > be safely skipped.  So why is that segment in the image to start with, and why
> > > > > > > is it marked PT_LOAD if it is not needed?  This is very puzzling...
> > > > > >
> > > > > > Actually I have added comments in the header of this function.
> > > > >
> > > > > Indeed there is a mention of memsz in the function's header but it doesn't
> > > > > mention _why_ this is needed, and that is what I'm looking for.
> > > > >
> > > > > >
> > > > > > memsz= 0 with PT_LOAD issue, I have asked the toolchain's vendor,
> > > > > > they said that this case is allowed by elf spec...
> > > > > >
> > > > > > And in the "pru_rproc.c" and "mtk_scp.c", seems they met same problem
> > > > > > they also check the filesz in their internal xxx_elf_load_segments() function.
> > > > >
> > > > > In both cases they are skipping PT_LOAD sections where "filesz" is '0', which
> > > > > makes sense because we don't know how many bytes to copy.  But here you are
> > > > > skipping over a PT_LOAD section with a potentially valid filesz, and that is the
> > > > > part I don't understand.
> > > >
> > > > Ok, I can use filesz instead. For my case, filesz = memsz = 0,
> > > > it is the same result I want.
> >
> > If that is the case then rproc_elf_load_segments() should work, i.e it won't
> > copy anything.  If rproc_elf_load_segments() doesn't work for you then there are
> > corner cases you haven't told me about.
> >
> > > >
> > > > The reason why I use "memsz '' is because there is  "if (filesz > memsz) "
> > > > check after this,  if memsz is zero, then "filesz" should be zero too, other
> > > > values are not allowed.
> > >
> > > But I still think checking "!memsz" is better than filesz,  because
> > > memsz > filesz is allowed (filesz = 0),  the code below can be executed.
> > > filesz > memsz is not allowed.

The question remains the same - have you seen instances where memsz > filesz?
Also, can you point me to the reference where it is said that memsz is allowed?
And if it is allowed than how do we know that this program section has valid
data, because after all, filesz is 0?

> > >
> > > What do you think?
> >
> > I don't see a need to add a custom implementation for things that _may_ happen.
> > If using the default rproc_elf_load_segments() works than go with that.  We can deal
> > with problems if/when there is a need for it.
> >
> 
> The default rproc_elf_load_segments() with filesz = memsz = 0, then the
> rproc_da_to_va() return ptr=NULL, then rproc_elf_load_segments() will return
> with error.  So this is the reason to add a custom implementation.

Ok, I see about rproc_da_to_va() returning NULL and failing everything from
there one.

> 
> best regards
> wang shengjiu