Re: [Xen-devel] [PATCH v2] xen: get rid of paravirt op adjust_exception_frame
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
On 01/08/17 21:45, Andy Lutomirski wrote: > On Tue, Aug 1, 2017 at 3:39 AM, Juergen Grosswrote: >> 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
On Tue, Aug 1, 2017 at 4:38 PM, Andrew Cooperwrote: > 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
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
On Tue, Aug 1, 2017 at 3:39 AM, Juergen Grosswrote: > 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),