[PATCH 00/13] kdb: Add the ability to read iomapped memory via kdb + clean up "md" commands

[PATCH 00/13] kdb: Add the ability to read iomapped memory via kdb + clean up "md" commands

[Douglas Anderson]

The overall goal of this patch series is to add the ability to read
from IO mapped memory at the kdb prompt. This is something I've long
wished to have but never got around to implementing until now.

As I tried to implement this, I realized that the existing "md" code was
a bit of a mess. I spent a bunch of time cleaning up the function which
made the ability to support iomapped memory atop that pretty simple.

The cleanup code here is not quite a no-op. The "md" handling code has
some esoteric corner cases that it handled and, as part of this, I removed
some of the weird corners. I have a hard time believing anyone was relying
on these, but if you think someone is then please yell.

Also note that it would probably be good to add iomapped memory writes,
but this series is already pretty long so maybe that can be done later.


Douglas Anderson (13):
  kdb: Get rid of "minlen" for the "md" command
  kdb: Document the various "md" commands better
  kdb: Use "bool" in "md" implementation where appropriate
  kdb: Drop "offset" and "name" args to kdbgetaddrarg()
  kdb: Separate out "mdr" handling
  kdb: Remove "mdW" and "mdWcN" handling of "W" == 0
  kdb: Tweak "repeat" handling code for "mdW" and "mdWcN"
  kdb: In kdb_md() make `repeat` and `mdcount` calculations more obvious
  kdb: Use 'unsigned int' in kdb_md() where appropriate
  kdb: Replease simple_strtoul() with kstrtouint() in kdb_md()
  kdb: Abstract out parsing for mdWcN
  kdb: Add mdpW / mdpWcN commands
  kdb: Add mdi, mdiW / mdiWcN commands to show iomapped memory

 kernel/debug/kdb/kdb_bp.c      |   5 +-
 kernel/debug/kdb/kdb_bt.c      |   4 +-
 kernel/debug/kdb/kdb_main.c    | 309 +++++++++++++++++++--------------
 kernel/debug/kdb/kdb_private.h |   5 +-
 kernel/debug/kdb/kdb_support.c |  48 +++++
 5 files changed, 236 insertions(+), 135 deletions(-)

-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH 01/13] kdb: Get rid of "minlen" for the "md" command

[Douglas Anderson]

The "minlen" field should allow us to abbreviate the "md" command as
just "m". However, the kdb_md() function does a lot of checking of
argv[0] and the logic there simply doesn't handle if argv[0] is just
"m". While this could be fixed, "m" isn't really unique (it could be
the prefix for "mm" also) and it's only saving one letter. Remove the
setting of "minlen" to 1 for "md".

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
While digging into this, I found that "minlen" doesn't seem to mean
what's documented in the structure (AKA "Minimum legal # cmd chars
required"). If it worked as documented then you could abbreviate
"summary" with "summ", "summa", and "summar". In fact due to the
parameters passed to strncmp() only "summ" works as an
abbreviation. Fixing that could happen in a future patch if someone
were so inclined, but they'd have to decide whether to change the
behavior of kdb or whether to change the comment and keep the
behavior.

 kernel/debug/kdb/kdb_main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 664bae55f2c9..cbeb203785b4 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -2679,7 +2679,6 @@ static kdbtab_t maintab[] = {
 		.func = kdb_md,
 		.usage = "<vaddr>",
 		.help = "Display Memory Contents, also mdWcN, e.g. md8c1",
-		.minlen = 1,
 		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
 	},
 	{	.name = "mdr",
-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH 02/13] kdb: Document the various "md" commands better

[Douglas Anderson]

The documentation for the variouis "md" commands was inconsistent
about documenting the command arguments. It was also hard to figure
out what the differences between the "phys", "raw", and "symbolic"
versions was.

Update the help strings to make things more obvious.

