[PATCH 0/3] x86/HVM: misc improvements

[PATCH 0/3] x86/HVM: misc improvements

[Jan Beulich]

1: introduce hvm_get_cpl() and respective hook
2: support (emulate) UMIP
3: prefer structure assignment for seg reg copying

Signed-off-by: Jan Beulich <jbeulich@suse.com>


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

[PATCH 1/3] x86/HVM: introduce hvm_get_cpl() and respective hook

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 11871 bytes --]

... instead of repeating the same code in various places (and getting
it  wrong in some of them).

In vmx_inst_check_privilege() also stop open coding vmx_get_x86_mode().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2744,7 +2744,7 @@ static void hvm_unmap_entry(void *p)
 static int hvm_load_segment_selector(
     enum x86_segment seg, uint16_t sel, unsigned int eflags)
 {
-    struct segment_register desctab, cs, segr;
+    struct segment_register desctab, segr;
     struct desc_struct *pdesc, desc;
     u8 dpl, rpl, cpl;
     bool_t writable;
@@ -2776,7 +2776,6 @@ static int hvm_load_segment_selector(
     if ( (seg == x86_seg_ldtr) && (sel & 4) )
         goto fail;
 
-    hvm_get_segment_register(v, x86_seg_cs, &cs);
     hvm_get_segment_register(
         v, (sel & 4) ? x86_seg_ldtr : x86_seg_gdtr, &desctab);
 
@@ -2801,7 +2800,7 @@ static int hvm_load_segment_selector(
 
         dpl = (desc.b >> 13) & 3;
         rpl = sel & 3;
-        cpl = cs.sel & 3;
+        cpl = hvm_get_cpl(v);
 
         switch ( seg )
         {
@@ -3708,16 +3707,10 @@ void hvm_cpuid(unsigned int input, unsig
 
 bool hvm_check_cpuid_faulting(struct vcpu *v)
 {
-    struct segment_register sreg;
-
     if ( !v->arch.cpuid_faulting )
         return false;
 
-    hvm_get_segment_register(v, x86_seg_ss, &sreg);
-    if ( sreg.attr.fields.dpl == 0 )
-        return false;
-
-    return true;
+    return hvm_get_cpl(v) > 0;
 }
 
 static uint64_t _hvm_rdtsc_intercept(void)
@@ -3729,13 +3722,10 @@ static uint64_t _hvm_rdtsc_intercept(voi
     if ( currd->arch.vtsc )
         switch ( hvm_guest_x86_mode(curr) )
         {
-            struct segment_register sreg;
-
         case 8:
         case 4:
         case 2:
-            hvm_get_segment_register(curr, x86_seg_ss, &sreg);
-            if ( unlikely(sreg.attr.fields.dpl) )
+            if ( unlikely(hvm_get_cpl(curr)) )
             {
         case 1:
                 currd->arch.vtsc_usercount++;
@@ -4303,7 +4293,6 @@ int hvm_do_hypercall(struct cpu_user_reg
 {
     struct vcpu *curr = current;
     struct domain *currd = curr->domain;
-    struct segment_register sreg;
     int mode = hvm_guest_x86_mode(curr);
     unsigned long eax = regs->_eax;
 
@@ -4314,8 +4303,7 @@ int hvm_do_hypercall(struct cpu_user_reg
         /* Fallthrough to permission check. */
     case 4:
     case 2:
-        hvm_get_segment_register(curr, x86_seg_ss, &sreg);
-        if ( unlikely(sreg.attr.fields.dpl) )
+        if ( unlikely(hvm_get_cpl(curr)) )
         {
     default:
             regs->eax = -EPERM;
--- a/xen/arch/x86/hvm/monitor.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -78,8 +78,7 @@ static inline unsigned long gfn_of_rip(u
     struct segment_register sreg;
     uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
 
-    hvm_get_segment_register(curr, x86_seg_ss, &sreg);
-    if ( sreg.attr.fields.dpl == 3 )
+    if ( hvm_get_cpl(curr) == 3 )
         pfec |= PFEC_user_mode;
 
     hvm_get_segment_register(curr, x86_seg_cs, &sreg);
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -616,6 +616,11 @@ static void svm_sync_vmcb(struct vcpu *v
     svm_vmsave(arch_svm->vmcb);
 }
 
+static unsigned int svm_get_cpl(struct vcpu *v)
+{
+    return vmcb_get_cpl(v->arch.hvm_svm.vmcb);
+}
+
 static void svm_get_segment_register(struct vcpu *v, enum x86_segment seg,
                                      struct segment_register *reg)
 {
@@ -2231,6 +2236,7 @@ static struct hvm_function_table __initd
     .get_interrupt_shadow = svm_get_interrupt_shadow,
     .set_interrupt_shadow = svm_set_interrupt_shadow,
     .guest_x86_mode       = svm_guest_x86_mode,
+    .get_cpl              = svm_get_cpl,
     .get_segment_register = svm_get_segment_register,
     .set_segment_register = svm_set_segment_register,
     .get_shadow_gs_base   = svm_get_shadow_gs_base,
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -557,7 +557,7 @@ static void vmx_update_guest_vendor(stru
     vmx_vmcs_exit(v);
 }
 
-static int vmx_guest_x86_mode(struct vcpu *v)
+int vmx_guest_x86_mode(struct vcpu *v)
 {
     unsigned long cs_ar_bytes;
 
@@ -921,6 +921,26 @@ static void vmx_ctxt_switch_to(struct vc
 }
 
 
+unsigned int vmx_get_cpl(void)
+{
+    unsigned long attr;
+
+    __vmread(GUEST_SS_AR_BYTES, &attr);
+
+    return (attr >> 5) & 3;
+}
+
+static unsigned int _vmx_get_cpl(struct vcpu *v)
+{
+    unsigned int cpl;
+
+    vmx_vmcs_enter(v);
+    cpl = vmx_get_cpl();
+    vmx_vmcs_exit(v);
+
+    return cpl;
+}
+
 /* SDM volume 3b section 22.3.1.2: we can only enter virtual 8086 mode
  * if all of CS, SS, DS, ES, FS and GS are 16bit ring-3 data segments.
  * The guest thinks it's got ring-0 segments, so we need to fudge
@@ -2089,6 +2109,7 @@ static struct hvm_function_table __initd
     .get_interrupt_shadow = vmx_get_interrupt_shadow,
     .set_interrupt_shadow = vmx_set_interrupt_shadow,
     .guest_x86_mode       = vmx_guest_x86_mode,
+    .get_cpl              = _vmx_get_cpl,
     .get_segment_register = vmx_get_segment_register,
     .set_segment_register = vmx_set_segment_register,
     .get_shadow_gs_base   = vmx_get_shadow_gs_base,
@@ -3837,19 +3858,13 @@ void vmx_vmexit_handler(struct cpu_user_
     /* fall through */
     default:
     exit_and_crash:
-        {
-            struct segment_register ss;
-
-            gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n",
-                     exit_reason);
+        gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n", exit_reason);
 
-            hvm_get_segment_register(v, x86_seg_ss, &ss);
-            if ( ss.attr.fields.dpl )
-                hvm_inject_hw_exception(TRAP_invalid_op,
-                                        X86_EVENT_NO_EC);
-            else
-                domain_crash(v->domain);
-        }
+        if ( vmx_get_cpl() )
+            hvm_inject_hw_exception(TRAP_invalid_op,
+                                    X86_EVENT_NO_EC);
+        else
+            domain_crash(v->domain);
         break;
     }
 
@@ -3871,12 +3886,9 @@ out:
     if ( mode == 8 ? !is_canonical_address(regs->rip)
                    : regs->rip != regs->_eip )
     {
-        struct segment_register ss;
-
         gprintk(XENLOG_WARNING, "Bad rIP %lx for mode %u\n", regs->rip, mode);
 
-        hvm_get_segment_register(v, x86_seg_ss, &ss);
-        if ( ss.attr.fields.dpl )
+        if ( vmx_get_cpl() )
         {
             __vmread(VM_ENTRY_INTR_INFO, &intr_info);
             if ( !(intr_info & INTR_INFO_VALID_MASK) )
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -349,7 +349,6 @@ static inline u32 __n2_secondary_exec_co
 static int vmx_inst_check_privilege(struct cpu_user_regs *regs, int vmxop_check)
 {
     struct vcpu *v = current;
-    struct segment_register cs;
 
     if ( vmxop_check )
     {
@@ -360,15 +359,12 @@ static int vmx_inst_check_privilege(stru
     else if ( !vcpu_2_nvmx(v).vmxon_region_pa )
         goto invalid_op;
 
-    hvm_get_segment_register(v, x86_seg_cs, &cs);
-
-    if ( (regs->eflags & X86_EFLAGS_VM) ||
-         (hvm_long_mode_enabled(v) && cs.attr.fields.l == 0) )
+    if ( vmx_guest_x86_mode(v) < (hvm_long_mode_enabled(v) ? 8 : 2) )
         goto invalid_op;
     else if ( nestedhvm_vcpu_in_guestmode(v) )
         goto vmexit;
 
-    if ( (cs.sel & 3) > 0 )
+    if ( vmx_get_cpl() > 0 )
         goto gp_fault;
 
     return X86EMUL_OKAY;
@@ -413,16 +409,10 @@ static int decode_vmx_inst(struct cpu_us
     }
     else
     {
-        bool_t mode_64bit = 0;
+        bool mode_64bit = (vmx_guest_x86_mode(v) == 8);
 
         decode->type = VMX_INST_MEMREG_TYPE_MEMORY;
 
-        if ( hvm_long_mode_enabled(v) )
-        {
-            hvm_get_segment_register(v, x86_seg_cs, &seg);
-            mode_64bit = seg.attr.fields.l;
-        }
-
         if ( info.fields.segment > VMX_SREG_GS )
             goto gp_fault;
         hvm_get_segment_register(v, sreg_to_index[info.fields.segment], &seg);
--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -174,7 +174,6 @@ guest_walk_tables(struct vcpu *v, struct
 
     if ( is_hvm_domain(d) && !(pfec & PFEC_user_mode) )
     {
-        struct segment_register seg;
         const struct cpu_user_regs *regs = guest_cpu_user_regs();
 
         /* SMEP: kernel-mode instruction fetches from user-mode mappings
@@ -186,8 +185,6 @@ guest_walk_tables(struct vcpu *v, struct
         switch ( v->arch.smap_check_policy )
         {
         case SMAP_CHECK_HONOR_CPL_AC:
-            hvm_get_segment_register(v, x86_seg_ss, &seg);
-
             /*
              * SMAP: kernel-mode data accesses from user-mode mappings
              * should fault.
@@ -199,8 +196,7 @@ guest_walk_tables(struct vcpu *v, struct
              *   - Page fault in kernel mode
              */
             smap = hvm_smap_enabled(v) &&
-                   ((seg.attr.fields.dpl == 3) ||
-                    !(regs->eflags & X86_EFLAGS_AC));
+                   ((hvm_get_cpl(v) == 3) || !(regs->eflags & X86_EFLAGS_AC));
             break;
         case SMAP_CHECK_ENABLED:
             smap = hvm_smap_enabled(v);
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -1779,7 +1779,7 @@ void *sh_emulate_map_dest(struct vcpu *v
 #ifndef NDEBUG
     /* We don't emulate user-mode writes to page tables. */
     if ( has_hvm_container_domain(d)
-         ? hvm_get_seg_reg(x86_seg_ss, sh_ctxt)->attr.fields.dpl == 3
+         ? hvm_get_cpl(v) == 3
          : !guest_kernel_mode(v, guest_cpu_user_regs()) )
     {
         gdprintk(XENLOG_DEBUG, "User-mode write to pagetable reached "
--- a/xen/arch/x86/oprofile/xenoprof.c
+++ b/xen/arch/x86/oprofile/xenoprof.c
@@ -84,15 +84,12 @@ int xenoprofile_get_mode(struct vcpu *cu
 
     switch ( hvm_guest_x86_mode(curr) )
     {
-        struct segment_register ss;
-
     case 0: /* real mode */
         return 1;
     case 1: /* vm86 mode */
         return 0;
     default:
-        hvm_get_segment_register(curr, x86_seg_ss, &ss);
-        return (ss.sel & 3) != 3;
+        return hvm_get_cpl(curr) != 3;
     }
 }
 
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -117,6 +117,7 @@ struct hvm_function_table {
     unsigned int (*get_interrupt_shadow)(struct vcpu *v);
     void (*set_interrupt_shadow)(struct vcpu *v, unsigned int intr_shadow);
     int (*guest_x86_mode)(struct vcpu *v);
+    unsigned int (*get_cpl)(struct vcpu *v);
     void (*get_segment_register)(struct vcpu *v, enum x86_segment seg,
                                  struct segment_register *reg);
     void (*set_segment_register)(struct vcpu *v, enum x86_segment seg,
@@ -358,6 +359,12 @@ static inline void hvm_flush_guest_tlbs(
 void hvm_hypercall_page_initialise(struct domain *d,
                                    void *hypercall_page);
 
+static inline unsigned int
+hvm_get_cpl(struct vcpu *v)
+{
+    return hvm_funcs.get_cpl(v);
+}
+
 void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg,
                               struct segment_register *reg);
 void hvm_set_segment_register(struct vcpu *v, enum x86_segment seg,
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -550,6 +550,9 @@ static inline int __vmxon(u64 addr)
     return rc;
 }
 
+int vmx_guest_x86_mode(struct vcpu *v);
+unsigned int vmx_get_cpl(void);
+
 void vmx_inject_extint(int trap, uint8_t source);
 void vmx_inject_nmi(void);
 



[-- Attachment #2: x86-HVM-get-CPL.patch --]
[-- Type: text/plain, Size: 11923 bytes --]

x86/HVM: introduce hvm_get_cpl() and respective hook

... instead of repeating the same code in various places (and getting
it  wrong in some of them).

In vmx_inst_check_privilege() also stop open coding vmx_get_x86_mode().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2744,7 +2744,7 @@ static void hvm_unmap_entry(void *p)
 static int hvm_load_segment_selector(
     enum x86_segment seg, uint16_t sel, unsigned int eflags)
 {
-    struct segment_register desctab, cs, segr;
+    struct segment_register desctab, segr;
     struct desc_struct *pdesc, desc;
     u8 dpl, rpl, cpl;
     bool_t writable;
@@ -2776,7 +2776,6 @@ static int hvm_load_segment_selector(
     if ( (seg == x86_seg_ldtr) && (sel & 4) )
         goto fail;
 
-    hvm_get_segment_register(v, x86_seg_cs, &cs);
     hvm_get_segment_register(
         v, (sel & 4) ? x86_seg_ldtr : x86_seg_gdtr, &desctab);
 
@@ -2801,7 +2800,7 @@ static int hvm_load_segment_selector(
 
         dpl = (desc.b >> 13) & 3;
         rpl = sel & 3;
-        cpl = cs.sel & 3;
+        cpl = hvm_get_cpl(v);
 
         switch ( seg )
         {
@@ -3708,16 +3707,10 @@ void hvm_cpuid(unsigned int input, unsig
 
 bool hvm_check_cpuid_faulting(struct vcpu *v)
 {
-    struct segment_register sreg;
-
     if ( !v->arch.cpuid_faulting )
         return false;
 
-    hvm_get_segment_register(v, x86_seg_ss, &sreg);
-    if ( sreg.attr.fields.dpl == 0 )
-        return false;
-
-    return true;
+    return hvm_get_cpl(v) > 0;
 }
 
 static uint64_t _hvm_rdtsc_intercept(void)
@@ -3729,13 +3722,10 @@ static uint64_t _hvm_rdtsc_intercept(voi
     if ( currd->arch.vtsc )
         switch ( hvm_guest_x86_mode(curr) )
         {
-            struct segment_register sreg;
-
         case 8:
         case 4:
         case 2:
-            hvm_get_segment_register(curr, x86_seg_ss, &sreg);
-            if ( unlikely(sreg.attr.fields.dpl) )
+            if ( unlikely(hvm_get_cpl(curr)) )
             {
         case 1:
                 currd->arch.vtsc_usercount++;
@@ -4303,7 +4293,6 @@ int hvm_do_hypercall(struct cpu_user_reg
 {
     struct vcpu *curr = current;
     struct domain *currd = curr->domain;
-    struct segment_register sreg;
     int mode = hvm_guest_x86_mode(curr);
     unsigned long eax = regs->_eax;
 
@@ -4314,8 +4303,7 @@ int hvm_do_hypercall(struct cpu_user_reg
         /* Fallthrough to permission check. */
     case 4:
     case 2:
-        hvm_get_segment_register(curr, x86_seg_ss, &sreg);
-        if ( unlikely(sreg.attr.fields.dpl) )
+        if ( unlikely(hvm_get_cpl(curr)) )
         {
     default:
             regs->eax = -EPERM;
--- a/xen/arch/x86/hvm/monitor.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -78,8 +78,7 @@ static inline unsigned long gfn_of_rip(u
     struct segment_register sreg;
     uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
 
-    hvm_get_segment_register(curr, x86_seg_ss, &sreg);
-    if ( sreg.attr.fields.dpl == 3 )
+    if ( hvm_get_cpl(curr) == 3 )
         pfec |= PFEC_user_mode;
 
     hvm_get_segment_register(curr, x86_seg_cs, &sreg);
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -616,6 +616,11 @@ static void svm_sync_vmcb(struct vcpu *v
     svm_vmsave(arch_svm->vmcb);
 }
 
+static unsigned int svm_get_cpl(struct vcpu *v)
+{
+    return vmcb_get_cpl(v->arch.hvm_svm.vmcb);
+}
+
 static void svm_get_segment_register(struct vcpu *v, enum x86_segment seg,
                                      struct segment_register *reg)
 {
@@ -2231,6 +2236,7 @@ static struct hvm_function_table __initd
     .get_interrupt_shadow = svm_get_interrupt_shadow,
     .set_interrupt_shadow = svm_set_interrupt_shadow,
     .guest_x86_mode       = svm_guest_x86_mode,
+    .get_cpl              = svm_get_cpl,
     .get_segment_register = svm_get_segment_register,
     .set_segment_register = svm_set_segment_register,
     .get_shadow_gs_base   = svm_get_shadow_gs_base,
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -557,7 +557,7 @@ static void vmx_update_guest_vendor(stru
     vmx_vmcs_exit(v);
 }
 
-static int vmx_guest_x86_mode(struct vcpu *v)
+int vmx_guest_x86_mode(struct vcpu *v)
 {
     unsigned long cs_ar_bytes;
 
@@ -921,6 +921,26 @@ static void vmx_ctxt_switch_to(struct vc
 }
 
 
+unsigned int vmx_get_cpl(void)
+{
+    unsigned long attr;
+
+    __vmread(GUEST_SS_AR_BYTES, &attr);
+
+    return (attr >> 5) & 3;
+}
+
+static unsigned int _vmx_get_cpl(struct vcpu *v)
+{
+    unsigned int cpl;
+
+    vmx_vmcs_enter(v);
+    cpl = vmx_get_cpl();
+    vmx_vmcs_exit(v);
+
+    return cpl;
+}
+
 /* SDM volume 3b section 22.3.1.2: we can only enter virtual 8086 mode
  * if all of CS, SS, DS, ES, FS and GS are 16bit ring-3 data segments.
  * The guest thinks it's got ring-0 segments, so we need to fudge
@@ -2089,6 +2109,7 @@ static struct hvm_function_table __initd
     .get_interrupt_shadow = vmx_get_interrupt_shadow,
     .set_interrupt_shadow = vmx_set_interrupt_shadow,
     .guest_x86_mode       = vmx_guest_x86_mode,
+    .get_cpl              = _vmx_get_cpl,
     .get_segment_register = vmx_get_segment_register,
     .set_segment_register = vmx_set_segment_register,
     .get_shadow_gs_base   = vmx_get_shadow_gs_base,
@@ -3837,19 +3858,13 @@ void vmx_vmexit_handler(struct cpu_user_
     /* fall through */
     default:
     exit_and_crash:
-        {
-            struct segment_register ss;
-
-            gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n",
-                     exit_reason);
+        gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n", exit_reason);
 
-            hvm_get_segment_register(v, x86_seg_ss, &ss);
-            if ( ss.attr.fields.dpl )
-                hvm_inject_hw_exception(TRAP_invalid_op,
-                                        X86_EVENT_NO_EC);
-            else
-                domain_crash(v->domain);
-        }
+        if ( vmx_get_cpl() )
+            hvm_inject_hw_exception(TRAP_invalid_op,
+                                    X86_EVENT_NO_EC);
+        else
+            domain_crash(v->domain);
         break;
     }
 
@@ -3871,12 +3886,9 @@ out:
     if ( mode == 8 ? !is_canonical_address(regs->rip)
                    : regs->rip != regs->_eip )
     {
-        struct segment_register ss;
-
         gprintk(XENLOG_WARNING, "Bad rIP %lx for mode %u\n", regs->rip, mode);
 
-        hvm_get_segment_register(v, x86_seg_ss, &ss);
-        if ( ss.attr.fields.dpl )
+        if ( vmx_get_cpl() )
         {
             __vmread(VM_ENTRY_INTR_INFO, &intr_info);
             if ( !(intr_info & INTR_INFO_VALID_MASK) )
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -349,7 +349,6 @@ static inline u32 __n2_secondary_exec_co
 static int vmx_inst_check_privilege(struct cpu_user_regs *regs, int vmxop_check)
 {
     struct vcpu *v = current;
-    struct segment_register cs;
 
     if ( vmxop_check )
     {
@@ -360,15 +359,12 @@ static int vmx_inst_check_privilege(stru
     else if ( !vcpu_2_nvmx(v).vmxon_region_pa )
         goto invalid_op;
 
-    hvm_get_segment_register(v, x86_seg_cs, &cs);
-
-    if ( (regs->eflags & X86_EFLAGS_VM) ||
-         (hvm_long_mode_enabled(v) && cs.attr.fields.l == 0) )
+    if ( vmx_guest_x86_mode(v) < (hvm_long_mode_enabled(v) ? 8 : 2) )
         goto invalid_op;
     else if ( nestedhvm_vcpu_in_guestmode(v) )
         goto vmexit;
 
-    if ( (cs.sel & 3) > 0 )
+    if ( vmx_get_cpl() > 0 )
         goto gp_fault;
 
     return X86EMUL_OKAY;
@@ -413,16 +409,10 @@ static int decode_vmx_inst(struct cpu_us
     }
     else
     {
-        bool_t mode_64bit = 0;
+        bool mode_64bit = (vmx_guest_x86_mode(v) == 8);
 
         decode->type = VMX_INST_MEMREG_TYPE_MEMORY;
 
-        if ( hvm_long_mode_enabled(v) )
-        {
-            hvm_get_segment_register(v, x86_seg_cs, &seg);
-            mode_64bit = seg.attr.fields.l;
-        }
-
         if ( info.fields.segment > VMX_SREG_GS )
             goto gp_fault;
         hvm_get_segment_register(v, sreg_to_index[info.fields.segment], &seg);
--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -174,7 +174,6 @@ guest_walk_tables(struct vcpu *v, struct
 
     if ( is_hvm_domain(d) && !(pfec & PFEC_user_mode) )
     {
-        struct segment_register seg;
         const struct cpu_user_regs *regs = guest_cpu_user_regs();
 
         /* SMEP: kernel-mode instruction fetches from user-mode mappings
@@ -186,8 +185,6 @@ guest_walk_tables(struct vcpu *v, struct
         switch ( v->arch.smap_check_policy )
         {
         case SMAP_CHECK_HONOR_CPL_AC:
-            hvm_get_segment_register(v, x86_seg_ss, &seg);
-
             /*
              * SMAP: kernel-mode data accesses from user-mode mappings
              * should fault.
@@ -199,8 +196,7 @@ guest_walk_tables(struct vcpu *v, struct
              *   - Page fault in kernel mode
              */
             smap = hvm_smap_enabled(v) &&
-                   ((seg.attr.fields.dpl == 3) ||
-                    !(regs->eflags & X86_EFLAGS_AC));
+                   ((hvm_get_cpl(v) == 3) || !(regs->eflags & X86_EFLAGS_AC));
             break;
         case SMAP_CHECK_ENABLED:
             smap = hvm_smap_enabled(v);
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -1779,7 +1779,7 @@ void *sh_emulate_map_dest(struct vcpu *v
 #ifndef NDEBUG
     /* We don't emulate user-mode writes to page tables. */
     if ( has_hvm_container_domain(d)
-         ? hvm_get_seg_reg(x86_seg_ss, sh_ctxt)->attr.fields.dpl == 3
+         ? hvm_get_cpl(v) == 3
          : !guest_kernel_mode(v, guest_cpu_user_regs()) )
     {
         gdprintk(XENLOG_DEBUG, "User-mode write to pagetable reached "
--- a/xen/arch/x86/oprofile/xenoprof.c
+++ b/xen/arch/x86/oprofile/xenoprof.c
@@ -84,15 +84,12 @@ int xenoprofile_get_mode(struct vcpu *cu
 
     switch ( hvm_guest_x86_mode(curr) )
     {
-        struct segment_register ss;
-
     case 0: /* real mode */
         return 1;
     case 1: /* vm86 mode */
         return 0;
     default:
-        hvm_get_segment_register(curr, x86_seg_ss, &ss);
-        return (ss.sel & 3) != 3;
+        return hvm_get_cpl(curr) != 3;
     }
 }
 
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -117,6 +117,7 @@ struct hvm_function_table {
     unsigned int (*get_interrupt_shadow)(struct vcpu *v);
     void (*set_interrupt_shadow)(struct vcpu *v, unsigned int intr_shadow);
     int (*guest_x86_mode)(struct vcpu *v);
+    unsigned int (*get_cpl)(struct vcpu *v);
     void (*get_segment_register)(struct vcpu *v, enum x86_segment seg,
                                  struct segment_register *reg);
     void (*set_segment_register)(struct vcpu *v, enum x86_segment seg,
@@ -358,6 +359,12 @@ static inline void hvm_flush_guest_tlbs(
 void hvm_hypercall_page_initialise(struct domain *d,
                                    void *hypercall_page);
 
+static inline unsigned int
+hvm_get_cpl(struct vcpu *v)
+{
+    return hvm_funcs.get_cpl(v);
+}
+
 void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg,
                               struct segment_register *reg);
 void hvm_set_segment_register(struct vcpu *v, enum x86_segment seg,
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -550,6 +550,9 @@ static inline int __vmxon(u64 addr)
     return rc;
 }
 
+int vmx_guest_x86_mode(struct vcpu *v);
+unsigned int vmx_get_cpl(void);
+
 void vmx_inject_extint(int trap, uint8_t source);
 void vmx_inject_nmi(void);
 

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 1/3] x86/HVM: introduce hvm_get_cpl() and respective hook

[Razvan Cojocaru]

On 12/06/2016 01:43 PM, Jan Beulich wrote:
> ... instead of repeating the same code in various places (and getting
> it  wrong in some of them).
> 
> In vmx_inst_check_privilege() also stop open coding vmx_get_x86_mode().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

May I just say I find this very useful.


Thanks,
Razvan

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

Re: [PATCH 1/3] x86/HVM: introduce hvm_get_cpl() and respective hook

[Andrew Cooper]

On 06/12/16 11:43, Jan Beulich wrote:
> @@ -921,6 +921,26 @@ static void vmx_ctxt_switch_to(struct vc
>  }
>  
>  
> +unsigned int vmx_get_cpl(void)
> +{
> +    unsigned long attr;
> +
> +    __vmread(GUEST_SS_AR_BYTES, &attr);
> +
> +    return (attr >> 5) & 3;
> +}
> +
> +static unsigned int _vmx_get_cpl(struct vcpu *v)
> +{
> +    unsigned int cpl;
> +
> +    vmx_vmcs_enter(v);
> +    cpl = vmx_get_cpl();
> +    vmx_vmcs_exit(v);
> +
> +    return cpl;
> +}

Our ususal convention for functions with leading underscores would have
these the other way around.

As an alternative, how about vmx_hvmfunc_get_cpl() ? It is never called
directly.

Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 1/3] x86/HVM: introduce hvm_get_cpl() and respective hook

[Jan Beulich]

>>> On 06.12.16 at 14:49, <andrew.cooper3@citrix.com> wrote:
> On 06/12/16 11:43, Jan Beulich wrote:
>> @@ -921,6 +921,26 @@ static void vmx_ctxt_switch_to(struct vc
>>  }
>>  
>>  
>> +unsigned int vmx_get_cpl(void)
>> +{
>> +    unsigned long attr;
>> +
>> +    __vmread(GUEST_SS_AR_BYTES, &attr);
>> +
>> +    return (attr >> 5) & 3;
>> +}
>> +
>> +static unsigned int _vmx_get_cpl(struct vcpu *v)
>> +{
>> +    unsigned int cpl;
>> +
>> +    vmx_vmcs_enter(v);
>> +    cpl = vmx_get_cpl();
>> +    vmx_vmcs_exit(v);
>> +
>> +    return cpl;
>> +}
> 
> Our ususal convention for functions with leading underscores would have
> these the other way around.

Our usual convention doesn't fit here: The static function is the
only one allowed to have an underscore prefix.

> As an alternative, how about vmx_hvmfunc_get_cpl() ? It is never called
> directly.

I don't like this. If anything, vmx_do_get_cpl(), albeit it's only
slightly better imo.

> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Let me know if you can live with the existing naming.

Jan


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

Re: [PATCH 1/3] x86/HVM: introduce hvm_get_cpl() and respective hook

[Andrew Cooper]

On 06/12/16 14:07, Jan Beulich wrote:
>>>> On 06.12.16 at 14:49, <andrew.cooper3@citrix.com> wrote:
>> On 06/12/16 11:43, Jan Beulich wrote:
>>> @@ -921,6 +921,26 @@ static void vmx_ctxt_switch_to(struct vc
>>>  }
>>>  
>>>  
>>> +unsigned int vmx_get_cpl(void)
>>> +{
>>> +    unsigned long attr;
>>> +
>>> +    __vmread(GUEST_SS_AR_BYTES, &attr);
>>> +
>>> +    return (attr >> 5) & 3;
>>> +}
>>> +
>>> +static unsigned int _vmx_get_cpl(struct vcpu *v)
>>> +{
>>> +    unsigned int cpl;
>>> +
>>> +    vmx_vmcs_enter(v);
>>> +    cpl = vmx_get_cpl();
>>> +    vmx_vmcs_exit(v);
>>> +
>>> +    return cpl;
>>> +}
>> Our ususal convention for functions with leading underscores would have
>> these the other way around.
> Our usual convention doesn't fit here: The static function is the
> only one allowed to have an underscore prefix.
>
>> As an alternative, how about vmx_hvmfunc_get_cpl() ? It is never called
>> directly.
> I don't like this. If anything, vmx_do_get_cpl(), albeit it's only
> slightly better imo.

That would also be slightly better, if you don't mind switching to it.

>
>> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Let me know if you can live with the existing naming.

If not, then fine.

~Andrew

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

Re: [PATCH 1/3] x86/HVM: introduce hvm_get_cpl() and respective hook

[Boris Ostrovsky]

On 12/06/2016 06:43 AM, Jan Beulich wrote:
> ... instead of repeating the same code in various places (and getting
> it  wrong in some of them).
>
> In vmx_inst_check_privilege() also stop open coding vmx_get_x86_mode().

vmx_guest_x86_mode()

>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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


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

Re: [PATCH 1/3] x86/HVM: introduce hvm_get_cpl() and respective hook

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, December 06, 2016 7:43 PM
> 
> ... instead of repeating the same code in various places (and getting
> it  wrong in some of them).
> 
> In vmx_inst_check_privilege() also stop open coding vmx_get_x86_mode().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 

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

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

Ping: [PATCH 1/3] x86/HVM: introduce hvm_get_cpl() and respective hook

[Jan Beulich]

>>> On 06.12.16 at 12:43, <JBeulich@suse.com> wrote:
> --- a/xen/arch/x86/mm/guest_walk.c
> +++ b/xen/arch/x86/mm/guest_walk.c
> @@ -174,7 +174,6 @@ guest_walk_tables(struct vcpu *v, struct
>  
>      if ( is_hvm_domain(d) && !(pfec & PFEC_user_mode) )
>      {
> -        struct segment_register seg;
>          const struct cpu_user_regs *regs = guest_cpu_user_regs();
>  
>          /* SMEP: kernel-mode instruction fetches from user-mode mappings
> @@ -186,8 +185,6 @@ guest_walk_tables(struct vcpu *v, struct
>          switch ( v->arch.smap_check_policy )
>          {
>          case SMAP_CHECK_HONOR_CPL_AC:
> -            hvm_get_segment_register(v, x86_seg_ss, &seg);
> -
>              /*
>               * SMAP: kernel-mode data accesses from user-mode mappings
>               * should fault.
> @@ -199,8 +196,7 @@ guest_walk_tables(struct vcpu *v, struct
>               *   - Page fault in kernel mode
>               */
>              smap = hvm_smap_enabled(v) &&
> -                   ((seg.attr.fields.dpl == 3) ||
> -                    !(regs->eflags & X86_EFLAGS_AC));
> +                   ((hvm_get_cpl(v) == 3) || !(regs->eflags & X86_EFLAGS_AC));
>              break;
>          case SMAP_CHECK_ENABLED:
>              smap = hvm_smap_enabled(v);

George, any word on these?

> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -1779,7 +1779,7 @@ void *sh_emulate_map_dest(struct vcpu *v
>  #ifndef NDEBUG
>      /* We don't emulate user-mode writes to page tables. */
>      if ( has_hvm_container_domain(d)
> -         ? hvm_get_seg_reg(x86_seg_ss, sh_ctxt)->attr.fields.dpl == 3
> +         ? hvm_get_cpl(v) == 3
>           : !guest_kernel_mode(v, guest_cpu_user_regs()) )
>      {
>          gdprintk(XENLOG_DEBUG, "User-mode write to pagetable reached "

Tim, how about this one?

Thanks, Jan


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

Re: Ping: [PATCH 1/3] x86/HVM: introduce hvm_get_cpl() and respective hook

[Tim Deegan]

At 01:45 -0700 on 13 Dec (1481593536), Jan Beulich wrote:
> > --- a/xen/arch/x86/mm/shadow/common.c
> > +++ b/xen/arch/x86/mm/shadow/common.c
> > @@ -1779,7 +1779,7 @@ void *sh_emulate_map_dest(struct vcpu *v
> >  #ifndef NDEBUG
> >      /* We don't emulate user-mode writes to page tables. */
> >      if ( has_hvm_container_domain(d)
> > -         ? hvm_get_seg_reg(x86_seg_ss, sh_ctxt)->attr.fields.dpl == 3
> > +         ? hvm_get_cpl(v) == 3
> >           : !guest_kernel_mode(v, guest_cpu_user_regs()) )
> >      {
> >          gdprintk(XENLOG_DEBUG, "User-mode write to pagetable reached "
> 
> Tim, how about this one?

Sorry, didn't see that there.  Acked-by: Tim Deegan <tim@xen.org>

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

[PATCH 2/3] x86/HVM: support (emulate) UMIP

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 12921 bytes --]

There are three noteworthy drawbacks:
1) The intercepts we need to enable here are CPL-independent, i.e. we
   now have to emulate certain instructions for ring 0.
2) On VMX there's no intercept for SMSW, so the emulation isn't really
   complete there.
3) The CR4 write intercept on SVM is lower priority than all exception
   checks, so we need to intercept #GP.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The tool stack change could be left out - it updates a table which is
rather out of date anyway.
---
This once again points out that handle_mmio() is rather badly named, as
it's about more than just MMIO. Since we have hvm_emulate_one()
already, I am, however, lacking an idea for a good alternative name.

--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -158,6 +158,7 @@ int libxl_cpuid_parse_config(libxl_cpuid
         {"de",           0x00000001, NA, CPUID_REG_EDX,  2,  1},
         {"vme",          0x00000001, NA, CPUID_REG_EDX,  1,  1},
         {"fpu",          0x00000001, NA, CPUID_REG_EDX,  0,  1},
+        {"umip",         0x00000007,  0, CPUID_REG_ECX,  2,  1},
         {"topoext",      0x80000001, NA, CPUID_REG_ECX, 22,  1},
         {"tbm",          0x80000001, NA, CPUID_REG_ECX, 21,  1},
         {"nodeid",       0x80000001, NA, CPUID_REG_ECX, 19,  1},
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -154,6 +154,13 @@ static void __init calculate_hvm_feature
     __set_bit(X86_FEATURE_APIC, hvm_featureset);
 
     /*
+     * Xen can often provide UMIP emulation to HVM guests even if the host
+     * doesn't have such functionality.
+     */
+    if ( cpu_has_vmx_dt_exiting || cpu_has_svm )
+        __set_bit(X86_FEATURE_UMIP, hvm_featureset);
+
+    /*
      * On AMD, PV guests are entirely unable to use SYSENTER as Xen runs in
      * long mode (and init_amd() has cleared it out of host capabilities), but
      * HVM guests are able if running in protected mode.
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1008,6 +1008,8 @@ unsigned long hvm_cr4_guest_reserved_bit
               X86_CR4_OSFXSR : 0) |
              (leaf1_edx & cpufeat_mask(X86_FEATURE_SSE) ?
               X86_CR4_OSXMMEXCPT : 0) |
+             (leaf7_0_ecx & cpufeat_mask(X86_FEATURE_UMIP) ?
+              X86_CR4_UMIP : 0) |
              ((restore || nestedhvm_enabled(v->domain)) &&
               (leaf1_ecx & cpufeat_mask(X86_FEATURE_VMX)) ?
               X86_CR4_VMXE : 0) |
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -570,6 +570,29 @@ void svm_update_guest_cr(struct vcpu *v,
         if ( paging_mode_hap(v->domain) )
             value &= ~X86_CR4_PAE;
         value |= v->arch.hvm_vcpu.guest_cr[4];
+
+        if ( !cpu_has_umip )
+        {
+            u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
+
+            if ( v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_UMIP )
+            {
+                value &= ~X86_CR4_UMIP;
+                ASSERT(vmcb_get_cr_intercepts(vmcb) & CR_INTERCEPT_CR0_READ);
+                general1_intercepts |= GENERAL1_INTERCEPT_IDTR_READ |
+                                       GENERAL1_INTERCEPT_GDTR_READ |
+                                       GENERAL1_INTERCEPT_LDTR_READ |
+                                       GENERAL1_INTERCEPT_TR_READ;
+            }
+            else
+                general1_intercepts &= ~(GENERAL1_INTERCEPT_IDTR_READ |
+                                         GENERAL1_INTERCEPT_GDTR_READ |
+                                         GENERAL1_INTERCEPT_LDTR_READ |
+                                         GENERAL1_INTERCEPT_TR_READ);
+
+            vmcb_set_general1_intercepts(vmcb, general1_intercepts);
+        }
+
         vmcb_set_cr4(vmcb, value);
         break;
     default:
@@ -2444,6 +2467,13 @@ void svm_vmexit_handler(struct cpu_user_
         svm_fpu_dirty_intercept();
         break;  
 
+    case VMEXIT_EXCEPTION_GP:
+        HVMTRACE_1D(TRAP, TRAP_gp_fault);
+        /* We only care about ring 0 faults with error code zero. */
+        if ( vmcb->exitinfo1 || vmcb_get_cpl(vmcb) || !handle_mmio() )
+            hvm_inject_hw_exception(TRAP_gp_fault, vmcb->exitinfo1);
+        break;
+
     case VMEXIT_EXCEPTION_PF: {
         unsigned long va;
         va = vmcb->exitinfo2;
@@ -2551,7 +2581,25 @@ void svm_vmexit_handler(struct cpu_user_
             hvm_inject_hw_exception(TRAP_gp_fault, 0);
         break;
 
-    case VMEXIT_CR0_READ ... VMEXIT_CR15_READ:
+    case VMEXIT_IDTR_READ:
+    case VMEXIT_GDTR_READ:
+    case VMEXIT_LDTR_READ:
+    case VMEXIT_TR_READ:
+        ASSERT((v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_UMIP) && !cpu_has_umip);
+        if ( vmcb_get_cpl(vmcb) || !handle_mmio() )
+            hvm_inject_hw_exception(TRAP_gp_fault, 0);
+        break;
+
+    case VMEXIT_CR0_READ:
+        if ( (v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_UMIP) &&
+             vmcb_get_cpl(vmcb) )
+        {
+            ASSERT(!cpu_has_umip);
+            hvm_inject_hw_exception(TRAP_gp_fault, 0);
+            break;
+        }
+        /* fall through */
+    case VMEXIT_CR1_READ ... VMEXIT_CR15_READ:
     case VMEXIT_CR0_WRITE ... VMEXIT_CR15_WRITE:
         if ( cpu_has_svm_decode && (vmcb->exitinfo1 & (1ULL << 63)) )
             svm_vmexit_do_cr_access(vmcb, regs);
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -194,6 +194,10 @@ static int construct_vmcb(struct vcpu *v
         HVM_TRAP_MASK
         | (1U << TRAP_no_device);
 
+    /* For UMIP emulation intercept #GP to catch faulting CR4 writes. */
+    if ( !cpu_has_umip )
+        vmcb->_exception_intercepts |= 1U << TRAP_gp_fault;
+
     if ( paging_mode_hap(v->domain) )
     {
         vmcb->_np_enable = 1; /* enable nested paging */
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -227,6 +227,7 @@ static int vmx_init_vmcs_config(void)
         opt = (SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
                SECONDARY_EXEC_WBINVD_EXITING |
                SECONDARY_EXEC_ENABLE_EPT |
+               SECONDARY_EXEC_DESCRIPTOR_TABLE_EXITING |
                SECONDARY_EXEC_ENABLE_RDTSCP |
                SECONDARY_EXEC_PAUSE_LOOP_EXITING |
                SECONDARY_EXEC_ENABLE_INVPCID |
@@ -987,6 +988,10 @@ static int construct_vmcs(struct vcpu *v
 
     v->arch.hvm_vmx.secondary_exec_control = vmx_secondary_exec_control;
 
+    /* Disable descriptor table exiting: It's controlled by CR4.UMIP writes. */
+    v->arch.hvm_vmx.secondary_exec_control &=
+        ~SECONDARY_EXEC_DESCRIPTOR_TABLE_EXITING;
+
     /* Disable VPID for now: we decide when to enable it on VMENTER. */
     v->arch.hvm_vmx.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_VPID;
 
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1531,6 +1531,22 @@ static void vmx_update_guest_cr(struct v
             v->arch.hvm_vcpu.hw_cr[4] &=
                 ~(X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_PKE);
         }
+
+        if ( !cpu_has_umip )
+        {
+            if ( (v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_UMIP) )
+            {
+                ASSERT(cpu_has_vmx_dt_exiting);
+                v->arch.hvm_vcpu.hw_cr[4] &= ~X86_CR4_UMIP;
+                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_update_secondary_exec_control(v);
+        }
+
         __vmwrite(GUEST_CR4, v->arch.hvm_vcpu.hw_cr[4]);
         break;
     default:
@@ -3853,6 +3869,11 @@ void vmx_vmexit_handler(struct cpu_user_
 
     case EXIT_REASON_ACCESS_GDTR_OR_IDTR:
     case EXIT_REASON_ACCESS_LDTR_OR_TR:
+        ASSERT((v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_UMIP) && !cpu_has_umip);
+        if ( vmx_get_cpl() || !handle_mmio() )
+            hvm_inject_hw_exception(TRAP_gp_fault, 0);
+        break;
+
     case EXIT_REASON_VMX_PREEMPTION_TIMER_EXPIRED:
     case EXIT_REASON_INVPCID:
     /* fall through */
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1360,7 +1360,7 @@ protmode_load_seg(
     const struct x86_emulate_ops *ops)
 {
     enum x86_segment sel_seg = (sel & 4) ? x86_seg_ldtr : x86_seg_gdtr;
-    struct { uint32_t a, b; } desc;
+    struct { uint32_t a, b; } desc, desc_hi = {};
     uint8_t dpl, rpl;
     int cpl = get_cpl(ctxt, ops);
     uint32_t a_flag = 0x100;
@@ -1411,9 +1411,6 @@ protmode_load_seg(
         /* System segments must have S flag == 0. */
         if ( desc.b & (1u << 12) )
             goto raise_exn;
-        /* We do not support 64-bit descriptor types. */
-        if ( in_longmode(ctxt, ops) )
-            return X86EMUL_UNHANDLEABLE;
     }
     /* User segments must have S flag == 1. */
     else if ( !(desc.b & (1u << 12)) )
@@ -1487,6 +1484,33 @@ protmode_load_seg(
         goto raise_exn;
     }
 
+    if ( !is_x86_user_segment(seg) )
+    {
+        int lm = in_longmode(ctxt, ops);
+
+        if ( lm < 0 )
+            return X86EMUL_UNHANDLEABLE;
+        if ( lm )
+        {
+            switch ( rc = ops->read(sel_seg, (sel & 0xfff8) + 8,
+                                    &desc_hi, sizeof(desc_hi), ctxt) )
+            {
+            case X86EMUL_OKAY:
+                break;
+
+            case X86EMUL_EXCEPTION:
+                if ( !ctxt->event_pending )
+                    goto raise_exn;
+                /* fall through */
+            default:
+                return rc;
+            }
+            if ( (desc_hi.b & 0x00001f00) ||
+                 !is_canonical_address((uint64_t)desc_hi.a << 32) )
+                goto raise_exn;
+        }
+    }
+
     /* Ensure Accessed flag is set. */
     if ( a_flag && !(desc.b & a_flag) )
     {
@@ -1513,7 +1537,8 @@ protmode_load_seg(
         desc.b = new_desc_b;
     }
 
-    sreg->base = (((desc.b <<  0) & 0xff000000u) |
+    sreg->base = (((uint64_t)desc_hi.a << 32) |
+                  ((desc.b <<  0) & 0xff000000u) |
                   ((desc.b << 16) & 0x00ff0000u) |
                   ((desc.a >> 16) & 0x0000ffffu));
     sreg->attr.bytes = (((desc.b >>  8) & 0x00ffu) |
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -72,6 +72,7 @@ XEN_CPUFEATURE(XEN_SMAP,        (FSCAPIN
 #define cpu_has_smep            boot_cpu_has(X86_FEATURE_SMEP)
 #define cpu_has_smap            boot_cpu_has(X86_FEATURE_SMAP)
 #define cpu_has_fpu_sel         (!boot_cpu_has(X86_FEATURE_NO_FPU_SEL))
+#define cpu_has_umip            boot_cpu_has(X86_FEATURE_UMIP)
 #define cpu_has_ffxsr           ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) \
                                  && boot_cpu_has(X86_FEATURE_FFXSR))
 #define cpu_has_x2apic          boot_cpu_has(X86_FEATURE_X2APIC)
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -346,6 +346,8 @@ extern u64 vmx_ept_vpid_cap;
     (vmx_cpu_based_exec_control & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS)
 #define cpu_has_vmx_ept \
     (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT)
+#define cpu_has_vmx_dt_exiting \
+    (vmx_secondary_exec_control & SECONDARY_EXEC_DESCRIPTOR_TABLE_EXITING)
 #define cpu_has_vmx_vpid \
     (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_VPID)
 #define cpu_has_monitor_trap_flag \
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -82,6 +82,7 @@
 #define X86_CR4_PCE        0x00000100 /* enable performance counters at ipl 3 */
 #define X86_CR4_OSFXSR     0x00000200 /* enable fast FPU save and restore */
 #define X86_CR4_OSXMMEXCPT 0x00000400 /* enable unmasked SSE exceptions */
+#define X86_CR4_UMIP       0x00000800 /* disable user mode S[GIL]DT/STR/SMSW */
 #define X86_CR4_VMXE       0x00002000 /* enable VMX */
 #define X86_CR4_SMXE       0x00004000 /* enable SMX */
 #define X86_CR4_FSGSBASE   0x00010000 /* enable {rd,wr}{fs,gs}base */
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -224,6 +224,7 @@ XEN_CPUFEATURE(AVX512VL,      5*32+31) /
 /* Intel-defined CPU features, CPUID level 0x00000007:0.ecx, word 6 */
 XEN_CPUFEATURE(PREFETCHWT1,   6*32+ 0) /*A  PREFETCHWT1 instruction */
 XEN_CPUFEATURE(AVX512VBMI,    6*32+ 1) /*A  AVX-512 Vector Byte Manipulation Instrs */
+XEN_CPUFEATURE(UMIP,          6*32+ 2) /*S  User Mode Instruction Prevention */
 XEN_CPUFEATURE(PKU,           6*32+ 3) /*H  Protection Keys for Userspace */
 XEN_CPUFEATURE(OSPKE,         6*32+ 4) /*!  OS Protection Keys Enable */
 



[-- Attachment #2: x86-HVM-UMIP.patch --]
[-- Type: text/plain, Size: 12952 bytes --]

x86/HVM: support (emulate) UMIP

There are three noteworthy drawbacks:
1) The intercepts we need to enable here are CPL-independent, i.e. we
   now have to emulate certain instructions for ring 0.
2) On VMX there's no intercept for SMSW, so the emulation isn't really
   complete there.
3) The CR4 write intercept on SVM is lower priority than all exception
   checks, so we need to intercept #GP.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The tool stack change could be left out - it updates a table which is
rather out of date anyway.
---
This once again points out that handle_mmio() is rather badly named, as
it's about more than just MMIO. Since we have hvm_emulate_one()
already, I am, however, lacking an idea for a good alternative name.

--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -158,6 +158,7 @@ int libxl_cpuid_parse_config(libxl_cpuid
         {"de",           0x00000001, NA, CPUID_REG_EDX,  2,  1},
         {"vme",          0x00000001, NA, CPUID_REG_EDX,  1,  1},
         {"fpu",          0x00000001, NA, CPUID_REG_EDX,  0,  1},
+        {"umip",         0x00000007,  0, CPUID_REG_ECX,  2,  1},
         {"topoext",      0x80000001, NA, CPUID_REG_ECX, 22,  1},
         {"tbm",          0x80000001, NA, CPUID_REG_ECX, 21,  1},
         {"nodeid",       0x80000001, NA, CPUID_REG_ECX, 19,  1},
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -154,6 +154,13 @@ static void __init calculate_hvm_feature
     __set_bit(X86_FEATURE_APIC, hvm_featureset);
 
     /*
+     * Xen can often provide UMIP emulation to HVM guests even if the host
+     * doesn't have such functionality.
+     */
+    if ( cpu_has_vmx_dt_exiting || cpu_has_svm )
+        __set_bit(X86_FEATURE_UMIP, hvm_featureset);
+
+    /*
      * On AMD, PV guests are entirely unable to use SYSENTER as Xen runs in
      * long mode (and init_amd() has cleared it out of host capabilities), but
      * HVM guests are able if running in protected mode.
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1008,6 +1008,8 @@ unsigned long hvm_cr4_guest_reserved_bit
               X86_CR4_OSFXSR : 0) |
              (leaf1_edx & cpufeat_mask(X86_FEATURE_SSE) ?
               X86_CR4_OSXMMEXCPT : 0) |
+             (leaf7_0_ecx & cpufeat_mask(X86_FEATURE_UMIP) ?
+              X86_CR4_UMIP : 0) |
              ((restore || nestedhvm_enabled(v->domain)) &&
               (leaf1_ecx & cpufeat_mask(X86_FEATURE_VMX)) ?
               X86_CR4_VMXE : 0) |
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -570,6 +570,29 @@ void svm_update_guest_cr(struct vcpu *v,
         if ( paging_mode_hap(v->domain) )
             value &= ~X86_CR4_PAE;
         value |= v->arch.hvm_vcpu.guest_cr[4];
+
+        if ( !cpu_has_umip )
+        {
+            u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
+
+            if ( v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_UMIP )
+            {
+                value &= ~X86_CR4_UMIP;
+                ASSERT(vmcb_get_cr_intercepts(vmcb) & CR_INTERCEPT_CR0_READ);
+                general1_intercepts |= GENERAL1_INTERCEPT_IDTR_READ |
+                                       GENERAL1_INTERCEPT_GDTR_READ |
+                                       GENERAL1_INTERCEPT_LDTR_READ |
+                                       GENERAL1_INTERCEPT_TR_READ;
+            }
+            else
+                general1_intercepts &= ~(GENERAL1_INTERCEPT_IDTR_READ |
+                                         GENERAL1_INTERCEPT_GDTR_READ |
+                                         GENERAL1_INTERCEPT_LDTR_READ |
+                                         GENERAL1_INTERCEPT_TR_READ);
+
+            vmcb_set_general1_intercepts(vmcb, general1_intercepts);
+        }
+
         vmcb_set_cr4(vmcb, value);
         break;
     default:
@@ -2444,6 +2467,13 @@ void svm_vmexit_handler(struct cpu_user_
         svm_fpu_dirty_intercept();
         break;  
 
+    case VMEXIT_EXCEPTION_GP:
+        HVMTRACE_1D(TRAP, TRAP_gp_fault);
+        /* We only care about ring 0 faults with error code zero. */
+        if ( vmcb->exitinfo1 || vmcb_get_cpl(vmcb) || !handle_mmio() )
+            hvm_inject_hw_exception(TRAP_gp_fault, vmcb->exitinfo1);
+        break;
+
     case VMEXIT_EXCEPTION_PF: {
         unsigned long va;
         va = vmcb->exitinfo2;
@@ -2551,7 +2581,25 @@ void svm_vmexit_handler(struct cpu_user_
             hvm_inject_hw_exception(TRAP_gp_fault, 0);
         break;
 
-    case VMEXIT_CR0_READ ... VMEXIT_CR15_READ:
+    case VMEXIT_IDTR_READ:
+    case VMEXIT_GDTR_READ:
+    case VMEXIT_LDTR_READ:
+    case VMEXIT_TR_READ:
+        ASSERT((v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_UMIP) && !cpu_has_umip);
+        if ( vmcb_get_cpl(vmcb) || !handle_mmio() )
+            hvm_inject_hw_exception(TRAP_gp_fault, 0);
+        break;
+
+    case VMEXIT_CR0_READ:
+        if ( (v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_UMIP) &&
+             vmcb_get_cpl(vmcb) )
+        {
+            ASSERT(!cpu_has_umip);
+            hvm_inject_hw_exception(TRAP_gp_fault, 0);
+            break;
+        }
+        /* fall through */
+    case VMEXIT_CR1_READ ... VMEXIT_CR15_READ:
     case VMEXIT_CR0_WRITE ... VMEXIT_CR15_WRITE:
         if ( cpu_has_svm_decode && (vmcb->exitinfo1 & (1ULL << 63)) )
             svm_vmexit_do_cr_access(vmcb, regs);
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -194,6 +194,10 @@ static int construct_vmcb(struct vcpu *v
         HVM_TRAP_MASK
         | (1U << TRAP_no_device);
 
+    /* For UMIP emulation intercept #GP to catch faulting CR4 writes. */
+    if ( !cpu_has_umip )
+        vmcb->_exception_intercepts |= 1U << TRAP_gp_fault;
+
     if ( paging_mode_hap(v->domain) )
     {
         vmcb->_np_enable = 1; /* enable nested paging */
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -227,6 +227,7 @@ static int vmx_init_vmcs_config(void)
         opt = (SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
                SECONDARY_EXEC_WBINVD_EXITING |
                SECONDARY_EXEC_ENABLE_EPT |
+               SECONDARY_EXEC_DESCRIPTOR_TABLE_EXITING |
                SECONDARY_EXEC_ENABLE_RDTSCP |
                SECONDARY_EXEC_PAUSE_LOOP_EXITING |
                SECONDARY_EXEC_ENABLE_INVPCID |
@@ -987,6 +988,10 @@ static int construct_vmcs(struct vcpu *v
 
     v->arch.hvm_vmx.secondary_exec_control = vmx_secondary_exec_control;
 
+    /* Disable descriptor table exiting: It's controlled by CR4.UMIP writes. */
+    v->arch.hvm_vmx.secondary_exec_control &=
+        ~SECONDARY_EXEC_DESCRIPTOR_TABLE_EXITING;
+
     /* Disable VPID for now: we decide when to enable it on VMENTER. */
     v->arch.hvm_vmx.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_VPID;
 
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1531,6 +1531,22 @@ static void vmx_update_guest_cr(struct v
             v->arch.hvm_vcpu.hw_cr[4] &=
                 ~(X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_PKE);
         }
+
+        if ( !cpu_has_umip )
+        {
+            if ( (v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_UMIP) )
+            {
+                ASSERT(cpu_has_vmx_dt_exiting);
+                v->arch.hvm_vcpu.hw_cr[4] &= ~X86_CR4_UMIP;
+                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_update_secondary_exec_control(v);
+        }
+
         __vmwrite(GUEST_CR4, v->arch.hvm_vcpu.hw_cr[4]);
         break;
     default:
@@ -3853,6 +3869,11 @@ void vmx_vmexit_handler(struct cpu_user_
 
     case EXIT_REASON_ACCESS_GDTR_OR_IDTR:
     case EXIT_REASON_ACCESS_LDTR_OR_TR:
+        ASSERT((v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_UMIP) && !cpu_has_umip);
+        if ( vmx_get_cpl() || !handle_mmio() )
+            hvm_inject_hw_exception(TRAP_gp_fault, 0);
+        break;
+
     case EXIT_REASON_VMX_PREEMPTION_TIMER_EXPIRED:
     case EXIT_REASON_INVPCID:
     /* fall through */
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1360,7 +1360,7 @@ protmode_load_seg(
     const struct x86_emulate_ops *ops)
 {
     enum x86_segment sel_seg = (sel & 4) ? x86_seg_ldtr : x86_seg_gdtr;
-    struct { uint32_t a, b; } desc;
+    struct { uint32_t a, b; } desc, desc_hi = {};
     uint8_t dpl, rpl;
     int cpl = get_cpl(ctxt, ops);
     uint32_t a_flag = 0x100;
@@ -1411,9 +1411,6 @@ protmode_load_seg(
         /* System segments must have S flag == 0. */
         if ( desc.b & (1u << 12) )
             goto raise_exn;
-        /* We do not support 64-bit descriptor types. */
-        if ( in_longmode(ctxt, ops) )
-            return X86EMUL_UNHANDLEABLE;
     }
     /* User segments must have S flag == 1. */
     else if ( !(desc.b & (1u << 12)) )
@@ -1487,6 +1484,33 @@ protmode_load_seg(
         goto raise_exn;
     }
 
+    if ( !is_x86_user_segment(seg) )
+    {
+        int lm = in_longmode(ctxt, ops);
+
+        if ( lm < 0 )
+            return X86EMUL_UNHANDLEABLE;
+        if ( lm )
+        {
+            switch ( rc = ops->read(sel_seg, (sel & 0xfff8) + 8,
+                                    &desc_hi, sizeof(desc_hi), ctxt) )
+            {
+            case X86EMUL_OKAY:
+                break;
+
+            case X86EMUL_EXCEPTION:
+                if ( !ctxt->event_pending )
+                    goto raise_exn;
+                /* fall through */
+            default:
+                return rc;
+            }
+            if ( (desc_hi.b & 0x00001f00) ||
+                 !is_canonical_address((uint64_t)desc_hi.a << 32) )
+                goto raise_exn;
+        }
+    }
+
     /* Ensure Accessed flag is set. */
     if ( a_flag && !(desc.b & a_flag) )
     {
@@ -1513,7 +1537,8 @@ protmode_load_seg(
         desc.b = new_desc_b;
     }
 
-    sreg->base = (((desc.b <<  0) & 0xff000000u) |
+    sreg->base = (((uint64_t)desc_hi.a << 32) |
+                  ((desc.b <<  0) & 0xff000000u) |
                   ((desc.b << 16) & 0x00ff0000u) |
                   ((desc.a >> 16) & 0x0000ffffu));
     sreg->attr.bytes = (((desc.b >>  8) & 0x00ffu) |
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -72,6 +72,7 @@ XEN_CPUFEATURE(XEN_SMAP,        (FSCAPIN
 #define cpu_has_smep            boot_cpu_has(X86_FEATURE_SMEP)
 #define cpu_has_smap            boot_cpu_has(X86_FEATURE_SMAP)
 #define cpu_has_fpu_sel         (!boot_cpu_has(X86_FEATURE_NO_FPU_SEL))
+#define cpu_has_umip            boot_cpu_has(X86_FEATURE_UMIP)
 #define cpu_has_ffxsr           ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) \
                                  && boot_cpu_has(X86_FEATURE_FFXSR))
 #define cpu_has_x2apic          boot_cpu_has(X86_FEATURE_X2APIC)
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -346,6 +346,8 @@ extern u64 vmx_ept_vpid_cap;
     (vmx_cpu_based_exec_control & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS)
 #define cpu_has_vmx_ept \
     (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT)
+#define cpu_has_vmx_dt_exiting \
+    (vmx_secondary_exec_control & SECONDARY_EXEC_DESCRIPTOR_TABLE_EXITING)
 #define cpu_has_vmx_vpid \
     (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_VPID)
 #define cpu_has_monitor_trap_flag \
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -82,6 +82,7 @@
 #define X86_CR4_PCE        0x00000100 /* enable performance counters at ipl 3 */
 #define X86_CR4_OSFXSR     0x00000200 /* enable fast FPU save and restore */
 #define X86_CR4_OSXMMEXCPT 0x00000400 /* enable unmasked SSE exceptions */
+#define X86_CR4_UMIP       0x00000800 /* disable user mode S[GIL]DT/STR/SMSW */
 #define X86_CR4_VMXE       0x00002000 /* enable VMX */
 #define X86_CR4_SMXE       0x00004000 /* enable SMX */
 #define X86_CR4_FSGSBASE   0x00010000 /* enable {rd,wr}{fs,gs}base */
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -224,6 +224,7 @@ XEN_CPUFEATURE(AVX512VL,      5*32+31) /
 /* Intel-defined CPU features, CPUID level 0x00000007:0.ecx, word 6 */
 XEN_CPUFEATURE(PREFETCHWT1,   6*32+ 0) /*A  PREFETCHWT1 instruction */
 XEN_CPUFEATURE(AVX512VBMI,    6*32+ 1) /*A  AVX-512 Vector Byte Manipulation Instrs */
+XEN_CPUFEATURE(UMIP,          6*32+ 2) /*S  User Mode Instruction Prevention */
 XEN_CPUFEATURE(PKU,           6*32+ 3) /*H  Protection Keys for Userspace */
 XEN_CPUFEATURE(OSPKE,         6*32+ 4) /*!  OS Protection Keys Enable */
 

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 2/3] x86/HVM: support (emulate) UMIP

[Andrew Cooper]

On 06/12/16 11:44, Jan Beulich wrote:
> There are three noteworthy drawbacks:
> 1) The intercepts we need to enable here are CPL-independent, i.e. we
>    now have to emulate certain instructions for ring 0.
> 2) On VMX there's no intercept for SMSW, so the emulation isn't really
>    complete there.
> 3) The CR4 write intercept on SVM is lower priority than all exception
>    checks, so we need to intercept #GP.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I have had a brief look over the patch, but not reviewed it in detail yet.

First of all, would you mind pulling the 64bit segment implementation
into a separate patch?  They are somewhat orthogonal to UMIP, and the
code looks fine.  They will also let me finish my comprehensive user and
system segment emulation handling XTF tests which I developed during the
XSA-191 work.

As for UMIP itself, there are a number of issues which we should
consider here.

First, this adds quite a lot of emulation and extra handling in security
sensitive areas.  That isn't a problem per say, but given concerns with
emulation in general (and indeed the efforts to remove all emulation
from some usecases), making it unilaterally enabled is a problem.

As such, I think emulated-UMIP is an option which the user must
explicitly opt-in to.  The easiest option might be to defer adding
emulated-UMIP until I have split the default and max featureset options
in the CPUID policy ABI (which is the task I am currently working ok).

However, it would also require only enabling the SVM GP intercept in the
hvm_update_guest_vendor() path (which should be renamed to something
slightly more generic like hvm_cpuid_policy_updated()).


Given the complexity of the interactions, I think we should have a
dedicated XTF test for UMIP behaviour, similar to the CPUID faulting
one.  I can see about implementing that if you don't want to.

> ---
> The tool stack change could be left out - it updates a table which is
> rather out of date anyway.
> ---
> This once again points out that handle_mmio() is rather badly named, as
> it's about more than just MMIO. Since we have hvm_emulate_one()
> already, I am, however, lacking an idea for a good alternative name.

As am I.  A number of its current uses are not for MMIO at all, and
would ideally go with some further restriction of emulated instruction.

~Andrew

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

Re: [PATCH 2/3] x86/HVM: support (emulate) UMIP

[Jan Beulich]

>>> On 06.12.16 at 15:47, <andrew.cooper3@citrix.com> wrote:
> On 06/12/16 11:44, Jan Beulich wrote:
>> This once again points out that handle_mmio() is rather badly named, as
>> it's about more than just MMIO. Since we have hvm_emulate_one()
>> already, I am, however, lacking an idea for a good alternative name.
> 
> As am I.  A number of its current uses are not for MMIO at all, and
> would ideally go with some further restriction of emulated instruction.

Actually meanwhile I think I have a plan here, which could go
together with renaming the function: To efficiently make use of
the new .validate() hook the emulator obtains with the PV priv-op
patch, I'd like to have callers of handle_mmio() pass in their hook
function (and I think we should not make this optional).

Jan


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

Re: [PATCH 2/3] x86/HVM: support (emulate) UMIP

[Andrew Cooper]

On 06/12/16 14:55, Jan Beulich wrote:
>>>> On 06.12.16 at 15:47, <andrew.cooper3@citrix.com> wrote:
>> On 06/12/16 11:44, Jan Beulich wrote:
>>> This once again points out that handle_mmio() is rather badly named, as
>>> it's about more than just MMIO. Since we have hvm_emulate_one()
>>> already, I am, however, lacking an idea for a good alternative name.
>> As am I.  A number of its current uses are not for MMIO at all, and
>> would ideally go with some further restriction of emulated instruction.
> Actually meanwhile I think I have a plan here, which could go
> together with renaming the function: To efficiently make use of
> the new .validate() hook the emulator obtains with the PV priv-op
> patch, I'd like to have callers of handle_mmio() pass in their hook
> function (and I think we should not make this optional).

I haven't got that far in the PV series yet, but this plan sounds good
to me.

~Andrew

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

Re: [PATCH 2/3] x86/HVM: support (emulate) UMIP

[Jan Beulich]

>>> On 06.12.16 at 15:47, <andrew.cooper3@citrix.com> wrote:
> As for UMIP itself, there are a number of issues which we should
> consider here.
> 
> First, this adds quite a lot of emulation and extra handling in security
> sensitive areas.  That isn't a problem per say, but given concerns with
> emulation in general (and indeed the efforts to remove all emulation
> from some usecases), making it unilaterally enabled is a problem.

As mentioned in the commit description.

> As such, I think emulated-UMIP is an option which the user must
> explicitly opt-in to.  The easiest option might be to defer adding
> emulated-UMIP until I have split the default and max featureset options
> in the CPUID policy ABI (which is the task I am currently working ok).

Makes sense.

> However, it would also require only enabling the SVM GP intercept in the
> hvm_update_guest_vendor() path (which should be renamed to something
> slightly more generic like hvm_cpuid_policy_updated()).

Why that? We always need it intercepted as long as the guest
wants UMIP, but the hardware doesn't offer it. The feature isn't
tied to the vendor being Intel or some such.

Jan


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

Re: [PATCH 2/3] x86/HVM: support (emulate) UMIP

[Andrew Cooper]

On 08/12/16 12:20, Jan Beulich wrote:
>
>> However, it would also require only enabling the SVM GP intercept in the
>> hvm_update_guest_vendor() path (which should be renamed to something
>> slightly more generic like hvm_cpuid_policy_updated()).
> Why that? We always need it intercepted as long as the guest
> wants UMIP, but the hardware doesn't offer it. The feature isn't
> tied to the vendor being Intel or some such.

The hvm_update_guest_vendor() path is post-domain_create() way of
signalling "cpuid has changed - you might want to reconfigure intercepts".

It is currently used only to alter the #UD intercept based on the set
CPUID vendor (hence its name), but the name now looks rather short sighted.

With the proposal of having emulated-UMIP as explicitly opt-in, the
required intercepts shouldn't be enabled at domain_create() time, and
should be enabled later after the toolstack has set a policy.

~Andrew

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

Re: [PATCH 2/3] x86/HVM: support (emulate) UMIP

[Boris Ostrovsky]

On 12/06/2016 06:44 AM, Jan Beulich wrote:
> There are three noteworthy drawbacks:
> 1) The intercepts we need to enable here are CPL-independent, i.e. we
>    now have to emulate certain instructions for ring 0.
> 2) On VMX there's no intercept for SMSW, so the emulation isn't really
>    complete there.
> 3) The CR4 write intercept on SVM is lower priority than all exception
>    checks, so we need to intercept #GP.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> The tool stack change could be left out - it updates a table which is
> rather out of date anyway.
> ---
> This once again points out that handle_mmio() is rather badly named, as
> it's about more than just MMIO. Since we have hvm_emulate_one()
> already, I am, however, lacking an idea for a good alternative name.
>
> --- a/tools/libxl/libxl_cpuid.c
> +++ b/tools/libxl/libxl_cpuid.c
> @@ -158,6 +158,7 @@ int libxl_cpuid_parse_config(libxl_cpuid
>          {"de",           0x00000001, NA, CPUID_REG_EDX,  2,  1},
>          {"vme",          0x00000001, NA, CPUID_REG_EDX,  1,  1},
>          {"fpu",          0x00000001, NA, CPUID_REG_EDX,  0,  1},
> +        {"umip",         0x00000007,  0, CPUID_REG_ECX,  2,  1},
>          {"topoext",      0x80000001, NA, CPUID_REG_ECX, 22,  1},
>          {"tbm",          0x80000001, NA, CPUID_REG_ECX, 21,  1},
>          {"nodeid",       0x80000001, NA, CPUID_REG_ECX, 19,  1},
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -154,6 +154,13 @@ static void __init calculate_hvm_feature
>      __set_bit(X86_FEATURE_APIC, hvm_featureset);
>  
>      /*
> +     * Xen can often provide UMIP emulation to HVM guests even if the host
> +     * doesn't have such functionality.
> +     */
> +    if ( cpu_has_vmx_dt_exiting || cpu_has_svm )
> +        __set_bit(X86_FEATURE_UMIP, hvm_featureset);

I don't think I understand how this is going to work for processors that
don't support UMIP.

How, for example, can guest_cr[4] have X86_CR4_UMIP set on these
processors when CPUID will not show the feature being there?

-boris


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

Re: [PATCH 2/3] x86/HVM: support (emulate) UMIP

[Jan Beulich]

>>> On 06.12.16 at 17:23, <boris.ostrovsky@oracle.com> wrote:
> On 12/06/2016 06:44 AM, Jan Beulich wrote:
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -154,6 +154,13 @@ static void __init calculate_hvm_feature
>>      __set_bit(X86_FEATURE_APIC, hvm_featureset);
>>  
>>      /*
>> +     * Xen can often provide UMIP emulation to HVM guests even if the host
>> +     * doesn't have such functionality.
>> +     */
>> +    if ( cpu_has_vmx_dt_exiting || cpu_has_svm )
>> +        __set_bit(X86_FEATURE_UMIP, hvm_featureset);
> 
> I don't think I understand how this is going to work for processors that
> don't support UMIP.
> 
> How, for example, can guest_cr[4] have X86_CR4_UMIP set on these
> processors when CPUID will not show the feature being there?

What we allow the guest to see and what we store into hardware
registers are two different things: Note how svm_update_guest_cr()
masks off X86_CR4_UMIP from the vale to be put into the VMCB.

Jan


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

Re: [PATCH 2/3] x86/HVM: support (emulate) UMIP

[Boris Ostrovsky]

On 12/07/2016 06:29 AM, Jan Beulich wrote:
>>>> On 06.12.16 at 17:23, <boris.ostrovsky@oracle.com> wrote:
>> On 12/06/2016 06:44 AM, Jan Beulich wrote:
>>> --- a/xen/arch/x86/cpuid.c
>>> +++ b/xen/arch/x86/cpuid.c
>>> @@ -154,6 +154,13 @@ static void __init calculate_hvm_feature
>>>      __set_bit(X86_FEATURE_APIC, hvm_featureset);
>>>  
>>>      /*
>>> +     * Xen can often provide UMIP emulation to HVM guests even if the host
>>> +     * doesn't have such functionality.
>>> +     */
>>> +    if ( cpu_has_vmx_dt_exiting || cpu_has_svm )
>>> +        __set_bit(X86_FEATURE_UMIP, hvm_featureset);
>> I don't think I understand how this is going to work for processors that
>> don't support UMIP.
>>
>> How, for example, can guest_cr[4] have X86_CR4_UMIP set on these
>> processors when CPUID will not show the feature being there?
> What we allow the guest to see and what we store into hardware
> registers are two different things: Note how svm_update_guest_cr()
> masks off X86_CR4_UMIP from the vale to be put into the VMCB.

So that was kind of my question --- why would a guest ever try to set
this bit? As far as it is concerned, UMIP is not available and the guest
is then trying to set an unsupported bit in cr4. And that should result
in a #GP.

-boris

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

Re: [PATCH 2/3] x86/HVM: support (emulate) UMIP

[Jan Beulich]

>>> On 07.12.16 at 16:10, <boris.ostrovsky@oracle.com> wrote:
> On 12/07/2016 06:29 AM, Jan Beulich wrote:
>>>>> On 06.12.16 at 17:23, <boris.ostrovsky@oracle.com> wrote:
>>> On 12/06/2016 06:44 AM, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/cpuid.c
>>>> +++ b/xen/arch/x86/cpuid.c
>>>> @@ -154,6 +154,13 @@ static void __init calculate_hvm_feature
>>>>      __set_bit(X86_FEATURE_APIC, hvm_featureset);
>>>>  
>>>>      /*
>>>> +     * Xen can often provide UMIP emulation to HVM guests even if the host
>>>> +     * doesn't have such functionality.
>>>> +     */
>>>> +    if ( cpu_has_vmx_dt_exiting || cpu_has_svm )
>>>> +        __set_bit(X86_FEATURE_UMIP, hvm_featureset);
>>> I don't think I understand how this is going to work for processors that
>>> don't support UMIP.
>>>
>>> How, for example, can guest_cr[4] have X86_CR4_UMIP set on these
>>> processors when CPUID will not show the feature being there?
>> What we allow the guest to see and what we store into hardware
>> registers are two different things: Note how svm_update_guest_cr()
>> masks off X86_CR4_UMIP from the vale to be put into the VMCB.
> 
> So that was kind of my question --- why would a guest ever try to set
> this bit? As far as it is concerned, UMIP is not available and the guest
> is then trying to set an unsupported bit in cr4. And that should result
> in a #GP.

But the code fragment above adds the respective CPUID bit to the
permitted features. Believe me, I've tried this with a UMIP-enabled
Linux (including proper CPUID based detection).

Jan


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

Re: [PATCH 2/3] x86/HVM: support (emulate) UMIP

[Boris Ostrovsky]

On 12/07/2016 10:14 AM, Jan Beulich wrote:
>>>> On 07.12.16 at 16:10, <boris.ostrovsky@oracle.com> wrote:
>> On 12/07/2016 06:29 AM, Jan Beulich wrote:
>>>>>> On 06.12.16 at 17:23, <boris.ostrovsky@oracle.com> wrote:
>>>> On 12/06/2016 06:44 AM, Jan Beulich wrote:
>>>>> --- a/xen/arch/x86/cpuid.c
>>>>> +++ b/xen/arch/x86/cpuid.c
>>>>> @@ -154,6 +154,13 @@ static void __init calculate_hvm_feature
>>>>>      __set_bit(X86_FEATURE_APIC, hvm_featureset);
>>>>>  
>>>>>      /*
>>>>> +     * Xen can often provide UMIP emulation to HVM guests even if the host
>>>>> +     * doesn't have such functionality.
>>>>> +     */
>>>>> +    if ( cpu_has_vmx_dt_exiting || cpu_has_svm )
>>>>> +        __set_bit(X86_FEATURE_UMIP, hvm_featureset);
>>>> I don't think I understand how this is going to work for processors that
>>>> don't support UMIP.
>>>>
>>>> How, for example, can guest_cr[4] have X86_CR4_UMIP set on these
>>>> processors when CPUID will not show the feature being there?
>>> What we allow the guest to see and what we store into hardware
>>> registers are two different things: Note how svm_update_guest_cr()
>>> masks off X86_CR4_UMIP from the vale to be put into the VMCB.
>> So that was kind of my question --- why would a guest ever try to set
>> this bit? As far as it is concerned, UMIP is not available and the guest
>> is then trying to set an unsupported bit in cr4. And that should result
>> in a #GP.
> But the code fragment above adds the respective CPUID bit to the
> permitted features. Believe me, I've tried this with a UMIP-enabled
> Linux (including proper CPUID based detection).

Are you referring to these patches: https://lkml.org/lkml/2016/11/8/68 ?
If yes then they look to be Intel-specific.

If AMD decides to use CPUID0x7.ecx[2] for something else --- won't this
be a problem for this patch?

-boris



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

Re: [PATCH 2/3] x86/HVM: support (emulate) UMIP

[Jan Beulich]

>>> On 07.12.16 at 16:31, <boris.ostrovsky@oracle.com> wrote:
> On 12/07/2016 10:14 AM, Jan Beulich wrote:
>>>>> On 07.12.16 at 16:10, <boris.ostrovsky@oracle.com> wrote:
>>> On 12/07/2016 06:29 AM, Jan Beulich wrote:
>>>>>>> On 06.12.16 at 17:23, <boris.ostrovsky@oracle.com> wrote:
>>>>> On 12/06/2016 06:44 AM, Jan Beulich wrote:
>>>>>> --- a/xen/arch/x86/cpuid.c
>>>>>> +++ b/xen/arch/x86/cpuid.c
>>>>>> @@ -154,6 +154,13 @@ static void __init calculate_hvm_feature
>>>>>>      __set_bit(X86_FEATURE_APIC, hvm_featureset);
>>>>>>  
>>>>>>      /*
>>>>>> +     * Xen can often provide UMIP emulation to HVM guests even if the host
>>>>>> +     * doesn't have such functionality.
>>>>>> +     */
>>>>>> +    if ( cpu_has_vmx_dt_exiting || cpu_has_svm )
>>>>>> +        __set_bit(X86_FEATURE_UMIP, hvm_featureset);
>>>>> I don't think I understand how this is going to work for processors that
>>>>> don't support UMIP.
>>>>>
>>>>> How, for example, can guest_cr[4] have X86_CR4_UMIP set on these
>>>>> processors when CPUID will not show the feature being there?
>>>> What we allow the guest to see and what we store into hardware
>>>> registers are two different things: Note how svm_update_guest_cr()
>>>> masks off X86_CR4_UMIP from the vale to be put into the VMCB.
>>> So that was kind of my question --- why would a guest ever try to set
>>> this bit? As far as it is concerned, UMIP is not available and the guest
>>> is then trying to set an unsupported bit in cr4. And that should result
>>> in a #GP.
>> But the code fragment above adds the respective CPUID bit to the
>> permitted features. Believe me, I've tried this with a UMIP-enabled
>> Linux (including proper CPUID based detection).
> 
> Are you referring to these patches: https://lkml.org/lkml/2016/11/8/68 ?
> If yes then they look to be Intel-specific.

No, I had written my own before these had been posted. I did
actually post mine too, but that was a week or so after theirs,
and theirs appears to be more complete.

> If AMD decides to use CPUID0x7.ecx[2] for something else --- won't this
> be a problem for this patch?

I think the two vendors meanwhile do a good job not interfering with
one another's CPUID bits. We'd have ugly problems elsewhere if any
new dual purpose CPUID bit appeared.

Jan


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

Re: [PATCH 2/3] x86/HVM: support (emulate) UMIP

[Andrew Cooper]

On 07/12/16 15:39, Jan Beulich wrote:
>>>> On 07.12.16 at 16:31, <boris.ostrovsky@oracle.com> wrote:
>> On 12/07/2016 10:14 AM, Jan Beulich wrote:
>>>>>> On 07.12.16 at 16:10, <boris.ostrovsky@oracle.com> wrote:
>>>> On 12/07/2016 06:29 AM, Jan Beulich wrote:
>>>>>>>> On 06.12.16 at 17:23, <boris.ostrovsky@oracle.com> wrote:
>>>>>> On 12/06/2016 06:44 AM, Jan Beulich wrote:
>>>>>>> --- a/xen/arch/x86/cpuid.c
>>>>>>> +++ b/xen/arch/x86/cpuid.c
>>>>>>> @@ -154,6 +154,13 @@ static void __init calculate_hvm_feature
>>>>>>>      __set_bit(X86_FEATURE_APIC, hvm_featureset);
>>>>>>>  
>>>>>>>      /*
>>>>>>> +     * Xen can often provide UMIP emulation to HVM guests even if the host
>>>>>>> +     * doesn't have such functionality.
>>>>>>> +     */
>>>>>>> +    if ( cpu_has_vmx_dt_exiting || cpu_has_svm )
>>>>>>> +        __set_bit(X86_FEATURE_UMIP, hvm_featureset);
>>>>>> I don't think I understand how this is going to work for processors that
>>>>>> don't support UMIP.
>>>>>>
>>>>>> How, for example, can guest_cr[4] have X86_CR4_UMIP set on these
>>>>>> processors when CPUID will not show the feature being there?
>>>>> What we allow the guest to see and what we store into hardware
>>>>> registers are two different things: Note how svm_update_guest_cr()
>>>>> masks off X86_CR4_UMIP from the vale to be put into the VMCB.
>>>> So that was kind of my question --- why would a guest ever try to set
>>>> this bit? As far as it is concerned, UMIP is not available and the guest
>>>> is then trying to set an unsupported bit in cr4. And that should result
>>>> in a #GP.
>>> But the code fragment above adds the respective CPUID bit to the
>>> permitted features. Believe me, I've tried this with a UMIP-enabled
>>> Linux (including proper CPUID based detection).
>> Are you referring to these patches: https://lkml.org/lkml/2016/11/8/68 ?
>> If yes then they look to be Intel-specific.
> No, I had written my own before these had been posted. I did
> actually post mine too, but that was a week or so after theirs,
> and theirs appears to be more complete.
>
>> If AMD decides to use CPUID0x7.ecx[2] for something else --- won't this
>> be a problem for this patch?
> I think the two vendors meanwhile do a good job not interfering with
> one another's CPUID bits. We'd have ugly problems elsewhere if any
> new dual purpose CPUID bit appeared.

[root@minuet-1 ~]# head /proc/cpuinfo
processor    : 0
vendor_id    : AuthenticAMD
cpu family    : 21
model        : 96

[root@minuet-1 ~]# xen-cpuid
nr_features: 10
                      KEY 1d       1c       e1d      e1c      Da1     
7b0      7c0      e7d      e8b      7d0     

Static sets:
Known                    
b7ebfbff:fffef3ff:efd3fbff:2469bfff:0000000f:fdbfffff:0000001b:00000500:00000001:0000000c
Special                  
10000200:88200000:00000000:00000002:00000000:00002040:00000010:00000000:00000000:00000000
PV Mask                  
17c9cbf5:f6f83203:e2500800:042109e3:00000007:fdaf0b39:00000003:00000000:00000001:0000000c
HVM Shadow Mask          
17cbfbff:f7f83223:ea500800:04218df7:0000000f:fdbf4bbb:00000003:00000000:00000001:0000000c
HVM Hap Mask             
17cbfbff:f7fa3223:ee500800:04218df7:0000000f:fdbf4fbb:0000000b:00000000:00000001:0000000c

Dynamic sets:
Raw                      
178bfbff:fed8320b:2fd3fbff:2febbfff:00000001:000001a9:00000000:000037d9:00000000:00000000
Host                     
178bf3ff:f6d8320b:2fd3fbff:2469bfff:00000001:000001a9:00000000:00000500:00000000:00000000
PV                       
1789c3f5:f6f83203:23d1cbf5:042109e3:00000001:00000129:00000000:00000000:00000000:00000000
HVM                      
178bfbff:f6f83203:2fd3fbff:04218df7:00000001:000001a9:00000000:00000000:00000000:00000000


This processor already implements some of Intel's features from
0x7[0].ebx, including SMEP.  According to marketing, AMD Zen processors
will add the ADX, RDSEED, SHA, CLFLUSHOPT and SMAP features

I can't reasonably see them using a different feature word.

~Andrew

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

Re: [PATCH 2/3] x86/HVM: support (emulate) UMIP

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Thursday, December 08, 2016 12:07 AM
> 
> On 07/12/16 15:39, Jan Beulich wrote:
> >>>> On 07.12.16 at 16:31, <boris.ostrovsky@oracle.com> wrote:
> >> On 12/07/2016 10:14 AM, Jan Beulich wrote:
> >>>>>> On 07.12.16 at 16:10, <boris.ostrovsky@oracle.com> wrote:
> >>>> On 12/07/2016 06:29 AM, Jan Beulich wrote:
> >>>>>>>> On 06.12.16 at 17:23, <boris.ostrovsky@oracle.com> wrote:
> >>>>>> On 12/06/2016 06:44 AM, Jan Beulich wrote:
> >>>>>>> --- a/xen/arch/x86/cpuid.c
> >>>>>>> +++ b/xen/arch/x86/cpuid.c
> >>>>>>> @@ -154,6 +154,13 @@ static void __init calculate_hvm_feature
> >>>>>>>      __set_bit(X86_FEATURE_APIC, hvm_featureset);
> >>>>>>>
> >>>>>>>      /*
> >>>>>>> +     * Xen can often provide UMIP emulation to HVM guests even if the host
> >>>>>>> +     * doesn't have such functionality.
> >>>>>>> +     */
> >>>>>>> +    if ( cpu_has_vmx_dt_exiting || cpu_has_svm )
> >>>>>>> +        __set_bit(X86_FEATURE_UMIP, hvm_featureset);
> >>>>>> I don't think I understand how this is going to work for processors that
> >>>>>> don't support UMIP.
> >>>>>>
> >>>>>> How, for example, can guest_cr[4] have X86_CR4_UMIP set on these
> >>>>>> processors when CPUID will not show the feature being there?
> >>>>> What we allow the guest to see and what we store into hardware
> >>>>> registers are two different things: Note how svm_update_guest_cr()
> >>>>> masks off X86_CR4_UMIP from the vale to be put into the VMCB.
> >>>> So that was kind of my question --- why would a guest ever try to set
> >>>> this bit? As far as it is concerned, UMIP is not available and the guest
> >>>> is then trying to set an unsupported bit in cr4. And that should result
> >>>> in a #GP.
> >>> But the code fragment above adds the respective CPUID bit to the
> >>> permitted features. Believe me, I've tried this with a UMIP-enabled
> >>> Linux (including proper CPUID based detection).
> >> Are you referring to these patches: https://lkml.org/lkml/2016/11/8/68 ?
> >> If yes then they look to be Intel-specific.
> > No, I had written my own before these had been posted. I did
> > actually post mine too, but that was a week or so after theirs,
> > and theirs appears to be more complete.
> >
> >> If AMD decides to use CPUID0x7.ecx[2] for something else --- won't this
> >> be a problem for this patch?
> > I think the two vendors meanwhile do a good job not interfering with
> > one another's CPUID bits. We'd have ugly problems elsewhere if any
> > new dual purpose CPUID bit appeared.
> 
> [root@minuet-1 ~]# head /proc/cpuinfo
> processor    : 0
> vendor_id    : AuthenticAMD
> cpu family    : 21
> model        : 96
> 
> [root@minuet-1 ~]# xen-cpuid
> nr_features: 10
>                       KEY 1d       1c       e1d      e1c      Da1
> 7b0      7c0      e7d      e8b      7d0
> 
> Static sets:
> Known
> b7ebfbff:fffef3ff:efd3fbff:2469bfff:0000000f:fdbfffff:0000001b:00000500:00000001:
> 0000000c
> Special
> 10000200:88200000:00000000:00000002:00000000:00002040:00000010:00000000:000
> 00000:00000000
> PV Mask
> 17c9cbf5:f6f83203:e2500800:042109e3:00000007:fdaf0b39:00000003:00000000:0000
> 0001:0000000c
> HVM Shadow Mask
> 17cbfbff:f7f83223:ea500800:04218df7:0000000f:fdbf4bbb:00000003:00000000:00000
> 001:0000000c
> HVM Hap Mask
> 17cbfbff:f7fa3223:ee500800:04218df7:0000000f:fdbf4fbb:0000000b:00000000:00000
> 001:0000000c
> 
> Dynamic sets:
> Raw
> 178bfbff:fed8320b:2fd3fbff:2febbfff:00000001:000001a9:00000000:000037d9:000000
> 00:00000000
> Host
> 178bf3ff:f6d8320b:2fd3fbff:2469bfff:00000001:000001a9:00000000:00000500:00000
> 000:00000000
> PV
> 1789c3f5:f6f83203:23d1cbf5:042109e3:00000001:00000129:00000000:00000000:0000
> 0000:00000000
> HVM
> 178bfbff:f6f83203:2fd3fbff:04218df7:00000001:000001a9:00000000:00000000:00000
> 000:00000000
> 
> 
> This processor already implements some of Intel's features from
> 0x7[0].ebx, including SMEP.  According to marketing, AMD Zen processors
> will add the ADX, RDSEED, SHA, CLFLUSHOPT and SMAP features
> 
> I can't reasonably see them using a different feature word.
> 
> ~Andrew

There is agreement that common features will use the same fields.

Thanks
Kevin

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

Re: [PATCH 2/3] x86/HVM: support (emulate) UMIP

[Suravee Suthikulpanit]

On 12/7/16 23:06, Andrew Cooper wrote:
> On 07/12/16 15:39, Jan Beulich wrote:
>>>>> On 07.12.16 at 16:31, <boris.ostrovsky@oracle.com> wrote:
>>> On 12/07/2016 10:14 AM, Jan Beulich wrote:
>>>>>>> On 07.12.16 at 16:10, <boris.ostrovsky@oracle.com> wrote:
>>>>> On 12/07/2016 06:29 AM, Jan Beulich wrote:
>>>>>>>>> On 06.12.16 at 17:23, <boris.ostrovsky@oracle.com> wrote:
>>>>>>> On 12/06/2016 06:44 AM, Jan Beulich wrote:
>>>>>>>> --- a/xen/arch/x86/cpuid.c
>>>>>>>> +++ b/xen/arch/x86/cpuid.c
>>>>>>>> @@ -154,6 +154,13 @@ static void __init calculate_hvm_feature
>>>>>>>>      __set_bit(X86_FEATURE_APIC, hvm_featureset);
>>>>>>>>
>>>>>>>>      /*
>>>>>>>> +     * Xen can often provide UMIP emulation to HVM guests even if the host
>>>>>>>> +     * doesn't have such functionality.
>>>>>>>> +     */
>>>>>>>> +    if ( cpu_has_vmx_dt_exiting || cpu_has_svm )
>>>>>>>> +        __set_bit(X86_FEATURE_UMIP, hvm_featureset);
>>>>>>> I don't think I understand how this is going to work for processors that
>>>>>>> don't support UMIP.
>>>>>>>
>>>>>>> How, for example, can guest_cr[4] have X86_CR4_UMIP set on these
>>>>>>> processors when CPUID will not show the feature being there?
>>>>>> What we allow the guest to see and what we store into hardware
>>>>>> registers are two different things: Note how svm_update_guest_cr()
>>>>>> masks off X86_CR4_UMIP from the vale to be put into the VMCB.
>>>>> So that was kind of my question --- why would a guest ever try to set
>>>>> this bit? As far as it is concerned, UMIP is not available and the guest
>>>>> is then trying to set an unsupported bit in cr4. And that should result
>>>>> in a #GP.
>>>> But the code fragment above adds the respective CPUID bit to the
>>>> permitted features. Believe me, I've tried this with a UMIP-enabled
>>>> Linux (including proper CPUID based detection).
>>> Are you referring to these patches: https://lkml.org/lkml/2016/11/8/68 ?
>>> If yes then they look to be Intel-specific.
>> No, I had written my own before these had been posted. I did
>> actually post mine too, but that was a week or so after theirs,
>> and theirs appears to be more complete.
>>
>>> If AMD decides to use CPUID0x7.ecx[2] for something else --- won't this
>>> be a problem for this patch?
>> I think the two vendors meanwhile do a good job not interfering with
>> one another's CPUID bits. We'd have ugly problems elsewhere if any
>> new dual purpose CPUID bit appeared.
>
> [root@minuet-1 ~]# head /proc/cpuinfo
> processor    : 0
> vendor_id    : AuthenticAMD
> cpu family    : 21
> model        : 96
>
> [root@minuet-1 ~]# xen-cpuid
> nr_features: 10
>                       KEY 1d       1c       e1d      e1c      Da1
> 7b0      7c0      e7d      e8b      7d0
>
> Static sets:
> Known
> b7ebfbff:fffef3ff:efd3fbff:2469bfff:0000000f:fdbfffff:0000001b:00000500:00000001:0000000c
> Special
> 10000200:88200000:00000000:00000002:00000000:00002040:00000010:00000000:00000000:00000000
> PV Mask
> 17c9cbf5:f6f83203:e2500800:042109e3:00000007:fdaf0b39:00000003:00000000:00000001:0000000c
> HVM Shadow Mask
> 17cbfbff:f7f83223:ea500800:04218df7:0000000f:fdbf4bbb:00000003:00000000:00000001:0000000c
> HVM Hap Mask
> 17cbfbff:f7fa3223:ee500800:04218df7:0000000f:fdbf4fbb:0000000b:00000000:00000001:0000000c
>
> Dynamic sets:
> Raw
> 178bfbff:fed8320b:2fd3fbff:2febbfff:00000001:000001a9:00000000:000037d9:00000000:00000000
> Host
> 178bf3ff:f6d8320b:2fd3fbff:2469bfff:00000001:000001a9:00000000:00000500:00000000:00000000
> PV
> 1789c3f5:f6f83203:23d1cbf5:042109e3:00000001:00000129:00000000:00000000:00000000:00000000
> HVM
> 178bfbff:f6f83203:2fd3fbff:04218df7:00000001:000001a9:00000000:00000000:00000000:00000000
>
>
> This processor already implements some of Intel's features from
> 0x7[0].ebx, including SMEP.  According to marketing, AMD Zen processors
> will add the ADX, RDSEED, SHA, CLFLUSHOPT and SMAP features
>
> I can't reasonably see them using a different feature word.
>
> ~Andrew
>

I have checked on the family17h, the CPUID Fn0000_0007_ECX is reserved 
same as other older models.

Suravee


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

[PATCH 3/3] x86/HVM: prefer structure assignment for seg reg copying

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3192 bytes --]

This makes things type safe.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1434,7 +1434,8 @@ static int hvmemul_read_segment(
     if ( IS_ERR(sreg) )
          return -PTR_ERR(sreg);
 
-    memcpy(reg, sreg, sizeof(struct segment_register));
+    *reg = *sreg;
+
     return X86EMUL_OKAY;
 }
 
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -654,39 +654,39 @@ static void svm_get_segment_register(str
     switch ( seg )
     {
     case x86_seg_cs:
-        memcpy(reg, &vmcb->cs, sizeof(*reg));
+        *reg = vmcb->cs;
         break;
     case x86_seg_ds:
-        memcpy(reg, &vmcb->ds, sizeof(*reg));
+        *reg = vmcb->ds;
         break;
     case x86_seg_es:
-        memcpy(reg, &vmcb->es, sizeof(*reg));
+        *reg = vmcb->es;
         break;
     case x86_seg_fs:
         svm_sync_vmcb(v);
-        memcpy(reg, &vmcb->fs, sizeof(*reg));
+        *reg = vmcb->fs;
         break;
     case x86_seg_gs:
         svm_sync_vmcb(v);
-        memcpy(reg, &vmcb->gs, sizeof(*reg));
+        *reg = vmcb->gs;
         break;
     case x86_seg_ss:
-        memcpy(reg, &vmcb->ss, sizeof(*reg));
+        *reg = vmcb->ss;
         reg->attr.fields.dpl = vmcb->_cpl;
         break;
     case x86_seg_tr:
         svm_sync_vmcb(v);
-        memcpy(reg, &vmcb->tr, sizeof(*reg));
+        *reg = vmcb->tr;
         break;
     case x86_seg_gdtr:
-        memcpy(reg, &vmcb->gdtr, sizeof(*reg));
+        *reg = vmcb->gdtr;
         break;
     case x86_seg_idtr:
-        memcpy(reg, &vmcb->idtr, sizeof(*reg));
+        *reg = vmcb->idtr;
         break;
     case x86_seg_ldtr:
         svm_sync_vmcb(v);
-        memcpy(reg, &vmcb->ldtr, sizeof(*reg));
+        *reg = vmcb->ldtr;
         break;
     default:
         BUG();
@@ -729,26 +729,26 @@ static void svm_set_segment_register(str
     switch ( seg )
     {
     case x86_seg_cs:
-        memcpy(&vmcb->cs, reg, sizeof(*reg));
+        vmcb->cs = *reg;
         break;
     case x86_seg_ds:
-        memcpy(&vmcb->ds, reg, sizeof(*reg));
+        vmcb->ds = *reg;
         break;
     case x86_seg_es:
-        memcpy(&vmcb->es, reg, sizeof(*reg));
+        vmcb->es = *reg;
         break;
     case x86_seg_fs:
-        memcpy(&vmcb->fs, reg, sizeof(*reg));
+        vmcb->fs = *reg;
         break;
     case x86_seg_gs:
-        memcpy(&vmcb->gs, reg, sizeof(*reg));
+        vmcb->gs = *reg;
         break;
     case x86_seg_ss:
-        memcpy(&vmcb->ss, reg, sizeof(*reg));
+        vmcb->ss = *reg;
         vmcb->_cpl = vmcb->ss.attr.fields.dpl;
         break;
     case x86_seg_tr:
-        memcpy(&vmcb->tr, reg, sizeof(*reg));
+        vmcb->tr = *reg;
         break;
     case x86_seg_gdtr:
         vmcb->gdtr.base = reg->base;
@@ -759,7 +759,7 @@ static void svm_set_segment_register(str
         vmcb->idtr.limit = reg->limit;
         break;
     case x86_seg_ldtr:
-        memcpy(&vmcb->ldtr, reg, sizeof(*reg));
+        vmcb->ldtr = *reg;
         break;
     default:
         BUG();




[-- Attachment #2: x86-HVM-sreg-copying.patch --]
[-- Type: text/plain, Size: 3246 bytes --]

x86/HVM: prefer structure assignment for seg reg copying

This makes things type safe.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1434,7 +1434,8 @@ static int hvmemul_read_segment(
     if ( IS_ERR(sreg) )
          return -PTR_ERR(sreg);
 
-    memcpy(reg, sreg, sizeof(struct segment_register));
+    *reg = *sreg;
+
     return X86EMUL_OKAY;
 }
 
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -654,39 +654,39 @@ static void svm_get_segment_register(str
     switch ( seg )
     {
     case x86_seg_cs:
-        memcpy(reg, &vmcb->cs, sizeof(*reg));
+        *reg = vmcb->cs;
         break;
     case x86_seg_ds:
-        memcpy(reg, &vmcb->ds, sizeof(*reg));
+        *reg = vmcb->ds;
         break;
     case x86_seg_es:
-        memcpy(reg, &vmcb->es, sizeof(*reg));
+        *reg = vmcb->es;
         break;
     case x86_seg_fs:
         svm_sync_vmcb(v);
-        memcpy(reg, &vmcb->fs, sizeof(*reg));
+        *reg = vmcb->fs;
         break;
     case x86_seg_gs:
         svm_sync_vmcb(v);
-        memcpy(reg, &vmcb->gs, sizeof(*reg));
+        *reg = vmcb->gs;
         break;
     case x86_seg_ss:
-        memcpy(reg, &vmcb->ss, sizeof(*reg));
+        *reg = vmcb->ss;
         reg->attr.fields.dpl = vmcb->_cpl;
         break;
     case x86_seg_tr:
         svm_sync_vmcb(v);
-        memcpy(reg, &vmcb->tr, sizeof(*reg));
+        *reg = vmcb->tr;
         break;
     case x86_seg_gdtr:
-        memcpy(reg, &vmcb->gdtr, sizeof(*reg));
+        *reg = vmcb->gdtr;
         break;
     case x86_seg_idtr:
-        memcpy(reg, &vmcb->idtr, sizeof(*reg));
+        *reg = vmcb->idtr;
         break;
     case x86_seg_ldtr:
         svm_sync_vmcb(v);
-        memcpy(reg, &vmcb->ldtr, sizeof(*reg));
+        *reg = vmcb->ldtr;
         break;
     default:
         BUG();
@@ -729,26 +729,26 @@ static void svm_set_segment_register(str
     switch ( seg )
     {
     case x86_seg_cs:
-        memcpy(&vmcb->cs, reg, sizeof(*reg));
+        vmcb->cs = *reg;
         break;
     case x86_seg_ds:
-        memcpy(&vmcb->ds, reg, sizeof(*reg));
+        vmcb->ds = *reg;
         break;
     case x86_seg_es:
-        memcpy(&vmcb->es, reg, sizeof(*reg));
+        vmcb->es = *reg;
         break;
     case x86_seg_fs:
-        memcpy(&vmcb->fs, reg, sizeof(*reg));
+        vmcb->fs = *reg;
         break;
     case x86_seg_gs:
-        memcpy(&vmcb->gs, reg, sizeof(*reg));
+        vmcb->gs = *reg;
         break;
     case x86_seg_ss:
-        memcpy(&vmcb->ss, reg, sizeof(*reg));
+        vmcb->ss = *reg;
         vmcb->_cpl = vmcb->ss.attr.fields.dpl;
         break;
     case x86_seg_tr:
-        memcpy(&vmcb->tr, reg, sizeof(*reg));
+        vmcb->tr = *reg;
         break;
     case x86_seg_gdtr:
         vmcb->gdtr.base = reg->base;
@@ -759,7 +759,7 @@ static void svm_set_segment_register(str
         vmcb->idtr.limit = reg->limit;
         break;
     case x86_seg_ldtr:
-        memcpy(&vmcb->ldtr, reg, sizeof(*reg));
+        vmcb->ldtr = *reg;
         break;
     default:
         BUG();

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 3/3] x86/HVM: prefer structure assignment for seg reg copying

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 06 December 2016 11:45
> To: xen-devel <xen-devel@lists.xenproject.org>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Andrew
> Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>; George Dunlap <George.Dunlap@citrix.com>;
> Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Subject: [PATCH 3/3] x86/HVM: prefer structure assignment for seg reg
> copying
> 
> This makes things type safe.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> 
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1434,7 +1434,8 @@ static int hvmemul_read_segment(
>      if ( IS_ERR(sreg) )
>           return -PTR_ERR(sreg);
> 
> -    memcpy(reg, sreg, sizeof(struct segment_register));
> +    *reg = *sreg;
> +
>      return X86EMUL_OKAY;
>  }
> 
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -654,39 +654,39 @@ static void svm_get_segment_register(str
>      switch ( seg )
>      {
>      case x86_seg_cs:
> -        memcpy(reg, &vmcb->cs, sizeof(*reg));
> +        *reg = vmcb->cs;
>          break;
>      case x86_seg_ds:
> -        memcpy(reg, &vmcb->ds, sizeof(*reg));
> +        *reg = vmcb->ds;
>          break;
>      case x86_seg_es:
> -        memcpy(reg, &vmcb->es, sizeof(*reg));
> +        *reg = vmcb->es;
>          break;
>      case x86_seg_fs:
>          svm_sync_vmcb(v);
> -        memcpy(reg, &vmcb->fs, sizeof(*reg));
> +        *reg = vmcb->fs;
>          break;
>      case x86_seg_gs:
>          svm_sync_vmcb(v);
> -        memcpy(reg, &vmcb->gs, sizeof(*reg));
> +        *reg = vmcb->gs;
>          break;
>      case x86_seg_ss:
> -        memcpy(reg, &vmcb->ss, sizeof(*reg));
> +        *reg = vmcb->ss;
>          reg->attr.fields.dpl = vmcb->_cpl;
>          break;
>      case x86_seg_tr:
>          svm_sync_vmcb(v);
> -        memcpy(reg, &vmcb->tr, sizeof(*reg));
> +        *reg = vmcb->tr;
>          break;
>      case x86_seg_gdtr:
> -        memcpy(reg, &vmcb->gdtr, sizeof(*reg));
> +        *reg = vmcb->gdtr;
>          break;
>      case x86_seg_idtr:
> -        memcpy(reg, &vmcb->idtr, sizeof(*reg));
> +        *reg = vmcb->idtr;
>          break;
>      case x86_seg_ldtr:
>          svm_sync_vmcb(v);
> -        memcpy(reg, &vmcb->ldtr, sizeof(*reg));
> +        *reg = vmcb->ldtr;
>          break;
>      default:
>          BUG();
> @@ -729,26 +729,26 @@ static void svm_set_segment_register(str
>      switch ( seg )
>      {
>      case x86_seg_cs:
> -        memcpy(&vmcb->cs, reg, sizeof(*reg));
> +        vmcb->cs = *reg;
>          break;
>      case x86_seg_ds:
> -        memcpy(&vmcb->ds, reg, sizeof(*reg));
> +        vmcb->ds = *reg;
>          break;
>      case x86_seg_es:
> -        memcpy(&vmcb->es, reg, sizeof(*reg));
> +        vmcb->es = *reg;
>          break;
>      case x86_seg_fs:
> -        memcpy(&vmcb->fs, reg, sizeof(*reg));
> +        vmcb->fs = *reg;
>          break;
>      case x86_seg_gs:
> -        memcpy(&vmcb->gs, reg, sizeof(*reg));
> +        vmcb->gs = *reg;
>          break;
>      case x86_seg_ss:
> -        memcpy(&vmcb->ss, reg, sizeof(*reg));
> +        vmcb->ss = *reg;
>          vmcb->_cpl = vmcb->ss.attr.fields.dpl;
>          break;
>      case x86_seg_tr:
> -        memcpy(&vmcb->tr, reg, sizeof(*reg));
> +        vmcb->tr = *reg;
>          break;
>      case x86_seg_gdtr:
>          vmcb->gdtr.base = reg->base;
> @@ -759,7 +759,7 @@ static void svm_set_segment_register(str
>          vmcb->idtr.limit = reg->limit;
>          break;
>      case x86_seg_ldtr:
> -        memcpy(&vmcb->ldtr, reg, sizeof(*reg));
> +        vmcb->ldtr = *reg;
>          break;
>      default:
>          BUG();
> 
> 


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

Re: [PATCH 3/3] x86/HVM: prefer structure assignment for seg reg copying

[Andrew Cooper]

On 06/12/16 11:45, Jan Beulich wrote:
> This makes things type safe.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 3/3] x86/HVM: prefer structure assignment for seg reg copying

[Boris Ostrovsky]

On 12/06/2016 06:45 AM, Jan Beulich wrote:
> This makes things type safe.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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


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