On Thu, May 18, 2023 at 3:34 PM Stefano Garzarella <sgarzare@xxxxxxxxxx> wrote:
>
> On Thu, May 18, 2023 at 7:24 AM Jason Wang <jasowang@xxxxxxxxxx> wrote:
> >
> > On Wed, May 17, 2023 at 3:00 PM Stefano Garzarella <sgarzare@xxxxxxxxxx> wrote:
> > >
> > > On Wed, May 17, 2023 at 7:26 AM Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > > >
> > > > On Wed, May 17, 2023 at 2:26 AM Shannon Nelson <shannon.nelson@xxxxxxx> 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@xxxxxxx>
> > > > > >>>> ---
> > > > > >>>> 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@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
