Re: [PATCH] elf loader support for auxvec base platform string

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 17 Jul 2008 16:35:39 +1000 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> Hi Linus, Andrew !
> 
> Should I seek somebody's ack before merging a patch like the one below ?

I think it's good to do so.

> I'm a bit reluctant to merge via the powerpc.git tree some changes to
> generic files without at least an ack from somebody else :-)

It tends to happen.  People often don't notice unless it a) crashes or
b) spits warnings or c) screws up my tree or d) all the above plus
more.

> There have been some debate on whether this AT_BASE_PLATFORM is the
> right approach, though I haven't seen them reach any useful conclusion
> and our toolchain people internally are screaming for it...
> 
> Cheers,
> Ben.
> 
> On Tue, 2008-07-15 at 18:58 -0500, Nathan Lynch wrote:
> > Some IBM POWER-based platforms have the ability to run in a
> > mode which mostly appears to the OS as a different processor from the
> > actual hardware.  For example, a Power6 system may appear to be a
> > Power5+, which makes the AT_PLATFORM value "power5+".  This means that
> > programs are restricted to the ISA supported by Power5+;
> > Power6-specific instructions are treated as illegal.
> > 
> > However, some applications (virtual machines, optimized libraries) can
> > benefit from knowledge of the underlying CPU model.  A new aux vector
> > entry, AT_BASE_PLATFORM, will denote the actual hardware.  For
> > example, on a Power6 system in Power5+ compatibility mode, AT_PLATFORM
> > will be "power5+" and AT_BASE_PLATFORM will be "power6".  The idea is
> > that AT_PLATFORM indicates the instruction set supported, while
> > AT_BASE_PLATFORM indicates the underlying microarchitecture.
> > 
> > If the architecture has defined ELF_BASE_PLATFORM, copy that value to
> > the user stack in the same manner as ELF_PLATFORM.
> > 
> > Signed-off-by: Nathan Lynch <ntl@pobox.com>
> > ---
> >  fs/binfmt_elf.c        |   23 +++++++++++++++++++++++
> >  include/linux/auxvec.h |    5 ++++-
> >  2 files changed, 27 insertions(+), 1 deletions(-)
> > 
> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > index d48ff5f..834c2c4 100644
> > --- a/fs/binfmt_elf.c
> > +++ b/fs/binfmt_elf.c
> > @@ -131,6 +131,10 @@ static int padzero(unsigned long elf_bss)
> >  #define STACK_ALLOC(sp, len) ({ sp -= len ; sp; })
> >  #endif
> >  
> > +#ifndef ELF_BASE_PLATFORM
> > +#define ELF_BASE_PLATFORM NULL
> > +#endif

Please add a comment which explains what this is.

Please also add a comment telling the world in which header file the
architecture *must* define this macro and then ensure that that header is
included into this file by reliable means.  asm/elf.h looks OK.

> >  static int
> >  create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
> >  		unsigned long load_addr, unsigned long interp_load_addr)
> > @@ -142,7 +146,9 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
> >  	elf_addr_t __user *envp;
> >  	elf_addr_t __user *sp;
> >  	elf_addr_t __user *u_platform;
> > +	elf_addr_t __user *u_base_platform;
> >  	const char *k_platform = ELF_PLATFORM;
> > +	const char *k_base_platform = ELF_BASE_PLATFORM;
> >  	int items;
> >  	elf_addr_t *elf_info;
> >  	int ei_index = 0;
> > @@ -172,6 +178,19 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
> >  			return -EFAULT;
> >  	}
> >  
> > +	/*
> > +	 * If this architecture has a "base" platform capability
> > +	 * string, copy it to userspace.
> > +	 */
> > +	u_base_platform = NULL;
> > +	if (k_base_platform) {
> > +		size_t len = strlen(k_base_platform) + 1;
> > +
> > +		u_base_platform = (elf_addr_t __user *)STACK_ALLOC(p, len);
> > +		if (__copy_to_user(u_base_platform, k_base_platform, len))
> > +			return -EFAULT;
> > +	}

>From my reading, this change will result in no additional code
generation on non-powerpc architectures.  This is good.  If poss, could
you please verify that theory and perhaps drop a note in the changelog
about that?


Apart from that - acked-by-me