On Wed, Jul 20, 2016 at 12:19:21PM -0700, Geoff Levand wrote:
> > > +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.
>
> > > +
> > > +> > > > 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.
> > 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
[...]
> > > +> > > > 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.
>
> > 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.
> > > +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.
> > 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.
