Date: Thu, 23 May 2013 13:41:27 -0400
From: Vivek Goyal <vgoyal@redhat.com>
To: Michael Holzheu <holzheu@linux.vnet.ibm.com>
Cc: Jan Willeke <willeke@de.ibm.com>,
        Martin Schwidefsky <schwidefsky@de.ibm.com>,
        Heiko Carstens <heiko.carstens@de.ibm.com>,
        linux-kernel@vger.kernel.org, kexec@lists.infradead.org
Subject: Re: [PATCH v2 1/3] kdump: Introduce ELF header in new memory feature
Message-ID: <20130523174127.GA12290@redhat.com>
References: <1369245517-16204-1-git-send-email-holzheu@linux.vnet.ibm.com>
 <1369245517-16204-2-git-send-email-holzheu@linux.vnet.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1369245517-16204-2-git-send-email-holzheu@linux.vnet.ibm.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4306
Lines: 117

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@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/