Re: [Xen-devel] [PATCH v2] xen: get rid of paravirt op adjust_exception_frame

2017-08-03 Thread Juergen Gross 
On 02/08/17 01:52, Andy Lutomirski wrote:
> On Tue, Aug 1, 2017 at 4:38 PM, Andrew Cooper  
> wrote:
>> On 01/08/2017 20:45, Andy Lutomirski wrote:
>>> Also, IMO it would be nice to fully finish the job.  Remaining steps are:
>>>
>>> 1. Unsuck the SYSCALL entries on Xen PV.
>>> 2. Unsuck the SYENTER entry on Xen PV.
>>> 3. Make a xen_nmi that's actually correct (should be trivial)
>>>
>>> #1 is here:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/entry_syscall=14fee348e3e86c994400d68085217d1232a637d6
>>
>> If the
>>
>> /* Zero-extending 32-bit regs, do not remove */
>> movl%eax, %eax
>>
>> comments are to be believed, then the entry logic needs reordering
>> slightly to:
>>
>> GLOBAL(entry_*_compat_after_hwframe)
>> movl%eax, %eax/* Zero-extending 32-bit regs, do not remove */
>> pushq%rax/* pt_regs->orig_ax */
> 
> D'oh, right.  Juergen, want to make that change?

Hmm, I'd prefer you sending an update which I could test and ack then.


Juergen


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] xen: get rid of paravirt op adjust_exception_frame

2017-08-02 Thread Juergen Gross 
On 01/08/17 21:45, Andy Lutomirski wrote:
> On Tue, Aug 1, 2017 at 3:39 AM, Juergen Gross  wrote:
>> When running as Xen pv-guest the exception frame on the stack contains
>> %r11 and %rcx additional to the other data pushed by the processor.
>>
>> Instead of having a paravirt op being called for each exception type
>> prepend the Xen specific code to each exception entry. When running as
>> Xen pv-guest just use the exception entry with prepended instructions,
>> otherwise use the entry without the Xen specific code.
>>
>> Signed-off-by: Juergen Gross 
>> ---
>>  arch/x86/entry/entry_64.S | 22 ++
>>  arch/x86/entry/entry_64_compat.S  |  1 -
>>  arch/x86/include/asm/desc.h   | 16 ++
>>  arch/x86/include/asm/paravirt.h   |  5 ---
>>  arch/x86/include/asm/paravirt_types.h |  4 ---
>>  arch/x86/include/asm/proto.h  |  3 ++
>>  arch/x86/include/asm/traps.h  | 51 +--
>>  arch/x86/kernel/asm-offsets_64.c  |  1 -
>>  arch/x86/kernel/paravirt.c|  3 --
>>  arch/x86/kernel/traps.c   | 57 
>> ++-
>>  arch/x86/xen/enlighten_pv.c   | 16 +-
>>  arch/x86/xen/irq.c|  3 --
>>  arch/x86/xen/xen-asm_64.S | 45 ---
>>  arch/x86/xen/xen-ops.h|  1 -
>>  14 files changed, 147 insertions(+), 81 deletions(-)
>>
>> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
>> index a9a8027a6c0e..602bcf68a32c 100644
>> --- a/arch/x86/entry/entry_64.S
>> +++ b/arch/x86/entry/entry_64.S
>> @@ -745,7 +745,6 @@ ENTRY(\sym)
>> .endif
>>
>> ASM_CLAC
>> -   PARAVIRT_ADJUST_EXCEPTION_FRAME
>>
>> .ifeq \has_error_code
>> pushq   $-1 /* ORIG_RAX: no syscall to 
>> restart */
>> @@ -901,7 +900,7 @@ ENTRY(do_softirq_own_stack)
>>  END(do_softirq_own_stack)
>>
>>  #ifdef CONFIG_XEN
>> -idtentry xen_hypervisor_callback xen_do_hypervisor_callback has_error_code=0
>> +idtentry hypervisor_callback xen_do_hypervisor_callback has_error_code=0
>>
>>  /*
>>   * A note on the "critical region" in our callback handler.
>> @@ -967,8 +966,6 @@ ENTRY(xen_failsafe_callback)
>> movq8(%rsp), %r11
>> addq$0x30, %rsp
>> pushq   $0  /* RIP */
>> -   pushq   %r11
>> -   pushq   %rcx
>> jmp general_protection
>>  1: /* Segment mismatch => Category 1 (Bad segment). Retry the IRET. */
>> movq(%rsp), %rcx
>> @@ -997,9 +994,8 @@ idtentry int3   do_int3  
>>has_error_code=0paranoid=1 shift_ist=DEBUG_STACK
>>  idtentry stack_segment do_stack_segmenthas_error_code=1
>>
>>  #ifdef CONFIG_XEN
>> -idtentry xen_debug do_debughas_error_code=0
>> -idtentry xen_int3  do_int3 has_error_code=0
>> -idtentry xen_stack_segment do_stack_segmenthas_error_code=1
>> +idtentry xendebug  do_debughas_error_code=0
>> +idtentry xenint3   do_int3 has_error_code=0
>>  #endif
>>
>>  idtentry general_protectiondo_general_protection   has_error_code=1
>> @@ -1161,18 +1157,6 @@ END(error_exit)
>>  /* Runs on exception stack */
>>  ENTRY(nmi)
>> /*
>> -* Fix up the exception frame if we're on Xen.
>> -* PARAVIRT_ADJUST_EXCEPTION_FRAME is guaranteed to push at most
>> -* one value to the stack on native, so it may clobber the rdx
>> -* scratch slot, but it won't clobber any of the important
>> -* slots past it.
>> -*
>> -* Xen is a different story, because the Xen frame itself overlaps
>> -* the "NMI executing" variable.
>> -*/
> 
> Based on Andrew Cooper's email, it sounds like this function is just
> straight-up broken on Xen PV.  Maybe change the comment to "XXX:
> broken on Xen PV" or similar.

Fine with me.

> 
>> +#if defined(CONFIG_X86_64) && defined(CONFIG_XEN_PV)
>> +#define pv_trap_entry(name) (void *)(xen_pv_domain() ? xen_ ## name : name)
>> +#else
>> +#define pv_trap_entry(name) (void *)(name)
>> +#endif
>> +
> 
> Seems reasonable to me.
> 
>>  #ifdef CONFIG_X86_64
>>
>>  static inline void pack_gate(gate_desc *gate, unsigned type, unsigned long 
>> func,
>> @@ -482,6 +490,14 @@ static inline void _set_gate(int gate, unsigned type, 
>> void *addr,
>> 0, 0, __KERNEL_CS); \
>> } while (0)
>>
>> +#define set_intr_gate_pv(n, addr)  \
>> +   do {\
>> +   set_intr_gate_notrace(n, pv_trap_entry(addr));  \
>> +   _trace_set_gate(n, GATE_INTERRUPT,  \
>> +

Re: [Xen-devel] [PATCH v2] xen: get rid of paravirt op adjust_exception_frame

2017-08-01 Thread Andy Lutomirski 
On Tue, Aug 1, 2017 at 4:38 PM, Andrew Cooper  wrote:
> On 01/08/2017 20:45, Andy Lutomirski wrote:
>> Also, IMO it would be nice to fully finish the job.  Remaining steps are:
>>
>> 1. Unsuck the SYSCALL entries on Xen PV.
>> 2. Unsuck the SYENTER entry on Xen PV.
>> 3. Make a xen_nmi that's actually correct (should be trivial)
>>
>> #1 is here:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/entry_syscall=14fee348e3e86c994400d68085217d1232a637d6
>
> If the
>
> /* Zero-extending 32-bit regs, do not remove */
> movl%eax, %eax
>
> comments are to be believed, then the entry logic needs reordering
> slightly to:
>
> GLOBAL(entry_*_compat_after_hwframe)
> movl%eax, %eax/* Zero-extending 32-bit regs, do not remove */
> pushq%rax/* pt_regs->orig_ax */

D'oh, right.  Juergen, want to make that change?

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] xen: get rid of paravirt op adjust_exception_frame

2017-08-01 Thread Andrew Cooper 
On 01/08/2017 20:45, Andy Lutomirski wrote:
> Also, IMO it would be nice to fully finish the job.  Remaining steps are:
>
> 1. Unsuck the SYSCALL entries on Xen PV.
> 2. Unsuck the SYENTER entry on Xen PV.
> 3. Make a xen_nmi that's actually correct (should be trivial)
>
> #1 is here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/entry_syscall=14fee348e3e86c994400d68085217d1232a637d6

If the

/* Zero-extending 32-bit regs, do not remove */
movl%eax, %eax

comments are to be believed, then the entry logic needs reordering
slightly to:

GLOBAL(entry_*_compat_after_hwframe)
movl%eax, %eax/* Zero-extending 32-bit regs, do not remove */
pushq%rax/* pt_regs->orig_ax */

~Andrew

(It is unfortunate this can't be simplified to pushq %eax)

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] xen: get rid of paravirt op adjust_exception_frame

2017-08-01 Thread Andy Lutomirski 
On Tue, Aug 1, 2017 at 3:39 AM, Juergen Gross  wrote:
> When running as Xen pv-guest the exception frame on the stack contains
> %r11 and %rcx additional to the other data pushed by the processor.
>
> Instead of having a paravirt op being called for each exception type
> prepend the Xen specific code to each exception entry. When running as
> Xen pv-guest just use the exception entry with prepended instructions,
> otherwise use the entry without the Xen specific code.
>
> Signed-off-by: Juergen Gross 
> ---
>  arch/x86/entry/entry_64.S | 22 ++
>  arch/x86/entry/entry_64_compat.S  |  1 -
>  arch/x86/include/asm/desc.h   | 16 ++
>  arch/x86/include/asm/paravirt.h   |  5 ---
>  arch/x86/include/asm/paravirt_types.h |  4 ---
>  arch/x86/include/asm/proto.h  |  3 ++
>  arch/x86/include/asm/traps.h  | 51 +--
>  arch/x86/kernel/asm-offsets_64.c  |  1 -
>  arch/x86/kernel/paravirt.c|  3 --
>  arch/x86/kernel/traps.c   | 57 
> ++-
>  arch/x86/xen/enlighten_pv.c   | 16 +-
>  arch/x86/xen/irq.c|  3 --
>  arch/x86/xen/xen-asm_64.S | 45 ---
>  arch/x86/xen/xen-ops.h|  1 -
>  14 files changed, 147 insertions(+), 81 deletions(-)
>
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index a9a8027a6c0e..602bcf68a32c 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -745,7 +745,6 @@ ENTRY(\sym)
> .endif
>
> ASM_CLAC
> -   PARAVIRT_ADJUST_EXCEPTION_FRAME
>
> .ifeq \has_error_code
> pushq   $-1 /* ORIG_RAX: no syscall to 
> restart */
> @@ -901,7 +900,7 @@ ENTRY(do_softirq_own_stack)
>  END(do_softirq_own_stack)
>
>  #ifdef CONFIG_XEN
> -idtentry xen_hypervisor_callback xen_do_hypervisor_callback has_error_code=0
> +idtentry hypervisor_callback xen_do_hypervisor_callback has_error_code=0
>
>  /*
>   * A note on the "critical region" in our callback handler.
> @@ -967,8 +966,6 @@ ENTRY(xen_failsafe_callback)
> movq8(%rsp), %r11
> addq$0x30, %rsp
> pushq   $0  /* RIP */
> -   pushq   %r11
> -   pushq   %rcx
> jmp general_protection
>  1: /* Segment mismatch => Category 1 (Bad segment). Retry the IRET. */
> movq(%rsp), %rcx
> @@ -997,9 +994,8 @@ idtentry int3   do_int3   
>   has_error_code=0paranoid=1 shift_ist=DEBUG_STACK
>  idtentry stack_segment do_stack_segmenthas_error_code=1
>
>  #ifdef CONFIG_XEN
> -idtentry xen_debug do_debughas_error_code=0
> -idtentry xen_int3  do_int3 has_error_code=0
> -idtentry xen_stack_segment do_stack_segmenthas_error_code=1
> +idtentry xendebug  do_debughas_error_code=0
> +idtentry xenint3   do_int3 has_error_code=0
>  #endif
>
>  idtentry general_protectiondo_general_protection   has_error_code=1
> @@ -1161,18 +1157,6 @@ END(error_exit)
>  /* Runs on exception stack */
>  ENTRY(nmi)
> /*
> -* Fix up the exception frame if we're on Xen.
> -* PARAVIRT_ADJUST_EXCEPTION_FRAME is guaranteed to push at most
> -* one value to the stack on native, so it may clobber the rdx
> -* scratch slot, but it won't clobber any of the important
> -* slots past it.
> -*
> -* Xen is a different story, because the Xen frame itself overlaps
> -* the "NMI executing" variable.
> -*/

Based on Andrew Cooper's email, it sounds like this function is just
straight-up broken on Xen PV.  Maybe change the comment to "XXX:
broken on Xen PV" or similar.

> +#if defined(CONFIG_X86_64) && defined(CONFIG_XEN_PV)
> +#define pv_trap_entry(name) (void *)(xen_pv_domain() ? xen_ ## name : name)
> +#else
> +#define pv_trap_entry(name) (void *)(name)
> +#endif
> +

Seems reasonable to me.

>  #ifdef CONFIG_X86_64
>
>  static inline void pack_gate(gate_desc *gate, unsigned type, unsigned long 
> func,
> @@ -482,6 +490,14 @@ static inline void _set_gate(int gate, unsigned type, 
> void *addr,
> 0, 0, __KERNEL_CS); \
> } while (0)
>
> +#define set_intr_gate_pv(n, addr)  \
> +   do {\
> +   set_intr_gate_notrace(n, pv_trap_entry(addr));  \
> +   _trace_set_gate(n, GATE_INTERRUPT,  \
> +   pv_trap_entry(trace_##addr),\
> +   0, 0, __KERNEL_CS); \
> +   } while (0)

Any reason this can't be set_intr_gate((n),