public inbox for kexec@lists.infradead.org
 help / color / mirror / Atom feed
* [PATCH] kexec/fs2dt: fix endianess conversion
@ 2013-10-15 17:05 Laurent Dufour
  2013-10-16  7:28 ` Simon Horman
  0 siblings, 1 reply; 5+ messages in thread
From: Laurent Dufour @ 2013-10-15 17:05 UTC (permalink / raw)
  To: horms, kexec

While reviewing fs2dt.c in the common kexec directory, in order to use it to
support little endian ppc64 architecture, I found some endianess
conversion's issues.

In dt_reserve, dt_base is a pointer and thus should not be converted.

In checkprop, values read from the device tree are big endian encoded and
should be converted if CPU is running in little endian mode.

In add_dyn_reconf_usable_mem_property__, fix extraneous endianess conversion
of rlen which should be natively used to increase the dt pointer.

Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
---
 kexec/fs2dt.c |   19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/kexec/fs2dt.c b/kexec/fs2dt.c
index 242a15e..5d774ae 100644
--- a/kexec/fs2dt.c
+++ b/kexec/fs2dt.c
@@ -84,7 +84,7 @@ static void dt_reserve(unsigned **dt_ptr, unsigned words)
 		offset = *dt_ptr - dt_base;
 		dt_base = new_dt;
 		dt_cur_size = new_size;
-		*dt_ptr = cpu_to_be32((unsigned)dt_base + offset);
+		*dt_ptr = dt_base + offset;
 		memset(*dt_ptr, 0, (new_size - offset)*4);
 	}
 }
@@ -112,19 +112,22 @@ static void checkprop(char *name, unsigned *data, int len)
 	if ((data == NULL) && (base || size || end))
 		die("unrecoverable error: no property data");
 	else if (!strcmp(name, "linux,rtas-base"))
-		base = *data;
+		base = be32_to_cpu(*data);
 	else if (!strcmp(name, "linux,tce-base"))
-		base = *(unsigned long long *) data;
+		base = be64_to_cpu(*(unsigned long long *) data);
 	else if (!strcmp(name, "rtas-size") ||
 			!strcmp(name, "linux,tce-size"))
-		size = *data;
+		size = be32_to_cpu(*data);
 	else if (reuse_initrd && !strcmp(name, "linux,initrd-start"))
 		if (len == 8)
-			base = *(unsigned long long *) data;
+			base = be64_to_cpu(*(unsigned long long *) data);
 		else
-			base = *data;
+			base = be32_to_cpu(*data);
 	else if (reuse_initrd && !strcmp(name, "linux,initrd-end"))
-		end = *(unsigned long long *) data;
+		if (len == 8)
+			end = be64_to_cpu(*(unsigned long long *) data);
+		else
+			end = be32_to_cpu(*data);
 
 	if (size && end)
 		die("unrecoverable error: size and end set at same time\n");
@@ -267,7 +270,7 @@ static void add_dyn_reconf_usable_mem_property__(int fd)
 	pad_structure_block(rlen);
 	memcpy(dt, ranges, rlen);
 	free(ranges);
-	dt += cpu_to_be32((rlen + 3)/4);
+	dt += (rlen + 3)/4;
 }
 
 static void add_dyn_reconf_usable_mem_property(struct dirent *dp, int fd)


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] kexec/fs2dt: fix endianess conversion
  2013-10-15 17:05 [PATCH] kexec/fs2dt: fix endianess conversion Laurent Dufour
@ 2013-10-16  7:28 ` Simon Horman
  2013-10-16  9:33   ` Laurent Dufour
  0 siblings, 1 reply; 5+ messages in thread
From: Simon Horman @ 2013-10-16  7:28 UTC (permalink / raw)
  To: Laurent Dufour; +Cc: kexec

On Tue, Oct 15, 2013 at 07:05:28PM +0200, Laurent Dufour wrote:
> While reviewing fs2dt.c in the common kexec directory, in order to use it to
> support little endian ppc64 architecture, I found some endianess
> conversion's issues.
> 
> In dt_reserve, dt_base is a pointer and thus should not be converted.
> 
> In checkprop, values read from the device tree are big endian encoded and
> should be converted if CPU is running in little endian mode.
> 
> In add_dyn_reconf_usable_mem_property__, fix extraneous endianess conversion
> of rlen which should be natively used to increase the dt pointer.

These changes seem logical to me.

However, I'm unsure how the code might have worked as-is.
In particular I'm pretty sure ARM was running in little endian
mode when I worked on DT support for it. Perhaps these code
paths were not executed. Or the errors cancelled themselves out somehow?

> 
> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> ---
>  kexec/fs2dt.c |   19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/kexec/fs2dt.c b/kexec/fs2dt.c
> index 242a15e..5d774ae 100644
> --- a/kexec/fs2dt.c
> +++ b/kexec/fs2dt.c
> @@ -84,7 +84,7 @@ static void dt_reserve(unsigned **dt_ptr, unsigned words)
>  		offset = *dt_ptr - dt_base;
>  		dt_base = new_dt;
>  		dt_cur_size = new_size;
> -		*dt_ptr = cpu_to_be32((unsigned)dt_base + offset);
> +		*dt_ptr = dt_base + offset;
>  		memset(*dt_ptr, 0, (new_size - offset)*4);
>  	}
>  }
> @@ -112,19 +112,22 @@ static void checkprop(char *name, unsigned *data, int len)
>  	if ((data == NULL) && (base || size || end))
>  		die("unrecoverable error: no property data");
>  	else if (!strcmp(name, "linux,rtas-base"))
> -		base = *data;
> +		base = be32_to_cpu(*data);
>  	else if (!strcmp(name, "linux,tce-base"))
> -		base = *(unsigned long long *) data;
> +		base = be64_to_cpu(*(unsigned long long *) data);
>  	else if (!strcmp(name, "rtas-size") ||
>  			!strcmp(name, "linux,tce-size"))
> -		size = *data;
> +		size = be32_to_cpu(*data);
>  	else if (reuse_initrd && !strcmp(name, "linux,initrd-start"))
>  		if (len == 8)
> -			base = *(unsigned long long *) data;
> +			base = be64_to_cpu(*(unsigned long long *) data);
>  		else
> -			base = *data;
> +			base = be32_to_cpu(*data);
>  	else if (reuse_initrd && !strcmp(name, "linux,initrd-end"))
> -		end = *(unsigned long long *) data;
> +		if (len == 8)
> +			end = be64_to_cpu(*(unsigned long long *) data);
> +		else
> +			end = be32_to_cpu(*data);
>  
>  	if (size && end)
>  		die("unrecoverable error: size and end set at same time\n");
> @@ -267,7 +270,7 @@ static void add_dyn_reconf_usable_mem_property__(int fd)
>  	pad_structure_block(rlen);
>  	memcpy(dt, ranges, rlen);
>  	free(ranges);
> -	dt += cpu_to_be32((rlen + 3)/4);
> +	dt += (rlen + 3)/4;
>  }
>  
>  static void add_dyn_reconf_usable_mem_property(struct dirent *dp, int fd)
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] kexec/fs2dt: fix endianess conversion
  2013-10-16  7:28 ` Simon Horman
