diff mbox

[1/4] powerpc/prom: avoid endian conversions for linux, memory-limit node

Message ID 150175227508.9806.17545018023658850483.stgit@hbathini.in.ibm.com
State New
Headers show

Commit Message

Hari Bathini Aug. 3, 2017, 9:24 a.m. UTC
As linux,memory-limit node is set and also later used by the kernel,
avoid endian conversions for this property.

Fixes: 493adffcb43f ("powerpc: Make prom_init.c endian safe")
Cc: stable@vger.kernel.org # 3.12+
Cc: Anton Blanchard <anton@ozlabs.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/prom_init.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Benjamin Herrenschmidt Aug. 4, 2017, 1:37 a.m. UTC | #1
On Thu, 2017-08-03 at 14:54 +0530, Hari Bathini wrote:
> As linux,memory-limit node is set and also later used by the kernel,
> avoid endian conversions for this property.
> 
> Fixes: 493adffcb43f ("powerpc: Make prom_init.c endian safe")
> Cc: stable@vger.kernel.org # 3.12+
> Cc: Anton Blanchard <anton@ozlabs.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/prom_init.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index 613f79f..723df83 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -3180,9 +3180,8 @@ unsigned long __init prom_init(unsigned long r3, unsigned long r4,
>  	 * Fill in some infos for use by the kernel later on
>  	 */
>  	if (prom_memory_limit) {
> -		__be64 val = cpu_to_be64(prom_memory_limit);
>  		prom_setprop(prom.chosen, "/chosen", "linux,memory-limit",
> -			     &val, sizeof(val));
> +			     &prom_memory_limit, sizeof(prom_memory_limit));
>  	}
>  #ifdef CONFIG_PPC64
>  	if (prom_iommu_off)

NACK. The device-tree is big endian by convention.

Ben.
Benjamin Herrenschmidt Aug. 4, 2017, 1:47 a.m. UTC | #2
On Fri, 2017-08-04 at 11:37 +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2017-08-03 at 14:54 +0530, Hari Bathini wrote:
> > As linux,memory-limit node is set and also later used by the kernel,
> > avoid endian conversions for this property.
> > 
> > Fixes: 493adffcb43f ("powerpc: Make prom_init.c endian safe")
> > Cc: stable@vger.kernel.org # 3.12+
> > Cc: Anton Blanchard <anton@ozlabs.org>
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/kernel/prom_init.c |    3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> > index 613f79f..723df83 100644
> > --- a/arch/powerpc/kernel/prom_init.c
> > +++ b/arch/powerpc/kernel/prom_init.c
> > @@ -3180,9 +3180,8 @@ unsigned long __init prom_init(unsigned long r3, unsigned long r4,
> >  	 * Fill in some infos for use by the kernel later on
> >  	 */
> >  	if (prom_memory_limit) {
> > -		__be64 val = cpu_to_be64(prom_memory_limit);
> >  		prom_setprop(prom.chosen, "/chosen", "linux,memory-limit",
> > -			     &val, sizeof(val));
> > +			     &prom_memory_limit, sizeof(prom_memory_limit));
> >  	}
> >  #ifdef CONFIG_PPC64
> >  	if (prom_iommu_off)
> 
> NACK. The device-tree is big endian by convention

Also that probably breaks kexec.

Cheers,
Ben.
Michael Ellerman Aug. 4, 2017, 3:51 a.m. UTC | #3
Hari Bathini <hbathini@linux.vnet.ibm.com> writes:

> As linux,memory-limit node is set and also later used by the kernel,
> avoid endian conversions for this property.
>
> Fixes: 493adffcb43f ("powerpc: Make prom_init.c endian safe")
> Cc: stable@vger.kernel.org # 3.12+
> Cc: Anton Blanchard <anton@ozlabs.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/prom_init.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

As Ben said, this is not OK. The flat device tree is a data
structure with a specified format[1], we don't violate the spec just to
avoid an endian swap.

Is there an actual bug you're trying to solve?

cheers


[1]: https://www.devicetree.org/

