| Message ID | 1473156779-11759-5-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 06.09.16 at 12:12, <andrew.cooper3@citrix.com> wrote: > In a similar style to hvm_do_hypercall(). The C version is far easier to > understand and edit than the assembly versions. > > There are a few small differences however. The register clobbering values > have changed (to match the HVM side), and in particular clobber the upper > 32bits of 64bit arguments. The hypercall and performance counter record are > reordered to increase code sharing between the 32bit and 64bit cases. > > The sole callers of __trace_hypercall_entry() were the assembly code. Given > the new C layout, it is more convenient to fold __trace_hypercall_entry() into > pv_hypercall(), and call __trace_hypercall() directly. > > Finally, pv_hypercall() will treat a NULL hypercall function pointer as > -ENOSYS, allowing further cleanup. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
On Tue, Sep 6, 2016 at 11:12 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > In a similar style to hvm_do_hypercall(). The C version is far easier to > understand and edit than the assembly versions. > > There are a few small differences however. The register clobbering values > have changed (to match the HVM side), and in particular clobber the upper > 32bits of 64bit arguments. The hypercall and performance counter record are > reordered to increase code sharing between the 32bit and 64bit cases. > > The sole callers of __trace_hypercall_entry() were the assembly code. Given > the new C layout, it is more convenient to fold __trace_hypercall_entry() into > pv_hypercall(), and call __trace_hypercall() directly. > > Finally, pv_hypercall() will treat a NULL hypercall function pointer as > -ENOSYS, allowing further cleanup. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > > v2: > * Use guest_mode_kernel() rather than TF_kernel_mode > * Consistent use of 32bit stores > * Don't truncate rax for 64bit domains > * Move eax return assignment into C > --- > xen/arch/x86/hypercall.c | 120 +++++++++++++++++++++++++++++++++++++ > xen/arch/x86/trace.c | 27 --------- > xen/arch/x86/x86_64/asm-offsets.c | 1 - > xen/arch/x86/x86_64/compat/entry.S | 69 +-------------------- > xen/arch/x86/x86_64/entry.S | 61 +------------------ > 5 files changed, 124 insertions(+), 154 deletions(-) > > diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c > index 4b42f86..13a89a0 100644 > --- a/xen/arch/x86/hypercall.c > +++ b/xen/arch/x86/hypercall.c > @@ -17,7 +17,12 @@ > * Copyright (c) 2015,2016 Citrix Systems Ltd. > */ > > +#include <xen/compiler.h> > #include <xen/hypercall.h> > +#include <xen/trace.h> > + > +extern hypercall_fn_t *const hypercall_table[NR_hypercalls], > + *const compat_hypercall_table[NR_hypercalls]; > > #define ARGS(x, n) \ > [ __HYPERVISOR_ ## x ] = (n) > @@ -111,6 +116,121 @@ const uint8_t compat_hypercall_args_table[NR_hypercalls] = > > #undef ARGS > > +void pv_hypercall(struct cpu_user_regs *regs) > +{ > + struct vcpu *curr = current; > +#ifndef NDEBUG > + unsigned long old_rip = regs->rip; > +#endif > + unsigned long eax; > + > + ASSERT(guest_kernel_mode(curr, regs)); > + > + eax = is_pv_32bit_vcpu(curr) ? regs->_eax : regs->eax; > + > + if ( (eax >= NR_hypercalls) || !hypercall_table[eax] ) > + { > + regs->eax = -ENOSYS; > + return; > + } > + > + if ( !is_pv_32bit_vcpu(curr) ) > + { > + unsigned long rdi = regs->rdi; > + unsigned long rsi = regs->rsi; > + unsigned long rdx = regs->rdx; > + unsigned long r10 = regs->r10; > + unsigned long r8 = regs->r8; > + unsigned long r9 = regs->r9; > + > +#ifndef NDEBUG > + /* Deliberately corrupt parameter regs not used by this hypercall. */ > + switch ( hypercall_args_table[eax] ) > + { > + case 0: rdi = 0xdeadbeefdeadf00dUL; > + case 1: rsi = 0xdeadbeefdeadf00dUL; > + case 2: rdx = 0xdeadbeefdeadf00dUL; > + case 3: r10 = 0xdeadbeefdeadf00dUL; > + case 4: r8 = 0xdeadbeefdeadf00dUL; > + case 5: r9 = 0xdeadbeefdeadf00dUL; > + } Out of curiosity, is Coverity going to complain about the lack of /* FALLTHROUGH */ in these stanzas, or is there some magic that lets it know this is intended? (Or does it ignore DEBUG code?) -George
On 06/09/16 18:44, George Dunlap wrote: > On Tue, Sep 6, 2016 at 11:12 AM, Andrew Cooper > <andrew.cooper3@citrix.com> wrote: >> In a similar style to hvm_do_hypercall(). The C version is far easier to >> understand and edit than the assembly versions. >> >> There are a few small differences however. The register clobbering values >> have changed (to match the HVM side), and in particular clobber the upper >> 32bits of 64bit arguments. The hypercall and performance counter record are >> reordered to increase code sharing between the 32bit and 64bit cases. >> >> The sole callers of __trace_hypercall_entry() were the assembly code. Given >> the new C layout, it is more convenient to fold __trace_hypercall_entry() into >> pv_hypercall(), and call __trace_hypercall() directly. >> >> Finally, pv_hypercall() will treat a NULL hypercall function pointer as >> -ENOSYS, allowing further cleanup. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> >> v2: >> * Use guest_mode_kernel() rather than TF_kernel_mode >> * Consistent use of 32bit stores >> * Don't truncate rax for 64bit domains >> * Move eax return assignment into C >> --- >> xen/arch/x86/hypercall.c | 120 +++++++++++++++++++++++++++++++++++++ >> xen/arch/x86/trace.c | 27 --------- >> xen/arch/x86/x86_64/asm-offsets.c | 1 - >> xen/arch/x86/x86_64/compat/entry.S | 69 +-------------------- >> xen/arch/x86/x86_64/entry.S | 61 +------------------ >> 5 files changed, 124 insertions(+), 154 deletions(-) >> >> diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c >> index 4b42f86..13a89a0 100644 >> --- a/xen/arch/x86/hypercall.c >> +++ b/xen/arch/x86/hypercall.c >> @@ -17,7 +17,12 @@ >> * Copyright (c) 2015,2016 Citrix Systems Ltd. >> */ >> >> +#include <xen/compiler.h> >> #include <xen/hypercall.h> >> +#include <xen/trace.h> >> + >> +extern hypercall_fn_t *const hypercall_table[NR_hypercalls], >> + *const compat_hypercall_table[NR_hypercalls]; >> >> #define ARGS(x, n) \ >> [ __HYPERVISOR_ ## x ] = (n) >> @@ -111,6 +116,121 @@ const uint8_t compat_hypercall_args_table[NR_hypercalls] = >> >> #undef ARGS >> >> +void pv_hypercall(struct cpu_user_regs *regs) >> +{ >> + struct vcpu *curr = current; >> +#ifndef NDEBUG >> + unsigned long old_rip = regs->rip; >> +#endif >> + unsigned long eax; >> + >> + ASSERT(guest_kernel_mode(curr, regs)); >> + >> + eax = is_pv_32bit_vcpu(curr) ? regs->_eax : regs->eax; >> + >> + if ( (eax >= NR_hypercalls) || !hypercall_table[eax] ) >> + { >> + regs->eax = -ENOSYS; >> + return; >> + } >> + >> + if ( !is_pv_32bit_vcpu(curr) ) >> + { >> + unsigned long rdi = regs->rdi; >> + unsigned long rsi = regs->rsi; >> + unsigned long rdx = regs->rdx; >> + unsigned long r10 = regs->r10; >> + unsigned long r8 = regs->r8; >> + unsigned long r9 = regs->r9; >> + >> +#ifndef NDEBUG >> + /* Deliberately corrupt parameter regs not used by this hypercall. */ >> + switch ( hypercall_args_table[eax] ) >> + { >> + case 0: rdi = 0xdeadbeefdeadf00dUL; >> + case 1: rsi = 0xdeadbeefdeadf00dUL; >> + case 2: rdx = 0xdeadbeefdeadf00dUL; >> + case 3: r10 = 0xdeadbeefdeadf00dUL; >> + case 4: r8 = 0xdeadbeefdeadf00dUL; >> + case 5: r9 = 0xdeadbeefdeadf00dUL; >> + } > Out of curiosity, is Coverity going to complain about the lack of /* > FALLTHROUGH */ in these stanzas, or is there some magic that lets it > know this is intended? (Or does it ignore DEBUG code?) From the docs: Because there are many reasons why a case intentionally does not end with a break statement, the MISSING_BREAK checker does not report defects in cases that: * Are followed by a case that starts with a break. * End with a comment. The checker assumes that this comment is acknowledging a fallthrough. The comment can start anywhere on the last line, or be a multi-line C comment. * Are empty. * Have no control flow path because, for example, there is a return statement. * Have at least one conditional statement that contains a break statement. * Start and end on the same line. * Have a top-level statement that is a call to a function that can end the program. * Fall through to another case that has a similar numeric value when interpreted as ASCII. Values are considered similar when both are whitespace values (such as space, tab, or newline), or the two values are different cases (uppercase or lowercase) of the same letter. I suspect we are hitting the "start and end on the same line" exclusion. It is worth noting that the HVM code is already like this, and passing fine. ~Andrew
On Tue, Sep 6, 2016 at 6:49 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 06/09/16 18:44, George Dunlap wrote: > > On Tue, Sep 6, 2016 at 11:12 AM, Andrew Cooper > <andrew.cooper3@citrix.com> wrote: > > In a similar style to hvm_do_hypercall(). The C version is far easier to > understand and edit than the assembly versions. > > There are a few small differences however. The register clobbering values > have changed (to match the HVM side), and in particular clobber the upper > 32bits of 64bit arguments. The hypercall and performance counter record are > reordered to increase code sharing between the 32bit and 64bit cases. > > The sole callers of __trace_hypercall_entry() were the assembly code. Given > the new C layout, it is more convenient to fold __trace_hypercall_entry() > into > pv_hypercall(), and call __trace_hypercall() directly. > > Finally, pv_hypercall() will treat a NULL hypercall function pointer as > -ENOSYS, allowing further cleanup. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > > v2: > * Use guest_mode_kernel() rather than TF_kernel_mode > * Consistent use of 32bit stores > * Don't truncate rax for 64bit domains > * Move eax return assignment into C > --- > xen/arch/x86/hypercall.c | 120 > +++++++++++++++++++++++++++++++++++++ > xen/arch/x86/trace.c | 27 --------- > xen/arch/x86/x86_64/asm-offsets.c | 1 - > xen/arch/x86/x86_64/compat/entry.S | 69 +-------------------- > xen/arch/x86/x86_64/entry.S | 61 +------------------ > 5 files changed, 124 insertions(+), 154 deletions(-) > > diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c > index 4b42f86..13a89a0 100644 > --- a/xen/arch/x86/hypercall.c > +++ b/xen/arch/x86/hypercall.c > @@ -17,7 +17,12 @@ > * Copyright (c) 2015,2016 Citrix Systems Ltd. > */ > > +#include <xen/compiler.h> > #include <xen/hypercall.h> > +#include <xen/trace.h> > + > +extern hypercall_fn_t *const hypercall_table[NR_hypercalls], > + *const compat_hypercall_table[NR_hypercalls]; > > #define ARGS(x, n) \ > [ __HYPERVISOR_ ## x ] = (n) > @@ -111,6 +116,121 @@ const uint8_t > compat_hypercall_args_table[NR_hypercalls] = > > #undef ARGS > > +void pv_hypercall(struct cpu_user_regs *regs) > +{ > + struct vcpu *curr = current; > +#ifndef NDEBUG > + unsigned long old_rip = regs->rip; > +#endif > + unsigned long eax; > + > + ASSERT(guest_kernel_mode(curr, regs)); > + > + eax = is_pv_32bit_vcpu(curr) ? regs->_eax : regs->eax; > + > + if ( (eax >= NR_hypercalls) || !hypercall_table[eax] ) > + { > + regs->eax = -ENOSYS; > + return; > + } > + > + if ( !is_pv_32bit_vcpu(curr) ) > + { > + unsigned long rdi = regs->rdi; > + unsigned long rsi = regs->rsi; > + unsigned long rdx = regs->rdx; > + unsigned long r10 = regs->r10; > + unsigned long r8 = regs->r8; > + unsigned long r9 = regs->r9; > + > +#ifndef NDEBUG > + /* Deliberately corrupt parameter regs not used by this hypercall. > */ > + switch ( hypercall_args_table[eax] ) > + { > + case 0: rdi = 0xdeadbeefdeadf00dUL; > + case 1: rsi = 0xdeadbeefdeadf00dUL; > + case 2: rdx = 0xdeadbeefdeadf00dUL; > + case 3: r10 = 0xdeadbeefdeadf00dUL; > + case 4: r8 = 0xdeadbeefdeadf00dUL; > + case 5: r9 = 0xdeadbeefdeadf00dUL; > + } > > Out of curiosity, is Coverity going to complain about the lack of /* > FALLTHROUGH */ in these stanzas, or is there some magic that lets it > know this is intended? (Or does it ignore DEBUG code?) > > From the docs: > > Because there are many reasons why a case intentionally does not end with a > break statement, the MISSING_BREAK checker does not report defects in cases > that: > > Are followed by a case that starts with a break. > > End with a comment. The checker assumes that this comment is acknowledging a > fallthrough. The comment can start anywhere on the last line, or be a > multi-line C comment. > > Are empty. > > Have no control flow path because, for example, there is a return statement. > > Have at least one conditional statement that contains a break statement. > > Start and end on the same line. > > Have a top-level statement that is a call to a function that can end the > program. > > Fall through to another case that has a similar numeric value when > interpreted as ASCII. Values are considered similar when both are whitespace > values (such as space, tab, or newline), or the two values are different > cases (uppercase or lowercase) of the same letter. > > > I suspect we are hitting the "start and end on the same line" exclusion. It > is worth noting that the HVM code is already like this, and passing fine. Cool, thanks. -George
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c index 4b42f86..13a89a0 100644 --- a/xen/arch/x86/hypercall.c +++ b/xen/arch/x86/hypercall.c @@ -17,7 +17,12 @@ * Copyright (c) 2015,2016 Citrix Systems Ltd. */ +#include <xen/compiler.h> #include <xen/hypercall.h> +#include <xen/trace.h> + +extern hypercall_fn_t *const hypercall_table[NR_hypercalls], + *const compat_hypercall_table[NR_hypercalls]; #define ARGS(x, n) \ [ __HYPERVISOR_ ## x ] = (n) @@ -111,6 +116,121 @@ const uint8_t compat_hypercall_args_table[NR_hypercalls] = #undef ARGS +void pv_hypercall(struct cpu_user_regs *regs) +{ + struct vcpu *curr = current; +#ifndef NDEBUG + unsigned long old_rip = regs->rip; +#endif + unsigned long eax; + + ASSERT(guest_kernel_mode(curr, regs)); + + eax = is_pv_32bit_vcpu(curr) ? regs->_eax : regs->eax; + + if ( (eax >= NR_hypercalls) || !hypercall_table[eax] ) + { + regs->eax = -ENOSYS; + return; + } + + if ( !is_pv_32bit_vcpu(curr) ) + { + unsigned long rdi = regs->rdi; + unsigned long rsi = regs->rsi; + unsigned long rdx = regs->rdx; + unsigned long r10 = regs->r10; + unsigned long r8 = regs->r8; + unsigned long r9 = regs->r9; + +#ifndef NDEBUG + /* Deliberately corrupt parameter regs not used by this hypercall. */ + switch ( hypercall_args_table[eax] ) + { + case 0: rdi = 0xdeadbeefdeadf00dUL; + case 1: rsi = 0xdeadbeefdeadf00dUL; + case 2: rdx = 0xdeadbeefdeadf00dUL; + case 3: r10 = 0xdeadbeefdeadf00dUL; + case 4: r8 = 0xdeadbeefdeadf00dUL; + case 5: r9 = 0xdeadbeefdeadf00dUL; + } +#endif + if ( unlikely(tb_init_done) ) + { + unsigned long args[6] = { rdi, rsi, rdx, r10, r8, r9 }; + + __trace_hypercall(TRC_PV_HYPERCALL_V2, eax, args); + } + + regs->eax = hypercall_table[eax](rdi, rsi, rdx, r10, r8, r9); + +#ifndef NDEBUG + if ( regs->rip == old_rip ) + { + /* Deliberately corrupt parameter regs used by this hypercall. */ + switch ( hypercall_args_table[eax] ) + { + case 6: regs->r9 = 0xdeadbeefdeadf00dUL; + case 5: regs->r8 = 0xdeadbeefdeadf00dUL; + case 4: regs->r10 = 0xdeadbeefdeadf00dUL; + case 3: regs->rdx = 0xdeadbeefdeadf00dUL; + case 2: regs->rsi = 0xdeadbeefdeadf00dUL; + case 1: regs->rdi = 0xdeadbeefdeadf00dUL; + } + } +#endif + } + else + { + unsigned int ebx = regs->_ebx; + unsigned int ecx = regs->_ecx; + unsigned int edx = regs->_edx; + unsigned int esi = regs->_esi; + unsigned int edi = regs->_edi; + unsigned int ebp = regs->_ebp; + +#ifndef NDEBUG + /* Deliberately corrupt parameter regs not used by this hypercall. */ + switch ( compat_hypercall_args_table[eax] ) + { + case 0: ebx = 0xdeadf00d; + case 1: ecx = 0xdeadf00d; + case 2: edx = 0xdeadf00d; + case 3: esi = 0xdeadf00d; + case 4: edi = 0xdeadf00d; + case 5: ebp = 0xdeadf00d; + } +#endif + + if ( unlikely(tb_init_done) ) + { + unsigned long args[6] = { ebx, ecx, edx, esi, edi, ebp }; + + __trace_hypercall(TRC_PV_HYPERCALL_V2, eax, args); + } + + regs->_eax = compat_hypercall_table[eax](ebx, ecx, edx, esi, edi, ebp); + +#ifndef NDEBUG + if ( regs->rip == old_rip ) + { + /* Deliberately corrupt parameter regs used by this hypercall. */ + switch ( compat_hypercall_args_table[eax] ) + { + case 6: regs->_ebp = 0xdeadf00d; + case 5: regs->_edi = 0xdeadf00d; + case 4: regs->_esi = 0xdeadf00d; + case 3: regs->_edx = 0xdeadf00d; + case 2: regs->_ecx = 0xdeadf00d; + case 1: regs->_ebx = 0xdeadf00d; + } + } +#endif + } + + perfc_incr(hypercalls); +} + /* * Local variables: * mode: C diff --git a/xen/arch/x86/trace.c b/xen/arch/x86/trace.c index bd8596c..f11b309 100644 --- a/xen/arch/x86/trace.c +++ b/xen/arch/x86/trace.c @@ -6,33 +6,6 @@ #include <xen/sched.h> #include <xen/trace.h> -void __trace_hypercall_entry(void) -{ - struct cpu_user_regs *regs = guest_cpu_user_regs(); - unsigned long args[6]; - - if ( is_pv_32bit_vcpu(current) ) - { - args[0] = regs->ebx; - args[1] = regs->ecx; - args[2] = regs->edx; - args[3] = regs->esi; - args[4] = regs->edi; - args[5] = regs->ebp; - } - else - { - args[0] = regs->rdi; - args[1] = regs->rsi; - args[2] = regs->rdx; - args[3] = regs->r10; - args[4] = regs->r8; - args[5] = regs->r9; - } - - __trace_hypercall(TRC_PV_HYPERCALL_V2, regs->eax, args); -} - void __trace_pv_trap(int trapnr, unsigned long eip, int use_error_code, unsigned error_code) { diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c index 05d2b85..64905c6 100644 --- a/xen/arch/x86/x86_64/asm-offsets.c +++ b/xen/arch/x86/x86_64/asm-offsets.c @@ -153,7 +153,6 @@ void __dummy__(void) BLANK(); #ifdef CONFIG_PERF_COUNTERS - DEFINE(ASM_PERFC_hypercalls, PERFC_hypercalls); DEFINE(ASM_PERFC_exceptions, PERFC_exceptions); BLANK(); #endif diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S index f0eae56..e3cc10d 100644 --- a/xen/arch/x86/x86_64/compat/entry.S +++ b/xen/arch/x86/x86_64/compat/entry.S @@ -25,71 +25,10 @@ UNLIKELY_START(ne, msi_check) LOAD_C_CLOBBERED compat=1 ax=0 UNLIKELY_END(msi_check) - movl UREGS_rax(%rsp),%eax GET_CURRENT(bx) - cmpl $NR_hypercalls,%eax - jae compat_bad_hypercall -#ifndef NDEBUG - /* Deliberately corrupt parameter regs not used by this hypercall. */ - pushq UREGS_rbx(%rsp); pushq %rcx; pushq %rdx; pushq %rsi; pushq %rdi - pushq UREGS_rbp+5*8(%rsp) - leaq compat_hypercall_args_table(%rip),%r10 - movl $6,%ecx - subb (%r10,%rax,1),%cl - movq %rsp,%rdi - movl $0xDEADBEEF,%eax - rep stosq - popq %r8 ; popq %r9 ; xchgl %r8d,%r9d /* Args 5&6: zero extend */ - popq %rdx; popq %rcx; xchgl %edx,%ecx /* Args 3&4: zero extend */ - popq %rdi; popq %rsi; xchgl %edi,%esi /* Args 1&2: zero extend */ - movl UREGS_rax(%rsp),%eax - pushq %rax - pushq UREGS_rip+8(%rsp) -#define SHADOW_BYTES 16 /* Shadow EIP + shadow hypercall # */ -#else - /* Relocate argument registers and zero-extend to 64 bits. */ - xchgl %ecx,%esi /* Arg 2, Arg 4 */ - movl %edx,%edx /* Arg 3 */ - movl %edi,%r8d /* Arg 5 */ - movl %ebp,%r9d /* Arg 6 */ - movl UREGS_rbx(%rsp),%edi /* Arg 1 */ -#define SHADOW_BYTES 0 /* No on-stack shadow state */ -#endif - cmpb $0,tb_init_done(%rip) -UNLIKELY_START(ne, compat_trace) - call __trace_hypercall_entry - /* Restore the registers that __trace_hypercall_entry clobbered. */ - movl UREGS_rax+SHADOW_BYTES(%rsp),%eax /* Hypercall # */ - movl UREGS_rbx+SHADOW_BYTES(%rsp),%edi /* Arg 1 */ - movl UREGS_rcx+SHADOW_BYTES(%rsp),%esi /* Arg 2 */ - movl UREGS_rdx+SHADOW_BYTES(%rsp),%edx /* Arg 3 */ - movl UREGS_rsi+SHADOW_BYTES(%rsp),%ecx /* Arg 4 */ - movl UREGS_rdi+SHADOW_BYTES(%rsp),%r8d /* Arg 5 */ - movl UREGS_rbp+SHADOW_BYTES(%rsp),%r9d /* Arg 6 */ -#undef SHADOW_BYTES -UNLIKELY_END(compat_trace) - leaq compat_hypercall_table(%rip),%r10 - PERFC_INCR(hypercalls, %rax, %rbx) - callq *(%r10,%rax,8) -#ifndef NDEBUG - /* Deliberately corrupt parameter regs used by this hypercall. */ - popq %r10 # Shadow RIP - cmpq %r10,UREGS_rip+8(%rsp) - popq %rcx # Shadow hypercall index - jne compat_skip_clobber /* If RIP has changed then don't clobber. */ - leaq compat_hypercall_args_table(%rip),%r10 - movb (%r10,%rcx,1),%cl - movl $0xDEADBEEF,%r10d - testb %cl,%cl; jz compat_skip_clobber; movl %r10d,UREGS_rbx(%rsp) - cmpb $2, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rcx(%rsp) - cmpb $3, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rdx(%rsp) - cmpb $4, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rsi(%rsp) - cmpb $5, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rdi(%rsp) - cmpb $6, %cl; jb compat_skip_clobber; movl %r10d,UREGS_rbp(%rsp) -compat_skip_clobber: -#endif - movl %eax,UREGS_rax(%rsp) # save the return value + mov %rsp, %rdi + call pv_hypercall /* %rbx: struct vcpu */ ENTRY(compat_test_all_events) @@ -167,10 +106,6 @@ compat_process_trap: call compat_create_bounce_frame jmp compat_test_all_events -compat_bad_hypercall: - movl $-ENOSYS,UREGS_rax(%rsp) - jmp compat_test_all_events - /* %rbx: struct vcpu, interrupts disabled */ ENTRY(compat_restore_all_guest) ASSERT_INTERRUPTS_DISABLED diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S index 086fcec..7b25f6b 100644 --- a/xen/arch/x86/x86_64/entry.S +++ b/xen/arch/x86/x86_64/entry.S @@ -101,61 +101,8 @@ ENTRY(lstar_enter) testb $TF_kernel_mode,VCPU_thread_flags(%rbx) jz switch_to_kernel -/*hypercall:*/ - movq %r10,%rcx - cmpq $NR_hypercalls,%rax - jae bad_hypercall -#ifndef NDEBUG - /* Deliberately corrupt parameter regs not used by this hypercall. */ - pushq %rdi; pushq %rsi; pushq %rdx; pushq %rcx; pushq %r8 ; pushq %r9 - leaq hypercall_args_table(%rip),%r10 - movq $6,%rcx - sub (%r10,%rax,1),%cl - movq %rsp,%rdi - movl $0xDEADBEEF,%eax - rep stosq - popq %r9 ; popq %r8 ; popq %rcx; popq %rdx; popq %rsi; popq %rdi - movq UREGS_rax(%rsp),%rax - pushq %rax - pushq UREGS_rip+8(%rsp) -#define SHADOW_BYTES 16 /* Shadow EIP + shadow hypercall # */ -#else -#define SHADOW_BYTES 0 /* No on-stack shadow state */ -#endif - cmpb $0,tb_init_done(%rip) -UNLIKELY_START(ne, trace) - call __trace_hypercall_entry - /* Restore the registers that __trace_hypercall_entry clobbered. */ - movq UREGS_rax+SHADOW_BYTES(%rsp),%rax /* Hypercall # */ - movq UREGS_rdi+SHADOW_BYTES(%rsp),%rdi /* Arg 1 */ - movq UREGS_rsi+SHADOW_BYTES(%rsp),%rsi /* Arg 2 */ - movq UREGS_rdx+SHADOW_BYTES(%rsp),%rdx /* Arg 3 */ - movq UREGS_r10+SHADOW_BYTES(%rsp),%rcx /* Arg 4 */ - movq UREGS_r8 +SHADOW_BYTES(%rsp),%r8 /* Arg 5 */ - movq UREGS_r9 +SHADOW_BYTES(%rsp),%r9 /* Arg 6 */ -#undef SHADOW_BYTES -UNLIKELY_END(trace) - leaq hypercall_table(%rip),%r10 - PERFC_INCR(hypercalls, %rax, %rbx) - callq *(%r10,%rax,8) -#ifndef NDEBUG - /* Deliberately corrupt parameter regs used by this hypercall. */ - popq %r10 # Shadow RIP - cmpq %r10,UREGS_rip+8(%rsp) - popq %rcx # Shadow hypercall index - jne skip_clobber /* If RIP has changed then don't clobber. */ - leaq hypercall_args_table(%rip),%r10 - movb (%r10,%rcx,1),%cl - movl $0xDEADBEEF,%r10d - cmpb $1,%cl; jb skip_clobber; movq %r10,UREGS_rdi(%rsp) - cmpb $2,%cl; jb skip_clobber; movq %r10,UREGS_rsi(%rsp) - cmpb $3,%cl; jb skip_clobber; movq %r10,UREGS_rdx(%rsp) - cmpb $4,%cl; jb skip_clobber; movq %r10,UREGS_r10(%rsp) - cmpb $5,%cl; jb skip_clobber; movq %r10,UREGS_r8(%rsp) - cmpb $6,%cl; jb skip_clobber; movq %r10,UREGS_r9(%rsp) -skip_clobber: -#endif - movq %rax,UREGS_rax(%rsp) # save the return value + mov %rsp, %rdi + call pv_hypercall /* %rbx: struct vcpu */ test_all_events: @@ -231,10 +178,6 @@ process_trap: call create_bounce_frame jmp test_all_events -bad_hypercall: - movq $-ENOSYS,UREGS_rax(%rsp) - jmp test_all_events - ENTRY(sysenter_entry) sti pushq $FLAT_USER_SS
In a similar style to hvm_do_hypercall(). The C version is far easier to understand and edit than the assembly versions. There are a few small differences however. The register clobbering values have changed (to match the HVM side), and in particular clobber the upper 32bits of 64bit arguments. The hypercall and performance counter record are reordered to increase code sharing between the 32bit and 64bit cases. The sole callers of __trace_hypercall_entry() were the assembly code. Given the new C layout, it is more convenient to fold __trace_hypercall_entry() into pv_hypercall(), and call __trace_hypercall() directly. Finally, pv_hypercall() will treat a NULL hypercall function pointer as -ENOSYS, allowing further cleanup. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> v2: * Use guest_mode_kernel() rather than TF_kernel_mode * Consistent use of 32bit stores * Don't truncate rax for 64bit domains * Move eax return assignment into C --- xen/arch/x86/hypercall.c | 120 +++++++++++++++++++++++++++++++++++++ xen/arch/x86/trace.c | 27 --------- xen/arch/x86/x86_64/asm-offsets.c | 1 - xen/arch/x86/x86_64/compat/entry.S | 69 +-------------------- xen/arch/x86/x86_64/entry.S | 61 +------------------ 5 files changed, 124 insertions(+), 154 deletions(-)