[PATCH] arm64: Add purgatory printing
Matthias Brugger
mbrugger at suse.com
Fri Sep 18 04:31:45 EDT 2020
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 at kernel.org> wrote:
>>
>> From: Matthias Brugger <mbrugger at 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 at infradead.org>
>> Signed-off-by: Matthias Brugger <mbrugger at 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
>
More information about the kexec
mailing list