On Fri, Jan 10, 2025 at 6:09 PM Jonah Palmer <jonah.pal...@oracle.com> wrote:
>
> Propagates the GPAs (in_xlat_addr/out_xlat_addr) of a VirtQueueElement
> to vhost_svq_translate_addr() to translate to IOVAs via GPA->IOVA tree
> when descriptors are backed by guest memory.
>
> For descriptors backed by guest memory, the translation is performed
> using GPAs via the GPA->IOVA tree. GPAs are unique in the guest's
> address space, so this ensures unambiguous IOVA translations.
>
> For descriptors not backed by guest memory, the existing IOVA->HVA tree
> is used.
>
> This avoids the issue where different GPAs map to the same HVA, causing
> the HVA->IOVA translation to potentially return an IOVA associated with
> the wrong intended GPA.
>
If SVQ is the only one needing xlat_addrs we can create a new
SVQElement, following the code of VirtIOBlockReq or VirtIOSCSIReq for
example.
But why do we need it? As long as svq->desc_state[qemu_head].elem !=
NULL the GPA is elem->in_addr or elem->out_addr, and we can use that
to look into the GPA tree, isn't it? If we don't have any elem, then
we need to go with "old" HVA -> IOVA lookup.
> Signed-off-by: Jonah Palmer <jonah.pal...@oracle.com>
> ---
> hw/virtio/vhost-shadow-virtqueue.c | 49 ++++++++++++++++++++++--------
> 1 file changed, 37 insertions(+), 12 deletions(-)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c
> b/hw/virtio/vhost-shadow-virtqueue.c
> index 37aca8b431..be0db94ab7 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -78,24 +78,37 @@ uint16_t vhost_svq_available_slots(const
> VhostShadowVirtqueue *svq)
> * @vaddr: Translated IOVA addresses
> * @iovec: Source qemu's VA addresses
> * @num: Length of iovec and minimum length of vaddr
> + * @gpas: Descriptors' GPAs, if backed by guest memory
> */
> static bool vhost_svq_translate_addr(const VhostShadowVirtqueue *svq,
> hwaddr *addrs, const struct iovec
> *iovec,
> - size_t num)
> + size_t num, const hwaddr *gpas)
> {
> if (num == 0) {
> return true;
> }
>
> for (size_t i = 0; i < num; ++i) {
> - DMAMap needle = {
> - .translated_addr = (hwaddr)(uintptr_t)iovec[i].iov_base,
> - .size = iovec[i].iov_len,
> - };
> + const DMAMap *map;
> + DMAMap needle;
> Int128 needle_last, map_last;
> size_t off;
>
> - const DMAMap *map = vhost_iova_tree_find_iova(svq->iova_tree,
> &needle);
> + if (gpas) {
> + /* Search the GPA->IOVA tree */
> + needle = (DMAMap) {
> + .translated_addr = gpas[i],
> + .size = iovec[i].iov_len,
> + };
> + map = vhost_iova_tree_find_gpa(svq->iova_tree, &needle);
> + } else {
> + /* Searh the IOVA->HVA tree */
> + needle = (DMAMap) {
> + .translated_addr = (hwaddr)(uintptr_t)iovec[i].iov_base,
> + .size = iovec[i].iov_len,
> + };
> + map = vhost_iova_tree_find_iova(svq->iova_tree, &needle);
> + }
> /*
> * Map cannot be NULL since iova map contains all guest space and
> * qemu already has a physical address mapped
> @@ -132,12 +145,14 @@ static bool vhost_svq_translate_addr(const
> VhostShadowVirtqueue *svq,
> * @num: iovec length
> * @more_descs: True if more descriptors come in the chain
> * @write: True if they are writeable descriptors
> + * @gpas: Descriptors' GPAs, if backed by guest memory
> *
> * Return true if success, false otherwise and print error.
> */
> static bool vhost_svq_vring_write_descs(VhostShadowVirtqueue *svq, hwaddr
> *sg,
> const struct iovec *iovec, size_t
> num,
> - bool more_descs, bool write)
> + bool more_descs, bool write,
> + const hwaddr *gpas)
> {
> uint16_t i = svq->free_head, last = svq->free_head;
> unsigned n;
> @@ -149,7 +164,7 @@ static bool
> vhost_svq_vring_write_descs(VhostShadowVirtqueue *svq, hwaddr *sg,
> return true;
> }
>
> - ok = vhost_svq_translate_addr(svq, sg, iovec, num);
> + ok = vhost_svq_translate_addr(svq, sg, iovec, num, gpas);
> if (unlikely(!ok)) {
> return false;
> }
> @@ -175,7 +190,8 @@ static bool
> vhost_svq_vring_write_descs(VhostShadowVirtqueue *svq, hwaddr *sg,
> static bool vhost_svq_add_split(VhostShadowVirtqueue *svq,
> const struct iovec *out_sg, size_t out_num,
> const struct iovec *in_sg, size_t in_num,
> - unsigned *head)
> + unsigned *head, const hwaddr *in_gpas,
> + const hwaddr *out_gpas)
> {
> unsigned avail_idx;
> vring_avail_t *avail = svq->vring.avail;
> @@ -192,12 +208,13 @@ static bool vhost_svq_add_split(VhostShadowVirtqueue
> *svq,
> }
>
> ok = vhost_svq_vring_write_descs(svq, sgs, out_sg, out_num, in_num > 0,
> - false);
> + false, out_gpas);
> if (unlikely(!ok)) {
> return false;
> }
>
> - ok = vhost_svq_vring_write_descs(svq, sgs, in_sg, in_num, false, true);
> + ok = vhost_svq_vring_write_descs(svq, sgs, in_sg, in_num, false, true,
> + in_gpas);
> if (unlikely(!ok)) {
> return false;
> }
> @@ -253,12 +270,20 @@ int vhost_svq_add(VhostShadowVirtqueue *svq, const
> struct iovec *out_sg,
> unsigned qemu_head;
> unsigned ndescs = in_num + out_num;
> bool ok;
> + hwaddr *in_gpas = NULL;
> + hwaddr *out_gpas = NULL;
>
> if (unlikely(ndescs > vhost_svq_available_slots(svq))) {
> return -ENOSPC;
> }
>
> - ok = vhost_svq_add_split(svq, out_sg, out_num, in_sg, in_num,
> &qemu_head);
> + if (elem) {
> + in_gpas = elem->in_xlat_addr;
> + out_gpas = elem->out_xlat_addr;
> + }
> +
> + ok = vhost_svq_add_split(svq, out_sg, out_num, in_sg, in_num, &qemu_head,
> + in_gpas, out_gpas);
> if (unlikely(!ok)) {
> return -EINVAL;
> }
> --
> 2.43.5
>
