Re: [RFC v3 25/29] vhost: Add custom IOTLB translations to SVQ

[Eugenio Perez Martin <eperezma@redhat.com>]

On Thu, Jun 3, 2021 at 5:39 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2021/6/3 上午1:51, Eugenio Perez Martin 写道:
> > On Wed, Jun 2, 2021 at 11:52 AM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> 在 2021/5/20 上午12:28, Eugenio Pérez 写道:
> >>> Use translations added in IOVAReverseMaps in SVQ if the vhost device
> >>> does not support the mapping of the full qemu's virtual address space.
> >>> In other cases, Shadow Virtqueue still uses the qemu's virtual address
> >>> of the buffer pointed by the descriptor, which has been translated
> >>> already by qemu's VirtQueue machinery.
> >>
> >> I'd say let stick to a single kind of translation (iova allocator) that
> >> works for all the cases first and add optimizations on top.
> >>
> > Ok, I will start from here for the next revision.
> >
> >>> Now every element needs to store the previous address also, so VirtQueue
> >>> can consume the elements properly. This adds a little overhead per VQ
> >>> element, having to allocate more memory to stash them. As a possible
> >>> optimization, this allocation could be avoided if the descriptor is not
> >>> a chain but a single one, but this is left undone.
> >>>
> >>> Checking also for vhost_set_iotlb_callback to send used ring remapping.
> >>> This is only needed for kernel, and would print an error in case of
> >>> vhost devices with its own mapping (vdpa).
> >>>
> >>> This could change for other callback, like checking for
> >>> vhost_force_iommu, enable_custom_iommu, or another. Another option could
> >>> be to, at least, extract the check of "is map(used, writable) needed?"
> >>> in another function. But at the moment just copy the check used in
> >>> vhost_dev_start here.
> >>>
> >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>> ---
> >>>    hw/virtio/vhost-shadow-virtqueue.c | 134 ++++++++++++++++++++++++++---
> >>>    hw/virtio/vhost.c                  |  29 +++++--
> >>>    2 files changed, 145 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> >>> index 934d3bb27b..a92da979d1 100644
> >>> --- a/hw/virtio/vhost-shadow-virtqueue.c
> >>> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> >>> @@ -10,12 +10,19 @@
> >>>    #include "hw/virtio/vhost-shadow-virtqueue.h"
> >>>    #include "hw/virtio/vhost.h"
> >>>    #include "hw/virtio/virtio-access.h"
> >>> +#include "hw/virtio/vhost-iova-tree.h"
> >>>
> >>>    #include "standard-headers/linux/vhost_types.h"
> >>>
> >>>    #include "qemu/error-report.h"
> >>>    #include "qemu/main-loop.h"
> >>>
> >>> +typedef struct SVQElement {
> >>> +    VirtQueueElement elem;
> >>> +    void **in_sg_stash;
> >>> +    void **out_sg_stash;
> >>
> >> Any reason for the trick like this?
> >>
> >> Can we simply use iovec and iov_copy() here?
> >>
> > At the moment the device writes the buffer directly to the guest's
> > memory, and SVQ only translates the descriptor. In this scenario,
> > there would be no need for iov_copy, isn't it?
>
>
> It depends on which kinds of translation you used.
>
> If I read the code correctly, stash is used for storing HVAs after the
> HVA->IOVA translation.
>
> This looks exactly the work of iov (and do we guarantee the there will
> be a 1:1 translation?)
>
> And if the mapping is 1:1 you can simply use iov_copy().
>
> But this wont' be a option if we will always use iova allocator.
>

Right, the stash is only used in case of iova allocator. In case of
1:1 translation, svq->iova_map is never !NULL and _stash/_unstash
functions are never called.

And yes, I could have used iov_copy [1], but the check of overlapping
would have been unnecessary. It was like using memmove vs memset in my
head.

Thanks!

[1] I thought you meant iov_to_buf in your last mail, so please omit
the part of the buffer copy in my answer :).

>
> >
> > The reason for stash and unstash them was to allow the 1:1 mapping
> > with qemu memory and IOMMU and iova allocator to work with less
> > changes, In particular, the reason for unstash is that virtqueue_fill,
> > expects qemu pointers to set the guest memory page as dirty in
> > virtqueue_unmap_sg->dma_memory_unmap.
> >
> > Now I think that just storing the iova address from the allocator in a
> > separated field and using a wrapper to get the IOVA addresses in SVQ
> > would be a better idea, so I would change to this if everyone agrees.
>
>
> I agree.
>
> Thanks
>
>
> >
> > Thanks!
> >
> >> Thanks
> >>
> >>
>