| Message ID | 20220726092248.128336-12-frankja@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | dump: Add arch section and s390x PV dump | expand |
Hi On Tue, Jul 26, 2022 at 1:23 PM Janosch Frank <frankja@linux.ibm.com> wrote: > > As sections don't have a type like the notes do we need another way to > determine their contents. The string table allows us to assign each > section an identification string which architectures can then use to > tag their sections with. > > There will be no string table if the architecture doesn't add custom > sections which are introduced in a following patch. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > dump/dump.c | 81 ++++++++++++++++++++++++++++++++++++++++++- > include/sysemu/dump.h | 1 + > 2 files changed, 81 insertions(+), 1 deletion(-) > > diff --git a/dump/dump.c b/dump/dump.c > index 3cf846d0a0..298a1e923f 100644 > --- a/dump/dump.c > +++ b/dump/dump.c > @@ -99,6 +99,7 @@ static int dump_cleanup(DumpState *s) > close(s->fd); > g_free(s->guest_note); > g_free(s->elf_header); > + g_array_unref(s->string_table_buf); > s->guest_note = NULL; > if (s->resume) { > if (s->detached) { > @@ -357,21 +358,72 @@ static size_t prepare_elf_section_hdr_zero(DumpState *s) > return dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr); > } > > +static void write_elf_section_hdr_string(DumpState *s, void *buff) > +{ Mildly annoyed that we use "write_" prefix for actually writing to the fd, and sometimes to pre-fill (or "prepare_" structures). Would you mind cleaning that up? > + Elf32_Shdr shdr32; > + Elf64_Shdr shdr64; > + int shdr_size; > + void *shdr = buff; Why assign here? > + > + if (dump_is_64bit(s)) { > + shdr_size = sizeof(Elf64_Shdr); > + memset(&shdr64, 0, shdr_size); > + shdr64.sh_type = SHT_STRTAB; > + shdr64.sh_offset = s->section_offset + s->elf_section_data_size; > + shdr64.sh_name = s->string_table_buf->len; > + g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab")); > + shdr64.sh_size = s->string_table_buf->len; > + shdr = &shdr64; > + } else { > + shdr_size = sizeof(Elf32_Shdr); > + memset(&shdr32, 0, shdr_size); > + shdr32.sh_type = SHT_STRTAB; > + shdr32.sh_offset = s->section_offset + s->elf_section_data_size; > + shdr32.sh_name = s->string_table_buf->len; > + g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab")); > + shdr32.sh_size = s->string_table_buf->len; > + shdr = &shdr32; > + } > + > + memcpy(buff, shdr, shdr_size); > +} > + > static void prepare_elf_section_hdrs(DumpState *s) > { > size_t len, sizeof_shdr; > + Elf64_Ehdr *hdr64 = s->elf_header; > + Elf32_Ehdr *hdr32 = s->elf_header; > + void *buff_hdr; > > /* > * Section ordering: > * - HDR zero (if needed) > + * - String table hdr > */ > sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr); > len = sizeof_shdr * s->shdr_num; > s->elf_section_hdrs = g_malloc0(len); > + buff_hdr = s->elf_section_hdrs; > > /* Write special section first */ > if (s->phdr_num == PN_XNUM) { > prepare_elf_section_hdr_zero(s); > + buff_hdr += sizeof_shdr; > + } > + > + if (s->shdr_num < 2) { > + return; > + } > + > + /* > + * String table needs to be last section since strings are added > + * via arch_sections_write_hdr(). > + */ This comment isn't exactly relevant yet, since that code isn't there, but ok. > + write_elf_section_hdr_string(s, buff_hdr); > + if (dump_is_64bit(s)) { > + hdr64->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1); > + } else { > + hdr32->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1); > } > } > > @@ -401,11 +453,18 @@ static void write_elf_sections(DumpState *s, Error **errp) > { > int ret; > > - /* Write section zero */ > + /* Write section zero and arch sections */ > ret = fd_write_vmcore(s->elf_section_data, s->elf_section_data_size, s); > if (ret < 0) { > error_setg_errno(errp, -ret, "dump: failed to write section data"); > } > + > + /* Write string table data */ > + ret = fd_write_vmcore(s->string_table_buf->data, > + s->string_table_buf->len, s); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "dump: failed to write string table data"); > + } > } > > static void write_data(DumpState *s, void *buf, int length, Error **errp) > @@ -713,6 +772,7 @@ static void create_vmcore(DumpState *s, Error **errp) > return; > } > > + /* Iterate over memory and dump it to file */ > dump_iterate(s, errp); > if (*errp) { > return; > @@ -1695,6 +1755,13 @@ static void dump_init(DumpState *s, int fd, bool has_format, > s->has_filter = has_filter; > s->begin = begin; > s->length = length; > + /* First index is 0, it's the special null name */ > + s->string_table_buf = g_array_new(FALSE, TRUE, 1); > + /* > + * Allocate the null name, due to the clearing option set to true > + * it will be 0. > + */ > + g_array_set_size(s->string_table_buf, 1); I wonder if GByteArray wouldn't be more appropriate, even if it doesn't have the clearing option. If it's just for one byte, ... > > memory_mapping_list_init(&s->list); > > @@ -1855,6 +1922,18 @@ static void dump_init(DumpState *s, int fd, bool has_format, > } > } > > + /* > + * calculate shdr_num and elf_section_data_size so we know the offsets and > + * sizes of all parts. > + * > + * If phdr_num overflowed we have at least one section header > + * More sections/hdrs can be added by the architectures > + */ > + if (s->shdr_num > 1) { > + /* Reserve the string table */ > + s->shdr_num += 1; > + } > + > tmp = (s->phdr_num == PN_XNUM) ? s->sh_info : s->phdr_num; > if (dump_is_64bit(s)) { > s->shdr_offset = sizeof(Elf64_Ehdr); > diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h > index 696e6c67d6..299b611180 100644 > --- a/include/sysemu/dump.h > +++ b/include/sysemu/dump.h > @@ -178,6 +178,7 @@ typedef struct DumpState { > void *elf_section_hdrs; > uint64_t elf_section_data_size; > void *elf_section_data; > + GArray *string_table_buf; /* String table section */ > > uint8_t *note_buf; /* buffer for notes */ > size_t note_buf_offset; /* the writing place in note_buf */ > -- > 2.34.1 >
On 7/26/22 13:25, Marc-André Lureau wrote: > Hi > > On Tue, Jul 26, 2022 at 1:23 PM Janosch Frank <frankja@linux.ibm.com> wrote: >> >> As sections don't have a type like the notes do we need another way to >> determine their contents. The string table allows us to assign each >> section an identification string which architectures can then use to >> tag their sections with. >> >> There will be no string table if the architecture doesn't add custom >> sections which are introduced in a following patch. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> dump/dump.c | 81 ++++++++++++++++++++++++++++++++++++++++++- >> include/sysemu/dump.h | 1 + >> 2 files changed, 81 insertions(+), 1 deletion(-) >> >> diff --git a/dump/dump.c b/dump/dump.c >> index 3cf846d0a0..298a1e923f 100644 >> --- a/dump/dump.c >> +++ b/dump/dump.c >> @@ -99,6 +99,7 @@ static int dump_cleanup(DumpState *s) >> close(s->fd); >> g_free(s->guest_note); >> g_free(s->elf_header); >> + g_array_unref(s->string_table_buf); >> s->guest_note = NULL; >> if (s->resume) { >> if (s->detached) { >> @@ -357,21 +358,72 @@ static size_t prepare_elf_section_hdr_zero(DumpState *s) >> return dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr); >> } >> >> +static void write_elf_section_hdr_string(DumpState *s, void *buff) >> +{ > > Mildly annoyed that we use "write_" prefix for actually writing to the > fd, and sometimes to pre-fill (or "prepare_" structures). Would you > mind cleaning that up? > Yes, absolutely >> + Elf32_Shdr shdr32; >> + Elf64_Shdr shdr64; >> + int shdr_size; >> + void *shdr = buff; > > Why assign here? Great question > >> + >> + if (dump_is_64bit(s)) { >> + shdr_size = sizeof(Elf64_Shdr); >> + memset(&shdr64, 0, shdr_size); >> + shdr64.sh_type = SHT_STRTAB; >> + shdr64.sh_offset = s->section_offset + s->elf_section_data_size; >> + shdr64.sh_name = s->string_table_buf->len; >> + g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab")); >> + shdr64.sh_size = s->string_table_buf->len; >> + shdr = &shdr64; >> + } else { >> + shdr_size = sizeof(Elf32_Shdr); >> + memset(&shdr32, 0, shdr_size); >> + shdr32.sh_type = SHT_STRTAB; >> + shdr32.sh_offset = s->section_offset + s->elf_section_data_size; >> + shdr32.sh_name = s->string_table_buf->len; >> + g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab")); >> + shdr32.sh_size = s->string_table_buf->len; >> + shdr = &shdr32; >> + } >> + >> + memcpy(buff, shdr, shdr_size); >> +} >> + >> static void prepare_elf_section_hdrs(DumpState *s) >> { >> size_t len, sizeof_shdr; >> + Elf64_Ehdr *hdr64 = s->elf_header; >> + Elf32_Ehdr *hdr32 = s->elf_header; >> + void *buff_hdr; >> >> /* >> * Section ordering: >> * - HDR zero (if needed) >> + * - String table hdr >> */ >> sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr); >> len = sizeof_shdr * s->shdr_num; >> s->elf_section_hdrs = g_malloc0(len); >> + buff_hdr = s->elf_section_hdrs; >> >> /* Write special section first */ >> if (s->phdr_num == PN_XNUM) { >> prepare_elf_section_hdr_zero(s); >> + buff_hdr += sizeof_shdr; >> + } >> + >> + if (s->shdr_num < 2) { >> + return; >> + } >> + >> + /* >> + * String table needs to be last section since strings are added >> + * via arch_sections_write_hdr(). >> + */ > > This comment isn't exactly relevant yet, since that code isn't there, but ok. What about something like this, it's a bit more precise and I'll add the arch_sections_write_hdr() reference in the next patch? /* * String table needs to be the last section since it will be populated when adding other elf structures. */ [..] >> s->length = length; >> + /* First index is 0, it's the special null name */ >> + s->string_table_buf = g_array_new(FALSE, TRUE, 1); >> + /* >> + * Allocate the null name, due to the clearing option set to true >> + * it will be 0. >> + */ >> + g_array_set_size(s->string_table_buf, 1); > > I wonder if GByteArray wouldn't be more appropriate, even if it > doesn't have the clearing option. If it's just for one byte, ... I don't really care but I need a decision on it to change it :)
On Tue, Jul 26, 2022 at 4:55 PM Janosch Frank <frankja@linux.ibm.com> wrote: > On 7/26/22 13:25, Marc-André Lureau wrote: > > Hi > > > > On Tue, Jul 26, 2022 at 1:23 PM Janosch Frank <frankja@linux.ibm.com> > wrote: > >> > >> As sections don't have a type like the notes do we need another way to > >> determine their contents. The string table allows us to assign each > >> section an identification string which architectures can then use to > >> tag their sections with. > >> > >> There will be no string table if the architecture doesn't add custom > >> sections which are introduced in a following patch. > >> > >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > >> --- > >> dump/dump.c | 81 ++++++++++++++++++++++++++++++++++++++++++- > >> include/sysemu/dump.h | 1 + > >> 2 files changed, 81 insertions(+), 1 deletion(-) > >> > >> diff --git a/dump/dump.c b/dump/dump.c > >> index 3cf846d0a0..298a1e923f 100644 > >> --- a/dump/dump.c > >> +++ b/dump/dump.c > >> @@ -99,6 +99,7 @@ static int dump_cleanup(DumpState *s) > >> close(s->fd); > >> g_free(s->guest_note); > >> g_free(s->elf_header); > >> + g_array_unref(s->string_table_buf); > >> s->guest_note = NULL; > >> if (s->resume) { > >> if (s->detached) { > >> @@ -357,21 +358,72 @@ static size_t > prepare_elf_section_hdr_zero(DumpState *s) > >> return dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr); > >> } > >> > >> +static void write_elf_section_hdr_string(DumpState *s, void *buff) > >> +{ > > > > Mildly annoyed that we use "write_" prefix for actually writing to the > > fd, and sometimes to pre-fill (or "prepare_" structures). Would you > > mind cleaning that up? > > > > Yes, absolutely > > >> + Elf32_Shdr shdr32; > >> + Elf64_Shdr shdr64; > >> + int shdr_size; > >> + void *shdr = buff; > > > > Why assign here? > > Great question > :) > > > > >> + > >> + if (dump_is_64bit(s)) { > >> + shdr_size = sizeof(Elf64_Shdr); > >> + memset(&shdr64, 0, shdr_size); > >> + shdr64.sh_type = SHT_STRTAB; > >> + shdr64.sh_offset = s->section_offset + > s->elf_section_data_size; > >> + shdr64.sh_name = s->string_table_buf->len; > >> + g_array_append_vals(s->string_table_buf, ".strtab", > sizeof(".strtab")); > >> + shdr64.sh_size = s->string_table_buf->len; > >> + shdr = &shdr64; > >> + } else { > >> + shdr_size = sizeof(Elf32_Shdr); > >> + memset(&shdr32, 0, shdr_size); > >> + shdr32.sh_type = SHT_STRTAB; > >> + shdr32.sh_offset = s->section_offset + > s->elf_section_data_size; > >> + shdr32.sh_name = s->string_table_buf->len; > >> + g_array_append_vals(s->string_table_buf, ".strtab", > sizeof(".strtab")); > >> + shdr32.sh_size = s->string_table_buf->len; > >> + shdr = &shdr32; > >> + } > >> + > >> + memcpy(buff, shdr, shdr_size); > >> +} > >> + > >> static void prepare_elf_section_hdrs(DumpState *s) > >> { > >> size_t len, sizeof_shdr; > >> + Elf64_Ehdr *hdr64 = s->elf_header; > >> + Elf32_Ehdr *hdr32 = s->elf_header; > >> + void *buff_hdr; > >> > >> /* > >> * Section ordering: > >> * - HDR zero (if needed) > >> + * - String table hdr > >> */ > >> sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : > sizeof(Elf32_Shdr); > >> len = sizeof_shdr * s->shdr_num; > >> s->elf_section_hdrs = g_malloc0(len); > >> + buff_hdr = s->elf_section_hdrs; > >> > >> /* Write special section first */ > >> if (s->phdr_num == PN_XNUM) { > >> prepare_elf_section_hdr_zero(s); > >> + buff_hdr += sizeof_shdr; > >> + } > >> + > >> + if (s->shdr_num < 2) { > >> + return; > >> + } > >> + > >> + /* > >> + * String table needs to be last section since strings are added > >> + * via arch_sections_write_hdr(). > >> + */ > > > > This comment isn't exactly relevant yet, since that code isn't there, > but ok. > > What about something like this, it's a bit more precise and I'll add the > arch_sections_write_hdr() reference in the next patch? > > /* > * String table needs to be the last section since it will be populated > when adding other elf structures. > */ > > ok > [..] > >> s->length = length; > >> + /* First index is 0, it's the special null name */ > >> + s->string_table_buf = g_array_new(FALSE, TRUE, 1); > >> + /* > >> + * Allocate the null name, due to the clearing option set to true > >> + * it will be 0. > >> + */ > >> + g_array_set_size(s->string_table_buf, 1); > > > > I wonder if GByteArray wouldn't be more appropriate, even if it > > doesn't have the clearing option. If it's just for one byte, ... > > I don't really care but I need a decision on it to change it :) > I haven't tried, but I think it would be a better fit.
On 7/26/22 15:12, Marc-André Lureau wrote: > On Tue, Jul 26, 2022 at 4:55 PM Janosch Frank <frankja@linux.ibm.com> wrote: > >> On 7/26/22 13:25, Marc-André Lureau wrote: >>> Hi >>> >>> On Tue, Jul 26, 2022 at 1:23 PM Janosch Frank <frankja@linux.ibm.com> >> wrote: >>>> >>>> As sections don't have a type like the notes do we need another way to >>>> determine their contents. The string table allows us to assign each >>>> section an identification string which architectures can then use to >>>> tag their sections with. >>>> >>>> There will be no string table if the architecture doesn't add custom >>>> sections which are introduced in a following patch. >>>> >>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> [...] >> [..] >>>> s->length = length; >>>> + /* First index is 0, it's the special null name */ >>>> + s->string_table_buf = g_array_new(FALSE, TRUE, 1); >>>> + /* >>>> + * Allocate the null name, due to the clearing option set to true >>>> + * it will be 0. >>>> + */ >>>> + g_array_set_size(s->string_table_buf, 1); >>> >>> I wonder if GByteArray wouldn't be more appropriate, even if it >>> doesn't have the clearing option. If it's just for one byte, ... >> >> I don't really care but I need a decision on it to change it :) >> > > I haven't tried, but I think it would be a better fit. Looking at this a second time there's an issue you should consider: GByteArray uses guint8 while the GArray uses gchars which are apparently compatible with normal C chars. I.e. I need to cast all strings to (const guint8 *) when appending them to the GByteArray.
Hi On Tue, Jul 26, 2022 at 6:26 PM Janosch Frank <frankja@linux.ibm.com> wrote: > On 7/26/22 15:12, Marc-André Lureau wrote: > > On Tue, Jul 26, 2022 at 4:55 PM Janosch Frank <frankja@linux.ibm.com> > wrote: > > > >> On 7/26/22 13:25, Marc-André Lureau wrote: > >>> Hi > >>> > >>> On Tue, Jul 26, 2022 at 1:23 PM Janosch Frank <frankja@linux.ibm.com> > >> wrote: > >>>> > >>>> As sections don't have a type like the notes do we need another way to > >>>> determine their contents. The string table allows us to assign each > >>>> section an identification string which architectures can then use to > >>>> tag their sections with. > >>>> > >>>> There will be no string table if the architecture doesn't add custom > >>>> sections which are introduced in a following patch. > >>>> > >>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > [...] > >> [..] > >>>> s->length = length; > >>>> + /* First index is 0, it's the special null name */ > >>>> + s->string_table_buf = g_array_new(FALSE, TRUE, 1); > >>>> + /* > >>>> + * Allocate the null name, due to the clearing option set to true > >>>> + * it will be 0. > >>>> + */ > >>>> + g_array_set_size(s->string_table_buf, 1); > >>> > >>> I wonder if GByteArray wouldn't be more appropriate, even if it > >>> doesn't have the clearing option. If it's just for one byte, ... > >> > >> I don't really care but I need a decision on it to change it :) > >> > > > > I haven't tried, but I think it would be a better fit. > > Looking at this a second time there's an issue you should consider: > > GByteArray uses guint8 while the GArray uses gchars which are apparently > compatible with normal C chars. > > I.e. I need to cast all strings to (const guint8 *) when appending them > to the GByteArray. > Agh, boring.. well, we also have include/qemu/buffer.h that could be considered perhaps
On 7/26/22 11:22, Janosch Frank wrote: > As sections don't have a type like the notes do we need another way to Having a string table seems like a good idea to me, as we don't know the requirements any architecture might have, but sections do have sh_type. Could we use one of those, e.g. one of the processor specific ones? Is SHT_PROGBITS The section holds information defined by the program, whose format and meaning are determined solely by the program. appropriate for us? It seems to me that our program is the guest and it doesn't determine the meta information on how to decrypt the dump. > determine their contents. The string table allows us to assign each > section an identification string which architectures can then use to > tag their sections with. > > There will be no string table if the architecture doesn't add custom > sections which are introduced in a following patch. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > dump/dump.c | 81 ++++++++++++++++++++++++++++++++++++++++++- > include/sysemu/dump.h | 1 + > 2 files changed, 81 insertions(+), 1 deletion(-) > > diff --git a/dump/dump.c b/dump/dump.c > index 3cf846d0a0..298a1e923f 100644 > --- a/dump/dump.c > +++ b/dump/dump.c [...] > static void prepare_elf_section_hdrs(DumpState *s) > { > size_t len, sizeof_shdr; > + Elf64_Ehdr *hdr64 = s->elf_header; > + Elf32_Ehdr *hdr32 = s->elf_header; > + void *buff_hdr; > > /* > * Section ordering: > * - HDR zero (if needed) > + * - String table hdr > */ > sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr); > len = sizeof_shdr * s->shdr_num; > s->elf_section_hdrs = g_malloc0(len); > + buff_hdr = s->elf_section_hdrs; > > /* Write special section first */ > if (s->phdr_num == PN_XNUM) { > prepare_elf_section_hdr_zero(s); > + buff_hdr += sizeof_shdr; > + } > + > + if (s->shdr_num < 2) { > + return; > + } > + > + /* > + * String table needs to be last section since strings are added > + * via arch_sections_write_hdr(). > + */ > + write_elf_section_hdr_string(s, buff_hdr); > + if (dump_is_64bit(s)) { > + hdr64->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1); > + } else { > + hdr32->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1); > } Might want to assert e_shstrndx < SHN_LORESERVE (0xff00) or handle that case correctly. > } > > @@ -401,11 +453,18 @@ static void write_elf_sections(DumpState *s, Error **errp) > { > int ret; > > - /* Write section zero */ > + /* Write section zero and arch sections */ Since that doesn't concern the string section it seems wrong to change this in this patch. > ret = fd_write_vmcore(s->elf_section_data, s->elf_section_data_size, s); > if (ret < 0) { > error_setg_errno(errp, -ret, "dump: failed to write section data"); > } > + > + /* Write string table data */ > + ret = fd_write_vmcore(s->string_table_buf->data, > + s->string_table_buf->len, s); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "dump: failed to write string table data"); > + } > } > > static void write_data(DumpState *s, void *buf, int length, Error **errp) > @@ -713,6 +772,7 @@ static void create_vmcore(DumpState *s, Error **errp) > return; > } > > + /* Iterate over memory and dump it to file */ This should be going into the patch introducing dump_end. > dump_iterate(s, errp); > if (*errp) { > return; > @@ -1695,6 +1755,13 @@ static void dump_init(DumpState *s, int fd, bool has_format, > s->has_filter = has_filter; > s->begin = begin; > s->length = length; > + /* First index is 0, it's the special null name */ > + s->string_table_buf = g_array_new(FALSE, TRUE, 1); > + /* > + * Allocate the null name, due to the clearing option set to true > + * it will be 0. > + */ > + g_array_set_size(s->string_table_buf, 1); > > memory_mapping_list_init(&s->list); > > @@ -1855,6 +1922,18 @@ static void dump_init(DumpState *s, int fd, bool has_format, > } > } > > + /* > + * calculate shdr_num and elf_section_data_size so we know the offsets and > + * sizes of all parts. > + * > + * If phdr_num overflowed we have at least one section header > + * More sections/hdrs can be added by the architectures > + */ This needs to be adjusted like we talked about, i.e. adding the special 0 section unless it already exists. > + if (s->shdr_num > 1) { > + /* Reserve the string table */ > + s->shdr_num += 1; > + } > + > tmp = (s->phdr_num == PN_XNUM) ? s->sh_info : s->phdr_num; > if (dump_is_64bit(s)) { > s->shdr_offset = sizeof(Elf64_Ehdr); > diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h > index 696e6c67d6..299b611180 100644 > --- a/include/sysemu/dump.h > +++ b/include/sysemu/dump.h > @@ -178,6 +178,7 @@ typedef struct DumpState { > void *elf_section_hdrs; > uint64_t elf_section_data_size; > void *elf_section_data; > + GArray *string_table_buf; /* String table section */ > > uint8_t *note_buf; /* buffer for notes */ > size_t note_buf_offset; /* the writing place in note_buf */
On 7/28/22 15:41, Marc-André Lureau wrote: > Hi > > On Tue, Jul 26, 2022 at 6:26 PM Janosch Frank <frankja@linux.ibm.com> wrote: > >> On 7/26/22 15:12, Marc-André Lureau wrote: >>> On Tue, Jul 26, 2022 at 4:55 PM Janosch Frank <frankja@linux.ibm.com> >> wrote: >>> >>>> On 7/26/22 13:25, Marc-André Lureau wrote: >>>>> Hi >>>>> >>>>> On Tue, Jul 26, 2022 at 1:23 PM Janosch Frank <frankja@linux.ibm.com> >>>> wrote: >>>>>> >>>>>> As sections don't have a type like the notes do we need another way to >>>>>> determine their contents. The string table allows us to assign each >>>>>> section an identification string which architectures can then use to >>>>>> tag their sections with. >>>>>> >>>>>> There will be no string table if the architecture doesn't add custom >>>>>> sections which are introduced in a following patch. >>>>>> >>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> [...] >>>> [..] >>>>>> s->length = length; >>>>>> + /* First index is 0, it's the special null name */ >>>>>> + s->string_table_buf = g_array_new(FALSE, TRUE, 1); >>>>>> + /* >>>>>> + * Allocate the null name, due to the clearing option set to true >>>>>> + * it will be 0. >>>>>> + */ >>>>>> + g_array_set_size(s->string_table_buf, 1); >>>>> >>>>> I wonder if GByteArray wouldn't be more appropriate, even if it >>>>> doesn't have the clearing option. If it's just for one byte, ... >>>> >>>> I don't really care but I need a decision on it to change it :) >>>> >>> >>> I haven't tried, but I think it would be a better fit. >> >> Looking at this a second time there's an issue you should consider: >> >> GByteArray uses guint8 while the GArray uses gchars which are apparently >> compatible with normal C chars. >> >> I.e. I need to cast all strings to (const guint8 *) when appending them >> to the GByteArray. >> > > Agh, boring.. well, we also have include/qemu/buffer.h that could be > considered perhaps > Why should I change it to something that's hardly being used, i.e. what's the problem here?
On 7/29/22 21:35, Janis Schoetterl-Glausch wrote: > On 7/26/22 11:22, Janosch Frank wrote: >> As sections don't have a type like the notes do we need another way to > > Having a string table seems like a good idea to me, as we don't know > the requirements any architecture might have, but sections do have sh_type. > Could we use one of those, e.g. one of the processor specific ones? I'd like to avoid defining more constants in elf.h if at all possible. > Is > SHT_PROGBITS > The section holds information defined by the program, > whose format and meaning are determined solely by the program. > appropriate for us? It seems to me that our program is the guest and > it doesn't determine the meta information on how to decrypt the dump. SHT_PROGBITS is being set in the last patch (PV dump) for our arch headers. The architecture writes the full shdr into the header buffer and can set any type. > >> determine their contents. The string table allows us to assign each >> section an identification string which architectures can then use to >> tag their sections with. >> >> There will be no string table if the architecture doesn't add custom >> sections which are introduced in a following patch. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- [...] >> + /* >> + * String table needs to be last section since strings are added >> + * via arch_sections_write_hdr(). >> + */ >> + write_elf_section_hdr_string(s, buff_hdr); >> + if (dump_is_64bit(s)) { >> + hdr64->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1); >> + } else { >> + hdr32->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1); >> } > > Might want to assert e_shstrndx < SHN_LORESERVE (0xff00) or handle that case correctly. Right, more things to worry about > >> } >> >> @@ -401,11 +453,18 @@ static void write_elf_sections(DumpState *s, Error **errp) >> { >> int ret; >> >> - /* Write section zero */ >> + /* Write section zero and arch sections */ > > Since that doesn't concern the string section it seems wrong to change this in this patch. I'm currently doing another round of cleanups anyway :)
diff --git a/dump/dump.c b/dump/dump.c index 3cf846d0a0..298a1e923f 100644 --- a/dump/dump.c +++ b/dump/dump.c @@ -99,6 +99,7 @@ static int dump_cleanup(DumpState *s) close(s->fd); g_free(s->guest_note); g_free(s->elf_header); + g_array_unref(s->string_table_buf); s->guest_note = NULL; if (s->resume) { if (s->detached) { @@ -357,21 +358,72 @@ static size_t prepare_elf_section_hdr_zero(DumpState *s) return dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr); } +static void write_elf_section_hdr_string(DumpState *s, void *buff) +{ + Elf32_Shdr shdr32; + Elf64_Shdr shdr64; + int shdr_size; + void *shdr = buff; + + if (dump_is_64bit(s)) { + shdr_size = sizeof(Elf64_Shdr); + memset(&shdr64, 0, shdr_size); + shdr64.sh_type = SHT_STRTAB; + shdr64.sh_offset = s->section_offset + s->elf_section_data_size; + shdr64.sh_name = s->string_table_buf->len; + g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab")); + shdr64.sh_size = s->string_table_buf->len; + shdr = &shdr64; + } else { + shdr_size = sizeof(Elf32_Shdr); + memset(&shdr32, 0, shdr_size); + shdr32.sh_type = SHT_STRTAB; + shdr32.sh_offset = s->section_offset + s->elf_section_data_size; + shdr32.sh_name = s->string_table_buf->len; + g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab")); + shdr32.sh_size = s->string_table_buf->len; + shdr = &shdr32; + } + + memcpy(buff, shdr, shdr_size); +} + static void prepare_elf_section_hdrs(DumpState *s) { size_t len, sizeof_shdr; + Elf64_Ehdr *hdr64 = s->elf_header; + Elf32_Ehdr *hdr32 = s->elf_header; + void *buff_hdr; /* * Section ordering: * - HDR zero (if needed) + * - String table hdr */ sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr); len = sizeof_shdr * s->shdr_num; s->elf_section_hdrs = g_malloc0(len); + buff_hdr = s->elf_section_hdrs; /* Write special section first */ if (s->phdr_num == PN_XNUM) { prepare_elf_section_hdr_zero(s); + buff_hdr += sizeof_shdr; + } + + if (s->shdr_num < 2) { + return; + } + + /* + * String table needs to be last section since strings are added + * via arch_sections_write_hdr(). + */ + write_elf_section_hdr_string(s, buff_hdr); + if (dump_is_64bit(s)) { + hdr64->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1); + } else { + hdr32->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1); } } @@ -401,11 +453,18 @@ static void write_elf_sections(DumpState *s, Error **errp) { int ret; - /* Write section zero */ + /* Write section zero and arch sections */ ret = fd_write_vmcore(s->elf_section_data, s->elf_section_data_size, s); if (ret < 0) { error_setg_errno(errp, -ret, "dump: failed to write section data"); } + + /* Write string table data */ + ret = fd_write_vmcore(s->string_table_buf->data, + s->string_table_buf->len, s); + if (ret < 0) { + error_setg_errno(errp, -ret, "dump: failed to write string table data"); + } } static void write_data(DumpState *s, void *buf, int length, Error **errp) @@ -713,6 +772,7 @@ static void create_vmcore(DumpState *s, Error **errp) return; } + /* Iterate over memory and dump it to file */ dump_iterate(s, errp); if (*errp) { return; @@ -1695,6 +1755,13 @@ static void dump_init(DumpState *s, int fd, bool has_format, s->has_filter = has_filter; s->begin = begin; s->length = length; + /* First index is 0, it's the special null name */ + s->string_table_buf = g_array_new(FALSE, TRUE, 1); + /* + * Allocate the null name, due to the clearing option set to true + * it will be 0. + */ + g_array_set_size(s->string_table_buf, 1); memory_mapping_list_init(&s->list); @@ -1855,6 +1922,18 @@ static void dump_init(DumpState *s, int fd, bool has_format, } } + /* + * calculate shdr_num and elf_section_data_size so we know the offsets and + * sizes of all parts. + * + * If phdr_num overflowed we have at least one section header + * More sections/hdrs can be added by the architectures + */ + if (s->shdr_num > 1) { + /* Reserve the string table */ + s->shdr_num += 1; + } + tmp = (s->phdr_num == PN_XNUM) ? s->sh_info : s->phdr_num; if (dump_is_64bit(s)) { s->shdr_offset = sizeof(Elf64_Ehdr); diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h index 696e6c67d6..299b611180 100644 --- a/include/sysemu/dump.h +++ b/include/sysemu/dump.h @@ -178,6 +178,7 @@ typedef struct DumpState { void *elf_section_hdrs; uint64_t elf_section_data_size; void *elf_section_data; + GArray *string_table_buf; /* String table section */ uint8_t *note_buf; /* buffer for notes */ size_t note_buf_offset; /* the writing place in note_buf */
As sections don't have a type like the notes do we need another way to determine their contents. The string table allows us to assign each section an identification string which architectures can then use to tag their sections with. There will be no string table if the architecture doesn't add custom sections which are introduced in a following patch. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- dump/dump.c | 81 ++++++++++++++++++++++++++++++++++++++++++- include/sysemu/dump.h | 1 + 2 files changed, 81 insertions(+), 1 deletion(-)