Re: [Qemu-devel] [PATCH 4/7] dump: add vmcoreinfo ELF note

[Laszlo Ersek]

On 07/05/17 23:52, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jul 5, 2017 at 1:48 AM, Laszlo Ersek  wrote:
>> On 06/29/17 15:23, Marc-André Lureau wrote:

>>> @@ -258,6 +280,8 @@ static void write_elf64_notes(WriteCoreDumpFunction f, 
>>> DumpState *s,
>>>  return;
>>>  }
>>>  }
>>> +
>>> +write_vmcoreinfo_note(f, s, errp);
>>>  }
>>>
>>>  static void write_elf32_note(DumpState *s, Error **errp)
>>> @@ -303,6 +327,8 @@ static void write_elf32_notes(WriteCoreDumpFunction f, 
>>> DumpState *s,
>>>  return;
>>>  }
>>>  }
>>> +
>>> +write_vmcoreinfo_note(f, s, errp);
>>>  }
>>>
>>
>> Wait, I'm confused again. You explained why it was OK to hook this logic
>> into the kdump handling too, but I don't think I understand your
>> explanation, so let me repeat my confusion below :)
>>
>> In the ELF case, this code works fine, I think. As long as the guest
>> provided us with a well-formed note, a well-formed note will be appended
>> to the ELF dump.
>>
>> But, this code is also invoked in the kdump case, and I don't understand
>> why that's a good thing. If I understand the next patch correctly, the
>> kdump format already provides crash with a (trimmed) copy of the guest
>> kernels vmcoreinfo note. So in the kdump case, why do we have to create
>> yet another copy of the guest kernel's vmcoreinfo note?
>>
>> Thus, my confusion persists, and I can only think (again) that
>> write_vmcoreinfo_note() should be called from dump_begin() only (at the
>> end). (And the s->note_size adjustment should take that into account.)
>>
>> Alternatively, we should keep this logic, and drop patch #5.
> 
> You are right, sorry for my misunderstanding, although crash is seems
> fine as long as size/offsets are ok.
> 
> So, instead of duplicating the info, let's keep the complete ELF note
> (with the header) in the dump, and simply adjust kdump header to point
> to the adjusted offset/size. This has the advantage of not making the
> code unnecessarily more complicated wrt s->note_size handling etc, and
> is consistent with the rest of the elf notes.

I guess that's OK, although I don't really see why even that is
necessary. The vmcoreinfo note should be possible to find in the kdump
output with the rest of the ELF notes, even without pointing some kdump
header fields into that specific note.

Snipping the rest because I'm going to see updates on those things in v2
anyway (which you've just posted).

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 4/7] dump: add vmcoreinfo ELF note

[Marc-André Lureau]

Hi

