Re: [PATCH] lguest: simplify lguest_iret

[Denys Vlasenko]

On 03/23/2015 04:30 AM, Rusty Russell wrote:
> Denys Vlasenko <dvlasenk@xxxxxxxxxx> writes:
>> Signed-off-by: Denys Vlasenko <dvlasenk@xxxxxxxxxx>
>> CC: lguest@xxxxxxxxxxxxxxxx
>> CC: x86@xxxxxxxxxx
>> CC: linux-kernel@xxxxxxxxxxxxxxx
> 
> Oh, thanks, applied!
> 
> And now it's down to one instruction, we could change
> try_deliver_interrupt() to handle this case (rather than ignoring the
> interrupt altogether): just jump to the handler and leave the
> stack set up.
> 
> Here's a pair of inline patches which attempt to do that (untested!).
> 
> Thanks,
> Rusty.
> 
> lguest: suppress interrupts for single insn, not range.
> 
> The last patch reduced our interrupt-suppression region to one address,
> so simplify the code somewhat.
> 
> Also, remove the obsolete undefined instruction ranges and the comment
> which refers to lguest_guest.S instead of head_32.S.
> 
> Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
> 
> diff --git a/arch/x86/include/asm/lguest.h b/arch/x86/include/asm/lguest.h
> index e2d4a4afa8c3..3bbc07a57a31 100644
> --- a/arch/x86/include/asm/lguest.h
> +++ b/arch/x86/include/asm/lguest.h
> @@ -20,13 +20,10 @@ extern unsigned long switcher_addr;
>  /* Found in switcher.S */
>  extern unsigned long default_idt_entries[];
>  
> -/* Declarations for definitions in lguest_guest.S */
> -extern char lguest_noirq_start[], lguest_noirq_end[];
> +/* Declarations for definitions in arch/x86/lguest/head_32.S */
> +extern char lguest_noirq_iret[];
>  extern const char lgstart_cli[], lgend_cli[];
> -extern const char lgstart_sti[], lgend_sti[];
> -extern const char lgstart_popf[], lgend_popf[];
>  extern const char lgstart_pushf[], lgend_pushf[];
> -extern const char lgstart_iret[], lgend_iret[];
>  
>  extern void lguest_iret(void);
>  extern void lguest_init(void);
> diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c
> index ac4453d8520e..4c8cf656f21f 100644
> --- a/arch/x86/lguest/boot.c
> +++ b/arch/x86/lguest/boot.c
> @@ -87,8 +87,7 @@
>  
>  struct lguest_data lguest_data = {
>  	.hcall_status = { [0 ... LHCALL_RING_SIZE-1] = 0xFF },
> -	.noirq_start = (u32)lguest_noirq_start,
> -	.noirq_end = (u32)lguest_noirq_end,
> +	.noirq_iret = (u32)lguest_noirq_iret,
>  	.kernel_address = PAGE_OFFSET,
>  	.blocked_interrupts = { 1 }, /* Block timer interrupts */
>  	.syscall_vec = SYSCALL_VECTOR,
> diff --git a/arch/x86/lguest/head_32.S b/arch/x86/lguest/head_32.S
> index 583732cc5d18..128fe93161b4 100644
> --- a/arch/x86/lguest/head_32.S
> +++ b/arch/x86/lguest/head_32.S
> @@ -133,9 +133,8 @@ ENTRY(lg_restore_fl)
>  	ret
>  /*:*/
>  
> -/* These demark the EIP range where host should never deliver interrupts. */
> -.global lguest_noirq_start
> -.global lguest_noirq_end
> +/* These demark the EIP where host should never deliver interrupts. */
> +.global lguest_noirq_iret
>  
>  /*M:004
>   * When the Host reflects a trap or injects an interrupt into the Guest, it
> @@ -174,12 +173,11 @@ ENTRY(lg_restore_fl)
>   *
>   * The second is harder: copying eflags to lguest_data.irq_enabled will turn
>   * interrupts on before we're finished, so we could be interrupted before we
> - * return to userspace or wherever.  Our solution to this is to surround the
> - * code with lguest_noirq_start: and lguest_noirq_end: labels.  We tell the
> + * return to userspace or wherever.  Our solution to this is to tell the
>   * Host that it is *never* to interrupt us there, even if interrupts seem to be
>   * enabled. (It's not necessary to protect pop instruction, since
> - * data gets updated only after it completes, so we end up surrounding
> - * just one instruction, iret).
> + * data gets updated only after it completes, so we only need to protect
> + * one instruction, iret).
>   */
>  ENTRY(lguest_iret)
>  	pushl	2*4(%esp)
> @@ -190,6 +188,5 @@ ENTRY(lguest_iret)
>  	 * prefix makes sure we use the stack segment, which is still valid.
>  	 */
>  	popl	%ss:lguest_data+LGUEST_DATA_irq_enabled
> -lguest_noirq_start:
> +lguest_noirq_iret:
>  	iret
> -lguest_noirq_end:
> diff --git a/drivers/lguest/hypercalls.c b/drivers/lguest/hypercalls.c
> index 1219af493c0f..19a32280731d 100644
> --- a/drivers/lguest/hypercalls.c
> +++ b/drivers/lguest/hypercalls.c
> @@ -211,10 +211,9 @@ static void initialize(struct lg_cpu *cpu)
>  
>  	/*
>  	 * The Guest tells us where we're not to deliver interrupts by putting
> -	 * the range of addresses into "struct lguest_data".
> +	 * the instruction address into "struct lguest_data".
>  	 */
> -	if (get_user(cpu->lg->noirq_start, &cpu->lg->lguest_data->noirq_start)
> -	    || get_user(cpu->lg->noirq_end, &cpu->lg->lguest_data->noirq_end))
> +	if (get_user(cpu->lg->noirq_iret, &cpu->lg->lguest_data->noirq_iret))
>  		kill_guest(cpu, "bad guest page %p", cpu->lg->lguest_data);
>  
>  	/*
> diff --git a/drivers/lguest/interrupts_and_traps.c b/drivers/lguest/interrupts_and_traps.c
> index 70dfcdc29f1f..6d4c072b61e1 100644
> --- a/drivers/lguest/interrupts_and_traps.c
> +++ b/drivers/lguest/interrupts_and_traps.c
> @@ -204,8 +204,7 @@ void try_deliver_interrupt(struct lg_cpu *cpu, unsigned int irq, bool more)
>  	 * They may be in the middle of an iret, where they asked us never to
>  	 * deliver interrupts.
>  	 */
> -	if (cpu->regs->eip >= cpu->lg->noirq_start &&
> -	   (cpu->regs->eip < cpu->lg->noirq_end))
> +	if (cpu->regs->eip == cpu->lg->noirq_iret)
>  		return;
>  
>  	/* If they're halted, interrupts restart them. */
> @@ -395,8 +394,9 @@ static bool direct_trap(unsigned int num)
>   * The Guest has the ability to turn its interrupt gates into trap gates,
>   * if it is careful.  The Host will let trap gates can go directly to the
>   * Guest, but the Guest needs the interrupts atomically disabled for an
> - * interrupt gate.  It can do this by pointing the trap gate at instructions
> - * within noirq_start and noirq_end, where it can safely disable interrupts.
> + * interrupt gate.  The Host could provide a mechanism to register more
> + * "no-interrupt" regions, and the Guest could point the trap gate at
> + * instructions within that region, where it can safely disable interrupts.
>   */
>  
>  /*M:006
> diff --git a/drivers/lguest/lg.h b/drivers/lguest/lg.h
> index 307e8b39e7d1..ac8ad0461e80 100644
> --- a/drivers/lguest/lg.h
> +++ b/drivers/lguest/lg.h
> @@ -102,7 +102,7 @@ struct lguest {
>  
>  	struct pgdir pgdirs[4];
>  
> -	unsigned long noirq_start, noirq_end;
> +	unsigned long noirq_iret;
>  
>  	unsigned int stack_pages;
>  	u32 tsc_khz;
> diff --git a/include/linux/lguest.h b/include/linux/lguest.h
> index 9962c6bb1311..6db19f35f7c5 100644
> --- a/include/linux/lguest.h
> +++ b/include/linux/lguest.h
> @@ -61,8 +61,8 @@ struct lguest_data {
>  	u32 tsc_khz;
>  
>  /* Fields initialized by the Guest at boot: */
> -	/* Instruction range to suppress interrupts even if enabled */
> -	unsigned long noirq_start, noirq_end;
> +	/* Instruction to suppress interrupts even if enabled */
> +	unsigned long noirq_iret;
>  	/* Address above which page tables are all identical. */
>  	unsigned long kernel_address;
>  	/* The vector to try to use for system calls (0x40 or 0x80). */
> 
> lguest: handle traps on the "interrupt suppressed" iret instruction.
> 
> Lguest's "iret" is non-atomic, as it needs to restore the interrupt
> state before the real iret (the guest can't actually suppress
> interrupts).  For this reason, the host discards an interrupt if it
> occurs in this (1-instruction) window.
> 
> We can do better, by emulating the iret execution, then immediately
> setting up the interrupt handler.  In fact, we don't need to do much,
> as emulating the iret and setting up th stack for the interrupt handler
> basically cancel each other out.


>  		/*
> +		 * They may be about to iret, where they asked us never to
> +		 * deliver interrupts.  In this case, we can emulate that iret
> +		 * then immediately deliver the interrupt.  This is bascially
> +		 * a noop: the iret would pop the interrupt frame and restore
> +		 * eflags, and then we'd set it up again.  So just restore the
> +		 * eflags word and jump straight to the handler in this case.
>  		 */
> +		if (cpu->regs->eip >= cpu->lg->noirq_start &&
> +		    (cpu->regs->eip < cpu->lg->noirq_end)) {
> +			restore_eflags(cpu);

In truth, this is not _exactly_ true for irets to CPL3.

If a new interrupt comes right after iret, then
a new transition to CPL0 will happen.

This means ss:esp will be loaded from tss.ss0:tss.sp0.

Meaning, that the new iret frame may be in a different place
than the one which was used by iret.

There is no good reason for CPL0 code to move iret frame around,
but who knows. As an example, look what 32-bit Linux kernel does
with NMI iret frames... it's mind bending.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/