[PATCH] arm64: Add purgatory printing

[PATCH] arm64: Add purgatory printing

[matthias.bgg]

From: Matthias Brugger <mbrugger@suse.com>

Add option to allow purgatory printing on arm64 hardware
by passing the console name which should be used.
Based on a patch by Geoff Levand.

Cc: Geoff Levand <geoff@infradead.org>
Signed-off-by: Matthias Brugger <mbrugger@suse.com>
---
 kexec/arch/arm64/include/arch/options.h |  6 ++-
 kexec/arch/arm64/kexec-arm64.c          | 61 +++++++++++++++++++++++++
 purgatory/arch/arm64/purgatory-arm64.c  | 17 ++++++-
 3 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/kexec/arch/arm64/include/arch/options.h b/kexec/arch/arm64/include/arch/options.h
index a17d933..be7d169 100644
--- a/kexec/arch/arm64/include/arch/options.h
+++ b/kexec/arch/arm64/include/arch/options.h
@@ -5,7 +5,8 @@
 #define OPT_DTB			((OPT_MAX)+1)
 #define OPT_INITRD		((OPT_MAX)+2)
 #define OPT_REUSE_CMDLINE	((OPT_MAX)+3)
-#define OPT_ARCH_MAX		((OPT_MAX)+4)
+#define OPT_CONSOLE		((OPT_MAX)+4)
+#define OPT_ARCH_MAX		((OPT_MAX)+5)
 
 #define KEXEC_ARCH_OPTIONS \
 	KEXEC_OPTIONS \
@@ -13,6 +14,7 @@
 	{ "command-line",  1, NULL, OPT_APPEND }, \
 	{ "dtb",           1, NULL, OPT_DTB }, \
 	{ "initrd",        1, NULL, OPT_INITRD }, \
+	{ "console",       1, NULL, OPT_CONSOLE }, \
 	{ "ramdisk",       1, NULL, OPT_INITRD }, \
 	{ "reuse-cmdline", 0, NULL, OPT_REUSE_CMDLINE }, \
 
@@ -25,6 +27,7 @@ static const char arm64_opts_usage[] __attribute__ ((unused)) =
 "     --command-line=STRING Set the kernel command line to STRING.\n"
 "     --dtb=FILE            Use FILE as the device tree blob.\n"
 "     --initrd=FILE         Use FILE as the kernel initial ramdisk.\n"
+"     --console=STRING      Console used for purgatory printing.\n"
 "     --ramdisk=FILE        Use FILE as the kernel initial ramdisk.\n"
 "     --reuse-cmdline       Use kernel command line from running system.\n";
 
@@ -32,6 +35,7 @@ struct arm64_opts {
 	const char *command_line;
 	const char *dtb;
 	const char *initrd;
+	const char *console;
 };
 
 extern struct arm64_opts arm64_opts;
diff --git a/kexec/arch/arm64/kexec-arm64.c b/kexec/arch/arm64/kexec-arm64.c
index 45ebc54..44c9e6e 100644
--- a/kexec/arch/arm64/kexec-arm64.c
+++ b/kexec/arch/arm64/kexec-arm64.c
@@ -165,6 +165,8 @@ int arch_process_options(int argc, char **argv)
 			break;
 		case OPT_KEXEC_FILE_SYSCALL:
 			do_kexec_file_syscall = 1;
+		case OPT_CONSOLE:
+			arm64_opts.console = optarg;
 			break;
 		default:
 			break; /* Ignore core and unknown options. */
@@ -180,12 +182,62 @@ int arch_process_options(int argc, char **argv)
 	dbgprintf("%s:%d: dtb: %s\n", __func__, __LINE__,
 		(do_kexec_file_syscall && arm64_opts.dtb ? "(ignored)" :
 							arm64_opts.dtb));
+	dbgprintf("%s:%d: console: %s\n", __func__, __LINE__,
+		arm64_opts.console);
+
 	if (do_kexec_file_syscall)
 		arm64_opts.dtb = NULL;
 
 	return 0;
 }
 
+/**
+ * find_purgatory_sink - Find a sink for purgatory output.
+ */
+
+static uint64_t find_purgatory_sink(const char *console)
+{
+	int fd, ret;
+	char folder[255], device[255], mem[255];
+	struct stat sb;
+	char buffer[18];
+	uint64_t iomem = 0x0;
+
+	if (!console)
+		return 0;
+
+	sprintf(device, "/sys/class/tty/%s", console);
+	if (!stat(folder, &sb) == 0 && S_ISDIR(sb.st_mode)) {
+		fprintf(stderr, "kexec: %s: No valid console found for %s\n",
+			__func__, device);
+		return 0;
+	}
+
+	sprintf(mem, "%s%s", device, "/iomem_base");
+	printf("console memory read from %s\n", mem);
+
+	fd = open(mem, O_RDONLY);
+	if (fd < 0) {
+		fprintf(stderr, "kexec: %s: No able to open %s\n",
+			__func__, mem);
+		return 0;
+	}
+
+	memset(buffer, '\0', sizeof(char) * 18);
+	ret = read(fd, buffer, 18);
+	if (ret < 0) {
+		fprintf(stderr, "kexec: %s: not able to read fd\n", __func__);
+		close(fd);
+		return 0;
+	}
+
+	sscanf(buffer, "%lx", &iomem);
+	printf("console memory is at %#lx\n", iomem);
+
+	close(fd);
+	return iomem;
+}
+
 /**
  * struct dtb - Info about a binary device tree.
  *
@@ -637,6 +689,7 @@ int arm64_load_other_segments(struct kexec_info *info,
 	unsigned long hole_min;
 	unsigned long hole_max;
 	unsigned long initrd_end;
+	uint64_t purgatory_sink;
 	char *initrd_buf = NULL;
 	struct dtb dtb;
 	char command_line[COMMAND_LINE_SIZE] = "";
@@ -654,6 +707,11 @@ int arm64_load_other_segments(struct kexec_info *info,
 		command_line[sizeof(command_line) - 1] = 0;
 	}
 
+	purgatory_sink = find_purgatory_sink(arm64_opts.console);
+
+	dbgprintf("%s:%d: purgatory sink: 0x%" PRIx64 "\n", __func__, __LINE__,
+		purgatory_sink);
+
 	if (arm64_opts.dtb) {
 		dtb.name = "dtb_user";
 		dtb.buf = slurp_file(arm64_opts.dtb, &dtb.size);
@@ -742,6 +800,9 @@ int arm64_load_other_segments(struct kexec_info *info,
 
 	info->entry = (void *)elf_rel_get_addr(&info->rhdr, "purgatory_start");
 
+	elf_rel_set_symbol(&info->rhdr, "arm64_sink", &purgatory_sink,
+		sizeof(purgatory_sink));
+
 	elf_rel_set_symbol(&info->rhdr, "arm64_kernel_entry", &image_base,
 		sizeof(image_base));
 
diff --git a/purgatory/arch/arm64/purgatory-arm64.c b/purgatory/arch/arm64/purgatory-arm64.c
index fe50fcf..b4d8578 100644
--- a/purgatory/arch/arm64/purgatory-arm64.c
+++ b/purgatory/arch/arm64/purgatory-arm64.c
@@ -5,15 +5,30 @@
 #include <stdint.h>
 #include <purgatory.h>
 
+/* Symbols set by kexec. */
+
+uint8_t *arm64_sink __attribute__ ((section ("data")));
+extern void (*arm64_kernel_entry)(uint64_t, uint64_t, uint64_t, uint64_t);
+extern uint64_t arm64_dtb_addr;
+
 void putchar(int ch)
 {
-	/* Nothing for now */
+	if (!arm64_sink)
+		return;
+
+	*arm64_sink = ch;
+
+	if (ch == '\n')
+		*arm64_sink = '\r';
 }
 
 void post_verification_setup_arch(void)
 {
+	printf("purgatory: booting kernel now\n");
 }
 
 void setup_arch(void)
 {
+	printf("purgatory: entry=%lx\n", (unsigned long)arm64_kernel_entry);
+	printf("purgatory: dtb=%lx\n", arm64_dtb_addr);
 }
-- 
2.28.0


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] arm64: Add purgatory printing

[Geoff Levand]

Hi Matthias,

On 9/17/20 12:38 PM, matthias.bgg@kernel.org wrote:
Add option to allow purgatory printing on arm64 hardware
> by passing the console name which should be used.

> +static uint64_t find_purgatory_sink(const char *console)
> +{
> +	int fd, ret;
> +	char folder[255], device[255], mem[255];
> +	struct stat sb;
> +	char buffer[18];
> +	uint64_t iomem = 0x0;
> +
> +	if (!console)
> +		return 0;
> +
> +	sprintf(device, "/sys/class/tty/%s", console);
> +	if (!stat(folder, &sb) == 0 && S_ISDIR(sb.st_mode)) {
> +		fprintf(stderr, "kexec: %s: No valid console found for %s\n",
> +			__func__, device);
> +		return 0;
> +	}
> +
> +	sprintf(mem, "%s%s", device, "/iomem_base");
> +	printf("console memory read from %s\n", mem);
> +
> +	fd = open(mem, O_RDONLY);
> +	if (fd < 0) {
> +		fprintf(stderr, "kexec: %s: No able to open %s\n",
> +			__func__, mem);
> +		return 0;
> +	}
> +
> +	memset(buffer, '\0', sizeof(char) * 18);

I think I'd like to just see 'memset(buffer, 0, sizeof(buffer));'.

> +	ret = read(fd, buffer, 18);

And 'ret = read(fd, buffer, sizeof(buffer));'.

> +	if (ret < 0) {
> +		fprintf(stderr, "kexec: %s: not able to read fd\n", __func__);
> +		close(fd);
> +		return 0;
> +	}
> +
> +	sscanf(buffer, "%lx", &iomem);
> +	printf("console memory is at %#lx\n", iomem);
> +
> +	close(fd);
> +	return iomem;
> +}
> +

Otherwise, looks OK.

-Geoff

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] arm64: Add purgatory printing

[Bhupesh SHARMA]

Hi Matthias,

Thanks for the patch. Some nitpicks inline:

On Fri, Sep 18, 2020 at 1:09 AM <matthias.bgg@kernel.org> wrote:
>
> From: Matthias Brugger <mbrugger@suse.com>
>
> Add option to allow purgatory printing on arm64 hardware
> by passing the console name which should be used.
> Based on a patch by Geoff Levand.
>
> Cc: Geoff Levand <geoff@infradead.org>
> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> ---
>  kexec/arch/arm64/include/arch/options.h |  6 ++-
>  kexec/arch/arm64/kexec-arm64.c          | 61 +++++++++++++++++++++++++
>  purgatory/arch/arm64/purgatory-arm64.c  | 17 ++++++-
>  3 files changed, 82 insertions(+), 2 deletions(-)

Probably we also need to update the man page 'kexec/kexec.8' to add
documentation about the newly introduced argument.

> diff --git a/kexec/arch/arm64/include/arch/options.h b/kexec/arch/arm64/include/arch/options.h
> index a17d933..be7d169 100644
> --- a/kexec/arch/arm64/include/arch/options.h
> +++ b/kexec/arch/arm64/include/arch/options.h
> @@ -5,7 +5,8 @@
>  #define OPT_DTB                        ((OPT_MAX)+1)
>  #define OPT_INITRD             ((OPT_MAX)+2)
>  #define OPT_REUSE_CMDLINE      ((OPT_MAX)+3)
> -#define OPT_ARCH_MAX           ((OPT_MAX)+4)
> +#define OPT_CONSOLE            ((OPT_MAX)+4)
> +#define OPT_ARCH_MAX           ((OPT_MAX)+5)
>
>  #define KEXEC_ARCH_OPTIONS \
>         KEXEC_OPTIONS \
> @@ -13,6 +14,7 @@
>         { "command-line",  1, NULL, OPT_APPEND }, \
>         { "dtb",           1, NULL, OPT_DTB }, \
>         { "initrd",        1, NULL, OPT_INITRD }, \
> +       { "console",       1, NULL, OPT_CONSOLE }, \
>         { "ramdisk",       1, NULL, OPT_INITRD }, \
>         { "reuse-cmdline", 0, NULL, OPT_REUSE_CMDLINE }, \
>
> @@ -25,6 +27,7 @@ static const char arm64_opts_usage[] __attribute__ ((unused)) =
>  "     --command-line=STRING Set the kernel command line to STRING.\n"
>  "     --dtb=FILE            Use FILE as the device tree blob.\n"
>  "     --initrd=FILE         Use FILE as the kernel initial ramdisk.\n"
> +"     --console=STRING      Console used for purgatory printing.\n"
>  "     --ramdisk=FILE        Use FILE as the kernel initial ramdisk.\n"
>  "     --reuse-cmdline       Use kernel command line from running system.\n";

Just a thought... sometimes the console string is also available as a
part of the '--reuse-cmdline' command line argument passed to the
kdump kernel. Can we also try to extract the --console string from the
cmdline provided to the primary kernel itself?

