Re: [PATCH v5 1/5] vmcore: Introduce ELF header in new memory feature

[Michael Holzheu <holzheu@linux.vnet.ibm.com>]

On Mon, 1 Jul 2013 13:37:27 -0400
Vivek Goyal <vgoyal@redhat.com> wrote:

> On Fri, Jun 28, 2013 at 10:15:52AM +0200, Michael Holzheu wrote:
> > On Thu, 27 Jun 2013 16:23:34 -0400
> > Vivek Goyal <vgoyal@redhat.com> wrote:
> > 
> > > On Thu, Jun 27, 2013 at 03:32:02PM -0400, Vivek Goyal wrote:
> > > > On Fri, Jun 21, 2013 at 04:17:03PM +0200, Michael Holzheu wrote:
> > > > > On Fri, 14 Jun 2013 14:54:02 -0400
> > > > > Vivek Goyal <vgoyal@redhat.com> wrote:
> > 
> > [snip]
> > 
> > > Thinking more about it, I think let us cleanup with this little ugly
> > > bit too so that future changes become easy.
> > > 
> > > Current convention is that elfcorehdr_addr and elfcorehdr_size are
> > > already set by arch code by the time vmcore.c starts reading it. Can't
> > > s390 allocate elf headers in early boot code and elfcorehdr_addr? Then
> > > we don't have to call elfcorehdr_alloc().
> > > 
> > > And once we are done with reading headers, we can call elfcorehdr_free()
> > > and s390 could free memory and set elfcorehdr_addr to ELFCORE_ADDR_ERR
> > > and elfcorehdr_size=0. That would signify that one can not try to read
> > > elf headers now and it must have been freed.
> > > 
> > > is_kdump_kernel() will continue to work as elfcorehdr_addr is
> > > ELFCORE_ADDR_ERR. And that will mean that either elfcorehdr were not
> > > readable/usable to begin with or they have been freed now.
> > 
> > Hello Vivek,
> > 
> > We would like to keep the alloc/free symmetry as you have suggested in a
> > previous mail. This also has the advantage that we do not have to rely
> > on the ordering of init calls.
> > 
> > Wouldn't it be sufficient to just set elfcorehdr_addr to ELFCORE_ADDR_ERR
> > after elfcorehdr_free() and remove the comment?
> > 
> 
> Hi Michael,
> 
> This has only one problem and that is what's the initialization semantics
> of elfcorehdr_addr. 
> 
> So far we expected it to be initialized very early in boot process and
> once this is set, any component in the system could figure out if it
> is kdump kernel (is_kdump_kernel()) and do kdump specific things.
> 
> But if we move initialization of this variable little late, then it
> might be a problem for early users of is_kdump_kernel(). Though right
> now I don't see drivers making use of it and only arch specific early
> boot up code seems to have it.
> 
> So either we can stick to existing semantics of initializing headers
> early or we could create a separate variable for is_kdump_kernel() which
> is set in early boot and then one can delay initialization of
> elfcorehdr_addr() in vmcore_init().

The later makes much sense to me. This would also make the s390 code
easier to read since we then could exchange some early kdump checks
with the official is_kdump() function.

Perhaps we can do that as an additional patch after we are ready with
this patch series.

> 
> Given the fact that I don't see any users of is_kdump_kernel() in arch
> independent directory, and I am assuming that you will tackle all early
> users of is_kdump_kernel() in s390, I will be fine even with your patch
> below.

Ok great, so I will send you the current patch set version 6 with all the
discussed changes.

Thanks!
Michael