On Wed, Jul 5, 2017 at 1:48 AM, Laszlo Ersek  wrote:
> On 06/29/17 15:23, Marc-André Lureau wrote:
>> Read the vmcoreinfo ELF PT_NOTE from guest memory when vmcoreinfo
>> device provides the location, and write it as an ELF note in the dump.
>>
>> There are now 2 possible sources of phys_base information.
>>
>> (1) arch guessed value from arch_dump_info_get()
>
> The function is called cpu_get_dump_info().
>
>> (2) vmcoreinfo ELF note NUMBER(phys_base)= field
>>
>> NUMBER(phys_base) in vmcoreinfo has only been recently introduced
>> in Linux 4.10 (401721ecd1dc "kexec: export the value of phys_base
>> instead of symbol address").
>>
>> Since (2) has better chances to be accurate, the guessed value is
>> replaced by the value from the vmcoreinfo ELF note.
>>
>> The phys_base value is stored in the same dump field locations as
>> before, and may duplicate the information available in the vmcoreinfo
>> ELF PT_NOTE. Crash tools should be prepared to handle this case.
>>
>> Signed-off-by: Marc-André Lureau 
>> ---
>>  include/sysemu/dump.h |   2 +
>>  dump.c| 117 
>> ++
>>  2 files changed, 119 insertions(+)
>>
>> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
>> index 2672a15f8b..111a7dcaa4 100644
>> --- a/include/sysemu/dump.h
>> +++ b/include/sysemu/dump.h
>> @@ -192,6 +192,8 @@ typedef struct DumpState {
>>* this could be used to calculate
>>* how much work we have
>>* finished. */
>> +uint8_t *vmcoreinfo; /* ELF note content */
>> +size_t vmcoreinfo_size;
>>  } DumpState;
>>
>>  uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
>> diff --git a/dump.c b/dump.c
>> index d9090a24cc..8fda5cc1ed 100644
>> --- a/dump.c
>> +++ b/dump.c
>> @@ -26,6 +26,8 @@
>>  #include "qapi/qmp/qerror.h"
>>  #include "qmp-commands.h"
>>  #include "qapi-event.h"
>> +#include "qemu/error-report.h"
>> +#include "hw/acpi/vmcoreinfo.h"
>>
>>  #include 
>>  #ifdef CONFIG_LZO
>> @@ -38,6 +40,11 @@
>>  #define ELF_MACHINE_UNAME "Unknown"
>>  #endif
>>
>> +#define ELF_NOTE_SIZE(hdr_size, name_size, desc_size)   \
>> +((DIV_ROUND_UP((hdr_size), 4)   \
>> +  + DIV_ROUND_UP((name_size), 4)\
>> +  + DIV_ROUND_UP((desc_size), 4)) * 4)
>> +
>
> This looks really useful to me, but (I think?) we generally leave the
> operator hanging at the end of the line:
>
> #define ELF_NOTE_SIZE(hdr_size, name_size, desc_size) \
> ((DIV_ROUND_UP((hdr_size), 4) +   \
>   DIV_ROUND_UP((name_size), 4) +  \
>   DIV_ROUND_UP((desc_size), 4)) * 4)

ok

>
>>  uint16_t cpu_to_dump16(DumpState *s, uint16_t val)
>>  {
>>  if (s->dump_info.d_endian == ELFDATA2LSB) {
>> @@ -76,6 +83,8 @@ static int dump_cleanup(DumpState *s)
>>  guest_phys_blocks_free(>guest_phys_blocks);
>>  memory_mapping_list_free(>list);
>>  close(s->fd);
>> +g_free(s->vmcoreinfo);
>> +s->vmcoreinfo = NULL;
>>  if (s->resume) {
>>  if (s->detached) {
>>  qemu_mutex_lock_iothread();
>> @@ -235,6 +244,19 @@ static inline int cpu_index(CPUState *cpu)
>>  return cpu->cpu_index + 1;
>>  }
>>
>> +static void write_vmcoreinfo_note(WriteCoreDumpFunction f, DumpState *s,
>> +  Error **errp)
>> +{
>> +int ret;
>> +
>> +if (s->vmcoreinfo) {
>> +ret = f(s->vmcoreinfo, s->vmcoreinfo_size, s);
>> +if (ret < 0) {
>> +error_setg(errp, "dump: failed to write vmcoreinfo");
>> +}
>> +}
>> +}
>> +
>>  static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
>>Error **errp)
>>  {
>> @@ -258,6 +280,8 @@ static void write_elf64_notes(WriteCoreDumpFunction f, 
>> DumpState *s,
>>  return;
>>  }
>>  }
>> +
>> +write_vmcoreinfo_note(f, s, errp);
>>  }
>>
>>  static void write_elf32_note(DumpState *s, Error **errp)
>> @@ -303,6 +327,8 @@ static void write_elf32_notes(WriteCoreDumpFunction f, 
>> DumpState *s,
>>  return;
>>  }
>>  }
>> +
>> +write_vmcoreinfo_note(f, s, errp);
>>  }
>>
>
> Wait, I'm confused again. You explained why it was OK to hook this logic
> into the kdump handling too, but I don't think I understand your
> explanation, so let me repeat my confusion below :)
>
> In the ELF case, this code works fine, I think. As long as the guest
> provided us with a well-formed note, a well-formed note will be appended
> to the ELF dump.
>
> But, this code is also invoked in the kdump case, and I don't understand
> why that's a good thing. If I understand the next patch correctly, the
> kdump format already provides crash with a (trimmed) copy of the guest
> kernels vmcoreinfo note. So in the kdump case, why do we have to create
> yet 

Re: [Qemu-devel] [PATCH 4/7] dump: add vmcoreinfo ELF note

[Laszlo Ersek]

On 06/29/17 15:23, Marc-André Lureau wrote:
> Read the vmcoreinfo ELF PT_NOTE from guest memory when vmcoreinfo
> device provides the location, and write it as an ELF note in the dump.
> 
> There are now 2 possible sources of phys_base information.
> 
> (1) arch guessed value from arch_dump_info_get()

The function is called cpu_get_dump_info().

