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

[PATCH v4 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 v4 1/3] hvm/svm: Enable Breakpoint events

[Alexandru Isaila]

This commit implements the breakpoint events for svm.
At the moment, the Breakpoint vmexit is not forwarded to the monitor
layer.
This patch adds the hvm_monitor_debug call to the VMEXIT_EXCEPTION_BP.
Also, the Software Breakpoint cap is moved from the Intel arch to the
common part of the code.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

---
Changes since V3:
	-Rebase to the latest staging
---
 xen/arch/x86/hvm/monitor.c    |  5 +++++
 xen/arch/x86/hvm/svm/svm.c    | 50 +++++++++++++++++++++++++++++++++++--------
 xen/arch/x86/hvm/vmx/vmx.c    |  5 -----
 xen/include/asm-x86/monitor.h |  4 ++--
 4 files changed, 48 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
index 131b852..5d568a3 100644
--- a/xen/arch/x86/hvm/monitor.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -133,6 +133,11 @@ static inline unsigned long gfn_of_rip(unsigned long rip)
 int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type,
                       unsigned long trap_type, unsigned long insn_length)
 {
+   /*
+    * rc < 0 error in monitor/vm_event, crash
+    * !rc    continue normally
+    * rc > 0 paused waiting for response, work here is done
+    */
     struct vcpu *curr = current;
     struct arch_domain *ad = &curr->domain->arch;
     vm_event_request_t req = {};
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 81cf5b8..abd3fe5 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 debug_state = v->domain->debugger_attached
+                || v->domain->arch.monitor.software_breakpoint_enabled;
     bool_t vcpu_guestmode = 0;
     struct vlapic *vlapic = vcpu_vlapic(v);
 
@@ -2404,6 +2406,19 @@ 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,
+    };
+
+    event.insn_len = insn_len;
+    hvm_inject_event(&event);
+}
+
 static struct hvm_function_table __initdata svm_function_table = {
     .name                 = "SVM",
     .cpu_up_prepare       = svm_cpu_up_prepare,
@@ -2616,14 +2631,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 )
-            break;
-        __update_guest_eip(regs, inst_len);
-        current->arch.gdbsx_vcpu_event = TRAP_int3;
-        domain_pause_for_debugger();
+        inst_len = __get_instruction_length(v, INSTR_INT3);
+
+        if ( inst_len == 0 )
+             break;
+
+        if ( v->domain->debugger_attached )
+        {
+            /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not update RIP. */
+            __update_guest_eip(regs, inst_len);
+            current->arch.gdbsx_vcpu_event = TRAP_int3;
+            domain_pause_for_debugger();
+        }
+        else
+        {
+           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);
+        }
         break;
 
     case VMEXIT_EXCEPTION_NM:
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index d35cf55..5cd689e 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3703,11 +3703,6 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
                                        HVM_MONITOR_DEBUG_EXCEPTION,
                                        trap_type, insn_len);
 
-                /*
-                 * rc < 0 error in monitor/vm_event, crash
-                 * !rc    continue normally
-                 * rc > 0 paused waiting for response, work here is done
-                 */
                 if ( rc < 0 )
                     goto exit_and_crash;
                 if ( !rc )
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 9ef6dff..b1902f2 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -77,13 +77,13 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
     if ( !is_hvm_domain(d) )
         return capabilities;
 
-    capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
+    capabilities = ((1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST) |
+                    (1U << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT));
 
     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_DEBUG_EXCEPTION) |
                          (1U << XEN_DOMCTL_MONITOR_EVENT_CPUID) |
                          (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT) |
-- 
2.7.4


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

[PATCH v4 2/3] hvm/svm: Enable MSR events

[Alexandru Isaila]

At this moment there is no function to enable msr interception on svm.

This patch implements this function and moves the mov to msr monitor
event
form the Intel arch side to the common capabilities.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

---
    Changes since V3:
        -Rebase to the latest staging
---
 xen/arch/x86/hvm/svm/svm.c    | 9 +++++++++
 xen/include/asm-x86/monitor.h | 4 ++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index abd3fe5..ecef6bd 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;
@@ -2457,6 +2465,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 b1902f2..9a8f9d9 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -78,12 +78,12 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
         return capabilities;
 
     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 ( cpu_has_vmx )
     {
         capabilities |= ((1U << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
-                         (1U << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
                          (1U << XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION) |
                          (1U << XEN_DOMCTL_MONITOR_EVENT_CPUID) |
                          (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT) |
-- 
2.7.4


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

[PATCH v4 3/3] hvm/svm: Enable CR events

[Alexandru Isaila]

The CR_INTERCEPT_CR3_WRITE intercept is out of the vmcb->_cr_intercepts
so the AMD arch can't intercept CR events.

This patch implements the CR intercept by adding the flag on a
write_ctrlreg event. The monitor write ctrlreg event is moved from the
Intel side to the common capabilities side.

We just need to enable the SVM intercept and then hvm_mov_to_cr() will
forward the event on to the monitor when appropriate.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

---
    Changes since V3:
        -Rebase to the latest staging
---
 xen/arch/x86/hvm/svm/svm.c    | 11 +++++++++++
 xen/include/asm-x86/monitor.h |  6 +++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index ecef6bd..e36ad05 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 9a8f9d9..59a2610 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -79,12 +79,12 @@ 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 ( cpu_has_vmx )
     {
-        capabilities |= ((1U << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
-                         (1U << XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION) |
+        capabilities |= ((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));
-- 
2.7.4


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

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

[Andrew Cooper]

On 15/02/18 10:22, Alexandru Isaila wrote:
> 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.

Ok - this series is looking much clearer now.  Thanks.

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> (although there is
one trivial style issue in patch 1 I can fix on commit).

You are still waiting for SVM acks on patches 1 and 3 by the looks of
things.

One thing I note however is that patch 2 and 3 both turn on intercepts
and have no way of turning them back off.  This appears to be consistent
with the Intel side of things, but it is suboptimal for the guest when
an introspection agent detaches.

For the CPUID/MSR policy side of things, Jan has talked me in to
changing how cpuid_policy_updated() works, and implementing it as a
recalculation of the intercepts on the return-to-guest path.  It occurs
to me that this usefully extends to changes requested by the
introspection agent.

~Andrew

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

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

[Razvan Cojocaru]

On 02/15/2018 02:36 PM, Andrew Cooper wrote:
> One thing I note however is that patch 2 and 3 both turn on intercepts
> and have no way of turning them back off.  This appears to be consistent
> with the Intel side of things, but it is suboptimal for the guest when
> an introspection agent detaches.

That's very true (I've also pointed this out to Bitweasil when we've
discussed the CR3 intercept crash after the noflush bit started to get
used).

I've also considered this when I've added the MSR write subscription
code, however it didn't feel safe to just disable those exits when the
introspection agent unplugs.

For one, we'd have to store the previous state somewhere (we might be
interested in a MSR, for example, for which exits were already enabled
before we subscribed to it - we shouldn't disable exits for it then).

And even if we did keep the previous state somewhere (with the assorted
problems - where do we allocate space for it? etc.) - it's theoretically
possible that some other Xen subsystem fiddles with the exits in the
meantime, so the state we remember may not be the current state of
affairs wrt exits.

Am I overthinking this?

> For the CPUID/MSR policy side of things, Jan has talked me in to
> changing how cpuid_policy_updated() works, and implementing it as a
> recalculation of the intercepts on the return-to-guest path.  It occurs
> to me that this usefully extends to changes requested by the
> introspection agent.

Thanks, we'll look that up.


Thanks,
Razvan

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

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

[Boris Ostrovsky]

On 02/15/2018 05:22 AM, Alexandru Isaila wrote:
> 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.
>



Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

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

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

[Andrew Cooper]

On 15/02/18 13:05, Razvan Cojocaru wrote:
> On 02/15/2018 02:36 PM, Andrew Cooper wrote:
>> One thing I note however is that patch 2 and 3 both turn on intercepts
>> and have no way of turning them back off.  This appears to be consistent
>> with the Intel side of things, but it is suboptimal for the guest when
>> an introspection agent detaches.
> That's very true (I've also pointed this out to Bitweasil when we've
> discussed the CR3 intercept crash after the noflush bit started to get
> used).
>
> I've also considered this when I've added the MSR write subscription
> code, however it didn't feel safe to just disable those exits when the
> introspection agent unplugs.

The problem is that we don't have a source of information to derive the
intercepts from, which is also what is causing chaos trying to get
nested virt working.

We only have what is programmed into the hardware structures, which
suffers from "multiple-producers" syndrome.

> For one, we'd have to store the previous state somewhere (we might be
> interested in a MSR, for example, for which exits were already enabled
> before we subscribed to it - we shouldn't disable exits for it then).
>
> And even if we did keep the previous state somewhere (with the assorted
> problems - where do we allocate space for it? etc.) - it's theoretically
> possible that some other Xen subsystem fiddles with the exits in the
> meantime, so the state we remember may not be the current state of
> affairs wrt exits.
>
> Am I overthinking this?

No - I don't think so.

The only way to solve this problem is to have all the information for
each agent (including Xen itself) interested in controlling the
behaviour of the guest to be available, hooked off struct domain/vcpu as
appropriate, and one single function to combine everyones view of the
world into the hardware configuration.

If you can't unambiguously recalculate the contents of the VMCS/VMCB,
then something is broken.

>
>> For the CPUID/MSR policy side of things, Jan has talked me in to
>> changing how cpuid_policy_updated() works, and implementing it as a
>> recalculation of the intercepts on the return-to-guest path.  It occurs
>> to me that this usefully extends to changes requested by the
>> introspection agent.
> Thanks, we'll look that up.

It doesn't exist yet :)

~Andrew

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