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