| Message ID | 20170706101611.27031-6-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/06/17 12:16, Marc-André Lureau wrote: > kdump header provides offset and size of the vmcoreinfo ELF note, > append it if available. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > dump.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/dump.c b/dump.c > index f699198204..dd416ad271 100644 > --- a/dump.c > +++ b/dump.c > @@ -839,6 +839,16 @@ static void create_header32(DumpState *s, Error **errp) > kh->dump_level = cpu_to_dump32(s, DUMP_LEVEL); > > offset_note = DISKDUMP_HEADER_BLOCKS * block_size + size; > + if (s->vmcoreinfo) { > + uint64_t hsize, name_size, size_vmcoreinfo_desc, offset_vmcoreinfo; > + > + get_note_sizes(s, s->vmcoreinfo, &hsize, &name_size, &size_vmcoreinfo_desc); > + offset_vmcoreinfo = offset_note + s->note_size - s->vmcoreinfo_size + > + (DIV_ROUND_UP(hsize, 4) + DIV_ROUND_UP(name_size, 4)) * 4; > + kh->offset_vmcoreinfo = cpu_to_dump64(s, offset_vmcoreinfo); > + kh->size_vmcoreinfo = cpu_to_dump32(s, size_vmcoreinfo_desc); > + } > + > kh->offset_note = cpu_to_dump64(s, offset_note); > kh->note_size = cpu_to_dump32(s, s->note_size); > > @@ -939,6 +949,16 @@ static void create_header64(DumpState *s, Error **errp) > kh->dump_level = cpu_to_dump32(s, DUMP_LEVEL); > > offset_note = DISKDUMP_HEADER_BLOCKS * block_size + size; > + if (s->vmcoreinfo) { > + uint64_t hsize, name_size, size_vmcoreinfo_desc, offset_vmcoreinfo; > + > + get_note_sizes(s, s->vmcoreinfo, &hsize, &name_size, &size_vmcoreinfo_desc); > + offset_vmcoreinfo = offset_note + s->note_size - s->vmcoreinfo_size + > + (DIV_ROUND_UP(hsize, 4) + DIV_ROUND_UP(name_size, 4)) * 4; > + kh->offset_vmcoreinfo = cpu_to_dump64(s, offset_vmcoreinfo); > + kh->size_vmcoreinfo = cpu_to_dump64(s, size_vmcoreinfo_desc); > + } > + > kh->offset_note = cpu_to_dump64(s, offset_note); > kh->note_size = cpu_to_dump64(s, s->note_size); > > I continue to think that this patch is unnecessary, but if you insist, it does look OK to me. Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Hi On Thu, Jul 6, 2017 at 7:13 PM, Laszlo Ersek <lersek@redhat.com> wrote: > On 07/06/17 12:16, Marc-André Lureau wrote: >> kdump header provides offset and size of the vmcoreinfo ELF note, >> append it if available. >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> --- >> dump.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> diff --git a/dump.c b/dump.c >> index f699198204..dd416ad271 100644 >> --- a/dump.c >> +++ b/dump.c >> @@ -839,6 +839,16 @@ static void create_header32(DumpState *s, Error **errp) >> kh->dump_level = cpu_to_dump32(s, DUMP_LEVEL); >> >> offset_note = DISKDUMP_HEADER_BLOCKS * block_size + size; >> + if (s->vmcoreinfo) { >> + uint64_t hsize, name_size, size_vmcoreinfo_desc, offset_vmcoreinfo; >> + >> + get_note_sizes(s, s->vmcoreinfo, &hsize, &name_size, &size_vmcoreinfo_desc); >> + offset_vmcoreinfo = offset_note + s->note_size - s->vmcoreinfo_size + >> + (DIV_ROUND_UP(hsize, 4) + DIV_ROUND_UP(name_size, 4)) * 4; >> + kh->offset_vmcoreinfo = cpu_to_dump64(s, offset_vmcoreinfo); >> + kh->size_vmcoreinfo = cpu_to_dump32(s, size_vmcoreinfo_desc); >> + } >> + >> kh->offset_note = cpu_to_dump64(s, offset_note); >> kh->note_size = cpu_to_dump32(s, s->note_size); >> >> @@ -939,6 +949,16 @@ static void create_header64(DumpState *s, Error **errp) >> kh->dump_level = cpu_to_dump32(s, DUMP_LEVEL); >> >> offset_note = DISKDUMP_HEADER_BLOCKS * block_size + size; >> + if (s->vmcoreinfo) { >> + uint64_t hsize, name_size, size_vmcoreinfo_desc, offset_vmcoreinfo; >> + >> + get_note_sizes(s, s->vmcoreinfo, &hsize, &name_size, &size_vmcoreinfo_desc); >> + offset_vmcoreinfo = offset_note + s->note_size - s->vmcoreinfo_size + >> + (DIV_ROUND_UP(hsize, 4) + DIV_ROUND_UP(name_size, 4)) * 4; >> + kh->offset_vmcoreinfo = cpu_to_dump64(s, offset_vmcoreinfo); >> + kh->size_vmcoreinfo = cpu_to_dump64(s, size_vmcoreinfo_desc); >> + } >> + >> kh->offset_note = cpu_to_dump64(s, offset_note); >> kh->note_size = cpu_to_dump64(s, s->note_size); >> >> > > I continue to think that this patch is unnecessary, but if you insist, > it does look OK to me. > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> Without it, crash doesn't read the vmcoreinfo PT_NOTE. And for some reason, the phys_base in the header wasn't enough (to be doubled checked). Any comment Dave about crash handling of vmcoreinfo in kdump files?
----- Original Message ----- > Hi > > On Thu, Jul 6, 2017 at 7:13 PM, Laszlo Ersek <lersek@redhat.com> wrote: > > On 07/06/17 12:16, Marc-André Lureau wrote: > >> kdump header provides offset and size of the vmcoreinfo ELF note, > >> append it if available. > >> > >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > >> --- > >> dump.c | 20 ++++++++++++++++++++ > >> 1 file changed, 20 insertions(+) > >> > >> diff --git a/dump.c b/dump.c > >> index f699198204..dd416ad271 100644 > >> --- a/dump.c > >> +++ b/dump.c > >> @@ -839,6 +839,16 @@ static void create_header32(DumpState *s, Error > >> **errp) > >> kh->dump_level = cpu_to_dump32(s, DUMP_LEVEL); > >> > >> offset_note = DISKDUMP_HEADER_BLOCKS * block_size + size; > >> + if (s->vmcoreinfo) { > >> + uint64_t hsize, name_size, size_vmcoreinfo_desc, > >> offset_vmcoreinfo; > >> + > >> + get_note_sizes(s, s->vmcoreinfo, &hsize, &name_size, > >> &size_vmcoreinfo_desc); > >> + offset_vmcoreinfo = offset_note + s->note_size - > >> s->vmcoreinfo_size + > >> + (DIV_ROUND_UP(hsize, 4) + DIV_ROUND_UP(name_size, 4)) * 4; > >> + kh->offset_vmcoreinfo = cpu_to_dump64(s, offset_vmcoreinfo); > >> + kh->size_vmcoreinfo = cpu_to_dump32(s, size_vmcoreinfo_desc); > >> + } > >> + > >> kh->offset_note = cpu_to_dump64(s, offset_note); > >> kh->note_size = cpu_to_dump32(s, s->note_size); > >> > >> @@ -939,6 +949,16 @@ static void create_header64(DumpState *s, Error > >> **errp) > >> kh->dump_level = cpu_to_dump32(s, DUMP_LEVEL); > >> > >> offset_note = DISKDUMP_HEADER_BLOCKS * block_size + size; > >> + if (s->vmcoreinfo) { > >> + uint64_t hsize, name_size, size_vmcoreinfo_desc, > >> offset_vmcoreinfo; > >> + > >> + get_note_sizes(s, s->vmcoreinfo, &hsize, &name_size, > >> &size_vmcoreinfo_desc); > >> + offset_vmcoreinfo = offset_note + s->note_size - > >> s->vmcoreinfo_size + > >> + (DIV_ROUND_UP(hsize, 4) + DIV_ROUND_UP(name_size, 4)) * 4; > >> + kh->offset_vmcoreinfo = cpu_to_dump64(s, offset_vmcoreinfo); > >> + kh->size_vmcoreinfo = cpu_to_dump64(s, size_vmcoreinfo_desc); > >> + } > >> + > >> kh->offset_note = cpu_to_dump64(s, offset_note); > >> kh->note_size = cpu_to_dump64(s, s->note_size); > >> > >> > > > > I continue to think that this patch is unnecessary, but if you insist, > > it does look OK to me. > > > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > Without it, crash doesn't read the vmcoreinfo PT_NOTE. And for some > reason, the phys_base in the header wasn't enough (to be doubled > checked). > > Any comment Dave about crash handling of vmcoreinfo in kdump files? It just reads the kdump_sub_header's offset_vmcoreinfo and size_vmcoreinfo fields to gather the vmcoreinfo data into a local buffer of memory, and scans the strings for whatever it's looking for. With respect to phys_base, the only thing that might be of consequence is this fairly recent change that's currently only in the github repo, queued for crash-7.2.0: commit a4a538caca140a8e948bbdae2be311168db7a1eb Author: Dave Anderson <anderson@redhat.com> Date: Tue May 2 16:51:53 2017 -0400 Fix for Linux 4.10 and later kdump dumpfiles, or kernels that have backported commit 401721ecd1dcb0a428aa5d6832ee05ffbdbffbbe, titled "kexec: export the value of phys_base instead of symbol address". Without the patch, if the x86_64 "phys_base" value in the VMCOREINFO note is a negative decimal number, the crash session fails during session intialization with a "page excluded" or "seek error" when reading "page_offset_base". (anderson@redhat.com) Also, crash-7.1.9 was the first version that started looking in the vmcoreinfo data for phys_base instead of in the kdump_sub_header. Dave > > > > -- > Marc-André Lureau >
On 07/06/17 20:09, Dave Anderson wrote: > > > ----- Original Message ----- >> Hi >> >> On Thu, Jul 6, 2017 at 7:13 PM, Laszlo Ersek <lersek@redhat.com> wrote: >>> On 07/06/17 12:16, Marc-André Lureau wrote: >>>> kdump header provides offset and size of the vmcoreinfo ELF note, >>>> append it if available. >>>> >>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>>> --- >>>> dump.c | 20 ++++++++++++++++++++ >>>> 1 file changed, 20 insertions(+) >>>> >>>> diff --git a/dump.c b/dump.c >>>> index f699198204..dd416ad271 100644 >>>> --- a/dump.c >>>> +++ b/dump.c >>>> @@ -839,6 +839,16 @@ static void create_header32(DumpState *s, Error >>>> **errp) >>>> kh->dump_level = cpu_to_dump32(s, DUMP_LEVEL); >>>> >>>> offset_note = DISKDUMP_HEADER_BLOCKS * block_size + size; >>>> + if (s->vmcoreinfo) { >>>> + uint64_t hsize, name_size, size_vmcoreinfo_desc, >>>> offset_vmcoreinfo; >>>> + >>>> + get_note_sizes(s, s->vmcoreinfo, &hsize, &name_size, >>>> &size_vmcoreinfo_desc); >>>> + offset_vmcoreinfo = offset_note + s->note_size - >>>> s->vmcoreinfo_size + >>>> + (DIV_ROUND_UP(hsize, 4) + DIV_ROUND_UP(name_size, 4)) * 4; >>>> + kh->offset_vmcoreinfo = cpu_to_dump64(s, offset_vmcoreinfo); >>>> + kh->size_vmcoreinfo = cpu_to_dump32(s, size_vmcoreinfo_desc); >>>> + } >>>> + >>>> kh->offset_note = cpu_to_dump64(s, offset_note); >>>> kh->note_size = cpu_to_dump32(s, s->note_size); >>>> >>>> @@ -939,6 +949,16 @@ static void create_header64(DumpState *s, Error >>>> **errp) >>>> kh->dump_level = cpu_to_dump32(s, DUMP_LEVEL); >>>> >>>> offset_note = DISKDUMP_HEADER_BLOCKS * block_size + size; >>>> + if (s->vmcoreinfo) { >>>> + uint64_t hsize, name_size, size_vmcoreinfo_desc, >>>> offset_vmcoreinfo; >>>> + >>>> + get_note_sizes(s, s->vmcoreinfo, &hsize, &name_size, >>>> &size_vmcoreinfo_desc); >>>> + offset_vmcoreinfo = offset_note + s->note_size - >>>> s->vmcoreinfo_size + >>>> + (DIV_ROUND_UP(hsize, 4) + DIV_ROUND_UP(name_size, 4)) * 4; >>>> + kh->offset_vmcoreinfo = cpu_to_dump64(s, offset_vmcoreinfo); >>>> + kh->size_vmcoreinfo = cpu_to_dump64(s, size_vmcoreinfo_desc); >>>> + } >>>> + >>>> kh->offset_note = cpu_to_dump64(s, offset_note); >>>> kh->note_size = cpu_to_dump64(s, s->note_size); >>>> >>>> >>> >>> I continue to think that this patch is unnecessary, but if you insist, > >>> it does look OK to me. >>> >>> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >> >> Without it, crash doesn't read the vmcoreinfo PT_NOTE. And for some >> reason, the phys_base in the header wasn't enough (to be doubled >> checked). >> >> Any comment Dave about crash handling of vmcoreinfo in kdump files? > > It just reads the kdump_sub_header's offset_vmcoreinfo and size_vmcoreinfo > fields to gather the vmcoreinfo data into a local buffer of memory, and > scans the strings for whatever it's looking for. > > With respect to phys_base, the only thing that might be of consequence > is this fairly recent change that's currently only in the github repo, > queued for crash-7.2.0: > > commit a4a538caca140a8e948bbdae2be311168db7a1eb > Author: Dave Anderson <anderson@redhat.com> > Date: Tue May 2 16:51:53 2017 -0400 > > Fix for Linux 4.10 and later kdump dumpfiles, or kernels that have > backported commit 401721ecd1dcb0a428aa5d6832ee05ffbdbffbbe, titled > "kexec: export the value of phys_base instead of symbol address". > Without the patch, if the x86_64 "phys_base" value in the VMCOREINFO > note is a negative decimal number, the crash session fails during > session intialization with a "page excluded" or "seek error" when > reading "page_offset_base". > (anderson@redhat.com) > > Also, crash-7.1.9 was the first version that started looking in the > vmcoreinfo data for phys_base instead of in the kdump_sub_header. OK, if crash (or earlier versions of crash) need this QEMU patch, then I'm fine with it -- my R-b stands. Thanks Laszlo
diff --git a/dump.c b/dump.c index f699198204..dd416ad271 100644 --- a/dump.c +++ b/dump.c @@ -839,6 +839,16 @@ static void create_header32(DumpState *s, Error **errp) kh->dump_level = cpu_to_dump32(s, DUMP_LEVEL); offset_note = DISKDUMP_HEADER_BLOCKS * block_size + size; + if (s->vmcoreinfo) { + uint64_t hsize, name_size, size_vmcoreinfo_desc, offset_vmcoreinfo; + + get_note_sizes(s, s->vmcoreinfo, &hsize, &name_size, &size_vmcoreinfo_desc); + offset_vmcoreinfo = offset_note + s->note_size - s->vmcoreinfo_size + + (DIV_ROUND_UP(hsize, 4) + DIV_ROUND_UP(name_size, 4)) * 4; + kh->offset_vmcoreinfo = cpu_to_dump64(s, offset_vmcoreinfo); + kh->size_vmcoreinfo = cpu_to_dump32(s, size_vmcoreinfo_desc); + } + kh->offset_note = cpu_to_dump64(s, offset_note); kh->note_size = cpu_to_dump32(s, s->note_size); @@ -939,6 +949,16 @@ static void create_header64(DumpState *s, Error **errp) kh->dump_level = cpu_to_dump32(s, DUMP_LEVEL); offset_note = DISKDUMP_HEADER_BLOCKS * block_size + size; + if (s->vmcoreinfo) { + uint64_t hsize, name_size, size_vmcoreinfo_desc, offset_vmcoreinfo; + + get_note_sizes(s, s->vmcoreinfo, &hsize, &name_size, &size_vmcoreinfo_desc); + offset_vmcoreinfo = offset_note + s->note_size - s->vmcoreinfo_size + + (DIV_ROUND_UP(hsize, 4) + DIV_ROUND_UP(name_size, 4)) * 4; + kh->offset_vmcoreinfo = cpu_to_dump64(s, offset_vmcoreinfo); + kh->size_vmcoreinfo = cpu_to_dump64(s, size_vmcoreinfo_desc); + } + kh->offset_note = cpu_to_dump64(s, offset_note); kh->note_size = cpu_to_dump64(s, s->note_size);
kdump header provides offset and size of the vmcoreinfo ELF note, append it if available. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- dump.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)