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@xxxxxxxxxx> wrote:From: Matthias Brugger <mbrugger@xxxxxxxx> 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@xxxxxxxxxxxxx> Signed-off-by: Matthias Brugger <mbrugger@xxxxxxxx> --- 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_baseAnd 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.0Otherwise the patch looks ok to me. Thanks, Bhupesh
_______________________________________________ kexec mailing list kexec@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/kexec
