All of lore.kernel.org
 help / color / mirror / Atom feed
From: will.deacon@arm.com (Will Deacon)
To: linux-arm-kernel@lists.infradead.org
Subject: [RESEND PATCH] ARM: kdump: Add vmcore_elf64_check_arch
Date: Mon, 16 Jun 2014 14:38:49 +0100	[thread overview]
Message-ID: <20140616133849.GG16758@arm.com> (raw)
In-Reply-To: <5395B4D0.6040302@huawei.com>

On Mon, Jun 09, 2014 at 02:21:20PM +0100, Liu hua wrote:
> ? 2014/5/6 20:15, Will Deacon ??:
> > On Sat, May 03, 2014 at 02:44:46PM +0100, Liu Hua wrote:
> Sorry to reply you so late. These days I am working on kdump feature for LPAE enabled
> kernel. And now I think this patch is not good.
> 
> > This function looks identical to elf_check_arch. Why do we need to duplicate
> > that code? You could use some pre-processor magic to make the core part of
> > the functions agnostic to header type.
> 
> At the begging I think I should add elf_check_arch_64 just as elf_check_arch to
> do complicated check.
> 
> But for ARM32, elf_check_arch_64 would not be used except for kdump. No programs
> with elf64 header can be loaded into 32bit ARM kernel. So I think I can just check
> the "e_machine", just as what other platform does. I think other checks is useless
> for kdump.
> 
> > In fact, if elf_check_arch could handle both header types then the generic
> > definition of vmcore_elf64_check_arch in include/linux/crash_dump.h would
> > work out of the box.
> 
> As I mentioned, I afraid this will make the code hard to understand.( Sorry
> for my former incorrect patch).
> 
> How about this patch?
> 
> diff --git a/arch/arm/include/asm/elf.h b/arch/arm/include/asm/elf.h
> index f4b46d3..9424542 100644
> --- a/arch/arm/include/asm/elf.h
> +++ b/arch/arm/include/asm/elf.h
> @@ -97,7 +97,7 @@ struct elf32_hdr;
>  extern int elf_check_arch(const struct elf32_hdr *);
>  #define elf_check_arch elf_check_arch
> 
> -#define vmcore_elf64_check_arch(x) (0)
> +#define vmcore_elf64_check_arch(x) ((x)->e_machine == EM_ARM)

Is there no way we can get away with using the generic version here? What
goes wrong if you let vmcore_elf64_check_arch fallback to elf_check_arch?

Will

WARNING: multiple messages have this Message-ID (diff)
From: Will Deacon <will.deacon@arm.com>
To: Liu hua <sdu.liu@huawei.com>
Cc: "linux@arm.linux.org.uk" <linux@arm.linux.org.uk>,
	"wangnan0@huawei.com" <wangnan0@huawei.com>,
	"peifeiyue@huawei.com" <peifeiyue@huawei.com>,
	Catalin Marinas <Catalin.Marinas@arm.com>,
	"mika.westerberg@iki.fi" <mika.westerberg@iki.fi>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"alonid@stratoscale.com" <alonid@stratoscale.com>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [RESEND PATCH] ARM: kdump: Add vmcore_elf64_check_arch
Date: Mon, 16 Jun 2014 14:38:49 +0100	[thread overview]
Message-ID: <20140616133849.GG16758@arm.com> (raw)
In-Reply-To: <5395B4D0.6040302@huawei.com>

On Mon, Jun 09, 2014 at 02:21:20PM +0100, Liu hua wrote:
> 于 2014/5/6 20:15, Will Deacon 写道:
> > On Sat, May 03, 2014 at 02:44:46PM +0100, Liu Hua wrote:
> Sorry to reply you so late. These days I am working on kdump feature for LPAE enabled
> kernel. And now I think this patch is not good.
> 
> > This function looks identical to elf_check_arch. Why do we need to duplicate
> > that code? You could use some pre-processor magic to make the core part of
> > the functions agnostic to header type.
> 
> At the begging I think I should add elf_check_arch_64 just as elf_check_arch to
> do complicated check.
> 
> But for ARM32, elf_check_arch_64 would not be used except for kdump. No programs
> with elf64 header can be loaded into 32bit ARM kernel. So I think I can just check
> the "e_machine", just as what other platform does. I think other checks is useless
> for kdump.
> 
> > In fact, if elf_check_arch could handle both header types then the generic
> > definition of vmcore_elf64_check_arch in include/linux/crash_dump.h would
> > work out of the box.
> 
> As I mentioned, I afraid this will make the code hard to understand.( Sorry
> for my former incorrect patch).
> 
> How about this patch?
> 
> diff --git a/arch/arm/include/asm/elf.h b/arch/arm/include/asm/elf.h
> index f4b46d3..9424542 100644
> --- a/arch/arm/include/asm/elf.h
> +++ b/arch/arm/include/asm/elf.h
> @@ -97,7 +97,7 @@ struct elf32_hdr;
>  extern int elf_check_arch(const struct elf32_hdr *);
>  #define elf_check_arch elf_check_arch
> 
> -#define vmcore_elf64_check_arch(x) (0)
> +#define vmcore_elf64_check_arch(x) ((x)->e_machine == EM_ARM)

Is there no way we can get away with using the generic version here? What
goes wrong if you let vmcore_elf64_check_arch fallback to elf_check_arch?

Will

  reply	other threads:[~2014-06-16 13:38 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-05-03 13:44 [RESEND PATCH] ARM: kdump: Add vmcore_elf64_check_arch Liu Hua
2014-05-03 13:44 ` Liu Hua
2014-05-06 12:15 ` Will Deacon
2014-05-06 12:15   ` Will Deacon
2014-06-09 13:21   ` Liu hua
2014-06-09 13:21     ` Liu hua
2014-06-16 13:38     ` Will Deacon [this message]
2014-06-16 13:38       ` Will Deacon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140616133849.GG16758@arm.com \
    --to=will.deacon@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.