> @@ -32,6 +35,7 @@ struct arm64_opts {
>         const char *command_line;
>         const char *dtb;
>         const char *initrd;
> +       const char *console;
>  };
>
>  extern struct arm64_opts arm64_opts;
> diff --git a/kexec/arch/arm64/kexec-arm64.c b/kexec/arch/arm64/kexec-arm64.c
> index 45ebc54..44c9e6e 100644
> --- a/kexec/arch/arm64/kexec-arm64.c
> +++ b/kexec/arch/arm64/kexec-arm64.c
> @@ -165,6 +165,8 @@ int arch_process_options(int argc, char **argv)
>                         break;
>                 case OPT_KEXEC_FILE_SYSCALL:
>                         do_kexec_file_syscall = 1;
> +               case OPT_CONSOLE:
> +                       arm64_opts.console = optarg;
>                         break;
>                 default:
>                         break; /* Ignore core and unknown options. */
> @@ -180,12 +182,62 @@ int arch_process_options(int argc, char **argv)
>         dbgprintf("%s:%d: dtb: %s\n", __func__, __LINE__,
>                 (do_kexec_file_syscall && arm64_opts.dtb ? "(ignored)" :
>                                                         arm64_opts.dtb));
> +       dbgprintf("%s:%d: console: %s\n", __func__, __LINE__,
> +               arm64_opts.console);
> +
>         if (do_kexec_file_syscall)
>                 arm64_opts.dtb = NULL;
>
>         return 0;
>  }
>
> +/**
> + * find_purgatory_sink - Find a sink for purgatory output.
> + */
> +
> +static uint64_t find_purgatory_sink(const char *console)
> +{
> +       int fd, ret;
> +       char folder[255], device[255], mem[255];
> +       struct stat sb;
> +       char buffer[18];

Just trying to understand the reasoning behind keeping the buffer 18
chars long. Can the bytes read from the console exceed the array size
here (may be a boundary check is required here to avoid overflows)?

> +       uint64_t iomem = 0x0;
> +
> +       if (!console)
> +               return 0;
> +
> +       sprintf(device, "/sys/class/tty/%s", console);
> +       if (!stat(folder, &sb) == 0 && S_ISDIR(sb.st_mode)) {
> +               fprintf(stderr, "kexec: %s: No valid console found for %s\n",
> +                       __func__, device);
> +               return 0;
> +       }
> +
> +       sprintf(mem, "%s%s", device, "/iomem_base");
> +       printf("console memory read from %s\n", mem);
> +
> +       fd = open(mem, O_RDONLY);
> +       if (fd < 0) {
> +               fprintf(stderr, "kexec: %s: No able to open %s\n",
> +                       __func__, mem);
> +               return 0;
> +       }
> +
> +       memset(buffer, '\0', sizeof(char) * 18);
> +       ret = read(fd, buffer, 18);
> +       if (ret < 0) {
> +               fprintf(stderr, "kexec: %s: not able to read fd\n", __func__);
> +               close(fd);
> +               return 0;
> +       }
> +
> +       sscanf(buffer, "%lx", &iomem);
> +       printf("console memory is at %#lx\n", iomem);
> +
> +       close(fd);
> +       return iomem;
> +}
> +
>  /**
>   * struct dtb - Info about a binary device tree.
>   *
> @@ -637,6 +689,7 @@ int arm64_load_other_segments(struct kexec_info *info,
>         unsigned long hole_min;
>         unsigned long hole_max;
>         unsigned long initrd_end;
> +       uint64_t purgatory_sink;
>         char *initrd_buf = NULL;
>         struct dtb dtb;
>         char command_line[COMMAND_LINE_SIZE] = "";
> @@ -654,6 +707,11 @@ int arm64_load_other_segments(struct kexec_info *info,
>                 command_line[sizeof(command_line) - 1] = 0;
>         }
>
> +       purgatory_sink = find_purgatory_sink(arm64_opts.console);
> +
> +       dbgprintf("%s:%d: purgatory sink: 0x%" PRIx64 "\n", __func__, __LINE__,
> +               purgatory_sink);
> +
>         if (arm64_opts.dtb) {
>                 dtb.name = "dtb_user";
>                 dtb.buf = slurp_file(arm64_opts.dtb, &dtb.size);
> @@ -742,6 +800,9 @@ int arm64_load_other_segments(struct kexec_info *info,
>
>         info->entry = (void *)elf_rel_get_addr(&info->rhdr, "purgatory_start");
>
> +       elf_rel_set_symbol(&info->rhdr, "arm64_sink", &purgatory_sink,
> +               sizeof(purgatory_sink));
> +
>         elf_rel_set_symbol(&info->rhdr, "arm64_kernel_entry", &image_base,
>                 sizeof(image_base));
>
> diff --git a/purgatory/arch/arm64/purgatory-arm64.c b/purgatory/arch/arm64/purgatory-arm64.c
> index fe50fcf..b4d8578 100644
> --- a/purgatory/arch/arm64/purgatory-arm64.c
> +++ b/purgatory/arch/arm64/purgatory-arm64.c
> @@ -5,15 +5,30 @@
>  #include <stdint.h>
>  #include <purgatory.h>
>
> +/* Symbols set by kexec. */
> +
> +uint8_t *arm64_sink __attribute__ ((section ("data")));
> +extern void (*arm64_kernel_entry)(uint64_t, uint64_t, uint64_t, uint64_t);
> +extern uint64_t arm64_dtb_addr;
> +
>  void putchar(int ch)
>  {
> -       /* Nothing for now */
> +       if (!arm64_sink)
> +               return;
> +
> +       *arm64_sink = ch;
> +
> +       if (ch == '\n')
> +               *arm64_sink = '\r';
>  }
>
>  void post_verification_setup_arch(void)
>  {
> +       printf("purgatory: booting kernel now\n");
>  }
>
>  void setup_arch(void)
>  {
> +       printf("purgatory: entry=%lx\n", (unsigned long)arm64_kernel_entry);
> +       printf("purgatory: dtb=%lx\n", arm64_dtb_addr);
>  }
> --
> 2.28.0

Otherwise the patch looks ok to me.

Thanks,
Bhupesh

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] arm64: Add purgatory printing

[Matthias Brugger]

Hi Geoff,

On 18/09/2020 04:04, Geoff Levand wrote:
> Hi Matthias,
> 
> On 9/17/20 12:38 PM, matthias.bgg@kernel.org wrote:
> Add option to allow purgatory printing on arm64 hardware
>> by passing the console name which should be used.
> 
>> +static uint64_t find_purgatory_sink(const char *console)
>> +{
>> +	int fd, ret;
>> +	char folder[255], device[255], mem[255];
>> +	struct stat sb;
>> +	char buffer[18];
>> +	uint64_t iomem = 0x0;
>> +
>> +	if (!console)
>> +		return 0;
>> +
>> +	sprintf(device, "/sys/class/tty/%s", console);
>> +	if (!stat(folder, &sb) == 0 && S_ISDIR(sb.st_mode)) {
>> +		fprintf(stderr, "kexec: %s: No valid console found for %s\n",
>> +			__func__, device);
>> +		return 0;
>> +	}
>> +
>> +	sprintf(mem, "%s%s", device, "/iomem_base");
>> +	printf("console memory read from %s\n", mem);
>> +
>> +	fd = open(mem, O_RDONLY);
>> +	if (fd < 0) {
>> +		fprintf(stderr, "kexec: %s: No able to open %s\n",
>> +			__func__, mem);
>> +		return 0;
>> +	}
>> +
>> +	memset(buffer, '\0', sizeof(char) * 18);
> 
> I think I'd like to just see 'memset(buffer, 0, sizeof(buffer));'.
> 
>> +	ret = read(fd, buffer, 18);
> 
> And 'ret = read(fd, buffer, sizeof(buffer));'.
> 

You are correct, I'll add it to v2.

Thanks for the quick review!

Matthias

>> +	if (ret < 0) {
>> +		fprintf(stderr, "kexec: %s: not able to read fd\n", __func__);
>> +		close(fd);
>> +		return 0;
>> +	}
>> +
>> +	sscanf(buffer, "%lx", &iomem);
>> +	printf("console memory is at %#lx\n", iomem);
>> +
>> +	close(fd);
>> +	return iomem;
>> +}
>> +
> 
> Otherwise, looks OK.
> 
> -Geoff
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] arm64: Add purgatory printing

[Matthias Brugger]

On 18/09/2020 07:16, Bhupesh SHARMA wrote:
> Hi Matthias,
> 
> Thanks for the patch. Some nitpicks inline:
> 
> On Fri, Sep 18, 2020 at 1:09 AM <matthias.bgg@kernel.org> wrote:
>>
>> From: Matthias Brugger <mbrugger@suse.com>
>>
>> Add option to allow purgatory printing on arm64 hardware
>> by passing the console name which should be used.
>> Based on a patch by Geoff Levand.
>>
>> Cc: Geoff Levand <geoff@infradead.org>
>> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
>> ---
>>   kexec/arch/arm64/include/arch/options.h |  6 ++-
>>   kexec/arch/arm64/kexec-arm64.c          | 61 +++++++++++++++++++++++++
>>   purgatory/arch/arm64/purgatory-arm64.c  | 17 ++++++-
>>   3 files changed, 82 insertions(+), 2 deletions(-)
> 
> Probably we also need to update the man page 'kexec/kexec.8' to add
> documentation about the newly introduced argument.
> 

I checked the documentation and under "ARCHITECTURE OPTIONS" and it only 
documents a subset of the x86(_64) architecture specific commands. So I think 
there is more work to do to get the documentation right.

But after having a look, I think we might want to use "--serial" as option 
instead of "--console" to be in sync with x86. Although many architectures 
reinvent the options, if we can get it as near to x86 as possible, I think that 
would be a good thing. I'll do that in v2.

>> diff --git a/kexec/arch/arm64/include/arch/options.h b/kexec/arch/arm64/include/arch/options.h
>> index a17d933..be7d169 100644
>> --- a/kexec/arch/arm64/include/arch/options.h
>> +++ b/kexec/arch/arm64/include/arch/options.h
>> @@ -5,7 +5,8 @@
>>   #define OPT_DTB                        ((OPT_MAX)+1)
>>   #define OPT_INITRD             ((OPT_MAX)+2)
>>   #define OPT_REUSE_CMDLINE      ((OPT_MAX)+3)
>> -#define OPT_ARCH_MAX           ((OPT_MAX)+4)
>> +#define OPT_CONSOLE            ((OPT_MAX)+4)
>> +#define OPT_ARCH_MAX           ((OPT_MAX)+5)
>>
>>   #define KEXEC_ARCH_OPTIONS \
>>          KEXEC_OPTIONS \
>> @@ -13,6 +14,7 @@
>>          { "command-line",  1, NULL, OPT_APPEND }, \
>>          { "dtb",           1, NULL, OPT_DTB }, \
>>          { "initrd",        1, NULL, OPT_INITRD }, \
>> +       { "console",       1, NULL, OPT_CONSOLE }, \
>>          { "ramdisk",       1, NULL, OPT_INITRD }, \
>>          { "reuse-cmdline", 0, NULL, OPT_REUSE_CMDLINE }, \
>>
>> @@ -25,6 +27,7 @@ static const char arm64_opts_usage[] __attribute__ ((unused)) =
>>   "     --command-line=STRING Set the kernel command line to STRING.\n"
>>   "     --dtb=FILE            Use FILE as the device tree blob.\n"
>>   "     --initrd=FILE         Use FILE as the kernel initial ramdisk.\n"
>> +"     --console=STRING      Console used for purgatory printing.\n"
>>   "     --ramdisk=FILE        Use FILE as the kernel initial ramdisk.\n"
>>   "     --reuse-cmdline       Use kernel command line from running system.\n";
> 
> Just a thought... sometimes the console string is also available as a
> part of the '--reuse-cmdline' command line argument passed to the
> kdump kernel. Can we also try to extract the --console string from the
> cmdline provided to the primary kernel itself?
> 

