| Message ID | 20191204094335.24603-5-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/svm: (Post TASK_SWITCH) cleanup | expand |
On 04.12.2019 10:43, Andrew Cooper wrote: > Introduce vmcb.e1.* and vmcb.e2.* to provide names to fields in exitinfo{1,2} > respectively. Implement the task switch names for now, and clean up the > TASK_SWITCH handler. "e1" and "e2" look overly short - and hence possibly ambiguous - to me. Make them perhaps "ei1" and "ei2"? Furthermore, seeing ... > @@ -2795,19 +2792,12 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) > if ( insn_len < 0 && (insn_len = svm_get_task_switch_insn_len()) == 0 ) > goto crash_or_fault; > > - if ( (vmcb->exitinfo2 >> 36) & 1 ) > - reason = TSW_iret; > - else if ( (vmcb->exitinfo2 >> 38) & 1 ) > - reason = TSW_jmp; > - else > - reason = TSW_call_or_int; > - if ( (vmcb->exitinfo2 >> 44) & 1 ) > - errcode = (uint32_t)vmcb->exitinfo2; > - > - hvm_task_switch(vmcb->exitinfo1, reason, errcode, insn_len, > - (vmcb->exitinfo2 & (1ul << 48)) ? X86_EFLAGS_RF : 0); > + hvm_task_switch(vmcb->e1.task_switch.sel, > + vmcb->e2.task_switch.iret ? TSW_iret : > + vmcb->e2.task_switch.jmp ? TSW_jmp : TSW_call_or_int, > + vmcb->e2.task_switch.ev ? vmcb->e2.task_switch.ec : -1, > + insn_len, vmcb->e2.task_switch.rf ? X86_EFLAGS_RF : 0); ... this, wouldn't it make sense to simply have "ei" covering both parts, no longer making it a requirement to use (and hence look up) the numeric suffixes at use sites? > --- a/xen/include/asm-x86/hvm/svm/vmcb.h > +++ b/xen/include/asm-x86/hvm/svm/vmcb.h > @@ -418,8 +418,30 @@ struct vmcb_struct { > vintr_t _vintr; /* offset 0x60 - cleanbit 3 */ > u64 interrupt_shadow; /* offset 0x68 */ > u64 exitcode; /* offset 0x70 */ > - u64 exitinfo1; /* offset 0x78 */ > - u64 exitinfo2; /* offset 0x80 */ > + union { > + u64 exitinfo1; /* offset 0x78 */ uint64_t (also below)? Jan
On 04/12/2019 10:24, Jan Beulich wrote: > On 04.12.2019 10:43, Andrew Cooper wrote: >> Introduce vmcb.e1.* and vmcb.e2.* to provide names to fields in exitinfo{1,2} >> respectively. Implement the task switch names for now, and clean up the >> TASK_SWITCH handler. > "e1" and "e2" look overly short - and hence possibly ambiguous - > to me. Make them perhaps "ei1" and "ei2"? Written on their own like that perhaps, but the ei[12] versions are equally ambiguous. However, they are only ever used with vmcb-> in code, so there is no issue with ambiguity. > Furthermore, seeing ... > >> @@ -2795,19 +2792,12 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) >> if ( insn_len < 0 && (insn_len = svm_get_task_switch_insn_len()) == 0 ) >> goto crash_or_fault; >> >> - if ( (vmcb->exitinfo2 >> 36) & 1 ) >> - reason = TSW_iret; >> - else if ( (vmcb->exitinfo2 >> 38) & 1 ) >> - reason = TSW_jmp; >> - else >> - reason = TSW_call_or_int; >> - if ( (vmcb->exitinfo2 >> 44) & 1 ) >> - errcode = (uint32_t)vmcb->exitinfo2; >> - >> - hvm_task_switch(vmcb->exitinfo1, reason, errcode, insn_len, >> - (vmcb->exitinfo2 & (1ul << 48)) ? X86_EFLAGS_RF : 0); >> + hvm_task_switch(vmcb->e1.task_switch.sel, >> + vmcb->e2.task_switch.iret ? TSW_iret : >> + vmcb->e2.task_switch.jmp ? TSW_jmp : TSW_call_or_int, >> + vmcb->e2.task_switch.ev ? vmcb->e2.task_switch.ec : -1, >> + insn_len, vmcb->e2.task_switch.rf ? X86_EFLAGS_RF : 0); > ... this, wouldn't it make sense to simply have "ei" covering both > parts, no longer making it a requirement to use (and hence look up) > the numeric suffixes at use sites? Net delta is: diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h b/xen/include/asm-x86/hvm/svm/vmcb.h index 02b5e86b49..864618ccf9 100644 --- a/xen/include/asm-x86/hvm/svm/vmcb.h +++ b/xen/include/asm-x86/hvm/svm/vmcb.h @@ -419,17 +419,15 @@ struct vmcb_struct { u64 interrupt_shadow; /* offset 0x68 */ u64 exitcode; /* offset 0x70 */ union { - u64 exitinfo1; /* offset 0x78 */ + struct { + uint64_t exitinfo1; /* offset 0x78 */ + uint64_t exitinfo2; /* offset 0x80 */ + }; union { struct { uint16_t sel; - } task_switch; - } e1; - }; - union { - u64 exitinfo2; /* offset 0x80 */ - union { - struct { + uint64_t :48; + uint32_t ec; uint32_t :4; bool iret:1; @@ -440,7 +438,7 @@ struct vmcb_struct { uint32_t :3; bool rf:1; } task_switch; - } e2; + } ei; }; intinfo_t exitintinfo; /* offset 0x88 */ u64 _np_enable; /* offset 0x90 - cleanbit 4 */ LGTM. ~Andrew
On 04.12.2019 21:04, Andrew Cooper wrote: > Net delta is: > > diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h > b/xen/include/asm-x86/hvm/svm/vmcb.h > index 02b5e86b49..864618ccf9 100644 > --- a/xen/include/asm-x86/hvm/svm/vmcb.h > +++ b/xen/include/asm-x86/hvm/svm/vmcb.h > @@ -419,17 +419,15 @@ struct vmcb_struct { > u64 interrupt_shadow; /* offset 0x68 */ > u64 exitcode; /* offset 0x70 */ > union { > - u64 exitinfo1; /* offset 0x78 */ > + struct { > + uint64_t exitinfo1; /* offset 0x78 */ > + uint64_t exitinfo2; /* offset 0x80 */ > + }; > union { > struct { > uint16_t sel; > - } task_switch; > - } e1; > - }; > - union { > - u64 exitinfo2; /* offset 0x80 */ > - union { > - struct { > + uint64_t :48; > + > uint32_t ec; > uint32_t :4; > bool iret:1; > @@ -440,7 +438,7 @@ struct vmcb_struct { > uint32_t :3; > bool rf:1; > } task_switch; > - } e2; > + } ei; > }; > intinfo_t exitintinfo; /* offset 0x88 */ > u64 _np_enable; /* offset 0x90 - cleanbit 4 */ > > LGTM. And the result then Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index 263ae03bfd..6c68bcee59 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -2746,10 +2746,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) svm_vmexit_do_invalidate_cache(regs, exit_reason == VMEXIT_INVD); break; - case VMEXIT_TASK_SWITCH: { - enum hvm_task_switch_reason reason; - int32_t errcode = -1; - + case VMEXIT_TASK_SWITCH: /* * All TASK_SWITCH intercepts have fault-like semantics. NRIP is * never provided, even for instruction-induced task switches, but we @@ -2795,19 +2792,12 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) if ( insn_len < 0 && (insn_len = svm_get_task_switch_insn_len()) == 0 ) goto crash_or_fault; - if ( (vmcb->exitinfo2 >> 36) & 1 ) - reason = TSW_iret; - else if ( (vmcb->exitinfo2 >> 38) & 1 ) - reason = TSW_jmp; - else - reason = TSW_call_or_int; - if ( (vmcb->exitinfo2 >> 44) & 1 ) - errcode = (uint32_t)vmcb->exitinfo2; - - hvm_task_switch(vmcb->exitinfo1, reason, errcode, insn_len, - (vmcb->exitinfo2 & (1ul << 48)) ? X86_EFLAGS_RF : 0); + hvm_task_switch(vmcb->e1.task_switch.sel, + vmcb->e2.task_switch.iret ? TSW_iret : + vmcb->e2.task_switch.jmp ? TSW_jmp : TSW_call_or_int, + vmcb->e2.task_switch.ev ? vmcb->e2.task_switch.ec : -1, + insn_len, vmcb->e2.task_switch.rf ? X86_EFLAGS_RF : 0); break; - } case VMEXIT_CPUID: if ( (insn_len = svm_get_insn_len(v, INSTR_CPUID)) == 0 ) diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h b/xen/include/asm-x86/hvm/svm/vmcb.h index fc67a88660..02b5e86b49 100644 --- a/xen/include/asm-x86/hvm/svm/vmcb.h +++ b/xen/include/asm-x86/hvm/svm/vmcb.h @@ -418,8 +418,30 @@ struct vmcb_struct { vintr_t _vintr; /* offset 0x60 - cleanbit 3 */ u64 interrupt_shadow; /* offset 0x68 */ u64 exitcode; /* offset 0x70 */ - u64 exitinfo1; /* offset 0x78 */ - u64 exitinfo2; /* offset 0x80 */ + union { + u64 exitinfo1; /* offset 0x78 */ + union { + struct { + uint16_t sel; + } task_switch; + } e1; + }; + union { + u64 exitinfo2; /* offset 0x80 */ + union { + struct { + uint32_t ec; + uint32_t :4; + bool iret:1; + uint32_t :1; + bool jmp:1; + uint32_t :5; + bool ev:1; + uint32_t :3; + bool rf:1; + } task_switch; + } e2; + }; intinfo_t exitintinfo; /* offset 0x88 */ u64 _np_enable; /* offset 0x90 - cleanbit 4 */ u64 res08[2];
Introduce vmcb.e1.* and vmcb.e2.* to provide names to fields in exitinfo{1,2} respectively. Implement the task switch names for now, and clean up the TASK_SWITCH handler. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/hvm/svm/svm.c | 22 ++++++---------------- xen/include/asm-x86/hvm/svm/vmcb.h | 26 ++++++++++++++++++++++++-- 2 files changed, 30 insertions(+), 18 deletions(-)