| Message ID | 20230302113421.174582-9-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | vdpa_sim: add support for user VA | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Thu, Mar 2, 2023 at 7:35 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > The new "use_va" module parameter (default: false) is used in > vdpa_alloc_device() to inform the vDPA framework that the device > supports VA. > > vringh is initialized to use VA only when "use_va" is true and the > user's mm has been bound. So, only when the bus supports user VA > (e.g. vhost-vdpa). > > vdpasim_mm_work_fn work is used to attach the kthread to the user > address space when the .bind_mm callback is invoked, and to detach > it when the .unbind_mm callback is invoked. > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > > Notes: > v2: > - `use_va` set to true by default [Eugenio] > - supported the new unbind_mm callback [Jason] > - removed the unbind_mm call in vdpasim_do_reset() [Jason] > - avoided to release the lock while call kthread_flush_work() since we > are now using a mutex to protect the device state > > drivers/vdpa/vdpa_sim/vdpa_sim.h | 1 + > drivers/vdpa/vdpa_sim/vdpa_sim.c | 98 +++++++++++++++++++++++++++++++- > 2 files changed, 97 insertions(+), 2 deletions(-) > > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h > index 4774292fba8c..3a42887d05d9 100644 > --- a/drivers/vdpa/vdpa_sim/vdpa_sim.h > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h > @@ -59,6 +59,7 @@ struct vdpasim { > struct vdpasim_virtqueue *vqs; > struct kthread_worker *worker; > struct kthread_work work; > + struct mm_struct *mm_bound; > struct vdpasim_dev_attr dev_attr; > /* mutex to synchronize virtqueue state */ > struct mutex mutex; > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c > index a28103a67ae7..eda26bc33df5 100644 > --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c > @@ -35,10 +35,77 @@ module_param(max_iotlb_entries, int, 0444); > MODULE_PARM_DESC(max_iotlb_entries, > "Maximum number of iotlb entries for each address space. 0 means unlimited. (default: 2048)"); > > +static bool use_va = true; > +module_param(use_va, bool, 0444); > +MODULE_PARM_DESC(use_va, "Enable/disable the device's ability to use VA"); > + > #define VDPASIM_QUEUE_ALIGN PAGE_SIZE > #define VDPASIM_QUEUE_MAX 256 > #define VDPASIM_VENDOR_ID 0 > > +struct vdpasim_mm_work { > + struct kthread_work work; > + struct mm_struct *mm; > + bool bind; > + int ret; > +}; > + > +static void vdpasim_mm_work_fn(struct kthread_work *work) > +{ > + struct vdpasim_mm_work *mm_work = > + container_of(work, struct vdpasim_mm_work, work); > + > + mm_work->ret = 0; > + > + if (mm_work->bind) { > + kthread_use_mm(mm_work->mm); > + //TODO: should we attach the cgroup of the mm owner? > + } else { > + kthread_unuse_mm(mm_work->mm); > + } > +} > + > +static void vdpasim_worker_queue_mm(struct vdpasim *vdpasim, > + struct vdpasim_mm_work *mm_work) > +{ Nit: we need to tweak the name as it does flush besides queuing the work. > + struct kthread_work *work = &mm_work->work; > + > + kthread_init_work(work, vdpasim_mm_work_fn); > + kthread_queue_work(vdpasim->worker, work); > + > + kthread_flush_work(work); > +} > + > +static int vdpasim_worker_bind_mm(struct vdpasim *vdpasim, > + struct mm_struct *new_mm) > +{ > + struct vdpasim_mm_work mm_work; > + > + mm_work.mm = new_mm; > + mm_work.bind = true; > + > + vdpasim_worker_queue_mm(vdpasim, &mm_work); > + > + if (!mm_work.ret) > + vdpasim->mm_bound = new_mm; > + > + return mm_work.ret; > +} > + > +static void vdpasim_worker_unbind_mm(struct vdpasim *vdpasim) > +{ > + struct vdpasim_mm_work mm_work; > + > + if (!vdpasim->mm_bound) > + return; > + > + mm_work.mm = vdpasim->mm_bound; > + mm_work.bind = false; Can we simply use mm_work.mm = NULL for unbinding? > + > + vdpasim_worker_queue_mm(vdpasim, &mm_work); > + > + vdpasim->mm_bound = NULL; And change the mm_bound in the worker? Thanks > +} > static struct vdpasim *vdpa_to_sim(struct vdpa_device *vdpa) > { > return container_of(vdpa, struct vdpasim, vdpa); > @@ -59,8 +126,10 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx) > { > struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx]; > uint16_t last_avail_idx = vq->vring.last_avail_idx; > + bool va_enabled = use_va && vdpasim->mm_bound; > > - vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, true, false, > + vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, true, > + va_enabled, > (struct vring_desc *)(uintptr_t)vq->desc_addr, > (struct vring_avail *) > (uintptr_t)vq->driver_addr, > @@ -151,7 +220,7 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *dev_attr, > vdpa = __vdpa_alloc_device(NULL, ops, > dev_attr->ngroups, dev_attr->nas, > dev_attr->alloc_size, > - dev_attr->name, false); > + dev_attr->name, use_va); > if (IS_ERR(vdpa)) { > ret = PTR_ERR(vdpa); > goto err_alloc; > @@ -571,6 +640,27 @@ static int vdpasim_set_map(struct vdpa_device *vdpa, unsigned int asid, > return ret; > } > > +static int vdpasim_bind_mm(struct vdpa_device *vdpa, struct mm_struct *mm) > +{ > + struct vdpasim *vdpasim = vdpa_to_sim(vdpa); > + int ret; > + > + mutex_lock(&vdpasim->mutex); > + ret = vdpasim_worker_bind_mm(vdpasim, mm); > + mutex_unlock(&vdpasim->mutex); > + > + return ret; > +} > + > +static void vdpasim_unbind_mm(struct vdpa_device *vdpa) > +{ > + struct vdpasim *vdpasim = vdpa_to_sim(vdpa); > + > + mutex_lock(&vdpasim->mutex); > + vdpasim_worker_unbind_mm(vdpasim); > + mutex_unlock(&vdpasim->mutex); > +} > + > static int vdpasim_dma_map(struct vdpa_device *vdpa, unsigned int asid, > u64 iova, u64 size, > u64 pa, u32 perm, void *opaque) > @@ -667,6 +757,8 @@ static const struct vdpa_config_ops vdpasim_config_ops = { > .set_group_asid = vdpasim_set_group_asid, > .dma_map = vdpasim_dma_map, > .dma_unmap = vdpasim_dma_unmap, > + .bind_mm = vdpasim_bind_mm, > + .unbind_mm = vdpasim_unbind_mm, > .free = vdpasim_free, > }; > > @@ -701,6 +793,8 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = { > .get_iova_range = vdpasim_get_iova_range, > .set_group_asid = vdpasim_set_group_asid, > .set_map = vdpasim_set_map, > + .bind_mm = vdpasim_bind_mm, > + .unbind_mm = vdpasim_unbind_mm, > .free = vdpasim_free, > }; > > -- > 2.39.2 >
On Tue, Mar 14, 2023 at 01:36:13PM +0800, Jason Wang wrote: >On Thu, Mar 2, 2023 at 7:35 PM Stefano Garzarella <sgarzare@redhat.com> wrote: >> >> The new "use_va" module parameter (default: false) is used in >> vdpa_alloc_device() to inform the vDPA framework that the device >> supports VA. >> >> vringh is initialized to use VA only when "use_va" is true and the >> user's mm has been bound. So, only when the bus supports user VA >> (e.g. vhost-vdpa). >> >> vdpasim_mm_work_fn work is used to attach the kthread to the user >> address space when the .bind_mm callback is invoked, and to detach >> it when the .unbind_mm callback is invoked. >> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> --- >> >> Notes: >> v2: >> - `use_va` set to true by default [Eugenio] >> - supported the new unbind_mm callback [Jason] >> - removed the unbind_mm call in vdpasim_do_reset() [Jason] >> - avoided to release the lock while call kthread_flush_work() since we >> are now using a mutex to protect the device state >> >> drivers/vdpa/vdpa_sim/vdpa_sim.h | 1 + >> drivers/vdpa/vdpa_sim/vdpa_sim.c | 98 +++++++++++++++++++++++++++++++- >> 2 files changed, 97 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h >> index 4774292fba8c..3a42887d05d9 100644 >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.h >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h >> @@ -59,6 +59,7 @@ struct vdpasim { >> struct vdpasim_virtqueue *vqs; >> struct kthread_worker *worker; >> struct kthread_work work; >> + struct mm_struct *mm_bound; >> struct vdpasim_dev_attr dev_attr; >> /* mutex to synchronize virtqueue state */ >> struct mutex mutex; >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c >> index a28103a67ae7..eda26bc33df5 100644 >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c >> @@ -35,10 +35,77 @@ module_param(max_iotlb_entries, int, 0444); >> MODULE_PARM_DESC(max_iotlb_entries, >> "Maximum number of iotlb entries for each address space. 0 means unlimited. (default: 2048)"); >> >> +static bool use_va = true; >> +module_param(use_va, bool, 0444); >> +MODULE_PARM_DESC(use_va, "Enable/disable the device's ability to use VA"); >> + >> #define VDPASIM_QUEUE_ALIGN PAGE_SIZE >> #define VDPASIM_QUEUE_MAX 256 >> #define VDPASIM_VENDOR_ID 0 >> >> +struct vdpasim_mm_work { >> + struct kthread_work work; >> + struct mm_struct *mm; >> + bool bind; >> + int ret; >> +}; >> + >> +static void vdpasim_mm_work_fn(struct kthread_work *work) >> +{ >> + struct vdpasim_mm_work *mm_work = >> + container_of(work, struct vdpasim_mm_work, work); >> + >> + mm_work->ret = 0; >> + >> + if (mm_work->bind) { >> + kthread_use_mm(mm_work->mm); >> + //TODO: should we attach the cgroup of the mm owner? >> + } else { >> + kthread_unuse_mm(mm_work->mm); >> + } >> +} >> + >> +static void vdpasim_worker_queue_mm(struct vdpasim *vdpasim, >> + struct vdpasim_mm_work *mm_work) >> +{ > >Nit: we need to tweak the name as it does flush besides queuing the work. Yep, or split in 2 functions. > >> + struct kthread_work *work = &mm_work->work; >> + >> + kthread_init_work(work, vdpasim_mm_work_fn); >> + kthread_queue_work(vdpasim->worker, work); >> + >> + kthread_flush_work(work); >> +} >> + >> +static int vdpasim_worker_bind_mm(struct vdpasim *vdpasim, >> + struct mm_struct *new_mm) >> +{ >> + struct vdpasim_mm_work mm_work; >> + >> + mm_work.mm = new_mm; >> + mm_work.bind = true; >> + >> + vdpasim_worker_queue_mm(vdpasim, &mm_work); >> + >> + if (!mm_work.ret) >> + vdpasim->mm_bound = new_mm; >> + >> + return mm_work.ret; >> +} >> + >> +static void vdpasim_worker_unbind_mm(struct vdpasim *vdpasim) >> +{ >> + struct vdpasim_mm_work mm_work; >> + >> + if (!vdpasim->mm_bound) >> + return; >> + >> + mm_work.mm = vdpasim->mm_bound; >> + mm_work.bind = false; > >Can we simply use mm_work.mm = NULL for unbinding? > >> + >> + vdpasim_worker_queue_mm(vdpasim, &mm_work); >> + >> + vdpasim->mm_bound = NULL; > >And change the mm_bound in the worker? Yep, I need to put `vdpasim` in struct vdpasim_mm_work. I'll do in the next version. Thanks, Stefano
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h index 4774292fba8c..3a42887d05d9 100644 --- a/drivers/vdpa/vdpa_sim/vdpa_sim.h +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h @@ -59,6 +59,7 @@ struct vdpasim { struct vdpasim_virtqueue *vqs; struct kthread_worker *worker; struct kthread_work work; + struct mm_struct *mm_bound; struct vdpasim_dev_attr dev_attr; /* mutex to synchronize virtqueue state */ struct mutex mutex; diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c index a28103a67ae7..eda26bc33df5 100644 --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c @@ -35,10 +35,77 @@ module_param(max_iotlb_entries, int, 0444); MODULE_PARM_DESC(max_iotlb_entries, "Maximum number of iotlb entries for each address space. 0 means unlimited. (default: 2048)"); +static bool use_va = true; +module_param(use_va, bool, 0444); +MODULE_PARM_DESC(use_va, "Enable/disable the device's ability to use VA"); + #define VDPASIM_QUEUE_ALIGN PAGE_SIZE #define VDPASIM_QUEUE_MAX 256 #define VDPASIM_VENDOR_ID 0 +struct vdpasim_mm_work { + struct kthread_work work; + struct mm_struct *mm; + bool bind; + int ret; +}; + +static void vdpasim_mm_work_fn(struct kthread_work *work) +{ + struct vdpasim_mm_work *mm_work = + container_of(work, struct vdpasim_mm_work, work); + + mm_work->ret = 0; + + if (mm_work->bind) { + kthread_use_mm(mm_work->mm); + //TODO: should we attach the cgroup of the mm owner? + } else { + kthread_unuse_mm(mm_work->mm); + } +} + +static void vdpasim_worker_queue_mm(struct vdpasim *vdpasim, + struct vdpasim_mm_work *mm_work) +{ + struct kthread_work *work = &mm_work->work; + + kthread_init_work(work, vdpasim_mm_work_fn); + kthread_queue_work(vdpasim->worker, work); + + kthread_flush_work(work); +} + +static int vdpasim_worker_bind_mm(struct vdpasim *vdpasim, + struct mm_struct *new_mm) +{ + struct vdpasim_mm_work mm_work; + + mm_work.mm = new_mm; + mm_work.bind = true; + + vdpasim_worker_queue_mm(vdpasim, &mm_work); + + if (!mm_work.ret) + vdpasim->mm_bound = new_mm; + + return mm_work.ret; +} + +static void vdpasim_worker_unbind_mm(struct vdpasim *vdpasim) +{ + struct vdpasim_mm_work mm_work; + + if (!vdpasim->mm_bound) + return; + + mm_work.mm = vdpasim->mm_bound; + mm_work.bind = false; + + vdpasim_worker_queue_mm(vdpasim, &mm_work); + + vdpasim->mm_bound = NULL; +} static struct vdpasim *vdpa_to_sim(struct vdpa_device *vdpa) { return container_of(vdpa, struct vdpasim, vdpa); @@ -59,8 +126,10 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx) { struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx]; uint16_t last_avail_idx = vq->vring.last_avail_idx; + bool va_enabled = use_va && vdpasim->mm_bound; - vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, true, false, + vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, true, + va_enabled, (struct vring_desc *)(uintptr_t)vq->desc_addr, (struct vring_avail *) (uintptr_t)vq->driver_addr, @@ -151,7 +220,7 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *dev_attr, vdpa = __vdpa_alloc_device(NULL, ops, dev_attr->ngroups, dev_attr->nas, dev_attr->alloc_size, - dev_attr->name, false); + dev_attr->name, use_va); if (IS_ERR(vdpa)) { ret = PTR_ERR(vdpa); goto err_alloc; @@ -571,6 +640,27 @@ static int vdpasim_set_map(struct vdpa_device *vdpa, unsigned int asid, return ret; } +static int vdpasim_bind_mm(struct vdpa_device *vdpa, struct mm_struct *mm) +{ + struct vdpasim *vdpasim = vdpa_to_sim(vdpa); + int ret; + + mutex_lock(&vdpasim->mutex); + ret = vdpasim_worker_bind_mm(vdpasim, mm); + mutex_unlock(&vdpasim->mutex); + + return ret; +} + +static void vdpasim_unbind_mm(struct vdpa_device *vdpa) +{ + struct vdpasim *vdpasim = vdpa_to_sim(vdpa); + + mutex_lock(&vdpasim->mutex); + vdpasim_worker_unbind_mm(vdpasim); + mutex_unlock(&vdpasim->mutex); +} + static int vdpasim_dma_map(struct vdpa_device *vdpa, unsigned int asid, u64 iova, u64 size, u64 pa, u32 perm, void *opaque) @@ -667,6 +757,8 @@ static const struct vdpa_config_ops vdpasim_config_ops = { .set_group_asid = vdpasim_set_group_asid, .dma_map = vdpasim_dma_map, .dma_unmap = vdpasim_dma_unmap, + .bind_mm = vdpasim_bind_mm, + .unbind_mm = vdpasim_unbind_mm, .free = vdpasim_free, }; @@ -701,6 +793,8 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = { .get_iova_range = vdpasim_get_iova_range, .set_group_asid = vdpasim_set_group_asid, .set_map = vdpasim_set_map, + .bind_mm = vdpasim_bind_mm, + .unbind_mm = vdpasim_unbind_mm, .free = vdpasim_free, };
The new "use_va" module parameter (default: false) is used in vdpa_alloc_device() to inform the vDPA framework that the device supports VA. vringh is initialized to use VA only when "use_va" is true and the user's mm has been bound. So, only when the bus supports user VA (e.g. vhost-vdpa). vdpasim_mm_work_fn work is used to attach the kthread to the user address space when the .bind_mm callback is invoked, and to detach it when the .unbind_mm callback is invoked. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- Notes: v2: - `use_va` set to true by default [Eugenio] - supported the new unbind_mm callback [Jason] - removed the unbind_mm call in vdpasim_do_reset() [Jason] - avoided to release the lock while call kthread_flush_work() since we are now using a mutex to protect the device state drivers/vdpa/vdpa_sim/vdpa_sim.h | 1 + drivers/vdpa/vdpa_sim/vdpa_sim.c | 98 +++++++++++++++++++++++++++++++- 2 files changed, 97 insertions(+), 2 deletions(-)