Hi Akashi,
Apologies for delay in replying. Somehow my email filter rules messed up
and the review email was sent to another folder.
Please see my comments inline:
On 05/08/2018 07:44 AM, AKASHI Takahiro wrote:
Bhupesh,
On Mon, Apr 30, 2018 at 02:52:35AM +0530, Bhupesh Sharma wrote:
At several occasions it would be useful to dump the fdt
blob being passed to the second (kexec/kdump) kernel
when '-d' flag is specified while invoking kexec/kdump.
This allows one to look at the device-tree fields that
is being passed to the secondary kernel and accordingly
debug issues.
This can be specially useful for the arm64 case, where
kexec_load() or kdump passes important information like
'linux,usable-memory' ranges to the second kernel, and
the correctness of the ranges can be verified by
looking at the device-tree dump with '-d' flag specified.
Signed-off-by: Bhupesh Sharma <bhsharma@xxxxxxxxxx>
---
kexec/dt-ops.c | 143 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
kexec/dt-ops.h | 1 +
2 files changed, 144 insertions(+)
diff --git a/kexec/dt-ops.c b/kexec/dt-ops.c
index 915dbf55afd2..cb95920e5c3e 100644
--- a/kexec/dt-ops.c
+++ b/kexec/dt-ops.c
@@ -8,6 +8,10 @@
#include "kexec.h"
#include "dt-ops.h"
+#define ALIGN(x, a) (((x) + ((a) - 1)) & ~((a) - 1))
+#define PALIGN(p, a) ((void *)(ALIGN((unsigned long)(p), (a))))
Pointer doesn't always fit to unsigned long.
+#define GET_CELL(p) (p += 4, *((const uint32_t *)(p-4)))
Tricky :) Inline function would be better.
Ok.
+
static const char n_chosen[] = "/chosen";
static const char p_bootargs[] = "bootargs";
@@ -143,3 +147,142 @@ int dtb_delete_property(char *dtb, const char *node, const char *prop)
return result;
}
+
+static uint64_t is_printable_string(const void* data, uint64_t len)
+{
+ const char *s = data;
+ const char *ss;
+
+ /* Check for zero length strings */
+ if (len == 0)
+ return 0;
+
+ /* String must be terminated with a '\0' */
+ if (s[len - 1] != '\0')
+ return 0;
This (== '\0') can hit even if the data is numeric.
Indeed, which means that this is not a string and this case would be
followed up by the following else checks:
<snip..>
} else if ((len % 4) == 0) {
dbgprintf(" = <");
for (i = 0; i < len; i += 4) {
dbgprintf("0x%08x%s",
fdt32_to_cpu(GET_CELL(p)),
i < (len - 4) ? " " : "");
}
dbgprintf(">");
}
<snip..>
+
+ ss = s;
+ while (*s)
+ s++;
You don't check for the length, so
See below..
+ /* Traverse till we hit a '\0' or reach 'len' */
+ if (*s != '\0')
+ return 0;
This will never hit.
.. again this means that this is not a string and this case would be
followed up by the following else checks (see above)
+ if ((s + 1 - ss) < len) {
+ /* Handle special cases such as 'bootargs' properties
+ * in dtb which are actually strings, but they may have
+ * a format where (s + 1 - ss) < len remains true.
+ *
+ * We can catch such cases by checking if (s + 1 - ss)
+ * is greater than 1
What does this actually mean? Elaborate, please.
For 'bootargs' we can have some formats (specifically for distributions
like Fedora), where we can have a additional strings added to the
primary bootargs (for e.g. LANG=en_US.UTF-8) mainly by grub
configuration files. These are not relevant while passing to the
secondary kernel and hence can be ignored while dumping the dtb blob as
well.
These additional strings makes the bootargs string a string of strings.
The above check is to catch the same, without which the
'is_printable_string' check fails and the bootargs are printed out as data.
We can go for an exhaustive implementation here to catch such cases, but
since I am not sure it will handle all such distribution specific
additional strings, so I opted for a simple implementation instead
(which works for all cases which I could test with on Fedora)
+ */
+ if ((s + 1 - ss) > 1)
+ return 1;
+
+ return 0;
+ }
+
+ return 1;
+}
+
+static void print_data(const char* data, uint64_t len)
+{
+ uint64_t i;
+ const char *p = data;
+
+ /* Check for non-zero length */
+ if (len == 0)
+ return;
+
+ if (is_printable_string(data, len)) {
+ dbgprintf(" = \"%s\"", (const char *)data);
+ } else if ((len % 4) == 0) {
+ dbgprintf(" = <");
+ for (i = 0; i < len; i += 4) {
+ dbgprintf("0x%08x%s",
+ fdt32_to_cpu(GET_CELL(p)),
+ i < (len - 4) ? " " : "");
+ }
+ dbgprintf(">");
+ } else {
+ dbgprintf(" = [");
+ for (i = 0; i < len; i++)
+ dbgprintf("%02x%s", *p++,
+ i < len - 1 ? " " : "");
+ dbgprintf("]");
+ }
+}
+
+void dump_fdt(void* fdt)
+{
+ struct fdt_header *bph;
+ const char* p_struct;
+ const char* p_strings;
+ const char* p;
+ const char* s;
+ const char* t;
I prefer more meaningful names.
Ok.
+ uint32_t off_dt;
+ uint32_t off_str;
+ uint32_t tag;
+ uint64_t sz;
+ uint64_t depth;
+ uint64_t shift;
+ uint32_t version;
+
+ depth = 0;
+ shift = 4;
+
+ bph = fdt;
+ off_dt = fdt32_to_cpu(bph->off_dt_struct);
+ off_str = fdt32_to_cpu(bph->off_dt_strings);
+ p_struct = (const char*)fdt + off_dt;
+ p_strings = (const char*)fdt + off_str;
+ version = fdt32_to_cpu(bph->version);
+
+ p = p_struct;
+ while ((tag = fdt32_to_cpu(GET_CELL(p))) != FDT_END) {
+
+ if (tag == FDT_BEGIN_NODE) {
+ s = p;
+ p = PALIGN(p + strlen(s) + 1, 4);
+
+ if (*s == '\0')
+ s = "/";
+
+ dbgprintf("%*s%s {\n", (int)(depth * shift), " ", s);
+
+ depth++;
+ continue;
+ }
+
+ if (tag == FDT_END_NODE) {
+ depth--;
+
+ dbgprintf("%*s};\n", (int)(depth * shift), " ");
+ continue;
+ }
+
+ if (tag == FDT_NOP) {
+ dbgprintf("%*s// [NOP]\n", (int)(depth * shift), " ");
+ continue;
+ }
+
+ if (tag != FDT_PROP) {
+ dbgprintf("%*s ** Unknown tag 0x%08x\n",
+ (int)(depth * shift), " ", tag);
+ break;
+ }
+ sz = fdt32_to_cpu(GET_CELL(p));
+ s = p_strings + fdt32_to_cpu(GET_CELL(p));
+ if (version < 16 && sz >= 8)
+ p = PALIGN(p, 8);
+ t = p;
't' is used only here, so
+
+ p = PALIGN(p + sz, 4);
+
+ dbgprintf("%*s%s", (int)(depth * shift), " ", s);
+ print_data(t, sz);
+ dbgprintf(";\n");
dbgprintf("%*s%s", (int)(depth * shift), " ", s);
print_data(p, sz);
dbgprintf(";\n");
p = PALIGN(p + sz, 4);
This will work.
Ok.
<..snip..>
Will send out a v4 shortly to address the comments.
Regards,
Bhupesh
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