>
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index 613f79f..723df83 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -3180,9 +3180,8 @@ unsigned long __init prom_init(unsigned long r3, unsigned long r4,
>  	 * Fill in some infos for use by the kernel later on
>  	 */
>  	if (prom_memory_limit) {
> -		__be64 val = cpu_to_be64(prom_memory_limit);
>  		prom_setprop(prom.chosen, "/chosen", "linux,memory-limit",
> -			     &val, sizeof(val));
> +			     &prom_memory_limit, sizeof(prom_memory_limit));
>  	}
>  #ifdef CONFIG_PPC64
>  	if (prom_iommu_off)
Hari Bathini Aug. 4, 2017, 5:32 a.m. UTC | #4
On Friday 04 August 2017 09:21 AM, Michael Ellerman wrote:
> Hari Bathini <hbathini@linux.vnet.ibm.com> writes:
>
>> As linux,memory-limit node is set and also later used by the kernel,
>> avoid endian conversions for this property.
>>
>> Fixes: 493adffcb43f ("powerpc: Make prom_init.c endian safe")
>> Cc: stable@vger.kernel.org # 3.12+
>> Cc: Anton Blanchard <anton@ozlabs.org>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
>> ---
>>   arch/powerpc/kernel/prom_init.c |    3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
> As Ben said, this is not OK. The flat device tree is a data
> structure with a specified format[1], we don't violate the spec just to
> avoid an endian swap.
>
> Is there an actual bug you're trying to solve?

Yep. While retrieving this property in prom.c, no endian conversion is 
being done.
It was broken for a while. Let me do the endian swap in prom.c while 
retrieving..

Thanks
Hari

> [1]: https://www.devicetree.org/
>
>> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
>> index 613f79f..723df83 100644
>> --- a/arch/powerpc/kernel/prom_init.c
>> +++ b/arch/powerpc/kernel/prom_init.c
>> @@ -3180,9 +3180,8 @@ unsigned long __init prom_init(unsigned long r3, unsigned long r4,
>>   	 * Fill in some infos for use by the kernel later on
>>   	 */
>>   	if (prom_memory_limit) {
>> -		__be64 val = cpu_to_be64(prom_memory_limit);
>>   		prom_setprop(prom.chosen, "/chosen", "linux,memory-limit",
>> -			     &val, sizeof(val));
>> +			     &prom_memory_limit, sizeof(prom_memory_limit));
>>   	}
>>   #ifdef CONFIG_PPC64
>>   	if (prom_iommu_off)
Hari Bathini Aug. 4, 2017, 5:35 a.m. UTC | #5
On Friday 04 August 2017 07:17 AM, Benjamin Herrenschmidt wrote:
> On Fri, 2017-08-04 at 11:37 +1000, Benjamin Herrenschmidt wrote:
>> On Thu, 2017-08-03 at 14:54 +0530, Hari Bathini wrote:
>>> As linux,memory-limit node is set and also later used by the kernel,
>>> avoid endian conversions for this property.
>>>
>>> Fixes: 493adffcb43f ("powerpc: Make prom_init.c endian safe")
>>> Cc: stable@vger.kernel.org # 3.12+
>>> Cc: Anton Blanchard <anton@ozlabs.org>
>>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
>>> ---
>>>   arch/powerpc/kernel/prom_init.c |    3 +--
>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
>>> index 613f79f..723df83 100644
>>> --- a/arch/powerpc/kernel/prom_init.c
>>> +++ b/arch/powerpc/kernel/prom_init.c
>>> @@ -3180,9 +3180,8 @@ unsigned long __init prom_init(unsigned long r3, unsigned long r4,
>>>   	 * Fill in some infos for use by the kernel later on
>>>   	 */
>>>   	if (prom_memory_limit) {
>>> -		__be64 val = cpu_to_be64(prom_memory_limit);
>>>   		prom_setprop(prom.chosen, "/chosen", "linux,memory-limit",
>>> -			     &val, sizeof(val));
>>> +			     &prom_memory_limit, sizeof(prom_memory_limit));
>>>   	}
>>>   #ifdef CONFIG_PPC64
>>>   	if (prom_iommu_off)
>> NACK. The device-tree is big endian by convention
> Also that probably breaks kexec.
>

Actually, mem= is broken for a while as endian conversion is done for 
linux,memory-limit node
in prom_init.c but not in prom.c. Will post fix with endian conversion 
done in prom.c..

