Hi Gerd,
On 5/6/21 11:10 AM, Gerd Hoffmann wrote:
> dma_memory_map() may map only a part of the request. Happens if the
> request can't be mapped in one go, for example due to a iommu creating
> a linear dma mapping for scattered physical pages. Should that be the
> case virtio-gpu must call dma_memory_map() again with the remaining
> range instead of simply throwing an error.
>
> Note that this change implies the number of iov entries may differ from
> the number of mapping entries sent by the guest. Therefore the iov_len
> bookkeeping needs some updates too, we have to explicitly pass around
> the iov length now.
>
> Reported-by: Auger Eric <eric.au...@redhat.com>
> Signed-off-by: Gerd Hoffmann <kra...@redhat.com>
> ---
> include/hw/virtio/virtio-gpu.h | 3 +-
> hw/display/virtio-gpu-3d.c | 7 ++--
> hw/display/virtio-gpu.c | 75 ++++++++++++++++++++--------------
> 3 files changed, 51 insertions(+), 34 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index fae149235c58..0d15af41d96d 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -209,7 +209,8 @@ void virtio_gpu_get_edid(VirtIOGPU *g,
> int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
> struct virtio_gpu_resource_attach_backing
> *ab,
> struct virtio_gpu_ctrl_command *cmd,
> - uint64_t **addr, struct iovec **iov);
> + uint64_t **addr, struct iovec **iov,
> + uint32_t *niov);
> void virtio_gpu_cleanup_mapping_iov(VirtIOGPU *g,
> struct iovec *iov, uint32_t count);
> void virtio_gpu_process_cmdq(VirtIOGPU *g);
> diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c
> index d98964858e13..72c14d91324b 100644
> --- a/hw/display/virtio-gpu-3d.c
> +++ b/hw/display/virtio-gpu-3d.c
> @@ -283,22 +283,23 @@ static void virgl_resource_attach_backing(VirtIOGPU *g,
> {
> struct virtio_gpu_resource_attach_backing att_rb;
> struct iovec *res_iovs;
> + uint32_t res_niov;
> int ret;
>
> VIRTIO_GPU_FILL_CMD(att_rb);
> trace_virtio_gpu_cmd_res_back_attach(att_rb.resource_id);
>
> - ret = virtio_gpu_create_mapping_iov(g, &att_rb, cmd, NULL, &res_iovs);
> + ret = virtio_gpu_create_mapping_iov(g, &att_rb, cmd, NULL, &res_iovs,
> &res_niov);
> if (ret != 0) {
> cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> return;
> }
>
> ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
> - res_iovs, att_rb.nr_entries);
> + res_iovs, res_niov);
>
> if (ret != 0)
> - virtio_gpu_cleanup_mapping_iov(g, res_iovs, att_rb.nr_entries);
> + virtio_gpu_cleanup_mapping_iov(g, res_iovs, res_niov);
> }
>
> static void virgl_resource_detach_backing(VirtIOGPU *g,
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index c9f5e36fd076..1dd3648f32a3 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -608,11 +608,12 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,
> int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
> struct virtio_gpu_resource_attach_backing
> *ab,
> struct virtio_gpu_ctrl_command *cmd,
> - uint64_t **addr, struct iovec **iov)
> + uint64_t **addr, struct iovec **iov,
> + uint32_t *niov)
> {
> struct virtio_gpu_mem_entry *ents;
> size_t esize, s;
> - int i;
> + int e, v;
>
> if (ab->nr_entries > 16384) {
> qemu_log_mask(LOG_GUEST_ERROR,
> @@ -633,37 +634,53 @@ int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
> return -1;
> }
>
> - *iov = g_malloc0(sizeof(struct iovec) * ab->nr_entries);
> + *iov = NULL;
> if (addr) {
> - *addr = g_malloc0(sizeof(uint64_t) * ab->nr_entries);
> + *addr = NULL;
> }
> - for (i = 0; i < ab->nr_entries; i++) {
> - uint64_t a = le64_to_cpu(ents[i].addr);
> - uint32_t l = le32_to_cpu(ents[i].length);
> - hwaddr len = l;
> - (*iov)[i].iov_base = dma_memory_map(VIRTIO_DEVICE(g)->dma_as,
> - a, &len,
> DMA_DIRECTION_TO_DEVICE);
> - (*iov)[i].iov_len = len;
> - if (addr) {
> - (*addr)[i] = a;
> - }
> - if (!(*iov)[i].iov_base || len != l) {
> - qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map MMIO memory
> for"
> - " resource %d element %d\n",
> - __func__, ab->resource_id, i);
> - if ((*iov)[i].iov_base) {
> - i++; /* cleanup the 'i'th map */
> + for (e = 0, v = 0; e < ab->nr_entries; e++) {
> + uint64_t a = le64_to_cpu(ents[e].addr);
> + uint32_t l = le32_to_cpu(ents[e].length);
> + hwaddr len;
> + void *map;
> +
> + do {
> + len = l;
> + map = dma_memory_map(VIRTIO_DEVICE(g)->dma_as,
> + a, &len, DMA_DIRECTION_TO_DEVICE);
> + if (!map) {
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map MMIO
> memory for"
> + " resource %d element %d\n",
> + __func__, ab->resource_id, e);
> + virtio_gpu_cleanup_mapping_iov(g, *iov, v);
> + g_free(ents);
> + *iov = NULL;
> + if (addr) {
> + g_free(*addr);
> + *addr = NULL;
> + }
> + return -1;
> }
> - virtio_gpu_cleanup_mapping_iov(g, *iov, i);
> - g_free(ents);
> - *iov = NULL;
> +
> + if (!(v % 16)) {
> + *iov = g_realloc(*iov, sizeof(struct iovec) * (v + 16));
> + if (addr) {
> + *addr = g_realloc(*addr, sizeof(uint64_t) * (v + 16));
nit: just wondering why you chose to do the alloc by slice of 16 instead
of doing the usual allocation at the beginning and re-allocating the iov
when l != len. Do you think the perf will be better with this
implementation? Looks the guest does concatenation quite often so most
probably your implementation is best.
Reviewed-by: Eric Auger <eric.au...@redhat.com>
Tested-by: Eric Auger <eric.au...@redhat.com>
Thanks!
Eric
> + }
> + }
> + (*iov)[v].iov_base = map;
> + (*iov)[v].iov_len = len;
> if (addr) {
> - g_free(*addr);
> - *addr = NULL;
> + (*addr)[v] = a;
> }
> - return -1;
> - }
> +
> + a += len;
> + l -= len;
> + v += 1;
> + } while (l > 0);
> }
> + *niov = v;
> +
> g_free(ents);
> return 0;
> }
> @@ -717,13 +734,11 @@ virtio_gpu_resource_attach_backing(VirtIOGPU *g,
> return;
> }
>
> - ret = virtio_gpu_create_mapping_iov(g, &ab, cmd, &res->addrs, &res->iov);
> + ret = virtio_gpu_create_mapping_iov(g, &ab, cmd, &res->addrs, &res->iov,
> &res->iov_cnt);
> if (ret != 0) {
> cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> return;
> }
> -
> - res->iov_cnt = ab.nr_entries;
> }
>
> static void
>
