[PATCH v1 0/4] hvm/svm: Enable vm events for SVM

[PATCH v1 0/4] hvm/svm: Enable vm events for SVM

[Alexandru Isaila]

Hi all,

This series provides a skeleton for enabling vm_events on SVM. For the
first step, the MSR, CR, Breakpoint and GuestRequest have been tested
and added to the capabilities list.

Cheers,

Alexandru Isaila

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

[PATCH v1 1/4] asm-x86/monitor: Enable svm monitor events

[Alexandru Isaila]

This commit separates the svm caps from the vmx caps.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
 xen/include/asm-x86/monitor.h | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index a0444d1..3706b7a 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -74,21 +74,28 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
      * At the moment only Intel HVM domains are supported. However, event
      * delivery could be extended to AMD and PV domains.
      */
-    if ( !is_hvm_domain(d) || !cpu_has_vmx )
+    if ( !is_hvm_domain(d) )
         return capabilities;
 
-    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_DEBUG_EXCEPTION) |
-                   (1U << XEN_DOMCTL_MONITOR_EVENT_CPUID) |
-                   (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT) |
-                   (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() )
-        capabilities |= (1U << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
+    if( cpu_has_vmx )
+    {
+        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_DEBUG_EXCEPTION) |
+                       (1U << XEN_DOMCTL_MONITOR_EVENT_CPUID) |
+                       (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT) |
+                       (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() )
+            capabilities |= (1U << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
+    }
+    else if ( cpu_has_svm )
+    {
+        capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
+    }
 
     if ( hvm_funcs.set_descriptor_access_exiting )
         capabilities |= (1U << XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS);
-- 
2.7.4


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

[PATCH v1 2/4] hvm/svm: Enable Breakpoint events

[Alexandru Isaila]

This commit enables the breakpoint events for svm.

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

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index dcbd550..14a5f60 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -59,6 +59,7 @@
 #include <asm/hap.h>
 #include <asm/apic.h>
 #include <asm/debugger.h>
+#include <asm/hvm/monitor.h>
 #include <asm/xstate.h>
 
 void svm_asm_do_resume(void);
@@ -1079,7 +1080,8 @@ static void svm_ctxt_switch_to(struct vcpu *v)
 static void noreturn svm_do_resume(struct vcpu *v)
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
-    bool_t debug_state = v->domain->debugger_attached;
+    bool_t debug_state = v->domain->debugger_attached
+                || v->domain->arch.monitor.software_breakpoint_enabled;
     bool_t vcpu_guestmode = 0;
     struct vlapic *vlapic = vcpu_vlapic(v);
 
@@ -2407,6 +2409,23 @@ 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)
+{
+    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,
+    };
+
+    if ( event.type >= X86_EVENTTYPE_SW_INTERRUPT )
+        event.insn_len = insn_len;
+    else
+        event.insn_len = 0;
+
+    hvm_inject_event(&event);
+}
+
 static struct hvm_function_table __initdata svm_function_table = {
     .name                 = "SVM",
     .cpu_up_prepare       = svm_cpu_up_prepare,
@@ -2619,14 +2638,31 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
         break;
 
     case VMEXIT_EXCEPTION_BP:
-        if ( !v->domain->debugger_attached )
-            goto unexpected_exit_type;
-        /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not update RIP. */
-        if ( (inst_len = __get_instruction_length(v, INSTR_INT3)) == 0 )
+        inst_len = __get_instruction_length(v, INSTR_INT3);
+
+        if ( inst_len == 0 )
             break;
-        __update_guest_eip(regs, inst_len);
-        current->arch.gdbsx_vcpu_event = TRAP_int3;
-        domain_pause_for_debugger();
+
+        if ( !v->domain->debugger_attached )
+        {
+         /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not update RIP. */
+            int rc;
+
+            rc = hvm_monitor_debug(regs->rip,
+                                   HVM_MONITOR_SOFTWARE_BREAKPOINT,
+                                   X86_EVENTTYPE_SW_EXCEPTION,
+                                   inst_len);
+            if ( rc < 0 )
+                goto unexpected_exit_type;
+            if ( !rc )
+                svm_propagate_intr(v, inst_len);
+        }
+        else
+        {
+            __update_guest_eip(regs, inst_len);
+            current->arch.gdbsx_vcpu_event = TRAP_int3;
+            domain_pause_for_debugger();
+        }
         break;
 
     case VMEXIT_EXCEPTION_NM:
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 3706b7a..68a210a 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -94,7 +94,8 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
     }
     else if ( cpu_has_svm )
     {
-        capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
+        capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST) |
+                       (1U << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT);
     }
 
     if ( hvm_funcs.set_descriptor_access_exiting )
