| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJaqyWcvHx7kwcTceN2jazT0nKNo1r5zdzqWHqpxdna-kCS1RA@mail.gmail.com>
Date: Mon, 9 Jun 2025 08:03:13 +0200
From: Eugenio Perez Martin <eperezma@...hat.com>
To: Jason Wang <jasowang@...hat.com>
Cc: Yongji Xie <xieyongji@...edance.com>, Cindy Lu <lulu@...hat.com>,
linux-kernel@...r.kernel.org, Stefano Garzarella <sgarzare@...hat.com>,
Stefan Hajnoczi <stefanha@...hat.com>, Maxime Coquelin <mcoqueli@...hat.com>,
"Michael S. Tsirkin" <mst@...hat.com>, virtualization@...ts.linux.dev,
Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Laurent Vivier <lvivier@...hat.com>
Subject: Re: [RFC 2/6] vduse: add vq group support
On Mon, Jun 9, 2025 at 3:55 AM Jason Wang <jasowang@...hat.com> wrote:
>
> On Fri, Jun 6, 2025 at 7:50 PM Eugenio Pérez <eperezma@...hat.com> wrote:
> >
> > The virtqueue group is the minimal set of virtqueues that must share an
> > address space. And the address space identifier could only be attached
> > to a specific virtqueue group. The virtqueue is attached to a
> > virtqueue group for all the life of the device.
> >
> > During vDPA device allocation, the VDUSE device needs to report the
> > number of virtqueue groups supported. At this moment only vhost_vdpa is
> > able to do it.
> >
> > This helps to isolate the environments for the virtqueue that will not
> > be assigned directly. E.g in the case of virtio-net, the control
> > virtqueue will not be assigned directly to guest.
> >
> > As we need to back the vq groups with a struct device for the file
> > operations, let's keep this number as low as possible at the moment: 2.
> > We can back one VQ group with the vduse device and the other one with
> > the vdpa device.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@...hat.com>
> > ---
> > drivers/vdpa/vdpa_user/vduse_dev.c | 44 +++++++++++++++++++++++++++++-
> > include/uapi/linux/vduse.h | 17 +++++++++++-
> > 2 files changed, 59 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > index 6a9a37351310..6fa687bc4912 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -46,6 +46,11 @@
> > #define VDUSE_IOVA_SIZE (VDUSE_MAX_BOUNCE_SIZE + 128 * 1024 * 1024)
> > #define VDUSE_MSG_DEFAULT_TIMEOUT 30
> >
> > +/*
> > + * Let's make it 2 for simplicity.
> > + */
> > +#define VDUSE_MAX_VQ_GROUPS 2
> > +
> > #define IRQ_UNBOUND -1
> >
> > struct vduse_virtqueue {
> > @@ -114,6 +119,7 @@ struct vduse_dev {
> > u8 status;
> > u32 vq_num;
> > u32 vq_align;
> > + u32 ngroups;
> > struct vduse_umem *umem;
> > struct mutex mem_lock;
> > unsigned int bounce_size;
> > @@ -592,6 +598,25 @@ static int vduse_vdpa_set_vq_state(struct vdpa_device *vdpa, u16 idx,
> > return 0;
> > }
> >
> > +static u32 vduse_get_vq_group(struct vdpa_device *vdpa, u16 idx)
> > +{
> > + struct vduse_dev *dev = vdpa_to_vduse(vdpa);
> > + struct vduse_dev_msg msg = { 0 };
> > + int ret;
> > +
> > + if (dev->api_version < VDUSE_API_VERSION_1)
> > + return 0;
> > +
> > + msg.req.type = VDUSE_GET_VQ_GROUP;
> > + msg.req.vq_group.index = idx;
>
> Considering there will be a set_group_asid request, could the kernel
> cache the result so we don't need to bother with requests from
> userspace?
>
Yes we can, actually a previous version did it. But what's the use? It
is not used in the dataplane, so we reduce complexity if we don't
store it.
What's more, in the case of the net device, the vq number -> vq group
association can change in a reset as the CVQ is either the last one or
#2 if MQ is negotiated. We need to code when to reset this
association, so complexity grows even more. And the vq group are not
asked by QEMU after that point anyway.
> > +
> > + ret = vduse_dev_msg_sync(dev, &msg);
> > + if (ret)
> > + return ret;
> > +
> > + return msg.resp.vq_group.num;
> > +}
> > +
> > static int vduse_vdpa_get_vq_state(struct vdpa_device *vdpa, u16 idx,
> > struct vdpa_vq_state *state)
> > {
> > @@ -789,6 +814,7 @@ static const struct vdpa_config_ops vduse_vdpa_config_ops = {
> > .set_vq_cb = vduse_vdpa_set_vq_cb,
> > .set_vq_num = vduse_vdpa_set_vq_num,
> > .get_vq_size = vduse_vdpa_get_vq_size,
> > + .get_vq_group = vduse_get_vq_group,
> > .set_vq_ready = vduse_vdpa_set_vq_ready,
> > .get_vq_ready = vduse_vdpa_get_vq_ready,
> > .set_vq_state = vduse_vdpa_set_vq_state,
> > @@ -1850,6 +1876,16 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > dev->device_features = config->features;
> > dev->device_id = config->device_id;
> > dev->vendor_id = config->vendor_id;
> > + if (dev->api_version >= 1) {
> > + if (config->ngroups > VDUSE_MAX_VQ_GROUPS) {
> > + pr_err("Not creating a VDUSE device with %u vq groups. Max: %u",
> > + config->ngroups, VDUSE_MAX_VQ_GROUPS);
> > + goto err_ngroups;
> > + }
> > + dev->ngroups = config->ngroups ?: 1;
> > + } else {
> > + dev->ngroups = 1;
> > + }
> > dev->name = kstrdup(config->name, GFP_KERNEL);
> > if (!dev->name)
> > goto err_str;
> > @@ -1885,6 +1921,7 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > idr_remove(&vduse_idr, dev->minor);
> > err_idr:
> > kfree(dev->name);
> > +err_ngroups:
> > err_str:
> > vduse_dev_destroy(dev);
> > err:
> > @@ -2003,13 +2040,18 @@ static struct vduse_mgmt_dev *vduse_mgmt;
> > static int vduse_dev_init_vdpa(struct vduse_dev *dev, const char *name)
> > {
> > struct vduse_vdpa *vdev;
> > + __u32 ngroups = 1;
> > int ret;
> >
> > if (dev->vdev)
> > return -EEXIST;
> >
> > + if (vdev->dev->api_version >= VDUSE_API_VERSION_1)
> > + ngroups = vdev->dev->ngroups;
> > +
> > vdev = vdpa_alloc_device(struct vduse_vdpa, vdpa, dev->dev,
> > - &vduse_vdpa_config_ops, 1, 1, name, true);
> > + &vduse_vdpa_config_ops, ngroups, 1, name,
> > + true);
> > if (IS_ERR(vdev))
> > return PTR_ERR(vdev);
> >
> > diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> > index 9a56d0416bfe..a779bcddac58 100644
> > --- a/include/uapi/linux/vduse.h
> > +++ b/include/uapi/linux/vduse.h
> > @@ -45,7 +45,8 @@ struct vduse_dev_config {
> > __u64 features;
> > __u32 vq_num;
> > __u32 vq_align;
> > - __u32 reserved[13];
> > + __u32 ngroups; /* if VDUSE_API_VERSION >= 1 */
> > + __u32 reserved[12];
> > __u32 config_size;
> > __u8 config[];
> > };
> > @@ -160,6 +161,16 @@ struct vduse_vq_state_packed {
> > __u16 last_used_idx;
> > };
> >
> > +/**
> > + * struct vduse_vq_group - virtqueue group
> > + * @num: Index of the virtqueue group
> > + * @num: Group
> > + */
> > +struct vduse_vq_group {
> > + __u32 index;
> > + __u32 num;
> > +};
> > +
> > /**
> > * struct vduse_vq_info - information of a virtqueue
> > * @index: virtqueue index
> > @@ -182,6 +193,7 @@ struct vduse_vq_info {
> > union {
> > struct vduse_vq_state_split split;
> > struct vduse_vq_state_packed packed;
> > + struct vduse_vq_group group;
> > };
> > __u8 ready;
> > };
> > @@ -274,6 +286,7 @@ enum vduse_req_type {
> > VDUSE_GET_VQ_STATE,
> > VDUSE_SET_STATUS,
> > VDUSE_UPDATE_IOTLB,
> > + VDUSE_GET_VQ_GROUP,
> > };
> >
> > /**
> > @@ -328,6 +341,7 @@ struct vduse_dev_request {
> > struct vduse_vq_state vq_state;
> > struct vduse_dev_status s;
> > struct vduse_iova_range iova;
> > + struct vduse_vq_group vq_group; /* Only if vduse api version >= 1 */
> > __u32 padding[32];
>
> This should be [31] otherwise we break the uAPI?
>
> > };
> > };
> > @@ -350,6 +364,7 @@ struct vduse_dev_response {
> > __u32 reserved[4];
> > union {
> > struct vduse_vq_state vq_state;
> > + struct vduse_vq_group vq_group; /* Only if vduse api version >= 1 */
> > __u32 padding[32];
>
> And here?
>
Yes, thank you for both catches!
> Thanks
>
> > };
> > };
> > --
> > 2.49.0
> >
>
Powered by blists - more mailing lists