This small series enables virtio-net device type in VDUSE.
With it, basic operation have been tested, both with
virtio-vdpa and vhost-vdpa using DPDK Vhost library series
adding VDUSE support [0] using split rings layout.
Control queue support (and so multiqueue) has also been
tested, but require a Kernel series from Jason Wang
relaxing control queue polling [1] to function reliably.
Other than that, we have identified a few gaps:
1. Reconnection:
a. VDUSE_VQ_GET_INFO ioctl() returns always 0 for avail
index, even after the virtqueue has already been
processed. Is that expected? I have tried instead to
get the driver's avail index directly from the avail
ring, but it does not seem reliable as I sometimes get
"id %u is not a head!\n" warnings. Also such solution
would not be possible with packed ring, as we need to
know the wrap counters values.
b. Missing IOCTLs: it would be handy to have new IOCTLs to
query Virtio device status, and retrieve the config
space set at VDUSE_CREATE_DEV time.
2. VDUSE application as non-root:
We need to run the VDUSE application as non-root. There
is some race between the time the UDEV rule is applied
and the time the device starts being used. Discussing
with Jason, he suggested we may have a VDUSE daemon run
as root that would create the VDUSE device, manages its
rights and then pass its file descriptor to the VDUSE
app. However, with current IOCTLs, it means the VDUSE
daemon would need to know several information that
belongs to the VDUSE app implementing the device such
as supported Virtio features, config space, etc...
If we go that route, maybe we should have a control
IOCTL to create the device which would just pass the
device type. Then another device IOCTL to perform the
initialization. Would that make sense?
3. Coredump:
In order to be able to perform post-mortem analysis, DPDK
Vhost library marks pages used for vrings and descriptors
buffers as MADV_DODUMP using madvise(). However with
VDUSE it fails with -EINVAL. My understanding is that we
set VM_DONTEXPAND flag to the VMAs and madvise's
MADV_DODUMP fails if it is present. I'm not sure to
understand why madvise would prevent MADV_DODUMP if
VM_DONTEXPAND is set. Any thoughts?
[0]:
https://patchwork.dpdk.org/project/dpdk/list/?series=27594&state=%2A&archive=both
[1]:
https://lore.kernel.org/lkml/CACGkMEtgrxN3PPwsDo4oOsnsSLJfEmBEZ0WvjGRr3whU+QasUg
at mail.gmail.com/T/
Maxime Coquelin (2):
vduse: validate block features only with block devices
vduse: enable Virtio-net device type
drivers/vdpa/vdpa_user/vduse_dev.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
--
2.39.2
Maxime Coquelin
2023-Apr-19 13:43 UTC
[RFC 1/2] vduse: validate block features only with block devices
This patch is preliminary work to enable network device
type support to VDUSE.
As VIRTIO_BLK_F_CONFIG_WCE shares the same value as
VIRTIO_NET_F_HOST_TSO4, we need to restrict its check
to Virtio-blk device type.
Signed-off-by: Maxime Coquelin <maxime.coquelin at redhat.com>
---
drivers/vdpa/vdpa_user/vduse_dev.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c
b/drivers/vdpa/vdpa_user/vduse_dev.c
index 0c3b48616a9f..6fa598a03d8e 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1416,13 +1416,14 @@ static bool device_is_allowed(u32 device_id)
return false;
}
-static bool features_is_valid(u64 features)
+static bool features_is_valid(struct vduse_dev_config *config)
{
- if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
+ if (!(config->features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
return false;
/* Now we only support read-only configuration space */
- if (features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE))
+ if ((config->device_id == VIRTIO_ID_BLOCK) &&
+ (config->features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE)))
return false;
return true;
@@ -1446,7 +1447,7 @@ static bool vduse_validate_config(struct vduse_dev_config
*config)
if (!device_is_allowed(config->device_id))
return false;
- if (!features_is_valid(config->features))
+ if (!features_is_valid(config))
return false;
return true;
--
2.39.2
This patch adds Virtio-net device type to the supported
devices types.
Signed-off-by: Maxime Coquelin <maxime.coquelin at redhat.com>
---
drivers/vdpa/vdpa_user/vduse_dev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c
b/drivers/vdpa/vdpa_user/vduse_dev.c
index 6fa598a03d8e..26b7e29cb900 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -131,6 +131,7 @@ static struct workqueue_struct *vduse_irq_wq;
static u32 allowed_device_id[] = {
VIRTIO_ID_BLOCK,
+ VIRTIO_ID_NET,
};
static inline struct vduse_dev *vdpa_to_vduse(struct vdpa_device *vdpa)
@@ -1738,6 +1739,7 @@ static const struct vdpa_mgmtdev_ops vdpa_dev_mgmtdev_ops
= {
static struct virtio_device_id id_table[] = {
{ VIRTIO_ID_BLOCK, VIRTIO_DEV_ANY_ID },
+ { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
{ 0 },
};
--
2.39.2
On Wed, Apr 19, 2023 at 9:43?PM Maxime Coquelin <maxime.coquelin at redhat.com> wrote:> > This small series enables virtio-net device type in VDUSE. > With it, basic operation have been tested, both with > virtio-vdpa and vhost-vdpa using DPDK Vhost library series > adding VDUSE support [0] using split rings layout. > > Control queue support (and so multiqueue) has also been > tested, but require a Kernel series from Jason Wang > relaxing control queue polling [1] to function reliably. > > Other than that, we have identified a few gaps: > > 1. Reconnection: > a. VDUSE_VQ_GET_INFO ioctl() returns always 0 for avail > index, even after the virtqueue has already been > processed. Is that expected? I have tried instead to > get the driver's avail index directly from the avail > ring, but it does not seem reliable as I sometimes get > "id %u is not a head!\n" warnings. Also such solution > would not be possible with packed ring, as we need to > know the wrap counters values.Looking at the codes, it only returns the value that is set via set_vq_state(). I think it is expected to be called before the datapath runs. So when bound to virtio-vdpa, it is expected to return 0. But we need to fix the packed virtqueue case, I wonder if we need to call set_vq_state() explicitly in virtio-vdpa before starting the device. When bound to vhost-vdpa, Qemu will call VHOST_SET_VRING_BASE which will end up a call to set_vq_state(). Unfortunately, it doesn't support packed ring which needs some extension.> > b. Missing IOCTLs: it would be handy to have new IOCTLs to > query Virtio device status,What's the use case of this ioctl? It looks to me userspace is notified on each status change now: static int vduse_dev_set_status(struct vduse_dev *dev, u8 status) { struct vduse_dev_msg msg = { 0 }; msg.req.type = VDUSE_SET_STATUS; msg.req.s.status = status; return vduse_dev_msg_sync(dev, &msg); }> and retrieve the config > space set at VDUSE_CREATE_DEV time.In order to be safe, VDUSE avoids writable config space. Otherwise drivers could block on config writing forever. That's why we don't do it now. We need to harden the config write before we can proceed to this I think.> > 2. VDUSE application as non-root: > We need to run the VDUSE application as non-root. There > is some race between the time the UDEV rule is applied > and the time the device starts being used. Discussing > with Jason, he suggested we may have a VDUSE daemon run > as root that would create the VDUSE device, manages its > rights and then pass its file descriptor to the VDUSE > app. However, with current IOCTLs, it means the VDUSE > daemon would need to know several information that > belongs to the VDUSE app implementing the device such > as supported Virtio features, config space, etc... > If we go that route, maybe we should have a control > IOCTL to create the device which would just pass the > device type. Then another device IOCTL to perform the > initialization. Would that make sense?I think so. We can hear from others.> > 3. Coredump: > In order to be able to perform post-mortem analysis, DPDK > Vhost library marks pages used for vrings and descriptors > buffers as MADV_DODUMP using madvise(). However with > VDUSE it fails with -EINVAL. My understanding is that we > set VM_DONTEXPAND flag to the VMAs and madvise's > MADV_DODUMP fails if it is present. I'm not sure to > understand why madvise would prevent MADV_DODUMP if > VM_DONTEXPAND is set. Any thoughts?Adding Peter who may know the answer. Thanks> > [0]: https://patchwork.dpdk.org/project/dpdk/list/?series=27594&state=%2A&archive=both > [1]: https://lore.kernel.org/lkml/CACGkMEtgrxN3PPwsDo4oOsnsSLJfEmBEZ0WvjGRr3whU+QasUg at mail.gmail.com/T/ > > Maxime Coquelin (2): > vduse: validate block features only with block devices > vduse: enable Virtio-net device type > > drivers/vdpa/vdpa_user/vduse_dev.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > -- > 2.39.2 >