Stefano Garzarella
2023-May-16 07:49 UTC
[PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base
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. Thanks, Stefano
Shannon Nelson
2023-May-16 18:26 UTC
[PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base
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? 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 at pensando.io. > To view this discussion on the web visit > https://groups.google.com/a/pensando.io/d/msgid/drivers/q6cmfha36sdkgflwwd3pr4sw7rgajag4ahgjbpfjrr76w4o2b6%403yc7zs5u65s4.