| Message ID | 1466701647-21733-5-git-send-email-tamas@tklengyel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 23.06.16 at 19:07, <tamas@tklengyel.com> wrote: > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -3373,10 +3373,39 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) > HVMTRACE_1D(TRAP_DEBUG, exit_qualification); > write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE); > if ( !v->domain->debugger_attached ) > - vmx_propagate_intr(intr_info); > + { > + unsigned long insn_len = 0; > + int rc; > + unsigned long trap_type = MASK_EXTR(intr_info, > + INTR_INFO_INTR_TYPE_MASK); > + > + if ( trap_type >= X86_EVENTTYPE_SW_INTERRUPT ) > + __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_len); > + > + rc = hvm_monitor_debug(regs->eip, > + HVM_MONITOR_DEBUG_EXCEPTION, > + trap_type, insn_len); > + > + /* > + * !rc continue normally > + * rc > paused waiting for response, work here is done > + * rc < error, fall-through to exit_and_crash > + */ > + if ( !rc ) > + { > + vmx_propagate_intr(intr_info); > + break; > + } > + if ( rc > 0 ) > + break; > + } > else > + { > domain_pause_for_debugger(); > - break; > + break; > + } > + > + goto exit_and_crash; I continue to think this is sub-optimal structuring (at once needlessly making the patch larger), but it'll be the VMX maintainers to judge. For the few pieces it is relevant for: Acked-by: Jan Beulich <jbeulich@suse.com> Jan
On 06/23/2016 08:07 PM, Tamas K Lengyel wrote: > Since in-guest debug exceptions are now unconditionally trapped to Xen, adding > a hook for vm_event subscribers to tap into this new always-on guest event. We > rename along the way hvm_event_breakpoint_type to hvm_event_type to better > match the various events that can be passed with it. We also introduce the > necessary monitor_op domctl's to enable subscribing to the events. > > This patch also provides monitor subscribers to int3 events proper access > to the instruction length necessary for accurate event-reinjection. Without > this subscribers manually have to evaluate if the int3 instruction has any > prefix attached which would change the instruction length. > > Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com> > --- > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com> > Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Jun Nakajima <jun.nakajima@intel.com> > Cc: Kevin Tian <kevin.tian@intel.com> > > v6: Add comment describing rc condition values for the monitor call > v5: Transmit the proper insn_length for int3 events as well to fix > the current bug with prefixed int3 instructions. > --- > tools/libxc/include/xenctrl.h | 3 +- > tools/libxc/xc_monitor.c | 25 +++++++++++++++ > tools/tests/xen-access/xen-access.c | 63 ++++++++++++++++++++++++++++++++----- > xen/arch/x86/hvm/monitor.c | 21 +++++++++++-- > xen/arch/x86/hvm/vmx/vmx.c | 55 +++++++++++++++++++++++++------- > xen/arch/x86/monitor.c | 16 ++++++++++ > xen/include/asm-x86/domain.h | 2 ++ > xen/include/asm-x86/hvm/monitor.h | 7 +++-- > xen/include/asm-x86/monitor.h | 3 +- > xen/include/public/domctl.h | 6 ++++ > xen/include/public/vm_event.h | 14 +++++++-- > 11 files changed, 186 insertions(+), 29 deletions(-) > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h > index 4f5d954..4a85b4a 100644 > --- a/tools/libxc/include/xenctrl.h > +++ b/tools/libxc/include/xenctrl.h > @@ -2165,7 +2165,8 @@ int xc_monitor_software_breakpoint(xc_interface *xch, domid_t domain_id, > bool enable); > int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id, > bool enable, bool sync); > - > +int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id, > + bool enable, bool sync); > /** > * This function enables / disables emulation for each REP for a > * REP-compatible instruction. > diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c > index 78131b2..264992c 100644 > --- a/tools/libxc/xc_monitor.c > +++ b/tools/libxc/xc_monitor.c > @@ -156,3 +156,28 @@ int xc_monitor_emulate_each_rep(xc_interface *xch, domid_t domain_id, > > return do_domctl(xch, &domctl); > } > + > +int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id, > + bool enable, bool sync) > +{ > + DECLARE_DOMCTL; > + > + domctl.cmd = XEN_DOMCTL_monitor_op; > + domctl.domain = domain_id; > + domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE > + : XEN_DOMCTL_MONITOR_OP_DISABLE; > + domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION; > + domctl.u.monitor_op.u.debug_exception.sync = sync; > + > + return do_domctl(xch, &domctl); > +} > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * tab-width: 4 > + * indent-tabs-mode: nil > + * End: > + */ > diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c > index f26e723..e615476 100644 > --- a/tools/tests/xen-access/xen-access.c > +++ b/tools/tests/xen-access/xen-access.c > @@ -53,6 +53,10 @@ > #define ERROR(a, b...) fprintf(stderr, a "\n", ## b) > #define PERROR(a, b...) fprintf(stderr, a ": %s\n", ## b, strerror(errno)) > > +/* From xen/include/asm-x86/processor.h */ > +#define X86_TRAP_DEBUG 1 > +#define X86_TRAP_INT3 3 > + > typedef struct vm_event { > domid_t domain_id; > xenevtchn_handle *xce_handle; > @@ -333,7 +337,7 @@ void usage(char* progname) > { > fprintf(stderr, "Usage: %s [-m] <domain_id> write|exec", progname); > #if defined(__i386__) || defined(__x86_64__) > - fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec"); > + fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec|debug"); > #endif > fprintf(stderr, > "\n" > @@ -354,10 +358,12 @@ int main(int argc, char *argv[]) > xc_interface *xch; > xenmem_access_t default_access = XENMEM_access_rwx; > xenmem_access_t after_first_access = XENMEM_access_rwx; > + int memaccess = 0; > int required = 0; > int breakpoint = 0; > int shutting_down = 0; > int altp2m = 0; > + int debug = 0; > uint16_t altp2m_view_id = 0; > > char* progname = argv[0]; > @@ -391,11 +397,13 @@ int main(int argc, char *argv[]) > { > default_access = XENMEM_access_rx; > after_first_access = XENMEM_access_rwx; > + memaccess = 1; > } > else if ( !strcmp(argv[0], "exec") ) > { > default_access = XENMEM_access_rw; > after_first_access = XENMEM_access_rwx; > + memaccess = 1; > } > #if defined(__i386__) || defined(__x86_64__) > else if ( !strcmp(argv[0], "breakpoint") ) > @@ -406,11 +414,17 @@ int main(int argc, char *argv[]) > { > default_access = XENMEM_access_rx; > altp2m = 1; > + memaccess = 1; > } > else if ( !strcmp(argv[0], "altp2m_exec") ) > { > default_access = XENMEM_access_rw; > altp2m = 1; > + memaccess = 1; > + } > + else if ( !strcmp(argv[0], "debug") ) > + { > + debug = 1; > } > #endif > else > @@ -446,7 +460,7 @@ int main(int argc, char *argv[]) > } > > /* With altp2m we just create a new, restricted view of the memory */ > - if ( altp2m ) > + if ( memaccess && altp2m ) > { > xen_pfn_t gfn = 0; > unsigned long perm_set = 0; > @@ -493,7 +507,7 @@ int main(int argc, char *argv[]) > } > } > > - if ( !altp2m ) > + if ( memaccess && !altp2m ) > { > /* Set the default access type and convert all pages to it */ > rc = xc_set_mem_access(xch, domain_id, default_access, ~0ull, 0); > @@ -524,6 +538,16 @@ int main(int argc, char *argv[]) > } > } > > + if ( debug ) > + { > + rc = xc_monitor_debug_exceptions(xch, domain_id, 1, 1); > + if ( rc < 0 ) > + { > + ERROR("Error %d setting debug exception listener with vm_event\n", rc); > + goto exit; > + } > + } > + > /* Wait for access */ > for (;;) > { > @@ -534,6 +558,8 @@ int main(int argc, char *argv[]) > > if ( breakpoint ) > rc = xc_monitor_software_breakpoint(xch, domain_id, 0); > + if ( debug ) > + rc = xc_monitor_debug_exceptions(xch, domain_id, 0, 0); > > if ( altp2m ) > { > @@ -635,22 +661,22 @@ int main(int argc, char *argv[]) > rsp.u.mem_access = req.u.mem_access; > break; > case VM_EVENT_REASON_SOFTWARE_BREAKPOINT: > - printf("Breakpoint: rip=%016"PRIx64", gfn=%"PRIx64" (vcpu %d)\n", > + printf("Breakpoint: rip=%016"PRIx64" gfn=%"PRIx64" (vcpu %d)\n", You seem to have removed the comma here (',') after the first "PRIx64", but ... > req.data.regs.x86.rip, > req.u.software_breakpoint.gfn, > req.vcpu_id); > > /* Reinject */ > - rc = xc_hvm_inject_trap( > - xch, domain_id, req.vcpu_id, 3, > - HVMOP_TRAP_sw_exc, -1, 0, 0); > + rc = xc_hvm_inject_trap(xch, domain_id, req.vcpu_id, > + X86_TRAP_INT3, > + req.u.software_breakpoint.type, -1, > + req.u.software_breakpoint.insn_length, 0); > if (rc < 0) > { > ERROR("Error %d injecting breakpoint\n", rc); > interrupted = -1; > continue; > } > - > break; > case VM_EVENT_REASON_SINGLESTEP: > printf("Singlestep: rip=%016"PRIx64", vcpu %d, altp2m %u\n", > @@ -669,6 +695,27 @@ int main(int argc, char *argv[]) > rsp.flags |= VM_EVENT_FLAG_TOGGLE_SINGLESTEP; > > break; > + case VM_EVENT_REASON_DEBUG_EXCEPTION: > + printf("Debug exception: rip=%016"PRIx64", vcpu %d. Type: %u. Length: %u\n", ... this newly added line uses the old style (with a comma after the first "PRIx64"). Minor issue maybe, I just don't understand why the first modification was made. > + req.data.regs.x86.rip, > + req.vcpu_id, > + req.u.debug_exception.type, > + req.u.debug_exception.insn_length); > + > + /* Reinject */ > + rc = xc_hvm_inject_trap(xch, domain_id, req.vcpu_id, > + X86_TRAP_DEBUG, > + req.u.debug_exception.type, -1, > + req.u.debug_exception.insn_length, > + req.data.regs.x86.cr2); > + if (rc < 0) > + { > + ERROR("Error %d injecting breakpoint\n", rc); > + interrupted = -1; > + continue; > + } > + > + break; > default: > fprintf(stderr, "UNKNOWN REASON CODE %d\n", req.reason); > } > diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c > index 472926c..bbe5952 100644 > --- a/xen/arch/x86/hvm/monitor.c > +++ b/xen/arch/x86/hvm/monitor.c > @@ -87,12 +87,13 @@ static inline unsigned long gfn_of_rip(unsigned long rip) > return paging_gva_to_gfn(curr, sreg.base + rip, &pfec); > } > > -int hvm_monitor_breakpoint(unsigned long rip, > - enum hvm_monitor_breakpoint_type type) > +int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type, > + unsigned long trap_type, unsigned long insn_length) > { > struct vcpu *curr = current; > struct arch_domain *ad = &curr->domain->arch; > vm_event_request_t req = {}; > + bool_t sync; > > switch ( type ) > { > @@ -101,6 +102,9 @@ int hvm_monitor_breakpoint(unsigned long rip, > return 0; > req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT; > req.u.software_breakpoint.gfn = gfn_of_rip(rip); > + req.u.software_breakpoint.type = trap_type; > + req.u.software_breakpoint.insn_length = insn_length; > + sync = 1; > break; > > case HVM_MONITOR_SINGLESTEP_BREAKPOINT: > @@ -108,6 +112,17 @@ int hvm_monitor_breakpoint(unsigned long rip, > return 0; > req.reason = VM_EVENT_REASON_SINGLESTEP; > req.u.singlestep.gfn = gfn_of_rip(rip); > + sync = 1; > + break; > + > + case HVM_MONITOR_DEBUG_EXCEPTION: > + if ( !ad->monitor.debug_exception_enabled ) > + return 0; > + req.reason = VM_EVENT_REASON_DEBUG_EXCEPTION; > + req.u.debug_exception.gfn = gfn_of_rip(rip); > + req.u.debug_exception.type = trap_type; > + req.u.debug_exception.insn_length = insn_length; > + sync = !!ad->monitor.debug_exception_sync; > break; > > default: > @@ -116,7 +131,7 @@ int hvm_monitor_breakpoint(unsigned long rip, > > req.vcpu_id = curr->vcpu_id; > > - return vm_event_monitor_traps(curr, 1, &req); > + return vm_event_monitor_traps(curr, sync, &req); What would be a basic use-case for this event to be sent without pausing the VCPU (i.e. with sync != 1)? Thanks, Razvan
> From: Tamas K Lengyel [mailto:tamas@tklengyel.com] > Sent: Friday, June 24, 2016 1:07 AM > > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > index 03fcba7..4b69ca6 100644 > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -3373,10 +3373,39 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) > HVMTRACE_1D(TRAP_DEBUG, exit_qualification); > write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE); > if ( !v->domain->debugger_attached ) > - vmx_propagate_intr(intr_info); > + { > + unsigned long insn_len = 0; > + int rc; > + unsigned long trap_type = MASK_EXTR(intr_info, > + INTR_INFO_INTR_TYPE_MASK); > + > + if ( trap_type >= X86_EVENTTYPE_SW_INTERRUPT ) > + __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_len); > + > + rc = hvm_monitor_debug(regs->eip, > + HVM_MONITOR_DEBUG_EXCEPTION, > + trap_type, insn_len); > + > + /* > + * !rc continue normally > + * rc > paused waiting for response, work here is done Did you mean "rc > 0, vcpu is paused and waiting for response..."? Please make the comment clear to understand. > + * rc < error, fall-through to exit_and_crash > + */ > + if ( !rc ) > + { > + vmx_propagate_intr(intr_info); > + break; > + } > + if ( rc > 0 ) > + break; > + } > else > + { > domain_pause_for_debugger(); > - break; > + break; > + } > + > + goto exit_and_crash; Putting goto as the last line within a 'case' looks a bit strange. What about putting goto directly under a "if ( rc < 0 )" check earlier? if ( !rc ) ... if ( rc < 0 ) goto exit_and_crash; } else domain_pause_for_debugger(); break; Thanks Kevin
>>> On 24.06.16 at 13:20, <kevin.tian@intel.com> wrote: >> From: Tamas K Lengyel [mailto:tamas@tklengyel.com] >> Sent: Friday, June 24, 2016 1:07 AM >> >> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c >> index 03fcba7..4b69ca6 100644 >> --- a/xen/arch/x86/hvm/vmx/vmx.c >> +++ b/xen/arch/x86/hvm/vmx/vmx.c >> @@ -3373,10 +3373,39 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) >> HVMTRACE_1D(TRAP_DEBUG, exit_qualification); >> write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE); >> if ( !v->domain->debugger_attached ) >> - vmx_propagate_intr(intr_info); >> + { >> + unsigned long insn_len = 0; >> + int rc; >> + unsigned long trap_type = MASK_EXTR(intr_info, >> + INTR_INFO_INTR_TYPE_MASK); >> + >> + if ( trap_type >= X86_EVENTTYPE_SW_INTERRUPT ) >> + __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_len); >> + >> + rc = hvm_monitor_debug(regs->eip, >> + HVM_MONITOR_DEBUG_EXCEPTION, >> + trap_type, insn_len); >> + >> + /* >> + * !rc continue normally >> + * rc > paused waiting for response, work here is done > > Did you mean "rc > 0, vcpu is paused and waiting for response..."? Please > make the comment clear to understand. > >> + * rc < error, fall-through to exit_and_crash >> + */ >> + if ( !rc ) >> + { >> + vmx_propagate_intr(intr_info); >> + break; >> + } >> + if ( rc > 0 ) >> + break; >> + } >> else >> + { >> domain_pause_for_debugger(); >> - break; >> + break; >> + } >> + >> + goto exit_and_crash; > > Putting goto as the last line within a 'case' looks a bit strange. What > about putting goto directly under a "if ( rc < 0 )" check earlier? > > if ( !rc ) > ... > if ( rc < 0 ) > goto exit_and_crash; > } > else > domain_pause_for_debugger(); > break; Thanks, Kevin - indeed that's exactly what I had asked for already on the previous iteration. Jan
On Fri, Jun 24, 2016 at 5:24 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 24.06.16 at 13:20, <kevin.tian@intel.com> wrote: >>> From: Tamas K Lengyel [mailto:tamas@tklengyel.com] >>> Sent: Friday, June 24, 2016 1:07 AM >>> >>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c >>> index 03fcba7..4b69ca6 100644 >>> --- a/xen/arch/x86/hvm/vmx/vmx.c >>> +++ b/xen/arch/x86/hvm/vmx/vmx.c >>> @@ -3373,10 +3373,39 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) >>> HVMTRACE_1D(TRAP_DEBUG, exit_qualification); >>> write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE); >>> if ( !v->domain->debugger_attached ) >>> - vmx_propagate_intr(intr_info); >>> + { >>> + unsigned long insn_len = 0; >>> + int rc; >>> + unsigned long trap_type = MASK_EXTR(intr_info, >>> + INTR_INFO_INTR_TYPE_MASK); >>> + >>> + if ( trap_type >= X86_EVENTTYPE_SW_INTERRUPT ) >>> + __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_len); >>> + >>> + rc = hvm_monitor_debug(regs->eip, >>> + HVM_MONITOR_DEBUG_EXCEPTION, >>> + trap_type, insn_len); >>> + >>> + /* >>> + * !rc continue normally >>> + * rc > paused waiting for response, work here is done >> >> Did you mean "rc > 0, vcpu is paused and waiting for response..."? Please >> make the comment clear to understand. Yes, sure. >> >>> + * rc < error, fall-through to exit_and_crash >>> + */ >>> + if ( !rc ) >>> + { >>> + vmx_propagate_intr(intr_info); >>> + break; >>> + } >>> + if ( rc > 0 ) >>> + break; >>> + } >>> else >>> + { >>> domain_pause_for_debugger(); >>> - break; >>> + break; >>> + } >>> + >>> + goto exit_and_crash; >> >> Putting goto as the last line within a 'case' looks a bit strange. What >> about putting goto directly under a "if ( rc < 0 )" check earlier? >> >> if ( !rc ) >> ... >> if ( rc < 0 ) >> goto exit_and_crash; >> } >> else >> domain_pause_for_debugger(); >> break; > > Thanks, Kevin - indeed that's exactly what I had asked for already > on the previous iteration. > I'm OK with adding the rc < 0 case, it's just that the fall-through style is already used for handling the int3 events for example. Should I fix that too while I'm at it so the code is consistent? Tamas
>> { >> @@ -635,22 +661,22 @@ int main(int argc, char *argv[]) >> rsp.u.mem_access = req.u.mem_access; >> break; >> case VM_EVENT_REASON_SOFTWARE_BREAKPOINT: >> - printf("Breakpoint: rip=%016"PRIx64", gfn=%"PRIx64" (vcpu %d)\n", >> + printf("Breakpoint: rip=%016"PRIx64" gfn=%"PRIx64" (vcpu %d)\n", > > You seem to have removed the comma here (',') after the first "PRIx64", > but ... > >> req.data.regs.x86.rip, >> req.u.software_breakpoint.gfn, >> req.vcpu_id); >> >> /* Reinject */ >> - rc = xc_hvm_inject_trap( >> - xch, domain_id, req.vcpu_id, 3, >> - HVMOP_TRAP_sw_exc, -1, 0, 0); >> + rc = xc_hvm_inject_trap(xch, domain_id, req.vcpu_id, >> + X86_TRAP_INT3, >> + req.u.software_breakpoint.type, -1, >> + req.u.software_breakpoint.insn_length, 0); >> if (rc < 0) >> { >> ERROR("Error %d injecting breakpoint\n", rc); >> interrupted = -1; >> continue; >> } >> - >> break; >> case VM_EVENT_REASON_SINGLESTEP: >> printf("Singlestep: rip=%016"PRIx64", vcpu %d, altp2m %u\n", >> @@ -669,6 +695,27 @@ int main(int argc, char *argv[]) >> rsp.flags |= VM_EVENT_FLAG_TOGGLE_SINGLESTEP; >> >> break; >> + case VM_EVENT_REASON_DEBUG_EXCEPTION: >> + printf("Debug exception: rip=%016"PRIx64", vcpu %d. Type: %u. Length: %u\n", > > ... this newly added line uses the old style (with a comma after the > first "PRIx64"). Minor issue maybe, I just don't understand why the > first modification was made. Yea, for no reason. Will add the comma back above. > >> + req.data.regs.x86.rip, >> + req.vcpu_id, >> + req.u.debug_exception.type, >> + req.u.debug_exception.insn_length); >> + >> + /* Reinject */ >> + rc = xc_hvm_inject_trap(xch, domain_id, req.vcpu_id, >> + X86_TRAP_DEBUG, >> + req.u.debug_exception.type, -1, >> + req.u.debug_exception.insn_length, >> + req.data.regs.x86.cr2); >> + if (rc < 0) >> + { >> + ERROR("Error %d injecting breakpoint\n", rc); >> + interrupted = -1; >> + continue; >> + } >> + >> + break; >> default: >> fprintf(stderr, "UNKNOWN REASON CODE %d\n", req.reason); >> } >> diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c >> index 472926c..bbe5952 100644 >> --- a/xen/arch/x86/hvm/monitor.c >> +++ b/xen/arch/x86/hvm/monitor.c >> @@ -87,12 +87,13 @@ static inline unsigned long gfn_of_rip(unsigned long rip) >> return paging_gva_to_gfn(curr, sreg.base + rip, &pfec); >> } >> >> -int hvm_monitor_breakpoint(unsigned long rip, >> - enum hvm_monitor_breakpoint_type type) >> +int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type, >> + unsigned long trap_type, unsigned long insn_length) >> { >> struct vcpu *curr = current; >> struct arch_domain *ad = &curr->domain->arch; >> vm_event_request_t req = {}; >> + bool_t sync; >> >> switch ( type ) >> { >> @@ -101,6 +102,9 @@ int hvm_monitor_breakpoint(unsigned long rip, >> return 0; >> req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT; >> req.u.software_breakpoint.gfn = gfn_of_rip(rip); >> + req.u.software_breakpoint.type = trap_type; >> + req.u.software_breakpoint.insn_length = insn_length; >> + sync = 1; >> break; >> >> case HVM_MONITOR_SINGLESTEP_BREAKPOINT: >> @@ -108,6 +112,17 @@ int hvm_monitor_breakpoint(unsigned long rip, >> return 0; >> req.reason = VM_EVENT_REASON_SINGLESTEP; >> req.u.singlestep.gfn = gfn_of_rip(rip); >> + sync = 1; >> + break; >> + >> + case HVM_MONITOR_DEBUG_EXCEPTION: >> + if ( !ad->monitor.debug_exception_enabled ) >> + return 0; >> + req.reason = VM_EVENT_REASON_DEBUG_EXCEPTION; >> + req.u.debug_exception.gfn = gfn_of_rip(rip); >> + req.u.debug_exception.type = trap_type; >> + req.u.debug_exception.insn_length = insn_length; >> + sync = !!ad->monitor.debug_exception_sync; >> break; >> >> default: >> @@ -116,7 +131,7 @@ int hvm_monitor_breakpoint(unsigned long rip, >> >> req.vcpu_id = curr->vcpu_id; >> >> - return vm_event_monitor_traps(curr, 1, &req); >> + return vm_event_monitor_traps(curr, sync, &req); > > > What would be a basic use-case for this event to be sent without pausing > the VCPU (i.e. with sync != 1)? If you wish to passively monitor the occurrences of debug events in the guest you can use the !sync option. That will make Xen automatically reinject the event as it would normally but will send you a notification. I don't have a particular use for this but since Xen already had all the necessary wiring ready for it so we might as well make it available as an option, similar to the other !sync events. Tamas
On 06/24/16 21:48, Tamas K Lengyel wrote: >>> { >>> @@ -635,22 +661,22 @@ int main(int argc, char *argv[]) >>> rsp.u.mem_access = req.u.mem_access; >>> break; >>> case VM_EVENT_REASON_SOFTWARE_BREAKPOINT: >>> - printf("Breakpoint: rip=%016"PRIx64", gfn=%"PRIx64" (vcpu %d)\n", >>> + printf("Breakpoint: rip=%016"PRIx64" gfn=%"PRIx64" (vcpu %d)\n", >> >> You seem to have removed the comma here (',') after the first "PRIx64", >> but ... >> >>> req.data.regs.x86.rip, >>> req.u.software_breakpoint.gfn, >>> req.vcpu_id); >>> >>> /* Reinject */ >>> - rc = xc_hvm_inject_trap( >>> - xch, domain_id, req.vcpu_id, 3, >>> - HVMOP_TRAP_sw_exc, -1, 0, 0); >>> + rc = xc_hvm_inject_trap(xch, domain_id, req.vcpu_id, >>> + X86_TRAP_INT3, >>> + req.u.software_breakpoint.type, -1, >>> + req.u.software_breakpoint.insn_length, 0); >>> if (rc < 0) >>> { >>> ERROR("Error %d injecting breakpoint\n", rc); >>> interrupted = -1; >>> continue; >>> } >>> - >>> break; >>> case VM_EVENT_REASON_SINGLESTEP: >>> printf("Singlestep: rip=%016"PRIx64", vcpu %d, altp2m %u\n", >>> @@ -669,6 +695,27 @@ int main(int argc, char *argv[]) >>> rsp.flags |= VM_EVENT_FLAG_TOGGLE_SINGLESTEP; >>> >>> break; >>> + case VM_EVENT_REASON_DEBUG_EXCEPTION: >>> + printf("Debug exception: rip=%016"PRIx64", vcpu %d. Type: %u. Length: %u\n", >> >> ... this newly added line uses the old style (with a comma after the >> first "PRIx64"). Minor issue maybe, I just don't understand why the >> first modification was made. > > Yea, for no reason. Will add the comma back above. > >> >>> + req.data.regs.x86.rip, >>> + req.vcpu_id, >>> + req.u.debug_exception.type, >>> + req.u.debug_exception.insn_length); >>> + >>> + /* Reinject */ >>> + rc = xc_hvm_inject_trap(xch, domain_id, req.vcpu_id, >>> + X86_TRAP_DEBUG, >>> + req.u.debug_exception.type, -1, >>> + req.u.debug_exception.insn_length, >>> + req.data.regs.x86.cr2); >>> + if (rc < 0) >>> + { >>> + ERROR("Error %d injecting breakpoint\n", rc); >>> + interrupted = -1; >>> + continue; >>> + } >>> + >>> + break; >>> default: >>> fprintf(stderr, "UNKNOWN REASON CODE %d\n", req.reason); >>> } >>> diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c >>> index 472926c..bbe5952 100644 >>> --- a/xen/arch/x86/hvm/monitor.c >>> +++ b/xen/arch/x86/hvm/monitor.c >>> @@ -87,12 +87,13 @@ static inline unsigned long gfn_of_rip(unsigned long rip) >>> return paging_gva_to_gfn(curr, sreg.base + rip, &pfec); >>> } >>> >>> -int hvm_monitor_breakpoint(unsigned long rip, >>> - enum hvm_monitor_breakpoint_type type) >>> +int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type, >>> + unsigned long trap_type, unsigned long insn_length) >>> { >>> struct vcpu *curr = current; >>> struct arch_domain *ad = &curr->domain->arch; >>> vm_event_request_t req = {}; >>> + bool_t sync; >>> >>> switch ( type ) >>> { >>> @@ -101,6 +102,9 @@ int hvm_monitor_breakpoint(unsigned long rip, >>> return 0; >>> req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT; >>> req.u.software_breakpoint.gfn = gfn_of_rip(rip); >>> + req.u.software_breakpoint.type = trap_type; >>> + req.u.software_breakpoint.insn_length = insn_length; >>> + sync = 1; >>> break; >>> >>> case HVM_MONITOR_SINGLESTEP_BREAKPOINT: >>> @@ -108,6 +112,17 @@ int hvm_monitor_breakpoint(unsigned long rip, >>> return 0; >>> req.reason = VM_EVENT_REASON_SINGLESTEP; >>> req.u.singlestep.gfn = gfn_of_rip(rip); >>> + sync = 1; >>> + break; >>> + >>> + case HVM_MONITOR_DEBUG_EXCEPTION: >>> + if ( !ad->monitor.debug_exception_enabled ) >>> + return 0; >>> + req.reason = VM_EVENT_REASON_DEBUG_EXCEPTION; >>> + req.u.debug_exception.gfn = gfn_of_rip(rip); >>> + req.u.debug_exception.type = trap_type; >>> + req.u.debug_exception.insn_length = insn_length; >>> + sync = !!ad->monitor.debug_exception_sync; >>> break; >>> >>> default: >>> @@ -116,7 +131,7 @@ int hvm_monitor_breakpoint(unsigned long rip, >>> >>> req.vcpu_id = curr->vcpu_id; >>> >>> - return vm_event_monitor_traps(curr, 1, &req); >>> + return vm_event_monitor_traps(curr, sync, &req); >> >> >> What would be a basic use-case for this event to be sent without pausing >> the VCPU (i.e. with sync != 1)? > > If you wish to passively monitor the occurrences of debug events in > the guest you can use the !sync option. That will make Xen > automatically reinject the event as it would normally but will send > you a notification. I don't have a particular use for this but since > Xen already had all the necessary wiring ready for it so we might as > well make it available as an option, similar to the other !sync > events. Fair enough, thanks for the explanation. Thanks, Razvan
>>> On 24.06.16 at 18:29, <tamas@tklengyel.com> wrote: > On Fri, Jun 24, 2016 at 5:24 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 24.06.16 at 13:20, <kevin.tian@intel.com> wrote: >>>> From: Tamas K Lengyel [mailto:tamas@tklengyel.com] >>>> + * rc < error, fall-through to exit_and_crash >>>> + */ >>>> + if ( !rc ) >>>> + { >>>> + vmx_propagate_intr(intr_info); >>>> + break; >>>> + } >>>> + if ( rc > 0 ) >>>> + break; >>>> + } >>>> else >>>> + { >>>> domain_pause_for_debugger(); >>>> - break; >>>> + break; >>>> + } >>>> + >>>> + goto exit_and_crash; >>> >>> Putting goto as the last line within a 'case' looks a bit strange. What >>> about putting goto directly under a "if ( rc < 0 )" check earlier? >>> >>> if ( !rc ) >>> ... >>> if ( rc < 0 ) >>> goto exit_and_crash; >>> } >>> else >>> domain_pause_for_debugger(); >>> break; >> >> Thanks, Kevin - indeed that's exactly what I had asked for already >> on the previous iteration. >> > > I'm OK with adding the rc < 0 case, it's just that the fall-through > style is already used for handling the int3 events for example. Should > I fix that too while I'm at it so the code is consistent? Especially if you did it in a separate patch, I for one would appreciate that. Jan
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index 4f5d954..4a85b4a 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -2165,7 +2165,8 @@ int xc_monitor_software_breakpoint(xc_interface *xch, domid_t domain_id, bool enable); int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id, bool enable, bool sync); - +int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id, + bool enable, bool sync); /** * This function enables / disables emulation for each REP for a * REP-compatible instruction. diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c index 78131b2..264992c 100644 --- a/tools/libxc/xc_monitor.c +++ b/tools/libxc/xc_monitor.c @@ -156,3 +156,28 @@ int xc_monitor_emulate_each_rep(xc_interface *xch, domid_t domain_id, return do_domctl(xch, &domctl); } + +int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id, + bool enable, bool sync) +{ + DECLARE_DOMCTL; + + domctl.cmd = XEN_DOMCTL_monitor_op; + domctl.domain = domain_id; + domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE + : XEN_DOMCTL_MONITOR_OP_DISABLE; + domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION; + domctl.u.monitor_op.u.debug_exception.sync = sync; + + return do_domctl(xch, &domctl); +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c index f26e723..e615476 100644 --- a/tools/tests/xen-access/xen-access.c +++ b/tools/tests/xen-access/xen-access.c @@ -53,6 +53,10 @@ #define ERROR(a, b...) fprintf(stderr, a "\n", ## b) #define PERROR(a, b...) fprintf(stderr, a ": %s\n", ## b, strerror(errno)) +/* From xen/include/asm-x86/processor.h */ +#define X86_TRAP_DEBUG 1 +#define X86_TRAP_INT3 3 + typedef struct vm_event { domid_t domain_id; xenevtchn_handle *xce_handle; @@ -333,7 +337,7 @@ void usage(char* progname) { fprintf(stderr, "Usage: %s [-m] <domain_id> write|exec", progname); #if defined(__i386__) || defined(__x86_64__) - fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec"); + fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec|debug"); #endif fprintf(stderr, "\n" @@ -354,10 +358,12 @@ int main(int argc, char *argv[]) xc_interface *xch; xenmem_access_t default_access = XENMEM_access_rwx; xenmem_access_t after_first_access = XENMEM_access_rwx; + int memaccess = 0; int required = 0; int breakpoint = 0; int shutting_down = 0; int altp2m = 0; + int debug = 0; uint16_t altp2m_view_id = 0; char* progname = argv[0]; @@ -391,11 +397,13 @@ int main(int argc, char *argv[]) { default_access = XENMEM_access_rx; after_first_access = XENMEM_access_rwx; + memaccess = 1; } else if ( !strcmp(argv[0], "exec") ) { default_access = XENMEM_access_rw; after_first_access = XENMEM_access_rwx; + memaccess = 1; } #if defined(__i386__) || defined(__x86_64__) else if ( !strcmp(argv[0], "breakpoint") ) @@ -406,11 +414,17 @@ int main(int argc, char *argv[]) { default_access = XENMEM_access_rx; altp2m = 1; + memaccess = 1; } else if ( !strcmp(argv[0], "altp2m_exec") ) { default_access = XENMEM_access_rw; altp2m = 1; + memaccess = 1; + } + else if ( !strcmp(argv[0], "debug") ) + { + debug = 1; } #endif else @@ -446,7 +460,7 @@ int main(int argc, char *argv[]) } /* With altp2m we just create a new, restricted view of the memory */ - if ( altp2m ) + if ( memaccess && altp2m ) { xen_pfn_t gfn = 0; unsigned long perm_set = 0; @@ -493,7 +507,7 @@ int main(int argc, char *argv[]) } } - if ( !altp2m ) + if ( memaccess && !altp2m ) { /* Set the default access type and convert all pages to it */ rc = xc_set_mem_access(xch, domain_id, default_access, ~0ull, 0); @@ -524,6 +538,16 @@ int main(int argc, char *argv[]) } } + if ( debug ) + { + rc = xc_monitor_debug_exceptions(xch, domain_id, 1, 1); + if ( rc < 0 ) + { + ERROR("Error %d setting debug exception listener with vm_event\n", rc); + goto exit; + } + } + /* Wait for access */ for (;;) { @@ -534,6 +558,8 @@ int main(int argc, char *argv[]) if ( breakpoint ) rc = xc_monitor_software_breakpoint(xch, domain_id, 0); + if ( debug ) + rc = xc_monitor_debug_exceptions(xch, domain_id, 0, 0); if ( altp2m ) { @@ -635,22 +661,22 @@ int main(int argc, char *argv[]) rsp.u.mem_access = req.u.mem_access; break; case VM_EVENT_REASON_SOFTWARE_BREAKPOINT: - printf("Breakpoint: rip=%016"PRIx64", gfn=%"PRIx64" (vcpu %d)\n", + printf("Breakpoint: rip=%016"PRIx64" gfn=%"PRIx64" (vcpu %d)\n", req.data.regs.x86.rip, req.u.software_breakpoint.gfn, req.vcpu_id); /* Reinject */ - rc = xc_hvm_inject_trap( - xch, domain_id, req.vcpu_id, 3, - HVMOP_TRAP_sw_exc, -1, 0, 0); + rc = xc_hvm_inject_trap(xch, domain_id, req.vcpu_id, + X86_TRAP_INT3, + req.u.software_breakpoint.type, -1, + req.u.software_breakpoint.insn_length, 0); if (rc < 0) { ERROR("Error %d injecting breakpoint\n", rc); interrupted = -1; continue; } - break; case VM_EVENT_REASON_SINGLESTEP: printf("Singlestep: rip=%016"PRIx64", vcpu %d, altp2m %u\n", @@ -669,6 +695,27 @@ int main(int argc, char *argv[]) rsp.flags |= VM_EVENT_FLAG_TOGGLE_SINGLESTEP; break; + case VM_EVENT_REASON_DEBUG_EXCEPTION: + printf("Debug exception: rip=%016"PRIx64", vcpu %d. Type: %u. Length: %u\n", + req.data.regs.x86.rip, + req.vcpu_id, + req.u.debug_exception.type, + req.u.debug_exception.insn_length); + + /* Reinject */ + rc = xc_hvm_inject_trap(xch, domain_id, req.vcpu_id, + X86_TRAP_DEBUG, + req.u.debug_exception.type, -1, + req.u.debug_exception.insn_length, + req.data.regs.x86.cr2); + if (rc < 0) + { + ERROR("Error %d injecting breakpoint\n", rc); + interrupted = -1; + continue; + } + + break; default: fprintf(stderr, "UNKNOWN REASON CODE %d\n", req.reason); } diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c index 472926c..bbe5952 100644 --- a/xen/arch/x86/hvm/monitor.c +++ b/xen/arch/x86/hvm/monitor.c @@ -87,12 +87,13 @@ static inline unsigned long gfn_of_rip(unsigned long rip) return paging_gva_to_gfn(curr, sreg.base + rip, &pfec); } -int hvm_monitor_breakpoint(unsigned long rip, - enum hvm_monitor_breakpoint_type type) +int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type, + unsigned long trap_type, unsigned long insn_length) { struct vcpu *curr = current; struct arch_domain *ad = &curr->domain->arch; vm_event_request_t req = {}; + bool_t sync; switch ( type ) { @@ -101,6 +102,9 @@ int hvm_monitor_breakpoint(unsigned long rip, return 0; req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT; req.u.software_breakpoint.gfn = gfn_of_rip(rip); + req.u.software_breakpoint.type = trap_type; + req.u.software_breakpoint.insn_length = insn_length; + sync = 1; break; case HVM_MONITOR_SINGLESTEP_BREAKPOINT: @@ -108,6 +112,17 @@ int hvm_monitor_breakpoint(unsigned long rip, return 0; req.reason = VM_EVENT_REASON_SINGLESTEP; req.u.singlestep.gfn = gfn_of_rip(rip); + sync = 1; + break; + + case HVM_MONITOR_DEBUG_EXCEPTION: + if ( !ad->monitor.debug_exception_enabled ) + return 0; + req.reason = VM_EVENT_REASON_DEBUG_EXCEPTION; + req.u.debug_exception.gfn = gfn_of_rip(rip); + req.u.debug_exception.type = trap_type; + req.u.debug_exception.insn_length = insn_length; + sync = !!ad->monitor.debug_exception_sync; break; default: @@ -116,7 +131,7 @@ int hvm_monitor_breakpoint(unsigned long rip, req.vcpu_id = curr->vcpu_id; - return vm_event_monitor_traps(curr, 1, &req); + return vm_event_monitor_traps(curr, sync, &req); } /* diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 03fcba7..4b69ca6 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -3373,10 +3373,39 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) HVMTRACE_1D(TRAP_DEBUG, exit_qualification); write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE); if ( !v->domain->debugger_attached ) - vmx_propagate_intr(intr_info); + { + unsigned long insn_len = 0; + int rc; + unsigned long trap_type = MASK_EXTR(intr_info, + INTR_INFO_INTR_TYPE_MASK); + + if ( trap_type >= X86_EVENTTYPE_SW_INTERRUPT ) + __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_len); + + rc = hvm_monitor_debug(regs->eip, + HVM_MONITOR_DEBUG_EXCEPTION, + trap_type, insn_len); + + /* + * !rc continue normally + * rc > paused waiting for response, work here is done + * rc < error, fall-through to exit_and_crash + */ + if ( !rc ) + { + vmx_propagate_intr(intr_info); + break; + } + if ( rc > 0 ) + break; + } else + { domain_pause_for_debugger(); - break; + break; + } + + goto exit_and_crash; case TRAP_int3: { HVMTRACE_1D(TRAP, vector); @@ -3388,9 +3417,14 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) break; } else { - int rc = - hvm_monitor_breakpoint(regs->eip, - HVM_MONITOR_SOFTWARE_BREAKPOINT); + unsigned long insn_len; + int rc; + + __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_len); + rc = hvm_monitor_debug(regs->eip, + HVM_MONITOR_SOFTWARE_BREAKPOINT, + X86_EVENTTYPE_SW_EXCEPTION, + insn_len); if ( !rc ) { @@ -3398,11 +3432,8 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) .vector = TRAP_int3, .type = X86_EVENTTYPE_SW_EXCEPTION, .error_code = HVM_DELIVER_NO_ERROR_CODE, + .insn_len = insn_len }; - unsigned long insn_len; - - __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_len); - trap.insn_len = insn_len; hvm_inject_trap(&trap); break; } @@ -3717,8 +3748,10 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) vmx_update_cpu_exec_control(v); if ( v->arch.hvm_vcpu.single_step ) { - hvm_monitor_breakpoint(regs->eip, - HVM_MONITOR_SINGLESTEP_BREAKPOINT); + hvm_monitor_debug(regs->eip, + HVM_MONITOR_SINGLESTEP_BREAKPOINT, + 0, 0); + if ( v->domain->debugger_attached ) domain_pause_for_debugger(); } diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index a271161..205df41 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -224,6 +224,22 @@ int arch_monitor_domctl_event(struct domain *d, break; } + case XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION: + { + bool_t old_status = ad->monitor.debug_exception_enabled; + + if ( unlikely(old_status == requested_status) ) + return -EEXIST; + + domain_pause(d); + ad->monitor.debug_exception_enabled = requested_status; + ad->monitor.debug_exception_sync = requested_status ? + mop->u.debug_exception.sync : + 0; + domain_unpause(d); + break; + } + default: /* * Should not be reached unless arch_monitor_get_capabilities() is diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h index 7c27f9e..8f64ae9 100644 --- a/xen/include/asm-x86/domain.h +++ b/xen/include/asm-x86/domain.h @@ -403,6 +403,8 @@ struct arch_domain unsigned int write_ctrlreg_onchangeonly : 4; unsigned int singlestep_enabled : 1; unsigned int software_breakpoint_enabled : 1; + unsigned int debug_exception_enabled : 1; + unsigned int debug_exception_sync : 1; struct monitor_msr_bitmap *msr_bitmap; } monitor; diff --git a/xen/include/asm-x86/hvm/monitor.h b/xen/include/asm-x86/hvm/monitor.h index 55d435e..8b0f119 100644 --- a/xen/include/asm-x86/hvm/monitor.h +++ b/xen/include/asm-x86/hvm/monitor.h @@ -23,10 +23,11 @@ #include <xen/paging.h> #include <public/vm_event.h> -enum hvm_monitor_breakpoint_type +enum hvm_monitor_debug_type { HVM_MONITOR_SOFTWARE_BREAKPOINT, HVM_MONITOR_SINGLESTEP_BREAKPOINT, + HVM_MONITOR_DEBUG_EXCEPTION, }; /* @@ -39,8 +40,8 @@ bool_t hvm_monitor_cr(unsigned int index, unsigned long value, #define hvm_monitor_crX(cr, new, old) \ hvm_monitor_cr(VM_EVENT_X86_##cr, new, old) void hvm_monitor_msr(unsigned int msr, uint64_t value); -int hvm_monitor_breakpoint(unsigned long rip, - enum hvm_monitor_breakpoint_type type); +int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type, + unsigned long trap_type, unsigned long insn_length); #endif /* __ASM_X86_HVM_MONITOR_H__ */ diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h index 94b6768..a9db3c0 100644 --- a/xen/include/asm-x86/monitor.h +++ b/xen/include/asm-x86/monitor.h @@ -77,7 +77,8 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d) capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) | (1U << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) | (1U << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) | - (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST); + (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST) | + (1U << XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION); /* Since we know this is on VMX, we can just call the hvm func */ if ( hvm_is_singlestep_supported() ) diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 7be3924..30020ba 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -1080,6 +1080,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t); #define XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP 2 #define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT 3 #define XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST 4 +#define XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION 5 struct xen_domctl_monitor_op { uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */ @@ -1114,6 +1115,11 @@ struct xen_domctl_monitor_op { /* Pause vCPU until response */ uint8_t sync; } guest_request; + + struct { + /* Pause vCPU until response */ + uint8_t sync; + } debug_exception; } u; }; typedef struct xen_domctl_monitor_op xen_domctl_monitor_op_t; diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h index 9270d52..68bddfb 100644 --- a/xen/include/public/vm_event.h +++ b/xen/include/public/vm_event.h @@ -29,7 +29,7 @@ #include "xen.h" -#define VM_EVENT_INTERFACE_VERSION 0x00000001 +#define VM_EVENT_INTERFACE_VERSION 0x00000002 #if defined(__XEN__) || defined(__XEN_TOOLS__) @@ -119,6 +119,8 @@ #define VM_EVENT_REASON_SINGLESTEP 7 /* An event has been requested via HVMOP_guest_request_vm_event. */ #define VM_EVENT_REASON_GUEST_REQUEST 8 +/* A debug exception was caught */ +#define VM_EVENT_REASON_DEBUG_EXCEPTION 9 /* Supported values for the vm_event_write_ctrlreg index. */ #define VM_EVENT_X86_CR0 0 @@ -203,8 +205,15 @@ struct vm_event_write_ctrlreg { uint64_t old_value; }; +struct vm_event_singlestep { + uint64_t gfn; +}; + struct vm_event_debug { uint64_t gfn; + uint32_t insn_length; + uint8_t type; /* HVMOP_TRAP_* */ + uint8_t _pad[3]; }; struct vm_event_mov_to_msr { @@ -247,8 +256,9 @@ typedef struct vm_event_st { struct vm_event_mem_access mem_access; struct vm_event_write_ctrlreg write_ctrlreg; struct vm_event_mov_to_msr mov_to_msr; + struct vm_event_singlestep singlestep; struct vm_event_debug software_breakpoint; - struct vm_event_debug singlestep; + struct vm_event_debug debug_exception; } u; union {
Since in-guest debug exceptions are now unconditionally trapped to Xen, adding a hook for vm_event subscribers to tap into this new always-on guest event. We rename along the way hvm_event_breakpoint_type to hvm_event_type to better match the various events that can be passed with it. We also introduce the necessary monitor_op domctl's to enable subscribing to the events. This patch also provides monitor subscribers to int3 events proper access to the instruction length necessary for accurate event-reinjection. Without this subscribers manually have to evaluate if the int3 instruction has any prefix attached which would change the instruction length. Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Jun Nakajima <jun.nakajima@intel.com> Cc: Kevin Tian <kevin.tian@intel.com> v6: Add comment describing rc condition values for the monitor call v5: Transmit the proper insn_length for int3 events as well to fix the current bug with prefixed int3 instructions. --- tools/libxc/include/xenctrl.h | 3 +- tools/libxc/xc_monitor.c | 25 +++++++++++++++ tools/tests/xen-access/xen-access.c | 63 ++++++++++++++++++++++++++++++++----- xen/arch/x86/hvm/monitor.c | 21 +++++++++++-- xen/arch/x86/hvm/vmx/vmx.c | 55 +++++++++++++++++++++++++------- xen/arch/x86/monitor.c | 16 ++++++++++ xen/include/asm-x86/domain.h | 2 ++ xen/include/asm-x86/hvm/monitor.h | 7 +++-- xen/include/asm-x86/monitor.h | 3 +- xen/include/public/domctl.h | 6 ++++ xen/include/public/vm_event.h | 14 +++++++-- 11 files changed, 186 insertions(+), 29 deletions(-)