[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 13/18] dump/dump: Add arch section support
From: |
Janis Schoetterl-Glausch |
Subject: |
Re: [PATCH v5 13/18] dump/dump: Add arch section support |
Date: |
Thu, 01 Sep 2022 11:26:13 +0200 |
User-agent: |
Evolution 3.42.4 (3.42.4-2.fc35) |
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 <frankja@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 */
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH v5 13/18] dump/dump: Add arch section support,
Janis Schoetterl-Glausch <=