* [PATCH v2] hvm/svm: Implement Debug events @ 2018-03-20 9:40 Alexandru Isaila 2018-03-20 15:53 ` Andrew Cooper 2018-03-20 16:49 ` Tamas K Lengyel 0 siblings, 2 replies; 10+ messages in thread From: Alexandru Isaila @ 2018-03-20 9:40 UTC (permalink / raw) To: xen-devel Cc: tamas, suravee.suthikulpanit, rcojocaru, andrew.cooper3, jbeulich, Alexandru Isaila, boris.ostrovsky At this moment the Debug events for the AMD architecture are not forwarded to the monitor layer. This patch adds the Debug event to the common capabilities, adds the VMEXIT_ICEBP then forwards the event to the monitor layer. Chapter 2: SVM Processor and Platform Extensions: "Note: A vector 1 exception generated by the single byte INT1 instruction (also known as ICEBP) does not trigger the #DB intercept. Software should use the dedicated ICEBP intercept to intercept ICEBP" --- Changes since V1: - Get inst_len from __get_instruction_length() - Updated __get_instruction_length() for the INSTR_ICEBP instruction Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> --- xen/arch/x86/hvm/svm/emulate.c | 1 + xen/arch/x86/hvm/svm/svm.c | 37 +++++++++++++++++++++++++---------- xen/arch/x86/hvm/svm/vmcb.c | 2 +- xen/include/asm-x86/hvm/svm/emulate.h | 1 + xen/include/asm-x86/monitor.h | 4 ++-- 5 files changed, 32 insertions(+), 13 deletions(-) diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c index e1a1581..172369e 100644 --- a/xen/arch/x86/hvm/svm/emulate.c +++ b/xen/arch/x86/hvm/svm/emulate.c @@ -80,6 +80,7 @@ static const struct { [INSTR_RDTSC] = { X86EMUL_OPC(0x0f, 0x31) }, [INSTR_RDMSR] = { X86EMUL_OPC(0x0f, 0x32) }, [INSTR_CPUID] = { X86EMUL_OPC(0x0f, 0xa2) }, + [INSTR_ICEBP] = { X86EMUL_OPC( 0, 0xf1) }, }; int __get_instruction_length_from_list(struct vcpu *v, diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index c34f5b5..d4f2290 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -1109,7 +1109,8 @@ static void noreturn svm_do_resume(struct vcpu *v) { struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; bool debug_state = (v->domain->debugger_attached || - v->domain->arch.monitor.software_breakpoint_enabled); + v->domain->arch.monitor.software_breakpoint_enabled || + v->domain->arch.monitor.debug_exception_enabled); bool_t vcpu_guestmode = 0; struct vlapic *vlapic = vcpu_vlapic(v); @@ -2438,16 +2439,15 @@ static bool svm_get_pending_event(struct vcpu *v, struct x86_event *info) return true; } -static void svm_propagate_intr(struct vcpu *v, unsigned long insn_len) +static void svm_propagate_intr(unsigned long insn_len, int16_t vector, uint8_t type) { - struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; struct x86_event event = { - .vector = vmcb->eventinj.fields.type, - .type = vmcb->eventinj.fields.type, - .error_code = vmcb->exitinfo1, + .vector = vector, + .type = type, + .error_code = X86_EVENT_NO_EC, + .insn_len = insn_len, }; - event.insn_len = insn_len; hvm_inject_event(&event); } @@ -2655,10 +2655,27 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) /* Asynchronous event, handled when we STGI'd after the VMEXIT. */ HVMTRACE_0D(SMI); break; - + case VMEXIT_ICEBP: case VMEXIT_EXCEPTION_DB: if ( !v->domain->debugger_attached ) - hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); + { + int rc; + unsigned long trap_type = exit_reason == VMEXIT_ICEBP ? + X86_EVENTTYPE_PRI_SW_EXCEPTION : X86_EVENTTYPE_HW_EXCEPTION; + + inst_len = 0; + + if ( trap_type >= X86_EVENTTYPE_SW_INTERRUPT ) + inst_len = __get_instruction_length(v, INSTR_ICEBP); + + rc = hvm_monitor_debug(regs->rip, + HVM_MONITOR_DEBUG_EXCEPTION, + trap_type, inst_len); + if ( rc < 0 ) + goto unexpected_exit_type; + if ( !rc ) + svm_propagate_intr(inst_len, TRAP_debug, trap_type); + } else domain_pause_for_debugger(); break; @@ -2687,7 +2704,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) if ( rc < 0 ) goto unexpected_exit_type; if ( !rc ) - svm_propagate_intr(v, inst_len); + svm_propagate_intr(inst_len, TRAP_int3, X86_EVENTTYPE_SW_EXCEPTION); } break; diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c index ae60d8d..06920d3 100644 --- a/xen/arch/x86/hvm/svm/vmcb.c +++ b/xen/arch/x86/hvm/svm/vmcb.c @@ -73,7 +73,7 @@ static int construct_vmcb(struct vcpu *v) GENERAL2_INTERCEPT_STGI | GENERAL2_INTERCEPT_CLGI | GENERAL2_INTERCEPT_SKINIT | GENERAL2_INTERCEPT_MWAIT | GENERAL2_INTERCEPT_WBINVD | GENERAL2_INTERCEPT_MONITOR | - GENERAL2_INTERCEPT_XSETBV; + GENERAL2_INTERCEPT_XSETBV | GENERAL2_INTERCEPT_ICEBP; /* Intercept all debug-register writes. */ vmcb->_dr_intercepts = ~0u; diff --git a/xen/include/asm-x86/hvm/svm/emulate.h b/xen/include/asm-x86/hvm/svm/emulate.h index 7c1dcd1..3de8236 100644 --- a/xen/include/asm-x86/hvm/svm/emulate.h +++ b/xen/include/asm-x86/hvm/svm/emulate.h @@ -38,6 +38,7 @@ enum instruction_index { INSTR_STGI, INSTR_CLGI, INSTR_INVLPGA, + INSTR_ICEBP, INSTR_MAX_COUNT /* Must be last - Number of instructions supported */ }; diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h index 99ed4b87..c5a86d1 100644 --- a/xen/include/asm-x86/monitor.h +++ b/xen/include/asm-x86/monitor.h @@ -82,12 +82,12 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d) (1U << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) | (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT) | (1U << XEN_DOMCTL_MONITOR_EVENT_CPUID) | + (1U << XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION) | (1U << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG)); if ( cpu_has_vmx ) { - capabilities |= ((1U << XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION) | - (1U << XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED)); + capabilities |= (1U << XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED); /* Since we know this is on VMX, we can just call the hvm func */ if ( hvm_is_singlestep_supported() ) -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] hvm/svm: Implement Debug events 2018-03-20 9:40 [PATCH v2] hvm/svm: Implement Debug events Alexandru Isaila @ 2018-03-20 15:53 ` Andrew Cooper 2018-03-20 16:05 ` Jan Beulich 2018-03-20 16:49 ` Tamas K Lengyel 1 sibling, 1 reply; 10+ messages in thread From: Andrew Cooper @ 2018-03-20 15:53 UTC (permalink / raw) To: Alexandru Isaila, xen-devel Cc: boris.ostrovsky, tamas, jbeulich, suravee.suthikulpanit, rcojocaru On 20/03/18 09:40, Alexandru Isaila wrote: > At this moment the Debug events for the AMD architecture are not > forwarded to the monitor layer. > > This patch adds the Debug event to the common capabilities, adds > the VMEXIT_ICEBP then forwards the event to the monitor layer. > > Chapter 2: SVM Processor and Platform Extensions: "Note: A vector 1 > exception generated by the single byte INT1 > instruction (also known as ICEBP) does not trigger the #DB > intercept. Software should use the dedicated ICEBP > intercept to intercept ICEBP" > > --- > Changes since V1: > - Get inst_len from __get_instruction_length() > - Updated __get_instruction_length() for the INSTR_ICEBP > instruction > > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > --- > xen/arch/x86/hvm/svm/emulate.c | 1 + > xen/arch/x86/hvm/svm/svm.c | 37 +++++++++++++++++++++++++---------- > xen/arch/x86/hvm/svm/vmcb.c | 2 +- > xen/include/asm-x86/hvm/svm/emulate.h | 1 + > xen/include/asm-x86/monitor.h | 4 ++-- > 5 files changed, 32 insertions(+), 13 deletions(-) > > diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c > index e1a1581..172369e 100644 > --- a/xen/arch/x86/hvm/svm/emulate.c > +++ b/xen/arch/x86/hvm/svm/emulate.c > @@ -80,6 +80,7 @@ static const struct { > [INSTR_RDTSC] = { X86EMUL_OPC(0x0f, 0x31) }, > [INSTR_RDMSR] = { X86EMUL_OPC(0x0f, 0x32) }, > [INSTR_CPUID] = { X86EMUL_OPC(0x0f, 0xa2) }, > + [INSTR_ICEBP] = { X86EMUL_OPC( 0, 0xf1) }, This list is currently sorted by opcode. The new addition should be between INT3 and HLT. > }; > > int __get_instruction_length_from_list(struct vcpu *v, > diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c > index c34f5b5..d4f2290 100644 > --- a/xen/arch/x86/hvm/svm/svm.c > +++ b/xen/arch/x86/hvm/svm/svm.c > @@ -1109,7 +1109,8 @@ static void noreturn svm_do_resume(struct vcpu *v) > { > struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; > bool debug_state = (v->domain->debugger_attached || > - v->domain->arch.monitor.software_breakpoint_enabled); > + v->domain->arch.monitor.software_breakpoint_enabled || > + v->domain->arch.monitor.debug_exception_enabled); > bool_t vcpu_guestmode = 0; > struct vlapic *vlapic = vcpu_vlapic(v); > > @@ -2438,16 +2439,15 @@ static bool svm_get_pending_event(struct vcpu *v, struct x86_event *info) > return true; > } > > -static void svm_propagate_intr(struct vcpu *v, unsigned long insn_len) > +static void svm_propagate_intr(unsigned long insn_len, int16_t vector, uint8_t type) Hmm - not sure where the old unsigned long came from, but it isn't really correct. Also, as this function no longer propagates the contents of the vmcb, it is now mis-named. Please could you delete this function and use: diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h index 2376ed6..843dafe 100644 --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -407,6 +407,19 @@ void hvm_migrate_pirqs(struct vcpu *v); void hvm_inject_event(const struct x86_event *event); +static inline void hvm_inject_exception( + unsigned int vector, unsigned int type, unsigned int insn_len) +{ + struct x86_event event = { + .vector = vector, + .type = type, + .insn_len = insn_len, + .error_code = X86_EVENT_NO_EC, + }; + + hvm_inject_event(&event); +} + static inline void hvm_inject_hw_exception(unsigned int vector, int errcode) { struct x86_event event = { as a new common helper. (I'm not terribly happy with the name, but I can't think of a better alternative, seeing as it is needed for both software and hardware exceptions.) > { > - struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; > struct x86_event event = { > - .vector = vmcb->eventinj.fields.type, > - .type = vmcb->eventinj.fields.type, > - .error_code = vmcb->exitinfo1, > + .vector = vector, > + .type = type, > + .error_code = X86_EVENT_NO_EC, > + .insn_len = insn_len, > }; > > - event.insn_len = insn_len; > hvm_inject_event(&event); > } > > @@ -2655,10 +2655,27 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) > /* Asynchronous event, handled when we STGI'd after the VMEXIT. */ > HVMTRACE_0D(SMI); > break; > - Please retain this newline. > + case VMEXIT_ICEBP: > case VMEXIT_EXCEPTION_DB: > if ( !v->domain->debugger_attached ) > - hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC); > + { > + int rc; > + unsigned long trap_type = exit_reason == VMEXIT_ICEBP ? unsigned int. > + X86_EVENTTYPE_PRI_SW_EXCEPTION : X86_EVENTTYPE_HW_EXCEPTION; > + > + inst_len = 0; > + > + if ( trap_type >= X86_EVENTTYPE_SW_INTERRUPT ) > + inst_len = __get_instruction_length(v, INSTR_ICEBP); > + > + rc = hvm_monitor_debug(regs->rip, > + HVM_MONITOR_DEBUG_EXCEPTION, > + trap_type, inst_len); > + if ( rc < 0 ) > + goto unexpected_exit_type; > + if ( !rc ) > + svm_propagate_intr(inst_len, TRAP_debug, trap_type); > + } > else > domain_pause_for_debugger(); > break; > @@ -2687,7 +2704,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) > if ( rc < 0 ) > goto unexpected_exit_type; > if ( !rc ) > - svm_propagate_intr(v, inst_len); > + svm_propagate_intr(inst_len, TRAP_int3, X86_EVENTTYPE_SW_EXCEPTION); > } > break; > > diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c > index ae60d8d..06920d3 100644 > --- a/xen/arch/x86/hvm/svm/vmcb.c > +++ b/xen/arch/x86/hvm/svm/vmcb.c > @@ -73,7 +73,7 @@ static int construct_vmcb(struct vcpu *v) > GENERAL2_INTERCEPT_STGI | GENERAL2_INTERCEPT_CLGI | > GENERAL2_INTERCEPT_SKINIT | GENERAL2_INTERCEPT_MWAIT | > GENERAL2_INTERCEPT_WBINVD | GENERAL2_INTERCEPT_MONITOR | > - GENERAL2_INTERCEPT_XSETBV; > + GENERAL2_INTERCEPT_XSETBV | GENERAL2_INTERCEPT_ICEBP; This particular change wants to be conditional on debug monitoring being enabled. In the general case, we don't want to intercept ICEBP, especially as re-injecting it isn't fully implemented. ~Andrew > > /* Intercept all debug-register writes. */ > vmcb->_dr_intercepts = ~0u; > diff --git a/xen/include/asm-x86/hvm/svm/emulate.h b/xen/include/asm-x86/hvm/svm/emulate.h > index 7c1dcd1..3de8236 100644 > --- a/xen/include/asm-x86/hvm/svm/emulate.h > +++ b/xen/include/asm-x86/hvm/svm/emulate.h > @@ -38,6 +38,7 @@ enum instruction_index { > INSTR_STGI, > INSTR_CLGI, > INSTR_INVLPGA, > + INSTR_ICEBP, > INSTR_MAX_COUNT /* Must be last - Number of instructions supported */ > }; > > diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h > index 99ed4b87..c5a86d1 100644 > --- a/xen/include/asm-x86/monitor.h > +++ b/xen/include/asm-x86/monitor.h > @@ -82,12 +82,12 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d) > (1U << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) | > (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT) | > (1U << XEN_DOMCTL_MONITOR_EVENT_CPUID) | > + (1U << XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION) | > (1U << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG)); > > if ( cpu_has_vmx ) > { > - capabilities |= ((1U << XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION) | > - (1U << XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED)); > + capabilities |= (1U << XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED); > > /* Since we know this is on VMX, we can just call the hvm func */ > if ( hvm_is_singlestep_supported() ) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] hvm/svm: Implement Debug events 2018-03-20 15:53 ` Andrew Cooper @ 2018-03-20 16:05 ` Jan Beulich 2018-03-20 16:14 ` Andrew Cooper 0 siblings, 1 reply; 10+ messages in thread From: Jan Beulich @ 2018-03-20 16:05 UTC (permalink / raw) To: Andrew Cooper Cc: tamas, rcojocaru, xen-devel, suravee.suthikulpanit, Alexandru Isaila, boris.ostrovsky >>> On 20.03.18 at 16:53, <andrew.cooper3@citrix.com> wrote: > On 20/03/18 09:40, Alexandru Isaila wrote: >> @@ -2438,16 +2439,15 @@ static bool svm_get_pending_event(struct vcpu *v, struct x86_event *info) >> return true; >> } >> >> -static void svm_propagate_intr(struct vcpu *v, unsigned long insn_len) >> +static void svm_propagate_intr(unsigned long insn_len, int16_t vector, > uint8_t type) > > Hmm - not sure where the old unsigned long came from, but it isn't > really correct. Also, as this function no longer propagates the > contents of the vmcb, it is now mis-named. > > Please could you delete this function and use: > > diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h > index 2376ed6..843dafe 100644 > --- a/xen/include/asm-x86/hvm/hvm.h > +++ b/xen/include/asm-x86/hvm/hvm.h > @@ -407,6 +407,19 @@ void hvm_migrate_pirqs(struct vcpu *v); > > void hvm_inject_event(const struct x86_event *event); > > +static inline void hvm_inject_exception( > + unsigned int vector, unsigned int type, unsigned int insn_len) > +{ > + struct x86_event event = { > + .vector = vector, > + .type = type, > + .insn_len = insn_len, > + .error_code = X86_EVENT_NO_EC, > + }; > + > + hvm_inject_event(&event); > +} > + > static inline void hvm_inject_hw_exception(unsigned int vector, int > errcode) > { > struct x86_event event = { > > as a new common helper. (I'm not terribly happy with the name, but I > can't think of a better alternative, seeing as it is needed for both > software and hardware exceptions.) We'll need some better name though - "exception" together with an unconditional X86_EVENT_NO_EC is not really a reasonable combination. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] hvm/svm: Implement Debug events 2018-03-20 16:05 ` Jan Beulich @ 2018-03-20 16:14 ` Andrew Cooper 2018-03-20 16:33 ` Alexandru Stefan ISAILA 2018-03-20 16:44 ` Jan Beulich 0 siblings, 2 replies; 10+ messages in thread From: Andrew Cooper @ 2018-03-20 16:14 UTC (permalink / raw) To: Jan Beulich Cc: tamas, rcojocaru, xen-devel, suravee.suthikulpanit, Alexandru Isaila, boris.ostrovsky On 20/03/18 16:05, Jan Beulich wrote: >>>> On 20.03.18 at 16:53, <andrew.cooper3@citrix.com> wrote: >> On 20/03/18 09:40, Alexandru Isaila wrote: >>> @@ -2438,16 +2439,15 @@ static bool svm_get_pending_event(struct vcpu *v, struct x86_event *info) >>> return true; >>> } >>> >>> -static void svm_propagate_intr(struct vcpu *v, unsigned long insn_len) >>> +static void svm_propagate_intr(unsigned long insn_len, int16_t vector, >> uint8_t type) >> >> Hmm - not sure where the old unsigned long came from, but it isn't >> really correct. Also, as this function no longer propagates the >> contents of the vmcb, it is now mis-named. >> >> Please could you delete this function and use: >> >> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h >> index 2376ed6..843dafe 100644 >> --- a/xen/include/asm-x86/hvm/hvm.h >> +++ b/xen/include/asm-x86/hvm/hvm.h >> @@ -407,6 +407,19 @@ void hvm_migrate_pirqs(struct vcpu *v); >> >> void hvm_inject_event(const struct x86_event *event); >> >> +static inline void hvm_inject_exception( >> + unsigned int vector, unsigned int type, unsigned int insn_len) >> +{ >> + struct x86_event event = { >> + .vector = vector, >> + .type = type, >> + .insn_len = insn_len, >> + .error_code = X86_EVENT_NO_EC, >> + }; >> + >> + hvm_inject_event(&event); >> +} >> + >> static inline void hvm_inject_hw_exception(unsigned int vector, int >> errcode) >> { >> struct x86_event event = { >> >> as a new common helper. (I'm not terribly happy with the name, but I >> can't think of a better alternative, seeing as it is needed for both >> software and hardware exceptions.) > We'll need some better name though - "exception" together with > an unconditional X86_EVENT_NO_EC is not really a reasonable > combination. Alternatively, keep the current name, extend the parameter list with an "int error_code" and have the new callers pass X86_EVENT_NO_EC in? ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] hvm/svm: Implement Debug events 2018-03-20 16:14 ` Andrew Cooper @ 2018-03-20 16:33 ` Alexandru Stefan ISAILA 2018-03-20 16:44 ` Jan Beulich 1 sibling, 0 replies; 10+ messages in thread From: Alexandru Stefan ISAILA @ 2018-03-20 16:33 UTC (permalink / raw) To: JBeulich@suse.com, andrew.cooper3@citrix.com Cc: boris.ostrovsky@oracle.com, tamas@tklengyel.com, rcojocaru@bitdefender.com, suravee.suthikulpanit@amd.com, xen-devel@lists.xen.org On Ma, 2018-03-20 at 16:14 +0000, Andrew Cooper wrote: > On 20/03/18 16:05, Jan Beulich wrote: > > > > > > > > > > > > > > > > > > > On 20.03.18 at 16:53, <andrew.cooper3@citrix.com> wrote: > > > On 20/03/18 09:40, Alexandru Isaila wrote: > > > > > > > > @@ -2438,16 +2439,15 @@ static bool > > > > svm_get_pending_event(struct vcpu *v, struct x86_event *info) > > > > return true; > > > > } > > > > > > > > -static void svm_propagate_intr(struct vcpu *v, unsigned long > > > > insn_len) > > > > +static void svm_propagate_intr(unsigned long insn_len, int16_t > > > > vector, > > > uint8_t type) > > > > > > Hmm - not sure where the old unsigned long came from, but it > > > isn't > > > really correct. Also, as this function no longer propagates the > > > contents of the vmcb, it is now mis-named. > > > > > > Please could you delete this function and use: > > > > > > diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm- > > > x86/hvm/hvm.h > > > index 2376ed6..843dafe 100644 > > > --- a/xen/include/asm-x86/hvm/hvm.h > > > +++ b/xen/include/asm-x86/hvm/hvm.h > > > @@ -407,6 +407,19 @@ void hvm_migrate_pirqs(struct vcpu *v); > > > > > > void hvm_inject_event(const struct x86_event *event); > > > > > > +static inline void hvm_inject_exception( > > > + unsigned int vector, unsigned int type, unsigned int > > > insn_len) > > > +{ > > > + struct x86_event event = { > > > + .vector = vector, > > > + .type = type, > > > + .insn_len = insn_len, > > > + .error_code = X86_EVENT_NO_EC, > > > + }; > > > + > > > + hvm_inject_event(&event); > > > +} > > > + > > > static inline void hvm_inject_hw_exception(unsigned int vector, > > > int > > > errcode) > > > { > > > struct x86_event event = { > > > > > > as a new common helper. (I'm not terribly happy with the name, > > > but I > > > can't think of a better alternative, seeing as it is needed for > > > both > > > software and hardware exceptions.) > > We'll need some better name though - "exception" together with > > an unconditional X86_EVENT_NO_EC is not really a reasonable > > combination. > Alternatively, keep the current name, extend the parameter list with > an > "int error_code" and have the new callers pass X86_EVENT_NO_EC in? > > ~Andrew > Keeping the name with an extra parameter sounds better to me. Then hvm_inject_exception is it. ~Alex ________________________ This email was scanned by Bitdefender _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] hvm/svm: Implement Debug events 2018-03-20 16:14 ` Andrew Cooper 2018-03-20 16:33 ` Alexandru Stefan ISAILA @ 2018-03-20 16:44 ` Jan Beulich 1 sibling, 0 replies; 10+ messages in thread From: Jan Beulich @ 2018-03-20 16:44 UTC (permalink / raw) To: Andrew Cooper Cc: tamas, rcojocaru, xen-devel, suravee.suthikulpanit, Alexandru Isaila, boris.ostrovsky >>> On 20.03.18 at 17:14, <andrew.cooper3@citrix.com> wrote: > On 20/03/18 16:05, Jan Beulich wrote: >>>>> On 20.03.18 at 16:53, <andrew.cooper3@citrix.com> wrote: >>> Please could you delete this function and use: >>> >>> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h >>> index 2376ed6..843dafe 100644 >>> --- a/xen/include/asm-x86/hvm/hvm.h >>> +++ b/xen/include/asm-x86/hvm/hvm.h >>> @@ -407,6 +407,19 @@ void hvm_migrate_pirqs(struct vcpu *v); >>> >>> void hvm_inject_event(const struct x86_event *event); >>> >>> +static inline void hvm_inject_exception( >>> + unsigned int vector, unsigned int type, unsigned int insn_len) >>> +{ >>> + struct x86_event event = { >>> + .vector = vector, >>> + .type = type, >>> + .insn_len = insn_len, >>> + .error_code = X86_EVENT_NO_EC, >>> + }; >>> + >>> + hvm_inject_event(&event); >>> +} >>> + >>> static inline void hvm_inject_hw_exception(unsigned int vector, int >>> errcode) >>> { >>> struct x86_event event = { >>> >>> as a new common helper. (I'm not terribly happy with the name, but I >>> can't think of a better alternative, seeing as it is needed for both >>> software and hardware exceptions.) >> We'll need some better name though - "exception" together with >> an unconditional X86_EVENT_NO_EC is not really a reasonable >> combination. > > Alternatively, keep the current name, extend the parameter list with an > "int error_code" and have the new callers pass X86_EVENT_NO_EC in? Fine with me. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] hvm/svm: Implement Debug events 2018-03-20 9:40 [PATCH v2] hvm/svm: Implement Debug events Alexandru Isaila 2018-03-20 15:53 ` Andrew Cooper @ 2018-03-20 16:49 ` Tamas K Lengyel 2018-03-20 16:57 ` Andrew Cooper 2018-03-20 17:07 ` Alexandru Stefan ISAILA 1 sibling, 2 replies; 10+ messages in thread From: Tamas K Lengyel @ 2018-03-20 16:49 UTC (permalink / raw) To: Alexandru Isaila Cc: Suravee Suthikulpanit, Razvan Cojocaru, Andrew Cooper, Xen-devel, Jan Beulich, Boris Ostrovsky On Tue, Mar 20, 2018 at 3:40 AM, Alexandru Isaila <aisaila@bitdefender.com> wrote: > At this moment the Debug events for the AMD architecture are not > forwarded to the monitor layer. > > This patch adds the Debug event to the common capabilities, adds > the VMEXIT_ICEBP then forwards the event to the monitor layer. > > Chapter 2: SVM Processor and Platform Extensions: "Note: A vector 1 > exception generated by the single byte INT1 > instruction (also known as ICEBP) does not trigger the #DB > intercept. Software should use the dedicated ICEBP > intercept to intercept ICEBP" > > --- > Changes since V1: > - Get inst_len from __get_instruction_length() > - Updated __get_instruction_length() for the INSTR_ICEBP > instruction > > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > --- > xen/arch/x86/hvm/svm/emulate.c | 1 + > xen/arch/x86/hvm/svm/svm.c | 37 +++++++++++++++++++++++++---------- > xen/arch/x86/hvm/svm/vmcb.c | 2 +- > xen/include/asm-x86/hvm/svm/emulate.h | 1 + > xen/include/asm-x86/monitor.h | 4 ++-- > 5 files changed, 32 insertions(+), 13 deletions(-) > > diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c > index e1a1581..172369e 100644 > --- a/xen/arch/x86/hvm/svm/emulate.c > +++ b/xen/arch/x86/hvm/svm/emulate.c > @@ -80,6 +80,7 @@ static const struct { > [INSTR_RDTSC] = { X86EMUL_OPC(0x0f, 0x31) }, > [INSTR_RDMSR] = { X86EMUL_OPC(0x0f, 0x32) }, > [INSTR_CPUID] = { X86EMUL_OPC(0x0f, 0xa2) }, > + [INSTR_ICEBP] = { X86EMUL_OPC( 0, 0xf1) }, > }; > > int __get_instruction_length_from_list(struct vcpu *v, > diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c > index c34f5b5..d4f2290 100644 > --- a/xen/arch/x86/hvm/svm/svm.c > +++ b/xen/arch/x86/hvm/svm/svm.c > @@ -1109,7 +1109,8 @@ static void noreturn svm_do_resume(struct vcpu *v) > { > struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; > bool debug_state = (v->domain->debugger_attached || > - v->domain->arch.monitor.software_breakpoint_enabled); > + v->domain->arch.monitor.software_breakpoint_enabled || > + v->domain->arch.monitor.debug_exception_enabled); Please explain the rationale why this is included under "debug_exception" and not "software_breakpoint". Thanks, Tamas _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] hvm/svm: Implement Debug events 2018-03-20 16:49 ` Tamas K Lengyel @ 2018-03-20 16:57 ` Andrew Cooper 2018-03-20 17:07 ` Alexandru Stefan ISAILA 1 sibling, 0 replies; 10+ messages in thread From: Andrew Cooper @ 2018-03-20 16:57 UTC (permalink / raw) To: Tamas K Lengyel, Alexandru Isaila Cc: Boris Ostrovsky, Razvan Cojocaru, Jan Beulich, Suravee Suthikulpanit, Xen-devel On 20/03/18 16:49, Tamas K Lengyel wrote: > On Tue, Mar 20, 2018 at 3:40 AM, Alexandru Isaila > <aisaila@bitdefender.com> wrote: >> At this moment the Debug events for the AMD architecture are not >> forwarded to the monitor layer. >> >> This patch adds the Debug event to the common capabilities, adds >> the VMEXIT_ICEBP then forwards the event to the monitor layer. >> >> Chapter 2: SVM Processor and Platform Extensions: "Note: A vector 1 >> exception generated by the single byte INT1 >> instruction (also known as ICEBP) does not trigger the #DB >> intercept. Software should use the dedicated ICEBP >> intercept to intercept ICEBP" >> >> --- >> Changes since V1: >> - Get inst_len from __get_instruction_length() >> - Updated __get_instruction_length() for the INSTR_ICEBP >> instruction >> >> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> >> --- >> xen/arch/x86/hvm/svm/emulate.c | 1 + >> xen/arch/x86/hvm/svm/svm.c | 37 +++++++++++++++++++++++++---------- >> xen/arch/x86/hvm/svm/vmcb.c | 2 +- >> xen/include/asm-x86/hvm/svm/emulate.h | 1 + >> xen/include/asm-x86/monitor.h | 4 ++-- >> 5 files changed, 32 insertions(+), 13 deletions(-) >> >> diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c >> index e1a1581..172369e 100644 >> --- a/xen/arch/x86/hvm/svm/emulate.c >> +++ b/xen/arch/x86/hvm/svm/emulate.c >> @@ -80,6 +80,7 @@ static const struct { >> [INSTR_RDTSC] = { X86EMUL_OPC(0x0f, 0x31) }, >> [INSTR_RDMSR] = { X86EMUL_OPC(0x0f, 0x32) }, >> [INSTR_CPUID] = { X86EMUL_OPC(0x0f, 0xa2) }, >> + [INSTR_ICEBP] = { X86EMUL_OPC( 0, 0xf1) }, >> }; >> >> int __get_instruction_length_from_list(struct vcpu *v, >> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c >> index c34f5b5..d4f2290 100644 >> --- a/xen/arch/x86/hvm/svm/svm.c >> +++ b/xen/arch/x86/hvm/svm/svm.c >> @@ -1109,7 +1109,8 @@ static void noreturn svm_do_resume(struct vcpu *v) >> { >> struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; >> bool debug_state = (v->domain->debugger_attached || >> - v->domain->arch.monitor.software_breakpoint_enabled); >> + v->domain->arch.monitor.software_breakpoint_enabled || >> + v->domain->arch.monitor.debug_exception_enabled); > Please explain the rationale why this is included under > "debug_exception" and not "software_breakpoint". FTR (as I haven't commented on this aspect yet), I think it is important that however it is classified, it behaves the same on different hardware. i.e. monitoring of ICEBP is reported consistently between Intel and AMD. Beyond that, I'm not fussed exactly how it is classified. Fundamentally, it is a software breakpoint because it is (only) a real usable instruction, but in reality, the only time you'll ever see it is test code, malware attempting to exploit hypervisor vulnerabilities, or when someone really has got an ICE hooked up to a real system (at which point it gets intercepted in the lower levels of the CPU and disappears behind our backs). ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] hvm/svm: Implement Debug events 2018-03-20 16:49 ` Tamas K Lengyel 2018-03-20 16:57 ` Andrew Cooper @ 2018-03-20 17:07 ` Alexandru Stefan ISAILA 2018-03-20 18:28 ` Tamas K Lengyel 1 sibling, 1 reply; 10+ messages in thread From: Alexandru Stefan ISAILA @ 2018-03-20 17:07 UTC (permalink / raw) To: tamas@tklengyel.com Cc: suravee.suthikulpanit@amd.com, rcojocaru@bitdefender.com, andrew.cooper3@citrix.com, xen-devel@lists.xen.org, jbeulich@suse.com, boris.ostrovsky@oracle.com On Ma, 2018-03-20 at 10:49 -0600, Tamas K Lengyel wrote: > On Tue, Mar 20, 2018 at 3:40 AM, Alexandru Isaila > <aisaila@bitdefender.com> wrote: > > > > At this moment the Debug events for the AMD architecture are not > > forwarded to the monitor layer. > > > > This patch adds the Debug event to the common capabilities, adds > > the VMEXIT_ICEBP then forwards the event to the monitor layer. > > > > Chapter 2: SVM Processor and Platform Extensions: "Note: A vector 1 > > exception generated by the single byte INT1 > > instruction (also known as ICEBP) does not trigger the #DB > > intercept. Software should use the dedicated ICEBP > > intercept to intercept ICEBP" > > > > --- > > Changes since V1: > > - Get inst_len from __get_instruction_length() > > - Updated __get_instruction_length() for the INSTR_ICEBP > > instruction > > > > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > > --- > > xen/arch/x86/hvm/svm/emulate.c | 1 + > > xen/arch/x86/hvm/svm/svm.c | 37 > > +++++++++++++++++++++++++---------- > > xen/arch/x86/hvm/svm/vmcb.c | 2 +- > > xen/include/asm-x86/hvm/svm/emulate.h | 1 + > > xen/include/asm-x86/monitor.h | 4 ++-- > > 5 files changed, 32 insertions(+), 13 deletions(-) > > > > diff --git a/xen/arch/x86/hvm/svm/emulate.c > > b/xen/arch/x86/hvm/svm/emulate.c > > index e1a1581..172369e 100644 > > --- a/xen/arch/x86/hvm/svm/emulate.c > > +++ b/xen/arch/x86/hvm/svm/emulate.c > > @@ -80,6 +80,7 @@ static const struct { > > [INSTR_RDTSC] = { X86EMUL_OPC(0x0f, 0x31) }, > > [INSTR_RDMSR] = { X86EMUL_OPC(0x0f, 0x32) }, > > [INSTR_CPUID] = { X86EMUL_OPC(0x0f, 0xa2) }, > > + [INSTR_ICEBP] = { X86EMUL_OPC( 0, 0xf1) }, > > }; > > > > int __get_instruction_length_from_list(struct vcpu *v, > > diff --git a/xen/arch/x86/hvm/svm/svm.c > > b/xen/arch/x86/hvm/svm/svm.c > > index c34f5b5..d4f2290 100644 > > --- a/xen/arch/x86/hvm/svm/svm.c > > +++ b/xen/arch/x86/hvm/svm/svm.c > > @@ -1109,7 +1109,8 @@ static void noreturn svm_do_resume(struct > > vcpu *v) > > { > > struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; > > bool debug_state = (v->domain->debugger_attached || > > - v->domain- > > >arch.monitor.software_breakpoint_enabled); > > + v->domain- > > >arch.monitor.software_breakpoint_enabled || > > + v->domain- > > >arch.monitor.debug_exception_enabled); > Please explain the rationale why this is included under > "debug_exception" and not "software_breakpoint". > > Thanks, > Tamas > Hi Tamas, I've included the icebp under the debug_exception only to have consistency with the vmx side. I saw this behavior after running the swint-emulation from xtf. Regards, Alex ________________________ This email was scanned by Bitdefender _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] hvm/svm: Implement Debug events 2018-03-20 17:07 ` Alexandru Stefan ISAILA @ 2018-03-20 18:28 ` Tamas K Lengyel 0 siblings, 0 replies; 10+ messages in thread From: Tamas K Lengyel @ 2018-03-20 18:28 UTC (permalink / raw) To: Alexandru Stefan ISAILA Cc: suravee.suthikulpanit@amd.com, rcojocaru@bitdefender.com, andrew.cooper3@citrix.com, xen-devel@lists.xen.org, jbeulich@suse.com, boris.ostrovsky@oracle.com On Tue, Mar 20, 2018 at 11:07 AM, Alexandru Stefan ISAILA <aisaila@bitdefender.com> wrote: > On Ma, 2018-03-20 at 10:49 -0600, Tamas K Lengyel wrote: >> On Tue, Mar 20, 2018 at 3:40 AM, Alexandru Isaila >> <aisaila@bitdefender.com> wrote: >> > >> > At this moment the Debug events for the AMD architecture are not >> > forwarded to the monitor layer. >> > >> > This patch adds the Debug event to the common capabilities, adds >> > the VMEXIT_ICEBP then forwards the event to the monitor layer. >> > >> > Chapter 2: SVM Processor and Platform Extensions: "Note: A vector 1 >> > exception generated by the single byte INT1 >> > instruction (also known as ICEBP) does not trigger the #DB >> > intercept. Software should use the dedicated ICEBP >> > intercept to intercept ICEBP" >> > >> > --- >> > Changes since V1: >> > - Get inst_len from __get_instruction_length() >> > - Updated __get_instruction_length() for the INSTR_ICEBP >> > instruction >> > >> > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> >> > --- >> > xen/arch/x86/hvm/svm/emulate.c | 1 + >> > xen/arch/x86/hvm/svm/svm.c | 37 >> > +++++++++++++++++++++++++---------- >> > xen/arch/x86/hvm/svm/vmcb.c | 2 +- >> > xen/include/asm-x86/hvm/svm/emulate.h | 1 + >> > xen/include/asm-x86/monitor.h | 4 ++-- >> > 5 files changed, 32 insertions(+), 13 deletions(-) >> > >> > diff --git a/xen/arch/x86/hvm/svm/emulate.c >> > b/xen/arch/x86/hvm/svm/emulate.c >> > index e1a1581..172369e 100644 >> > --- a/xen/arch/x86/hvm/svm/emulate.c >> > +++ b/xen/arch/x86/hvm/svm/emulate.c >> > @@ -80,6 +80,7 @@ static const struct { >> > [INSTR_RDTSC] = { X86EMUL_OPC(0x0f, 0x31) }, >> > [INSTR_RDMSR] = { X86EMUL_OPC(0x0f, 0x32) }, >> > [INSTR_CPUID] = { X86EMUL_OPC(0x0f, 0xa2) }, >> > + [INSTR_ICEBP] = { X86EMUL_OPC( 0, 0xf1) }, >> > }; >> > >> > int __get_instruction_length_from_list(struct vcpu *v, >> > diff --git a/xen/arch/x86/hvm/svm/svm.c >> > b/xen/arch/x86/hvm/svm/svm.c >> > index c34f5b5..d4f2290 100644 >> > --- a/xen/arch/x86/hvm/svm/svm.c >> > +++ b/xen/arch/x86/hvm/svm/svm.c >> > @@ -1109,7 +1109,8 @@ static void noreturn svm_do_resume(struct >> > vcpu *v) >> > { >> > struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; >> > bool debug_state = (v->domain->debugger_attached || >> > - v->domain- >> > >arch.monitor.software_breakpoint_enabled); >> > + v->domain- >> > >arch.monitor.software_breakpoint_enabled || >> > + v->domain- >> > >arch.monitor.debug_exception_enabled); >> Please explain the rationale why this is included under >> "debug_exception" and not "software_breakpoint". >> >> Thanks, >> Tamas >> > Hi Tamas, > > I've included the icebp under the debug_exception only to have > consistency with the vmx side. I saw this behavior after running > the swint-emulation from xtf. > Sounds good, as long as we are consistent it's good. Thanks, Tamas _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-03-20 18:28 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-03-20 9:40 [PATCH v2] hvm/svm: Implement Debug events Alexandru Isaila 2018-03-20 15:53 ` Andrew Cooper 2018-03-20 16:05 ` Jan Beulich 2018-03-20 16:14 ` Andrew Cooper 2018-03-20 16:33 ` Alexandru Stefan ISAILA 2018-03-20 16:44 ` Jan Beulich 2018-03-20 16:49 ` Tamas K Lengyel 2018-03-20 16:57 ` Andrew Cooper 2018-03-20 17:07 ` Alexandru Stefan ISAILA 2018-03-20 18:28 ` Tamas K Lengyel
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.