On Thu, 2022-08-11 at 12:11 +0000, Janosch Frank wrote: > Add hooks which architectures can use to add arbitrary data to custom > sections. > > Signed-off-by: Janosch Frank <fran...@linux.ibm.com> > --- > dump/dump.c | 120 ++++++++++++++++++++++++++++++------- > include/sysemu/dump-arch.h | 3 + > 2 files changed, 100 insertions(+), 23 deletions(-) > > diff --git a/dump/dump.c b/dump/dump.c > index 0d6dbf453a..65b18fc602 100644 > --- a/dump/dump.c > +++ b/dump/dump.c > @@ -430,7 +430,7 @@ static void prepare_elf_section_hdr_string(DumpState *s, > void *buff) > memcpy(buff, shdr, shdr_size); > } > > -static void prepare_elf_section_hdrs(DumpState *s) > +static void prepare_elf_section_hdrs(DumpState *s, Error **errp) > { > size_t len, sizeof_shdr; > void *buff_hdr; > @@ -438,6 +438,7 @@ static void prepare_elf_section_hdrs(DumpState *s) > /* > * Section ordering: > * - HDR zero > + * - Arch section hdrs > * - String table hdr > */ > sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr); > @@ -465,6 +466,16 @@ static void prepare_elf_section_hdrs(DumpState *s) > return; > } > > + /* Add architecture defined section headers */ > + if (s->dump_info.arch_sections_write_hdr_fn) { > + buff_hdr += s->dump_info.arch_sections_write_hdr_fn(s, buff_hdr); > + > + if (s->shdr_num >= SHN_LORESERVE) {
/* TODO: raise limit by encoding via sh_link */ > + error_setg_errno(errp, EINVAL, "dump: too many architecture > defined sections"); > + return; > + } > + } > + > /* > * String table is the last section since strings are added via > * arch_sections_write_hdr(). > @@ -477,7 +488,10 @@ static void write_elf_section_headers(DumpState *s, > Error **errp) > size_t sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : > sizeof(Elf32_Shdr); > int ret; > > - prepare_elf_section_hdrs(s); > + prepare_elf_section_hdrs(s, errp); > + if (*errp) { You're depending on errp not being NULL here, which it isn't, but it doesn't seem like good style to me. error.h recommends to also indicate success/failure via the return value if possible. prepare_elf_section_hdrs returns void right now, so it's easy so side step it this way. (ERRP_GUARD would work too of course) > + return; > + } > > ret = fd_write_vmcore(s->elf_section_hdrs, s->shdr_num * sizeof_shdr, s); > if (ret < 0) { > @@ -485,6 +499,30 @@ static void write_elf_section_headers(DumpState *s, > Error **errp) > } > } > > +static void write_elf_sections(DumpState *s, Error **errp) > +{ > + int ret; > + > + if (!s->elf_section_data_size) { > + return; > + } > + > + /* Write architecture section data */ > + 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 architecture > section data"); Looks like two spaces between section and data. > + return; > + } > + > + /* Write string table */ > + 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) > { > int ret; > @@ -744,6 +782,24 @@ static void dump_iterate(DumpState *s, Error **errp) > } > } > > +static void dump_end(DumpState *s, Error **errp) > +{ > + ERRP_GUARD(); > + > + if (!s->elf_section_data_size) { > + return; > + } > + s->elf_section_data = g_malloc0(s->elf_section_data_size); Why zero initialize the memory, do you depend on that? > + > + /* Adds the architecture defined section data to s->elf_section_data */ > + if (s->dump_info.arch_sections_write_fn) { > + s->dump_info.arch_sections_write_fn(s, s->elf_section_data); > + } > + > + /* write sections to vmcore */ > + write_elf_sections(s, errp); > +} > + > static void create_vmcore(DumpState *s, Error **errp) > { > ERRP_GUARD(); > @@ -758,6 +814,9 @@ static void create_vmcore(DumpState *s, Error **errp) > if (*errp) { > return; > } > + > + /* Write the section data */ > + dump_end(s, errp); > } > > static int write_start_flat_header(int fd) > @@ -1883,38 +1942,53 @@ static void dump_init(DumpState *s, int fd, bool > has_format, > } > > /* > - * calculate phdr_num > - * > - * the type of ehdr->e_phnum is uint16_t, so we should avoid overflow > + * Adds the number of architecture sections to shdr_num, sets > + * string_table_usage and sets elf_section_data_size so we know > + * the offsets and sizes of all parts. > */ > - s->phdr_num = 1; /* PT_NOTE */ > - if (s->list.num < UINT16_MAX - 2) { > - s->shdr_num = 0; > - s->phdr_num += s->list.num; > - } else { > - /* sh_info of section 0 holds the real number of phdrs */ > - s->shdr_num = 1; > + if (s->dump_info.arch_sections_add_fn) { > + s->dump_info.arch_sections_add_fn(s); > > - /* the type of shdr->sh_info is uint32_t, so we should avoid > overflow */ > - if (s->list.num <= UINT32_MAX - 1) { > - s->phdr_num += s->list.num; > - } else { > - s->phdr_num = UINT32_MAX; > + if (s->shdr_num) { > + s->string_table_usage = true; > } > } I'm not convinced that this is the right patch for the stuff below. > /* > - * calculate shdr_num and elf_section_data_size so we know the offsets > and > - * sizes of all parts. > + * Calculate phdr_num > * > - * If phdr_num overflowed we have at least one section header > - * More sections/hdrs can be added by the architectures > + * The absolute maximum amount of phdrs is UINT32_MAX - 1 as > + * sh_info is 32 bit. There's special handling once we go over > + * UINT16_MAX - 1 but that is handled in the ehdr and section > + * code. > */ > - if (s->shdr_num > 1) { > - /* Reserve the string table */ > + s->phdr_num = 1; /* Reserve PT_NOTE */ > + if (s->list.num <= UINT32_MAX - 1) { > + s->phdr_num += s->list.num; > + } else { > + s->phdr_num = UINT32_MAX; > + } > + > + /* > + * The first section header is always a special one in which most > + * fields are 0. > + * > + * We need it if we have custom sections and if phdr_num >= > + * PN_XNUM so we can write the real phdr_num into sh_info. > + */ > + if (s->shdr_num || s->phdr_num >= PN_XNUM) { > s->shdr_num += 1; > } > > + /* Reserve the string table for the arch sections if needed */ > + if (s->string_table_usage) { > + s->shdr_num += 1; > + } > + > + /* > + * Now that the number of section and program headers is known we > + * can calculate the offsets of the headers and data. > + */ > if (dump_is_64bit(s)) { > s->shdr_offset = sizeof(Elf64_Ehdr); > s->phdr_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num; > diff --git a/include/sysemu/dump-arch.h b/include/sysemu/dump-arch.h > index e25b02e990..824c0c1152 100644 > --- a/include/sysemu/dump-arch.h > +++ b/include/sysemu/dump-arch.h > @@ -21,6 +21,9 @@ typedef struct ArchDumpInfo { > uint32_t page_size; /* The target's page size. If it's variable and > * unknown, then this should be the maximum. */ > uint64_t phys_base; /* The target's physmem base. */ > + void (*arch_sections_add_fn)(DumpState *s); > + uint64_t (*arch_sections_write_hdr_fn)(DumpState *s, uint8_t *buff); > + void (*arch_sections_write_fn)(DumpState *s, uint8_t *buff); > } ArchDumpInfo; > > struct GuestPhysBlockList; /* memory_mapping.h */