The expression `((void *)&__timens_vdso_data - (void *)&__vdso_data)`
seems harmless, but it actually results in quite a bit of code and two
jumps, in a place that's supposed to be somewhat lean on code. The value
of that calculation is always 3*PAGE_SIZE, as it turns out. Changing it
to that results in a more modest cmov instruction being emitted. It also
makes it a bit more clear what's happening.

To accomplish this, define offset macros in vvar.h, which can be shared
by C code and by the linker script that decides where these pages will
actually go.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 arch/x86/entry/vdso/vdso-layout.lds.S | 11 +++++------
 arch/x86/include/asm/vdso/getrandom.h |  2 +-
 arch/x86/include/asm/vvar.h           |  5 +++++
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/arch/x86/entry/vdso/vdso-layout.lds.S b/arch/x86/entry/vdso/vdso-layout.lds.S
index bafa73f09e92..ddd6999b6946 100644
--- a/arch/x86/entry/vdso/vdso-layout.lds.S
+++ b/arch/x86/entry/vdso/vdso-layout.lds.S
@@ -16,17 +16,16 @@ SECTIONS
 	 * segment.
 	 */
 
-	vvar_start = . - 4 * PAGE_SIZE;
-	vvar_page  = vvar_start;
-
 	/* Place all vvars at the offsets in asm/vvar.h. */
 #define EMIT_VVAR(name, offset) vvar_ ## name = vvar_page + offset;
 #include <asm/vvar.h>
 #undef EMIT_VVAR
 
-	pvclock_page = vvar_start + PAGE_SIZE;
-	hvclock_page = vvar_start + 2 * PAGE_SIZE;
-	timens_page  = vvar_start + 3 * PAGE_SIZE;
+	vvar_start = . - 4 * PAGE_SIZE;
+	vvar_page    = vvar_start + VVAR_PAGE_OFFSET * PAGE_SIZE;
+	pvclock_page = vvar_start + PVCLOCK_PAGE_OFFSET * PAGE_SIZE;
+	hvclock_page = vvar_start + HVCLOCK_PAGE_OFFSET * PAGE_SIZE;
+	timens_page  = vvar_start + TIMENS_PAGE_OFFSET * PAGE_SIZE;
 
 #undef _ASM_X86_VVAR_H
 	/* Place all vvars in timens too at the offsets in asm/vvar.h. */
