[PATCH v3 0/2] Make it possible to reserve memory on 64bit platform

[PATCH v3 0/2] Make it possible to reserve memory on 64bit platform

[Wesley Zhao]

I was trying to reserve some memory to save logs incase that Android panic or hang and then
I can read the logs from QNX side from the memory reserved before on the Qualcomm 8155 hypervisor platform,
and I find the "reserve=" parameter only support 32bit,so I made some change and send these patches.

I run the cmdline_kunit.c and got these:
[    1.663048] 1..1
[    1.663107]     # Subtest: cmdline
[    1.663145]     1..3
[    1.663795]     ok 1 - cmdline_test_noint
[    1.664139]     ok 2 - cmdline_test_lead_int
[    1.664553]     ok 3 - cmdline_test_tail_int
[    1.664788] ok 1 - cmdline

Additionaly:
	I test on the qemu with some cmdline like[qemu-system-x86_64 -kernel linux-next/arch/x86_64/boot/bzImage
	-hda ubuntu-system.ext4 -append "root=/dev/sda init=/bin/bash console=ttyS0 reserve=0x180000000,0x123456"
	-nographic] and check the /proc/iomem with 180000000-180123455 : reserved.
	And some other tests with the get_option with the parameter(-12345678) and so on


Wesley Zhao (2):
  lib/cmdline: add new function get_option_ull()
  resource: Make it possible to reserve memory on 64bit platform

 include/linux/kernel.h |  2 ++
 kernel/resource.c      |  6 ++--
 lib/cmdline.c          | 94 ++++++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 85 insertions(+), 17 deletions(-)

-- 
2.7.4

[PATCH v3 1/2] lib/cmdline: add new function get_option_ull()

[Wesley Zhao]

In the future we would pass the unsigned long long parameter
like(0x123456781234) in cmdline on the 64bit platform, so add a new
option parse function get_option_ull()

Signed-off-by: Wesley Zhao <zhaowei1102@thundersoft.com>
---
 include/linux/kernel.h |  2 ++
 lib/cmdline.c          | 94 ++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 82 insertions(+), 14 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index f7902d8c1048..eb1f0b14a0c5 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -348,6 +348,8 @@ extern __scanf(2, 0)
 int vsscanf(const char *, const char *, va_list);
 
 extern int get_option(char **str, int *pint);
+extern int get_option_ll(char **str, long long *pll);
+extern int get_option_ull(char **str, unsigned long long *pull);
 extern char *get_options(const char *str, int nints, int *ints);
 extern unsigned long long memparse(const char *ptr, char **retptr);
 extern bool parse_option_str(const char *str, const char *option);
diff --git a/lib/cmdline.c b/lib/cmdline.c
index b390dd03363b..6030fc70e0f5 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -31,6 +31,81 @@ static int get_range(char **str, int *pint, int n)
 		*pint++ = x;
 	return inc_counter;
 }
