thanks! Merged to master at 4a63f6ca874e..15ae76bd6cbd (from, to]
You can see the entire diff with 'git diff' or at https://github.com/brho/akaros/compare/4a63f6ca874e...15ae76bd6cbd On 2016-08-01 at 15:29 "Ronald G. Minnich" <[email protected]> wrote: > 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; > } > -- 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.