-- 
2.7.4


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

[PATCH v1 3/4] hvm/svm: Enable MSR events

[Alexandru Isaila]

This commit enables MSR events for svm.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
 xen/arch/x86/hvm/svm/svm.c    | 9 +++++++++
 xen/include/asm-x86/monitor.h | 3 ++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 14a5f60..dc3ad0b 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -163,6 +163,14 @@ void svm_intercept_msr(struct vcpu *v, uint32_t msr, int flags)
         __clear_bit(msr * 2 + 1, msr_bit);
 }
 
+static void svm_enable_msr_interception(struct domain *d, uint32_t msr)
+{
+    struct vcpu *v;
+
+    for_each_vcpu ( d, v )
+        svm_intercept_msr(v, msr, MSR_INTERCEPT_WRITE);
+}
+
 static void svm_save_dr(struct vcpu *v)
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
@@ -2464,6 +2472,7 @@ static struct hvm_function_table __initdata svm_function_table = {
     .fpu_dirty_intercept  = svm_fpu_dirty_intercept,
     .msr_read_intercept   = svm_msr_read_intercept,
     .msr_write_intercept  = svm_msr_write_intercept,
+    .enable_msr_interception = svm_enable_msr_interception,
     .set_rdtsc_exiting    = svm_set_rdtsc_exiting,
     .set_descriptor_access_exiting = svm_set_descriptor_access_exiting,
     .get_insn_bytes       = svm_get_insn_bytes,
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 68a210a..4d8887e 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -95,7 +95,8 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
     else if ( cpu_has_svm )
     {
         capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST) |
-                       (1U << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT);
+                       (1U << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) |
+                       (1U << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR);
     }
 
     if ( hvm_funcs.set_descriptor_access_exiting )
-- 
2.7.4


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

[PATCH v1 4/4] hvm/svm: Enable CR events

[Alexandru Isaila]

This commit enables controlregister events for svm.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
 xen/arch/x86/hvm/svm/svm.c    | 11 +++++++++++
 xen/include/asm-x86/monitor.h |  3 ++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index dc3ad0b..4b34586 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -60,6 +60,7 @@
 #include <asm/apic.h>
 #include <asm/debugger.h>
 #include <asm/hvm/monitor.h>
+#include <asm/monitor.h>
 #include <asm/xstate.h>
 
 void svm_asm_do_resume(void);
@@ -560,6 +561,16 @@ void svm_update_guest_cr(struct vcpu *v, unsigned int cr)
                 svm_fpu_enter(v);
         }
 