Well the problem is, that there can be more then one consoles present, so which 
one would be the correct one?
I see this more like a debug feature which you use knowing which console you are 
looking for the debug messages. In the end it only helps you to see if kdump 
failed in the production system kernel or in the crash kernel.

>> @@ -32,6 +35,7 @@ struct arm64_opts {
>>          const char *command_line;
>>          const char *dtb;
>>          const char *initrd;
>> +       const char *console;
>>   };
>>
>>   extern struct arm64_opts arm64_opts;
>> diff --git a/kexec/arch/arm64/kexec-arm64.c b/kexec/arch/arm64/kexec-arm64.c
>> index 45ebc54..44c9e6e 100644
>> --- a/kexec/arch/arm64/kexec-arm64.c
>> +++ b/kexec/arch/arm64/kexec-arm64.c
>> @@ -165,6 +165,8 @@ int arch_process_options(int argc, char **argv)
>>                          break;
>>                  case OPT_KEXEC_FILE_SYSCALL:
>>                          do_kexec_file_syscall = 1;
>> +               case OPT_CONSOLE:
>> +                       arm64_opts.console = optarg;
>>                          break;
>>                  default:
>>                          break; /* Ignore core and unknown options. */
>> @@ -180,12 +182,62 @@ int arch_process_options(int argc, char **argv)
>>          dbgprintf("%s:%d: dtb: %s\n", __func__, __LINE__,
>>                  (do_kexec_file_syscall && arm64_opts.dtb ? "(ignored)" :
>>                                                          arm64_opts.dtb));
>> +       dbgprintf("%s:%d: console: %s\n", __func__, __LINE__,
>> +               arm64_opts.console);
>> +
>>          if (do_kexec_file_syscall)
>>                  arm64_opts.dtb = NULL;
>>
>>          return 0;
>>   }
>>
>> +/**
>> + * find_purgatory_sink - Find a sink for purgatory output.
>> + */
>> +
>> +static uint64_t find_purgatory_sink(const char *console)
>> +{
>> +       int fd, ret;
>> +       char folder[255], device[255], mem[255];
>> +       struct stat sb;
>> +       char buffer[18];
> 
> Just trying to understand the reasoning behind keeping the buffer 18
> chars long. Can the bytes read from the console exceed the array size
> here (may be a boundary check is required here to avoid overflows)?
> 

The buffer having a size of 18 is somewhat random. We use it to read the string 
at for example
/sys/class/tty/ttyAMA0/iomem_base

And we read the size of the buffer, so no overflow is possible here. But: we 
expect a value like 0x94080000 which has actually 10 characters.

>> +       uint64_t iomem = 0x0;
>> +
>> +       if (!console)
>> +               return 0;
>> +
>> +       sprintf(device, "/sys/class/tty/%s", console);

But your comment made me think of another thing. If the console string from the 
command line is really long the sprintf would cause overflows. In v2 I'll update 
the code to use snprintf instead.

Regards,
Matthias

>> +       if (!stat(folder, &sb) == 0 && S_ISDIR(sb.st_mode)) {
>> +               fprintf(stderr, "kexec: %s: No valid console found for %s\n",
>> +                       __func__, device);
>> +               return 0;
>> +       }
>> +
>> +       sprintf(mem, "%s%s", device, "/iomem_base");
>> +       printf("console memory read from %s\n", mem);
>> +
>> +       fd = open(mem, O_RDONLY);
>> +       if (fd < 0) {
>> +               fprintf(stderr, "kexec: %s: No able to open %s\n",
>> +                       __func__, mem);
>> +               return 0;
>> +       }
>> +
>> +       memset(buffer, '\0', sizeof(char) * 18);
>> +       ret = read(fd, buffer, 18);
>> +       if (ret < 0) {
>> +               fprintf(stderr, "kexec: %s: not able to read fd\n", __func__);
>> +               close(fd);
>> +               return 0;
>> +       }
>> +
>> +       sscanf(buffer, "%lx", &iomem);
>> +       printf("console memory is at %#lx\n", iomem);
>> +
>> +       close(fd);
>> +       return iomem;
>> +}
>> +
>>   /**
>>    * struct dtb - Info about a binary device tree.
>>    *
>> @@ -637,6 +689,7 @@ int arm64_load_other_segments(struct kexec_info *info,
>>          unsigned long hole_min;
>>          unsigned long hole_max;
>>          unsigned long initrd_end;
>> +       uint64_t purgatory_sink;
>>          char *initrd_buf = NULL;
>>          struct dtb dtb;
>>          char command_line[COMMAND_LINE_SIZE] = "";
>> @@ -654,6 +707,11 @@ int arm64_load_other_segments(struct kexec_info *info,
>>                  command_line[sizeof(command_line) - 1] = 0;
>>          }
>>
>> +       purgatory_sink = find_purgatory_sink(arm64_opts.console);
>> +
>> +       dbgprintf("%s:%d: purgatory sink: 0x%" PRIx64 "\n", __func__, __LINE__,
>> +               purgatory_sink);
>> +
>>          if (arm64_opts.dtb) {
>>                  dtb.name = "dtb_user";
>>                  dtb.buf = slurp_file(arm64_opts.dtb, &dtb.size);
>> @@ -742,6 +800,9 @@ int arm64_load_other_segments(struct kexec_info *info,
>>
>>          info->entry = (void *)elf_rel_get_addr(&info->rhdr, "purgatory_start");
>>
>> +       elf_rel_set_symbol(&info->rhdr, "arm64_sink", &purgatory_sink,
>> +               sizeof(purgatory_sink));
>> +
>>          elf_rel_set_symbol(&info->rhdr, "arm64_kernel_entry", &image_base,
>>                  sizeof(image_base));
>>
>> diff --git a/purgatory/arch/arm64/purgatory-arm64.c b/purgatory/arch/arm64/purgatory-arm64.c
>> index fe50fcf..b4d8578 100644
>> --- a/purgatory/arch/arm64/purgatory-arm64.c
>> +++ b/purgatory/arch/arm64/purgatory-arm64.c
>> @@ -5,15 +5,30 @@
>>   #include <stdint.h>
>>   #include <purgatory.h>
>>
>> +/* Symbols set by kexec. */
>> +
>> +uint8_t *arm64_sink __attribute__ ((section ("data")));
>> +extern void (*arm64_kernel_entry)(uint64_t, uint64_t, uint64_t, uint64_t);
>> +extern uint64_t arm64_dtb_addr;
>> +
>>   void putchar(int ch)
>>   {
>> -       /* Nothing for now */
>> +       if (!arm64_sink)
>> +               return;
>> +
>> +       *arm64_sink = ch;
>> +
>> +       if (ch == '\n')
>> +               *arm64_sink = '\r';
>>   }
>>
>>   void post_verification_setup_arch(void)
>>   {
>> +       printf("purgatory: booting kernel now\n");
>>   }
>>
>>   void setup_arch(void)
>>   {
>> +       printf("purgatory: entry=%lx\n", (unsigned long)arm64_kernel_entry);
>> +       printf("purgatory: dtb=%lx\n", arm64_dtb_addr);
>>   }
>> --
>> 2.28.0
> 
> Otherwise the patch looks ok to me.
> 
> Thanks,
> Bhupesh
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] arm64: Add purgatory printing