@ 2013-10-16  9:33   ` Laurent Dufour
  2013-10-17  1:42     ` Simon Horman
  0 siblings, 1 reply; 5+ messages in thread
From: Laurent Dufour @ 2013-10-16  9:33 UTC (permalink / raw)
  To: Simon Horman; +Cc: kexec

On 16/10/2013 09:28, Simon Horman wrote:
> On Tue, Oct 15, 2013 at 07:05:28PM +0200, Laurent Dufour wrote:
>> While reviewing fs2dt.c in the common kexec directory, in order to use it to
>> support little endian ppc64 architecture, I found some endianess
>> conversion's issues.
>>
>> In dt_reserve, dt_base is a pointer and thus should not be converted.
>>
>> In checkprop, values read from the device tree are big endian encoded and
>> should be converted if CPU is running in little endian mode.
>>
>> In add_dyn_reconf_usable_mem_property__, fix extraneous endianess conversion
>> of rlen which should be natively used to increase the dt pointer.
> 
> These changes seem logical to me.
> 
> However, I'm unsure how the code might have worked as-is.
> In particular I'm pretty sure ARM was running in little endian
> mode when I worked on DT support for it. Perhaps these code
> paths were not executed. Or the errors cancelled themselves out somehow?

I've to admit that I don't know how this has worked for ARM.
I run this patch with a set of change I'm working on to support PPC64
little endian architecture. My tests were done with kexec-tools built in
little endian mode, using this fs2dt.c file and I need this fix to build
a valid device tree.

