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

+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;