Skip to contentSkip to conversation

Xen i386 paravirtualization support

RFC PATCH In review Chris Wright 20 participants 101 messages Jul 18, 2006 – Jul 27, 2006

Subsystems (7)
lore
Share & tools
lore
#0 lore raw

cover Xen i386 paravirtualization support

Unlike full virtualization in which the virtual machine provides the same platform interface as running natively on the hardware, paravirtualization requires modification to the guest operating system to work with the platform interface provided by the hypervisor.

Xen was designed with performance in mind. Calls to the hypervisor are minimized, batched if necessary, and non-critical codepaths are left unmodified in the case where the privileged instruction can be trapped and emulated by the hypervisor. The Xen API is designed to be OS agnostic and has had Linux, NetBSD, FreeBSD, Solaris, Plan9 and Netware ported to it. Xen also provides support for running directly on native hardware.

The following patch series provides the minimal support required to launch Xen paravirtual guests on standard x86 hardware running the Xen hypervisor. These patches effectively port the Linux kernel to run on the platform interface provided by Xen. This port is done as an i386 subarch.

With these patches you will be able to launch an unprivileged guest running the modified Linux kernel and unmodified userspace. This guest is x86, UP only, runs in shadow translated mode, and has no direct access to hardware. This simplifies the patchset to the minimum functionality needed to support a paravirtualized guest. It's worth noting that a fair amount of this patchset deals with paravirtualizing I/O, not just CPU-only. The additional missing functionality is primarily about full SMP support, optimizations such as direct writable page tables, and the management interface.

At a high-level, the patches provide the following:

- Kconfig and Makefile changes required to support Xen
- subarch changes to allow more platform functionality to be implemented by an i386 subarch
- Xen subarch implementation
- start of day code for running in the hypervisor provided environment (paging enabled)
- basic Xen drivers to provide a fully functional guest

The Xen platform API encapsulates the following types of requirements:

- idt, gdt, ldt (descriptor table handling)
- cr2, fpu_taskswitch, debug registers (privileged register handling)
- mmu (page table, tlb, and cache handling)
- memory reservations
- time and timer
- vcpu (init, up/down vcpu)
- schedule (processor yield, shutdown, etc)
- event channel (generalized virtual interrupt handling)
- grant table (shared memory interface for high speed interdomain communication)
- block device I/O
- network device I/O
- console device I/O
- Xen feature map
- Xen version info

Thanks to all have reviewed earlier versions of these patches. -chris
--

#1 lore raw

RFC PATCH 1/33 Add apply_to_page_range() function

Add a new mm function apply_to_page_range() which applies a given function to every pte in a given virtual address range in a given mm structure. This is a generic alternative to cut-and-pasting the Linux idiomatic pagetable walking code in every place that a sequence of PTEs must be accessed.

Although this interface is intended to be useful in a wide range of situations, it is currently used specifically by several Xen subsystems, for example: to ensure that pagetables have been allocated for a virtual address range, and to construct batched special pagetable update requests to map I/O memory (in ioremap()).

---
 include/linux/mm.h |    5 ++
 mm/memory.c        |   94 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 99 insertions(+)
include/linux/mm.h +5 -0
diff -r 84a7a7b38774 include/linux/mm.h
@@ -1013,6 +1013,11 @@ struct page *follow_page(struct vm_area_
 #define FOLL_GET	0x04	/* do get_page on page */
 #define FOLL_ANON	0x08	/* give ZERO_PAGE if no pgtable */
 
+typedef int (*pte_fn_t)(pte_t *pte, struct page *pmd_page, unsigned long addr,
+			void *data);
+extern int apply_to_page_range(struct mm_struct *mm, unsigned long address,
+			       unsigned long size, pte_fn_t fn, void *data);
+
 #ifdef CONFIG_PROC_FS
 void vm_stat_account(struct mm_struct *, unsigned long, struct file *, long);
 #else
mm/memory.c +94 -0

100 lines not on this page · raw message

--

#2 lore raw

Re: [RFC PATCH 01/33] Add apply_to_page_range() function

Quoted 9 lines · Chris Wright

...

mm/memory.c +2 -0
@@ -1358,6 +1358,100 @@ int remap_pfn_range(struct vm_area_struc
 }
 EXPORT_SYMBOL(remap_pfn_range);
 
+static inline int apply_to_pte_range(struct mm_struct *mm, pmd_t *pmd,
+				     unsigned long addr, unsigned long end,

mm/memory.c int remap_pfn_range(struct vm_area_struc · line 1413

Quoted 10 lines
mm/memory.c +7 -0
+				     pte_fn_t fn, void *data)
...
+static inline int apply_to_pmd_range(struct mm_struct *mm, pud_t *pud,
+				     unsigned long addr, unsigned long end,
+				     pte_fn_t fn, void *data)
...
+static inline int apply_to_pud_range(struct mm_struct *mm, pgd_t *pgd,
+				     unsigned long addr, unsigned long end,
+				     pte_fn_t fn, void *data)
...

Please avoid "inline" in C files.

(gcc already automatically inlines static functions with only one caller.)

cu
Adrian

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed
#3 lore raw

Re: [RFC PATCH 01/33] Add apply_to_page_range() function

Quoted 23 lines · * Adrian Bunk ([email protected]) wrote:
Quoted 9 lines · Chris Wright

...

mm/memory.c +2 -0
@@ -1358,6 +1358,100 @@ int remap_pfn_range(struct vm_area_struc
 }
 EXPORT_SYMBOL(remap_pfn_range);
 
+static inline int apply_to_pte_range(struct mm_struct *mm, pmd_t *pmd,
+				     unsigned long addr, unsigned long end,

mm/memory.c int remap_pfn_range(struct vm_area_struc · line 1413

Quoted 10 lines
mm/memory.c +7 -0
+				     pte_fn_t fn, void *data)
...
+static inline int apply_to_pmd_range(struct mm_struct *mm, pud_t *pud,
+				     unsigned long addr, unsigned long end,
+				     pte_fn_t fn, void *data)
...
+static inline int apply_to_pud_range(struct mm_struct *mm, pgd_t *pgd,
+				     unsigned long addr, unsigned long end,
+				     pte_fn_t fn, void *data)
...

Please avoid "inline" in C files.

Quoted 1 line

(gcc already automatically inlines static functions with only one caller.)

Sure, that's fair. The surrounding similar code follows the same format as above, perhaps you plan to patch?

#4 lore raw

Re: [RFC PATCH 01/33] Add apply_to_page_range() function

Quoted 24 lines · Chris Wright
Quoted 23 lines · * Adrian Bunk ([email protected]) wrote:
Quoted 9 lines · Chris Wright

...

mm/memory.c +2 -0
@@ -1358,6 +1358,100 @@ int remap_pfn_range(struct vm_area_struc
 }
 EXPORT_SYMBOL(remap_pfn_range);
 
+static inline int apply_to_pte_range(struct mm_struct *mm, pmd_t *pmd,
+				     unsigned long addr, unsigned long end,

mm/memory.c int remap_pfn_range(struct vm_area_struc · line 1413

Quoted 10 lines
mm/memory.c +7 -0
+				     pte_fn_t fn, void *data)
...
+static inline int apply_to_pmd_range(struct mm_struct *mm, pud_t *pud,
+				     unsigned long addr, unsigned long end,
+				     pte_fn_t fn, void *data)
...
+static inline int apply_to_pud_range(struct mm_struct *mm, pgd_t *pgd,
+				     unsigned long addr, unsigned long end,
+				     pte_fn_t fn, void *data)
...

Please avoid "inline" in C files.

Quoted 4 lines
Quoted 1 line

(gcc already automatically inlines static functions with only one caller.)

Sure, that's fair. The surrounding similar code follows the same format as above, perhaps you plan to patch?

Already part of a bigger item on my TODO list for some months.

Priority: low...

cu
Adrian

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed
#5 lore raw

RFC PATCH 2/33 Add sync bitops

In reply to Chris Wright

Add "always lock'd" implementations of set_bit, clear_bit and change_bit and the corresponding test_and_ functions. Also add "always lock'd" implementation of cmpxchg. These give guaranteed strong synchronisation and are required for non-SMP kernels running on an SMP hypervisor.

---
 include/asm-i386/synch_bitops.h |  166 ++++++++++++++++++++++++++++++++++++++++
 include/asm-i386/system.h       |   33 +++++++
 2 files changed, 199 insertions(+)
include/asm-i386/system.h +33 -0
diff -r 935903fb1136 include/asm-i386/system.h
@@ -263,6 +263,9 @@ static inline unsigned long __xchg(unsig
 #define cmpxchg(ptr,o,n)\
 	((__typeof__(*(ptr)))__cmpxchg((ptr),(unsigned long)(o),\
 					(unsigned long)(n),sizeof(*(ptr))))
+#define synch_cmpxchg(ptr,o,n)\
+	((__typeof__(*(ptr)))__synch_cmpxchg((ptr),(unsigned long)(o),\
+					(unsigned long)(n),sizeof(*(ptr))))
 #endif
 
 static inline unsigned long __cmpxchg(volatile void *ptr, unsigned long old,
@@ -291,6 +294,36 @@ static inline unsigned long __cmpxchg(vo
 	}
 	return old;
 }
+
+#define __LOCK_PREFIX "lock ; "
+static inline unsigned long __synch_cmpxchg(volatile void *ptr,
+					    unsigned long old,
+					    unsigned long new, int size)
+{
+	unsigned long prev;
+	switch (size) {
+	case 1:
+		__asm__ __volatile__(__LOCK_PREFIX "cmpxchgb %b1,%2"
+				     : "=a"(prev)
+				     : "q"(new), "m"(*__xg(ptr)), "0"(old)
+				     : "memory");
+		return prev;
+	case 2:
+		__asm__ __volatile__(__LOCK_PREFIX "cmpxchgw %w1,%2"
+				     : "=a"(prev)
+				     : "r"(new), "m"(*__xg(ptr)), "0"(old)
+				     : "memory");
+		return prev;
+	case 4:
+		__asm__ __volatile__(__LOCK_PREFIX "cmpxchgl %1,%2"
+				     : "=a"(prev)
+				     : "r"(new), "m"(*__xg(ptr)), "0"(old)
+				     : "memory");
+		return prev;
+	}
+	return old;
+}
+#undef __LOCK_PREFIX
 
 #ifndef CONFIG_X86_CMPXCHG
 /*
include/asm-i386/synch_bitops.h new file +166 -0

166 lines not on this page · raw message

--

#6 lore raw

Re: [RFC PATCH 02/33] Add sync bitops

Quoted 6 lines · Chris Wright

plain text document attachment (synch-ops)
Add "always lock'd" implementations of set_bit, clear_bit and change_bit and the corresponding test_and_ functions. Also add "always lock'd" implementation of cmpxchg. These give guaranteed strong synchronisation and are required for non-SMP kernels running on an SMP hypervisor.

Hi,

this sounds really like the wrong approach; you know you're compiling for xen, so why not just make set_bit() and the others use the lock'd instructions at compile time?

Greetings,
Arjan van de Ven

#7 lore raw

Re: [RFC PATCH 02/33] Add sync bitops

Quoted 7 lines · Arjan van de Ven
Quoted 6 lines

plain text document attachment (synch-ops)
Add "always lock'd" implementations of set_bit, clear_bit and change_bit and the corresponding test_and_ functions. Also add "always lock'd" implementation of cmpxchg. These give guaranteed strong synchronisation and are required for non-SMP kernels running on an SMP hypervisor.

Quoted 5 lines

Hi,

this sounds really like the wrong approach; you know you're compiling for xen, so why not just make set_bit() and the others use the lock'd instructions at compile time?

Then all users of bitops would unnecessarily pay the price when running in a single-CPU guest. Only a few of our bitops callers absolutely require the lock prefix in all cases.

-- Keir

#8 lore raw

Re: [RFC PATCH 02/33] Add sync bitops

In reply to Arjan van de Ven
Quoted 8 lines · Arjan van de Ven
Quoted 6 lines · Chris Wright

plain text document attachment (synch-ops)
Add "always lock'd" implementations of set_bit, clear_bit and change_bit and the corresponding test_and_ functions. Also add "always lock'd" implementation of cmpxchg. These give guaranteed strong synchronisation and are required for non-SMP kernels running on an SMP hypervisor.

Quoted 5 lines

Hi,

this sounds really like the wrong approach; you know you're compiling for xen, so why not just make set_bit() and the others use the lock'd instructions at compile time?

I guess because they only need it for a small subset of set_bits.

-Andi

#9 lore raw

Re: [RFC PATCH 02/33] Add sync bitops

In reply to Chris Wright
Quoted 19 lines · Chris Wright

Add "always lock'd" implementations of set_bit, clear_bit and change_bit and the corresponding test_and_ functions. Also add "always lock'd" implementation of cmpxchg. These give guaranteed strong synchronisation and are required for non-SMP kernels running on an SMP hypervisor.

---
 include/asm-i386/synch_bitops.h |  166 ++++++++++++++++++++++++++++++++++++++++
 include/asm-i386/system.h       |   33 +++++++
 2 files changed, 199 insertions(+)

diff -r 935903fb1136 include/asm-i386/system.h

include/asm-i386/system.h static inline unsigned long __xchg(unsig · line 269

Quoted 8 lines
include/asm-i386/system.h +3 -0
@@ -263,6 +263,9 @@ static inline unsigned long __xchg(unsig
 #define cmpxchg(ptr,o,n)\
 	((__typeof__(*(ptr)))__cmpxchg((ptr),(unsigned long)(o),\
 					(unsigned long)(n),sizeof(*(ptr))))
+#define synch_cmpxchg(ptr,o,n)\
+	((__typeof__(*(ptr)))__synch_cmpxchg((ptr),(unsigned long)(o),\
+					(unsigned long)(n),sizeof(*(ptr))))
 #endif
...

Do I miss anything, or is CONFIG_X86_XEN=y, CONFIG_M386=y an allowed configuration that will result in a compile error?

cu
Adrian

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed
#10 lore raw

RFC PATCH 3/33 Add nosegneg capability to the vsyscall page notes

In reply to Chris Wright

Add the "nosegneg" fake capabilty to the vsyscall page notes. This is used by the runtime linker to select a glibc version which then disables negative-offset accesses to the thread-local segment via %gs. These accesses require emulation in Xen (because segments are truncated to protect the hypervisor address space) and avoiding them provides a measurable performance boost.

---
 arch/i386/kernel/vsyscall-note.S |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+)
arch/i386/kernel/vsyscall-note.S +29 -0
diff -r b93c0dae6c49 arch/i386/kernel/vsyscall-note.S
@@ -3,6 +3,7 @@
  * Here we can supply some information useful to userland.
  */
 
+#include <linux/config.h>
 #include <linux/uts.h>
 #include <linux/version.h>
 
@@ -23,3 +24,31 @@ 3:	.balign 4;		/* pad out section */			 
 	ASM_ELF_NOTE_BEGIN(".note.kernel-version", "a", UTS_SYSNAME, 0)
 	.long LINUX_VERSION_CODE
 	ASM_ELF_NOTE_END
+
+#ifdef CONFIG_XEN
+/*
+ * Add a special note telling glibc's dynamic linker a fake hardware
+ * flavor that it will use to choose the search path for libraries in the
+ * same way it uses real hardware capabilities like "mmx".
+ * We supply "nosegneg" as the fake capability, to indicate that we
+ * do not like negative offsets in instructions using segment overrides,
+ * since we implement those inefficiently.  This makes it possible to
+ * install libraries optimized to avoid those access patterns in someplace
+ * like /lib/i686/tls/nosegneg.  Note that an /etc/ld.so.conf.d/file
+ * corresponding to the bits here is needed to make ldconfig work right.
+ * It should contain:
+ *	hwcap 0 nosegneg
+ * to match the mapping of bit to name that we give here.
+ */
+#define NOTE_KERNELCAP_BEGIN(ncaps, mask) \
+	ASM_ELF_NOTE_BEGIN(".note.kernelcap", "a", "GNU", 2) \
+	.long ncaps, mask
+#define NOTE_KERNELCAP(bit, name) \
+	.byte bit; .asciz name
+#define NOTE_KERNELCAP_END ASM_ELF_NOTE_END
+
+NOTE_KERNELCAP_BEGIN(1, 1)
+NOTE_KERNELCAP(1, "nosegneg")
+NOTE_KERNELCAP_END
+#endif
+

--

#11 lore raw

RFC PATCH 4/33 Add XEN config options and disable unsupported config options.

In reply to Chris Wright

The XEN config option is selected from the i386 subarch menu by choosing the X86_XEN "Xen-compatible" subarch.

The XEN_SHADOW_MODE option defines the memory virtualization mode for the kernel -- with it enabled, the kernel expects the hypervisor to perform translation between pseudo-physical and machine addresses on its behalf.

The disabled config options are:
- DOUBLEFAULT: are trapped by Xen and not virtualized
- HZ: defaults to 100 in Xen VMs
- Power management: not supported in unprivileged VMs
- SMP: not supported in this set of patches
- X86_{UP,LOCAL,IO}_APIC: not supported in unprivileged VMs

---
 arch/i386/Kconfig       |   18 ++++++++++++++----
 arch/i386/Kconfig.debug |    1 +
 drivers/xen/Kconfig     |   21 +++++++++++++++++++++
 kernel/Kconfig.hz       |    4 ++--
 kernel/Kconfig.preempt  |    1 +
 5 files changed, 39 insertions(+), 6 deletions(-)
arch/i386/Kconfig +15 -5
diff -r 5851dd4e1b8f arch/i386/Kconfig
@@ -67,6 +67,7 @@ menu "Processor type and features"
 
 config SMP
 	bool "Symmetric multi-processing support"
+	depends on !X86_XEN
 	---help---
 	  This enables support for systems with more than one CPU. If you have
 	  a system with only one CPU, like most personal computers, say N. If
@@ -102,6 +103,12 @@ config X86_PC
 	bool "PC-compatible"
 	help
 	  Choose this option if your computer is a standard PC or compatible.
+
+config X86_XEN
+	bool "Xen-compatible"
+	help
+	  Choose this option if you plan to run this kernel on top of the
+	  Xen Hypervisor.
 
 config X86_ELAN
 	bool "AMD Elan"
@@ -211,6 +218,7 @@ source "arch/i386/Kconfig.cpu"
 
 config HPET_TIMER
 	bool "HPET Timer Support"
+	depends on !X86_XEN
 	help
 	  This enables the use of the HPET for the kernel's internal timer.
 	  HPET is the next generation timer replacing legacy 8254s.
@@ -261,7 +269,7 @@ source "kernel/Kconfig.preempt"
 
 config X86_UP_APIC
 	bool "Local APIC support on uniprocessors"
-	depends on !SMP && !(X86_VISWS || X86_VOYAGER)
+	depends on !SMP && !(X86_VISWS || X86_VOYAGER || X86_XEN)
 	help
 	  A local APIC (Advanced Programmable Interrupt Controller) is an
 	  integrated interrupt controller in the CPU. If you have a single-CPU
@@ -286,12 +294,12 @@ config X86_UP_IOAPIC
 
 config X86_LOCAL_APIC
 	bool
-	depends on X86_UP_APIC || ((X86_VISWS || SMP) && !X86_VOYAGER)
+	depends on X86_UP_APIC || ((X86_VISWS || SMP) && !(X86_VOYAGER || X86_XEN))
 	default y
 
 config X86_IO_APIC
 	bool
-	depends on X86_UP_IOAPIC || (SMP && !(X86_VISWS || X86_VOYAGER))
+	depends on X86_UP_IOAPIC || (SMP && !(X86_VISWS || X86_VOYAGER || X86_XEN))
 	default y
 
 config X86_VISWS_APIC
@@ -739,7 +747,7 @@ source kernel/Kconfig.hz
 
 config KEXEC
 	bool "kexec system call (EXPERIMENTAL)"
-	depends on EXPERIMENTAL
+	depends on EXPERIMENTAL && !X86_XEN
 	help
 	  kexec is a system call that implements the ability to shutdown your
 	  current kernel, and to start another kernel.  It is like a reboot
@@ -808,7 +816,7 @@ config ARCH_ENABLE_MEMORY_HOTPLUG
 	depends on HIGHMEM
 
 menu "Power management options (ACPI, APM)"
-	depends on !X86_VOYAGER
+	depends on !(X86_VOYAGER || X86_XEN)
 
 source kernel/power/Kconfig
 
@@ -1144,6 +1152,8 @@ source "security/Kconfig"
 
 source "crypto/Kconfig"
 
+source "drivers/xen/Kconfig"
+
 source "lib/Kconfig"
 
 #
arch/i386/Kconfig.cpu +1 -1
diff -r 5851dd4e1b8f arch/i386/Kconfig.cpu
@@ -251,7 +251,7 @@ config X86_PPRO_FENCE
 
 config X86_F00F_BUG
 	bool
-	depends on M586MMX || M586TSC || M586 || M486 || M386
+	depends on (M586MMX || M586TSC || M586 || M486 || M386) && !XEN
 	default y
 
 config X86_WP_WORKS_OK
arch/i386/Kconfig.debug +1 -0
diff -r 5851dd4e1b8f arch/i386/Kconfig.debug
@@ -79,6 +79,7 @@ config DOUBLEFAULT
 config DOUBLEFAULT
 	default y
 	bool "Enable doublefault exception handler" if EMBEDDED
+	depends on !X86_XEN
 	help
           This option allows trapping of rare doublefault exceptions that
           would otherwise cause a system to silently reboot. Disabling this
kernel/Kconfig.hz +2 -2
diff -r 5851dd4e1b8f kernel/Kconfig.hz
@@ -3,7 +3,7 @@
 #
 
 choice
-	prompt "Timer frequency"
+	prompt "Timer frequency" if !XEN
 	default HZ_250
 	help
 	 Allows the configuration of the timer frequency. It is customary
@@ -40,7 +40,7 @@ endchoice
 
 config HZ
 	int
-	default 100 if HZ_100
+	default 100 if HZ_100 || XEN
 	default 250 if HZ_250
 	default 1000 if HZ_1000
 
kernel/Kconfig.preempt +1 -0
diff -r 5851dd4e1b8f kernel/Kconfig.preempt
@@ -35,6 +35,7 @@ config PREEMPT_VOLUNTARY
 
 config PREEMPT
 	bool "Preemptible Kernel (Low-Latency Desktop)"
+	depends on !XEN
 	help
 	  This option reduces the latency of the kernel by making
 	  all kernel code (that is not executing in a critical section)
drivers/xen/Kconfig new file +21 -0
diff -r 5851dd4e1b8f drivers/xen/Kconfig
@@ -0,0 +1,21 @@
+#
+# This Kconfig describe xen options
+#
+
+mainmenu "Xen Configuration"
+
+config XEN
+	bool
+	default y if X86_XEN
+	help
+	  This is the Linux Xen port.
+
+if XEN
+
+config XEN_SHADOW_MODE
+	bool
+	default y
+	help
+	  Fakes out a shadow mode kernel
+
+endif

--

#12 lore raw

Re: [RFC PATCH 04/33] Add XEN config options and disable unsupported config options.

Quoted 4 lines

The disabled config options are:
- DOUBLEFAULT: are trapped by Xen and not virtualized
- HZ: defaults to 100 in Xen VMs

Hi,

this makes no real sense with dynamic ticks coming up... HZ=1000 should be perfectly fine then...

arch/i386/Kconfig source kernel/Kconfig.hz · line 753

Quoted 7 lines

config KEXEC

arch/i386/Kconfig +1 -1
 	bool "kexec system call (EXPERIMENTAL)"
-	depends on EXPERIMENTAL
+	depends on EXPERIMENTAL && !X86_XEN
 	help
 	  kexec is a system call that implements the ability to shutdown your
 	  current kernel, and to start another kernel.  It is like a reboot

hmmm why is kexec incompatible with xen? Don't you want to support crash dumps from guests?

drivers/xen/Kconfig line 20

Quoted 6 lines
drivers/xen/Kconfig new file +6 -0
+config XEN_SHADOW_MODE
+	bool
+	default y
+	help
+	  Fakes out a shadow mode kernel
+

this probably wants a better description...

#13 lore raw

Re: [RFC PATCH 04/33] Add XEN config options and disable unsupported config options.

Quoted 11 lines · Arjan van de Ven
Quoted 8 lines

config KEXEC

arch/i386/Kconfig +1 -1
 	bool "kexec system call (EXPERIMENTAL)"
-	depends on EXPERIMENTAL
+	depends on EXPERIMENTAL && !X86_XEN
 	help
 	  kexec is a system call that implements the ability to shutdown 

your
current kernel, and to start another kernel. It is like a reboot

hmmm why is kexec incompatible with xen? Don't you want to support crash dumps from guests?

There are kexec patches for Xen which haven't yet been merged into our tree. Even without that we can take coredumps of guests from the control VM, without the guest's active involvement.

-- Keir

#14 lore raw

RFC PATCH 5/33 Makefile support to build Xen subarch

In reply to Chris Wright

Use arch/i386/mach-xen when building Xen subarch. The separate subarchitecture allows us to hide details of interfacing with the hypervisor from i386 common code.

---
 arch/i386/Makefile          |    5 +++++
 arch/i386/mach-xen/Makefile |    7 +++++++
 2 files changed, 12 insertions(+)
arch/i386/Makefile +5 -0
diff -r 02d1f5dae39e arch/i386/Makefile
@@ -71,6 +71,10 @@ mflags-$(CONFIG_X86_SUMMIT) := -Iinclude
 mflags-$(CONFIG_X86_SUMMIT) := -Iinclude/asm-i386/mach-summit
 mcore-$(CONFIG_X86_SUMMIT)  := mach-default
 
+# Xen subarch support
+mflags-$(CONFIG_X86_XEN)	:= -Iinclude/asm-i386/mach-xen
+mcore-$(CONFIG_X86_XEN)		:= mach-xen
+
 # generic subarchitecture
 mflags-$(CONFIG_X86_GENERICARCH) := -Iinclude/asm-i386/mach-generic
 mcore-$(CONFIG_X86_GENERICARCH) := mach-default
@@ -99,6 +103,7 @@ drivers-$(CONFIG_PM)			+= arch/i386/powe
 
 CFLAGS += $(mflags-y)
 AFLAGS += $(mflags-y)
+CPPFLAGS += $(mflags-y)
 
 boot := arch/i386/boot
 
arch/i386/mach-xen/Makefile new file +7 -0
diff -r 02d1f5dae39e arch/i386/mach-xen/Makefile
@@ -0,0 +1,7 @@
+#
+# Makefile for the linux kernel.
+#
+
+obj-y				:= setup.o
+
+setup-y				:= ../mach-default/setup.o

--

#15 lore raw

Re: [RFC PATCH 05/33] Makefile support to build Xen subarch

Quoted 4 lines · Chris Wright

plain text document attachment (i386-mach-xen)
Use arch/i386/mach-xen when building Xen subarch. The separate subarchitecture allows us to hide details of interfacing with the hypervisor from i386 common code.

Hi,

please change the order of your patches next time, by sending the makefile and config options first you'll break git bisect...

Greetings,
Arjan van de Ven

#16 lore raw

Re: [RFC PATCH 05/33] Makefile support to build Xen subarch

Quoted 7 lines · On Tue, 18 Jul 2006 12:00:13 +0200 Arjan van de Ven <[email protected]> wrote:
Quoted 4 lines · Chris Wright

plain text document attachment (i386-mach-xen)
Use arch/i386/mach-xen when building Xen subarch. The separate subarchitecture allows us to hide details of interfacing with the hypervisor from i386 common code.

Quoted 5 lines

Hi,

please change the order of your patches next time, by sending the makefile and config options first you'll break git bisect...

No, this patch is OK (in that respect). As long as the git-bisect user sets CONFIG_XEN=n (and surely he will), things will compile.

That being said,

+mflags-$(CONFIG_X86_XEN) := -Iinclude/asm-i386/mach-xen
+mcore-$(CONFIG_X86_XEN) := mach-xen
+

 # generic subarchitecture
 mflags-$(CONFIG_X86_GENERICARCH) := -Iinclude/asm-i386/mach-generic
 mcore-$(CONFIG_X86_GENERICARCH) := mach-default
@@ -99,6 +103,7 @@ drivers-$(CONFIG_PM)			+= arch/i386/powe

CFLAGS += $(mflags-y)
AFLAGS += $(mflags-y)
+CPPFLAGS += $(mflags-y)

This change affects _all_ subarchitectures (by potentially altering their CPPFLAGS) and it's rather a mystery why one subarch needs the -Ifoo in its CPPFLAGS whereas all the others do not.

#17 lore raw

Re: [RFC PATCH 05/33] Makefile support to build Xen subarch

Quoted 10 lines · Andrew Morton
arch/i386/Makefile +1 -0
@@ -99,6 +103,7 @@ drivers-$(CONFIG_PM)			+= arch/i386/powe
 
 CFLAGS += $(mflags-y)
 AFLAGS += $(mflags-y)
+CPPFLAGS += $(mflags-y)
 

This change affects _all_ subarchitectures (by potentially altering their CPPFLAGS) and it's rather a mystery why one subarch needs the -Ifoo in its CPPFLAGS whereas all the others do not.

The reason is that arch-i386/kernel/vmlinux.lds.S is run through CPP, and it includes asm/thread_info.h and asm/page.h, which end up including "mach_page.h" (which this patch series introduces). There is a version in both mach-default/mach_page.h and mach-xen/mach_page.h, so the -I is necessary for non-Xen sub-arches as well.

I guess the conservative approach would be to only add this -I for the vmlinux.lds target, assuming there are no later compile problems. On the flip-side, would you want C and Asm code getting a different set of includes from "manually" preprocessed-files? I would think you'd want either defines/includes at all, or to have them identical.

The CPPFLAGS assignment also appears to make the previous two lines redundant, since a_flags and c_flags (which is what actually gets used for compilation) end up having CPPFLAGS incorporated into them.

J

#18 lore raw

Re: [RFC PATCH 05/33] Makefile support to build Xen subarch

In reply to Arjan van de Ven
Quoted 3 lines · Arjan van de Ven

please change the order of your patches next time, by sending the makefile and config options first you'll break git bisect...

I think Chris was planning on doing this, but I suspect it got put to one side in order to get the patch set out in time for KS.

J

#19 lore raw

RFC PATCH 6/33 Add Xen interface header files

In reply to Chris Wright

Add Xen interface header files. These are taken fairly directly from the Xen tree and hence the style is not entirely in accordance with Linux guidelines. There is a tension between fitting with Linux coding rules and ease of maintenance.

---
 include/xen/interface/arch-x86_32.h   |  187 ++++++++++++++
 include/xen/interface/event_channel.h |  195 +++++++++++++++
 include/xen/interface/features.h      |   43 +++
 include/xen/interface/grant_table.h   |  301 +++++++++++++++++++++++
 include/xen/interface/io/blkif.h      |   75 +++++
 include/xen/interface/io/console.h    |   23 +
 include/xen/interface/io/netif.h      |   74 +++++
 include/xen/interface/io/ring.h       |  252 +++++++++++++++++++
 include/xen/interface/io/xenbus.h     |   42 +++
 include/xen/interface/io/xs_wire.h    |   87 ++++++
 include/xen/interface/memory.h        |  145 +++++++++++
 include/xen/interface/physdev.h       |   61 ++++
 include/xen/interface/sched.h         |   77 ++++++
 include/xen/interface/vcpu.h          |  109 ++++++++
 include/xen/interface/version.h       |   60 ++++
 include/xen/interface/xen.h           |  431 ++++++++++++++++++++++++++++++++++
 16 files changed, 2162 insertions(+)
include/xen/interface/arch-x86_32.h new file +187 -0
diff -r 116580580453 include/xen/interface/arch-x86_32.h
@@ -0,0 +1,187 @@
+/******************************************************************************
+ * arch-x86_32.h
+ * 
+ * Guest OS interface to x86 32-bit Xen.
+ * 
+ * Copyright (c) 2004, K A Fraser
+ */
+
+#ifndef __XEN_PUBLIC_ARCH_X86_32_H__
+#define __XEN_PUBLIC_ARCH_X86_32_H__
+
+#ifdef __XEN__
+#define __DEFINE_GUEST_HANDLE(name, type) \
+    typedef struct { type *p; } __guest_handle_ ## name
+#else
+#define __DEFINE_GUEST_HANDLE(name, type) \
+    typedef type * __guest_handle_ ## name
+#endif
+
+#define DEFINE_GUEST_HANDLE_STRUCT(name) \
+	__DEFINE_GUEST_HANDLE(name, struct name)
+#define DEFINE_GUEST_HANDLE(name) __DEFINE_GUEST_HANDLE(name, name)
+#define GUEST_HANDLE(name)        __guest_handle_ ## name
+
+#ifndef __ASSEMBLY__
+/* Guest handles for primitive C types. */
+__DEFINE_GUEST_HANDLE(uchar, unsigned char);
+__DEFINE_GUEST_HANDLE(uint,  unsigned int);
+__DEFINE_GUEST_HANDLE(ulong, unsigned long);
+DEFINE_GUEST_HANDLE(char);
+DEFINE_GUEST_HANDLE(int);
+DEFINE_GUEST_HANDLE(long);
+DEFINE_GUEST_HANDLE(void);
+#endif
+
+/*
+ * SEGMENT DESCRIPTOR TABLES
+ */
+/*
+ * A number of GDT entries are reserved by Xen. These are not situated at the
+ * start of the GDT because some stupid OSes export hard-coded selector values
+ * in their ABI. These hard-coded values are always near the start of the GDT,
+ * so Xen places itself out of the way, at the far end of the GDT.
+ */
+#define FIRST_RESERVED_GDT_PAGE  14
+#define FIRST_RESERVED_GDT_BYTE  (FIRST_RESERVED_GDT_PAGE * 4096)
+#define FIRST_RESERVED_GDT_ENTRY (FIRST_RESERVED_GDT_BYTE / 8)
+
+/*
+ * These flat segments are in the Xen-private section of every GDT. Since these
+ * are also present in the initial GDT, many OSes will be able to avoid
+ * installing their own GDT.
+ */
+#define FLAT_RING1_CS 0xe019    /* GDT index 259 */
+#define FLAT_RING1_DS 0xe021    /* GDT index 260 */
+#define FLAT_RING1_SS 0xe021    /* GDT index 260 */
+#define FLAT_RING3_CS 0xe02b    /* GDT index 261 */
+#define FLAT_RING3_DS 0xe033    /* GDT index 262 */
+#define FLAT_RING3_SS 0xe033    /* GDT index 262 */
+
+#define FLAT_KERNEL_CS FLAT_RING1_CS
+#define FLAT_KERNEL_DS FLAT_RING1_DS
+#define FLAT_KERNEL_SS FLAT_RING1_SS
+#define FLAT_USER_CS    FLAT_RING3_CS
+#define FLAT_USER_DS    FLAT_RING3_DS
+#define FLAT_USER_SS    FLAT_RING3_SS
+
+/* And the trap vector is... */
+#define TRAP_INSTR "int $0x82"
+
+/*
+ * Virtual addresses beyond this are not modifiable by guest OSes. The 
+ * machine->physical mapping table starts at this address, read-only.
+ */
+#ifdef CONFIG_X86_PAE
+#define __HYPERVISOR_VIRT_START 0xF5800000
+#else
+#define __HYPERVISOR_VIRT_START 0xFC000000
+#endif
+
+#ifndef HYPERVISOR_VIRT_START
+#define HYPERVISOR_VIRT_START mk_unsigned_long(__HYPERVISOR_VIRT_START)
+#endif
+
+#ifndef machine_to_phys_mapping
+#define machine_to_phys_mapping ((unsigned long *)HYPERVISOR_VIRT_START)
+#endif
+
+/* Maximum number of virtual CPUs in multi-processor guests. */
+#define MAX_VIRT_CPUS 32
+
+#ifndef __ASSEMBLY__
+
+/*
+ * Send an array of these to HYPERVISOR_set_trap_table()
+ */
+#define TI_GET_DPL(_ti)      ((_ti)->flags & 3)
+#define TI_GET_IF(_ti)       ((_ti)->flags & 4)
+#define TI_SET_DPL(_ti,_dpl) ((_ti)->flags |= (_dpl))
+#define TI_SET_IF(_ti,_if)   ((_ti)->flags |= ((!!(_if))<<2))
+struct trap_info {
+    uint8_t       vector;  /* exception vector                              */
+    uint8_t       flags;   /* 0-3: privilege level; 4: clear event enable?  */
+    uint16_t      cs;      /* code selector                                 */
+    unsigned long address; /* code offset                                   */
+};
+DEFINE_GUEST_HANDLE_STRUCT(trap_info);
+
+struct cpu_user_regs {
+    uint32_t ebx;
+    uint32_t ecx;
+    uint32_t edx;
+    uint32_t esi;
+    uint32_t edi;
+    uint32_t ebp;
+    uint32_t eax;
+    uint16_t error_code;    /* private */
+    uint16_t entry_vector;  /* private */
+    uint32_t eip;
+    uint16_t cs;
+    uint8_t  saved_upcall_mask;
+    uint8_t  _pad0;
+    uint32_t eflags;        /* eflags.IF == !saved_upcall_mask */
+    uint32_t esp;
+    uint16_t ss, _pad1;
+    uint16_t es, _pad2;
+    uint16_t ds, _pad3;
+    uint16_t fs, _pad4;
+    uint16_t gs, _pad5;
+};
+DEFINE_GUEST_HANDLE_STRUCT(cpu_user_regs);
+
+typedef uint64_t tsc_timestamp_t; /* RDTSC timestamp */
+
+/*
+ * The following is all CPU context. Note that the fpu_ctxt block is filled 
+ * in by FXSAVE if the CPU has feature FXSR; otherwise FSAVE is used.
+ */
+struct vcpu_guest_context {
+    /* FPU registers come first so they can be aligned for FXSAVE/FXRSTOR. */
+    struct { char x[512]; } fpu_ctxt;       /* User-level FPU registers     */
+#define VGCF_I387_VALID (1<<0)
+#define VGCF_HVM_GUEST  (1<<1)
+#define VGCF_IN_KERNEL  (1<<2)
+    unsigned long flags;                    /* VGCF_* flags                 */
+    struct cpu_user_regs user_regs;         /* User-level CPU registers     */
+    struct trap_info trap_ctxt[256];        /* Virtual IDT                  */
+    unsigned long ldt_base, ldt_ents;       /* LDT (linear address, # ents) */
+    unsigned long gdt_frames[16], gdt_ents; /* GDT (machine frames, # ents) */
+    unsigned long kernel_ss, kernel_sp;     /* Virtual TSS (only SS1/SP1)   */
+    unsigned long ctrlreg[8];               /* CR0-CR7 (control registers)  */
+    unsigned long debugreg[8];              /* DB0-DB7 (debug registers)    */
+    unsigned long event_callback_cs;        /* CS:EIP of event callback     */
+    unsigned long event_callback_eip;
+    unsigned long failsafe_callback_cs;     /* CS:EIP of failsafe callback  */
+    unsigned long failsafe_callback_eip;
+    unsigned long vm_assist;                /* VMASST_TYPE_* bitmap */
+};
+DEFINE_GUEST_HANDLE_STRUCT(vcpu_guest_context);
+
+struct arch_shared_info {
+    unsigned long max_pfn;                  /* max pfn that appears in table */
+    /* Frame containing list of mfns containing list of mfns containing p2m. */
+    unsigned long pfn_to_mfn_frame_list_list;
+    unsigned long nmi_reason;
+};
+
+struct arch_vcpu_info {
+    unsigned long cr2;
+    unsigned long pad[5]; /* sizeof(struct vcpu_info) == 64 */
+};
+
+#endif /* !__ASSEMBLY__ */
+
+/*
+ * Prefix forces emulation of some non-trapping instructions.
+ * Currently only CPUID.
+ */
+#ifdef __ASSEMBLY__
+#define XEN_EMULATE_PREFIX .byte 0x0f,0x0b,0x78,0x65,0x6e ;
+#define XEN_CPUID          XEN_EMULATE_PREFIX cpuid
+#else
+#define XEN_EMULATE_PREFIX ".byte 0x0f,0x0b,0x78,0x65,0x6e ; "
+#define XEN_CPUID          XEN_EMULATE_PREFIX "cpuid"
+#endif
+
+#endif
include/xen/interface/event_channel.h new file +195 -0

195 lines not on this page · raw message

include/xen/interface/features.h new file +43 -0

43 lines not on this page · raw message

include/xen/interface/grant_table.h new file +301 -0

301 lines not on this page · raw message

include/xen/interface/io/blkif.h new file +75 -0

75 lines not on this page · raw message

include/xen/interface/io/console.h new file +23 -0
diff -r 116580580453 include/xen/interface/io/console.h
@@ -0,0 +1,23 @@
+/******************************************************************************
+ * console.h
+ * 
+ * Console I/O interface for Xen guest OSes.
+ * 
+ * Copyright (c) 2005, Keir Fraser
+ */
+
+#ifndef __XEN_PUBLIC_IO_CONSOLE_H__
+#define __XEN_PUBLIC_IO_CONSOLE_H__
+
+typedef uint32_t XENCONS_RING_IDX;
+
+#define MASK_XENCONS_IDX(idx, ring) ((idx) & (sizeof(ring)-1))
+
+struct xencons_interface {
+    char in[1024];
+    char out[2048];
+    XENCONS_RING_IDX in_cons, in_prod;
+    XENCONS_RING_IDX out_cons, out_prod;
+};
+
+#endif /* __XEN_PUBLIC_IO_CONSOLE_H__ */
include/xen/interface/io/netif.h new file +78 -0

78 lines not on this page · raw message

include/xen/interface/io/ring.h new file +260 -0

260 lines not on this page · raw message

include/xen/interface/io/xenbus.h new file +42 -0

42 lines not on this page · raw message

include/xen/interface/io/xs_wire.h new file +87 -0

87 lines not on this page · raw message

include/xen/interface/memory.h new file +145 -0

145 lines not on this page · raw message

include/xen/interface/physdev.h new file +61 -0

61 lines not on this page · raw message

include/xen/interface/sched.h new file +77 -0

77 lines not on this page · raw message

include/xen/interface/vcpu.h new file +109 -0

109 lines not on this page · raw message

include/xen/interface/version.h new file +60 -0

60 lines not on this page · raw message

include/xen/interface/xen.h new file +431 -0

431 lines not on this page · raw message

--

#20 lore raw

RFC PATCH 7/33 Hypervisor interface header files.

In reply to Chris Wright

Define macros and inline functions for doing hypercalls into the hypervisor.

---
 include/asm-i386/hypercall.h  |  309 ++++++++++++++++++++++++++++++++++++++++++
 include/asm-i386/hypervisor.h |   70 +++++++++
 2 files changed, 379 insertions(+)
include/asm-i386/hypercall.h new file +309 -0
diff -r 56221ea961f7 include/asm-i386/hypercall.h
@@ -0,0 +1,309 @@
+/******************************************************************************
+ * hypercall.h
+ *
+ * Linux-specific hypervisor handling.
+ *
+ * Copyright (c) 2002-2004, K A Fraser
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation; or, when distributed
+ * separately from the Linux kernel or incorporated into other
+ * software packages, subject to the following license:
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this source file (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use, copy, modify,
+ * merge, publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#ifndef __HYPERCALL_H__
+#define __HYPERCALL_H__
+
+#include <linux/config.h>
+
+#include <xen/interface/xen.h>
+#include <xen/interface/sched.h>
+
+#define __STR(x) #x
+#define STR(x) __STR(x)
+
+#define _hypercall0(type, name)			\
+({						\
+	long __res;				\
+	asm volatile (				\
+		"call hypercall_page + ("STR(__HYPERVISOR_##name)" * 32)"\
+		: "=a" (__res)			\
+		:				\
+		: "memory" );			\
+	(type)__res;				\
+})
+
+#define _hypercall1(type, name, a1)				\
+({								\
+	long __res, __ign1;					\
+	asm volatile (						\
+		"call hypercall_page + ("STR(__HYPERVISOR_##name)" * 32)"\
+		: "=a" (__res), "=b" (__ign1)			\
+		: "1" ((long)(a1))				\
+		: "memory" );					\
+	(type)__res;						\
+})
+
+#define _hypercall2(type, name, a1, a2)				\
+({								\
+	long __res, __ign1, __ign2;				\
+	asm volatile (						\
+		"call hypercall_page + ("STR(__HYPERVISOR_##name)" * 32)"\
+		: "=a" (__res), "=b" (__ign1), "=c" (__ign2)	\
+		: "1" ((long)(a1)), "2" ((long)(a2))		\
+		: "memory" );					\
+	(type)__res;						\
+})
+
+#define _hypercall3(type, name, a1, a2, a3)			\
+({								\
+	long __res, __ign1, __ign2, __ign3;			\
+	asm volatile (						\
+		"call hypercall_page + ("STR(__HYPERVISOR_##name)" * 32)"\
+		: "=a" (__res), "=b" (__ign1), "=c" (__ign2), 	\
+		"=d" (__ign3)					\
+		: "1" ((long)(a1)), "2" ((long)(a2)),		\
+		"3" ((long)(a3))				\
+		: "memory" );					\
+	(type)__res;						\
+})
+
+#define _hypercall4(type, name, a1, a2, a3, a4)			\
+({								\
+	long __res, __ign1, __ign2, __ign3, __ign4;		\
+	asm volatile (						\
+		"call hypercall_page + ("STR(__HYPERVISOR_##name)" * 32)"\
+		: "=a" (__res), "=b" (__ign1), "=c" (__ign2),	\
+		"=d" (__ign3), "=S" (__ign4)			\
+		: "1" ((long)(a1)), "2" ((long)(a2)),		\
+		"3" ((long)(a3)), "4" ((long)(a4))		\
+		: "memory" );					\
+	(type)__res;						\
+})
+
+#define _hypercall5(type, name, a1, a2, a3, a4, a5)		\
+({								\
+	long __res, __ign1, __ign2, __ign3, __ign4, __ign5;	\
+	asm volatile (						\
+		"call hypercall_page + ("STR(__HYPERVISOR_##name)" * 32)"\
+		: "=a" (__res), "=b" (__ign1), "=c" (__ign2),	\
+		"=d" (__ign3), "=S" (__ign4), "=D" (__ign5)	\
+		: "1" ((long)(a1)), "2" ((long)(a2)),		\
+		"3" ((long)(a3)), "4" ((long)(a4)),		\
+		"5" ((long)(a5))				\
+		: "memory" );					\
+	(type)__res;						\
+})
+
+static inline int
+HYPERVISOR_set_trap_table(
+	struct trap_info *table)
+{
+	return _hypercall1(int, set_trap_table, table);
+}
+
+static inline int
+HYPERVISOR_mmu_update(
+	struct mmu_update *req, int count, int *success_count, domid_t domid)
+{
+	return _hypercall4(int, mmu_update, req, count, success_count, domid);
+}
+
+static inline int
+HYPERVISOR_mmuext_op(
+	struct mmuext_op *op, int count, int *success_count, domid_t domid)
+{
+	return _hypercall4(int, mmuext_op, op, count, success_count, domid);
+}
+
+static inline int
+HYPERVISOR_set_gdt(
+	unsigned long *frame_list, int entries)
+{
+	return _hypercall2(int, set_gdt, frame_list, entries);
+}
+
+static inline int
+HYPERVISOR_stack_switch(
+	unsigned long ss, unsigned long esp)
+{
+	return _hypercall2(int, stack_switch, ss, esp);
+}
+
+static inline int
+HYPERVISOR_set_callbacks(
+	unsigned long event_selector, unsigned long event_address,
+	unsigned long failsafe_selector, unsigned long failsafe_address)
+{
+	return _hypercall4(int, set_callbacks,
+			   event_selector, event_address,
+			   failsafe_selector, failsafe_address);
+}
+
+static inline int
+HYPERVISOR_fpu_taskswitch(
+	int set)
+{
+	return _hypercall1(int, fpu_taskswitch, set);
+}
+
+static inline int
+HYPERVISOR_sched_op(
+	int cmd, unsigned long arg)
+{
+	return _hypercall2(int, sched_op, cmd, arg);
+}
+
+static inline long
+HYPERVISOR_set_timer_op(
+	u64 timeout)
+{
+	unsigned long timeout_hi = (unsigned long)(timeout>>32);
+	unsigned long timeout_lo = (unsigned long)timeout;
+	return _hypercall2(long, set_timer_op, timeout_lo, timeout_hi);
+}
+
+static inline int
+HYPERVISOR_set_debugreg(
+	int reg, unsigned long value)
+{
+	return _hypercall2(int, set_debugreg, reg, value);
+}
+
+static inline unsigned long
+HYPERVISOR_get_debugreg(
+	int reg)
+{
+	return _hypercall1(unsigned long, get_debugreg, reg);
+}
+
+static inline int
+HYPERVISOR_update_descriptor(
+	u64 ma, u64 desc)
+{
+	return _hypercall4(int, update_descriptor, ma, ma>>32, desc, desc>>32);
+}
+
+static inline int
+HYPERVISOR_memory_op(
+	unsigned int cmd, void *arg)
+{
+	return _hypercall2(int, memory_op, cmd, arg);
+}
+
+static inline int
+HYPERVISOR_multicall(
+	void *call_list, int nr_calls)
+{
+	return _hypercall2(int, multicall, call_list, nr_calls);
+}
+
+static inline int
+HYPERVISOR_update_va_mapping(
+	unsigned long va, pte_t new_val, unsigned long flags)
+{
+	unsigned long pte_hi = 0;
+#ifdef CONFIG_X86_PAE
+	pte_hi = new_val.pte_high;
+#endif
+	return _hypercall4(int, update_va_mapping, va,
+			   new_val.pte_low, pte_hi, flags);
+}
+
+static inline int
+HYPERVISOR_event_channel_op(
+	void *op)
+{
+	return _hypercall1(int, event_channel_op, op);
+}
+
+static inline int
+HYPERVISOR_xen_version(
+	int cmd, void *arg)
+{
+	return _hypercall2(int, xen_version, cmd, arg);
+}
+
+static inline int
+HYPERVISOR_console_io(
+	int cmd, int count, char *str)
+{
+	return _hypercall3(int, console_io, cmd, count, str);
+}
+
+static inline int
+HYPERVISOR_physdev_op(
+	void *physdev_op)
+{
+	return _hypercall1(int, physdev_op, physdev_op);
+}
+
+static inline int
+HYPERVISOR_grant_table_op(
+	unsigned int cmd, void *uop, unsigned int count)
+{
+	return _hypercall3(int, grant_table_op, cmd, uop, count);
+}
+
+static inline int
+HYPERVISOR_update_va_mapping_otherdomain(
+	unsigned long va, pte_t new_val, unsigned long flags, domid_t domid)
+{
+	unsigned long pte_hi = 0;
+#ifdef CONFIG_X86_PAE
+	pte_hi = new_val.pte_high;
+#endif
+	return _hypercall5(int, update_va_mapping_otherdomain, va,
+			   new_val.pte_low, pte_hi, flags, domid);
+}
+
+static inline int
+HYPERVISOR_vm_assist(
+	unsigned int cmd, unsigned int type)
+{
+	return _hypercall2(int, vm_assist, cmd, type);
+}
+
+static inline int
+HYPERVISOR_vcpu_op(
+	int cmd, int vcpuid, void *extra_args)
+{
+	return _hypercall3(int, vcpu_op, cmd, vcpuid, extra_args);
+}
+
+static inline int
+HYPERVISOR_suspend(
+	unsigned long srec)
+{
+	return _hypercall3(int, sched_op, SCHEDOP_shutdown,
+			   SHUTDOWN_suspend, srec);
+}
+
+static inline int
+HYPERVISOR_nmi_op(
+	unsigned long op,
+	unsigned long arg)
+{
+	return _hypercall2(int, nmi_op, op, arg);
+}
+
+#endif /* __HYPERCALL_H__ */
include/asm-i386/hypervisor.h new file +69 -0

69 lines not on this page · raw message

--

#21 lore raw

RFC PATCH 8/33 Add vmlinuz build target.

In reply to Chris Wright

The vmlinuz image is a stripped and compressed kernel image, it is smaller than the vmlinux image and the Xen domain builder supports loading compressed images directly.

---
 arch/i386/Makefile      |    5 +++--
 arch/i386/boot/Makefile |   10 +++++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)
arch/i386/Makefile +3 -2
diff -r 61d7f9ade4e3 arch/i386/Makefile
@@ -108,15 +108,16 @@ boot := arch/i386/boot
 boot := arch/i386/boot
 
 PHONY += zImage bzImage compressed zlilo bzlilo \
-         zdisk bzdisk fdimage fdimage144 fdimage288 isoimage install
+         zdisk bzdisk fdimage fdimage144 fdimage288 isoimage install vmlinuz
 
 all: bzImage
 
 # KBUILD_IMAGE specify target image being built
                     KBUILD_IMAGE := $(boot)/bzImage
 zImage zlilo zdisk: KBUILD_IMAGE := arch/i386/boot/zImage
+vmlinuz:            KBUILD_IMAGE := $(boot)/vmlinuz
 
-zImage bzImage: vmlinux
+zImage bzImage vmlinuz: vmlinux
 	$(Q)$(MAKE) $(build)=$(boot) $(KBUILD_IMAGE)
 
 compressed: zImage
arch/i386/boot/Makefile +9 -1
diff -r 61d7f9ade4e3 arch/i386/boot/Makefile
@@ -26,7 +26,7 @@ SVGA_MODE := -DSVGA_MODE=NORMAL_VGA
 #RAMDISK := -DRAMDISK=512
 
 targets		:= vmlinux.bin bootsect bootsect.o \
-		   setup setup.o zImage bzImage
+		   setup setup.o zImage bzImage vmlinuz
 subdir- 	:= compressed
 
 hostprogs-y	:= tools/build
@@ -128,5 +128,13 @@ zlilo: $(BOOTIMAGE)
 	cp System.map $(INSTALL_PATH)/
 	if [ -x /sbin/lilo ]; then /sbin/lilo; else /etc/lilo/install; fi
 
+$(obj)/vmlinuz: $(obj)/vmlinux-stripped FORCE
+	$(call if_changed,gzip)
+	@echo 'Kernel: $@ is ready' ' (#'`cat .version`')'
+
+$(obj)/vmlinux-stripped: OBJCOPYFLAGS := -g --strip-unneeded
+$(obj)/vmlinux-stripped: vmlinux FORCE
+	$(call if_changed,objcopy)
+
 install:
 	sh $(srctree)/$(src)/install.sh $(KERNELRELEASE) $(BOOTIMAGE) System.map "$(INSTALL_PATH)"

--

#22 lore raw

RFC PATCH 9/33 Add start-of-day setup hooks to subarch

In reply to Chris Wright

Implement the start-of-day subarchitecture setup hooks for booting on Xen. Add subarch macros for determining loader type and initrd location.

---
 arch/i386/kernel/setup.c                   |    2 +
 arch/i386/mach-default/Makefile            |    2 -
 arch/i386/mach-default/setup-memory.c      |   43 ++++++++++++++++++++++
 arch/i386/mach-default/setup.c             |   43 ----------------------
 arch/i386/mach-xen/Makefile                |    2 -
 arch/i386/mach-xen/setup-xen.c             |   54 ++++++++++++++++++++++++++++
 include/asm-i386/hypervisor.h              |    4 ++
 include/asm-i386/mach-default/mach_setup.h |   12 ++++++
 include/asm-i386/mach-xen/mach_setup.h     |   15 +++++++
 include/asm-i386/mach-xen/setup_arch.h     |    9 ++++
 include/asm-i386/setup.h                   |   20 ++++++----
 11 files changed, 153 insertions(+), 53 deletions(-)
arch/i386/kernel/setup.c +2 -0
diff -r a5848bce3730 arch/i386/kernel/setup.c
@@ -458,6 +458,7 @@ static void __init print_memory_map(char
 	}
 }
 
+#ifndef HAVE_ARCH_E820_SANITIZE
 /*
  * Sanitize the BIOS e820 map.
  *
@@ -677,6 +678,7 @@ int __init copy_e820_map(struct e820entr
 	} while (biosmap++,--nr_map);
 	return 0;
 }
+#endif
 
 #if defined(CONFIG_EDD) || defined(CONFIG_EDD_MODULE)
 struct edd edd;
arch/i386/mach-default/Makefile +1 -1
diff -r a5848bce3730 arch/i386/mach-default/Makefile
@@ -2,4 +2,4 @@
 # Makefile for the linux kernel.
 #
 
-obj-y				:= setup.o
+obj-y				:= setup.o setup-memory.o
arch/i386/mach-default/setup.c +0 -43

52 lines not on this page · raw message

arch/i386/mach-xen/Makefile +1 -1
diff -r a5848bce3730 arch/i386/mach-xen/Makefile
@@ -2,6 +2,6 @@
 # Makefile for the linux kernel.
 #
 
-obj-y				:= setup.o
+obj-y				:= setup.o setup-xen.o
 
 setup-y				:= ../mach-default/setup.o
include/asm-i386/hypervisor.h +4 -0
diff -r a5848bce3730 include/asm-i386/hypervisor.h
@@ -55,6 +55,10 @@ extern struct shared_info *HYPERVISOR_sh
 extern struct shared_info *HYPERVISOR_shared_info;
 extern struct start_info *xen_start_info;
 
+/* arch/i386/mach-xen/entry.S */
+extern void hypervisor_callback(void);
+extern void failsafe_callback(void);
+
 /* arch/i386/mach-xen/evtchn.c */
 /* Force a proper event-channel callback from Xen. */
 extern void force_evtchn_callback(void);
include/asm-i386/setup.h +12 -8
diff -r a5848bce3730 include/asm-i386/setup.h
@@ -49,10 +49,10 @@ extern unsigned char boot_params[PARAM_S
 #define VIDEO_MODE (*(unsigned short *) (PARAM+0x1FA))
 #define ORIG_ROOT_DEV (*(unsigned short *) (PARAM+0x1FC))
 #define AUX_DEVICE_INFO (*(unsigned char *) (PARAM+0x1FF))
-#define LOADER_TYPE (*(unsigned char *) (PARAM+0x210))
+#define LOADER_TYPE MACH_LOADER_TYPE
 #define KERNEL_START (*(unsigned long *) (PARAM+0x214))
-#define INITRD_START (*(unsigned long *) (PARAM+0x218))
-#define INITRD_SIZE (*(unsigned long *) (PARAM+0x21c))
+#define INITRD_START MACH_INITRD_START
+#define INITRD_SIZE MACH_INITRD_SIZE
 #define EDID_INFO   (*(struct edid_info *) (PARAM+0x140))
 #define EDD_NR     (*(unsigned char *) (PARAM+EDDNR))
 #define EDD_MBR_SIG_NR (*(unsigned char *) (PARAM+EDD_MBR_SIG_NR_BUF))
@@ -65,15 +65,19 @@ extern unsigned char boot_params[PARAM_S
  */
 #define LOWMEMSIZE()	(0x9f000)
 
+extern unsigned long init_pg_tables_end;
+
 struct e820entry;
 
-char * __init machine_specific_memory_setup(void);
+char * machine_specific_memory_setup(void);
 
-int __init copy_e820_map(struct e820entry * biosmap, int nr_map);
-int __init sanitize_e820_map(struct e820entry * biosmap, char * pnr_map);
-void __init add_memory_region(unsigned long long start,
-			      unsigned long long size, int type);
+int copy_e820_map(struct e820entry * biosmap, int nr_map);
+int sanitize_e820_map(struct e820entry * biosmap, char * pnr_map);
+void add_memory_region(unsigned long long start,
+		       unsigned long long size, int type);
 
 #endif /* __ASSEMBLY__ */
 
+#include <mach_setup.h>
+
 #endif /* _i386_SETUP_H */
arch/i386/mach-default/setup-memory.c new file +44 -0

44 lines not on this page · raw message

arch/i386/mach-xen/setup-xen.c new file +54 -0

54 lines not on this page · raw message

include/asm-i386/mach-default/mach_setup.h new file +12 -0
diff -r a5848bce3730 include/asm-i386/mach-default/mach_setup.h
@@ -0,0 +1,12 @@
+#ifndef __ASM_MACH_SETUP_H
+#define __ASM_MACH_SETUP_H
+
+#ifndef __ASSEMBLY__
+
+#define MACH_LOADER_TYPE (*(unsigned char *) (PARAM+0x210))
+#define MACH_INITRD_START (*(unsigned long *) (PARAM+0x218))
+#define MACH_INITRD_SIZE (*(unsigned long *) (PARAM+0x21c))
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* __ASM_MACH_SETUP_H */
include/asm-i386/mach-xen/mach_setup.h new file +15 -0
diff -r a5848bce3730 include/asm-i386/mach-xen/mach_setup.h
@@ -0,0 +1,15 @@
+#ifndef __ASM_MACH_SETUP_H
+#define __ASM_MACH_SETUP_H
+
+#ifndef __ASSEMBLY__
+
+#include <xen/interface/xen.h>
+
+#define MACH_LOADER_TYPE 0x6e6558	/* "Xen" */
+#define MACH_INITRD_START \
+	(xen_start_info->mod_start ? __pa(xen_start_info->mod_start) : 0)
+#define MACH_INITRD_SIZE (xen_start_info->mod_len)
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* __ASM_MACH_SETUP_H */
include/asm-i386/mach-xen/setup_arch.h new file +9 -0
diff -r a5848bce3730 include/asm-i386/mach-xen/setup_arch.h
@@ -0,0 +1,9 @@
+
+#define ARCH_SETUP machine_specific_arch_setup();
+
+extern struct start_info *xen_start_info;
+extern struct shared_info *HYPERVISOR_shared_info;
+
+void __init machine_specific_arch_setup(void);
+
+#define HAVE_ARCH_E820_SANITIZE

--

#23 lore raw

Re: [RFC PATCH 09/33] Add start-of-day setup hooks to subarch

Quoted 17 lines · Chris Wright

plain text document attachment (i386-setup)
Implement the start-of-day subarchitecture setup hooks for booting on Xen. Add subarch macros for determining loader type and initrd location.

diff -r a5848bce3730 arch/i386/kernel/setup.c

arch/i386/kernel/setup.c +1 -0
@@ -458,6 +458,7 @@ static void __init print_memory_map(char
 	}
 }
 
+#ifndef HAVE_ARCH_E820_SANITIZE
 /*
  * Sanitize the BIOS e820 map.
  *

arch/i386/kernel/setup.c int __init copy_e820_map(struct e820entr · line 682

Quoted 5 lines
arch/i386/kernel/setup.c +1 -0
@@ -677,6 +678,7 @@ int __init copy_e820_map(struct e820entr
 	} while (biosmap++,--nr_map);
 	return 0;
 }
+#endif
 

Hi,

what is this for? Isn't this 1) undocumented and 2) unclear and 3) ugly ? (I'm pretty sure the HAVE_ARCH_* stuff is highly deprecated for new things nowadays)

Greetings,
Arjan van de Ven

#24 lore raw

Re: [RFC PATCH 09/33] Add start-of-day setup hooks to subarch

Quoted 4 lines · Arjan van de Ven

what is this for? Isn't this 1) undocumented and 2) unclear and 3) ugly ? (I'm pretty sure the HAVE_ARCH_* stuff is highly deprecated for new things nowadays)

It appears to be completely unnecessary. Xen doesn't use sanitize_e820_map(), but there's no obvious harm in leaving it there (and it's init code, so there's no long-term cost).

J

#25 lore raw

Re: [RFC PATCH 09/33] Add start-of-day setup hooks to subarch

In reply to Chris Wright

arch/i386/mach-xen/setup-xen.c line 18

Quoted 6 lines · Chris Wright

...

arch/i386/mach-xen/setup-xen.c new file +2 -0
...
+struct start_info *xen_start_info;
+EXPORT_SYMBOL(xen_start_info);

EXPORT_SYMBOL_GPL?

arch/i386/mach-xen/setup-xen.c line 25

Quoted 7 lines
arch/i386/mach-xen/setup-xen.c new file +6 -0
+/*
+ * Point at the empty zero page to start with. We map the real shared_info
+ * page as soon as fixmap is up and running.
+ */
+struct shared_info *HYPERVISOR_shared_info = (struct shared_info *)empty_zero_page;
+EXPORT_SYMBOL(HYPERVISOR_shared_info);
...

EXPORT_SYMBOL_GPL?

cu
Adrian

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed
#26 lore raw

Re: [RFC PATCH 09/33] Add start-of-day setup hooks to subarch

Quoted 4 lines · Adrian Bunk

arch/i386/mach-xen/setup-xen.c line 18

Quoted 2 lines
arch/i386/mach-xen/setup-xen.c new file +2 -0
+struct start_info *xen_start_info;
+EXPORT_SYMBOL(xen_start_info);

EXPORT_SYMBOL_GPL?

Possibly.

Quoted 11 lines
Quoted 9 lines

+/*
+ * Point at the empty zero page to start with. We map the real shared_info
+ * page as soon as fixmap is up and running.
+ */
+struct shared_info *HYPERVISOR_shared_info = (struct shared_info *)empty_zero_page; +EXPORT_SYMBOL(HYPERVISOR_shared_info);
...

EXPORT_SYMBOL_GPL?

Interrupt-control macros (local_irq_enable/disable and friends) used this symbol, so GPLing it made non-GPL modules fail. We made a bunch of the macros proper functions so this may no longer be the case.

-- Keir

#27 lore raw

Re: [RFC PATCH 09/33] Add start-of-day setup hooks to subarch

Quoted 8 lines · Keir Fraser
Quoted 6 lines
Quoted 4 lines

+struct shared_info *HYPERVISOR_shared_info = (struct shared_info *)empty_zero_page; +EXPORT_SYMBOL(HYPERVISOR_shared_info);
...

EXPORT_SYMBOL_GPL?

Interrupt-control macros (local_irq_enable/disable and friends) used this symbol, so GPLing it made non-GPL modules fail. We made a bunch of the macros proper functions so this may no longer be the case.

Given that this is just a page exported from the hypervisor, it doesn't make much sense to impose a GPL requirement on this symbol (to put it another way, this is more part of the Xen ABI than the Linux API, so Linux can't make much claim to it).

J

#28 lore raw

RFC PATCH 10/33 add support for Xen feature queries

In reply to Chris Wright

Add support for parsing and interpreting hypervisor feature flags. These allow the kernel to determine what features are provided by the underlying hypervisor. For example, whether page tables need to be write protected explicitly by the kernel, and whether the kernel (appears to) run in ring 0 rather than ring 1. This information allows the kernel to improve performance by avoiding unnecessary actions.

---
 arch/i386/mach-xen/setup-xen.c |    2 ++
 drivers/Makefile               |    1 +
 drivers/xen/Makefile           |    3 +++
 drivers/xen/core/Makefile      |    3 +++
 drivers/xen/core/features.c    |   31 +++++++++++++++++++++++++++++++
 include/asm-i386/hypervisor.h  |    1 +
 include/xen/features.h         |   20 ++++++++++++++++++++
 7 files changed, 61 insertions(+)
arch/i386/mach-xen/setup-xen.c +2 -0
diff -r 1f0f17cb7700 arch/i386/mach-xen/setup-xen.c
@@ -40,6 +40,8 @@ char * __init machine_specific_memory_se
 
 void __init machine_specific_arch_setup(void)
 {
+	setup_xen_features();
+
 #ifdef CONFIG_ACPI
 	if (!(xen_start_info->flags & SIF_INITDOMAIN)) {
 		printk(KERN_INFO "ACPI in unprivileged domain disabled\n");
drivers/Makefile +1 -0
diff -r 1f0f17cb7700 drivers/Makefile
@@ -31,6 +31,7 @@ obj-$(CONFIG_NUBUS)		+= nubus/
 obj-$(CONFIG_NUBUS)		+= nubus/
 obj-$(CONFIG_ATM)		+= atm/
 obj-$(CONFIG_PPC_PMAC)		+= macintosh/
+obj-$(CONFIG_XEN)		+= xen/
 obj-$(CONFIG_IDE)		+= ide/
 obj-$(CONFIG_FC4)		+= fc4/
 obj-$(CONFIG_SCSI)		+= scsi/
include/asm-i386/hypervisor.h +1 -0
diff -r 1f0f17cb7700 include/asm-i386/hypervisor.h
@@ -40,6 +40,7 @@
 
 #include <xen/interface/xen.h>
 #include <xen/interface/version.h>
+#include <xen/features.h>
 
 #include <asm/ptrace.h>
 #include <asm/page.h>
drivers/xen/Makefile new file +3 -0
diff -r 1f0f17cb7700 drivers/xen/Makefile
@@ -0,0 +1,3 @@
+
+obj-y	+= core/
+
drivers/xen/core/Makefile new file +3 -0
diff -r 1f0f17cb7700 drivers/xen/core/Makefile
@@ -0,0 +1,3 @@
+
+obj-y	:= features.o
+
drivers/xen/core/features.c new file +31 -0
diff -r 1f0f17cb7700 drivers/xen/core/features.c
@@ -0,0 +1,31 @@
+/******************************************************************************
+ * features.c
+ *
+ * Xen feature flags.
+ *
+ * Copyright (c) 2006, Ian Campbell, XenSource Inc.
+ */
+#include <linux/types.h>
+#include <linux/cache.h>
+#include <linux/module.h>
+#include <asm/hypervisor.h>
+#include <xen/features.h>
+
+u8 xen_features[XENFEAT_NR_SUBMAPS * 32] __read_mostly;
+EXPORT_SYMBOL_GPL(xen_features);
+
+void setup_xen_features(void)
+{
+#if 0	/* hypercalls come later in the series */
+	struct xen_feature_info fi;
+	int i, j;
+
+	for (i = 0; i < XENFEAT_NR_SUBMAPS; i++) {
+		fi.submap_idx = i;
+		if (HYPERVISOR_xen_version(XENVER_get_features, &fi) < 0)
+			break;
+		for (j=0; j<32; j++)
+			xen_features[i*32+j] = !!(fi.submap & 1<<j);
+	}
+#endif
+}
include/xen/features.h new file +20 -0
diff -r 1f0f17cb7700 include/xen/features.h
@@ -0,0 +1,20 @@
+/******************************************************************************
+ * features.h
+ *
+ * Query the features reported by Xen.
+ *
+ * Copyright (c) 2006, Ian Campbell
+ */
+
+#ifndef __ASM_XEN_FEATURES_H__
+#define __ASM_XEN_FEATURES_H__
+
+#include <xen/interface/version.h>
+
+extern void setup_xen_features(void);
+
+extern u8 xen_features[XENFEAT_NR_SUBMAPS * 32];
+
+#define xen_feature(flag)	(xen_features[flag])
+
+#endif /* __ASM_XEN_FEATURES_H__ */

--

#29 lore raw

RFC PATCH 11/33 Add Xen-specific memory management definitions

In reply to Chris Wright

Add extra memory management definitions used by Xen-specific code. These allow conversion between 'pseudophysical' memory addresses, which provide the illusion of a physically contiguous memory map, and underlying real machine addresses. This conversion is neceesary when interpreting and updating PTEs. Also support write protection of page mappings, which is needed to allow successful validation of page tables.

The current definitions are incomplete and only a stub implementation, allowing us to re-use existing code (drivers) which references these memory management code changes.

---
 arch/i386/mach-xen/Makefile               |    2 
 arch/i386/mach-xen/memory.c               |   31 +++++++++++
 include/asm-i386/mach-default/mach_page.h |    4 +
 include/asm-i386/mach-xen/mach_page.h     |   75 +++++++++++++++++++++++++++++
 include/asm-i386/page.h                   |    2 
 5 files changed, 113 insertions(+), 1 deletion(-)
arch/i386/mach-xen/Makefile +1 -1
diff -r 1495c1bc43a1 arch/i386/mach-xen/Makefile
@@ -2,6 +2,6 @@
 # Makefile for the linux kernel.
 #
 
-obj-y				:= setup.o setup-xen.o
+obj-y				:= setup.o setup-xen.o memory.o
 
 setup-y				:= ../mach-default/setup.o
include/asm-i386/page.h +2 -0
diff -r 1495c1bc43a1 include/asm-i386/page.h
@@ -82,6 +82,8 @@ typedef struct { unsigned long pgprot; }
 /* to align the pointer to the (next) page boundary */
 #define PAGE_ALIGN(addr)	(((addr)+PAGE_SIZE-1)&PAGE_MASK)
 
+#include <mach_page.h>
+
 /*
  * This handles the memory map.. We could make this a config
  * option, but too many people screw it up, and too few need
arch/i386/mach-xen/memory.c new file +31 -0
diff -r 1495c1bc43a1 arch/i386/mach-xen/memory.c
@@ -0,0 +1,31 @@
+
+#include <linux/stddef.h>
+#include <asm/desc.h>
+#include <asm/hypervisor.h>
+#include <asm/page.h>
+#include <asm/pgtable.h>
+
+maddr_t arbitrary_virt_to_machine(unsigned long address)
+{
+	pte_t *pte = lookup_address(address);
+	maddr_t m;
+
+	BUG_ON(pte == NULL);
+
+	m = (maddr_t)pte_mfn(*pte) << PAGE_SHIFT;
+	return m | (address & (PAGE_SIZE - 1));
+}
+
+void make_lowmem_page_readonly(unsigned long address, unsigned int feature)
+{
+	pte_t *pte;
+	int rc;
+
+	if (xen_feature(feature))
+		return;
+
+	pte = lookup_address(address);
+	BUG_ON(pte == NULL);
+	rc = HYPERVISOR_update_va_mapping(address, pte_wrprotect(*pte), 0);
+	BUG_ON(rc);
+}
include/asm-i386/mach-default/mach_page.h new file +4 -0
diff -r 1495c1bc43a1 include/asm-i386/mach-default/mach_page.h
@@ -0,0 +1,4 @@
+#ifndef __ASM_MACH_PAGE_H
+#define __ASM_MACH_PAGE_H
+
+#endif /* __ASM_MACH_PAGE_H */
include/asm-i386/mach-xen/mach_page.h new file +75 -0

75 lines not on this page · raw message

--

#30 lore raw

RFC PATCH 12/33 Change __FIXADDR_TOP to leave room for the hypervisor.

In reply to Chris Wright

Move the definition of __FIXADDR_TOP into a subarch include file so that it can be overridden for subarch xen -- the hypervisor needs about 64MB at the top of the address space.

---
 include/asm-i386/fixmap.h                   |    8 +-------
 include/asm-i386/mach-default/mach_fixmap.h |   11 +++++++++++
 include/asm-i386/mach-xen/mach_fixmap.h     |   11 +++++++++++
 3 files changed, 23 insertions(+), 7 deletions(-)
include/asm-i386/fixmap.h +1 -7
diff -r 1c88b225d413 include/asm-i386/fixmap.h
@@ -13,13 +13,7 @@
 #ifndef _ASM_FIXMAP_H
 #define _ASM_FIXMAP_H
 
-
-/* used by vmalloc.c, vsyscall.lds.S.
- *
- * Leave one empty page between vmalloc'ed areas and
- * the start of the fixmap.
- */
-#define __FIXADDR_TOP	0xfffff000
+#include <mach_fixmap.h>
 
 #ifndef __ASSEMBLY__
 #include <linux/kernel.h>
include/asm-i386/mach-default/mach_fixmap.h new file +11 -0
diff -r 1c88b225d413 include/asm-i386/mach-default/mach_fixmap.h
@@ -0,0 +1,11 @@
+#ifndef __ASM_MACH_FIXMAP_H
+#define __ASM_MACH_FIXMAP_H
+
+/* used by vmalloc.c, vsyscall.lds.S.
+ *
+ * Leave one empty page between vmalloc'ed areas and
+ * the start of the fixmap.
+ */
+#define __FIXADDR_TOP	0xfffff000
+
+#endif /* __ASM_MACH_FIXMAP_H */
include/asm-i386/mach-xen/mach_fixmap.h new file +13 -0
diff -r 1c88b225d413 include/asm-i386/mach-xen/mach_fixmap.h
@@ -0,0 +1,13 @@
+#ifndef __ASM_MACH_FIXMAP_H
+#define __ASM_MACH_FIXMAP_H
+
+#include <xen/interface/xen.h>
+
+/* used by vmalloc.c, vsyscall.lds.S.
+ *
+ * Leave one empty page between vmalloc'ed areas and
+ * the start of the fixmap.
+ */
+#define __FIXADDR_TOP	(HYPERVISOR_VIRT_START - 2 * PAGE_SIZE)
+
+#endif /* __ASM_MACH_FIXMAP_H */

--

#31 lore raw

RFC PATCH 13/33 Add a new head.S start-of-day file for booting on Xen.

In reply to Chris Wright

When running on Xen, the kernel is started with paging enabled. Also don't check for cpu features which are present on all cpus supported by Xen.

Don't define segments which are not supported when running on Xen.

Define the __xen_guest section which exports information about the kernel to the domain builder.

---
 arch/i386/Makefile          |    7 +-
 arch/i386/kernel/head-cpu.S |   86 +++++++++++++++++++++++++++++
 arch/i386/kernel/head.S     |   71 ++----------------------
 arch/i386/mach-xen/Makefile |    2
 arch/i386/mach-xen/head.S   |  127 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 226 insertions(+), 67 deletions(-)
arch/i386/Makefile +5 -2
diff -r 1fae6e99c029 arch/i386/Makefile
@@ -48,6 +48,9 @@ CFLAGS				+= $(shell if [ $(call cc-vers
 
 CFLAGS += $(cflags-y)
 
+# Default subarch head files
+head-y := arch/i386/kernel/head.o arch/i386/kernel/init_task.o
+
 # Default subarch .c files
 mcore-y  := mach-default
 
@@ -74,6 +77,8 @@ mcore-$(CONFIG_X86_SUMMIT)  := mach-defa
 # Xen subarch support
 mflags-$(CONFIG_X86_XEN)	:= -Iinclude/asm-i386/mach-xen
 mcore-$(CONFIG_X86_XEN)		:= mach-xen
+head-$(CONFIG_X86_XEN)		:= arch/i386/mach-xen/head.o \
+				   arch/i386/kernel/init_task.o
 
 # generic subarchitecture
 mflags-$(CONFIG_X86_GENERICARCH) := -Iinclude/asm-i386/mach-generic
@@ -87,8 +92,6 @@ core-$(CONFIG_X86_ES7000)	:= arch/i386/m
 
 # default subarch .h files
 mflags-y += -Iinclude/asm-i386/mach-default
-
-head-y := arch/i386/kernel/head.o arch/i386/kernel/init_task.o
 
 libs-y 					+= arch/i386/lib/
 core-y					+= arch/i386/kernel/ \
arch/i386/kernel/head.S +6 -65

90 lines not on this page · raw message

arch/i386/mach-xen/Makefile +2 -0
diff -r 1fae6e99c029 arch/i386/mach-xen/Makefile
@@ -2,6 +2,8 @@
 # Makefile for the linux kernel.
 #
 
+extra-y				:= head.o
+
 obj-y				:= setup.o setup-xen.o memory.o
 
 setup-y				:= ../mach-default/setup.o
arch/i386/kernel/head-cpu.S new file +86 -0

86 lines not on this page · raw message

arch/i386/mach-xen/head.S new file +127 -0

127 lines not on this page · raw message

--

#32 lore raw

Re: [RFC PATCH 13/33] Add a new head.S start-of-day file for booting on Xen.

Quoted 4 lines · Chris Wright

plain text document attachment (i386-head.S)
When running on Xen, the kernel is started with paging enabled. Also don't check for cpu features which are present on all cpus supported by Xen.

Hi,

I didn't see much of this last sentence in the actual patch, which is good, because I just don't see any reason to do that at all; if they features are there anyway, why not preserve their detection, it's not hurting and it means you need to be less different "just because"...

Greetings,
Arjan van de Ven

#33 lore raw

Re: [RFC PATCH 13/33] Add a new head.S start-of-day file for booting on Xen.

Quoted 5 lines · Arjan van de Ven

I didn't see much of this last sentence in the actual patch, which is good, because I just don't see any reason to do that at all; if they features are there anyway, why not preserve their detection, it's not hurting and it means you need to be less different "just because"...

mach-xen/head.S is simpler than kernel/head.S, partly because it doesn't contain the CPU identification and feature tests (cpuid is assumed, though ironically, we don't actually use it).

J

#34 lore raw

RFC PATCH 14/33 subarch support for controlling interrupt delivery

In reply to Chris Wright

Abstract the code that controls interrupt delivery, and add a separate subarch implementation for Xen that manipulates a shared-memory event delivery mask.

---
 arch/i386/mach-xen/Makefile                   |    2 
 arch/i386/mach-xen/interrupt.c                |   67 +++++++++++++++++++++++++
 include/asm-i386/irqflags.h                   |   61 +---------------------
 include/asm-i386/mach-default/mach_irqflags.h |   64 +++++++++++++++++++++++
 include/asm-i386/mach-default/mach_system.h   |    4 +
 include/asm-i386/mach-xen/mach_irqflags.h     |   29 ++++++++++
 include/asm-i386/mach-xen/mach_system.h       |    5 +
 7 files changed, 174 insertions(+), 58 deletions(-)
arch/i386/mach-xen/Makefile +1 -1
diff -r 092b7e56c85d arch/i386/mach-xen/Makefile
@@ -4,6 +4,6 @@
 
 extra-y				:= head.o
 
-obj-y				:= setup.o setup-xen.o memory.o
+obj-y				:= setup.o setup-xen.o memory.o interrupt.o
 
 setup-y				:= ../mach-default/setup.o
include/asm-i386/irqflags.h +4 -57

81 lines not on this page · raw message

arch/i386/mach-xen/interrupt.c new file +67 -0

67 lines not on this page · raw message

include/asm-i386/mach-default/mach_irqflags.h new file +64 -0

64 lines not on this page · raw message

include/asm-i386/mach-default/mach_system.h new file +4 -0
diff -r 092b7e56c85d include/asm-i386/mach-default/mach_system.h
@@ -0,0 +1,4 @@
+#ifndef __ASM_MACH_SYSTEM_H
+#define __ASM_MACH_SYSTEM_H
+
+#endif /* __ASM_MACH_SYSTEM_H */
include/asm-i386/mach-xen/mach_irqflags.h new file +29 -0
diff -r 092b7e56c85d include/asm-i386/mach-xen/mach_irqflags.h
@@ -0,0 +1,29 @@
+#ifndef ASM_MACH_IRQFLAGS_H
+#define ASM_MACH_IRQFLAGS_H
+
+#ifndef __ASSEMBLY__
+
+#ifdef CONFIG_SMP
+#define __vcpu_id smp_processor_id()
+#else
+#define __vcpu_id 0
+#endif
+
+/* interrupt control.. */
+
+unsigned long __raw_local_save_flags(void);
+void raw_local_irq_restore(unsigned long flags);
+void raw_local_irq_disable(void);
+void raw_local_irq_enable(void);
+
+#define raw_safe_halt()		do { } while(0)
+#define halt()			do { } while(0)
+
+static inline int raw_irqs_disabled_flags(unsigned long flags)
+{
+	return flags != 0;
+}
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* ASM_MACH_IRQFLAGS_H */
include/asm-i386/mach-xen/mach_system.h new file +5 -0
diff -r 092b7e56c85d include/asm-i386/mach-xen/mach_system.h
@@ -0,0 +1,5 @@
+#ifndef __ASM_MACH_SYSTEM_H
+#define __ASM_MACH_SYSTEM_H
+
+
+#endif /* __ASM_MACH_SYSTEM_H */

--

#35 lore raw

RFC PATCH 15/33 move segment checks to subarch

In reply to Chris Wright

We allow for the fact that the guest kernel may not run in ring 0. This requires some abstraction in a few places when setting %cs or checking privilege level (user vs kernel).

---
 arch/i386/kernel/process.c                   |    2 +-
 arch/i386/mm/fault.c                         |    8 +++++---
 include/asm-i386/mach-default/mach_segment.h |    8 ++++++++
 include/asm-i386/mach-xen/mach_segment.h     |    9 +++++++++
 include/asm-i386/mach-xen/mach_system.h      |    1 +
 include/asm-i386/ptrace.h                    |    6 ++++--
 include/asm-i386/segment.h                   |    2 ++
 include/asm-i386/system.h                    |    1 +
 8 files changed, 31 insertions(+), 6 deletions(-)
arch/i386/kernel/process.c +1 -1
diff -r 0bc9790d1ce3 arch/i386/kernel/process.c
@@ -346,7 +346,7 @@ int kernel_thread(int (*fn)(void *), voi
 	regs.xes = __USER_DS;
 	regs.orig_eax = -1;
 	regs.eip = (unsigned long) kernel_thread_helper;
-	regs.xcs = __KERNEL_CS;
+	regs.xcs = get_kernel_cs();
 	regs.eflags = X86_EFLAGS_IF | X86_EFLAGS_SF | X86_EFLAGS_PF | 0x2;
 
 	/* Ok, create the new process.. */
arch/i386/mm/fault.c +5 -3
diff -r 0bc9790d1ce3 arch/i386/mm/fault.c
@@ -28,6 +28,8 @@
 #include <asm/desc.h>
 #include <asm/kdebug.h>
 
+#include <mach_segment.h>
+
 extern void die(const char *,struct pt_regs *,long);
 
 #ifdef CONFIG_KPROBES
@@ -119,10 +121,10 @@ static inline unsigned long get_segment_
 	}
 
 	/* The standard kernel/user address space limit. */
-	*eip_limit = (seg & 3) ? USER_DS.seg : KERNEL_DS.seg;
+	*eip_limit = (seg & USER_MODE_MASK) ? USER_DS.seg : KERNEL_DS.seg;
 	
 	/* By far the most common cases. */
-	if (likely(seg == __USER_CS || seg == __KERNEL_CS))
+	if (likely(seg == __USER_CS || seg == get_kernel_cs()))
 		return eip;
 
 	/* Check the segment exists, is within the current LDT/GDT size,
@@ -437,7 +439,7 @@ good_area:
 	switch (error_code & 3) {
 		default:	/* 3: write, present */
 #ifdef TEST_VERIFY_AREA
-			if (regs->cs == KERNEL_CS)
+			if (regs->cs == get_kernel_cs())
 				printk("WP fault at %08lx\n", regs->eip);
 #endif
 			/* fall through */
include/asm-i386/ptrace.h +4 -2
diff -r 0bc9790d1ce3 include/asm-i386/ptrace.h
@@ -1,5 +1,7 @@
 #ifndef _I386_PTRACE_H
 #define _I386_PTRACE_H
+
+#include <mach_segment.h>
 
 #define EBX 0
 #define ECX 1
@@ -73,11 +75,11 @@ extern void send_sigtrap(struct task_str
  */
 static inline int user_mode(struct pt_regs *regs)
 {
-	return (regs->xcs & 3) != 0;
+	return (regs->xcs & USER_MODE_MASK) != 0;
 }
 static inline int user_mode_vm(struct pt_regs *regs)
 {
-	return ((regs->xcs & 3) | (regs->eflags & VM_MASK)) != 0;
+	return ((regs->xcs & USER_MODE_MASK) | (regs->eflags & VM_MASK)) != 0;
 }
 #define instruction_pointer(regs) ((regs)->eip)
 #if defined(CONFIG_SMP) && defined(CONFIG_FRAME_POINTER)
include/asm-i386/segment.h +2 -0
diff -r 0bc9790d1ce3 include/asm-i386/segment.h
@@ -1,5 +1,7 @@
 #ifndef _ASM_SEGMENT_H
 #define _ASM_SEGMENT_H
+
+#include <mach_segment.h>
 
 /*
  * The layout of the per-CPU GDT under Linux:
include/asm-i386/system.h +1 -0
diff -r 0bc9790d1ce3 include/asm-i386/system.h
@@ -487,6 +487,7 @@ static inline unsigned long long __cmpxc
 #endif
 
 #include <linux/irqflags.h>
+#include <mach_system.h>
 
 /*
  * disable hlt during certain critical i/o operations
include/asm-i386/mach-xen/mach_system.h +1 -0
diff -r 0bc9790d1ce3 include/asm-i386/mach-xen/mach_system.h
@@ -1,5 +1,6 @@
 #ifndef __ASM_MACH_SYSTEM_H
 #define __ASM_MACH_SYSTEM_H
 
+#include <asm/hypervisor.h>
 
 #endif /* __ASM_MACH_SYSTEM_H */
include/asm-i386/mach-default/mach_segment.h new file +8 -0
diff -r 0bc9790d1ce3 include/asm-i386/mach-default/mach_segment.h
@@ -0,0 +1,8 @@
+#ifndef __ASM_MACH_SEGMENT_H
+#define __ASM_MACH_SEGMENT_H
+
+#define USER_MODE_MASK 3
+
+#define get_kernel_cs() __KERNEL_CS
+
+#endif /* __ASM_MACH_SEGMENT_H */
include/asm-i386/mach-xen/mach_segment.h new file +9 -0
diff -r 0bc9790d1ce3 include/asm-i386/mach-xen/mach_segment.h
@@ -0,0 +1,9 @@
+#ifndef __ASM_MACH_SEGMENT_H
+#define __ASM_MACH_SEGMENT_H
+
+#define USER_MODE_MASK 2
+
+#define get_kernel_cs() \
+	(__KERNEL_CS + (xen_feature(XENFEAT_supervisor_mode_kernel) ? 0 : 1))
+
+#endif /* __ASM_MACH_SEGMENT_H */

--

#36 lore raw

Re: [RFC PATCH 15/33] move segment checks to subarch

arch/i386/kernel/process.c int kernel_thread(int (*fn)(void *), voi · line 349

Quoted 7 lines · Chris Wright

plain text document attachment (i386-segments)
We allow for the fact that the guest kernel may not run in ring 0. This requires some abstraction in a few places when setting %cs or checking privilege level (user vs kernel).

arch/i386/kernel/process.c +1 -1
-	regs.xcs = __KERNEL_CS;
+	regs.xcs = get_kernel_cs();

Hi,

wouldn't this patch be simpler if __KERNEL_CS just became the macro that currently is get_kernel_cs() for the XEN case? then code like this doesn't need changing at all...

Greetings,
Arjan van de Ven

#37 lore raw

Re: [RFC PATCH 15/33] move segment checks to subarch

Quoted 15 lines · Arjan van de Ven

arch/i386/kernel/process.c int kernel_thread(int (*fn)(void *), voi · line 349

Quoted 10 lines · Chris Wright

plain text document attachment (i386-segments)
We allow for the fact that the guest kernel may not run in ring 0. This requires some abstraction in a few places when setting %cs or checking privilege level (user vs kernel).

arch/i386/kernel/process.c +1 -1
-	regs.xcs = __KERNEL_CS;
+	regs.xcs = get_kernel_cs();

Hi,

Quoted 4 lines

wouldn't this patch be simpler if __KERNEL_CS just became the macro that currently is get_kernel_cs() for the XEN case? then code like this doesn't need changing at all...

The tradeoff is that then you can't use __KERNEL_CS is assembler code, and it is used in entry.S to detect NMI / debug trap workarounds - which don't actually need to be paravirtualized, as it is easier to hide the nasty cases which cause those side effects.

Zach

#38 lore raw

Re: [RFC PATCH 15/33] move segment checks to subarch

In reply to Chris Wright
Quoted 4 lines · Chris Wright

plain text document attachment (i386-segments)
We allow for the fact that the guest kernel may not run in ring 0. This requires some abstraction in a few places when setting %cs or checking privilege level (user vs kernel).

Zach had an alternate patch for this, which didn't assume the kernel ran in a compile-time known ring, but is otherwise very similar. I've put it below for discussion (but Zach now tells me the asm parts are not
required: Zach, can you mod this patch and comment?).

Your patch #16 finishes the job you started here, by doing the mods to entry.S. I think it's cleaner to have all this in one patch (and it can go in almost immediately AFAICT).

Comments?
Rusty.

Name: Kernel Ring Cleanups
Status: Booted on 2.6.18-rc1

This is Zach's patch to clean up assumptions about the kernel running in ring 0 (which it doesn't when running paravirtualized).

1) Remove the hardcoded 3 and introduce #define SEGMENT_RPL_MASK 3
2) Add a get_kernel_rpl() function
3) Create COMPARE_SEGMENT_STACK and COMPARE_SEGMENT_REG macros which can mask out the bottom two bits (RPL) when comparing for paravirtualization.

arch/i386/kernel/entry.S +14 -22
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .27795-linux-2.6.18-rc1/arch/i386/kernel/entry.S .27795-linux-2.6.18-rc1.updated/arch/i386/kernel/entry.S
@@ -228,9 +228,11 @@ ret_from_intr:
 	GET_THREAD_INFO(%ebp)
 check_userspace:
 	movl EFLAGS(%esp), %eax		# mix EFLAGS and CS
+	andl $VM_MASK, %eax
 	movb CS(%esp), %al
-	testl $(VM_MASK | 3), %eax
-	jz resume_kernel
+	andb $SEGMENT_RPL_MASK, %al
+	cmpl $SEGMENT_RPL_MASK, %eax
+	jb resume_kernel		# returning to kernel or vm86-space
 ENTRY(resume_userspace)
  	cli				# make sure we don't miss an interrupt
 					# setting need_resched or sigpending
@@ -517,22 +519,16 @@ syscall_badsys:
 	/* put ESP to the proper location */ \
 	movl %eax, %esp;
 #define UNWIND_ESPFIX_STACK \
-	pushl %eax; \
-	CFI_ADJUST_CFA_OFFSET 4; \
-	movl %ss, %eax; \
-	/* see if on 16bit stack */ \
-	cmpw $__ESPFIX_SS, %ax; \
-	je 28f; \
-27:	popl %eax; \
-	CFI_ADJUST_CFA_OFFSET -4; \
+	COMPARE_SEGMENT_REG(__ESPFIX_SS, %ss); \
+	jne 28f; \
 .section .fixup,"ax"; \
-28:	movl $__KERNEL_DS, %eax; \
+	movl $__KERNEL_DS, %eax; \
 	movl %eax, %ds; \
 	movl %eax, %es; \
 	/* switch to 32bit stack */ \
 	FIXUP_ESPFIX_STACK; \
-	jmp 27b; \
-.previous
+.previous \
+28: ; 
 
 /*
  * Build the entry stubs and pointer table with
@@ -629,6 +625,7 @@ error_code:
 	popl %ecx
 	CFI_ADJUST_CFA_OFFSET -4
 	/*CFI_REGISTER es, ecx*/
+	movl EAX(%esp), %eax
 	movl ES(%esp), %edi		# get the function address
 	movl ORIG_EAX(%esp), %edx	# get the error code
 	movl %eax, ORIG_EAX(%esp)
@@ -694,12 +691,12 @@ device_not_available_emulate:
  * the instruction that would have done it for sysenter.
  */
 #define FIX_STACK(offset, ok, label)		\
-	cmpw $__KERNEL_CS,4(%esp);		\
+	COMPARE_SEGMENT_STACK(__KERNEL_CS, 4);	\
 	jne ok;					\
 label:						\
 	movl TSS_sysenter_esp0+offset(%esp),%esp;	\
 	pushfl;					\
-	pushl $__KERNEL_CS;			\
+	push  %cs;				\
 	pushl $sysenter_past_esp
 
 KPROBE_ENTRY(debug)
@@ -727,12 +724,7 @@ debug_stack_correct:
  */
 ENTRY(nmi)
 	RING0_INT_FRAME
-	pushl %eax
-	CFI_ADJUST_CFA_OFFSET 4
-	movl %ss, %eax
-	cmpw $__ESPFIX_SS, %ax
-	popl %eax
-	CFI_ADJUST_CFA_OFFSET -4
+	COMPARE_SEGMENT_REG(__ESPFIX_SS, %ss)
 	je nmi_16bit_stack
 	cmpl $sysenter_entry,(%esp)
 	je nmi_stack_fixup
@@ -763,7 +755,7 @@ nmi_stack_fixup:
 	FIX_STACK(12,nmi_stack_correct, 1)
 	jmp nmi_stack_correct
 nmi_debug_stack_check:
-	cmpw $__KERNEL_CS,16(%esp)
+	COMPARE_SEGMENT_STACK(__KERNEL_CS, 16)
 	jne nmi_stack_correct
 	cmpl $debug,(%esp)
 	jb nmi_stack_correct
arch/i386/kernel/process.c +1 -1
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .27795-linux-2.6.18-rc1/arch/i386/kernel/process.c .27795-linux-2.6.18-rc1.updated/arch/i386/kernel/process.c
@@ -346,7 +346,7 @@ int kernel_thread(int (*fn)(void *), voi
 	regs.xes = __USER_DS;
 	regs.orig_eax = -1;
 	regs.eip = (unsigned long) kernel_thread_helper;
-	regs.xcs = __KERNEL_CS;
+	regs.xcs = __KERNEL_CS | get_kernel_rpl();
 	regs.eflags = X86_EFLAGS_IF | X86_EFLAGS_SF | X86_EFLAGS_PF | 0x2;
 
 	/* Ok, create the new process.. */
arch/i386/kernel/traps.c +2 -2
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .27795-linux-2.6.18-rc1/arch/i386/kernel/traps.c .27795-linux-2.6.18-rc1.updated/arch/i386/kernel/traps.c
@@ -1034,10 +1034,10 @@ fastcall void setup_x86_bogus_stack(unsi
 	memcpy((void *)(stack_bot + iret_frame16_off), &regs->eip, 20);
 	/* fill in the switch pointers */
 	switch16_ptr[0] = (regs->esp & 0xffff0000) | iret_frame16_off;
-	switch16_ptr[1] = __ESPFIX_SS;
+	switch16_ptr[1] = __ESPFIX_SS | get_kernel_rpl();
 	switch32_ptr[0] = (unsigned long)stk + sizeof(struct pt_regs) +
 		8 - CPU_16BIT_STACK_SIZE;
-	switch32_ptr[1] = __KERNEL_DS;
+	switch32_ptr[1] = __KERNEL_DS | get_kernel_rpl();
 }
 
 fastcall unsigned char * fixup_x86_bogus_stack(unsigned short sp)
include/asm-i386/ptrace.h +3 -2
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .27795-linux-2.6.18-rc1/include/asm-i386/ptrace.h .27795-linux-2.6.18-rc1.updated/include/asm-i386/ptrace.h
@@ -60,6 +60,7 @@ struct pt_regs {
 #ifdef __KERNEL__
 
 #include <asm/vm86.h>
+#include <asm/segment.h>
 
 struct task_struct;
 extern void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs, int error_code);
@@ -73,11 +74,11 @@ extern void send_sigtrap(struct task_str
  */
 static inline int user_mode(struct pt_regs *regs)
 {
-	return (regs->xcs & 3) != 0;
+	return (regs->xcs & SEGMENT_RPL_MASK) == 3;
 }
 static inline int user_mode_vm(struct pt_regs *regs)
 {
-	return ((regs->xcs & 3) | (regs->eflags & VM_MASK)) != 0;
+	return (((regs->xcs & SEGMENT_RPL_MASK) | (regs->eflags & VM_MASK)) >= 3);
 }
 #define instruction_pointer(regs) ((regs)->eip)
 #if defined(CONFIG_SMP) && defined(CONFIG_FRAME_POINTER)
include/asm-i386/segment.h +16 -0
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .27795-linux-2.6.18-rc1/include/asm-i386/segment.h .27795-linux-2.6.18-rc1.updated/include/asm-i386/segment.h
@@ -112,4 +112,20 @@
  */
 #define IDT_ENTRIES 256
 
+/* Bottom three bits of xcs give the ring privilege level */
+#define SEGMENT_RPL_MASK 0x3
+
+#define get_kernel_rpl()  0
+
+#define COMPARE_SEGMENT_STACK(segment, offset)	\
+	cmpw $segment, offset(%esp);
+
+#define COMPARE_SEGMENT_REG(segment, reg)	\
+	pushl %eax;				\
+	CFI_ADJUST_CFA_OFFSET 4;		\
+	mov   reg, %eax;			\
+	cmpw  $segment,%ax;			\
+	popl  %eax;				\
+	CFI_ADJUST_CFA_OFFSET -4
+
 #endif
Signature · 1 line
Help! Save Australia from the worst of the DMCA: http://linux.org.au/law
#39 lore raw

Re: [RFC PATCH 15/33] move segment checks to subarch

Quoted 10 lines · * Rusty Russell ([email protected]) wrote:
Quoted 4 lines · Chris Wright

plain text document attachment (i386-segments)
We allow for the fact that the guest kernel may not run in ring 0. This requires some abstraction in a few places when setting %cs or checking privilege level (user vs kernel).

Zach had an alternate patch for this, which didn't assume the kernel ran in a compile-time known ring, but is otherwise very similar. I've put it below for discussion (but Zach now tells me the asm parts are not
required: Zach, can you mod this patch and comment?).

This patch also doesn't have a compile time known ring, it's using get_kernel_cs() because the Xen method for booting native is dynamic and would resolve to ring 0 in that case (XENFEAT_supervisor_mode_kernel).

Quoted 3 lines

Your patch #16 finishes the job you started here, by doing the mods to entry.S. I think it's cleaner to have all this in one patch (and it can go in almost immediately AFAICT).

I totally agree. I actually started doing rearranging the patch order last night and that was one of the bits that got consolidated. It was getting light this morning and I gave up since I was wasting lots of time fixing patch rejects from all the rearranging, and needed a bit of sleep before KS;-)

Quoted 6 lines

Comments?
Rusty.

Name: Kernel Ring Cleanups
Status: Booted on 2.6.18-rc1

Looks fine to me, granted I haven't tried to boot it yet. I'll double check and report back.

thanks,
-chris

#40 lore raw

Re: [Xen-devel] Re: [RFC PATCH 15/33] move segment checks to subarch

Quoted 7 lines · Chris Wright
Quoted 6 lines · * Rusty Russell ([email protected]) wrote:
Quoted 4 lines · Chris Wright

plain text document attachment (i386-segments)
We allow for the fact that the guest kernel may not run in ring 0. This requires some abstraction in a few places when setting %cs or checking privilege level (user vs kernel).

Quoted 8 lines
Quoted 4 lines

Zach had an alternate patch for this, which didn't assume the kernel ran in a compile-time known ring, but is otherwise very similar. I've put it below for discussion (but Zach now tells me the asm parts are not
required: Zach, can you mod this patch and comment?).

This patch also doesn't have a compile time known ring, it's using get_kernel_cs() because the Xen method for booting native is dynamic and would resolve to ring 0 in that case (XENFEAT_supervisor_mode_kernel).

I was referring to the different ways the two patches figure out whether we're in user mode:

Yours:
static inline int user_mode(struct pt_regs *regs)

 {
       return (regs->xcs & USER_MODE_MASK) != 0;

}

Where you have for native:

	#define USER_MODE_MASK 3
vs Xen:
	#define USER_MODE_MASK 2

Zach's patch does this:

static inline int user_mode(struct pt_regs *regs)

 {
	return (regs->xcs & SEGMENT_RPL_MASK) == 3;

}

I'm no x86pert, but the latter seems more generic to me (user mode is ring 3, vs. usermode is anything >= 2). Perhaps they are in fact equivalent?

Thanks!
Rusty.

Signature · 1 line
Help! Save Australia from the worst of the DMCA: http://linux.org.au/law
#41 lore raw

RFC PATCH 16/33 Add support for Xen to entry.S.

In reply to Chris Wright

- change cli/sti
- change test for user mode return to work for kernel mode in ring1
- check hypervisor saved event mask on return from exception
- add entry points for the hypervisor upcall handlers
- avoid math emulation check when running on Xen
- add nmi handler for running on Xen

---
 arch/i386/kernel/asm-offsets.c |   26 +++++++
 arch/i386/kernel/entry.S       |  141 ++++++++++++++++++++++++++++++++++++-----
 arch/i386/mach-xen/setup-xen.c |   19 +++++
 drivers/xen/core/features.c    |    2
 4 files changed, 169 insertions(+), 19 deletions(-)
arch/i386/kernel/entry.S +124 -17
diff -r 5cca1805b8a7 arch/i386/kernel/entry.S
@@ -76,8 +76,39 @@ NT_MASK		= 0x00004000
 NT_MASK		= 0x00004000
 VM_MASK		= 0x00020000
 
+#ifndef CONFIG_XEN
+#define DISABLE_INTERRUPTS	cli
+#define ENABLE_INTERRUPTS	sti
+#else
+#include <xen/interface/xen.h>
+
+EVENT_MASK	= 0x2E
+
+/* Offsets into shared_info_t. */
+#define evtchn_upcall_pending		/* 0 */
+#define evtchn_upcall_mask		1
+
+#ifdef CONFIG_SMP
+/* Set %esi to point to the appropriate vcpu structure */
+#define GET_VCPU_INFO		movl TI_cpu(%ebp),%esi			; \
+				shl  $SIZEOF_VCPU_INFO_SHIFT,%esi	; \
+				addl HYPERVISOR_shared_info,%esi
+#else
+#define GET_VCPU_INFO		movl HYPERVISOR_shared_info,%esi
+#endif
+
+/* The following end up using/clobbering %esi, because of GET_VCPU_INFO */
+#define __DISABLE_INTERRUPTS	movb $1,evtchn_upcall_mask(%esi)
+#define DISABLE_INTERRUPTS	GET_VCPU_INFO				; \
+				__DISABLE_INTERRUPTS
+#define ENABLE_INTERRUPTS	GET_VCPU_INFO				; \
+				movb $0,evtchn_upcall_mask(%esi)
+#define __TEST_PENDING		testb $0xFF,evtchn_upcall_pending(%esi)
+#endif
+
+
 #ifdef CONFIG_PREEMPT
-#define preempt_stop		cli; TRACE_IRQS_OFF
+#define preempt_stop		DISABLE_INTERRUPTS; TRACE_IRQS_OFF
 #else
 #define preempt_stop
 #define resume_kernel		restore_nocheck
@@ -229,10 +260,10 @@ check_userspace:
 check_userspace:
 	movl EFLAGS(%esp), %eax		# mix EFLAGS and CS
 	movb CS(%esp), %al
-	testl $(VM_MASK | 3), %eax
+	testl $(VM_MASK | USER_MODE_MASK), %eax
 	jz resume_kernel
 ENTRY(resume_userspace)
- 	cli				# make sure we don't miss an interrupt
+	DISABLE_INTERRUPTS		# make sure we don't miss an interrupt
 					# setting need_resched or sigpending
 					# between sampling and the iret
 	movl TI_flags(%ebp), %ecx
@@ -243,7 +274,7 @@ ENTRY(resume_userspace)
 
 #ifdef CONFIG_PREEMPT
 ENTRY(resume_kernel)
-	cli
+	DISABLE_INTERRUPTS
 	cmpl $0,TI_preempt_count(%ebp)	# non-zero preempt_count ?
 	jnz restore_nocheck
 need_resched:
@@ -271,7 +302,7 @@ sysenter_past_esp:
 	 * No need to follow this irqs on/off section: the syscall
 	 * disabled irqs and here we enable it straight after entry:
 	 */
-	sti
+	ENABLE_INTERRUPTS
 	pushl $(__USER_DS)
 	CFI_ADJUST_CFA_OFFSET 4
 	/*CFI_REL_OFFSET ss, 0*/
@@ -316,7 +347,7 @@ 1:	movl (%ebp),%ebp
 	jae syscall_badsys
 	call *sys_call_table(,%eax,4)
 	movl %eax,EAX(%esp)
-	cli
+	DISABLE_INTERRUPTS
 	TRACE_IRQS_OFF
 	movl TI_flags(%ebp), %ecx
 	testw $_TIF_ALLWORK_MASK, %cx
@@ -326,7 +357,7 @@ 1:	movl (%ebp),%ebp
 	movl OLDESP(%esp), %ecx
 	xorl %ebp,%ebp
 	TRACE_IRQS_ON
-	sti
+	ENABLE_INTERRUPTS
 	sysexit
 	CFI_ENDPROC
 
@@ -352,7 +383,7 @@ syscall_call:
 	call *sys_call_table(,%eax,4)
 	movl %eax,EAX(%esp)		# store the return value
 syscall_exit:
-	cli				# make sure we don't miss an interrupt
+	DISABLE_INTERRUPTS		# make sure we don't miss an interrupt
 					# setting need_resched or sigpending
 					# between sampling and the iret
 	TRACE_IRQS_OFF
@@ -361,6 +392,7 @@ syscall_exit:
 	jne syscall_exit_work
 
 restore_all:
+#ifndef CONFIG_XEN
 	movl EFLAGS(%esp), %eax		# mix EFLAGS, SS and CS
 	# Warning: OLDSS(%esp) contains the wrong/random values if we
 	# are returning to the kernel.
@@ -372,6 +404,27 @@ restore_all:
 	CFI_REMEMBER_STATE
 	je ldt_ss			# returning to user-space with LDT SS
 restore_nocheck:
+#else
+restore_nocheck:
+	/* Return back to the hypervisor if we're not returning to user-mode */
+	movl EFLAGS(%esp), %eax		# mix EFLAGS and CS
+	movb CS(%esp), %al
+	andl $(VM_MASK | 3), %eax
+	cmpl $3, %eax
+	jne hypervisor_iret
+	ENABLE_INTERRUPTS
+	__TEST_PENDING
+	jz restore_regs_and_iret
+	__DISABLE_INTERRUPTS
+	jmp do_hypervisor_callback
+hypervisor_iret:
+	TRACE_IRQS_IRET
+	RESTORE_REGS
+	addl $4, %esp
+	CFI_ADJUST_CFA_OFFSET -4
+	jmp  hypercall_page + (__HYPERVISOR_iret * 32)
+#endif	/* CONFIG_XEN */
+restore_regs_and_iret:
 	TRACE_IRQS_IRET
 restore_nocheck_notrace:
 	RESTORE_REGS
@@ -380,8 +433,10 @@ 1:	iret
 1:	iret
 .section .fixup,"ax"
 iret_exc:
+#ifndef CONFIG_XEN
 	TRACE_IRQS_ON
-	sti
+	ENABLE_INTERRUPTS
+#endif
 	pushl $0			# no error code
 	pushl $do_iret_error
 	jmp error_code
@@ -392,6 +447,7 @@ iret_exc:
 .previous
 
 	CFI_RESTORE_STATE
+#ifndef CONFIG_XEN
 ldt_ss:
 	larl OLDSS(%esp), %eax
 	jnz restore_nocheck
@@ -405,7 +461,7 @@ ldt_ss:
 	 * dosemu and wine happy. */
 	subl $8, %esp		# reserve space for switch16 pointer
 	CFI_ADJUST_CFA_OFFSET 8
-	cli
+	DISABLE_INTERRUPTS
 	TRACE_IRQS_OFF
 	movl %esp, %eax
 	/* Set up the 16bit stack frame with switch32 pointer on top,
@@ -420,6 +476,7 @@ 1:	iret
 	.align 4
 	.long 1b,iret_exc
 .previous
+#endif
 	CFI_ENDPROC
 
 	# perform work that needs to be done immediately before resumption
@@ -430,7 +487,7 @@ work_pending:
 	jz work_notifysig
 work_resched:
 	call schedule
-	cli				# make sure we don't miss an interrupt
+	DISABLE_INTERRUPTS		# make sure we don't miss an interrupt
 					# setting need_resched or sigpending
 					# between sampling and the iret
 	TRACE_IRQS_OFF
@@ -486,7 +543,7 @@ syscall_exit_work:
 	testb $(_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT|_TIF_SINGLESTEP), %cl
 	jz work_pending
 	TRACE_IRQS_ON
-	sti				# could let do_syscall_trace() call
+	ENABLE_INTERRUPTS		# could let do_syscall_trace() call
 					# schedule() instead
 	movl %esp, %eax
 	movl $1, %edx
@@ -508,6 +565,7 @@ syscall_badsys:
 	jmp resume_userspace
 	CFI_ENDPROC
 
+#ifndef CONFIG_XEN
 #define FIXUP_ESPFIX_STACK \
 	movl %esp, %eax; \
 	/* switch to 32bit stack using the pointer on top of 16bit stack */ \
@@ -586,6 +644,9 @@ ENTRY(name)				\
 
 /* The include is where all of the SMP etc. interrupts come from */
 #include "entry_arch.h"
+#else
+#define UNWIND_ESPFIX_STACK
+#endif
 
 ENTRY(divide_error)
 	RING0_INT_FRAME
@@ -642,6 +703,44 @@ error_code:
 	jmp ret_from_exception
 	CFI_ENDPROC
 
+#ifdef CONFIG_XEN
+ENTRY(hypervisor_callback)
+	pushl %eax
+	SAVE_ALL
+do_hypervisor_callback:
+	push %esp
+	call evtchn_do_upcall
+	add  $4,%esp
+	jmp  ret_from_intr
+
+# Hypervisor uses this for application faults while it executes.
+ENTRY(failsafe_callback)
+1:	popl %ds
+2:	popl %es
+3:	popl %fs
+4:	popl %gs
+	subl $4,%esp
+	SAVE_ALL
+	jmp  ret_from_exception
+.section .fixup,"ax";	\
+6:	movl $0,(%esp);	\
+	jmp 1b;		\
+7:	movl $0,(%esp);	\
+	jmp 2b;		\
+8:	movl $0,(%esp);	\
+	jmp 3b;		\
+9:	movl $0,(%esp);	\
+	jmp 4b;		\
+.previous;		\
+.section __ex_table,"a";\
+	.align 4;	\
+	.long 1b,6b;	\
+	.long 2b,7b;	\
+	.long 3b,8b;	\
+	.long 4b,9b;	\
+.previous
+#endif
+
 ENTRY(coprocessor_error)
 	RING0_INT_FRAME
 	pushl $0
@@ -665,18 +764,20 @@ ENTRY(device_not_available)
 	pushl $-1			# mark this as an int
 	CFI_ADJUST_CFA_OFFSET 4
 	SAVE_ALL
+#ifndef CONFIG_XEN
 	movl %cr0, %eax
 	testl $0x4, %eax		# EM (math emulation bit)
-	jne device_not_available_emulate
-	preempt_stop
-	call math_state_restore
-	jmp ret_from_exception
-device_not_available_emulate:
+	je device_available_emulate
 	pushl $0			# temporary storage for ORIG_EIP
 	CFI_ADJUST_CFA_OFFSET 4
 	call math_emulate
 	addl $4, %esp
 	CFI_ADJUST_CFA_OFFSET -4
+	jmp ret_from_exception
+device_available_emulate:
+#endif
+	preempt_stop
+	call math_state_restore
 	jmp ret_from_exception
 	CFI_ENDPROC
 
@@ -717,6 +818,8 @@ debug_stack_correct:
 	jmp ret_from_exception
 	CFI_ENDPROC
 	.previous .text
+
+#ifndef CONFIG_XEN
 /*
  * NMI is doubly nasty. It can happen _while_ we're handling
  * a debug fault, and the debug fault hasn't yet been able to
@@ -801,6 +904,10 @@ 1:	iret
 	.align 4
 	.long 1b,iret_exc
 .previous
+#else
+ENTRY(nmi)
+	jmp restore_all
+#endif
 
 KPROBE_ENTRY(int3)
 	RING0_INT_FRAME
arch/i386/mach-xen/setup-xen.c +19 -0
diff -r 5cca1805b8a7 arch/i386/mach-xen/setup-xen.c
@@ -40,7 +40,26 @@ char * __init machine_specific_memory_se
 
 void __init machine_specific_arch_setup(void)
 {
+	struct physdev_op op;
+
 	setup_xen_features();
+
+	HYPERVISOR_shared_info =
+		(struct shared_info *)__va(xen_start_info->shared_info);
+	memset(empty_zero_page, 0, sizeof(empty_zero_page));
+
+	HYPERVISOR_vm_assist(VMASST_CMD_enable, VMASST_TYPE_4gb_segments);
+
+	HYPERVISOR_set_callbacks(
+	    __KERNEL_CS, (unsigned long)hypervisor_callback,
+	    __KERNEL_CS, (unsigned long)failsafe_callback);
+
+	init_pg_tables_end = __pa(xen_start_info->pt_base) +
+		PFN_PHYS(xen_start_info->nr_pt_frames);
+
+	op.cmd = PHYSDEVOP_SET_IOPL;
+	op.u.set_iopl.iopl = 1;
+	HYPERVISOR_physdev_op(&op);
 
 #ifdef CONFIG_ACPI
 	if (!(xen_start_info->flags & SIF_INITDOMAIN)) {
drivers/xen/core/features.c +0 -2
diff -r 5cca1805b8a7 drivers/xen/core/features.c
@@ -16,7 +16,6 @@ EXPORT_SYMBOL_GPL(xen_features);
 
 void setup_xen_features(void)
 {
-#if 0	/* hypercalls come later in the series */
 	struct xen_feature_info fi;
 	int i, j;
 
@@ -27,5 +26,4 @@ void setup_xen_features(void)
 		for (j=0; j<32; j++)
 			xen_features[i*32+j] = !!(fi.submap & 1<<j);
 	}
-#endif
 }
arch/i386/kernel/asm-offsets.c +26 -0
diff -r 5cca1805b8a7 arch/i386/kernel/asm-offsets.c
@@ -15,6 +15,7 @@
 #include <asm/processor.h>
 #include <asm/thread_info.h>
 #include <asm/elf.h>
+#include <xen/interface/xen.h>
 
 #define DEFINE(sym, val) \
         asm volatile("\n->" #sym " %0 " #val : : "i" (val))
@@ -23,6 +24,27 @@
 
 #define OFFSET(sym, str, mem) \
 	DEFINE(sym, offsetof(struct str, mem));
+
+/* return log2(const) computed at compile time */
+extern inline int get_shift(const int x)
+{
+	extern int __bad_size_in_get_shift(void);
+	if (__builtin_constant_p(x)) {
+		switch(x) {
+		case 1:		return 0;
+		case 2:		return 1;
+		case 4:		return 2;
+		case 8:		return 3;
+		case 16:	return 4;
+		case 32:	return 5;
+		case 64:	return 6;
+		case 128:	return 7;
+		case 256:	return 8;
+		}
+	}
+
+	return __bad_size_in_get_shift();
+}
 
 void foo(void)
 {
@@ -74,4 +96,8 @@ void foo(void)
 	DEFINE(VDSO_PRELINK, VDSO_PRELINK);
 
 	OFFSET(crypto_tfm_ctx_offset, crypto_tfm, __crt_ctx);
+
+	BLANK();
+	DEFINE(SIZEOF_VCPU_INFO, sizeof(struct vcpu_info));
+	DEFINE(SIZEOF_VCPU_INFO_SHIFT, get_shift(sizeof(struct vcpu_info)));
 }

--

#42 lore raw

Re: [RFC PATCH 16/33] Add support for Xen to entry.S.

Quoted 7 lines · Chris Wright
arch/i386/kernel/entry.S +1 -4
+#ifndef CONFIG_XEN
 	movl %cr0, %eax
 	testl $0x4, %eax		# EM (math emulation bit)
-	jne device_not_available_emulate
-	preempt_stop
-	call math_state_restore
-	jmp ret_from_exception

arch/i386/kernel/entry.S ENTRY(device_not_available) · line 778

Quoted 10 lines
arch/i386/kernel/entry.S +4 -1
-device_not_available_emulate:
+	je device_available_emulate
 	pushl $0			# temporary storage for ORIG_EIP
 	CFI_ADJUST_CFA_OFFSET 4
 	call math_emulate
 	addl $4, %esp
 	CFI_ADJUST_CFA_OFFSET -4
+	jmp ret_from_exception
+device_available_emulate:
+#endif

Hi,

can you explain what this chunk is for? It appears to be for the non-xen case due to the ifndef, yet it seems to visibly change the code for that... that deserves an explanation for sure ...

Greetings,
Arjan van de Ven

#43 lore raw

Re: [RFC PATCH 16/33] Add support for Xen to entry.S.

Quoted 24 lines · Arjan van de Ven
Quoted 8 lines · Chris Wright
arch/i386/kernel/entry.S +1 -4
+#ifndef CONFIG_XEN
 	movl %cr0, %eax
 	testl $0x4, %eax		# EM (math emulation bit)
-	jne device_not_available_emulate
-	preempt_stop
-	call math_state_restore
-	jmp ret_from_exception

arch/i386/kernel/entry.S ENTRY(device_not_available) · line 778

Quoted 10 lines
arch/i386/kernel/entry.S +4 -1
-device_not_available_emulate:
+	je device_available_emulate
 	pushl $0			# temporary storage for ORIG_EIP
 	CFI_ADJUST_CFA_OFFSET 4
 	call math_emulate
 	addl $4, %esp
 	CFI_ADJUST_CFA_OFFSET -4
+	jmp ret_from_exception
+device_available_emulate:
+#endif

Hi,

Quoted 4 lines

can you explain what this chunk is for? It appears to be for the non-xen case due to the ifndef, yet it seems to visibly change the code for that... that deserves an explanation for sure ...

This inverts the logic of the test so that the lines:

preempt_stop
call math_state_restore
jmp ret_from_exception

can be hoisted out of the ifdef and shared in the Xen and non-Xen cases.

J

#44 lore raw

Re: [RFC PATCH 16/33] Add support for Xen to entry.S.

In reply to Chris Wright
Quoted 28 lines · Chris Wright

plain text document attachment (i386-entry.S)
- change cli/sti
- change test for user mode return to work for kernel mode in ring1
- check hypervisor saved event mask on return from exception
- add entry points for the hypervisor upcall handlers
- avoid math emulation check when running on Xen
- add nmi handler for running on Xen

---
 arch/i386/kernel/asm-offsets.c |   26 +++++++
 arch/i386/kernel/entry.S       |  141 ++++++++++++++++++++++++++++++++++++-----
 arch/i386/mach-xen/setup-xen.c |   19 +++++
 drivers/xen/core/features.c    |    2
 4 files changed, 169 insertions(+), 19 deletions(-)

diff -r 5cca1805b8a7 arch/i386/kernel/entry.S

arch/i386/kernel/entry.S +1 -0
@@ -76,8 +76,39 @@ NT_MASK		= 0x00004000
 NT_MASK		= 0x00004000
 VM_MASK		= 0x00020000
 
+#ifndef CONFIG_XEN

arch/i386/kernel/entry.S NT_MASK = 0x00004000 · line 89

Quoted 10 lines
arch/i386/kernel/entry.S +10 -0
+#define DISABLE_INTERRUPTS	cli
+#define ENABLE_INTERRUPTS	sti
+#else
+#include <xen/interface/xen.h>
+
+EVENT_MASK	= 0x2E
+
+/* Offsets into shared_info_t. */
+#define evtchn_upcall_pending		/* 0 */
+#define evtchn_upcall_mask		1

Erk... Can we get these into asm-offsets?

Quoted 10 lines
arch/i386/kernel/entry.S +10 -0
+
+#ifdef CONFIG_SMP
+/* Set %esi to point to the appropriate vcpu structure */
+#define GET_VCPU_INFO		movl TI_cpu(%ebp),%esi			; \
+				shl  $SIZEOF_VCPU_INFO_SHIFT,%esi	; \
+				addl HYPERVISOR_shared_info,%esi
+#else
+#define GET_VCPU_INFO		movl HYPERVISOR_shared_info,%esi
+#endif
+

arch/i386/kernel/entry.S NT_MASK = 0x00004000 · line 109

Quoted 10 lines
arch/i386/kernel/entry.S +10 -0
+/* The following end up using/clobbering %esi, because of GET_VCPU_INFO */
+#define __DISABLE_INTERRUPTS	movb $1,evtchn_upcall_mask(%esi)
+#define DISABLE_INTERRUPTS	GET_VCPU_INFO				; \
+				__DISABLE_INTERRUPTS
+#define ENABLE_INTERRUPTS	GET_VCPU_INFO				; \
+				movb $0,evtchn_upcall_mask(%esi)
+#define __TEST_PENDING		testb $0xFF,evtchn_upcall_pending(%esi)
+#endif
+
+

Actually, is it possible to move these to a header somewhere? In the paravirt_ops patches I used the names CLI and STI (copied from the VMI patches), but didn't allow them to clobber any regs. They're defined in asm-i386/paravirt.h.

If we want to allow them to clobber %esi, then your names are probably better, but we should clobber %esi in the !CONFIG_XEN case too if CONFIG_DEBUG_KERNEL.

Rusty.

Signature · 1 line
Help! Save Australia from the worst of the DMCA: http://linux.org.au/law
#45 lore raw

Re: [RFC PATCH 16/33] Add support for Xen to entry.S.

Quoted 28 lines · * Rusty Russell ([email protected]) wrote:
Quoted 27 lines · Chris Wright

plain text document attachment (i386-entry.S)
- change cli/sti
- change test for user mode return to work for kernel mode in ring1
- check hypervisor saved event mask on return from exception
- add entry points for the hypervisor upcall handlers
- avoid math emulation check when running on Xen
- add nmi handler for running on Xen

---
 arch/i386/kernel/asm-offsets.c |   26 +++++++
 arch/i386/kernel/entry.S       |  141 ++++++++++++++++++++++++++++++++++++-----
 arch/i386/mach-xen/setup-xen.c |   19 +++++
 drivers/xen/core/features.c    |    2
 4 files changed, 169 insertions(+), 19 deletions(-)

diff -r 5cca1805b8a7 arch/i386/kernel/entry.S

arch/i386/kernel/entry.S +0 -0
@@ -76,8 +76,39 @@ NT_MASK		= 0x00004000
 NT_MASK		= 0x00004000
 VM_MASK		= 0x00020000
 
Quoted 13 lines

arch/i386/kernel/entry.S NT_MASK = 0x00004000 · line 89

Quoted 11 lines
arch/i386/kernel/entry.S +11 -0
+#ifndef CONFIG_XEN
+#define DISABLE_INTERRUPTS	cli
+#define ENABLE_INTERRUPTS	sti
+#else
+#include <xen/interface/xen.h>
+
+EVENT_MASK	= 0x2E
+
+/* Offsets into shared_info_t. */
+#define evtchn_upcall_pending		/* 0 */
+#define evtchn_upcall_mask		1

Erk... Can we get these into asm-offsets?

Hmm, we put the vcpu shift in, guess we missed that. Thanks for noticing.

Quoted 15 lines
Quoted 15 lines
arch/i386/kernel/entry.S +15 -0
+
+#ifdef CONFIG_SMP
+/* Set %esi to point to the appropriate vcpu structure */
+#define GET_VCPU_INFO		movl TI_cpu(%ebp),%esi			; \
+				shl  $SIZEOF_VCPU_INFO_SHIFT,%esi	; \
+				addl HYPERVISOR_shared_info,%esi
+#else
+#define GET_VCPU_INFO		movl HYPERVISOR_shared_info,%esi
+#endif
+
+/* The following end up using/clobbering %esi, because of GET_VCPU_INFO */
+#define __DISABLE_INTERRUPTS	movb $1,evtchn_upcall_mask(%esi)
+#define DISABLE_INTERRUPTS	GET_VCPU_INFO				; \
+				__DISABLE_INTERRUPTS
+#define ENABLE_INTERRUPTS	GET_VCPU_INFO				; \
Quoted 8 lines

arch/i386/kernel/entry.S NT_MASK = 0x00004000 · line 107

Quoted 3 lines
arch/i386/kernel/entry.S +3 -0
+				movb $0,evtchn_upcall_mask(%esi)
+#define __TEST_PENDING		testb $0xFF,evtchn_upcall_pending(%esi)
+#endif

Actually, is it possible to move these to a header somewhere? In the paravirt_ops patches I used the names CLI and STI (copied from the VMI patches), but didn't allow them to clobber any regs. They're defined in asm-i386/paravirt.h.

Sure, although it's only used here. Got a preference?

Quoted 3 lines

If we want to allow them to clobber %esi, then your names are probably better, but we should clobber %esi in the !CONFIG_XEN case too if CONFIG_DEBUG_KERNEL.

That makes sense to me, just to catch those bugs.

thanks,
-chris

#46 lore raw

Re: [RFC PATCH 16/33] Add support for Xen to entry.S.

Quoted 74 lines · Chris Wright
Quoted 29 lines · * Rusty Russell ([email protected]) wrote:
Quoted 27 lines · Chris Wright

plain text document attachment (i386-entry.S)
- change cli/sti
- change test for user mode return to work for kernel mode in ring1
- check hypervisor saved event mask on return from exception
- add entry points for the hypervisor upcall handlers
- avoid math emulation check when running on Xen
- add nmi handler for running on Xen

---
 arch/i386/kernel/asm-offsets.c |   26 +++++++
 arch/i386/kernel/entry.S       |  141 ++++++++++++++++++++++++++++++++++++-----
 arch/i386/mach-xen/setup-xen.c |   19 +++++
 drivers/xen/core/features.c    |    2
 4 files changed, 169 insertions(+), 19 deletions(-)

diff -r 5cca1805b8a7 arch/i386/kernel/entry.S

arch/i386/kernel/entry.S +0 -0
@@ -76,8 +76,39 @@ NT_MASK		= 0x00004000
 NT_MASK		= 0x00004000
 VM_MASK		= 0x00020000
 
Quoted 14 lines

arch/i386/kernel/entry.S NT_MASK = 0x00004000

Quoted 12 lines
arch/i386/kernel/entry.S +11 -0
+#ifndef CONFIG_XEN
+#define DISABLE_INTERRUPTS	cli
+#define ENABLE_INTERRUPTS	sti
+#else
+#include <xen/interface/xen.h>
+
+EVENT_MASK	= 0x2E
+
+/* Offsets into shared_info_t. */
+#define evtchn_upcall_pending		/* 0 */
+#define evtchn_upcall_mask		1
 

Erk... Can we get these into asm-offsets?

Hmm, we put the vcpu shift in, guess we missed that. Thanks for noticing.

Quoted 15 lines
Quoted 15 lines
arch/i386/kernel/entry.S +15 -0
+
+#ifdef CONFIG_SMP
+/* Set %esi to point to the appropriate vcpu structure */
+#define GET_VCPU_INFO		movl TI_cpu(%ebp),%esi			; \
+				shl  $SIZEOF_VCPU_INFO_SHIFT,%esi	; \
+				addl HYPERVISOR_shared_info,%esi
+#else
+#define GET_VCPU_INFO		movl HYPERVISOR_shared_info,%esi
+#endif
+
+/* The following end up using/clobbering %esi, because of GET_VCPU_INFO */
+#define __DISABLE_INTERRUPTS	movb $1,evtchn_upcall_mask(%esi)
+#define DISABLE_INTERRUPTS	GET_VCPU_INFO				; \
+				__DISABLE_INTERRUPTS
+#define ENABLE_INTERRUPTS	GET_VCPU_INFO				; \
Quoted 9 lines

arch/i386/kernel/entry.S NT_MASK = 0x00004000

Quoted 4 lines
arch/i386/kernel/entry.S +3 -0
+				movb $0,evtchn_upcall_mask(%esi)
+#define __TEST_PENDING		testb $0xFF,evtchn_upcall_pending(%esi)
+#endif
 

Actually, is it possible to move these to a header somewhere? In the paravirt_ops patches I used the names CLI and STI (copied from the VMI patches), but didn't allow them to clobber any regs. They're defined in asm-i386/paravirt.h.

Quoted 2 lines

Sure, although it's only used here. Got a preference?

It might be an idea to use something other than STI, CLI and CPUID, since the assembler is case-insensitive. We've already had one bug where CPUID didn't get replaced and the assembler quietly accepted it as-is.

J

#47 lore raw

Re: [RFC PATCH 16/33] Add support for Xen to entry.S.

Quoted 3 lines · * Jeremy Fitzhardinge ([email protected]) wrote:

It might be an idea to use something other than STI, CLI and CPUID, since the assembler is case-insensitive. We've already had one bug where CPUID didn't get replaced and the assembler quietly accepted it as-is.

Yes, having been bitten by that, I'd have to second that.

thanks,
-chris

#48 lore raw

RFC PATCH 17/33 Support loading an initrd when running on Xen

In reply to Chris Wright

Due to the initial physical memory layout when booting on Xen, the initrd image ends up below min_low_pfn (as registered with the bootstrap memory allocator). Add Xen subarch support to enable initrd_below_start_ok flag, and disable initrd_reserve_bootmem.

---
 arch/i386/kernel/setup.c                    |    4 +++-
 include/asm-i386/mach-xen/setup_arch_post.h |    6 ++++++
 2 files changed, 9 insertions(+), 1 deletion(-)
arch/i386/kernel/setup.c +3 -1
diff -r f4c730a90338 arch/i386/kernel/setup.c
@@ -1205,6 +1205,7 @@ extern unsigned long __init setup_memory
 extern unsigned long __init setup_memory(void);
 extern void zone_sizes_init(void);
 #endif /* !CONFIG_NEED_MULTIPLE_NODES */
+int initrd_reserve_bootmem = 1;
 
 void __init setup_bootmem_allocator(void)
 {
@@ -1265,7 +1266,8 @@ void __init setup_bootmem_allocator(void
 #ifdef CONFIG_BLK_DEV_INITRD
 	if (LOADER_TYPE && INITRD_START) {
 		if (INITRD_START + INITRD_SIZE <= (max_low_pfn << PAGE_SHIFT)) {
-			reserve_bootmem(INITRD_START, INITRD_SIZE);
+			if (initrd_reserve_bootmem)
+				reserve_bootmem(INITRD_START, INITRD_SIZE);
 			initrd_start =
 				INITRD_START ? INITRD_START + PAGE_OFFSET : 0;
 			initrd_end = initrd_start+INITRD_SIZE;
arch/i386/mach-xen/setup-xen.c +8 -0
diff -r f4c730a90338 arch/i386/mach-xen/setup-xen.c
@@ -5,6 +5,8 @@
 #include <linux/config.h>
 #include <linux/module.h>
 #include <linux/mm.h>
+#include <linux/bootmem.h>
+#include <linux/initrd.h>
 
 #include <asm/e820.h>
 #include <asm/setup.h>
@@ -68,4 +70,10 @@ void __init machine_specific_arch_setup(
 	memcpy(saved_command_line, xen_start_info->cmd_line,
 	       MAX_GUEST_CMDLINE > COMMAND_LINE_SIZE ?
 	       COMMAND_LINE_SIZE : MAX_GUEST_CMDLINE);
+
+	if (INITRD_START &&
+	    INITRD_START + PAGE_OFFSET < min_low_pfn << PAGE_SHIFT) {
+		initrd_reserve_bootmem = 0;
+		initrd_below_start_ok = 1;
+	}
 }
include/asm-i386/setup.h +1 -0
diff -r f4c730a90338 include/asm-i386/setup.h
@@ -66,6 +66,7 @@ extern unsigned char boot_params[PARAM_S
 #define LOWMEMSIZE()	(0x9f000)
 
 extern unsigned long init_pg_tables_end;
+extern int initrd_reserve_bootmem;
 
 struct e820entry;
 

--

#49 lore raw

RFC PATCH 18/33 Subarch support for CPUID instruction

In reply to Chris Wright

Allow subarchitectures to modify the CPUID instruction. This allows the subarch to provide a limited set of CPUID feature flags during CPU identification. Add a subarch implementation for Xen that traps to the hypervisor where unsupported feature flags can be hidden from guests.

---
 arch/i386/kernel/head-cpu.S                    |    4 ++--
 include/asm-i386/mach-default/mach_processor.h |    7 +++++++
 include/asm-i386/mach-xen/mach_processor.h     |    9 +++++++++
 include/asm-i386/processor.h                   |   13 +++++++------
 4 files changed, 25 insertions(+), 8 deletions(-)
arch/i386/kernel/head-cpu.S +4 -2
diff -r 122f3fbdb7a4 arch/i386/kernel/head-cpu.S
@@ -1,4 +1,6 @@
 /* Some macros for head.S */
+
+#include <mach_processor.h>
 
 .macro CPU_GDT_TABLE
 	.quad 0x0000000000000000	/* NULL descriptor */
@@ -57,7 +59,7 @@
 .macro CPUID_GET_VENDOR_INFO cpuid_level, x86_vendor_id
 	/* get vendor info */
 	xorl %eax,%eax			# call CPUID with 0 -> return vendor ID
-	cpuid
+	CPUID
 	movl %eax,\cpuid_level		# save CPUID level
 	movl %ebx,\x86_vendor_id	# lo 4 chars
 	movl %edx,\x86_vendor_id+4	# next 4 chars
@@ -73,7 +75,7 @@
  */
 .macro CPUID_GET_CPU_TYPE family, model, mask, capability
 	movl $1,%eax		# Use the CPUID instruction to get CPU type
-	cpuid
+	CPUID
 	movb %al,%cl		# save reg for future use
 	andb $0x0f,%ah		# mask processor family
 	movb %ah,\family
include/asm-i386/processor.h +7 -6
diff -r 122f3fbdb7a4 include/asm-i386/processor.h
@@ -20,6 +20,7 @@
 #include <linux/threads.h>
 #include <asm/percpu.h>
 #include <linux/cpumask.h>
+#include <mach_processor.h>
 
 /* flag for disabling the tsc */
 extern int tsc_disable;
@@ -147,7 +148,7 @@ static inline void detect_ht(struct cpui
  */
 static inline void cpuid(unsigned int op, unsigned int *eax, unsigned int *ebx, unsigned int *ecx, unsigned int *edx)
 {
-	__asm__("cpuid"
+	__asm__(CPUID_STR
 		: "=a" (*eax),
 		  "=b" (*ebx),
 		  "=c" (*ecx),
@@ -159,7 +160,7 @@ static inline void cpuid_count(int op, i
 static inline void cpuid_count(int op, int count, int *eax, int *ebx, int *ecx,
 	       	int *edx)
 {
-	__asm__("cpuid"
+	__asm__(CPUID_STR
 		: "=a" (*eax),
 		  "=b" (*ebx),
 		  "=c" (*ecx),
@@ -174,7 +175,7 @@ static inline unsigned int cpuid_eax(uns
 {
 	unsigned int eax;
 
-	__asm__("cpuid"
+	__asm__(CPUID_STR
 		: "=a" (eax)
 		: "0" (op)
 		: "bx", "cx", "dx");
@@ -184,7 +185,7 @@ static inline unsigned int cpuid_ebx(uns
 {
 	unsigned int eax, ebx;
 
-	__asm__("cpuid"
+	__asm__(CPUID_STR
 		: "=a" (eax), "=b" (ebx)
 		: "0" (op)
 		: "cx", "dx" );
@@ -194,7 +195,7 @@ static inline unsigned int cpuid_ecx(uns
 {
 	unsigned int eax, ecx;
 
-	__asm__("cpuid"
+	__asm__(CPUID_STR
 		: "=a" (eax), "=c" (ecx)
 		: "0" (op)
 		: "bx", "dx" );
@@ -204,7 +205,7 @@ static inline unsigned int cpuid_edx(uns
 {
 	unsigned int eax, edx;
 
-	__asm__("cpuid"
+	__asm__(CPUID_STR
 		: "=a" (eax), "=d" (edx)
 		: "0" (op)
 		: "bx", "cx");
include/asm-i386/mach-default/mach_processor.h new file +7 -0
diff -r 122f3fbdb7a4 include/asm-i386/mach-default/mach_processor.h
@@ -0,0 +1,7 @@
+#ifndef __ASM_MACH_PROCESSOR_H
+#define __ASM_MACH_PROCESSOR_H
+
+#define CPUID cpuid
+#define CPUID_STR "cpuid"
+
+#endif /* __ASM_MACH_PROCESSOR_H */
include/asm-i386/mach-xen/mach_processor.h new file +9 -0
diff -r 122f3fbdb7a4 include/asm-i386/mach-xen/mach_processor.h
@@ -0,0 +1,9 @@
+#ifndef __ASM_MACH_PROCESSOR_H
+#define __ASM_MACH_PROCESSOR_H
+
+#include <xen/interface/arch-x86_32.h>
+
+#define CPUID XEN_CPUID
+#define CPUID_STR XEN_CPUID
+
+#endif /* __ASM_MACH_PROCESSOR_H */

--

#50 lore raw

Re: [RFC PATCH 18/33] Subarch support for CPUID instruction

Quoted 5 lines · Chris Wright

plain text document attachment (i386-cpuid)
Allow subarchitectures to modify the CPUID instruction. This allows the subarch to provide a limited set of CPUID feature flags during CPU identification. Add a subarch implementation for Xen that traps to the hypervisor where unsupported feature flags can be hidden from guests.

Hi,

I'm wondering if this is entirely the wrong level of abstraction; to me it feels the subarch shouldn't override the actual cpuid, but the cpu feature flags that linux uses. That's a lot less messy: cpuid has many many pieces of information which are near impossible to filter in practice, however filtering the USAGE of it is trivial; linux basically flattens the cpuid namespace into a simple bitmap of "what the kernel can use". That is really what the subarch should filter/fixup, just like we do for cpu quirks etc etc.

Greetings,
Arjan van de Ven

#51 lore raw

Re: [RFC PATCH 18/33] Subarch support for CPUID instruction

Quoted 5 lines · Arjan van de Ven
Quoted 4 lines

Allow subarchitectures to modify the CPUID instruction. This allows the subarch to provide a limited set of CPUID feature flags during CPU identification. Add a subarch implementation for Xen that traps to the hypervisor where unsupported feature flags can be hidden from guests.

Quoted 8 lines

I'm wondering if this is entirely the wrong level of abstraction; to me it feels the subarch shouldn't override the actual cpuid, but the cpu feature flags that linux uses. That's a lot less messy: cpuid has many many pieces of information which are near impossible to filter in practice, however filtering the USAGE of it is trivial; linux basically flattens the cpuid namespace into a simple bitmap of "what the kernel can use". That is really what the subarch should filter/fixup, just like we do for cpu quirks etc etc.

Maybe we should have that *as well*, but it makes sense to allow the hypervisor to apply a filter too. For example, whether it supports PSE, FXSAVE/FXRSTOR, etc. These are things the 'platform' is telling the OS -- not something the OS can filter for itself. To trap on CPUID invocations requires the guest to use a special code sequence for CPUID, since the instruction will never normally fault. Hence moving to mach-* to hide this detail.

-- Keir

#52 lore raw

Re: [RFC PATCH 18/33] Subarch support for CPUID instruction

Quoted 18 lines · Keir Fraser
Quoted 6 lines · Arjan van de Ven
Quoted 5 lines

Allow subarchitectures to modify the CPUID instruction. This allows the subarch to provide a limited set of CPUID feature flags during CPU identification. Add a subarch implementation for Xen that traps to the
hypervisor where unsupported feature flags can be hidden from guests.

Quoted 9 lines

I'm wondering if this is entirely the wrong level of abstraction; to me it feels the subarch shouldn't override the actual cpuid, but the cpu feature flags that linux uses. That's a lot less messy: cpuid has many many pieces of information which are near impossible to filter in practice, however filtering the USAGE of it is trivial; linux basically flattens the cpuid namespace into a simple bitmap of "what the kernel can use". That is really what the subarch should filter/fixup, just like
we do for cpu quirks etc etc.

Quoted 4 lines

Maybe we should have that *as well*, but it makes sense to allow the hypervisor to apply a filter too. For example, whether it supports PSE, FXSAVE/FXRSTOR, etc. These are things the 'platform' is telling the OS -- not something the OS can filter for itself.

To some degree "Xen" is just a magic type of cpu that can/should have a quirk to filter these out... it just feels wrong and fragile to me to do it via a cpuid filter... once you have the "other" filter as I suggested I bet the need for the cpuid filter just goes away...

#53 lore raw

Re: [RFC PATCH 18/33] Subarch support for CPUID instruction

In reply to Arjan van de Ven
Quoted 11 lines · Arjan van de Ven
Quoted 6 lines · Chris Wright

plain text document attachment (i386-cpuid)
Allow subarchitectures to modify the CPUID instruction. This allows the subarch to provide a limited set of CPUID feature flags during CPU identification. Add a subarch implementation for Xen that traps to the hypervisor where unsupported feature flags can be hidden from guests.

Hi,

Quoted 9 lines

I'm wondering if this is entirely the wrong level of abstraction; to me it feels the subarch shouldn't override the actual cpuid, but the cpu feature flags that linux uses. That's a lot less messy: cpuid has many many pieces of information which are near impossible to filter in practice, however filtering the USAGE of it is trivial; linux basically flattens the cpuid namespace into a simple bitmap of "what the kernel can use". That is really what the subarch should filter/fixup, just like we do for cpu quirks etc etc.

You really need a CPUID hook. The instruction is non-virtualizable, and anything claiming to be a hypervisor really has to support masking and flattening the cpuid namespace for the instruction itself. It is used in assembler code and very early in boot. The alternative is injecting a bunch of Xen-specific code to filter feature bits into the i386 layer, which is both bad for Linux and bad for Xen - and was quite ugly in the last set of Xen patches.

Zach

maintainer
#54 lore raw

Re: [RFC PATCH 18/33] Subarch support for CPUID instruction

From: Zachary Amsden <[email protected]>
Date: Tue, 18 Jul 2006 04:33:52 -0700

Quoted 7 lines

You really need a CPUID hook. The instruction is non-virtualizable, and anything claiming to be a hypervisor really has to support masking and flattening the cpuid namespace for the instruction itself. It is used in assembler code and very early in boot. The alternative is injecting a bunch of Xen-specific code to filter feature bits into the i386 layer, which is both bad for Linux and bad for Xen - and was quite ugly in the last set of Xen patches.

Userspace will still see the full set of cpuid bits, since it can still execute cpuid unimpeded, is this ok?

#55 lore raw

Re: [RFC PATCH 18/33] Subarch support for CPUID instruction

Quoted 11 lines · * David Miller ([email protected]) wrote:

From: Zachary Amsden <[email protected]>
Date: Tue, 18 Jul 2006 04:33:52 -0700

Quoted 7 lines

You really need a CPUID hook. The instruction is non-virtualizable, and anything claiming to be a hypervisor really has to support masking and flattening the cpuid namespace for the instruction itself. It is used in assembler code and very early in boot. The alternative is injecting a bunch of Xen-specific code to filter feature bits into the i386 layer, which is both bad for Linux and bad for Xen - and was quite ugly in the last set of Xen patches.

Quoted 2 lines

Userspace will still see the full set of cpuid bits, since it can still execute cpuid unimpeded, is this ok?

Yup, it's for the kernel (things like systenter vs int 80 in vdso).

thanks,
-chris

#56 lore raw

RFC PATCH 19/33 Support gdt/idt/ldt handling on Xen.

In reply to Chris Wright

Move the macros which handle gdt/idt/ldt's into a subarch include file and add implementations for running on Xen.

---
 arch/i386/mach-xen/memory.c               |   17 +++++++
 include/asm-i386/desc.h                   |   65 ++---------------------------
 include/asm-i386/mach-default/mach_desc.h |   67 ++++++++++++++++++++++++++++++
 include/asm-i386/mach-xen/mach_desc.h     |   62 +++++++++++++++++++++++++++
 4 files changed, 151 insertions(+), 60 deletions(-)
arch/i386/mach-xen/memory.c +17 -0
diff -r 3e5209fcdb56 arch/i386/mach-xen/memory.c
@@ -29,3 +29,20 @@ void make_lowmem_page_readonly(unsigned 
 	rc = HYPERVISOR_update_va_mapping(address, pte_wrprotect(*pte), 0);
 	BUG_ON(rc);
 }
+
+void load_gdt(struct Xgt_desc_struct *gdt_descr)
+{
+	unsigned long frames[16];
+	unsigned long va;
+	int f;
+
+	for (va = gdt_descr->address, f = 0;
+	     va < gdt_descr->address + gdt_descr->size;
+	     va += PAGE_SIZE, f++) {
+		frames[f] = virt_to_mfn(va);
+		make_lowmem_page_readonly(va,
+					  XENFEAT_writable_descriptor_tables);
+	}
+	if (HYPERVISOR_set_gdt(frames, gdt_descr->size / 8))
+		BUG();
+}
include/asm-i386/desc.h +8 -52

98 lines not on this page · raw message

include/asm-i386/mach-default/mach_desc.h new file +62 -0

62 lines not on this page · raw message

include/asm-i386/mach-xen/mach_desc.h new file +51 -0

51 lines not on this page · raw message

--

#57 lore raw

RFC PATCH 20/33 subarch support for interrupt and exception gates

In reply to Chris Wright

Abstract the code that sets up interrupt and exception gates, and add a separate subarch implementation for Xen.

---
 arch/i386/kernel/traps.c                   |   35 ++++++-----------------------
 include/asm-i386/mach-default/mach_idt.h   |   24 +++++++++++++++++++
 include/asm-i386/mach-xen/mach_idt.h       |   25 ++++++++++++++++++++
 include/asm-i386/mach-xen/setup_arch_pre.h |    2 +
 4 files changed, 59 insertions(+), 27 deletions(-)
arch/i386/kernel/traps.c +8 -27
diff -r 29559215e367 arch/i386/kernel/traps.c
@@ -1084,29 +1084,11 @@ void __init trap_init_f00f_bug(void)
 }
 #endif
 
-#define _set_gate(gate_addr,type,dpl,addr,seg) \
-do { \
-  int __d0, __d1; \
-  __asm__ __volatile__ ("movw %%dx,%%ax\n\t" \
-	"movw %4,%%dx\n\t" \
-	"movl %%eax,%0\n\t" \
-	"movl %%edx,%1" \
-	:"=m" (*((long *) (gate_addr))), \
-	 "=m" (*(1+(long *) (gate_addr))), "=&a" (__d0), "=&d" (__d1) \
-	:"i" ((short) (0x8000+(dpl<<13)+(type<<8))), \
-	 "3" ((char *) (addr)),"2" ((seg) << 16)); \
-} while (0)
-
-
-/*
- * This needs to use 'idt_table' rather than 'idt', and
- * thus use the _nonmapped_ version of the IDT, as the
- * Pentium F0 0F bugfix can have resulted in the mapped
- * IDT being write-protected.
- */
+#include <mach_idt.h>
+
 void set_intr_gate(unsigned int n, void *addr)
 {
-	_set_gate(idt_table+n,14,0,addr,__KERNEL_CS);
+	_set_gate(n,14,0,addr,__KERNEL_CS);
 }
 
 /*
@@ -1114,24 +1096,23 @@ void set_intr_gate(unsigned int n, void 
  */
 static inline void set_system_intr_gate(unsigned int n, void *addr)
 {
-	_set_gate(idt_table+n, 14, 3, addr, __KERNEL_CS);
+	_set_gate(n, 14, 3, addr, __KERNEL_CS);
 }
 
 static void __init set_trap_gate(unsigned int n, void *addr)
 {
-	_set_gate(idt_table+n,15,0,addr,__KERNEL_CS);
+	_set_gate(n,15,0,addr,__KERNEL_CS);
 }
 
 static void __init set_system_gate(unsigned int n, void *addr)
 {
-	_set_gate(idt_table+n,15,3,addr,__KERNEL_CS);
+	_set_gate(n,15,3,addr,__KERNEL_CS);
 }
 
 static void __init set_task_gate(unsigned int n, unsigned int gdt_entry)
 {
-	_set_gate(idt_table+n,5,0,0,(gdt_entry<<3));
-}
-
+	_set_gate(n,5,0,0,(gdt_entry<<3));
+}
 
 void __init trap_init(void)
 {
arch/i386/mach-xen/setup-xen.c +2 -0
diff -r 29559215e367 arch/i386/mach-xen/setup-xen.c
@@ -23,6 +23,8 @@ EXPORT_SYMBOL(xen_start_info);
  */
 struct shared_info *HYPERVISOR_shared_info = (struct shared_info *)empty_zero_page;
 EXPORT_SYMBOL(HYPERVISOR_shared_info);
+
+struct trap_info xen_trap_table[257];
 
 /**
  * machine_specific_memory_setup - Hook for machine specific memory setup.
include/asm-i386/mach-xen/setup_arch.h +1 -0
diff -r 29559215e367 include/asm-i386/mach-xen/setup_arch.h
@@ -3,6 +3,7 @@
 
 extern struct start_info *xen_start_info;
 extern struct shared_info *HYPERVISOR_shared_info;
+extern struct trap_info xen_trap_table[257];
 
 void __init machine_specific_arch_setup(void);
 
include/asm-i386/mach-default/mach_idt.h new file +24 -0
diff -r 29559215e367 include/asm-i386/mach-default/mach_idt.h
@@ -0,0 +1,24 @@
+#ifndef __ASM_MACH_IDT_H
+#define __ASM_MACH_IDT_H
+
+/*
+ * This needs to use 'idt_table' rather than 'idt', and
+ * thus use the _nonmapped_ version of the IDT, as the
+ * Pentium F0 0F bugfix can have resulted in the mapped
+ * IDT being write-protected.
+ */
+#define _set_gate(vector,type,dpl,addr,seg) \
+do { \
+  int __d0, __d1; \
+  struct gate_struct *__gate_addr = idt_table + (vector); \
+  __asm__ __volatile__ ("movw %%dx,%%ax\n\t" \
+	"movw %4,%%dx\n\t" \
+	"movl %%eax,%0\n\t" \
+	"movl %%edx,%1" \
+	:"=m" (*((long *) __gate_addr)), \
+	 "=m" (*(1+(long *) __gate_addr)), "=&a" (__d0), "=&d" (__d1) \
+	:"i" ((short) (0x8000+(dpl<<13)+(type<<8))), \
+	 "3" ((char *) (addr)),"2" ((seg) << 16)); \
+} while (0)
+
+#endif /* __ASM_MACH_IDT_H */
include/asm-i386/mach-xen/mach_idt.h new file +25 -0
diff -r 29559215e367 include/asm-i386/mach-xen/mach_idt.h
@@ -0,0 +1,25 @@
+#ifndef __ASM_MACH_IDT_H
+#define __ASM_MACH_IDT_H
+
+static inline void _set_gate(unsigned int vector, uint8_t type, uint8_t dpl,
+			     void *addr, uint16_t seg)
+{
+	struct trap_info *t = xen_trap_table;
+
+	BUG_ON(vector > 256);
+
+	if (type == 5)
+		return;
+
+	while (t->address && t->vector != vector)
+		t++;
+
+	t->vector = vector;
+	t->cs = seg;
+	TI_SET_DPL(t, dpl);
+	if (type == 14 || vector == 7)
+		TI_SET_IF(t, 1);
+	t->address = (unsigned long)addr;
+}
+
+#endif /* __ASM_MACH_IDT_H */

--

#58 lore raw

RFC PATCH 21/33 subarch support for control register accesses

In reply to Chris Wright

Abstract the code that accesses control register, and add a separate subarch implementation for Xen.

---
 include/asm-i386/mach-default/mach_system.h |   57 +++++++++++++++++++++++++++
 include/asm-i386/mach-xen/mach_system.h     |   54 +++++++++++++++++++++++++
 include/asm-i386/system.h                   |   57 ---------------------------
 3 files changed, 111 insertions(+), 57 deletions(-)
include/asm-i386/mach-default/mach_system.h +57 -0
diff -r 6b500a8da576 include/asm-i386/mach-default/mach_system.h
@@ -3,4 +3,61 @@
 
 #define clearsegment(seg)
 
+#define read_cr0() ({ \
+	unsigned int __dummy; \
+	__asm__ __volatile__( \
+		"movl %%cr0,%0\n\t" \
+		:"=r" (__dummy)); \
+	__dummy; \
+})
+#define write_cr0(x) \
+	__asm__ __volatile__("movl %0,%%cr0": :"r" (x))
+
+#define read_cr2() ({ \
+	unsigned int __dummy; \
+	__asm__ __volatile__( \
+		"movl %%cr2,%0\n\t" \
+		:"=r" (__dummy)); \
+	__dummy; \
+})
+#define write_cr2(x) \
+	__asm__ __volatile__("movl %0,%%cr2": :"r" (x))
+
+#define read_cr3() ({ \
+	unsigned int __dummy; \
+	__asm__ ( \
+		"movl %%cr3,%0\n\t" \
+		:"=r" (__dummy)); \
+	__dummy; \
+})
+#define write_cr3(x) \
+	__asm__ __volatile__("movl %0,%%cr3": :"r" (x))
+
+#define read_cr4() ({ \
+	unsigned int __dummy; \
+	__asm__( \
+		"movl %%cr4,%0\n\t" \
+		:"=r" (__dummy)); \
+	__dummy; \
+})
+#define read_cr4_safe() ({			      \
+	unsigned int __dummy;			      \
+	/* This could fault if %cr4 does not exist */ \
+	__asm__("1: movl %%cr4, %0		\n"   \
+		"2:				\n"   \
+		".section __ex_table,\"a\"	\n"   \
+		".long 1b,2b			\n"   \
+		".previous			\n"   \
+		: "=r" (__dummy): "0" (0));	      \
+	__dummy;				      \
+})
+#define write_cr4(x) \
+	__asm__ __volatile__("movl %0,%%cr4": :"r" (x))
+
+/*
+ * Clear and set 'TS' bit respectively
+ */
+#define clts() __asm__ __volatile__ ("clts")
+#define stts() write_cr0(8 | read_cr0())
+
 #endif /* __ASM_MACH_SYSTEM_H */
include/asm-i386/mach-xen/mach_system.h +54 -0

58 lines not on this page · raw message

include/asm-i386/system.h +0 -57

63 lines not on this page · raw message

--

#59 lore raw

RFC PATCH 22/33 subarch stack pointer update

In reply to Chris Wright

Register the new kernel ('ring 0') stack pointer with the hypervisor during context switch.

---
 include/asm-i386/mach-default/mach_processor.h |    7 +++++++
 include/asm-i386/mach-xen/mach_processor.h     |    8 ++++++++
 include/asm-i386/processor.h                   |    1 +
 3 files changed, 16 insertions(+)
include/asm-i386/mach-default/mach_processor.h +7 -0
diff -r 60dd30eed8f3 include/asm-i386/mach-default/mach_processor.h
@@ -4,4 +4,11 @@
 #define CPUID cpuid
 #define CPUID_STR "cpuid"
 
+#ifndef __ASSEMBLY__
+static inline void mach_update_kernel_stack(unsigned long esp0,
+					    unsigned short ss0)
+{
+}
+#endif
+
 #endif /* __ASM_MACH_PROCESSOR_H */
include/asm-i386/mach-xen/mach_processor.h +8 -0
diff -r 60dd30eed8f3 include/asm-i386/mach-xen/mach_processor.h
@@ -6,4 +6,12 @@
 #define CPUID XEN_CPUID
 #define CPUID_STR XEN_CPUID
 
+#ifndef __ASSEMBLY__
+static inline void mach_update_kernel_stack(unsigned long esp0,
+					    unsigned short ss0)
+{
+	HYPERVISOR_stack_switch(ss0, esp0);
+}
+#endif
+
 #endif /* __ASM_MACH_PROCESSOR_H */
include/asm-i386/processor.h +1 -0
diff -r 60dd30eed8f3 include/asm-i386/processor.h
@@ -500,6 +500,7 @@ static inline void load_esp0(struct tss_
 		tss->ss1 = thread->sysenter_cs;
 		wrmsr(MSR_IA32_SYSENTER_CS, thread->sysenter_cs, 0);
 	}
+	mach_update_kernel_stack(tss->esp0, tss->ss0);
 }
 
 #define start_thread(regs, new_eip, new_esp) do {		\

--

#60 lore raw

RFC PATCH 23/33 subarch TLB support

In reply to Chris Wright

Paravirtualize TLB flushes by using the flush interfaces provided by the hypervisor. These hide the details of cross-CPU shootdowns and allow significant optimisations (for example, by avoiding shooting down on virtual CPUs that are descheduled). This is considerably faster in most cases than performing virtual IPIs in the guest kernel.

---
 include/asm-i386/mach-default/mach_tlbflush.h |   59 ++++++++++++++++++++++++++
 include/asm-i386/mach-xen/mach_tlbflush.h     |   25 +++++++++++
 include/asm-i386/tlbflush.h                   |   55 ------------------------
 3 files changed, 85 insertions(+), 54 deletions(-)
include/asm-i386/tlbflush.h +1 -54
diff -r 32949f913f10 include/asm-i386/tlbflush.h
@@ -5,64 +5,11 @@
 #include <linux/mm.h>
 #include <asm/processor.h>
 
-#define __flush_tlb()							\
-	do {								\
-		unsigned int tmpreg;					\
-									\
-		__asm__ __volatile__(					\
-			"movl %%cr3, %0;              \n"		\
-			"movl %0, %%cr3;  # flush TLB \n"		\
-			: "=r" (tmpreg)					\
-			:: "memory");					\
-	} while (0)
-
-/*
- * Global pages have to be flushed a bit differently. Not a real
- * performance problem because this does not happen often.
- */
-#define __flush_tlb_global()						\
-	do {								\
-		unsigned int tmpreg, cr4, cr4_orig;			\
-									\
-		__asm__ __volatile__(					\
-			"movl %%cr4, %2;  # turn off PGE     \n"	\
-			"movl %2, %1;                        \n"	\
-			"andl %3, %1;                        \n"	\
-			"movl %1, %%cr4;                     \n"	\
-			"movl %%cr3, %0;                     \n"	\
-			"movl %0, %%cr3;  # flush TLB        \n"	\
-			"movl %2, %%cr4;  # turn PGE back on \n"	\
-			: "=&r" (tmpreg), "=&r" (cr4), "=&r" (cr4_orig)	\
-			: "i" (~X86_CR4_PGE)				\
-			: "memory");					\
-	} while (0)
-
 extern unsigned long pgkern_mask;
-
-# define __flush_tlb_all()						\
-	do {								\
-		if (cpu_has_pge)					\
-			__flush_tlb_global();				\
-		else							\
-			__flush_tlb();					\
-	} while (0)
 
 #define cpu_has_invlpg	(boot_cpu_data.x86 > 3)
 
-#define __flush_tlb_single(addr) \
-	__asm__ __volatile__("invlpg %0": :"m" (*(char *) addr))
-
-#ifdef CONFIG_X86_INVLPG
-# define __flush_tlb_one(addr) __flush_tlb_single(addr)
-#else
-# define __flush_tlb_one(addr)						\
-	do {								\
-		if (cpu_has_invlpg)					\
-			__flush_tlb_single(addr);			\
-		else							\
-			__flush_tlb();					\
-	} while (0)
-#endif
+#include <mach_tlbflush.h>
 
 /*
  * TLB flushing:
include/asm-i386/mach-default/mach_tlbflush.h new file +59 -0

59 lines not on this page · raw message

include/asm-i386/mach-xen/mach_tlbflush.h new file +25 -0
diff -r 32949f913f10 include/asm-i386/mach-xen/mach_tlbflush.h
@@ -0,0 +1,25 @@
+#ifndef __ASM_MACH_TLBFLUSH_H
+#define __ASM_MACH_TLBFLUSH_H
+
+static inline void xen_tlb_flush(void)
+{
+        struct mmuext_op op;
+        op.cmd = MMUEXT_TLB_FLUSH_LOCAL;
+        BUG_ON(HYPERVISOR_mmuext_op(&op, 1, NULL, DOMID_SELF) < 0);
+}
+
+static inline void xen_invlpg(unsigned long ptr)
+{
+        struct mmuext_op op;
+        op.cmd = MMUEXT_INVLPG_LOCAL;
+        op.arg1.linear_addr = ptr & PAGE_MASK;
+        BUG_ON(HYPERVISOR_mmuext_op(&op, 1, NULL, DOMID_SELF) < 0);
+}
+
+#define __flush_tlb() xen_tlb_flush()
+#define __flush_tlb_global() xen_tlb_flush()
+#define __flush_tlb_all() xen_tlb_flush()
+#define __flush_tlb_single(addr) xen_invlpg(addr)
+#define __flush_tlb_one(addr) __flush_tlb_single(addr)
+
+#endif /* __ASM_MACH_TLBFLUSH_H */

--

maintainer
#61 lore raw

Re: [RFC PATCH 23/33] subarch TLB support

From: Chris Wright <[email protected]>
Date: Tue, 18 Jul 2006 00:00:23 -0700

Quoted 1 line

+ BUG_ON(HYPERVISOR_mmuext_op(&op, 1, NULL, DOMID_SELF) < 0);

Although it happens to work currently, I think we should get out of the habit of putting operations with wanted side effects into BUG_ON() calls. The following is therefore more preferable:

	ret = HYPERVISOR_mmuext_op(&op, 1, NULL, DOMID_SELF);
	BUG_ON(ret < 0);

If this were ASSERT() in userspace, turning off debugging at build time would make the evaluations inside of the macro never occur. It is my opinion that BUG_ON() should behave similarly.

#62 lore raw

Re: [RFC PATCH 23/33] subarch TLB support

Quoted 9 lines · * David Miller ([email protected]) wrote:

From: Chris Wright <[email protected]>
Date: Tue, 18 Jul 2006 00:00:23 -0700

Quoted 1 line

+ BUG_ON(HYPERVISOR_mmuext_op(&op, 1, NULL, DOMID_SELF) < 0);

Although it happens to work currently, I think we should get out of the habit of putting operations with wanted side effects into BUG_ON() calls. The following is therefore more preferable:

Quoted 6 lines
	ret = HYPERVISOR_mmuext_op(&op, 1, NULL, DOMID_SELF);
	BUG_ON(ret < 0);

If this were ASSERT() in userspace, turning off debugging at build time would make the evaluations inside of the macro never occur. It is my opinion that BUG_ON() should behave similarly.

Good point, I'll clean those up.

thanks,
-chris

#63 lore raw

RFC PATCH 24/33 Add support for Xen event channels.

In reply to Chris Wright

Support Xen event channels instead of the i8259 PIC.

Event channels are used to inject events into the kernel, either from the hypervisor or from another VM. The injected events are mapped to interrupts.

If an event needs to be injected, the hypervisor causes an upcall into the kernel. The upcall handler then scans the event pending bitmap and calls do_IRQ for each pending event.

---
 arch/i386/kernel/Makefile               |    6
 drivers/xen/core/evtchn.c               |  876 ++++++++++++++++++++++++++++++++
 include/asm-i386/hw_irq.h               |    4
 include/asm-i386/mach-xen/irq_vectors.h |  109 +++
 include/xen/evtchn.h                    |  116 ++++
 5 files changed, 1109 insertions(+), 2 deletions(-)
arch/i386/kernel/Makefile +5 -1
diff -r d67cfade70d7 arch/i386/kernel/Makefile
@@ -5,7 +5,7 @@ extra-y := head.o init_task.o vmlinux.ld
 extra-y := head.o init_task.o vmlinux.lds
 
 obj-y	:= process.o semaphore.o signal.o entry.o traps.o irq.o \
-		ptrace.o time.o ioport.o ldt.o setup.o i8259.o sys_i386.o \
+		ptrace.o time.o ioport.o ldt.o setup.o hw_irq.o sys_i386.o \
 		pci-dma.o i386_ksyms.o i387.o bootflag.o \
 		quirks.o i8237.o topology.o alternative.o i8253.o tsc.o
 
@@ -42,6 +42,10 @@ EXTRA_AFLAGS   := -traditional
 EXTRA_AFLAGS   := -traditional
 
 obj-$(CONFIG_SCx200)		+= scx200.o
+
+hw_irq-y			:= i8259.o
+
+hw_irq-$(CONFIG_XEN)		:= ../../../drivers/xen/core/evtchn.o
 
 # vsyscall.o contains the vsyscall DSO images as __initdata.
 # We must build both images before we can assemble it.
drivers/xen/core/evtchn.c new file +880 -0

880 lines not on this page · raw message

include/asm-i386/mach-xen/irq_vectors.h new file +109 -0

109 lines not on this page · raw message

include/xen/evtchn.h new file +116 -0

116 lines not on this page · raw message

--

#64 lore raw

RFC PATCH 25/33 Implement timekeeping for Xen

In reply to Chris Wright

Use the hypervisor as the basis of the guest's time. This means that the hypervisor wallclock time is used to emulate the cmos clock, and set the inital system clock at boot.

It also registers a Xen clocksource, so the system clock is kept in sync with the hypervisor's clock.

This does not implement setting the hypervisor clock; nor does it implement non-independent time, so hypervisor wallclock changes will not affect the guest.

---
 arch/i386/mach-default/Makefile       |    2
 arch/i386/mach-default/setup-time.c   |   16 +
 arch/i386/mach-default/setup.c        |   14 -
 drivers/xen/core/Makefile             |    2
 drivers/xen/core/time.c               |  285 +++++++++++++++++++++++++++++++++
 include/asm-i386/mach-xen/do_timer.h  |    3
 include/asm-i386/mach-xen/mach_time.h |   16 +
 7 files changed, 322 insertions(+), 16 deletions(-)
arch/i386/mach-default/Makefile +1 -1
diff -r 2657dae4b5cd arch/i386/mach-default/Makefile
@@ -2,4 +2,4 @@
 # Makefile for the linux kernel.
 #
 
-obj-y				:= setup.o setup-memory.o
+obj-y				:= setup.o setup-memory.o setup-time.o
arch/i386/mach-default/setup.c +0 -14
diff -r 2657dae4b5cd arch/i386/mach-default/setup.c
@@ -77,20 +77,6 @@ void __init trap_init_hook(void)
 {
 }
 
-static struct irqaction irq0  = { timer_interrupt, IRQF_DISABLED, CPU_MASK_NONE, "timer", NULL, NULL};
-
-/**
- * time_init_hook - do any specific initialisations for the system timer.
- *
- * Description:
- *	Must plug the system timer interrupt source at HZ into the IRQ listed
- *	in irq_vectors.h:TIMER_IRQ
- **/
-void __init time_init_hook(void)
-{
-	setup_irq(0, &irq0);
-}
-
 #ifdef CONFIG_MCA
 /**
  * mca_nmi_hook - hook into MCA specific NMI chain
drivers/xen/core/Makefile +1 -1
diff -r 2657dae4b5cd drivers/xen/core/Makefile
@@ -1,3 +1,3 @@
 
-obj-y	:= features.o
+obj-y	:= features.o time.o
 
arch/i386/mach-default/setup-time.c new file +16 -0
diff -r 2657dae4b5cd arch/i386/mach-default/setup-time.c
@@ -0,0 +1,16 @@
+#include <linux/interrupt.h>
+#include <asm/arch_hooks.h>
+
+static struct irqaction irq0  = { timer_interrupt, IRQF_DISABLED, CPU_MASK_NONE, "timer", NULL, NULL};
+
+/**
+ * time_init_hook - do any specific initialisations for the system timer.
+ *
+ * Description:
+ *	Must plug the system timer interrupt source at HZ into the IRQ listed
+ *	in irq_vectors.h:TIMER_IRQ
+ **/
+void __init time_init_hook(void)
+{
+	setup_irq(0, &irq0);
+}
drivers/xen/core/time.c new file +362 -0

362 lines not on this page · raw message

include/asm-i386/mach-xen/do_timer.h new file +3 -0
diff -r 2657dae4b5cd include/asm-i386/mach-xen/do_timer.h
@@ -0,0 +1,3 @@
+
+void do_timer_interrupt_hook(struct pt_regs *regs);
+
include/asm-i386/mach-xen/mach_time.h new file +16 -0
diff -r 2657dae4b5cd include/asm-i386/mach-xen/mach_time.h
@@ -0,0 +1,16 @@
+#ifndef _MACH_TIME_H
+#define _MACH_TIME_H
+
+/* Still need these definitions for other uses of cmos/nvram */
+#include <linux/mc146818rtc.h>
+
+/* for check timing call set_rtc_mmss() 500ms     */
+/* used in arch/i386/time.c::do_timer_interrupt() */
+#define USEC_AFTER	500000
+#define USEC_BEFORE	500000
+
+/* In Xen, "CMOS" time is fetched from the hypervisor */
+int mach_set_rtc_mmss(unsigned long nowtime);
+unsigned long mach_get_cmos_time(void);
+
+#endif /* !_MACH_TIME_H */

--

#65 lore raw

Re: [RFC PATCH 25/33] Implement timekeeping for Xen

Quoted 7 lines · Chris Wright

plain text document attachment (xen-time)
Use the hypervisor as the basis of the guest's time. This means that the hypervisor wallclock time is used to emulate the cmos clock, and set the inital system clock at boot.

It also registers a Xen clocksource, so the system clock is kept in sync with the hypervisor's clock.

Interesting. The Andi has been bugging me for a similarly designed per-cpu TSC clocksource, but just for generic use. I'm a little skeptical that it will be 100% without error, since anything dealing w/ the TSCs have been nothing but trouble in my mind, but this looks like a good proving ground for the concept.

It was mentioned to me that the clocksource approach helped cleanup some of the xen time changes (is that really true? :), but there were still some outstanding issues (time inconsistencies, perhaps?). I'm just curious if there are any details about the issues there, or if I misunderstood?

Quoted 3 lines

This does not implement setting the hypervisor clock; nor does it implement non-independent time, so hypervisor wallclock changes will not affect the guest.

Hmmm. I'm not sure if I understood that last line or not. I guess I need to think a bit about CLOCK_REALTIME vs CLOCK_MONOTONIC wrt hypervisiors.

I guess the question is "who owns time?" the guest OS (does it have its own CLOCK_REALTIME, independent of other guests?) or does the hypervisor? What does NTPd running on a guest actually adjust?

Anyway, some comments and boneheaded questions below.

Quoted 23 lines

diff -r 2657dae4b5cd drivers/xen/core/time.c

drivers/xen/core/time.c new file +19 -0
@@ -0,0 +1,362 @@
+/* 
+ * Xen-specific time functions
+ */
+
+#include <linux/kernel.h>
+#include <linux/time.h>
+#include <linux/interrupt.h>
+#include <linux/clocksource.h>
+#include <linux/kernel_stat.h>
+
+#include <asm/arch_hooks.h>
+#include <asm/hypervisor.h>
+
+#include <xen/evtchn.h>
+#include <xen/interface/xen.h>
+#include <xen/interface/vcpu.h>
+
+#include "mach_time.h"
+#include "do_timer.h"

drivers/xen/core/time.c line 29

Quoted 10 lines
drivers/xen/core/time.c new file +10 -0
+
+
+/* Permitted clock jitter, in nsecs, beyond which a warning will be printed. */
+static unsigned long permitted_clock_jitter = 10000000UL; /* 10ms */
+static int __init __permitted_clock_jitter(char *str)
+{
+	permitted_clock_jitter = simple_strtoul(str, NULL, 0);
+	return 1;
+}
+__setup("permitted_clock_jitter=", __permitted_clock_jitter);

permitted_clock_jitter is a little vague and might get confused w/ the NTP notion of jitter. Is there a better name, or could we get a xen_ prefix there?

drivers/xen/core/time.c line 37

Quoted 6 lines
drivers/xen/core/time.c new file +6 -0
+/* These are perodically updated in shared_info, and then copied here. */
+struct shadow_time_info {
+	u64 tsc_timestamp;     /* TSC at last update of time vals.  */
+	u64 system_timestamp;  /* Time, in nanosecs, since boot.    */
+	u32 tsc_to_nsec_mul;
+	u32 tsc_to_usec_mul;

Hmmm. Keeping separate cycle->usec and cycle->nsec multipliers is an interesting optimization. I'd even consider it for the generic clocksource code, but I suspect recalculating the independent adjustment factors for both kills the performance benefit. Have you actually compaired against the cost of the /1000 going from nsec to usec?

drivers/xen/core/time.c line 39

Quoted 2 lines
drivers/xen/core/time.c new file +2 -0
+	int tsc_shift;
+	u32 version;

Errr.. Why is a version value necessary?

drivers/xen/core/time.c line 46

Quoted 6 lines
drivers/xen/core/time.c new file +6 -0
+
+static DEFINE_PER_CPU(struct shadow_time_info, shadow_time);
+
+/* Keep track of last time we did processing/updating of jiffies and xtime. */
+static u64 processed_system_time;   /* System time (ns) at last processing. */
+static DEFINE_PER_CPU(u64, processed_system_time);

Errr. That would confuse me right off. Global and per-cpu values having the same name?

drivers/xen/core/time.c line 50

Quoted 3 lines
drivers/xen/core/time.c new file +3 -0
+/* How much CPU time was spent blocked and how much was 'stolen'? */
+static DEFINE_PER_CPU(u64, processed_stolen_time);
+static DEFINE_PER_CPU(u64, processed_blocked_time);

These seem like more generic accounting structures. Surely other virtualized arches have something similar? Something that should be looked into.

Quoted 49 lines
drivers/xen/core/time.c new file +49 -0
+/* Current runstate of each CPU (updated automatically by the hypervisor). */
+static DEFINE_PER_CPU(struct vcpu_runstate_info, runstate);
+
+/* Must be signed, as it's compared with s64 quantities which can be -ve. */
+#define NS_PER_TICK (1000000000LL/HZ)
+
+/*
+ * Reads a consistent set of time-base values from Xen, into a shadow data
+ * area.
+ */
+static void get_time_values_from_xen(void)
+{
+	struct shared_info      *s = HYPERVISOR_shared_info;
+	struct vcpu_time_info   *src;
+	struct shadow_time_info *dst;
+
+	src = &s->vcpu_info[smp_processor_id()].time;
+	dst = &per_cpu(shadow_time, smp_processor_id());
+
+	do {
+		dst->version = src->version;
+		rmb();
+		dst->tsc_timestamp     = src->tsc_timestamp;
+		dst->system_timestamp  = src->system_time;
+		dst->tsc_to_nsec_mul   = src->tsc_to_system_mul;
+		dst->tsc_shift         = src->tsc_shift;
+		rmb();
+	} while ((src->version & 1) | (dst->version ^ src->version));
+
+	dst->tsc_to_usec_mul = dst->tsc_to_nsec_mul / 1000;
+}
+
+static inline int time_values_up_to_date(int cpu)
+{
+	struct vcpu_time_info   *src;
+	struct shadow_time_info *dst;
+
+	src = &HYPERVISOR_shared_info->vcpu_info[cpu].time;
+	dst = &per_cpu(shadow_time, cpu);
+
+	rmb();
+	return (dst->version == src->version);
+}
+
+/*
+ * Scale a 64-bit delta by scaling and multiplying by a 32-bit fraction,
+ * yielding a 64-bit result.
+ */
+static inline u64 scale_delta(u64 delta, u32 mul_frac, int shift)

drivers/xen/core/time.c line 110

Quoted 10 lines
drivers/xen/core/time.c new file +10 -0
+{
+	u64 product;
+#ifdef __i386__
+	u32 tmp1, tmp2;
+#endif
+
+	if (shift < 0)
+		delta >>= -shift;
+	else
+		delta <<= shift;

I think there is a shift_right() macro that can avoid this.

Also I'm not sure I follow why you shift before multiply instead of multiply before shift? Does that not hurt your precision?

Quoted 11 lines
drivers/xen/core/time.c new file +11 -0
+#ifdef __i386__
+	__asm__ (
+		"mul  %5       ; "
+		"mov  %4,%%eax ; "
+		"mov  %%edx,%4 ; "
+		"mul  %5       ; "
+		"xor  %5,%5    ; "
+		"add  %4,%%eax ; "
+		"adc  %5,%%edx ; "
+		: "=A" (product), "=r" (tmp1), "=r" (tmp2)
+		: "a" ((u32)delta), "1" ((u32)(delta >> 32)), "2" (mul_frac) );

drivers/xen/core/time.c line 132

Quoted 10 lines
drivers/xen/core/time.c new file +10 -0
+#elif __x86_64__
+	__asm__ (
+		"mul %%rdx ; shrd $32,%%rdx,%%rax"
+		: "=a" (product) : "0" (delta), "d" ((u64)mul_frac) );
+#else
+#error implement me!
+#endif
+
+	return product;
+}

I think we need some generic mul_llxl_ll() wrappers here.

drivers/xen/core/time.c line 140

Quoted 8 lines
drivers/xen/core/time.c new file +8 -0
+
+static u64 get_nsec_offset(struct shadow_time_info *shadow)
+{
+	u64 now, delta;
+	rdtscll(now);
+	delta = now - shadow->tsc_timestamp;
+	return scale_delta(delta, shadow->tsc_to_nsec_mul, shadow->tsc_shift);
+}

get_nsec_offset is a little generic for a name. I know xen_ prefixes everywhere are irritating, but maybe something a little more specific would be a good idea.

Quoted 86 lines
drivers/xen/core/time.c new file +86 -0
+
+void do_timer_interrupt_hook(struct pt_regs *regs)
+{
+	s64 delta, delta_cpu, stolen, blocked;
+	u64 sched_time;
+	int i, cpu = smp_processor_id();
+	struct shadow_time_info *shadow = &per_cpu(shadow_time, cpu);
+	struct vcpu_runstate_info *runstate = &per_cpu(runstate, cpu);
+
+	do {
+		get_time_values_from_xen();
+
+		/* Obtain a consistent snapshot of elapsed wallclock cycles. */
+		delta = delta_cpu =
+			shadow->system_timestamp + get_nsec_offset(shadow);
+		delta     -= processed_system_time;
+		delta_cpu -= per_cpu(processed_system_time, cpu);
+
+		/*
+		 * Obtain a consistent snapshot of stolen/blocked cycles. We
+		 * can use state_entry_time to detect if we get preempted here.
+		 */
+		do {
+			sched_time = runstate->state_entry_time;
+			barrier();
+			stolen = runstate->time[RUNSTATE_runnable] +
+				runstate->time[RUNSTATE_offline] -
+				per_cpu(processed_stolen_time, cpu);
+			blocked = runstate->time[RUNSTATE_blocked] -
+				per_cpu(processed_blocked_time, cpu);
+			barrier();
+		} while (sched_time != runstate->state_entry_time);
+	} while (!time_values_up_to_date(cpu));
+
+	if ((unlikely(delta < -(s64)permitted_clock_jitter) ||
+	     unlikely(delta_cpu < -(s64)permitted_clock_jitter))
+	    && printk_ratelimit()) {
+		printk("Timer ISR/%d: Time went backwards: "
+		       "delta=%lld delta_cpu=%lld shadow=%lld "
+		       "off=%lld processed=%lld cpu_processed=%lld\n",
+		       cpu, delta, delta_cpu, shadow->system_timestamp,
+		       (s64)get_nsec_offset(shadow),
+		       processed_system_time,
+		       per_cpu(processed_system_time, cpu));
+		for (i = 0; i < num_online_cpus(); i++)
+			printk(" %d: %lld\n", i,
+			       per_cpu(processed_system_time, i));
+	}
+
+	/* System-wide jiffy work. */
+	while (delta >= NS_PER_TICK) {
+		delta -= NS_PER_TICK;
+		processed_system_time += NS_PER_TICK;
+		do_timer(regs);
+	}
+	/*
+	 * Account stolen ticks.
+	 * HACK: Passing NULL to account_steal_time()
+	 * ensures that the ticks are accounted as stolen.
+	 */
+	if ((stolen > 0) && (delta_cpu > 0)) {
+		delta_cpu -= stolen;
+		if (unlikely(delta_cpu < 0))
+			stolen += delta_cpu; /* clamp local-time progress */
+		do_div(stolen, NS_PER_TICK);
+		per_cpu(processed_stolen_time, cpu) += stolen * NS_PER_TICK;
+		per_cpu(processed_system_time, cpu) += stolen * NS_PER_TICK;
+		account_steal_time(NULL, (cputime_t)stolen);
+	}
+
+	/*
+	 * Account blocked ticks.
+	 * HACK: Passing idle_task to account_steal_time()
+	 * ensures that the ticks are accounted as idle/wait.
+	 */
+	if ((blocked > 0) && (delta_cpu > 0)) {
+		delta_cpu -= blocked;
+		if (unlikely(delta_cpu < 0))
+			blocked += delta_cpu; /* clamp local-time progress */
+		do_div(blocked, NS_PER_TICK);
+		per_cpu(processed_blocked_time, cpu) += blocked * NS_PER_TICK;
+		per_cpu(processed_system_time, cpu)  += blocked * NS_PER_TICK;
+		account_steal_time(idle_task(cpu), (cputime_t)blocked);
+	}
+
+	update_process_times(user_mode_vm(regs));

drivers/xen/core/time.c line 237

Quoted 10 lines
drivers/xen/core/time.c new file +10 -0
+}
+
+static cycle_t xen_clocksource_read(void)
+{
+	struct shadow_time_info *shadow = &per_cpu(shadow_time, smp_processor_id());
+
+	get_time_values_from_xen();
+
+	return shadow->system_timestamp + get_nsec_offset(shadow);
+}

Does get_time_values_from_xen() really need to be called on every clocksource_read call?

Quoted 41 lines
drivers/xen/core/time.c new file +41 -0
+static void xen_get_wallclock(struct timespec *ts)
+{
+	const struct shared_info *s = HYPERVISOR_shared_info;
+	u32 version;
+	u64 delta;
+	struct timespec now;
+
+	/* get wallclock at system boot */
+	do {
+		version = s->wc_version;
+		rmb();
+		now.tv_sec  = s->wc_sec;
+		now.tv_nsec = s->wc_nsec;
+		rmb();
+	} while ((s->wc_version & 1) | (version ^ s->wc_version));
+
+	delta = xen_clocksource_read();	/* time since system boot */
+	delta += now.tv_sec * (u64)NSEC_PER_SEC + now.tv_nsec;
+
+	now.tv_nsec = do_div(delta, NSEC_PER_SEC);
+	now.tv_sec = delta;
+
+	set_normalized_timespec(ts, now.tv_sec, now.tv_nsec);
+}
+
+unsigned long mach_get_cmos_time(void)
+{
+	struct timespec ts;
+
+	xen_get_wallclock(&ts);
+
+	return ts.tv_sec;
+}
+
+int mach_set_rtc_mmss(unsigned long now)
+{
+	/* do nothing for domU */
+	return -1;
+}
+
+static void init_cpu_khz(void)

drivers/xen/core/time.c line 289

Quoted 10 lines
drivers/xen/core/time.c new file +10 -0
+{
+	u64 __cpu_khz = 1000000ULL << 32;
+	struct vcpu_time_info *info;
+	info = &HYPERVISOR_shared_info->vcpu_info[0].time;
+	do_div(__cpu_khz, info->tsc_to_system_mul);
+	if (info->tsc_shift < 0)
+		cpu_khz = __cpu_khz << -info->tsc_shift;
+	else
+		cpu_khz = __cpu_khz >> info->tsc_shift;
+}

Err.. That could use some comments.

drivers/xen/core/time.c line 299

Quoted 9 lines
drivers/xen/core/time.c new file +9 -0
+static struct clocksource xen_clocksource = {
+	.name = "xen",
+	.rating = 400,
+	.read = xen_clocksource_read,
+	.mask = ~0,
+	.mult = 1,		/* time directly in nanoseconds */
+	.shift = 0,
+	.is_continuous = 1
+};

Hmmm. The 1/0 mul/shift pair is interesting. Is it expected that NTP does not ever adjust this clocksource? If not the clocksource_adjust() function won't do well with this at all, so you might consider something like:
#define XEN_SHIFT 22
.mult = 1<<XEN_SHIFT
.shift = XEN_SHIFT

Quoted 6 lines
drivers/xen/core/time.c new file +6 -0
+static void init_missing_ticks_accounting(int cpu)
+{
+	struct vcpu_register_runstate_memory_area area;
+	struct vcpu_runstate_info *runstate = &per_cpu(runstate, cpu);
+
+	memset(runstate, 0, sizeof(*runstate));

drivers/xen/core/time.c line 316

Quoted 10 lines
drivers/xen/core/time.c new file +10 -0
+
+	area.addr.v = runstate;
+	HYPERVISOR_vcpu_op(VCPUOP_register_runstate_memory_area, cpu, &area);
+
+	per_cpu(processed_blocked_time, cpu) =
+		runstate->time[RUNSTATE_blocked];
+	per_cpu(processed_stolen_time, cpu) =
+		runstate->time[RUNSTATE_runnable] +
+		runstate->time[RUNSTATE_offline];
+}

Again, this accounting seems like it could be generically useful.

Quoted 13 lines
drivers/xen/core/time.c new file +13 -0
+__init void time_init_hook(void)
+{
+	get_time_values_from_xen();
+
+	processed_system_time = per_cpu(shadow_time, 0).system_timestamp;
+	per_cpu(processed_system_time, 0) = processed_system_time;
+
+	init_cpu_khz();
+	printk(KERN_INFO "Xen reported: %u.%03u MHz processor.\n",
+	       cpu_khz / 1000, cpu_khz % 1000);
+
+	/* Cannot request_irq() until kmem is initialised. */
+	late_time_init = setup_cpu0_timer_irq;

drivers/xen/core/time.c line 362

Quoted 10 lines
drivers/xen/core/time.c new file +10 -0
+
+	init_missing_ticks_accounting(0);
+
+	clocksource_register(&xen_clocksource);
+
+	/* Set initial system time with full resolution */
+	xen_get_wallclock(&xtime);
+	set_normalized_timespec(&wall_to_monotonic,
+				-xtime.tv_sec, -xtime.tv_nsec);
+}

Some mention of which functions require to hold what on xtime_lock would be useful as well (applies to this function as well as the previous ones already commented on).

My only thoughts after looking at it: Using nanoseconds as a primary unit is often easier to work with, but less efficient. So rather then keeping a tsc_timestamp + system_timestamp in two different units, why not keep a calculated TSC base that includes the "cycles since boot" which is adjusted in the same manner internally to Xen as the system_timestamp is. Then let the timekeeping code do the conversion for you.

I haven't fully thought about what else it would affect in the above (I realize stolen_time, etc is in nsecs), but it might be something to consider.

Am I making any sense or just babbling?

thanks
-john

#66 lore raw

Re: [RFC PATCH 25/33] Implement timekeeping for Xen

Hi John,

I was hoping to catch up with you at OLS to talk about this, but, well, you know...

Quoted 6 lines · john stultz

Interesting. The Andi has been bugging me for a similarly designed per-cpu TSC clocksource, but just for generic use. I'm a little skeptical that it will be 100% without error, since anything dealing w/ the TSCs have been nothing but trouble in my mind, but this looks like a good proving ground for the concept.

I haven't had much direct experience with it, but Ian Pratt says it has been pretty successful in practice. The big problem has been CPUs (typically AMD) which drop into a thermal throttling state without telling anyone (no interrupt, etc). Xen doesn't really support CPU speed switching yet, so that hasn't been a problem.

Quoted 6 lines

It was mentioned to me that the clocksource approach helped cleanup some of the xen time changes (is that really true? :), but there were still some outstanding issues (time inconsistencies, perhaps?). I'm just curious if there are any details about the issues there, or if I misunderstood?

No, the time rework did make it much easier to plug Xen in. The original Xen time patch was basically a complete copy of arch/i386/kernel/time.c with additional hacks. That patch did do more than this once, since I was primarily aiming for basically functional & clean rather than fully functional, on the assumption that the patch would generate this discussion so we could sort out the rest of the issues.

This Xen time patch deals with:

* setting the system clock at boot
* ticks
* lost & stolen ticks
* tickless idle

In addition to this, we would also like to be able to absolutely slave the guest time to the hypervisor time, so that even if the hypervisor time changes there still a very small (or ideally 0) skew between the guest time.

Quoted 5 lines
Quoted 4 lines

This does not implement setting the hypervisor clock; nor does it implement non-independent time, so hypervisor wallclock changes will not affect the guest.

Quoted 8 lines

Hmmm. I'm not sure if I understood that last line or not. I guess I need to think a bit about CLOCK_REALTIME vs CLOCK_MONOTONIC wrt hypervisiors.

I guess the question is "who owns time?" the guest OS (does it have its own CLOCK_REALTIME, independent of other guests?) or does the hypervisor? What does NTPd running on a guest actually adjust?

In Xen there are two types of guest domain: dom0 and domU. Dom0 (of which there can be only one) can perform privileged hypervisor operations, has direct access to hardware, etc; only dom0 can change hypervisor time. DomU domains are typically not privileged, only have virtual devices, etc. With respect to time, domains can either operate with "dependent time" or "independent time".

With independent time, a guest maintains its own notion of time. It may be initially set from the hypervisor, or it might use something else, like ntp. It may also choose to use the hypervisor as a clock source, or it can use any other clock. This is the easy case, since its the same as a stand-alone kernel.

With dependent time, the guest time is slaved to the hypervisor time. At any point in time, all dependent-time guests should have a very small (ideally 0) skew between each other and the hypervisor itself, regardless of how the hypervisor time changes. It would never make sense to use ntp or a non-hypervisor clocksource in a dependent-time guest. settimeofday and adjtimex would be null operations (I'm not sure if it makes sense to have a dependent-time dom0).

There are two problems in implementing this at present. The first is that the clock abstractions have nicely moved all the maintenance of the system time of day into the core, with no need for the arch-specific code to know about it. This is nice, but it isn't clear to me how we'd implement a dependent-time guest.

The second problem is that at present the only way to set the hypervisor time is with a simple stepwise settimeofday interface, rather than a time-warping adjtimex interface. This means that there's at least time potential for a other guests with slaved clocks could see non-monotonic time. We probably need to have a full time implementation of a clock within Xen itself, so that the dom0 adjtimex calls just call down into it rather than really maintaining a local clock.

The added complication is that we don't want to make a real hypercall into the hypervisor every time to fetch the time. To avoid this there is a shared memory region mapped from the hypervisor into the guests with contains periodically updated time information. This is updated by the hypervisor timer interrupt (100Hz?), and so is fairly low precision. To get more precise time, the guests extrapolate from this using the tsc. The obvious trouble with this extrapolation is that if the hypervisor time slows down, the guest could see non-monotonic time because their extrapolation over-shoots the real hypervisor tick-to-tick time step.

Also, we had quite a few discussions at OLS about introducing a general hypervisor interface layer, and how to handle time is obviously a part of that. I would imagine that most of this is a common problem to any hypervisor-based system rather than something specific to Xen (though obviously the details might vary).

Quoted 5 lines
Quoted 5 lines
drivers/xen/core/time.c new file +5 -0
+/* Permitted clock jitter, in nsecs, beyond which a warning will be printed. */
+static unsigned long permitted_clock_jitter = 10000000UL; /* 10ms */
+static int __init __permitted_clock_jitter(char *str)
+{
+	permitted_clock_jitter = simple_strtoul(str, NULL, 0);
Quoted 9 lines

drivers/xen/core/time.c line 29

Quoted 4 lines
drivers/xen/core/time.c new file +3 -0
+	return 1;
+}
+__setup("permitted_clock_jitter=", __permitted_clock_jitter);

permitted_clock_jitter is a little vague and might get confused w/ the NTP notion of jitter. Is there a better name, or could we get a xen_ prefix there?

Sure. I copied this over without really looking into it deeply, so I need to work out what this really means. Would "permitted_backstep" might be a better name? Adding "xen_" is a no-brainer though, though it might be something which will be common to all systems with virtualized time.

Quoted 5 lines
Quoted 5 lines
drivers/xen/core/time.c new file +5 -0
+/* These are perodically updated in shared_info, and then copied here. */
+struct shadow_time_info {
+	u64 tsc_timestamp;     /* TSC at last update of time vals.  */
+	u64 system_timestamp;  /* Time, in nanosecs, since boot.    */
+	u32 tsc_to_nsec_mul;
Quoted 9 lines

drivers/xen/core/time.c line 37

Quoted 2 lines
drivers/xen/core/time.c new file +1 -0
+	u32 tsc_to_usec_mul;

Hmmm. Keeping separate cycle->usec and cycle->nsec multipliers is an interesting optimization. I'd even consider it for the generic clocksource code, but I suspect recalculating the independent adjustment factors for both kills the performance benefit. Have you actually compaired against the cost of the /1000 going from nsec to usec?

This is also something I copied over. It wasn't obvious to me that it would be a big win either. In fact, this patch doesn't use tsc_to_usec at all, so its completely redundant.

Quoted 6 lines

drivers/xen/core/time.c line 39

Quoted 3 lines
drivers/xen/core/time.c new file +2 -0
+	int tsc_shift;
+	u32 version;

Errr.. Why is a version value necessary?

This structure is derived from the time structure Xen maintains in shared memory. If the shared memory version matches the local shadow version, we don't need to sync the two (I'm not sure if this is actually used in this patch).

Quoted 11 lines

drivers/xen/core/time.c line 46

Quoted 7 lines
drivers/xen/core/time.c new file +6 -0
+
+static DEFINE_PER_CPU(struct shadow_time_info, shadow_time);
+
+/* Keep track of last time we did processing/updating of jiffies and xtime. */
+static u64 processed_system_time;   /* System time (ns) at last processing. */
+static DEFINE_PER_CPU(u64, processed_system_time);

Errr. That would confuse me right off. Global and per-cpu values having the same name?

Yes, I was wondering about that myself.

Quoted 9 lines

drivers/xen/core/time.c line 50

Quoted 4 lines
drivers/xen/core/time.c new file +3 -0
+/* How much CPU time was spent blocked and how much was 'stolen'? */
+static DEFINE_PER_CPU(u64, processed_stolen_time);
+static DEFINE_PER_CPU(u64, processed_blocked_time);

These seem like more generic accounting structures. Surely other virtualized arches have something similar? Something that should be looked into.

Yes.

Quoted 8 lines

drivers/xen/core/time.c line 110

Quoted 5 lines
drivers/xen/core/time.c new file +4 -0
+	if (shift < 0)
+		delta >>= -shift;
+	else
+		delta <<= shift;

I think there is a shift_right() macro that can avoid this.

OK.

Quoted 3 lines

Also I'm not sure I follow why you shift before multiply instead of multiply before shift? Does that not hurt your precision?

It would seem so. I'm not sure what the original thought was here.

Quoted 16 lines
Quoted 16 lines
drivers/xen/core/time.c new file +16 -0
+#ifdef __i386__
+	__asm__ (
+		"mul  %5       ; "
+		"mov  %4,%%eax ; "
+		"mov  %%edx,%4 ; "
+		"mul  %5       ; "
+		"xor  %5,%5    ; "
+		"add  %4,%%eax ; "
+		"adc  %5,%%edx ; "
+		: "=A" (product), "=r" (tmp1), "=r" (tmp2)
+		: "a" ((u32)delta), "1" ((u32)(delta >> 32)), "2" (mul_frac) );
+#elif __x86_64__
+	__asm__ (
+		"mul %%rdx ; shrd $32,%%rdx,%%rax"
+		: "=a" (product) : "0" (delta), "d" ((u64)mul_frac) );
+#else
Quoted 9 lines

drivers/xen/core/time.c line 132

Quoted 6 lines
drivers/xen/core/time.c new file +5 -0
+#error implement me!
+#endif
+
+	return product;
+}

I think we need some generic mul_llxl_ll() wrappers here.

OK.

Quoted 7 lines

Review

Quoted 3 lines
(quoted diff) +2 -0
+
+static u64 get_nsec_offset(struct shadow_time_info *shadow)

get_nsec_offset is a little generic for a name. I know xen_ prefixes everywhere are irritating, but maybe something a little more specific would be a good idea.

It's static, so it isn't affecting the global namespace. Do you just mean in terms of making it easy to find with tags or backtraces?

Quoted 4 lines
Quoted 4 lines
drivers/xen/core/time.c new file +4 -0
+static cycle_t xen_clocksource_read(void)
+{
+	struct shadow_time_info *shadow = &per_cpu(shadow_time, smp_processor_id());
+
Quoted 9 lines

drivers/xen/core/time.c line 237

Quoted 5 lines
drivers/xen/core/time.c new file +4 -0
+	get_time_values_from_xen();
+
+	return shadow->system_timestamp + get_nsec_offset(shadow);
+}

Does get_time_values_from_xen() really need to be called on every clocksource_read call?

In principle it shouldn't cost much, if anything. Hm, it doesn't look like it uses the version-comparison optimisation, but even without that it just copies some values with a low likelihood of needing to iterate. The version-comparison test would eliminate the tsc_to_usec divide as well (though that's redundant anyway).

We could call it less and rely on longer extrapolations of time, but I'm not sure it's worth it when traded against the possibility of non-monotonicity.

Quoted 6 lines
Quoted 6 lines
drivers/xen/core/time.c new file +6 -0
+static void init_cpu_khz(void)
+{
+	u64 __cpu_khz = 1000000ULL << 32;
+	struct vcpu_time_info *info;
+	info = &HYPERVISOR_shared_info->vcpu_info[0].time;
+	do_div(__cpu_khz, info->tsc_to_system_mul);
Quoted 9 lines

drivers/xen/core/time.c line 289

Quoted 6 lines
drivers/xen/core/time.c new file +5 -0
+	if (info->tsc_shift < 0)
+		cpu_khz = __cpu_khz << -info->tsc_shift;
+	else
+		cpu_khz = __cpu_khz >> info->tsc_shift;
+}

Err.. That could use some comments.

Yep.

Quoted 11 lines

drivers/xen/core/time.c line 299

Quoted 10 lines
drivers/xen/core/time.c new file +9 -0
+static struct clocksource xen_clocksource = {
+	.name = "xen",
+	.rating = 400,
+	.read = xen_clocksource_read,
+	.mask = ~0,
+	.mult = 1,		/* time directly in nanoseconds */
+	.shift = 0,
+	.is_continuous = 1
+};
Quoted 8 lines

Hmmm. The 1/0 mul/shift pair is interesting. Is it expected that NTP does not ever adjust this clocksource? If not the clocksource_adjust() function won't do well with this at all, so you might consider something like:
#define XEN_SHIFT 22
.mult = 1<<XEN_SHIFT
.shift = XEN_SHIFT

OK. I added the 1/0 pair without really thinking about the implications for ntp. It does make sense to use ntp, so I'll fix that.

Quoted 11 lines
Quoted 11 lines
drivers/xen/core/time.c new file +11 -0
+static void init_missing_ticks_accounting(int cpu)
+{
+	struct vcpu_register_runstate_memory_area area;
+	struct vcpu_runstate_info *runstate = &per_cpu(runstate, cpu);
+
+	memset(runstate, 0, sizeof(*runstate));
+
+	area.addr.v = runstate;
+	HYPERVISOR_vcpu_op(VCPUOP_register_runstate_memory_area, cpu, &area);
+
+	per_cpu(processed_blocked_time, cpu) =
Quoted 9 lines

drivers/xen/core/time.c line 316

Quoted 6 lines
drivers/xen/core/time.c new file +5 -0
+		runstate->time[RUNSTATE_blocked];
+	per_cpu(processed_stolen_time, cpu) =
+		runstate->time[RUNSTATE_runnable] +
+		runstate->time[RUNSTATE_offline];
+}

Again, this accounting seems like it could be generically useful.

OK.

Quoted 20 lines
Quoted 20 lines
drivers/xen/core/time.c new file +20 -0
+__init void time_init_hook(void)
+{
+	get_time_values_from_xen();
+
+	processed_system_time = per_cpu(shadow_time, 0).system_timestamp;
+	per_cpu(processed_system_time, 0) = processed_system_time;
+
+	init_cpu_khz();
+	printk(KERN_INFO "Xen reported: %u.%03u MHz processor.\n",
+	       cpu_khz / 1000, cpu_khz % 1000);
+
+	/* Cannot request_irq() until kmem is initialised. */
+	late_time_init = setup_cpu0_timer_irq;
+
+	init_missing_ticks_accounting(0);
+
+	clocksource_register(&xen_clocksource);
+
+	/* Set initial system time with full resolution */
+	xen_get_wallclock(&xtime);
Quoted 9 lines

drivers/xen/core/time.c line 362

Quoted 4 lines
drivers/xen/core/time.c new file +3 -0
+	set_normalized_timespec(&wall_to_monotonic,
+				-xtime.tv_sec, -xtime.tv_nsec);
+}

Some mention of which functions require to hold what on xtime_lock would be useful as well (applies to this function as well as the previous ones already commented on).

This is called from time_init(), which sets xtime without holding the lock. I originally took the lock here, but removed it when I noticed that time_init() didn't bother.

Quoted 8 lines

My only thoughts after looking at it: Using nanoseconds as a primary unit is often easier to work with, but less efficient. So rather then keeping a tsc_timestamp + system_timestamp in two different units, why not keep a calculated TSC base that includes the "cycles since boot" which is adjusted in the same manner internally to Xen as the system_timestamp is. Then let the timekeeping code do the conversion for you.

It's worth considering; we'll need to consider how that changes Xen's interface, but I think we'll need to look at that anyway.

Quoted 5 lines

I haven't fully thought about what else it would affect in the above (I realize stolen_time, etc is in nsecs), but it might be something to consider.

Am I making any sense or just babbling?

Definitely makes sense. And I'd like to know your thoughts about how we can take more direct control of the system wallclock in a clean and sensible manner (or perhaps we shouldn't; maybe the right answer is that all the guests run an ntp server pointing at dom0, though that has its own downsides).

J

#67 lore raw

RFC PATCH 26/33 subarch suport for idle loop (NO_IDLE_HZ for Xen)

In reply to Chris Wright

Paravirtualize the idle loop to explicitly trap to the hypervisor when blocking, and to use the NO_IDLE_HZ functionality introduced by s390 to inform the rcu subsystem that the CPU is quiescent.

---
 arch/i386/mach-xen/setup-xen.c        |   59 ++++++++++++++++++++++++++++++++++
 drivers/xen/Kconfig                   |    8 ++++
 drivers/xen/core/time.c               |   21 ++++++++++++
 include/asm-i386/mach-xen/mach_time.h |    3 +
 4 files changed, 91 insertions(+)
arch/i386/mach-xen/setup-xen.c +59 -0
diff -r 793d8e45fb1e arch/i386/mach-xen/setup-xen.c
@@ -7,6 +7,7 @@
 #include <linux/mm.h>
 #include <linux/bootmem.h>
 #include <linux/initrd.h>
+#include <linux/pm.h>
 
 #include <asm/e820.h>
 #include <asm/setup.h>
@@ -15,6 +16,9 @@
 
 #include <xen/interface/physdev.h>
 
+#include "mach_time.h"
+
+static void xen_idle(void);
 
 struct start_info *xen_start_info;
 EXPORT_SYMBOL(xen_start_info);
@@ -82,4 +86,59 @@ void __init machine_specific_arch_setup(
 		initrd_reserve_bootmem = 0;
 		initrd_below_start_ok = 1;
 	}
+
+
+	pm_idle = xen_idle;
 }
+
+/*
+ * stop_hz_timer / start_hz_timer - enter/exit 'tickless mode' on an idle cpu
+ * These functions are based on implementations from arch/s390/kernel/time.c
+ */
+static void stop_hz_timer(void)
+{
+	unsigned int cpu = smp_processor_id();
+	unsigned long j;
+
+	cpu_set(cpu, nohz_cpu_mask);
+
+	/* See matching smp_mb in rcu_start_batch in rcupdate.c.  These mbs  */
+	/* ensure that if __rcu_pending (nested in rcu_needs_cpu) fetches a  */
+	/* value of rcp->cur that matches rdp->quiescbatch and allows us to  */
+	/* stop the hz timer then the cpumasks created for subsequent values */
+	/* of cur in rcu_start_batch are guaranteed to pick up the updated   */
+	/* nohz_cpu_mask and so will not depend on this cpu.                 */
+
+	smp_mb();
+
+	/* Leave ourselves in 'tick mode' if rcu or softirq or timer pending. */
+	if (rcu_needs_cpu(cpu) || local_softirq_pending() ||
+	    (j = next_timer_interrupt(), time_before_eq(j, jiffies))) {
+		cpu_clear(cpu, nohz_cpu_mask);
+		j = jiffies + 1;
+	}
+
+	BUG_ON(HYPERVISOR_set_timer_op(jiffies_to_st(j)) != 0);
+}
+
+static void start_hz_timer(void)
+{
+	cpu_clear(smp_processor_id(), nohz_cpu_mask);
+}
+
+static void xen_idle(void)
+{
+	local_irq_disable();
+
+	if (need_resched())
+		local_irq_enable();
+	else {
+		current_thread_info()->status &= ~TS_POLLING;
+		smp_mb__after_clear_bit();
+		stop_hz_timer();
+		/* Blocking includes an implicit local_irq_enable(). */
+		HYPERVISOR_sched_op(SCHEDOP_block, 0);
+		start_hz_timer();
+		current_thread_info()->status |= TS_POLLING;
+	}
+}
drivers/xen/Kconfig +8 -0
diff -r 793d8e45fb1e drivers/xen/Kconfig
@@ -12,6 +12,14 @@ config XEN
 
 if XEN
 
+config NO_IDLE_HZ
+	bool
+	default y
+	help
+	  Switches the regular HZ timer off when the system is going idle.
+	  This helps Xen to detect that the Linux system is idle, reducing
+	  the overhead of idle systems.
+
 config XEN_SHADOW_MODE
 	bool
 	default y
drivers/xen/core/time.c +21 -0
diff -r 793d8e45fb1e drivers/xen/core/time.c
@@ -227,6 +227,27 @@ void do_timer_interrupt_hook(struct pt_r
 	update_process_times(user_mode_vm(regs));
 }
 
+
+/* Convert jiffies to Xen system time. */
+u64 jiffies_to_st(unsigned long j)
+{
+	unsigned long seq;
+	long delta;
+	u64 st;
+
+	do {
+		seq = read_seqbegin(&xtime_lock);
+		delta = j - jiffies;
+		/* NB. The next check can trigger in some wrap-around cases,
+		 * but that's ok: we'll just end up with a shorter timeout. */
+		if (delta < 1)
+			delta = 1;
+		st = processed_system_time + (delta * (u64)NS_PER_TICK);
+	} while (read_seqretry(&xtime_lock, seq));
+
+	return st;
+}
+
 static cycle_t xen_clocksource_read(void)
 {
 	struct shadow_time_info *shadow = &per_cpu(shadow_time, smp_processor_id());
include/asm-i386/mach-xen/mach_time.h +3 -0
diff -r 793d8e45fb1e include/asm-i386/mach-xen/mach_time.h
@@ -13,4 +13,7 @@ int mach_set_rtc_mmss(unsigned long nowt
 int mach_set_rtc_mmss(unsigned long nowtime);
 unsigned long mach_get_cmos_time(void);
 
+/* Convert jiffies to Xen system time. */
+u64 jiffies_to_st(unsigned long j);
+
 #endif /* !_MACH_TIME_H */

--

#68 lore raw

RFC PATCH 27/33 Add the Xen virtual console driver.

In reply to Chris Wright

This provides a bootstrap and ongoing emergency console which is intended to be available from very early during boot and at all times thereafter, in contrast with alternatives such as UDP-based syslogd, or logging in via ssh. The protocol is based on a simple shared-memory ring buffer.

---
 drivers/char/Kconfig               |    1
 drivers/xen/Makefile               |    1
 drivers/xen/console/Makefile       |    2
 drivers/xen/console/console.c      |  622 ++++++++++++++++++++++++++++++++++++
 drivers/xen/console/xencons_ring.c |  141 ++++++++
 include/xen/xencons.h              |   14
 6 files changed, 781 insertions(+)
drivers/char/Kconfig +1 -0
diff -r df362b1b58f8 drivers/char/Kconfig
@@ -6,6 +6,7 @@ menu "Character devices"
 
 config VT
 	bool "Virtual terminal" if EMBEDDED
+	depends on !XEN
 	select INPUT
 	default y if !VIOCONS
 	---help---
drivers/xen/Makefile +1 -0
diff -r df362b1b58f8 drivers/xen/Makefile
@@ -1,3 +1,4 @@
 
 obj-y	+= core/
+obj-y	+= console/
 
arch/i386/kernel/early_printk.c +2 -0
diff -r df362b1b58f8 arch/i386/kernel/early_printk.c
@@ -1,2 +1,4 @@
 
+#ifndef CONFIG_XEN
 #include "../../x86_64/kernel/early_printk.c"
+#endif
drivers/xen/console/Makefile new file +2 -0
diff -r df362b1b58f8 drivers/xen/console/Makefile
@@ -0,0 +1,2 @@
+
+obj-y	:= console.o xencons_ring.o
drivers/xen/console/console.c new file +663 -0

663 lines not on this page · raw message

drivers/xen/console/xencons_ring.c new file +141 -0

141 lines not on this page · raw message

include/xen/xencons.h new file +14 -0
diff -r df362b1b58f8 include/xen/xencons.h
@@ -0,0 +1,14 @@
+#ifndef __ASM_XENCONS_H__
+#define __ASM_XENCONS_H__
+
+void xencons_force_flush(void);
+void xencons_resume(void);
+
+/* Interrupt work hooks. Receive data, or kick data out. */
+void xencons_rx(char *buf, unsigned len, struct pt_regs *regs);
+void xencons_tx(void);
+
+int xencons_ring_init(void);
+int xencons_ring_send(const char *data, unsigned len);
+
+#endif /* __ASM_XENCONS_H__ */

--

#69 lore raw

Re: [RFC PATCH 27/33] Add the Xen virtual console driver.

drivers/xen/console/console.c line 330

Quoted 13 lines · Chris Wright
drivers/xen/console/console.c new file +13 -0
+			} else if (sysrq_requested) {
+				unsigned long sysrq_timeout =
+					sysrq_requested + HZ*2;
+				sysrq_requested = 0;
+				if (time_before(jiffies, sysrq_timeout)) {
+					spin_unlock_irqrestore(
+						&xencons_lock, flags);
+					handle_sysrq(
+						buf[i], regs, xencons_tty);
+					spin_lock_irqsave(
+						&xencons_lock, flags);
+					continue;
+				}

Lindent can be harmful...

Quoted 8 lines
drivers/xen/console/console.c new file +8 -0
+static void xencons_wait_until_sent(struct tty_struct *tty, int timeout)
+{
+	unsigned long orig_jiffies = jiffies;
+
+	if (TTY_INDEX(tty) != 0)
+		return;
+
+	while (DRV(tty->driver)->chars_in_buffer(tty)) {

drivers/xen/console/console.c line 537

Quoted 10 lines
drivers/xen/console/console.c new file +10 -0
+		set_current_state(TASK_INTERRUPTIBLE);
+		schedule_timeout(1);
+		if (signal_pending(current))
+			break;
+		if (timeout && time_after(jiffies, orig_jiffies + timeout))
+			break;
+	}
+
+	set_current_state(TASK_RUNNING);
+}

hmm somehow I find this code scary; we had similar code recently elsewhere where this turned out to be a real issue; you now sleep for "1" time, so you sleep for a fixed time if you aren't getting wakeups, but if you are getting wakeups your code is upside down, I would expect it to look like

+ set_current_state(TASK_INTERRUPTIBLE);
+ while (DRV(tty->driver)->chars_in_buffer(tty))
+ schedule_timeout(1);
+ if (signal_pending(current))
+ break;
+ if (timeout && time_after(jiffies, orig_jiffies + timeout))
+ break;
+ set_current_state(TASK_INTERRUPTIBLE);
+ }

instead, so that you don't have the wakeup race..

#70 lore raw

Re: [RFC PATCH 27/33] Add the Xen virtual console driver.

Quoted 16 lines · Arjan van de Ven

hmm somehow I find this code scary; we had similar code recently elsewhere where this turned out to be a real issue; you now sleep for "1" time, so you sleep for a fixed time if you aren't getting wakeups, but if you are getting wakeups your code is upside down, I would expect it to look like

drivers/xen/console/console.c new file +9 -0
+	set_current_state(TASK_INTERRUPTIBLE);
+	while (DRV(tty->driver)->chars_in_buffer(tty))
+		schedule_timeout(1);
+		if (signal_pending(current))
+			break;
+		if (timeout && time_after(jiffies, orig_jiffies + timeout))
+			break;
+		set_current_state(TASK_INTERRUPTIBLE);
+	}
Quoted 1 line

instead, so that you don't have the wakeup race..

There's no wakeup signal, so no possibility of a wakeup race. That's why we schedule_timeout() instead of wait_event() or similar. This code is only used to flush the console when the kernel crashes, so we can get the full oops, so waiting a little bit too long is acceptable.

Your suggested change is perhaps more idiomatic though, and less jarring for reviewers. :-)

Thanks for your comments by the way. Reviewing lots of patches isn't much fun.

-- Keir

#71 lore raw

Re: [RFC PATCH 27/33] Add the Xen virtual console driver.

In reply to Chris Wright
Quoted 9 lines · Chris Wright

plain text document attachment (xen-console)
This provides a bootstrap and ongoing emergency console which is intended to be available from very early during boot and at all times thereafter, in contrast with alternatives such as UDP-based syslogd, or logging in via ssh. The protocol is based on a simple shared-memory ring buffer.

+/*
+ * Modes:

Quoted 8 lines
+ *  'xencons=off'  [XC_OFF]:     Console is disabled.
+ *  'xencons=tty'  [XC_TTY]:     Console attached to '/dev/tty[0-9]+'.
+ *  'xencons=ttyS' [XC_SERIAL]:  Console attached to '/dev/ttyS[0-9]+'.

+ * [XC_DEFAULT]: DOM0 -> XC_SERIAL ; all others -> XC_TTY.
+ *
+ * NB. In mode XC_TTY, we create dummy consoles for tty2-63. This suppresses
+ * warnings from standard distro startup scripts.
+ */

The console driver should not be hijacking /dev/tty or /dev/ttyS devices. Instead, it should use its own char device, like the pSeries hypervisor console does (/dev/hvc).

Yes, I understand that installers, inittab, and securetty files will need to be updated. Luckily, there are a couple of distros interested in supporting Xen who I suspect will be able to help.

Signature · 2 lines
Hollis Blanchard
IBM Linux Technology Center
#72 lore raw

RFC PATCH 28/33 Add Xen grant table support

In reply to Chris Wright

Add Xen 'grant table' driver which allows granting of access to selected local memory pages by other virtual machines and, symmetrically, the mapping of remote memory pages which other virtual machines have granted access to.

This driver is a prerequisite for many of the Xen virtual device drivers, which grant the 'device driver domain' restricted and temporary access to only those memory pages that are currently involved in I/O operations.

---
 drivers/xen/core/Makefile |    2
 drivers/xen/core/gnttab.c |  422 ++++++++++++++++++++++++++++++++++++++++++++++
 include/xen/gnttab.h      |  105 +++++++++++
 3 files changed, 528 insertions(+), 1 deletion(-)
drivers/xen/core/Makefile +1 -1
diff -r ac01ce08887e drivers/xen/core/Makefile
@@ -1,3 +1,3 @@
 
-obj-y	:= features.o time.o
+obj-y	:= features.o time.o gnttab.o
 
drivers/xen/core/gnttab.c new file +426 -0

426 lines not on this page · raw message

include/xen/gnttab.h new file +107 -0

107 lines not on this page · raw message

--

#73 lore raw

Re: [Xen-devel] [RFC PATCH 28/33] Add Xen grant table support

I don't think that the current grant-table API/implementation quite meets the requirements for revocation.

Specifically I think the requirements are that:

1) When a domain starts to reclaim a set of resources it allocated for inter-domain communication this process is guaranteed to complete without cooperation from any other domain.

2) When a domain reclaims resources it allocated for inter-domain communication it can't cause a page fault or other exception in any other domain.

The current API/implementation meets the second requirement but not the first.

It's possible to make the current implementation meet the first requirement if you relax it a bit and allow the superuser to kill an offending domain via domain 0 but this expands the scope of the failure from a single inter-domain communication channel to a whole domain which is undesirable.

Another potential issue with grant-tables is support for efficient N-way communication but I haven't investigated that personally.

A final point is that quite a lot of fairly tricky code is required to combine grant-tables, event-channels and xenbus before you can create an inter-domain communication channel which can be correctly disconnected and reconnected across module load and unload.

It's appropriate to have a low-level API at the hypervisor because it keeps the hypervisor as small as possible and therefore easier to audit.

But, whatever the low-level API, whether grant-tables or something which has better support for revocation and n-way communication, I think there needs to be a small library to implement a higher level API that is more convenient for driver authors to use directly.

#74 lore raw

Re: [RFC PATCH 28/33] Add Xen grant table support

In reply to Chris Wright
Quoted 16 lines · Chris Wright
drivers/xen/core/gnttab.c new file +15 -0
+int gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly)
+{
+	u16 flags, nflags;
+
+	nflags = shared[ref].flags;
+	do {
+		if ((flags = nflags) & (GTF_reading|GTF_writing)) {
+			printk(KERN_ALERT "WARNING: g.e. still in use!\n");
+			return 0;
+		}
+	} while ((nflags = synch_cmpxchg(&shared[ref].flags, flags, 0)) !=
+		 flags);
+
+	return 1;
+}
Quoted 1 line

From patch 06/33:

+/*
+ * A grant table comprises a packed array of grant entries in one or more
+ * page frames shared between Xen and a guest.
+ * [XEN]: This field is written by Xen and read by the sharing guest.
+ * [GST]: This field is written by the guest and read by Xen.
+ */
+struct grant_entry {

+    /* GTF_xxx: various type and flag information.  [XEN,GST] */
+    uint16_t flags;
+    /* The domain being granted foreign privileges. [GST] */
+    domid_t  domid;
+    /*

+ * GTF_permit_access: Frame that @domid is allowed to map and
access. [GST]
+ * GTF_accept_transfer: Frame whose ownership transferred by
@domid. [XEN]
+ */
+ uint32_t frame;
+};

I object to these uses of (synch_)cmpxchg on a uint16_t in common code. Many architectures, including PowerPC, do not support 2-byte atomic operations, but this code is common to all Xen architectures.

Signature · 2 lines
Hollis Blanchard
IBM Linux Technology Center
#75 lore raw

Re: [RFC PATCH 28/33] Add Xen grant table support

Quoted 3 lines · Hollis Blanchard

I object to these uses of (synch_)cmpxchg on a uint16_t in common code. Many architectures, including PowerPC, do not support 2-byte atomic operations, but this code is common to all Xen architectures.

We'll use synch_cmpxchg_subword() in the next iteration of these patches. It's already been applied to our main Xen repository but hadn't been applied to our merge repo when these patches were created.

-- Keir

#76 lore raw

Re: [RFC PATCH 28/33] Add Xen grant table support

In reply to Hollis Blanchard
Quoted 3 lines

I object to these uses of (synch_)cmpxchg on a uint16_t in common code. Many architectures, including PowerPC, do not support 2-byte atomic operations, but this code is common to all Xen architectures.

RMW operations you mean, I suppose. On PowerPC, all (naturally aligned) halfword operations are atomic; there do not exist any RMW operations; but word and doubleword atomic RMW operations can be emulated.

Boils down to the same thing of course, but it isn't the same ;-)

Segher

#77 lore raw

RFC PATCH 29/33 Add Xen driver utility functions.

In reply to Chris Wright

Allocate/destroy a 'vmalloc' VM area: alloc_vm_area and free_vm_area The alloc function ensures that page tables are constructed for the region of kernel virtual address space and mapped into init_mm.

Lock an area so that PTEs are accessible in the current address space:
lock_vm_area and unlock_vm_area
The lock function prevents context switches to a lazy mm that doesn't have the area mapped into its page tables. It also ensures that the page tables are mapped into the current mm by causing the page fault handler to copy the page directory pointers from init_mm into the current mm.

---

 drivers/xen/Makefile      |    2 +
 drivers/xen/util.c        |   70 ++++++++++++++++++++++++++++++++++++++++++++++
 include/xen/driver_util.h |   16 ++++++++++
 3 files changed, 88 insertions(+)
drivers/xen/Makefile +2 -0
diff -r 797b2283b1d5 drivers/xen/Makefile
@@ -1,3 +1,5 @@
+
+obj-y	+= util.o
 
 obj-y	+= core/
 obj-y	+= console/
drivers/xen/util.c new file +70 -0

70 lines not on this page · raw message

include/xen/driver_util.h new file +16 -0
diff -r 797b2283b1d5 include/xen/driver_util.h
@@ -0,0 +1,16 @@
+
+#ifndef __ASM_XEN_DRIVER_UTIL_H__
+#define __ASM_XEN_DRIVER_UTIL_H__
+
+#include <linux/config.h>
+#include <linux/vmalloc.h>
+
+/* Allocate/destroy a 'vmalloc' VM area. */
+extern struct vm_struct *alloc_vm_area(unsigned long size);
+extern void free_vm_area(struct vm_struct *area);
+
+/* Lock an area so that PTEs are accessible in the current address space. */
+extern void lock_vm_area(struct vm_struct *area);
+extern void unlock_vm_area(struct vm_struct *area);
+
+#endif /* __ASM_XEN_DRIVER_UTIL_H__ */

--

#78 lore raw

RFC PATCH 30/33 Add the Xenbus sysfs and virtual device hotplug driver.

In reply to Chris Wright

This communicates with the machine control software via a registry residing in a controlling virtual machine. This allows dynamic creation, destruction and modification of virtual device configurations (network devices, block devices and CPUS, to name some examples).

---
 drivers/xen/Makefile               |    1
 drivers/xen/xenbus/Makefile        |    7
 drivers/xen/xenbus/xenbus_client.c |  528 ++++++++++++++++++++
 drivers/xen/xenbus/xenbus_comms.c  |  222 ++++++++
 drivers/xen/xenbus/xenbus_comms.h  |   43 +
 drivers/xen/xenbus/xenbus_probe.c  |  954 +++++++++++++++++++++++++++++++++++++
 drivers/xen/xenbus/xenbus_xs.c     |  828 ++++++++++++++++++++++++++++++++
 include/xen/xenbus.h               |  199 +++++++
 8 files changed, 2782 insertions(+)
drivers/xen/Makefile +1 -0
diff -r 1caedd7495e5 drivers/xen/Makefile
@@ -3,4 +3,5 @@ obj-y	+= util.o
 
 obj-y	+= core/
 obj-y	+= console/
+obj-y	+= xenbus/
 
drivers/xen/xenbus/Makefile new file +7 -0
diff -r 1caedd7495e5 drivers/xen/xenbus/Makefile
@@ -0,0 +1,7 @@
+obj-y	+= xenbus.o
+
+xenbus-objs =
+xenbus-objs += xenbus_client.o
+xenbus-objs += xenbus_comms.o
+xenbus-objs += xenbus_xs.o
+xenbus-objs += xenbus_probe.o
drivers/xen/xenbus/xenbus_client.c new file +528 -0

528 lines not on this page · raw message

drivers/xen/xenbus/xenbus_comms.c new file +222 -0

222 lines not on this page · raw message

drivers/xen/xenbus/xenbus_comms.h new file +43 -0

43 lines not on this page · raw message

drivers/xen/xenbus/xenbus_probe.c new file +954 -0

954 lines not on this page · raw message

drivers/xen/xenbus/xenbus_xs.c new file +828 -0

828 lines not on this page · raw message

include/xen/xenbus.h new file +199 -0

199 lines not on this page · raw message

--

#79 lore raw

RFC PATCH 31/33 Add Xen subarch reboot support

In reply to Chris Wright

Add remote reboot capability, so that a virtual machine can be rebooted, halted or 'powered off' by external management tools.

---
 arch/i386/kernel/Makefile |    1 
 drivers/xen/core/reboot.c |  232 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 233 insertions(+)
arch/i386/kernel/Makefile +1 -0
diff -r 3c95ca587d82 arch/i386/kernel/Makefile
@@ -48,6 +48,7 @@ hw_irq-y			:= i8259.o
 hw_irq-y			:= i8259.o
 
 hw_irq-$(CONFIG_XEN)		:= ../../../drivers/xen/core/evtchn.o
+reboot-$(CONFIG_XEN)		:= ../../../drivers/xen/core/reboot.o
 
 # vsyscall.o contains the vsyscall DSO images as __initdata.
 # We must build both images before we can assemble it.
drivers/xen/core/reboot.c new file +232 -0

232 lines not on this page · raw message

--

#80 lore raw

Re: [RFC PATCH 31/33] Add Xen subarch reboot support

drivers/xen/core/reboot.c line 69

Quoted 8 lines · Chris Wright

...

drivers/xen/core/reboot.c new file +3 -0
...
+EXPORT_SYMBOL(machine_restart);
+EXPORT_SYMBOL(machine_halt);
+EXPORT_SYMBOL(machine_power_off);
...

Why?

cu
Adrian

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed
#81 lore raw

RFC PATCH 32/33 Add the Xen virtual network device driver.

In reply to Chris Wright

The network device frontend driver allows the kernel to access network devices exported exported by a virtual machine containing a physical network device driver.

---

 drivers/net/Kconfig             |    2
 drivers/xen/Kconfig.net         |   14
 drivers/xen/Makefile            |    3
 drivers/xen/net_driver_util.c   |   58 +
 drivers/xen/netfront/Makefile   |    4
 drivers/xen/netfront/netfront.c | 1326 ++++++++++++++++++++++++++++++++++++++++ include/xen/net_driver_util.h   |   48 +
 7 files changed, 1455 insertions(+)
drivers/net/Kconfig +2 -0
diff -r eadc12b20f35 drivers/net/Kconfig
@@ -2325,6 +2325,8 @@ source "drivers/atm/Kconfig"
 
 source "drivers/s390/net/Kconfig"
 
+source "drivers/xen/Kconfig.net"
+
 config ISERIES_VETH
 	tristate "iSeries Virtual Ethernet driver support"
 	depends on PPC_ISERIES
drivers/xen/Makefile +2 -0
diff -r eadc12b20f35 drivers/xen/Makefile
@@ -5,3 +5,5 @@ obj-y	+= console/
 obj-y	+= console/
 obj-y	+= xenbus/
 
+obj-$(CONFIG_XEN_NETDEV_FRONTEND)	+= netfront/
+
drivers/xen/Kconfig.net new file +14 -0
diff -r eadc12b20f35 drivers/xen/Kconfig.net
@@ -0,0 +1,14 @@
+menu "Xen network device drivers"
+        depends on NETDEVICES && XEN
+
+config XEN_NETDEV_FRONTEND
+	tristate "Network-device frontend driver"
+	depends on XEN
+	default y
+	help
+	  The network-device frontend driver allows the kernel to access
+	  network interfaces within another guest OS. Unless you are building a
+	  dedicated device-driver domain, or your master control domain
+	  (domain 0), then you almost certainly want to say Y here.
+
+endmenu
drivers/xen/netfront/Makefile new file +4 -0
diff -r eadc12b20f35 drivers/xen/netfront/Makefile
@@ -0,0 +1,4 @@
+
+obj-$(CONFIG_XEN_NETDEV_FRONTEND)	:= xennet.o
+
+xennet-objs := netfront.o
drivers/xen/netfront/netfront.c new file +1584 -0

1,584 lines not on this page · raw message

--

#82 lore raw

Re: [RFC PATCH 32/33] Add the Xen virtual network device driver.

Quoted 18 lines · Chris Wright
drivers/xen/netfront/netfront.c new file +18 -0
+
+/** Send a packet on a net device to encourage switches to learn the
+ * MAC. We send a fake ARP request.
+ *
+ * @param dev device
+ * @return 0 on success, error code otherwise
+ */
+static int send_fake_arp(struct net_device *dev)
+{
+	struct sk_buff *skb;
+	u32             src_ip, dst_ip;
+
+	dst_ip = INADDR_BROADCAST;
+	src_ip = inet_select_addr(dev, dst_ip, RT_SCOPE_LINK);
+
+	/* No IP? Then nothing to do. */
+	if (src_ip == 0)
+		return 0;

drivers/xen/netfront/netfront.c line 452

Quoted 10 lines
drivers/xen/netfront/netfront.c new file +10 -0
+
+	skb = arp_create(ARPOP_REPLY, ETH_P_ARP,
+			 dst_ip, dev, src_ip,
+			 /*dst_hw*/ NULL, /*src_hw*/ NULL,
+			 /*target_hw*/ dev->dev_addr);
+	if (skb == NULL)
+		return -ENOMEM;
+
+	return dev_queue_xmit(skb);
+}

Hi,

Hmmm maybe it's me, but something bugs me if a NIC driver is going to send IP level ARP packets... that just feels very very wrong and is a blatant layering violation.... shouldn't the ifup/ifconfig scripts just be fixed instead if this is critical behavior?

Greetings,
Arjan van de Ven

#83 lore raw

Re: [RFC PATCH 32/33] Add the Xen virtual network device driver.

Quoted 4 lines · Arjan van de Ven

Hmmm maybe it's me, but something bugs me if a NIC driver is going to send IP level ARP packets... that just feels very very wrong and is a blatant layering violation.... shouldn't the ifup/ifconfig scripts just be fixed instead if this is critical behavior?

Maybe we should be faking this out from our hotplug scripts in the control VM, although triggering this from user space is probably a bit of a pain. Regardless, the function can be removed from the driver if it's too distasteful: it's only a performance 'hack' to get network traffic more quickly redirected when migrating a VM between physical hosts. Things won't break horribly if it's removed.

-- Keir

#84 lore raw

Re: [RFC PATCH 32/33] Add the Xen virtual network device driver.

Quoted 7 lines · Keir Fraser
Quoted 4 lines · Arjan van de Ven

Hmmm maybe it's me, but something bugs me if a NIC driver is going to send IP level ARP packets... that just feels very very wrong and is a blatant layering violation.... shouldn't the ifup/ifconfig scripts just be fixed instead if this is critical behavior?

Quoted 6 lines

Maybe we should be faking this out from our hotplug scripts in the control VM, although triggering this from user space is probably a bit of a pain. Regardless, the function can be removed from the driver if it's too distasteful: it's only a performance 'hack' to get network traffic more quickly redirected when migrating a VM between physical hosts. Things won't break horribly if it's removed.

that sounds like a perfect job for a udev-kind of notification to userspace, eg "hey I migrated", and then udev/hotplug can just do this sending of arps.. maybe they also want to do other things just after a migration (which sounds entirely reasonable to me) such as renewing the dhcp lease, resetting the periodic userspace hardware polling timers in "hal", network manager etc etc...

so it looks entirely useful for me to have a userspace upcall for "post migration", and once you have that that udev side of things can just do this...

(question is if you also want a pre-migration upcall; my gut feeling says "no" because upcalls are async and you may end up running that upcall on the new physical machine)

Greetings,
Arjan van de Ven

#85 lore raw

Re: [RFC PATCH 32/33] Add the Xen virtual network device driver.

Quoted 17 lines · Arjan van de Ven
Quoted 7 lines · Keir Fraser
Quoted 4 lines · Arjan van de Ven

Hmmm maybe it's me, but something bugs me if a NIC driver is going to send IP level ARP packets... that just feels very very wrong and is a blatant layering violation.... shouldn't the ifup/ifconfig scripts just be fixed instead if this is critical behavior?

Quoted 6 lines

Maybe we should be faking this out from our hotplug scripts in the control VM, although triggering this from user space is probably a bit of a pain. Regardless, the function can be removed from the driver if it's too distasteful: it's only a performance 'hack' to get network traffic more quickly redirected when migrating a VM between physical hosts. Things won't break horribly if it's removed.

Quoted 6 lines

that sounds like a perfect job for a udev-kind of notification to userspace, eg "hey I migrated", and then udev/hotplug can just do this sending of arps.. maybe they also want to do other things just after a migration (which sounds entirely reasonable to me) such as renewing the dhcp lease, resetting the periodic userspace hardware polling timers in "hal", network manager etc etc...

I think we had this discussion the last time these patches came around. Other virtual environments (VMWare, POWER) have the same issue, and there are probably other scenarios (bonding?) that could use the same function. This doesn't belong in the driver.

Dave B

#86 lore raw

Re: [RFC PATCH 32/33] Add the Xen virtual network device driver.

In reply to Arjan van de Ven
Quoted 4 lines · Arjan van de Ven

Hmmm maybe it's me, but something bugs me if a NIC driver is going to send IP level ARP packets... that just feels very very wrong and is a blatant layering violation....

It is but the bonding driver has been setting precedence for years now on sending ARPs from a driver;->
It does make a lot of sense to put it in user space. More interesting policies may include sending more than just ARPs and once you hard-code in the kernel you loose that flexibility.

Quoted 3 lines

shouldn't the ifup/ifconfig scripts just
be fixed instead if this is critical behavior?

I dont think the ifup/ifconfig provide operational status (i.e link up/down) - or do they? If they can be made to invoke scripts in such a case then we are set.

Note: you will get netlink events when devices are created or devices change their admin (via ifconfig) or operational (link down/up) status. [Try running "ip monitor" to see]

One could write a little daemon that reacts to these specific events. The problem has been some people claiming that daemons are a bad idea from a usability perspective. Patrick has mentioned he may be working on a daemon in user space that does exactly that. The other alternative is to do the udev thing and have the kernel invoke a script whenever an event of interest happens.

cheers,
jamal

#87 lore raw

Re: [RFC PATCH 32/33] Add the Xen virtual network device driver.

Quoted 4 lines · jamal

I dont think the ifup/ifconfig provide operational status (i.e link up/down) - or do they? If they can be made to invoke scripts in such a case then we are set.

In fact, that's a very good reason why this shouldn't be in netfront. Indeed, it shouldn't be in the guest at all. The reason is that the guest has no idea whether the physical carrier is present.

It's much better for the host to send the ARP packet on behalf of the guest since the host knows the carrier status and the guest's MAC address.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#88 lore raw

Re: [RFC PATCH 32/33] Add the Xen virtual network device driver.

Quoted 5 lines · Herbert Xu
Quoted 3 lines · jamal

I dont think the ifup/ifconfig provide operational status (i.e link up/down) - or do they? If they can be made to invoke scripts in such a case then we are set.

Quoted 7 lines

In fact, that's a very good reason why this shouldn't be in netfront. Indeed, it shouldn't be in the guest at all. The reason is that the guest has no idea whether the physical carrier is present.

It's much better for the host to send the ARP packet on behalf of the guest since the host knows the carrier status and the guest's MAC address.

But sending ARPs is not the right thing if the guest is expecting to use IPv6 networking, in which case unsolicited neighbor advertisements are the right thing to do. The driver just doesn't seem to be the right place to do this, as it doesn't/ shouldn't need to know the difference between IPv4/IPv6.

#89 lore raw

Re: [RFC PATCH 32/33] Add the Xen virtual network device driver.

Quoted 6 lines · John Haller

But sending ARPs is not the right thing if the guest is expecting to use IPv6 networking, in which case unsolicited neighbor advertisements are the right thing to do. The driver just doesn't seem to be the right place to do this, as it doesn't/ shouldn't need to know the difference between IPv4/IPv6.

In this case it doesn't really matter because AFAIK they're trying to get switches to notice that the MAC has moved. So all you need is some packet that the switches can grok.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#90 lore raw

Re: [RFC PATCH 32/33] Add the Xen virtual network device driver.

In reply to Chris Wright

drivers/xen/netfront/netfront.c line 77

Quoted 12 lines

diff -r eadc12b20f35 drivers/xen/netfront/netfront.c

drivers/xen/netfront/netfront.c new file +6 -0
@@ -0,0 +1,1584 @@
 
+static inline void init_skb_shinfo(struct sk_buff *skb)
+{
+	atomic_set(&(skb_shinfo(skb)->dataref), 1);
+	skb_shinfo(skb)->nr_frags = 0;
+	skb_shinfo(skb)->frag_list = NULL;
+}

Shouldn't this move to skbuff.h?

#91 lore raw

Re: [RFC PATCH 32/33] Add the Xen virtual network device driver.

Quoted 6 lines · Stephen Hemminger
Quoted 5 lines

diff -r eadc12b20f35 drivers/xen/netfront/netfront.c

drivers/xen/netfront/netfront.c Fri Jun 09 15:03:12 2006 -0400 renamed from dev/null Thu Jan 01 00:00:00 1970 +0000 +0 -0
@@ -0,0 +1,1584 @@
Quoted 8 lines

drivers/xen/netfront/netfront.c Fri Jun 09 15:03:12 2006 -0400 line 6

Quoted 6 lines
drivers/xen/netfront/netfront.c Fri Jun 09 15:03:12 2006 -0400 renamed from dev/null Thu Jan 01 00:00:00 1970 +0000 +6 -0
+static inline void init_skb_shinfo(struct sk_buff *skb)
+{
+     atomic_set(&(skb_shinfo(skb)->dataref), 1);
+     skb_shinfo(skb)->nr_frags = 0;
+     skb_shinfo(skb)->frag_list = NULL;
+}

Shouldn't this move to skbuff.h?

If and when my dom0=>domU GSO patches are applied, this will simply disappear.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
maintainer
#92 lore raw

Re: [RFC PATCH 32/33] Add the Xen virtual network device driver.

In reply to Chris Wright

From: Chris Wright <[email protected]>
Date: Tue, 18 Jul 2006 00:00:32 -0700

drivers/xen/netfront/netfront.c line 61

Quoted 3 lines
drivers/xen/netfront/netfront.c new file +3 -0
+#ifdef CONFIG_XEN_BALLOON
+#include <xen/balloon.h>
+#endif

Let's put the ifdefs in xen/balloon.h not in the files including it.

drivers/xen/netfront/netfront.c line 629

Quoted 4 lines
drivers/xen/netfront/netfront.c new file +4 -0
+#ifdef CONFIG_XEN_BALLOON
+	/* Tell the ballon driver what is going on. */
+	balloon_update_driver_allowance(i);
+#endif

Similarly let's define empty do-nothing functions in xen/balloon.h when the config option isn't set so we don't need to crap up the C sources with these ifdefs.

#93 lore raw

Re: [RFC PATCH 32/33] Add the Xen virtual network device driver.

Quoted 10 lines · * David Miller ([email protected]) wrote:

From: Chris Wright <[email protected]>
Date: Tue, 18 Jul 2006 00:00:32 -0700

drivers/xen/netfront/netfront.c line 61

Quoted 3 lines
drivers/xen/netfront/netfront.c new file +3 -0
+#ifdef CONFIG_XEN_BALLOON
+#include <xen/balloon.h>
+#endif

Let's put the ifdefs in xen/balloon.h not in the files including it.

Quoted 8 lines

drivers/xen/netfront/netfront.c line 629

Quoted 4 lines
drivers/xen/netfront/netfront.c new file +4 -0
+#ifdef CONFIG_XEN_BALLOON
+	/* Tell the ballon driver what is going on. */
+	balloon_update_driver_allowance(i);
+#endif

Similarly let's define empty do-nothing functions in xen/balloon.h when the config option isn't set so we don't need to crap up the C sources with these ifdefs.

Yeah, sorry it's kept more as a reminder to me. Upstream Xen doesn't do this, but this patchset doesn't support ballooning yet, so there just is not xen/balloon.h in the set. When merging with upstream Xen there's patch rejects either way, so I agree, better to just drop this since it's just future placeholder.

thanks,
-chris

#94 lore raw

RFC PATCH 33/33 Add Xen virtual block device driver.

In reply to Chris Wright

The block device frontend driver allows the kernel to access block devices exported exported by a virtual machine containing a physical block device driver.

---
 drivers/block/Kconfig           |    2
 drivers/xen/Kconfig.blk         |   14
 drivers/xen/Makefile            |    1
 drivers/xen/blkfront/Makefile   |    5
 drivers/xen/blkfront/blkfront.c |  813 ++++++++++++++++++++++++++++++++++++++++
 drivers/xen/blkfront/block.h    |  156 +++++++
 drivers/xen/blkfront/vbd.c      |  216 ++++++++++
 7 files changed, 1207 insertions(+)
drivers/block/Kconfig +2 -0
diff -r e6d9b50572f5 drivers/block/Kconfig
@@ -450,6 +450,8 @@ config CDROM_PKTCDVD_WCACHE
 
 source "drivers/s390/block/Kconfig"
 
+source "drivers/xen/Kconfig.blk"
+
 config ATA_OVER_ETH
 	tristate "ATA over Ethernet support"
 	depends on NET
drivers/xen/Makefile +1 -0
diff -r e6d9b50572f5 drivers/xen/Makefile
@@ -5,5 +5,6 @@ obj-y	+= console/
 obj-y	+= console/
 obj-y	+= xenbus/
 
+obj-$(CONFIG_XEN_BLKDEV_FRONTEND)	+= blkfront/
 obj-$(CONFIG_XEN_NETDEV_FRONTEND)	+= netfront/
 
drivers/xen/Kconfig.blk new file +14 -0
diff -r e6d9b50572f5 drivers/xen/Kconfig.blk
@@ -0,0 +1,14 @@
+menu "Xen block device drivers"
+        depends on XEN
+
+config XEN_BLKDEV_FRONTEND
+	tristate "Block device frontend driver"
+	depends on XEN
+	default y
+	help
+	  The block device frontend driver allows the kernel to access block
+	  devices exported from a device driver virtual machine. Unless you
+	  are building a dedicated device driver virtual machine, then you
+	  almost certainly want to say Y here.
+
+endmenu
drivers/xen/blkfront/Makefile new file +5 -0
diff -r e6d9b50572f5 drivers/xen/blkfront/Makefile
@@ -0,0 +1,5 @@
+
+obj-$(CONFIG_XEN_BLKDEV_FRONTEND)	:= xenblk.o
+
+xenblk-objs := blkfront.o vbd.o
+
drivers/xen/blkfront/blkfront.c new file +811 -0

811 lines not on this page · raw message

drivers/xen/blkfront/block.h new file +155 -0

155 lines not on this page · raw message

drivers/xen/blkfront/vbd.c new file +214 -0

214 lines not on this page · raw message

--

#95 lore raw

Re: [RFC PATCH 33/33] Add Xen virtual block device driver.

Quoted 4 lines · Chris Wright

plain text document attachment (blkfront)
The block device frontend driver allows the kernel to access block devices exported exported by a virtual machine containing a physical block device driver.

Hi,

as first general comment, I think that some of the memory allocation GFP_ flags are possibly incorrect; I would expect several places to use GFP_NOIO rather than GFP_KERNEL, to avoid recursion/deadlocks

drivers/xen/blkfront/blkfront.c line 714

Quoted 9 lines
drivers/xen/blkfront/blkfront.c new file +9 -0
+static void blkif_recover(struct blkfront_info *info)
+{
+	int i;
+	struct blkif_request *req;
+	struct blk_shadow *copy;
+	int j;
+
+	/* Stage 1: Make a safe copy of the shadow state. */
+	copy = kmalloc(sizeof(info->shadow), GFP_KERNEL | __GFP_NOFAIL);

like here..

drivers/xen/blkfront/blkfront.c line 715

Quoted 1 line
drivers/xen/blkfront/blkfront.c new file +1 -0
+	memcpy(copy, info->shadow, sizeof(info->shadow));

and __GFP_NOFAIL is usually horrid; is this because error recovery was an afterthought, or because it's physically impossible? In addition __GFP_NOFAIL in a block device driver is... an interesting way to add OOM deadlocks... have the VM guys looked into this yet?

drivers/xen/blkfront/block.h line 65

Quoted 6 lines
drivers/xen/blkfront/block.h new file +6 -0
+#if 1
+#define IPRINTK(fmt, args...) \
+    printk(KERN_INFO "xen_blk: " fmt, ##args)
+#else
+#define IPRINTK(fmt, args...) ((void)0)
+#endif

hmm isn't this a duplication of the pr_debug() and dev_dbg() infrastructure? Please don't reinvent new ones..

#96 lore raw

Re: [RFC PATCH 33/33] Add Xen virtual block device driver.

Quoted 14 lines · Arjan van de Ven

as first general comment, I think that some of the memory allocation GFP_ flags are possibly incorrect; I would expect several places to use GFP_NOIO rather than GFP_KERNEL, to avoid recursion/deadlocks

drivers/xen/blkfront/blkfront.c line 714

Quoted 5 lines
drivers/xen/blkfront/blkfront.c new file +3 -0
+static void blkif_recover(struct blkfront_info *info)
 
+	/* Stage 1: Make a safe copy of the shadow state. */
+	copy = kmalloc(sizeof(info->shadow), GFP_KERNEL | __GFP_NOFAIL);

like here..

Quoted 5 lines

and __GFP_NOFAIL is usually horrid; is this because error recovery was an afterthought, or because it's physically impossible? In addition __GFP_NOFAIL in a block device driver is... an interesting way to add OOM deadlocks... have the VM guys looked into this yet?

In this particular case, it's only used on the resume path, which I'm guessing would not lead to IO recursion. There doesn't seem to be any particular reason for this to be NOFAIL though (but I haven't really analyzed it).

There don't appear to be any memory allocations on the IO path; they're all in setup code.

J

#97 lore raw

RFC PATCH 33/33 Add Xen virtual block device driver.

In reply to Chris Wright
Quoted 4 lines · Chris Wright

The block device frontend driver allows the kernel to access block devices exported exported by a virtual machine containing a physical block device driver.

First, I think this belongs in drivers/block (and the network driver belongs in drivers/net). If we're going to bring xen to the party, lets not leave it hiding out in a corner.

drivers/xen/blkfront/blkfront.c line 70

Quoted 13 lines
drivers/xen/blkfront/blkfront.c new file +13 -0
+static void connect(struct blkfront_info *);
+static void blkfront_closing(struct xenbus_device *);
+static int blkfront_remove(struct xenbus_device *);
+static int talk_to_backend(struct xenbus_device *, struct blkfront_info *);
+static int setup_blkring(struct xenbus_device *, struct blkfront_info *);
+
+static void kick_pending_request_queues(struct blkfront_info *);
+
+static irqreturn_t blkif_int(int irq, void *dev_id, struct pt_regs *ptregs);
+static void blkif_restart_queue(void *arg);
+static void blkif_recover(struct blkfront_info *);
+static void blkif_completion(struct blk_shadow *);
+static void blkif_free(struct blkfront_info *, int);

I'm pretty sure you can rearrange the code to get rid of the forward references.

Quoted 11 lines
drivers/xen/blkfront/blkfront.c new file +11 -0
+/**
+ * We are reconnecting to the backend, due to a suspend/resume, or a backend
+ * driver restart.  We tear down our blkif structure and recreate it, but
+ * leave the device-layer structures intact so that this is transparent to the
+ * rest of the kernel.
+ */
+static int blkfront_resume(struct xenbus_device *dev)
+{
+	struct blkfront_info *info = dev->dev.driver_data;
+	int err;
+

drivers/xen/blkfront/blkfront.c line 143

Quoted 10 lines
drivers/xen/blkfront/blkfront.c new file +10 -0
+	DPRINTK("blkfront_resume: %s\n", dev->nodename);
+
+	blkif_free(info, 1);
+
+	err = talk_to_backend(dev, info);
+	if (!err)
+		blkif_recover(info);
+
+	return err;
+}

Should blkfront_resume grab blkif_io_lock?

drivers/xen/blkfront/blkfront.c line 262

Quoted 6 lines
drivers/xen/blkfront/blkfront.c new file +6 -0
+	switch (backend_state) {
+	case XenbusStateUnknown:
+	case XenbusStateInitialising:
+	case XenbusStateInitWait:
+	case XenbusStateInitialised:
+	case XenbusStateClosed:

This actually should get fixed elsewhere, but SillyCaps???

Quoted 7 lines
drivers/xen/blkfront/blkfront.c new file +7 -0
+static inline int GET_ID_FROM_FREELIST(
+	struct blkfront_info *info)
+{
+	unsigned long free = info->shadow_free;
+	BUG_ON(free > BLK_RING_SIZE);
+	info->shadow_free = info->shadow[free].req.id;
+	info->shadow[free].req.id = 0x0fffffee; /* debug */

drivers/xen/blkfront/blkfront.c line 384

Quoted 10 lines
drivers/xen/blkfront/blkfront.c new file +10 -0
+	return free;
+}
+
+static inline void ADD_ID_TO_FREELIST(
+	struct blkfront_info *info, unsigned long id)
+{
+	info->shadow[id].req.id  = info->shadow_free;
+	info->shadow[id].request = 0;
+	info->shadow_free = id;
+}

A real nit..but why are these routines SHOUTING?

drivers/xen/blkfront/blkfront.c line 432

Quoted 5 lines
drivers/xen/blkfront/blkfront.c new file +5 -0
+int blkif_release(struct inode *inode, struct file *filep)
+{
+	struct blkfront_info *info = inode->i_bdev->bd_disk->private_data;
+	info->users--;
+	if (info->users == 0) {

Hrm...this strikes me as racey. Don't you need at least a memory barrier here to handle SMP?

drivers/xen/blkfront/vbd.c line 52

Quoted 4 lines
drivers/xen/blkfront/vbd.c new file +4 -0
+static struct xlbd_major_info xvd_major_info = {
+	.major = 201,
+	.type = &xvd_type_info
+};

I've forgotten what the current policy is around new major numbers.

Dave B

#98 lore raw

Re: [RFC PATCH 33/33] Add Xen virtual block device driver.

Quoted 8 lines · Dave Boutcher
Quoted 3 lines · Chris Wright

The block device frontend driver allows the kernel to access block devices exported exported by a virtual machine containing a physical block device driver.

First, I think this belongs in drivers/block (and the network driver belongs in drivers/net). If we're going to bring xen to the party, lets not leave it hiding out in a corner.

Strongly agreed.

Quoted 8 lines
Quoted 8 lines
drivers/xen/blkfront/blkfront.c new file +8 -0
+static void connect(struct blkfront_info *);
+static void blkfront_closing(struct xenbus_device *);
+static int blkfront_remove(struct xenbus_device *);
+static int talk_to_backend(struct xenbus_device *, struct blkfront_info *);
+static int setup_blkring(struct xenbus_device *, struct blkfront_info *);
+
+static void kick_pending_request_queues(struct blkfront_info *);
+
Quoted 8 lines

drivers/xen/blkfront/blkfront.c line 70

Quoted 5 lines
drivers/xen/blkfront/blkfront.c new file +5 -0
+static irqreturn_t blkif_int(int irq, void *dev_id, struct pt_regs *ptregs);
+static void blkif_restart_queue(void *arg);
+static void blkif_recover(struct blkfront_info *);
+static void blkif_completion(struct blk_shadow *);
+static void blkif_free(struct blkfront_info *, int);

I'm pretty sure you can rearrange the code to get rid of the forward references.

I've never thought this was useful... If the function ordering helps the human...

Quoted 8 lines

drivers/xen/blkfront/blkfront.c line 262

Quoted 6 lines
drivers/xen/blkfront/blkfront.c new file +6 -0
+	switch (backend_state) {
+	case XenbusStateUnknown:
+	case XenbusStateInitialising:
+	case XenbusStateInitWait:
+	case XenbusStateInitialised:
+	case XenbusStateClosed:

This actually should get fixed elsewhere, but SillyCaps???

Agreed.

#99 lore raw

Re: [RFC PATCH 33/33] Add Xen virtual block device driver.

In reply to Dave Boutcher
Quoted 9 lines · * Dave Boutcher ([email protected]) wrote:
Quoted 4 lines · Chris Wright

The block device frontend driver allows the kernel to access block devices exported exported by a virtual machine containing a physical block device driver.

First, I think this belongs in drivers/block (and the network driver belongs in drivers/net). If we're going to bring xen to the party, lets not leave it hiding out in a corner.

Yeah, I think so too.

Quoted 8 lines

drivers/xen/blkfront/blkfront.c line 262

Quoted 6 lines
drivers/xen/blkfront/blkfront.c new file +6 -0
+	switch (backend_state) {
+	case XenbusStateUnknown:
+	case XenbusStateInitialising:
+	case XenbusStateInitWait:
+	case XenbusStateInitialised:
+	case XenbusStateClosed:

This actually should get fixed elsewhere, but SillyCaps???

I really don't care either way. There's no shortage of this style specifically for enums, in fact there's a wide variety of interesting styles for enums.

Quoted 11 lines
Quoted 11 lines
drivers/xen/blkfront/blkfront.c new file +11 -0
+static inline int GET_ID_FROM_FREELIST(
+	struct blkfront_info *info)
+{
+	unsigned long free = info->shadow_free;
+	BUG_ON(free > BLK_RING_SIZE);
+	info->shadow_free = info->shadow[free].req.id;
+	info->shadow[free].req.id = 0x0fffffee; /* debug */
+	return free;
+}
+
+static inline void ADD_ID_TO_FREELIST(
Quoted 8 lines

drivers/xen/blkfront/blkfront.c line 384

Quoted 6 lines
drivers/xen/blkfront/blkfront.c new file +6 -0
+	struct blkfront_info *info, unsigned long id)
+{
+	info->shadow[id].req.id  = info->shadow_free;
+	info->shadow[id].request = 0;
+	info->shadow_free = id;
+}

A real nit..but why are these routines SHOUTING?

GOOD QUESTION! I had missed that, thanks. Seems likely it was just half converted from macro. I see no reason not to clean that up the rest of the way.

Quoted 9 lines

drivers/xen/blkfront/blkfront.c line 432

Quoted 5 lines
drivers/xen/blkfront/blkfront.c new file +5 -0
+int blkif_release(struct inode *inode, struct file *filep)
+{
+	struct blkfront_info *info = inode->i_bdev->bd_disk->private_data;
+	info->users--;
+	if (info->users == 0) {

Hrm...this strikes me as racey. Don't you need at least a memory barrier here to handle SMP?

Quoted 6 lines

drivers/xen/blkfront/vbd.c line 52

Quoted 4 lines
drivers/xen/blkfront/vbd.c new file +4 -0
+static struct xlbd_major_info xvd_major_info = {
+	.major = 201,
+	.type = &xvd_type_info
+};

I've forgotten what the current policy is around new major numbers.

Damn, this is rather funny. There was a number reserved, but somehow this is the wrong one (should be 202 according to lanana[1]). Thanks, I'll fix that up.

thanks,
-chris

[1] http://www.lanana.org/docs/device-list/devices-2.6+.txt

#100 lore raw

Re: [RFC PATCH 33/33] Add Xen virtual block device driver.

In reply to Dave Boutcher
Quoted 46 lines · Dave Boutcher
Quoted 4 lines · Chris Wright

The block device frontend driver allows the kernel to access block devices exported exported by a virtual machine containing a physical block device driver.

First, I think this belongs in drivers/block (and the network driver belongs in drivers/net). If we're going to bring xen to the party, lets not leave it hiding out in a corner.

Quoted 4 lines
drivers/xen/blkfront/blkfront.c new file +3 -0
+static void connect(struct blkfront_info *);
+static void blkfront_closing(struct xenbus_device *);
+static int blkfront_remove(struct xenbus_device *);

drivers/xen/blkfront/blkfront.c line 70

Quoted 10 lines
drivers/xen/blkfront/blkfront.c new file +10 -0
+static int talk_to_backend(struct xenbus_device *, struct blkfront_info *);
+static int setup_blkring(struct xenbus_device *, struct blkfront_info *);
+
+static void kick_pending_request_queues(struct blkfront_info *);
+
+static irqreturn_t blkif_int(int irq, void *dev_id, struct pt_regs *ptregs);
+static void blkif_restart_queue(void *arg);
+static void blkif_recover(struct blkfront_info *);
+static void blkif_completion(struct blk_shadow *);
+static void blkif_free(struct blkfront_info *, int);

I'm pretty sure you can rearrange the code to get rid of the forward references.

Quoted 15 lines
drivers/xen/blkfront/blkfront.c new file +15 -0
+/**
+ * We are reconnecting to the backend, due to a suspend/resume, or a backend
+ * driver restart.  We tear down our blkif structure and recreate it, but
+ * leave the device-layer structures intact so that this is transparent to the
+ * rest of the kernel.
+ */
+static int blkfront_resume(struct xenbus_device *dev)
+{
+	struct blkfront_info *info = dev->dev.driver_data;
+	int err;
+
+	DPRINTK("blkfront_resume: %s\n", dev->nodename);
+
+	blkif_free(info, 1);
+
Quoted 9 lines

drivers/xen/blkfront/blkfront.c line 143

Quoted 7 lines
drivers/xen/blkfront/blkfront.c new file +6 -0
+	err = talk_to_backend(dev, info);
+	if (!err)
+		blkif_recover(info);
+
+	return err;
+}

Should blkfront_resume grab blkif_io_lock?

There should be no concurrent activity until info->connected has been set to BLKIF_STATE_CONNECTED, which doesn't happen until blkif_recover has completed successfully. blkif_queue_request and blkif_int both test the connection state before doing anything. (Not sure if a concurrent XenBus event can happen though.)

Quoted 23 lines
Quoted 9 lines
drivers/xen/blkfront/blkfront.c new file +7 -0
+static inline int GET_ID_FROM_FREELIST(
+	struct blkfront_info *info)
+{
+	unsigned long free = info->shadow_free;
+	BUG_ON(free > BLK_RING_SIZE);
+	info->shadow_free = info->shadow[free].req.id;
+	info->shadow[free].req.id = 0x0fffffee; /* debug */

drivers/xen/blkfront/blkfront.c line 384

Quoted 10 lines
drivers/xen/blkfront/blkfront.c new file +10 -0
+	return free;
+}
+
+static inline void ADD_ID_TO_FREELIST(
+	struct blkfront_info *info, unsigned long id)
+{
+	info->shadow[id].req.id  = info->shadow_free;
+	info->shadow[id].request = 0;
+	info->shadow_free = id;
+}

A real nit..but why are these routines SHOUTING?

Quoted 10 lines

drivers/xen/blkfront/blkfront.c line 432

Quoted 6 lines
drivers/xen/blkfront/blkfront.c new file +5 -0
+int blkif_release(struct inode *inode, struct file *filep)
+{
+	struct blkfront_info *info = inode->i_bdev->bd_disk->private_data;
+	info->users--;
+	if (info->users == 0) {

Hrm...this strikes me as racey. Don't you need at least a memory barrier here to handle SMP?

Hm. Doesn't look good to me.

Quoted 8 lines

drivers/xen/blkfront/vbd.c line 52

Quoted 5 lines
drivers/xen/blkfront/vbd.c new file +4 -0
+static struct xlbd_major_info xvd_major_info = {
+	.major = 201,
+	.type = &xvd_type_info
+};

I've forgotten what the current policy is around new major numbers.

This is wrong. 201 is allocated to Veritas, but 202 has been allocated for the Xen VBD.

J