[PATCH v2 1/3] kdump: Introduce ELF header in new memory feature

[Michael Holzheu]

Hello Vivek,

Thanks for your reply. I will integrate your feedback into the v3 patch
set and will make the following changes:

- Get rid of ELFCORE_ADDR_NEWMEM (sorry for that:)
- Make read_from_crash_header() only read from kernel
- Move read_from_crash_header() to weak function
  arch_read_from_crash_header()
- Implement read_from_crash_header() strong function for s390
- Set elfcorehdr_addr to address of new ELF header

Best Regards,
Michael

On Thu, 23 May 2013 13:41:27 -0400
Vivek Goyal <vgoyal at redhat.com> wrote:

> On Wed, May 22, 2013 at 07:58:35PM +0200, Michael Holzheu wrote:
> > or s390 we create the ELF core header in the 2nd kernel
> > with a small trick. We relocate the addresses in the ELF header in
> > a way that for the /proc/vmcore code it seems to be in the 1st
> > kernel (old) memory and the read_from_oldmem() returns the correct
> > data. This allows the /proc/vmcore code to use the ELF header in the
> > 2nd kernel.
> > 
> > This patch now exchanges the old mechanism with the new and much
> > cleaner function call override feature that now offcially allows to
> > create the ELF core header in the 2nd kernel.
> > 
> > To use the new feature the following has to be done by the
> > architecture backend code:
> > 
> >  * Set elfcorehdr_addr to ELFCORE_ADDR_NEWMEM
> >    -> is_kdump_kernel() will return true
> 
> setting elfcorehdr_addr to ELFCORE_ADDR_NEWMEM looks really odd to me.
> There is no need for arch independent code to know whether crash
> headers are in new memory or not. Our old assumption was that
> everything is in old memory. Now that is broken because of s390. So
> arch should be able to override read_from_crash_header() call and
> read headers from new memory. If need be s390 can maintain another
> variable for this state to figure out where headers are.
> 
> Also arch should be able to  set elfcorehdr_addr to virtual address
> of ELF header. is_kdump_kernel() does not care whether address stored
> in elfcorehdr_addr is physical or virtual.
> 
> IOW, generic code would not care whether headers are in new memory
> or old memory. All the operations on header will be abstracted with
> the help of helper functions.
> 
> Can we please get rid of this NEWMEM stuff.
> 
> 
> >  * Override arch_get_crash_header() to return the address of the ELF
> >    header in new memory.
> >  * Override arch_free_crash_header() to free the memory of the ELF
> >    header in new memory.
> > 
> > Signed-off-by: Michael Holzheu <holzheu at linux.vnet.ibm.com>
> > ---
> >  fs/proc/vmcore.c           | 80
> > +++++++++++++++++++++++++++++++++++-----------
> > include/linux/crash_dump.h |  3 ++ 2 files changed, 65
> > insertions(+), 18 deletions(-)
> > 
> > diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
> > index 6ba32f8..d97ed31 100644
> > --- a/fs/proc/vmcore.c
> > +++ b/fs/proc/vmcore.c
> > @@ -123,6 +123,47 @@ static ssize_t read_from_oldmem(char *buf,
> > size_t count, return read;
> >  }
> >  
> > +/*
> > + * Read from the current (new) kernel memory
> > + */
> > +static ssize_t read_from_newmem(char *buf, size_t count, u64
> > *ppos, int userbuf) +{
> > +	if (userbuf) {
> > +		if (copy_to_user(buf, (void *)*ppos, count))
> > +			return -EFAULT;
> > +	} else {
> > +		memcpy(buf, (void *)*ppos, count);
> > +	}
> > +	*ppos += count;
> > +	return count;
> > +}
> > +
> 
> Why do we need read_from_newmem? I thought arch code will override
> read_from_crash_header() and write a variant of read_from_newmem()
> internally. I think could still be an helper function in vmcore.c if
> there are many arch keeping headers in new memory so that we don't
> have same code across multiple arches. But s390 seems to be the only
> expcetion at this point of time.
> 
> > +/*
> > + * Provide access to the header
> > + */
> > +static ssize_t read_from_crash_header(char *buf, size_t count, u64
> > *ppos,
> > +				      int userbuf)
> > +{
> > +	if (elfcorehdr_addr == ELFCORE_ADDR_NEWMEM)
> > +		return read_from_newmem(buf, count, ppos, userbuf);
> > +	else
> > +		return read_from_oldmem(buf, count, ppos, userbuf);
> > +}
> > +
> 
> I thought read_from_crash_header() will simply be
> 
> read_from_crash_header() {
> 	return read_from_oldmem()
> }
> 
> And arch code will override it to read the headers from new memory.
> That way generic code does not have to know whether headers are in
> old memory or in new memory or somewhere else.
> 
> Also current usage of read_from_crash_header() seems to be that we are
> not copying header data to user space buffer directly. Generic code
> will process headers and copy them in kernel memory and it will be
> copied to user space from there if need be.
> 
> So for time being I think it should be just fine to assume that
> read_from_crash_header() is copying data to kernel memory only.
> 
> Thanks
> Vivek
>