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

On Thu, 17 Jul 2008 16:35:39 +1000 Benjamin Herrenschmidt [off-list ref] 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:

quoted

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 <redacted>
---
 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.

quoted

 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