This simplifies the code and improves error diagnostics. I've tested some of the error handling paths by trying to set up a VM with bad parameters and also inserting some calls to error() in a few places.
Change-Id: I5437f559f618c132be751897785aee6da3b771be Signed-off-by: Ronald G. Minnich <[email protected]> --- kern/arch/x86/vmm/intel/vmx.c | 76 ++++++++++++++++++++----------------------- kern/arch/x86/vmm/vmm.c | 46 ++++++++++++-------------- 2 files changed, 56 insertions(+), 66 deletions(-) diff --git a/kern/arch/x86/vmm/intel/vmx.c b/kern/arch/x86/vmm/intel/vmx.c index 1ea4666..27a3329 100644 --- a/kern/arch/x86/vmm/intel/vmx.c +++ b/kern/arch/x86/vmm/intel/vmx.c @@ -687,16 +687,16 @@ setup_vmcs_config(void *p) *ret = 0; } -static struct vmcs * -__vmx_alloc_vmcs(int node) +static struct vmcs *__vmx_alloc_vmcs(int node) { struct vmcs *vmcs; vmcs = get_cont_pages_node(node, vmcs_config.order, MEM_WAIT); if (!vmcs) - return 0; + error(ENOMEM, "__vmx_alloc_vmcs: Could not get %d contig pages", + vmcs_config.order); memset(vmcs, 0, vmcs_config.size); - vmcs->revision_id = vmcs_config.revision_id; /* vmcs revision id */ + vmcs->revision_id = vmcs_config.revision_id; /* vmcs revision id */ printd("%d: set rev id %d\n", core_id(), vmcs->revision_id); return vmcs; } @@ -729,8 +729,7 @@ vmx_free_vmcs(struct vmcs *vmcs) * Note that host-state that does change is set elsewhere. E.g., host-state * that is set differently for each CPU is set in __vmx_setup_pcpu(), not here. */ -static void -vmx_setup_constant_host_state(void) +static void vmx_setup_constant_host_state(void) { uint32_t low32, high32; unsigned long tmpl; @@ -804,42 +803,42 @@ construct_eptp(physaddr_t root_hpa) /* Helper: some fields of the VMCS need a physical page address, e.g. the VAPIC * page. We have the user address. This converts the user to phys addr and * sets that up in the VMCS. Returns 0 on success, -1 o/w. */ -static int vmcs_set_pgaddr(struct proc *p, void *u_addr, unsigned long field) +static void vmcs_set_pgaddr(struct proc *p, void *u_addr, + unsigned long field, char *what) { uintptr_t kva; physaddr_t paddr; /* Enforce page alignment */ kva = uva2kva(p, ROUNDDOWN(u_addr, PGSIZE), PGSIZE, PROT_WRITE); - if (!kva) { - set_error(EINVAL, "Unmapped pgaddr %p for VMCS", u_addr); - return -1; - } + if (!kva) + error(EINVAL, "Unmapped pgaddr %p for VMCS page %s", u_addr, what); + paddr = PADDR(kva); - /* TODO: need to pin the page. A munmap would actually be okay (though - * probably we should kill the process), but we need to keep the page from - * being reused. A refcnt would do the trick, which we decref when we - * destroy the guest core/vcpu. */ + /* TODO: need to pin the page. A munmap would actually be okay + * (though probably we should kill the process), but we need to + * keep the page from being reused. A refcnt would do the trick, + * which we decref when we destroy the guest core/vcpu. Note that + * this is an assert, not an error, because it represents an error + * in the kernel itself. */ assert(!PGOFF(paddr)); vmcs_writel(field, paddr); /* Pages are inserted twice. Once, with the full paddr. The next field is * the upper 32 bits of the paddr. */ vmcs_writel(field + 1, paddr >> 32); - return 0; } /** * vmx_setup_initial_guest_state - configures the initial state of guest * registers and the VMCS. Returns 0 on success, -1 o/w. */ -static int vmx_setup_initial_guest_state(struct proc *p, - struct vmm_gpcore_init *gpci) +static void vmx_setup_initial_guest_state(struct proc *p, + struct vmm_gpcore_init *gpci) { unsigned long tmpl; unsigned long cr4 = X86_CR4_PAE | X86_CR4_VMXE | X86_CR4_OSXMMEXCPT | X86_CR4_PGE | X86_CR4_OSFXSR; uint32_t protected_mode = X86_CR0_PG | X86_CR0_PE; - int ret = 0; #if 0 do @@ -948,11 +947,11 @@ static int vmx_setup_initial_guest_state(struct proc *p, /* Initialize parts based on the users info. If one of them fails, we'll do * the others but then error out. */ - ret |= vmcs_set_pgaddr(p, gpci->posted_irq_desc, POSTED_INTR_DESC_ADDR); - ret |= vmcs_set_pgaddr(p, gpci->vapic_addr, VIRTUAL_APIC_PAGE_ADDR); - ret |= vmcs_set_pgaddr(p, gpci->apic_addr, APIC_ACCESS_ADDR); - - return ret; + vmcs_set_pgaddr(p, gpci->posted_irq_desc, POSTED_INTR_DESC_ADDR, + "posted_irq_desc"); + vmcs_set_pgaddr(p, gpci->vapic_addr, VIRTUAL_APIC_PAGE_ADDR, + "vapic_addr"); + vmcs_set_pgaddr(p, gpci->apic_addr, APIC_ACCESS_ADDR, "apic_addr"); } static void __vmx_disable_intercept_for_msr(unsigned long *msr_bitmap, @@ -1126,37 +1125,34 @@ static void vmx_setup_vmcs(struct guest_pcore *gpc) struct guest_pcore *create_guest_pcore(struct proc *p, struct vmm_gpcore_init *gpci) { - struct guest_pcore *gpc = kmalloc(sizeof(struct guest_pcore), - MEM_WAIT); - int ret; - + ERRSTACK(1); + struct guest_pcore *gpc = kmalloc(sizeof(struct guest_pcore), MEM_WAIT); if (!gpc) - return NULL; + error(ENOMEM, "create_guest_pcore could not allocate gpc"); + + if (waserror()) { + kfree(gpc); + nexterror(); + } memset(gpc, 0, sizeof(*gpc)); - gpc->proc = p; /* uncounted (weak) reference */ + /* Warning: p here is uncounted (weak) reference */ + gpc->proc = p; gpc->vmcs = vmx_alloc_vmcs(); printd("%d: gpc->vmcs is %p\n", core_id(), gpc->vmcs); - if (!gpc->vmcs) - goto fail_vmcs; gpc->cpu = -1; vmx_load_guest_pcore(gpc); vmx_setup_vmcs(gpc); - ret = vmx_setup_initial_guest_state(p, gpci); + vmx_setup_initial_guest_state(p, gpci); vmx_unload_guest_pcore(gpc); gpc->xcr0 = __proc_global_info.x86_default_xcr0; gpc->posted_irq_desc = gpci->posted_irq_desc; - - if (!ret) - return gpc; - -fail_vmcs: - kfree(gpc); - return NULL; + poperror(); + return gpc; } /** diff --git a/kern/arch/x86/vmm/vmm.c b/kern/arch/x86/vmm/vmm.c index 552f2c1..0781544 100644 --- a/kern/arch/x86/vmm/vmm.c +++ b/kern/arch/x86/vmm/vmm.c @@ -70,50 +70,44 @@ void vmm_pcpu_init(void) int vmm_struct_init(struct proc *p, unsigned int nr_guest_pcores, struct vmm_gpcore_init *u_gpcis, int flags) { + ERRSTACK(1); struct vmm *vmm = &p->vmm; unsigned int i; struct vmm_gpcore_init gpci; - if (flags & ~VMM_ALL_FLAGS) { - set_errstr("%s: flags is 0x%lx, VMM_ALL_FLAGS is 0x%lx\n", __func__, - flags, VMM_ALL_FLAGS); - set_errno(EINVAL); - return 0; - } + if (flags & ~VMM_ALL_FLAGS) + error(EINVAL, "%s: flags is 0x%lx, VMM_ALL_FLAGS is 0x%lx\n", __func__, + flags, VMM_ALL_FLAGS); vmm->flags = flags; - if (!x86_supports_vmx) { - set_errno(ENODEV); - return 0; - } + if (!x86_supports_vmx) + error(ENODEV, "This CPU does not support VMX"); qlock(&vmm->qlock); - if (vmm->vmmcp) { - set_errno(EINVAL); + if (waserror()) { qunlock(&vmm->qlock); - return 0; + nexterror(); } + + /* TODO: just use an atomic test instead of all this locking stuff? */ + if (vmm->vmmcp) + error(EAGAIN, "We're already running a vmmcp?"); /* Set this early, so cleanup checks the gpc array */ vmm->vmmcp = TRUE; nr_guest_pcores = MIN(nr_guest_pcores, num_cores); vmm->amd = 0; - vmm->guest_pcores = kzmalloc(sizeof(void*) * nr_guest_pcores, - MEM_WAIT); + vmm->guest_pcores = kzmalloc(sizeof(void *) * nr_guest_pcores, MEM_WAIT); + if (!vmm->guest_pcores) + error(ENOMEM, "Allocation of vmm->guest_pcores failed"); + for (i = 0; i < nr_guest_pcores; i++) { - if (copy_from_user(&gpci, &u_gpcis[i], - sizeof(struct vmm_gpcore_init))) { - set_error(EINVAL, "Bad pointer %p for gps", u_gpcis); - break; - } + if (copy_from_user(&gpci, &u_gpcis[i], sizeof(struct vmm_gpcore_init))) + error(EINVAL, "Bad pointer %p for gps", u_gpcis); vmm->guest_pcores[i] = create_guest_pcore(p, &gpci); - /* If we failed, we'll clean it up when the process dies */ - if (!vmm->guest_pcores[i]) { - set_errno(ENOMEM); - break; - } + vmm->nr_guest_pcores = i; } - vmm->nr_guest_pcores = i; for (int i = 0; i < VMM_VMEXIT_NR_TYPES; i++) vmm->vmexits[i] = 0; qunlock(&vmm->qlock); + poperror(); return i; } -- 2.8.0.rc3.226.g39d4020 -- You received this message because you are subscribed to the Google Groups "Akaros" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. For more options, visit https://groups.google.com/d/optout.