Thanks
Hari
Michael Ellerman Aug. 4, 2017, 10:14 a.m. UTC | #6
Hari Bathini <hbathini@linux.vnet.ibm.com> writes:

> On Friday 04 August 2017 09:21 AM, Michael Ellerman wrote:
>> Hari Bathini <hbathini@linux.vnet.ibm.com> writes:
>>
>>> As linux,memory-limit node is set and also later used by the kernel,
>>> avoid endian conversions for this property.
>>>
>>> Fixes: 493adffcb43f ("powerpc: Make prom_init.c endian safe")
>>> Cc: stable@vger.kernel.org # 3.12+
>>> Cc: Anton Blanchard <anton@ozlabs.org>
>>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
>>> ---
>>>   arch/powerpc/kernel/prom_init.c |    3 +--
>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>> As Ben said, this is not OK. The flat device tree is a data
>> structure with a specified format[1], we don't violate the spec just to
>> avoid an endian swap.
>>
>> Is there an actual bug you're trying to solve?
>
> Yep. While retrieving this property in prom.c, no endian conversion is 
> being done.
> It was broken for a while. Let me do the endian swap in prom.c while 
> retrieving..

Does it actually not work though, mem=x on the command line?

I think that code in prom.c is basically dead code, it's still there
because we were afraid removing it would break something. These days we
parse the command line early enough that we don't need those properties.

cheers
Hari Bathini Aug. 4, 2017, 6:38 p.m. UTC | #7
On Friday 04 August 2017 03:44 PM, Michael Ellerman wrote:
> Hari Bathini <hbathini@linux.vnet.ibm.com> writes:
>
>> On Friday 04 August 2017 09:21 AM, Michael Ellerman wrote:
>>> Hari Bathini <hbathini@linux.vnet.ibm.com> writes:
>>>
>>>> As linux,memory-limit node is set and also later used by the kernel,
>>>> avoid endian conversions for this property.
>>>>
>>>> Fixes: 493adffcb43f ("powerpc: Make prom_init.c endian safe")
>>>> Cc: stable@vger.kernel.org # 3.12+
>>>> Cc: Anton Blanchard <anton@ozlabs.org>
>>>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
>>>> ---
>>>>    arch/powerpc/kernel/prom_init.c |    3 +--
>>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>> As Ben said, this is not OK. The flat device tree is a data
>>> structure with a specified format[1], we don't violate the spec just to
>>> avoid an endian swap.
>>>
>>> Is there an actual bug you're trying to solve?
>> Yep. While retrieving this property in prom.c, no endian conversion is
>> being done.
>> It was broken for a while. Let me do the endian swap in prom.c while
>> retrieving..
> Does it actually not work though, mem=x on the command line?

mem=X works fine. The problem is with the early cmdline parsing of
'mem=' in prom_init, which is treating fadump_reserve_mem=X as
mem=X. So, when fadump_reserve_mem=X is passed, endian swapped
version of X is set to memory_limit as early parser takes it for mem=X
and linux,memory-limit read is not endian safe currently. This bug
was not hit so far as prom_memory_limit is set only when X is
< ram_top && > alloc_bottom which is not the case generally.

> I think that code in prom.c is basically dead code, it's still there
> because we were afraid removing it would break something. These days we
> parse the command line early enough that we don't need those properties.
>
>

This problem is not seen with mem=X as memory_limit is overwritten
with the right value as soon as parse_early_param() is called in prom.

Should I just get rid of linux,memory-limit node and mem=X handling
from early_cmdline_parse() in prom_init as this has been broken for
a while and nobody seem to have had a problem with that?

Thanks
Hari
diff mbox

Patch

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index 613f79f..723df83 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -3180,9 +3180,8 @@  unsigned long __init prom_init(unsigned long r3, unsigned long r4,
 	 * Fill in some infos for use by the kernel later on
 	 */
 	if (prom_memory_limit) {
-		__be64 val = cpu_to_be64(prom_memory_limit);
 		prom_setprop(prom.chosen, "/chosen", "linux,memory-limit",
-			     &val, sizeof(val));
+			     &prom_memory_limit, sizeof(prom_memory_limit));
 	}
 #ifdef CONFIG_PPC64
 	if (prom_iommu_off)