casaroli commented on code in PR #21408:
URL: https://github.com/apache/nuttx/pull/21408#discussion_r4469837792
##########
drivers/virtio/virtio.c:
##########
@@ -356,3 +359,108 @@ int virtio_unregister_device(FAR struct virtio_device
*device)
nxmutex_unlock(&g_virtio_bus.lock);
return ret;
}
+
+#ifdef CONFIG_ARCH_ADDRENV
+
+/****************************************************************************
+ * Name: virtio_addrenv_free_largest
+ *
+ * Description:
+ * Return the largest buffer virtio_addrenv_map() can bounce, for a
+ * transport whose device reaches guest physical memory directly.
+ *
+ * Input Parameters:
+ * vdev - The virtio device
+ *
+ * Returned Value:
+ * The largest mappable length in bytes.
+ *
+ ****************************************************************************/
+
+size_t virtio_addrenv_free_largest(FAR struct virtio_device *vdev)
+{
+ struct mallinfo info = kmm_mallinfo();
+
+ /* mxordblk counts the chunk header kmm_malloc() needs as well. */
+
+ return info.mxordblk > 64 ? info.mxordblk - 64 : 0;
+}
+
+/****************************************************************************
+ * Name: virtio_addrenv_map
+ *
+ * Description:
+ * Map a buffer for a transport whose device reaches guest physical memory
+ * directly. A user address is only virtually contiguous, while the device
+ * accesses len bytes from the physical address of its first byte, so it
+ * is bounced through the kernel heap. A kernel address is linearly
+ * mapped and used as is.
+ *
+ * Input Parameters:
+ * vdev - The virtio device
+ * buf - The buffer to map
+ * len - The buffer length in bytes
+ * dir - The transfer direction
+ *
+ * Returned Value:
+ * The address to give the device, or NULL when out of memory.
+ *
+ ****************************************************************************/
+
+FAR void *virtio_addrenv_map(FAR struct virtio_device *vdev, FAR void *buf,
+ size_t len, enum virtio_dma_map dir)
+{
+ FAR void *bounce;
+
+ if (!up_addrenv_user_vaddr((uintptr_t)buf))
+ {
+ return buf;
+ }
+
+ bounce = kmm_malloc(len);
Review Comment:
A failed read still copies the whole bounce into the caller's buffer, and
`kmm_malloc()` does not clear it: `virtio_blk_rdwr_one()` unmaps at line 298,
before the status test. rv-virt:knetnsh64 with a QEMU blkdebug read error:
`read()` of 4096 bytes returns EIO and all 4096 bytes of the buffer change;
master changes 0.
Fix: clear the bounce for `VIRTIO_DMA_FROM_DEVICE` at map: `kmm_zalloc()`
here, and `memset()` before the flush in `rptun_map_buf()`.
##########
drivers/virtio/virtio.c:
##########
@@ -356,3 +359,108 @@ int virtio_unregister_device(FAR struct virtio_device
*device)
nxmutex_unlock(&g_virtio_bus.lock);
return ret;
}
+
+#ifdef CONFIG_ARCH_ADDRENV
+
+/****************************************************************************
+ * Name: virtio_addrenv_free_largest
+ *
+ * Description:
+ * Return the largest buffer virtio_addrenv_map() can bounce, for a
+ * transport whose device reaches guest physical memory directly.
+ *
+ * Input Parameters:
+ * vdev - The virtio device
+ *
+ * Returned Value:
+ * The largest mappable length in bytes.
+ *
+ ****************************************************************************/
+
+size_t virtio_addrenv_free_largest(FAR struct virtio_device *vdev)
+{
+ struct mallinfo info = kmm_mallinfo();
+
+ /* mxordblk counts the chunk header kmm_malloc() needs as well. */
+
+ return info.mxordblk > 64 ? info.mxordblk - 64 : 0;
Review Comment:
`virtio_free_largest()` runs for every request, so this walks the whole
kernel heap with the heap lock held, also for a kernel buffer that is not
bounced. In an SMP interrupt `mm_lock()` returns -EAGAIN, so this returns 0 and
virtio-blk returns -ENOMEM on the path that `virtio_blk_wait_complete()` keeps
for interrupt and panic context.
Fix: return a fixed limit (for example 64 KiB) with no heap walk; a failed
`kmm_malloc()` already gives -ENOMEM.
##########
drivers/virtio/virtio-blk.c:
##########
@@ -147,62 +151,123 @@ static void virtio_blk_wait_complete(FAR struct
virtqueue *vq,
}
/****************************************************************************
- * Name: virtio_blk_rdwr
+ * Name: virtio_blk_rdwr_one
*
* Description:
- * Common function for read and write
+ * Transfer a single chunk, sized to the shared memory a bounce transport
+ * can map at once. Map req/data/resp, submit, wait, then unmap.
+ *
+ * Returned Value:
+ * The number of sectors transferred (> 0) on success, 0 when the shared
+ * memory cannot hold even one sector this round, or a negated errno on
Review Comment:
Lines 162 and 203-204 say the function returns 0 and the caller reports the
sectors already transferred, but it returns -ENOMEM (line 212) and the caller
returns the error (line 339). Lines 326 of this file and 164, 205 of
virtio-rng.c say map is a no-op on MMIO/PCI, which is not true with
CONFIG_ARCH_ADDRENV.
Fix: update these comments.
--
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]
