* Elliot Berman [off-list ref] [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!


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel