> -----Original Message-----
> From: Eugenio Perez Martin <[email protected]>
> Sent: 2025年12月19日 16:12
> To: Wafer <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Angus Chen
> <[email protected]>
> Subject: Re: [PATCH v3 2/4] vdpa: implement a statically allocated buffer for
> SVQ
>
> External Mail: This email originated from OUTSIDE of the organization!
> Do not click links, open attachments or provide ANY information unless you
> recognize the sender and know the content is safe.
>
>
> On Tue, Dec 16, 2025 at 2:55 AM Wafer Xie <[email protected]> wrote:
> >
> > From: wafer Xie <[email protected]>
> >
> > allocated and initialized when creating the vhost-vdpa device, and
> > release the indirect buffer when vhost-vdpa is stopped.
> >
> > Signed-off-by: wafer Xie <[email protected]>
> > ---
> > hw/virtio/vhost-shadow-virtqueue.c | 25 +++++
> > hw/virtio/vhost-vdpa.c | 163 ++++++++++++++++++++++++++++-
> > 2 files changed, 187 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.c
> > b/hw/virtio/vhost-shadow-virtqueue.c
> > index 2481d49345..85eff1d841 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -708,6 +708,25 @@ void vhost_svq_start(VhostShadowVirtqueue
> *svq, VirtIODevice *vdev,
> > for (unsigned i = 0; i < svq->vring.num - 1; i++) {
> > svq->desc_next[i] = i + 1;
> > }
> > +
> > + /* Initialize indirect descriptor state */
> > + svq->indirect_enabled = false;
> > + svq->current_indirect_buf = -1;
> > + for (int i = 0; i < SVQ_NUM_INDIRECT_BUFS; i++) {
> > + svq->indirect_bufs[i].desc = NULL;
> > + svq->indirect_bufs[i].iova = 0;
> > + svq->indirect_bufs[i].size = 0;
> > + svq->indirect_bufs[i].state = SVQ_INDIRECT_BUF_FREED;
> > + svq->indirect_bufs[i].num_descs = 0;
> > + svq->indirect_bufs[i].freed_descs = 0;
> > + svq->indirect_bufs[i].freeing_descs = 0;
> > + svq->indirect_bufs[i].freed_head = 0;
> > + }
> > +
> > + /* Initialize desc_state indirect_buf_idx to -1 */
> > + for (unsigned i = 0; i < svq->vring.num; i++) {
> > + svq->desc_state[i].indirect_buf_idx = -1;
> > + }
> > }
> >
> > /**
> > @@ -748,6 +767,10 @@ void vhost_svq_stop(VhostShadowVirtqueue *svq)
> > munmap(svq->vring.desc, vhost_svq_driver_area_size(svq));
> > munmap(svq->vring.used, vhost_svq_device_area_size(svq));
> > event_notifier_set_handler(&svq->hdev_call, NULL);
> > +
> > + /* Reset indirect descriptor state (memory is freed by vhost-vdpa) */
> > + svq->indirect_enabled = false;
> > + svq->current_indirect_buf = -1;
> > }
> >
> > /**
> > @@ -765,6 +788,8 @@ VhostShadowVirtqueue *vhost_svq_new(const
> VhostShadowVirtqueueOps *ops,
> > event_notifier_init_fd(&svq->svq_kick, VHOST_FILE_UNBIND);
> > svq->ops = ops;
> > svq->ops_opaque = ops_opaque;
> > + svq->indirect_enabled = false;
> > + svq->current_indirect_buf = -1;
> > return svq;
> > }
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index
> > 7061b6e1a3..816868d153 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -584,6 +584,155 @@ static int vhost_vdpa_get_dev_features(struct
> vhost_dev *dev,
> > return ret;
> > }
> >
> > +/**
> > + * Allocate a single indirect descriptor buffer for SVQ
> > + *
> > + * @v: vhost_vdpa instance
> > + * @num_descs: Number of descriptors (should be vring.num)
> > + * @buf: Output buffer structure to fill
> > + *
> > + * Returns true on success, false on failure.
> > + */
> > +static bool vhost_vdpa_alloc_indirect_buf(struct vhost_vdpa *v,
> > + uint16_t num_descs,
> > + SVQIndirectDescBuf *buf) {
> > + size_t desc_size = sizeof(vring_desc_t) * num_descs;
> > + size_t alloc_size = ROUND_UP(desc_size,
> qemu_real_host_page_size());
> > + DMAMap needle = {
> > + .size = alloc_size - 1,
> > + .perm = IOMMU_RO,
> > + };
> > + vring_desc_t *indirect_desc;
> > + int r;
> > +
> > + /* Allocate memory for indirect descriptors */
> > + indirect_desc = mmap(NULL, alloc_size, PROT_READ | PROT_WRITE,
> > + MAP_SHARED | MAP_ANONYMOUS, -1, 0);
> > + if (indirect_desc == MAP_FAILED) {
> > + error_report("Cannot allocate indirect descriptor buffer");
> > + return false;
> > + }
> > +
> > + /* Allocate IOVA for the indirect descriptor buffer */
> > + r = vhost_iova_tree_map_alloc(v->shared->iova_tree, &needle,
> > + (hwaddr)(uintptr_t)indirect_desc);
> > + if (unlikely(r != IOVA_OK)) {
> > + error_report("Cannot allocate iova for indirect descriptors (%d)",
> > r);
> > + munmap(indirect_desc, alloc_size);
> > + return false;
> > + }
> > +
> > + /* Map to device */
> > + r = vhost_vdpa_dma_map(v->shared, v->address_space_id,
> needle.iova,
> > + alloc_size, indirect_desc, true);
> > + if (unlikely(r != 0)) {
> > + error_report("Cannot map indirect descriptors to device: %s (%d)",
> > + g_strerror(-r), -r);
> > + vhost_iova_tree_remove(v->shared->iova_tree, needle);
> > + munmap(indirect_desc, alloc_size);
> > + return false;
> > + }
> > +
> > + buf->desc = indirect_desc;
> > + buf->iova = needle.iova;
> > + buf->size = alloc_size;
> > + buf->state = SVQ_INDIRECT_BUF_FREED;
> > + buf->num_descs = num_descs;
> > + buf->freed_descs = num_descs; /* All descriptors are free initially */
> > + buf->freeing_descs = 0;
> > + buf->freed_head = 0;
> > +
> > + return true;
> > +}
> > +
> > +/**
> > + * Free a single indirect descriptor buffer
> > + *
> > + * @v: vhost_vdpa instance
> > + * @buf: Buffer to free
> > + */
> > +static void vhost_vdpa_free_indirect_buf(struct vhost_vdpa *v,
> > + SVQIndirectDescBuf *buf) {
> > + DMAMap needle;
> > + const DMAMap *result;
> > + int r;
> > +
> > + if (!buf->desc) {
> > + return;
> > + }
> > +
> > + needle = (DMAMap) {
> > + .translated_addr = (uint64_t)(uintptr_t)buf->desc,
> > + };
> > + result = vhost_iova_tree_find_iova(v->shared->iova_tree,
> > + &needle);
> > +
> > + if (result) {
> > + r = vhost_vdpa_dma_unmap(v->shared, v->address_space_id,
> > + result->iova, buf->size);
> > + if (unlikely(r != 0)) {
> > + error_report("Cannot unmap indirect descriptors: %s (%d)",
> > + g_strerror(-r), -r);
> > + }
> > +
> > + vhost_iova_tree_remove(v->shared->iova_tree, *result);
> > + }
> > +
> > + munmap(buf->desc, buf->size);
> > + buf->desc = NULL;
> > + buf->iova = 0;
> > + buf->size = 0;
> > +}
> > +
> > +/**
> > + * Initialize indirect descriptor buffers for a single SVQ
> > + *
> > + * @v: vhost_vdpa instance
> > + * @svq: Shadow virtqueue to initialize
> > + *
> > + * Returns true on success, false on failure.
> > + */
> > +static bool vhost_vdpa_svq_init_indirect(struct vhost_vdpa *v,
> > + VhostShadowVirtqueue *svq)
> > +{
> > + if (!svq->vring.num) {
> > + return true;
> > + }
> > +
> > + /* Allocate indirect descriptor buffers for this SVQ */
> > + for (int j = 0; j < SVQ_NUM_INDIRECT_BUFS; j++) {
> > + if (!vhost_vdpa_alloc_indirect_buf(v, svq->vring.num * 4,
> > + &svq->indirect_bufs[j])) {
>
> Why not allocate a single array of size "SVQ_NUM_INDIRECT_BUFS *
> svq->vring.num * 4"? It should help management and maybe even
> performance as memory accesses are more predictable.
>
We cannot assume or rely on the device to populate the used ring in the same
order as the available ring.
With a single large array, this may introduce holes, which makes managing freed
descriptors difficult.
In addition, the virtio specification requires indirect descriptor addresses to
be contiguous, which further complicates management with a single array.
> > + /* Cleanup already allocated buffers */
> > + for (int k = 0; k < j; k++) {
> > + vhost_vdpa_free_indirect_buf(v, &svq->indirect_bufs[k]);
> > + }
> > + return false;
> > + }
> > + }
> > +
> > + svq->indirect_enabled = true;
> > + svq->current_indirect_buf = 0;
> > + return true;
> > +}
> > +
> > +/**
> > + * Cleanup indirect descriptor buffers for a single SVQ
> > + *
> > + * @v: vhost_vdpa instance
> > + * @svq: Shadow virtqueue to cleanup
> > + */
> > +static void vhost_vdpa_svq_cleanup_indirect(struct vhost_vdpa *v,
> > + VhostShadowVirtqueue
> > +*svq) {
> > + for (int j = 0; j < SVQ_NUM_INDIRECT_BUFS; j++) {
> > + vhost_vdpa_free_indirect_buf(v, &svq->indirect_bufs[j]);
> > + }
> > + svq->indirect_enabled = false;
> > + svq->current_indirect_buf = -1;
> > +}
> > +
> > static void vhost_vdpa_init_svq(struct vhost_dev *hdev, struct
> > vhost_vdpa *v) {
> > g_autoptr(GPtrArray) shadow_vqs = NULL; @@ -791,8 +940,11 @@
> > static void vhost_vdpa_svq_cleanup(struct vhost_dev *dev)
> > size_t idx;
> >
> > for (idx = 0; idx < v->shadow_vqs->len; ++idx) {
> > - vhost_svq_stop(g_ptr_array_index(v->shadow_vqs, idx));
> > + VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs,
> idx);
> > + vhost_vdpa_svq_cleanup_indirect(v, svq);
>
> All usages of vhost_vdpa_svq_cleanup_indirect go with vhost_svq_stop.
> We can call vhost_vdpa_svq_cleanup_indirect from vhost_svq_stop or even
> embed it.
>
Thank you for the suggestion. I will modify this in the next version of the
patch.
However, I have one question: in hw/virtio/vhost-vdpa.c:vhost_vdpa_svq_cleanup,
why does it only call vhost_svq_stop(svq) without calling
vhost_vdpa_svq_unmap_rings(dev, svq)?
>
> > + vhost_svq_stop(svq);
> > }
> > +
> > g_ptr_array_free(v->shadow_vqs, true); }
> >
> > @@ -1299,6 +1451,13 @@ static bool vhost_vdpa_svqs_start(struct
> vhost_dev *dev)
> > error_setg_errno(&err, -r, "Cannot set device address");
> > goto err_set_addr;
> > }
> > +
> > + /* Initialize indirect descriptor buffers for this SVQ */
> > + if (!vhost_vdpa_svq_init_indirect(v, svq)) {
> > + /* Non-fatal: will fallback to chain mode */
> > + warn_report("Cannot initialize indirect descriptor for SVQ %u",
> > + virtio_get_queue_index(vq));
> > + }
> > }
> >
> > return true;
> > @@ -1313,6 +1472,7 @@ err:
> > error_reportf_err(err, "Cannot setup SVQ %u: ", i);
> > for (unsigned j = 0; j < i; ++j) {
> > VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs,
> > j);
> > + vhost_vdpa_svq_cleanup_indirect(v, svq);
> > vhost_vdpa_svq_unmap_rings(dev, svq);
> > vhost_svq_stop(svq);
> > }
> > @@ -1331,6 +1491,7 @@ static void vhost_vdpa_svqs_stop(struct
> vhost_dev *dev)
> > for (unsigned i = 0; i < v->shadow_vqs->len; ++i) {
> > VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs,
> > i);
> >
> > + vhost_vdpa_svq_cleanup_indirect(v, svq);
> > vhost_svq_stop(svq);
> > vhost_vdpa_svq_unmap_rings(dev, svq);
> >
> > --
> > 2.48.1
> >
