[PATCH v1] hvm/svm: Implement Debug events

[PATCH v1] hvm/svm: Implement Debug events

[Alexandru Isaila]

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"

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
 xen/arch/x86/hvm/svm/svm.c    | 41 +++++++++++++++++++++++++++++------------
 xen/arch/x86/hvm/svm/vmcb.c   |  2 +-
 xen/include/asm-x86/monitor.h |  4 ++--
 3 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c34f5b5..aa1feaa 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,16 +2655,33 @@ 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 = vmcb->nextrip - vmcb->rip;
+
+            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;
 
-    case VMEXIT_EXCEPTION_BP:
-        inst_len = __get_instruction_length(v, INSTR_INT3);
+    case VMEXIT_EXCEPTION_BP:;
+        inst_len = vmcb->nextrip - vmcb->rip;
 
         if ( inst_len == 0 )
              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/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

Re: [PATCH v1] hvm/svm: Implement Debug events

[Andrew Cooper]

On 19/03/18 14:07, Alexandru Isaila wrote:
> -    case VMEXIT_EXCEPTION_BP:
> -        inst_len = __get_instruction_length(v, INSTR_INT3);
> +    case VMEXIT_EXCEPTION_BP:;
> +        inst_len = vmcb->nextrip - vmcb->rip;

Sorry, but no.  This will break on older AMD hardware.  You must retain
the __get_instruction_length().

NextRIP support was only introduced in Gen2 SVM, and we still support Gen1.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] hvm/svm: Implement Debug events

[Tamas K Lengyel]

On Mon, Mar 19, 2018 at 8:07 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"
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> ---
>  xen/arch/x86/hvm/svm/svm.c    | 41 +++++++++++++++++++++++++++++------------
>  xen/arch/x86/hvm/svm/vmcb.c   |  2 +-
>  xen/include/asm-x86/monitor.h |  4 ++--
>  3 files changed, 32 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index c34f5b5..aa1feaa 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);

I'm not sure this should be bundled under "debug_exception", on Intel
this event type usually gets you things like singlestepping. To me
ICEBP sounds like it would better fit under "software_breakpoint".
Thoughts?

>      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,16 +2655,33 @@ 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 = vmcb->nextrip - vmcb->rip;
> +
> +            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;
>
> -    case VMEXIT_EXCEPTION_BP:
> -        inst_len = __get_instruction_length(v, INSTR_INT3);
> +    case VMEXIT_EXCEPTION_BP:;
> +        inst_len = vmcb->nextrip - vmcb->rip;
>
>          if ( inst_len == 0 )
>               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/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

Re: [PATCH v1] hvm/svm: Implement Debug events

[Razvan Cojocaru]

On 03/19/2018 04:48 PM, Tamas K Lengyel wrote:
> On Mon, Mar 19, 2018 at 8:07 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"
>>
>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>> ---
>>  xen/arch/x86/hvm/svm/svm.c    | 41 +++++++++++++++++++++++++++++------------
>>  xen/arch/x86/hvm/svm/vmcb.c   |  2 +-
>>  xen/include/asm-x86/monitor.h |  4 ++--
>>  3 files changed, 32 insertions(+), 15 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>> index c34f5b5..aa1feaa 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);
> 
> I'm not sure this should be bundled under "debug_exception", on Intel
> this event type usually gets you things like singlestepping. To me
> ICEBP sounds like it would better fit under "software_breakpoint".
> Thoughts?
I also found it a bit curious that ICEBP is under the DEBUG vmexit with
VMX/Intel, but that's how it currently goes (you can see this easily by
running the swint-emulation XTF test). Xen-access will get this as a
debug (not breakpoint) event.

So if we move this, we'd need to also move it on VMX, for consistency.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] hvm/svm: Implement Debug events

[Alexandru Stefan ISAILA]

On Lu, 2018-03-19 at 14:22 +0000, Andrew Cooper wrote:
> On 19/03/18 14:07, Alexandru Isaila wrote:
> > 
> > -    case VMEXIT_EXCEPTION_BP:
> > -        inst_len = __get_instruction_length(v, INSTR_INT3);
> > +    case VMEXIT_EXCEPTION_BP:;
> > +        inst_len = vmcb->nextrip - vmcb->rip;
> Sorry, but no.  This will break on older AMD hardware.  You must
> retain
> the __get_instruction_length().
> 
> NextRIP support was only introduced in Gen2 SVM, and we still support
> Gen1.
> 
> ~Andrew
Yes, you are right, I will re-use the __get_instruction_length() and
add support for the ICEBP instruction fort the next version of the
patch. Just did some tests and it works fine for the int3/int $3
instruction length.

Thanks for the heads-up. 

~Alex 
> 
> ________________________
> This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel