From 3dbbb2533e45542d926f811a4b6b2eb8f95d4b49 Mon Sep 17 00:00:00 2001 From: Andrew Davis Date: Fri, 15 Mar 2024 17:48:42 -0500 Subject: [PATCH 1/2] virtqueue: Fix comment on shm_io and fix type This should hold a pointer to an metal_io_region, make that the type. Also fix the comment, this region holds the address of the message buffers, not the vring's descriptor table nor available/used rings. It is only used for virt-to-phys/phys-to-vert on the buffers pointed to by these descriptors. This comment seems to have cause an issue in virtio_mmio_drv where this region was used to translate the address of the vring descriptors. This may have worked if the vring descriptors where part of the same IO space as the buffers they point to, but this is not guaranteed to always be the case. Fix that here. Signed-off-by: Andrew Davis --- lib/include/openamp/virtqueue.h | 4 ++-- lib/virtio_mmio/virtio_mmio_drv.c | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/include/openamp/virtqueue.h b/lib/include/openamp/virtqueue.h index 1a9d2e8fe..819439871 100644 --- a/lib/include/openamp/virtqueue.h +++ b/lib/include/openamp/virtqueue.h @@ -114,10 +114,10 @@ struct virtqueue { uint16_t vq_queued_cnt; /** - * Metal I/O region of the vrings and buffers. + * Metal I/O region of the buffers. * This structure is used for conversion between virtual and physical addresses. */ - void *shm_io; + struct metal_io_region *shm_io; /** * Head of the free chain in the descriptor table. If there are no free descriptors, diff --git a/lib/virtio_mmio/virtio_mmio_drv.c b/lib/virtio_mmio/virtio_mmio_drv.c index 5f42180a6..42f93e22f 100644 --- a/lib/virtio_mmio/virtio_mmio_drv.c +++ b/lib/virtio_mmio/virtio_mmio_drv.c @@ -310,7 +310,6 @@ struct virtqueue *virtio_mmio_setup_virtqueue(struct virtio_device *vdev, } vdev->role = role_bk; vq->priv = cb_arg; - virtqueue_set_shmem_io(vq, vmdev->shm_io); /* Writing selection register VIRTIO_MMIO_QUEUE_SEL. In pure AMP * mode this needs to be followed by a synchronization w/ the device @@ -325,7 +324,7 @@ struct virtqueue *virtio_mmio_setup_virtqueue(struct virtio_device *vdev, virtio_mmio_write32(vdev, VIRTIO_MMIO_QUEUE_NUM, vq->vq_nentries); virtio_mmio_write32(vdev, VIRTIO_MMIO_QUEUE_ALIGN, 4096); virtio_mmio_write32(vdev, VIRTIO_MMIO_QUEUE_PFN, - ((uintptr_t)metal_io_virt_to_phys(vq->shm_io, + ((uintptr_t)metal_io_virt_to_phys(vmdev->shm_io, (char *)vq->vq_ring.desc)) / 4096); vdev->vrings_info[vdev->vrings_num].vq = vq; From e05c41a1977730824506a1cc5bf9c19cd3b784e4 Mon Sep 17 00:00:00 2001 From: Andrew Davis Date: Wed, 13 Mar 2024 17:49:57 -0500 Subject: [PATCH 2/2] virtio: Use physical addresses for buffers in virtio Have the higher level rpmsg_virtio handle the virt-to-phys and back conversion. The rpmsg_virtio layer already has the shared buffer memory metal IO. This removes the need to pass in the buffer region metal_io struct down to the virtio layer, which otherwise does nothing with the contents of the buffers. Signed-off-by: Andrew Davis --- lib/include/openamp/virtqueue.h | 13 ++++++------ lib/rpmsg/rpmsg_virtio.c | 31 +++++++++++++++-------------- lib/virtio/virtqueue.c | 35 ++++++++------------------------- 3 files changed, 31 insertions(+), 48 deletions(-) diff --git a/lib/include/openamp/virtqueue.h b/lib/include/openamp/virtqueue.h index 819439871..dcef05712 100644 --- a/lib/include/openamp/virtqueue.h +++ b/lib/include/openamp/virtqueue.h @@ -65,8 +65,8 @@ extern "C" { /** @brief Buffer descriptor. */ struct virtqueue_buf { - /** Address of the buffer. */ - void *buf; + /** Address (guest-physical) */ + uint64_t buf; /** Size of the buffer. */ int len; @@ -285,12 +285,13 @@ void *virtqueue_get_buffer(struct virtqueue *vq, uint32_t *len, uint16_t *idx); * * @param vq Pointer to VirtIO queue control block * @param avail_idx Pointer to index used in vring desc table + * @param buffer Physical address of buffer * @param len Length of buffer * - * @return Pointer to available buffer + * @return Function status */ -void *virtqueue_get_available_buffer(struct virtqueue *vq, uint16_t *avail_idx, - uint32_t *len); +int virtqueue_get_available_buffer(struct virtqueue *vq, uint16_t *avail_idx, + uint64_t *buffer, uint32_t *len); /** * @internal @@ -381,7 +382,7 @@ void virtqueue_notification(struct virtqueue *vq); uint32_t virtqueue_get_desc_size(struct virtqueue *vq); uint32_t virtqueue_get_buffer_length(struct virtqueue *vq, uint16_t idx); -void *virtqueue_get_buffer_addr(struct virtqueue *vq, uint16_t idx); +uint64_t virtqueue_get_buffer_addr(struct virtqueue *vq, uint16_t idx); /** * @brief Test if virtqueue is empty diff --git a/lib/rpmsg/rpmsg_virtio.c b/lib/rpmsg/rpmsg_virtio.c index 1a9d0bfaa..1d3de1ef0 100644 --- a/lib/rpmsg/rpmsg_virtio.c +++ b/lib/rpmsg/rpmsg_virtio.c @@ -120,7 +120,7 @@ static void rpmsg_virtio_return_buffer(struct rpmsg_virtio_device *rvdev, (void)idx; /* Initialize buffer node */ - vqbuf.buf = buffer; + vqbuf.buf = metal_io_virt_to_phys(rvdev->shbuf_io, buffer); vqbuf.len = len; ret = virtqueue_add_buffer(rvdev->rvq, &vqbuf, 0, 1, buffer); RPMSG_ASSERT(ret == VQUEUE_SUCCESS, "add buffer failed\r\n"); @@ -162,7 +162,7 @@ static int rpmsg_virtio_enqueue_buffer(struct rpmsg_virtio_device *rvdev, (void)idx; /* Initialize buffer node */ - vqbuf.buf = buffer; + vqbuf.buf = metal_io_virt_to_phys(rvdev->shbuf_io, buffer); vqbuf.len = len; return virtqueue_add_buffer(rvdev->svq, &vqbuf, 1, 0, buffer); } @@ -224,7 +224,12 @@ static void *rpmsg_virtio_get_tx_buffer(struct rpmsg_virtio_device *rvdev, #endif /*!VIRTIO_DEVICE_ONLY*/ #ifndef VIRTIO_DRIVER_ONLY } else if (role == RPMSG_REMOTE) { - data = virtqueue_get_available_buffer(rvdev->svq, idx, len); + uint64_t phys_data; + int status; + + status = virtqueue_get_available_buffer(rvdev->svq, idx, &phys_data, len); + if (!status) + data = metal_io_phys_to_virt(rvdev->shbuf_io, phys_data); #endif /*!VIRTIO_DRIVER_ONLY*/ } @@ -256,8 +261,12 @@ static void *rpmsg_virtio_get_rx_buffer(struct rpmsg_virtio_device *rvdev, #ifndef VIRTIO_DRIVER_ONLY if (role == RPMSG_REMOTE) { - data = - virtqueue_get_available_buffer(rvdev->rvq, idx, len); + uint64_t phys_data; + int status; + + status = virtqueue_get_available_buffer(rvdev->rvq, idx, &phys_data, len); + if (!status) + data = metal_io_phys_to_virt(rvdev->shbuf_io, phys_data); } #endif /*!VIRTIO_DRIVER_ONLY*/ @@ -815,7 +824,7 @@ int rpmsg_init_vdev_with_config(struct rpmsg_virtio_device *rvdev, const char *vq_names[RPMSG_NUM_VRINGS]; vq_callback callback[RPMSG_NUM_VRINGS]; int status; - unsigned int i, role; + unsigned int role; if (!rvdev || !vdev || !shm_io) return RPMSG_ERR_PARAM; @@ -918,14 +927,6 @@ int rpmsg_init_vdev_with_config(struct rpmsg_virtio_device *rvdev, */ virtqueue_disable_cb(rvdev->svq); - /* TODO: can have a virtio function to set the shared memory I/O */ - for (i = 0; i < RPMSG_NUM_VRINGS; i++) { - struct virtqueue *vq; - - vq = vdev->vrings_info[i].vq; - vq->shm_io = shm_io; - } - #ifndef VIRTIO_DEVICE_ONLY if (role == RPMSG_HOST) { struct virtqueue_buf vqbuf; @@ -943,7 +944,7 @@ int rpmsg_init_vdev_with_config(struct rpmsg_virtio_device *rvdev, goto err; } - vqbuf.buf = buffer; + vqbuf.buf = metal_io_virt_to_phys(shm_io, buffer); metal_io_block_set(shm_io, metal_io_virt_to_offset(shm_io, diff --git a/lib/virtio/virtqueue.c b/lib/virtio/virtqueue.c index 2544ee360..93069df3c 100644 --- a/lib/virtio/virtqueue.c +++ b/lib/virtio/virtqueue.c @@ -28,24 +28,6 @@ static int virtqueue_nused(struct virtqueue *vq); static int virtqueue_navail(struct virtqueue *vq); #endif -/* Default implementation of P2V based on libmetal */ -static inline void *virtqueue_phys_to_virt(struct virtqueue *vq, - metal_phys_addr_t phys) -{ - struct metal_io_region *io = vq->shm_io; - - return metal_io_phys_to_virt(io, phys); -} - -/* Default implementation of V2P based on libmetal */ -static inline metal_phys_addr_t virtqueue_virt_to_phys(struct virtqueue *vq, - void *buf) -{ - struct metal_io_region *io = vq->shm_io; - - return metal_io_virt_to_phys(io, buf); -} - int virtqueue_create(struct virtio_device *virt_dev, unsigned short id, const char *name, struct vring_alloc_info *ring, void (*callback)(struct virtqueue *vq), @@ -182,11 +164,11 @@ uint32_t virtqueue_get_buffer_length(struct virtqueue *vq, uint16_t idx) return vq->vq_ring.desc[idx].len; } -void *virtqueue_get_buffer_addr(struct virtqueue *vq, uint16_t idx) +uint64_t virtqueue_get_buffer_addr(struct virtqueue *vq, uint16_t idx) { VRING_INVALIDATE(&vq->vq_ring.desc[idx].addr, sizeof(vq->vq_ring.desc[idx].addr)); - return virtqueue_phys_to_virt(vq, vq->vq_ring.desc[idx].addr); + return vq->vq_ring.desc[idx].addr; } void virtqueue_free(struct virtqueue *vq) @@ -202,18 +184,17 @@ void virtqueue_free(struct virtqueue *vq) } } -void *virtqueue_get_available_buffer(struct virtqueue *vq, uint16_t *avail_idx, - uint32_t *len) +int virtqueue_get_available_buffer(struct virtqueue *vq, uint16_t *avail_idx, + uint64_t *buffer, uint32_t *len) { uint16_t head_idx = 0; - void *buffer; atomic_thread_fence(memory_order_seq_cst); /* Avail.idx is updated by driver, invalidate it */ VRING_INVALIDATE(&vq->vq_ring.avail->idx, sizeof(vq->vq_ring.avail->idx)); if (vq->vq_available_idx == vq->vq_ring.avail->idx) { - return NULL; + return ERROR_VRING_NO_BUFF; } VQUEUE_BUSY(vq); @@ -228,12 +209,12 @@ void *virtqueue_get_available_buffer(struct virtqueue *vq, uint16_t *avail_idx, /* Invalidate the desc entry written by driver before accessing it */ VRING_INVALIDATE(&vq->vq_ring.desc[*avail_idx], sizeof(vq->vq_ring.desc[*avail_idx])); - buffer = virtqueue_phys_to_virt(vq, vq->vq_ring.desc[*avail_idx].addr); + *buffer = vq->vq_ring.desc[*avail_idx].addr; *len = vq->vq_ring.desc[*avail_idx].len; VQUEUE_IDLE(vq); - return buffer; + return 0; } int virtqueue_add_consumed_buffer(struct virtqueue *vq, uint16_t head_idx, @@ -414,7 +395,7 @@ static uint16_t vq_ring_add_buffer(struct virtqueue *vq, /* CACHE: No need to invalidate desc because it is only written by driver */ dp = &desc[idx]; - dp->addr = virtqueue_virt_to_phys(vq, buf_list[i].buf); + dp->addr = buf_list[i].buf; dp->len = buf_list[i].len; dp->flags = 0;