arm64 kexec kernel patches

The current implementation of ipi_cpu_stop() is just a tight infinite loop around cpu_relax(). Add a check for a valid cpu_die method of the appropriate cpu_operations structure, and if a valid method is found, transfer control to that method.

The core kexec code calls the arch specific machine_shutdown() routine to shutdown any SMP secondary CPUs. The current implementation of the arm64 machine_shutdown() uses smp_send_stop(), which ultimately runs ipi_cpu_stop() on the secondary CPUs. The infinite loop implementation of the current ipi_cpu_stop() does not have any mechanism to get the CPU into a state compatable with a kexec re-boot.

Signed-off-by Geoff Levand <[email protected]>

---
 arch/arm64/kernel/smp.c | 8 ++++++++
 1 file changed, 8 insertions(+)

arch/arm64/kernel/smp.c +0 -0

index f0a141d..020bbd5 100644

arch/arm64/kernel/smp.c static void ipi_cpu_stop(unsigned int cpu) · line 515

Quoted 5 lines · Geoff Levand

arch/arm64/kernel/smp.c +5 -0

+	/* If we have the cup_ops use them. */
+
+	if (cpu_ops[cpu]->cpu_disable && cpu_ops[cpu]->cpu_die
+		&& !cpu_ops[cpu]->cpu_disable(cpu))
+		cpu_ops[cpu]->cpu_die(cpu);

For PSCI 0.2 support, we're going to need a cpu_kill callback which we can't call from the dying CPU. Specifically, we'll need to poll CPU_AFFINITY_INFO to ensure that secondaries have _actually_ left the kernel and aren't going to be adversely affected by the kernel text getting clobbered.

As we're going to wire that up to the cpu hotplug infrastructure it would be nice to perform the hotplug for kexec by reusing the generic hotplug infrastructure rather than calling portions of the arm64 implementation directly.

arch/arm64/kernel/smp.c static void ipi_cpu_stop(unsigned int cpu) · line 520

Quoted 5 lines

arch/arm64/kernel/smp.c +3 -0

+
+	/* Spin here if the cup_ops fail. */
+
 	while (1)
 		cpu_relax();

This seems very dodgy to me. If a CPU doesn't actually die it's going to be spinning in some memory that we may later clobber. At that point the CPU will do arbitrarily bad things when it begins executing whatever its currently executing instructions (or vectors) were replaced by, and you will waste hours trying to figure out what went wrong (See 8121cf312a19 "ARM: 7766/1: versatile: don't mark pen as __INIT" for a similar mess).

If we fail to hotplug a CPU we at minimum need some acknowledgement that we failed. I would rather we failed to kexec entirely in that case.

Add two new minimal routines smp_spin_table_cpu_disable() and smp_spin_table_cpu_die() and hook them up to the smp_spin_table_ops instance.

Kexec support will use smp_spin_table_cpu_die() for re-boot of spin table CPUs, but also needs a compatible smp_spin_table_cpu_disable() to allow execution to reach smp_spin_table_cpu_die().

Signed-off-by Geoff Levand <[email protected]>

---
 arch/arm64/kernel/smp_spin_table.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

arch/arm64/kernel/smp_spin_table.c +0 -0

index 7a530d2..26c780b 100644

arch/arm64/kernel/smp_spin_table.c +4 -0

+}
+
 const struct cpu_operations smp_spin_table_ops = {
 	.name		= "spin-table",
 	.cpu_init	= smp_spin_table_cpu_init,
 	.cpu_prepare	= smp_spin_table_cpu_prepare,
 	.cpu_boot	= smp_spin_table_cpu_boot,
 	.cpu_postboot	= smp_spin_table_cpu_postboot,
+	.cpu_disable	= smp_spin_table_cpu_disable,
+	.cpu_die	= smp_spin_table_cpu_die,
 };

Signature · 8 lines

1.9.1



_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Kexec uses the cpu_die method of struct cpu_operations, so add defined(CONFIG_KEXEC) to the preprocessor conditional that enables cpu_die.

Signed-off-by Geoff Levand <[email protected]>

---
 arch/arm64/kernel/psci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/arm64/kernel/psci.c +1 -1

index ea4828a..0e5fa69 100644
@@ -253,7 +253,7 @@ static int cpu_psci_cpu_boot(unsigned int cpu)
 	return err;
 }
 
