[Lguest] [patch] core.c i386 disentangle

[Rusty Russell]

On Tue, 2007-08-14 at 14:54 +0200, Jes Sorensen wrote:
> Hi,
> 
> Here's a patch to disentangle a lot of i386 specific gibberish :) from
> core.c. I've tried doing it in the right order etc. but it's possible
> you'll disagree with how I put some of it.
> 
> I don't have an x86 build box handy so it's likely the patch needs a bit
> of work to build on i386, but at least it makes me compile core.c on
> ia64 without getting loads of warnings.
> 
> The patch is against Rusty's hg patch queue as of this morning (my
> time).
> 
> Builds for me<tm>!
> 
> Rusty will you be able to mingle this to build on x86, or do you require
> me to try and track down an x86 box somewhere here and fix any breakage?
> 
> Thanks,
> Jes
> 
> 
> plain text document attachment (lg-core-i386-seperate.diff)
> Seperate i368 architecture specific from core.c and move it to
> i386_guest.c and add header file entries to match.
> 
> Signed-off-by: Jes Sorensen <jes at sgi.com>
> 
> ---
>  drivers/lguest/core.c                 |  497 ---------------------------------
>  drivers/lguest/i386_guest.c           |  510 ++++++++++++++++++++++++++++++++++
>  drivers/lguest/interrupts_and_traps.c |   18 -
>  drivers/lguest/lg.h                   |   55 ---
>  drivers/lguest/segments.c             |   26 -
>  include/asm-i386/lguest.h             |   54 +++
>  6 files changed, 600 insertions(+), 560 deletions(-)

Hi Jes,

	I think you've misunderstood the role of "i386_guest.c".   That's the
*guest* side of i386.  We need a new file (i386.c? i386_core.c?) which
has all the host-side of i386.  Some of this, of course, will be moved
to x86.c when we share with x86_64, but that's ok for later.

> +#ifdef CONFIG_HIGHMEM
>  #include <asm/highmem.h>
> +#endif

Erk... change this to linux/highmem.h and I think everyone will be
happy.

> -/*H:030 Let's jump straight to the the main loop which runs the Guest.
> - * Remember, this is called by the Launcher reading /dev/lguest, and we keep
> - * going around and around until something interesting happens. */
> -int run_guest(struct lguest *lg, unsigned long __user *user)
> -{

I wonder if we want to move this whole function, or try to hook in arch
stuff in a sane way, eg:

> -	/* We stop running once the Guest is dead. */
> -	while (!lg->dead) {
> -		/* We need to initialize this, otherwise gcc complains.  It's
> -		 * not (yet) clever enough to see that it's initialized when we
> -		 * need it. */
> -		unsigned int cr2 = 0; /* Damn gcc */

Obviously would need to get rid of that...

> -
> -		/* First we run any hypercalls the Guest wants done: either in
> -		 * the hypercall ring in "struct lguest_data", or directly by
> -		 * using int 31 (LGUEST_TRAP_ENTRY). */
> -		do_hypercalls(lg);
> -		/* It's possible the Guest did a SEND_DMA hypercall to the
> -		 * Launcher, in which case we return from the read() now. */
> -		if (lg->dma_is_pending) {
> -			if (put_user(lg->pending_dma, user) ||
> -			    put_user(lg->pending_key, user+1))
> -				return -EFAULT;
> -			return sizeof(unsigned long)*2;
> -		}
> -
> -		/* Check for signals */
> -		if (signal_pending(current))
> -			return -ERESTARTSYS;
> -
> -		/* If Waker set break_out, return to Launcher. */
> -		if (lg->break_out)
> -			return -EAGAIN;
> -
> -		/* Check if there are any interrupts which can be delivered
> -		 * now: if so, this sets up the hander to be executed when we
> -		 * next run the Guest. */
> -		maybe_do_interrupt(lg);
> -
> -		/* All long-lived kernel loops need to check with this horrible
> -		 * thing called the freezer.  If the Host is trying to suspend,
> -		 * it stops us. */
> -		try_to_freeze();
> -
> -		/* Just make absolutely sure the Guest is still alive.  One of
> -		 * those hypercalls could have been fatal, for example. */
> -		if (lg->dead)
> -			break;
> -
> -		/* If the Guest asked to be stopped, we sleep.  The Guest's
> -		 * clock timer or LHCALL_BREAK from the Waker will wake us. */
> -		if (lg->halted) {
> -			set_current_state(TASK_INTERRUPTIBLE);
> -			schedule();
> -			continue;
> -		}
> -
> -		/* OK, now we're ready to jump into the Guest.  First we put up
> -		 * the "Do Not Disturb" sign: */
> -		local_irq_disable();

This is all generic.

> -
> -		/* 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)
> -			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. */
> -		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. */
> -		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.  */
> -
> -		/* 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)
> -			cr2 = read_cr2();
> -		/* Similarly, if we took a trap because the Guest used the FPU,
> -		 * we have to restore the FPU it expects to see. */
> -		else if (lg->regs->trapnum == 7)
> -			math_state_restore();
> -
> -		/* Restore SYSENTER if it's supposed to be on. */
> -		if (boot_cpu_has(X86_FEATURE_SEP))
> -			wrmsr(MSR_IA32_SYSENTER_CS, __KERNEL_CS, 0);

This could all be wrapped in lguest_arch_run_guest() (x86 will have to
have somewhere to stash the cr2, but that's easy).

> -
> -		/* Now we're ready to be interrupted or moved to other CPUs */
> -		local_irq_enable();

Generic.

> -
> -		/* OK, so what happened? */
> -		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. */
> -			if (lg->regs->errcode == 0) {
> -				if (emulate_insn(lg))
> -					continue;
> -			}
> -			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. */
> -			if (demand_page(lg, cr2, lg->regs->errcode))
> -				continue;
> -
> -			/* 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 */
> -			if (lg->lguest_data
> -			    && put_user(cr2, &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 (!lg->ts)
> -				continue;
> -			break;
> -		case 32 ... 255:
> -			/* These values mean a real interrupt occurred, in
> -			 * which case the Host handler has already been run.
> -			 * We just do a friendly check if another process
> -			 * should now be run, then fall through to loop
> -			 * around: */
> -			cond_resched();
> -		case LGUEST_TRAP_ENTRY: /* Handled at top of loop */
> -			continue;
> -		}

if (lguest_arch_handle_trap(lg))
	continue;

> -
> -		/* If we get here, it's a trap the Guest wants to know
> -		 * about. */
> -		if (deliver_trap(lg, lg->regs->trapnum))
> -			continue;
> -
> -		/* If the Guest doesn't have a handler (either it hasn't
> -		 * registered any yet, or it's one of the faults we don't let
> -		 * it handle), it dies with a cryptic error message. */
> -		kill_guest(lg, "unhandled trap %li at %#lx (%#lx)",
> -			   lg->regs->trapnum, lg->regs->eip,
> -			   lg->regs->trapnum == 14 ? cr2 : lg->regs->errcode);

lguest_arch_deliver_trap(lg);

Rest looks fine...

Thanks,
Rusty.