[PATCH v4 1/2] dt-ops: Add helper API to dump fdt blob

Bhupesh Sharma bhsharma at redhat.com
Sat Jun 30 13:54:24 PDT 2018


Hi Simon,

Thanks for the review. Please see my comments inline.

On 06/27/2018 05:32 PM, Simon Horman wrote:
> On Thu, Jun 21, 2018 at 03:54:37PM +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 at redhat.com>
>> ---
>>   kexec/dt-ops.c | 141 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   kexec/dt-ops.h |   1 +
>>   2 files changed, 142 insertions(+)
>>
>> diff --git a/kexec/dt-ops.c b/kexec/dt-ops.c
>> index 915dbf55afd2..80ebfd31b4b6 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))))
>> +#define GET_CELL(p)	(p += 4, *((const uint32_t *)(p - 4)))
>> +
> 
> I see the above in scripts/dtc/util.c in the kernel tree, which is licensed
> GPL v2, whereas this file is licensed GPL v2 or later. Is there is
> also a published GPL v2 or later version somewhere?

Indeed, it is published in several places.
libfdt is GPL/BSD dual-licensed, i.e., it may be used
either under the terms of the GPL, or under the terms of the 2-clause
BSD license (aka the ISC license). See 
<https://github.com/dgibson/dtc/blob/master/README.license> for details.

I am not sure if we will have licensing issues with including a GPL/BSD 
dual-licensed code in a GPL v2 or later code. I think it should be ok, 
but IANAL :)

Do you see any possible licensing conflicts here?

>>   static const char n_chosen[] = "/chosen";
>>   
>>   static const char p_bootargs[] = "bootargs";
>> @@ -143,3 +147,140 @@ 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)
> 
> uint64_t seems to be a strange return type. Perhaps bool would be best.
> 
>> +{
>> +	const char *s = data;
>> +	const char *ss;
> 
> I think naming s as str and ss as start would enhance
> code readability.

Sure, will fix this in v5.

>> +
>> +	/* Check for zero length strings */
>> +	if (len == 0)
>> +		return 0;
>> +
>> +	/* String must be terminated with a '\0' */
>> +	if (s[len - 1] != '\0')
>> +		return 0;
>> +
>> +	ss = s;
>> +	while (*s)
>> +		s++;
>> +
>> +	/* Traverse till we hit a '\0' or reach 'len' */
>> +	if (*s != '\0')
>> +		return 0;
> 
> The two conditions above are: while s is not 0; if s != 0.
> How can the if condition ever be true?
> 
> Also, do you want to check that the characters traversed are printable?
> If not perhaps the name of the function should change.

Ok.

>> +
>> +	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
>> +		 */
>> +		if ((s + 1 - ss) > 1)
>> +			return 1;
> 
> Is this covering the case where null-terminated strings are imeded in 'data'?
> And ensuring that the is at least one non-null byte in the first string?
> If so, I think the comment could be improved, explaining things in terms of
> multiple strings. And I'm not sure you need the outermost if condition.

Yes, I can improve the comment to explain the condition you mentioned above.

>> +
>> +		return 0;
>> +	}
>> +
>> +	return 1;
>> +}
>> +
>> +static void print_data(const char* data, uint64_t len)
>> +{
>> +	uint64_t i;
>> +	const char *p_data = 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_data)),
>> +					i < (len - 4) ? " " : "");
>> +		}
>> +		dbgprintf(">");
> 
> Is there a performance benefit of using fdt32_to_cpu()
> that justifies the extra complexity here?

Yes, I see that fdt32_to_cpu() is considerably faster as compared to 
other implementations I tried.

>> +	} else {
>> +		dbgprintf(" = [");
>> +		for (i = 0; i < len; i++)
>> +			dbgprintf("%02x%s", *p_data++,
>> +					i < len - 1 ? " " : "");
>> +		dbgprintf("]");
>> +	}
>> +}
>> +
>> +void dump_fdt(void* fdt)
>> +{
>> +	struct fdt_header *bph;
>> +	const char* p_struct;
>> +	const char* p_strings;
>> +	const char* p_data;
>> +	const char* s_data;
>> +	uint32_t off_dt;
>> +	uint32_t off_str;
>> +	uint32_t tag;
>> +	uint64_t sz;
>> +	uint64_t depth;
>> +	uint64_t shift;
> 
> Can they type of depth and shift be int?
> It would avoid the (int) casting below.

Hmm. I am not sure, but I can try and use 'int' in v5, if things work 
fine during local checks.

>> +	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_data = p_struct;
>> +	while ((tag = fdt32_to_cpu(GET_CELL(p_data))) != FDT_END) {
>> +		if (tag == FDT_BEGIN_NODE) {
>> +			s_data = p_data;
>> +			p_data = PALIGN(p_data + strlen(s_data) + 1, 4);
>> +
>> +			if (*s_data == '\0')
>> +				s_data = "/";
>> +
>> +			dbgprintf("%*s%s {\n", (int)(depth * shift), " ", s_data);
>> +
>> +			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_data));
>> +		s_data = p_strings + fdt32_to_cpu(GET_CELL(p_data));
>> +		if (version < 16 && sz >= 8)
>> +			p_data = PALIGN(p_data, 8);
>> +
>> +		dbgprintf("%*s%s", (int)(depth * shift), " ", s_data);
>> +		print_data(p_data, sz);
>> +		dbgprintf(";\n");
>> +
>> +		p_data = PALIGN(p_data + sz, 4);
>> +	}
>> +}
>> diff --git a/kexec/dt-ops.h b/kexec/dt-ops.h
>> index e70d15d8ffbf..25b9b569f2b7 100644
>> --- a/kexec/dt-ops.h
>> +++ b/kexec/dt-ops.h
>> @@ -9,5 +9,6 @@ int dtb_set_property(char **dtb, off_t *dtb_size, const char *node,
>>   	const char *prop, const void *value, int value_len);
>>   
>>   int dtb_delete_property(char *dtb, const char *node, const char *prop);
>> +void dump_fdt(void *fdt) ;
>>   
>>   #endif
>> -- 
>> 2.7.4
>>

Regards,
Bhupesh



More information about the kexec mailing list