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

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 <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Hari Bathini <redacted>
---
 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)

Early cmdline parser looks for "mem=" substring in the cmdline
string but it could also succeed when cmdline string contains
parameters like 'fadump_reserve_mem=' or such leading to undesired
results. Add support to skip proceeding in such case.

Fixes: cf68787b68a2 ("powerpc/prom_init: Evaluate mem kernel parameter for early allocation")
Cc: stable@vger.kernel.org # 2.6.32+
Cc: Benjamin Krill <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Hari Bathini <redacted>
---
 arch/powerpc/kernel/prom_init.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index 7030145..3057a32 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c

@@ -649,7 +649,7 @@ static void __init early_cmdline_parse(void)
 	}
 #endif
 	opt = strstr(prom_cmd_line, "mem=");
-	if (opt) {
+	if (opt && is_substring_param(prom_cmd_line, opt)) {
 		opt += 4;
 		prom_memory_limit = prom_memparse(opt, (const char **)&opt);
 #ifdef CONFIG_PPC64

Early cmdline parser looks for "disable_radix" substring in the cmdline
string but it could also succeed when cmdline string contains parameters
like 'x_disable_radix' or disable_radix_type=' or such causing undesired
actions. Add support to skip proceeding in such cases.

Fixes: 014d02cbf16b ("powerpc: Update to new option-vector-5 format for CAS")
Cc: stable@vger.kernel.org # 4.11+
Cc: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Hari Bathini <redacted>
---
 arch/powerpc/kernel/prom_init.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index 3057a32..169e32c 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c

@@ -660,8 +660,16 @@ static void __init early_cmdline_parse(void)
 
 	opt = strstr(prom_cmd_line, "disable_radix");
 	if (opt) {
-		prom_debug("Radix disabled from cmdline\n");
-		prom_radix_disable = true;
+		/*
+		 * Check if this is prefix or suffix of some other parameter
+		 * before proceeding.
+		 */
+		p = (char *)(opt + 13);
+		if ((*p == ' ' || *p == '"' || *p == '\0') &&
+		    is_substring_param(prom_cmd_line, opt)) {
+			prom_debug("Radix disabled from cmdline\n");
+			prom_radix_disable = true;
+		}
 	}
 }
 

Parameters 'mem=', 'iommu=' and the like, which affect the iommu are
parsed early in the boot process. This parser looks for a parameter
substring like 'iommu=' in the cmdline string but it could also succeed
when cmdline string contains parameters like 'x_iommu=' or such leading
to undesired results. Add support to skip proceeding in such cases.

Fixes: 9b6b563c0d2d ("powerpc: Merge in the ppc64 version of the prom code.")
Cc: stable@vger.kernel.org # 2.6.15+
Cc: Paul Mackerras <redacted>
Signed-off-by: Hari Bathini <redacted>
---
 arch/powerpc/kernel/prom_init.c |   22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index 723df83..7030145 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c

@@ -594,6 +594,26 @@ static unsigned long prom_memparse(const char *ptr, const char **retptr)
 }
 
 /*
+ * Check if str is a suffix of another param as "mem=" could
+ * be "iomem=" as well.
+ */
+static bool is_substring_param(const char *cmdline, const char *str)
+{
+	char *p;
+	bool ret = false;
+
+	if (cmdline == str)
+		ret = true;
+	else {
+		p = (char *)(str - 1);
+		if (*p == ' ' || *p == '"')
+			ret = true;
+	}
+
+	return ret;
+}
+
+/*
  * Early parsing of the command line passed to the kernel, used for
  * "mem=x" and the options that affect the iommu
  */

@@ -617,7 +637,7 @@ static void __init early_cmdline_parse(void)
 
 #ifdef CONFIG_PPC64
 	opt = strstr(prom_cmd_line, "iommu=");
-	if (opt) {
+	if (opt && is_substring_param(prom_cmd_line, opt)) {
 		prom_printf("iommu opt is: %s\n", opt);
 		opt += 6;
 		while (*opt && *opt == ' ')

On Thu, 2017-08-03 at 14:54 +0530, Hari Bathini wrote:

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:

quoted

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 <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Hari Bathini <redacted>
---
 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 [off-list ref] 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 <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Hari Bathini <redacted>
---
 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/

On Friday 04 August 2017 09:21 AM, Michael Ellerman wrote:

Hari Bathini [off-list ref] writes:

quoted

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 <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Hari Bathini <redacted>
---
  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/

quoted

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:

quoted

On Thu, 2017-08-03 at 14:54 +0530, Hari Bathini wrote:

quoted

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 <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Hari Bathini <redacted>
---
  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 [off-list ref] writes:

On Friday 04 August 2017 09:21 AM, Michael Ellerman wrote:

quoted

Hari Bathini [off-list ref] writes:

quoted

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 <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Hari Bathini <redacted>
---
  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 [off-list ref] writes:

quoted

On Friday 04 August 2017 09:21 AM, Michael Ellerman wrote:

quoted

Hari Bathini [off-list ref] writes:

quoted

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 <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Hari Bathini <redacted>
---
   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

Hi Hari,

[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.13-rc3 next-20170804]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hari-Bathini/powerpc-prom-avoid-endian-conversions-for-linux-memory-limit-node/20170804-060315
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-mpc836x_rdk_defconfig (attached as .config)
compiler: powerpc-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=powerpc 

Note: the linux-review/Hari-Bathini/powerpc-prom-avoid-endian-conversions-for-linux-memory-limit-node/20170804-060315 HEAD 10b214586e70ec6cd64fea14d0e0997cf74a6fea builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

quoted

arch/powerpc/kernel/prom_init.c:600:13: error: 'is_substring_param' defined but not used [-Werror=unused-function]

    static bool is_substring_param(const char *cmdline, const char *str)
                ^~~~~~~~~~~~~~~~~~
   cc1: all warnings being treated as errors

vim +/is_substring_param +600 arch/powerpc/kernel/prom_init.c

   595	
   596	/*
   597	 * Check if str is a suffix of another param as "mem=" could
   598	 * be "iomem=" as well.
   599	 */
 > 600	static bool is_substring_param(const char *cmdline, const char *str)
   601	{
   602		char *p;
   603		bool ret = false;
   604	
   605		if (cmdline == str)
   606			ret = true;
   607		else {
   608			p = (char *)(str - 1);
   609			if (*p == ' ' || *p == '"')
   610				ret = true;
   611		}
   612	
   613		return ret;
   614	}
   615	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation