On Thu, Sep 23, 2021 at 1:55 AM Mathieu Poirier
[off-list ref] wrote:

On Wed, Sep 22, 2021 at 09:35:54AM +0800, Shengjiu Wang wrote:

quoted

Hi Mathieu

On Fri, Sep 17, 2021 at 11:22 PM Mathieu Poirier
[off-list ref] wrote:

quoted

On Fri, Sep 17, 2021 at 05:44:44PM +0800, Shengjiu Wang wrote:

quoted

On Fri, Sep 17, 2021 at 1:20 PM Shengjiu Wang [off-list ref] wrote:

quoted

On Fri, Sep 17, 2021 at 1:00 AM Mathieu Poirier
[off-list ref] wrote:

quoted

[...]

quoted

quoted

quoted

+
+/**
+ * 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.

quoted

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.

quoted

quoted

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

quoted

quoted

quoted

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.

quoted

best regards
wang shengjiu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel