Re: [Sound-open-firmware] [PATCH RFC] vhost: add an SOF Audio DSP driver

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

> +config VHOST_SOF
> +	tristate "Vhost SOF driver"
> +	depends on SND_SOC_SOF

you probably want to make sure VHOST_SOF and SND_SOC_SOF are both 
built-in or module. Without constraints you are likely to trigger errors 
with randconfig. It's a classic.

> +	select VHOST
> +	select VHOST_RPMSG
> +	default n

not needed, default is always no.

> +	---help---
> +	  SOF vhost VirtIO driver. It exports the same IPC interface, as the
> +	  one, used for Audio DSP communication, to Linux VirtIO guests.

[...]

> +struct vhost_dsp {
> +	struct vhost_rpmsg vrdev;
> +
> +	struct sof_vhost_client *snd;
> +
> +	bool active;

I am struggling with this definition, it seems to be a local flag but 
how is it aligned to the actual DSP status?
In other words, can you have cases where the dsp is considered 'active' 
here but might be pm_runtime suspended?
I am sure you've thought of it based on previous threads, it'd be worth 
adding comments.

> +
> +	/* RPMsg address of the position update endpoint */
> +	u32 posn_addr;
> +	/* position update buffer and work */
> +	struct vhost_work posn_work;
> +	struct sof_ipc_stream_posn posn;
> +
> +	/* IPC request buffer */
> +	struct sof_rpmsg_ipc_req ipc_buf;
> +	/* IPC response buffer */
> +	u8 reply_buf[SOF_IPC_MSG_MAX_SIZE];
> +	/*
> +	 * data response header, captured audio data is copied directly from the
> +	 * DMA buffer

so zero-copy is not supported for now, right?

> +	 */
> +	struct sof_rpmsg_data_resp data_resp;
> +};
> +
> +/* A guest is booting */
> +static int vhost_dsp_activate(struct vhost_dsp *dsp)
> +{
> +	unsigned int i;
> +	int ret = 0;
> +
> +	mutex_lock(&dsp->vrdev.dev.mutex);
> +
> +	/* Wait until all the VirtQueues have been initialised */
> +	if (!dsp->active) {
> +		struct vhost_virtqueue *vq;
> +
> +		for (i = 0, vq = dsp->vrdev.vq;
> +		     i < ARRAY_SIZE(dsp->vrdev.vq);
> +		     i++, vq++) {
> +			/* .private_data is required != NULL */
> +			vq->private_data = vq;
> +			/* needed for re-initialisation upon guest reboot */
> +			ret = vhost_vq_init_access(vq);
> +			if (ret)
> +				vq_err(vq,
> +				       "%s(): error %d initialising vq #%d\n",
> +				       __func__, ret, i);
> +		}
> +
> +		/* Send an RPMsg namespace announcement */
> +		if (!ret && !vhost_rpmsg_ns_announce(&dsp->vrdev, "sof_rpmsg",
> +						     SOF_RPMSG_ADDR_IPC))
> +			dsp->active = true;
> +	}
> +
> +	mutex_unlock(&dsp->vrdev.dev.mutex);
> +
> +	return ret;
> +}
> +
> +/* A guest is powered off or reset */
> +static void vhost_dsp_deactivate(struct vhost_dsp *dsp)
> +{
> +	unsigned int i;
> +
> +	mutex_lock(&dsp->vrdev.dev.mutex);
> +
> +	if (dsp->active) {
> +		struct vhost_virtqueue *vq;
> +
> +		dsp->active = false;

can you explain why this is not symmetrical with _activate above: there 
is no rmpsg communication here?

> +
> +		/* If a VM reboots sof_vhost_client_release() isn't called */
> +		sof_vhost_topology_purge(dsp->snd);
> +
> +		/* signal, that we're inactive */
> +		for (i = 0, vq = dsp->vrdev.vq;
> +		     i < ARRAY_SIZE(dsp->vrdev.vq);
> +		     i++, vq++) {
> +			mutex_lock(&vq->mutex);
> +			vq->private_data = NULL;
> +			mutex_unlock(&vq->mutex);
> +		}
> +	}
> +
> +	mutex_unlock(&dsp->vrdev.dev.mutex);
> +}
> +

[...]

> +/* .ioctl(): we only use VHOST_SET_RUNNING in a non-default way */
> +static long vhost_dsp_ioctl(struct file *filp, unsigned int ioctl,
> +			    unsigned long arg)
> +{
> +	struct vhost_dsp *dsp = filp->private_data;
> +	void __user *argp = (void __user *)arg;
> +	struct vhost_adsp_topology tplg;
> +	u64 __user *featurep = argp;
> +	u64 features;
> +	int start;
> +	long ret;
> +
> +	switch (ioctl) {
> +	case VHOST_GET_FEATURES:
> +		features = VHOST_DSP_FEATURES;
> +		if (copy_to_user(featurep, &features, sizeof(features)))
> +			return -EFAULT;
> +		return 0;
> +	case VHOST_SET_FEATURES:
> +		if (copy_from_user(&features, featurep, sizeof(features)))
> +			return -EFAULT;
> +		return vhost_dsp_set_features(dsp, features);
> +	case VHOST_GET_BACKEND_FEATURES:
> +		features = 0;
> +		if (copy_to_user(featurep, &features, sizeof(features)))
> +			return -EFAULT;
> +		return 0;
> +	case VHOST_SET_BACKEND_FEATURES:
> +		if (copy_from_user(&features, featurep, sizeof(features)))
> +			return -EFAULT;
> +		if (features)
> +			return -EOPNOTSUPP;
> +		return 0;
> +	case VHOST_RESET_OWNER:
> +		mutex_lock(&dsp->vrdev.dev.mutex);
> +		ret = vhost_dev_check_owner(&dsp->vrdev.dev);
> +		if (!ret) {
> +			struct vhost_umem *umem =
> +				vhost_dev_reset_owner_prepare();
> +			if (!umem) {
> +				ret = -ENOMEM;
> +			} else {
> +				vhost_dev_stop(&dsp->vrdev.dev);
> +				vhost_dev_reset_owner(&dsp->vrdev.dev, umem);
> +			}
> +		}
> +		mutex_unlock(&dsp->vrdev.dev.mutex);
> +		return ret;
> +	case VHOST_SET_OWNER:
> +		mutex_lock(&dsp->vrdev.dev.mutex);
> +		ret = vhost_dev_set_owner(&dsp->vrdev.dev);
> +		mutex_unlock(&dsp->vrdev.dev.mutex);
> +		return ret;
> +	case VHOST_SET_RUNNING:
> +		if (copy_from_user(&start, argp, sizeof(start)))
> +			return -EFAULT;
> +
> +		if (start)
> +			return vhost_dsp_activate(dsp);
> +
> +		vhost_dsp_deactivate(dsp);
> +		return 0;
> +	case VHOST_ADSP_SET_GUEST_TPLG:
> +		if (copy_from_user(&tplg, argp, sizeof(tplg)))
> +			return -EFAULT;
> +		return sof_vhost_set_tplg(dsp->snd, &tplg);
> +	}

All cases seem to use return, so the the code below seems to be the 
default: case?

> +	mutex_lock(&dsp->vrdev.dev.mutex);
> +	ret = vhost_dev_ioctl(&dsp->vrdev.dev, ioctl, argp);
> +	if (ret == -ENOIOCTLCMD)
> +		ret = vhost_vring_ioctl(&dsp->vrdev.dev, ioctl, argp);
> +	mutex_unlock(&dsp->vrdev.dev.mutex);
> +
> +	return ret;
> +}

[...]

> +static ssize_t vhost_dsp_ipc_write(struct vhost_rpmsg *vr,
> +				   struct vhost_rpmsg_iter *iter)
> +{
> +	struct vhost_dsp *dsp = container_of(vr, struct vhost_dsp, vrdev);
> +
> +	return vhost_rpmsg_copy(vr, iter, dsp->reply_buf,
> +				vhost_rpmsg_iter_len(iter)) ==
> +		vhost_rpmsg_iter_len(iter) ? 0 : -EIO;
> +}

This is rather convoluted code, with the same function called on both 
sides of a comparison.

