vmm: Remove nasty rdmsr() macro (XCC)
authorBarret Rhoden <brho@cs.berkeley.edu>
Thu, 17 Aug 2017 16:25:38 +0000 (12:25 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Fri, 25 Aug 2017 18:41:49 +0000 (14:41 -0400)
This cleaned up the kernel's MSR code a bit.  The rdmsr() macro actually
cluttered up some of the code, especially in the "just read the host MSRs
and copy to the VMCS" section.

Also, the order of the parameters to rdmsr() was unclear.  In fact, we had
a bug in the user version of the code for emsr_ok (rdx was first, not
second).  Whoops.

We had another user bug in miscenable, where we didn't track PEBS properly.
I just removed that function completely, since it is never called.  Similar
with emsr_mustmatch, both for the kernel and user.

Also note that the user MSR code will panic if you use it.  Previously,
you'd get a GPF if you tried an MSR instruction from userspace.  Instead of
just deleting it all, I left it there with a panic().  Someone else can use
the #arch device to access the MSRs if we feel it is useful.

Reinstall your kernel headers.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/arch/x86/ros/vmx.h
kern/arch/x86/vmm/intel/vmx.c
user/vmm/vmxmsr.c

index e0a008e..4dfe24e 100644 (file)
@@ -646,15 +646,6 @@ struct vmcs {
 
 typedef uint64_t gpa_t;
 typedef uint64_t gva_t;
-/* TODO: remove these nasty macros */
-#define rdmsrl(msr, val) (val) = read_msr((msr))
-#define rdmsr(msr, low, high)                                                  \
-do {                                                                           \
-       uint64_t m = read_msr(msr);                                                \
-                                                                                  \
-       low = m & 0xffffffff;                                                      \
-       high = m >> 32;                                                            \
-} while (0)
 
 struct vmx_capability {
        uint32_t ept;
index f656aeb..f21dadb 100644 (file)
@@ -404,12 +404,15 @@ check_vmxec_controls(struct vmxec const *v, bool have_true_msr,
 {
        bool err = false;
        uint32_t vmx_msr_low, vmx_msr_high;
+       uint64_t msr_val;
        uint32_t reserved_0, reserved_1, changeable_bits, try0, try1;
 
        if (have_true_msr)
-               rdmsr(v->truemsr, vmx_msr_low, vmx_msr_high);
+               msr_val = read_msr(v->truemsr);
        else
-               rdmsr(v->msr, vmx_msr_low, vmx_msr_high);
+               msr_val = read_msr(v->msr);
+       vmx_msr_high = high32(msr_val);
+       vmx_msr_low = low32(msr_val);
 
        if (vmx_msr_low & ~vmx_msr_high)
                warn("JACKPOT: Conflicting VMX ec ctls for %s, high 0x%08x low 0x%08x",
@@ -673,7 +676,9 @@ setup_vmcs_config(void *p)
 
        /* Read in the caps for runtime checks.  This MSR is only available if
         * secondary controls and ept or vpid is on, which we check earlier */
-       rdmsr(MSR_IA32_VMX_EPT_VPID_CAP, vmx_capability.ept, vmx_capability.vpid);
+       vmx_msr = read_msr(MSR_IA32_VMX_EPT_VPID_CAP);
+       vmx_capability.vpid = high32(vmx_msr);
+       vmx_capability.ept = low32(vmx_msr);
 
        *ret = 0;
 }
@@ -722,8 +727,6 @@ vmx_free_vmcs(struct vmcs *vmcs)
  */
 static void vmx_setup_constant_host_state(void)
 {
-       uint32_t low32, high32;
-       unsigned long tmpl;
        pseudodesc_t dt;
 
        vmcs_writel(HOST_CR0, rcr0() & ~X86_CR0_TS);    /* 22.2.3 */
@@ -742,18 +745,13 @@ static void vmx_setup_constant_host_state(void)
        extern void vmexit_handler(void);
        vmcs_writel(HOST_RIP, (unsigned long)vmexit_handler);
 
-       rdmsr(MSR_IA32_SYSENTER_CS, low32, high32);
-       vmcs_write32(HOST_IA32_SYSENTER_CS, low32);
-       rdmsrl(MSR_IA32_SYSENTER_EIP, tmpl);
-       vmcs_writel(HOST_IA32_SYSENTER_EIP, tmpl);      /* 22.2.3 */
+       vmcs_write32(HOST_IA32_SYSENTER_CS, read_msr(MSR_IA32_SYSENTER_CS));
+       vmcs_writel(HOST_IA32_SYSENTER_EIP, read_msr(MSR_IA32_SYSENTER_EIP));
 
-       rdmsr(MSR_EFER, low32, high32);
-       vmcs_write32(HOST_IA32_EFER, low32);
+       vmcs_write32(HOST_IA32_EFER, read_msr(MSR_EFER));
 
-       if (vmcs_config.vmexit_ctrl & VM_EXIT_LOAD_IA32_PAT) {
-               rdmsr(MSR_IA32_CR_PAT, low32, high32);
-               vmcs_write64(HOST_IA32_PAT, low32 | ((uint64_t) high32 << 32));
-       }
+       if (vmcs_config.vmexit_ctrl & VM_EXIT_LOAD_IA32_PAT)
+               vmcs_write64(HOST_IA32_PAT, read_msr(MSR_IA32_CR_PAT));
 
        vmcs_write16(HOST_FS_SELECTOR, 0);      /* 22.2.4 */
        vmcs_write16(HOST_GS_SELECTOR, 0);      /* 22.2.4 */
@@ -1131,7 +1129,7 @@ static int __vmx_enable(struct vmcs *vmxon_buf) {
                return -EBUSY;
        }
 
-       rdmsrl(MSR_IA32_FEATURE_CONTROL, old);
+       old = read_msr(MSR_IA32_FEATURE_CONTROL);
 
        test_bits = FEATURE_CONTROL_LOCKED;
        test_bits |= FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
index 48aa1fb..39ee1ce 100644 (file)
@@ -38,25 +38,17 @@ struct emmsr {
        bool written;
        uint32_t edx, eax;
 };
-// Might need to mfence rdmsr.  supposedly wrmsr serializes, but not for x2APIC
+
 static inline uint64_t read_msr(uint32_t reg)
 {
-       uint32_t edx, eax;
-       asm volatile("rdmsr; mfence" : "=d"(edx), "=a"(eax) : "c"(reg));
-       return (uint64_t)edx << 32 | eax;
+       panic("Not implemented for userspace");
 }
 
 static inline void write_msr(uint32_t reg, uint64_t val)
 {
-       asm volatile("wrmsr" : : "d"((uint32_t)(val >> 32)),
-                                "a"((uint32_t)(val & 0xFFFFFFFF)),
-                                "c"(reg));
+       panic("Not implemented for userspace");
 }
 
-static int emsr_miscenable(struct guest_thread *vm_thread, struct emmsr *,
-                           uint32_t);
-static int emsr_mustmatch(struct guest_thread *vm_thread, struct emmsr *,
-                          uint32_t);
 static int emsr_readonly(struct guest_thread *vm_thread, struct emmsr *,
                          uint32_t);
 static int emsr_readzero(struct guest_thread *vm_thread, struct emmsr *,
@@ -69,84 +61,29 @@ struct emmsr emmsrs[] = {
        {MSR_RAPL_POWER_UNIT, "MSR_RAPL_POWER_UNIT", emsr_readzero},
 };
 
-static uint64_t set_low32(uint64_t hi, uint32_t lo)
-{
-       return (hi & 0xffffffff00000000ULL) | lo;
-}
-
-static uint64_t set_low16(uint64_t hi, uint16_t lo)
+static inline uint32_t low32(uint64_t val)
 {
-       return (hi & 0xffffffffffff0000ULL) | lo;
+       return val & 0xffffffff;
 }
 
-static uint64_t set_low8(uint64_t hi, uint8_t lo)
+static inline uint32_t high32(uint64_t val)
 {
-       return (hi & 0xffffffffffffff00ULL) | lo;
-}
-
-/* this may be the only register that needs special handling.
- * If there others then we might want to extend teh emmsr struct.
- */
-static int emsr_miscenable(struct guest_thread *vm_thread, struct emmsr *msr,
-                           uint32_t opcode) {
-       uint32_t eax, edx;
-       struct vm_trapframe *vm_tf = &(vm_thread->uthread.u_ctx.tf.vm_tf);
-
-       rdmsr(msr->reg, eax, edx);
-       /* we just let them read the misc msr for now. */
-       if (opcode == EXIT_REASON_MSR_READ) {
-               vm_tf->tf_rax = set_low32(vm_tf->tf_rax, eax);
-               vm_tf->tf_rax |= MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL;
-               vm_tf->tf_rdx = set_low32(vm_tf->tf_rdx, edx);
-               return 0;
-       } else {
-               /* if they are writing what is already written, that's ok. */
-               if (((uint32_t) vm_tf->tf_rax == eax)
-                   && ((uint32_t) vm_tf->tf_rdx == edx))
-                       return 0;
-       }
-       fprintf(stderr,
-               "%s: Wanted to write 0x%x:0x%x, but could not; value was 0x%x:0x%x\n",
-                msr->name, (uint32_t) vm_tf->tf_rdx,
-                (uint32_t) vm_tf->tf_rax, edx, eax);
-       return SHUTDOWN_UNHANDLED_EXIT_REASON;
-}
-
-static int emsr_mustmatch(struct guest_thread *vm_thread, struct emmsr *msr,
-                          uint32_t opcode) {
-       uint32_t eax, edx;
-       struct vm_trapframe *vm_tf = &(vm_thread->uthread.u_ctx.tf.vm_tf);
-
-       rdmsr(msr->reg, eax, edx);
-       /* we just let them read the misc msr for now. */
-       if (opcode == EXIT_REASON_MSR_READ) {
-               vm_tf->tf_rax = set_low32(vm_tf->tf_rax, eax);
-               vm_tf->tf_rdx = set_low32(vm_tf->tf_rdx, edx);
-               return 0;
-       } else {
-               /* if they are writing what is already written, that's ok. */
-               if (((uint32_t) vm_tf->tf_rax == eax)
-                   && ((uint32_t) vm_tf->tf_rdx == edx))
-                       return 0;
-       }
-       fprintf(stderr,
-               "%s: Wanted to write 0x%x:0x%x, but could not; value was 0x%x:0x%x\n",
-                msr->name, (uint32_t) vm_tf->tf_rdx,
-                (uint32_t) vm_tf->tf_rax, edx, eax);
-       return SHUTDOWN_UNHANDLED_EXIT_REASON;
+       return val >> 32;
 }
 
 static int emsr_ok(struct guest_thread *vm_thread, struct emmsr *msr,
                    uint32_t opcode)
 {
        struct vm_trapframe *vm_tf = &(vm_thread->uthread.u_ctx.tf.vm_tf);
+       uint64_t msr_val;
 
        if (opcode == EXIT_REASON_MSR_READ) {
-               rdmsr(msr->reg, vm_tf->tf_rdx, vm_tf->tf_rax);
+               msr_val = read_msr(msr->reg);
+               vm_tf->tf_rax = low32(msr_val);
+               vm_tf->tf_rdx = high32(msr_val);
        } else {
-               uint64_t val =
-                       (uint64_t) vm_tf->tf_rdx << 32 | vm_tf->tf_rax;
-               write_msr(msr->reg, val);
+               msr_val = (vm_tf->tf_rdx << 32) | vm_tf->tf_rax;
+               write_msr(msr->reg, msr_val);
        }
        return 0;
 }
@@ -154,14 +91,13 @@ static int emsr_ok(struct guest_thread *vm_thread, struct emmsr *msr,
 static int emsr_readonly(struct guest_thread *vm_thread, struct emmsr *msr,
                          uint32_t opcode)
 {
-       uint32_t eax, edx;
        struct vm_trapframe *vm_tf = &(vm_thread->uthread.u_ctx.tf.vm_tf);
+       uint64_t msr_val;
 
-       rdmsr((uint32_t) vm_tf->tf_rcx, eax, edx);
-       /* we just let them read the misc msr for now. */
+       msr_val = read_msr(msr->reg);
        if (opcode == EXIT_REASON_MSR_READ) {
-               vm_tf->tf_rax = set_low32(vm_tf->tf_rax, eax);
-               vm_tf->tf_rdx = set_low32(vm_tf->tf_rdx, edx);
+               vm_tf->tf_rax = low32(msr_val);
+               vm_tf->tf_rdx = high32(msr_val);
                return 0;
        }
 
@@ -189,24 +125,22 @@ static int emsr_fakewrite(struct guest_thread *vm_thread, struct emmsr *msr,
                           uint32_t opcode)
 {
        uint32_t eax, edx;
+       uint64_t msr_val;
        struct vm_trapframe *vm_tf = &(vm_thread->uthread.u_ctx.tf.vm_tf);
 
        if (!msr->written) {
-               rdmsr(msr->reg, eax, edx);
+               msr_val = read_msr(msr->reg);
+               eax = low32(msr_val);
+               edx = high32(msr_val);
        } else {
-               edx = msr->edx;
                eax = msr->eax;
+               edx = msr->edx;
        }
-       /* we just let them read the misc msr for now. */
        if (opcode == EXIT_REASON_MSR_READ) {
-               vm_tf->tf_rax = set_low32(vm_tf->tf_rax, eax);
-               vm_tf->tf_rdx = set_low32(vm_tf->tf_rdx, edx);
+               vm_tf->tf_rax = eax;
+               vm_tf->tf_rdx = edx;
                return 0;
        } else {
-               /* if they are writing what is already written, that's ok. */
-               if (((uint32_t) vm_tf->tf_rax == eax)
-                   && ((uint32_t) vm_tf->tf_rdx == edx))
-                       return 0;
                msr->edx = vm_tf->tf_rdx;
                msr->eax = vm_tf->tf_rax;
                msr->written = true;