| Message ID | 150175227508.9806.17545018023658850483.stgit@hbathini.in.ibm.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
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.
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.
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)
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)
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
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
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 --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)
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(-)