Message-ID: <CAA+D8AOXF_0qeg0H8zeSkz5bj8VQT95B1yMo98jSxxHUooYViw@mail.gmail.com>
Date:   Thu, 23 Sep 2021 09:48:00 +0800
From:   Shengjiu Wang <shengjiu.wang@...il.com>
To:     Mathieu Poirier <mathieu.poirier@...aro.org>
Cc:     Shengjiu Wang <shengjiu.wang@....com>,
        Ohad Ben Cohen <ohad@...ery.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Sascha Hauer <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        Daniel Baluta <daniel.baluta@....com>,
        NXP Linux Team <linux-imx@....com>,
        "open list:REMOTE PROCESSOR (REMOTEPROC) SUBSYSTEM" 
        <linux-remoteproc@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 3/4] remoteproc: imx_dsp_rproc: Add remoteproc driver
 for DSP on i.MX

On Thu, Sep 23, 2021 at 1:55 AM Mathieu Poirier
<mathieu.poirier@...aro.org> wrote:
>
> 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@...aro.org> 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@...il.com> wrote:
> > > > >
> > > > > On Fri, Sep 17, 2021 at 1:00 AM Mathieu Poirier
> > > > > <mathieu.poirier@...aro.org> 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?

https://refspecs.linuxbase.org/elf/elf.pdf

This is the specification. page 40,  p_filesz and p_memsz can be zero.

p_filesz This member gives the number of bytes in the file image of
the segment; it may be
zero.
p_memsz This member gives the number of bytes in the memory image of
the segment; it
may be zero.

And page 41,  p_memsz can > p_filesz.

PT_LOAD The array element specifies a loadable segment, described by
p_filesz and
p_memsz. The bytes from the file are mapped to the beginning of the memory
segment. If the segment's memory size (p_memsz) is larger than the file size
(p_filesz), the "extra'' bytes are defined to hold the value 0 and to follow the
segment's initialized area. The file size may not be larger than the
memory size.
Loadable segment entries in the program header table appear in ascending order,
sorted on the p_vaddr member


best regards
wang shengjiu

>
> > > >
> > > > 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

Powered by blists - more mailing lists