don't care about Don't Care bits (#211)

The previous code cared about Don't Care bits in capability fields for controls by terminating with an error when encountering any unknown ones. This causes problems on newer CPUs, which add new such bits for new features. It is safe and recommended to simply retain the current value in the respective control for such bits, and this change does that. Some rearchitecting was needed, because the previous code treated the capabilities as global.

Co-authored-by: Julien Oster <julieno@apple.com>

include/xhyve/vmm/intel/vmx.h

@@ -46,6 +46,7 @@ struct vmxstate {
 	uint64_t nextrip;	/* next instruction to be executed by guest */
 	int	lastcpu;	/* host cpu that this 'vcpu' last ran on */
 	uint16_t vpid;
+    uint32_t entry_ctls;
 };
 #pragma clang diagnostic pop
 

include/xhyve/vmm/intel/vmx_msr.h

@@ -41,5 +41,5 @@ void vmx_msr_guest_init(struct vmx *vmx, int vcpuid);
 int vmx_rdmsr(struct vmx *, int vcpuid, u_int num, uint64_t *val);
 int vmx_wrmsr(struct vmx *, int vcpuid, u_int num, uint64_t val);
 
-int vmx_set_ctlreg(hv_vmx_capability_t cap_field, uint32_t ones_mask,
+int vmx_set_ctlreg(int vcpu_id, uint32_t field, hv_vmx_capability_t cap_field, uint32_t ones_mask,
 				   uint32_t zeros_mask, uint32_t *retval);

src/vmm/intel/vmx.c

@@ -118,8 +118,6 @@
 #define	HANDLED		1
 #define	UNHANDLED	0
 
-static uint32_t pinbased_ctls, procbased_ctls, procbased_ctls2;
-static uint32_t exit_ctls, entry_ctls;
 static uint64_t cr0_ones_mask, cr0_zeros_mask;
 static uint64_t cr4_ones_mask, cr4_zeros_mask;
 
@@ -476,60 +474,6 @@ vmx_init(void)
 			xhyve_abort("hv_vm_create failed\n");
 	}
 
-	/* Check support for primary processor-based VM-execution controls */
-	error = vmx_set_ctlreg(HV_VMX_CAP_PROCBASED,
-			       PROCBASED_CTLS_ONE_SETTING,
-			       PROCBASED_CTLS_ZERO_SETTING, &procbased_ctls);
-	if (error) {
-		printf("vmx_init: processor does not support desired primary "
-		       "processor-based controls\n");
-		return (error);
-	}
-
-	/* Clear the processor-based ctl bits that are set on demand */
-	procbased_ctls &= ~PROCBASED_CTLS_WINDOW_SETTING;
-
-	/* Check support for secondary processor-based VM-execution controls */
-	error = vmx_set_ctlreg(HV_VMX_CAP_PROCBASED2,
-			       PROCBASED_CTLS2_ONE_SETTING,
-			       PROCBASED_CTLS2_ZERO_SETTING, &procbased_ctls2);
-	if (error) {
-		printf("vmx_init: processor does not support desired secondary "
-		       "processor-based controls\n");
-		return (error);
-	}
-
-	/* Check support for pin-based VM-execution controls */
-	error = vmx_set_ctlreg(HV_VMX_CAP_PINBASED,
-			       PINBASED_CTLS_ONE_SETTING,
-			       PINBASED_CTLS_ZERO_SETTING, &pinbased_ctls);
-	if (error) {
-		printf("vmx_init: processor does not support desired "
-		       "pin-based controls\n");
-		return (error);
-	}
-
-	/* Check support for VM-exit controls */
-	error = vmx_set_ctlreg(HV_VMX_CAP_EXIT,
-			       VM_EXIT_CTLS_ONE_SETTING,
-			       VM_EXIT_CTLS_ZERO_SETTING,
-			       &exit_ctls);
-	if (error) {
-		printf("vmx_init: processor does not support desired "
-		    "exit controls\n");
-		return (error);
-	}
-
-	/* Check support for VM-entry controls */
-	error = vmx_set_ctlreg(HV_VMX_CAP_ENTRY,
-	    VM_ENTRY_CTLS_ONE_SETTING, VM_ENTRY_CTLS_ZERO_SETTING,
-	    &entry_ctls);
-	if (error) {
-		printf("vmx_init: processor does not support desired "
-		    "entry controls\n");
-		return (error);
-	}
-
 	/*
 	 * Check support for optional features by testing them
 	 * as individual bits
@@ -629,11 +573,72 @@ vmx_vcpu_init(void *arg, int vcpuid) {
 
 	vmx_msr_guest_init(vmx, vcpuid);
 
+	uint32_t pinbased_ctls = 0;
+	uint32_t procbased_ctls = 0;
+	uint32_t procbased_ctls2 = 0;
+	uint32_t exit_ctls = 0;
+	vmx->state[vcpuid].entry_ctls = 0;
+
+	/* Check support for primary processor-based VM-execution controls */
+	error = vmx_set_ctlreg(vcpuid, VMCS_PRI_PROC_BASED_CTLS,
+						   HV_VMX_CAP_PROCBASED,
+						   PROCBASED_CTLS_ONE_SETTING,
+						   PROCBASED_CTLS_ZERO_SETTING, &procbased_ctls);
+	if (error) {
+		printf("vmx_init: processor does not support desired primary "
+			   "processor-based controls\n");
+		return (error);
+	}
+
+	/* Clear the processor-based ctl bits that are set on demand */
+	procbased_ctls &= ~PROCBASED_CTLS_WINDOW_SETTING;
+
+	/* Check support for secondary processor-based VM-execution controls */
+	error = vmx_set_ctlreg(vcpuid, VMCS_SEC_PROC_BASED_CTLS, HV_VMX_CAP_PROCBASED2,
+						   PROCBASED_CTLS2_ONE_SETTING,
+						   PROCBASED_CTLS2_ZERO_SETTING, &procbased_ctls2);
+	if (error) {
+		printf("vmx_init: processor does not support desired secondary "
+			   "processor-based controls\n");
+		return (error);
+	}
+
+	/* Check support for pin-based VM-execution controls */
+	error = vmx_set_ctlreg(vcpuid, VMCS_PIN_BASED_CTLS, HV_VMX_CAP_PINBASED,
+						   PINBASED_CTLS_ONE_SETTING,
+						   PINBASED_CTLS_ZERO_SETTING, &pinbased_ctls);
+	if (error) {
+		printf("vmx_init: processor does not support desired "
+			   "pin-based controls\n");
+		return (error);
+	}
+
+	/* Check support for VM-exit controls */
+	error = vmx_set_ctlreg(vcpuid, VMCS_EXIT_CTLS, HV_VMX_CAP_EXIT,
+						   VM_EXIT_CTLS_ONE_SETTING,
+						   VM_EXIT_CTLS_ZERO_SETTING,
+						   &exit_ctls);
+	if (error) {
+		printf("vmx_init: processor does not support desired "
+			   "exit controls\n");
+		return (error);
+	}
+
+	/* Check support for VM-entry controls */
+	error = vmx_set_ctlreg(vcpuid, VMCS_ENTRY_CTLS, HV_VMX_CAP_ENTRY,
+						   VM_ENTRY_CTLS_ONE_SETTING, VM_ENTRY_CTLS_ZERO_SETTING,
+						   &vmx->state[vcpuid].entry_ctls);
+	if (error) {
+		printf("vmx_init: processor does not support desired "
+			   "entry controls\n");
+		return (error);
+	}
+
 	vmcs_write(vcpuid, VMCS_PIN_BASED_CTLS, pinbased_ctls);
 	vmcs_write(vcpuid, VMCS_PRI_PROC_BASED_CTLS, procbased_ctls);
 	vmcs_write(vcpuid, VMCS_SEC_PROC_BASED_CTLS, procbased_ctls2);
 	vmcs_write(vcpuid, VMCS_EXIT_CTLS, exit_ctls);
-	vmcs_write(vcpuid, VMCS_ENTRY_CTLS, entry_ctls);
+	vmcs_write(vcpuid, VMCS_ENTRY_CTLS, vmx->state[vcpuid].entry_ctls);
 
 	/* exception bitmap */
 	if (vcpu_trace_exceptions())
@@ -2380,8 +2385,8 @@ vmx_setreg(void *arg, int vcpu, int reg, uint64_t val)
 		 * value of EFER.LMA must be identical to "IA-32e mode guest"
 		 * bit in the VM-entry control.
 		 */
-		if ((entry_ctls & VM_ENTRY_LOAD_EFER) != 0 &&
-		    (reg == VM_REG_GUEST_EFER)) {
+		if ((vmx->state[vcpu].entry_ctls & VM_ENTRY_LOAD_EFER) != 0 &&
+			(reg == VM_REG_GUEST_EFER)) {
 			vmcs_getreg(vcpu, VMCS_IDENT(VMCS_ENTRY_CTLS), &ctls);
 			if (val & EFER_LMA)
 				ctls |= VM_ENTRY_GUEST_LMA;

src/vmm/intel/vmx_msr.c

@@ -57,8 +57,9 @@ vmx_ctl_allows_zero_setting(uint64_t msr_val, int bitpos)
 		return (FALSE);
 }
 
-int vmx_set_ctlreg(hv_vmx_capability_t cap_field, uint32_t ones_mask,
-	uint32_t zeros_mask, uint32_t *retval)
+int vmx_set_ctlreg(int vcpu_id, uint32_t field,
+				   hv_vmx_capability_t cap_field, uint32_t ones_mask,
+				   uint32_t zeros_mask, uint32_t *retval)
 {
 	int i;
 	uint64_t cap;
@@ -73,6 +74,8 @@ int vmx_set_ctlreg(hv_vmx_capability_t cap_field, uint32_t ones_mask,
 		return EINVAL;
 	}
 
+	uint64_t current = vmcs_read(vcpu_id, field);
+
 	for (i = 0; i < 32; i++) {
 		one_allowed = vmx_ctl_allows_one_setting(cap, i);
 		zero_allowed = vmx_ctl_allows_zero_setting(cap, i);
@@ -102,11 +105,8 @@ int vmx_set_ctlreg(hv_vmx_capability_t cap_field, uint32_t ones_mask,
 			} else if (ones_mask & (1 << i)) {
 				*retval |= 1 << i;
 			} else {
-				/* XXX: don't allow unspecified don't cares */
-				fprintf(stderr,
-					"vmx_set_ctlreg: cap_field: %d bit: %d unspecified "
-					"don't care\n", cap_field, i);
-				return (EINVAL);
+				// Unknown: keep existing value.
+				*retval = (*retval & ~(1 << i)) | (current & (1 << i));
 			}
 		}
 	}

xhyve.xcodeproj/project.pbxproj

@@ -264,6 +264,7 @@
 				3F1934931BF7C0D40099CC46 /* Products */,
 			);
 			sourceTree = "<group>";
+			usesTabs = 1;
 		};
 		3F1934931BF7C0D40099CC46 /* Products */ = {
 			isa = PBXGroup;