Messages in this thread | | | Date | Fri, 24 Mar 2023 15:39:50 +0100 | From | Stefano Garzarella <> | Subject | Re: [PATCH v3 4/8] vringh: support VA with iotlb |
| |
On Thu, Mar 23, 2023 at 03:43:34PM +0100, Eugenio Perez Martin wrote: >On Thu, Mar 23, 2023 at 11:46 AM Stefano Garzarella <sgarzare@redhat.com> wrote: >> >> On Thu, Mar 23, 2023 at 09:09:14AM +0100, Eugenio Perez Martin wrote: >> >On Tue, Mar 21, 2023 at 4:43 PM Stefano Garzarella <sgarzare@redhat.com> wrote: >> >> >> >> vDPA supports the possibility to use user VA in the iotlb messages. >> >> So, let's add support for user VA in vringh to use it in the vDPA >> >> simulators. >> >> >> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> >> --- >> >> >> >> Notes: >> >> v3: >> >> - refactored avoiding code duplication [Eugenio] >> >> v2: >> >> - replace kmap_atomic() with kmap_local_page() [see previous patch] >> >> - fix cast warnings when build with W=1 C=1 >> >> >> >> include/linux/vringh.h | 5 +- >> >> drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +- >> >> drivers/vdpa/vdpa_sim/vdpa_sim.c | 4 +- >> >> drivers/vhost/vringh.c | 153 +++++++++++++++++++++++------- >> >> 4 files changed, 127 insertions(+), 37 deletions(-) >> >> >> >> diff --git a/include/linux/vringh.h b/include/linux/vringh.h >> >> index 1991a02c6431..d39b9f2dcba0 100644 >> >> --- a/include/linux/vringh.h >> >> +++ b/include/linux/vringh.h >> >> @@ -32,6 +32,9 @@ struct vringh { >> >> /* Can we get away with weak barriers? */ >> >> bool weak_barriers; >> >> >> >> + /* Use user's VA */ >> >> + bool use_va; >> >> + >> >> /* Last available index we saw (ie. where we're up to). */ >> >> u16 last_avail_idx; >> >> >> >> @@ -279,7 +282,7 @@ 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, >> >> + unsigned int num, bool weak_barriers, bool use_va, >> >> struct vring_desc *desc, >> >> struct vring_avail *avail, >> >> struct vring_used *used); >> >> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c >> >> index 520646ae7fa0..dfd0e000217b 100644 >> >> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c >> >> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c >> >> @@ -2537,7 +2537,7 @@ static int setup_cvq_vring(struct mlx5_vdpa_dev *mvdev) >> >> >> >> if (mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)) >> >> err = vringh_init_iotlb(&cvq->vring, mvdev->actual_features, >> >> - MLX5_CVQ_MAX_ENT, false, >> >> + MLX5_CVQ_MAX_ENT, false, false, >> >> (struct vring_desc *)(uintptr_t)cvq->desc_addr, >> >> (struct vring_avail *)(uintptr_t)cvq->driver_addr, >> >> (struct vring_used *)(uintptr_t)cvq->device_addr); >> >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c >> >> index eea23c630f7c..47cdf2a1f5b8 100644 >> >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c >> >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c >> >> @@ -60,7 +60,7 @@ 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; >> >> >> >> - vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, true, >> >> + vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, true, false, >> >> (struct vring_desc *)(uintptr_t)vq->desc_addr, >> >> (struct vring_avail *) >> >> (uintptr_t)vq->driver_addr, >> >> @@ -92,7 +92,7 @@ static void vdpasim_vq_reset(struct vdpasim *vdpasim, >> >> vq->cb = NULL; >> >> vq->private = NULL; >> >> vringh_init_iotlb(&vq->vring, vdpasim->dev_attr.supported_features, >> >> - VDPASIM_QUEUE_MAX, false, NULL, NULL, NULL); >> >> + VDPASIM_QUEUE_MAX, false, false, NULL, NULL, NULL); >> >> >> >> vq->vring.notify = NULL; >> >> } >> >> diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c >> >> index 0ba3ef809e48..72c88519329a 100644 >> >> --- a/drivers/vhost/vringh.c >> >> +++ b/drivers/vhost/vringh.c >> >> @@ -1094,10 +1094,18 @@ EXPORT_SYMBOL(vringh_need_notify_kern); >> >> >> >> #if IS_REACHABLE(CONFIG_VHOST_IOTLB) >> >> >> >> +struct iotlb_vec { >> >> + union { >> >> + struct iovec *iovec; >> >> + struct bio_vec *bvec; >> >> + } iov; >> >> + size_t count; >> >> + bool is_iovec; >> >> +}; >> >> + >> >> static int iotlb_translate(const struct vringh *vrh, >> >> u64 addr, u64 len, u64 *translated, >> >> - struct bio_vec iov[], >> >> - int iov_size, u32 perm) >> >> + struct iotlb_vec *ivec, u32 perm) >> >> { >> >> struct vhost_iotlb_map *map; >> >> struct vhost_iotlb *iotlb = vrh->iotlb; >> >> @@ -1107,9 +1115,9 @@ static int iotlb_translate(const struct vringh *vrh, >> >> spin_lock(vrh->iotlb_lock); >> >> >> >> while (len > s) { >> >> - u64 size, pa, pfn; >> >> + u64 size; >> >> >> >> - if (unlikely(ret >= iov_size)) { >> >> + if (unlikely(ret >= ivec->count)) { >> >> ret = -ENOBUFS; >> >> break; >> >> } >> >> @@ -1124,10 +1132,22 @@ static int iotlb_translate(const struct vringh *vrh, >> >> } >> >> >> >> size = map->size - addr + map->start; >> >> - pa = map->addr + addr - map->start; >> >> - pfn = pa >> PAGE_SHIFT; >> >> - bvec_set_page(&iov[ret], pfn_to_page(pfn), min(len - s, size), >> >> - pa & (PAGE_SIZE - 1)); >> >> + if (ivec->is_iovec) { >> >> + struct iovec *iovec = ivec->iov.iovec; >> >> + >> >> + iovec[ret].iov_len = min(len - s, size); >> >> + iovec[ret].iov_base = (void __user *)(unsigned long) >> > >> >s/unsigned long/uintptr_t ? >> > >> >> yep, good catch! >> >> As I wrote to Jason, I think I'll take it out of the if and just declare >> an uintptr_t variable, since I'm using it also in the else branch. >> >> > >> > >> >> + (map->addr + addr - map->start); >> >> + } else { >> >> + u64 pa = map->addr + addr - map->start; >> >> + u64 pfn = pa >> PAGE_SHIFT; >> >> + struct bio_vec *bvec = ivec->iov.bvec; >> >> + >> >> + bvec_set_page(&bvec[ret], pfn_to_page(pfn), >> >> + min(len - s, size), >> >> + pa & (PAGE_SIZE - 1)); >> >> + } >> >> + >> >> s += size; >> >> addr += size; >> >> ++ret; >> >> @@ -1141,26 +1161,42 @@ static int iotlb_translate(const struct vringh *vrh, >> >> return ret; >> >> } >> >> >> >> +#define IOTLB_IOV_SIZE 16 >> > >> >I'm fine with defining here, but maybe it is better to isolate the >> >change in a previous patch or reuse another well known macro? >> >> Yep, good point! >> >> Do you have any well known macro to suggest? >> > >Not really, 16 seems like a convenience value here actually :). Maybe >replace _SIZE with _STRIDE or similar?
Ack, I will add IOTLB_IOV_STRIDE in a preparation patch before this one.
> >I keep the Acked-by even if the final name is IOTLB_IOV_SIZE though.
Thanks, I changed a bit this patch following Jason's and your suggestions.
I'd like an explicit Acked-by on the next version if it is okay with you.
Thanks, Stefano
| |