The fix is dt_reserve is hit unless the device tree is large enough. I
had to reduce INIT_TREE_WORDS to hit it.

I think the ones in checkprop, add_dyn_reconf_usable_mem_property__  are
Power specific since RTAS and 'ibm,dynamic-memory' are required to hit them.

This may explain why this issue are not seen in ARM mode.

This being said, I wondering if the files fs2dt.c defined in the
arch/ppc64 and arch/ppc directories need to be kept. What do you think
about removing those files and belong on the common kexec/fs2dt.c file
for all the supported architecture ?

Cheers,
Laurent.

>>
>> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>> ---
>>  kexec/fs2dt.c |   19 +++++++++++--------
>>  1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/kexec/fs2dt.c b/kexec/fs2dt.c
>> index 242a15e..5d774ae 100644
>> --- a/kexec/fs2dt.c
>> +++ b/kexec/fs2dt.c
>> @@ -84,7 +84,7 @@ static void dt_reserve(unsigned **dt_ptr, unsigned words)
>>  		offset = *dt_ptr - dt_base;
>>  		dt_base = new_dt;
>>  		dt_cur_size = new_size;
>> -		*dt_ptr = cpu_to_be32((unsigned)dt_base + offset);
>> +		*dt_ptr = dt_base + offset;
>>  		memset(*dt_ptr, 0, (new_size - offset)*4);
>>  	}
>>  }
>> @@ -112,19 +112,22 @@ static void checkprop(char *name, unsigned *data, int len)
>>  	if ((data == NULL) && (base || size || end))
>>  		die("unrecoverable error: no property data");
>>  	else if (!strcmp(name, "linux,rtas-base"))
>> -		base = *data;
>> +		base = be32_to_cpu(*data);
>>  	else if (!strcmp(name, "linux,tce-base"))
>> -		base = *(unsigned long long *) data;
>> +		base = be64_to_cpu(*(unsigned long long *) data);
>>  	else if (!strcmp(name, "rtas-size") ||
>>  			!strcmp(name, "linux,tce-size"))
>> -		size = *data;
>> +		size = be32_to_cpu(*data);
>>  	else if (reuse_initrd && !strcmp(name, "linux,initrd-start"))
>>  		if (len == 8)
>> -			base = *(unsigned long long *) data;
>> +			base = be64_to_cpu(*(unsigned long long *) data);
>>  		else
>> -			base = *data;
>> +			base = be32_to_cpu(*data);
>>  	else if (reuse_initrd && !strcmp(name, "linux,initrd-end"))
>> -		end = *(unsigned long long *) data;
>> +		if (len == 8)
>> +			end = be64_to_cpu(*(unsigned long long *) data);
>> +		else
>> +			end = be32_to_cpu(*data);
>>  
>>  	if (size && end)
>>  		die("unrecoverable error: size and end set at same time\n");
>> @@ -267,7 +270,7 @@ static void add_dyn_reconf_usable_mem_property__(int fd)
>>  	pad_structure_block(rlen);
>>  	memcpy(dt, ranges, rlen);
>>  	free(ranges);
>> -	dt += cpu_to_be32((rlen + 3)/4);
>> +	dt += (rlen + 3)/4;
>>  }
>>  
>>  static void add_dyn_reconf_usable_mem_property(struct dirent *dp, int fd)
>>
> 


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] kexec/fs2dt: fix endianess conversion
  2013-10-16  9:33   ` Laurent Dufour
@ 2013-10-17  1:42     ` Simon Horman
  2013-10-17  1:44       ` Simon Horman
  0 siblings, 1 reply; 5+ messages in thread
From: Simon Horman @ 2013-10-17  1:42 UTC (permalink / raw)
  To: Laurent Dufour; +Cc: kexec

On Wed, Oct 16, 2013 at 11:33:43AM +0200, Laurent Dufour wrote:
> On 16/10/2013 09:28, Simon Horman wrote:
> > On Tue, Oct 15, 2013 at 07:05:28PM +0200, Laurent Dufour wrote:
> >> While reviewing fs2dt.c in the common kexec directory, in order to use it to
> >> support little endian ppc64 architecture, I found some endianess
> >> conversion's issues.
> >>
> >> In dt_reserve, dt_base is a pointer and thus should not be converted.
> >>
> >> In checkprop, values read from the device tree are big endian encoded and
> >> should be converted if CPU is running in little endian mode.
> >>
> >> In add_dyn_reconf_usable_mem_property__, fix extraneous endianess conversion
> >> of rlen which should be natively used to increase the dt pointer.
> > 
> > These changes seem logical to me.
> > 
> > However, I'm unsure how the code might have worked as-is.
> > In particular I'm pretty sure ARM was running in little endian
> > mode when I worked on DT support for it. Perhaps these code
> > paths were not executed. Or the errors cancelled themselves out somehow?
> 
> I've to admit that I don't know how this has worked for ARM.
> I run this patch with a set of change I'm working on to support PPC64
> little endian architecture. My tests were done with kexec-tools built in
> little endian mode, using this fs2dt.c file and I need this fix to build
> a valid device tree.
> 
> The fix is dt_reserve is hit unless the device tree is large enough. I
> had to reduce INIT_TREE_WORDS to hit it.
> 
> I think the ones in checkprop, add_dyn_reconf_usable_mem_property__  are
> Power specific since RTAS and 'ibm,dynamic-memory' are required to hit them.
> 
> This may explain why this issue are not seen in ARM mode.

Perhaps, its a bit of a mystery.
I think that I will apply your change as it does seem to be correct.

> 
> This being said, I wondering if the files fs2dt.c defined in the
> arch/ppc64 and arch/ppc directories need to be kept. What do you think
> about removing those files and belong on the common kexec/fs2dt.c file
> for all the supported architecture ?

I haven't examined the details but that sounds like a good idea to me.

> 
> Cheers,
> Laurent.
> 
> >>
> >> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> >> ---
> >>  kexec/fs2dt.c |   19 +++++++++++--------
> >>  1 file changed, 11 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/kexec/fs2dt.c b/kexec/fs2dt.c
> >> index 242a15e..5d774ae 100644
> >> --- a/kexec/fs2dt.c
> >> +++ b/kexec/fs2dt.c
> >> @@ -84,7 +84,7 @@ static void dt_reserve(unsigned **dt_ptr, unsigned words)
> >>  		offset = *dt_ptr - dt_base;
> >>  		dt_base = new_dt;
> >>  		dt_cur_size = new_size;
> >> -		*dt_ptr = cpu_to_be32((unsigned)dt_base + offset);
> >> +		*dt_ptr = dt_base + offset;
> >>  		memset(*dt_ptr, 0, (new_size - offset)*4);
> >>  	}
> >>  }
> >> @@ -112,19 +112,22 @@ static void checkprop(char *name, unsigned *data, int len)
> >>  	if ((data == NULL) && (base || size || end))
> >>  		die("unrecoverable error: no property data");
> >>  	else if (!strcmp(name, "linux,rtas-base"))
> >> -		base = *data;
> >> +		base = be32_to_cpu(*data);
> >>  	else if (!strcmp(name, "linux,tce-base"))
> >> -		base = *(unsigned long long *) data;
> >> +		base = be64_to_cpu(*(unsigned long long *) data);
> >>  	else if (!strcmp(name, "rtas-size") ||
> >>  			!strcmp(name, "linux,tce-size"))
> >> -		size = *data;
> >> +		size = be32_to_cpu(*data);
> >>  	else if (reuse_initrd && !strcmp(name, "linux,initrd-start"))
> >>  		if (len == 8)
> >> -			base = *(unsigned long long *) data;
> >> +			base = be64_to_cpu(*(unsigned long long *) data);
> >>  		else
> >> -			base = *data;
> >> +			base = be32_to_cpu(*data);
> >>  	else if (reuse_initrd && !strcmp(name, "linux,initrd-end"))
> >> -		end = *(unsigned long long *) data;
> >> +		if (len == 8)
> >> +			end = be64_to_cpu(*(unsigned long long *) data);
> >> +		else
> >> +			end = be32_to_cpu(*data);
> >>  
> >>  	if (size && end)
> >>  		die("unrecoverable error: size and end set at same time\n");
> >> @@ -267,7 +270,7 @@ static void add_dyn_reconf_usable_mem_property__(int fd)
> >>  	pad_structure_block(rlen);
> >>  	memcpy(dt, ranges, rlen);
> >>  	free(ranges);
> >> -	dt += cpu_to_be32((rlen + 3)/4);
> >> +	dt += (rlen + 3)/4;
> >>  }
> >>  
> >>  static void add_dyn_reconf_usable_mem_property(struct dirent *dp, int fd)
> >>
> > 
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] kexec/fs2dt: fix endianess conversion
  2013-10-17  1:42     ` Simon Horman
