* [PATCH RFC for-next 0/4] Reimpliement {compat_, }create_bounce_frame() in C
@ 2017-05-08 15:48 Andrew Cooper
2017-05-08 15:48 ` [PATCH 1/4] x86/pv: Drop int80_bounce from struct pv_vcpu Andrew Cooper
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Andrew Cooper @ 2017-05-08 15:48 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
Bloat-o-meter reports:
add/remove: 1/1 grow/shrink: 0/1 up/down: 1211/-2587 (-1376)
function old new delta
pv_create_exception_frame - 1211 +1211
asm_domain_crash_synchronous 65 - -65
context_switch 3565 1043 -2522
but as the assembly symbols aren't tagged with type/size information, it can't
spot the removal of {compat_,}create_bounce_frame() at all.
This series is based on top of the "[PATCH for-4.9 0/2] x86/pv: Misc fixes"
series, which were discovered while developing this series.
Andrew Cooper (4):
x86/pv: Drop int80_bounce from struct pv_vcpu
x86/pv: Introduce pv_create_exception_frame()
x86/pv: Drop {compat_,}create_bounce_frame() and use the C version instead
x86/pv: Implement the failsafe callback using the general path
xen/arch/x86/domain.c | 102 ++--------------------
xen/arch/x86/pv/traps.c | 152 ++++++++++++++++++++++++++++++++
xen/arch/x86/traps.c | 27 ------
xen/arch/x86/x86_64/asm-offsets.c | 1 -
xen/arch/x86/x86_64/compat/entry.S | 116 ++-----------------------
xen/arch/x86/x86_64/compat/traps.c | 4 -
xen/arch/x86/x86_64/entry.S | 173 +++++++------------------------------
xen/arch/x86/x86_64/traps.c | 14 ---
xen/include/asm-x86/domain.h | 1 -
xen/include/asm-x86/processor.h | 3 +-
xen/include/xen/sched.h | 7 --
11 files changed, 198 insertions(+), 402 deletions(-)
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/4] x86/pv: Drop int80_bounce from struct pv_vcpu
2017-05-08 15:48 [PATCH RFC for-next 0/4] Reimpliement {compat_, }create_bounce_frame() in C Andrew Cooper
@ 2017-05-08 15:48 ` Andrew Cooper
2017-05-09 14:49 ` Jan Beulich
2017-05-08 15:48 ` [PATCH 2/4] x86/pv: Introduce pv_create_exception_frame() Andrew Cooper
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2017-05-08 15:48 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
The int80_bounce field of struct pv_vcpu is a bit of an odd special case,
because it is a simple derivation of trap_ctxt[0x80], which is also stored.
It is also the only use of {compat_,}create_bounce_frame() which isn't
referencing the plain trap_bounce field of struct pv_vcpu. (And altering this
property the purpose of this patch.)
Remove the int80_bounce field entirely, along with init_int80_direct_trap(),
which in turn requires that the int80_direct_trap() path gain logic previously
contained in init_int80_direct_trap().
This does admittedly make the int80 fastpath slightly longer, but these few
instructions are in the noise compared to the architectural context switch
overhead, and it now matches the syscall/sysenter paths (which have far less
architectural overhead already).
No behavioural change from the guests point of view.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/domain.c | 2 --
xen/arch/x86/traps.c | 4 ----
xen/arch/x86/x86_64/asm-offsets.c | 1 -
xen/arch/x86/x86_64/compat/traps.c | 4 ----
xen/arch/x86/x86_64/entry.S | 32 ++++++++++++++++++++++++++++----
xen/arch/x86/x86_64/traps.c | 14 --------------
xen/include/asm-x86/domain.h | 1 -
xen/include/asm-x86/processor.h | 2 --
8 files changed, 28 insertions(+), 32 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 2ef1c9f..7b301e3 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -833,8 +833,6 @@ int arch_set_info_guest(
goto out;
}
- init_int80_direct_trap(v);
-
/* IOPL privileges are virtualised. */
v->arch.pv_vcpu.iopl = v->arch.user_regs.eflags & X86_EFLAGS_IOPL;
v->arch.user_regs.eflags &= ~X86_EFLAGS_IOPL;
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 27fdf12..ece2c13 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -4013,7 +4013,6 @@ long do_set_trap_table(XEN_GUEST_HANDLE_PARAM(const_trap_info_t) traps)
if ( guest_handle_is_null(traps) )
{
memset(dst, 0, NR_VECTORS * sizeof(*dst));
- init_int80_direct_trap(curr);
return 0;
}
@@ -4035,9 +4034,6 @@ long do_set_trap_table(XEN_GUEST_HANDLE_PARAM(const_trap_info_t) traps)
memcpy(&dst[cur.vector], &cur, sizeof(cur));
- if ( cur.vector == 0x80 )
- init_int80_direct_trap(curr);
-
guest_handle_add_offset(traps, 1);
if ( hypercall_preempt_check() )
diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index e136af6..adf2749 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -61,7 +61,6 @@ void __dummy__(void)
OFFSET(VCPU_domain, struct vcpu, domain);
OFFSET(VCPU_vcpu_info, struct vcpu, vcpu_info);
OFFSET(VCPU_trap_bounce, struct vcpu, arch.pv_vcpu.trap_bounce);
- OFFSET(VCPU_int80_bounce, struct vcpu, arch.pv_vcpu.int80_bounce);
OFFSET(VCPU_thread_flags, struct vcpu, arch.flags);
OFFSET(VCPU_event_addr, struct vcpu, arch.pv_vcpu.event_callback_eip);
OFFSET(VCPU_event_sel, struct vcpu, arch.pv_vcpu.event_callback_cs);
diff --git a/xen/arch/x86/x86_64/compat/traps.c b/xen/arch/x86/x86_64/compat/traps.c
index 1751ec6..0ab2c31 100644
--- a/xen/arch/x86/x86_64/compat/traps.c
+++ b/xen/arch/x86/x86_64/compat/traps.c
@@ -339,7 +339,6 @@ int compat_set_trap_table(XEN_GUEST_HANDLE(trap_info_compat_t) traps)
if ( guest_handle_is_null(traps) )
{
memset(dst, 0, NR_VECTORS * sizeof(*dst));
- init_int80_direct_trap(current);
return 0;
}
@@ -358,9 +357,6 @@ int compat_set_trap_table(XEN_GUEST_HANDLE(trap_info_compat_t) traps)
XLAT_trap_info(dst + cur.vector, &cur);
- if ( cur.vector == 0x80 )
- init_int80_direct_trap(current);
-
guest_handle_add_offset(traps, 1);
if ( hypercall_preempt_check() )
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 65c771f..57952d0 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -233,12 +233,36 @@ UNLIKELY_END(msi_check)
GET_CURRENT(bx)
- /* Check that the callback is non-null. */
- leaq VCPU_int80_bounce(%rbx),%rdx
- cmpb $0,TRAPBOUNCE_flags(%rdx)
+ mov VCPU_trap_ctxt(%rbx), %rsi
+ mov VCPU_domain(%rbx), %rax
+
+ /*
+ * if ( null_trap_bounce(v, &v->arch.pv_vcpu.trap_ctxt[0x80]) )
+ * goto int80_slow_path;
+ */
+ mov 0x80 * TRAPINFO_sizeof + TRAPINFO_eip(%rsi), %rdi
+ movzwl 0x80 * TRAPINFO_sizeof + TRAPINFO_cs (%rsi), %ecx
+
+ mov %ecx, %edx
+ and $~3, %edx
+
+ testb $1, DOMAIN_is_32bit_pv(%rax)
+ cmove %rdi, %rdx
+
+ test %rdx, %rdx
jz int80_slow_path
- movq VCPU_domain(%rbx),%rax
+ /* Construct trap_bounce from trap_ctxt[0x80]. */
+ lea VCPU_trap_bounce(%rbx), %rdx
+ movw %cx, TRAPBOUNCE_cs(%rdx)
+ movq %rdi, TRAPBOUNCE_eip(%rdx)
+
+ /* TB_flags = TBF_EXCEPTION | (TI_GET_IF(ti) ? TBF_INTERRUPT : 0); */
+ testb $4, 0x80 * TRAPINFO_sizeof + TRAPINFO_flags(%rsi)
+ setnz %cl
+ lea TBF_EXCEPTION(, %rcx, TBF_INTERRUPT), %ecx
+ movb %cl, TRAPBOUNCE_flags(%rdx)
+
testb $1,DOMAIN_is_32bit_pv(%rax)
jnz compat_int80_direct_trap
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index 78f4105..69f1dc2 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -422,20 +422,6 @@ void subarch_percpu_traps_init(void)
wrmsrl(MSR_SYSCALL_MASK, XEN_SYSCALL_MASK);
}
-void init_int80_direct_trap(struct vcpu *v)
-{
- struct trap_info *ti = &v->arch.pv_vcpu.trap_ctxt[0x80];
- struct trap_bounce *tb = &v->arch.pv_vcpu.int80_bounce;
-
- tb->cs = ti->cs;
- tb->eip = ti->address;
-
- if ( null_trap_bounce(v, tb) )
- tb->flags = 0;
- else
- tb->flags = TBF_EXCEPTION | (TI_GET_IF(ti) ? TBF_INTERRUPT : 0);
-}
-
static long register_guest_callback(struct callback_register *reg)
{
long ret = 0;
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 6ab987f..ece016b 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -473,7 +473,6 @@ struct pv_vcpu
/* Bounce information for propagating an exception to guest OS. */
struct trap_bounce trap_bounce;
- struct trap_bounce int80_bounce;
/* I/O-port access bitmap. */
XEN_GUEST_HANDLE(uint8) iobmp; /* Guest kernel vaddr of the bitmap. */
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 1d1a4ff..50435e3 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -466,8 +466,6 @@ extern idt_entry_t *idt_tables[];
DECLARE_PER_CPU(struct tss_struct, init_tss);
-extern void init_int80_direct_trap(struct vcpu *v);
-
extern void write_ptbase(struct vcpu *v);
void destroy_gdt(struct vcpu *d);
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/4] x86/pv: Introduce pv_create_exception_frame()
2017-05-08 15:48 [PATCH RFC for-next 0/4] Reimpliement {compat_, }create_bounce_frame() in C Andrew Cooper
2017-05-08 15:48 ` [PATCH 1/4] x86/pv: Drop int80_bounce from struct pv_vcpu Andrew Cooper
@ 2017-05-08 15:48 ` Andrew Cooper
2017-05-09 15:58 ` Jan Beulich
2017-05-08 15:48 ` [PATCH 3/4] x86/pv: Drop {compat_, }create_bounce_frame() and use the C version instead Andrew Cooper
2017-05-08 15:48 ` [PATCH 4/4] x86/pv: Implement the failsafe callback using the general path Andrew Cooper
3 siblings, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2017-05-08 15:48 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
This is a C implementation of {compat_,}create_bounce_frame(), based loosely
on the existing failsafe implementation in load_segments(). It picks up all
injection information from the trap_bounce structure.
One minor improvement is that at not point is regs->cs left with an rpl of 0
on the root stack frame.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/pv/traps.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 133 insertions(+)
diff --git a/xen/arch/x86/pv/traps.c b/xen/arch/x86/pv/traps.c
index 51125a8..8973b23 100644
--- a/xen/arch/x86/pv/traps.c
+++ b/xen/arch/x86/pv/traps.c
@@ -32,6 +32,139 @@ void do_entry_int82(struct cpu_user_regs *regs)
}
/*
+ * This function emulates the behaviour of hardware when Xen needs to inject
+ * an event into into a guest.
+ *
+ * It may switch from user mode to kernel mode, will write an appropriate
+ * hardware exception frame (including Xen-specific extras), and alter the
+ * root stack frame to invoke the guest kernels correct entry point on exit
+ * from the hypervisor.
+ */
+void pv_create_exception_frame(void)
+{
+ struct vcpu *curr = current;
+ struct trap_bounce *tb = &curr->arch.pv_vcpu.trap_bounce;
+ struct cpu_user_regs *regs = guest_cpu_user_regs();
+ const bool user_mode_frame = !guest_kernel_mode(curr, regs);
+ uint8_t *evt_mask = &vcpu_info(curr, evtchn_upcall_mask);
+ unsigned long rflags;
+ unsigned int bytes, missing;
+
+ ASSERT_NOT_IN_ATOMIC();
+
+ if ( unlikely(null_trap_bounce(curr, tb)) )
+ {
+ gprintk(XENLOG_ERR, "Fatal: Attempting to inject null trap bounce\n");
+ __domain_crash_synchronous();
+ }
+
+ /* Fold the upcall mask and architectural IOPL into the guests rflags. */
+ rflags = regs->rflags & ~(X86_EFLAGS_IF | X86_EFLAGS_IOPL);
+ rflags |= ((*evt_mask ? 0 : X86_EFLAGS_IF) |
+ (VM_ASSIST(curr->domain, architectural_iopl)
+ ? curr->arch.pv_vcpu.iopl : 0));
+
+ if ( is_pv_32bit_vcpu(curr) )
+ {
+ /* { [ERRCODE,] EIP, CS/MASK , EFLAGS, [ESP, SS] } */
+ unsigned int frame[6], *ptr = frame, ksp =
+ (user_mode_frame ? curr->arch.pv_vcpu.kernel_sp : regs->esp);
+
+ if ( tb->flags & TBF_EXCEPTION_ERRCODE )
+ *ptr++ = tb->error_code;
+
+ *ptr++ = regs->eip;
+ *ptr++ = regs->cs | (((unsigned int)*evt_mask) << 16);
+ *ptr++ = rflags;
+
+ if ( user_mode_frame )
+ {
+ *ptr++ = regs->esp;
+ *ptr++ = regs->ss;
+ }
+
+ /* Copy the constructed frame to the guest kernel stack. */
+ bytes = _p(ptr) - _p(frame);
+ ksp -= bytes;
+
+ if ( unlikely((missing = __copy_to_user(_p(ksp), frame, bytes)) != 0) )
+ {
+ gprintk(XENLOG_ERR, "Fatal: Fault while writing exception frame\n");
+ show_page_walk(ksp + missing);
+ __domain_crash_synchronous();
+ }
+
+ /* Rewrite our stack frame. */
+ regs->rip = (uint32_t)tb->eip;
+ regs->cs = tb->cs;
+ regs->eflags &= ~(X86_EFLAGS_VM | X86_EFLAGS_RF |
+ X86_EFLAGS_NT | X86_EFLAGS_TF);
+ regs->rsp = ksp;
+ if ( user_mode_frame )
+ regs->ss = curr->arch.pv_vcpu.kernel_ss;
+ }
+ else
+ {
+ /* { RCX, R11, [ERRCODE,] RIP, CS/MASK, RFLAGS, RSP, SS } */
+ unsigned long frame[7], *ptr = frame, ksp =
+ (user_mode_frame ? curr->arch.pv_vcpu.kernel_sp : regs->rsp) & ~0xf;
+
+ if ( user_mode_frame )
+ toggle_guest_mode(curr);
+
+ *ptr++ = regs->rcx;
+ *ptr++ = regs->r11;
+
+ if ( tb->flags & TBF_EXCEPTION_ERRCODE )
+ *ptr++ = tb->error_code;
+
+ *ptr++ = regs->rip;
+ *ptr++ = (user_mode_frame ? regs->cs : regs->cs & ~3) |
+ ((unsigned long)(*evt_mask) << 32);
+ *ptr++ = rflags;
+ *ptr++ = regs->rsp;
+ *ptr++ = regs->ss;
+
+ /* Copy the constructed frame to the guest kernel stack. */
+ bytes = _p(ptr) - _p(frame);
+ ksp -= bytes;
+
+ if ( unlikely(!__addr_ok(ksp)) )
+ {
+ gprintk(XENLOG_ERR, "Fatal: Bad guest kernel stack %p\n", _p(ksp));
+ __domain_crash_synchronous();
+ }
+ else if ( unlikely((missing =
+ __copy_to_user(_p(ksp), frame, bytes)) != 0) )
+ {
+ gprintk(XENLOG_ERR, "Fatal: Fault while writing exception frame\n");
+ show_page_walk(ksp + missing);
+ __domain_crash_synchronous();
+ }
+
+ /* Rewrite our stack frame. */
+ regs->entry_vector |= TRAP_syscall;
+ regs->rip = tb->eip;
+ regs->cs = FLAT_KERNEL_CS;
+ regs->rflags &= ~(X86_EFLAGS_AC | X86_EFLAGS_VM | X86_EFLAGS_RF |
+ X86_EFLAGS_NT | X86_EFLAGS_TF);
+ regs->rsp = ksp;
+ regs->ss = FLAT_KERNEL_SS;
+ }
+
+ /* Mask events if requested. */
+ if ( tb->flags & TBF_INTERRUPT )
+ *evt_mask = 1;
+
+ /*
+ * Clobber the injection information now it has been completed. Buggy
+ * attempts to inject the same event twice will hit the null_trap_bounce()
+ * check above.
+ */
+ *tb = (struct trap_bounce){};
+}
+
+/*
* Local variables:
* mode: C
* c-file-style: "BSD"
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/4] x86/pv: Drop {compat_, }create_bounce_frame() and use the C version instead
2017-05-08 15:48 [PATCH RFC for-next 0/4] Reimpliement {compat_, }create_bounce_frame() in C Andrew Cooper
2017-05-08 15:48 ` [PATCH 1/4] x86/pv: Drop int80_bounce from struct pv_vcpu Andrew Cooper
2017-05-08 15:48 ` [PATCH 2/4] x86/pv: Introduce pv_create_exception_frame() Andrew Cooper
@ 2017-05-08 15:48 ` Andrew Cooper
2017-05-09 16:16 ` Jan Beulich
2017-05-08 15:48 ` [PATCH 4/4] x86/pv: Implement the failsafe callback using the general path Andrew Cooper
3 siblings, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2017-05-08 15:48 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
The clobbering of TRAPBOUNCE_flags in .L{compat_}bounce_exception is subsumed
by the logic at the end of pv_create_bounce_frame().
This cleanup removes all callers of asm_domain_crash_synchronous(), which is
therefore dropped as well.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/traps.c | 23 ------
xen/arch/x86/x86_64/compat/entry.S | 116 ++----------------------------
xen/arch/x86/x86_64/entry.S | 141 ++-----------------------------------
xen/include/xen/sched.h | 7 --
4 files changed, 10 insertions(+), 277 deletions(-)
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index ece2c13..73a9c7c 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -4195,29 +4195,6 @@ unsigned long do_get_debugreg(int reg)
return -EINVAL;
}
-void asm_domain_crash_synchronous(unsigned long addr)
-{
- /*
- * We need clear AC bit here because in entry.S AC is set
- * by ASM_STAC to temporarily allow accesses to user pages
- * which is prevented by SMAP by default.
- *
- * For some code paths, where this function is called, clac()
- * is not needed, but adding clac() here instead of each place
- * asm_domain_crash_synchronous() is called can reduce the code
- * redundancy, and it is harmless as well.
- */
- clac();
-
- if ( addr == 0 )
- addr = this_cpu(last_extable_addr);
-
- printk("domain_crash_sync called from entry.S: fault at %p %pS\n",
- _p(addr), _p(addr));
-
- __domain_crash_synchronous();
-}
-
/*
* Local variables:
* mode: C
diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
index 90bda09..1cd4672 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -51,7 +51,7 @@ compat_test_guest_events:
movl VCPU_event_sel(%rbx),%eax
movw %ax,TRAPBOUNCE_cs(%rdx)
movb $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
- call compat_create_bounce_frame
+ call pv_create_exception_frame
jmp compat_test_all_events
ALIGN
@@ -95,7 +95,7 @@ compat_process_nmi:
/* FALLTHROUGH */
compat_process_trap:
leaq VCPU_trap_bounce(%rbx),%rdx
- call compat_create_bounce_frame
+ call pv_create_exception_frame
jmp compat_test_all_events
/* %rbx: struct vcpu, interrupts disabled */
@@ -181,8 +181,7 @@ ENTRY(compat_post_handle_exception)
testb $TBF_EXCEPTION,TRAPBOUNCE_flags(%rdx)
jz compat_test_all_events
.Lcompat_bounce_exception:
- call compat_create_bounce_frame
- movb $0,TRAPBOUNCE_flags(%rdx)
+ call pv_create_exception_frame
jmp compat_test_all_events
/* See lstar_enter for entry register state. */
@@ -234,115 +233,10 @@ ENTRY(compat_sysenter)
movl $FLAT_COMPAT_USER_SS,UREGS_ss(%rsp)
cmovzl %ecx,%eax
movw %ax,TRAPBOUNCE_cs(%rdx)
- call compat_create_bounce_frame
+ call pv_create_exception_frame
jmp compat_test_all_events
ENTRY(compat_int80_direct_trap)
CR4_PV32_RESTORE
- call compat_create_bounce_frame
+ call pv_create_exception_frame
jmp compat_test_all_events
-
-/* CREATE A BASIC EXCEPTION FRAME ON GUEST OS (RING-1) STACK: */
-/* {[ERRCODE,] EIP, CS, EFLAGS, [ESP, SS]} */
-/* %rdx: trap_bounce, %rbx: struct vcpu */
-/* On return only %rbx and %rdx are guaranteed non-clobbered. */
-compat_create_bounce_frame:
- ASSERT_INTERRUPTS_ENABLED
- mov %fs,%edi
- ASM_STAC
- testb $2,UREGS_cs+8(%rsp)
- jz 1f
- /* Push new frame at registered guest-OS stack base. */
- movl VCPU_kernel_sp(%rbx),%esi
-.Lft1: mov VCPU_kernel_ss(%rbx),%fs
- subl $2*4,%esi
- movl UREGS_rsp+8(%rsp),%eax
-.Lft2: movl %eax,%fs:(%rsi)
- movl UREGS_ss+8(%rsp),%eax
-.Lft3: movl %eax,%fs:4(%rsi)
- jmp 2f
-1: /* In kernel context already: push new frame at existing %rsp. */
- movl UREGS_rsp+8(%rsp),%esi
-.Lft4: mov UREGS_ss+8(%rsp),%fs
-2:
- movq VCPU_domain(%rbx),%r8
- subl $3*4,%esi
- movq VCPU_vcpu_info(%rbx),%rax
- pushq COMPAT_VCPUINFO_upcall_mask(%rax)
- testb $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
- setnz %ch # TBF_INTERRUPT -> set upcall mask
- orb %ch,COMPAT_VCPUINFO_upcall_mask(%rax)
- popq %rax
- shll $16,%eax # Bits 16-23: saved_upcall_mask
- movw UREGS_cs+8(%rsp),%ax # Bits 0-15: CS
-.Lft5: movl %eax,%fs:4(%rsi) # CS / saved_upcall_mask
- shrl $16,%eax
- testb %al,%al # Bits 0-7: saved_upcall_mask
- setz %ch # %ch == !saved_upcall_mask
- movl UREGS_eflags+8(%rsp),%eax
- andl $~(X86_EFLAGS_IF|X86_EFLAGS_IOPL),%eax
- addb %ch,%ch # Bit 9 (EFLAGS.IF)
- orb %ch,%ah # Fold EFLAGS.IF into %eax
- xorl %ecx,%ecx # if ( VM_ASSIST(v->domain, architectural_iopl) )
- testb $1 << VMASST_TYPE_architectural_iopl,DOMAIN_vm_assist(%r8)
- cmovnzl VCPU_iopl(%rbx),%ecx # Bits 13:12 (EFLAGS.IOPL)
- orl %ecx,%eax # Fold EFLAGS.IOPL into %eax
-.Lft6: movl %eax,%fs:2*4(%rsi) # EFLAGS
- movl UREGS_rip+8(%rsp),%eax
-.Lft7: movl %eax,%fs:(%rsi) # EIP
- testb $TBF_EXCEPTION_ERRCODE,TRAPBOUNCE_flags(%rdx)
- jz 1f
- subl $4,%esi
- movl TRAPBOUNCE_error_code(%rdx),%eax
-.Lft8: movl %eax,%fs:(%rsi) # ERROR CODE
-1:
- ASM_CLAC
- /* Rewrite our stack frame and return to guest-OS mode. */
- /* IA32 Ref. Vol. 3: TF, VM, RF and NT flags are cleared on trap. */
- andl $~(X86_EFLAGS_VM|X86_EFLAGS_RF|\
- X86_EFLAGS_NT|X86_EFLAGS_TF),UREGS_eflags+8(%rsp)
- mov %fs,UREGS_ss+8(%rsp)
- movl %esi,UREGS_rsp+8(%rsp)
-.Lft13: mov %edi,%fs
- movzwl TRAPBOUNCE_cs(%rdx),%eax
- /* Null selectors (0-3) are not allowed. */
- testl $~3,%eax
-UNLIKELY_START(z, compat_bounce_null_selector)
- lea UNLIKELY_DISPATCH_LABEL(compat_bounce_null_selector)(%rip), %rdi
- jmp asm_domain_crash_synchronous /* Does not return */
-__UNLIKELY_END(compat_bounce_null_selector)
- movl %eax,UREGS_cs+8(%rsp)
- movl TRAPBOUNCE_eip(%rdx),%eax
- movl %eax,UREGS_rip+8(%rsp)
- ret
-.section .fixup,"ax"
-.Lfx13:
- xorl %edi,%edi
- jmp .Lft13
-.previous
- _ASM_EXTABLE(.Lft1, dom_crash_sync_extable)
- _ASM_EXTABLE(.Lft2, compat_crash_page_fault)
- _ASM_EXTABLE(.Lft3, compat_crash_page_fault_4)
- _ASM_EXTABLE(.Lft4, dom_crash_sync_extable)
- _ASM_EXTABLE(.Lft5, compat_crash_page_fault_4)
- _ASM_EXTABLE(.Lft6, compat_crash_page_fault_8)
- _ASM_EXTABLE(.Lft7, compat_crash_page_fault)
- _ASM_EXTABLE(.Lft8, compat_crash_page_fault)
- _ASM_EXTABLE(.Lft13, .Lfx13)
-
-compat_crash_page_fault_8:
- addl $4,%esi
-compat_crash_page_fault_4:
- addl $4,%esi
-compat_crash_page_fault:
-.Lft14: mov %edi,%fs
- ASM_CLAC
- movl %esi,%edi
- call show_page_walk
- jmp dom_crash_sync_extable
-.section .fixup,"ax"
-.Lfx14:
- xorl %edi,%edi
- jmp .Lft14
-.previous
- _ASM_EXTABLE(.Lft14, .Lfx14)
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 57952d0..7d59051 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -28,7 +28,7 @@ ENTRY(switch_to_kernel)
setc %cl
leal (,%rcx,TBF_INTERRUPT),%ecx
movb %cl,TRAPBOUNCE_flags(%rdx)
- call create_bounce_frame
+ call pv_create_exception_frame
andl $~X86_EFLAGS_DF,UREGS_eflags(%rsp)
jmp test_all_events
@@ -131,7 +131,7 @@ test_guest_events:
movq VCPU_event_addr(%rbx),%rax
movq %rax,TRAPBOUNCE_eip(%rdx)
movb $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
- call create_bounce_frame
+ call pv_create_exception_frame
jmp test_all_events
ALIGN
@@ -175,7 +175,7 @@ process_nmi:
/* FALLTHROUGH */
process_trap:
leaq VCPU_trap_bounce(%rbx),%rdx
- call create_bounce_frame
+ call pv_create_exception_frame
jmp test_all_events
ENTRY(sysenter_entry)
@@ -266,7 +266,7 @@ UNLIKELY_END(msi_check)
testb $1,DOMAIN_is_32bit_pv(%rax)
jnz compat_int80_direct_trap
- call create_bounce_frame
+ call pv_create_exception_frame
jmp test_all_events
int80_slow_path:
@@ -281,136 +281,6 @@ int80_slow_path:
subq $2,UREGS_rip(%rsp)
jmp handle_exception_saved
-/* CREATE A BASIC EXCEPTION FRAME ON GUEST OS STACK: */
-/* { RCX, R11, [ERRCODE,] RIP, CS, RFLAGS, RSP, SS } */
-/* %rdx: trap_bounce, %rbx: struct vcpu */
-/* On return only %rbx and %rdx are guaranteed non-clobbered. */
-create_bounce_frame:
- ASSERT_INTERRUPTS_ENABLED
- testb $TF_kernel_mode,VCPU_thread_flags(%rbx)
- jnz 1f
- /* Push new frame at registered guest-OS stack base. */
- pushq %rdx
- movq %rbx,%rdi
- call toggle_guest_mode
- popq %rdx
- movq VCPU_kernel_sp(%rbx),%rsi
- jmp 2f
-1: /* In kernel context already: push new frame at existing %rsp. */
- movq UREGS_rsp+8(%rsp),%rsi
- andb $0xfc,UREGS_cs+8(%rsp) # Indicate kernel context to guest.
-2: andq $~0xf,%rsi # Stack frames are 16-byte aligned.
- movq $HYPERVISOR_VIRT_START+1,%rax
- cmpq %rax,%rsi
- movq $HYPERVISOR_VIRT_END+8*8,%rax
- sbb %ecx,%ecx # In +ve address space? Then okay.
- cmpq %rax,%rsi
- adc %ecx,%ecx # Above Xen private area? Then okay.
-UNLIKELY_START(g, create_bounce_frame_bad_sp)
- lea UNLIKELY_DISPATCH_LABEL(create_bounce_frame_bad_sp)(%rip), %rdi
- jmp asm_domain_crash_synchronous /* Does not return */
-__UNLIKELY_END(create_bounce_frame_bad_sp)
-
-#define STORE_GUEST_STACK(reg, n) \
-0: movq %reg,(n)*8(%rsi); \
- _ASM_EXTABLE(0b, domain_crash_page_fault_ ## n ## x8)
-
- subq $7*8,%rsi
- movq UREGS_ss+8(%rsp),%rax
- ASM_STAC
- movq VCPU_domain(%rbx),%rdi
- STORE_GUEST_STACK(rax,6) # SS
- movq UREGS_rsp+8(%rsp),%rax
- STORE_GUEST_STACK(rax,5) # RSP
- movq VCPU_vcpu_info(%rbx),%rax
- pushq VCPUINFO_upcall_mask(%rax)
- testb $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
- setnz %ch # TBF_INTERRUPT -> set upcall mask
- orb %ch,VCPUINFO_upcall_mask(%rax)
- popq %rax
- shlq $32,%rax # Bits 32-39: saved_upcall_mask
- movw UREGS_cs+8(%rsp),%ax # Bits 0-15: CS
- STORE_GUEST_STACK(rax,3) # CS / saved_upcall_mask
- shrq $32,%rax
- testb $0xFF,%al # Bits 0-7: saved_upcall_mask
- setz %ch # %ch == !saved_upcall_mask
- movl UREGS_eflags+8(%rsp),%eax
- andl $~(X86_EFLAGS_IF|X86_EFLAGS_IOPL),%eax
- addb %ch,%ch # Bit 9 (EFLAGS.IF)
- orb %ch,%ah # Fold EFLAGS.IF into %eax
- xorl %ecx,%ecx # if ( VM_ASSIST(v->domain, architectural_iopl) )
- testb $1 << VMASST_TYPE_architectural_iopl,DOMAIN_vm_assist(%rdi)
- cmovnzl VCPU_iopl(%rbx),%ecx # Bits 13:12 (EFLAGS.IOPL)
- orl %ecx,%eax # Fold EFLAGS.IOPL into %eax
- STORE_GUEST_STACK(rax,4) # RFLAGS
- movq UREGS_rip+8(%rsp),%rax
- STORE_GUEST_STACK(rax,2) # RIP
- testb $TBF_EXCEPTION_ERRCODE,TRAPBOUNCE_flags(%rdx)
- jz 1f
- subq $8,%rsi
- movl TRAPBOUNCE_error_code(%rdx),%eax
- STORE_GUEST_STACK(rax,2) # ERROR CODE
-1:
- movq UREGS_r11+8(%rsp),%rax
- STORE_GUEST_STACK(rax,1) # R11
- movq UREGS_rcx+8(%rsp),%rax
- STORE_GUEST_STACK(rax,0) # RCX
- ASM_CLAC
-
-#undef STORE_GUEST_STACK
-
- /* Rewrite our stack frame and return to guest-OS mode. */
- /* IA32 Ref. Vol. 3: TF, VM, RF and NT flags are cleared on trap. */
- /* Also clear AC: alignment checks shouldn't trigger in kernel mode. */
- orl $TRAP_syscall,UREGS_entry_vector+8(%rsp)
- andl $~(X86_EFLAGS_AC|X86_EFLAGS_VM|X86_EFLAGS_RF|\
- X86_EFLAGS_NT|X86_EFLAGS_TF),UREGS_eflags+8(%rsp)
- movq $FLAT_KERNEL_SS,UREGS_ss+8(%rsp)
- movq %rsi,UREGS_rsp+8(%rsp)
- movq $FLAT_KERNEL_CS,UREGS_cs+8(%rsp)
- movq TRAPBOUNCE_eip(%rdx),%rax
- testq %rax,%rax
-UNLIKELY_START(z, create_bounce_frame_bad_bounce_ip)
- lea UNLIKELY_DISPATCH_LABEL(create_bounce_frame_bad_bounce_ip)(%rip), %rdi
- jmp asm_domain_crash_synchronous /* Does not return */
-__UNLIKELY_END(create_bounce_frame_bad_bounce_ip)
- movq %rax,UREGS_rip+8(%rsp)
- ret
-
- .pushsection .fixup, "ax", @progbits
- # Numeric tags below represent the intended overall %rsi adjustment.
-domain_crash_page_fault_6x8:
- addq $8,%rsi
-domain_crash_page_fault_5x8:
- addq $8,%rsi
-domain_crash_page_fault_4x8:
- addq $8,%rsi
-domain_crash_page_fault_3x8:
- addq $8,%rsi
-domain_crash_page_fault_2x8:
- addq $8,%rsi
-domain_crash_page_fault_1x8:
- addq $8,%rsi
-domain_crash_page_fault_0x8:
- ASM_CLAC
- movq %rsi,%rdi
- call show_page_walk
-ENTRY(dom_crash_sync_extable)
- ASM_CLAC
- # Get out of the guest-save area of the stack.
- GET_STACK_END(ax)
- leaq STACK_CPUINFO_FIELD(guest_cpu_user_regs)(%rax),%rsp
- # create_bounce_frame() temporarily clobbers CS.RPL. Fix up.
- __GET_CURRENT(ax)
- movq VCPU_domain(%rax),%rax
- testb $1,DOMAIN_is_32bit_pv(%rax)
- setz %al
- leal (%rax,%rax,2),%eax
- orb %al,UREGS_cs(%rsp)
- xorl %edi,%edi
- jmp asm_domain_crash_synchronous /* Does not return */
- .popsection
-
ENTRY(common_interrupt)
SAVE_ALL CLAC
CR4_PV32_RESTORE
@@ -506,8 +376,7 @@ handle_exception_saved:
testb $TBF_EXCEPTION,TRAPBOUNCE_flags(%rdx)
jz test_all_events
.Lbounce_exception:
- call create_bounce_frame
- movb $0,TRAPBOUNCE_flags(%rdx)
+ call pv_create_exception_frame
jmp test_all_events
/* No special register assumptions. */
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 1127ca9..a0ef63a 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -632,13 +632,6 @@ void noreturn __domain_crash_synchronous(void);
__domain_crash_synchronous(); \
} while (0)
-/*
- * Called from assembly code, with an optional address to help indicate why
- * the crash occured. If addr is 0, look up address from last extable
- * redirection.
- */
-void noreturn asm_domain_crash_synchronous(unsigned long addr);
-
#define set_current_state(_s) do { current->state = (_s); } while (0)
void scheduler_init(void);
int sched_init_vcpu(struct vcpu *v, unsigned int processor);
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/4] x86/pv: Implement the failsafe callback using the general path
2017-05-08 15:48 [PATCH RFC for-next 0/4] Reimpliement {compat_, }create_bounce_frame() in C Andrew Cooper
` (2 preceding siblings ...)
2017-05-08 15:48 ` [PATCH 3/4] x86/pv: Drop {compat_, }create_bounce_frame() and use the C version instead Andrew Cooper
@ 2017-05-08 15:48 ` Andrew Cooper
2017-05-09 16:22 ` Jan Beulich
3 siblings, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2017-05-08 15:48 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
Reintroduce TBF_FAILSAFE and update pv_create_exception_frame() to cope with
the additional data segment registers.
load_segments() now fills in trap_bounce, and lets the general return-to-guest
path inject the exception.
Bloat-o-meter reports:
add/remove: 0/0 grow/shrink: 1/1 up/down: 123/-2522 (-2399)
function old new delta
pv_create_exception_frame 1088 1211 +123
context_switch 3565 1043 -2522
which I suspect is largely due to the quantity of code hidden behind
put_user().
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
xen/arch/x86/domain.c | 100 +++-------------------------------------
xen/arch/x86/pv/traps.c | 31 ++++++++++---
xen/include/asm-x86/processor.h | 1 +
3 files changed, 33 insertions(+), 99 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 7b301e3..c533e05 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1275,100 +1275,14 @@ static void load_segments(struct vcpu *n)
if ( unlikely(!all_segs_okay) )
{
- struct pv_vcpu *pv = &n->arch.pv_vcpu;
- struct cpu_user_regs *regs = guest_cpu_user_regs();
- unsigned long *rsp =
- (unsigned long *)(((n->arch.flags & TF_kernel_mode)
- ? regs->rsp : pv->kernel_sp) & ~0xf);
- unsigned long cs_and_mask, rflags;
-
- /* Fold upcall mask and architectural IOPL into RFLAGS.IF. */
- rflags = regs->rflags & ~(X86_EFLAGS_IF|X86_EFLAGS_IOPL);
- rflags |= !vcpu_info(n, evtchn_upcall_mask) << 9;
- if ( VM_ASSIST(n->domain, architectural_iopl) )
- rflags |= n->arch.pv_vcpu.iopl;
-
- if ( is_pv_32bit_vcpu(n) )
- {
- unsigned int *esp = ring_1(regs) ?
- (unsigned int *)regs->rsp :
- (unsigned int *)pv->kernel_sp;
- int ret = 0;
-
- /* CS longword also contains full evtchn_upcall_mask. */
- cs_and_mask = (unsigned short)regs->cs |
- ((unsigned int)vcpu_info(n, evtchn_upcall_mask) << 16);
-
- if ( !ring_1(regs) )
- {
- ret = put_user(regs->ss, esp-1);
- ret |= put_user(regs->esp, esp-2);
- esp -= 2;
- }
-
- if ( ret |
- put_user(rflags, esp-1) |
- put_user(cs_and_mask, esp-2) |
- put_user(regs->eip, esp-3) |
- put_user(uregs->gs, esp-4) |
- put_user(uregs->fs, esp-5) |
- put_user(uregs->es, esp-6) |
- put_user(uregs->ds, esp-7) )
- {
- gprintk(XENLOG_ERR,
- "error while creating compat failsafe callback frame\n");
- domain_crash(n->domain);
- }
+ bool disable = n->arch.vgc_flags & VGCF_failsafe_disables_events;
- if ( n->arch.vgc_flags & VGCF_failsafe_disables_events )
- vcpu_info(n, evtchn_upcall_mask) = 1;
-
- regs->entry_vector |= TRAP_syscall;
- regs->eflags &= ~(X86_EFLAGS_VM|X86_EFLAGS_RF|X86_EFLAGS_NT|
- X86_EFLAGS_IOPL|X86_EFLAGS_TF);
- regs->ss = FLAT_COMPAT_KERNEL_SS;
- regs->esp = (unsigned long)(esp-7);
- regs->cs = FLAT_COMPAT_KERNEL_CS;
- regs->eip = pv->failsafe_callback_eip;
- return;
- }
-
- if ( !(n->arch.flags & TF_kernel_mode) )
- toggle_guest_mode(n);
- else
- regs->cs &= ~3;
-
- /* CS longword also contains full evtchn_upcall_mask. */
- cs_and_mask = (unsigned long)regs->cs |
- ((unsigned long)vcpu_info(n, evtchn_upcall_mask) << 32);
-
- if ( put_user(regs->ss, rsp- 1) |
- put_user(regs->rsp, rsp- 2) |
- put_user(rflags, rsp- 3) |
- put_user(cs_and_mask, rsp- 4) |
- put_user(regs->rip, rsp- 5) |
- put_user(uregs->gs, rsp- 6) |
- put_user(uregs->fs, rsp- 7) |
- put_user(uregs->es, rsp- 8) |
- put_user(uregs->ds, rsp- 9) |
- put_user(regs->r11, rsp-10) |
- put_user(regs->rcx, rsp-11) )
- {
- gprintk(XENLOG_ERR,
- "error while creating failsafe callback frame\n");
- domain_crash(n->domain);
- }
-
- if ( n->arch.vgc_flags & VGCF_failsafe_disables_events )
- vcpu_info(n, evtchn_upcall_mask) = 1;
-
- regs->entry_vector |= TRAP_syscall;
- regs->rflags &= ~(X86_EFLAGS_AC|X86_EFLAGS_VM|X86_EFLAGS_RF|
- X86_EFLAGS_NT|X86_EFLAGS_IOPL|X86_EFLAGS_TF);
- regs->ss = FLAT_KERNEL_SS;
- regs->rsp = (unsigned long)(rsp-11);
- regs->cs = FLAT_KERNEL_CS;
- regs->rip = pv->failsafe_callback_eip;
+ n->arch.pv_vcpu.trap_bounce = (struct trap_bounce){
+ .flags = (TBF_FAILSAFE | TBF_EXCEPTION |
+ (disable ? TBF_INTERRUPT : 0)),
+ .cs = FLAT_COMPAT_KERNEL_CS, /* Ignored for 64bit guests. */
+ .eip = n->arch.pv_vcpu.failsafe_callback_eip
+ };
}
}
diff --git a/xen/arch/x86/pv/traps.c b/xen/arch/x86/pv/traps.c
index 8973b23..e372ecd 100644
--- a/xen/arch/x86/pv/traps.c
+++ b/xen/arch/x86/pv/traps.c
@@ -44,7 +44,8 @@ void pv_create_exception_frame(void)
{
struct vcpu *curr = current;
struct trap_bounce *tb = &curr->arch.pv_vcpu.trap_bounce;
- struct cpu_user_regs *regs = guest_cpu_user_regs();
+ struct cpu_user_regs *regs = guest_cpu_user_regs(),
+ *uregs = &curr->arch.user_regs;
const bool user_mode_frame = !guest_kernel_mode(curr, regs);
uint8_t *evt_mask = &vcpu_info(curr, evtchn_upcall_mask);
unsigned long rflags;
@@ -66,10 +67,18 @@ void pv_create_exception_frame(void)
if ( is_pv_32bit_vcpu(curr) )
{
- /* { [ERRCODE,] EIP, CS/MASK , EFLAGS, [ESP, SS] } */
- unsigned int frame[6], *ptr = frame, ksp =
+ /* { [DS-GS,] [ERRCODE,] EIP, CS/MASK , EFLAGS, [ESP, SS] } */
+ unsigned int frame[10], *ptr = frame, ksp =
(user_mode_frame ? curr->arch.pv_vcpu.kernel_sp : regs->esp);
+ if ( tb->flags & TBF_FAILSAFE )
+ {
+ *ptr++ = uregs->ds;
+ *ptr++ = uregs->es;
+ *ptr++ = uregs->fs;
+ *ptr++ = uregs->gs;
+ }
+
if ( tb->flags & TBF_EXCEPTION_ERRCODE )
*ptr++ = tb->error_code;
@@ -100,13 +109,15 @@ void pv_create_exception_frame(void)
regs->eflags &= ~(X86_EFLAGS_VM | X86_EFLAGS_RF |
X86_EFLAGS_NT | X86_EFLAGS_TF);
regs->rsp = ksp;
- if ( user_mode_frame )
+ if ( tb->flags & TBF_FAILSAFE )
+ regs->ss = FLAT_COMPAT_KERNEL_SS;
+ else if ( user_mode_frame )
regs->ss = curr->arch.pv_vcpu.kernel_ss;
}
else
{
- /* { RCX, R11, [ERRCODE,] RIP, CS/MASK, RFLAGS, RSP, SS } */
- unsigned long frame[7], *ptr = frame, ksp =
+ /* { RCX, R11, [DS-GS,] [ERRCODE,] RIP, CS/MASK, RFLAGS, RSP, SS } */
+ unsigned long frame[11], *ptr = frame, ksp =
(user_mode_frame ? curr->arch.pv_vcpu.kernel_sp : regs->rsp) & ~0xf;
if ( user_mode_frame )
@@ -115,6 +126,14 @@ void pv_create_exception_frame(void)
*ptr++ = regs->rcx;
*ptr++ = regs->r11;
+ if ( tb->flags & TBF_FAILSAFE )
+ {
+ *ptr++ = uregs->ds;
+ *ptr++ = uregs->es;
+ *ptr++ = uregs->fs;
+ *ptr++ = uregs->gs;
+ }
+
if ( tb->flags & TBF_EXCEPTION_ERRCODE )
*ptr++ = tb->error_code;
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 50435e3..76414df 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -60,6 +60,7 @@
/* 'trap_bounce' flags values */
#define TBF_EXCEPTION 1
#define TBF_EXCEPTION_ERRCODE 2
+#define TBF_FAILSAFE 4
#define TBF_INTERRUPT 8
/* 'arch_vcpu' flags values */
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] x86/pv: Drop int80_bounce from struct pv_vcpu
2017-05-08 15:48 ` [PATCH 1/4] x86/pv: Drop int80_bounce from struct pv_vcpu Andrew Cooper
@ 2017-05-09 14:49 ` Jan Beulich
2017-05-09 15:09 ` Andrew Cooper
0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2017-05-09 14:49 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, Xen-devel
>>> On 08.05.17 at 17:48, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -233,12 +233,36 @@ UNLIKELY_END(msi_check)
>
> GET_CURRENT(bx)
>
> - /* Check that the callback is non-null. */
> - leaq VCPU_int80_bounce(%rbx),%rdx
> - cmpb $0,TRAPBOUNCE_flags(%rdx)
> + mov VCPU_trap_ctxt(%rbx), %rsi
> + mov VCPU_domain(%rbx), %rax
> +
> + /*
> + * if ( null_trap_bounce(v, &v->arch.pv_vcpu.trap_ctxt[0x80]) )
> + * goto int80_slow_path;
> + */
> + mov 0x80 * TRAPINFO_sizeof + TRAPINFO_eip(%rsi), %rdi
> + movzwl 0x80 * TRAPINFO_sizeof + TRAPINFO_cs (%rsi), %ecx
> +
> + mov %ecx, %edx
> + and $~3, %edx
> +
> + testb $1, DOMAIN_is_32bit_pv(%rax)
While we may have other such instances, this is dangerous (and long
ago I think we had actual issues with constructs like this one). Either
"cmpb" against zero, or "testb" with 0xff.
> + cmove %rdi, %rdx
As there's nothing "equal" here, but only the question of ZF being
set of clear, the more natural form would be "cmovz" (just like you
use "jz" and "setnz" below).
> + test %rdx, %rdx
> jz int80_slow_path
>
> - movq VCPU_domain(%rbx),%rax
> + /* Construct trap_bounce from trap_ctxt[0x80]. */
> + lea VCPU_trap_bounce(%rbx), %rdx
> + movw %cx, TRAPBOUNCE_cs(%rdx)
> + movq %rdi, TRAPBOUNCE_eip(%rdx)
> +
> + /* TB_flags = TBF_EXCEPTION | (TI_GET_IF(ti) ? TBF_INTERRUPT : 0); */
> + testb $4, 0x80 * TRAPINFO_sizeof + TRAPINFO_flags(%rsi)
> + setnz %cl
> + lea TBF_EXCEPTION(, %rcx, TBF_INTERRUPT), %ecx
> + movb %cl, TRAPBOUNCE_flags(%rdx)
In code further up you avoided mnemonic suffixes where possible,
yet in this code section you're (partly) using them even when a
register operand makes them redundant.
Since I understand this code will go away with subsequent patches
anyway, I don't insist on changing these though. Hence with or
without the adjustments
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] x86/pv: Drop int80_bounce from struct pv_vcpu
2017-05-09 14:49 ` Jan Beulich
@ 2017-05-09 15:09 ` Andrew Cooper
0 siblings, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2017-05-09 15:09 UTC (permalink / raw)
To: Jan Beulich; +Cc: Wei Liu, Xen-devel
On 09/05/17 15:49, Jan Beulich wrote:
>>>> On 08.05.17 at 17:48, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/x86_64/entry.S
>> +++ b/xen/arch/x86/x86_64/entry.S
>> @@ -233,12 +233,36 @@ UNLIKELY_END(msi_check)
>>
>> GET_CURRENT(bx)
>>
>> - /* Check that the callback is non-null. */
>> - leaq VCPU_int80_bounce(%rbx),%rdx
>> - cmpb $0,TRAPBOUNCE_flags(%rdx)
>> + mov VCPU_trap_ctxt(%rbx), %rsi
>> + mov VCPU_domain(%rbx), %rax
>> +
>> + /*
>> + * if ( null_trap_bounce(v, &v->arch.pv_vcpu.trap_ctxt[0x80]) )
>> + * goto int80_slow_path;
>> + */
>> + mov 0x80 * TRAPINFO_sizeof + TRAPINFO_eip(%rsi), %rdi
>> + movzwl 0x80 * TRAPINFO_sizeof + TRAPINFO_cs (%rsi), %ecx
>> +
>> + mov %ecx, %edx
>> + and $~3, %edx
>> +
>> + testb $1, DOMAIN_is_32bit_pv(%rax)
> While we may have other such instances, this is dangerous (and long
> ago I think we had actual issues with constructs like this one). Either
> "cmpb" against zero, or "testb" with 0xff.
Hmm. This was a straight copy of the logic to switch between the exit
paths.
I think I will prepare a fix for this general issue in a separate patch,
to avoid being mixed in with this logical change.
>
>> + cmove %rdi, %rdx
> As there's nothing "equal" here, but only the question of ZF being
> set of clear, the more natural form would be "cmovz" (just like you
> use "jz" and "setnz" below).
>
>> + test %rdx, %rdx
>> jz int80_slow_path
>>
>> - movq VCPU_domain(%rbx),%rax
>> + /* Construct trap_bounce from trap_ctxt[0x80]. */
>> + lea VCPU_trap_bounce(%rbx), %rdx
>> + movw %cx, TRAPBOUNCE_cs(%rdx)
>> + movq %rdi, TRAPBOUNCE_eip(%rdx)
>> +
>> + /* TB_flags = TBF_EXCEPTION | (TI_GET_IF(ti) ? TBF_INTERRUPT : 0); */
>> + testb $4, 0x80 * TRAPINFO_sizeof + TRAPINFO_flags(%rsi)
>> + setnz %cl
>> + lea TBF_EXCEPTION(, %rcx, TBF_INTERRUPT), %ecx
>> + movb %cl, TRAPBOUNCE_flags(%rdx)
> In code further up you avoided mnemonic suffixes where possible,
> yet in this code section you're (partly) using them even when a
> register operand makes them redundant.
Ah yes. I had a bug originally where I was moving %ecx rather than %cl
and clobbering flags, so reintroduced the suffixes to match the struct
trap_bounce definition.
>
> Since I understand this code will go away with subsequent patches
> anyway, I don't insist on changing these though. Hence with or
> without the adjustments
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
This code (hopefully) isn't going to stay around long, but it is
certainly not removed in this series. I will try to leave it in a good
state.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] x86/pv: Introduce pv_create_exception_frame()
2017-05-08 15:48 ` [PATCH 2/4] x86/pv: Introduce pv_create_exception_frame() Andrew Cooper
@ 2017-05-09 15:58 ` Jan Beulich
2017-05-09 17:09 ` Andrew Cooper
0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2017-05-09 15:58 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, Xen-devel
>>> On 08.05.17 at 17:48, <andrew.cooper3@citrix.com> wrote:
> +void pv_create_exception_frame(void)
> +{
> + struct vcpu *curr = current;
> + struct trap_bounce *tb = &curr->arch.pv_vcpu.trap_bounce;
const (twice)?
> + struct cpu_user_regs *regs = guest_cpu_user_regs();
> + const bool user_mode_frame = !guest_kernel_mode(curr, regs);
> + uint8_t *evt_mask = &vcpu_info(curr, evtchn_upcall_mask);
> + unsigned long rflags;
Does this really need to be "long"?
> + unsigned int bytes, missing;
> +
> + ASSERT_NOT_IN_ATOMIC();
> +
> + if ( unlikely(null_trap_bounce(curr, tb)) )
> + {
> + gprintk(XENLOG_ERR, "Fatal: Attempting to inject null trap bounce\n");
> + __domain_crash_synchronous();
Why not domain_crash() followed by "return"?
> + }
> +
> + /* Fold the upcall mask and architectural IOPL into the guests rflags. */
> + rflags = regs->rflags & ~(X86_EFLAGS_IF | X86_EFLAGS_IOPL);
> + rflags |= ((*evt_mask ? 0 : X86_EFLAGS_IF) |
> + (VM_ASSIST(curr->domain, architectural_iopl)
> + ? curr->arch.pv_vcpu.iopl : 0));
> +
> + if ( is_pv_32bit_vcpu(curr) )
> + {
> + /* { [ERRCODE,] EIP, CS/MASK , EFLAGS, [ESP, SS] } */
> + unsigned int frame[6], *ptr = frame, ksp =
> + (user_mode_frame ? curr->arch.pv_vcpu.kernel_sp : regs->esp);
> +
> + if ( tb->flags & TBF_EXCEPTION_ERRCODE )
> + *ptr++ = tb->error_code;
> +
> + *ptr++ = regs->eip;
> + *ptr++ = regs->cs | (((unsigned int)*evt_mask) << 16);
Do you really need the cast here? In no case is there a need for the
parentheses around the cast expression.
> + *ptr++ = rflags;
> +
> + if ( user_mode_frame )
> + {
> + *ptr++ = regs->esp;
> + *ptr++ = regs->ss;
> + }
> +
> + /* Copy the constructed frame to the guest kernel stack. */
> + bytes = _p(ptr) - _p(frame);
> + ksp -= bytes;
> +
> + if ( unlikely((missing = __copy_to_user(_p(ksp), frame, bytes)) != 0) )
While I don't think we need to be really bothered, it's perhaps still
worth noting in a comment that the wrapping behavior here is
wrong (and slightly worse than the assembly original), due to
(implicit) address arithmetic all being done with 64-bit operands.
> + {
> + gprintk(XENLOG_ERR, "Fatal: Fault while writing exception frame\n");
> + show_page_walk(ksp + missing);
> + __domain_crash_synchronous();
> + }
> +
> + /* Rewrite our stack frame. */
> + regs->rip = (uint32_t)tb->eip;
> + regs->cs = tb->cs;
> + regs->eflags &= ~(X86_EFLAGS_VM | X86_EFLAGS_RF |
> + X86_EFLAGS_NT | X86_EFLAGS_TF);
You write ->rip above and ->rsp below - preferably those would
become ->eip and ->esp, but alternatively (for consistency) this
may want switching to ->rflags.
> + regs->rsp = ksp;
> + if ( user_mode_frame )
> + regs->ss = curr->arch.pv_vcpu.kernel_ss;
> + }
> + else
> + {
> + /* { RCX, R11, [ERRCODE,] RIP, CS/MASK, RFLAGS, RSP, SS } */
> + unsigned long frame[7], *ptr = frame, ksp =
I clearly count 8 elements in the comment.
> + (user_mode_frame ? curr->arch.pv_vcpu.kernel_sp : regs->rsp) & ~0xf;
> +
> + if ( user_mode_frame )
> + toggle_guest_mode(curr);
> +
> + *ptr++ = regs->rcx;
> + *ptr++ = regs->r11;
> +
> + if ( tb->flags & TBF_EXCEPTION_ERRCODE )
> + *ptr++ = tb->error_code;
> +
> + *ptr++ = regs->rip;
> + *ptr++ = (user_mode_frame ? regs->cs : regs->cs & ~3) |
> + ((unsigned long)(*evt_mask) << 32);
Stray parentheses again.
> + *ptr++ = rflags;
> + *ptr++ = regs->rsp;
> + *ptr++ = regs->ss;
> +
> + /* Copy the constructed frame to the guest kernel stack. */
> + bytes = _p(ptr) - _p(frame);
> + ksp -= bytes;
> +
> + if ( unlikely(!__addr_ok(ksp)) )
> + {
> + gprintk(XENLOG_ERR, "Fatal: Bad guest kernel stack %p\n", _p(ksp));
> + __domain_crash_synchronous();
> + }
> + else if ( unlikely((missing =
> + __copy_to_user(_p(ksp), frame, bytes)) != 0) )
> + {
> + gprintk(XENLOG_ERR, "Fatal: Fault while writing exception frame\n");
> + show_page_walk(ksp + missing);
> + __domain_crash_synchronous();
> + }
> +
> + /* Rewrite our stack frame. */
> + regs->entry_vector |= TRAP_syscall;
> + regs->rip = tb->eip;
> + regs->cs = FLAT_KERNEL_CS;
> + regs->rflags &= ~(X86_EFLAGS_AC | X86_EFLAGS_VM | X86_EFLAGS_RF |
> + X86_EFLAGS_NT | X86_EFLAGS_TF);
> + regs->rsp = ksp;
> + regs->ss = FLAT_KERNEL_SS;
> + }
> +
> + /* Mask events if requested. */
> + if ( tb->flags & TBF_INTERRUPT )
> + *evt_mask = 1;
> +
> + /*
> + * Clobber the injection information now it has been completed. Buggy
> + * attempts to inject the same event twice will hit the null_trap_bounce()
> + * check above.
> + */
> + *tb = (struct trap_bounce){};
Ah, so that prevents tb becoming a pointer to const. I wonder
though whether, on a rather hot path, we really want to zap the
entire structure here. As I can see the value in satisfying
null_trap_bounce(), how about zapping just ->eip / ->cs on the
split paths above?
Overall, did you compare generated code with the current
assembly implementation? That one surely would have had some
room for improvement, so the result here at least shouldn't be
worse than that.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] x86/pv: Drop {compat_, }create_bounce_frame() and use the C version instead
2017-05-08 15:48 ` [PATCH 3/4] x86/pv: Drop {compat_, }create_bounce_frame() and use the C version instead Andrew Cooper
@ 2017-05-09 16:16 ` Jan Beulich
2017-05-09 17:24 ` Andrew Cooper
0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2017-05-09 16:16 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, Xen-devel
>>> On 08.05.17 at 17:48, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -51,7 +51,7 @@ compat_test_guest_events:
> movl VCPU_event_sel(%rbx),%eax
> movw %ax,TRAPBOUNCE_cs(%rdx)
> movb $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
> - call compat_create_bounce_frame
> + call pv_create_exception_frame
> jmp compat_test_all_events
>
> ALIGN
> @@ -95,7 +95,7 @@ compat_process_nmi:
> /* FALLTHROUGH */
> compat_process_trap:
> leaq VCPU_trap_bounce(%rbx),%rdx
> - call compat_create_bounce_frame
> + call pv_create_exception_frame
> jmp compat_test_all_events
The leaq should go away then too.
> @@ -181,8 +181,7 @@ ENTRY(compat_post_handle_exception)
> testb $TBF_EXCEPTION,TRAPBOUNCE_flags(%rdx)
> jz compat_test_all_events
> .Lcompat_bounce_exception:
> - call compat_create_bounce_frame
> - movb $0,TRAPBOUNCE_flags(%rdx)
> + call pv_create_exception_frame
> jmp compat_test_all_events
Considering this recurring pattern of call/jmp I wonder whether we
could reduce the branch tracking structure utilization in the CPU a
little by folding these paths.
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -28,7 +28,7 @@ ENTRY(switch_to_kernel)
> setc %cl
> leal (,%rcx,TBF_INTERRUPT),%ecx
> movb %cl,TRAPBOUNCE_flags(%rdx)
> - call create_bounce_frame
> + call pv_create_exception_frame
> andl $~X86_EFLAGS_DF,UREGS_eflags(%rsp)
> jmp test_all_events
>
> @@ -131,7 +131,7 @@ test_guest_events:
> movq VCPU_event_addr(%rbx),%rax
> movq %rax,TRAPBOUNCE_eip(%rdx)
> movb $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
> - call create_bounce_frame
> + call pv_create_exception_frame
> jmp test_all_events
>
> ALIGN
> @@ -175,7 +175,7 @@ process_nmi:
> /* FALLTHROUGH */
> process_trap:
> leaq VCPU_trap_bounce(%rbx),%rdx
> - call create_bounce_frame
> + call pv_create_exception_frame
> jmp test_all_events
Leftover leaq again.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] x86/pv: Implement the failsafe callback using the general path
2017-05-08 15:48 ` [PATCH 4/4] x86/pv: Implement the failsafe callback using the general path Andrew Cooper
@ 2017-05-09 16:22 ` Jan Beulich
0 siblings, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2017-05-09 16:22 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, Xen-devel
>>> On 08.05.17 at 17:48, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1275,100 +1275,14 @@ static void load_segments(struct vcpu *n)
>
> if ( unlikely(!all_segs_okay) )
> {
> - struct pv_vcpu *pv = &n->arch.pv_vcpu;
> - struct cpu_user_regs *regs = guest_cpu_user_regs();
> - unsigned long *rsp =
> - (unsigned long *)(((n->arch.flags & TF_kernel_mode)
> - ? regs->rsp : pv->kernel_sp) & ~0xf);
> - unsigned long cs_and_mask, rflags;
> -
> - /* Fold upcall mask and architectural IOPL into RFLAGS.IF. */
> - rflags = regs->rflags & ~(X86_EFLAGS_IF|X86_EFLAGS_IOPL);
> - rflags |= !vcpu_info(n, evtchn_upcall_mask) << 9;
> - if ( VM_ASSIST(n->domain, architectural_iopl) )
> - rflags |= n->arch.pv_vcpu.iopl;
> -
> - if ( is_pv_32bit_vcpu(n) )
> - {
> - unsigned int *esp = ring_1(regs) ?
> - (unsigned int *)regs->rsp :
> - (unsigned int *)pv->kernel_sp;
> - int ret = 0;
> -
> - /* CS longword also contains full evtchn_upcall_mask. */
> - cs_and_mask = (unsigned short)regs->cs |
> - ((unsigned int)vcpu_info(n, evtchn_upcall_mask) << 16);
> -
> - if ( !ring_1(regs) )
> - {
> - ret = put_user(regs->ss, esp-1);
> - ret |= put_user(regs->esp, esp-2);
> - esp -= 2;
> - }
> -
> - if ( ret |
> - put_user(rflags, esp-1) |
> - put_user(cs_and_mask, esp-2) |
> - put_user(regs->eip, esp-3) |
> - put_user(uregs->gs, esp-4) |
> - put_user(uregs->fs, esp-5) |
> - put_user(uregs->es, esp-6) |
> - put_user(uregs->ds, esp-7) )
> - {
> - gprintk(XENLOG_ERR,
> - "error while creating compat failsafe callback frame\n");
> - domain_crash(n->domain);
> - }
> + bool disable = n->arch.vgc_flags & VGCF_failsafe_disables_events;
>
> - if ( n->arch.vgc_flags & VGCF_failsafe_disables_events )
> - vcpu_info(n, evtchn_upcall_mask) = 1;
> -
> - regs->entry_vector |= TRAP_syscall;
> - regs->eflags &= ~(X86_EFLAGS_VM|X86_EFLAGS_RF|X86_EFLAGS_NT|
> - X86_EFLAGS_IOPL|X86_EFLAGS_TF);
> - regs->ss = FLAT_COMPAT_KERNEL_SS;
> - regs->esp = (unsigned long)(esp-7);
> - regs->cs = FLAT_COMPAT_KERNEL_CS;
> - regs->eip = pv->failsafe_callback_eip;
> - return;
> - }
> -
> - if ( !(n->arch.flags & TF_kernel_mode) )
> - toggle_guest_mode(n);
> - else
> - regs->cs &= ~3;
> -
> - /* CS longword also contains full evtchn_upcall_mask. */
> - cs_and_mask = (unsigned long)regs->cs |
> - ((unsigned long)vcpu_info(n, evtchn_upcall_mask) << 32);
> -
> - if ( put_user(regs->ss, rsp- 1) |
> - put_user(regs->rsp, rsp- 2) |
> - put_user(rflags, rsp- 3) |
> - put_user(cs_and_mask, rsp- 4) |
> - put_user(regs->rip, rsp- 5) |
> - put_user(uregs->gs, rsp- 6) |
> - put_user(uregs->fs, rsp- 7) |
> - put_user(uregs->es, rsp- 8) |
> - put_user(uregs->ds, rsp- 9) |
> - put_user(regs->r11, rsp-10) |
> - put_user(regs->rcx, rsp-11) )
> - {
> - gprintk(XENLOG_ERR,
> - "error while creating failsafe callback frame\n");
> - domain_crash(n->domain);
> - }
> -
> - if ( n->arch.vgc_flags & VGCF_failsafe_disables_events )
> - vcpu_info(n, evtchn_upcall_mask) = 1;
> -
> - regs->entry_vector |= TRAP_syscall;
> - regs->rflags &= ~(X86_EFLAGS_AC|X86_EFLAGS_VM|X86_EFLAGS_RF|
> - X86_EFLAGS_NT|X86_EFLAGS_IOPL|X86_EFLAGS_TF);
> - regs->ss = FLAT_KERNEL_SS;
> - regs->rsp = (unsigned long)(rsp-11);
> - regs->cs = FLAT_KERNEL_CS;
> - regs->rip = pv->failsafe_callback_eip;
> + n->arch.pv_vcpu.trap_bounce = (struct trap_bounce){
> + .flags = (TBF_FAILSAFE | TBF_EXCEPTION |
> + (disable ? TBF_INTERRUPT : 0)),
Pointless outer parentheses.
> --- a/xen/arch/x86/pv/traps.c
> +++ b/xen/arch/x86/pv/traps.c
> @@ -44,7 +44,8 @@ void pv_create_exception_frame(void)
> {
> struct vcpu *curr = current;
> struct trap_bounce *tb = &curr->arch.pv_vcpu.trap_bounce;
> - struct cpu_user_regs *regs = guest_cpu_user_regs();
> + struct cpu_user_regs *regs = guest_cpu_user_regs(),
> + *uregs = &curr->arch.user_regs;
These should be const, even if this requires a fully separate
declaration.
With these taken care of (and suitable re-basing over changes to
earlier patches),
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] x86/pv: Introduce pv_create_exception_frame()
2017-05-09 15:58 ` Jan Beulich
@ 2017-05-09 17:09 ` Andrew Cooper
2017-05-10 7:43 ` Jan Beulich
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2017-05-09 17:09 UTC (permalink / raw)
To: Jan Beulich; +Cc: Wei Liu, Xen-devel
On 09/05/17 16:58, Jan Beulich wrote:
>>>> On 08.05.17 at 17:48, <andrew.cooper3@citrix.com> wrote:
>> +void pv_create_exception_frame(void)
>> +{
>> + struct vcpu *curr = current;
>> + struct trap_bounce *tb = &curr->arch.pv_vcpu.trap_bounce;
> const (twice)?
>
>> + struct cpu_user_regs *regs = guest_cpu_user_regs();
>> + const bool user_mode_frame = !guest_kernel_mode(curr, regs);
>> + uint8_t *evt_mask = &vcpu_info(curr, evtchn_upcall_mask);
>> + unsigned long rflags;
> Does this really need to be "long"?
The answer to several of these questions are "probably not, but that's
how load_segments() did it".
>
>> + unsigned int bytes, missing;
>> +
>> + ASSERT_NOT_IN_ATOMIC();
>> +
>> + if ( unlikely(null_trap_bounce(curr, tb)) )
>> + {
>> + gprintk(XENLOG_ERR, "Fatal: Attempting to inject null trap bounce\n");
>> + __domain_crash_synchronous();
> Why not domain_crash() followed by "return"?
Because the existing code uses synchronous crashes.
Looking again at the callsites of pv_create_exception_frame(), we
immediately jump back to {compat_,}test_all_events, which proceeds to
run softirqs again.
Therefore, domain_crash() and a return should work. (I think?)
>
>> + }
>> +
>> + /* Fold the upcall mask and architectural IOPL into the guests rflags. */
>> + rflags = regs->rflags & ~(X86_EFLAGS_IF | X86_EFLAGS_IOPL);
>> + rflags |= ((*evt_mask ? 0 : X86_EFLAGS_IF) |
>> + (VM_ASSIST(curr->domain, architectural_iopl)
>> + ? curr->arch.pv_vcpu.iopl : 0));
>> +
>> + if ( is_pv_32bit_vcpu(curr) )
>> + {
>> + /* { [ERRCODE,] EIP, CS/MASK , EFLAGS, [ESP, SS] } */
>> + unsigned int frame[6], *ptr = frame, ksp =
>> + (user_mode_frame ? curr->arch.pv_vcpu.kernel_sp : regs->esp);
>> +
>> + if ( tb->flags & TBF_EXCEPTION_ERRCODE )
>> + *ptr++ = tb->error_code;
>> +
>> + *ptr++ = regs->eip;
>> + *ptr++ = regs->cs | (((unsigned int)*evt_mask) << 16);
> Do you really need the cast here?
Does it promote correctly if the top bit of the mask is set?
> In no case is there a need for the
> parentheses around the cast expression.
>
>> + *ptr++ = rflags;
>> +
>> + if ( user_mode_frame )
>> + {
>> + *ptr++ = regs->esp;
>> + *ptr++ = regs->ss;
>> + }
>> +
>> + /* Copy the constructed frame to the guest kernel stack. */
>> + bytes = _p(ptr) - _p(frame);
>> + ksp -= bytes;
>> +
>> + if ( unlikely((missing = __copy_to_user(_p(ksp), frame, bytes)) != 0) )
> While I don't think we need to be really bothered, it's perhaps still
> worth noting in a comment that the wrapping behavior here is
> wrong (and slightly worse than the assembly original), due to
> (implicit) address arithmetic all being done with 64-bit operands.
Ah - At some point, I had a comment here explaining the lack of an
__access_ok() check, but it appears to have got lost in a rebase. I
will try to reinstate it.
The wrapping behaviour around the 4GB => 0 boundary is undefined, and
different between Intel and AMD (as we discovered with XSA-186). If we
passing the exception back to the guest we would need to swap #PF for
#SS (for Intel), or properly wrap around (for AMD).
Would it be ok just to comment this point and leave it as is?
>
>> + {
>> + gprintk(XENLOG_ERR, "Fatal: Fault while writing exception frame\n");
>> + show_page_walk(ksp + missing);
>> + __domain_crash_synchronous();
>> + }
>> +
>> + /* Rewrite our stack frame. */
>> + regs->rip = (uint32_t)tb->eip;
>> + regs->cs = tb->cs;
>> + regs->eflags &= ~(X86_EFLAGS_VM | X86_EFLAGS_RF |
>> + X86_EFLAGS_NT | X86_EFLAGS_TF);
> You write ->rip above and ->rsp below - preferably those would
> become ->eip and ->esp, but alternatively (for consistency) this
> may want switching to ->rflags.
Ah - these are deliberately 64bit values even in the 32bit path, so a
32bit guest with an unexpected 64bit code segment will be truncated back
into its own range.
I will comment this point, and switch to using rflags.
>
>> + regs->rsp = ksp;
>> + if ( user_mode_frame )
>> + regs->ss = curr->arch.pv_vcpu.kernel_ss;
>> + }
>> + else
>> + {
>> + /* { RCX, R11, [ERRCODE,] RIP, CS/MASK, RFLAGS, RSP, SS } */
>> + unsigned long frame[7], *ptr = frame, ksp =
> I clearly count 8 elements in the comment.
:)
>
>> + (user_mode_frame ? curr->arch.pv_vcpu.kernel_sp : regs->rsp) & ~0xf;
>> +
>> + if ( user_mode_frame )
>> + toggle_guest_mode(curr);
>> +
>> + *ptr++ = regs->rcx;
>> + *ptr++ = regs->r11;
>> +
>> + if ( tb->flags & TBF_EXCEPTION_ERRCODE )
>> + *ptr++ = tb->error_code;
>> +
>> + *ptr++ = regs->rip;
>> + *ptr++ = (user_mode_frame ? regs->cs : regs->cs & ~3) |
>> + ((unsigned long)(*evt_mask) << 32);
> Stray parentheses again.
>
>> + *ptr++ = rflags;
>> + *ptr++ = regs->rsp;
>> + *ptr++ = regs->ss;
>> +
>> + /* Copy the constructed frame to the guest kernel stack. */
>> + bytes = _p(ptr) - _p(frame);
>> + ksp -= bytes;
>> +
>> + if ( unlikely(!__addr_ok(ksp)) )
>> + {
>> + gprintk(XENLOG_ERR, "Fatal: Bad guest kernel stack %p\n", _p(ksp));
>> + __domain_crash_synchronous();
>> + }
>> + else if ( unlikely((missing =
>> + __copy_to_user(_p(ksp), frame, bytes)) != 0) )
>> + {
>> + gprintk(XENLOG_ERR, "Fatal: Fault while writing exception frame\n");
>> + show_page_walk(ksp + missing);
>> + __domain_crash_synchronous();
>> + }
>> +
>> + /* Rewrite our stack frame. */
>> + regs->entry_vector |= TRAP_syscall;
>> + regs->rip = tb->eip;
>> + regs->cs = FLAT_KERNEL_CS;
>> + regs->rflags &= ~(X86_EFLAGS_AC | X86_EFLAGS_VM | X86_EFLAGS_RF |
>> + X86_EFLAGS_NT | X86_EFLAGS_TF);
>> + regs->rsp = ksp;
>> + regs->ss = FLAT_KERNEL_SS;
>> + }
>> +
>> + /* Mask events if requested. */
>> + if ( tb->flags & TBF_INTERRUPT )
>> + *evt_mask = 1;
>> +
>> + /*
>> + * Clobber the injection information now it has been completed. Buggy
>> + * attempts to inject the same event twice will hit the null_trap_bounce()
>> + * check above.
>> + */
>> + *tb = (struct trap_bounce){};
> Ah, so that prevents tb becoming a pointer to const. I wonder
> though whether, on a rather hot path, we really want to zap the
> entire structure here. As I can see the value in satisfying
> null_trap_bounce(), how about zapping just ->eip / ->cs on the
> split paths above?
This ends up being two 8-byte writes of zeroes into a cache-hot line; it
isn't by any means a slow part of this path, whereas the 16bit write to
clobber just %cs would be.
Irrespective of that, the following patch depends on this clobbering of
->flags.
> Overall, did you compare generated code with the current
> assembly implementation? That one surely would have had some
> room for improvement, so the result here at least shouldn't be
> worse than that.
The final C version (including failsafe, and some error handling the asm
functions didn't have) is a bit less than twice the size of the asm
functions in terms of absolute size.
I haven't done any performance analysis, but I trust the compiler to
make better code overall (there are definitely pipeline stalls in the
asm versions), and wouldn't be surprised if it is faster to execute.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] x86/pv: Drop {compat_, }create_bounce_frame() and use the C version instead
2017-05-09 16:16 ` Jan Beulich
@ 2017-05-09 17:24 ` Andrew Cooper
2017-05-10 7:50 ` Jan Beulich
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2017-05-09 17:24 UTC (permalink / raw)
To: Jan Beulich; +Cc: Wei Liu, Xen-devel
On 09/05/17 17:16, Jan Beulich wrote:
>>>> On 08.05.17 at 17:48, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/x86_64/compat/entry.S
>> +++ b/xen/arch/x86/x86_64/compat/entry.S
>> @@ -51,7 +51,7 @@ compat_test_guest_events:
>> movl VCPU_event_sel(%rbx),%eax
>> movw %ax,TRAPBOUNCE_cs(%rdx)
>> movb $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
>> - call compat_create_bounce_frame
>> + call pv_create_exception_frame
>> jmp compat_test_all_events
>>
>> ALIGN
>> @@ -95,7 +95,7 @@ compat_process_nmi:
>> /* FALLTHROUGH */
>> compat_process_trap:
>> leaq VCPU_trap_bounce(%rbx),%rdx
>> - call compat_create_bounce_frame
>> + call pv_create_exception_frame
>> jmp compat_test_all_events
> The leaq should go away then too.
So it can.
>
>> @@ -181,8 +181,7 @@ ENTRY(compat_post_handle_exception)
>> testb $TBF_EXCEPTION,TRAPBOUNCE_flags(%rdx)
>> jz compat_test_all_events
>> .Lcompat_bounce_exception:
>> - call compat_create_bounce_frame
>> - movb $0,TRAPBOUNCE_flags(%rdx)
>> + call pv_create_exception_frame
>> jmp compat_test_all_events
> Considering this recurring pattern of call/jmp I wonder whether we
> could reduce the branch tracking structure utilization in the CPU a
> little by folding these paths.
I think we can lift all of this softirq/event/nmi/mce handling logic up
into C, which will remove almost all of the guest-handling asm code in
entry.S
However, it would still retain this "goto again" structure, and will
eventually have to drop back into asm to actually return to the guest.
The problem (which I haven't got a good solution for yet) is that, if I
make the C functions noreturn, I will need to duplicate them so they can
jmp to the proper return point. The alternative would be a moderately
hard to predict conditional jump.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] x86/pv: Introduce pv_create_exception_frame()
2017-05-09 17:09 ` Andrew Cooper
@ 2017-05-10 7:43 ` Jan Beulich
0 siblings, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2017-05-10 7:43 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, Xen-devel
>>> On 09.05.17 at 19:09, <andrew.cooper3@citrix.com> wrote:
> On 09/05/17 16:58, Jan Beulich wrote:
>>>>> On 08.05.17 at 17:48, <andrew.cooper3@citrix.com> wrote:
>>> +void pv_create_exception_frame(void)
>>> +{
>>> + struct vcpu *curr = current;
>>> + struct trap_bounce *tb = &curr->arch.pv_vcpu.trap_bounce;
>> const (twice)?
>>
>>> + struct cpu_user_regs *regs = guest_cpu_user_regs();
>>> + const bool user_mode_frame = !guest_kernel_mode(curr, regs);
>>> + uint8_t *evt_mask = &vcpu_info(curr, evtchn_upcall_mask);
>>> + unsigned long rflags;
>> Does this really need to be "long"?
>
> The answer to several of these questions are "probably not, but that's
> how load_segments() did it".
Since you overhaul it, no reason to not do sensible adjustments,
I would think.
>>> + unsigned int bytes, missing;
>>> +
>>> + ASSERT_NOT_IN_ATOMIC();
>>> +
>>> + if ( unlikely(null_trap_bounce(curr, tb)) )
>>> + {
>>> + gprintk(XENLOG_ERR, "Fatal: Attempting to inject null trap bounce\n");
>>> + __domain_crash_synchronous();
>> Why not domain_crash() followed by "return"?
>
> Because the existing code uses synchronous crashes.
>
> Looking again at the callsites of pv_create_exception_frame(), we
> immediately jump back to {compat_,}test_all_events, which proceeds to
> run softirqs again.
>
> Therefore, domain_crash() and a return should work. (I think?)
I think so too.
>>> + }
>>> +
>>> + /* Fold the upcall mask and architectural IOPL into the guests rflags. */
>>> + rflags = regs->rflags & ~(X86_EFLAGS_IF | X86_EFLAGS_IOPL);
>>> + rflags |= ((*evt_mask ? 0 : X86_EFLAGS_IF) |
>>> + (VM_ASSIST(curr->domain, architectural_iopl)
>>> + ? curr->arch.pv_vcpu.iopl : 0));
>>> +
>>> + if ( is_pv_32bit_vcpu(curr) )
>>> + {
>>> + /* { [ERRCODE,] EIP, CS/MASK , EFLAGS, [ESP, SS] } */
>>> + unsigned int frame[6], *ptr = frame, ksp =
>>> + (user_mode_frame ? curr->arch.pv_vcpu.kernel_sp : regs->esp);
>>> +
>>> + if ( tb->flags & TBF_EXCEPTION_ERRCODE )
>>> + *ptr++ = tb->error_code;
>>> +
>>> + *ptr++ = regs->eip;
>>> + *ptr++ = regs->cs | (((unsigned int)*evt_mask) << 16);
>> Do you really need the cast here?
>
> Does it promote correctly if the top bit of the mask is set?
It's a uint8_t -> plain int conversion, which is a zero-extension.
>>> + *ptr++ = rflags;
>>> +
>>> + if ( user_mode_frame )
>>> + {
>>> + *ptr++ = regs->esp;
>>> + *ptr++ = regs->ss;
>>> + }
>>> +
>>> + /* Copy the constructed frame to the guest kernel stack. */
>>> + bytes = _p(ptr) - _p(frame);
>>> + ksp -= bytes;
>>> +
>>> + if ( unlikely((missing = __copy_to_user(_p(ksp), frame, bytes)) != 0) )
>> While I don't think we need to be really bothered, it's perhaps still
>> worth noting in a comment that the wrapping behavior here is
>> wrong (and slightly worse than the assembly original), due to
>> (implicit) address arithmetic all being done with 64-bit operands.
>
> Ah - At some point, I had a comment here explaining the lack of an
> __access_ok() check, but it appears to have got lost in a rebase. I
> will try to reinstate it.
Just to avoid misunderstandings - if anything other than a comment,
it ought to be __compat_access_ok().
> The wrapping behaviour around the 4GB => 0 boundary is undefined, and
> different between Intel and AMD (as we discovered with XSA-186). If we
> passing the exception back to the guest we would need to swap #PF for
> #SS (for Intel), or properly wrap around (for AMD).
>
> Would it be ok just to comment this point and leave it as is?
Sure - that's what I did ask for.
>>> + /*
>>> + * Clobber the injection information now it has been completed. Buggy
>>> + * attempts to inject the same event twice will hit the null_trap_bounce()
>>> + * check above.
>>> + */
>>> + *tb = (struct trap_bounce){};
>> Ah, so that prevents tb becoming a pointer to const. I wonder
>> though whether, on a rather hot path, we really want to zap the
>> entire structure here. As I can see the value in satisfying
>> null_trap_bounce(), how about zapping just ->eip / ->cs on the
>> split paths above?
>
> This ends up being two 8-byte writes of zeroes into a cache-hot line; it
> isn't by any means a slow part of this path, whereas the 16bit write to
> clobber just %cs would be.
>
> Irrespective of that, the following patch depends on this clobbering of
> ->flags.
Right, I did see this. Keep it as it is then.
>> Overall, did you compare generated code with the current
>> assembly implementation? That one surely would have had some
>> room for improvement, so the result here at least shouldn't be
>> worse than that.
>
> The final C version (including failsafe, and some error handling the asm
> functions didn't have) is a bit less than twice the size of the asm
> functions in terms of absolute size.
Size alone is no meaningful criteria anyway.
> I haven't done any performance analysis, but I trust the compiler to
> make better code overall (there are definitely pipeline stalls in the
> asm versions), and wouldn't be surprised if it is faster to execute.
That's the hope, but if it was double the number on instructions on
the common path (we can surely leave error paths out of
consideration), I wouldn't be convinced these are faster to execute
than the asm code with a couple of stalls (some of which we surely
could take care of). If this wasn't on some highly critical paths, I
wouldn't be worried, but this code really needs to be fast (besides
the very reasonable desire of making it better maintainable). For
example, at this point I'm not (yet) convinced making this a single
function serving both 64- and 32-bit guests is really the right choice,
even if it's only one or two extra conditionals.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] x86/pv: Drop {compat_, }create_bounce_frame() and use the C version instead
2017-05-09 17:24 ` Andrew Cooper
@ 2017-05-10 7:50 ` Jan Beulich
0 siblings, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2017-05-10 7:50 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, Xen-devel
>>> On 09.05.17 at 19:24, <andrew.cooper3@citrix.com> wrote:
> On 09/05/17 17:16, Jan Beulich wrote:
>>>>> On 08.05.17 at 17:48, <andrew.cooper3@citrix.com> wrote:
>>> @@ -181,8 +181,7 @@ ENTRY(compat_post_handle_exception)
>>> testb $TBF_EXCEPTION,TRAPBOUNCE_flags(%rdx)
>>> jz compat_test_all_events
>>> .Lcompat_bounce_exception:
>>> - call compat_create_bounce_frame
>>> - movb $0,TRAPBOUNCE_flags(%rdx)
>>> + call pv_create_exception_frame
>>> jmp compat_test_all_events
>> Considering this recurring pattern of call/jmp I wonder whether we
>> could reduce the branch tracking structure utilization in the CPU a
>> little by folding these paths.
>
> I think we can lift all of this softirq/event/nmi/mce handling logic up
> into C, which will remove almost all of the guest-handling asm code in
> entry.S
>
> However, it would still retain this "goto again" structure, and will
> eventually have to drop back into asm to actually return to the guest.
>
> The problem (which I haven't got a good solution for yet) is that, if I
> make the C functions noreturn, I will need to duplicate them so they can
> jmp to the proper return point. The alternative would be a moderately
> hard to predict conditional jump.
Quite likely it would be possible to avoid source level duplication,
having the compiler produce two (or more) variants distinguished
mainly by that final jump. Question is whether the extra cache
(and to a lesser degree TLB) pressure wouldn't be worse than a
frequently mis-predicted branch. Another option might be to use
an indirect branch instead, making sure this uses a register
operand, with the involved register loaded well ahead of its use.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-05-10 7:50 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-08 15:48 [PATCH RFC for-next 0/4] Reimpliement {compat_, }create_bounce_frame() in C Andrew Cooper
2017-05-08 15:48 ` [PATCH 1/4] x86/pv: Drop int80_bounce from struct pv_vcpu Andrew Cooper
2017-05-09 14:49 ` Jan Beulich
2017-05-09 15:09 ` Andrew Cooper
2017-05-08 15:48 ` [PATCH 2/4] x86/pv: Introduce pv_create_exception_frame() Andrew Cooper
2017-05-09 15:58 ` Jan Beulich
2017-05-09 17:09 ` Andrew Cooper
2017-05-10 7:43 ` Jan Beulich
2017-05-08 15:48 ` [PATCH 3/4] x86/pv: Drop {compat_, }create_bounce_frame() and use the C version instead Andrew Cooper
2017-05-09 16:16 ` Jan Beulich
2017-05-09 17:24 ` Andrew Cooper
2017-05-10 7:50 ` Jan Beulich
2017-05-08 15:48 ` [PATCH 4/4] x86/pv: Implement the failsafe callback using the general path Andrew Cooper
2017-05-09 16:22 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.