As part of this, add "bogus" commands to the table for "mdW" and
"mdWcN" so we don't have to obscurely reference them in the normal
"md" help. These bogus commands don't really hurt since kdb_md()
validates argv[0] enough.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 kernel/debug/kdb/kdb_main.c | 39 ++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index cbeb203785b4..47e037c3c002 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -1516,14 +1516,9 @@ static int kdb_mdr(unsigned long addr, unsigned int count)
 }
 
 /*
- * kdb_md - This function implements the 'md', 'md1', 'md2', 'md4',
- *	'md8' 'mdr' and 'mds' commands.
+ * kdb_md - This function implements the guts of the various 'md' commands.
  *
- *	md|mds  [<addr arg> [<line count> [<radix>]]]
- *	mdWcN	[<addr arg> [<line count> [<radix>]]]
- *		where W = is the width (1, 2, 4 or 8) and N is the count.
- *		for eg., md1c20 reads 20 bytes, 1 at a time.
- *	mdr  <addr arg>,<byte count>
+ * See the kdb help for syntax.
  */
 static void kdb_md_line(const char *fmtstr, unsigned long addr,
 			int symbolic, int nosect, int bytesperword,
@@ -2677,26 +2672,38 @@ EXPORT_SYMBOL_GPL(kdb_unregister);
 static kdbtab_t maintab[] = {
 	{	.name = "md",
 		.func = kdb_md,
-		.usage = "<vaddr>",
-		.help = "Display Memory Contents, also mdWcN, e.g. md8c1",
+		.usage = "<vaddr> [<lines> [<radix>]]",
+		.help = "Display RAM using BYTESPERWORD; show MDCOUNT lines",
 		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
 	},
-	{	.name = "mdr",
+	{	.name = "mdW",
 		.func = kdb_md,
-		.usage = "<vaddr> <bytes>",
-		.help = "Display Raw Memory",
+		.usage = "<vaddr> [<lines> [<radix>]]",
+		.help = "Display RAM using word size (W) of 1, 2, 4, or 8",
+		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
+	},
+	{	.name = "mdWcN",
+		.func = kdb_md,
+		.usage = "<vaddr> [<lines> [<radix>]]",
+		.help = "Display RAM using word size (W); show N words",
 		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
 	},
 	{	.name = "mdp",
 		.func = kdb_md,
-		.usage = "<paddr> <bytes>",
-		.help = "Display Physical Memory",
+		.usage = "<paddr> [<lines> [<radix>]]",
+		.help = "Display RAM given a physical address",
+		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
+	},
+	{	.name = "mdr",
+		.func = kdb_md,
+		.usage = "<vaddr> <bytes>",
+		.help = "Display RAM as a stream of raw bytes",
 		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
 	},
 	{	.name = "mds",
 		.func = kdb_md,
-		.usage = "<vaddr>",
-		.help = "Display Memory Symbolically",
+		.usage = "<vaddr> [<lines>]",
+		.help = "Display RAM 1 native word/line; find words in kallsyms",
 		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
 	},
 	{	.name = "mm",
-- 
2.45.2.627.g7a2c4fd464-goog

Re: [PATCH 02/13] kdb: Document the various "md" commands better

[Daniel Thompson]

On Mon, Jun 17, 2024 at 05:34:36PM -0700, Douglas Anderson wrote:
> The documentation for the variouis "md" commands was inconsistent
> about documenting the command arguments. It was also hard to figure
> out what the differences between the "phys", "raw", and "symbolic"
> versions was.
>
> Update the help strings to make things more obvious.
>
> As part of this, add "bogus" commands to the table for "mdW" and
> "mdWcN" so we don't have to obscurely reference them in the normal
> "md" help. These bogus commands don't really hurt since kdb_md()
> validates argv[0] enough.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  kernel/debug/kdb/kdb_main.c | 39 ++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index cbeb203785b4..47e037c3c002 100644
> --- a/kernel/debug/kdb/kdb_main.c
> +++ b/kernel/debug/kdb/kdb_main.c
> @@ -1516,14 +1516,9 @@ static int kdb_mdr(unsigned long addr, unsigned int count)
>  }
>
>  /*
> - * kdb_md - This function implements the 'md', 'md1', 'md2', 'md4',
> - *	'md8' 'mdr' and 'mds' commands.
> + * kdb_md - This function implements the guts of the various 'md' commands.
>   *
> - *	md|mds  [<addr arg> [<line count> [<radix>]]]
> - *	mdWcN	[<addr arg> [<line count> [<radix>]]]
> - *		where W = is the width (1, 2, 4 or 8) and N is the count.
> - *		for eg., md1c20 reads 20 bytes, 1 at a time.
> - *	mdr  <addr arg>,<byte count>
> + * See the kdb help for syntax.
>   */
>  static void kdb_md_line(const char *fmtstr, unsigned long addr,
>  			int symbolic, int nosect, int bytesperword,
> @@ -2677,26 +2672,38 @@ EXPORT_SYMBOL_GPL(kdb_unregister);
>  static kdbtab_t maintab[] = {
>  	{	.name = "md",
>  		.func = kdb_md,
> -		.usage = "<vaddr>",
> -		.help = "Display Memory Contents, also mdWcN, e.g. md8c1",
> +		.usage = "<vaddr> [<lines> [<radix>]]",
> +		.help = "Display RAM using BYTESPERWORD; show MDCOUNT lines",

I'd prefer "memory" over "RAM" because it's what the mnemonic is
abbreviating. This applies to all of the below but I won't be adding a
"same here" for all of them.

Where we have to crush something to fit into one line we'd than have to
break the pattern and choose from thing like:

1. Show memory
2. Display RAM
3. Display mem

Personally I prefer #1 but could probably cope with #2.


>  		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
>  	},
> -	{	.name = "mdr",
> +	{	.name = "mdW",
>  		.func = kdb_md,
> -		.usage = "<vaddr> <bytes>",
> -		.help = "Display Raw Memory",
> +		.usage = "<vaddr> [<lines> [<radix>]]",
> +		.help = "Display RAM using word size (W) of 1, 2, 4, or 8",

We need an "e.g. md8" in here somewhere. Otherwise it is not at all
obvious that W is a wildcard.

I guess that alternatively you could also try naming the command with
hint that W is a wild card (what happens if you register a command
called md<W>?).


> +		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
> +	},
> +	{	.name = "mdWcN",
> +		.func = kdb_md,
> +		.usage = "<vaddr> [<lines> [<radix>]]",
> +		.help = "Display RAM using word size (W); show N words",

Same here.


>  		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
>  	},
>  	{	.name = "mdp",
>  		.func = kdb_md,
> -		.usage = "<paddr> <bytes>",
> -		.help = "Display Physical Memory",
> +		.usage = "<paddr> [<lines> [<radix>]]",
> +		.help = "Display RAM given a physical address",
> +		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
> +	},
> +	{	.name = "mdr",
> +		.func = kdb_md,
> +		.usage = "<vaddr> <bytes>",
> +		.help = "Display RAM as a stream of raw bytes",
>  		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
>  	},
>  	{	.name = "mds",
>  		.func = kdb_md,
> -		.usage = "<vaddr>",
> -		.help = "Display Memory Symbolically",
> +		.usage = "<vaddr> [<lines>]",
> +		.help = "Display RAM 1 native word/line; find words in kallsyms",
>  		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
>  	},
>  	{	.name = "mm",
> --
> 2.45.2.627.g7a2c4fd464-goog
>

Re: [PATCH 02/13] kdb: Document the various "md" commands better

[Doug Anderson]

Hi,

On Tue, Jun 18, 2024 at 4:24 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Mon, Jun 17, 2024 at 05:34:36PM -0700, Douglas Anderson wrote:
> > The documentation for the variouis "md" commands was inconsistent
> > about documenting the command arguments. It was also hard to figure
> > out what the differences between the "phys", "raw", and "symbolic"
> > versions was.
> >
> > Update the help strings to make things more obvious.
> >
> > As part of this, add "bogus" commands to the table for "mdW" and
> > "mdWcN" so we don't have to obscurely reference them in the normal
> > "md" help. These bogus commands don't really hurt since kdb_md()
> > validates argv[0] enough.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> >  kernel/debug/kdb/kdb_main.c | 39 ++++++++++++++++++++++---------------
> >  1 file changed, 23 insertions(+), 16 deletions(-)
> >
> > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> > index cbeb203785b4..47e037c3c002 100644
> > --- a/kernel/debug/kdb/kdb_main.c
> > +++ b/kernel/debug/kdb/kdb_main.c
> > @@ -1516,14 +1516,9 @@ static int kdb_mdr(unsigned long addr, unsigned int count)
> >  }
> >
> >  /*
> > - * kdb_md - This function implements the 'md', 'md1', 'md2', 'md4',
> > - *   'md8' 'mdr' and 'mds' commands.
> > + * kdb_md - This function implements the guts of the various 'md' commands.
> >   *
> > - *   md|mds  [<addr arg> [<line count> [<radix>]]]
> > - *   mdWcN   [<addr arg> [<line count> [<radix>]]]
> > - *           where W = is the width (1, 2, 4 or 8) and N is the count.
> > - *           for eg., md1c20 reads 20 bytes, 1 at a time.
> > - *   mdr  <addr arg>,<byte count>
> > + * See the kdb help for syntax.
> >   */
> >  static void kdb_md_line(const char *fmtstr, unsigned long addr,
> >                       int symbolic, int nosect, int bytesperword,
> > @@ -2677,26 +2672,38 @@ EXPORT_SYMBOL_GPL(kdb_unregister);
> >  static kdbtab_t maintab[] = {
> >       {       .name = "md",
> >               .func = kdb_md,
> > -             .usage = "<vaddr>",
> > -             .help = "Display Memory Contents, also mdWcN, e.g. md8c1",
> > +             .usage = "<vaddr> [<lines> [<radix>]]",
> > +             .help = "Display RAM using BYTESPERWORD; show MDCOUNT lines",
>
> I'd prefer "memory" over "RAM" because it's what the mnemonic is
> abbreviating. This applies to all of the below but I won't be adding a
> "same here" for all of them.
>
> Where we have to crush something to fit into one line we'd than have to
> break the pattern and choose from thing like:
>
> 1. Show memory
> 2. Display RAM
> 3. Display mem
>
> Personally I prefer #1 but could probably cope with #2.

I'm not dead set on RAM, but at least for me RAM was more clarifying.
Specifically when it said "memory" I always assumed it would take in
any memory address and when I first looked at this I tried to figure
out why memory addresses in the IO space didn't work with these
commands. I figured I was holding it wrong only to find out that the
commands specifically limit you to just the RAM range of memory
addresses.

That being said, I don't feel strongly so if you really like "memory"
I'll change it back.


> >               .flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
> >       },
> > -     {       .name = "mdr",
> > +     {       .name = "mdW",
> >               .func = kdb_md,
> > -             .usage = "<vaddr> <bytes>",
> > -             .help = "Display Raw Memory",
> > +             .usage = "<vaddr> [<lines> [<radix>]]",
> > +             .help = "Display RAM using word size (W) of 1, 2, 4, or 8",
>
> We need an "e.g. md8" in here somewhere. Otherwise it is not at all
> obvious that W is a wildcard.
>
> I guess that alternatively you could also try naming the command with
> hint that W is a wild card (what happens if you register a command
> called md<W>?).
>
>
> > +             .flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
> > +     },
> > +     {       .name = "mdWcN",
> > +             .func = kdb_md,
> > +             .usage = "<vaddr> [<lines> [<radix>]]",
> > +             .help = "Display RAM using word size (W); show N words",
>
> Same here.

Sure, so:

.name = "md<W>",
.help = "Display RAM using word size 1, 2, 4, or 8; e.g. md8",

.name = "md<W>c<N>",
.help = "Display RAM using word size W; show N words; e.g. md4c6",

...or changing RAM to "memory" if you don't buy my argument above.

We're definitely ending up over the 80 character mark here, but I
assume that's OK. We were even before my change.

I'll assume that I don't need the "e.g." for all the followup (mdp,
mdi) variants introduced in later patches?


Random question: for the mdWcN variant, should I make specifying
<lines> illegal? It's pretty silly to let the user specify a word
count and then immediately override it. In that case, do I bump
"<radix>" to the 2nd argument or just don't allow "<radix>" for mdWcN?
That would need to be done in a later patch, obviously...

-Doug

[PATCH 03/13] kdb: Use "bool" in "md" implementation where appropriate

[Douglas Anderson]

Like much of kdb, the "md" implementation used the "int" type to store
boolean values and used 0 for false and 1 for true. While this worked
(and used to be the only way to do things back in the day), we've had
"bool" for a lot of years now. Move the "md" functions to use "bool".

While touching this, we touch a line next to a comment using a style
that's nonstandard for the kdb codebase. Update it in passing.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 kernel/debug/kdb/kdb_main.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 47e037c3c002..88121334d189 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -1521,8 +1521,8 @@ static int kdb_mdr(unsigned long addr, unsigned int count)
  * See the kdb help for syntax.
  */
 static void kdb_md_line(const char *fmtstr, unsigned long addr,
-			int symbolic, int nosect, int bytesperword,
-			int num, int repeat, int phys)
+			bool symbolic, bool nosect, int bytesperword,
+			int num, int repeat, bool phys)
 {
 	/* print just one line of data */
 	kdb_symtab_t symtab;
@@ -1590,15 +1590,15 @@ static int kdb_md(int argc, const char **argv)
 	static unsigned long last_addr;
 	static int last_radix, last_bytesperword, last_repeat;
 	int radix = 16, mdcount = 8, bytesperword = KDB_WORD_SIZE, repeat;
-	int nosect = 0;
 	char fmtchar, fmtstr[64];
 	unsigned long addr;
 	unsigned long word;
 	long offset = 0;
-	int symbolic = 0;
-	int valid = 0;
-	int phys = 0;
-	int raw = 0;
+	bool nosect = false;
+	bool symbolic = false;
+	bool valid = false;
+	bool phys = false;
+	bool raw = false;
 
 	kdbgetintenv("MDCOUNT", &mdcount);
 	kdbgetintenv("RADIX", &radix);
@@ -1609,7 +1609,7 @@ static int kdb_md(int argc, const char **argv)
 
 	if (strcmp(argv[0], "mdr") == 0) {
 		if (argc == 2 || (argc == 0 && last_addr != 0))
-			valid = raw = 1;
+			valid = raw = true;
 		else
 			return KDB_ARGCOUNT;
 	} else if (isdigit(argv[0][2])) {
@@ -1622,7 +1622,7 @@ static int kdb_md(int argc, const char **argv)
 		last_bytesperword = bytesperword;
 		repeat = mdcount * 16 / bytesperword;
 		if (!argv[0][3])
-			valid = 1;
+			valid = true;
 		else if (argv[0][3] == 'c' && argv[0][4]) {
 			char *p;
 			repeat = simple_strtoul(argv[0] + 4, &p, 10);
@@ -1631,11 +1631,11 @@ static int kdb_md(int argc, const char **argv)
 		}
 		last_repeat = repeat;
 	} else if (strcmp(argv[0], "md") == 0)
-		valid = 1;
+		valid = true;
 	else if (strcmp(argv[0], "mds") == 0)
-		valid = 1;
+		valid = true;
 	else if (strcmp(argv[0], "mdp") == 0) {
-		phys = valid = 1;
+		phys = valid = true;
 	}
 	if (!valid)
 		return KDB_NOTFOUND;
@@ -1730,13 +1730,19 @@ static int kdb_md(int argc, const char **argv)
 	last_bytesperword = bytesperword;
 
 	if (strcmp(argv[0], "mds") == 0) {
-		symbolic = 1;
-		/* Do not save these changes as last_*, they are temporary mds
+		int tmp;
+
+		symbolic = true;
+
+		/*
+		 * Do not save these changes as last_*, they are temporary mds
 		 * overrides.
 		 */
 		bytesperword = KDB_WORD_SIZE;
 		repeat = mdcount;
-		kdbgetintenv("NOSECT", &nosect);
+
+		if (!kdbgetintenv("NOSECT", &tmp))
+			nosect = tmp;
 	}
 
 	/* Round address down modulo BYTESPERWORD */
-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH 04/13] kdb: Drop "offset" and "name" args to kdbgetaddrarg()

[Douglas Anderson]

Every caller to kdbgetaddrarg() didn't care about the "offset" and
"name" returned by the function. Some passed NULL and some passed the
address of a bogus local variable that was never looked at. Drop the
arguments.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 kernel/debug/kdb/kdb_bp.c      |  5 +----
 kernel/debug/kdb/kdb_bt.c      |  4 +---
 kernel/debug/kdb/kdb_main.c    | 37 ++++++++--------------------------
 kernel/debug/kdb/kdb_private.h |  4 ++--
 4 files changed, 12 insertions(+), 38 deletions(-)

diff --git a/kernel/debug/kdb/kdb_bp.c b/kernel/debug/kdb/kdb_bp.c
index 372025cf1ca3..98659f7dd744 100644
--- a/kernel/debug/kdb/kdb_bp.c
+++ b/kernel/debug/kdb/kdb_bp.c
@@ -279,8 +279,6 @@ static int kdb_bp(int argc, const char **argv)
 	int i, bpno;
 	kdb_bp_t *bp, *bp_check;
 	int diag;
-	char *symname = NULL;
-	long offset = 0ul;
 	int nextarg;
 	kdb_bp_t template = {0};
 
@@ -299,8 +297,7 @@ static int kdb_bp(int argc, const char **argv)
 	}
 
 	nextarg = 1;
-	diag = kdbgetaddrarg(argc, argv, &nextarg, &template.bp_addr,
-			     &offset, &symname);
+	diag = kdbgetaddrarg(argc, argv, &nextarg, &template.bp_addr);
 	if (diag)
 		return diag;
 	if (!template.bp_addr)
diff --git a/kernel/debug/kdb/kdb_bt.c b/kernel/debug/kdb/kdb_bt.c
index 10b454554ab0..af86744c1e2f 100644
--- a/kernel/debug/kdb/kdb_bt.c
+++ b/kernel/debug/kdb/kdb_bt.c
@@ -130,7 +130,6 @@ kdb_bt(int argc, const char **argv)
 	int btaprompt = 1;
 	int nextarg;
 	unsigned long addr;
-	long offset;
 
 	/* Prompt after each proc in bta */
 	kdbgetintenv("BTAPROMPT", &btaprompt);
@@ -205,8 +204,7 @@ kdb_bt(int argc, const char **argv)
 	} else {
 		if (argc) {
 			nextarg = 1;
-			diag = kdbgetaddrarg(argc, argv, &nextarg, &addr,
-					     &offset, NULL);
+			diag = kdbgetaddrarg(argc, argv, &nextarg, &addr);
 			if (diag)
 				return diag;
 			kdb_show_stack(kdb_current_task, (void *)addr);
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 88121334d189..74db5c0cc5ad 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -532,16 +532,12 @@ static int kdb_check_regs(void)
  *	regs	- Register state at time of KDB entry
  * Outputs:
  *	*value	- receives the value of the address-expression
- *	*offset - receives the offset specified, if any
- *	*name   - receives the symbol name, if any
  *	*nextarg - index to next unparsed argument in argv[]
  * Returns:
  *	zero is returned on success, a kdb diagnostic code is
  *      returned on error.
  */
-int kdbgetaddrarg(int argc, const char **argv, int *nextarg,
-		  unsigned long *value,  long *offset,
-		  char **name)
+int kdbgetaddrarg(int argc, const char **argv, int *nextarg, unsigned long *value)
 {
 	unsigned long addr;
 	unsigned long off = 0;
@@ -615,12 +611,8 @@ int kdbgetaddrarg(int argc, const char **argv, int *nextarg,
 
 	(*nextarg)++;
 
-	if (name)
-		*name = symname;
 	if (value)
 		*value = addr;
-	if (offset && name && *name)
-		*offset = addr - symtab.sym_start;
 
 	if ((*nextarg > argc)
 	 && (symbol == '\0'))
@@ -664,9 +656,6 @@ int kdbgetaddrarg(int argc, const char **argv, int *nextarg,
 	if (!positive)
 		off = -off;
 
-	if (offset)
-		*offset += off;
-
 	if (value)
 		*value += off;
 
@@ -1116,14 +1105,10 @@ int kdb_parse(const char *cmdstr)
 	 */
 	{
 		unsigned long value;
-		char *name = NULL;
-		long offset;
 		int nextarg = 0;
 
-		if (kdbgetaddrarg(0, (const char **)argv, &nextarg,
-				  &value, &offset, &name)) {
+		if (kdbgetaddrarg(0, (const char **)argv, &nextarg, &value))
 			return KDB_NOTFOUND;
-		}
 
 		kdb_printf("%s = ", argv[0]);
 		kdb_symbol_print(value, NULL, KDB_SP_DEFAULT);
@@ -1593,7 +1578,6 @@ static int kdb_md(int argc, const char **argv)
 	char fmtchar, fmtstr[64];
 	unsigned long addr;
 	unsigned long word;
-	long offset = 0;
 	bool nosect = false;
 	bool symbolic = false;
 	bool valid = false;
@@ -1656,8 +1640,7 @@ static int kdb_md(int argc, const char **argv)
 	if (argc) {
 		unsigned long val;
 		int diag, nextarg = 1;
-		diag = kdbgetaddrarg(argc, argv, &nextarg, &addr,
-				     &offset, NULL);
+		diag = kdbgetaddrarg(argc, argv, &nextarg, &addr);
 		if (diag)
 			return diag;
 		if (argc > nextarg+2)
@@ -1793,7 +1776,6 @@ static int kdb_mm(int argc, const char **argv)
 {
 	int diag;
 	unsigned long addr;
-	long offset = 0;
 	unsigned long contents;
 	int nextarg;
 	int width;
@@ -1805,13 +1787,13 @@ static int kdb_mm(int argc, const char **argv)
 		return KDB_ARGCOUNT;
 
 	nextarg = 1;
-	diag = kdbgetaddrarg(argc, argv, &nextarg, &addr, &offset, NULL);
+	diag = kdbgetaddrarg(argc, argv, &nextarg, &addr);
 	if (diag)
 		return diag;
 
 	if (nextarg > argc)
 		return KDB_ARGCOUNT;
-	diag = kdbgetaddrarg(argc, argv, &nextarg, &contents, NULL, NULL);
+	diag = kdbgetaddrarg(argc, argv, &nextarg, &contents);
 	if (diag)
 		return diag;
 
@@ -1837,7 +1819,6 @@ static int kdb_go(int argc, const char **argv)
 	unsigned long addr;
 	int diag;
 	int nextarg;
-	long offset;
 
 	if (raw_smp_processor_id() != kdb_initial_cpu) {
 		kdb_printf("go must execute on the entry cpu, "
@@ -1847,8 +1828,7 @@ static int kdb_go(int argc, const char **argv)
 	}
 	if (argc == 1) {
 		nextarg = 1;
-		diag = kdbgetaddrarg(argc, argv, &nextarg,
-				     &addr, &offset, NULL);
+		diag = kdbgetaddrarg(argc, argv, &nextarg, &addr);
 		if (diag)
 			return diag;
 	} else if (argc) {
@@ -2043,14 +2023,13 @@ static int kdb_ef(int argc, const char **argv)
 {
 	int diag;
 	unsigned long addr;
-	long offset;
 	int nextarg;
 
 	if (argc != 1)
 		return KDB_ARGCOUNT;
 
 	nextarg = 1;
-	diag = kdbgetaddrarg(argc, argv, &nextarg, &addr, &offset, NULL);
+	diag = kdbgetaddrarg(argc, argv, &nextarg, &addr);
 	if (diag)
 		return diag;
 	show_regs((struct pt_regs *)addr);
@@ -2547,7 +2526,7 @@ static int kdb_per_cpu(int argc, const char **argv)
 	if (argc < 1 || argc > 3)
 		return KDB_ARGCOUNT;
 
-	diag = kdbgetaddrarg(argc, argv, &nextarg, &symaddr, NULL, NULL);
+	diag = kdbgetaddrarg(argc, argv, &nextarg, &symaddr);
 	if (diag)
 		return diag;
 
diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h
index 548fd4059bf9..1f685d9f16f9 100644
--- a/kernel/debug/kdb/kdb_private.h
+++ b/kernel/debug/kdb/kdb_private.h
@@ -105,8 +105,8 @@ extern int kdb_putword(unsigned long, unsigned long, size_t);
 extern int kdbgetularg(const char *, unsigned long *);
 extern int kdbgetu64arg(const char *, u64 *);
 extern char *kdbgetenv(const char *);
-extern int kdbgetaddrarg(int, const char **, int*, unsigned long *,
-			 long *, char **);
+extern int kdbgetaddrarg(int argc, const char **argv, int *nextarg,
+			 unsigned long *value);
 extern int kdbgetsymval(const char *, kdb_symtab_t *);
 extern int kdbnearsym(unsigned long, kdb_symtab_t *);
 extern char *kdb_strdup(const char *str, gfp_t type);
-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH 05/13] kdb: Separate out "mdr" handling

[Douglas Anderson]

Though the "mdr" has a similar purpose to the other "md" commands in
that they all display memory, the actual code to implement it has
almost nothing in common with the rest of the commands. Separate
things out.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 kernel/debug/kdb/kdb_main.c | 65 ++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 34 deletions(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 74db5c0cc5ad..c013b014a7d3 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -1480,23 +1480,42 @@ int kdb_main_loop(kdb_reason_t reason, kdb_reason_t reason2, int error,
 /*
  * kdb_mdr - This function implements the guts of the 'mdr', memory
  * read command.
- *	mdr  <addr arg>,<byte count>
- * Inputs:
- *	addr	Start address
- *	count	Number of bytes
- * Returns:
- *	Always 0.  Any errors are detected and printed by kdb_getarea.
+ *	mdr  <addr arg> <byte count>
  */
-static int kdb_mdr(unsigned long addr, unsigned int count)
+static int kdb_mdr(int argc, const char **argv)
 {
+	static unsigned long addr;
+	static unsigned long count;
 	unsigned char c;
-	while (count--) {
+	unsigned long i;
+	int diag;
+
+	/*
+	 * Parse args. The only valid options are argc == 2 (both address and
+	 * byte_count provided) and argc == 0 ("repeat" AKA continue previous
+	 * display).
+	 */
+	if (argc == 2) {
+		int nextarg = 1;
+
+		diag = kdbgetaddrarg(argc, argv, &nextarg, &addr);
+		if (diag)
+			return diag;
+		diag = kdbgetularg(argv[nextarg], &count);
+		if (diag)
+			return diag;
+	} else if (argc != 0) {
+		return KDB_ARGCOUNT;
+	}
+
+	for (i = 0; i < count; i++) {
 		if (kdb_getarea(c, addr))
 			return 0;
 		kdb_printf("%02x", c);
 		addr++;
 	}
 	kdb_printf("\n");
+
 	return 0;
 }
 
@@ -1582,7 +1601,6 @@ static int kdb_md(int argc, const char **argv)
 	bool symbolic = false;
 	bool valid = false;
 	bool phys = false;
-	bool raw = false;
 
 	kdbgetintenv("MDCOUNT", &mdcount);
 	kdbgetintenv("RADIX", &radix);
@@ -1591,12 +1609,7 @@ static int kdb_md(int argc, const char **argv)
 	/* Assume 'md <addr>' and start with environment values */
 	repeat = mdcount * 16 / bytesperword;
 
-	if (strcmp(argv[0], "mdr") == 0) {
-		if (argc == 2 || (argc == 0 && last_addr != 0))
-			valid = raw = true;
-		else
-			return KDB_ARGCOUNT;
-	} else if (isdigit(argv[0][2])) {
+	if (isdigit(argv[0][2])) {
 		bytesperword = (int)(argv[0][2] - '0');
 		if (bytesperword == 0) {
 			bytesperword = last_bytesperword;
@@ -1631,10 +1644,7 @@ static int kdb_md(int argc, const char **argv)
 		radix = last_radix;
 		bytesperword = last_bytesperword;
 		repeat = last_repeat;
-		if (raw)
-			mdcount = repeat;
-		else
-			mdcount = ((repeat * bytesperword) + 15) / 16;
+		mdcount = ((repeat * bytesperword) + 15) / 16;
 	}
 
 	if (argc) {
@@ -1650,10 +1660,7 @@ static int kdb_md(int argc, const char **argv)
 			diag = kdbgetularg(argv[nextarg], &val);
 			if (!diag) {
 				mdcount = (int) val;
-				if (raw)
-					repeat = mdcount;
-				else
-					repeat = mdcount * 16 / bytesperword;
+				repeat = mdcount * 16 / bytesperword;
 			}
 		}
 		if (argc >= nextarg+1) {
@@ -1663,16 +1670,6 @@ static int kdb_md(int argc, const char **argv)
 		}
 	}
 
-	if (strcmp(argv[0], "mdr") == 0) {
-		int ret;
-		last_addr = addr;
-		ret = kdb_mdr(addr, mdcount);
-		last_addr += mdcount;
-		last_repeat = mdcount;
-		last_bytesperword = bytesperword; // to make REPEAT happy
-		return ret;
-	}
-
 	switch (radix) {
 	case 10:
 		fmtchar = 'd';
@@ -2680,7 +2677,7 @@ static kdbtab_t maintab[] = {
 		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
 	},
 	{	.name = "mdr",
-		.func = kdb_md,
+		.func = kdb_mdr,
 		.usage = "<vaddr> <bytes>",
 		.help = "Display RAM as a stream of raw bytes",
 		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
-- 
2.45.2.627.g7a2c4fd464-goog

Re: [PATCH 05/13] kdb: Separate out "mdr" handling

[Daniel Thompson]

On Mon, Jun 17, 2024 at 05:34:39PM -0700, Douglas Anderson wrote:
> Though the "mdr" has a similar purpose to the other "md" commands in
> that they all display memory, the actual code to implement it has
> almost nothing in common with the rest of the commands. Separate
> things out.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  kernel/debug/kdb/kdb_main.c | 65 ++++++++++++++++++-------------------
>  1 file changed, 31 insertions(+), 34 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index 74db5c0cc5ad..c013b014a7d3 100644
> --- a/kernel/debug/kdb/kdb_main.c
> +++ b/kernel/debug/kdb/kdb_main.c
> @@ -1480,23 +1480,42 @@ int kdb_main_loop(kdb_reason_t reason, kdb_reason_t reason2, int error,
>  /*
>   * kdb_mdr - This function implements the guts of the 'mdr', memory

This is out of date (this function no longer implements the guts... it
just implements the whole thing).

With that change (and just to remind myself for next time):
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


>   * read command.
> - *	mdr  <addr arg>,<byte count>
> - * Inputs:
> - *	addr	Start address
> - *	count	Number of bytes
> - * Returns:
> - *	Always 0.  Any errors are detected and printed by kdb_getarea.
> + *	mdr  <addr arg> <byte count>
>   */
> -static int kdb_mdr(unsigned long addr, unsigned int count)
> +static int kdb_mdr(int argc, const char **argv)
>  {
> +	static unsigned long addr;
> +	static unsigned long count;
>  	unsigned char c;
> -	while (count--) {
> +	unsigned long i;
> +	int diag;
> +
> +	/*
> +	 * Parse args. The only valid options are argc == 2 (both address and
> +	 * byte_count provided) and argc == 0 ("repeat" AKA continue previous
> +	 * display).
> +	 */
> +	if (argc == 2) {
> +		int nextarg = 1;
> +
> +		diag = kdbgetaddrarg(argc, argv, &nextarg, &addr);
> +		if (diag)
> +			return diag;
> +		diag = kdbgetularg(argv[nextarg], &count);
> +		if (diag)
> +			return diag;
> +	} else if (argc != 0) {
> +		return KDB_ARGCOUNT;
> +	}
> +
> +	for (i = 0; i < count; i++) {
>  		if (kdb_getarea(c, addr))
>  			return 0;
>  		kdb_printf("%02x", c);
>  		addr++;
>  	}
>  	kdb_printf("\n");
> +
>  	return 0;
>  }
>
> @@ -1582,7 +1601,6 @@ static int kdb_md(int argc, const char **argv)
>  	bool symbolic = false;
>  	bool valid = false;
>  	bool phys = false;
> -	bool raw = false;
>
>  	kdbgetintenv("MDCOUNT", &mdcount);
>  	kdbgetintenv("RADIX", &radix);
> @@ -1591,12 +1609,7 @@ static int kdb_md(int argc, const char **argv)
>  	/* Assume 'md <addr>' and start with environment values */
>  	repeat = mdcount * 16 / bytesperword;
>
> -	if (strcmp(argv[0], "mdr") == 0) {
> -		if (argc == 2 || (argc == 0 && last_addr != 0))
> -			valid = raw = true;
> -		else
> -			return KDB_ARGCOUNT;
> -	} else if (isdigit(argv[0][2])) {
> +	if (isdigit(argv[0][2])) {
>  		bytesperword = (int)(argv[0][2] - '0');
>  		if (bytesperword == 0) {
>  			bytesperword = last_bytesperword;
> @@ -1631,10 +1644,7 @@ static int kdb_md(int argc, const char **argv)
>  		radix = last_radix;
>  		bytesperword = last_bytesperword;
>  		repeat = last_repeat;
> -		if (raw)
> -			mdcount = repeat;
> -		else
> -			mdcount = ((repeat * bytesperword) + 15) / 16;
> +		mdcount = ((repeat * bytesperword) + 15) / 16;
>  	}
>
>  	if (argc) {
> @@ -1650,10 +1660,7 @@ static int kdb_md(int argc, const char **argv)
>  			diag = kdbgetularg(argv[nextarg], &val);
>  			if (!diag) {
>  				mdcount = (int) val;
> -				if (raw)
> -					repeat = mdcount;
> -				else
> -					repeat = mdcount * 16 / bytesperword;
> +				repeat = mdcount * 16 / bytesperword;
>  			}
>  		}
>  		if (argc >= nextarg+1) {
> @@ -1663,16 +1670,6 @@ static int kdb_md(int argc, const char **argv)
>  		}
>  	}
>
> -	if (strcmp(argv[0], "mdr") == 0) {
> -		int ret;
> -		last_addr = addr;
> -		ret = kdb_mdr(addr, mdcount);
> -		last_addr += mdcount;
> -		last_repeat = mdcount;
> -		last_bytesperword = bytesperword; // to make REPEAT happy
> -		return ret;
> -	}
> -
>  	switch (radix) {
>  	case 10:
>  		fmtchar = 'd';
> @@ -2680,7 +2677,7 @@ static kdbtab_t maintab[] = {
>  		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
>  	},
>  	{	.name = "mdr",
> -		.func = kdb_md,
> +		.func = kdb_mdr,
>  		.usage = "<vaddr> <bytes>",
>  		.help = "Display RAM as a stream of raw bytes",
>  		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
> --
> 2.45.2.627.g7a2c4fd464-goog
>

[PATCH 06/13] kdb: Remove "mdW" and "mdWcN" handling of "W" == 0

[Douglas Anderson]

The "mdW" and "mdWcN" generally lets the user control more carefully
what word size we display memory in and exactly how many words should
be displayed. Specifically, "md4" says to display memory w/ 4
bytes-per word and "md4c6" says to display 6 words of memory w/
4-bytes-per word.

The kdb "md" implementation has a special rule for when "W" is 0. In
this case:
* If you run with "W" == 0 and you've never run a kdb "md" command
  this reboot then it will pick 4 bytes-per-word, ignoring the normal
  default from the environment.
* If you run with "W" == 0 and you've run a kdb "md" command this
  reboot then it will pick up the bytes per word of the last command.

As an example:
  [1]kdb> md2 0xffffff80c8e2b280 1
  0xffffff80c8e2b280 0200 0000 0000 0000 e000 8235 0000 0000   ...
  [1]kdb> md0 0xffffff80c8e2b280 1
  0xffffff80c8e2b280 0200 0000 0000 0000 e000 8235 0000 0000   ...
  [1]kdb> md 0xffffff80c8e2b280 1
  0xffffff80c8e2b280 0000000000000200 000000008235e000   ...
  [1]kdb> md0 0xffffff80c8e2b280 1
  0xffffff80c8e2b280 0000000000000200 000000008235e000   ...

This doesn't seem like particularly useful behavior and adds a bunch
of complexity to the arg parsing. Remove it.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 kernel/debug/kdb/kdb_main.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index c013b014a7d3..700b4e355545 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -1611,11 +1611,6 @@ static int kdb_md(int argc, const char **argv)
 
 	if (isdigit(argv[0][2])) {
 		bytesperword = (int)(argv[0][2] - '0');
-		if (bytesperword == 0) {
-			bytesperword = last_bytesperword;
-			if (bytesperword == 0)
-				bytesperword = 4;
-		}
 		last_bytesperword = bytesperword;
 		repeat = mdcount * 16 / bytesperword;
 		if (!argv[0][3])
-- 
2.45.2.627.g7a2c4fd464-goog

Re: [PATCH 06/13] kdb: Remove "mdW" and "mdWcN" handling of "W" == 0

[Daniel Thompson]

On Mon, Jun 17, 2024 at 05:34:40PM -0700, Douglas Anderson wrote:
> The "mdW" and "mdWcN" generally lets the user control more carefully
> what word size we display memory in and exactly how many words should
> be displayed. Specifically, "md4" says to display memory w/ 4
> bytes-per word and "md4c6" says to display 6 words of memory w/
> 4-bytes-per word.
>
> The kdb "md" implementation has a special rule for when "W" is 0. In
> this case:
> * If you run with "W" == 0 and you've never run a kdb "md" command
>   this reboot then it will pick 4 bytes-per-word, ignoring the normal
>   default from the environment.
> * If you run with "W" == 0 and you've run a kdb "md" command this
>   reboot then it will pick up the bytes per word of the last command.
>
> As an example:
>   [1]kdb> md2 0xffffff80c8e2b280 1
>   0xffffff80c8e2b280 0200 0000 0000 0000 e000 8235 0000 0000   ...
>   [1]kdb> md0 0xffffff80c8e2b280 1
>   0xffffff80c8e2b280 0200 0000 0000 0000 e000 8235 0000 0000   ...
>   [1]kdb> md 0xffffff80c8e2b280 1
>   0xffffff80c8e2b280 0000000000000200 000000008235e000   ...
>   [1]kdb> md0 0xffffff80c8e2b280 1
>   0xffffff80c8e2b280 0000000000000200 000000008235e000   ...
>
> This doesn't seem like particularly useful behavior and adds a bunch
> of complexity to the arg parsing. Remove it.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  kernel/debug/kdb/kdb_main.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index c013b014a7d3..700b4e355545 100644
> --- a/kernel/debug/kdb/kdb_main.c
> +++ b/kernel/debug/kdb/kdb_main.c
> @@ -1611,11 +1611,6 @@ static int kdb_md(int argc, const char **argv)
>
>  	if (isdigit(argv[0][2])) {
>  		bytesperword = (int)(argv[0][2] - '0');
> -		if (bytesperword == 0) {
> -			bytesperword = last_bytesperword;
> -			if (bytesperword == 0)
> -				bytesperword = 4;
> -		}
>  		last_bytesperword = bytesperword;
>  		repeat = mdcount * 16 / bytesperword;

Isn't this now a divide-by-zero?


Daniel.

Re: [PATCH 06/13] kdb: Remove "mdW" and "mdWcN" handling of "W" == 0

[Doug Anderson]

Hi,

On Tue, Jun 18, 2024 at 4:38 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Mon, Jun 17, 2024 at 05:34:40PM -0700, Douglas Anderson wrote:
> > The "mdW" and "mdWcN" generally lets the user control more carefully
> > what word size we display memory in and exactly how many words should
> > be displayed. Specifically, "md4" says to display memory w/ 4
> > bytes-per word and "md4c6" says to display 6 words of memory w/
> > 4-bytes-per word.
> >
> > The kdb "md" implementation has a special rule for when "W" is 0. In
> > this case:
> > * If you run with "W" == 0 and you've never run a kdb "md" command
> >   this reboot then it will pick 4 bytes-per-word, ignoring the normal
> >   default from the environment.
> > * If you run with "W" == 0 and you've run a kdb "md" command this
> >   reboot then it will pick up the bytes per word of the last command.
> >
> > As an example:
> >   [1]kdb> md2 0xffffff80c8e2b280 1
> >   0xffffff80c8e2b280 0200 0000 0000 0000 e000 8235 0000 0000   ...
> >   [1]kdb> md0 0xffffff80c8e2b280 1
> >   0xffffff80c8e2b280 0200 0000 0000 0000 e000 8235 0000 0000   ...
> >   [1]kdb> md 0xffffff80c8e2b280 1
> >   0xffffff80c8e2b280 0000000000000200 000000008235e000   ...
> >   [1]kdb> md0 0xffffff80c8e2b280 1
> >   0xffffff80c8e2b280 0000000000000200 000000008235e000   ...
> >
> > This doesn't seem like particularly useful behavior and adds a bunch
> > of complexity to the arg parsing. Remove it.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> >  kernel/debug/kdb/kdb_main.c | 5 -----
> >  1 file changed, 5 deletions(-)
> >
> > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> > index c013b014a7d3..700b4e355545 100644
> > --- a/kernel/debug/kdb/kdb_main.c
> > +++ b/kernel/debug/kdb/kdb_main.c
> > @@ -1611,11 +1611,6 @@ static int kdb_md(int argc, const char **argv)
> >
> >       if (isdigit(argv[0][2])) {
> >               bytesperword = (int)(argv[0][2] - '0');
> > -             if (bytesperword == 0) {
> > -                     bytesperword = last_bytesperword;
> > -                     if (bytesperword == 0)
> > -                             bytesperword = 4;
> > -             }
> >               last_bytesperword = bytesperword;
> >               repeat = mdcount * 16 / bytesperword;
>
> Isn't this now a divide-by-zero?

Dang, you're right. It goes away in a later patch, though, since we
stop re-calculating everything until the end when things are
validated. I'll plan to reorder this patch to be after the patch
("kdb: In kdb_md() make `repeat` and `mdcount` calculations more
obvious").

[PATCH 07/13] kdb: Tweak "repeat" handling code for "mdW" and "mdWcN"

[Douglas Anderson]

In general, "md"-style commands are meant to be "repeated". This is a
feature of kdb and "md"-style commands get it enabled because they
have the flag KDB_REPEAT_NO_ARGS. What this means is that if you type
"md4c2 0xffffff808ef05400" and then keep hitting return on the "kdb>"
prompt that you'll read more and more memory. For instance:
  [5]kdb> md4c2 0xffffff808ef05400
  0xffffff808ef05400 00000204 00000000                     ........
  [5]kdb>
  0xffffff808ef05408 8235e000 00000000                     ..5.....
  [5]kdb>
  0xffffff808ef05410 00000003 00000001                     ........

As a side effect of the way kdb works is implemented, you can get the
same behavior as the above by typing the command again with no
arguments. Though it seems unlikely anyone would do this it shouldn't
really hurt:
  [5]kdb> md4c2 0xffffff808ef05400
  0xffffff808ef05400 00000204 00000000                     ........
  [5]kdb> md4c2
  0xffffff808ef05408 8235e000 00000000                     ..5.....
  [5]kdb> md4c2
  0xffffff808ef05410 00000003 00000001                     ........

In general supporting "repeat" should be easy. If argc is 0 then we
just copy the results of the arg parsing from the last time, making
sure that the address has been updated. This is all handled nicely in
the "if (argc == 0)" clause in kdb_md().

Oddly, the "mdW" and "mdWcN" code seems to update "last_bytesperword"
and "last_repeat", which doesn't seem like it should be necessary. It
appears that this code is needed to make this use case work, though
it's a bit unclear if this is truly an important feature to support:
  [1]kdb> md2c3 0xffffff80c8e2b280
  0xffffff80c8e2b280 0200 0000 0000                            ...
  [1]kdb> md2c4
  0xffffff80c8e2b286 0000 e000 8235 0000                       ...

In order to abstract the code better, remove the code updating
"last_bytesperword" and "last_repeat" from the "mdW" and "mdWcN"
handling. This breaks the above case where the user tweaked "argv[0]"
and then tried to somehow leverage the "repeat" code to do something
smart, but that feels like it was a misfeature anyway.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 kernel/debug/kdb/kdb_main.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 700b4e355545..3c6fffa8509a 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -1611,7 +1611,6 @@ static int kdb_md(int argc, const char **argv)
 
 	if (isdigit(argv[0][2])) {
 		bytesperword = (int)(argv[0][2] - '0');
-		last_bytesperword = bytesperword;
 		repeat = mdcount * 16 / bytesperword;
 		if (!argv[0][3])
 			valid = true;
@@ -1621,7 +1620,6 @@ static int kdb_md(int argc, const char **argv)
 			mdcount = ((repeat * bytesperword) + 15) / 16;
 			valid = !*p;
 		}
-		last_repeat = repeat;
 	} else if (strcmp(argv[0], "md") == 0)
 		valid = true;
 	else if (strcmp(argv[0], "mds") == 0)
-- 
2.45.2.627.g7a2c4fd464-goog

Re: [PATCH 07/13] kdb: Tweak "repeat" handling code for "mdW" and "mdWcN"

[Daniel Thompson]

On Mon, Jun 17, 2024 at 05:34:41PM -0700, Douglas Anderson wrote:
> In general, "md"-style commands are meant to be "repeated". This is a
> feature of kdb and "md"-style commands get it enabled because they
> have the flag KDB_REPEAT_NO_ARGS. What this means is that if you type
> "md4c2 0xffffff808ef05400" and then keep hitting return on the "kdb>"
> prompt that you'll read more and more memory. For instance:
>   [5]kdb> md4c2 0xffffff808ef05400
>   0xffffff808ef05400 00000204 00000000                     ........
>   [5]kdb>
>   0xffffff808ef05408 8235e000 00000000                     ..5.....
>   [5]kdb>
>   0xffffff808ef05410 00000003 00000001                     ........
>
> As a side effect of the way kdb works is implemented, you can get the
> same behavior as the above by typing the command again with no
> arguments. Though it seems unlikely anyone would do this it shouldn't
> really hurt:
>   [5]kdb> md4c2 0xffffff808ef05400
>   0xffffff808ef05400 00000204 00000000                     ........
>   [5]kdb> md4c2
>   0xffffff808ef05408 8235e000 00000000                     ..5.....
>   [5]kdb> md4c2
>   0xffffff808ef05410 00000003 00000001                     ........
>
> In general supporting "repeat" should be easy. If argc is 0 then we
> just copy the results of the arg parsing from the last time, making
> sure that the address has been updated. This is all handled nicely in
> the "if (argc == 0)" clause in kdb_md().
>
> Oddly, the "mdW" and "mdWcN" code seems to update "last_bytesperword"
> and "last_repeat", which doesn't seem like it should be necessary. It
> appears that this code is needed to make this use case work, though
> it's a bit unclear if this is truly an important feature to support:
>   [1]kdb> md2c3 0xffffff80c8e2b280
>   0xffffff80c8e2b280 0200 0000 0000                            ...
>   [1]kdb> md2c4
>   0xffffff80c8e2b286 0000 e000 8235 0000                       ...
>
> In order to abstract the code better, remove the code updating
> "last_bytesperword" and "last_repeat" from the "mdW" and "mdWcN"
> handling. This breaks the above case where the user tweaked "argv[0]"
> and then tried to somehow leverage the "repeat" code to do something
> smart, but that feels like it was a misfeature anyway.

I'm not too keen on "successfully" doing the wrong thing.

In that light I'd view this as a feature that is arguably simpler to
implement than it is to error check *not* implementing it. In other
words by the time you add error checking to the argc == 0 path to
spot mismatches then you are better off honouring the user request
rather then telling them they got it wrong.


Daniel.


PS I have never done so but I also wondered if it is reasonable to use
   this feature to manually decompose structures. For example:

     md1c1 structure_pointer; md1c7; md8c1; md8c1; md2c2

Re: [PATCH 07/13] kdb: Tweak "repeat" handling code for "mdW" and "mdWcN"

[Doug Anderson]

Hi,

On Tue, Jun 18, 2024 at 5:57 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Mon, Jun 17, 2024 at 05:34:41PM -0700, Douglas Anderson wrote:
> > In general, "md"-style commands are meant to be "repeated". This is a
> > feature of kdb and "md"-style commands get it enabled because they
> > have the flag KDB_REPEAT_NO_ARGS. What this means is that if you type
> > "md4c2 0xffffff808ef05400" and then keep hitting return on the "kdb>"
> > prompt that you'll read more and more memory. For instance:
> >   [5]kdb> md4c2 0xffffff808ef05400
> >   0xffffff808ef05400 00000204 00000000                     ........
> >   [5]kdb>
> >   0xffffff808ef05408 8235e000 00000000                     ..5.....
> >   [5]kdb>
> >   0xffffff808ef05410 00000003 00000001                     ........
> >
> > As a side effect of the way kdb works is implemented, you can get the
> > same behavior as the above by typing the command again with no
> > arguments. Though it seems unlikely anyone would do this it shouldn't
> > really hurt:
> >   [5]kdb> md4c2 0xffffff808ef05400
> >   0xffffff808ef05400 00000204 00000000                     ........
> >   [5]kdb> md4c2
> >   0xffffff808ef05408 8235e000 00000000                     ..5.....
> >   [5]kdb> md4c2
> >   0xffffff808ef05410 00000003 00000001                     ........
> >
> > In general supporting "repeat" should be easy. If argc is 0 then we
> > just copy the results of the arg parsing from the last time, making
> > sure that the address has been updated. This is all handled nicely in
> > the "if (argc == 0)" clause in kdb_md().
> >
> > Oddly, the "mdW" and "mdWcN" code seems to update "last_bytesperword"
> > and "last_repeat", which doesn't seem like it should be necessary. It
> > appears that this code is needed to make this use case work, though
> > it's a bit unclear if this is truly an important feature to support:
> >   [1]kdb> md2c3 0xffffff80c8e2b280
> >   0xffffff80c8e2b280 0200 0000 0000                            ...
> >   [1]kdb> md2c4
> >   0xffffff80c8e2b286 0000 e000 8235 0000                       ...
> >
> > In order to abstract the code better, remove the code updating
> > "last_bytesperword" and "last_repeat" from the "mdW" and "mdWcN"
> > handling. This breaks the above case where the user tweaked "argv[0]"
> > and then tried to somehow leverage the "repeat" code to do something
> > smart, but that feels like it was a misfeature anyway.
>
> I'm not too keen on "successfully" doing the wrong thing.
>
> In that light I'd view this as a feature that is arguably simpler to
> implement than it is to error check *not* implementing it. In other
> words by the time you add error checking to the argc == 0 path to
> spot mismatches then you are better off honouring the user request
> rather then telling them they got it wrong.

Hmmm. How about we store the last "argv0" and we just immediately fail
if "argc == 0" and "argv[0]" doesn't match the previous one?

The override rules for lines / word count is already complicated
enough and my head was spinning trying to get this right and reason
about it. I tried several times and each time I thought I had it
working cleanly I ended up with some other weird corner case that was
broken. For instance, with the current code this case is broken (IMO):

[3]kdb> md2c4 0xffff0000d8948040
0xffff0000d8948040 0204 0000 0000 0000                       ........
[3]kdb> md2
0xffff0000d8948048 3000 824f 0000 0000 0003 0000 0001 0000   .0O.............
0xffff0000d8948058 0003 0000 0000 0000 0000 0000 0000 0000   ................
0xffff0000d8948068 8000 884a 8000 ffff 0001 0000 0100 0040   ..J...........@.
0xffff0000d8948078 0000 0000 0001 0000 0000 0000 0000 0000   ................
0xffff0000d8948088 0030 0000 0000 0000 000a 0000 0000 0000   0...............
0xffff0000d8948098 f0e6 fffb 0000 0000 9340 809b 0000 ffff   ........@.......
0xffff0000d89480a8 0005 0000 0003 0000 0001 0000 0078 0000   ............x...
0xffff0000d89480b8 0078 0000 0078 0000 0000 0000 0000 0000   x...x...........

Specifically I would have expected the "last" wordcount to persist but
instead it fell back to the default mdcount. Maybe the above is right
but it's distinctly non-obvious.


> Daniel.
>
>
> PS I have never done so but I also wondered if it is reasonable to use
>    this feature to manually decompose structures. For example:
>
>      md1c1 structure_pointer; md1c7; md8c1; md8c1; md2c2

Sure, you can come up with cases where it could be useful, but it
feels like there are other ways to accomplish the same thing w/ less
complexity.


-Doug

[PATCH 08/13] kdb: In kdb_md() make `repeat` and `mdcount` calculations more obvious

[Douglas Anderson]

In kdb_md(), the `mdcount` variable is the number of lines that the
"md" command will output.

In kdb_md(), the `repeat` variable is the number of "words" that the
"md" command will output.

The relationship between these two variables and how they are
specified is a bit convoluted.

You can adjust `mdcount` via the MDCOUNT environment variable. You can
then override the MDCOUNT environment variable by passing a number of
<lines> as an argument to the command.

You can adjust `repeat` using the `mdWcN` variant of the command where
"N" is the number of words to output.

The rules for how these get applied right now:
* By default, we'll the MDCOUNT environment variable.
* If `mdWcN` is used, the repeat will override.
* If <lines> is specified, the mdcount will override.
* When we're "repeating" a previous command (AKA argc is 0) then we'll
  load in the last_repeat.

If you've specified `repeat` then the `mdcount` can be calculated as:
  mdcount = DIV_ROUND_UP(repeat * bytes_per_word, bytes_per_line)

In other words, if you want to display 9 words, each word is 2 bytes,
and you want 16 bytes per line then you'll take up 2 lines. This would
look like:
  [1]kdb> md2c9 0xffffff80e000c340
  0xffffff80e000c340 0204 0000 0000 0000 e000 8235 0000 0000
  0xffffff80e000c350 0003

If you've specified `mdcount` then `repeat` is simply:
  repeat = mdcount * bytes_per_line / bytes_per_word

Let's make all this logic more obvious by initializing `repeat` to 0
and then setting it to non-zero when it should override. Then we can
do all the math at once.

While changing this, use the proper DIV_ROUND_UP() macro and introcue
a constant for KDB_MD_BYTES_PER_LINE. We'll also make and "if else"
more obvious so we know things always get initialized.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 kernel/debug/kdb/kdb_main.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 3c6fffa8509a..fcd5292351a7 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -1589,11 +1589,13 @@ static void kdb_md_line(const char *fmtstr, unsigned long addr,
 		   " ", cbuf);
 }
 
+#define KDB_MD_BYTES_PER_LINE	16
+
 static int kdb_md(int argc, const char **argv)
 {
 	static unsigned long last_addr;
 	static int last_radix, last_bytesperword, last_repeat;
-	int radix = 16, mdcount = 8, bytesperword = KDB_WORD_SIZE, repeat;
+	int radix = 16, mdcount = 8, bytesperword = KDB_WORD_SIZE, repeat = 0;
 	char fmtchar, fmtstr[64];
 	unsigned long addr;
 	unsigned long word;
@@ -1606,18 +1608,13 @@ static int kdb_md(int argc, const char **argv)
 	kdbgetintenv("RADIX", &radix);
 	kdbgetintenv("BYTESPERWORD", &bytesperword);
 
-	/* Assume 'md <addr>' and start with environment values */
-	repeat = mdcount * 16 / bytesperword;
-
 	if (isdigit(argv[0][2])) {
 		bytesperword = (int)(argv[0][2] - '0');
-		repeat = mdcount * 16 / bytesperword;
 		if (!argv[0][3])
 			valid = true;
 		else if (argv[0][3] == 'c' && argv[0][4]) {
 			char *p;
 			repeat = simple_strtoul(argv[0] + 4, &p, 10);
-			mdcount = ((repeat * bytesperword) + 15) / 16;
 			valid = !*p;
 		}
 	} else if (strcmp(argv[0], "md") == 0)
@@ -1637,10 +1634,7 @@ static int kdb_md(int argc, const char **argv)
 		radix = last_radix;
 		bytesperword = last_bytesperword;
 		repeat = last_repeat;
-		mdcount = ((repeat * bytesperword) + 15) / 16;
-	}
-
-	if (argc) {
+	} else {
 		unsigned long val;
 		int diag, nextarg = 1;
 		diag = kdbgetaddrarg(argc, argv, &nextarg, &addr);
@@ -1652,8 +1646,9 @@ static int kdb_md(int argc, const char **argv)
 		if (argc >= nextarg) {
 			diag = kdbgetularg(argv[nextarg], &val);
 			if (!diag) {
-				mdcount = (int) val;
-				repeat = mdcount * 16 / bytesperword;
+				mdcount = val;
+				/* Specifying <lines> overrides repeat count. */
+				repeat = 0;
 			}
 		}
 		if (argc >= nextarg+1) {
@@ -1699,6 +1694,13 @@ static int kdb_md(int argc, const char **argv)
 		return KDB_BADWIDTH;
 	}
 
+	/* If repeat is non-zero then it overrides */
+	if (repeat)
+		mdcount = DIV_ROUND_UP(repeat * bytesperword, KDB_MD_BYTES_PER_LINE);
+	else
+		repeat = mdcount * 16 / bytesperword;
+
+	/* Always just save `repeat` since `mdcount` can be calculated from it */
 	last_repeat = repeat;
 	last_bytesperword = bytesperword;
 
-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH 09/13] kdb: Use 'unsigned int' in kdb_md() where appropriate

[Douglas Anderson]

Several of the integers in kdb_md() should be marked unsigned. Mark
them as such. When doing this, we need to add an explicit cast to the
address masking or it ends up getting truncated down to "int" size.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 kernel/debug/kdb/kdb_main.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index fcd5292351a7..c064ff093670 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -1594,8 +1594,8 @@ static void kdb_md_line(const char *fmtstr, unsigned long addr,
 static int kdb_md(int argc, const char **argv)
 {
 	static unsigned long last_addr;
-	static int last_radix, last_bytesperword, last_repeat;
-	int radix = 16, mdcount = 8, bytesperword = KDB_WORD_SIZE, repeat = 0;
+	static unsigned int last_radix, last_bytesperword, last_repeat;
+	unsigned int radix = 16, mdcount = 8, bytesperword = KDB_WORD_SIZE, repeat = 0;
 	char fmtchar, fmtstr[64];
 	unsigned long addr;
 	unsigned long word;
@@ -1722,11 +1722,11 @@ static int kdb_md(int argc, const char **argv)
 
 	/* Round address down modulo BYTESPERWORD */
 
-	addr &= ~(bytesperword-1);
+	addr &= ~((unsigned long)bytesperword - 1);
 
 	while (repeat > 0) {
 		unsigned long a;
-		int n, z, num = (symbolic ? 1 : (16 / bytesperword));
+		unsigned int n, z, num = (symbolic ? 1 : (16 / bytesperword));
 
 		if (KDB_FLAG(CMD_INTERRUPT))
 			return 0;
@@ -1745,7 +1745,7 @@ static int kdb_md(int argc, const char **argv)
 		repeat -= n;
 		z = (z + num - 1) / num;
 		if (z > 2) {
-			int s = num * (z-2);
+			unsigned int s = num * (z-2);
 			kdb_printf(kdb_machreg_fmt0 "-" kdb_machreg_fmt0
 				   " zero suppressed\n",
 				addr, addr + bytesperword * s - 1);
-- 
2.45.2.627.g7a2c4fd464-goog

Re: [PATCH 09/13] kdb: Use 'unsigned int' in kdb_md() where appropriate

[Daniel Thompson]

On Mon, Jun 17, 2024 at 05:34:43PM -0700, Douglas Anderson wrote:
> Several of the integers in kdb_md() should be marked unsigned. Mark
> them as such. When doing this, we need to add an explicit cast to the
> address masking or it ends up getting truncated down to "int" size.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  kernel/debug/kdb/kdb_main.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index fcd5292351a7..c064ff093670 100644
> --- a/kernel/debug/kdb/kdb_main.c
> +++ b/kernel/debug/kdb/kdb_main.c
> @@ -1594,8 +1594,8 @@ static void kdb_md_line(const char *fmtstr, unsigned long addr,
>  static int kdb_md(int argc, const char **argv)
>  {
>  	static unsigned long last_addr;
> -	static int last_radix, last_bytesperword, last_repeat;
> -	int radix = 16, mdcount = 8, bytesperword = KDB_WORD_SIZE, repeat = 0;
> +	static unsigned int last_radix, last_bytesperword, last_repeat;
> +	unsigned int radix = 16, mdcount = 8, bytesperword = KDB_WORD_SIZE, repeat = 0;
>  	char fmtchar, fmtstr[64];
>  	unsigned long addr;
>  	unsigned long word;
> @@ -1722,11 +1722,11 @@ static int kdb_md(int argc, const char **argv)
>
>  	/* Round address down modulo BYTESPERWORD */
>
> -	addr &= ~(bytesperword-1);
> +	addr &= ~((unsigned long)bytesperword - 1);

I think the round_down() macro will take care of the cast for you (and
probably render the comment pointless too).

Other than that it looks like a good change.


Daniel.

[PATCH 10/13] kdb: Replease simple_strtoul() with kstrtouint() in kdb_md()

[Douglas Anderson]

The docs say you should use kstrtouint() instead of simple_strtoul(),
so do so. This nicely simplfies the code a little.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 kernel/debug/kdb/kdb_main.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index c064ff093670..6dcbf4ea4bcd 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -1612,11 +1612,8 @@ static int kdb_md(int argc, const char **argv)
 		bytesperword = (int)(argv[0][2] - '0');
 		if (!argv[0][3])
 			valid = true;
-		else if (argv[0][3] == 'c' && argv[0][4]) {
-			char *p;
-			repeat = simple_strtoul(argv[0] + 4, &p, 10);
-			valid = !*p;
-		}
+		else if (argv[0][3] == 'c' && argv[0][4])
+			valid = kstrtouint(argv[0] + 4, 10, &repeat) == 0;
 	} else if (strcmp(argv[0], "md") == 0)
 		valid = true;
 	else if (strcmp(argv[0], "mds") == 0)
-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH 11/13] kdb: Abstract out parsing for mdWcN

[Douglas Anderson]

We'd like to use the "WcN" parsing for some other "md"
variants. Abstract it out.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 kernel/debug/kdb/kdb_main.c | 55 +++++++++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 9 deletions(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 6dcbf4ea4bcd..1a37c9bb505c 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -1591,6 +1591,49 @@ static void kdb_md_line(const char *fmtstr, unsigned long addr,
 
 #define KDB_MD_BYTES_PER_LINE	16
 
+/**
+ * kdb_md_parse_arg0() - Parse argv[0] for "md" command
+ *
+ * @cmd:         The name of the command, like "md"
+ * @arg0:        The value of argv[0].
+ * @repeat:      If argv0 modifies repeat count we'll adjust here.
+ * @bytesperword Ifargv0 modifies bytesperword we'll adjust here.
+ *
+ * Return: true if this was a valid cmd; false otherwise.
+ */
+static bool kdb_md_parse_arg0(const char *cmd, const char *arg0,
+			      int *repeat, int *bytesperword)
+{
+	int cmdlen = strlen(cmd);
+
+	/* arg0 must _start_ with the command string or it's a no-go. */
+	if (strncmp(cmd, arg0, cmdlen) != 0)
+		return false;
+
+	/* If it's just the base command, we're done and it's good. */
+	if (arg0[cmdlen] == '\0')
+		return true;
+
+	/*
+	 * The first byte after the base command must be bytes per word, a
+	 * digit. The actual value of bytesperword will be validated later.
+	 */
+	if (!isdigit(arg0[cmdlen]))
+		return false;
+	*bytesperword = (int)(arg0[cmdlen] - '0');
+	cmdlen++;
+
+	/* After the bytes per word must be end of string or a 'c'. */
+	if (arg0[cmdlen] == '\0')
+		return true;
+	if (arg0[cmdlen] != 'c')
+		return false;
+	cmdlen++;
+
+	/* After the "c" is the repeat. */
+	return kstrtouint(arg0 + cmdlen, 10, repeat) == 0;
+}
+
 static int kdb_md(int argc, const char **argv)
 {
 	static unsigned long last_addr;
@@ -1608,19 +1651,13 @@ static int kdb_md(int argc, const char **argv)
 	kdbgetintenv("RADIX", &radix);
 	kdbgetintenv("BYTESPERWORD", &bytesperword);
 
-	if (isdigit(argv[0][2])) {
-		bytesperword = (int)(argv[0][2] - '0');
-		if (!argv[0][3])
-			valid = true;
-		else if (argv[0][3] == 'c' && argv[0][4])
-			valid = kstrtouint(argv[0] + 4, 10, &repeat) == 0;
-	} else if (strcmp(argv[0], "md") == 0)
+	if (kdb_md_parse_arg0("md", argv[0], &repeat, &bytesperword))
 		valid = true;
 	else if (strcmp(argv[0], "mds") == 0)
 		valid = true;
-	else if (strcmp(argv[0], "mdp") == 0) {
+	else if (strcmp(argv[0], "mdp") == 0)
 		phys = valid = true;
-	}
+
 	if (!valid)
 		return KDB_NOTFOUND;
 
-- 
2.45.2.627.g7a2c4fd464-goog

Re: [PATCH 11/13] kdb: Abstract out parsing for mdWcN

[kernel test robot]

Hi Douglas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on v6.10-rc4]
[also build test WARNING on linus/master next-20240618]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Douglas-Anderson/kdb-Get-rid-of-minlen-for-the-md-command/20240618-084245
base:   v6.10-rc4
patch link:    https://lore.kernel.org/r/20240617173426.11.I899d035485269f5110a3323fbb1680fbba718e4c%40changeid
patch subject: [PATCH 11/13] kdb: Abstract out parsing for mdWcN
config: arc-randconfig-001-20240619 (https://download.01.org/0day-ci/archive/20240619/202406190433.U3alc3Xi-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240619/202406190433.U3alc3Xi-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406190433.U3alc3Xi-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> kernel/debug/kdb/kdb_main.c:1606: warning: Function parameter or struct member 'bytesperword' not described in 'kdb_md_parse_arg0'


vim +1606 kernel/debug/kdb/kdb_main.c

  1593	
  1594	/**
  1595	 * kdb_md_parse_arg0() - Parse argv[0] for "md" command
  1596	 *
  1597	 * @cmd:         The name of the command, like "md"
  1598	 * @arg0:        The value of argv[0].
  1599	 * @repeat:      If argv0 modifies repeat count we'll adjust here.
  1600	 * @bytesperword Ifargv0 modifies bytesperword we'll adjust here.
  1601	 *
  1602	 * Return: true if this was a valid cmd; false otherwise.
  1603	 */
  1604	static bool kdb_md_parse_arg0(const char *cmd, const char *arg0,
  1605				      int *repeat, int *bytesperword)
> 1606	{
  1607		int cmdlen = strlen(cmd);
  1608	
  1609		/* arg0 must _start_ with the command string or it's a no-go. */
  1610		if (strncmp(cmd, arg0, cmdlen) != 0)
  1611			return false;
  1612	
  1613		/* If it's just the base command, we're done and it's good. */
  1614		if (arg0[cmdlen] == '\0')
  1615			return true;
  1616	
  1617		/*
  1618		 * The first byte after the base command must be bytes per word, a
  1619		 * digit. The actual value of bytesperword will be validated later.
  1620		 */
  1621		if (!isdigit(arg0[cmdlen]))
  1622			return false;
  1623		*bytesperword = (int)(arg0[cmdlen] - '0');
  1624		cmdlen++;
  1625	
  1626		/* After the bytes per word must be end of string or a 'c'. */
  1627		if (arg0[cmdlen] == '\0')
  1628			return true;
  1629		if (arg0[cmdlen] != 'c')
  1630			return false;
  1631		cmdlen++;
  1632	
  1633		/* After the "c" is the repeat. */
  1634		return kstrtouint(arg0 + cmdlen, 10, repeat) == 0;
  1635	}
  1636	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 12/13] kdb: Add mdpW / mdpWcN commands

[Douglas Anderson]

When specifying a physical address allow specifying the word side and
the number of words.

NOTE: we don't do this for the "mds" command since it's pretty much a
different beast.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 kernel/debug/kdb/kdb_main.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 1a37c9bb505c..be72657741a5 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -1653,10 +1653,10 @@ static int kdb_md(int argc, const char **argv)
 
 	if (kdb_md_parse_arg0("md", argv[0], &repeat, &bytesperword))
 		valid = true;
+	else if (kdb_md_parse_arg0("mdp", argv[0], &repeat, &bytesperword))
+		phys = valid = true;
 	else if (strcmp(argv[0], "mds") == 0)
 		valid = true;
-	else if (strcmp(argv[0], "mdp") == 0)
-		phys = valid = true;
 
 	if (!valid)
 		return KDB_NOTFOUND;
@@ -2705,6 +2705,18 @@ static kdbtab_t maintab[] = {
 		.help = "Display RAM given a physical address",
 		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
 	},
+	{	.name = "mdpW",
+		.func = kdb_md,
+		.usage = "<paddr> [<lines> [<radix>]]",
+		.help = "Display RAM given a PA using word size (W)",
+		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
+	},
+	{	.name = "mdpWcN",
+		.func = kdb_md,
+		.usage = "<paddr> [<lines> [<radix>]]",
+		.help = "Display RAM given a PA using word size (W); show N words",
+		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
+	},
 	{	.name = "mdr",
 		.func = kdb_mdr,
 		.usage = "<vaddr> <bytes>",
-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH 13/13] kdb: Add mdi, mdiW / mdiWcN commands to show iomapped memory

[Douglas Anderson]

Add commands that are like the other "md" commands but that allow you
to read memory that's in the IO space.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 kernel/debug/kdb/kdb_main.c    | 38 +++++++++++++++++++++++----
 kernel/debug/kdb/kdb_private.h |  1 +
 kernel/debug/kdb/kdb_support.c | 48 ++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 5 deletions(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index be72657741a5..a90d1e1482c2 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -1526,7 +1526,7 @@ static int kdb_mdr(int argc, const char **argv)
  */
 static void kdb_md_line(const char *fmtstr, unsigned long addr,
 			bool symbolic, bool nosect, int bytesperword,
-			int num, int repeat, bool phys)
+			int num, int repeat, bool phys, bool do_iomap)
 {
 	/* print just one line of data */
 	kdb_symtab_t symtab;
@@ -1543,7 +1543,10 @@ static void kdb_md_line(const char *fmtstr, unsigned long addr,
 		kdb_printf(kdb_machreg_fmt0 " ", addr);
 
 	for (i = 0; i < num && repeat--; i++) {
-		if (phys) {
+		if (do_iomap) {
+			if (kdb_getioword(&word, addr, bytesperword))
+				break;
+		} else if (phys) {
 			if (kdb_getphysword(&word, addr, bytesperword))
 				break;
 		} else if (kdb_getword(&word, addr, bytesperword))
@@ -1646,6 +1649,7 @@ static int kdb_md(int argc, const char **argv)
 	bool symbolic = false;
 	bool valid = false;
 	bool phys = false;
+	bool do_iomap = false;
 
 	kdbgetintenv("MDCOUNT", &mdcount);
 	kdbgetintenv("RADIX", &radix);
@@ -1655,6 +1659,8 @@ static int kdb_md(int argc, const char **argv)
 		valid = true;
 	else if (kdb_md_parse_arg0("mdp", argv[0], &repeat, &bytesperword))
 		phys = valid = true;
+	else if (kdb_md_parse_arg0("mdi", argv[0], &repeat, &bytesperword))
+		do_iomap = valid = true;
 	else if (strcmp(argv[0], "mds") == 0)
 		valid = true;
 
@@ -1765,7 +1771,11 @@ static int kdb_md(int argc, const char **argv)
 		if (KDB_FLAG(CMD_INTERRUPT))
 			return 0;
 		for (a = addr, z = 0; z < repeat; a += bytesperword, ++z) {
-			if (phys) {
+			if (do_iomap) {
+				if (kdb_getioword(&word, a, bytesperword)
+						|| word)
+					break;
+			} else if (phys) {
 				if (kdb_getphysword(&word, a, bytesperword)
 						|| word)
 					break;
@@ -1774,7 +1784,7 @@ static int kdb_md(int argc, const char **argv)
 		}
 		n = min(num, repeat);
 		kdb_md_line(fmtstr, addr, symbolic, nosect, bytesperword,
-			    num, repeat, phys);
+			    num, repeat, phys, do_iomap);
 		addr += bytesperword * n;
 		repeat -= n;
 		z = (z + num - 1) / num;
@@ -2604,7 +2614,7 @@ static int kdb_per_cpu(int argc, const char **argv)
 		kdb_printf("%5d ", cpu);
 		kdb_md_line(fmtstr, addr,
 			bytesperword == KDB_WORD_SIZE,
-			1, bytesperword, 1, 1, 0);
+			true, bytesperword, 1, 1, false, false);
 	}
 #undef KDB_PCU
 	return 0;
@@ -2717,6 +2727,24 @@ static kdbtab_t maintab[] = {
 		.help = "Display RAM given a PA using word size (W); show N words",
 		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
 	},
+	{	.name = "mdi",
+		.func = kdb_md,
+		.usage = "<paddr> <bytes>",
+		.help = "Display IO Memory",
+		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
+	},
+	{	.name = "mdiW",
+		.func = kdb_md,
+		.usage = "<paddr> <bytes>",
+		.help = "Display IO Memory using word size (W)",
+		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
+	},
+	{	.name = "mdiWcN",
+		.func = kdb_md,
+		.usage = "<paddr> <bytes>",
+		.help = "Display IO Memory using word size (W); show N words",
+		.flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
+	},
 	{	.name = "mdr",
 		.func = kdb_mdr,
 		.usage = "<vaddr> <bytes>",
diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h
index 1f685d9f16f9..caece6240140 100644
--- a/kernel/debug/kdb/kdb_private.h
+++ b/kernel/debug/kdb/kdb_private.h
@@ -97,6 +97,7 @@ extern int kdb_putarea_size(unsigned long, void *, size_t);
 #define kdb_getarea(x, addr) kdb_getarea_size(&(x), addr, sizeof((x)))
 #define kdb_putarea(addr, x) kdb_putarea_size(addr, &(x), sizeof((x)))
 
+extern int kdb_getioword(unsigned long *word, unsigned long addr, size_t size);
 extern int kdb_getphysword(unsigned long *word,
 			unsigned long addr, size_t size);
 extern int kdb_getword(unsigned long *, unsigned long, size_t);
diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
index 0a39497140bf..5a4e3a0e96a5 100644
--- a/kernel/debug/kdb/kdb_support.c
+++ b/kernel/debug/kdb/kdb_support.c
@@ -19,6 +19,7 @@
 #include <linux/ptrace.h>
 #include <linux/highmem.h>
 #include <linux/hardirq.h>
+#include <linux/io.h>
 #include <linux/delay.h>
 #include <linux/uaccess.h>
 #include <linux/kdb.h>
@@ -331,6 +332,53 @@ static int kdb_getphys(void *res, unsigned long addr, size_t size)
 	return 0;
 }
 
+/*
+ * kdb_getioword
+ * Inputs:
+ *	word	Pointer to the word to receive the result.
+ *	addr	Address of the area to copy.
+ *	size	Size of the area.
+ * Returns:
+ *	0 for success, < 0 for error.
+ */
+int kdb_getioword(unsigned long *word, unsigned long addr, size_t size)
+{
+	void __iomem *mapped = ioremap(addr, size);
+	int diag = 0;
+
+	*word = 0;	/* Default value if addr or size is invalid */
+
+	if (!mapped)
+		return KDB_BADADDR;
+
+	switch (size) {
+	case 1:
+		*word = readb(mapped);
+		break;
+	case 2:
+		*word = readw(mapped);
+		break;
+	case 4:
+		*word = readl(mapped);
+		break;
+	case 8:
+#ifdef CONFIG_64BIT
+		if (size <= sizeof(*word)) {
+			*word = readq(mapped);
+			break;
+		}
+#endif
+		fallthrough;
+	default:
+		kdb_func_printf("bad width %zu\n", size);
+		diag = KDB_BADWIDTH;
+	}
+
+	iounmap(mapped);
+
+	return diag;
+}
+
 /*
  * kdb_getphysword
  * Inputs:
-- 
2.45.2.627.g7a2c4fd464-goog

Re: [PATCH 13/13] kdb: Add mdi, mdiW / mdiWcN commands to show iomapped memory

[Daniel Thompson]

On Mon, Jun 17, 2024 at 05:34:47PM -0700, Douglas Anderson wrote:
> Add commands that are like the other "md" commands but that allow you
> to read memory that's in the IO space.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Sorry to be the bearer of bad news but...


> ---
> <snip>
> +/*
> + * kdb_getioword
> + * Inputs:
> + *	word	Pointer to the word to receive the result.
> + *	addr	Address of the area to copy.
> + *	size	Size of the area.
> + * Returns:
> + *	0 for success, < 0 for error.
> + */
> +int kdb_getioword(unsigned long *word, unsigned long addr, size_t size)
> +{
> +	void __iomem *mapped = ioremap(addr, size);

ioremap() is a might_sleep() function. It's unsafe to call it from the
debug trap handler.

I'm afraid I don't know a safe alternative either. Machinary such as
kmap_atomic() needs a page and iomem won't have one.


Daniel.

Re: [PATCH 13/13] kdb: Add mdi, mdiW / mdiWcN commands to show iomapped memory

[Doug Anderson]

Hi,

On Tue, Jun 18, 2024 at 8:59 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Mon, Jun 17, 2024 at 05:34:47PM -0700, Douglas Anderson wrote:
> > Add commands that are like the other "md" commands but that allow you
> > to read memory that's in the IO space.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
> Sorry to be the bearer of bad news but...
>
>
> > ---
> > <snip>
> > +/*
> > + * kdb_getioword
> > + * Inputs:
> > + *   word    Pointer to the word to receive the result.
> > + *   addr    Address of the area to copy.
> > + *   size    Size of the area.
> > + * Returns:
> > + *   0 for success, < 0 for error.
> > + */
> > +int kdb_getioword(unsigned long *word, unsigned long addr, size_t size)
> > +{
> > +     void __iomem *mapped = ioremap(addr, size);
>
> ioremap() is a might_sleep() function. It's unsafe to call it from the
> debug trap handler.

Hmmmm. Do you have a pointer to documentation or code showing that
it's a might_sleep() function. I was worried about this initially but
I couldn't find any documentation or code indicating it to be so. I
also got no warnings when I ran my prototype code and then the code
worked fine, so I assumed that it must somehow work. Sigh...

Looking more closely, maybe this is:

ioremap() -> ioremap_prot() -> generic_ioremap_prot() ->
__get_vm_area_caller() -> __get_vm_area_node() -> __get_vm_area_node()

...and that has a kernel allocation with GFP_KERNEL?

I guess it also then calls alloc_vmap_area()  which has might_sleep()...

I'll have to track down why no warnings triggered...

> I'm afraid I don't know a safe alternative either. Machinary such as
> kmap_atomic() needs a page and iomem won't have one.

Ugh. It would be nice to come up with something since it's not
uncommon to need to look at the state of hardware registers when a
crash happens. In the past I've managed to get into gdb, track down a
global variable where someone has already mapped the memory, and then
use gdb to look at the memory. It's always a big pain, though...

...even if I could just look up the virtual address where someone else
had already mapped it that might be enough. At least I wouldn't need
to go track down the globals myself...

...anyway, I guess I'll ponder on it and poke if I have time...


-Doug

Re: [PATCH 13/13] kdb: Add mdi, mdiW / mdiWcN commands to show iomapped memory

[Daniel Thompson]

On Tue, Jun 18, 2024 at 12:33:05PM -0700, Doug Anderson wrote:
> Hi,
>
> On Tue, Jun 18, 2024 at 8:59 AM Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> >
> > On Mon, Jun 17, 2024 at 05:34:47PM -0700, Douglas Anderson wrote:
> > > Add commands that are like the other "md" commands but that allow you
> > > to read memory that's in the IO space.
> > >
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >
> > Sorry to be the bearer of bad news but...
> >
> >
> > > ---
> > > <snip>
> > > +/*
> > > + * kdb_getioword
> > > + * Inputs:
> > > + *   word    Pointer to the word to receive the result.
> > > + *   addr    Address of the area to copy.
> > > + *   size    Size of the area.
> > > + * Returns:
> > > + *   0 for success, < 0 for error.
> > > + */
> > > +int kdb_getioword(unsigned long *word, unsigned long addr, size_t size)
> > > +{
> > > +     void __iomem *mapped = ioremap(addr, size);
> >
> > ioremap() is a might_sleep() function. It's unsafe to call it from the
> > debug trap handler.
>
> Hmmmm. Do you have a pointer to documentation or code showing that
> it's a might_sleep() function. I was worried about this initially but
> I couldn't find any documentation or code indicating it to be so. I
> also got no warnings when I ran my prototype code and then the code
> worked fine, so I assumed that it must somehow work. Sigh...
>
> Looking more closely, maybe this is:
>
> ioremap() -> ioremap_prot() -> generic_ioremap_prot() ->
> __get_vm_area_caller() -> __get_vm_area_node() -> __get_vm_area_node()
>
> ...and that has a kernel allocation with GFP_KERNEL?

To be honest there were a lot of problems, I just simplified.

__get_vm_area_node() already commences with a BUG_ON(in_interrupt())
before it ends up doing a GFP_KERNEL memory allocation.

I think there are multiple calls to might_sleep() (for example from
__get_vm_area_node() -> alloc_vmap_area() ).

However even if we had preallocated some vmap addresses for peek/poke
there are still problems in ioremap_page_range() too. For example:

generic_ioremap_prot()
  -> ioremap_page_range()
    -> find_vm_area()
      -> find_vmap_area()
        -> spin_lock()

We could go further down the rabbit hole and pre-lookup the VM area too
but then we hit.

generic_ioremap_prot()
  -> ioremap_page_range()
    -> vmap_page_range()
      -> vmap_range_noflush()
        -> might_sleep()

It is remotely possible that the only lock vmap_page_range() takes is
init_mm->page_table_lock but I doubt we can be sure of that.


> I guess it also then calls alloc_vmap_area()  which has might_sleep()...
>
> I'll have to track down why no warnings triggered...
>
> > I'm afraid I don't know a safe alternative either. Machinary such as
> > kmap_atomic() needs a page and iomem won't have one.
>
> Ugh. It would be nice to come up with something since it's not
> uncommon to need to look at the state of hardware registers when a
> crash happens. In the past I've managed to get into gdb, track down a
> global variable where someone has already mapped the memory, and then
> use gdb to look at the memory. It's always a big pain, though...
>
> ...even if I could just look up the virtual address where someone else
> had already mapped it that might be enough. At least I wouldn't need
> to go track down the globals myself...
>
> ...anyway, I guess I'll ponder on it and poke if I have time...

I've often thought about implementing longjmp-on-spin-wait for kgdb for
these kind of reasons. For example I have long wanted to be able to let
the user see /proc/interrupts before the usespace comes up but the spin
locks get in the way.

This approach wouldn't make calling ioremap() safe (since we could end
up bailing out halfway through a non-atomic operation) but it could at
least give control back to kdb and let the user know they have ruined
their system.

I know... there is a *reason* I've never quite got round to writing
this!


Daniel.

Re: [PATCH 13/13] kdb: Add mdi, mdiW / mdiWcN commands to show iomapped memory

[Doug Anderson]

Hi,

On Fri, Jun 21, 2024 at 8:43 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> For example I have long wanted to be able to let
> the user see /proc/interrupts before the usespace comes up but the spin
> locks get in the way.

BTW the "gdb" scripts are supposed to handle this with
"lx-interruptlist", though when I try it right now it doesn't seem to
work.

...actually, there's also a script "lx-iomem" which sounds great but
only has the physical addresses. :(

-Doug