@ 2013-10-17  1:44       ` Simon Horman
  0 siblings, 0 replies; 5+ messages in thread
From: Simon Horman @ 2013-10-17  1:44 UTC (permalink / raw)
  To: Laurent Dufour; +Cc: kexec

On Thu, Oct 17, 2013 at 10:42:53AM +0900, Simon Horman wrote:
> On Wed, Oct 16, 2013 at 11:33:43AM +0200, Laurent Dufour wrote:
> > On 16/10/2013 09:28, Simon Horman wrote:
> > > On Tue, Oct 15, 2013 at 07:05:28PM +0200, Laurent Dufour wrote:
> > >> While reviewing fs2dt.c in the common kexec directory, in order to use it to
> > >> support little endian ppc64 architecture, I found some endianess
> > >> conversion's issues.
> > >>
> > >> In dt_reserve, dt_base is a pointer and thus should not be converted.
> > >>
> > >> In checkprop, values read from the device tree are big endian encoded and
> > >> should be converted if CPU is running in little endian mode.
> > >>
> > >> In add_dyn_reconf_usable_mem_property__, fix extraneous endianess conversion
> > >> of rlen which should be natively used to increase the dt pointer.
> > > 
> > > These changes seem logical to me.
> > > 
> > > However, I'm unsure how the code might have worked as-is.
> > > In particular I'm pretty sure ARM was running in little endian
> > > mode when I worked on DT support for it. Perhaps these code
> > > paths were not executed. Or the errors cancelled themselves out somehow?
> > 
> > I've to admit that I don't know how this has worked for ARM.
> > I run this patch with a set of change I'm working on to support PPC64
> > little endian architecture. My tests were done with kexec-tools built in
> > little endian mode, using this fs2dt.c file and I need this fix to build
> > a valid device tree.
> > 
> > The fix is dt_reserve is hit unless the device tree is large enough. I
> > had to reduce INIT_TREE_WORDS to hit it.
> > 
> > I think the ones in checkprop, add_dyn_reconf_usable_mem_property__  are
> > Power specific since RTAS and 'ibm,dynamic-memory' are required to hit them.
> > 
> > This may explain why this issue are not seen in ARM mode.
> 
> Perhaps, its a bit of a mystery.
> I think that I will apply your change as it does seem to be correct.

And I have. It should appear on kernel.org shortly.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-10-17  1:44 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-15 17:05 [PATCH] kexec/fs2dt: fix endianess conversion Laurent Dufour
2013-10-16  7:28 ` Simon Horman
2013-10-16  9:33   ` Laurent Dufour
2013-10-17  1:42     ` Simon Horman
2013-10-17  1:44       ` Simon Horman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox