Stefano Garzarella
2021-Jan-28 14:41 UTC
[PATCH RFC v2 02/10] vringh: add 'iotlb_lock' to synchronize iotlb accesses
Usually iotlb accesses are synchronized with a spinlock.
Let's request it as a new parameter in vringh_set_iotlb() and
hold it when we navigate the iotlb in iotlb_translate() to avoid
race conditions with any new additions/deletions of ranges from
the ioltb.
Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>
---
include/linux/vringh.h | 6 +++++-
drivers/vdpa/vdpa_sim/vdpa_sim.c | 3 ++-
drivers/vhost/vringh.c | 9 ++++++++-
3 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/include/linux/vringh.h b/include/linux/vringh.h
index 59bd50f99291..9c077863c8f6 100644
--- a/include/linux/vringh.h
+++ b/include/linux/vringh.h
@@ -46,6 +46,9 @@ struct vringh {
/* IOTLB for this vring */
struct vhost_iotlb *iotlb;
+ /* spinlock to synchronize IOTLB accesses */
+ spinlock_t *iotlb_lock;
+
/* The function to call to notify the guest about added buffers */
void (*notify)(struct vringh *);
};
@@ -258,7 +261,8 @@ static inline __virtio64 cpu_to_vringh64(const struct vringh
*vrh, u64 val)
#if IS_REACHABLE(CONFIG_VHOST_IOTLB)
-void vringh_set_iotlb(struct vringh *vrh, struct vhost_iotlb *iotlb);
+void vringh_set_iotlb(struct vringh *vrh, struct vhost_iotlb *iotlb,
+ spinlock_t *iotlb_lock);
int vringh_init_iotlb(struct vringh *vrh, u64 features,
unsigned int num, bool weak_barriers,
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index 2183a833fcf4..53238989713d 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -284,7 +284,8 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr
*dev_attr)
goto err_iommu;
for (i = 0; i < dev_attr->nvqs; i++)
- vringh_set_iotlb(&vdpasim->vqs[i].vring, vdpasim->iommu);
+ vringh_set_iotlb(&vdpasim->vqs[i].vring, vdpasim->iommu,
+ &vdpasim->iommu_lock);
ret = iova_cache_get();
if (ret)
diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
index 85d85faba058..f68122705719 100644
--- a/drivers/vhost/vringh.c
+++ b/drivers/vhost/vringh.c
@@ -1074,6 +1074,8 @@ static int iotlb_translate(const struct vringh *vrh,
int ret = 0;
u64 s = 0;
+ spin_lock(vrh->iotlb_lock);
+
while (len > s) {
u64 size, pa, pfn;
@@ -1103,6 +1105,8 @@ static int iotlb_translate(const struct vringh *vrh,
++ret;
}
+ spin_unlock(vrh->iotlb_lock);
+
return ret;
}
@@ -1262,10 +1266,13 @@ EXPORT_SYMBOL(vringh_init_iotlb);
* vringh_set_iotlb - initialize a vringh for a ring with IOTLB.
* @vrh: the vring
* @iotlb: iotlb associated with this vring
+ * @iotlb_lock: spinlock to synchronize the iotlb accesses
*/
-void vringh_set_iotlb(struct vringh *vrh, struct vhost_iotlb *iotlb)
+void vringh_set_iotlb(struct vringh *vrh, struct vhost_iotlb *iotlb,
+ spinlock_t *iotlb_lock)
{
vrh->iotlb = iotlb;
+ vrh->iotlb_lock = iotlb_lock;
}
EXPORT_SYMBOL(vringh_set_iotlb);
--
2.29.2
Jason Wang
2021-Jan-29 07:43 UTC
[PATCH RFC v2 02/10] vringh: add 'iotlb_lock' to synchronize iotlb accesses
On 2021/1/28 ??10:41, Stefano Garzarella wrote:> Usually iotlb accesses are synchronized with a spinlock. > Let's request it as a new parameter in vringh_set_iotlb() and > hold it when we navigate the iotlb in iotlb_translate() to avoid > race conditions with any new additions/deletions of ranges from > the ioltb.Patch looks fine but I wonder if this is the best approach comparing to do locking by the caller. Thanks> > Signed-off-by: Stefano Garzarella <sgarzare at redhat.com> > --- > include/linux/vringh.h | 6 +++++- > drivers/vdpa/vdpa_sim/vdpa_sim.c | 3 ++- > drivers/vhost/vringh.c | 9 ++++++++- > 3 files changed, 15 insertions(+), 3 deletions(-) > > diff --git a/include/linux/vringh.h b/include/linux/vringh.h > index 59bd50f99291..9c077863c8f6 100644 > --- a/include/linux/vringh.h > +++ b/include/linux/vringh.h > @@ -46,6 +46,9 @@ struct vringh { > /* IOTLB for this vring */ > struct vhost_iotlb *iotlb; > > + /* spinlock to synchronize IOTLB accesses */ > + spinlock_t *iotlb_lock; > + > /* The function to call to notify the guest about added buffers */ > void (*notify)(struct vringh *); > }; > @@ -258,7 +261,8 @@ static inline __virtio64 cpu_to_vringh64(const struct vringh *vrh, u64 val) > > #if IS_REACHABLE(CONFIG_VHOST_IOTLB) > > -void vringh_set_iotlb(struct vringh *vrh, struct vhost_iotlb *iotlb); > +void vringh_set_iotlb(struct vringh *vrh, struct vhost_iotlb *iotlb, > + spinlock_t *iotlb_lock); > > int vringh_init_iotlb(struct vringh *vrh, u64 features, > unsigned int num, bool weak_barriers, > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c > index 2183a833fcf4..53238989713d 100644 > --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c > @@ -284,7 +284,8 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *dev_attr) > goto err_iommu; > > for (i = 0; i < dev_attr->nvqs; i++) > - vringh_set_iotlb(&vdpasim->vqs[i].vring, vdpasim->iommu); > + vringh_set_iotlb(&vdpasim->vqs[i].vring, vdpasim->iommu, > + &vdpasim->iommu_lock); > > ret = iova_cache_get(); > if (ret) > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c > index 85d85faba058..f68122705719 100644 > --- a/drivers/vhost/vringh.c > +++ b/drivers/vhost/vringh.c > @@ -1074,6 +1074,8 @@ static int iotlb_translate(const struct vringh *vrh, > int ret = 0; > u64 s = 0; > > + spin_lock(vrh->iotlb_lock); > + > while (len > s) { > u64 size, pa, pfn; > > @@ -1103,6 +1105,8 @@ static int iotlb_translate(const struct vringh *vrh, > ++ret; > } > > + spin_unlock(vrh->iotlb_lock); > + > return ret; > } > > @@ -1262,10 +1266,13 @@ EXPORT_SYMBOL(vringh_init_iotlb); > * vringh_set_iotlb - initialize a vringh for a ring with IOTLB. > * @vrh: the vring > * @iotlb: iotlb associated with this vring > + * @iotlb_lock: spinlock to synchronize the iotlb accesses > */ > -void vringh_set_iotlb(struct vringh *vrh, struct vhost_iotlb *iotlb) > +void vringh_set_iotlb(struct vringh *vrh, struct vhost_iotlb *iotlb, > + spinlock_t *iotlb_lock) > { > vrh->iotlb = iotlb; > + vrh->iotlb_lock = iotlb_lock; > } > EXPORT_SYMBOL(vringh_set_iotlb); >