Re: [PATCH v29 9/9] Documentation: dt: chosen properties for arm64 kdump

[AKASHI Takahiro <takahiro.akashi-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On Fri, Jan 13, 2017 at 11:17:56AM +0000, Mark Rutland wrote:
> On Fri, Jan 13, 2017 at 06:13:49PM +0900, AKASHI Takahiro wrote:
> > On Thu, Jan 12, 2017 at 03:39:45PM +0000, Mark Rutland wrote:
> > > On Wed, Dec 28, 2016 at 01:37:34PM +0900, AKASHI Takahiro wrote:
> > > > +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).
> 
> The rewriting I describe is in export_crashk_values() in
> arch/powerpc/kernel/machine_kexec.c, where the code deletes existing the
> properties, and adds new ones, to the DT exposed to userspace.
> 
> So I think we're just quibbling over the definition of "rewrite".

Gotcha

> > > 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.
> 
> Ok.
> 
> My disagreement is with using the DT as a channel to convey information
> from the kernel to userspace.
> 
> I'm more than happy for a new file or other mechanism to express this
> information. For example, we could add
> /sys/kernel/kexec_crash_{base,size} or similar.

It may make sense because /sys/kernel/kexec_crash_size already exists,
so why not kexec_crash_base?
My concern, however, is that this kind of interface might prevent us from
allowing multiple regions to be reserved for crash dump kernel in the future.
(There is an assumption that we have only one region at least on arm64 though.)

Thanks,
-Takahiro AKASHI

> 
> > > 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.)
> 
> I'm happy to drop it, then.
> 
> > > > +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.
> 
> Cheers!
> 
> Thanks,
> Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html