Stefano Garzarella
2023-May-17 07:00 UTC
[PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base
On Wed, May 17, 2023 at 7:26?AM Jason Wang <jasowang at redhat.com> wrote:> > On Wed, May 17, 2023 at 2:26?AM Shannon Nelson <shannon.nelson at 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 at 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
Jason Wang
2023-May-18 05:23 UTC
[PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base
On Wed, May 17, 2023 at 3:00?PM Stefano Garzarella <sgarzare at redhat.com> wrote:> > On Wed, May 17, 2023 at 7:26?AM Jason Wang <jasowang at redhat.com> wrote: > > > > On Wed, May 17, 2023 at 2:26?AM Shannon Nelson <shannon.nelson at 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 at 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 >