All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] x86/monitor: add support for descriptor access events
@ 2017-04-04  9:57 Adrian Pop
  2017-04-04 10:52 ` Razvan Cojocaru
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Adrian Pop @ 2017-04-04  9:57 UTC (permalink / raw)
  To: xen-devel
  Cc: Kevin Tian, Tamas K Lengyel, Wei Liu, Suravee Suthikulpanit,
	Razvan Cojocaru, Adrian Pop, Andrew Cooper, Ian Jackson,
	Jan Beulich, Jun Nakajima, Boris Ostrovsky

Adds monitor support for descriptor access events (reads & writes of
IDTR/GDTR/LDTR/TR) for the x86 architecture (VMX and SVM).

Signed-off-by: Adrian Pop <apop@bitdefender.com>
---
changes in v2:
- use two sub-structs (vmx/svm) for vm_event_desc_access (Tamas K
  Lengyel)
- more compact version of the descriptor VM-Exit handling on svm (Boris
  Ostrovsky)
- use bool instead of bool_t (Jan Beulich)
- use curr, currd for the struct vcpu and struct domain local variables
  (Jan Beulich)
- move hvm_emulate_ctxt into a narrower scope (Jan Beulich)
- remove leftover dead code (Jan Beulich)
- order the monitor events numerically (Jan Beulich)
- remove VM_EVENT_DESC_INVALID (Jan Beulich)
- crash the domain if the descriptor access instruction can't be
  emulated (Jan Beulich)
- call hvm_inject_event without checking for pending events (Andrew
  Cooper)
- introduce structures for the VM-Exit instruction information used for
  the descriptor instructions and use fewer magic numbers (Andrew
  Cooper, Jan Beulich)
---
 tools/libxc/include/xenctrl.h       |  2 ++
 tools/libxc/xc_monitor.c            | 14 ++++++++++
 tools/tests/xen-access/xen-access.c | 29 ++++++++++++++++++++-
 xen/arch/x86/hvm/hvm.c              | 37 ++++++++++++++++++++++++++
 xen/arch/x86/hvm/monitor.c          | 22 ++++++++++++++++
 xen/arch/x86/hvm/svm/svm.c          | 42 ++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/vmx/vmx.c          | 52 +++++++++++++++++++++++++++++++++++++
 xen/arch/x86/monitor.c              | 18 +++++++++++++
 xen/arch/x86/vm_event.c             |  6 +++++
 xen/include/asm-x86/domain.h        |  1 +
 xen/include/asm-x86/hvm/hvm.h       |  1 +
 xen/include/asm-x86/hvm/monitor.h   |  3 +++
 xen/include/asm-x86/hvm/support.h   |  3 +++
 xen/include/asm-x86/hvm/vmx/vmx.h   | 42 ++++++++++++++++++++++++++++++
 xen/include/asm-x86/monitor.h       |  3 ++-
 xen/include/public/domctl.h         |  1 +
 xen/include/public/vm_event.h       | 25 ++++++++++++++++++
 17 files changed, 299 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 2d97d36c38..2248041c5a 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2007,6 +2007,8 @@ int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, uint32_t msr,
 int xc_monitor_singlestep(xc_interface *xch, domid_t domain_id, bool enable);
 int xc_monitor_software_breakpoint(xc_interface *xch, domid_t domain_id,
                                    bool enable);
+int xc_monitor_descriptor_access(xc_interface *xch, domid_t domain_id,
+                                 bool enable);
 int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id,
                              bool enable, bool sync);
 int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index 15a7c32d52..f99b6e3a33 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -129,6 +129,20 @@ int xc_monitor_singlestep(xc_interface *xch, domid_t domain_id,
     return do_domctl(xch, &domctl);
 }
 
+int xc_monitor_descriptor_access(xc_interface *xch, domid_t domain_id,
+                                 bool enable)
+{
+    DECLARE_DOMCTL;
+
+    domctl.cmd = XEN_DOMCTL_monitor_op;
+    domctl.domain = domain_id;
+    domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
+                                    : XEN_DOMCTL_MONITOR_OP_DISABLE;
+    domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS;
+
+    return do_domctl(xch, &domctl);
+}
+
 int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id, bool enable,
                              bool sync)
 {
diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index 9d4f95756b..ff4d289b45 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -337,7 +337,7 @@ void usage(char* progname)
 {
     fprintf(stderr, "Usage: %s [-m] <domain_id> write|exec", progname);
 #if defined(__i386__) || defined(__x86_64__)
-            fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec|debug|cpuid");
+            fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec|debug|cpuid|desc_access");
 #elif defined(__arm__) || defined(__aarch64__)
             fprintf(stderr, "|privcall");
 #endif
@@ -368,6 +368,7 @@ int main(int argc, char *argv[])
     int altp2m = 0;
     int debug = 0;
     int cpuid = 0;
+    int desc_access = 0;
     uint16_t altp2m_view_id = 0;
 
     char* progname = argv[0];
@@ -434,6 +435,10 @@ int main(int argc, char *argv[])
     {
         cpuid = 1;
     }
+    else if ( !strcmp(argv[0], "desc_access") )
+    {
+        desc_access = 1;
+    }
 #elif defined(__arm__) || defined(__aarch64__)
     else if ( !strcmp(argv[0], "privcall") )
     {
@@ -571,6 +576,16 @@ int main(int argc, char *argv[])
         }
     }
 
+    if ( desc_access )
+    {
+        rc = xc_monitor_descriptor_access(xch, domain_id, 1);
+        if ( rc < 0 )
+        {
+            ERROR("Error %d setting descriptor access listener with vm_event\n", rc);
+            goto exit;
+        }
+    }
+
     if ( privcall )
     {
         rc = xc_monitor_privileged_call(xch, domain_id, 1);
@@ -595,6 +610,8 @@ int main(int argc, char *argv[])
                 rc = xc_monitor_debug_exceptions(xch, domain_id, 0, 0);
             if ( cpuid )
                 rc = xc_monitor_cpuid(xch, domain_id, 0);
+            if ( desc_access )
+                rc = xc_monitor_descriptor_access(xch, domain_id, 0);
 
             if ( privcall )
                 rc = xc_monitor_privileged_call(xch, domain_id, 0);
@@ -779,6 +796,16 @@ int main(int argc, char *argv[])
                 rsp.data = req.data;
                 rsp.data.regs.x86.rip += req.u.cpuid.insn_length;
                 break;
+            case VM_EVENT_REASON_DESCRIPTOR_ACCESS:
+                printf("Descriptor access: rip=%016"PRIx64", vcpu %d: "\
+                       "VMExit info=0x%"PRIx32", descriptor=%d, is write=%d\n",
+                       req.data.regs.x86.rip,
+                       req.vcpu_id,
+                       req.u.desc_access.arch.vmx.instr_info,
+                       req.u.desc_access.descriptor,
+                       req.u.desc_access.is_write);
+                rsp.flags |= VM_EVENT_FLAG_EMULATE;
+                break;
             default:
                 fprintf(stderr, "UNKNOWN REASON CODE %d\n", req.reason);
             }
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 0282986738..61383cb72f 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3572,6 +3572,43 @@ gp_fault:
     return X86EMUL_EXCEPTION;
 }
 
+int hvm_descriptor_access_intercept(uint64_t exit_info,
+                                    uint64_t vmx_exit_qualification,
+                                    uint8_t descriptor, bool is_write)
+{
+    struct vcpu *curr = current;
+    struct domain *currd = curr->domain;
+    int rc;
+
+    if ( currd->arch.monitor.descriptor_access_enabled )
+    {
+        ASSERT(curr->arch.vm_event);
+        hvm_monitor_descriptor_access(exit_info, vmx_exit_qualification,
+                                      descriptor, is_write);
+    }
+    else
+    {
+        struct hvm_emulate_ctxt ctxt = {};
+
+        hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
+        rc = hvm_emulate_one(&ctxt);
+        switch ( rc )
+        {
+        case X86EMUL_UNHANDLEABLE:
+            domain_crash(currd);
+            return X86EMUL_UNHANDLEABLE;
+        case X86EMUL_EXCEPTION:
+            hvm_inject_event(&ctxt.ctxt.event);
+            /* fall through */
+        default:
+            hvm_emulate_writeback(&ctxt);
+            break;
+        }
+    }
+
+    return X86EMUL_OKAY;
+}
+
 static bool is_cross_vendor(const struct x86_emulate_state *state,
                             const struct x86_emulate_ctxt *ctxt)
 {
diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
index f5cd245771..d60e4afd0c 100644
--- a/xen/arch/x86/hvm/monitor.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -72,6 +72,28 @@ void hvm_monitor_msr(unsigned int msr, uint64_t value)
     }
 }
 
+void hvm_monitor_descriptor_access(uint64_t exit_info,
+                                   uint64_t vmx_exit_qualification,
+                                   uint8_t descriptor, bool is_write)
+{
+    struct vcpu *curr = current;
+    vm_event_request_t req = {
+        .reason = VM_EVENT_REASON_DESCRIPTOR_ACCESS,
+        .u.desc_access.descriptor = descriptor,
+        .u.desc_access.is_write = is_write,
+    };
+    if ( cpu_has_vmx )
+    {
+        req.u.desc_access.arch.vmx.instr_info = exit_info;
+        req.u.desc_access.arch.vmx.exit_qualification = vmx_exit_qualification;
+    }
+    else
+    {
+        req.u.desc_access.arch.svm.exitinfo = exit_info;
+    }
+    monitor_traps(curr, 1, &req);
+}
+
 static inline unsigned long gfn_of_rip(unsigned long rip)
 {
     struct vcpu *curr = current;
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index b69789b35c..f78dee6249 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -856,6 +856,23 @@ static void svm_set_rdtsc_exiting(struct vcpu *v, bool_t enable)
     vmcb_set_general2_intercepts(vmcb, general2_intercepts);
 }
 
+static void svm_set_descriptor_access_exiting(struct vcpu *v, bool enable)
+{
+    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+    u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
+    u32 mask = GENERAL1_INTERCEPT_IDTR_READ | GENERAL1_INTERCEPT_GDTR_READ \
+            | GENERAL1_INTERCEPT_LDTR_READ | GENERAL1_INTERCEPT_TR_READ \
+            | GENERAL1_INTERCEPT_IDTR_WRITE | GENERAL1_INTERCEPT_GDTR_WRITE \
+            | GENERAL1_INTERCEPT_LDTR_WRITE | GENERAL1_INTERCEPT_TR_WRITE;
+
+    if ( enable )
+        general1_intercepts |= mask;
+    else
+        general1_intercepts &= ~mask;
+
+    vmcb_set_general1_intercepts(vmcb, general1_intercepts);
+}
+
 static unsigned int svm_get_insn_bytes(struct vcpu *v, uint8_t *buf)
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
@@ -2225,6 +2242,7 @@ static struct hvm_function_table __initdata svm_function_table = {
     .msr_read_intercept   = svm_msr_read_intercept,
     .msr_write_intercept  = svm_msr_write_intercept,
     .set_rdtsc_exiting    = svm_set_rdtsc_exiting,
+    .set_descriptor_access_exiting = svm_set_descriptor_access_exiting,
     .get_insn_bytes       = svm_get_insn_bytes,
 
     .nhvm_vcpu_initialise = nsvm_vcpu_initialise,
@@ -2644,6 +2662,30 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
         svm_vmexit_do_pause(regs);
         break;
 
+    case VMEXIT_IDTR_READ:
+    case VMEXIT_IDTR_WRITE:
+        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0,
+            VM_EVENT_DESC_IDTR, exit_reason == VMEXIT_IDTR_WRITE);
+        break;
+
+    case VMEXIT_GDTR_READ:
+    case VMEXIT_GDTR_WRITE:
+        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0,
+            VM_EVENT_DESC_GDTR, exit_reason == VMEXIT_GDTR_WRITE);
+        break;
+
+    case VMEXIT_LDTR_READ:
+    case VMEXIT_LDTR_WRITE:
+        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0,
+            VM_EVENT_DESC_LDTR, exit_reason == VMEXIT_LDTR_WRITE);
+        break;
+
+    case VMEXIT_TR_READ:
+    case VMEXIT_TR_WRITE:
+        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0,
+            VM_EVENT_DESC_TR, exit_reason == VMEXIT_TR_WRITE);
+        break;
+
     default:
     unexpected_exit_type:
         gdprintk(XENLOG_ERR, "unexpected VMEXIT: exit reason = %#"PRIx64", "
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index d201956c91..89b29576f4 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1343,6 +1343,20 @@ static void vmx_set_rdtsc_exiting(struct vcpu *v, bool_t enable)
     vmx_vmcs_exit(v);
 }
 
+static void vmx_set_descriptor_access_exiting(struct vcpu *v, bool enable)
+{
+    if ( enable )
+        v->arch.hvm_vmx.secondary_exec_control |=
+            SECONDARY_EXEC_DESCRIPTOR_TABLE_EXITING;
+    else
+        v->arch.hvm_vmx.secondary_exec_control &=
+            ~SECONDARY_EXEC_DESCRIPTOR_TABLE_EXITING;
+
+    vmx_vmcs_enter(v);
+    vmx_update_secondary_exec_control(v);
+    vmx_vmcs_exit(v);
+}
+
 static void vmx_init_hypercall_page(struct domain *d, void *hypercall_page)
 {
     char *p;
@@ -2230,6 +2244,7 @@ static struct hvm_function_table __initdata vmx_function_table = {
     .handle_cd            = vmx_handle_cd,
     .set_info_guest       = vmx_set_info_guest,
     .set_rdtsc_exiting    = vmx_set_rdtsc_exiting,
+    .set_descriptor_access_exiting = vmx_set_descriptor_access_exiting,
     .nhvm_vcpu_initialise = nvmx_vcpu_initialise,
     .nhvm_vcpu_destroy    = nvmx_vcpu_destroy,
     .nhvm_vcpu_reset      = nvmx_vcpu_reset,
@@ -3361,6 +3376,40 @@ static void vmx_handle_xrstors(void)
     domain_crash(current->domain);
 }
 
+static void vmx_handle_idt_or_gdt(idt_or_gdt_instr_info_t instr_info,
+                                  uint64_t exit_qualification)
+{
+    uint8_t descriptor = instr_info.instr_identity
+        ? VM_EVENT_DESC_IDTR : VM_EVENT_DESC_GDTR;
+
+    hvm_descriptor_access_intercept(instr_info.raw, exit_qualification,
+                                    descriptor, instr_info.instr_write);
+}
+
+static void vmx_handle_ldt_or_tr(ldt_or_tr_instr_info_t instr_info,
+                                 uint64_t exit_qualification)
+{
+    uint8_t descriptor = instr_info.instr_identity
+        ? VM_EVENT_DESC_TR : VM_EVENT_DESC_LDTR;
+
+    hvm_descriptor_access_intercept(instr_info.raw, exit_qualification,
+                                    descriptor, instr_info.instr_write);
+}
+
+static void vmx_handle_descriptor_access(uint32_t exit_reason)
+{
+    uint64_t instr_info;
+    uint64_t exit_qualification;
+
+    __vmread(EXIT_QUALIFICATION, &exit_qualification);
+    __vmread(VMX_INSTRUCTION_INFO, &instr_info);
+
+    if ( exit_reason == EXIT_REASON_ACCESS_GDTR_OR_IDTR )
+        vmx_handle_idt_or_gdt(instr_info, exit_qualification);
+    else
+        vmx_handle_ldt_or_tr(instr_info, exit_qualification);
+}
+
 static int vmx_handle_apic_write(void)
 {
     unsigned long exit_qualification;
@@ -3972,6 +4021,9 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
 
     case EXIT_REASON_ACCESS_GDTR_OR_IDTR:
     case EXIT_REASON_ACCESS_LDTR_OR_TR:
+        vmx_handle_descriptor_access(exit_reason);
+        break;
+
     case EXIT_REASON_VMX_PREEMPTION_TIMER_EXPIRED:
     case EXIT_REASON_INVPCID:
     /* fall through */
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 5f60743c22..eeb67f5b09 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -211,6 +211,24 @@ int arch_monitor_domctl_event(struct domain *d,
         break;
     }
 
+    case XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS:
+    {
+        bool old_status = ad->monitor.descriptor_access_enabled;
+        struct vcpu *v;
+
+        if ( unlikely(old_status == requested_status) )
+            return -EEXIST;
+
+        domain_pause(d);
+        ad->monitor.descriptor_access_enabled = requested_status;
+
+        for_each_vcpu ( d, v )
+            hvm_funcs.set_descriptor_access_exiting(v, requested_status);
+
+        domain_unpause(d);
+        break;
+    }
+
     case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
     {
         bool_t old_status = ad->monitor.software_breakpoint_enabled;
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index a7ce6ca194..502ccc2d44 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -229,6 +229,12 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp)
         }
         break;
 
+    case VM_EVENT_REASON_DESCRIPTOR_ACCESS:
+        if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA )
+            v->arch.vm_event->emul.read = rsp->data.emul.read;
+        v->arch.vm_event->emulate_flags = rsp->flags;
+        break;
+
     default:
         break;
     };
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index ec14cce81f..841e376bd8 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -404,6 +404,7 @@ struct arch_domain
         unsigned int debug_exception_enabled     : 1;
         unsigned int debug_exception_sync        : 1;
         unsigned int cpuid_enabled               : 1;
+        unsigned int descriptor_access_enabled   : 1;
         struct monitor_msr_bitmap *msr_bitmap;
     } monitor;
 
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index c854183a2e..d908e4aaba 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -173,6 +173,7 @@ struct hvm_function_table {
     void (*handle_cd)(struct vcpu *v, unsigned long value);
     void (*set_info_guest)(struct vcpu *v);
     void (*set_rdtsc_exiting)(struct vcpu *v, bool_t);
+    void (*set_descriptor_access_exiting)(struct vcpu *v, bool);
 
     /* Nested HVM */
     int (*nhvm_vcpu_initialise)(struct vcpu *v);
diff --git a/xen/include/asm-x86/hvm/monitor.h b/xen/include/asm-x86/hvm/monitor.h
index 85ca678d3f..d9efb3505e 100644
--- a/xen/include/asm-x86/hvm/monitor.h
+++ b/xen/include/asm-x86/hvm/monitor.h
@@ -38,6 +38,9 @@ bool_t hvm_monitor_cr(unsigned int index, unsigned long value,
 #define hvm_monitor_crX(cr, new, old) \
                         hvm_monitor_cr(VM_EVENT_X86_##cr, new, old)
 void hvm_monitor_msr(unsigned int msr, uint64_t value);
+void hvm_monitor_descriptor_access(uint64_t exit_info,
+                                   uint64_t vmx_exit_qualification,
+                                   uint8_t descriptor, bool is_write);
 int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type,
                       unsigned long trap_type, unsigned long insn_length);
 int hvm_monitor_cpuid(unsigned long insn_length, unsigned int leaf,
diff --git a/xen/include/asm-x86/hvm/support.h b/xen/include/asm-x86/hvm/support.h
index 632eb90f74..b99ff2a330 100644
--- a/xen/include/asm-x86/hvm/support.h
+++ b/xen/include/asm-x86/hvm/support.h
@@ -128,6 +128,9 @@ int hvm_set_efer(uint64_t value);
 int hvm_set_cr0(unsigned long value, bool_t may_defer);
 int hvm_set_cr3(unsigned long value, bool_t may_defer);
 int hvm_set_cr4(unsigned long value, bool_t may_defer);
+int hvm_descriptor_access_intercept(uint64_t exit_info,
+                                    uint64_t vmx_exit_qualification,
+                                    uint8_t descriptor, bool is_write);
 int hvm_mov_to_cr(unsigned int cr, unsigned int gpr);
 int hvm_mov_from_cr(unsigned int cr, unsigned int gpr);
 void hvm_ud_intercept(struct cpu_user_regs *);
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index 2b781ab120..b00ba52443 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -628,4 +628,46 @@ typedef struct {
     u16 eptp_index;
 } ve_info_t;
 
+/* VM-Exit instruction info for LIDT, LGDT, SIDT, SGDT */
+typedef union idt_or_gdt_instr_info {
+    unsigned long raw;
+    struct {
+        unsigned long scaling   :2,  /* bits 0:1 - Scaling */
+                                :5,  /* bits 6:2 - Undefined */
+        addr_size               :3,  /* bits 9:7 - Address size */
+                                :1,  /* bit 10 - Cleared to 0 */
+        operand_size            :1,  /* bit 11 - Operand size */
+                                :3,  /* bits 14:12 - Undefined */
+        segment_reg             :3,  /* bits 17:15 - Segment register */
+        index_reg               :4,  /* bits 21:18 - Index register */
+        index_reg_invalid       :1,  /* bit 22 - Index register invalid */
+        base_reg                :4,  /* bits 26:23 - Base register */
+        base_reg_invalid        :1,  /* bit 27 - Base register invalid */
+        instr_identity          :1,  /* bit 28 - 0:GDT, 1:IDT */
+        instr_write             :1,  /* bit 29 - 0:store, 1:load */
+                                :2;  /* bits 30:31 - Undefined */
+    };
+} __transparent__ idt_or_gdt_instr_info_t;
+
+/* VM-Exit instruction info for LLDT, LTR, SLDT, STR */
+typedef union ldt_or_tr_instr_info {
+    unsigned long raw;
+    struct {
+        unsigned long scaling   :2,  /* bits 0:1 - Scaling */
+                                :1,  /* bit 2 - Undefined */
+        reg1                    :4,  /* bits 6:3 - Reg1 */
+        addr_size               :3,  /* bits 9:7 - Address size */
+        mem_reg                 :1,  /* bit 10 - Mem/Reg */
+                                :4,  /* bits 14:11 - Undefined */
+        segment_reg             :3,  /* bits 17:15 - Segment register */
+        index_reg               :4,  /* bits 21:18 - Index register */
+        index_reg_invalid       :1,  /* bit 22 - Index register invalid */
+        base_reg                :4,  /* bits 26:23 - Base register */
+        base_reg_invalid        :1,  /* bit 27 - Base register invalid */
+        instr_identity          :1,  /* bit 28 - 0:LDT, 1:TR */
+        instr_write             :1,  /* bit 29 - 0:store, 1:load */
+                                :2;  /* bits 31:30 - Undefined */
+    };
+} __transparent__ ldt_or_tr_instr_info_t;
+
 #endif /* __ASM_X86_HVM_VMX_VMX_H__ */
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index e4093732d3..c3d2699103 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -77,7 +77,8 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
                    (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_INTERRUPT) |
+                   (1U << XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS);
 
     /* Since we know this is on VMX, we can just call the hvm func */
     if ( hvm_is_singlestep_supported() )
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 9e3ce21f71..e3ed4002d9 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1081,6 +1081,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
 #define XEN_DOMCTL_MONITOR_EVENT_CPUID                 6
 #define XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL       7
 #define XEN_DOMCTL_MONITOR_EVENT_INTERRUPT             8
+#define XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS           9
 
 struct xen_domctl_monitor_op {
     uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index b7487a12f3..f01e47159d 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -146,6 +146,8 @@
 #define VM_EVENT_REASON_PRIVILEGED_CALL         11
 /* An interrupt has been delivered. */
 #define VM_EVENT_REASON_INTERRUPT               12
+/* A descriptor table register was accessed. */
+#define VM_EVENT_REASON_DESCRIPTOR_ACCESS       13
 
 /* Supported values for the vm_event_write_ctrlreg index. */
 #define VM_EVENT_X86_CR0    0
@@ -259,6 +261,28 @@ struct vm_event_mov_to_msr {
     uint64_t value;
 };
 
+#define VM_EVENT_DESC_IDTR           1
+#define VM_EVENT_DESC_GDTR           2
+#define VM_EVENT_DESC_LDTR           3
+#define VM_EVENT_DESC_TR             4
+
+struct vm_event_desc_access {
+    union {
+        struct {
+            uint32_t instr_info;         /* VMX: VMCS Instruction-Information */
+            uint32_t _pad1;
+            uint64_t exit_qualification; /* VMX: VMCS Exit Qualification */
+        } vmx;
+        struct {
+            uint64_t exitinfo;           /* SVM: VMCB EXITINFO */
+            uint64_t _pad2;
+        } svm;
+    } arch;
+    uint8_t descriptor;                  /* VM_EVENT_DESC_* */
+    uint8_t is_write;
+    uint8_t _pad[6];
+};
+
 struct vm_event_cpuid {
     uint32_t insn_length;
     uint32_t leaf;
@@ -313,6 +337,7 @@ typedef struct vm_event_st {
         struct vm_event_mem_access            mem_access;
         struct vm_event_write_ctrlreg         write_ctrlreg;
         struct vm_event_mov_to_msr            mov_to_msr;
+        struct vm_event_desc_access           desc_access;
         struct vm_event_singlestep            singlestep;
         struct vm_event_debug                 software_breakpoint;
         struct vm_event_debug                 debug_exception;
-- 
2.12.1


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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] x86/monitor: add support for descriptor access events
  2017-04-04  9:57 [PATCH v2] x86/monitor: add support for descriptor access events Adrian Pop
@ 2017-04-04 10:52 ` Razvan Cojocaru
  2017-04-04 11:46   ` Adrian Pop
  2017-04-04 13:23 ` Boris Ostrovsky
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Razvan Cojocaru @ 2017-04-04 10:52 UTC (permalink / raw)
  To: Adrian Pop, xen-devel
  Cc: Kevin Tian, Tamas K Lengyel, Wei Liu, Jan Beulich, Jun Nakajima,
	Andrew Cooper, Ian Jackson, Suravee Suthikulpanit,
	Boris Ostrovsky

