On Thu, Jan 12, 2017 at 03:39:45PM +0000, Mark Rutland wrote:
> Hi,
>
> On Wed, Dec 28, 2016 at 01:37:34PM +0900, AKASHI Takahiro wrote:
> > From: James Morse <james.morse at arm.com>
> >
> > Add documentation for
> > linux,crashkernel-base and crashkernel-size,
> > linux,usable-memory-range
> > linux,elfcorehdr
> > used by arm64 kdump to decribe the kdump reserved area, and
> > the elfcorehdr's location within it.
> >
> > Signed-off-by: James Morse <james.morse at arm.com>
> > [takahiro.akashi at linaro.org: added "linux,crashkernel-base" and "-size" ]
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > Cc: devicetree at vger.kernel.org
> > Cc: Rob Herring <robh+dt at kernel.org>
> > Cc: Mark Rutland <mark.rutland at arm.com>
> > ---
> > Documentation/devicetree/bindings/chosen.txt | 50 ++++++++++++++++++++++++++++
> > 1 file changed, 50 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> > index 6ae9d82d4c37..7b115165e9ec 100644
> > --- a/Documentation/devicetree/bindings/chosen.txt
> > +++ b/Documentation/devicetree/bindings/chosen.txt
> > @@ -52,3 +52,53 @@ This property is set (currently only on PowerPC, and only needed on
> > book3e) by some versions of kexec-tools to tell the new kernel that it
> > is being booted by kexec, as the booting environment may differ (e.g.
> > a different secondary CPU release mechanism)
> > +
> > +linux,crashkernel-base
> > +linux,crashkernel-size
> > +----------------------
> > +
> > +These properties (currently used on PowerPC and arm64) indicates
> > +the base address and the size, respectively, of the reserved memory
> > +range for crash dump kernel.
>
> From this description, it's not clear to me what the (expected)
> consumers of this property are, nor what is expected to provide it.
>
> In previous rounds of review, I had assumed that this was used to
> describe a preference to the first kernel as to what region of memory
> should be used for a subsequent kdump kernel. Looking around, I'm not
> sure if I was correct in that assessment.
>
> I see that arch/powerpc seems to consume this property to configure
> crashk_res, but it also rewrites it based on crashk_res, presumably for
> the benefit of userspace. It's not clear to me how on powerpc the kdump
> kernel knows its memory range -- is more DT modification done in the
> kernel and/or userspace?
I don't believe that powerpc will rewrite the property any way.
As far as I know from *the source code*, powerpc kernel retrieves
the memory range for crash dump kernel from a kernel command line, i.e.
crashkernel=, and then exposes it through DT to userspace (assuming
kexec-tools).
> I disagree with modifying this property to expose it to userspace. For
Apart from the context of discussions, is this a shared consensus?
> arm64 we should either ensure that /proc/iomem is consistently usable
> (and have userspace consistently use it), or we should expose a new file
> specifically to expose this information.
The thing that I had in my mind when adding this property is that
/proc/iomem would be obsolete in the future, then we should have
an alternative in hand.
> Further, I do not think we need this property. It makes more sense to me
> for the preference of a a region to be described to the *first* kernel
> using the command line consistently.
>
> So I think we should drop this property, and not use it on arm64. Please
> document this as powerpc only.
OK, but if we drop the property from arm64 code, we have no reason
to leave its description in this patch.
(In fact, there are a few more (undocumented) properties that only ppc
uses for kdump.)
> > +e.g.
> > +
> > +/ {
> > + chosen {
> > + linux,crashkernel-base = <0x9 0xf0000000>;
> > + linux,crashkernel-size = <0x0 0x10000000>;
> > + };
> > +};
>
> > +
> > +linux,usable-memory-range
> > +-------------------------
> > +
> > +This property (currently used only on arm64) holds the memory range,
> > +the base address and the size, which can be used as system ram on
> > +the *current* kernel. Note that, if this property is present, any memory
> > +regions under "memory" nodes in DT blob or ones marked as "conventional
> > +memory" in EFI memory map should be ignored.
>
> Could you please replace this with:
>
> This property (arm64 only) holds a base address and size, describing a
> limited region in which memory may be considered available for use by
> the kernel. Memory outside of this range is not available for use.
>
> This property describes a limitation: memory within this range is only
> valid when also described through another mechanism that the kernel
> would otherwise use to determine available memory (e.g. memory nodes
> or the EFI memory map). Valid memory may be sparse within the range.
Sure.
Thanks,
-Takahiro AKASHI
> To clarify why we need this, given by above comments w.r.r. the
> linux,crashkernel-* properties:
>
> * It preserves all the original memory map information (e.g. memory
> nodes and/or EFI memory map)
>
> * It works consistently, regardless of how the kdump kernel would
> otherwise determine which memory to use (memory nodes, EFI, etc).
>
> * It will be simply and reliable for an in-kernel purgatory to insert,
> if we need a kexec_file_load()-based kdump (e.g. without requiring
> memory map rewrites, and avoiding clashes with command line
> parameters). For a first kernel, this is not as big a concern.
>
> > +linux,elfcorehdr
> > +----------------
> > +
> > +This property (currently used only on arm64) holds the memory range,
> > +the address and the size, of the elf core header which mainly describes
> > +the panicked kernel's memory layout as PT_LOAD segments of elf format.
> > +e.g.
> > +
> > +/ {
> > + chosen {
> > + linux,elfcorehdr = <0x9 0xfffff000 0x0 0x800>;
> > + };
> > +};
>
> This property looks fine to me.
>
> Thanks,
> Mark.