> (2) vmcoreinfo ELF note NUMBER(phys_base)= field
> 
> NUMBER(phys_base) in vmcoreinfo has only been recently introduced
> in Linux 4.10 (401721ecd1dc "kexec: export the value of phys_base
> instead of symbol address").
> 
> Since (2) has better chances to be accurate, the guessed value is
> replaced by the value from the vmcoreinfo ELF note.
> 
> The phys_base value is stored in the same dump field locations as
> before, and may duplicate the information available in the vmcoreinfo
> ELF PT_NOTE. Crash tools should be prepared to handle this case.
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  include/sysemu/dump.h |   2 +
>  dump.c| 117 
> ++
>  2 files changed, 119 insertions(+)
> 
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index 2672a15f8b..111a7dcaa4 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -192,6 +192,8 @@ typedef struct DumpState {
>* this could be used to calculate
>* how much work we have
>* finished. */
> +uint8_t *vmcoreinfo; /* ELF note content */
> +size_t vmcoreinfo_size;
>  } DumpState;
>  
>  uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
> diff --git a/dump.c b/dump.c
> index d9090a24cc..8fda5cc1ed 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -26,6 +26,8 @@
>  #include "qapi/qmp/qerror.h"
>  #include "qmp-commands.h"
>  #include "qapi-event.h"
> +#include "qemu/error-report.h"
> +#include "hw/acpi/vmcoreinfo.h"
>  
>  #include 
>  #ifdef CONFIG_LZO
> @@ -38,6 +40,11 @@
>  #define ELF_MACHINE_UNAME "Unknown"
>  #endif
>  
> +#define ELF_NOTE_SIZE(hdr_size, name_size, desc_size)   \
> +((DIV_ROUND_UP((hdr_size), 4)   \
> +  + DIV_ROUND_UP((name_size), 4)\
> +  + DIV_ROUND_UP((desc_size), 4)) * 4)
> +

This looks really useful to me, but (I think?) we generally leave the
operator hanging at the end of the line:

#define ELF_NOTE_SIZE(hdr_size, name_size, desc_size) \
((DIV_ROUND_UP((hdr_size), 4) +   \
  DIV_ROUND_UP((name_size), 4) +  \
  DIV_ROUND_UP((desc_size), 4)) * 4)

>  uint16_t cpu_to_dump16(DumpState *s, uint16_t val)
>  {
>  if (s->dump_info.d_endian == ELFDATA2LSB) {
> @@ -76,6 +83,8 @@ static int dump_cleanup(DumpState *s)
>  guest_phys_blocks_free(>guest_phys_blocks);
>  memory_mapping_list_free(>list);
>  close(s->fd);
> +g_free(s->vmcoreinfo);
> +s->vmcoreinfo = NULL;
>  if (s->resume) {
>  if (s->detached) {
>  qemu_mutex_lock_iothread();
> @@ -235,6 +244,19 @@ static inline int cpu_index(CPUState *cpu)
>  return cpu->cpu_index + 1;
>  }
>  
> +static void write_vmcoreinfo_note(WriteCoreDumpFunction f, DumpState *s,
> +  Error **errp)
> +{
> +int ret;
> +
> +if (s->vmcoreinfo) {
> +ret = f(s->vmcoreinfo, s->vmcoreinfo_size, s);
> +if (ret < 0) {
> +error_setg(errp, "dump: failed to write vmcoreinfo");
> +}
> +}
> +}
> +
>  static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
>Error **errp)
>  {
> @@ -258,6 +280,8 @@ static void write_elf64_notes(WriteCoreDumpFunction f, 
> DumpState *s,
>  return;
>  }
>  }
> +
> +write_vmcoreinfo_note(f, s, errp);
>  }
>  
>  static void write_elf32_note(DumpState *s, Error **errp)
> @@ -303,6 +327,8 @@ static void write_elf32_notes(WriteCoreDumpFunction f, 
> DumpState *s,
>  return;
>  }
>  }
> +
> +write_vmcoreinfo_note(f, s, errp);
>  }
>  

Wait, I'm confused again. You explained why it was OK to hook this logic
into the kdump handling too, but I don't think I understand your
explanation, so let me repeat my confusion below :)

In the ELF case, this code works fine, I think. As long as the guest
provided us with a well-formed note, a well-formed note will be appended
to the ELF dump.

But, this code is also invoked in the kdump case, and I don't understand
why that's a good thing. If I understand the next patch correctly, the
kdump format already provides crash with a (trimmed) copy of the guest
kernels vmcoreinfo note. So in the kdump case, why do we have to create
yet another copy of the guest kernel's vmcoreinfo note?

Thus, my confusion persists, and I can only think (again) that
write_vmcoreinfo_note() should be called from dump_begin() only (at the
end). (And the s->note_size 

[Qemu-devel] [PATCH 4/7] dump: add vmcoreinfo ELF note

[Marc-André Lureau]

Read the vmcoreinfo ELF PT_NOTE from guest memory when vmcoreinfo
device provides the location, and write it as an ELF note in the dump.

There are now 2 possible sources of phys_base information.

(1) arch guessed value from arch_dump_info_get()
(2) vmcoreinfo ELF note NUMBER(phys_base)= field

NUMBER(phys_base) in vmcoreinfo has only been recently introduced
in Linux 4.10 (401721ecd1dc "kexec: export the value of phys_base
instead of symbol address").

Since (2) has better chances to be accurate, the guessed value is
replaced by the value from the vmcoreinfo ELF note.

The phys_base value is stored in the same dump field locations as
before, and may duplicate the information available in the vmcoreinfo
ELF PT_NOTE. Crash tools should be prepared to handle this case.

Signed-off-by: Marc-André Lureau 
---
 include/sysemu/dump.h |   2 +
 dump.c| 117 ++
 2 files changed, 119 insertions(+)

diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 2672a15f8b..111a7dcaa4 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -192,6 +192,8 @@ typedef struct DumpState {
   * this could be used to calculate
   * how much work we have
   * finished. */
+uint8_t *vmcoreinfo; /* ELF note content */
+size_t vmcoreinfo_size;
 } DumpState;
 
 uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
diff --git a/dump.c b/dump.c
index d9090a24cc..8fda5cc1ed 100644
--- a/dump.c
+++ b/dump.c
@@ -26,6 +26,8 @@
 #include "qapi/qmp/qerror.h"
 #include "qmp-commands.h"
 #include "qapi-event.h"
+#include "qemu/error-report.h"
+#include "hw/acpi/vmcoreinfo.h"
 
 #include 
 #ifdef CONFIG_LZO
@@ -38,6 +40,11 @@
 #define ELF_MACHINE_UNAME "Unknown"
 #endif
 
+#define ELF_NOTE_SIZE(hdr_size, name_size, desc_size)   \
+((DIV_ROUND_UP((hdr_size), 4)   \
+  + DIV_ROUND_UP((name_size), 4)\
+  + DIV_ROUND_UP((desc_size), 4)) * 4)
+
 uint16_t cpu_to_dump16(DumpState *s, uint16_t val)
 {
 if (s->dump_info.d_endian == ELFDATA2LSB) {
@@ -76,6 +83,8 @@ static int dump_cleanup(DumpState *s)
 guest_phys_blocks_free(>guest_phys_blocks);
 memory_mapping_list_free(>list);
 close(s->fd);
+g_free(s->vmcoreinfo);
+s->vmcoreinfo = NULL;
 if (s->resume) {
 if (s->detached) {
 qemu_mutex_lock_iothread();
@@ -235,6 +244,19 @@ static inline int cpu_index(CPUState *cpu)
 return cpu->cpu_index + 1;
 }
 
+static void write_vmcoreinfo_note(WriteCoreDumpFunction f, DumpState *s,
+  Error **errp)
+{
+int ret;
+
+if (s->vmcoreinfo) {
+ret = f(s->vmcoreinfo, s->vmcoreinfo_size, s);
+if (ret < 0) {
+error_setg(errp, "dump: failed to write vmcoreinfo");
+}
+}
+}
+
 static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
   Error **errp)
 {
@@ -258,6 +280,8 @@ static void write_elf64_notes(WriteCoreDumpFunction f, 
DumpState *s,
 return;
 }
 }
+
+write_vmcoreinfo_note(f, s, errp);
 }
 
 static void write_elf32_note(DumpState *s, Error **errp)
@@ -303,6 +327,8 @@ static void write_elf32_notes(WriteCoreDumpFunction f, 
DumpState *s,
 return;
 }
 }
+
+write_vmcoreinfo_note(f, s, errp);
 }
 
 static void write_elf_section(DumpState *s, int type, Error **errp)
@@ -714,6 +740,44 @@ static int buf_write_note(const void *buf, size_t size, 
void *opaque)
 return 0;
 }
 
+/*
+ * This function retrieves various sizes from an elf header.
+ *
+ * @note has to be a valid ELF note. The return sizes are unmodified
+ * (not padded or rounded up to be multiple of 4).
+ */
+static void get_note_sizes(DumpState *s, const void *note,
+   uint64_t *note_head_size,
+   uint64_t *name_size,
+   uint64_t *desc_size)
+{
+uint64_t note_head_sz;
+uint64_t name_sz;
+uint64_t desc_sz;
+
+if (s->dump_info.d_class == ELFCLASS64) {
+const Elf64_Nhdr *hdr = note;
+note_head_sz = sizeof(Elf64_Nhdr);
+name_sz = hdr->n_namesz;
+desc_sz = hdr->n_descsz;
+} else {
+const Elf32_Nhdr *hdr = note;
+note_head_sz = sizeof(Elf32_Nhdr);
+name_sz = hdr->n_namesz;
+desc_sz = hdr->n_descsz;
+}
+
+if (note_head_size) {
+*note_head_size = note_head_sz;
+}
+if (name_size) {
+*name_size = name_sz;
+}
+if (desc_size) {
+*desc_size = desc_sz;
+}
+}
+
 /* write common header, sub header and elf note to vmcore */
 static void create_header32(DumpState *s, Error **errp)
 {
@@ -1488,10 +1552,38 @@ static int64_t dump_calculate_size(DumpState *s)
 return total;
 }
 
+static void