On 04/04/2017 12:57 PM, Adrian Pop wrote:
> Adds monitor support for descriptor access events (reads & writes of
> IDTR/GDTR/LDTR/TR) for the x86 architecture (VMX and SVM).
> 
> Signed-off-by: Adrian Pop <apop@bitdefender.com>
> ---
> changes in v2:
> - use two sub-structs (vmx/svm) for vm_event_desc_access (Tamas K
>   Lengyel)
> - more compact version of the descriptor VM-Exit handling on svm (Boris
>   Ostrovsky)
> - use bool instead of bool_t (Jan Beulich)
> - use curr, currd for the struct vcpu and struct domain local variables
>   (Jan Beulich)
> - move hvm_emulate_ctxt into a narrower scope (Jan Beulich)
> - remove leftover dead code (Jan Beulich)
> - order the monitor events numerically (Jan Beulich)
> - remove VM_EVENT_DESC_INVALID (Jan Beulich)
> - crash the domain if the descriptor access instruction can't be
>   emulated (Jan Beulich)
> - call hvm_inject_event without checking for pending events (Andrew
>   Cooper)
> - introduce structures for the VM-Exit instruction information used for
>   the descriptor instructions and use fewer magic numbers (Andrew
>   Cooper, Jan Beulich)

You've forgotten Wei Liu's ack for the first version of the patch.

