* [PATCH v2 0/3] vhost_vdpa: better PACKED support
@ 2023-04-24 22:50 Shannon Nelson via Virtualization
2023-04-24 22:50 ` [PATCH v2 1/3] vhost_vdpa: tell vqs about the negotiated Shannon Nelson via Virtualization
` (3 more replies)
0 siblings, 4 replies; 22+ messages in thread
From: Shannon Nelson via Virtualization @ 2023-04-24 22:50 UTC (permalink / raw)
To: jasowang, mst, virtualization; +Cc: drivers
While testing our vDPA driver with QEMU we found that vhost_vdpa was
missing some support for PACKED VQs. Adding these helped us get further
in our testing. The first patch makes sure that the vhost_vdpa vqs are
given the feature flags, as done in other vhost variants. The second
and third patches use the feature flags to properly format and pass
set/get_vq requests.
v1:
- included wrap counter in packing answers for VHOST_GET_VRING_BASE
- added comments to vhost_virtqueue fields last_avail_idx and last_used_idx
v0:
https://lists.linuxfoundation.org/pipermail/virtualization/2023-April/066253.html
- initial version
Shannon Nelson (3):
vhost_vdpa: tell vqs about the negotiated
vhost: support PACKED when setting-getting vring_base
vhost_vdpa: support PACKED when setting-getting vring_base
drivers/vhost/vdpa.c | 34 ++++++++++++++++++++++++++++++----
drivers/vhost/vhost.c | 18 +++++++++++++-----
drivers/vhost/vhost.h | 8 ++++++--
3 files changed, 49 insertions(+), 11 deletions(-)
--
2.17.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH v2 1/3] vhost_vdpa: tell vqs about the negotiated 2023-04-24 22:50 [PATCH v2 0/3] vhost_vdpa: better PACKED support Shannon Nelson via Virtualization @ 2023-04-24 22:50 ` Shannon Nelson via Virtualization 2023-04-24 22:50 ` [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base Shannon Nelson via Virtualization ` (2 subsequent siblings) 3 siblings, 0 replies; 22+ messages in thread From: Shannon Nelson via Virtualization @ 2023-04-24 22:50 UTC (permalink / raw) To: jasowang, mst, virtualization; +Cc: drivers As is done in the net, iscsi, and vsock vhost support, let the vdpa vqs know about the features that have been negotiated. This allows vhost to more safely make decisions based on the features, such as when using PACKED vs split queues. Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> Acked-by: Jason Wang <jasowang@redhat.com> --- drivers/vhost/vdpa.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index 7be9d9d8f01c..599b8cc238c7 100644 --- a/drivers/vhost/vdpa.c +++ b/drivers/vhost/vdpa.c @@ -385,7 +385,10 @@ static long vhost_vdpa_set_features(struct vhost_vdpa *v, u64 __user *featurep) { struct vdpa_device *vdpa = v->vdpa; const struct vdpa_config_ops *ops = vdpa->config; + struct vhost_dev *d = &v->vdev; + u64 actual_features; u64 features; + int i; /* * It's not allowed to change the features after they have @@ -400,6 +403,16 @@ static long vhost_vdpa_set_features(struct vhost_vdpa *v, u64 __user *featurep) if (vdpa_set_features(vdpa, features)) return -EINVAL; + /* let the vqs know what has been configured */ + actual_features = ops->get_driver_features(vdpa); + for (i = 0; i < d->nvqs; ++i) { + struct vhost_virtqueue *vq = d->vqs[i]; + + mutex_lock(&vq->mutex); + vq->acked_features = actual_features; + mutex_unlock(&vq->mutex); + } + return 0; } -- 2.17.1 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base 2023-04-24 22:50 [PATCH v2 0/3] vhost_vdpa: better PACKED support Shannon Nelson via Virtualization 2023-04-24 22:50 ` [PATCH v2 1/3] vhost_vdpa: tell vqs about the negotiated Shannon Nelson via Virtualization @ 2023-04-24 22:50 ` Shannon Nelson via Virtualization 2023-04-25 6:01 ` Jason Wang 2023-05-09 8:46 ` Stefano Garzarella 2023-04-24 22:50 ` [PATCH v2 3/3] vhost_vdpa: " Shannon Nelson via Virtualization 2023-04-25 6:08 ` [PATCH v2 0/3] vhost_vdpa: better PACKED support Michael S. Tsirkin 3 siblings, 2 replies; 22+ messages in thread From: Shannon Nelson via Virtualization @ 2023-04-24 22:50 UTC (permalink / raw) To: jasowang, mst, virtualization; +Cc: drivers Use the right structs for PACKED or split vqs when setting and getting the vring base. Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> --- drivers/vhost/vhost.c | 18 +++++++++++++----- drivers/vhost/vhost.h | 8 ++++++-- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index f11bdbe4c2c5..f64efda48f21 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -1633,17 +1633,25 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *arg r = -EFAULT; break; } - if (s.num > 0xffff) { - r = -EINVAL; - break; + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { + vq->last_avail_idx = s.num & 0xffff; + vq->last_used_idx = (s.num >> 16) & 0xffff; + } else { + if (s.num > 0xffff) { + r = -EINVAL; + break; + } + vq->last_avail_idx = s.num; } - vq->last_avail_idx = s.num; /* Forget the cached index value. */ vq->avail_idx = vq->last_avail_idx; break; case VHOST_GET_VRING_BASE: s.index = idx; - s.num = vq->last_avail_idx; + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) + s.num = (u32)vq->last_avail_idx | ((u32)vq->last_used_idx << 16); + else + s.num = vq->last_avail_idx; if (copy_to_user(argp, &s, sizeof s)) r = -EFAULT; break; diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h index 1647b750169c..6f73f29d5979 100644 --- a/drivers/vhost/vhost.h +++ b/drivers/vhost/vhost.h @@ -85,13 +85,17 @@ struct vhost_virtqueue { /* The routine to call when the Guest pings us, or timeout. */ vhost_work_fn_t handle_kick; - /* Last available index we saw. */ + /* Last available index we saw. + * Values are limited to 0x7fff, and the high bit is used as + * a wrap counter when using VIRTIO_F_RING_PACKED. */ u16 last_avail_idx; /* Caches available index value from user. */ u16 avail_idx; - /* Last index we used. */ + /* Last index we used. + * Values are limited to 0x7fff, and the high bit is used as + * a wrap counter when using VIRTIO_F_RING_PACKED. */ u16 last_used_idx; /* Used flags */ -- 2.17.1 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base 2023-04-24 22:50 ` [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base Shannon Nelson via Virtualization @ 2023-04-25 6:01 ` Jason Wang 2023-05-09 8:46 ` Stefano Garzarella 1 sibling, 0 replies; 22+ messages in thread From: Jason Wang @ 2023-04-25 6:01 UTC (permalink / raw) To: Shannon Nelson; +Cc: drivers, virtualization, mst On Tue, Apr 25, 2023 at 6:50 AM Shannon Nelson <shannon.nelson@amd.com> wrote: > > Use the right structs for PACKED or split vqs when setting and > getting the vring base. > > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> Acked-by: Jason Wang <jasowang@redhat.com> Thanks > --- > drivers/vhost/vhost.c | 18 +++++++++++++----- > drivers/vhost/vhost.h | 8 ++++++-- > 2 files changed, 19 insertions(+), 7 deletions(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index f11bdbe4c2c5..f64efda48f21 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -1633,17 +1633,25 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *arg > r = -EFAULT; > break; > } > - if (s.num > 0xffff) { > - r = -EINVAL; > - break; > + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { > + vq->last_avail_idx = s.num & 0xffff; > + vq->last_used_idx = (s.num >> 16) & 0xffff; > + } else { > + if (s.num > 0xffff) { > + r = -EINVAL; > + break; > + } > + vq->last_avail_idx = s.num; > } > - vq->last_avail_idx = s.num; > /* Forget the cached index value. */ > vq->avail_idx = vq->last_avail_idx; > break; > case VHOST_GET_VRING_BASE: > s.index = idx; > - s.num = vq->last_avail_idx; > + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) > + s.num = (u32)vq->last_avail_idx | ((u32)vq->last_used_idx << 16); > + else > + s.num = vq->last_avail_idx; > if (copy_to_user(argp, &s, sizeof s)) > r = -EFAULT; > break; > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h > index 1647b750169c..6f73f29d5979 100644 > --- a/drivers/vhost/vhost.h > +++ b/drivers/vhost/vhost.h > @@ -85,13 +85,17 @@ struct vhost_virtqueue { > /* The routine to call when the Guest pings us, or timeout. */ > vhost_work_fn_t handle_kick; > > - /* Last available index we saw. */ > + /* Last available index we saw. > + * Values are limited to 0x7fff, and the high bit is used as > + * a wrap counter when using VIRTIO_F_RING_PACKED. */ > u16 last_avail_idx; > > /* Caches available index value from user. */ > u16 avail_idx; > > - /* Last index we used. */ > + /* Last index we used. > + * Values are limited to 0x7fff, and the high bit is used as > + * a wrap counter when using VIRTIO_F_RING_PACKED. */ > u16 last_used_idx; > > /* Used flags */ > -- > 2.17.1 > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base 2023-04-24 22:50 ` [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base Shannon Nelson via Virtualization 2023-04-25 6:01 ` Jason Wang @ 2023-05-09 8:46 ` Stefano Garzarella 2023-05-15 20:41 ` Shannon Nelson via Virtualization 1 sibling, 1 reply; 22+ messages in thread From: Stefano Garzarella @ 2023-05-09 8:46 UTC (permalink / raw) To: Shannon Nelson; +Cc: virtualization, drivers, mst On Mon, Apr 24, 2023 at 03:50:30PM -0700, Shannon Nelson via Virtualization wrote: >Use the right structs for PACKED or split vqs when setting and >getting the vring base. > >Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> >--- > drivers/vhost/vhost.c | 18 +++++++++++++----- > drivers/vhost/vhost.h | 8 ++++++-- > 2 files changed, 19 insertions(+), 7 deletions(-) > >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >index f11bdbe4c2c5..f64efda48f21 100644 >--- a/drivers/vhost/vhost.c >+++ b/drivers/vhost/vhost.c >@@ -1633,17 +1633,25 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *arg > r = -EFAULT; > break; > } >- if (s.num > 0xffff) { >- r = -EINVAL; >- break; >+ if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { >+ vq->last_avail_idx = s.num & 0xffff; >+ vq->last_used_idx = (s.num >> 16) & 0xffff; >+ } else { >+ if (s.num > 0xffff) { >+ r = -EINVAL; >+ break; >+ } >+ vq->last_avail_idx = s.num; > } >- vq->last_avail_idx = s.num; > /* Forget the cached index value. */ > vq->avail_idx = vq->last_avail_idx; > break; > case VHOST_GET_VRING_BASE: > s.index = idx; >- s.num = vq->last_avail_idx; >+ if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) >+ s.num = (u32)vq->last_avail_idx | ((u32)vq->last_used_idx << 16); >+ else >+ s.num = vq->last_avail_idx; The changes LGTM, but since we are changing the UAPI, should we update the documentation of VHOST_SET_VRING_BASE and VHOST_GET_VRING_BASE in include/uapi/linux/vhost.h? Thanks, Stefano _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base 2023-05-09 8:46 ` Stefano Garzarella @ 2023-05-15 20:41 ` Shannon Nelson via Virtualization 2023-05-16 7:49 ` Stefano Garzarella 0 siblings, 1 reply; 22+ messages in thread From: Shannon Nelson via Virtualization @ 2023-05-15 20:41 UTC (permalink / raw) To: Stefano Garzarella; +Cc: virtualization, drivers, mst On 5/9/23 1:46 AM, Stefano Garzarella wrote: > On Mon, Apr 24, 2023 at 03:50:30PM -0700, Shannon Nelson via > Virtualization wrote: >> Use the right structs for PACKED or split vqs when setting and >> getting the vring base. >> >> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> >> --- >> drivers/vhost/vhost.c | 18 +++++++++++++----- >> drivers/vhost/vhost.h | 8 ++++++-- >> 2 files changed, 19 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >> index f11bdbe4c2c5..f64efda48f21 100644 >> --- a/drivers/vhost/vhost.c >> +++ b/drivers/vhost/vhost.c >> @@ -1633,17 +1633,25 @@ long vhost_vring_ioctl(struct vhost_dev *d, >> unsigned int ioctl, void __user *arg >> r = -EFAULT; >> break; >> } >> - if (s.num > 0xffff) { >> - r = -EINVAL; >> - break; >> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { >> + vq->last_avail_idx = s.num & 0xffff; >> + vq->last_used_idx = (s.num >> 16) & 0xffff; >> + } else { >> + if (s.num > 0xffff) { >> + r = -EINVAL; >> + break; >> + } >> + vq->last_avail_idx = s.num; >> } >> - vq->last_avail_idx = s.num; >> /* Forget the cached index value. */ >> vq->avail_idx = vq->last_avail_idx; >> break; >> case VHOST_GET_VRING_BASE: >> s.index = idx; >> - s.num = vq->last_avail_idx; >> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) >> + s.num = (u32)vq->last_avail_idx | >> ((u32)vq->last_used_idx << 16); >> + else >> + s.num = vq->last_avail_idx; > > The changes LGTM, but since we are changing the UAPI, should we > update the documentation of VHOST_SET_VRING_BASE and > VHOST_GET_VRING_BASE in include/uapi/linux/vhost.h? Correct me if I'm wrong, but I don't think we're changing anything in the UAPI here, just fixing code to work correctly with what is already happening. sln > > Thanks, > Stefano > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base 2023-05-15 20:41 ` Shannon Nelson via Virtualization @ 2023-05-16 7:49 ` Stefano Garzarella 2023-05-16 18:26 ` Shannon Nelson via Virtualization 0 siblings, 1 reply; 22+ messages in thread From: Stefano Garzarella @ 2023-05-16 7:49 UTC (permalink / raw) To: Shannon Nelson; +Cc: virtualization, drivers, mst On Mon, May 15, 2023 at 01:41:12PM -0700, Shannon Nelson wrote: >On 5/9/23 1:46 AM, Stefano Garzarella wrote: >>On Mon, Apr 24, 2023 at 03:50:30PM -0700, Shannon Nelson via >>Virtualization wrote: >>>Use the right structs for PACKED or split vqs when setting and >>>getting the vring base. >>> >>>Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> >>>--- >>>drivers/vhost/vhost.c | 18 +++++++++++++----- >>>drivers/vhost/vhost.h | 8 ++++++-- >>>2 files changed, 19 insertions(+), 7 deletions(-) >>> >>>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >>>index f11bdbe4c2c5..f64efda48f21 100644 >>>--- a/drivers/vhost/vhost.c >>>+++ b/drivers/vhost/vhost.c >>>@@ -1633,17 +1633,25 @@ long vhost_vring_ioctl(struct vhost_dev >>>*d, unsigned int ioctl, void __user *arg >>> r = -EFAULT; >>> break; >>> } >>>- if (s.num > 0xffff) { >>>- r = -EINVAL; >>>- break; >>>+ if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { >>>+ vq->last_avail_idx = s.num & 0xffff; >>>+ vq->last_used_idx = (s.num >> 16) & 0xffff; >>>+ } else { >>>+ if (s.num > 0xffff) { >>>+ r = -EINVAL; >>>+ break; >>>+ } >>>+ vq->last_avail_idx = s.num; >>> } >>>- vq->last_avail_idx = s.num; >>> /* Forget the cached index value. */ >>> vq->avail_idx = vq->last_avail_idx; >>> break; >>> case VHOST_GET_VRING_BASE: >>> s.index = idx; >>>- s.num = vq->last_avail_idx; >>>+ if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) >>>+ s.num = (u32)vq->last_avail_idx | >>>((u32)vq->last_used_idx << 16); >>>+ else >>>+ s.num = vq->last_avail_idx; >> >>The changes LGTM, but since we are changing the UAPI, should we >>update the documentation of VHOST_SET_VRING_BASE and >>VHOST_GET_VRING_BASE in include/uapi/linux/vhost.h? > >Correct me if I'm wrong, but I don't think we're changing anything in >the UAPI here, just fixing code to work correctly with what is already >happening. IIUC before this patch VHOST_GET_VRING_BASE and VHOST_SET_VRING_BASE never worked with packed virtqueue, since we were only handling last_avail_idx. Now we are supporting packed virtqueue, handling in vhost_vring_state.num both last_avail_idx and last_used_idx (with wrap counters). For example for VHOST_GET_VRING_BASE where is documented that the first 15 bits are last_avail_idx, the 16th the avail_wrap_counter, and the others are last_used_idx and used_wrap_counter? Maybe I missed something, but since this is UAPI, IMHO we should document the parameters of ioctls at least in include/uapi/linux/vhost.h. Thanks, Stefano _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base 2023-05-16 7:49 ` Stefano Garzarella @ 2023-05-16 18:26 ` Shannon Nelson via Virtualization 2023-05-17 5:26 ` Jason Wang 0 siblings, 1 reply; 22+ messages in thread From: Shannon Nelson via Virtualization @ 2023-05-16 18:26 UTC (permalink / raw) To: Stefano Garzarella; +Cc: virtualization, drivers, mst On 5/16/23 12:49 AM, Stefano Garzarella wrote: > On Mon, May 15, 2023 at 01:41:12PM -0700, Shannon Nelson wrote: >> On 5/9/23 1:46 AM, Stefano Garzarella wrote: >>> On Mon, Apr 24, 2023 at 03:50:30PM -0700, Shannon Nelson via >>> Virtualization wrote: >>>> Use the right structs for PACKED or split vqs when setting and >>>> getting the vring base. >>>> >>>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> >>>> --- >>>> drivers/vhost/vhost.c | 18 +++++++++++++----- >>>> drivers/vhost/vhost.h | 8 ++++++-- >>>> 2 files changed, 19 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >>>> index f11bdbe4c2c5..f64efda48f21 100644 >>>> --- a/drivers/vhost/vhost.c >>>> +++ b/drivers/vhost/vhost.c >>>> @@ -1633,17 +1633,25 @@ long vhost_vring_ioctl(struct vhost_dev >>>> *d, unsigned int ioctl, void __user *arg >>>> r = -EFAULT; >>>> break; >>>> } >>>> - if (s.num > 0xffff) { >>>> - r = -EINVAL; >>>> - break; >>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { >>>> + vq->last_avail_idx = s.num & 0xffff; >>>> + vq->last_used_idx = (s.num >> 16) & 0xffff; >>>> + } else { >>>> + if (s.num > 0xffff) { >>>> + r = -EINVAL; >>>> + break; >>>> + } >>>> + vq->last_avail_idx = s.num; >>>> } >>>> - vq->last_avail_idx = s.num; >>>> /* Forget the cached index value. */ >>>> vq->avail_idx = vq->last_avail_idx; >>>> break; >>>> case VHOST_GET_VRING_BASE: >>>> s.index = idx; >>>> - s.num = vq->last_avail_idx; >>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) >>>> + s.num = (u32)vq->last_avail_idx | >>>> ((u32)vq->last_used_idx << 16); >>>> + else >>>> + s.num = vq->last_avail_idx; >>> >>> The changes LGTM, but since we are changing the UAPI, should we >>> update the documentation of VHOST_SET_VRING_BASE and >>> VHOST_GET_VRING_BASE in include/uapi/linux/vhost.h? >> >> Correct me if I'm wrong, but I don't think we're changing anything in >> the UAPI here, just fixing code to work correctly with what is already >> happening. > > IIUC before this patch VHOST_GET_VRING_BASE and VHOST_SET_VRING_BASE > never worked with packed virtqueue, since we were only handling > last_avail_idx. Now we are supporting packed virtqueue, handling > in vhost_vring_state.num both last_avail_idx and last_used_idx (with > wrap counters). > > For example for VHOST_GET_VRING_BASE where is documented that the first > 15 bits are last_avail_idx, the 16th the avail_wrap_counter, and the > others are last_used_idx and used_wrap_counter? > > Maybe I missed something, but since this is UAPI, IMHO we should > document the parameters of ioctls at least in > include/uapi/linux/vhost.h. Perhaps Jason already has something written up that could be put in here from when he first added the wrap_counter a couple of years ago? sln > > Thanks, > Stefano > > -- > You received this message because you are subscribed to the Google > Groups "Pensando Drivers" group. > To unsubscribe from this group and stop receiving emails from it, send > an email to drivers+unsubscribe@pensando.io. > To view this discussion on the web visit > https://groups.google.com/a/pensando.io/d/msgid/drivers/q6cmfha36sdkgflwwd3pr4sw7rgajag4ahgjbpfjrr76w4o2b6%403yc7zs5u65s4. _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base 2023-05-16 18:26 ` Shannon Nelson via Virtualization @ 2023-05-17 5:26 ` Jason Wang 2023-05-17 7:00 ` Stefano Garzarella 0 siblings, 1 reply; 22+ messages in thread From: Jason Wang @ 2023-05-17 5:26 UTC (permalink / raw) To: Shannon Nelson; +Cc: drivers, mst, virtualization On Wed, May 17, 2023 at 2:26 AM Shannon Nelson <shannon.nelson@amd.com> wrote: > > On 5/16/23 12:49 AM, Stefano Garzarella wrote: > > On Mon, May 15, 2023 at 01:41:12PM -0700, Shannon Nelson wrote: > >> On 5/9/23 1:46 AM, Stefano Garzarella wrote: > >>> On Mon, Apr 24, 2023 at 03:50:30PM -0700, Shannon Nelson via > >>> Virtualization wrote: > >>>> Use the right structs for PACKED or split vqs when setting and > >>>> getting the vring base. > >>>> > >>>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > >>>> --- > >>>> drivers/vhost/vhost.c | 18 +++++++++++++----- > >>>> drivers/vhost/vhost.h | 8 ++++++-- > >>>> 2 files changed, 19 insertions(+), 7 deletions(-) > >>>> > >>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > >>>> index f11bdbe4c2c5..f64efda48f21 100644 > >>>> --- a/drivers/vhost/vhost.c > >>>> +++ b/drivers/vhost/vhost.c > >>>> @@ -1633,17 +1633,25 @@ long vhost_vring_ioctl(struct vhost_dev > >>>> *d, unsigned int ioctl, void __user *arg > >>>> r = -EFAULT; > >>>> break; > >>>> } > >>>> - if (s.num > 0xffff) { > >>>> - r = -EINVAL; > >>>> - break; > >>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { > >>>> + vq->last_avail_idx = s.num & 0xffff; > >>>> + vq->last_used_idx = (s.num >> 16) & 0xffff; > >>>> + } else { > >>>> + if (s.num > 0xffff) { > >>>> + r = -EINVAL; > >>>> + break; > >>>> + } > >>>> + vq->last_avail_idx = s.num; > >>>> } > >>>> - vq->last_avail_idx = s.num; > >>>> /* Forget the cached index value. */ > >>>> vq->avail_idx = vq->last_avail_idx; > >>>> break; > >>>> case VHOST_GET_VRING_BASE: > >>>> s.index = idx; > >>>> - s.num = vq->last_avail_idx; > >>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) > >>>> + s.num = (u32)vq->last_avail_idx | > >>>> ((u32)vq->last_used_idx << 16); > >>>> + else > >>>> + s.num = vq->last_avail_idx; > >>> > >>> The changes LGTM, but since we are changing the UAPI, should we > >>> update the documentation of VHOST_SET_VRING_BASE and > >>> VHOST_GET_VRING_BASE in include/uapi/linux/vhost.h? > >> > >> Correct me if I'm wrong, but I don't think we're changing anything in > >> the UAPI here, just fixing code to work correctly with what is already > >> happening. > > > > IIUC before this patch VHOST_GET_VRING_BASE and VHOST_SET_VRING_BASE > > never worked with packed virtqueue, since we were only handling > > last_avail_idx. Now we are supporting packed virtqueue, handling > > in vhost_vring_state.num both last_avail_idx and last_used_idx (with > > wrap counters). > > > > For example for VHOST_GET_VRING_BASE where is documented that the first > > 15 bits are last_avail_idx, the 16th the avail_wrap_counter, and the > > others are last_used_idx and used_wrap_counter? > > > > Maybe I missed something, but since this is UAPI, IMHO we should > > document the parameters of ioctls at least in > > include/uapi/linux/vhost.h. > > Perhaps Jason already has something written up that could be put in here > from when he first added the wrap_counter a couple of years ago? If you meant the virtio driver support for packed, I think it's different from the context which is vhost here. I agree with Stefano that we need to update the comments around GET_VRING_BASE and SET_VRING_BASE, then we are fine. Thanks > > sln > > > > > Thanks, > > Stefano > > > > -- > > You received this message because you are subscribed to the Google > > Groups "Pensando Drivers" group. > > To unsubscribe from this group and stop receiving emails from it, send > > an email to drivers+unsubscribe@pensando.io. > > To view this discussion on the web visit > > https://groups.google.com/a/pensando.io/d/msgid/drivers/q6cmfha36sdkgflwwd3pr4sw7rgajag4ahgjbpfjrr76w4o2b6%403yc7zs5u65s4. > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base 2023-05-17 5:26 ` Jason Wang @ 2023-05-17 7:00 ` Stefano Garzarella 2023-05-18 5:23 ` Jason Wang 0 siblings, 1 reply; 22+ messages in thread From: Stefano Garzarella @ 2023-05-17 7:00 UTC (permalink / raw) To: Jason Wang; +Cc: drivers, mst, virtualization On Wed, May 17, 2023 at 7:26 AM Jason Wang <jasowang@redhat.com> wrote: > > On Wed, May 17, 2023 at 2:26 AM Shannon Nelson <shannon.nelson@amd.com> wrote: > > > > On 5/16/23 12:49 AM, Stefano Garzarella wrote: > > > On Mon, May 15, 2023 at 01:41:12PM -0700, Shannon Nelson wrote: > > >> On 5/9/23 1:46 AM, Stefano Garzarella wrote: > > >>> On Mon, Apr 24, 2023 at 03:50:30PM -0700, Shannon Nelson via > > >>> Virtualization wrote: > > >>>> Use the right structs for PACKED or split vqs when setting and > > >>>> getting the vring base. > > >>>> > > >>>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > > >>>> --- > > >>>> drivers/vhost/vhost.c | 18 +++++++++++++----- > > >>>> drivers/vhost/vhost.h | 8 ++++++-- > > >>>> 2 files changed, 19 insertions(+), 7 deletions(-) > > >>>> > > >>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > >>>> index f11bdbe4c2c5..f64efda48f21 100644 > > >>>> --- a/drivers/vhost/vhost.c > > >>>> +++ b/drivers/vhost/vhost.c > > >>>> @@ -1633,17 +1633,25 @@ long vhost_vring_ioctl(struct vhost_dev > > >>>> *d, unsigned int ioctl, void __user *arg > > >>>> r = -EFAULT; > > >>>> break; > > >>>> } > > >>>> - if (s.num > 0xffff) { > > >>>> - r = -EINVAL; > > >>>> - break; > > >>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { > > >>>> + vq->last_avail_idx = s.num & 0xffff; > > >>>> + vq->last_used_idx = (s.num >> 16) & 0xffff; > > >>>> + } else { > > >>>> + if (s.num > 0xffff) { > > >>>> + r = -EINVAL; > > >>>> + break; > > >>>> + } > > >>>> + vq->last_avail_idx = s.num; > > >>>> } > > >>>> - vq->last_avail_idx = s.num; > > >>>> /* Forget the cached index value. */ > > >>>> vq->avail_idx = vq->last_avail_idx; > > >>>> break; > > >>>> case VHOST_GET_VRING_BASE: > > >>>> s.index = idx; > > >>>> - s.num = vq->last_avail_idx; > > >>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) > > >>>> + s.num = (u32)vq->last_avail_idx | > > >>>> ((u32)vq->last_used_idx << 16); > > >>>> + else > > >>>> + s.num = vq->last_avail_idx; > > >>> > > >>> The changes LGTM, but since we are changing the UAPI, should we > > >>> update the documentation of VHOST_SET_VRING_BASE and > > >>> VHOST_GET_VRING_BASE in include/uapi/linux/vhost.h? > > >> > > >> Correct me if I'm wrong, but I don't think we're changing anything in > > >> the UAPI here, just fixing code to work correctly with what is already > > >> happening. > > > > > > IIUC before this patch VHOST_GET_VRING_BASE and VHOST_SET_VRING_BASE > > > never worked with packed virtqueue, since we were only handling > > > last_avail_idx. Now we are supporting packed virtqueue, handling > > > in vhost_vring_state.num both last_avail_idx and last_used_idx (with > > > wrap counters). > > > > > > For example for VHOST_GET_VRING_BASE where is documented that the first > > > 15 bits are last_avail_idx, the 16th the avail_wrap_counter, and the > > > others are last_used_idx and used_wrap_counter? > > > > > > Maybe I missed something, but since this is UAPI, IMHO we should > > > document the parameters of ioctls at least in > > > include/uapi/linux/vhost.h. > > > > Perhaps Jason already has something written up that could be put in here > > from when he first added the wrap_counter a couple of years ago? > > If you meant the virtio driver support for packed, I think it's > different from the context which is vhost here. > > I agree with Stefano that we need to update the comments around > GET_VRING_BASE and SET_VRING_BASE, then we are fine. I'm thinking if we should also add a new VHOST_BACKEND_F_RING_PACKED feature (or something similar) to inform the user space that now we are able to handle packed virtqueue through vhost IOCTLs, otherwise how can the userspace know if it is supported or not? Thanks, Stefano _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base 2023-05-17 7:00 ` Stefano Garzarella @ 2023-05-18 5:23 ` Jason Wang 2023-05-18 7:34 ` Stefano Garzarella 0 siblings, 1 reply; 22+ messages in thread From: Jason Wang @ 2023-05-18 5:23 UTC (permalink / raw) To: Stefano Garzarella; +Cc: drivers, mst, virtualization On Wed, May 17, 2023 at 3:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > On Wed, May 17, 2023 at 7:26 AM Jason Wang <jasowang@redhat.com> wrote: > > > > On Wed, May 17, 2023 at 2:26 AM Shannon Nelson <shannon.nelson@amd.com> wrote: > > > > > > On 5/16/23 12:49 AM, Stefano Garzarella wrote: > > > > On Mon, May 15, 2023 at 01:41:12PM -0700, Shannon Nelson wrote: > > > >> On 5/9/23 1:46 AM, Stefano Garzarella wrote: > > > >>> On Mon, Apr 24, 2023 at 03:50:30PM -0700, Shannon Nelson via > > > >>> Virtualization wrote: > > > >>>> Use the right structs for PACKED or split vqs when setting and > > > >>>> getting the vring base. > > > >>>> > > > >>>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > > > >>>> --- > > > >>>> drivers/vhost/vhost.c | 18 +++++++++++++----- > > > >>>> drivers/vhost/vhost.h | 8 ++++++-- > > > >>>> 2 files changed, 19 insertions(+), 7 deletions(-) > > > >>>> > > > >>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > >>>> index f11bdbe4c2c5..f64efda48f21 100644 > > > >>>> --- a/drivers/vhost/vhost.c > > > >>>> +++ b/drivers/vhost/vhost.c > > > >>>> @@ -1633,17 +1633,25 @@ long vhost_vring_ioctl(struct vhost_dev > > > >>>> *d, unsigned int ioctl, void __user *arg > > > >>>> r = -EFAULT; > > > >>>> break; > > > >>>> } > > > >>>> - if (s.num > 0xffff) { > > > >>>> - r = -EINVAL; > > > >>>> - break; > > > >>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { > > > >>>> + vq->last_avail_idx = s.num & 0xffff; > > > >>>> + vq->last_used_idx = (s.num >> 16) & 0xffff; > > > >>>> + } else { > > > >>>> + if (s.num > 0xffff) { > > > >>>> + r = -EINVAL; > > > >>>> + break; > > > >>>> + } > > > >>>> + vq->last_avail_idx = s.num; > > > >>>> } > > > >>>> - vq->last_avail_idx = s.num; > > > >>>> /* Forget the cached index value. */ > > > >>>> vq->avail_idx = vq->last_avail_idx; > > > >>>> break; > > > >>>> case VHOST_GET_VRING_BASE: > > > >>>> s.index = idx; > > > >>>> - s.num = vq->last_avail_idx; > > > >>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) > > > >>>> + s.num = (u32)vq->last_avail_idx | > > > >>>> ((u32)vq->last_used_idx << 16); > > > >>>> + else > > > >>>> + s.num = vq->last_avail_idx; > > > >>> > > > >>> The changes LGTM, but since we are changing the UAPI, should we > > > >>> update the documentation of VHOST_SET_VRING_BASE and > > > >>> VHOST_GET_VRING_BASE in include/uapi/linux/vhost.h? > > > >> > > > >> Correct me if I'm wrong, but I don't think we're changing anything in > > > >> the UAPI here, just fixing code to work correctly with what is already > > > >> happening. > > > > > > > > IIUC before this patch VHOST_GET_VRING_BASE and VHOST_SET_VRING_BASE > > > > never worked with packed virtqueue, since we were only handling > > > > last_avail_idx. Now we are supporting packed virtqueue, handling > > > > in vhost_vring_state.num both last_avail_idx and last_used_idx (with > > > > wrap counters). > > > > > > > > For example for VHOST_GET_VRING_BASE where is documented that the first > > > > 15 bits are last_avail_idx, the 16th the avail_wrap_counter, and the > > > > others are last_used_idx and used_wrap_counter? > > > > > > > > Maybe I missed something, but since this is UAPI, IMHO we should > > > > document the parameters of ioctls at least in > > > > include/uapi/linux/vhost.h. > > > > > > Perhaps Jason already has something written up that could be put in here > > > from when he first added the wrap_counter a couple of years ago? > > > > If you meant the virtio driver support for packed, I think it's > > different from the context which is vhost here. > > > > I agree with Stefano that we need to update the comments around > > GET_VRING_BASE and SET_VRING_BASE, then we are fine. > > I'm thinking if we should also add a new VHOST_BACKEND_F_RING_PACKED > feature (or something similar) to inform the user space that now we > are able to handle packed virtqueue through vhost IOCTLs, otherwise > how can the userspace know if it is supported or not? I probably understand this but I think it should be done via VHOST_GET_FEAETURES. It would be a burden if we matianing duplicated features. Thanks > > Thanks, > Stefano > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base 2023-05-18 5:23 ` Jason Wang @ 2023-05-18 7:34 ` Stefano Garzarella 2023-05-18 7:52 ` Jason Wang 2023-06-02 11:36 ` Michael S. Tsirkin 0 siblings, 2 replies; 22+ messages in thread From: Stefano Garzarella @ 2023-05-18 7:34 UTC (permalink / raw) To: Jason Wang; +Cc: drivers, mst, virtualization On Thu, May 18, 2023 at 7:24 AM Jason Wang <jasowang@redhat.com> wrote: > > On Wed, May 17, 2023 at 3:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > On Wed, May 17, 2023 at 7:26 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > On Wed, May 17, 2023 at 2:26 AM Shannon Nelson <shannon.nelson@amd.com> wrote: > > > > > > > > On 5/16/23 12:49 AM, Stefano Garzarella wrote: > > > > > On Mon, May 15, 2023 at 01:41:12PM -0700, Shannon Nelson wrote: > > > > >> On 5/9/23 1:46 AM, Stefano Garzarella wrote: > > > > >>> On Mon, Apr 24, 2023 at 03:50:30PM -0700, Shannon Nelson via > > > > >>> Virtualization wrote: > > > > >>>> Use the right structs for PACKED or split vqs when setting and > > > > >>>> getting the vring base. > > > > >>>> > > > > >>>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > > > > >>>> --- > > > > >>>> drivers/vhost/vhost.c | 18 +++++++++++++----- > > > > >>>> drivers/vhost/vhost.h | 8 ++++++-- > > > > >>>> 2 files changed, 19 insertions(+), 7 deletions(-) > > > > >>>> > > > > >>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > > >>>> index f11bdbe4c2c5..f64efda48f21 100644 > > > > >>>> --- a/drivers/vhost/vhost.c > > > > >>>> +++ b/drivers/vhost/vhost.c > > > > >>>> @@ -1633,17 +1633,25 @@ long vhost_vring_ioctl(struct vhost_dev > > > > >>>> *d, unsigned int ioctl, void __user *arg > > > > >>>> r = -EFAULT; > > > > >>>> break; > > > > >>>> } > > > > >>>> - if (s.num > 0xffff) { > > > > >>>> - r = -EINVAL; > > > > >>>> - break; > > > > >>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { > > > > >>>> + vq->last_avail_idx = s.num & 0xffff; > > > > >>>> + vq->last_used_idx = (s.num >> 16) & 0xffff; > > > > >>>> + } else { > > > > >>>> + if (s.num > 0xffff) { > > > > >>>> + r = -EINVAL; > > > > >>>> + break; > > > > >>>> + } > > > > >>>> + vq->last_avail_idx = s.num; > > > > >>>> } > > > > >>>> - vq->last_avail_idx = s.num; > > > > >>>> /* Forget the cached index value. */ > > > > >>>> vq->avail_idx = vq->last_avail_idx; > > > > >>>> break; > > > > >>>> case VHOST_GET_VRING_BASE: > > > > >>>> s.index = idx; > > > > >>>> - s.num = vq->last_avail_idx; > > > > >>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) > > > > >>>> + s.num = (u32)vq->last_avail_idx | > > > > >>>> ((u32)vq->last_used_idx << 16); > > > > >>>> + else > > > > >>>> + s.num = vq->last_avail_idx; > > > > >>> > > > > >>> The changes LGTM, but since we are changing the UAPI, should we > > > > >>> update the documentation of VHOST_SET_VRING_BASE and > > > > >>> VHOST_GET_VRING_BASE in include/uapi/linux/vhost.h? > > > > >> > > > > >> Correct me if I'm wrong, but I don't think we're changing anything in > > > > >> the UAPI here, just fixing code to work correctly with what is already > > > > >> happening. > > > > > > > > > > IIUC before this patch VHOST_GET_VRING_BASE and VHOST_SET_VRING_BASE > > > > > never worked with packed virtqueue, since we were only handling > > > > > last_avail_idx. Now we are supporting packed virtqueue, handling > > > > > in vhost_vring_state.num both last_avail_idx and last_used_idx (with > > > > > wrap counters). > > > > > > > > > > For example for VHOST_GET_VRING_BASE where is documented that the first > > > > > 15 bits are last_avail_idx, the 16th the avail_wrap_counter, and the > > > > > others are last_used_idx and used_wrap_counter? > > > > > > > > > > Maybe I missed something, but since this is UAPI, IMHO we should > > > > > document the parameters of ioctls at least in > > > > > include/uapi/linux/vhost.h. > > > > > > > > Perhaps Jason already has something written up that could be put in here > > > > from when he first added the wrap_counter a couple of years ago? > > > > > > If you meant the virtio driver support for packed, I think it's > > > different from the context which is vhost here. > > > > > > I agree with Stefano that we need to update the comments around > > > GET_VRING_BASE and SET_VRING_BASE, then we are fine. > > > > I'm thinking if we should also add a new VHOST_BACKEND_F_RING_PACKED > > feature (or something similar) to inform the user space that now we > > are able to handle packed virtqueue through vhost IOCTLs, otherwise > > how can the userspace know if it is supported or not? > > I probably understand this but I think it should be done via > VHOST_GET_FEAETURES. It would be a burden if we matianing duplicated > features. Good point, I see. I think we should do one of these things, though: - mask VIRTIO_F_RING_PACKED in the stable kernels when VHOST_GET_FEAETURES is called - backport this patch on all stable kernels that support vhost-vdpa Maybe the last one makes more sense. Thanks, Stefano _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base 2023-05-18 7:34 ` Stefano Garzarella @ 2023-05-18 7:52 ` Jason Wang 2023-05-18 8:38 ` Michael S. Tsirkin 2023-06-02 11:36 ` Michael S. Tsirkin 1 sibling, 1 reply; 22+ messages in thread From: Jason Wang @ 2023-05-18 7:52 UTC (permalink / raw) To: Stefano Garzarella; +Cc: mst, Gautam Dawar, virtualization, drivers On Thu, May 18, 2023 at 3:34 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > On Thu, May 18, 2023 at 7:24 AM Jason Wang <jasowang@redhat.com> wrote: > > > > On Wed, May 17, 2023 at 3:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > > On Wed, May 17, 2023 at 7:26 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > On Wed, May 17, 2023 at 2:26 AM Shannon Nelson <shannon.nelson@amd.com> wrote: > > > > > > > > > > On 5/16/23 12:49 AM, Stefano Garzarella wrote: > > > > > > On Mon, May 15, 2023 at 01:41:12PM -0700, Shannon Nelson wrote: > > > > > >> On 5/9/23 1:46 AM, Stefano Garzarella wrote: > > > > > >>> On Mon, Apr 24, 2023 at 03:50:30PM -0700, Shannon Nelson via > > > > > >>> Virtualization wrote: > > > > > >>>> Use the right structs for PACKED or split vqs when setting and > > > > > >>>> getting the vring base. > > > > > >>>> > > > > > >>>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > > > > > >>>> --- > > > > > >>>> drivers/vhost/vhost.c | 18 +++++++++++++----- > > > > > >>>> drivers/vhost/vhost.h | 8 ++++++-- > > > > > >>>> 2 files changed, 19 insertions(+), 7 deletions(-) > > > > > >>>> > > > > > >>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > > > >>>> index f11bdbe4c2c5..f64efda48f21 100644 > > > > > >>>> --- a/drivers/vhost/vhost.c > > > > > >>>> +++ b/drivers/vhost/vhost.c > > > > > >>>> @@ -1633,17 +1633,25 @@ long vhost_vring_ioctl(struct vhost_dev > > > > > >>>> *d, unsigned int ioctl, void __user *arg > > > > > >>>> r = -EFAULT; > > > > > >>>> break; > > > > > >>>> } > > > > > >>>> - if (s.num > 0xffff) { > > > > > >>>> - r = -EINVAL; > > > > > >>>> - break; > > > > > >>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { > > > > > >>>> + vq->last_avail_idx = s.num & 0xffff; > > > > > >>>> + vq->last_used_idx = (s.num >> 16) & 0xffff; > > > > > >>>> + } else { > > > > > >>>> + if (s.num > 0xffff) { > > > > > >>>> + r = -EINVAL; > > > > > >>>> + break; > > > > > >>>> + } > > > > > >>>> + vq->last_avail_idx = s.num; > > > > > >>>> } > > > > > >>>> - vq->last_avail_idx = s.num; > > > > > >>>> /* Forget the cached index value. */ > > > > > >>>> vq->avail_idx = vq->last_avail_idx; > > > > > >>>> break; > > > > > >>>> case VHOST_GET_VRING_BASE: > > > > > >>>> s.index = idx; > > > > > >>>> - s.num = vq->last_avail_idx; > > > > > >>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) > > > > > >>>> + s.num = (u32)vq->last_avail_idx | > > > > > >>>> ((u32)vq->last_used_idx << 16); > > > > > >>>> + else > > > > > >>>> + s.num = vq->last_avail_idx; > > > > > >>> > > > > > >>> The changes LGTM, but since we are changing the UAPI, should we > > > > > >>> update the documentation of VHOST_SET_VRING_BASE and > > > > > >>> VHOST_GET_VRING_BASE in include/uapi/linux/vhost.h? > > > > > >> > > > > > >> Correct me if I'm wrong, but I don't think we're changing anything in > > > > > >> the UAPI here, just fixing code to work correctly with what is already > > > > > >> happening. > > > > > > > > > > > > IIUC before this patch VHOST_GET_VRING_BASE and VHOST_SET_VRING_BASE > > > > > > never worked with packed virtqueue, since we were only handling > > > > > > last_avail_idx. Now we are supporting packed virtqueue, handling > > > > > > in vhost_vring_state.num both last_avail_idx and last_used_idx (with > > > > > > wrap counters). > > > > > > > > > > > > For example for VHOST_GET_VRING_BASE where is documented that the first > > > > > > 15 bits are last_avail_idx, the 16th the avail_wrap_counter, and the > > > > > > others are last_used_idx and used_wrap_counter? > > > > > > > > > > > > Maybe I missed something, but since this is UAPI, IMHO we should > > > > > > document the parameters of ioctls at least in > > > > > > include/uapi/linux/vhost.h. > > > > > > > > > > Perhaps Jason already has something written up that could be put in here > > > > > from when he first added the wrap_counter a couple of years ago? > > > > > > > > If you meant the virtio driver support for packed, I think it's > > > > different from the context which is vhost here. > > > > > > > > I agree with Stefano that we need to update the comments around > > > > GET_VRING_BASE and SET_VRING_BASE, then we are fine. > > > > > > I'm thinking if we should also add a new VHOST_BACKEND_F_RING_PACKED > > > feature (or something similar) to inform the user space that now we > > > are able to handle packed virtqueue through vhost IOCTLs, otherwise > > > how can the userspace know if it is supported or not? > > > > I probably understand this but I think it should be done via > > VHOST_GET_FEAETURES. It would be a burden if we matianing duplicated > > features. > > Good point, I see. > > I think we should do one of these things, though: > - mask VIRTIO_F_RING_PACKED in the stable kernels when > VHOST_GET_FEAETURES is called > - backport this patch on all stable kernels that support vhost-vdpa > > Maybe the last one makes more sense. Not sure, it looks to me the packed support for vDPA was first added by Gautam. So it probably means that except for vp_vdpa, we don't have a vDPA parent that can do the packed virtuque now. Adding the relevant people here for more comment Thanks > > Thanks, > Stefano > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base 2023-05-18 7:52 ` Jason Wang @ 2023-05-18 8:38 ` Michael S. Tsirkin 2023-05-18 8:59 ` Jason Wang 0 siblings, 1 reply; 22+ messages in thread From: Michael S. Tsirkin @ 2023-05-18 8:38 UTC (permalink / raw) To: Jason Wang; +Cc: Gautam Dawar, virtualization, drivers On Thu, May 18, 2023 at 03:52:10PM +0800, Jason Wang wrote: > On Thu, May 18, 2023 at 3:34 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > On Thu, May 18, 2023 at 7:24 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > On Wed, May 17, 2023 at 3:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > > > > On Wed, May 17, 2023 at 7:26 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > On Wed, May 17, 2023 at 2:26 AM Shannon Nelson <shannon.nelson@amd.com> wrote: > > > > > > > > > > > > On 5/16/23 12:49 AM, Stefano Garzarella wrote: > > > > > > > On Mon, May 15, 2023 at 01:41:12PM -0700, Shannon Nelson wrote: > > > > > > >> On 5/9/23 1:46 AM, Stefano Garzarella wrote: > > > > > > >>> On Mon, Apr 24, 2023 at 03:50:30PM -0700, Shannon Nelson via > > > > > > >>> Virtualization wrote: > > > > > > >>>> Use the right structs for PACKED or split vqs when setting and > > > > > > >>>> getting the vring base. > > > > > > >>>> > > > > > > >>>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > > > > > > >>>> --- > > > > > > >>>> drivers/vhost/vhost.c | 18 +++++++++++++----- > > > > > > >>>> drivers/vhost/vhost.h | 8 ++++++-- > > > > > > >>>> 2 files changed, 19 insertions(+), 7 deletions(-) > > > > > > >>>> > > > > > > >>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > > > > >>>> index f11bdbe4c2c5..f64efda48f21 100644 > > > > > > >>>> --- a/drivers/vhost/vhost.c > > > > > > >>>> +++ b/drivers/vhost/vhost.c > > > > > > >>>> @@ -1633,17 +1633,25 @@ long vhost_vring_ioctl(struct vhost_dev > > > > > > >>>> *d, unsigned int ioctl, void __user *arg > > > > > > >>>> r = -EFAULT; > > > > > > >>>> break; > > > > > > >>>> } > > > > > > >>>> - if (s.num > 0xffff) { > > > > > > >>>> - r = -EINVAL; > > > > > > >>>> - break; > > > > > > >>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { > > > > > > >>>> + vq->last_avail_idx = s.num & 0xffff; > > > > > > >>>> + vq->last_used_idx = (s.num >> 16) & 0xffff; > > > > > > >>>> + } else { > > > > > > >>>> + if (s.num > 0xffff) { > > > > > > >>>> + r = -EINVAL; > > > > > > >>>> + break; > > > > > > >>>> + } > > > > > > >>>> + vq->last_avail_idx = s.num; > > > > > > >>>> } > > > > > > >>>> - vq->last_avail_idx = s.num; > > > > > > >>>> /* Forget the cached index value. */ > > > > > > >>>> vq->avail_idx = vq->last_avail_idx; > > > > > > >>>> break; > > > > > > >>>> case VHOST_GET_VRING_BASE: > > > > > > >>>> s.index = idx; > > > > > > >>>> - s.num = vq->last_avail_idx; > > > > > > >>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) > > > > > > >>>> + s.num = (u32)vq->last_avail_idx | > > > > > > >>>> ((u32)vq->last_used_idx << 16); > > > > > > >>>> + else > > > > > > >>>> + s.num = vq->last_avail_idx; > > > > > > >>> > > > > > > >>> The changes LGTM, but since we are changing the UAPI, should we > > > > > > >>> update the documentation of VHOST_SET_VRING_BASE and > > > > > > >>> VHOST_GET_VRING_BASE in include/uapi/linux/vhost.h? > > > > > > >> > > > > > > >> Correct me if I'm wrong, but I don't think we're changing anything in > > > > > > >> the UAPI here, just fixing code to work correctly with what is already > > > > > > >> happening. > > > > > > > > > > > > > > IIUC before this patch VHOST_GET_VRING_BASE and VHOST_SET_VRING_BASE > > > > > > > never worked with packed virtqueue, since we were only handling > > > > > > > last_avail_idx. Now we are supporting packed virtqueue, handling > > > > > > > in vhost_vring_state.num both last_avail_idx and last_used_idx (with > > > > > > > wrap counters). > > > > > > > > > > > > > > For example for VHOST_GET_VRING_BASE where is documented that the first > > > > > > > 15 bits are last_avail_idx, the 16th the avail_wrap_counter, and the > > > > > > > others are last_used_idx and used_wrap_counter? > > > > > > > > > > > > > > Maybe I missed something, but since this is UAPI, IMHO we should > > > > > > > document the parameters of ioctls at least in > > > > > > > include/uapi/linux/vhost.h. > > > > > > > > > > > > Perhaps Jason already has something written up that could be put in here > > > > > > from when he first added the wrap_counter a couple of years ago? > > > > > > > > > > If you meant the virtio driver support for packed, I think it's > > > > > different from the context which is vhost here. > > > > > > > > > > I agree with Stefano that we need to update the comments around > > > > > GET_VRING_BASE and SET_VRING_BASE, then we are fine. > > > > > > > > I'm thinking if we should also add a new VHOST_BACKEND_F_RING_PACKED > > > > feature (or something similar) to inform the user space that now we > > > > are able to handle packed virtqueue through vhost IOCTLs, otherwise > > > > how can the userspace know if it is supported or not? > > > > > > I probably understand this but I think it should be done via > > > VHOST_GET_FEAETURES. It would be a burden if we matianing duplicated > > > features. > > > > Good point, I see. > > > > I think we should do one of these things, though: > > - mask VIRTIO_F_RING_PACKED in the stable kernels when > > VHOST_GET_FEAETURES is called > > - backport this patch on all stable kernels that support vhost-vdpa > > > > Maybe the last one makes more sense. > > Not sure, it looks to me the packed support for vDPA was first added > by Gautam. So it probably means that except for vp_vdpa, we don't have > a vDPA parent that can do the packed virtuque now. Adding the relevant > people here for more comment > > Thanks BTW should we fix up vhost.c to support packed rings too? E.g. so we can migrate to vhost? There's an old patchset of mine that started work on this: https://lore.kernel.org/all/20200407011612.478226-1-mst%40redhat.com Is there need for this now? > > > > Thanks, > > Stefano > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base 2023-05-18 8:38 ` Michael S. Tsirkin @ 2023-05-18 8:59 ` Jason Wang 2023-05-18 9:42 ` Michael S. Tsirkin 0 siblings, 1 reply; 22+ messages in thread From: Jason Wang @ 2023-05-18 8:59 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Gautam Dawar, virtualization, drivers On Thu, May 18, 2023 at 4:38 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, May 18, 2023 at 03:52:10PM +0800, Jason Wang wrote: > > On Thu, May 18, 2023 at 3:34 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > > On Thu, May 18, 2023 at 7:24 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > On Wed, May 17, 2023 at 3:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > > > > > > On Wed, May 17, 2023 at 7:26 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > On Wed, May 17, 2023 at 2:26 AM Shannon Nelson <shannon.nelson@amd.com> wrote: > > > > > > > > > > > > > > On 5/16/23 12:49 AM, Stefano Garzarella wrote: > > > > > > > > On Mon, May 15, 2023 at 01:41:12PM -0700, Shannon Nelson wrote: > > > > > > > >> On 5/9/23 1:46 AM, Stefano Garzarella wrote: > > > > > > > >>> On Mon, Apr 24, 2023 at 03:50:30PM -0700, Shannon Nelson via > > > > > > > >>> Virtualization wrote: > > > > > > > >>>> Use the right structs for PACKED or split vqs when setting and > > > > > > > >>>> getting the vring base. > > > > > > > >>>> > > > > > > > >>>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > > > > > > > >>>> --- > > > > > > > >>>> drivers/vhost/vhost.c | 18 +++++++++++++----- > > > > > > > >>>> drivers/vhost/vhost.h | 8 ++++++-- > > > > > > > >>>> 2 files changed, 19 insertions(+), 7 deletions(-) > > > > > > > >>>> > > > > > > > >>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > > > > > >>>> index f11bdbe4c2c5..f64efda48f21 100644 > > > > > > > >>>> --- a/drivers/vhost/vhost.c > > > > > > > >>>> +++ b/drivers/vhost/vhost.c > > > > > > > >>>> @@ -1633,17 +1633,25 @@ long vhost_vring_ioctl(struct vhost_dev > > > > > > > >>>> *d, unsigned int ioctl, void __user *arg > > > > > > > >>>> r = -EFAULT; > > > > > > > >>>> break; > > > > > > > >>>> } > > > > > > > >>>> - if (s.num > 0xffff) { > > > > > > > >>>> - r = -EINVAL; > > > > > > > >>>> - break; > > > > > > > >>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { > > > > > > > >>>> + vq->last_avail_idx = s.num & 0xffff; > > > > > > > >>>> + vq->last_used_idx = (s.num >> 16) & 0xffff; > > > > > > > >>>> + } else { > > > > > > > >>>> + if (s.num > 0xffff) { > > > > > > > >>>> + r = -EINVAL; > > > > > > > >>>> + break; > > > > > > > >>>> + } > > > > > > > >>>> + vq->last_avail_idx = s.num; > > > > > > > >>>> } > > > > > > > >>>> - vq->last_avail_idx = s.num; > > > > > > > >>>> /* Forget the cached index value. */ > > > > > > > >>>> vq->avail_idx = vq->last_avail_idx; > > > > > > > >>>> break; > > > > > > > >>>> case VHOST_GET_VRING_BASE: > > > > > > > >>>> s.index = idx; > > > > > > > >>>> - s.num = vq->last_avail_idx; > > > > > > > >>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) > > > > > > > >>>> + s.num = (u32)vq->last_avail_idx | > > > > > > > >>>> ((u32)vq->last_used_idx << 16); > > > > > > > >>>> + else > > > > > > > >>>> + s.num = vq->last_avail_idx; > > > > > > > >>> > > > > > > > >>> The changes LGTM, but since we are changing the UAPI, should we > > > > > > > >>> update the documentation of VHOST_SET_VRING_BASE and > > > > > > > >>> VHOST_GET_VRING_BASE in include/uapi/linux/vhost.h? > > > > > > > >> > > > > > > > >> Correct me if I'm wrong, but I don't think we're changing anything in > > > > > > > >> the UAPI here, just fixing code to work correctly with what is already > > > > > > > >> happening. > > > > > > > > > > > > > > > > IIUC before this patch VHOST_GET_VRING_BASE and VHOST_SET_VRING_BASE > > > > > > > > never worked with packed virtqueue, since we were only handling > > > > > > > > last_avail_idx. Now we are supporting packed virtqueue, handling > > > > > > > > in vhost_vring_state.num both last_avail_idx and last_used_idx (with > > > > > > > > wrap counters). > > > > > > > > > > > > > > > > For example for VHOST_GET_VRING_BASE where is documented that the first > > > > > > > > 15 bits are last_avail_idx, the 16th the avail_wrap_counter, and the > > > > > > > > others are last_used_idx and used_wrap_counter? > > > > > > > > > > > > > > > > Maybe I missed something, but since this is UAPI, IMHO we should > > > > > > > > document the parameters of ioctls at least in > > > > > > > > include/uapi/linux/vhost.h. > > > > > > > > > > > > > > Perhaps Jason already has something written up that could be put in here > > > > > > > from when he first added the wrap_counter a couple of years ago? > > > > > > > > > > > > If you meant the virtio driver support for packed, I think it's > > > > > > different from the context which is vhost here. > > > > > > > > > > > > I agree with Stefano that we need to update the comments around > > > > > > GET_VRING_BASE and SET_VRING_BASE, then we are fine. > > > > > > > > > > I'm thinking if we should also add a new VHOST_BACKEND_F_RING_PACKED > > > > > feature (or something similar) to inform the user space that now we > > > > > are able to handle packed virtqueue through vhost IOCTLs, otherwise > > > > > how can the userspace know if it is supported or not? > > > > > > > > I probably understand this but I think it should be done via > > > > VHOST_GET_FEAETURES. It would be a burden if we matianing duplicated > > > > features. > > > > > > Good point, I see. > > > > > > I think we should do one of these things, though: > > > - mask VIRTIO_F_RING_PACKED in the stable kernels when > > > VHOST_GET_FEAETURES is called > > > - backport this patch on all stable kernels that support vhost-vdpa > > > > > > Maybe the last one makes more sense. > > > > Not sure, it looks to me the packed support for vDPA was first added > > by Gautam. So it probably means that except for vp_vdpa, we don't have > > a vDPA parent that can do the packed virtuque now. Adding the relevant > > people here for more comment > > > > Thanks > > BTW should we fix up vhost.c to support packed rings too? > E.g. so we can migrate to vhost? Better to have. > There's an old patchset of mine that started work on this: > > https://lore.kernel.org/all/20200407011612.478226-1-mst%40redhat.com > > Is there need for this now? Is there any benchmark for that? Thanks > > > > > > > Thanks, > > > Stefano > > > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base 2023-05-18 8:59 ` Jason Wang @ 2023-05-18 9:42 ` Michael S. Tsirkin 0 siblings, 0 replies; 22+ messages in thread From: Michael S. Tsirkin @ 2023-05-18 9:42 UTC (permalink / raw) To: Jason Wang; +Cc: Gautam Dawar, virtualization, drivers On Thu, May 18, 2023 at 04:59:17PM +0800, Jason Wang wrote: > On Thu, May 18, 2023 at 4:38 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Thu, May 18, 2023 at 03:52:10PM +0800, Jason Wang wrote: > > > On Thu, May 18, 2023 at 3:34 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > > > > On Thu, May 18, 2023 at 7:24 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > On Wed, May 17, 2023 at 3:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > > > > > > > > On Wed, May 17, 2023 at 7:26 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > > > On Wed, May 17, 2023 at 2:26 AM Shannon Nelson <shannon.nelson@amd.com> wrote: > > > > > > > > > > > > > > > > On 5/16/23 12:49 AM, Stefano Garzarella wrote: > > > > > > > > > On Mon, May 15, 2023 at 01:41:12PM -0700, Shannon Nelson wrote: > > > > > > > > >> On 5/9/23 1:46 AM, Stefano Garzarella wrote: > > > > > > > > >>> On Mon, Apr 24, 2023 at 03:50:30PM -0700, Shannon Nelson via > > > > > > > > >>> Virtualization wrote: > > > > > > > > >>>> Use the right structs for PACKED or split vqs when setting and > > > > > > > > >>>> getting the vring base. > > > > > > > > >>>> > > > > > > > > >>>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > > > > > > > > >>>> --- > > > > > > > > >>>> drivers/vhost/vhost.c | 18 +++++++++++++----- > > > > > > > > >>>> drivers/vhost/vhost.h | 8 ++++++-- > > > > > > > > >>>> 2 files changed, 19 insertions(+), 7 deletions(-) > > > > > > > > >>>> > > > > > > > > >>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > > > > > > >>>> index f11bdbe4c2c5..f64efda48f21 100644 > > > > > > > > >>>> --- a/drivers/vhost/vhost.c > > > > > > > > >>>> +++ b/drivers/vhost/vhost.c > > > > > > > > >>>> @@ -1633,17 +1633,25 @@ long vhost_vring_ioctl(struct vhost_dev > > > > > > > > >>>> *d, unsigned int ioctl, void __user *arg > > > > > > > > >>>> r = -EFAULT; > > > > > > > > >>>> break; > > > > > > > > >>>> } > > > > > > > > >>>> - if (s.num > 0xffff) { > > > > > > > > >>>> - r = -EINVAL; > > > > > > > > >>>> - break; > > > > > > > > >>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { > > > > > > > > >>>> + vq->last_avail_idx = s.num & 0xffff; > > > > > > > > >>>> + vq->last_used_idx = (s.num >> 16) & 0xffff; > > > > > > > > >>>> + } else { > > > > > > > > >>>> + if (s.num > 0xffff) { > > > > > > > > >>>> + r = -EINVAL; > > > > > > > > >>>> + break; > > > > > > > > >>>> + } > > > > > > > > >>>> + vq->last_avail_idx = s.num; > > > > > > > > >>>> } > > > > > > > > >>>> - vq->last_avail_idx = s.num; > > > > > > > > >>>> /* Forget the cached index value. */ > > > > > > > > >>>> vq->avail_idx = vq->last_avail_idx; > > > > > > > > >>>> break; > > > > > > > > >>>> case VHOST_GET_VRING_BASE: > > > > > > > > >>>> s.index = idx; > > > > > > > > >>>> - s.num = vq->last_avail_idx; > > > > > > > > >>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) > > > > > > > > >>>> + s.num = (u32)vq->last_avail_idx | > > > > > > > > >>>> ((u32)vq->last_used_idx << 16); > > > > > > > > >>>> + else > > > > > > > > >>>> + s.num = vq->last_avail_idx; > > > > > > > > >>> > > > > > > > > >>> The changes LGTM, but since we are changing the UAPI, should we > > > > > > > > >>> update the documentation of VHOST_SET_VRING_BASE and > > > > > > > > >>> VHOST_GET_VRING_BASE in include/uapi/linux/vhost.h? > > > > > > > > >> > > > > > > > > >> Correct me if I'm wrong, but I don't think we're changing anything in > > > > > > > > >> the UAPI here, just fixing code to work correctly with what is already > > > > > > > > >> happening. > > > > > > > > > > > > > > > > > > IIUC before this patch VHOST_GET_VRING_BASE and VHOST_SET_VRING_BASE > > > > > > > > > never worked with packed virtqueue, since we were only handling > > > > > > > > > last_avail_idx. Now we are supporting packed virtqueue, handling > > > > > > > > > in vhost_vring_state.num both last_avail_idx and last_used_idx (with > > > > > > > > > wrap counters). > > > > > > > > > > > > > > > > > > For example for VHOST_GET_VRING_BASE where is documented that the first > > > > > > > > > 15 bits are last_avail_idx, the 16th the avail_wrap_counter, and the > > > > > > > > > others are last_used_idx and used_wrap_counter? > > > > > > > > > > > > > > > > > > Maybe I missed something, but since this is UAPI, IMHO we should > > > > > > > > > document the parameters of ioctls at least in > > > > > > > > > include/uapi/linux/vhost.h. > > > > > > > > > > > > > > > > Perhaps Jason already has something written up that could be put in here > > > > > > > > from when he first added the wrap_counter a couple of years ago? > > > > > > > > > > > > > > If you meant the virtio driver support for packed, I think it's > > > > > > > different from the context which is vhost here. > > > > > > > > > > > > > > I agree with Stefano that we need to update the comments around > > > > > > > GET_VRING_BASE and SET_VRING_BASE, then we are fine. > > > > > > > > > > > > I'm thinking if we should also add a new VHOST_BACKEND_F_RING_PACKED > > > > > > feature (or something similar) to inform the user space that now we > > > > > > are able to handle packed virtqueue through vhost IOCTLs, otherwise > > > > > > how can the userspace know if it is supported or not? > > > > > > > > > > I probably understand this but I think it should be done via > > > > > VHOST_GET_FEAETURES. It would be a burden if we matianing duplicated > > > > > features. > > > > > > > > Good point, I see. > > > > > > > > I think we should do one of these things, though: > > > > - mask VIRTIO_F_RING_PACKED in the stable kernels when > > > > VHOST_GET_FEAETURES is called > > > > - backport this patch on all stable kernels that support vhost-vdpa > > > > > > > > Maybe the last one makes more sense. > > > > > > Not sure, it looks to me the packed support for vDPA was first added > > > by Gautam. So it probably means that except for vp_vdpa, we don't have > > > a vDPA parent that can do the packed virtuque now. Adding the relevant > > > people here for more comment > > > > > > Thanks > > > > BTW should we fix up vhost.c to support packed rings too? > > E.g. so we can migrate to vhost? > > Better to have. > > > There's an old patchset of mine that started work on this: > > > > https://lore.kernel.org/all/20200407011612.478226-1-mst%40redhat.com > > > > Is there need for this now? > > Is there any benchmark for that? > > Thanks I used one in tools/virtio, there was no real change. Might make sense to remake it since we fixed some bugs in EVENT_IDX support and things might have changed. > > > > > > > > > > Thanks, > > > > Stefano > > > > > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base 2023-05-18 7:34 ` Stefano Garzarella 2023-05-18 7:52 ` Jason Wang @ 2023-06-02 11:36 ` Michael S. Tsirkin 2023-06-05 8:17 ` Stefano Garzarella 1 sibling, 1 reply; 22+ messages in thread From: Michael S. Tsirkin @ 2023-06-02 11:36 UTC (permalink / raw) To: Stefano Garzarella; +Cc: virtualization, drivers On Thu, May 18, 2023 at 09:34:25AM +0200, Stefano Garzarella wrote: > I think we should do one of these things, though: > - mask VIRTIO_F_RING_PACKED in the stable kernels when > VHOST_GET_FEAETURES is called > - backport this patch on all stable kernels that support vhost-vdpa > > Maybe the last one makes more sense. > > Thanks, > Stefano OK which patches do you want to go to stable exactly? -- MST _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base 2023-06-02 11:36 ` Michael S. Tsirkin @ 2023-06-05 8:17 ` Stefano Garzarella 0 siblings, 0 replies; 22+ messages in thread From: Stefano Garzarella @ 2023-06-05 8:17 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: virtualization, drivers On Fri, Jun 02, 2023 at 07:36:26AM -0400, Michael S. Tsirkin wrote: >On Thu, May 18, 2023 at 09:34:25AM +0200, Stefano Garzarella wrote: >> I think we should do one of these things, though: >> - mask VIRTIO_F_RING_PACKED in the stable kernels when >> VHOST_GET_FEAETURES is called >> - backport this patch on all stable kernels that support vhost-vdpa >> >> Maybe the last one makes more sense. >> >> Thanks, >> Stefano > >OK which patches do you want to go to stable exactly? Initially I was thinking this entire series, but I think it's too risky, so on second thoughtwhat do you think about this: diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index 8c1aefc865f0..ac2152135b23 100644 --- a/drivers/vhost/vdpa.c +++ b/drivers/vhost/vdpa.c @@ -397,6 +397,12 @@ static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep) features = ops->get_device_features(vdpa); + /* + * IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) don't support + * packed virtqueue well yet, so let's filter the feature for now. + */ + features &= ~BIT_ULL(VIRTIO_F_RING_PACKED); + if (copy_to_user(featurep, &features, sizeof(features))) return -EFAULT; I can send the patch ASAP and we can apply it before this series. Thanks, Stefano _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 3/3] vhost_vdpa: support PACKED when setting-getting vring_base 2023-04-24 22:50 [PATCH v2 0/3] vhost_vdpa: better PACKED support Shannon Nelson via Virtualization 2023-04-24 22:50 ` [PATCH v2 1/3] vhost_vdpa: tell vqs about the negotiated Shannon Nelson via Virtualization 2023-04-24 22:50 ` [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base Shannon Nelson via Virtualization @ 2023-04-24 22:50 ` Shannon Nelson via Virtualization 2023-04-25 6:02 ` Jason Wang 2023-04-25 6:08 ` [PATCH v2 0/3] vhost_vdpa: better PACKED support Michael S. Tsirkin 3 siblings, 1 reply; 22+ messages in thread From: Shannon Nelson via Virtualization @ 2023-04-24 22:50 UTC (permalink / raw) To: jasowang, mst, virtualization; +Cc: drivers Use the right structs for PACKED or split vqs when setting and getting the vring base. Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> --- drivers/vhost/vdpa.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index 599b8cc238c7..fe392b67d5be 100644 --- a/drivers/vhost/vdpa.c +++ b/drivers/vhost/vdpa.c @@ -585,7 +585,14 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd, if (r) return r; - vq->last_avail_idx = vq_state.split.avail_index; + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { + vq->last_avail_idx = vq_state.packed.last_avail_idx | + (vq_state.packed.last_avail_counter << 15); + vq->last_used_idx = vq_state.packed.last_used_idx | + (vq_state.packed.last_used_counter << 15); + } else { + vq->last_avail_idx = vq_state.split.avail_index; + } break; } @@ -603,9 +610,15 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd, break; case VHOST_SET_VRING_BASE: - vq_state.split.avail_index = vq->last_avail_idx; - if (ops->set_vq_state(vdpa, idx, &vq_state)) - r = -EINVAL; + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { + vq_state.packed.last_avail_idx = vq->last_avail_idx & 0x7fff; + vq_state.packed.last_avail_counter = !!(vq->last_avail_idx & 0x8000); + vq_state.packed.last_used_idx = vq->last_used_idx & 0x7fff; + vq_state.packed.last_used_counter = !!(vq->last_used_idx & 0x8000); + } else { + vq_state.split.avail_index = vq->last_avail_idx; + } + r = ops->set_vq_state(vdpa, idx, &vq_state); break; case VHOST_SET_VRING_CALL: -- 2.17.1 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/3] vhost_vdpa: support PACKED when setting-getting vring_base 2023-04-24 22:50 ` [PATCH v2 3/3] vhost_vdpa: " Shannon Nelson via Virtualization @ 2023-04-25 6:02 ` Jason Wang 0 siblings, 0 replies; 22+ messages in thread From: Jason Wang @ 2023-04-25 6:02 UTC (permalink / raw) To: Shannon Nelson; +Cc: drivers, virtualization, mst On Tue, Apr 25, 2023 at 6:50 AM Shannon Nelson <shannon.nelson@amd.com> wrote: > > Use the right structs for PACKED or split vqs when setting and > getting the vring base. > > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> Acked-by: Jason Wang <jasowang@redhat.com> Thanks > --- > drivers/vhost/vdpa.c | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c > index 599b8cc238c7..fe392b67d5be 100644 > --- a/drivers/vhost/vdpa.c > +++ b/drivers/vhost/vdpa.c > @@ -585,7 +585,14 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd, > if (r) > return r; > > - vq->last_avail_idx = vq_state.split.avail_index; > + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { > + vq->last_avail_idx = vq_state.packed.last_avail_idx | > + (vq_state.packed.last_avail_counter << 15); > + vq->last_used_idx = vq_state.packed.last_used_idx | > + (vq_state.packed.last_used_counter << 15); > + } else { > + vq->last_avail_idx = vq_state.split.avail_index; > + } > break; > } > > @@ -603,9 +610,15 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd, > break; > > case VHOST_SET_VRING_BASE: > - vq_state.split.avail_index = vq->last_avail_idx; > - if (ops->set_vq_state(vdpa, idx, &vq_state)) > - r = -EINVAL; > + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) { > + vq_state.packed.last_avail_idx = vq->last_avail_idx & 0x7fff; > + vq_state.packed.last_avail_counter = !!(vq->last_avail_idx & 0x8000); > + vq_state.packed.last_used_idx = vq->last_used_idx & 0x7fff; > + vq_state.packed.last_used_counter = !!(vq->last_used_idx & 0x8000); > + } else { > + vq_state.split.avail_index = vq->last_avail_idx; > + } > + r = ops->set_vq_state(vdpa, idx, &vq_state); > break; > > case VHOST_SET_VRING_CALL: > -- > 2.17.1 > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 0/3] vhost_vdpa: better PACKED support 2023-04-24 22:50 [PATCH v2 0/3] vhost_vdpa: better PACKED support Shannon Nelson via Virtualization ` (2 preceding siblings ...) 2023-04-24 22:50 ` [PATCH v2 3/3] vhost_vdpa: " Shannon Nelson via Virtualization @ 2023-04-25 6:08 ` Michael S. Tsirkin 2023-04-25 16:21 ` Shannon Nelson via Virtualization 3 siblings, 1 reply; 22+ messages in thread From: Michael S. Tsirkin @ 2023-04-25 6:08 UTC (permalink / raw) To: Shannon Nelson; +Cc: drivers, virtualization On Mon, Apr 24, 2023 at 03:50:28PM -0700, Shannon Nelson wrote: > While testing our vDPA driver with QEMU we found that vhost_vdpa was > missing some support for PACKED VQs. Adding these helped us get further > in our testing. The first patch makes sure that the vhost_vdpa vqs are > given the feature flags, as done in other vhost variants. The second > and third patches use the feature flags to properly format and pass > set/get_vq requests. This missed the merge window most likely, unless there's a reason to send a second pull towards the end of it. We'll see. > v1: > - included wrap counter in packing answers for VHOST_GET_VRING_BASE > - added comments to vhost_virtqueue fields last_avail_idx and last_used_idx > > v0: > https://lists.linuxfoundation.org/pipermail/virtualization/2023-April/066253.html > - initial version > > Shannon Nelson (3): > vhost_vdpa: tell vqs about the negotiated > vhost: support PACKED when setting-getting vring_base > vhost_vdpa: support PACKED when setting-getting vring_base > > drivers/vhost/vdpa.c | 34 ++++++++++++++++++++++++++++++---- > drivers/vhost/vhost.c | 18 +++++++++++++----- > drivers/vhost/vhost.h | 8 ++++++-- > 3 files changed, 49 insertions(+), 11 deletions(-) > > -- > 2.17.1 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 0/3] vhost_vdpa: better PACKED support 2023-04-25 6:08 ` [PATCH v2 0/3] vhost_vdpa: better PACKED support Michael S. Tsirkin @ 2023-04-25 16:21 ` Shannon Nelson via Virtualization 0 siblings, 0 replies; 22+ messages in thread From: Shannon Nelson via Virtualization @ 2023-04-25 16:21 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: drivers, virtualization On 4/24/23 11:08 PM, Michael S. Tsirkin wrote: > > On Mon, Apr 24, 2023 at 03:50:28PM -0700, Shannon Nelson wrote: >> While testing our vDPA driver with QEMU we found that vhost_vdpa was >> missing some support for PACKED VQs. Adding these helped us get further >> in our testing. The first patch makes sure that the vhost_vdpa vqs are >> given the feature flags, as done in other vhost variants. The second >> and third patches use the feature flags to properly format and pass >> set/get_vq requests. > > This missed the merge window most likely, unless there's > a reason to send a second pull towards the end of it. We'll see. Understood - we'll keep our fingers crossed for good luck :-) Thanks for the help. sln > >> v1: >> - included wrap counter in packing answers for VHOST_GET_VRING_BASE >> - added comments to vhost_virtqueue fields last_avail_idx and last_used_idx >> >> v0: >> https://lists.linuxfoundation.org/pipermail/virtualization/2023-April/066253.html >> - initial version >> >> Shannon Nelson (3): >> vhost_vdpa: tell vqs about the negotiated >> vhost: support PACKED when setting-getting vring_base >> vhost_vdpa: support PACKED when setting-getting vring_base >> >> drivers/vhost/vdpa.c | 34 ++++++++++++++++++++++++++++++---- >> drivers/vhost/vhost.c | 18 +++++++++++++----- >> drivers/vhost/vhost.h | 8 ++++++-- >> 3 files changed, 49 insertions(+), 11 deletions(-) >> >> -- >> 2.17.1 > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2023-06-05 8:17 UTC | newest] Thread overview: 22+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-04-24 22:50 [PATCH v2 0/3] vhost_vdpa: better PACKED support Shannon Nelson via Virtualization 2023-04-24 22:50 ` [PATCH v2 1/3] vhost_vdpa: tell vqs about the negotiated Shannon Nelson via Virtualization 2023-04-24 22:50 ` [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base Shannon Nelson via Virtualization 2023-04-25 6:01 ` Jason Wang 2023-05-09 8:46 ` Stefano Garzarella 2023-05-15 20:41 ` Shannon Nelson via Virtualization 2023-05-16 7:49 ` Stefano Garzarella 2023-05-16 18:26 ` Shannon Nelson via Virtualization 2023-05-17 5:26 ` Jason Wang 2023-05-17 7:00 ` Stefano Garzarella 2023-05-18 5:23 ` Jason Wang 2023-05-18 7:34 ` Stefano Garzarella 2023-05-18 7:52 ` Jason Wang 2023-05-18 8:38 ` Michael S. Tsirkin 2023-05-18 8:59 ` Jason Wang 2023-05-18 9:42 ` Michael S. Tsirkin 2023-06-02 11:36 ` Michael S. Tsirkin 2023-06-05 8:17 ` Stefano Garzarella 2023-04-24 22:50 ` [PATCH v2 3/3] vhost_vdpa: " Shannon Nelson via Virtualization 2023-04-25 6:02 ` Jason Wang 2023-04-25 6:08 ` [PATCH v2 0/3] vhost_vdpa: better PACKED support Michael S. Tsirkin 2023-04-25 16:21 ` Shannon Nelson via Virtualization
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.