-#ifdef CONFIG_HOTPLUG_CPU
+#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_KEXEC)
 static int cpu_psci_cpu_disable(unsigned int cpu)
 {
 	/* Fail early if we don't have CPU_OFF support */

Hi Geoff,

Quoted 2 lines · Geoff Levand

Kexec uses the cpu_die method of struct cpu_operations, so add defined(CONFIG_KEXEC) to the preprocessor conditional that enables cpu_die.

Why not make kexec depend on !CONFIG_SMP || CONFIG_HOTPLUG_CPU instead?

From the POV of the PSCI code in the kernel, it's hotplugging a CPU. Why it's performing the hotplug operation shouldn't matter.

Quoted 4 lines

arch/arm64/kernel/psci.c +0 -0

@@ -284,7 +284,7 @@ const struct cpu_operations cpu_psci_ops = {
 	.cpu_init	= cpu_psci_cpu_init,
 	.cpu_prepare	= cpu_psci_cpu_prepare,
 	.cpu_boot	= cpu_psci_cpu_boot,

arch/arm64/kernel/psci.c const struct cpu_operations cpu_psci_ops = { · line 290

Quoted 5 lines

arch/arm64/kernel/psci.c +1 -1

-#ifdef CONFIG_HOTPLUG_CPU
+#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_KEXEC)
 	.cpu_disable	= cpu_psci_cpu_disable,
 	.cpu_die	= cpu_psci_cpu_die,
 #endif

Doesn't his cause the build to fail when KEXEC && !HOTPLUG_CPU? I didn't see cpu_ops.h updated similarly.

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 on arm64 (CONFIG_KEXEC).

This implementation supports re-boots of kernels with either PSCI or spin-table enable methods, but with some limitations on the match of 1st and 2nd stage kernels. The known limitations are checked in the kexec_compat_check() routine, which is called during a kexec_load syscall. If any limitations are reached an error is returned by the kexec_load syscall. Many of the limitations can be removed with some enhancment to the CPU shutdown management code in machine_kexec.c.

arch/arm64/kernel/machine_kexec.c new file +6 -0

+/**
+ * kexec_read_memreserve - Initialize memreserve info from a dtb.
+ */
+
+static int kexec_read_memreserve(const void *dtb, struct kexec_dt_info *info)
+{

arch/arm64/kernel/machine_kexec.c new file +6 -0

+static int kexec_setup_cpu_spin(const struct device_node *dn,
+       struct kexec_cpu_info_spin *info)
+{
+       int result;
+       u64 t1;
+

arch/arm64/kernel/machine_kexec.c new file +11 -0

+int machine_kexec_prepare(struct kimage *image)
+{
+       int result;
+       const struct kexec_segment *seg;
+       void *dtb;
+
+       machine_kexec_cleanup(NULL);
+
+       ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
+
+       if (!ctx) {

arch/arm64/kernel/machine_kexec.c new file +17 -0

+
+       result = kexec_dt_info_init(NULL, &ctx->dt_1st);
+
+       if (result)
+               goto on_error;
+
+       result = kexec_dt_info_init(dtb, &ctx->dt_2nd);
+
+       if (result)
+               goto on_error;
+
+       if (ctx->dt_2nd.spinner_count) {
+               BUG_ON(!ctx->dt_2nd.phy_memreserve_addr);
+               BUG_ON(kexec_cpu_spin_size >= ctx->dt_2nd.memreserve_size
+                       - kexec_spin_code_offset);
+       }
+

arch/arm64/kernel/machine_kexec.c new file +29 -0

+               va = phys_to_virt(ctx->dt_2nd.phy_memreserve_addr
+                       + kexec_spin_code_offset);
+
+               memcpy(va, kexec_cpu_spin, kexec_cpu_spin_size);
+
+               flush_icache_range((unsigned long)va,
+                       (unsigned long)va + kexec_cpu_spin_size);
+
+               /*
+                * Zero the release address for all the 2nd stage cpus.
+                */
+
+               for (cpu = 0; cpu < ctx->dt_2nd.cpu_count; cpu++) {
+                       u64 *release_addr;
+
+                       if (!ctx->dt_2nd.cpu_info[cpu].spinner)
+                               continue;
+
+                       release_addr = phys_to_virt(
+                               ctx->dt_2nd.cpu_info[cpu].spin.phy_release_addr);
+
+                       *release_addr = 0;
+
+                       __flush_dcache_area(release_addr, sizeof(u64));
+               }
+       }
+
+       /*
+        * Copy relocate_new_kernel to the reboot_code_buffer for use

+/*
+ * kexec_cpu_spin - Spin the CPU as described in the arm64/booting.txt document.
+ *
+ * Prototype: void kexec_cpu_spin(phys_addr_t release_addr, phys_addr_t signal);
+ *
+ * The caller must initialize release_addr to zero or a valid address
+ * prior to calling kexec_cpu_spin. Note that if the MMU will be turned on
+ * or off while the CPU is spinning here this code must be in an identity
+ * mapped page. The value written to release_addr must be in little endian
+ * order.

arch/arm64/kernel/relocate_kernel.S new file +8 -0

+/*
+ * 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

Quoted 11 lines · Geoff Levand

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 on arm64 (CONFIG_KEXEC).

This implementation supports re-boots of kernels with either PSCI or spin-table enable methods, but with some limitations on the match of 1st and 2nd stage kernels. The known limitations are checked in the kexec_compat_check() routine, which is called during a kexec_load syscall. If any limitations are reached an error is returned by the kexec_load syscall. Many of the limitations can be removed with some enhancment to the CPU shutdown management code in machine_kexec.c.

I think if we cannot offline a CPU through the generic hotplug infrastructure then we should fail to kexec. If we need a way of doing hotplug without PSCI then we should sort that out [1] rather than working around brokenness with more brokenness

I also don't think that kexec should care about the precise hotplug mechanism so long as it works. If the DTB passed to the new kernel describes a different mechanism than is currently in use, that's the caller's choice and I don't see any point second-guessing that -- we're already allowing them to boot an arbitrary kernel and so long as we leave the system in a sane state I don't see a good reason to deny the user from immediately destroying that.

[1] For hotplug without PSCI I think we can get away with adding an optional property to spin-table describing a (physical) address to branch to which returns CPUs to their original spin code. So long as the kernel zeroes the release address first we should be able to boot a CPU exactly as we managed to the first time.

For spin-table we'll also need to jump back up to EL2 when EL2 is present. It should be possible to do that within the spin-table code if we update the initial EL2 vector and get KVM to tear itself down before cpu_die happens.

arch/arm64/kernel/machine_kexec.c line 40

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c new file +6 -0

+/**
+ * struct kexec_cpu_info_spin - Info needed for the "spin table" enable method.
+ */
+
+struct kexec_cpu_info_spin {
+       phys_addr_t phy_release_addr; /* cpu order */

I assume you mean this is in the endianness of the current kernel? I was initially confused by the comment, and I think it might be better to drop it -- unless told that a variable is in a specific endianness I would assume that it's the current native endianness anyway.

As a general point, could you please use "phys" rather than "phy" in variable names? It'll make this more consistent with the rest of the arm64 code, easier to search for, and reads better IMO.

arch/arm64/kernel/machine_kexec.c line 78

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c new file +6 -0

+struct kexec_ctx {
+       struct kexec_dt_info dt_1st;
+       struct kexec_dt_info dt_2nd;
+};
+
+static struct kexec_ctx *ctx;

Is there any reason this should be dynamically allocated?

Do we even need the current DTB if we rely on hotplug?

arch/arm64/kernel/machine_kexec.c line 105

Quoted 9 lines

arch/arm64/kernel/machine_kexec.c new file +9 -0

+/**
+ * kexec_is_dtb_user - Helper routine to check the device tree header signature.
+ */
+
+static int kexec_is_dtb_user(const void *dtb)
+{
+       __be32 magic;
+
+       get_user(magic, (__be32 *)dtb);

Return value check? Unless we know this can't fail?

If it can fail, surely we should return an appropriate error and forward it to userspace. EFAULT?

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+       return kexec_is_dtb(magic);

And EINVAL if we can read this but it's not a DTB?

Quoted 11 lines

arch/arm64/kernel/machine_kexec.c new file +11 -0

+/**
+ * kexec_read_memreserve - Initialize memreserve info from a dtb.
+ */
+
+static int kexec_read_memreserve(const void *dtb, struct kexec_dt_info *info)
+{
+       const struct boot_param_header *h = dtb;
+       struct pair {
+               __be64 phy_addr;
+               __be64 size;
+       } const *pair;

arch/arm64/kernel/machine_kexec.c line 171

Quoted 5 lines

arch/arm64/kernel/machine_kexec.c new file +5 -0

+
+       pair = dtb + be32_to_cpu(h->off_mem_rsvmap);
+
+       if ((pair + 1)->size)
+               pr_warn("kexec: Multiple memreserve regions found.");

Huh? Multiple arbitrary memreserves are entirely valid. Why should we warn in that case?

arch/arm64/kernel/machine_kexec.c line 174

Quoted 3 lines

arch/arm64/kernel/machine_kexec.c new file +3 -0

+
+       info->phy_memreserve_addr = be64_to_cpu(pair->phy_addr);
+       info->memreserve_size = be64_to_cpu(pair->size);

So we're assuming that the memory described in an arbitrary memreserve entry (which is intended to describe memory which shouldn't be touched unless we know what we're doing) is for our arbitrary use!?

NAK.

We shouldn't need to special-case reserved memory handling if we rely on cpu hotplug. If we don't then the only functional option is to add a memreserve, but that will end up leaking a small amount of memory upon every kexec. I believe that the former is the only sane option.

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c new file +6 -0

+static int kexec_setup_cpu_spin(const struct device_node *dn,
+       struct kexec_cpu_info_spin *info)
+{
+       int result;
+       u64 t1;
+

arch/arm64/kernel/machine_kexec.c line 204

Quoted 10 lines

arch/arm64/kernel/machine_kexec.c new file +10 -0

+       memset(info, 0, sizeof(*info));
+
+       result = of_property_read_u64(dn, "cpu-release-addr", &t1);
+
+       if (result) {
+               pr_warn("kexec: Read cpu-release-addr failed.\n");
+               return result;
+       }
+
+       info->phy_release_addr = le64_to_cpu(t1);

Why are we calling le64_to_cpu here?

of_property_read_u64 reads a be64 value from dt into cpu endianness, so at the very least the annotation is the wrong way around.

Have you tested this with a BE kernel? We should ensure that LE->LE, LE->BE, BE->BE, BE->LE all work.

Quoted 5 lines

arch/arm64/kernel/machine_kexec.c new file +5 -0

+int kexec_cpu_info_init(const struct device_node *dn,
+       struct kexec_dt_info *info)
+{
+       int result;
+       unsigned int cpu;

arch/arm64/kernel/machine_kexec.c line 225

Quoted 5 lines

arch/arm64/kernel/machine_kexec.c new file +5 -0

+       const struct device_node *i;
+
+       info->cpu_info = kmalloc(
+               (1 + info->cpu_count) * sizeof(struct kexec_cpu_info),
+               GFP_KERNEL);

Why one more than the cpu count? I thought cpu_count covered all the CPUs in the dtb?

[...]

Quoted 9 lines

arch/arm64/kernel/machine_kexec.c new file +9 -0

+int kexec_dt_info_init(void *dtb, struct kexec_dt_info *info)
+{
+       int result;
+       struct device_node *i;
+       struct device_node *dn;
+
+       if (!dtb) {
+               /* 1st stage. */
+               dn = NULL;

arch/arm64/kernel/machine_kexec.c line 296

Quoted 4 lines

arch/arm64/kernel/machine_kexec.c new file +4 -0

+       } else {
+               /* 2nd stage. */
+
+               of_fdt_unflatten_tree(dtb, &dn);

This may fail. We should check that dn is not NULL before we try to use it later -- many of_* functions will traverse the current kernel's boot DT if provided with a NULL root.

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c new file +6 -0

+
+               result = kexec_read_memreserve(dtb, info);
+
+               if (result)
+                       return result;
+       }

arch/arm64/kernel/machine_kexec.c line 308

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c new file +6 -0

+
+       /*
+        * We may need to work with offline cpus to get them into the correct
+        * state for a given enable method to work, and so need an info_array
+        * that has info about all the platform cpus.
+        */

What exactly do we need to do to offline CPUs?

Quoted 7 lines

arch/arm64/kernel/machine_kexec.c new file +7 -0

+void kexec_spin_2(unsigned int cpu, phys_addr_t signal_1,
+       phys_addr_t phy_release_addr, phys_addr_t signal_2)
+{
+       typedef void (*fn_t)(phys_addr_t, phys_addr_t);
+
+       fn_t spin_3;
+

arch/arm64/kernel/machine_kexec.c line 342

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c new file +6 -0

+       atomic_dec((atomic_t *)signal_1);
+
+       /* Wait for next signal. */
+
+       while (!atomic_read((atomic_t *)signal_2))
+               (void)0;

Why not cpu_relax()?

[...]

arch/arm64/kernel/machine_kexec.c line 452

Quoted 7 lines

arch/arm64/kernel/machine_kexec.c new file +7 -0

+       /* Check for cpus still spinning in secondary_holding_pen. */
+
+       if (NR_CPUS < dt1->spinner_count) {
+               pr_err("kexec: Error: NR_CPUS too small for spin enable %u < %u.\n",
+                       NR_CPUS, dt1->spinner_count + 1);
+               result++;
+       }

In some cases people might describe fewer CPUs in the DTB than are actually present, which we should give up for also. I think we can alter secondary_holding_pen to be a bit more intelligent for that case and get any unexpected secondaries to write a flag to indicate their presence.

arch/arm64/kernel/machine_kexec.c line 474

Quoted 3 lines

arch/arm64/kernel/machine_kexec.c new file +3 -0

+void machine_crash_shutdown(struct pt_regs *regs)
+{
+}

Missing implementation? If it's fine for this to be empty it would be nice to have a comment to that effect.

Quoted 15 lines

arch/arm64/kernel/machine_kexec.c new file +15 -0

+int machine_kexec_prepare(struct kimage *image)
+{
+       int result;
+       const struct kexec_segment *seg;
+       void *dtb;
+
+       machine_kexec_cleanup(NULL);
+
+       ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
+
+       if (!ctx) {
+               pr_debug("%s: out of memory", __func__);
+               return -ENOMEM;
+       }
+

arch/arm64/kernel/machine_kexec.c line 502

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c new file +6 -0

+       seg = kexec_find_dtb_seg(image);
+       BUG_ON(!seg);
+
+       dtb = kexec_copy_dtb(seg);
+       BUG_ON(!dtb);
+       BUG_ON(!kexec_is_dtb(*(const __be32 *)dtb));

Why BUG_ON rather than report the failure and return an error?

Quoted 17 lines

arch/arm64/kernel/machine_kexec.c new file +17 -0

+
+       result = kexec_dt_info_init(NULL, &ctx->dt_1st);
+
+       if (result)
+               goto on_error;
+
+       result = kexec_dt_info_init(dtb, &ctx->dt_2nd);
+
+       if (result)
+               goto on_error;
+
+       if (ctx->dt_2nd.spinner_count) {
+               BUG_ON(!ctx->dt_2nd.phy_memreserve_addr);
+               BUG_ON(kexec_cpu_spin_size >= ctx->dt_2nd.memreserve_size
+                       - kexec_spin_code_offset);
+       }
+

arch/arm64/kernel/machine_kexec.c line 529

Quoted 10 lines

arch/arm64/kernel/machine_kexec.c new file +10 -0

+       result = kexec_compat_check(&ctx->dt_1st, &ctx->dt_2nd);
+
+       if (result)
+               goto on_error;
+
+       kexec_dtb_addr = seg->mem;
+       kexec_kimage_start = image->start;
+       kexec_spinner_count = ctx->dt_1st.spinner_count - 1;
+
+       smp_spin_table_set_die(kexec_spin_1);

I very much dislike hooking into the spin-table code like this.

[...]

arch/arm64/kernel/machine_kexec.c line 570

Quoted 7 lines

arch/arm64/kernel/machine_kexec.c new file +7 -0

+       if (ctx->dt_2nd.spinner_count) {
+               void *va;
+
+               /*
+               * Copy the spin code to the 2nd stage memreserve area as
+               * dictated by the arm64 boot specification.
+               */

The boot documentation says any spin code must be protected with a memreserve. This does not mean that the first memreserve is a special location that the spin table must be placed at. This comment seems to have the implication backwards.

Quoted 9 lines

+ * kexec_cpu_spin - Spin the CPU as described in the arm64/booting.txt document.
+ *
+ * Prototype: void kexec_cpu_spin(phys_addr_t release_addr, phys_addr_t signal);
+ *
+ * The caller must initialize release_addr to zero or a valid address
+ * prior to calling kexec_cpu_spin. Note that if the MMU will be turned on
+ * or off while the CPU is spinning here this code must be in an identity
+ * mapped page. The value written to release_addr must be in little endian
+ * order.

The MMU _must_ be off upon entry to the kernel, as is explicitly stated in Documentation/arm64/booting.txt, and I don't see why the spinning code should have the MMU on. It seems like an endless source of subtle bugs.

Quoted 14 lines

arch/arm64/kernel/relocate_kernel.S new file +14 -0

+/*
+ * 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.
+ */
+
+/* These definitions correspond to the kimage_entry flags in linux/kexec.h */
+

arch/arm64/kernel/relocate_kernel.S line 84

Quoted 4 lines

arch/arm64/kernel/relocate_kernel.S new file +4 -0

+#define IND_DESTINATION_BIT 0
+#define IND_INDIRECTION_BIT 1
+#define IND_DONE_BIT        2
+#define IND_SOURCE_BIT      3

These should live in linux/kexec.h -- the existing macros can be generated from these and we should be able to protect everything with #ifndef __ASSEMBLY__

Hi Mark,

Quoted 13 lines · Mark Rutland

Quoted 11 lines · Geoff Levand

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 on arm64 (CONFIG_KEXEC).

This implementation supports re-boots of kernels with either PSCI or spin-table enable methods, but with some limitations on the match of 1st and 2nd stage kernels. The known limitations are checked in the kexec_compat_check() routine, which is called during a kexec_load syscall. If any limitations are reached an error is returned by the kexec_load syscall. Many of the limitations can be removed with some enhancment to the CPU shutdown management code in machine_kexec.c.

Quoted 4 lines

I think if we cannot offline a CPU through the generic hotplug infrastructure then we should fail to kexec. If we need a way of doing hotplug without PSCI then we should sort that out [1] rather than working around brokenness with more brokenness

OK, as I mentioned in the cover message I agree with this.

Quoted 7 lines

I also don't think that kexec should care about the precise hotplug mechanism so long as it works. If the DTB passed to the new kernel describes a different mechanism than is currently in use, that's the caller's choice and I don't see any point second-guessing that -- we're already allowing them to boot an arbitrary kernel and so long as we leave the system in a sane state I don't see a good reason to deny the user from immediately destroying that.

One use case for kexec is to use linux as a bootloader. The 1st stage bootloader kernel should be able to boot any other kernel. For this to work the 1st stage kernel should do whatever it can to get the secondary cpus into a state compatible with the 2nd stage kernel. If any of the secondary cpus have a 1st stage enable method different from the 2nd stage enable method, then the 1st stage kernel should move the cpus to their 2nd stage enable method at shutdown. Also, any cpus stuck in the kernel secondary_holding_pen should to be moved to their 2nd stage enable method. I have not tried to implement this, but it seems to me that it can be done.

Do you see any reason why this would not work?

Quoted 5 lines

[1] For hotplug without PSCI I think we can get away with adding an optional property to spin-table describing a (physical) address to branch to which returns CPUs to their original spin code. So long as the kernel zeroes the release address first we should be able to boot a CPU exactly as we managed to the first time.

I think just this would be just the address of the cpu's spin code, with the restriction that the code is entered at this address also. If there is no code there, in the case of a PSCI to spin-table re-boot, then the 1st stage kernel needs to install some spin code. Also, shouldn't this be a required property to avoid the spin code memory leakage problem?

Quoted 4 lines

For spin-table we'll also need to jump back up to EL2 when EL2 is present. It should be possible to do that within the spin-table code if we update the initial EL2 vector and get KVM to tear itself down before cpu_die happens.

OK, I have not yet considered EL2.

Quoted 11 lines

arch/arm64/kernel/machine_kexec.c line 40

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c new file +6 -0

+/**
+ * struct kexec_cpu_info_spin - Info needed for the "spin table" enable method.
+ */
+
+struct kexec_cpu_info_spin {
+       phys_addr_t phy_release_addr; /* cpu order */

I assume you mean this is in the endianness of the current kernel? I was initially confused by the comment, and I think it might be better to drop it -- unless told that a variable is in a specific endianness I would assume that it's the current native endianness anyway.

The value is read as LE from the device tree, and this comment is to clarify that the conversion from LE to cpu has been done. Maybe 'cpu byte order' is less confusing? Normally, I think the term 'machine order' would be used, but I chose 'cpu' from the name of the le64_to_cpu routine.

Quoted 9 lines

arch/arm64/kernel/machine_kexec.c line 78

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c new file +6 -0

+struct kexec_ctx {
+       struct kexec_dt_info dt_1st;
+       struct kexec_dt_info dt_2nd;
+};
+
+static struct kexec_ctx *ctx;

Is there any reason this should be dynamically allocated?

Quoted 1 line

Do we even need the current DTB if we rely on hotplug?

I think so, for this implementation we need to at least check if the enable methods and cpu counts of the two DTs match and fail the kexec-load syscall if they do not.

Quoted 16 lines

arch/arm64/kernel/machine_kexec.c line 105

Quoted 10 lines

arch/arm64/kernel/machine_kexec.c new file +9 -0

+/**
+ * kexec_is_dtb_user - Helper routine to check the device tree header signature.
+ */
+
+static int kexec_is_dtb_user(const void *dtb)
+{
+       __be32 magic;
+
+       get_user(magic, (__be32 *)dtb);

Return value check? Unless we know this can't fail?

If it can fail, surely we should return an appropriate error and forward it to userspace. EFAULT?

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+       return kexec_is_dtb(magic);

And EINVAL if we can read this but it's not a DTB?

These kexec_is_dtb are just used to search for the DTB segment, so are expected to return false for non-DTB segments. I'll change the return type to bool to make the usage more clear.

Quoted 12 lines

Quoted 12 lines

arch/arm64/kernel/machine_kexec.c new file +11 -0

+/**
+ * kexec_read_memreserve - Initialize memreserve info from a dtb.
+ */
+
+static int kexec_read_memreserve(const void *dtb, struct kexec_dt_info *info)
+{
+       const struct boot_param_header *h = dtb;
+       struct pair {
+               __be64 phy_addr;
+               __be64 size;
+       } const *pair;

Quoted 8 lines

arch/arm64/kernel/machine_kexec.c line 171

Quoted 5 lines

arch/arm64/kernel/machine_kexec.c new file +5 -0

+
+       pair = dtb + be32_to_cpu(h->off_mem_rsvmap);
+
+       if ((pair + 1)->size)
+               pr_warn("kexec: Multiple memreserve regions found.");

Huh? Multiple arbitrary memreserves are entirely valid. Why should we warn in that case?

If a user reports a problem I thought this comment may be useful in debugging since the current implementation does not consider them.

Quoted 9 lines

arch/arm64/kernel/machine_kexec.c line 174

Quoted 4 lines

arch/arm64/kernel/machine_kexec.c new file +3 -0

+
+       info->phy_memreserve_addr = be64_to_cpu(pair->phy_addr);
+       info->memreserve_size = be64_to_cpu(pair->size);

So we're assuming that the memory described in an arbitrary memreserve entry (which is intended to describe memory which shouldn't be touched unless we know what we're doing) is for our arbitrary use!?

Quoted 6 lines

NAK.

We shouldn't need to special-case reserved memory handling if we rely on cpu hotplug. If we don't then the only functional option is to add a memreserve, but that will end up leaking a small amount of memory upon every kexec. I believe that the former is the only sane option.

I think the solution is to have the cpu spin code address property.

Quoted 20 lines

Quoted 7 lines

arch/arm64/kernel/machine_kexec.c new file +6 -0

+static int kexec_setup_cpu_spin(const struct device_node *dn,
+       struct kexec_cpu_info_spin *info)
+{
+       int result;
+       u64 t1;
+

arch/arm64/kernel/machine_kexec.c line 204

Quoted 10 lines

arch/arm64/kernel/machine_kexec.c new file +10 -0

+       memset(info, 0, sizeof(*info));
+
+       result = of_property_read_u64(dn, "cpu-release-addr", &t1);
+
+       if (result) {
+               pr_warn("kexec: Read cpu-release-addr failed.\n");
+               return result;
+       }
+
+       info->phy_release_addr = le64_to_cpu(t1);

Why are we calling le64_to_cpu here?

Quoted 2 lines

of_property_read_u64 reads a be64 value from dt into cpu endianness, so at the very least the annotation is the wrong way around.

I'll check it again. I read this and thought the conversion was needed:

The value will be written as a single 64-bit little-endian value, so CPUs must convert the read value to their native endianness before jumping to it.

Quoted 3 lines

Have you tested this with a BE kernel? We should ensure that LE->LE, LE->BE, BE->BE, BE->LE all work.

Not yet. I'm in the process of setting up a BE test environment.

Quoted 14 lines

arch/arm64/kernel/machine_kexec.c line 225

Quoted 11 lines

arch/arm64/kernel/machine_kexec.c new file +10 -0

+int kexec_cpu_info_init(const struct device_node *dn,
+       struct kexec_dt_info *info)
+{
+       int result;
+       unsigned int cpu;
+       const struct device_node *i;
+
+       info->cpu_info = kmalloc(
+               (1 + info->cpu_count) * sizeof(struct kexec_cpu_info),
+               GFP_KERNEL);

Why one more than the cpu count? I thought cpu_count covered all the CPUs in the dtb?

Yes, a left over from when the array was zero terminated. Thanks for such a detailed check!

Quoted 11 lines

[...]

Quoted 9 lines

arch/arm64/kernel/machine_kexec.c new file +9 -0

+int kexec_dt_info_init(void *dtb, struct kexec_dt_info *info)
+{
+       int result;
+       struct device_node *i;
+       struct device_node *dn;
+
+       if (!dtb) {
+               /* 1st stage. */
+               dn = NULL;

Quoted 8 lines

arch/arm64/kernel/machine_kexec.c line 296

Quoted 4 lines

arch/arm64/kernel/machine_kexec.c new file +4 -0

+       } else {
+               /* 2nd stage. */
+
+               of_fdt_unflatten_tree(dtb, &dn);

This may fail. We should check that dn is not NULL before we try to use it later -- many of_* functions will traverse the current kernel's boot DT if provided with a NULL root.

OK, I'll fix it.

Quoted 6 lines

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c new file +6 -0

+
+               result = kexec_read_memreserve(dtb, info);
+
+               if (result)
+                       return result;
+       }

Quoted 8 lines

arch/arm64/kernel/machine_kexec.c line 308

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c new file +6 -0

+
+       /*
+        * We may need to work with offline cpus to get them into the correct
+        * state for a given enable method to work, and so need an info_array
+        * that has info about all the platform cpus.
+        */

What exactly do we need to do to offline CPUs?

As mentioned above, to get them into a state expected by the 2nd stage kernel if we choose to do so, but to do the compatibility checks for this implementation. Maybe I'll change the wording of this comment.

MAINTAINERS F: include/linux/kexec.h · line 5154

Quoted 8 lines · Geoff Levand

MAINTAINERS +8 -0

+KEXEC FOR ARM64
+M:     Geoff Levand <[email protected]>
+W:     http://kernel.org/pub/linux/utils/kernel/kexec/
+L:     [email protected]
+L:     [email protected] (moderated for non-subscribers)
+S:     Maintained
+F:     arch/arm64/machine_kexec.c
+F:     arch/arm64/relocate_kernel.S

These entries missed the full directory name.

Anyway, this code already comes under the core arm64 MAINTAINERS entry and it doesn't make sense to have a special kexec case. Please add a proper header to the new files you introduce, including copyright and author information.

arch/arm64/kernel/machine_kexec.c new file +27 -0

+
+/**
+ * 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.  The cpu info array is zero terminated.
+ */
+
+int kexec_cpu_info_init(const struct device_node *dn,
+	struct kexec_dt_info *info)
+{
+	int result;
+	unsigned int cpu;
+	const struct device_node *i;
+
+	info->cpu_info = kmalloc(
+		(1 + info->cpu_count) * sizeof(struct kexec_cpu_info),
+		GFP_KERNEL);
+
+	if (!info->cpu_info) {
+		pr_debug("%s: out of memory", __func__);
+		return -ENOMEM;
+	}
+
+	info->spinner_count = 0;
+
+	for (cpu = 0, i = dn; cpu < info->cpu_count; cpu++) {

[snip]

Quoted 31 lines

arch/arm64/kernel/machine_kexec.c new file +31 -0

+
+/**
+ * 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.  The cpu info array is zero terminated.
+ */
+
+int kexec_cpu_info_init(const struct device_node *dn,
+	struct kexec_dt_info *info)
+{
+	int result;
+	unsigned int cpu;
+	const struct device_node *i;
+
+	info->cpu_info = kmalloc(
+		(1 + info->cpu_count) * sizeof(struct kexec_cpu_info),
+		GFP_KERNEL);
+
+	if (!info->cpu_info) {
+		pr_debug("%s: out of memory", __func__);
+		return -ENOMEM;
+	}
+
+	info->spinner_count = 0;
+
+	for (cpu = 0, i = dn; cpu < info->cpu_count; cpu++) {
+		struct kexec_cpu_info *cpu_info = &info->cpu_info[cpu];
+
+		i = of_find_node_by_type((struct device_node *)i, "cpu");
+

arch/arm64/kernel/machine_kexec.c line 244

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c new file +6 -0

+		BUG_ON(!i);
+
+		cpu_info->cpu = cpu;
+
+		result = cpu_read_ops((struct device_node *)i, cpu,
+			&cpu_info->cpu_ops);

cpu_ops memory is not allocated?

BTW cpu_read_ops will call cpu_get_ops which is marked as __init

Thanks
Dave

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Hi Maintainers,

This patchset adds support for kexec re-boots on arm64. I have tested with the VE fast model using various kernel config options with both spin and psci enable methods. I'll continue to test in the coming weeks.

I tried to re-use the existing hot plug cpu_ops support for the secondary CPU shutdown as much as possible, but needed to do some things specific to kexec that I couldn't do with what was already there. A significant change is in [PATCH 4/8] (arm64: Add smp_spin_table_set_die) where I add the ability to setup a custom cpu_die handler.

To get the the spin-table secondary CPUs into the proper state described in Documentation/arm64/booting.txt I use a three step spin loop. First in the kernel's virtual address space, then to the identity mapped address, then jump to the final spin code in the 2nd stage kernel's /memreserve/ area. To support this three step spin I needed [PATCH 5/8] (arm64: Split soft_restart into two stages). Please see the patch comments for more info. If we added the 2nd stage kernel's /memreserve/ area to the identity map we could eliminate the middle step and go from the VA space to the /memreserve/ area directly.

Please consider all patches for inclusion. Any comments or suggestions on how to improve would be very welcome.

To load a kexec kernel and execute a kexec re-boot on arm64 my patches to kexec-tools, which have not yet been merged upstream, are needed:

https://git.linaro.org/people/geoff.levand/kexec-tools.git

As I've covered in my reply to patch 7 [1] I don't think this is a good approach. I think a vastly better approach is to make kexec depend on cpu hotplug support in SMP, and enable a simple hotplug-capable boot protocol (e.g. extend spin-table with a cpu-return-addr).

That way the in-kernel portions of kexec can use the existing infrastructure without tonnes of point hacks, and we enable a generic hotplug capable mechanism for those systems which cannot implement PSCI.

Quoted 1 line

https://git.linaro.org/people/geoff.levand/kexec-tools.git

Is the master branch up-to-date? The commit dates on all branches I can see imply they haven't been updated in a while, and the code looks like it needs some cleanup (there are some unused functions, hard-coded values, etc).