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.
Jason Wang
2023-May-17 05:26 UTC
[PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base
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. 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 at pensando.io. > > To view this discussion on the web visit > > https://groups.google.com/a/pensando.io/d/msgid/drivers/q6cmfha36sdkgflwwd3pr4sw7rgajag4ahgjbpfjrr76w4o2b6%403yc7zs5u65s4. >