| Message ID | 1464907946-19242-8-git-send-email-tamas@tklengyel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 03.06.16 at 00:52, <tamas@tklengyel.com> wrote: > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -3377,10 +3377,33 @@ 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_length = 0; It's insn_len further down - please try to be consistent. > + 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_length); > + > + rc = hvm_monitor_debug(regs->eip, > + HVM_MONITOR_DEBUG_EXCEPTION, > + trap_type, insn_length); > + if ( !rc ) > + { > + vmx_propagate_intr(intr_info); > + break; > + } > + else if ( rc > 0 ) > + break; So you've removed the odd / hard to understand return value adjustment from hvm_monitor_debug(), but this isn't any better: What does the return value being positive really mean? And btw., no point using "else" after an unconditional "break" in the previous if(). > + } > else > + { > domain_pause_for_debugger(); > - break; > + break; > + } > + > + goto exit_and_crash; There was no such goto before, i.e. you introduce this. I'm rather hesitant to see such getting added without a good reason, and that good reason should be stated in a comment. Also it looks like the change would be easier to grok if you didn't alter the code down here, but instead inverted the earlier if: if ( unlikely(rc < 0) ) /* ... */ goto exit_and_crash; if ( !rc ) vmx_propagate_intr(intr_info); Which imo would get us closer to code being at least half way self-explanatory. Jan
On Jun 3, 2016 04:49, "Jan Beulich" <JBeulich@suse.com> wrote: > > >>> On 03.06.16 at 00:52, <tamas@tklengyel.com> wrote: > > --- a/xen/arch/x86/hvm/vmx/vmx.c > > +++ b/xen/arch/x86/hvm/vmx/vmx.c > > @@ -3377,10 +3377,33 @@ 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_length = 0; > > It's insn_len further down - please try to be consistent. > > > + 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_length); > > + > > + rc = hvm_monitor_debug(regs->eip, > > + HVM_MONITOR_DEBUG_EXCEPTION, > > + trap_type, insn_length); > > + if ( !rc ) > > + { > > + vmx_propagate_intr(intr_info); > > + break; > > + } > > + else if ( rc > 0 ) > > + break; > > So you've removed the odd / hard to understand return value > adjustment from hvm_monitor_debug(), but this isn't any better: > What does the return value being positive really mean? And btw., > no point using "else" after an unconditional "break" in the previous > if(). > As the commit message explains in the other patch rc is 1 when the vCPU is paused. This means a synchronous event where we are waiting for the vm_event response thus work here is done. > > + } > > else > > + { > > domain_pause_for_debugger(); > > - break; > > + break; > > + } > > + > > + goto exit_and_crash; > > There was no such goto before, i.e. you introduce this. I'm rather > hesitant to see such getting added without a good reason, and > that good reason should be stated in a comment. Also it looks like > the change would be easier to grok if you didn't alter the code > down here, but instead inverted the earlier if: > > if ( unlikely(rc < 0) ) > /* ... */ > goto exit_and_crash; > if ( !rc ) > vmx_propagate_intr(intr_info); > > Which imo would get us closer to code being at least half way > self-explanatory. > I agree it may be more intuitive that way but adding the goto the way I did is whats consistent with the already established handling of int3 events. I either go for consistency or reworking more code at other spots too. Tamas
>>> On 03.06.16 at 15:29, <tamas@tklengyel.com> wrote: > On Jun 3, 2016 04:49, "Jan Beulich" <JBeulich@suse.com> wrote: >> >> >>> On 03.06.16 at 00:52, <tamas@tklengyel.com> wrote: >> > --- a/xen/arch/x86/hvm/vmx/vmx.c >> > +++ b/xen/arch/x86/hvm/vmx/vmx.c >> > @@ -3377,10 +3377,33 @@ 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_length = 0; >> >> It's insn_len further down - please try to be consistent. >> >> > + 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_length); >> > + >> > + rc = hvm_monitor_debug(regs->eip, >> > + HVM_MONITOR_DEBUG_EXCEPTION, >> > + trap_type, insn_length); >> > + if ( !rc ) >> > + { >> > + vmx_propagate_intr(intr_info); >> > + break; >> > + } >> > + else if ( rc > 0 ) >> > + break; >> >> So you've removed the odd / hard to understand return value >> adjustment from hvm_monitor_debug(), but this isn't any better: >> What does the return value being positive really mean? And btw., >> no point using "else" after an unconditional "break" in the previous >> if(). > > As the commit message explains in the other patch rc is 1 when the vCPU is > paused. This means a synchronous event where we are waiting for the > vm_event response thus work here is done. The commit message of _another_ patch doesn't help at all a future reader to understand what's going on here. >> > + } >> > else >> > + { >> > domain_pause_for_debugger(); >> > - break; >> > + break; >> > + } >> > + >> > + goto exit_and_crash; >> >> There was no such goto before, i.e. you introduce this. I'm rather >> hesitant to see such getting added without a good reason, and >> that good reason should be stated in a comment. Also it looks like >> the change would be easier to grok if you didn't alter the code >> down here, but instead inverted the earlier if: >> >> if ( unlikely(rc < 0) ) >> /* ... */ >> goto exit_and_crash; >> if ( !rc ) >> vmx_propagate_intr(intr_info); >> >> Which imo would get us closer to code being at least half way >> self-explanatory. > > I agree it may be more intuitive that way but adding the goto the way I did > is whats consistent with the already established handling of int3 events. I > either go for consistency or reworking more code at other spots too. Well, as always I'll leave it to the maintainers to decide, but I think my suggestion would make this code better readable, and doesn't require immediate re-work elsewhere. Jan
On Jun 3, 2016 08:23, "Jan Beulich" <JBeulich@suse.com> wrote: > > >>> On 03.06.16 at 15:29, <tamas@tklengyel.com> wrote: > > On Jun 3, 2016 04:49, "Jan Beulich" <JBeulich@suse.com> wrote: > >> > >> >>> On 03.06.16 at 00:52, <tamas@tklengyel.com> wrote: > >> > --- a/xen/arch/x86/hvm/vmx/vmx.c > >> > +++ b/xen/arch/x86/hvm/vmx/vmx.c > >> > @@ -3377,10 +3377,33 @@ 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_length = 0; > >> > >> It's insn_len further down - please try to be consistent. > >> > >> > + 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_length); > >> > + > >> > + rc = hvm_monitor_debug(regs->eip, > >> > + HVM_MONITOR_DEBUG_EXCEPTION, > >> > + trap_type, insn_length); > >> > + if ( !rc ) > >> > + { > >> > + vmx_propagate_intr(intr_info); > >> > + break; > >> > + } > >> > + else if ( rc > 0 ) > >> > + break; > >> > >> So you've removed the odd / hard to understand return value > >> adjustment from hvm_monitor_debug(), but this isn't any better: > >> What does the return value being positive really mean? And btw., > >> no point using "else" after an unconditional "break" in the previous > >> if(). > > > > As the commit message explains in the other patch rc is 1 when the vCPU is > > paused. This means a synchronous event where we are waiting for the > > vm_event response thus work here is done. > > The commit message of _another_ patch doesn't help at all a future > reader to understand what's going on here. This is already used elsewhere in similar fashion so I don't see why we would need to treat this case any differently. Its not like I'm introducing a totally new way of doing this. So IMHO adding a comment would be an OK middle ground but my goal is really not to rework everything. > >> > + } > >> > else > >> > + { > >> > domain_pause_for_debugger(); > >> > - break; > >> > + break; > >> > + } > >> > + > >> > + goto exit_and_crash; > >> > >> There was no such goto before, i.e. you introduce this. I'm rather > >> hesitant to see such getting added without a good reason, and > >> that good reason should be stated in a comment. Also it looks like > >> the change would be easier to grok if you didn't alter the code > >> down here, but instead inverted the earlier if: > >> > >> if ( unlikely(rc < 0) ) > >> /* ... */ > >> goto exit_and_crash; > >> if ( !rc ) > >> vmx_propagate_intr(intr_info); > >> > >> Which imo would get us closer to code being at least half way > >> self-explanatory. > > > > I agree it may be more intuitive that way but adding the goto the way I did > > is whats consistent with the already established handling of int3 events. I > > either go for consistency or reworking more code at other spots too. > > Well, as always I'll leave it to the maintainers to decide, but I think > my suggestion would make this code better readable, and doesn't > require immediate re-work elsewhere. > If we are aiming for consistency then I think it does. Lets hear from the maintainers and will go from there. I rather not start reworking preexisting stuff because it tends to snowball into a lot more patches. Tamas
>>> On 03.06.16 at 16:34, <tamas@tklengyel.com> wrote: > On Jun 3, 2016 08:23, "Jan Beulich" <JBeulich@suse.com> wrote: >> >> >>> On 03.06.16 at 15:29, <tamas@tklengyel.com> wrote: >> > On Jun 3, 2016 04:49, "Jan Beulich" <JBeulich@suse.com> wrote: >> >> >> >> >>> On 03.06.16 at 00:52, <tamas@tklengyel.com> wrote: >> >> > --- a/xen/arch/x86/hvm/vmx/vmx.c >> >> > +++ b/xen/arch/x86/hvm/vmx/vmx.c >> >> > @@ -3377,10 +3377,33 @@ 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_length = 0; >> >> >> >> It's insn_len further down - please try to be consistent. >> >> >> >> > + 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_length); >> >> > + >> >> > + rc = hvm_monitor_debug(regs->eip, >> >> > + HVM_MONITOR_DEBUG_EXCEPTION, >> >> > + trap_type, insn_length); >> >> > + if ( !rc ) >> >> > + { >> >> > + vmx_propagate_intr(intr_info); >> >> > + break; >> >> > + } >> >> > + else if ( rc > 0 ) >> >> > + break; >> >> >> >> So you've removed the odd / hard to understand return value >> >> adjustment from hvm_monitor_debug(), but this isn't any better: >> >> What does the return value being positive really mean? And btw., >> >> no point using "else" after an unconditional "break" in the previous >> >> if(). >> > >> > As the commit message explains in the other patch rc is 1 when the vCPU is >> > paused. This means a synchronous event where we are waiting for the >> > vm_event response thus work here is done. >> >> The commit message of _another_ patch doesn't help at all a future >> reader to understand what's going on here. > > This is already used elsewhere in similar fashion so I don't see why we > would need to treat this case any differently. Its not like I'm introducing > a totally new way of doing this. So IMHO adding a comment would be an OK > middle ground but my goal is really not to rework everything. Nothing but a comment was what I was hoping for. And then later, in the remark regarding the odd code structure further down, I did say "Which imo would get us closer to code being at least half way self-explanatory," to indicate that if that adjustment was done, perhaps a comment may not even be needed. Jan
On Jun 3, 2016 08:45, "Jan Beulich" <JBeulich@suse.com> wrote: > > >>> On 03.06.16 at 16:34, <tamas@tklengyel.com> wrote: > > On Jun 3, 2016 08:23, "Jan Beulich" <JBeulich@suse.com> wrote: > >> > >> >>> On 03.06.16 at 15:29, <tamas@tklengyel.com> wrote: > >> > On Jun 3, 2016 04:49, "Jan Beulich" <JBeulich@suse.com> wrote: > >> >> > >> >> >>> On 03.06.16 at 00:52, <tamas@tklengyel.com> wrote: > >> >> > --- a/xen/arch/x86/hvm/vmx/vmx.c > >> >> > +++ b/xen/arch/x86/hvm/vmx/vmx.c > >> >> > @@ -3377,10 +3377,33 @@ 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_length = 0; > >> >> > >> >> It's insn_len further down - please try to be consistent. > >> >> > >> >> > + 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_length); > >> >> > + > >> >> > + rc = hvm_monitor_debug(regs->eip, > >> >> > + HVM_MONITOR_DEBUG_EXCEPTION, > >> >> > + trap_type, insn_length); > >> >> > + if ( !rc ) > >> >> > + { > >> >> > + vmx_propagate_intr(intr_info); > >> >> > + break; > >> >> > + } > >> >> > + else if ( rc > 0 ) > >> >> > + break; > >> >> > >> >> So you've removed the odd / hard to understand return value > >> >> adjustment from hvm_monitor_debug(), but this isn't any better: > >> >> What does the return value being positive really mean? And btw., > >> >> no point using "else" after an unconditional "break" in the previous > >> >> if(). > >> > > >> > As the commit message explains in the other patch rc is 1 when the vCPU is > >> > paused. This means a synchronous event where we are waiting for the > >> > vm_event response thus work here is done. > >> > >> The commit message of _another_ patch doesn't help at all a future > >> reader to understand what's going on here. > > > > This is already used elsewhere in similar fashion so I don't see why we > > would need to treat this case any differently. Its not like I'm introducing > > a totally new way of doing this. So IMHO adding a comment would be an OK > > middle ground but my goal is really not to rework everything. > > Nothing but a comment was what I was hoping for. And then later, > in the remark regarding the odd code structure further down, I did > say "Which imo would get us closer to code being at least half way > self-explanatory," to indicate that if that adjustment was done, > perhaps a comment may not even be needed. > Ack. I have nothing against adding a comment here. Tamas
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index 3085130..29d983e 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -2162,7 +2162,8 @@ int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id, bool enable, bool sync); int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id, bool enable); - +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 1e4a9d2..b0bf708 100644 --- a/tools/libxc/xc_monitor.c +++ b/tools/libxc/xc_monitor.c @@ -171,6 +171,21 @@ int xc_monitor_privileged_call(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 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 764c3e8..8458627 100644 --- a/xen/arch/x86/hvm/monitor.c +++ b/xen/arch/x86/hvm/monitor.c @@ -88,12 +88,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 ) { @@ -102,6 +103,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: @@ -109,6 +113,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: @@ -117,7 +132,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 4981574..17cf1f7 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -3377,10 +3377,33 @@ 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_length = 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_length); + + rc = hvm_monitor_debug(regs->eip, + HVM_MONITOR_DEBUG_EXCEPTION, + trap_type, insn_length); + if ( !rc ) + { + vmx_propagate_intr(intr_info); + break; + } + else if ( rc > 0 ) + break; + } else + { domain_pause_for_debugger(); - break; + break; + } + + goto exit_and_crash; case TRAP_int3: { HVMTRACE_1D(TRAP, vector); @@ -3392,9 +3415,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 ) { @@ -3403,9 +3431,6 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) .type = X86_EVENTTYPE_SW_EXCEPTION, .error_code = HVM_DELIVER_NO_ERROR_CODE, }; - unsigned long insn_len; - - __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_len); trap.insn_len = insn_len; hvm_inject_trap(&trap); break; @@ -3721,8 +3746,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 621f91a..e8b79f6 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -124,6 +124,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 165e533..1cf97c3 100644 --- a/xen/include/asm-x86/domain.h +++ b/xen/include/asm-x86/domain.h @@ -405,6 +405,8 @@ struct arch_domain unsigned int mov_to_msr_extended : 1; unsigned int singlestep_enabled : 1; unsigned int software_breakpoint_enabled : 1; + unsigned int debug_exception_enabled : 1; + unsigned int debug_exception_sync : 1; } monitor; /* Mem_access emulation control */ 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 0fee750..74a08f0f 100644 --- a/xen/include/asm-x86/monitor.h +++ b/xen/include/asm-x86/monitor.h @@ -74,7 +74,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 35adce2..b69b1bb 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -1081,6 +1081,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t); #define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT 3 #define XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST 4 #define XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL 5 +#define XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION 6 struct xen_domctl_monitor_op { uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */ @@ -1116,6 +1117,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 31801b2..729b73d 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__) @@ -121,6 +121,8 @@ #define VM_EVENT_REASON_GUEST_REQUEST 8 /* Privileged call executed (e.g. SMC) */ #define VM_EVENT_REASON_PRIVILEGED_CALL 9 +/* A debug exception was caught */ +#define VM_EVENT_REASON_DEBUG_EXCEPTION 10 /* Supported values for the vm_event_write_ctrlreg index. */ #define VM_EVENT_X86_CR0 0 @@ -268,8 +270,15 @@ struct vm_event_write_ctrlreg { uint64_t old_value; }; +struct vm_event_singlestep { + uint64_t gfn; +}; + struct vm_event_debug { uint64_t gfn; + uint8_t type; /* HVMOP_TRAP_* */ + uint8_t insn_length; + uint8_t _pad[6]; }; struct vm_event_mov_to_msr { @@ -324,7 +333,8 @@ typedef struct vm_event_st { struct vm_event_write_ctrlreg write_ctrlreg; struct vm_event_mov_to_msr mov_to_msr; struct vm_event_debug software_breakpoint; - struct vm_event_debug singlestep; + struct vm_event_singlestep singlestep; + struct vm_event_debug debug_exception; struct vm_event_privcall privcall; } u;
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. 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> 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 | 15 +++++++++ tools/tests/xen-access/xen-access.c | 63 ++++++++++++++++++++++++++++++++----- xen/arch/x86/hvm/monitor.c | 21 +++++++++++-- xen/arch/x86/hvm/vmx/vmx.c | 47 +++++++++++++++++++++------ 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, 169 insertions(+), 28 deletions(-)