Re: [patch] paravirt: VDSO page is essential

[Rusty Russell <rusty@rustcorp.com.au>]

On Mon, 2007-03-05 at 13:06 +0100, Ingo Molnar wrote:
> Subject: [patch] paravirt: VDSO page is essential
> From: Ingo Molnar <mingo@elte.hu>
> 
> commit 3bbf54725467d604698721384d858b5983b87e8f disables the VDSO for 
> CONFIG_PARAVIRT kernels. This #ifdeffery was a bad change: the VDSO is 
> an essential component of Linux, and this change forces all of them to 
> use int $0x80 - including sane ones like KVM. (If a hypervisor does not 
> handle the VDSO properly then it can work things around via the vdso=0 
> boot option. Or CONFIG_PARAVIRT should not have been merged. But in any 
> case, it is a basic taste issue: we DO NOT #ifdef around core features 
> like this!)

I agree with the criticism, dislike the snarly comments, and disagree
with this patch.

VDSO is only a problem if (1) the hypervisor wants to reserve the top
virtual address space (CONFIG_PARAVIRT=y), and (2) the glibc is old and
can't handle a VDSO mapped anywhere but 0xFFFFE000
(CONFIG_COMPAT_VDSO=y).

Now, KVM wants to use CONFIG_PARAVIRT=y but not reserve_top_address(),
so we should split the config option.  Let's not get too excited because
we kept it simple.  Patch (untested, but fairly simple) below.

BTW, I had a patch to do a runtime test (old glibc causes init to
assert, then disable vdso and try again): everyone hated it.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff -r f75715e64a3b arch/i386/Kconfig
--- a/arch/i386/Kconfig	Tue Mar 06 00:04:50 2007 +1100
+++ b/arch/i386/Kconfig	Tue Mar 06 00:20:44 2007 +1100
@@ -218,9 +218,18 @@ config PARAVIRT
 	  However, when run without a hypervisor the kernel is
 	  theoretically slower.  If in doubt, say N.
 
+config RESERVE_TOP
+	bool
+	help
+	  Many hypervisors want to reserve some amount of the top of
+	  virtual address space.  Unfortunately, old glibc needs the
+	  vdso page there, so we must disable vdso if COMPAT_VDSO is
+	  enabled as well as this option.
+
 config VMI
 	bool "VMI Paravirt-ops support"
 	depends on PARAVIRT && !NO_HZ
+	select RESERVE_TOP
 	default y
 	help
 	  VMI provides a paravirtualized interface to multiple hypervisors
@@ -893,9 +902,10 @@ config COMPAT_VDSO
 config COMPAT_VDSO
 	bool "Compat VDSO support"
 	default y
-	depends on !PARAVIRT
-	help
-	  Map the VDSO to the predictable old-style address too.
+	help
+	  Map the VDSO to the predictable old-style address too, or
+	  in the case of a VMI/Xen/lguest virtualized guest, don't create
+	  the VDSO at all.
 	---help---
 	  Say N here if you are running a sufficiently recent glibc
 	  version (2.3.3 or later), to remove the high-mapped
diff -r f75715e64a3b arch/i386/kernel/sysenter.c
--- a/arch/i386/kernel/sysenter.c	Tue Mar 06 00:04:50 2007 +1100
+++ b/arch/i386/kernel/sysenter.c	Tue Mar 06 00:21:42 2007 +1100
@@ -27,7 +27,7 @@
  * Should the kernel map a VDSO page into processes and pass its
  * address down to glibc upon exec()?
  */
-#ifdef CONFIG_PARAVIRT
+#if defined(CONFIG_COMPAT_VDSO) && defined(CONFIG_RESERVE_TOP)
 unsigned int __read_mostly vdso_enabled = 0;
 #else
 unsigned int __read_mostly vdso_enabled = 1;
diff -r f75715e64a3b arch/i386/mm/pgtable.c
--- a/arch/i386/mm/pgtable.c	Tue Mar 06 00:04:50 2007 +1100
+++ b/arch/i386/mm/pgtable.c	Tue Mar 06 00:06:00 2007 +1100
@@ -173,7 +173,7 @@ void reserve_top_address(unsigned long r
 	BUG_ON(fixmaps > 0);
 	printk(KERN_INFO "Reserving virtual address space above 0x%08x\n",
 	       (int)-reserve);
-#ifdef CONFIG_COMPAT_VDSO
+#ifndef CONFIG_RESERVE_TOP
 	BUG_ON(reserve != 0);
 #else
 	__FIXADDR_TOP = -reserve - PAGE_SIZE;