+        if ( paging_mode_hap(v->domain) )
+        {
+            uint32_t intercepts = vmcb_get_cr_intercepts(vmcb);
+
+            /* Trap CR3 updates if CR3 memory events are enabled. */
+            if ( v->domain->arch.monitor.write_ctrlreg_enabled &
+                 monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
+               vmcb_set_cr_intercepts(vmcb, intercepts | CR_INTERCEPT_CR3_WRITE);
+        }
+
         value = v->arch.hvm_vcpu.guest_cr[0] | hw_cr0_mask;
         if ( !paging_mode_hap(v->domain) )
             value |= X86_CR0_PG | X86_CR0_WP;
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 4d8887e..b06ee4e 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -96,7 +96,8 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
     {
         capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST) |
                        (1U << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) |
-                       (1U << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR);
+                       (1U << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
+                       (1U << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG);
     }
 
     if ( hvm_funcs.set_descriptor_access_exiting )
-- 
2.7.4


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

Re: [PATCH v1 1/4] asm-x86/monitor: Enable svm monitor events

[Tamas K Lengyel]

On Fri, Feb 2, 2018 at 2:37 AM, Alexandru Isaila
<aisaila@bitdefender.com> wrote:
> This commit separates the svm caps from the vmx caps.
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> ---
>  xen/include/asm-x86/monitor.h | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
> index a0444d1..3706b7a 100644
> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -74,21 +74,28 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
>       * At the moment only Intel HVM domains are supported. However, event
>       * delivery could be extended to AMD and PV domains.

I guess adjusting this comment here would now be also appropriate.

>       */
> -    if ( !is_hvm_domain(d) || !cpu_has_vmx )
> +    if ( !is_hvm_domain(d) )
>          return capabilities;
>
> -    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_DEBUG_EXCEPTION) |
> -                   (1U << XEN_DOMCTL_MONITOR_EVENT_CPUID) |
> -                   (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT) |
> -                   (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() )
> -        capabilities |= (1U << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
> +    if( cpu_has_vmx )
> +    {
> +        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_DEBUG_EXCEPTION) |
> +                       (1U << XEN_DOMCTL_MONITOR_EVENT_CPUID) |
> +                       (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT) |
> +                       (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() )
> +            capabilities |= (1U << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
> +    }
> +    else if ( cpu_has_svm )
> +    {
> +        capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);

If this is now supported on both svm and vmx, so you could just set it
outside the if blocks. We know we are dealing with an hvm domain at
this point.

Tamas

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

Re: [PATCH v1 2/4] hvm/svm: Enable Breakpoint events

[Tamas K Lengyel]

On Fri, Feb 2, 2018 at 2:37 AM, Alexandru Isaila
<aisaila@bitdefender.com> wrote:
> This commit enables the breakpoint events for svm.
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> ---
>  xen/arch/x86/hvm/svm/svm.c    | 52 ++++++++++++++++++++++++++++++++++++-------
>  xen/include/asm-x86/monitor.h |  3 ++-
>  2 files changed, 46 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index dcbd550..14a5f60 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -59,6 +59,7 @@
>  #include <asm/hap.h>
>  #include <asm/apic.h>
>  #include <asm/debugger.h>
> +#include <asm/hvm/monitor.h>
>  #include <asm/xstate.h>
>
>  void svm_asm_do_resume(void);
> @@ -1079,7 +1080,8 @@ static void svm_ctxt_switch_to(struct vcpu *v)
>  static void noreturn svm_do_resume(struct vcpu *v)
>  {
>      struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> -    bool_t debug_state = v->domain->debugger_attached;
> +    bool_t debug_state = v->domain->debugger_attached
> +                || v->domain->arch.monitor.software_breakpoint_enabled;
>      bool_t vcpu_guestmode = 0;
>      struct vlapic *vlapic = vcpu_vlapic(v);
>
> @@ -2407,6 +2409,23 @@ 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)
> +{
> +    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,
> +    };
> +
> +    if ( event.type >= X86_EVENTTYPE_SW_INTERRUPT )
> +        event.insn_len = insn_len;seems
> +    else
> +        event.insn_len = 0;
> +
> +    hvm_inject_event(&event);
> +}
> +
>  static struct hvm_function_table __initdata svm_function_table = {
>      .name                 = "SVM",
>      .cpu_up_prepare       = svm_cpu_up_prepare,
> @@ -2619,14 +2638,31 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>          break;
>
>      case VMEXIT_EXCEPTION_BP:
> -        if ( !v->domain->debugger_attached )
> -            goto unexpected_exit_type;
> -        /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not update RIP. */
> -        if ( (inst_len = __get_instruction_length(v, INSTR_INT3)) == 0 )
> +        inst_len = __get_instruction_length(v, INSTR_INT3);
> +
> +        if ( inst_len == 0 )
>              break;
> -        __update_guest_eip(regs, inst_len);
> -        current->arch.gdbsx_vcpu_event = TRAP_int3;
> -        domain_pause_for_debugger();
> +
> +        if ( !v->domain->debugger_attached )

I think this would be easier to follow if you switched it around.

> +        {
> +         /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not update RIP. */
> +            int rc;
> +
> +            rc = hvm_monitor_debug(regs->rip,
> +                                   HVM_MONITOR_SOFTWARE_BREAKPOINT,
> +                                   X86_EVENTTYPE_SW_EXCEPTION,
> +                                   inst_len);
> +            if ( rc < 0 )
> +                goto unexpected_exit_type;
> +            if ( !rc )
> +                svm_propagate_intr(v, inst_len);
> +        }
> +        else
> +        {
> +            __update_guest_eip(regs, inst_len);
> +            current->arch.gdbsx_vcpu_event = TRAP_int3;
> +            domain_pause_for_debugger();
> +        }
>          break;
>
>      case VMEXIT_EXCEPTION_NM:
> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
> index 3706b7a..68a210a 100644
> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -94,7 +94,8 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
>      }
>      else if ( cpu_has_svm )
>      {
> -        capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
> +        capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST) |
> +                       (1U << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT);

Since breakpoints are also supported for both svm and vmx, you can
just set it once, no need for the extra if block.

Tamas

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

Re: [PATCH v1 2/4] hvm/svm: Enable Breakpoint events

[Andrew Cooper]

On 02/02/18 09:37, Alexandru Isaila wrote:
> This commit enables the breakpoint events for svm.
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> ---
>  xen/arch/x86/hvm/svm/svm.c    | 52 ++++++++++++++++++++++++++++++++++++-------
>  xen/include/asm-x86/monitor.h |  3 ++-
>  2 files changed, 46 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index dcbd550..14a5f60 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -59,6 +59,7 @@
>  #include <asm/hap.h>
>  #include <asm/apic.h>
>  #include <asm/debugger.h>
> +#include <asm/hvm/monitor.h>
>  #include <asm/xstate.h>
>  
>  void svm_asm_do_resume(void);
> @@ -1079,7 +1080,8 @@ static void svm_ctxt_switch_to(struct vcpu *v)
>  static void noreturn svm_do_resume(struct vcpu *v)
>  {
>      struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> -    bool_t debug_state = v->domain->debugger_attached;
> +    bool_t debug_state = v->domain->debugger_attached
> +                || v->domain->arch.monitor.software_breakpoint_enabled;

As a minor note, please clean up bool_t => bool as you end up making
changes.

>      bool_t vcpu_guestmode = 0;
>      struct vlapic *vlapic = vcpu_vlapic(v);
>  
> @@ -2407,6 +2409,23 @@ 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)
> +{
> +    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,
> +    };
> +
> +    if ( event.type >= X86_EVENTTYPE_SW_INTERRUPT )
> +        event.insn_len = insn_len;
> +    else
> +        event.insn_len = 0;

IIRC, you need to always set insn_len.  The length handling is vastly
complicated (depends on event type and hardware availability, and in
some cases needs emulating anyway), but the lower injection levels
should DTRT.

If they don't, can you provide a concrete example which doesn't work and
we can see about what to do.

> +
> +    hvm_inject_event(&event);
> +}
> +
>  static struct hvm_function_table __initdata svm_function_table = {
>      .name                 = "SVM",
>      .cpu_up_prepare       = svm_cpu_up_prepare,
> @@ -2619,14 +2638,31 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>          break;
>  
>      case VMEXIT_EXCEPTION_BP:
> -        if ( !v->domain->debugger_attached )
> -            goto unexpected_exit_type;
> -        /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not update RIP. */
> -        if ( (inst_len = __get_instruction_length(v, INSTR_INT3)) == 0 )
> +        inst_len = __get_instruction_length(v, INSTR_INT3);
> +
> +        if ( inst_len == 0 )
>              break;
> -        __update_guest_eip(regs, inst_len);
> -        current->arch.gdbsx_vcpu_event = TRAP_int3;
> -        domain_pause_for_debugger();
> +
> +        if ( !v->domain->debugger_attached )
> +        {
> +         /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not update RIP. */
> +            int rc;
> +
> +            rc = hvm_monitor_debug(regs->rip,
> +                                   HVM_MONITOR_SOFTWARE_BREAKPOINT,
> +                                   X86_EVENTTYPE_SW_EXCEPTION,
> +                                   inst_len);
> +            if ( rc < 0 )
> +                goto unexpected_exit_type;
> +            if ( !rc )
> +                svm_propagate_intr(v, inst_len);
> +        }
> +        else
> +        {
> +            __update_guest_eip(regs, inst_len);
> +            current->arch.gdbsx_vcpu_event = TRAP_int3;
> +            domain_pause_for_debugger();
> +        }
>          break;
>  
>      case VMEXIT_EXCEPTION_NM:
> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
> index 3706b7a..68a210a 100644
> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -94,7 +94,8 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
>      }
>      else if ( cpu_has_svm )
>      {
> -        capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
> +        capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST) |
> +                       (1U << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT);

Please introduce an extra pair of brackets around the whole thing, so
automatic indentation in most editors DTRT.

~Andrew

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

Re: [PATCH v1 2/4] hvm/svm: Enable Breakpoint events

[Alexandru Stefan ISAILA]

On Vi, 2018-02-02 at 15:58 +0000, Andrew Cooper wrote:
> On 02/02/18 09:37, Alexandru Isaila wrote:
> >
> > This commit enables the breakpoint events for svm.
> >
> > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> > ---
> >  xen/arch/x86/hvm/svm/svm.c    | 52
> > ++++++++++++++++++++++++++++++++++++-------
> >  xen/include/asm-x86/monitor.h |  3 ++-
> >  2 files changed, 46 insertions(+), 9 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/svm/svm.c
> > b/xen/arch/x86/hvm/svm/svm.c
> > index dcbd550..14a5f60 100644
> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -59,6 +59,7 @@
> >  #include <asm/hap.h>
> >  #include <asm/apic.h>
> >  #include <asm/debugger.h>
> > +#include <asm/hvm/monitor.h>
> >  #include <asm/xstate.h>
> >
> >  void svm_asm_do_resume(void);
> > @@ -1079,7 +1080,8 @@ static void svm_ctxt_switch_to(struct vcpu
> > *v)
> >  static void noreturn svm_do_resume(struct vcpu *v)
> >  {
> >      struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> > -    bool_t debug_state = v->domain->debugger_attached;
> > +    bool_t debug_state = v->domain->debugger_attached
> > +                || v->domain-
> > >arch.monitor.software_breakpoint_enabled;
> As a minor note, please clean up bool_t => bool as you end up making
> changes.
Will do, must of passed it.
>
> >
> >      bool_t vcpu_guestmode = 0;
> >      struct vlapic *vlapic = vcpu_vlapic(v);
> >
> > @@ -2407,6 +2409,23 @@ 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)
> > +{
> > +    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,
> > +    };
> > +
> > +    if ( event.type >= X86_EVENTTYPE_SW_INTERRUPT )
> > +        event.insn_len = insn_len;
> > +    else
> > +        event.insn_len = 0;
> IIRC, you need to always set insn_len.  The length handling is vastly
> complicated (depends on event type and hardware availability, and in
> some cases needs emulating anyway), but the lower injection levels
> should DTRT.
>
> If they don't, can you provide a concrete example which doesn't work
> and
> we can see about what to do.
I've copied the functionality form vmx but I can remove the if
statement and always add the inst_len to the event.

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 v1 2/4] hvm/svm: Enable Breakpoint events

[Alexandru Stefan ISAILA]

