Hi
On Thu, Mar 31, 2022 at 1:47 PM Janosch Frank <fran...@linux.ibm.com> wrote:
> On 3/31/22 10:58, Marc-André Lureau wrote:
> > Hi
> >
> > On Wed, Mar 30, 2022 at 4:48 PM Janosch Frank <fran...@linux.ibm.com>
> wrote:
> >
> >> There's no need to have phdr_num and sh_info at the same time. We can
> >> make phdr_num 32 bit and set PN_XNUM when we write the header if
> >> phdr_num >= PN_XNUM.
> >>
> >
> > I am not fond of this change tbh, mixing variables, casting a u32 to
> u16..
> >
> > Could you provide more motivation? This seems to contradict also your
> > cleanup approach in the later patch "Add more offset variables".
>
> I can't fully follow you there. Where do I cast to u16 or mix variables?
>
"uint16_t phnum = s->phdr_num >= PN_XNUM ? PN_XNUM : s->phdr_num"
It's an implicit type casting from u32.
As for mixing variables, we used to have this obvious:
shdr64.sh_info = cpu_to_dump32(s, s->sh_info);
And now:
shdr64.sh_info = cpu_to_dump32(s, s->phdr_num);
> My idea for this change:
> ph_num is made independent of the data structure that it ends up in. We
> use ph_num as the only source for decisions and elf structure
> manipulation down the line since it contains the maximum possible number
> of section headers.
>
> This way we move the extra handling to the locations where it belongs
> and where an explanation for that behavior makes most sense:
> The ehdr write function and the section write function
>
> Without knowing the ELF spec, could you tell me what sh_info stores when
> first reading this code? Going from the name it might be section header
> information, whatever that would mean.
>
> I'd be happy to add comments to write_elf(32|64)_header() though so it's
> a bit more clear why we need to set PN_XNUM. I've already added one to
> dump_begin but that's not where we use PN_XNUM.
> >> Signed-off-by: Janosch Frank <fran...@linux.ibm.com>
> >> Reviewed-by: Richard Henderson <richard.hender...@linaro.org>
>
The more we could clarify and document the code, the better ;) But since
you already got the change approved by Richard, I won't hold it.
thanks
> >> ---
> >> dump/dump.c | 34 ++++++++++++++--------------------
> >> include/sysemu/dump.h | 3 +--
> >> 2 files changed, 15 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/dump/dump.c b/dump/dump.c
> >> index 58c4923fce..0e6187c962 100644
> >> --- a/dump/dump.c
> >> +++ b/dump/dump.c
> >> @@ -124,6 +124,7 @@ static int fd_write_vmcore(const void *buf, size_t
> >> size, void *opaque)
> >>
> >> static void write_elf64_header(DumpState *s, Error **errp)
> >> {
> >> + uint16_t phnum = s->phdr_num >= PN_XNUM ? PN_XNUM : s->phdr_num;
> >>
> >
> > Please use MIN()
> >
> >
> >> Elf64_Ehdr elf_header;
> >> int ret;
> >>
> >> @@ -138,9 +139,9 @@ static void write_elf64_header(DumpState *s, Error
> >> **errp)
> >> elf_header.e_ehsize = cpu_to_dump16(s, sizeof(elf_header));
> >> elf_header.e_phoff = cpu_to_dump64(s, sizeof(Elf64_Ehdr));
> >> elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf64_Phdr));
> >> - elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
> >> + elf_header.e_phnum = cpu_to_dump16(s, phnum);
> >> if (s->have_section) {
> >> - uint64_t shoff = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) *
> >> s->sh_info;
> >> + uint64_t shoff = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) *
> >> s->phdr_num;
> >>
> >> elf_header.e_shoff = cpu_to_dump64(s, shoff);
> >> elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr));
> >> @@ -155,6 +156,7 @@ static void write_elf64_header(DumpState *s, Error
> >> **errp)
> >>
> >> static void write_elf32_header(DumpState *s, Error **errp)
> >> {
> >> + uint16_t phnum = s->phdr_num >= PN_XNUM ? PN_XNUM : s->phdr_num;
> >>
> >
> > same
> >
> >
> >> Elf32_Ehdr elf_header;
> >> int ret;
> >>
> >> @@ -169,9 +171,9 @@ static void write_elf32_header(DumpState *s, Error
> >> **errp)
> >> elf_header.e_ehsize = cpu_to_dump16(s, sizeof(elf_header));
> >> elf_header.e_phoff = cpu_to_dump32(s, sizeof(Elf32_Ehdr));
> >> elf_header.e_phentsize = cpu_to_dump16(s, sizeof(Elf32_Phdr));
> >> - elf_header.e_phnum = cpu_to_dump16(s, s->phdr_num);
> >> + elf_header.e_phnum = cpu_to_dump16(s, phnum);
> >> if (s->have_section) {
> >> - uint32_t shoff = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) *
> >> s->sh_info;
> >> + uint32_t shoff = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) *
> >> s->phdr_num;
> >>
> >> elf_header.e_shoff = cpu_to_dump32(s, shoff);
> >> elf_header.e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr));
> >> @@ -358,12 +360,12 @@ static void write_elf_section(DumpState *s, int
> >> type, Error **errp)
> >> if (type == 0) {
> >> shdr_size = sizeof(Elf32_Shdr);
> >> memset(&shdr32, 0, shdr_size);
> >> - shdr32.sh_info = cpu_to_dump32(s, s->sh_info);
> >> + shdr32.sh_info = cpu_to_dump32(s, s->phdr_num);
> >> shdr = &shdr32;
> >> } else {
> >> shdr_size = sizeof(Elf64_Shdr);
> >> memset(&shdr64, 0, shdr_size);
> >> - shdr64.sh_info = cpu_to_dump32(s, s->sh_info);
> >> + shdr64.sh_info = cpu_to_dump32(s, s->phdr_num);
> >> shdr = &shdr64;
> >> }
> >>
> >> @@ -478,13 +480,6 @@ static void write_elf_loads(DumpState *s, Error
> >> **errp)
> >> hwaddr offset, filesz;
> >> MemoryMapping *memory_mapping;
> >> uint32_t phdr_index = 1;
> >> - uint32_t max_index;
> >> -
> >> - if (s->have_section) {
> >> - max_index = s->sh_info;
> >> - } else {
> >> - max_index = s->phdr_num;
> >> - }
> >>
> >> QTAILQ_FOREACH(memory_mapping, &s->list.head, next) {
> >> get_offset_range(memory_mapping->phys_addr,
> >> @@ -502,7 +497,7 @@ static void write_elf_loads(DumpState *s, Error
> **errp)
> >> return;
> >> }
> >>
> >> - if (phdr_index >= max_index) {
> >> + if (phdr_index >= s->phdr_num) {
> >> break;
> >> }
> >> }
> >> @@ -1801,22 +1796,21 @@ static void dump_init(DumpState *s, int fd, bool
> >> has_format,
> >> s->phdr_num += s->list.num;
> >> s->have_section = false;
> >> } else {
> >> + /* sh_info of section 0 holds the real number of phdrs */
> >> s->have_section = true;
> >> - s->phdr_num = PN_XNUM;
> >> - s->sh_info = 1; /* PT_NOTE */
> >>
> >> /* the type of shdr->sh_info is uint32_t, so we should avoid
> >> overflow */
> >> if (s->list.num <= UINT32_MAX - 1) {
> >> - s->sh_info += s->list.num;
> >> + s->phdr_num += s->list.num;
> >> } else {
> >> - s->sh_info = UINT32_MAX;
> >> + s->phdr_num = UINT32_MAX;
> >> }
> >> }
> >>
> >> if (s->dump_info.d_class == ELFCLASS64) {
> >> if (s->have_section) {
> >> s->memory_offset = sizeof(Elf64_Ehdr) +
> >> - sizeof(Elf64_Phdr) * s->sh_info +
> >> + sizeof(Elf64_Phdr) * s->phdr_num +
> >> sizeof(Elf64_Shdr) + s->note_size;
> >> } else {
> >> s->memory_offset = sizeof(Elf64_Ehdr) +
> >> @@ -1825,7 +1819,7 @@ static void dump_init(DumpState *s, int fd, bool
> >> has_format,
> >> } else {
> >> if (s->have_section) {
> >> s->memory_offset = sizeof(Elf32_Ehdr) +
> >> - sizeof(Elf32_Phdr) * s->sh_info +
> >> + sizeof(Elf32_Phdr) * s->phdr_num +
> >> sizeof(Elf32_Shdr) + s->note_size;
> >> } else {
> >> s->memory_offset = sizeof(Elf32_Ehdr) +
> >> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> >> index 250143cb5a..b463fc9c02 100644
> >> --- a/include/sysemu/dump.h
> >> +++ b/include/sysemu/dump.h
> >> @@ -154,8 +154,7 @@ typedef struct DumpState {
> >> GuestPhysBlockList guest_phys_blocks;
> >> ArchDumpInfo dump_info;
> >> MemoryMappingList list;
> >> - uint16_t phdr_num;
> >> - uint32_t sh_info;
> >> + uint32_t phdr_num;
> >> bool have_section;
> >> bool resume;
> >> bool detached;
> >> --
> >> 2.32.0
> >>
> >>
> >>
> >
>
>
--
Marc-André Lureau
