Re: [RFC v3 5/5] svq: Support translations via GPAs in vhost_svq_translate_addr

[Eugenio Perez Martin]

On Fri, Jan 10, 2025 at 6:09 PM Jonah Palmer <jonah.palmer@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.palmer@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
>