On Vi, 2018-02-02 at 08:42 -0700, Tamas K Lengyel wrote:
> On Fri, Feb 2, 2018 at 2:37 AM, Alexandru Isaila
> <aisaila@bitdefender.com> wrote:
> > 
> > This commit enables the breakpoint events for svm.
> > 
> > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> > ---
> >  xen/arch/x86/hvm/svm/svm.c    | 52
> > ++++++++++++++++++++++++++++++++++++-------
> >  xen/include/asm-x86/monitor.h |  3 ++-
> >  2 files changed, 46 insertions(+), 9 deletions(-)
> > 
> > diff --git a/xen/arch/x86/hvm/svm/svm.c
> > b/xen/arch/x86/hvm/svm/svm.c
> > index dcbd550..14a5f60 100644
> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -59,6 +59,7 @@
> >  #include <asm/hap.h>
> >  #include <asm/apic.h>
> >  #include <asm/debugger.h>
> > +#include <asm/hvm/monitor.h>
> >  #include <asm/xstate.h>
> > 
> >  void svm_asm_do_resume(void);
> > @@ -1079,7 +1080,8 @@ static void svm_ctxt_switch_to(struct vcpu
> > *v)
> >  static void noreturn svm_do_resume(struct vcpu *v)
> >  {
> >      struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> > -    bool_t debug_state = v->domain->debugger_attached;
> > +    bool_t debug_state = v->domain->debugger_attached
> > +                || v->domain-
> > >arch.monitor.software_breakpoint_enabled;
> >      bool_t vcpu_guestmode = 0;
> >      struct vlapic *vlapic = vcpu_vlapic(v);
> > 
> > @@ -2407,6 +2409,23 @@ 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)
> > +{
> > +    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,
> > +    };
> > +
> > +    if ( event.type >= X86_EVENTTYPE_SW_INTERRUPT )
> > +        event.insn_len = insn_len;seems
> > +    else
> > +        event.insn_len = 0;
> > +
> > +    hvm_inject_event(&event);
> > +}
> > +
> >  static struct hvm_function_table __initdata svm_function_table = {
> >      .name                 = "SVM",
> >      .cpu_up_prepare       = svm_cpu_up_prepare,
> > @@ -2619,14 +2638,31 @@ void svm_vmexit_handler(struct
> > cpu_user_regs *regs)
> >          break;
> > 
> >      case VMEXIT_EXCEPTION_BP:
> > -        if ( !v->domain->debugger_attached )
> > -            goto unexpected_exit_type;
> > -        /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not
> > update RIP. */
> > -        if ( (inst_len = __get_instruction_length(v, INSTR_INT3))
> > == 0 )
> > +        inst_len = __get_instruction_length(v, INSTR_INT3);
> > +
> > +        if ( inst_len == 0 )
> >              break;
> > -        __update_guest_eip(regs, inst_len);
> > -        current->arch.gdbsx_vcpu_event = TRAP_int3;
> > -        domain_pause_for_debugger();
> > +
> > +        if ( !v->domain->debugger_attached )
> I think this would be easier to follow if you switched it around.
This is just the way that is on the vmx side. If you think it's better
to change it then I will
> 
> > 
> > +        {
> > +         /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not
> > update RIP. */
> > +            int rc;
> > +
> > +            rc = hvm_monitor_debug(regs->rip,
> > +                                   HVM_MONITOR_SOFTWARE_BREAKPOINT
> > ,
> > +                                   X86_EVENTTYPE_SW_EXCEPTION,
> > +                                   inst_len);
> > +            if ( rc < 0 )
> > +                goto unexpected_exit_type;
> > +            if ( !rc )
> > +                svm_propagate_intr(v, inst_len);
> > +        }
> > +        else
> > +        {
> > +            __update_guest_eip(regs, inst_len);
> > +            current->arch.gdbsx_vcpu_event = TRAP_int3;
> > +            domain_pause_for_debugger();
> > +        }
> >          break;
> > 
> >      case VMEXIT_EXCEPTION_NM:
> > diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-
> > x86/monitor.h
> > index 3706b7a..68a210a 100644
> > --- a/xen/include/asm-x86/monitor.h
> > +++ b/xen/include/asm-x86/monitor.h
> > @@ -94,7 +94,8 @@ static inline uint32_t
> > arch_monitor_get_capabilities(struct domain *d)
> >      }
> >      else if ( cpu_has_svm )
> >      {
> > -        capabilities = (1U <<
> > XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
> > +        capabilities = (1U <<
> > XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST) |
> > +                       (1U <<
> > XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT);
> Since breakpoints are also supported for both svm and vmx, you can
> just set it once, no need for the extra if block.
> 
> Tamas
> 
> ________________________
> This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel