lguest: documentation update

Went through the documentation doing typo and content fixes.  This
patch contains only comment and whitespace changes.

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

2 files changed, 113 insertions, 78 deletions

diff --git a/drivers/lguest/x86/core.c b/drivers/lguest/x86/core.c
index 09d9207420dc..482aec2a9631 100644
--- a/drivers/lguest/x86/core.c
+++ b/drivers/lguest/x86/core.c
@@ -63,7 +63,7 @@ static struct lguest_pages *lguest_pages(unsigned int cpu)
 static DEFINE_PER_CPU(struct lguest *, last_guest);
 
 /*S:010
- * We are getting close to the Switcher.
+ * We approach the Switcher.
  *
  * Remember that each CPU has two pages which are visible to the Guest when it
  * runs on that CPU.  This has to contain the state for that Guest: we copy the
@@ -134,7 +134,7 @@ static void run_guest_once(struct lguest *lg, struct lguest_pages *pages)
 	 *
 	 * The lcall also pushes the old code segment (KERNEL_CS) onto the
 	 * stack, then the address of this call.  This stack layout happens to
-	 * exactly match the stack of an interrupt... */
+	 * exactly match the stack layout created by an interrupt... */
 	asm volatile("pushf; lcall *lguest_entry"
 		     /* This is how we tell GCC that %eax ("a") and %ebx ("b")
 		      * are changed by this routine.  The "=" means output. */
@@ -151,40 +151,46 @@ static void run_guest_once(struct lguest *lg, struct lguest_pages *pages)
 }
 /*:*/
 
+/*M:002 There are hooks in the scheduler which we can register to tell when we
+ * get kicked off the CPU (preempt_notifier_register()).  This would allow us
+ * to lazily disable SYSENTER which would regain some performance, and should
+ * also simplify copy_in_guest_info().  Note that we'd still need to restore
+ * things when we exit to Launcher userspace, but that's fairly easy.
+ *
+ * The hooks were designed for KVM, but we can also put them to good use. :*/
+
 /*H:040 This is the i386-specific code to setup and run the Guest.  Interrupts
  * are disabled: we own the CPU. */
 void lguest_arch_run_guest(struct lguest *lg)
 {
-	/* Remember the awfully-named TS bit?  If the Guest has asked
-	 * to set it we set it now, so we can trap and pass that trap
-	 * to the Guest if it uses the FPU. */
+	/* Remember the awfully-named TS bit?  If the Guest has asked to set it
+	 * we set it now, so we can trap and pass that trap to the Guest if it
+	 * uses the FPU. */
 	if (lg->ts)
 		lguest_set_ts();
 
-	/* SYSENTER is an optimized way of doing system calls.  We
-	 * can't allow it because it always jumps to privilege level 0.
-	 * A normal Guest won't try it because we don't advertise it in
-	 * CPUID, but a malicious Guest (or malicious Guest userspace
-	 * program) could, so we tell the CPU to disable it before
-	 * running the Guest. */
+	/* SYSENTER is an optimized way of doing system calls.  We can't allow
+	 * it because it always jumps to privilege level 0.  A normal Guest
+	 * won't try it because we don't advertise it in CPUID, but a malicious
+	 * Guest (or malicious Guest userspace program) could, so we tell the
+	 * CPU to disable it before running the Guest. */
 	if (boot_cpu_has(X86_FEATURE_SEP))
 		wrmsr(MSR_IA32_SYSENTER_CS, 0, 0);
 
-	/* Now we actually run the Guest.  It will pop back out when
-	 * something interesting happens, and we can examine its
-	 * registers to see what it was doing. */
+	/* Now we actually run the Guest.  It will return when something
+	 * interesting happens, and we can examine its registers to see what it
+	 * was doing. */
 	run_guest_once(lg, lguest_pages(raw_smp_processor_id()));
 
-	/* The "regs" pointer contains two extra entries which are not
-	 * really registers: a trap number which says what interrupt or
-	 * trap made the switcher code come back, and an error code
-	 * which some traps set.  */
+	/* Note that the "regs" pointer contains two extra entries which are
+	 * not really registers: a trap number which says what interrupt or
+	 * trap made the switcher code come back, and an error code which some
+	 * traps set.  */
 
-	/* If the Guest page faulted, then the cr2 register will tell
-	 * us the bad virtual address.  We have to grab this now,
-	 * because once we re-enable interrupts an interrupt could
-	 * fault and thus overwrite cr2, or we could even move off to a
-	 * different CPU. */
+	/* If the Guest page faulted, then the cr2 register will tell us the
+	 * bad virtual address.  We have to grab this now, because once we
+	 * re-enable interrupts an interrupt could fault and thus overwrite
+	 * cr2, or we could even move off to a different CPU. */
 	if (lg->regs->trapnum == 14)
 		lg->arch.last_pagefault = read_cr2();
 	/* Similarly, if we took a trap because the Guest used the FPU,
@@ -197,14 +203,15 @@ void lguest_arch_run_guest(struct lguest *lg)
 		wrmsr(MSR_IA32_SYSENTER_CS, __KERNEL_CS, 0);
 }
 
-/*H:130 Our Guest is usually so well behaved; it never tries to do things it
- * isn't allowed to.  Unfortunately, Linux's paravirtual infrastructure isn't
- * quite complete, because it doesn't contain replacements for the Intel I/O
- * instructions.  As a result, the Guest sometimes fumbles across one during
- * the boot process as it probes for various things which are usually attached
- * to a PC.
+/*H:130 Now we've examined the hypercall code; our Guest can make requests.
+ * Our Guest is usually so well behaved; it never tries to do things it isn't
+ * allowed to, and uses hypercalls instead.  Unfortunately, Linux's paravirtual
+ * infrastructure isn't quite complete, because it doesn't contain replacements
+ * for the Intel I/O instructions.  As a result, the Guest sometimes fumbles
+ * across one during the boot process as it probes for various things which are
+ * usually attached to a PC.
  *
- * When the Guest uses one of these instructions, we get trap #13 (General
+ * When the Guest uses one of these instructions, we get a trap (General
  * Protection Fault) and come here.  We see if it's one of those troublesome
  * instructions and skip over it.  We return true if we did. */
 static int emulate_insn(struct lguest *lg)
@@ -275,43 +282,43 @@ static int emulate_insn(struct lguest *lg)
 void lguest_arch_handle_trap(struct lguest *lg)
 {
 	switch (lg->regs->trapnum) {
-	case 13: /* We've intercepted a GPF. */
-		 /* Check if this was one of those annoying IN or OUT
-		  * instructions which we need to emulate.  If so, we
-		  * just go back into the Guest after we've done it. */
+	case 13: /* We've intercepted a General Protection Fault. */
+		/* Check if this was one of those annoying IN or OUT
+		 * instructions which we need to emulate.  If so, we just go
+		 * back into the Guest after we've done it. */
 		if (lg->regs->errcode == 0) {
 			if (emulate_insn(lg))
 				return;
 		}
 		break;
-	case 14: /* We've intercepted a page fault. */
-		 /* The Guest accessed a virtual address that wasn't
-		  * mapped.  This happens a lot: we don't actually set
-		  * up most of the page tables for the Guest at all when
-		  * we start: as it runs it asks for more and more, and
-		  * we set them up as required. In this case, we don't
-		  * even tell the Guest that the fault happened.
-		  *
-		  * The errcode tells whether this was a read or a
-		  * write, and whether kernel or userspace code. */
+	case 14: /* We've intercepted a Page Fault. */
+		/* The Guest accessed a virtual address that wasn't mapped.
+		 * This happens a lot: we don't actually set up most of the
+		 * page tables for the Guest at all when we start: as it runs
+		 * it asks for more and more, and we set them up as
+		 * required. In this case, we don't even tell the Guest that
+		 * the fault happened.
+		 *
+		 * The errcode tells whether this was a read or a write, and
+		 * whether kernel or userspace code. */
 		if (demand_page(lg, lg->arch.last_pagefault, lg->regs->errcode))
 			return;
 
-		 /* OK, it's really not there (or not OK): the Guest
-		  * needs to know.  We write out the cr2 value so it
-		  * knows where the fault occurred.
-		  *
-		  * Note that if the Guest were really messed up, this
-		  * could happen before it's done the INITIALIZE
-		  * hypercall, so lg->lguest_data will be NULL */
+		/* OK, it's really not there (or not OK): the Guest needs to
+		 * know.  We write out the cr2 value so it knows where the
+		 * fault occurred.
+		 *
+		 * Note that if the Guest were really messed up, this could
+		 * happen before it's done the LHCALL_LGUEST_INIT hypercall, so
+		 * lg->lguest_data could be NULL */
 		if (lg->lguest_data &&
 		    put_user(lg->arch.last_pagefault, &lg->lguest_data->cr2))
 			kill_guest(lg, "Writing cr2");
 		break;
 	case 7: /* We've intercepted a Device Not Available fault. */
-		/* If the Guest doesn't want to know, we already
-		 * restored the Floating Point Unit, so we just
-		 * continue without telling it. */
+		/* If the Guest doesn't want to know, we already restored the
+		 * Floating Point Unit, so we just continue without telling
+		 * it. */
 		if (!lg->ts)
 			return;
 		break;
@@ -536,9 +543,6 @@ int lguest_arch_init_hypercalls(struct lguest *lg)
 
 	return 0;
 }
-/* Now we've examined the hypercall code; our Guest can make requests.  There
- * is one other way we can do things for the Guest, as we see in
- * emulate_insn(). :*/
 
 /*L:030 lguest_arch_setup_regs()
  *
@@ -570,8 +574,8 @@ void lguest_arch_setup_regs(struct lguest *lg, unsigned long start)
 
 	/* %esi points to our boot information, at physical address 0, so don't
 	 * touch it. */
+
 	/* There are a couple of GDT entries the Guest expects when first
 	 * booting. */
-
 	setup_guest_gdt(lg);
 }
diff --git a/drivers/lguest/x86/switcher_32.S b/drivers/lguest/x86/switcher_32.S
index 1010b90b11fc..0af8baaa0d4a 100644
--- a/drivers/lguest/x86/switcher_32.S
+++ b/drivers/lguest/x86/switcher_32.S
@@ -6,6 +6,37 @@
  * are feeling invigorated and refreshed then the next, more challenging stage
  * can be found in "make Guest". :*/
 
+/*M:012 Lguest is meant to be simple: my rule of thumb is that 1% more LOC must
+ * gain at least 1% more performance.  Since neither LOC nor performance can be
+ * measured beforehand, it generally means implementing a feature then deciding
+ * if it's worth it.  And once it's implemented, who can say no?
+ *
+ * This is why I haven't implemented this idea myself.  I want to, but I
+ * haven't.  You could, though.
+ *
+ * The main place where lguest performance sucks is Guest page faulting.  When
+ * a Guest userspace process hits an unmapped page we switch back to the Host,
+ * walk the page tables, find it's not mapped, switch back to the Guest page
+ * fault handler, which calls a hypercall to set the page table entry, then
+ * finally returns to userspace.  That's two round-trips.
+ *
+ * If we had a small walker in the Switcher, we could quickly check the Guest
+ * page table and if the page isn't mapped, immediately reflect the fault back
+ * into the Guest.  This means the Switcher would have to know the top of the
+ * Guest page table and the page fault handler address.
+ *
+ * For simplicity, the Guest should only handle the case where the privilege
+ * level of the fault is 3 and probably only not present or write faults.  It
+ * should also detect recursive faults, and hand the original fault to the
+ * Host (which is actually really easy).
+ *
+ * Two questions remain.  Would the performance gain outweigh the complexity?
+ * And who would write the verse documenting it? :*/
+
+/*M:011 Lguest64 handles NMI.  This gave me NMI envy (until I looked at their
+ * code).  It's worth doing though, since it would let us use oprofile in the
+ * Host when a Guest is running. :*/
+
 /*S:100
  * Welcome to the Switcher itself!
  *
@@ -88,7 +119,7 @@ ENTRY(switch_to_guest)
 
 	// All saved and there's now five steps before us:
 	// Stack, GDT, IDT, TSS
-	// And last of all the page tables are flipped.
+	// Then last of all the page tables are flipped.
 
 	// Yet beware that our stack pointer must be
 	// Always valid lest an NMI hits
@@ -103,25 +134,25 @@ ENTRY(switch_to_guest)
 	lgdt	LGUEST_PAGES_guest_gdt_desc(%eax)
 
 	// The Guest's IDT we did partially
-	// Move to the "struct lguest_pages" as well.
+	// Copy to "struct lguest_pages" as well.
 	lidt	LGUEST_PAGES_guest_idt_desc(%eax)
 
 	// The TSS entry which controls traps
 	// Must be loaded up with "ltr" now:
+	// The GDT entry that TSS uses 
+	// Changes type when we load it: damn Intel!
 	// For after we switch over our page tables
-	// It (as the rest) will be writable no more.
-	// (The GDT entry TSS needs
-	// Changes type when we load it: damn Intel!)
+	// That entry will be read-only: we'd crash.
 	movl	$(GDT_ENTRY_TSS*8), %edx
 	ltr	%dx
 
 	// Look back now, before we take this last step!
 	// The Host's TSS entry was also marked used;
-	// Let's clear it again, ere we return.
+	// Let's clear it again for our return.
 	// The GDT descriptor of the Host
 	// Points to the table after two "size" bytes
 	movl	(LGUEST_PAGES_host_gdt_desc+2)(%eax), %edx
-	// Clear the type field of "used" (byte 5, bit 2)
+	// Clear "used" from type field (byte 5, bit 2)
 	andb	$0xFD, (GDT_ENTRY_TSS*8 + 5)(%edx)
 
 	// Once our page table's switched, the Guest is live!
@@ -131,7 +162,7 @@ ENTRY(switch_to_guest)
 
 	// The page table change did one tricky thing:
 	// The Guest's register page has been mapped
-	// Writable onto our %esp (stack) --
+	// Writable under our %esp (stack) --
 	// We can simply pop off all Guest regs.
 	popl	%eax
 	popl	%ebx
@@ -152,16 +183,15 @@ ENTRY(switch_to_guest)
 	addl	$8, %esp
 
 	// The last five stack slots hold return address
-	// And everything needed to change privilege
-	// Into the Guest privilege level of 1,
+	// And everything needed to switch privilege
+	// From Switcher's level 0 to Guest's 1,
 	// And the stack where the Guest had last left it.
 	// Interrupts are turned back on: we are Guest.
 	iret
 
-// There are two paths where we switch to the Host
+// We treat two paths to switch back to the Host
+// Yet both must save Guest state and restore Host
 // So we put the routine in a macro.
-// We are on our way home, back to the Host
-// Interrupted out of the Guest, we come here.
 #define SWITCH_TO_HOST							\
 	/* We save the Guest state: all registers first			\
 	 * Laid out just as "struct lguest_regs" defines */		\
@@ -194,7 +224,7 @@ ENTRY(switch_to_guest)
 	movl	%esp, %eax;						\
 	andl	$(~(1 << PAGE_SHIFT - 1)), %eax;			\
 	/* Save our trap number: the switch will obscure it		\
-	 * (The Guest regs are not mapped here in the Host)		\
+	 * (In the Host the Guest regs are not mapped here)		\
 	 * %ebx holds it safe for deliver_to_host */			\
 	movl	LGUEST_PAGES_regs_trapnum(%eax), %ebx;			\
 	/* The Host GDT, IDT and stack!					\
@@ -210,9 +240,9 @@ ENTRY(switch_to_guest)
 	/* Switch to Host's GDT, IDT. */				\
 	lgdt	LGUEST_PAGES_host_gdt_desc(%eax);			\
 	lidt	LGUEST_PAGES_host_idt_desc(%eax);			\
-	/* Restore the Host's stack where it's saved regs lie */	\
+	/* Restore the Host's stack where its saved regs lie */		\
 	movl	LGUEST_PAGES_host_sp(%eax), %esp;			\
-	/* Last the TSS: our Host is complete */			\
+	/* Last the TSS: our Host is returned */			\
 	movl	$(GDT_ENTRY_TSS*8), %edx;				\
 	ltr	%dx;							\
 	/* Restore now the regs saved right at the first. */		\
@@ -222,14 +252,15 @@ ENTRY(switch_to_guest)
 	popl	%ds;							\
 	popl	%es
 
-// Here's where we come when the Guest has just trapped:
-// (Which trap we'll see has been pushed on the stack).
+// The first path is trod when the Guest has trapped:
+// (Which trap it was has been pushed on the stack).
 // We need only switch back, and the Host will decode
 // Why we came home, and what needs to be done.
 return_to_host:
 	SWITCH_TO_HOST
 	iret
 
+// We are lead to the second path like so:
 // An interrupt, with some cause external
 // Has ajerked us rudely from the Guest's code
 // Again we must return home to the Host
@@ -238,7 +269,7 @@ deliver_to_host:
 	// But now we must go home via that place
 	// Where that interrupt was supposed to go
 	// Had we not been ensconced, running the Guest.
-	// Here we see the cleverness of our stack:
+	// Here we see the trickness of run_guest_once():
 	// The Host stack is formed like an interrupt
 	// With EIP, CS and EFLAGS layered.
 	// Interrupt handlers end with "iret"
@@ -263,7 +294,7 @@ deliver_to_host:
 	xorw	%ax, %ax
 	orl	%eax, %edx
 	// Now the address of the handler's in %edx
-	// We call it now: its "iret" takes us home.
+	// We call it now: its "iret" drops us home.
 	jmp	*%edx
 
 // Every interrupt can come to us here