Hi Kuan-Wei,
On 2026-07-06T21:54:42, Kuan-Wei Chiu <[email protected]> wrote:
> virtio: 9p: Add 9P transport driver
>
> Add virtio transport driver for the 9P filesystem.
>
> This driver binds to the virtio-9p device exposed by the hypervisor and
> registers the transport operations with the 9P client framework. It
> implements the transmit and receive routines.
>
> Signed-off-by: Kuan-Wei Chiu <[email protected]>
>
> drivers/virtio/Kconfig | 7 ++++
> drivers/virtio/Makefile | 1 +
> drivers/virtio/virtio-uclass.c | 1 +
> drivers/virtio/virtio_9p.c | 75
> ++++++++++++++++++++++++++++++++++++++++++
> include/virtio.h | 4 ++-
> 5 files changed, 87 insertions(+), 1 deletion(-)
> diff --git a/drivers/virtio/virtio_9p.c b/drivers/virtio/virtio_9p.c
> @@ -0,0 +1,75 @@
> + flush_dcache_range((ulong)tx, ALIGN((ulong)tx + tx_len,
> ARCH_DMA_MINALIGN));
> + flush_dcache_range((ulong)rx, ALIGN((ulong)rx + rx_len,
> ARCH_DMA_MINALIGN));
Just to check, why are these needed? No other virtio driver (nor
virtio_ring.c itself) does any cache maintenance, so on a non-coherent
system the vring descriptors would still be stale and flushing the
data buffers alone would not help. If virtio on QEMU is effectively
coherent, these are dead code and should be dropped; if you have hit a
real cache problem, it needs fixing in virtio_ring.c for all drivers
rather than here. What do you think?
> diff --git a/drivers/virtio/virtio_9p.c b/drivers/virtio/virtio_9p.c
> @@ -0,0 +1,75 @@
> + if (virtqueue_add(priv->vq, sgs, 1, 1))
> + return -EIO;
> +
> + virtqueue_kick(priv->vq);
> +
> + while (!virtqueue_get_buf(priv->vq, &len))
> + ;
Please can you propagate the error code from virtqueue_add() rather
than replacing it with -EIO
Also, this loop spins forever if the device never responds, hanging
U-Boot with no way out. I realise virtio_rng does the same, but a
filesystem request is a good deal larger, so please add a timeout
(e.g. using get_timer()) and return -ETIMEDOUT
BTW 'len' is the number of bytes the device actually wrote, but it is
discarded here. It would be worth passing it back to the client so it
can validate the size field in the reply header against what was
really received.
> diff --git a/drivers/virtio/virtio_9p.c b/drivers/virtio/virtio_9p.c
> @@ -0,0 +1,75 @@
> + priv->client.ops = &virtio_9p_ops;
> + priv->client.priv = priv;
> + p9_register_client(&priv->client);
Since you are adding a new virtio driver you may need a bind() method
which calls virtio_driver_features_init(), as virtio_net, virtio_blk
and virtio_rng all do. That would also let you negotiate
VIRTIO_9P_MOUNT_TAG and read the mount tag from the config space,
which is needed for the device selection mentioned in my reply to the
cover letter. It would be good to at least log the tag now, so that
selection can be added later.
> diff --git a/drivers/virtio/virtio_9p.c b/drivers/virtio/virtio_9p.c
> @@ -0,0 +1,75 @@
> +U_BOOT_DRIVER(virtio_9p) = {
> + .name = VIRTIO_9P_DRV_NAME,
> + .id = UCLASS_MISC,
> + .probe = virtio_9p_probe,
> + .priv_auto = sizeof(struct virtio_9p_priv),
> +};
This is missing .remove = virtio_reset and
.flags = DM_FLAG_ACTIVE_DMA, which the other virtio drivers have.
Without them the device is left with DMA active when U-Boot boots an
OS. The remove path also needs to unregister the client: priv is freed
on removal, so the pointer held by p9_register_client() in patch 1
would be left dangling and a later 'ls 9p - /' would use freed memory.
Regards,
Simon
