On 2016-07-20 at 17:57 "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.
Looks like a nice overall, esp if it helped you debug things. =)
Minor things below:
> diff --git a/kern/arch/x86/vmm/vmm.c b/kern/arch/x86/vmm/vmm.c
> index 552f2c1..33f5264 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? */
Yeah, I think an atomic_swap would work here. Not a huge deal either way.
> + 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;
So one thing that this might break is cleanup. In case of failure, the
old code would clean up whatever had been set up so far. (Say we set up
3 of 6 GPCs). That's what the "If we failed" comment was about, and why
nr_guest_pcores was set to 'i' even in the case of failure.
Basically, __vmm_struct_cleanup() will destroy guest pcores for i = 0 to
nr_guest_pcores. Now, I think we're leaking guest pcores in the case
of errors.
One easy fix for this would be to set vmm->nr_guest_pcores = i + 1 after
each successful create_guest_pcore() (basically at the bottom of the
loop).
Barret
--
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.
