From owner-freebsd-virtualization@FreeBSD.ORG Sat Apr 6 01:27:36 2013 Return-Path: Delivered-To: freebsd-virtualization@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 122D5D8B for ; Sat, 6 Apr 2013 01:27:36 +0000 (UTC) (envelope-from torek@torek.net) Received: from elf.torek.net (50-73-42-1-utah.hfc.comcastbusiness.net [50.73.42.1]) by mx1.freebsd.org (Postfix) with ESMTP id E1BED69E for ; Sat, 6 Apr 2013 01:27:34 +0000 (UTC) Received: from elf.torek.net (localhost [127.0.0.1]) by elf.torek.net (8.14.5/8.14.5) with ESMTP id r361RRGv068417 for ; Fri, 5 Apr 2013 19:27:27 -0600 (MDT) (envelope-from torek@torek.net) Message-Id: <201304060127.r361RRGv068417@elf.torek.net> From: Chris Torek To: freebsd-virtualization@freebsd.org Subject: two more small improvements Date: Fri, 05 Apr 2013 19:27:27 -0600 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.2.7 (elf.torek.net [127.0.0.1]); Fri, 05 Apr 2013 19:27:27 -0600 (MDT) X-BeenThere: freebsd-virtualization@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Discussion of various virtualization techniques FreeBSD supports." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 06 Apr 2013 01:27:36 -0000 When I first went to run bhyve on one hardware box I had the entire system crash. This turned out to be due to the BIOS disabling VMX. A message came out on the console, but the scripts continued anyway, and then, boom. I first looked at the wrong bit of code for fixing this, but I *think* I improved it anyway. :-) That's the first patch below. The second patch below (note: I'll put them somewhere on torek.net if that's better than just including them here) fixes the crash. The easiest way to reproduce it is to attempt to create a VM within a bhyve VM ("bh1" is a running VM): [before patch -- note, some wraparound fixed] root@bh1:~ # kldload vmm vmx_init: processor does not support VMX operation module_register_init: MOD_LOAD (vmm, 0xffffffff81a133b0, 0) error 6 root@bh1:~ # sysctl hw.vmm hw.vmm.destroy: beavis hw.vmm.create: beavis hw.vmm.pages_allocated: 0 root@bh1:~ # sysctl hw.vmm.create hw.vmm.create: beavis root@bh1:~ # sysctl hw.vmm.create= hw.vmm.create: beavisvm exit[1] reason VMX rip 0xffffffff81a1932c inst_length 6 error 0 exit_reason 50 qualification 0xfffffffffffffff0 vm exit[0] reason VMX rip 0xffffffff81a1932c inst_length 6 error 0 exit_reason 50 qualification 0xfffffffffffffff0 [after patch] root@bh1:~ # kldload vmm vmx_init: processor does not support VMX operation module_register_init: MOD_LOAD (vmm, 0xffffffff81a133d0, 0) error 6 root@bh1:~ # sysctl hw.vmm hw.vmm.destroy: beavis hw.vmm.create: beavis hw.vmm.pages_allocated: 0 root@bh1:~ # sysctl hw.vmm.create hw.vmm.create: beavis root@bh1:~ # sysctl hw.vmm.create= hw.vmm.create: beavis sysctl: hw.vmm.create=: Device not configured root@bh1:~ # Chris changeset: 1962:7e204f321854 user: Chris Torek date: Fri Apr 05 19:10:09 2013 -0600 files: sys/amd64/vmm/intel/vmx.c description: improve test for VMX-mode We only need VMX extensions to be enabled, not locked. Moreover, we should not care about any VMX-in-SMX settings. diff --git a/sys/amd64/vmm/intel/vmx.c b/sys/amd64/vmm/intel/vmx.c --- a/sys/amd64/vmm/intel/vmx.c +++ b/sys/amd64/vmm/intel/vmx.c @@ -436,14 +436,33 @@ return (ENXIO); } +/* + * Bits in MSR_IA32_FEATURE_CONTROL register. + * + * XXX move these to machine/specialreg.h + */ +#define MSR_IA32_FEAT_CTL_LOCK 0x01 /* locks the control reg */ +#define MSR_IA32_FEAT_CTL_VMX_SMX_EN 0x02 /* enable VMXON in SMX mode */ +#define MSR_IA32_FEAT_CTL_VMX_EN 0x04 /* enable VMXON (non-SMX) */ /* - * Verify that MSR_IA32_FEATURE_CONTROL lock and VMXON enable bits - * are set (bits 0 and 2 respectively). + * Verify that VMXON is allowed. + * + * According to Intel docs, we just need VMX_EN to be + * set. If the LOCK bit is not set we can set or clear + * VMX_EN ourselves. Once the LOCK bit is set no more + * changes are possible without a processor reset. + * + * Existing BIOSes currently set-and-lock the feature, but + * this code should work with BIOSes that don't lock it. */ feature_control = rdmsr(MSR_IA32_FEATURE_CONTROL); - if ((feature_control & 0x5) != 0x5) { - printf("vmx_init: VMX operation disabled by BIOS\n"); - return (ENXIO); + if ((feature_control & MSR_IA32_FEAT_CTL_VMX_EN) == 0) { + if (feature_control & MSR_IA32_FEAT_CTL_LOCK) { + printf("vmx_init: VMX operation disabled by BIOS\n"); + return (ENXIO); + } + feature_control |= MSR_IA32_FEAT_CTL_VMX_EN; + wrmsr(MSR_IA32_FEATURE_CONTROL, feature_control); } /* Check support for primary processor-based VM-execution controls */ changeset: 1963:6cea2a2ed727 tag: tip user: Chris Torek date: Fri Apr 05 19:14:05 2013 -0600 files: sys/amd64/include/vmm.h sys/amd64/vmm/vmm.c sys/amd64/vmm/vmm_dev.c description: prevent host OS crash when VMX is disabled If VMX is disabled in the BIOS, vmm_init correctly returns an error, but this still leaves the module loaded and running, and a later sysctl to create a virtual machine crashed the host. Have vm_create() check, and return an error for this condition. diff --git a/sys/amd64/include/vmm.h b/sys/amd64/include/vmm.h --- a/sys/amd64/include/vmm.h +++ b/sys/amd64/include/vmm.h @@ -87,7 +87,7 @@ extern struct vmm_ops vmm_ops_intel; extern struct vmm_ops vmm_ops_amd; -struct vm *vm_create(const char *name); +int vm_create(const char *name, struct vm **retval); void vm_destroy(struct vm *vm); const char *vm_name(struct vm *vm); int vm_malloc(struct vm *vm, vm_paddr_t gpa, size_t len); diff --git a/sys/amd64/vmm/vmm.c b/sys/amd64/vmm/vmm.c --- a/sys/amd64/vmm/vmm.c +++ b/sys/amd64/vmm/vmm.c @@ -213,6 +213,15 @@ vmmdev_init(); iommu_init(); error = vmm_init(); + if (error) { + /* + * Returning an error here does not force + * the module to be unloaded, so we have + * to make sure that vm_create()s will not + * proceed. + */ + ops = NULL; + } break; case MOD_UNLOAD: error = vmmdev_cleanup(); @@ -249,8 +258,8 @@ SYSCTL_NODE(_hw, OID_AUTO, vmm, CTLFLAG_RW, NULL, NULL); -struct vm * -vm_create(const char *name) +int +vm_create(const char *name, struct vm **retval) { int i; struct vm *vm; @@ -258,8 +267,11 @@ const int BSP = 0; + if (ops == NULL) + return (ENXIO); + if (name == NULL || strlen(name) >= VM_MAX_NAMELEN) - return (NULL); + return (EINVAL); vm = malloc(sizeof(struct vm), M_VM, M_WAITOK | M_ZERO); strcpy(vm->name, name); @@ -274,7 +286,8 @@ vm->iommu = iommu_create_domain(maxaddr); vm_activate_cpu(vm, BSP); - return (vm); + *retval = vm; + return (0); } static void diff --git a/sys/amd64/vmm/vmm_dev.c b/sys/amd64/vmm/vmm_dev.c --- a/sys/amd64/vmm/vmm_dev.c +++ b/sys/amd64/vmm/vmm_dev.c @@ -475,9 +475,9 @@ if (sc != NULL) return (EEXIST); - vm = vm_create(buf); - if (vm == NULL) - return (EINVAL); + error = vm_create(buf, &vm); + if (error) + return (error); sc = malloc(sizeof(struct vmmdev_softc), M_VMMDEV, M_WAITOK | M_ZERO); sc->vm = vm;