diff mbox

[V3] vDSO for SPARC

Message ID 87inhu1ef5.fsf@esperi.org.uk
State RFC
Delegated to: David Miller
Headers show

Commit Message

Nick Alcock Aug. 11, 2017, 12:23 p.m. UTC 
On 11 Aug 2017, Nagarathnam Muthusamy verbalised:

> On 08/10/2017 03:03 PM, David Miller wrote:
>> From: Nagarathnam Muthusamy <nagarathnam.muthusamy@oracle.com>
>> Date: Thu, 10 Aug 2017 17:34:47 -0400
>>
>>> Following patch is based on work done by Nick Alcock on 64-bit vDSO for sparc
>>> in Oracle linux. I have extended it to include support for 32-bit vDSO for sparc
>>> on 64-bit kernel.
>> This doesn't build:
>>
>> davem@patience:~/src/GIT/sparc-next$ make -s -j128
>> In file included from arch/sparc/vdso/vdso2c.c:141:0:
>> arch/sparc/vdso/vdso2c.h: In function ‘go64’:
>> arch/sparc/vdso/vdso2c.h:20:14: warning: variable ‘secstrings’ set but not used [-Wunused-but-set-variable]
>>    const char *secstrings;

secstrings and secstrings_hdr can be removed, or can be used in the
following loop to make it a bit less squirrelly: up to you. (However,
see below, because this may be a sign of the underlying problem.)

>>                ^~~~~~~~~~
>> In file included from arch/sparc/vdso/vdso2c.c:145:0:
>> arch/sparc/vdso/vdso2c.h: In function ‘go32’:
>> arch/sparc/vdso/vdso2c.h:20:14: warning: variable ‘secstrings’ set but not used [-Wunused-but-set-variable]
>>    const char *secstrings;
>>                ^~~~~~~~~~
>> Error: vvar_begin must be a multiple of 8192
>> make[2]: *** [arch/sparc/vdso/vdso-image-64.c] Error 1
>> make[2]: *** Waiting for unfinished jobs....
>> Error: vvar_begin must be a multiple of 8192
>> make[2]: *** [arch/sparc/vdso/vdso-image-32.c] Error 1
>> make[1]: *** [arch/sparc/vdso] Error 2
>>
>>
>> davem@patience:~/src/GIT/sparc-next$ gcc --version
>> gcc (GCC) 6.3.0
>> Copyright (C) 2016 Free Software Foundation, Inc.
>> This is free software; see the source for copying conditions.  There is NO
>> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>
> My gcc version is bit old (4.4.7) and the build passed in my system. I will reproduce the errors locally, fix them
> and send out an updated patch.

If this is any problem, it's a linker problem, or a problem with the
code in vdso2c around required_syms which sets up sym_vvar_start to be
equal to .vvar_start, since as you see here:

+	if (syms[sym_vvar_start] % 8192)
+		fail("vvar_begin must be a multiple of 8192\n");

and here:

Comments

David Miller Aug. 15, 2017, 5:35 a.m. UTC | #1 
From: Nick Alcock <nick.alcock@oracle.com>
Date: Fri, 11 Aug 2017 13:23:26 +0100

> unless you have a really serious linker bug,

Open your mind a little bit.

Maybe due to a bug in the Makefile the linker script doesn't get
used, or the wrong one is used.

Maybe parallel builds are not handled correctly.

Narrowly focusing on the linker file directly could compromise your
ability to diagnose this :-)

I'll try to look at this myself soon, I've been too busy to do so
lately.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Sept. 10, 2017, 3:41 a.m. UTC | #2 
From: David Miller <davem@davemloft.net>
Date: Mon, 14 Aug 2017 22:35:53 -0700 (PDT)

> From: Nick Alcock <nick.alcock@oracle.com>
> Date: Fri, 11 Aug 2017 13:23:26 +0100
> 
>> unless you have a really serious linker bug,
> 
> Open your mind a little bit.

Looking at the actual details, how did this ever work for anyone?

The arch/sparc/vdso/vdso.lds file says:

vvar_start = . -(1 << 12);

And "1 << 12" is 4096, therefore a multiple of 4096 and not 8192.

So, as requested, the object built gets:

davem@patience:~/src/GIT/sparc-next$ nm -n arch/sparc/vdso/vdso64.so.dbg
fffffffffffff000 a vvar_data
fffffffffffff000 a vvar_start

So why is vdso2c requiring an 8192 byte aligned value for the address
of 'vvar_start'?

