[PATCH 11/13] arm64/kexec: Add core kexec support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Geoff,

On Tue, Sep 09, 2014 at 11:49:05PM +0100, Geoff Levand wrote:
> Add three new files, kexec.h, machine_kexec.c and relocate_kernel.S to the
> arm64 architecture that add support for the kexec re-boot mechanism
> (CONFIG_KEXEC) on arm64 platforms.
>
> Signed-off-by: Geoff Levand <geoff at infradead.org>
> ---
>  arch/arm64/Kconfig                  |   8 +
>  arch/arm64/include/asm/kexec.h      |  52 +++
>  arch/arm64/kernel/Makefile          |   2 +
>  arch/arm64/kernel/machine_kexec.c   | 612 ++++++++++++++++++++++++++++++++++++
>  arch/arm64/kernel/relocate_kernel.S | 185 +++++++++++
>  include/uapi/linux/kexec.h          |   1 +
>  6 files changed, 860 insertions(+)
>  create mode 100644 arch/arm64/include/asm/kexec.h
>  create mode 100644 arch/arm64/kernel/machine_kexec.c
>  create mode 100644 arch/arm64/kernel/relocate_kernel.S
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index f0d3a2d..6f0e1f1 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -313,6 +313,14 @@ config ARCH_HAS_CACHE_LINE_SIZE
>
>  source "mm/Kconfig"
>
> +config KEXEC
> +       bool "kexec system call"
> +       ---help---
> +         kexec is a system call that implements the ability to shutdown your
> +         current kernel, and to start another kernel.  It is like a reboot
> +         but it is independent of the system firmware.   And like a reboot
> +         you can start any kernel with it, not just Linux.
> +
>  config XEN_DOM0
>         def_bool y
>         depends on XEN
> diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> new file mode 100644
> index 0000000..9a3932c
> --- /dev/null
> +++ b/arch/arm64/include/asm/kexec.h
> @@ -0,0 +1,52 @@
> +/*
> + * kexec for arm64
> + *
> + * Copyright (C) Linaro.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#if !defined(_ARM64_KEXEC_H)
> +#define _ARM64_KEXEC_H
> +
> +/* Maximum physical address we can use pages from */
> +
> +#define KEXEC_SOURCE_MEMORY_LIMIT (-1UL)
> +
> +/* Maximum address we can reach in physical address mode */
> +
> +#define KEXEC_DESTINATION_MEMORY_LIMIT (-1UL)
> +
> +/* Maximum address we can use for the control code buffer */
> +
> +#define KEXEC_CONTROL_MEMORY_LIMIT (-1UL)
> +

What are these used for? I see that other architectures seem to do the
same thing, but they look odd.

> +#define KEXEC_CONTROL_PAGE_SIZE        4096

What's this used for?

Does this work with 64k pages, and is there any reason we can't figure
out the actual size of the code (so we don't get bitten if it grows)?

> +
> +#define KEXEC_ARCH KEXEC_ARCH_ARM64
> +
> +#define ARCH_HAS_KIMAGE_ARCH
> +
> +#if !defined(__ASSEMBLY__)
> +
> +struct kimage_arch {
> +       void *ctx;
> +};
> +
> +/**
> + * crash_setup_regs() - save registers for the panic kernel
> + *
> + * @newregs: registers are saved here
> + * @oldregs: registers to be saved (may be %NULL)
> + */
> +
> +static inline void crash_setup_regs(struct pt_regs *newregs,
> +                                   struct pt_regs *oldregs)
> +{
> +}

It would be nice to know what we're going to do for this.

Is this a required function, or can we get away without crash kernel
support for the moment?

> +
> +#endif /* !defined(__ASSEMBLY__) */
> +
> +#endif
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index df7ef87..8b7c029 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -29,6 +29,8 @@ arm64-obj-$(CONFIG_ARM64_CPU_SUSPEND) += sleep.o suspend.o
>  arm64-obj-$(CONFIG_JUMP_LABEL)         += jump_label.o
>  arm64-obj-$(CONFIG_KGDB)               += kgdb.o
>  arm64-obj-$(CONFIG_EFI)                        += efi.o efi-stub.o efi-entry.o
> +arm64-obj-$(CONFIG_KEXEC)              += machine_kexec.o relocate_kernel.o    \
> +                                          cpu-properties.o
>
>  obj-y                                  += $(arm64-obj-y) vdso/
>  obj-m                                  += $(arm64-obj-m)
> diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c
> new file mode 100644
> index 0000000..043a3bc
> --- /dev/null
> +++ b/arch/arm64/kernel/machine_kexec.c
> @@ -0,0 +1,612 @@
> +/*
> + * kexec for arm64
> + *
> + * Copyright (C) Linaro.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/kexec.h>
> +#include <linux/of_fdt.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +
> +#include <asm/cacheflush.h>
> +#include <asm/cpu_ops.h>
> +#include <asm/system_misc.h>
> +
> +#include "cpu-properties.h"
> +
> +#if defined(DEBUG)
> +static const int debug = 1;
> +#else
> +static const int debug;
> +#endif

I don't think we need this.

> +
> +typedef struct dtb_buffer {char b[0]; } dtb_t;

It would be nice for this to be consistent with other dtb uses; if we
need a dtb type then it shouldn't be specific to kexec.

[...]

> +static struct kexec_ctx *current_ctx;
> +
> +static int kexec_ctx_alloc(struct kimage *image)
> +{
> +       BUG_ON(image->arch.ctx);
> +
> +       image->arch.ctx = kmalloc(sizeof(struct kexec_ctx), GFP_KERNEL);
> +
> +       if (!image->arch.ctx)
> +               return -ENOMEM;
> +
> +       current_ctx = (struct kexec_ctx *)image->arch.ctx;

This seems to be the only use of current_ctx. I take it this is a
leftover from debugging?

[...]

> +/**
> + * kexec_list_walk - Helper to walk the kimage page list.
> + */

Please keep this associated with the function it refers to (nothing
should be between this comment and the function prototype).

> +
> +#define IND_FLAGS (IND_DESTINATION | IND_INDIRECTION | IND_DONE | IND_SOURCE)

Can't this live in include/linux/kexec.h, where these flags are defined.

The meaning of these doesn't seem to be documented anywhere. Would you
be able to explain what each of these means?

> +static void kexec_list_walk(void *ctx, unsigned long kimage_head,
> +       void (*cb)(void *ctx, unsigned int flag, void *addr, void *dest))
> +{
> +       void *dest;
> +       unsigned long *entry;
> +
> +       for (entry = &kimage_head, dest = NULL; ; entry++) {
> +               unsigned int flag = *entry & IND_FLAGS;
> +               void *addr = phys_to_virt(*entry & PAGE_MASK);
> +
> +               switch (flag) {
> +               case IND_INDIRECTION:
> +                       entry = (unsigned long *)addr - 1;
> +                       cb(ctx, flag, addr, NULL);
> +                       break;
> +               case IND_DESTINATION:
> +                       dest = addr;
> +                       cb(ctx, flag, addr, NULL);
> +                       break;
> +               case IND_SOURCE:
> +                       cb(ctx, flag, addr, dest);
> +                       dest += PAGE_SIZE;

I really don't understand what's going on with dest here, but that's
probably because I don't understand the meaning of the flags.

> +                       break;
> +               case IND_DONE:
> +                       cb(ctx, flag , NULL, NULL);
> +                       return;
> +               default:
> +                       pr_devel("%s:%d unknown flag %xh\n", __func__, __LINE__,
> +                               flag);

Wouldn't pr_warn would be more appropriate here?

> +                       cb(ctx, flag, addr, NULL);
> +                       break;
> +               }
> +       }
> +}
> +
> +/**
> + * kexec_image_info - For debugging output.
> + */
> +
> +#define kexec_image_info(_i) _kexec_image_info(__func__, __LINE__, _i)
> +static void _kexec_image_info(const char *func, int line,
> +       const struct kimage *image)
> +{
> +       if (debug) {
> +               unsigned long i;
> +
> +               pr_devel("%s:%d:\n", func, line);
> +               pr_devel("  kexec image info:\n");
> +               pr_devel("    type:        %d\n", image->type);
> +               pr_devel("    start:       %lx\n", image->start);
> +               pr_devel("    head:        %lx\n", image->head);
> +               pr_devel("    nr_segments: %lu\n", image->nr_segments);
> +
> +               for (i = 0; i < image->nr_segments; i++) {
> +                       pr_devel("      segment[%lu]: %016lx - %016lx, "
> +                               "%lxh bytes, %lu pages\n",
> +                               i,
> +                               image->segment[i].mem,
> +                               image->segment[i].mem + image->segment[i].memsz,
> +                               image->segment[i].memsz,
> +                               image->segment[i].memsz /  PAGE_SIZE);
> +
> +                       if (kexec_is_dtb_user(image->segment[i].buf))
> +                               pr_devel("        dtb segment\n");
> +               }
> +       }
> +}

pr_devel is already dependent on DEBUG, so surely we don't need to check
the debug variable?

> +
> +/**
> + * kexec_find_dtb_seg - Helper routine to find the dtb segment.
> + */
> +
> +static const struct kexec_segment *kexec_find_dtb_seg(
> +       const struct kimage *image)
> +{
> +       int i;
> +
> +       for (i = 0; i < image->nr_segments; i++) {
> +               if (kexec_is_dtb_user(image->segment[i].buf))
> +                       return &image->segment[i];
> +       }
> +
> +       return NULL;
> +}

I'm really not keen on having the kernel guess which blobs need special
treatment, though we seem to do that for arm.

It would be far nicer if we could pass flags for each segment to
describe what it is (e.g. kernel image, dtb, other binary blob), so we
can do things like pass multiple DTBs (so we load two kernels at once
and pass each a unique DTB if we want to boot a new kernel + crashkernel
pair). Unfortunately that would require some fairly invasive rework of
the kexec core.

For secureboot we can't trust a dtb from userspace, and will have to use
kexec_file_load. To work with that we can either:

* Reuse the original DTB, patched with the new command line. This may
  have statefulness issues (for things like simplefb).

* Build a new DTB by flattening the current live tree. This would rely
  on drivers that modify state to patch the tree appropriately.

Both of those are somewhat ugly. :(

[...]

> +/**
> + * kexec_cpu_info_init - Initialize an array of kexec_cpu_info structures.
> + *
> + * Allocates a cpu info array and fills it with info for all cpus found in
> + * the device tree passed.
> + */
> +
> +static int kexec_cpu_info_init(const struct device_node *dn,
> +       struct kexec_boot_info *info)
> +{
> +       int result;
> +       unsigned int cpu;
> +
> +       info->cp = kmalloc(
> +               info->cpu_count * sizeof(*info->cp), GFP_KERNEL);
> +
> +       if (!info->cp) {
> +               pr_err("%s: Error: Out of memory.", __func__);
> +               return -ENOMEM;
> +       }
> +
> +       for (cpu = 0; cpu < info->cpu_count; cpu++) {
> +               struct cpu_properties *cp = &info->cp[cpu];
> +
> +               dn = of_find_node_by_type((struct device_node *)dn, "cpu");
> +
> +               if (!dn) {
> +                       pr_devel("%s:%d: bad node\n", __func__, __LINE__);
> +                       goto on_error;
> +               }
> +
> +               result = read_cpu_properties(cp, dn);
> +
> +               if (result) {
> +                       pr_devel("%s:%d: bad node\n", __func__, __LINE__);
> +                       goto on_error;
> +               }
> +
> +               if (cp->type == cpu_enable_method_psci)
> +                       pr_devel("%s:%d: cpu-%u: hwid-%llx, '%s'\n",
> +                               __func__, __LINE__, cpu, cp->hwid,
> +                               cp->enable_method);
> +               else
> +                       pr_devel("%s:%d: cpu-%u: hwid-%llx, '%s', "
> +                               "cpu-release-addr %llx\n",
> +                               __func__, __LINE__, cpu, cp->hwid,
> +                               cp->enable_method,
> +                               cp->cpu_release_addr);
> +       }
> +
> +       return 0;
> +
> +on_error:
> +       kfree(info->cp);
> +       info->cp = NULL;
> +       return -EINVAL;
> +}

I don't see why we should need this at all. If we use the hotplug
infrastructure, we don't need access to the enable-method and related
properties, and the kexec code need only deal with a single CPU.

The only case where kexec needs to deal with other CPUs is when some are
sat in the holding pen, but this code doesn't seem to handle that.

as I believe I mentioned before, we should be able to extend the holding
pen code to get those CPUs to increment a sat-in-pen counter and if
that's non-zero after SMP bringup we print a warning (and disallow
kexec).

[...]

> +/**
> +* kexec_compat_check - Iterator for kexec_cpu_check.
> +*/
> +
> +static int kexec_compat_check(const struct kexec_ctx *ctx)
> +{
> +       unsigned int cpu_1;
> +       unsigned int to_process;
> +
> +       to_process = min(ctx->first.cpu_count, ctx->second.cpu_count);
> +
> +       if (ctx->first.cpu_count != ctx->second.cpu_count)
> +               pr_warn("%s: Warning: CPU count mismatch %u != %u.\n",
> +                       __func__, ctx->first.cpu_count, ctx->second.cpu_count);
> +
> +       for (cpu_1 = 0; cpu_1 < ctx->first.cpu_count; cpu_1++) {
> +               unsigned int cpu_2;
> +               struct cpu_properties *cp_1 = &ctx->first.cp[cpu_1];
> +
> +               for (cpu_2 = 0; cpu_2 < ctx->second.cpu_count; cpu_2++) {
> +                       struct cpu_properties *cp_2 = &ctx->second.cp[cpu_2];
> +
> +                       if (cp_1->hwid != cp_2->hwid)
> +                               continue;
> +
> +                       if (!kexec_cpu_check(cp_1, cp_2))
> +                               return -EINVAL;
> +
> +                       to_process--;
> +               }
> +       }
> +
> +       if (to_process) {
> +               pr_warn("%s: Warning: Failed to process %u CPUs.\n", __func__,
> +                       to_process);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}

I don't see the point in checking this in the kernel. If I pass the
second stage kernel a new dtb where my enable methods are different,
that was my choice as a user. If that doesn't work, that's my fault.

There are plenty of other things that might be completely different that
we don't sanity check, so I don't see why enable methods should be any
different.

[...]

> +/**
> + * kexec_check_cpu_die -  Check if cpu_die() will work on secondary processors.
> + */
> +
> +static int kexec_check_cpu_die(void)
> +{
> +       unsigned int cpu;
> +       unsigned int sum = 0;
> +
> +       /* For simplicity this also checks the primary CPU. */
> +
> +       for_each_cpu(cpu, cpu_all_mask) {
> +               if (cpu && (!cpu_ops[cpu] || !cpu_ops[cpu]->cpu_disable ||
> +                       cpu_ops[cpu]->cpu_disable(cpu))) {
> +                       sum++;
> +                       pr_err("%s: Error: "
> +                               "CPU %u does not support hot un-plug.\n",
> +                               __func__, cpu);
> +               }
> +       }
> +
> +       return sum ? -EOPNOTSUPP : 0;
> +}

We should really use disable_nonboot_cpus() for this. That way we don't
end up with a slightly different hotplug implementation for kexec. The
above is missing cpu_kill calls, for example, and I'm worried by the
possibility of further drift over time.

I understand from our face-to-face discussion that you didn't want to
require the PM infrastructure that disable_nonboot_cpus currently pulls
in due to the being dependent on CONFIG_PM_SLEEP_SMP which selects
CONFIG_PM_SLEEP and so on. The solution to that is to refactor the
Kconfig so we can have disable_nonboot_cpus without all the other PM
infrastructure.

> +
> +/**
> + * machine_kexec_prepare - Prepare for a kexec reboot.
> + *
> + * Called from the core kexec code when a kernel image is loaded.
> + */
> +
> +int machine_kexec_prepare(struct kimage *image)
> +{
> +       int result;

This seems to always be an error code. Call it 'err'.

> +       dtb_t *dtb = NULL;
> +       struct kexec_ctx *ctx;
> +       const struct kexec_segment *dtb_seg;
> +
> +       kexec_image_info(image);
> +
> +       result = kexec_check_cpu_die();
> +
> +       if (result)
> +               goto on_error;
> +
> +       result = kexec_ctx_alloc(image);
> +
> +       if (result)
> +               goto on_error;
> +
> +       ctx = kexec_image_to_ctx(image);
> +
> +       result = kexec_boot_info_init(&ctx->first, NULL);
> +
> +       if (result)
> +               goto on_error;
> +
> +       dtb_seg = kexec_find_dtb_seg(image);
> +
> +       if (!dtb_seg) {
> +               result = -EINVAL;
> +               goto on_error;
> +       }
> +
> +       result = kexec_copy_dtb(dtb_seg, &dtb);
> +
> +       if (result)
> +               goto on_error;
> +
> +       result = kexec_boot_info_init(&ctx->second, dtb);
> +
> +       if (result)
> +               goto on_error;
> +
> +       result = kexec_compat_check(ctx);
> +
> +       if (result)
> +               goto on_error;
> +
> +       kexec_dtb_addr = dtb_seg->mem;
> +       kexec_kimage_start = image->start;
> +
> +       goto on_exit;
> +
> +on_error:
> +       kexec_ctx_clean(image);
> +on_exit:

on_* looks weird, and doesn't match the style of other labels in
arch/arm64. Could we call these 'out_clean' and 'out' instead?

> +       kfree(dtb);
> +       return result;
> +}
> +
> +/**
> + * kexec_list_flush_cb - Callback to flush the kimage list to PoC.
> + */
> +
> +static void kexec_list_flush_cb(void *ctx , unsigned int flag,
> +       void *addr, void *dest)
> +{
> +       switch (flag) {
> +       case IND_INDIRECTION:
> +       case IND_SOURCE:
> +               __flush_dcache_area(addr, PAGE_SIZE);

Is PAGE_SIZE always big enough? Do we not have a more accurate size?

Perhaps I've misunderstood what's going on here.

> +               break;
> +       default:
> +               break;
> +       }
> +}
> +
> +/**
> + * machine_kexec - Do the kexec reboot.
> + *
> + * Called from the core kexec code for a sys_reboot with LINUX_REBOOT_CMD_KEXEC.
> + */
> +
> +void machine_kexec(struct kimage *image)
> +{
> +       phys_addr_t reboot_code_buffer_phys;
> +       void *reboot_code_buffer;
> +       struct kexec_ctx *ctx = kexec_image_to_ctx(image);
> +
> +       BUG_ON(relocate_new_kernel_size > KEXEC_CONTROL_PAGE_SIZE);

It looks like relocate_new_kernel_size is a build-time constant. If we
need that to be less than KEXEC_CONTROL_PAGE_SIZE, then we should make
that a build-time check.

> +       BUG_ON(num_online_cpus() > 1);
> +       BUG_ON(!ctx);
> +
> +       kexec_image_info(image);
> +
> +       kexec_kimage_head = image->head;
> +
> +       reboot_code_buffer_phys = page_to_phys(image->control_code_page);
> +       reboot_code_buffer = phys_to_virt(reboot_code_buffer_phys);
> +
> +       pr_devel("%s:%d: control_code_page:        %p\n", __func__, __LINE__,
> +               (void *)image->control_code_page);

This is already a pointer. Is the cast to void necessary?

> +       pr_devel("%s:%d: reboot_code_buffer_phys:  %p\n", __func__, __LINE__,
> +               (void *)reboot_code_buffer_phys);

Use %pa and pass &reboot_code_buffer_phys, no cast necessary.

> +       pr_devel("%s:%d: reboot_code_buffer:       %p\n", __func__, __LINE__,
> +               reboot_code_buffer);
> +       pr_devel("%s:%d: relocate_new_kernel:      %p\n", __func__, __LINE__,
> +               relocate_new_kernel);
> +       pr_devel("%s:%d: relocate_new_kernel_size: %lxh(%lu) bytes\n", __func__,
> +               __LINE__, relocate_new_kernel_size, relocate_new_kernel_size);

Please use an '0x' prefix rather than a 'h' suffix. Do we need in print
in both hex and decimal?

> +
> +       pr_devel("%s:%d: kexec_dtb_addr:           %p\n", __func__, __LINE__,
> +               (void *)kexec_dtb_addr);
> +       pr_devel("%s:%d: kexec_kimage_head:        %p\n", __func__, __LINE__,
> +               (void *)kexec_kimage_head);
> +       pr_devel("%s:%d: kexec_kimage_start:       %p\n", __func__, __LINE__,
> +               (void *)kexec_kimage_start);

These are all unsigned long, so why not use the existing mechanism for
printing unsigned long?

> +
> +       /*
> +        * Copy relocate_new_kernel to the reboot_code_buffer for use
> +        * after the kernel is shut down.
> +        */
> +
> +       memcpy(reboot_code_buffer, relocate_new_kernel,
> +               relocate_new_kernel_size);
> +
> +       /* Assure reboot_code_buffer is copied. */
> +
> +       mb();

I don't think we need the mb if this is only to guarantee completion
before the cache flush -- cacheable memory accesses should hazard
against cache flushes by VA.

> +
> +       pr_info("Bye!\n");
> +
> +       local_disable(DAIF_ALL);

We can move these two right before the soft_restart, after the cache
maintenance. That way the print is closer to the exit of the current
kernel.

> +
> +       /* Flush the reboot_code_buffer in preparation for its execution. */
> +
> +       __flush_dcache_area(reboot_code_buffer, relocate_new_kernel_size);
> +
> +       /* Flush the kimage list. */
> +
> +       kexec_list_walk(NULL, image->head, kexec_list_flush_cb);
> +
> +       soft_restart(reboot_code_buffer_phys);
> +}
> +
> +void machine_crash_shutdown(struct pt_regs *regs)
> +{
> +       /* Empty routine needed to avoid build errors. */
> +}
> diff --git a/arch/arm64/kernel/relocate_kernel.S b/arch/arm64/kernel/relocate_kernel.S
> new file mode 100644
> index 0000000..92aba9d
> --- /dev/null
> +++ b/arch/arm64/kernel/relocate_kernel.S
> @@ -0,0 +1,185 @@
> +/*
> + * kexec for arm64
> + *
> + * Copyright (C) Linaro.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <asm/assembler.h>
> +#include <asm/memory.h>
> +#include <asm/page.h>
> +
> +/* The list entry flags. */
> +
> +#define IND_DESTINATION_BIT 0
> +#define IND_INDIRECTION_BIT 1
> +#define IND_DONE_BIT        2
> +#define IND_SOURCE_BIT      3

Given these ned to match the existing IND_* flags in
include/linux/kexec.h, and they aren't in any way specific to arm64,
please put these ina an asm-generic header and redefine the existing
IND_* flags in terms of them.

> +
> +/*
> + * relocate_new_kernel - Put the 2nd stage kernel image in place and boot it.
> + *
> + * The memory that the old kernel occupies may be overwritten when coping the
> + * new kernel to its final location.  To assure that the relocate_new_kernel
> + * routine which does that copy is not overwritten all code and data needed
> + * by relocate_new_kernel must be between the symbols relocate_new_kernel and
> + * relocate_new_kernel_end.  The machine_kexec() routine will copy
> + * relocate_new_kernel to the kexec control_code_page, a special page which
> + * has been set up to be preserved during the kernel copy operation.
> + */
> +
> +.align 3

Surely this isn't necessary?

> +
> +.globl relocate_new_kernel
> +relocate_new_kernel:
> +
> +       /* Setup the list loop variables. */
> +
> +       ldr     x10, kexec_kimage_head          /* x10 = list entry */

Any reason for using x10 rather than starting with x0? Or x18, if you
need to preserve the low registers?

> +
> +       mrs     x0, ctr_el0
> +       ubfm    x0, x0, #16, #19
> +       mov     x11, #4
> +       lsl     x11, x11, x0                    /* x11 = dcache line size */

Any reason we can't use dcache_line_size, given it's a macro?

> +
> +       mov     x12, xzr                        /* x12 = segment start */
> +       mov     x13, xzr                        /* x13 = entry ptr */
> +       mov     x14, xzr                        /* x14 = copy dest */
> +
> +       /* Check if the new kernel needs relocation. */
> +
> +       cbz     x10, .Ldone
> +       tbnz    x10, IND_DONE_BIT, .Ldone
> +
> +.Lloop:

Is there any reason for the '.L' on all of these? We only seem to do
that in the lib code that was imported from elsewhere, and it doesn't
match the rest of the arm64 asm.

> +       and     x15, x10, PAGE_MASK             /* x15 = addr */
> +
> +       /* Test the entry flags. */
> +
> +.Ltest_source:
> +       tbz     x10, IND_SOURCE_BIT, .Ltest_indirection
> +
> +       /* copy_page(x20 = dest, x21 = src) */
> +
> +       mov x20, x14
> +       mov x21, x15
> +
> +1:     ldp     x22, x23, [x21]
> +       ldp     x24, x25, [x21, #16]
> +       ldp     x26, x27, [x21, #32]
> +       ldp     x28, x29, [x21, #48]
> +       add     x21, x21, #64
> +       stnp    x22, x23, [x20]
> +       stnp    x24, x25, [x20, #16]
> +       stnp    x26, x27, [x20, #32]
> +       stnp    x28, x29, [x20, #48]
> +       add     x20, x20, #64
> +       tst     x21, #(PAGE_SIZE - 1)
> +       b.ne    1b

It's a shame we can't reuse copy_page directly. Could we not move the
body to a macro we can reuse here?

> +
> +       /* dest += PAGE_SIZE */
> +
> +       add     x14, x14, PAGE_SIZE
> +       b       .Lnext
> +
> +.Ltest_indirection:
> +       tbz     x10, IND_INDIRECTION_BIT, .Ltest_destination
> +
> +       /* ptr = addr */
> +
> +       mov     x13, x15
> +       b       .Lnext
> +
> +.Ltest_destination:
> +       tbz     x10, IND_DESTINATION_BIT, .Lnext
> +
> +       /* flush segment */
> +
> +       bl      .Lflush
> +       mov     x12, x15
> +
> +       /* dest = addr */
> +
> +       mov     x14, x15
> +
> +.Lnext:
> +       /* entry = *ptr++ */
> +
> +       ldr     x10, [x13]
> +       add     x13, x13, 8

This can be:

	ldr	x10, [x13], #8

> +
> +       /* while (!(entry & DONE)) */
> +
> +       tbz     x10, IND_DONE_BIT, .Lloop
> +
> +.Ldone:
> +       /* flush last segment */
> +
> +       bl      .Lflush
> +
> +       dsb     sy
> +       isb
> +       ic      ialluis

This doesn't look right; we need a dsb and an isb after the instruction
cache maintenance (or the icache could still be flushing when we branch
to the new kernel).

> +
> +       /* start_new_kernel */
> +
> +       ldr     x4, kexec_kimage_start
> +       ldr     x0, kexec_dtb_addr
> +       mov     x1, xzr
> +       mov     x2, xzr
> +       mov     x3, xzr
> +       br      x4
> +
> +/* flush - x11 = line size, x12 = start addr, x14 = end addr. */
> +
> +.Lflush:
> +       cbz     x12, 2f
> +       mov     x0, x12
> +       sub     x1, x11, #1
> +       bic     x0, x0, x1
> +1:     dc      civac, x0
> +       add     x0, x0, x11
> +       cmp     x0, x14
> +       b.lo    1b
> +2:     ret

It would be nice if this were earlier in the file, before its callers.

> +
> +.align 3

We should have a comment as to why this is needed (to keep the 64-bit
values below naturally aligned).

Thanks,
Mark.



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux