casaroli commented on code in PR #21408:
URL: https://github.com/apache/nuttx/pull/21408#discussion_r4467343942
##########
drivers/virtio/virtio-blk.c:
##########
@@ -212,19 +277,79 @@ static ssize_t virtio_blk_rdwr(FAR struct
virtio_blk_priv_s *priv,
virtio_blk_wait_complete(vq, &respsem);
- if (resp.status != VIRTIO_BLK_S_OK)
+ if (up_interrupt_context())
+ {
+ virtqueue_enable_cb_lock(vq, &priv->lock);
+ }
+
+ ret = chunk;
+
+out_resp:
+
+ /* unmap copies the device-written status back into resp; only after that
+ * is resp.status valid (on a bouncing transport resp is a bounce buffer).
+ */
+
+ virtio_unmap_buf(vdev, &resp, sizeof(resp), VIRTIO_DMA_FROM_DEVICE,
+ respmap);
+out_req:
+ virtio_unmap_buf(vdev, &req, sizeof(req), VIRTIO_DMA_TO_DEVICE, reqmap);
+out_data:
+ virtio_unmap_buf(vdev, buf, datalen, datadir, datamap);
+
+ if (ret >= 0 && resp.status != VIRTIO_BLK_S_OK)
{
vrterr("%s Error\n", write ? "Write" : "Read");
ret = -EIO;
}
-err:
- if (up_interrupt_context())
+ return ret;
+}
+
+/****************************************************************************
+ * Name: virtio_blk_rdwr
+ *
+ * Description:
+ * Common function for read and write
+ *
+ ****************************************************************************/
+
+static ssize_t virtio_blk_rdwr(FAR struct virtio_blk_priv_s *priv,
+ FAR void *buffer, blkcnt_t startsector,
+ unsigned int nsectors, bool write)
+{
+ blkcnt_t sector = startsector;
+ FAR uint8_t *buf = buffer;
+ ssize_t ret = 0;
+
+ /* The data payload may exceed the shared memory a bounce transport can
+ * map at once, so transfer it in chunks. On MMIO/PCI virtio_free_largest
+ * returns SIZE_MAX and map is a no-op, so this runs as a single pass.
+ */
+
+ while (nsectors > 0)
{
- virtqueue_enable_cb_lock(vq, &priv->lock);
+ ret = virtio_blk_rdwr_one(priv, buf, sector, nsectors, write);
+ if (ret < 0)
+ {
+ break;
+ }
+
+ buf += ret * priv->block_size;
+ sector += ret;
+ nsectors -= ret;
+ }
+
+ /* Report the number of sectors transferred, or the failure code when
+ * nothing completed.
+ */
+
+ if (sector > startsector)
+ {
+ ret = sector - startsector;
Review Comment:
A failed chunk after a good one returns the sectors done, and
`bchlib_read()`/`bchlib_write()` test only `ret < 0` and count all `nsectors`
(bchlib_read.c:153-163). The rest of the data is lost with no error.
Fix: return the negative `ret` when `virtio_blk_rdwr_one()` fails, whatever
the earlier chunks did.
##########
drivers/rptun/rptun.c:
##########
@@ -308,6 +318,137 @@ static void rptun_free_buf(FAR struct virtio_device
*vdev, FAR void *buf)
mm_free(carveout->heap, buf);
}
+/****************************************************************************
+ * Name: rptun_map_align
+ *
+ * Description:
+ * Return the alignment used for bounce buffers. Use the dcache line size
+ * so a bounce buffer never shares a cache line with a neighbour (avoids
+ * clobbering adjacent data on invalidate). Fall back to MM_ALIGN when the
+ * arch has no dcache, so the value is never zero.
+ *
+ ****************************************************************************/
+
+static inline_function size_t rptun_map_align(void)
+{
+ size_t linesize = up_get_dcache_linesize();
+
+ return linesize > MM_ALIGN ? linesize : MM_ALIGN;
+}
+
+/****************************************************************************
+ * Name: rptun_free_largest
+ *
+ * Description:
+ * Return the largest buffer a single rptun_map_buf call can satisfy.
+ * mm_heapfree_largest gives the largest mm_malloc allocation; subtract the
+ * extra space mm_memalign consumes for alignment so a map of the returned
+ * size is guaranteed to succeed.
+ *
+ ****************************************************************************/
+
+static size_t rptun_free_largest(FAR struct virtio_device *vdev)
+{
+ FAR struct rptun_carveout_s *carveout =
+ (FAR struct rptun_carveout_s *)vdev->mm_priv;
+ size_t align = rptun_map_align();
+ size_t largest = mm_heapfree_largest(carveout->heap);
+
+ /* mm_memalign over-allocates by up to 2 * align to find an aligned point,
+ * and map rounds len up to align. Reserve 3 * align to stay safe.
+ */
+
+ size_t overhead = 3 * align;
+
+ return largest > overhead ? largest - overhead : 0;
+}
+
+/****************************************************************************
+ * Name: rptun_map_buf
+ *
+ * Description:
+ * Map a buffer so the remote side can access it. The remote can only
+ * touch the carveout shared memory, so a buffer outside the carveout is
+ * bounced through a buffer allocated from the carveout heap. Returns NULL
+ * when the carveout heap cannot satisfy the allocation; the caller may
+ * query rptun_free_largest and retry with a smaller length.
+ *
+ ****************************************************************************/
+
+static FAR void *rptun_map_buf(FAR struct virtio_device *vdev,
+ FAR void *buf, size_t len,
+ enum virtio_dma_map dir)
+{
+ FAR struct rptun_carveout_s *carveout =
+ (FAR struct rptun_carveout_s *)vdev->mm_priv;
+ size_t align = rptun_map_align();
+ FAR void *bounce;
+ size_t maplen;
+
+ /* Already inside the carveout heap: the remote can access it directly. */
+
+ if (mm_heapmember(carveout->heap, buf))
+ {
+ return buf;
+ }
+
+ maplen = ALIGN_UP(len, align);
+ bounce = mm_memalign(carveout->heap, align, maplen);
+ if (bounce == NULL)
+ {
+ /* Out of shared memory: the caller decides whether to query
+ * rptun_free_largest and retry with a smaller length.
+ */
+
+ return NULL;
+ }
+
+ if (dir != VIRTIO_DMA_FROM_DEVICE)
Review Comment:
A VIRTIO_DMA_FROM_DEVICE bounce gets no cache maintenance at map, so with
CONFIG_OPENAMP_CACHE a dirty line from an earlier use of the block can be
written back over the peer's data.
Fix: call `RPTUN_FLUSH(bounce, maplen)` for every direction and do the
`memcpy()` only for `dir != VIRTIO_DMA_FROM_DEVICE`.
##########
drivers/virtio/virtio-rng.c:
##########
@@ -160,23 +161,34 @@ static ssize_t virtio_rng_read(FAR struct file *filep,
FAR char *buffer,
cookie.len = 0;
nxsem_init(&cookie.sem, 0, 0);
+ /* Map the user buffer so the device side can access it. On MMIO/PCI the
+ * peer reaches arbitrary memory, so map returns the buffer itself and the
+ * device writes the entropy straight into it (zero copy). On rptun the
+ * buffer is bounced through shared memory and unmap copies the result
+ * back below.
+ */
+
+ ret = virtio_map_buf(priv->vdev, buffer, buflen, VIRTIO_DMA_FROM_DEVICE,
Review Comment:
In BUILD_KERNEL `buffer` is a user virtual address and only its first byte
is translated, so the device writes `buflen` physically contiguous bytes while
the user pages are not adjacent. rv-virt:knetnsh64, `read()` of 12288 bytes on
alternate pages: the last 4096 bytes stay zero, master fills all of them.
Fix: keep the `virtio_zalloc_buf()` + `memcpy()` of master in
`virtio_rng_read()` when CONFIG_ARCH_ADDRENV is set, and use the map only
without it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
