Messages in this thread | | | Date | Thu, 9 Feb 2023 16:09:55 +0530 | From | Srivatsa Vaddagiri <> | Subject | Re: [PATCH v9 24/27] virt: gunyah: Add proxy-scheduled vCPUs |
| |
* Elliot Berman <quic_eberman@quicinc.com> [2023-01-20 14:46:23]:
> +static void gh_handle_mmio_return(struct gunyah_vcpu *vcpu, u64 *state) > +{ > + if (!vcpu->vcpu_run->mmio.is_write) > + memcpy(&state[0], vcpu->vcpu_run->mmio.data, vcpu->vcpu_run->mmio.len);
Would be good to do a bound check on length of memcpy I think (in case vcpu_run_resp->state_data[1] is wrong for example).
> + > + vcpu->handle_mmio = false; > + vcpu->vcpu_run->exit_reason = GH_VM_EXIT_UNKNOWN; > +} > +
// snip
> +static int gh_vcpu_run(struct gunyah_vcpu *vcpu) > +{ > + struct gh_hypercall_vcpu_run_resp vcpu_run_resp; > + u64 state_data[3] = { 0 }; > + int ret = 0; > + > + ret = gh_vm_ensure_started(vcpu->ghvm); > + if (ret) > + return ret;
Can we move this to VM_START ioctl and avoid this check in fast path? In case VM is not started, then I think gh_hypercall_vcpu_run() will fail which can catch erroneous use of VCPU_RUN w/o a preceding VM_START. Alternately we could use a flag in vcpu struct to check for this case (similar to test for vcpu->rsc below).
// snip
> + case GH_VCPU_STATE_EXPECTS_WAKEUP: > + case GH_VCPU_STATE_POWERED_OFF: > + ret = wait_for_completion_interruptible(&vcpu->ready);
I think we should end this wait in case immediate_exit is set as well.
> +static vm_fault_t gh_vcpu_fault(struct vm_fault *vmf) > +{ > + struct gunyah_vcpu *vcpu = vmf->vma->vm_file->private_data; > + struct page *page = NULL; > + > + if (vmf->pgoff == 0) > + page = virt_to_page(vcpu->vcpu_run); > + > + get_page(page);
We should avoid get_page in case page is NULL.
> + vmf->page = page; > + return 0; > +} > + > +static void gunyah_vcpu_unpopulate(struct gunyah_vm_resource_ticket *ticket, > + struct gunyah_resource *ghrsc) > +{ > + struct gunyah_vcpu *vcpu = container_of(ticket, struct gunyah_vcpu, ticket); > + > + vcpu->vcpu_run->immediate_exit = true;
We should poke the vcpu thread as well so that it can notice this. Otherwise it can continue to be in gh_hypercall_vcpu_run() or wait_for_completion_interruptible() for longer time to come.
> + mutex_lock(&vcpu->run_lock); > + free_irq(vcpu->rsc->irq, vcpu); > + vcpu->rsc = NULL; > + mutex_unlock(&vcpu->run_lock); > +} > + > +static long gunyah_vcpu_bind(struct gunyah_vm_function *f) > +{ > + struct gunyah_vcpu *vcpu; > + char name[MAX_VCPU_NAME]; > + struct file *file; > + struct page *page; > + int fd; > + long r; > + > + if (!gh_api_has_feature(GH_API_FEATURE_VCPU))
We should test for this feature before registering the function? What's the point in registering a function otherwise if it can't do its work!
| |