On 13.10.25 23:13, Raphael Norwitz wrote:
> Overall looks good just once comment.
>
> On Sat, Oct 11, 2025 at 7:24 PM Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> We miss call to unmap in cases when vhost_memory_map() returns
>> lenght less than requested (still we consider such cases as an
>> error). Let's fix it in vhost_memory_map().
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>> hw/virtio/vhost.c | 35 ++++++++++++++++++++++-------------
>> 1 file changed, 22 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index 773b91c0a0..8031c74e7b 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -453,11 +453,20 @@ static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
>> }
>>
>> static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
>> - hwaddr *plen, bool is_write)
>> + hwaddr len, bool is_write)
>> {
>> if (!vhost_dev_has_iommu(dev)) {
>> - return address_space_map(dev->vdev->dma_as, addr, plen, is_write,
>> - MEMTXATTRS_UNSPECIFIED);
>> + hwaddr mapped_len = len;
>> + void *res = address_space_map(dev->vdev->dma_as, addr, &mapped_len,
>> + is_write, MEMTXATTRS_UNSPECIFIED);
>> + if (!res) {
>> + return NULL;
>> + }
>> + if (len != mapped_len) {
>
> Should this be:
>
> address_space_unmap(dev->vdev->dma_as, res, mapped_len, is_write,
> MEMTXATTRS_UNSPECIFIED);
>
> rather than address_space_unmap(...0,0)?
Pre-patch we do "...0,0)" for already mapped memories.
Moreover, following commit above address_space_unmap() definition,
is_write and access_len are here to mark the memory as dirty.
In our case, that's an early fail, and we don't actually access the memory,
so it's correct to set is_write to zero, I think.
The latter 0 is also correct, as it's just unused when is_write==0.
>
>> + address_space_unmap(dev->vdev->dma_as, res, mapped_len, 0, 0);
>> + return NULL;
>> + }
>> + return res;
>> } else {
>> return (void *)(uintptr_t)addr;
>> }
>> @@ -1261,7 +1270,7 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
>> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
>> VirtioBusState *vbus = VIRTIO_BUS(qbus);
>> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
>> - hwaddr s, l, a;
>> + hwaddr l, a;
>> int r;
>> int vhost_vq_index = dev->vhost_ops->vhost_get_vq_index(dev, idx);
>> struct vhost_vring_file file = {
>> @@ -1301,24 +1310,24 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
>> }
>> }
>>
>> - vq->desc_size = s = l = virtio_queue_get_desc_size(vdev, idx);
>> + vq->desc_size = l = virtio_queue_get_desc_size(vdev, idx);
>> vq->desc_phys = a;
>> - vq->desc = vhost_memory_map(dev, a, &l, false);
>> - if (!vq->desc || l != s) {
>> + vq->desc = vhost_memory_map(dev, a, l, false);
>> + if (!vq->desc) {
>> r = -ENOMEM;
>> goto fail_alloc_desc;
>> }
>> - vq->avail_size = s = l = virtio_queue_get_avail_size(vdev, idx);
>> + vq->avail_size = l = virtio_queue_get_avail_size(vdev, idx);
>> vq->avail_phys = a = virtio_queue_get_avail_addr(vdev, idx);
>> - vq->avail = vhost_memory_map(dev, a, &l, false);
>> - if (!vq->avail || l != s) {
>> + vq->avail = vhost_memory_map(dev, a, l, false);
>> + if (!vq->avail) {
>> r = -ENOMEM;
>> goto fail_alloc_avail;
>> }
>> - vq->used_size = s = l = virtio_queue_get_used_size(vdev, idx);
>> + vq->used_size = l = virtio_queue_get_used_size(vdev, idx);
>> vq->used_phys = a = virtio_queue_get_used_addr(vdev, idx);
>> - vq->used = vhost_memory_map(dev, a, &l, true);
>> - if (!vq->used || l != s) {
>> + vq->used = vhost_memory_map(dev, a, l, true);
>> + if (!vq->used) {
>> r = -ENOMEM;
>> goto fail_alloc_used;
>> }
>> --
>> 2.48.1
>>
--
Best regards,
Vladimir