[PATCH v2] hvm/svm: Implement Debug events

[PATCH v2] 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"

---
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

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

[Andrew Cooper]

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

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

[Jan Beulich]

>>> 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

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

[Andrew Cooper]

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

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

[Alexandru Stefan ISAILA]

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

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

[Jan Beulich]

>>> 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

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

[Tamas K Lengyel]

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

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

[Andrew Cooper]

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

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

[Alexandru Stefan ISAILA]

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

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

[Tamas K Lengyel]

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