[PATCH v1 3/4] arm64: Add arm64 kexec support

On Wed, Jul 20, 2016 at 12:19:21PM -0700, Geoff Levand wrote:

quoted

quoted

+static uint64_t read_sink(const char *command_line)
+{
+> > 	> > uint64_t v;
+> > 	> > const char *p;
+
+> > 	> > if (arm64_opts.port)
+> > 	> > 	> > return arm64_opts.port;
+
+#if defined(ARM64_DEBUG_PORT)
+> > 	> > return (uint64_t)(ARM64_DEBUG_PORT);
+#endif
+> > 	> > if (!command_line)
+> > 	> > 	> > return 0;
+
+> > 	> > if (!(p = strstr(command_line, "earlyprintk=")) &&
+> > 	> > 	> > !(p = strstr(command_line, "earlycon=")))
+> > 	> > 	> > return 0;
+
+> > 	> > while (*p != ',')
+> > 	> > 	> > p++;
+
+> > 	> > p++;
+
+> > 	> > while (isspace(*p))
+> > 	> > 	> > p++;

Why do we skip spaces? As far as I am aware, there should not be any
spaces in the option.

Sure, I can remove it.

quoted

quoted

+
+> > 	> > if (*p == 0)
+> > 	> > 	> > return 0;
+
+> > 	> > errno = 0;
+
+> > 	> > v = strtoull(p, NULL, 0);
+
+> > 	> > if (errno)
+> > 	> > 	> > return 0;
+
+> > 	> > return v;
+}

It looks like the purgatory code expects angel SWI as the earlycon,

Maybe you saw the debug_brk macro in entry.S?  I should remove
that and just loop.

Ah, sorry. For some reason I got that confused with the sink code. My
bad.

Now I see that's assuming an 8-bit MMIO register.

quoted

whereas many other earlycons exist (with pl011 being extremely popular).
Regardless, if we assume a particular UART type, we should explicitly
verify that here. Otherwise the purgatory code will likely bring down
the system, and it will be very painful to debug.

Please explicitly check for the supported earlycon name.

Purgatory just writes bytes to the address given.  Are there
UARTs that don't have TX as the first port?

I'm not sure, but it's certainly possible. The generic earlycon binding
doesn't guarantee that the first address is a TX register. Even if they
don't exist today, they could in a month's time, so I don't think we
should assume anything.

Additionally, the width of that TX register can differ (e.g.
uart8250,mmio vs uart8250,mmio32), and some UARTs aren't very forgiving
if accessed with the wrong width.

To be safe, we could do a check when we get the address from
an earlycon parameter. 

Yup. I think you need a whitelist of UARTs that can be handled, along
with parsing for their options (e.g. mmio vs mmio32), giving up if
unknown options are spotted.

Here's what I found in the dts'.  The
first three are OK, but I don't know about the others.  

I believe you can find the full set with:

$ git grep EARLYCON_DECLARE

[...]

quoted

quoted

+> > 	> > if (result)
+> > 	> > 	> > fprintf(stderr, "kexec: Warning: No device tree available.\n");

There are other reasons we'd return an error (e.g. mismatched enable
methods), so this is somewhat misleading.

I believe that in all cases we log the specific reason first anyway, so
perhaps it's best to jsut remove this warning.

Yes, this could be removed.

quoted

Won't this also be very noisy in the case of ACPI with a stub DTB? In
that case ther are no cpu nodes, and may be no memory nodes.

Should we just remove check_cpu_nodes and everything associated with
it?  It is a lot of code, and all it does now is issue warnings.
It is still around from the early days of spin_table support.  

That sounds fine to me.

As for memory nodes, we currently look at the dt, then fall back
to iomem. We could switch the order, iomem then dt, but then
those just issue dbgprintf's.

Sure.

quoted

quoted

+int arm64_process_image_header(const struct arm64_image_header *h)
+{
+#if !defined(KERNEL_IMAGE_SIZE)
+# define KERNEL_IMAGE_SIZE (768 * 1024)
+#endif
+
+> > 	> > if (!arm64_header_check_magic(h))
+> > 	> > 	> > return -EINVAL;
+
+> > 	> > if (h->image_size) {
+> > 	> > 	> > arm64_mem.text_offset = le64_to_cpu(h->text_offset);
+> > 	> > 	> > arm64_mem.image_size = le64_to_cpu(h->image_size);
+> > 	> > } else {
+> > 	> > 	> > /* For 3.16 and older kernels. */
+> > 	> > 	> > arm64_mem.text_offset = 0x80000;
+> > 	> > 	> > arm64_mem.image_size = KERNEL_IMAGE_SIZE;
+> > 	> > }
+
+> > 	> > return 0;
+}

A v3.16 defconfig Image with the Linaro 14.09 GCC 4.9 toolchain is
6.3MB, so the chosen value for KERNEL_IMAGE_SIZE is far too small. I'm
not sure what to suggest as a better value, however, as I know that some
configurations are far bigger than that.

OK, I'll make it bigger, say 7.  When I set this up I expected
the distro maintainer to choose KERNEL_IMAGE_SIZE to match their
needs.

To give some headroom, bumping to 16 or so is probably a safer bet.

Perhaps it's worth logging a warning that we're guessing the effective
image size in this case? That could avoid a lot of head-scratching if
things do end up overlapping.

quoted

Do we expect to kexec to a v3.16 or earlier kernel, given we need a much
newer first kernel to have kexec in the first place? We could mandate
having a header with a non-zero image_size (i.e. the target kernel has
to be v3.16 or newer).

Kexec could be installed as a bootloader, and users may want the
ability to boot older installations, so I think it worthwile to
have.

Sure. I was under the impression that most distros had chosen v3.16 or
later, but I have no problem with trying to support earlier kernels.

Thanks,
Mark.