For the vm_event bits:

Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>


Thanks,
Razvan

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] x86/monitor: add support for descriptor access events
  2017-04-04 10:52 ` Razvan Cojocaru
@ 2017-04-04 11:46   ` Adrian Pop
  0 siblings, 0 replies; 14+ messages in thread
From: Adrian Pop @ 2017-04-04 11:46 UTC (permalink / raw)
  To: Razvan Cojocaru
  Cc: Kevin Tian, Tamas K Lengyel, Wei Liu, Jan Beulich, Jun Nakajima,
	Andrew Cooper, Ian Jackson, Suravee Suthikulpanit, xen-devel,
	Boris Ostrovsky

On Tue, Apr 04, 2017 at 01:52:24PM +0300, Razvan Cojocaru wrote:
> You've forgotten Wei Liu's ack for the first version of the patch.
> 
> For the vm_event bits:
> 
> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

Okay, will include them next time.  Thanks!

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] x86/monitor: add support for descriptor access events
  2017-04-04  9:57 [PATCH v2] x86/monitor: add support for descriptor access events Adrian Pop
  2017-04-04 10:52 ` Razvan Cojocaru
@ 2017-04-04 13:23 ` Boris Ostrovsky
  2017-04-04 13:45   ` Adrian Pop
  2017-04-04 15:26 ` Andrew Cooper
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Boris Ostrovsky @ 2017-04-04 13:23 UTC (permalink / raw)
  To: Adrian Pop, xen-devel
  Cc: Kevin Tian, Tamas K Lengyel, Wei Liu, Suravee Suthikulpanit,
	Razvan Cojocaru, Andrew Cooper, Ian Jackson, Jan Beulich,
	Jun Nakajima

On 04/04/2017 05:57 AM, Adrian Pop wrote:
> Adds monitor support for descriptor access events (reads & writes of
> IDTR/GDTR/LDTR/TR) for the x86 architecture (VMX and SVM).
>
> Signed-off-by: Adrian Pop <apop@bitdefender.com>


> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -856,6 +856,23 @@ static void svm_set_rdtsc_exiting(struct vcpu *v, bool_t enable)
>      vmcb_set_general2_intercepts(vmcb, general2_intercepts);
>  }
>  
> +static void svm_set_descriptor_access_exiting(struct vcpu *v, bool enable)
> +{
> +    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> +    u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
> +    u32 mask = GENERAL1_INTERCEPT_IDTR_READ | GENERAL1_INTERCEPT_GDTR_READ \
> +            | GENERAL1_INTERCEPT_LDTR_READ | GENERAL1_INTERCEPT_TR_READ \
> +            | GENERAL1_INTERCEPT_IDTR_WRITE | GENERAL1_INTERCEPT_GDTR_WRITE \
> +            | GENERAL1_INTERCEPT_LDTR_WRITE | GENERAL1_INTERCEPT_TR_WRITE;

I didn't notice this last time --- there is no need for line
continuation here.

With that fixed,

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>eason =
%#"PRIx64", "


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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] x86/monitor: add support for descriptor access events
  2017-04-04 13:23 ` Boris Ostrovsky
@ 2017-04-04 13:45   ` Adrian Pop
  0 siblings, 0 replies; 14+ messages in thread
