| Message ID | 20220811121111.9878-10-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dump: Add arch section and s390x PV dump | expand |
Hi On Thu, Aug 11, 2022 at 4:16 PM Janosch Frank <frankja@linux.ibm.com> wrote: > Currently we're writing the NULL section header if we overflow the > physical header number in the ELF header. But in the future we'll add > custom section headers AND section data. > > To facilitate this we need to rearange section handling a bit. As with > the other ELF headers we split the code into a prepare and a write > step. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > dump/dump.c | 83 +++++++++++++++++++++++++++++-------------- > include/sysemu/dump.h | 2 ++ > 2 files changed, 58 insertions(+), 27 deletions(-) > > diff --git a/dump/dump.c b/dump/dump.c > index a905316fe5..0051c71d08 100644 > --- a/dump/dump.c > +++ b/dump/dump.c > @@ -380,30 +380,57 @@ static void write_elf_phdr_note(DumpState *s, Error > **errp) > } > } > > -static void write_elf_section(DumpState *s, int type, Error **errp) > +static void prepare_elf_section_hdr_zero(DumpState *s) > { > - Elf32_Shdr shdr32; > - Elf64_Shdr shdr64; > - int shdr_size; > - void *shdr; > + if (dump_is_64bit(s)) { > + Elf64_Shdr *shdr64 = s->elf_section_hdrs; > + > + shdr64->sh_info = cpu_to_dump32(s, s->phdr_num); > + } else { > + Elf32_Shdr *shdr32 = s->elf_section_hdrs; > + > + shdr32->sh_info = cpu_to_dump32(s, s->phdr_num); > + } > +} > + > +static void prepare_elf_section_hdrs(DumpState *s) > +{ > + size_t len, sizeof_shdr; > + > + /* > + * Section ordering: > + * - HDR zero > + */ > + 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); > + > + /* > + * The first section header is ALWAYS a special initial section > + * header. > + * > + * The header should be 0 with one exception being that if > + * phdr_num is PN_XNUM then the sh_info field contains the real > + * number of segment entries. > + * > + * As we zero allocate the buffer we will only need to modify > + * sh_info for the PN_XNUM case. > + */ > + if (s->phdr_num >= PN_XNUM) { > + prepare_elf_section_hdr_zero(s); > + } > +} > + > +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; > > - if (type == 0) { > - shdr_size = sizeof(Elf32_Shdr); > - memset(&shdr32, 0, shdr_size); > - 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->phdr_num); > - shdr = &shdr64; > - } > + prepare_elf_section_hdrs(s); > > - ret = fd_write_vmcore(shdr, shdr_size, s); > + ret = fd_write_vmcore(s->elf_section_hdrs, s->shdr_num * sizeof_shdr, > s); > if (ret < 0) { > - error_setg_errno(errp, -ret, > - "dump: failed to write section header table"); > + error_setg_errno(errp, -ret, "dump: failed to write section > headers"); > } > } > > @@ -579,6 +606,12 @@ static void dump_begin(DumpState *s, Error **errp) > return; > } > > + /* write section headers to vmcore */ > + write_elf_section_headers(s, errp); > + if (*errp) { > + return; > + } > Can you make that move a separate commit? And please explain why this is valid, and also update the table in the comment too. Otherwise, changes look good to me. + > /* write PT_NOTE to vmcore */ > write_elf_phdr_note(s, errp); > if (*errp) { > @@ -591,14 +624,6 @@ static void dump_begin(DumpState *s, Error **errp) > return; > } > > - /* write section to vmcore */ > - if (s->shdr_num) { > - write_elf_section(s, 1, errp); > - if (*errp) { > - return; > - } > - } > - > /* write notes to vmcore */ > write_elf_notes(s, errp); > } > @@ -674,7 +699,11 @@ static void create_vmcore(DumpState *s, Error **errp) > return; > } > > + /* Iterate over memory and dump it to file */ > dump_iterate(s, errp); > + if (*errp) { > + return; > + } > } > > static int write_start_flat_header(int fd) > diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h > index b62513d87d..9995f65dc8 100644 > --- a/include/sysemu/dump.h > +++ b/include/sysemu/dump.h > @@ -177,6 +177,8 @@ typedef struct DumpState { > int64_t filter_area_begin; /* Start address of partial guest memory > area */ > int64_t filter_area_length; /* Length of partial guest memory area */ > > + void *elf_section_hdrs; /* Pointer to section header buffer */ > + > uint8_t *note_buf; /* buffer for notes */ > size_t note_buf_offset; /* the writing place in note_buf */ > uint32_t nr_cpus; /* number of guest's cpu */ > -- > 2.34.1 > > >
On Thu, 2022-08-11 at 12:11 +0000, Janosch Frank wrote: > Currently we're writing the NULL section header if we overflow the > physical header number in the ELF header. But in the future we'll add > custom section headers AND section data. > > To facilitate this we need to rearange section handling a bit. As with > the other ELF headers we split the code into a prepare and a write > step. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > dump/dump.c | 83 +++++++++++++++++++++++++++++-------------- > include/sysemu/dump.h | 2 ++ > 2 files changed, 58 insertions(+), 27 deletions(-) > > diff --git a/dump/dump.c b/dump/dump.c > index a905316fe5..0051c71d08 100644 > --- a/dump/dump.c > +++ b/dump/dump.c > @@ -380,30 +380,57 @@ static void write_elf_phdr_note(DumpState *s, Error **errp) > } > } > > -static void write_elf_section(DumpState *s, int type, Error **errp) > +static void prepare_elf_section_hdr_zero(DumpState *s) > { > - Elf32_Shdr shdr32; > - Elf64_Shdr shdr64; > - int shdr_size; > - void *shdr; > + if (dump_is_64bit(s)) { > + Elf64_Shdr *shdr64 = s->elf_section_hdrs; > + > + shdr64->sh_info = cpu_to_dump32(s, s->phdr_num); > + } else { > + Elf32_Shdr *shdr32 = s->elf_section_hdrs; > + > + shdr32->sh_info = cpu_to_dump32(s, s->phdr_num); > + } > +} > + > +static void prepare_elf_section_hdrs(DumpState *s) > +{ > + size_t len, sizeof_shdr; > + > + /* > + * Section ordering: > + * - HDR zero > + */ > + 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); I'm not seeing this being freed. > + > + /* > + * The first section header is ALWAYS a special initial section > + * header. > + * > + * The header should be 0 with one exception being that if > + * phdr_num is PN_XNUM then the sh_info field contains the real > + * number of segment entries. > + * > + * As we zero allocate the buffer we will only need to modify > + * sh_info for the PN_XNUM case. > + */ > + if (s->phdr_num >= PN_XNUM) { > + prepare_elf_section_hdr_zero(s); > + } > +} > + > +static void write_elf_section_headers(DumpState *s, Error **errp) [...] > @@ -579,6 +606,12 @@ static void dump_begin(DumpState *s, Error **errp) > return; > } > > + /* write section headers to vmcore */ > + write_elf_section_headers(s, errp); > + if (*errp) { > + return; > + } > + > /* write PT_NOTE to vmcore */ > write_elf_phdr_note(s, errp); > if (*errp) { > @@ -591,14 +624,6 @@ static void dump_begin(DumpState *s, Error **errp) > return; > } > > - /* write section to vmcore */ > - if (s->shdr_num) { > - write_elf_section(s, 1, errp); > - if (*errp) { > - return; > - } > - } > - Here you change the order of the headers, but the elf header is only fixed in patch 11. I agree that this should be a separate patch, with an explanation on why it's necessary. So basically squashed into patch 11, except I think the comment change in that one should go into another patch. > /* write notes to vmcore */ > write_elf_notes(s, errp); > } > @@ -674,7 +699,11 @@ static void create_vmcore(DumpState *s, Error **errp) > return; > } > > + /* Iterate over memory and dump it to file */ > dump_iterate(s, errp); > + if (*errp) { > + return; > + } > } > > static int write_start_flat_header(int fd) > diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h > index b62513d87d..9995f65dc8 100644 > --- a/include/sysemu/dump.h > +++ b/include/sysemu/dump.h > @@ -177,6 +177,8 @@ typedef struct DumpState { > int64_t filter_area_begin; /* Start address of partial guest memory area */ > int64_t filter_area_length; /* Length of partial guest memory area */ > > + void *elf_section_hdrs; /* Pointer to section header buffer */ > + > uint8_t *note_buf; /* buffer for notes */ > size_t note_buf_offset; /* the writing place in note_buf */ > uint32_t nr_cpus; /* number of guest's cpu */
diff --git a/dump/dump.c b/dump/dump.c index a905316fe5..0051c71d08 100644 --- a/dump/dump.c +++ b/dump/dump.c @@ -380,30 +380,57 @@ static void write_elf_phdr_note(DumpState *s, Error **errp) } } -static void write_elf_section(DumpState *s, int type, Error **errp) +static void prepare_elf_section_hdr_zero(DumpState *s) { - Elf32_Shdr shdr32; - Elf64_Shdr shdr64; - int shdr_size; - void *shdr; + if (dump_is_64bit(s)) { + Elf64_Shdr *shdr64 = s->elf_section_hdrs; + + shdr64->sh_info = cpu_to_dump32(s, s->phdr_num); + } else { + Elf32_Shdr *shdr32 = s->elf_section_hdrs; + + shdr32->sh_info = cpu_to_dump32(s, s->phdr_num); + } +} + +static void prepare_elf_section_hdrs(DumpState *s) +{ + size_t len, sizeof_shdr; + + /* + * Section ordering: + * - HDR zero + */ + 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); + + /* + * The first section header is ALWAYS a special initial section + * header. + * + * The header should be 0 with one exception being that if + * phdr_num is PN_XNUM then the sh_info field contains the real + * number of segment entries. + * + * As we zero allocate the buffer we will only need to modify + * sh_info for the PN_XNUM case. + */ + if (s->phdr_num >= PN_XNUM) { + prepare_elf_section_hdr_zero(s); + } +} + +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; - if (type == 0) { - shdr_size = sizeof(Elf32_Shdr); - memset(&shdr32, 0, shdr_size); - 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->phdr_num); - shdr = &shdr64; - } + prepare_elf_section_hdrs(s); - ret = fd_write_vmcore(shdr, shdr_size, s); + ret = fd_write_vmcore(s->elf_section_hdrs, s->shdr_num * sizeof_shdr, s); if (ret < 0) { - error_setg_errno(errp, -ret, - "dump: failed to write section header table"); + error_setg_errno(errp, -ret, "dump: failed to write section headers"); } } @@ -579,6 +606,12 @@ static void dump_begin(DumpState *s, Error **errp) return; } + /* write section headers to vmcore */ + write_elf_section_headers(s, errp); + if (*errp) { + return; + } + /* write PT_NOTE to vmcore */ write_elf_phdr_note(s, errp); if (*errp) { @@ -591,14 +624,6 @@ static void dump_begin(DumpState *s, Error **errp) return; } - /* write section to vmcore */ - if (s->shdr_num) { - write_elf_section(s, 1, errp); - if (*errp) { - return; - } - } - /* write notes to vmcore */ write_elf_notes(s, errp); } @@ -674,7 +699,11 @@ static void create_vmcore(DumpState *s, Error **errp) return; } + /* Iterate over memory and dump it to file */ dump_iterate(s, errp); + if (*errp) { + return; + } } static int write_start_flat_header(int fd) diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h index b62513d87d..9995f65dc8 100644 --- a/include/sysemu/dump.h +++ b/include/sysemu/dump.h @@ -177,6 +177,8 @@ typedef struct DumpState { int64_t filter_area_begin; /* Start address of partial guest memory area */ int64_t filter_area_length; /* Length of partial guest memory area */ + void *elf_section_hdrs; /* Pointer to section header buffer */ + uint8_t *note_buf; /* buffer for notes */ size_t note_buf_offset; /* the writing place in note_buf */ uint32_t nr_cpus; /* number of guest's cpu */
Currently we're writing the NULL section header if we overflow the physical header number in the ELF header. But in the future we'll add custom section headers AND section data. To facilitate this we need to rearange section handling a bit. As with the other ELF headers we split the code into a prepare and a write step. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- dump/dump.c | 83 +++++++++++++++++++++++++++++-------------- include/sysemu/dump.h | 2 ++ 2 files changed, 58 insertions(+), 27 deletions(-)