x86: Bring host CR0 into well-defined state during setup

Avoid any uncertainty about the state of CR0 left behind by Linux: check
for unexpectedly set reserved bits or required-1 bits, and otherwise set
our own state.

CC: Valentine Sinitsyn <valentine.sinitsyn@gmail.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

diff --git a/hypervisor/arch/x86/include/asm/percpu.h b/hypervisor/arch/x86/include/asm/percpu.h
index 1970a1335453e5073fbeb3a45e06fa217a827c74..183bff5605d6f81667c72e53f10e3e1700b5be5b 100644 (file)
--- a/hypervisor/arch/x86/include/asm/percpu.h
+++ b/hypervisor/arch/x86/include/asm/percpu.h
@@ -63,6 +63,7 @@ struct per_cpu {
        struct desc_table_reg linux_idtr;
        unsigned long linux_reg[NUM_ENTRY_REGS];
        unsigned long linux_ip;
+       unsigned long linux_cr0;
        unsigned long linux_cr3;
        struct segment linux_cs;
        struct segment linux_ds;

diff --git a/hypervisor/arch/x86/include/asm/vcpu.h b/hypervisor/arch/x86/include/asm/vcpu.h
index 339922215b5cbafc9905596a28c22c52d6aa26fc..6d95e1749bedd50b93f66070b51801d0a823cc44 100644 (file)
--- a/hypervisor/arch/x86/include/asm/vcpu.h
+++ b/hypervisor/arch/x86/include/asm/vcpu.h
@@ -21,6 +21,10 @@
 #include <asm/percpu.h>
 #include <asm/processor.h>
 
+#define X86_CR0_HOST_STATE \
+       (X86_CR0_PG | X86_CR0_WP | X86_CR0_NE | X86_CR0_ET | X86_CR0_TS | \
+        X86_CR0_MP | X86_CR0_PE)
+
 struct vcpu_io_bitmap {
        u8 *data;
        u32 size;

diff --git a/hypervisor/arch/x86/setup.c b/hypervisor/arch/x86/setup.c
index 8500669296243a53df9055c912e2039112d02579..89e2ebf58a5153cefcdd90489b9e8bd86a72bb5c 100644 (file)
--- a/hypervisor/arch/x86/setup.c
+++ b/hypervisor/arch/x86/setup.c
@@ -186,6 +186,8 @@ int arch_cpu_init(struct per_cpu *cpu_data)
        gdt[GDT_DESC_TSS] &= ~DESC_TSS_BUSY;
        asm volatile("ltr %%ax" : : "a" (GDT_DESC_TSS * 8));
 
+       cpu_data->linux_cr0 = read_cr0();
+
        /* swap CR3 */
        cpu_data->linux_cr3 = read_cr3();
        write_cr3(paging_hvirt2phys(hv_paging_structs.root_table));
@@ -251,6 +253,7 @@ void arch_cpu_restore(struct per_cpu *cpu_data, int return_code)
        vcpu_exit(cpu_data);
 
        write_msr(MSR_EFER, cpu_data->linux_efer);
+       write_cr0(cpu_data->linux_cr0);
        write_cr3(cpu_data->linux_cr3);
 
        asm volatile("lgdtq %0" : : "m" (cpu_data->linux_gdtr));

diff --git a/hypervisor/arch/x86/svm.c b/hypervisor/arch/x86/svm.c
index 52b4c8a7fcea218fac40c4799f5378e7bc793b77..299295c607ee5eef2b8bcaf2d672dd3631e02036 100644 (file)
--- a/hypervisor/arch/x86/svm.c
+++ b/hypervisor/arch/x86/svm.c
@@ -175,7 +175,7 @@ static int vmcb_setup(struct per_cpu *cpu_data)
 
        memset(vmcb, 0, sizeof(struct vmcb));
 
-       vmcb->cr0 = read_cr0() & SVM_CR0_ALLOWED_BITS;
+       vmcb->cr0 = cpu_data->linux_cr0 & SVM_CR0_ALLOWED_BITS;
        vmcb->cr3 = cpu_data->linux_cr3;
        vmcb->cr4 = read_cr4();
 
@@ -399,6 +399,20 @@ int vcpu_init(struct per_cpu *cpu_data)
        if (!vmcb_setup(cpu_data))
                return -EIO;
 
+       /*
+        * APM Volume 2, 3.1.1: "When writing the CR0 register, software should
+        * set the values of reserved bits to the values found during the
+        * previous CR0 read."
+        * But we want to avoid surprises with new features unknown to us but
+        * set by Linux. So check if any assumed revered bit was set and bail
+        * out if so.
+        */
+       if (cpu_data->linux_cr0 & X86_CR0_RESERVED)
+               return -EIO;
+
+       /* bring CR0 into well-defined state */
+       write_cr0(X86_CR0_HOST_STATE);
+
        write_msr(MSR_VM_HSAVE_PA, paging_hvirt2phys(cpu_data->host_state));
 
        return 0;
@@ -484,6 +498,7 @@ vcpu_deactivate_vmm(struct registers *guest_regs)
        write_msr(MSR_KERNGS_BASE, vmcb->kerngsbase);
        write_msr(MSR_IA32_PAT, vmcb->g_pat);
 
+       cpu_data->linux_cr0 = vmcb->cr0;
        cpu_data->linux_cr3 = vmcb->cr3;
 
        cpu_data->linux_gdtr.base = vmcb->gdtr.base;

diff --git a/hypervisor/arch/x86/vmx.c b/hypervisor/arch/x86/vmx.c
index f7606980b7e4d567bd5121b32216b0176de04bfa..b107f9f6648e80b9b8fe416bce1ba59b7fe1b376 100644 (file)
--- a/hypervisor/arch/x86/vmx.c
+++ b/hypervisor/arch/x86/vmx.c
@@ -471,7 +471,7 @@ static bool vmcs_setup(struct per_cpu *cpu_data)
                           sizeof(cpu_data->stack));
        ok &= vmcs_write64(HOST_RIP, (unsigned long)vmx_vmexit);
 
-       ok &= vmx_set_guest_cr(CR0_IDX, read_cr0());
+       ok &= vmx_set_guest_cr(CR0_IDX, cpu_data->linux_cr0);
        ok &= vmx_set_guest_cr(CR4_IDX, read_cr4());
 
        ok &= vmcs_write64(GUEST_CR3, cpu_data->linux_cr3);
@@ -583,8 +583,6 @@ int vcpu_init(struct per_cpu *cpu_data)
        cpu_data->vmcs.revision_id = revision_id;
        cpu_data->vmcs.shadow_indicator = 0;
 
-       // TODO: validate CR0
-
        /* Note: We assume that TXT is off */
        feature_ctrl = read_msr(MSR_IA32_FEATURE_CONTROL);
        mask = FEATURE_CONTROL_LOCKED |
@@ -598,6 +596,21 @@ int vcpu_init(struct per_cpu *cpu_data)
                write_msr(MSR_IA32_FEATURE_CONTROL, feature_ctrl);
        }
 
+       /*
+        * SDM Volume 3, 2.5: "When loading a control register, reserved bits
+        * should always be set to the values previously read."
+        * But we want to avoid surprises with new features unknown to us but
+        * set by Linux. So check if any assumed revered bit was set or should
+        * be set for VMX operation and bail out if so.
+        */
+       if ((cpu_data->linux_cr0 | cr_required1[CR0_IDX]) & X86_CR0_RESERVED)
+               return -EIO;
+       /*
+        * Bring CR0 into well-defined state. If it doesn't match with VMX
+        * requirements, vmxon will fail.
+        */
+       write_cr0(X86_CR0_HOST_STATE);
+
        write_cr4(cr4 | X86_CR4_VMXE);
        // TODO: validate CR4
 
@@ -662,6 +675,7 @@ vcpu_deactivate_vmm(struct registers *guest_regs)
        unsigned long linux_ip = vmcs_read64(GUEST_RIP);
        struct per_cpu *cpu_data = this_cpu_data();
 
+       cpu_data->linux_cr0 = vmcs_read64(GUEST_CR0);
        cpu_data->linux_cr3 = vmcs_read64(GUEST_CR3);
 
        cpu_data->linux_gdtr.base = vmcs_read64(GUEST_GDTR_BASE);