[Bhupesh SHARMA]

Hi Matthias,

On Fri, Sep 18, 2020 at 2:01 PM Matthias Brugger <mbrugger@suse.com> wrote:
>
>
>
> On 18/09/2020 07:16, Bhupesh SHARMA wrote:
> > Hi Matthias,
> >
> > Thanks for the patch. Some nitpicks inline:
> >
> > On Fri, Sep 18, 2020 at 1:09 AM <matthias.bgg@kernel.org> wrote:
> >>
> >> From: Matthias Brugger <mbrugger@suse.com>
> >>
> >> Add option to allow purgatory printing on arm64 hardware
> >> by passing the console name which should be used.
> >> Based on a patch by Geoff Levand.
> >>
> >> Cc: Geoff Levand <geoff@infradead.org>
> >> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> >> ---
> >>   kexec/arch/arm64/include/arch/options.h |  6 ++-
> >>   kexec/arch/arm64/kexec-arm64.c          | 61 +++++++++++++++++++++++++
> >>   purgatory/arch/arm64/purgatory-arm64.c  | 17 ++++++-
> >>   3 files changed, 82 insertions(+), 2 deletions(-)
> >
> > Probably we also need to update the man page 'kexec/kexec.8' to add
> > documentation about the newly introduced argument.
> >
>
> I checked the documentation and under "ARCHITECTURE OPTIONS" and it only
> documents a subset of the x86(_64) architecture specific commands. So I think
> there is more work to do to get the documentation right.
>
> But after having a look, I think we might want to use "--serial" as option
> instead of "--console" to be in sync with x86. Although many architectures
> reinvent the options, if we can get it as near to x86 as possible, I think that
> would be a good thing. I'll do that in v2.

That's a good suggestion. I think the '--serial' option is a better
one as compared to '--console'.