From: Adrian Pop @ 2017-04-04 13:45 UTC (permalink / raw)
  To: Boris Ostrovsky
  Cc: Kevin Tian, Tamas K Lengyel, Wei Liu, Jun Nakajima,
	Razvan Cojocaru, Andrew Cooper, Ian Jackson, Jan Beulich,
	Suravee Suthikulpanit, xen-devel

On Tue, Apr 04, 2017 at 09:23:28AM -0400, Boris Ostrovsky wrote:
> On 04/04/2017 05:57 AM, Adrian Pop wrote:
> > Adds monitor support for descriptor access events (reads & writes of
> > IDTR/GDTR/LDTR/TR) for the x86 architecture (VMX and SVM).
> >
> > Signed-off-by: Adrian Pop <apop@bitdefender.com>
> 
> 
> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -856,6 +856,23 @@ static void svm_set_rdtsc_exiting(struct vcpu *v, bool_t enable)
> >      vmcb_set_general2_intercepts(vmcb, general2_intercepts);
> >  }
> >  
> > +static void svm_set_descriptor_access_exiting(struct vcpu *v, bool enable)
> > +{
> > +    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> > +    u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
> > +    u32 mask = GENERAL1_INTERCEPT_IDTR_READ | GENERAL1_INTERCEPT_GDTR_READ \
> > +            | GENERAL1_INTERCEPT_LDTR_READ | GENERAL1_INTERCEPT_TR_READ \
> > +            | GENERAL1_INTERCEPT_IDTR_WRITE | GENERAL1_INTERCEPT_GDTR_WRITE \
> > +            | GENERAL1_INTERCEPT_LDTR_WRITE | GENERAL1_INTERCEPT_TR_WRITE;
> 
> I didn't notice this last time --- there is no need for line
> continuation here.
> 
> With that fixed,
> 
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Oh, yes.  I'll remove the escapes for the next version.  Thank you!

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] x86/monitor: add support for descriptor access events
  2017-04-04  9:57 [PATCH v2] x86/monitor: add support for descriptor access events Adrian Pop
  2017-04-04 10:52 ` Razvan Cojocaru
  2017-04-04 13:23 ` Boris Ostrovsky
@ 2017-04-04 15:26 ` Andrew Cooper
  2017-04-04 17:11   ` Adrian Pop
  2017-04-05  7:13 ` Tian, Kevin
  2017-04-05 14:26 ` Jan Beulich
  4 siblings, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2017-04-04 15:26 UTC (permalink / raw)
  To: Adrian Pop, xen-devel
  Cc: Kevin Tian, Tamas K Lengyel, Wei Liu, Suravee Suthikulpanit,
	Razvan Cojocaru, Ian Jackson, Jan Beulich, Jun Nakajima,
	Boris Ostrovsky

On 04/04/17 10:57, Adrian Pop wrote:
> diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
> index f5cd245771..d60e4afd0c 100644
> --- a/xen/arch/x86/hvm/monitor.c
> +++ b/xen/arch/x86/hvm/monitor.c
> @@ -72,6 +72,28 @@ void hvm_monitor_msr(unsigned int msr, uint64_t value)
>      }
>  }
>  
> +void hvm_monitor_descriptor_access(uint64_t exit_info,
> +                                   uint64_t vmx_exit_qualification,
> +                                   uint8_t descriptor, bool is_write)
> +{
> +    struct vcpu *curr = current;
> +    vm_event_request_t req = {
> +        .reason = VM_EVENT_REASON_DESCRIPTOR_ACCESS,
> +        .u.desc_access.descriptor = descriptor,
> +        .u.desc_access.is_write = is_write,
> +    };

Newline here

> +    if ( cpu_has_vmx )
> +    {
> +        req.u.desc_access.arch.vmx.instr_info = exit_info;
> +        req.u.desc_access.arch.vmx.exit_qualification = vmx_exit_qualification;
> +    }
> +    else
> +    {
> +        req.u.desc_access.arch.svm.exitinfo = exit_info;
> +    }

And here please.

> +    monitor_traps(curr, 1, &req);
> +}
> +
>  static inline unsigned long gfn_of_rip(unsigned long rip)
>  {
>      struct vcpu *curr = current;
> diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
> index 2b781ab120..b00ba52443 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -628,4 +628,46 @@ typedef struct {
>      u16 eptp_index;
>  } ve_info_t;
>  
> +/* VM-Exit instruction info for LIDT, LGDT, SIDT, SGDT */
> +typedef union idt_or_gdt_instr_info {
> +    unsigned long raw;
> +    struct {
> +        unsigned long scaling   :2,  /* bits 0:1 - Scaling */
> +                                :5,  /* bits 6:2 - Undefined */
> +        addr_size               :3,  /* bits 9:7 - Address size */
> +                                :1,  /* bit 10 - Cleared to 0 */
> +        operand_size            :1,  /* bit 11 - Operand size */
> +                                :3,  /* bits 14:12 - Undefined */
> +        segment_reg             :3,  /* bits 17:15 - Segment register */
> +        index_reg               :4,  /* bits 21:18 - Index register */
> +        index_reg_invalid       :1,  /* bit 22 - Index register invalid */
> +        base_reg                :4,  /* bits 26:23 - Base register */
> +        base_reg_invalid        :1,  /* bit 27 - Base register invalid */
> +        instr_identity          :1,  /* bit 28 - 0:GDT, 1:IDT */
> +        instr_write             :1,  /* bit 29 - 0:store, 1:load */
> +                                :2;  /* bits 30:31 - Undefined */

I think you need a :32 /* undefined */ in each of these, to avoid
breaking the Clang build, which cares that each half of the union have
the same bit size.

All of these can be fixed on commit if there are no other issues. 
Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] x86/monitor: add support for descriptor access events
  2017-04-04 15:26 ` Andrew Cooper
@ 2017-04-04 17:11   ` Adrian Pop
  0 siblings, 0 replies; 14+ messages in thread
From: Adrian Pop @ 2017-04-04 17:11 UTC (permalink / raw)
  To: Andrew Cooper, xen-devel
  Cc: Kevin Tian, Tamas K Lengyel, Wei Liu, Suravee Suthikulpanit,
	Razvan Cojocaru, Ian Jackson, Jan Beulich, Jun Nakajima,
	Boris Ostrovsky

On Tue, Apr 04, 2017 at 04:26:24PM +0100, Andrew Cooper wrote:
> On 04/04/17 10:57, Adrian Pop wrote:
> > diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
> > index f5cd245771..d60e4afd0c 100644
> > --- a/xen/arch/x86/hvm/monitor.c
> > +++ b/xen/arch/x86/hvm/monitor.c
> > @@ -72,6 +72,28 @@ void hvm_monitor_msr(unsigned int msr, uint64_t value)
> >      }
> >  }
> >  
> > +void hvm_monitor_descriptor_access(uint64_t exit_info,
> > +                                   uint64_t vmx_exit_qualification,
> > +                                   uint8_t descriptor, bool is_write)
> > +{
> > +    struct vcpu *curr = current;
> > +    vm_event_request_t req = {
> > +        .reason = VM_EVENT_REASON_DESCRIPTOR_ACCESS,
> > +        .u.desc_access.descriptor = descriptor,
> > +        .u.desc_access.is_write = is_write,
> > +    };
> 
> Newline here
 
Ok.

> > +    if ( cpu_has_vmx )
> > +    {
> > +        req.u.desc_access.arch.vmx.instr_info = exit_info;
> > +        req.u.desc_access.arch.vmx.exit_qualification = vmx_exit_qualification;
> > +    }
> > +    else
> > +    {
> > +        req.u.desc_access.arch.svm.exitinfo = exit_info;
> > +    }
> 
> And here please.

Ok.

> > +    monitor_traps(curr, 1, &req);
> > +}
> > +
> >  static inline unsigned long gfn_of_rip(unsigned long rip)
> >  {
> >      struct vcpu *curr = current;
> > diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
> > index 2b781ab120..b00ba52443 100644
> > --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> > +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> > @@ -628,4 +628,46 @@ typedef struct {
> >      u16 eptp_index;
> >  } ve_info_t;
> >  
> > +/* VM-Exit instruction info for LIDT, LGDT, SIDT, SGDT */
> > +typedef union idt_or_gdt_instr_info {
> > +    unsigned long raw;
> > +    struct {
> > +        unsigned long scaling   :2,  /* bits 0:1 - Scaling */
> > +                                :5,  /* bits 6:2 - Undefined */
> > +        addr_size               :3,  /* bits 9:7 - Address size */
> > +                                :1,  /* bit 10 - Cleared to 0 */
> > +        operand_size            :1,  /* bit 11 - Operand size */
> > +                                :3,  /* bits 14:12 - Undefined */
> > +        segment_reg             :3,  /* bits 17:15 - Segment register */
> > +        index_reg               :4,  /* bits 21:18 - Index register */
> > +        index_reg_invalid       :1,  /* bit 22 - Index register invalid */
> > +        base_reg                :4,  /* bits 26:23 - Base register */
> > +        base_reg_invalid        :1,  /* bit 27 - Base register invalid */
> > +        instr_identity          :1,  /* bit 28 - 0:GDT, 1:IDT */
> > +        instr_write             :1,  /* bit 29 - 0:store, 1:load */
> > +                                :2;  /* bits 30:31 - Undefined */
> 
> I think you need a :32 /* undefined */ in each of these, to avoid
> breaking the Clang build, which cares that each half of the union have
> the same bit size.

All right.

> All of these can be fixed on commit if there are no other issues. 
> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks!

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] x86/monitor: add support for descriptor access events
  2017-04-04  9:57 [PATCH v2] x86/monitor: add support for descriptor access events Adrian Pop
                   ` (2 preceding siblings ...)
  2017-04-04 15:26 ` Andrew Cooper
@ 2017-04-05  7:13 ` Tian, Kevin
  2017-04-05 14:26 ` Jan Beulich
  4 siblings, 0 replies; 14+ messages in thread
From: Tian, Kevin @ 2017-04-05  7:13 UTC (permalink / raw)
  To: Adrian Pop, xen-devel
  Cc: Tamas K Lengyel, Wei Liu, Suravee Suthikulpanit, Razvan Cojocaru,
	Andrew Cooper, Ian Jackson, Jan Beulich, Nakajima, Jun,
	Boris Ostrovsky

> From: Adrian Pop [mailto:apop@bitdefender.com]
> Sent: Tuesday, April 4, 2017 5:58 PM
> 
> Adds monitor support for descriptor access events (reads & writes of
> IDTR/GDTR/LDTR/TR) for the x86 architecture (VMX and SVM).
> 
> Signed-off-by: Adrian Pop <apop@bitdefender.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] x86/monitor: add support for descriptor access events
  2017-04-04  9:57 [PATCH v2] x86/monitor: add support for descriptor access events Adrian Pop
                   ` (3 preceding siblings ...)
  2017-04-05  7:13 ` Tian, Kevin
@ 2017-04-05 14:26 ` Jan Beulich
  2017-04-06  8:59   ` Adrian Pop
  4 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2017-04-05 14:26 UTC (permalink / raw)
  To: Adrian Pop
  Cc: Kevin Tian, Tamas K Lengyel, Wei Liu, Jun Nakajima,
	Razvan Cojocaru, Andrew Cooper, Ian Jackson,
	Suravee Suthikulpanit, xen-devel, Boris Ostrovsky

>>> On 04.04.17 at 11:57, <apop@bitdefender.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3572,6 +3572,43 @@ gp_fault:
>      return X86EMUL_EXCEPTION;
>  }
>  
> +int hvm_descriptor_access_intercept(uint64_t exit_info,
> +                                    uint64_t vmx_exit_qualification,
> +                                    uint8_t descriptor, bool is_write)