Nick, I will be honest and sat that I'm kinda irritated that I had to
track this down and the best you could come up with was blaming the
linker...  :-(
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Sept. 11, 2017, 4:03 a.m. UTC | #3 
From: David Miller <davem@davemloft.net>
Date: Sat, 09 Sep 2017 20:41:29 -0700 (PDT)

> The arch/sparc/vdso/vdso.lds file says:
> 
> vvar_start = . -(1 << 12);
> 
> And "1 << 12" is 4096, therefore a multiple of 4096 and not 8192.
> 
> So, as requested, the object built gets:
> 
> davem@patience:~/src/GIT/sparc-next$ nm -n arch/sparc/vdso/vdso64.so.dbg
> fffffffffffff000 a vvar_data
> fffffffffffff000 a vvar_start

Breaking this down further, the linker scripts are written assuming
they will be built by a host compiler that generates 64-bit target
code by default.

This is not a safe assumption.

My system generates 32-bit binaries from gcc by default, and that's
why you end up with a value of PAGE_SIZE which is 4096 propagating
through all of the generated linker scripts.

x86, where it seems like were at least used as a template for the
sparc linker scripts, gets away with this because the PAGE_SIZE
is the same on 32-bit and 64-bit.

Anyways, since 8192 is hard coded into vdso2c, just use 8192 as
PAGE_SIZE in vdso-layout.lds.S and maybe put a comment above it.

The next problem with the patch is that we get a warning because
the variable 'secstrings_hdr' is set but not used in vdso2c.h, and
compiler warnings are treated as errors under arch/sparc.

This is fixed simply by:

diff --git a/arch/sparc/vdso/vdso2c.h b/arch/sparc/vdso/vdso2c.h
index 8d1b5cf..b193e3d 100644
--- a/arch/sparc/vdso/vdso2c.h
+++ b/arch/sparc/vdso/vdso2c.h
@@ -17,9 +17,8 @@ static void BITSFUNC(go)(void *raw_addr, size_t raw_len,
 	unsigned long mapping_size;
 	int i;
 	unsigned long j;
-	const char *secstrings;
 
-	ELF(Shdr) *symtab_hdr = NULL, *strtab_hdr, *secstrings_hdr;
+	ELF(Shdr) *symtab_hdr = NULL, *strtab_hdr;
 	ELF(Ehdr) *hdr = (ELF(Ehdr) *)raw_addr;
 	ELF(Dyn) *dyn = 0, *dyn_end = 0;
 	INT_BITS syms[NSYMS] = {};
@@ -64,9 +63,6 @@ static void BITSFUNC(go)(void *raw_addr, size_t raw_len,
 	}
 
 	/* Walk the section table */
-	secstrings_hdr = raw_addr + GET_BE(&hdr->e_shoff) +
-		GET_BE(&hdr->e_shentsize)*GET_BE(&hdr->e_shstrndx);
-	secstrings = raw_addr + GET_BE(&secstrings_hdr->sh_offset);
 	for (i = 0; i < GET_BE(&hdr->e_shnum); i++) {
 		ELF(Shdr) *sh = raw_addr + GET_BE(&hdr->e_shoff) +
 			GET_BE(&hdr->e_shentsize) * i;
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nick Alcock Sept. 11, 2017, 2:34 p.m. UTC | #4 
Thank you very much for this!

On 10 Sep 2017, David Miller verbalised:

> From: David Miller <davem@davemloft.net>
> Date: Mon, 14 Aug 2017 22:35:53 -0700 (PDT)
>
>> From: Nick Alcock <nick.alcock@oracle.com>
>> Date: Fri, 11 Aug 2017 13:23:26 +0100
>> 
>>> unless you have a really serious linker bug,
>> 
>> Open your mind a little bit.
>
> Looking at the actual details, how did this ever work for anyone?

Probably chance. It did seem to work, though.

> The arch/sparc/vdso/vdso.lds file says:
>
> vvar_start = . -(1 << 12);
>
> And "1 << 12" is 4096, therefore a multiple of 4096 and not 8192.

Uh. Yeah. How the heck did I miss that? 50% chance it'll end up
correctly aligned *anyway*, of course, and I must have been lucky.

Blasted machine-variable page sizes. (You can tell this was derived from
the x86 code :/ )

> So, as requested, the object built gets:
>
> davem@patience:~/src/GIT/sparc-next$ nm -n arch/sparc/vdso/vdso64.so.dbg
> fffffffffffff000 a vvar_data
> fffffffffffff000 a vvar_start
>
> So why is vdso2c requiring an 8192 byte aligned value for the address
> of 'vvar_start'?

Because it has to be page-aligned. It's just, uh, the wrong alignment
for this architecture. Mea culpa.

> Nick, I will be honest and sat that I'm kinda irritated that I had to
> track this down and the best you could come up with was blaming the
> linker...  :-(

It's been two years since I looked at that code, and when Nagarathnam
sent his email I was at the wrong end of a high-latency satellite link
on holiday and then knee-deep in other time-critical stuff, I'm afraid.
I entirely forgot this thread was still alive.

On 11 Sep 2017, David Miller said:

> Breaking this down further, the linker scripts are written assuming
> they will be built by a host compiler that generates 64-bit target
> code by default.
>
> This is not a safe assumption.

One presumes it's not safe on x86-64 either, then.

> My system generates 32-bit binaries from gcc by default, and that's
> why you end up with a value of PAGE_SIZE which is 4096 propagating
> through all of the generated linker scripts.

*This* is down to distro variance, I'm afraid. (And yes, I think your
machine is correctly configured, and I should have tested on a
-m32-by-default GCC, but I didn't think of it.)
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
nagarathnam muthusamy Sept. 21, 2017, 3:12 p.m. UTC | #5 
Hi David,
     I have fixed the issues with the patch and sent an updated patch. I 
was able to reproduce the issue
in a qemu sparc emulator running Debian. I checked that the VDSO 
binaries pass compilation with the
new patch in that environment.

Thanks,
Nagarathnam.
On 9/10/2017 9:03 PM, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Sat, 09 Sep 2017 20:41:29 -0700 (PDT)
>
>> The arch/sparc/vdso/vdso.lds file says:
>>
>> vvar_start = . -(1 << 12);
>>
>> And "1 << 12" is 4096, therefore a multiple of 4096 and not 8192.
>>
>> So, as requested, the object built gets:
>>
>> davem@patience:~/src/GIT/sparc-next$ nm -n arch/sparc/vdso/vdso64.so.dbg
>> fffffffffffff000 a vvar_data
>> fffffffffffff000 a vvar_start
> Breaking this down further, the linker scripts are written assuming
> they will be built by a host compiler that generates 64-bit target
> code by default.
>
> This is not a safe assumption.
>
> My system generates 32-bit binaries from gcc by default, and that's
> why you end up with a value of PAGE_SIZE which is 4096 propagating
> through all of the generated linker scripts.
>
> x86, where it seems like were at least used as a template for the
> sparc linker scripts, gets away with this because the PAGE_SIZE
> is the same on 32-bit and 64-bit.
>
> Anyways, since 8192 is hard coded into vdso2c, just use 8192 as
> PAGE_SIZE in vdso-layout.lds.S and maybe put a comment above it.
>
> The next problem with the patch is that we get a warning because
> the variable 'secstrings_hdr' is set but not used in vdso2c.h, and
> compiler warnings are treated as errors under arch/sparc.
>
> This is fixed simply by:
>
> diff --git a/arch/sparc/vdso/vdso2c.h b/arch/sparc/vdso/vdso2c.h
> index 8d1b5cf..b193e3d 100644
> --- a/arch/sparc/vdso/vdso2c.h
> +++ b/arch/sparc/vdso/vdso2c.h
> @@ -17,9 +17,8 @@ static void BITSFUNC(go)(void *raw_addr, size_t raw_len,
>   	unsigned long mapping_size;
>   	int i;
>   	unsigned long j;
> -	const char *secstrings;
>   
> -	ELF(Shdr) *symtab_hdr = NULL, *strtab_hdr, *secstrings_hdr;
> +	ELF(Shdr) *symtab_hdr = NULL, *strtab_hdr;
>   	ELF(Ehdr) *hdr = (ELF(Ehdr) *)raw_addr;
>   	ELF(Dyn) *dyn = 0, *dyn_end = 0;
>   	INT_BITS syms[NSYMS] = {};
> @@ -64,9 +63,6 @@ static void BITSFUNC(go)(void *raw_addr, size_t raw_len,
>   	}
>   
>   	/* Walk the section table */
> -	secstrings_hdr = raw_addr + GET_BE(&hdr->e_shoff) +
> -		GET_BE(&hdr->e_shentsize)*GET_BE(&hdr->e_shstrndx);
> -	secstrings = raw_addr + GET_BE(&secstrings_hdr->sh_offset);
>   	for (i = 0; i < GET_BE(&hdr->e_shnum); i++) {
>   		ELF(Shdr) *sh = raw_addr + GET_BE(&hdr->e_shoff) +
>   			GET_BE(&hdr->e_shentsize) * i;

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

--- /dev/null
+++ b/arch/sparc/vdso/vdso-layout.lds.S
@@ -0,0 +1,106 @@ 
[...]
+SECTIONS
+{
+	/*
+	 * User/kernel shared data is before the vDSO.  This may be a little
+	 * uglier than putting it after the vDSO, but it avoids issues with
+	 * non-allocatable things that dangle past the end of the PT_LOAD
+	 * segment.
+	 */
+
+	vvar_start = . -PAGE_SIZE;
+	vvar_data = vvar_start;
+
+	. = SIZEOF_HEADERS;

unless you have a really serious linker bug, there is no way vvar_start
could fail to be at a multiple of 8192 bytes, AFAICS, and sym_vvar_start
should have been set up by vdso2c to point right at it.

The set but not used variable stuff suggests you've been doing stuff
inside vdso2c, which I was just able to carry across unmodified from x86
and which worked fine when I did that. I bet some of that is somehow
broken and the various symbol references in the vdso2c output are
wrong somehow.

-- 
NULL && (void)
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html