Re: [RFC 3/6] vduse: add vq group asid support
On Fri, Jun 13, 2025 at 3:21 AM Jason Wang wrote:
>
> On Thu, Jun 12, 2025 at 3:25 PM Eugenio Perez Martin
> wrote:
> >
> > On Thu, Jun 12, 2025 at 2:30 AM Jason Wang wrote:
> > >
> > > On Fri, Jun 6, 2025 at 7:50 PM Eugenio Pérez wrote:
> > > >
> > > > Make one IOTLB domain per address space, and allow the driver to assign
> > > > each ASID to a vq group. Each address space via an dedicated identifier
> > > > (ASID).
> > > >
> > > > During vDPA device allocation, the VDUSE device needs to report the
> > > > number of address spaces supported. Then the vdpa driver is able to
> > > > configure them. 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.
> > > >
> > > > TODO: Ideally, umem should not be duplicated. But it is hard or
> > > > impossible to refactor everything around one single umem. So should we
> > > > continue with device specifying umem per vq group?
> > >
> > > This is a good question.
> > >
> > > I think umem should be bound to address space and umem needs to be
> > > isolated.
> > >
> > > For the issue of complexity, we can simply extend the vduse_iova_umem
> > > to have an asid field. But it looks like it needs more work as:
> > >
> > > struct vduse_iova_umem {
> > > __u64 uaddr;
> > > __u64 iova;
> > > __u64 size;
> > > __u64 reserved[3];
> > > };
> > >
> > > Do we have a way to know if reserved is used or not (as we are lacking
> > > a flag field anyhow ).
> > >
> >
> > I'd say that we should work the same way as the rest of the structs:
> > We add the asid field, and if the API v1 is negotiated we handle it as
> > ASID. If it is not negotiated, it is reserved.
>
> Ok, that makes sense.
>
> >
> > > So we probably need a new uAPI like vduse_iova_umem_v2 that includes a
> > > flag field at least.
> > >
> > > >
> > > > Signed-off-by: Eugenio Pérez
> > > > ---
> > > > drivers/vdpa/vdpa_user/vduse_dev.c | 250 +
> > > > include/uapi/linux/vduse.h | 38 -
> > > > 2 files changed, 216 insertions(+), 72 deletions(-)
> > > >
> > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > index 6fa687bc4912..d51e4f26fe72 100644
> > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > @@ -51,6 +51,11 @@
> > > > */
> > > > #define VDUSE_MAX_VQ_GROUPS 2
> > > >
> > > > +/*
> > > > + * Let's make it 2 for simplicity.
> > > > + */
> > > > +#define VDUSE_MAX_ASID 2
> > >
> > > Similar to previous patch, it's better to increase this otherwise we
> > > need new uAPI or it requires the userspace to probe the maximum value
> > > once we decide to change it in the future.
> > >
> >
> > I'm ok with this, but what is a good max value? UINT32_MAX seems excessive?
>
> Maybe 64 or 256.
>
> >
> > This requires us to allocate arrays for both vduse_dev->domain and
> > vduse_dev->umem, so we need to set a reasonable value.
>
> Could we do the allocation based on the userspace privionsing?
>
Oh yes, my point is that we need to set a hard maximum so userland
cannot allocate an array of uint32_t[UINT32_MAX], for example. I guess
64 is reasonable. Is it worth it to dynamically allocate a whole array
with this maximum or is it better to make it just part of the struct?
I'm happy with both solutions :).
> >
> > > > +
> > > > #define IRQ_UNBOUND -1
> > > >
> > > > struct vduse_virtqueue {
> > > > @@ -92,7 +97,7 @@ struct vduse_dev {
> > > > struct vduse_vdpa *vdev;
> > > > struct device *dev;
> > > > struct vduse_virtqueue **vqs;
> > > > - struct vduse_iova_domain *domain;
> > > > + struct vduse_iova_domain *domain[VDUSE_MAX_ASID];
> > > > char *name;
> > > > struct mutex lock;
> > > > spinlock_t msg_lock;
> > > > @@ -120,7 +125,8 @@ struct vduse_dev {
> > > > u32 vq_num;
> > > > u32 vq_align;
> > > > u32 ngroups;
> > > > - struct vduse_umem *umem;
> > > > + u32 nas;
> > > > + struct vduse_umem *umem[VDUSE_MAX_ASID];
> > > > struct mutex mem_lock;
> > > > unsigned int bounce_size;
> > > > struct mutex domain_lock;
> > > > @@ -436,11 +442,14 @@ static __poll_t vduse_dev_poll(struct file *file,
> > > > poll_table *wait)
> > > > static void vduse_dev_reset(struct vduse_dev *dev)
> > > > {
> > > > int i;
> > > > - struct vduse_iova_domain *domain = dev->domain;
> > > >
> > > > /* The coherent mappings are handled in
> > > > vduse_dev_free_coherent() */
> > > > - if (domain && domain->bounce_map)
> > > > - vduse_domain_reset_bounce_map(domain);
> > > > + for (i = 0; i < dev->nas; i++) {
> > > > + struct vduse_iova_domain *domain =
Re: [RFC 3/6] vduse: add vq group asid support
On Thu, Jun 12, 2025 at 3:25 PM Eugenio Perez Martin
wrote:
>
> On Thu, Jun 12, 2025 at 2:30 AM Jason Wang wrote:
> >
> > On Fri, Jun 6, 2025 at 7:50 PM Eugenio Pérez wrote:
> > >
> > > Make one IOTLB domain per address space, and allow the driver to assign
> > > each ASID to a vq group. Each address space via an dedicated identifier
> > > (ASID).
> > >
> > > During vDPA device allocation, the VDUSE device needs to report the
> > > number of address spaces supported. Then the vdpa driver is able to
> > > configure them. 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.
> > >
> > > TODO: Ideally, umem should not be duplicated. But it is hard or
> > > impossible to refactor everything around one single umem. So should we
> > > continue with device specifying umem per vq group?
> >
> > This is a good question.
> >
> > I think umem should be bound to address space and umem needs to be isolated.
> >
> > For the issue of complexity, we can simply extend the vduse_iova_umem
> > to have an asid field. But it looks like it needs more work as:
> >
> > struct vduse_iova_umem {
> > __u64 uaddr;
> > __u64 iova;
> > __u64 size;
> > __u64 reserved[3];
> > };
> >
> > Do we have a way to know if reserved is used or not (as we are lacking
> > a flag field anyhow ).
> >
>
> I'd say that we should work the same way as the rest of the structs:
> We add the asid field, and if the API v1 is negotiated we handle it as
> ASID. If it is not negotiated, it is reserved.
Ok, that makes sense.
>
> > So we probably need a new uAPI like vduse_iova_umem_v2 that includes a
> > flag field at least.
> >
> > >
> > > Signed-off-by: Eugenio Pérez
> > > ---
> > > drivers/vdpa/vdpa_user/vduse_dev.c | 250 +
> > > include/uapi/linux/vduse.h | 38 -
> > > 2 files changed, 216 insertions(+), 72 deletions(-)
> > >
> > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > index 6fa687bc4912..d51e4f26fe72 100644
> > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > @@ -51,6 +51,11 @@
> > > */
> > > #define VDUSE_MAX_VQ_GROUPS 2
> > >
> > > +/*
> > > + * Let's make it 2 for simplicity.
> > > + */
> > > +#define VDUSE_MAX_ASID 2
> >
> > Similar to previous patch, it's better to increase this otherwise we
> > need new uAPI or it requires the userspace to probe the maximum value
> > once we decide to change it in the future.
> >
>
> I'm ok with this, but what is a good max value? UINT32_MAX seems excessive?
Maybe 64 or 256.
>
> This requires us to allocate arrays for both vduse_dev->domain and
> vduse_dev->umem, so we need to set a reasonable value.
Could we do the allocation based on the userspace privionsing?
>
> > > +
> > > #define IRQ_UNBOUND -1
> > >
> > > struct vduse_virtqueue {
> > > @@ -92,7 +97,7 @@ struct vduse_dev {
> > > struct vduse_vdpa *vdev;
> > > struct device *dev;
> > > struct vduse_virtqueue **vqs;
> > > - struct vduse_iova_domain *domain;
> > > + struct vduse_iova_domain *domain[VDUSE_MAX_ASID];
> > > char *name;
> > > struct mutex lock;
> > > spinlock_t msg_lock;
> > > @@ -120,7 +125,8 @@ struct vduse_dev {
> > > u32 vq_num;
> > > u32 vq_align;
> > > u32 ngroups;
> > > - struct vduse_umem *umem;
> > > + u32 nas;
> > > + struct vduse_umem *umem[VDUSE_MAX_ASID];
> > > struct mutex mem_lock;
> > > unsigned int bounce_size;
> > > struct mutex domain_lock;
> > > @@ -436,11 +442,14 @@ static __poll_t vduse_dev_poll(struct file *file,
> > > poll_table *wait)
> > > static void vduse_dev_reset(struct vduse_dev *dev)
> > > {
> > > int i;
> > > - struct vduse_iova_domain *domain = dev->domain;
> > >
> > > /* The coherent mappings are handled in vduse_dev_free_coherent()
> > > */
> > > - if (domain && domain->bounce_map)
> > > - vduse_domain_reset_bounce_map(domain);
> > > + for (i = 0; i < dev->nas; i++) {
> > > + struct vduse_iova_domain *domain = dev->domain[i];
> > > +
> > > + if (domain && domain->bounce_map)
> > > + vduse_domain_reset_bounce_map(domain);
> > > + }
> > >
> > > down_write(&dev->rwsem);
> > >
> > > @@ -617,6 +626,23 @@ static u32 vduse_get_vq_group(struct vdpa_device
> > > *vdpa, u16 idx)
> > > return msg.resp.vq_group.num;
> > > }
> > >
> > > +static int vduse_set_group_asid(struct vdpa_device *vdpa, unsigned int
> > > group,
> > > + unsigned int asid)
> > > +{
> > > + struct vduse_dev *dev = vdpa_to_vduse(vdpa);
> > > + struct vduse_dev_msg
Re: [RFC 3/6] vduse: add vq group asid support
On Thu, Jun 12, 2025 at 2:30 AM Jason Wang wrote:
>
> On Fri, Jun 6, 2025 at 7:50 PM Eugenio Pérez wrote:
> >
> > Make one IOTLB domain per address space, and allow the driver to assign
> > each ASID to a vq group. Each address space via an dedicated identifier
> > (ASID).
> >
> > During vDPA device allocation, the VDUSE device needs to report the
> > number of address spaces supported. Then the vdpa driver is able to
> > configure them. 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.
> >
> > TODO: Ideally, umem should not be duplicated. But it is hard or
> > impossible to refactor everything around one single umem. So should we
> > continue with device specifying umem per vq group?
>
> This is a good question.
>
> I think umem should be bound to address space and umem needs to be isolated.
>
> For the issue of complexity, we can simply extend the vduse_iova_umem
> to have an asid field. But it looks like it needs more work as:
>
> struct vduse_iova_umem {
> __u64 uaddr;
> __u64 iova;
> __u64 size;
> __u64 reserved[3];
> };
>
> Do we have a way to know if reserved is used or not (as we are lacking
> a flag field anyhow ).
>
I'd say that we should work the same way as the rest of the structs:
We add the asid field, and if the API v1 is negotiated we handle it as
ASID. If it is not negotiated, it is reserved.
> So we probably need a new uAPI like vduse_iova_umem_v2 that includes a
> flag field at least.
>
> >
> > Signed-off-by: Eugenio Pérez
> > ---
> > drivers/vdpa/vdpa_user/vduse_dev.c | 250 +
> > include/uapi/linux/vduse.h | 38 -
> > 2 files changed, 216 insertions(+), 72 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c
> > b/drivers/vdpa/vdpa_user/vduse_dev.c
> > index 6fa687bc4912..d51e4f26fe72 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -51,6 +51,11 @@
> > */
> > #define VDUSE_MAX_VQ_GROUPS 2
> >
> > +/*
> > + * Let's make it 2 for simplicity.
> > + */
> > +#define VDUSE_MAX_ASID 2
>
> Similar to previous patch, it's better to increase this otherwise we
> need new uAPI or it requires the userspace to probe the maximum value
> once we decide to change it in the future.
>
I'm ok with this, but what is a good max value? UINT32_MAX seems excessive?
This requires us to allocate arrays for both vduse_dev->domain and
vduse_dev->umem, so we need to set a reasonable value.
> > +
> > #define IRQ_UNBOUND -1
> >
> > struct vduse_virtqueue {
> > @@ -92,7 +97,7 @@ struct vduse_dev {
> > struct vduse_vdpa *vdev;
> > struct device *dev;
> > struct vduse_virtqueue **vqs;
> > - struct vduse_iova_domain *domain;
> > + struct vduse_iova_domain *domain[VDUSE_MAX_ASID];
> > char *name;
> > struct mutex lock;
> > spinlock_t msg_lock;
> > @@ -120,7 +125,8 @@ struct vduse_dev {
> > u32 vq_num;
> > u32 vq_align;
> > u32 ngroups;
> > - struct vduse_umem *umem;
> > + u32 nas;
> > + struct vduse_umem *umem[VDUSE_MAX_ASID];
> > struct mutex mem_lock;
> > unsigned int bounce_size;
> > struct mutex domain_lock;
> > @@ -436,11 +442,14 @@ static __poll_t vduse_dev_poll(struct file *file,
> > poll_table *wait)
> > static void vduse_dev_reset(struct vduse_dev *dev)
> > {
> > int i;
> > - struct vduse_iova_domain *domain = dev->domain;
> >
> > /* The coherent mappings are handled in vduse_dev_free_coherent() */
> > - if (domain && domain->bounce_map)
> > - vduse_domain_reset_bounce_map(domain);
> > + for (i = 0; i < dev->nas; i++) {
> > + struct vduse_iova_domain *domain = dev->domain[i];
> > +
> > + if (domain && domain->bounce_map)
> > + vduse_domain_reset_bounce_map(domain);
> > + }
> >
> > down_write(&dev->rwsem);
> >
> > @@ -617,6 +626,23 @@ static u32 vduse_get_vq_group(struct vdpa_device
> > *vdpa, u16 idx)
> > return msg.resp.vq_group.num;
> > }
> >
> > +static int vduse_set_group_asid(struct vdpa_device *vdpa, unsigned int
> > group,
> > + unsigned int asid)
> > +{
> > + struct vduse_dev *dev = vdpa_to_vduse(vdpa);
> > + struct vduse_dev_msg msg = { 0 };
> > +
> > + if (dev->api_version < VDUSE_API_VERSION_1 ||
> > + group >= dev->ngroups || asid >= dev->nas)
> > + return -EINVAL;
> > +
> > + msg.req.type = VDUSE_SET_VQ_GROUP_ASID;
> > + msg.req.vq_group_asid.group = group;
> > + msg.req.vq_group_asid.asid = asid;
> > +
> > + return vduse_dev_msg_sync(dev, &msg);
> > +}
> > +
> > static int
Re: [RFC 3/6] vduse: add vq group asid support
On Fri, Jun 6, 2025 at 7:50 PM Eugenio Pérez wrote:
>
> Make one IOTLB domain per address space, and allow the driver to assign
> each ASID to a vq group. Each address space via an dedicated identifier
> (ASID).
>
> During vDPA device allocation, the VDUSE device needs to report the
> number of address spaces supported. Then the vdpa driver is able to
> configure them. 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.
>
> TODO: Ideally, umem should not be duplicated. But it is hard or
> impossible to refactor everything around one single umem. So should we
> continue with device specifying umem per vq group?
This is a good question.
I think umem should be bound to address space and umem needs to be isolated.
For the issue of complexity, we can simply extend the vduse_iova_umem
to have an asid field. But it looks like it needs more work as:
struct vduse_iova_umem {
__u64 uaddr;
__u64 iova;
__u64 size;
__u64 reserved[3];
};
Do we have a way to know if reserved is used or not (as we are lacking
a flag field anyhow ).
So we probably need a new uAPI like vduse_iova_umem_v2 that includes a
flag field at least.
>
> Signed-off-by: Eugenio Pérez
> ---
> drivers/vdpa/vdpa_user/vduse_dev.c | 250 +
> include/uapi/linux/vduse.h | 38 -
> 2 files changed, 216 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c
> b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 6fa687bc4912..d51e4f26fe72 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -51,6 +51,11 @@
> */
> #define VDUSE_MAX_VQ_GROUPS 2
>
> +/*
> + * Let's make it 2 for simplicity.
> + */
> +#define VDUSE_MAX_ASID 2
Similar to previous patch, it's better to increase this otherwise we
need new uAPI or it requires the userspace to probe the maximum value
once we decide to change it in the future.
> +
> #define IRQ_UNBOUND -1
>
> struct vduse_virtqueue {
> @@ -92,7 +97,7 @@ struct vduse_dev {
> struct vduse_vdpa *vdev;
> struct device *dev;
> struct vduse_virtqueue **vqs;
> - struct vduse_iova_domain *domain;
> + struct vduse_iova_domain *domain[VDUSE_MAX_ASID];
> char *name;
> struct mutex lock;
> spinlock_t msg_lock;
> @@ -120,7 +125,8 @@ struct vduse_dev {
> u32 vq_num;
> u32 vq_align;
> u32 ngroups;
> - struct vduse_umem *umem;
> + u32 nas;
> + struct vduse_umem *umem[VDUSE_MAX_ASID];
> struct mutex mem_lock;
> unsigned int bounce_size;
> struct mutex domain_lock;
> @@ -436,11 +442,14 @@ static __poll_t vduse_dev_poll(struct file *file,
> poll_table *wait)
> static void vduse_dev_reset(struct vduse_dev *dev)
> {
> int i;
> - struct vduse_iova_domain *domain = dev->domain;
>
> /* The coherent mappings are handled in vduse_dev_free_coherent() */
> - if (domain && domain->bounce_map)
> - vduse_domain_reset_bounce_map(domain);
> + for (i = 0; i < dev->nas; i++) {
> + struct vduse_iova_domain *domain = dev->domain[i];
> +
> + if (domain && domain->bounce_map)
> + vduse_domain_reset_bounce_map(domain);
> + }
>
> down_write(&dev->rwsem);
>
> @@ -617,6 +626,23 @@ static u32 vduse_get_vq_group(struct vdpa_device *vdpa,
> u16 idx)
> return msg.resp.vq_group.num;
> }
>
> +static int vduse_set_group_asid(struct vdpa_device *vdpa, unsigned int group,
> + unsigned int asid)
> +{
> + struct vduse_dev *dev = vdpa_to_vduse(vdpa);
> + struct vduse_dev_msg msg = { 0 };
> +
> + if (dev->api_version < VDUSE_API_VERSION_1 ||
> + group >= dev->ngroups || asid >= dev->nas)
> + return -EINVAL;
> +
> + msg.req.type = VDUSE_SET_VQ_GROUP_ASID;
> + msg.req.vq_group_asid.group = group;
> + msg.req.vq_group_asid.asid = asid;
> +
> + return vduse_dev_msg_sync(dev, &msg);
> +}
> +
> static int vduse_vdpa_get_vq_state(struct vdpa_device *vdpa, u16 idx,
> struct vdpa_vq_state *state)
> {
> @@ -788,13 +814,13 @@ static int vduse_vdpa_set_map(struct vdpa_device *vdpa,
> struct vduse_dev *dev = vdpa_to_vduse(vdpa);
> int ret;
>
> - ret = vduse_domain_set_map(dev->domain, iotlb);
> + ret = vduse_domain_set_map(dev->domain[asid], iotlb);
> if (ret)
> return ret;
>
> ret = vduse_dev_update_iotlb(dev, 0ULL, ULLONG_MAX);
> if (ret) {
> - vduse_domain_clear_map(dev->domain, iotlb);
> + vduse_domain_clear_map(dev->domain[asid], iotlb);
> return ret;
> }
>
>
