Re: [RFC PATCH 11/35] Add support for Xen to entry.S.

Andi Kleen
2006-03-22T15:03:56Z

On Wednesday 22 March 2006 07:30, Chris Wright wrote:

> +
> +/* Offsets into shared_info_t. */
> +#define evtchn_upcall_pending /* 0 */
> +#define evtchn_upcall_mask 1
> +
> +#define sizeof_vcpu_shift 6

This should be all generated in asm-offsets.c

> +#else

This needs a comment explaining why it is different

> +restore_nocheck:
> + testl $(VM_MASK|NMI_MASK), EFLAGS(%esp)
> + jnz hypervisor_iret
> + movb EVENT_MASK(%esp), %al
> + notb %al # %al == ~saved_mask
> + GET_VCPU_INFO
> + andb evtchn_upcall_mask(%esi),%al
> + andb $1,%al # %al == mask & ~saved_mask

This would be faster if you used 32bit because it wouldn't cause
partial register stalls.

> + jnz restore_all_enable_events # != 0 => reenable event delivery
> +#endif
> RESTORE_REGS
> addl $4, %esp
> 1: iret
> .section .fixup,"ax"
> iret_exc:
> - sti
> +#ifndef CONFIG_XEN
> + ENABLE_INTERRUPTS
> +#endif
> pushl $0 # no error code
> pushl $do_iret_error
> jmp error_code
> @@ -269,6 +317,7 @@ iret_exc:
> .long 1b,iret_exc
> .previous
>
> +#ifndef CONFIG_XEN
> ldt_ss:

So are you sure that problem this ugly piece of code tries to work around
isn't in Xen kernels too? Or do you just not care? If yes add a comment.

> larl OLDSS(%esp), %eax
> jnz restore_nocheck
> @@ -281,7 +330,7 @@ ldt_ss:
> * CPUs, which we can try to work around to make
> * dosemu and wine happy. */
> subl $8, %esp # reserve space for switch16 pointer
> - cli
> + DISABLE_INTERRUPTS
> movl %esp, %eax
> /* Set up the 16bit stack frame with switch32 pointer on top,
> * and a switch16 pointer on top of the current frame. */
> @@ -293,6 +342,13 @@ ldt_ss:
> .align 4
> .long 1b,iret_exc
> .previous
> +#else

Needs comment.

> +hypervisor_iret:
> + andl $~NMI_MASK, EFLAGS(%esp)
> + RESTORE_REGS
> + addl $4, %esp
> + jmp hypercall_page + (__HYPERVISOR_iret * 32)
> +#endif
>

>
> ENTRY(divide_error)
> pushl $0 # no error code
> @@ -462,6 +522,126 @@ error_code:
> call *%edi
> jmp ret_from_exception
>
> +#ifdef CONFIG_XEN

It would be nicer to put that one into a separate file

> +# A note on the "critical region" in our callback handler.

> pushl $do_coprocessor_error
> @@ -475,17 +655,19 @@ ENTRY(simd_coprocessor_error)
> ENTRY(device_not_available)
> pushl $-1 # mark this as an int
> 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
> call math_emulate
> addl $4, %esp
> jmp ret_from_exception
> +device_available_emulate:
> +#endif
> + preempt_stop
> + call math_state_restore
> + jmp ret_from_exception

It's not quite clear why the Xen special case is needed here. Just because
of the CR0 access? If yes better put that one into a macro that just writes
0 to the register for the Xen case.

-Andi