Virtualization - Jun 2021 - [RFC v3 25/29] vhost: Add custom IOTLB translations to SVQ

? 2021/6/3 ??1:51, Eugenio Perez Martin ??:
> On Wed, Jun 2, 2021 at 11:52 AM Jason Wang <jasowang at 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 at 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.

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