Why uint8_t?

> +{
> +    struct vcpu *curr = current;
> +    struct domain *currd = curr->domain;
> +    int rc;
> +
> +    if ( currd->arch.monitor.descriptor_access_enabled )
> +    {
> +        ASSERT(curr->arch.vm_event);
> +        hvm_monitor_descriptor_access(exit_info, vmx_exit_qualification,
> +                                      descriptor, is_write);
> +    }
> +    else
> +    {
> +        struct hvm_emulate_ctxt ctxt = {};
> +
> +        hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
> +        rc = hvm_emulate_one(&ctxt);
> +        switch ( rc )

You don't really need to go through a local variable here.

> --- a/xen/arch/x86/hvm/monitor.c
> +++ b/xen/arch/x86/hvm/monitor.c
> @@ -72,6 +72,28 @@ void hvm_monitor_msr(unsigned int msr, uint64_t value)
>      }
>  }
>  
> +void hvm_monitor_descriptor_access(uint64_t exit_info,
> +                                   uint64_t vmx_exit_qualification,
> +                                   uint8_t descriptor, bool is_write)
> +{
> +    struct vcpu *curr = current;
> +    vm_event_request_t req = {
> +        .reason = VM_EVENT_REASON_DESCRIPTOR_ACCESS,
> +        .u.desc_access.descriptor = descriptor,
> +        .u.desc_access.is_write = is_write,
> +    };
> +    if ( cpu_has_vmx )
> +    {
> +        req.u.desc_access.arch.vmx.instr_info = exit_info;
> +        req.u.desc_access.arch.vmx.exit_qualification = vmx_exit_qualification;
> +    }
> +    else
> +    {
> +        req.u.desc_access.arch.svm.exitinfo = exit_info;
> +    }
> +    monitor_traps(curr, 1, &req);

true

> @@ -3361,6 +3376,40 @@ static void vmx_handle_xrstors(void)
>      domain_crash(current->domain);
>  }
>  
> +static void vmx_handle_idt_or_gdt(idt_or_gdt_instr_info_t instr_info,
> +                                  uint64_t exit_qualification)
> +{
> +    uint8_t descriptor = instr_info.instr_identity
> +        ? VM_EVENT_DESC_IDTR : VM_EVENT_DESC_GDTR;
> +
> +    hvm_descriptor_access_intercept(instr_info.raw, exit_qualification,
> +                                    descriptor, instr_info.instr_write);
> +}
> +
> +static void vmx_handle_ldt_or_tr(ldt_or_tr_instr_info_t instr_info,
> +                                 uint64_t exit_qualification)
> +{
> +    uint8_t descriptor = instr_info.instr_identity
> +        ? VM_EVENT_DESC_TR : VM_EVENT_DESC_LDTR;
> +
> +    hvm_descriptor_access_intercept(instr_info.raw, exit_qualification,
> +                                    descriptor, instr_info.instr_write);
> +}

I think these should be folded into their only caller (at once
eliminating the need to make those unions transparent ones).
And again - why uint8_t?