diff --git a/arch/x86/include/asm/vdso/getrandom.h b/arch/x86/include/asm/vdso/getrandom.h
index b96e674cafde..3569fe95aa4e 100644
--- a/arch/x86/include/asm/vdso/getrandom.h
+++ b/arch/x86/include/asm/vdso/getrandom.h
@@ -33,7 +33,7 @@ static __always_inline ssize_t getrandom_syscall(void *buffer, size_t len, unsig
 static __always_inline const struct vdso_rng_data *__arch_get_vdso_rng_data(void)
 {
 	if (IS_ENABLED(CONFIG_TIME_NS) && __vdso_data->clock_mode == VDSO_CLOCKMODE_TIMENS)
-		return (void *)&__vdso_rng_data + ((void *)&__timens_vdso_data - (void *)&__vdso_data);
+		return (void *)&__vdso_rng_data + (TIMENS_PAGE_OFFSET << CONFIG_PAGE_SHIFT);
 	return &__vdso_rng_data;
 }
 
diff --git a/arch/x86/include/asm/vvar.h b/arch/x86/include/asm/vvar.h
index 9d9af37f7cab..d2a2ffa72909 100644
--- a/arch/x86/include/asm/vvar.h
+++ b/arch/x86/include/asm/vvar.h
@@ -19,6 +19,11 @@
 #ifndef _ASM_X86_VVAR_H
 #define _ASM_X86_VVAR_H
 
+#define VVAR_PAGE_OFFSET	0
+#define PVCLOCK_PAGE_OFFSET	1
+#define HVCLOCK_PAGE_OFFSET	2
+#define TIMENS_PAGE_OFFSET	3
+
 #ifdef EMIT_VVAR
 /*
  * EMIT_VVAR() is used by the kernel linker script to put vvars in the
-- 
2.46.0

Hey tglx,

On Fri, Sep 06, 2024 at 09:06:55PM +0200, Jason A. Donenfeld wrote:
> The expression `((void *)&__timens_vdso_data - (void *)&__vdso_data)`
> seems harmless, but it actually results in quite a bit of code and two
> jumps, in a place that's supposed to be somewhat lean on code. The value
> of that calculation is always 3*PAGE_SIZE, as it turns out. Changing it
> to that results in a more modest cmov instruction being emitted. It also
> makes it a bit more clear what's happening.
> 
> To accomplish this, define offset macros in vvar.h, which can be shared
> by C code and by the linker script that decides where these pages will
> actually go.

I noticed we've only got a week left til the merge window opens, so I
thought I should poke you about this, if you want to take this through
tip. I can also take it through my random.git tree with your ack, if
that's easier for you. (Assuming, of course, that this patch is actually
correct.) Let me know.

Jason

On Mon, Sep 09 2024 at 17:44, Jason A. Donenfeld wrote:
> On Fri, Sep 06, 2024 at 09:06:55PM +0200, Jason A. Donenfeld wrote:
>> The expression `((void *)&__timens_vdso_data - (void *)&__vdso_data)`
>> seems harmless, but it actually results in quite a bit of code and two
>> jumps, in a place that's supposed to be somewhat lean on code. The value
>> of that calculation is always 3*PAGE_SIZE, as it turns out. Changing it
>> to that results in a more modest cmov instruction being emitted. It also
>> makes it a bit more clear what's happening.
>> 
>> To accomplish this, define offset macros in vvar.h, which can be shared
>> by C code and by the linker script that decides where these pages will
>> actually go.
>
> I noticed we've only got a week left til the merge window opens, so I
> thought I should poke you about this, if you want to take this through
> tip. I can also take it through my random.git tree with your ack, if
> that's easier for you. (Assuming, of course, that this patch is actually
> correct.) Let me know.

It's not the end of the world if this does not go in now. It's in my
back log and that VDSO stuff needs more care than this particular thing
as the recent discussion about vdso random on other architectures show.

Thanks,

        tglx

On Tue, Sep 10, 2024 at 02:08:11PM +0200, Thomas Gleixner wrote:
> On Mon, Sep 09 2024 at 17:44, Jason A. Donenfeld wrote:
> > On Fri, Sep 06, 2024 at 09:06:55PM +0200, Jason A. Donenfeld wrote:
> >> The expression `((void *)&__timens_vdso_data - (void *)&__vdso_data)`
> >> seems harmless, but it actually results in quite a bit of code and two
> >> jumps, in a place that's supposed to be somewhat lean on code. The value
> >> of that calculation is always 3*PAGE_SIZE, as it turns out. Changing it
> >> to that results in a more modest cmov instruction being emitted. It also
> >> makes it a bit more clear what's happening.
> >> 
> >> To accomplish this, define offset macros in vvar.h, which can be shared
> >> by C code and by the linker script that decides where these pages will
> >> actually go.
> >
> > I noticed we've only got a week left til the merge window opens, so I
> > thought I should poke you about this, if you want to take this through
> > tip. I can also take it through my random.git tree with your ack, if
> > that's easier for you. (Assuming, of course, that this patch is actually
> > correct.) Let me know.
> 
> It's not the end of the world if this does not go in now. It's in my
> back log and that VDSO stuff needs more care than this particular thing
> as the recent discussion about vdso random on other architectures show.

Okay, that makes sense. And thanks for taking a look.

Jason

On Tue, Sep 10, 2024 at 02:20:17PM +0200, Jason A. Donenfeld wrote:
> On Tue, Sep 10, 2024 at 02:08:11PM +0200, Thomas Gleixner wrote:
> > On Mon, Sep 09 2024 at 17:44, Jason A. Donenfeld wrote:
> > > On Fri, Sep 06, 2024 at 09:06:55PM +0200, Jason A. Donenfeld wrote:
> > >> The expression `((void *)&__timens_vdso_data - (void *)&__vdso_data)`
> > >> seems harmless, but it actually results in quite a bit of code and two
> > >> jumps, in a place that's supposed to be somewhat lean on code. The value
> > >> of that calculation is always 3*PAGE_SIZE, as it turns out. Changing it
> > >> to that results in a more modest cmov instruction being emitted. It also
> > >> makes it a bit more clear what's happening.
> > >> 
> > >> To accomplish this, define offset macros in vvar.h, which can be shared
> > >> by C code and by the linker script that decides where these pages will
> > >> actually go.
> > >
> > > I noticed we've only got a week left til the merge window opens, so I
> > > thought I should poke you about this, if you want to take this through
> > > tip. I can also take it through my random.git tree with your ack, if
> > > that's easier for you. (Assuming, of course, that this patch is actually
> > > correct.) Let me know.
> > 
> > It's not the end of the world if this does not go in now. It's in my
> > back log and that VDSO stuff needs more care than this particular thing
> > as the recent discussion about vdso random on other architectures show.
> 
> Okay, that makes sense. And thanks for taking a look.

Thought I'd poke again about this. It looks like this still applies to
tip.

Jason