Re: [PATCH 6/6] mini-os/x86-64 entry: check against nested events and try to fix up

[Xu Zhang <xzhang54@uic.edu>]

On 03/09/2013 04:44 PM, Jeremy Fitzhardinge wrote:
> On 03/08/2013 01:30 PM, Xu Zhang wrote:
>> +# [How we do the fixup]. We want to merge the current stack frame with the
>> +# just-interrupted frame. How we do this depends on where in the critical
>> +# region the interrupted handler was executing, and so how many saved
>> +# registers are in each frame. We do this quickly using the lookup table
>> +# 'critical_fixup_table'. For each byte offset in the critical region, it
>> +# provides the number of bytes which have already been popped from the
>> +# interrupted stack frame. This is the number of bytes from the current stack
>> +# that we need to copy at the end of the previous activation frame so that
>> +# we can continue as if we've never even reached 11 running in the old
>> +# activation frame.
>> +critical_region_fixup:
>> +		addq $critical_fixup_table - scrit, %rax
>> +		movzbq (%rax),%rax    # %rax contains num bytes popped
>> +		mov  %rsp,%rsi
>> +		add  %rax,%rsi        # %esi points at end of src region
>> +
>> +		movq RSP(%rsp),%rdi   # acquire interrupted %rsp from current stack frame
>> +		                      # %edi points at end of dst region
>> +		mov  %rax,%rcx
>> +		shr  $3,%rcx          # convert bytes into count of 64-bit entities
>> +		je   16f              # skip loop if nothing to copy
>> +15:		subq $8,%rsi          # pre-decrementing copy loop
>> +		subq $8,%rdi
>> +		movq (%rsi),%rax
>> +		movq %rax,(%rdi)
>> +		loop 15b
>> +16:		movq %rdi,%rsp        # final %rdi is top of merged stack
>> +		andb $KERNEL_CS_MASK,CS(%rsp)      # CS on stack might have changed
>> +		jmp  11b
>> +
>> +
>> +/* Nested event fixup look-up table*/
>> +critical_fixup_table:
>> +	.byte 0x00,0x00,0x00                    # XEN_TEST_PENDING(%rsi)
>> +	.byte 0x00,0x00,0x00,0x00,0x00,0x00     # jnz    14f
>> +	.byte 0x00,0x00,0x00,0x00               # mov    (%rsp),%r15
>> +	.byte 0x00,0x00,0x00,0x00,0x00          # mov    0x8(%rsp),%r14
>> +	.byte 0x00,0x00,0x00,0x00,0x00          # mov    0x10(%rsp),%r13
>> +	.byte 0x00,0x00,0x00,0x00,0x00          # mov    0x18(%rsp),%r12
>> +	.byte 0x00,0x00,0x00,0x00,0x00          # mov    0x20(%rsp),%rbp
>> +	.byte 0x00,0x00,0x00,0x00,0x00          # mov    0x28(%rsp),%rbx
>> +	.byte 0x00,0x00,0x00,0x00               # add    $0x30,%rsp
>> +	.byte 0x30,0x30,0x30,0x30               # mov    (%rsp),%r11
>> +	.byte 0x30,0x30,0x30,0x30,0x30          # mov    0x8(%rsp),%r10
>> +	.byte 0x30,0x30,0x30,0x30,0x30          # mov    0x10(%rsp),%r9
>> +	.byte 0x30,0x30,0x30,0x30,0x30          # mov    0x18(%rsp),%r8
>> +	.byte 0x30,0x30,0x30,0x30,0x30          # mov    0x20(%rsp),%rax
>> +	.byte 0x30,0x30,0x30,0x30,0x30          # mov    0x28(%rsp),%rcx
>> +	.byte 0x30,0x30,0x30,0x30,0x30          # mov    0x30(%rsp),%rdx
>> +	.byte 0x30,0x30,0x30,0x30,0x30          # mov    0x38(%rsp),%rsi
>> +	.byte 0x30,0x30,0x30,0x30,0x30          # mov    0x40(%rsp),%rdi
>> +	.byte 0x30,0x30,0x30,0x30               # add    $0x50,%rsp
>> +	.byte 0x80,0x80,0x80,0x80               # testl  $NMI_MASK,2*8(%rsp)
>> +	.byte 0x80,0x80,0x80,0x80
>> +	.byte 0x80,0x80                         # jnz    2f
>> +	.byte 0x80,0x80,0x80,0x80               # testb  $1,(xen_features+XENFEAT_supervisor_mode_kernel)
>> +	.byte 0x80,0x80,0x80,0x80
>> +	.byte 0x80,0x80                         # jnz    1f
>> +	.byte 0x80,0x80,0x80,0x80,0x80          # orb    $3,1*8(%rsp)
>> +	.byte 0x80,0x80,0x80,0x80,0x80          # orb    $3,4*8(%rsp)
>> +	.byte 0x80,0x80                         # iretq
>> +	.byte 0x80,0x80,0x80,0x80               # andl   $~NMI_MASK, 16(%rsp)
>> +	.byte 0x80,0x80,0x80,0x80
>> +	.byte 0x80,0x80                         # pushq  $\flag
>> +	.byte 0x78,0x78,0x78,0x78,0x78          # jmp    hypercall_page + (__HYPERVISOR_iret * 32)
>> +	.byte 0x00,0x00,0x00,0x00               # XEN_LOCKED_BLOCK_EVENTS(%rsi)
>> +	.byte 0x00,0x00,0x00                    # mov    %rsp,%rdi
>> +	.byte 0x00,0x00,0x00,0x00,0x00          # jmp    11b
> This looks super-fragile.  The original Xen-linux kernel code had a
> similar kind of fixup table, but I went to some lengths to make it as
> simple and robust as possible in the pvops kernels.  See the comment in
> xen-asm_32.S:
>
>   * Because the nested interrupt handler needs to deal with the current
>   * stack state in whatever form its in, we keep things simple by only
>   * using a single register which is pushed/popped on the stack.
>
> 64-bit pvops Linux always uses the iret hypercall, so the issue is moot
> there.  (In principle a nested kernel interrupt could avoid the iret,
> but it wasn't obvious that the extra complexity was worth it.)
>
>      J
>

Thanks very much for you feedbacks.

I guess the concern is that using a chunky look-up table like this is 
somewhat complicated and would be less easier to maintain? Samuel made 
some suggestions, making it less "fragile" to some extent:

Also, it would be good to check against critical section size change, in
case somebody e.g. changes a value, or a macro like XEN_PUT_VCPU_INFO.
For instance, stuff right after the table:

	.if (ecrit-scrit) != (critical_fixup_table_end - critical_fixup_table)
	.error "The critical has changed, the fixup table needs updating"
	.endif


I read through Xen-Linux 32bit implementation for this issue. If I 
understand it right, the use of look-up table is avoided by leaving 
RESTORE_ALL/REST out of the critical section. So when a nested event 
came, you always certain about the stack frame layout and how many bytes 
to copy over when fixing up (in Xen-Linux 32bit, PT_EIP/4 - 1). And the 
register that would be clobbered in order to read and write event mask 
(%eax in Xen-Linux 32bit case) shall be saved on stack. This time we 
only need to worry about this one register instead of many when fixing 
up the stack frame.

Basically both approaches are correct to me and do the same job. 
Xen-Linux 32bit is definitely a more neat solution. However, one thing 
to notice is that mini-os x86 32bit also uses a fixup table. I guess we 
could apply these patches as a temporary solution to make sure 
everything is correct and consistent, and having following patches to do 
the refinement.

Thanks,

-- 
xu

:q!