Jan


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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] x86/monitor: add support for descriptor access events
  2017-04-05 14:26 ` Jan Beulich
@ 2017-04-06  8:59   ` Adrian Pop
  2017-04-06  9:20     ` Jan Beulich
  0 siblings, 1 reply; 14+ messages in thread
From: Adrian Pop @ 2017-04-06  8:59 UTC (permalink / raw)
  To: Jan Beulich, xen-devel
  Cc: Kevin Tian, Tamas K Lengyel, Wei Liu, Jun Nakajima,
	Razvan Cojocaru, Andrew Cooper, Ian Jackson,
	Suravee Suthikulpanit, Boris Ostrovsky

Hello,

On Wed, Apr 05, 2017 at 08:26:27AM -0600, Jan Beulich wrote:
> >>> On 04.04.17 at 11:57, <apop@bitdefender.com> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -3572,6 +3572,43 @@ gp_fault:
> >      return X86EMUL_EXCEPTION;
> >  }
> >  
> > +int hvm_descriptor_access_intercept(uint64_t exit_info,
> > +                                    uint64_t vmx_exit_qualification,
> > +                                    uint8_t descriptor, bool is_write)
> 
> Why uint8_t?

The descriptor type from struct vm_event_desc_access is uint8_t since
there are only 4 possible descriptors:

> > +#define VM_EVENT_DESC_IDTR           1
> > +#define VM_EVENT_DESC_GDTR           2
> > +#define VM_EVENT_DESC_LDTR           3
> > +#define VM_EVENT_DESC_TR             4

Should it be something else?

> > +{
> > +    struct vcpu *curr = current;
> > +    struct domain *currd = curr->domain;
> > +    int rc;
> > +
> > +    if ( currd->arch.monitor.descriptor_access_enabled )
> > +    {
> > +        ASSERT(curr->arch.vm_event);
> > +        hvm_monitor_descriptor_access(exit_info, vmx_exit_qualification,
> > +                                      descriptor, is_write);
> > +    }
> > +    else
> > +    {
> > +        struct hvm_emulate_ctxt ctxt = {};
> > +
> > +        hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
> > +        rc = hvm_emulate_one(&ctxt);
> > +        switch ( rc )
> 
> You don't really need to go through a local variable here.

Ok.
 
> > --- a/xen/arch/x86/hvm/monitor.c
> > +++ b/xen/arch/x86/hvm/monitor.c
> > @@ -72,6 +72,28 @@ void hvm_monitor_msr(unsigned int msr, uint64_t value)
> >      }
> >  }
> >  
> > +void hvm_monitor_descriptor_access(uint64_t exit_info,
> > +                                   uint64_t vmx_exit_qualification,
> > +                                   uint8_t descriptor, bool is_write)
> > +{
> > +    struct vcpu *curr = current;
> > +    vm_event_request_t req = {
> > +        .reason = VM_EVENT_REASON_DESCRIPTOR_ACCESS,
> > +        .u.desc_access.descriptor = descriptor,
> > +        .u.desc_access.is_write = is_write,
> > +    };
> > +    if ( cpu_has_vmx )
> > +    {
> > +        req.u.desc_access.arch.vmx.instr_info = exit_info;
> > +        req.u.desc_access.arch.vmx.exit_qualification = vmx_exit_qualification;
> > +    }
> > +    else
> > +    {
> > +        req.u.desc_access.arch.svm.exitinfo = exit_info;
> > +    }
> > +    monitor_traps(curr, 1, &req);
> 
> true

Ok.

> > @@ -3361,6 +3376,40 @@ static void vmx_handle_xrstors(void)
> >      domain_crash(current->domain);
> >  }
> >  
> > +static void vmx_handle_idt_or_gdt(idt_or_gdt_instr_info_t instr_info,
> > +                                  uint64_t exit_qualification)
> > +{
> > +    uint8_t descriptor = instr_info.instr_identity
> > +        ? VM_EVENT_DESC_IDTR : VM_EVENT_DESC_GDTR;
> > +
> > +    hvm_descriptor_access_intercept(instr_info.raw, exit_qualification,
> > +                                    descriptor, instr_info.instr_write);
> > +}
> > +
> > +static void vmx_handle_ldt_or_tr(ldt_or_tr_instr_info_t instr_info,
> > +                                 uint64_t exit_qualification)
> > +{
> > +    uint8_t descriptor = instr_info.instr_identity
> > +        ? VM_EVENT_DESC_TR : VM_EVENT_DESC_LDTR;
> > +
> > +    hvm_descriptor_access_intercept(instr_info.raw, exit_qualification,
> > +                                    descriptor, instr_info.instr_write);
> > +}
> 
> I think these should be folded into their only caller (at once
> eliminating the need to make those unions transparent ones).

Ok.

> And again - why uint8_t?

Same as above.

> Jan

Thank you!

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] x86/monitor: add support for descriptor access events
  2017-04-06  8:59   ` Adrian Pop
@ 2017-04-06  9:20     ` Jan Beulich
  2017-04-06  9:37       ` Adrian Pop
  0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2017-04-06  9:20 UTC (permalink / raw)
  To: Adrian Pop
  Cc: Kevin Tian, Tamas K Lengyel, Wei Liu, Jun Nakajima,
	Razvan Cojocaru, Andrew Cooper, Ian Jackson,
	Suravee Suthikulpanit, xen-devel, Boris Ostrovsky

>>> On 06.04.17 at 10:59, <apop@bitdefender.com> wrote:
> On Wed, Apr 05, 2017 at 08:26:27AM -0600, Jan Beulich wrote:
>> >>> On 04.04.17 at 11:57, <apop@bitdefender.com> wrote:
>> > --- a/xen/arch/x86/hvm/hvm.c
>> > +++ b/xen/arch/x86/hvm/hvm.c
>> > @@ -3572,6 +3572,43 @@ gp_fault:
>> >      return X86EMUL_EXCEPTION;
>> >  }
>> >  
>> > +int hvm_descriptor_access_intercept(uint64_t exit_info,
>> > +                                    uint64_t vmx_exit_qualification,
>> > +                                    uint8_t descriptor, bool is_write)
>> 
>> Why uint8_t?
> 
> The descriptor type from struct vm_event_desc_access is uint8_t since
> there are only 4 possible descriptors:
> 
>> > +#define VM_EVENT_DESC_IDTR           1
>> > +#define VM_EVENT_DESC_GDTR           2
>> > +#define VM_EVENT_DESC_LDTR           3
>> > +#define VM_EVENT_DESC_TR             4
> 
> Should it be something else?

Well, you should avoid fixed width types where they're not really
needed (their use should signal a true dependency on the specified
width). "unsigned int" would be quite fine here afaict.

Jan


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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] x86/monitor: add support for descriptor access events
  2017-04-06  9:20     ` Jan Beulich
@ 2017-04-06  9:37       ` Adrian Pop
  2017-04-06 14:09         ` Jan Beulich
  0 siblings, 1 reply; 14+ messages in thread
From: Adrian Pop @ 2017-04-06  9:37 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Kevin Tian, Tamas K Lengyel, Wei Liu, Jun Nakajima,
	Razvan Cojocaru, Andrew Cooper, Ian Jackson,
	Suravee Suthikulpanit, xen-devel, Boris Ostrovsky

On Thu, Apr 06, 2017 at 03:20:21AM -0600, Jan Beulich wrote:
> >>> On 06.04.17 at 10:59, <apop@bitdefender.com> wrote:
> > On Wed, Apr 05, 2017 at 08:26:27AM -0600, Jan Beulich wrote:
> >> >>> On 04.04.17 at 11:57, <apop@bitdefender.com> wrote:
> >> > --- a/xen/arch/x86/hvm/hvm.c
> >> > +++ b/xen/arch/x86/hvm/hvm.c
> >> > @@ -3572,6 +3572,43 @@ gp_fault:
> >> >      return X86EMUL_EXCEPTION;
> >> >  }
> >> >  
> >> > +int hvm_descriptor_access_intercept(uint64_t exit_info,
> >> > +                                    uint64_t vmx_exit_qualification,
> >> > +                                    uint8_t descriptor, bool is_write)
> >> 
> >> Why uint8_t?
> > 
> > The descriptor type from struct vm_event_desc_access is uint8_t since
> > there are only 4 possible descriptors:
> > 
> >> > +#define VM_EVENT_DESC_IDTR           1
> >> > +#define VM_EVENT_DESC_GDTR           2
> >> > +#define VM_EVENT_DESC_LDTR           3
> >> > +#define VM_EVENT_DESC_TR             4
> > 
> > Should it be something else?
> 
> Well, you should avoid fixed width types where they're not really
> needed (their use should signal a true dependency on the specified
> width). "unsigned int" would be quite fine here afaict.

So should it be changed in the struct definition as well?

> >> > +struct vm_event_desc_access {
> >> > +    union {
> >> > +        struct {
> >> > +            uint32_t instr_info;         /* VMX: VMCS Instruction-Information */
> >> > +            uint32_t _pad1;
> >> > +            uint64_t exit_qualification; /* VMX: VMCS Exit Qualification */
> >> > +        } vmx;
> >> > +        struct {
> >> > +            uint64_t exitinfo;           /* SVM: VMCB EXITINFO */
> >> > +            uint64_t _pad2;
> >> > +        } svm;
> >> > +    } arch;
> >> > +    uint8_t descriptor;                  /* VM_EVENT_DESC_* */
> >> > +    uint8_t is_write;
> >> > +    uint8_t _pad[6];
> >> > +};

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] x86/monitor: add support for descriptor access events
  2017-04-06  9:37       ` Adrian Pop
@ 2017-04-06 14:09         ` Jan Beulich
  2017-04-07 10:01           ` Adrian Pop
  0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2017-04-06 14:09 UTC (permalink / raw)
  To: Adrian Pop
  Cc: Kevin Tian, Tamas K Lengyel, Wei Liu, Jun Nakajima,
	Razvan Cojocaru, Andrew Cooper, Ian Jackson,
	Suravee Suthikulpanit, xen-devel, Boris Ostrovsky

>>> On 06.04.17 at 11:37, <apop@bitdefender.com> wrote:
> On Thu, Apr 06, 2017 at 03:20:21AM -0600, Jan Beulich wrote:
>> >>> On 06.04.17 at 10:59, <apop@bitdefender.com> wrote:
>> > On Wed, Apr 05, 2017 at 08:26:27AM -0600, Jan Beulich wrote:
>> >> >>> On 04.04.17 at 11:57, <apop@bitdefender.com> wrote:
>> >> > --- a/xen/arch/x86/hvm/hvm.c
>> >> > +++ b/xen/arch/x86/hvm/hvm.c
>> >> > @@ -3572,6 +3572,43 @@ gp_fault:
>> >> >      return X86EMUL_EXCEPTION;
>> >> >  }
>> >> >  
>> >> > +int hvm_descriptor_access_intercept(uint64_t exit_info,
>> >> > +                                    uint64_t vmx_exit_qualification,
>> >> > +                                    uint8_t descriptor, bool is_write)
>> >> 
>> >> Why uint8_t?
>> > 
>> > The descriptor type from struct vm_event_desc_access is uint8_t since
>> > there are only 4 possible descriptors:
>> > 
>> >> > +#define VM_EVENT_DESC_IDTR           1
>> >> > +#define VM_EVENT_DESC_GDTR           2
>> >> > +#define VM_EVENT_DESC_LDTR           3
>> >> > +#define VM_EVENT_DESC_TR             4
>> > 
>> > Should it be something else?
>> 
>> Well, you should avoid fixed width types where they're not really
>> needed (their use should signal a true dependency on the specified
>> width). "unsigned int" would be quite fine here afaict.
> 
> So should it be changed in the struct definition as well?

You mean in the public interface? No, there you _need_ to use fixed
width types.

Jan


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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] x86/monitor: add support for descriptor access events
  2017-04-06 14:09         ` Jan Beulich
@ 2017-04-07 10:01           ` Adrian Pop
  0 siblings, 0 replies; 14+ messages in thread
From: Adrian Pop @ 2017-04-07 10:01 UTC (permalink / raw)
  To: Jan Beulich, xen-devel
  Cc: Kevin Tian, Tamas K Lengyel, Wei Liu, Jun Nakajima,
	Razvan Cojocaru, Andrew Cooper, Ian Jackson,
	Suravee Suthikulpanit, Boris Ostrovsky

On Thu, Apr 06, 2017 at 08:09:23AM -0600, Jan Beulich wrote:
> >>> On 06.04.17 at 11:37, <apop@bitdefender.com> wrote:
> > On Thu, Apr 06, 2017 at 03:20:21AM -0600, Jan Beulich wrote:
> >> >>> On 06.04.17 at 10:59, <apop@bitdefender.com> wrote:
> >> > On Wed, Apr 05, 2017 at 08:26:27AM -0600, Jan Beulich wrote:
> >> >> >>> On 04.04.17 at 11:57, <apop@bitdefender.com> wrote:
> >> >> > --- a/xen/arch/x86/hvm/hvm.c
> >> >> > +++ b/xen/arch/x86/hvm/hvm.c
> >> >> > @@ -3572,6 +3572,43 @@ gp_fault:
> >> >> >      return X86EMUL_EXCEPTION;
> >> >> >  }
> >> >> >  
> >> >> > +int hvm_descriptor_access_intercept(uint64_t exit_info,
> >> >> > +                                    uint64_t vmx_exit_qualification,
> >> >> > +                                    uint8_t descriptor, bool is_write)
> >> >> 
> >> >> Why uint8_t?
> >> > 
> >> > The descriptor type from struct vm_event_desc_access is uint8_t since
> >> > there are only 4 possible descriptors:
> >> > 
> >> >> > +#define VM_EVENT_DESC_IDTR           1
> >> >> > +#define VM_EVENT_DESC_GDTR           2
> >> >> > +#define VM_EVENT_DESC_LDTR           3
> >> >> > +#define VM_EVENT_DESC_TR             4
> >> > 
> >> > Should it be something else?
> >> 
> >> Well, you should avoid fixed width types where they're not really
> >> needed (their use should signal a true dependency on the specified
> >> width). "unsigned int" would be quite fine here afaict.
> > 
> > So should it be changed in the struct definition as well?
> 
> You mean in the public interface? No, there you _need_ to use fixed
> width types.

Ok, I'll make the changes and resend.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2017-04-07 11:21 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-04  9:57 [PATCH v2] x86/monitor: add support for descriptor access events Adrian Pop
2017-04-04 10:52 ` Razvan Cojocaru
2017-04-04 11:46   ` Adrian Pop
2017-04-04 13:23 ` Boris Ostrovsky
2017-04-04 13:45   ` Adrian Pop
2017-04-04 15:26 ` Andrew Cooper
2017-04-04 17:11   ` Adrian Pop
2017-04-05  7:13 ` Tian, Kevin
2017-04-05 14:26 ` Jan Beulich
2017-04-06  8:59   ` Adrian Pop
2017-04-06  9:20     ` Jan Beulich
2017-04-06  9:37       ` Adrian Pop
2017-04-06 14:09         ` Jan Beulich
2017-04-07 10:01           ` Adrian Pop

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.