+/**
+ *	get_option_ull - Parse unsigned long long from an option string
+ *	@str: option string
+ *	@pull: (output) unsigned long long value parsed from @str
+ *
+ *	Read an unsigned long long from an option string; if available accept a subsequent
+ *	comma as well.
+ *
+ *	Return values:
+ *	0 - no ull in string
+ *	1 - ull found, no subsequent comma
+ *	2 - ull found including a subsequent comma
+ *	3 - hyphen found to denote a range
+ */
+
+int get_option_ull(char **str, unsigned long long *pull)
+{
+	char *cur = *str;
+	unsigned long long value;
+
+	if (!cur || !(*cur))
+		return 0;
+	value = simple_strtoull(cur, str, 0);
+	if (pull)
+		*pull = value;
+	if (cur == *str)
+		return 0;
+	if (**str == ',') {
+		(*str)++;
+		return 2;
+	}
+	if (**str == '-')
+		return 3;
+
+	return 1;
+}
+EXPORT_SYMBOL(get_option_ull);
+
+/**
+ *	get_option_ll - Parse long long from an option string
+ *	@str: option string
+ *	@pll: (output) long long value parsed from @str
+ *
+ *	Read an long long from an option string; if available accept a subsequent
+ *	comma as well.
+ *
+ *	Return values:
+ *	0 - no ll in string
+ *	1 - ll found, no subsequent comma
+ *	2 - ll found including a subsequent comma
+ *	3 - hyphen found to denote a range
+ */
+
+int get_option_ll(char **str, long long *pll)
+{
+	char *cur = *str;
+	unsigned long long value;
+	int ret;
+
+	if (!cur || !(*cur))
+		return 0;
+	if (*cur == '-') {
+		(*str) = cur + 1;
+		ret = get_option_ull(str, &value);
+		if (pll)
+			*pll = -value;
+	} else {
+		ret = get_option_ull(str, &value);
+		if (pll)
+			*pll = value;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(get_option_ll);
 
 /**
  *	get_option - Parse integer from an option string
@@ -56,26 +131,17 @@ static int get_range(char **str, int *pint, int n)
 int get_option(char **str, int *pint)
 {
 	char *cur = *str;
-	int value;
+	long long value;
+	int ret;
 
 	if (!cur || !(*cur))
 		return 0;
-	if (*cur == '-')
-		value = -simple_strtoull(++cur, str, 0);
-	else
-		value = simple_strtoull(cur, str, 0);
+	ret = get_option_ll(str, &value);
+
 	if (pint)
 		*pint = value;
-	if (cur == *str)
-		return 0;
-	if (**str == ',') {
-		(*str)++;
-		return 2;
-	}
-	if (**str == '-')
-		return 3;
 
-	return 1;
+	return ret;
 }
 EXPORT_SYMBOL(get_option);
 
-- 
2.7.4

[PATCH v3 2/2] resource: Make it possible to reserve memory on 64bit platform

[Wesley Zhao]

For now "reserve=" is limitied to 32bit,not available on 64bit
platform,so we change the get_option() to get_option_ull(added in
patch: commit 4b6bfe96265e ("lib/cmdline: add new function
get_option_ull()"))

Signed-off-by: Wesley Zhao <zhaowei1102@thundersoft.com>
---
 kernel/resource.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 833394f9c608..ee2a0e5d196f 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1567,13 +1567,13 @@ static int __init reserve_setup(char *str)
 	static struct resource reserve[MAXRESERVE];
 
 	for (;;) {
-		unsigned int io_start, io_num;
+		unsigned long long io_start, io_num;
 		int x = reserved;
 		struct resource *parent;
 
-		if (get_option(&str, &io_start) != 2)
+		if (get_option_ull(&str, &io_start) != 2)
 			break;
-		if (get_option(&str, &io_num) == 0)
+		if (get_option_ull(&str, &io_num) == 0)
 			break;
 		if (x < MAXRESERVE) {
 			struct resource *res = reserve + x;
-- 
2.7.4

Re: [PATCH v3 0/2] Make it possible to reserve memory on 64bit platform

[Andy Shevchenko]

On Fri, Jan 22, 2021 at 07:55:35AM -0800, Wesley Zhao wrote:
> I was trying to reserve some memory to save logs incase that Android panic or hang and then
> I can read the logs from QNX side from the memory reserved before on the Qualcomm 8155 hypervisor platform,
> and I find the "reserve=" parameter only support 32bit,so I made some change and send these patches.

This part is okay.

> I run the cmdline_kunit.c and got these:
> [    1.663048] 1..1
> [    1.663107]     # Subtest: cmdline
> [    1.663145]     1..3
> [    1.663795]     ok 1 - cmdline_test_noint
> [    1.664139]     ok 2 - cmdline_test_lead_int
> [    1.664553]     ok 3 - cmdline_test_tail_int
> [    1.664788] ok 1 - cmdline

This is not okay, you have to have test cases to be added for your new API.
Besides the fact that you don't need it at all. See my further comments.

> Additionaly:
> 	I test on the qemu with some cmdline like[qemu-system-x86_64 -kernel linux-next/arch/x86_64/boot/bzImage
> 	-hda ubuntu-system.ext4 -append "root=/dev/sda init=/bin/bash console=ttyS0 reserve=0x180000000,0x123456"
> 	-nographic] and check the /proc/iomem with 180000000-180123455 : reserved.
> 	And some other tests with the get_option with the parameter(-12345678) and so on

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] lib/cmdline: add new function get_option_ull()

[Andy Shevchenko]

On Fri, Jan 22, 2021 at 07:55:36AM -0800, Wesley Zhao wrote:
> In the future we would pass the unsigned long long parameter
> like(0x123456781234) in cmdline on the 64bit platform, so add a new
> option parse function get_option_ull()

You missed period at the end of phrase.

Overall this patch is not needed at all.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] resource: Make it possible to reserve memory on 64bit platform

[Andy Shevchenko]

On Fri, Jan 22, 2021 at 07:55:37AM -0800, Wesley Zhao wrote:
> For now "reserve=" is limitied to 32bit,not available on 64bit
> platform,so we change the get_option() to get_option_ull(added in
> patch: commit 4b6bfe96265e ("lib/cmdline: add new function
> get_option_ull()"))

Do better grammar and punctuation.

"For now "reserve=" is limited to 32bit, and not available on 64bit platform,
so we change the get_option() to get_option_ull() which is added in the
previous patch."

But!

You don't need to alter cmdline.c at all, see memparse() for the details!
And you have been told already, look at other options. You failed to mentioned
in the cover letter what's wrong with recommended memmap= approach.

-- 
With Best Regards,
Andy Shevchenko