> +
> +/* Called only once to get guest's position update endpoint address */
> +static ssize_t vhost_dsp_posn_read(struct vhost_rpmsg *vr,
> +				   struct vhost_rpmsg_iter *iter)
> +{
> +	struct vhost_dsp *dsp = container_of(vr, struct vhost_dsp, vrdev);
> +	struct vhost_virtqueue *vq = &dsp->vrdev.vq[VIRTIO_RPMSG_REQUEST];
> +	size_t len = vhost_rpmsg_iter_len(iter);
> +	size_t nbytes;
> +
> +	if ((int)dsp->posn_addr >= 0) {

posn_addr is defined as a u32, so what are you trying to test here?

> +		vq_err(vq, "%s(): position queue address %u already set\n",
> +		       __func__, dsp->posn_addr);
> +		return -EINVAL;
> +	}
> +
> +	if (len != sizeof(dsp->posn_addr)) {

that also seems suspicious:

+	/* RPMsg address of the position update endpoint */
+	u32 posn_addr;

why would a length which should have different values have anything to 
do with a constant 4 bytes?

> +		vq_err(vq, "%s(): data count %zu invalid\n",
> +		       __func__, len);
> +		return -EINVAL;
> +	}
> +
> +	nbytes = vhost_rpmsg_copy(vr, iter, &dsp->posn_addr,
> +				  sizeof(dsp->posn_addr));
> +	if (nbytes != sizeof(dsp->posn_addr)) {

and again here?

> +		vq_err(vq,
> +		       "%s(): got %zu instead of %zu bytes position update\n",
> +		       __func__, nbytes, sizeof(dsp->posn_addr));
> +		return -EIO;
> +	}
> +
> +	pr_debug("%s(): guest position endpoint address 0x%x\n", __func__,
> +		 dsp->posn_addr);
> +
> +	return 0;
> +}
> +

[...]

> +static int vhost_dsp_open(struct inode *inode, struct file *filp)
> +{
> +	struct miscdevice *misc = filp->private_data;
> +	struct snd_sof_dev *sdev = dev_get_drvdata(misc->parent);
> +	struct vhost_dsp *dsp = kmalloc(sizeof(*dsp), GFP_KERNEL);
> +
> +	if (!dsp)
> +		return -ENOMEM;
> +
> +	dsp->vrdev.dev.mm = NULL;
> +	dsp->snd = sof_vhost_client_add(sdev, dsp);
> +	if (!dsp->snd) {
> +		kfree(dsp);
> +		return -ENOMEM;
> +	}
> +
> +	/*
> +	 * TODO: do we ever want to support multiple guest machines per DSP, if

That is a basic assumption yes.

> +	 * not, we might as well perform all allocations when registering the
> +	 * misc device.
> +	 */
> +	dsp->active = false;
> +	dsp->posn_addr = -EINVAL;
> +	dsp->posn.rhdr.error = -ENODATA;
> +
> +	vhost_rpmsg_init(&dsp->vrdev, vhost_dsp_ept, ARRAY_SIZE(vhost_dsp_ept));
> +	vhost_work_init(&dsp->posn_work, vhost_dsp_send_posn);
> +
> +	/* Overwrite file private data */
> +	filp->private_data = dsp;
> +
> +	return 0;
> +}

[...]

> +/* Always called from an interrupt thread context */
> +static int vhost_dsp_update_posn(struct vhost_dsp *dsp,
> +				 struct sof_ipc_stream_posn *posn)
> +{
> +	struct vhost_virtqueue *vq = &dsp->vrdev.vq[VIRTIO_RPMSG_RESPONSE];
> +	int ret;
> +
> +	if (!dsp->active)
> +		return 0;

is there a case where you can get a position update without the dsp 
being active?
And shouldn't this be an error?
> +
> +	memcpy(&dsp->posn, posn, sizeof(dsp->posn));
> +
> +	mutex_lock(&vq->mutex);
> +
> +	/*
> +	 * VirtQueues can only be processed in the context of the VMM process or
> +	 * a vhost work queue
> +	 */
> +	vhost_work_queue(&dsp->vrdev.dev, &dsp->posn_work);
> +
> +	mutex_unlock(&vq->mutex);
> +
> +	return ret;