[PATCH v2 0/3] vhost_vdpa: better PACKED support

[PATCH v2 0/3] vhost_vdpa: better PACKED support

[Shannon Nelson via Virtualization]

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

[PATCH v2 1/3] vhost_vdpa: tell vqs about the negotiated

[Shannon Nelson via Virtualization]

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

[PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base

[Shannon Nelson via Virtualization]

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

[PATCH v2 3/3] vhost_vdpa: support PACKED when setting-getting vring_base

[Shannon Nelson via Virtualization]

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

Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base

[Jason Wang]

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

Re: [PATCH v2 3/3] vhost_vdpa: support PACKED when setting-getting vring_base

[Jason Wang]

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

Re: [PATCH v2 0/3] vhost_vdpa: better PACKED support

[Michael S. Tsirkin]

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

Re: [PATCH v2 0/3] vhost_vdpa: better PACKED support

[Shannon Nelson via 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

Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base

[Stefano Garzarella]

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

Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base

[Shannon Nelson via Virtualization]

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

Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base

[Stefano Garzarella]

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

Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base

[Shannon Nelson via Virtualization]

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

Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base

[Jason Wang]

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

Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base

[Stefano Garzarella]

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

Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base

[Jason Wang]

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

Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base

[Stefano Garzarella]

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

Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base

[Jason Wang]

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

Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base

[Michael S. Tsirkin]

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

Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base

[Jason Wang]

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

Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base

[Michael S. Tsirkin]

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

Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base

[Michael S. Tsirkin]

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

Re: [PATCH v2 2/3] vhost: support PACKED when setting-getting vring_base

[Stefano Garzarella]

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