> >> diff --git a/kexec/arch/arm64/include/arch/options.h b/kexec/arch/arm64/include/arch/options.h
> >> index a17d933..be7d169 100644
> >> --- a/kexec/arch/arm64/include/arch/options.h
> >> +++ b/kexec/arch/arm64/include/arch/options.h
> >> @@ -5,7 +5,8 @@
> >>   #define OPT_DTB                        ((OPT_MAX)+1)
> >>   #define OPT_INITRD             ((OPT_MAX)+2)
> >>   #define OPT_REUSE_CMDLINE      ((OPT_MAX)+3)
> >> -#define OPT_ARCH_MAX           ((OPT_MAX)+4)
> >> +#define OPT_CONSOLE            ((OPT_MAX)+4)
> >> +#define OPT_ARCH_MAX           ((OPT_MAX)+5)
> >>
> >>   #define KEXEC_ARCH_OPTIONS \
> >>          KEXEC_OPTIONS \
> >> @@ -13,6 +14,7 @@
> >>          { "command-line",  1, NULL, OPT_APPEND }, \
> >>          { "dtb",           1, NULL, OPT_DTB }, \
> >>          { "initrd",        1, NULL, OPT_INITRD }, \
> >> +       { "console",       1, NULL, OPT_CONSOLE }, \
> >>          { "ramdisk",       1, NULL, OPT_INITRD }, \
> >>          { "reuse-cmdline", 0, NULL, OPT_REUSE_CMDLINE }, \
> >>
> >> @@ -25,6 +27,7 @@ static const char arm64_opts_usage[] __attribute__ ((unused)) =
> >>   "     --command-line=STRING Set the kernel command line to STRING.\n"
> >>   "     --dtb=FILE            Use FILE as the device tree blob.\n"
> >>   "     --initrd=FILE         Use FILE as the kernel initial ramdisk.\n"
> >> +"     --console=STRING      Console used for purgatory printing.\n"
> >>   "     --ramdisk=FILE        Use FILE as the kernel initial ramdisk.\n"
> >>   "     --reuse-cmdline       Use kernel command line from running system.\n";
> >
> > Just a thought... sometimes the console string is also available as a
> > part of the '--reuse-cmdline' command line argument passed to the
> > kdump kernel. Can we also try to extract the --console string from the
> > cmdline provided to the primary kernel itself?
> >
>
> Well the problem is, that there can be more then one consoles present, so which
> one would be the correct one?
> I see this more like a debug feature which you use knowing which console you are
> looking for the debug messages. In the end it only helps you to see if kdump
> failed in the production system kernel or in the crash kernel.

Indeed, I think we need to add some more comments to explain this
option and distinguish it better from the serial port(s) mentioned in
the '--reuse-cmdline' option.

> >> @@ -32,6 +35,7 @@ struct arm64_opts {
> >>          const char *command_line;
> >>          const char *dtb;
> >>          const char *initrd;
> >> +       const char *console;
> >>   };
> >>
> >>   extern struct arm64_opts arm64_opts;
> >> diff --git a/kexec/arch/arm64/kexec-arm64.c b/kexec/arch/arm64/kexec-arm64.c
> >> index 45ebc54..44c9e6e 100644
> >> --- a/kexec/arch/arm64/kexec-arm64.c
> >> +++ b/kexec/arch/arm64/kexec-arm64.c
> >> @@ -165,6 +165,8 @@ int arch_process_options(int argc, char **argv)
> >>                          break;
> >>                  case OPT_KEXEC_FILE_SYSCALL:
> >>                          do_kexec_file_syscall = 1;
> >> +               case OPT_CONSOLE:
> >> +                       arm64_opts.console = optarg;
> >>                          break;
> >>                  default:
> >>                          break; /* Ignore core and unknown options. */
> >> @@ -180,12 +182,62 @@ int arch_process_options(int argc, char **argv)
> >>          dbgprintf("%s:%d: dtb: %s\n", __func__, __LINE__,
> >>                  (do_kexec_file_syscall && arm64_opts.dtb ? "(ignored)" :
> >>                                                          arm64_opts.dtb));
> >> +       dbgprintf("%s:%d: console: %s\n", __func__, __LINE__,
> >> +               arm64_opts.console);
> >> +
> >>          if (do_kexec_file_syscall)
> >>                  arm64_opts.dtb = NULL;
> >>
> >>          return 0;
> >>   }
> >>
> >> +/**
> >> + * find_purgatory_sink - Find a sink for purgatory output.
> >> + */
> >> +
> >> +static uint64_t find_purgatory_sink(const char *console)
> >> +{
> >> +       int fd, ret;
> >> +       char folder[255], device[255], mem[255];
> >> +       struct stat sb;
> >> +       char buffer[18];
> >
> > Just trying to understand the reasoning behind keeping the buffer 18
> > chars long. Can the bytes read from the console exceed the array size
> > here (may be a boundary check is required here to avoid overflows)?
> >
>
> The buffer having a size of 18 is somewhat random. We use it to read the string
> at for example
> /sys/class/tty/ttyAMA0/iomem_base
>
> And we read the size of the buffer, so no overflow is possible here. But: we
> expect a value like 0x94080000 which has actually 10 characters.
>
> >> +       uint64_t iomem = 0x0;
> >> +
> >> +       if (!console)
> >> +               return 0;
> >> +
> >> +       sprintf(device, "/sys/class/tty/%s", console);
>
> But your comment made me think of another thing. If the console string from the
> command line is really long the sprintf would cause overflows. In v2 I'll update
> the code to use snprintf instead.

Indeed, I am mainly worried about overflow issues with sprintf (and
helpers), having recently spent time debugging several such overflow
issues in upstream code. If we have concerns we can allocate the char
array dynamically and free it after use. Also we can add some boundary
checks for the dynamically allocated char array and how it is printed
in the purgatory code (to make sure that there are no overflows here).

Thanks,
Bhupesh

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec