[PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Stefano Garzarella]

vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
don't support packed virtqueue well yet, so let's filter the
VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().

This way, even if the device supports it, we don't risk it being
negotiated, then the VMM is unable to set the vring state properly.

Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
Cc: stable@vger.kernel.org
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---

Notes:
    This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
    better PACKED support" series [1] and backported in stable branches.
    
    We can revert it when we are sure that everything is working with
    packed virtqueues.
    
    Thanks,
    Stefano
    
    [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/

 drivers/vhost/vdpa.c | 6 ++++++
 1 file changed, 6 insertions(+)

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;
 

base-commit: 9561de3a55bed6bdd44a12820ba81ec416e705a7
-- 
2.40.1

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Michael S. Tsirkin]

On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
> vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
> don't support packed virtqueue well yet, so let's filter the
> VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
> 
> This way, even if the device supports it, we don't risk it being
> negotiated, then the VMM is unable to set the vring state properly.
> 
> Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> Cc: stable@vger.kernel.org
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> 
> Notes:
>     This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
>     better PACKED support" series [1] and backported in stable branches.
>     
>     We can revert it when we are sure that everything is working with
>     packed virtqueues.
>     
>     Thanks,
>     Stefano
>     
>     [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/

I'm a bit lost here. So why am I merging "better PACKED support" then?
Does this patch make them a NOP?

>  drivers/vhost/vdpa.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> 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;
>  
> 
> base-commit: 9561de3a55bed6bdd44a12820ba81ec416e705a7
> -- 
> 2.40.1

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Stefano Garzarella]

On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote:
>On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
>> vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
>> don't support packed virtqueue well yet, so let's filter the
>> VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
>>
>> This way, even if the device supports it, we don't risk it being
>> negotiated, then the VMM is unable to set the vring state properly.
>>
>> Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>>
>> Notes:
>>     This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
>>     better PACKED support" series [1] and backported in stable branches.
>>
>>     We can revert it when we are sure that everything is working with
>>     packed virtqueues.
>>
>>     Thanks,
>>     Stefano
>>
>>     [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
>
>I'm a bit lost here. So why am I merging "better PACKED support" then?

To really support packed virtqueue with vhost-vdpa, at that point we 
would also have to revert this patch.

I wasn't sure if you wanted to queue the series for this merge window.
In that case do you think it is better to send this patch only for 
stable branches?

>Does this patch make them a NOP?

Yep, after applying the "better PACKED support" series and being sure 
that the IOCTLs of vhost-vdpa support packed virtqueue, we should revert 
this patch.

Let me know if you prefer a different approach.

I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel 
interprets them the right way, when it does not.

Thanks,
Stefano

>
>>  drivers/vhost/vdpa.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> 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;
>>
>>
>> base-commit: 9561de3a55bed6bdd44a12820ba81ec416e705a7
>> --
>> 2.40.1
>

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Michael S. Tsirkin]

On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote:
> On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
> > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
> > > don't support packed virtqueue well yet, so let's filter the
> > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
> > > 
> > > This way, even if the device supports it, we don't risk it being
> > > negotiated, then the VMM is unable to set the vring state properly.
> > > 
> > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > ---
> > > 
> > > Notes:
> > >     This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
> > >     better PACKED support" series [1] and backported in stable branches.
> > > 
> > >     We can revert it when we are sure that everything is working with
> > >     packed virtqueues.
> > > 
> > >     Thanks,
> > >     Stefano
> > > 
> > >     [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
> > 
> > I'm a bit lost here. So why am I merging "better PACKED support" then?
> 
> To really support packed virtqueue with vhost-vdpa, at that point we would
> also have to revert this patch.
> 
> I wasn't sure if you wanted to queue the series for this merge window.
> In that case do you think it is better to send this patch only for stable
> branches?
> > Does this patch make them a NOP?
> 
> Yep, after applying the "better PACKED support" series and being sure that
> the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this
> patch.
> 
> Let me know if you prefer a different approach.
> 
> I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel
> interprets them the right way, when it does not.
> 
> Thanks,
> Stefano
> 

If this fixes a bug can you add Fixes tags to each of them? Then it's ok
to merge in this window. Probably easier than the elaborate
mask/unmask dance.

> > 
> > >  drivers/vhost/vdpa.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > 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;
> > > 
> > > 
> > > base-commit: 9561de3a55bed6bdd44a12820ba81ec416e705a7
> > > --
> > > 2.40.1
> > 

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Stefano Garzarella]

On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote:
>On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote:
>> On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote:
>> > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
>> > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
>> > > don't support packed virtqueue well yet, so let's filter the
>> > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
>> > >
>> > > This way, even if the device supports it, we don't risk it being
>> > > negotiated, then the VMM is unable to set the vring state properly.
>> > >
>> > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
>> > > Cc: stable@vger.kernel.org
>> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> > > ---
>> > >
>> > > Notes:
>> > >     This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
>> > >     better PACKED support" series [1] and backported in stable branches.
>> > >
>> > >     We can revert it when we are sure that everything is working with
>> > >     packed virtqueues.
>> > >
>> > >     Thanks,
>> > >     Stefano
>> > >
>> > >     [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
>> >
>> > I'm a bit lost here. So why am I merging "better PACKED support" then?
>>
>> To really support packed virtqueue with vhost-vdpa, at that point we would
>> also have to revert this patch.
>>
>> I wasn't sure if you wanted to queue the series for this merge window.
>> In that case do you think it is better to send this patch only for stable
>> branches?
>> > Does this patch make them a NOP?
>>
>> Yep, after applying the "better PACKED support" series and being sure 
>> that
>> the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this
>> patch.
>>
>> Let me know if you prefer a different approach.
>>
>> I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel
>> interprets them the right way, when it does not.
>>
>> Thanks,
>> Stefano
>>
>
>If this fixes a bug can you add Fixes tags to each of them? Then it's ok
>to merge in this window. Probably easier than the elaborate
>mask/unmask dance.

CCing Shannon (the original author of the "better PACKED support"
series).

IIUC Shannon is going to send a v3 of that series to fix the
documentation, so Shannon can you also add the Fixes tags?

Thanks,
Stefano

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Michael S. Tsirkin]

On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote:
> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote:
> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote:
> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
> > > > > don't support packed virtqueue well yet, so let's filter the
> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
> > > > >
> > > > > This way, even if the device supports it, we don't risk it being
> > > > > negotiated, then the VMM is unable to set the vring state properly.
> > > > >
> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> > > > > Cc: stable@vger.kernel.org
> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > > ---
> > > > >
> > > > > Notes:
> > > > >     This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
> > > > >     better PACKED support" series [1] and backported in stable branches.
> > > > >
> > > > >     We can revert it when we are sure that everything is working with
> > > > >     packed virtqueues.
> > > > >
> > > > >     Thanks,
> > > > >     Stefano
> > > > >
> > > > >     [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
> > > >
> > > > I'm a bit lost here. So why am I merging "better PACKED support" then?
> > > 
> > > To really support packed virtqueue with vhost-vdpa, at that point we would
> > > also have to revert this patch.
> > > 
> > > I wasn't sure if you wanted to queue the series for this merge window.
> > > In that case do you think it is better to send this patch only for stable
> > > branches?
> > > > Does this patch make them a NOP?
> > > 
> > > Yep, after applying the "better PACKED support" series and being
> > > sure that
> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this
> > > patch.
> > > 
> > > Let me know if you prefer a different approach.
> > > 
> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel
> > > interprets them the right way, when it does not.
> > > 
> > > Thanks,
> > > Stefano
> > > 
> > 
> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok
> > to merge in this window. Probably easier than the elaborate
> > mask/unmask dance.
> 
> CCing Shannon (the original author of the "better PACKED support"
> series).
> 
> IIUC Shannon is going to send a v3 of that series to fix the
> documentation, so Shannon can you also add the Fixes tags?
> 
> Thanks,
> Stefano

Well this is in my tree already. Just reply with
Fixes: <>
to each and I will add these tags.

If I start dropping and rebasing this won't make it in this window.

-- 
MST

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Stefano Garzarella]

On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote:
>On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote:
>> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote:
>> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote:
>> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote:
>> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
>> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
>> > > > > don't support packed virtqueue well yet, so let's filter the
>> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
>> > > > >
>> > > > > This way, even if the device supports it, we don't risk it being
>> > > > > negotiated, then the VMM is unable to set the vring state properly.
>> > > > >
>> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
>> > > > > Cc: stable@vger.kernel.org
>> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> > > > > ---
>> > > > >
>> > > > > Notes:
>> > > > >     This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
>> > > > >     better PACKED support" series [1] and backported in stable branches.
>> > > > >
>> > > > >     We can revert it when we are sure that everything is working with
>> > > > >     packed virtqueues.
>> > > > >
>> > > > >     Thanks,
>> > > > >     Stefano
>> > > > >
>> > > > >     [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
>> > > >
>> > > > I'm a bit lost here. So why am I merging "better PACKED support" then?
>> > >
>> > > To really support packed virtqueue with vhost-vdpa, at that point we would
>> > > also have to revert this patch.
>> > >
>> > > I wasn't sure if you wanted to queue the series for this merge window.
>> > > In that case do you think it is better to send this patch only for stable
>> > > branches?
>> > > > Does this patch make them a NOP?
>> > >
>> > > Yep, after applying the "better PACKED support" series and being
>> > > sure that
>> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this
>> > > patch.
>> > >
>> > > Let me know if you prefer a different approach.
>> > >
>> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel
>> > > interprets them the right way, when it does not.
>> > >
>> > > Thanks,
>> > > Stefano
>> > >
>> >
>> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok
>> > to merge in this window. Probably easier than the elaborate
>> > mask/unmask dance.
>>
>> CCing Shannon (the original author of the "better PACKED support"
>> series).
>>
>> IIUC Shannon is going to send a v3 of that series to fix the
>> documentation, so Shannon can you also add the Fixes tags?
>>
>> Thanks,
>> Stefano
>
>Well this is in my tree already. Just reply with
>Fixes: <>
>to each and I will add these tags.

I tried, but it is not easy since we added the support for packed 
virtqueue in vdpa and vhost incrementally.

Initially I was thinking of adding the same tag used here:

Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")

Then I discovered that vq_state wasn't there, so I was thinking of

Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()")

So we would have to backport quite a few patches into the stable branches.
I don't know if it's worth it...

I still think it is better to disable packed in the stable branches,
otherwise I have to make a list of all the patches we need.

Any other ideas?

Thanks,
Stefano

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Michael S. Tsirkin]

On Mon, Jun 05, 2023 at 04:56:37PM +0200, Stefano Garzarella wrote:
> On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote:
> > > On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote:
> > > > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote:
> > > > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote:
> > > > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
> > > > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
> > > > > > > don't support packed virtqueue well yet, so let's filter the
> > > > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
> > > > > > >
> > > > > > > This way, even if the device supports it, we don't risk it being
> > > > > > > negotiated, then the VMM is unable to set the vring state properly.
> > > > > > >
> > > > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> > > > > > > Cc: stable@vger.kernel.org
> > > > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > ---
> > > > > > >
> > > > > > > Notes:
> > > > > > >     This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
> > > > > > >     better PACKED support" series [1] and backported in stable branches.
> > > > > > >
> > > > > > >     We can revert it when we are sure that everything is working with
> > > > > > >     packed virtqueues.
> > > > > > >
> > > > > > >     Thanks,
> > > > > > >     Stefano
> > > > > > >
> > > > > > >     [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
> > > > > >
> > > > > > I'm a bit lost here. So why am I merging "better PACKED support" then?
> > > > >
> > > > > To really support packed virtqueue with vhost-vdpa, at that point we would
> > > > > also have to revert this patch.
> > > > >
> > > > > I wasn't sure if you wanted to queue the series for this merge window.
> > > > > In that case do you think it is better to send this patch only for stable
> > > > > branches?
> > > > > > Does this patch make them a NOP?
> > > > >
> > > > > Yep, after applying the "better PACKED support" series and being
> > > > > sure that
> > > > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this
> > > > > patch.
> > > > >
> > > > > Let me know if you prefer a different approach.
> > > > >
> > > > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel
> > > > > interprets them the right way, when it does not.
> > > > >
> > > > > Thanks,
> > > > > Stefano
> > > > >
> > > >
> > > > If this fixes a bug can you add Fixes tags to each of them? Then it's ok
> > > > to merge in this window. Probably easier than the elaborate
> > > > mask/unmask dance.
> > > 
> > > CCing Shannon (the original author of the "better PACKED support"
> > > series).
> > > 
> > > IIUC Shannon is going to send a v3 of that series to fix the
> > > documentation, so Shannon can you also add the Fixes tags?
> > > 
> > > Thanks,
> > > Stefano
> > 
> > Well this is in my tree already. Just reply with
> > Fixes: <>
> > to each and I will add these tags.
> 
> I tried, but it is not easy since we added the support for packed virtqueue
> in vdpa and vhost incrementally.
> 
> Initially I was thinking of adding the same tag used here:
> 
> Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> 
> Then I discovered that vq_state wasn't there, so I was thinking of
> 
> Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()")
> 
> So we would have to backport quite a few patches into the stable branches.
> I don't know if it's worth it...
> 
> I still think it is better to disable packed in the stable branches,
> otherwise I have to make a list of all the patches we need.
> 
> Any other ideas?
> 
> Thanks,
> Stefano

OK so. You want me to apply this one now, and fixes in the next
kernel?

-- 
MST

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Jason Wang]

On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote:
> >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote:
> >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote:
> >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote:
> >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote:
> >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
> >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
> >> > > > > don't support packed virtqueue well yet, so let's filter the
> >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
> >> > > > >
> >> > > > > This way, even if the device supports it, we don't risk it being
> >> > > > > negotiated, then the VMM is unable to set the vring state properly.
> >> > > > >
> >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> >> > > > > Cc: stable@vger.kernel.org
> >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> >> > > > > ---
> >> > > > >
> >> > > > > Notes:
> >> > > > >     This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
> >> > > > >     better PACKED support" series [1] and backported in stable branches.
> >> > > > >
> >> > > > >     We can revert it when we are sure that everything is working with
> >> > > > >     packed virtqueues.
> >> > > > >
> >> > > > >     Thanks,
> >> > > > >     Stefano
> >> > > > >
> >> > > > >     [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
> >> > > >
> >> > > > I'm a bit lost here. So why am I merging "better PACKED support" then?
> >> > >
> >> > > To really support packed virtqueue with vhost-vdpa, at that point we would
> >> > > also have to revert this patch.
> >> > >
> >> > > I wasn't sure if you wanted to queue the series for this merge window.
> >> > > In that case do you think it is better to send this patch only for stable
> >> > > branches?
> >> > > > Does this patch make them a NOP?
> >> > >
> >> > > Yep, after applying the "better PACKED support" series and being
> >> > > sure that
> >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this
> >> > > patch.
> >> > >
> >> > > Let me know if you prefer a different approach.
> >> > >
> >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel
> >> > > interprets them the right way, when it does not.
> >> > >
> >> > > Thanks,
> >> > > Stefano
> >> > >
> >> >
> >> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok
> >> > to merge in this window. Probably easier than the elaborate
> >> > mask/unmask dance.
> >>
> >> CCing Shannon (the original author of the "better PACKED support"
> >> series).
> >>
> >> IIUC Shannon is going to send a v3 of that series to fix the
> >> documentation, so Shannon can you also add the Fixes tags?
> >>
> >> Thanks,
> >> Stefano
> >
> >Well this is in my tree already. Just reply with
> >Fixes: <>
> >to each and I will add these tags.
>
> I tried, but it is not easy since we added the support for packed
> virtqueue in vdpa and vhost incrementally.
>
> Initially I was thinking of adding the same tag used here:
>
> Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
>
> Then I discovered that vq_state wasn't there, so I was thinking of
>
> Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()")
>
> So we would have to backport quite a few patches into the stable branches.
> I don't know if it's worth it...
>
> I still think it is better to disable packed in the stable branches,
> otherwise I have to make a list of all the patches we need.
>
> Any other ideas?

AFAIK, except for vp_vdpa, pds seems to be the first parent that
supports packed virtqueue. Users should not notice anything wrong if
they don't use packed virtqueue. And the problem of vp_vdpa + packed
virtqueue came since the day0 of vp_vdpa. It seems fine to do nothing
I guess.

Thanks

>
> Thanks,
> Stefano
>
>

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Stefano Garzarella]

On Mon, Jun 05, 2023 at 05:44:50PM -0400, Michael S. Tsirkin wrote:
>On Mon, Jun 05, 2023 at 04:56:37PM +0200, Stefano Garzarella wrote:
>> On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote:
>> > On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote:
>> > > On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote:
>> > > > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote:
>> > > > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote:
>> > > > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
>> > > > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
>> > > > > > > don't support packed virtqueue well yet, so let's filter the
>> > > > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
>> > > > > > >
>> > > > > > > This way, even if the device supports it, we don't risk it being
>> > > > > > > negotiated, then the VMM is unable to set the vring state properly.
>> > > > > > >
>> > > > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
>> > > > > > > Cc: stable@vger.kernel.org
>> > > > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> > > > > > > ---
>> > > > > > >
>> > > > > > > Notes:
>> > > > > > >     This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
>> > > > > > >     better PACKED support" series [1] and backported in stable branches.
>> > > > > > >
>> > > > > > >     We can revert it when we are sure that everything is working with
>> > > > > > >     packed virtqueues.
>> > > > > > >
>> > > > > > >     Thanks,
>> > > > > > >     Stefano
>> > > > > > >
>> > > > > > >     [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
>> > > > > >
>> > > > > > I'm a bit lost here. So why am I merging "better PACKED support" then?
>> > > > >
>> > > > > To really support packed virtqueue with vhost-vdpa, at that point we would
>> > > > > also have to revert this patch.
>> > > > >
>> > > > > I wasn't sure if you wanted to queue the series for this merge window.
>> > > > > In that case do you think it is better to send this patch only for stable
>> > > > > branches?
>> > > > > > Does this patch make them a NOP?
>> > > > >
>> > > > > Yep, after applying the "better PACKED support" series and being
>> > > > > sure that
>> > > > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this
>> > > > > patch.
>> > > > >
>> > > > > Let me know if you prefer a different approach.
>> > > > >
>> > > > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel
>> > > > > interprets them the right way, when it does not.
>> > > > >
>> > > > > Thanks,
>> > > > > Stefano
>> > > > >
>> > > >
>> > > > If this fixes a bug can you add Fixes tags to each of them? Then it's ok
>> > > > to merge in this window. Probably easier than the elaborate
>> > > > mask/unmask dance.
>> > >
>> > > CCing Shannon (the original author of the "better PACKED support"
>> > > series).
>> > >
>> > > IIUC Shannon is going to send a v3 of that series to fix the
>> > > documentation, so Shannon can you also add the Fixes tags?
>> > >
>> > > Thanks,
>> > > Stefano
>> >
>> > Well this is in my tree already. Just reply with
>> > Fixes: <>
>> > to each and I will add these tags.
>>
>> I tried, but it is not easy since we added the support for packed virtqueue
>> in vdpa and vhost incrementally.
>>
>> Initially I was thinking of adding the same tag used here:
>>
>> Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
>>
>> Then I discovered that vq_state wasn't there, so I was thinking of
>>
>> Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()")
>>
>> So we would have to backport quite a few patches into the stable branches.
>> I don't know if it's worth it...
>>
>> I still think it is better to disable packed in the stable branches,
>> otherwise I have to make a list of all the patches we need.
>>
>> Any other ideas?
>>
>> Thanks,
>> Stefano
>
>OK so. You want me to apply this one now, and fixes in the next
>kernel?

Yep, it seems to me the least risky approach.

Thanks,
Stefano

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Stefano Garzarella]

On Tue, Jun 06, 2023 at 09:29:22AM +0800, Jason Wang wrote:
>On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote:
>> >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote:
>> >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote:
>> >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote:
>> >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote:
>> >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
>> >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
>> >> > > > > don't support packed virtqueue well yet, so let's filter the
>> >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
>> >> > > > >
>> >> > > > > This way, even if the device supports it, we don't risk it being
>> >> > > > > negotiated, then the VMM is unable to set the vring state properly.
>> >> > > > >
>> >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
>> >> > > > > Cc: stable@vger.kernel.org
>> >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> >> > > > > ---
>> >> > > > >
>> >> > > > > Notes:
>> >> > > > >     This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
>> >> > > > >     better PACKED support" series [1] and backported in stable branches.
>> >> > > > >
>> >> > > > >     We can revert it when we are sure that everything is working with
>> >> > > > >     packed virtqueues.
>> >> > > > >
>> >> > > > >     Thanks,
>> >> > > > >     Stefano
>> >> > > > >
>> >> > > > >     [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
>> >> > > >
>> >> > > > I'm a bit lost here. So why am I merging "better PACKED support" then?
>> >> > >
>> >> > > To really support packed virtqueue with vhost-vdpa, at that point we would
>> >> > > also have to revert this patch.
>> >> > >
>> >> > > I wasn't sure if you wanted to queue the series for this merge window.
>> >> > > In that case do you think it is better to send this patch only for stable
>> >> > > branches?
>> >> > > > Does this patch make them a NOP?
>> >> > >
>> >> > > Yep, after applying the "better PACKED support" series and being
>> >> > > sure that
>> >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this
>> >> > > patch.
>> >> > >
>> >> > > Let me know if you prefer a different approach.
>> >> > >
>> >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel
>> >> > > interprets them the right way, when it does not.
>> >> > >
>> >> > > Thanks,
>> >> > > Stefano
>> >> > >
>> >> >
>> >> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok
>> >> > to merge in this window. Probably easier than the elaborate
>> >> > mask/unmask dance.
>> >>
>> >> CCing Shannon (the original author of the "better PACKED support"
>> >> series).
>> >>
>> >> IIUC Shannon is going to send a v3 of that series to fix the
>> >> documentation, so Shannon can you also add the Fixes tags?
>> >>
>> >> Thanks,
>> >> Stefano
>> >
>> >Well this is in my tree already. Just reply with
>> >Fixes: <>
>> >to each and I will add these tags.
>>
>> I tried, but it is not easy since we added the support for packed
>> virtqueue in vdpa and vhost incrementally.
>>
>> Initially I was thinking of adding the same tag used here:
>>
>> Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
>>
>> Then I discovered that vq_state wasn't there, so I was thinking of
>>
>> Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()")
>>
>> So we would have to backport quite a few patches into the stable branches.
>> I don't know if it's worth it...
>>
>> I still think it is better to disable packed in the stable branches,
>> otherwise I have to make a list of all the patches we need.
>>
>> Any other ideas?
>
>AFAIK, except for vp_vdpa, pds seems to be the first parent that

IIUC also vduse and snet supports packed virtqueue.

>supports packed virtqueue. Users should not notice anything wrong if
>they don't use packed virtqueue. And the problem of vp_vdpa + packed
>virtqueue came since the day0 of vp_vdpa. It seems fine to do nothing
>I guess.

Okay, maybe I'm overthinking it, not having a specific problem I don't
object, it was just a concern about uAPI.

Thanks,
Stefano

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Michael S. Tsirkin]

On Tue, Jun 06, 2023 at 09:29:22AM +0800, Jason Wang wrote:
> On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >
> > On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote:
> > >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote:
> > >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote:
> > >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote:
> > >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote:
> > >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
> > >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
> > >> > > > > don't support packed virtqueue well yet, so let's filter the
> > >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
> > >> > > > >
> > >> > > > > This way, even if the device supports it, we don't risk it being
> > >> > > > > negotiated, then the VMM is unable to set the vring state properly.
> > >> > > > >
> > >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> > >> > > > > Cc: stable@vger.kernel.org
> > >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > >> > > > > ---
> > >> > > > >
> > >> > > > > Notes:
> > >> > > > >     This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
> > >> > > > >     better PACKED support" series [1] and backported in stable branches.
> > >> > > > >
> > >> > > > >     We can revert it when we are sure that everything is working with
> > >> > > > >     packed virtqueues.
> > >> > > > >
> > >> > > > >     Thanks,
> > >> > > > >     Stefano
> > >> > > > >
> > >> > > > >     [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
> > >> > > >
> > >> > > > I'm a bit lost here. So why am I merging "better PACKED support" then?
> > >> > >
> > >> > > To really support packed virtqueue with vhost-vdpa, at that point we would
> > >> > > also have to revert this patch.
> > >> > >
> > >> > > I wasn't sure if you wanted to queue the series for this merge window.
> > >> > > In that case do you think it is better to send this patch only for stable
> > >> > > branches?
> > >> > > > Does this patch make them a NOP?
> > >> > >
> > >> > > Yep, after applying the "better PACKED support" series and being
> > >> > > sure that
> > >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this
> > >> > > patch.
> > >> > >
> > >> > > Let me know if you prefer a different approach.
> > >> > >
> > >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel
> > >> > > interprets them the right way, when it does not.
> > >> > >
> > >> > > Thanks,
> > >> > > Stefano
> > >> > >
> > >> >
> > >> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok
> > >> > to merge in this window. Probably easier than the elaborate
> > >> > mask/unmask dance.
> > >>
> > >> CCing Shannon (the original author of the "better PACKED support"
> > >> series).
> > >>
> > >> IIUC Shannon is going to send a v3 of that series to fix the
> > >> documentation, so Shannon can you also add the Fixes tags?
> > >>
> > >> Thanks,
> > >> Stefano
> > >
> > >Well this is in my tree already. Just reply with
> > >Fixes: <>
> > >to each and I will add these tags.
> >
> > I tried, but it is not easy since we added the support for packed
> > virtqueue in vdpa and vhost incrementally.
> >
> > Initially I was thinking of adding the same tag used here:
> >
> > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> >
> > Then I discovered that vq_state wasn't there, so I was thinking of
> >
> > Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()")
> >
> > So we would have to backport quite a few patches into the stable branches.
> > I don't know if it's worth it...
> >
> > I still think it is better to disable packed in the stable branches,
> > otherwise I have to make a list of all the patches we need.
> >
> > Any other ideas?
> 
> AFAIK, except for vp_vdpa, pds seems to be the first parent that
> supports packed virtqueue. Users should not notice anything wrong if
> they don't use packed virtqueue. And the problem of vp_vdpa + packed
> virtqueue came since the day0 of vp_vdpa. It seems fine to do nothing
> I guess.
> 
> Thanks


I have a question though, what if down the road there
is a new feature that needs more changes? It will be
broken too just like PACKED no?
Shouldn't vdpa have an allowlist of features it knows how
to support?

> >
> > Thanks,
> > Stefano
> >
> >

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Stefano Garzarella]

On Tue, Jun 6, 2023 at 2:58 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Jun 06, 2023 at 09:29:22AM +0800, Jason Wang wrote:
> > On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > >
> > > On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote:
> > > >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote:
> > > >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote:
> > > >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote:
> > > >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote:
> > > >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
> > > >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
> > > >> > > > > don't support packed virtqueue well yet, so let's filter the
> > > >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
> > > >> > > > >
> > > >> > > > > This way, even if the device supports it, we don't risk it being
> > > >> > > > > negotiated, then the VMM is unable to set the vring state properly.
> > > >> > > > >
> > > >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> > > >> > > > > Cc: stable@vger.kernel.org
> > > >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > >> > > > > ---
> > > >> > > > >
> > > >> > > > > Notes:
> > > >> > > > >     This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
> > > >> > > > >     better PACKED support" series [1] and backported in stable branches.
> > > >> > > > >
> > > >> > > > >     We can revert it when we are sure that everything is working with
> > > >> > > > >     packed virtqueues.
> > > >> > > > >
> > > >> > > > >     Thanks,
> > > >> > > > >     Stefano
> > > >> > > > >
> > > >> > > > >     [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
> > > >> > > >
> > > >> > > > I'm a bit lost here. So why am I merging "better PACKED support" then?
> > > >> > >
> > > >> > > To really support packed virtqueue with vhost-vdpa, at that point we would
> > > >> > > also have to revert this patch.
> > > >> > >
> > > >> > > I wasn't sure if you wanted to queue the series for this merge window.
> > > >> > > In that case do you think it is better to send this patch only for stable
> > > >> > > branches?
> > > >> > > > Does this patch make them a NOP?
> > > >> > >
> > > >> > > Yep, after applying the "better PACKED support" series and being
> > > >> > > sure that
> > > >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this
> > > >> > > patch.
> > > >> > >
> > > >> > > Let me know if you prefer a different approach.
> > > >> > >
> > > >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel
> > > >> > > interprets them the right way, when it does not.
> > > >> > >
> > > >> > > Thanks,
> > > >> > > Stefano
> > > >> > >
> > > >> >
> > > >> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok
> > > >> > to merge in this window. Probably easier than the elaborate
> > > >> > mask/unmask dance.
> > > >>
> > > >> CCing Shannon (the original author of the "better PACKED support"
> > > >> series).
> > > >>
> > > >> IIUC Shannon is going to send a v3 of that series to fix the
> > > >> documentation, so Shannon can you also add the Fixes tags?
> > > >>
> > > >> Thanks,
> > > >> Stefano
> > > >
> > > >Well this is in my tree already. Just reply with
> > > >Fixes: <>
> > > >to each and I will add these tags.
> > >
> > > I tried, but it is not easy since we added the support for packed
> > > virtqueue in vdpa and vhost incrementally.
> > >
> > > Initially I was thinking of adding the same tag used here:
> > >
> > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> > >
> > > Then I discovered that vq_state wasn't there, so I was thinking of
> > >
> > > Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()")
> > >
> > > So we would have to backport quite a few patches into the stable branches.
> > > I don't know if it's worth it...
> > >
> > > I still think it is better to disable packed in the stable branches,
> > > otherwise I have to make a list of all the patches we need.
> > >
> > > Any other ideas?
> >
> > AFAIK, except for vp_vdpa, pds seems to be the first parent that
> > supports packed virtqueue. Users should not notice anything wrong if
> > they don't use packed virtqueue. And the problem of vp_vdpa + packed
> > virtqueue came since the day0 of vp_vdpa. It seems fine to do nothing
> > I guess.
> >
> > Thanks
>
>
> I have a question though, what if down the road there
> is a new feature that needs more changes? It will be
> broken too just like PACKED no?
> Shouldn't vdpa have an allowlist of features it knows how
> to support?

It looks like we had it, but we took it out (by the way, we were
enabling packed even though we didn't support it):
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b

The only problem I see is that for each new feature we have to modify 
the kernel.
Could we have new features that don't require handling by vhost-vdpa?

Thanks,
Stefano

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Michael S. Tsirkin]

On Wed, Jun 07, 2023 at 10:39:15AM +0200, Stefano Garzarella wrote:
> On Tue, Jun 6, 2023 at 2:58 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Jun 06, 2023 at 09:29:22AM +0800, Jason Wang wrote:
> > > On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > > >
> > > > On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote:
> > > > >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote:
> > > > >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote:
> > > > >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote:
> > > > >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote:
> > > > >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
> > > > >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
> > > > >> > > > > don't support packed virtqueue well yet, so let's filter the
> > > > >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
> > > > >> > > > >
> > > > >> > > > > This way, even if the device supports it, we don't risk it being
> > > > >> > > > > negotiated, then the VMM is unable to set the vring state properly.
> > > > >> > > > >
> > > > >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> > > > >> > > > > Cc: stable@vger.kernel.org
> > > > >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > >> > > > > ---
> > > > >> > > > >
> > > > >> > > > > Notes:
> > > > >> > > > >     This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
> > > > >> > > > >     better PACKED support" series [1] and backported in stable branches.
> > > > >> > > > >
> > > > >> > > > >     We can revert it when we are sure that everything is working with
> > > > >> > > > >     packed virtqueues.
> > > > >> > > > >
> > > > >> > > > >     Thanks,
> > > > >> > > > >     Stefano
> > > > >> > > > >
> > > > >> > > > >     [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
> > > > >> > > >
> > > > >> > > > I'm a bit lost here. So why am I merging "better PACKED support" then?
> > > > >> > >
> > > > >> > > To really support packed virtqueue with vhost-vdpa, at that point we would
> > > > >> > > also have to revert this patch.
> > > > >> > >
> > > > >> > > I wasn't sure if you wanted to queue the series for this merge window.
> > > > >> > > In that case do you think it is better to send this patch only for stable
> > > > >> > > branches?
> > > > >> > > > Does this patch make them a NOP?
> > > > >> > >
> > > > >> > > Yep, after applying the "better PACKED support" series and being
> > > > >> > > sure that
> > > > >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this
> > > > >> > > patch.
> > > > >> > >
> > > > >> > > Let me know if you prefer a different approach.
> > > > >> > >
> > > > >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel
> > > > >> > > interprets them the right way, when it does not.
> > > > >> > >
> > > > >> > > Thanks,
> > > > >> > > Stefano
> > > > >> > >
> > > > >> >
> > > > >> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok
> > > > >> > to merge in this window. Probably easier than the elaborate
> > > > >> > mask/unmask dance.
> > > > >>
> > > > >> CCing Shannon (the original author of the "better PACKED support"
> > > > >> series).
> > > > >>
> > > > >> IIUC Shannon is going to send a v3 of that series to fix the
> > > > >> documentation, so Shannon can you also add the Fixes tags?
> > > > >>
> > > > >> Thanks,
> > > > >> Stefano
> > > > >
> > > > >Well this is in my tree already. Just reply with
> > > > >Fixes: <>
> > > > >to each and I will add these tags.
> > > >
> > > > I tried, but it is not easy since we added the support for packed
> > > > virtqueue in vdpa and vhost incrementally.
> > > >
> > > > Initially I was thinking of adding the same tag used here:
> > > >
> > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> > > >
> > > > Then I discovered that vq_state wasn't there, so I was thinking of
> > > >
> > > > Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()")
> > > >
> > > > So we would have to backport quite a few patches into the stable branches.
> > > > I don't know if it's worth it...
> > > >
> > > > I still think it is better to disable packed in the stable branches,
> > > > otherwise I have to make a list of all the patches we need.
> > > >
> > > > Any other ideas?
> > >
> > > AFAIK, except for vp_vdpa, pds seems to be the first parent that
> > > supports packed virtqueue. Users should not notice anything wrong if
> > > they don't use packed virtqueue. And the problem of vp_vdpa + packed
> > > virtqueue came since the day0 of vp_vdpa. It seems fine to do nothing
> > > I guess.
> > >
> > > Thanks
> >
> >
> > I have a question though, what if down the road there
> > is a new feature that needs more changes? It will be
> > broken too just like PACKED no?
> > Shouldn't vdpa have an allowlist of features it knows how
> > to support?
> 
> It looks like we had it, but we took it out (by the way, we were
> enabling packed even though we didn't support it):
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b
> 
> The only problem I see is that for each new feature we have to modify 
> the kernel.
> Could we have new features that don't require handling by vhost-vdpa?
> 
> Thanks,
> Stefano

Jason what do you say to reverting this?

-- 
MST

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Michael S. Tsirkin]

On Tue, Jun 06, 2023 at 12:09:11PM +0200, Stefano Garzarella wrote:
> On Mon, Jun 05, 2023 at 05:44:50PM -0400, Michael S. Tsirkin wrote:
> > On Mon, Jun 05, 2023 at 04:56:37PM +0200, Stefano Garzarella wrote:
> > > On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote:
> > > > On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote:
> > > > > On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote:
> > > > > > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote:
> > > > > > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote:
> > > > > > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
> > > > > > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
> > > > > > > > > don't support packed virtqueue well yet, so let's filter the
> > > > > > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
> > > > > > > > >
> > > > > > > > > This way, even if the device supports it, we don't risk it being
> > > > > > > > > negotiated, then the VMM is unable to set the vring state properly.
> > > > > > > > >
> > > > > > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> > > > > > > > > Cc: stable@vger.kernel.org
> > > > > > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > Notes:
> > > > > > > > >     This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
> > > > > > > > >     better PACKED support" series [1] and backported in stable branches.
> > > > > > > > >
> > > > > > > > >     We can revert it when we are sure that everything is working with
> > > > > > > > >     packed virtqueues.
> > > > > > > > >
> > > > > > > > >     Thanks,
> > > > > > > > >     Stefano
> > > > > > > > >
> > > > > > > > >     [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
> > > > > > > >
> > > > > > > > I'm a bit lost here. So why am I merging "better PACKED support" then?
> > > > > > >
> > > > > > > To really support packed virtqueue with vhost-vdpa, at that point we would
> > > > > > > also have to revert this patch.
> > > > > > >
> > > > > > > I wasn't sure if you wanted to queue the series for this merge window.
> > > > > > > In that case do you think it is better to send this patch only for stable
> > > > > > > branches?
> > > > > > > > Does this patch make them a NOP?
> > > > > > >
> > > > > > > Yep, after applying the "better PACKED support" series and being
> > > > > > > sure that
> > > > > > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this
> > > > > > > patch.
> > > > > > >
> > > > > > > Let me know if you prefer a different approach.
> > > > > > >
> > > > > > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel
> > > > > > > interprets them the right way, when it does not.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Stefano
> > > > > > >
> > > > > >
> > > > > > If this fixes a bug can you add Fixes tags to each of them? Then it's ok
> > > > > > to merge in this window. Probably easier than the elaborate
> > > > > > mask/unmask dance.
> > > > >
> > > > > CCing Shannon (the original author of the "better PACKED support"
> > > > > series).
> > > > >
> > > > > IIUC Shannon is going to send a v3 of that series to fix the
> > > > > documentation, so Shannon can you also add the Fixes tags?
> > > > >
> > > > > Thanks,
> > > > > Stefano
> > > >
> > > > Well this is in my tree already. Just reply with
> > > > Fixes: <>
> > > > to each and I will add these tags.
> > > 
> > > I tried, but it is not easy since we added the support for packed virtqueue
> > > in vdpa and vhost incrementally.
> > > 
> > > Initially I was thinking of adding the same tag used here:
> > > 
> > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> > > 
> > > Then I discovered that vq_state wasn't there, so I was thinking of
> > > 
> > > Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()")
> > > 
> > > So we would have to backport quite a few patches into the stable branches.
> > > I don't know if it's worth it...
> > > 
> > > I still think it is better to disable packed in the stable branches,
> > > otherwise I have to make a list of all the patches we need.
> > > 
> > > Any other ideas?
> > > 
> > > Thanks,
> > > Stefano
> > 
> > OK so. You want me to apply this one now, and fixes in the next
> > kernel?
> 
> Yep, it seems to me the least risky approach.
> 
> Thanks,
> Stefano

I'd like something more general though. Bring back the allowlist?
Jason, your thoughts?

-- 
MST

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Jason Wang]

On Wed, Jun 7, 2023 at 5:43 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Jun 07, 2023 at 10:39:15AM +0200, Stefano Garzarella wrote:
> > On Tue, Jun 6, 2023 at 2:58 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, Jun 06, 2023 at 09:29:22AM +0800, Jason Wang wrote:
> > > > On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > > > >
> > > > > On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote:
> > > > > >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote:
> > > > > >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote:
> > > > > >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote:
> > > > > >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote:
> > > > > >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
> > > > > >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
> > > > > >> > > > > don't support packed virtqueue well yet, so let's filter the
> > > > > >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
> > > > > >> > > > >
> > > > > >> > > > > This way, even if the device supports it, we don't risk it being
> > > > > >> > > > > negotiated, then the VMM is unable to set the vring state properly.
> > > > > >> > > > >
> > > > > >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> > > > > >> > > > > Cc: stable@vger.kernel.org
> > > > > >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > > >> > > > > ---
> > > > > >> > > > >
> > > > > >> > > > > Notes:
> > > > > >> > > > >     This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
> > > > > >> > > > >     better PACKED support" series [1] and backported in stable branches.
> > > > > >> > > > >
> > > > > >> > > > >     We can revert it when we are sure that everything is working with
> > > > > >> > > > >     packed virtqueues.
> > > > > >> > > > >
> > > > > >> > > > >     Thanks,
> > > > > >> > > > >     Stefano
> > > > > >> > > > >
> > > > > >> > > > >     [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
> > > > > >> > > >
> > > > > >> > > > I'm a bit lost here. So why am I merging "better PACKED support" then?
> > > > > >> > >
> > > > > >> > > To really support packed virtqueue with vhost-vdpa, at that point we would
> > > > > >> > > also have to revert this patch.
> > > > > >> > >
> > > > > >> > > I wasn't sure if you wanted to queue the series for this merge window.
> > > > > >> > > In that case do you think it is better to send this patch only for stable
> > > > > >> > > branches?
> > > > > >> > > > Does this patch make them a NOP?
> > > > > >> > >
> > > > > >> > > Yep, after applying the "better PACKED support" series and being
> > > > > >> > > sure that
> > > > > >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this
> > > > > >> > > patch.
> > > > > >> > >
> > > > > >> > > Let me know if you prefer a different approach.
> > > > > >> > >
> > > > > >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel
> > > > > >> > > interprets them the right way, when it does not.
> > > > > >> > >
> > > > > >> > > Thanks,
> > > > > >> > > Stefano
> > > > > >> > >
> > > > > >> >
> > > > > >> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok
> > > > > >> > to merge in this window. Probably easier than the elaborate
> > > > > >> > mask/unmask dance.
> > > > > >>
> > > > > >> CCing Shannon (the original author of the "better PACKED support"
> > > > > >> series).
> > > > > >>
> > > > > >> IIUC Shannon is going to send a v3 of that series to fix the
> > > > > >> documentation, so Shannon can you also add the Fixes tags?
> > > > > >>
> > > > > >> Thanks,
> > > > > >> Stefano
> > > > > >
> > > > > >Well this is in my tree already. Just reply with
> > > > > >Fixes: <>
> > > > > >to each and I will add these tags.
> > > > >
> > > > > I tried, but it is not easy since we added the support for packed
> > > > > virtqueue in vdpa and vhost incrementally.
> > > > >
> > > > > Initially I was thinking of adding the same tag used here:
> > > > >
> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> > > > >
> > > > > Then I discovered that vq_state wasn't there, so I was thinking of
> > > > >
> > > > > Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()")
> > > > >
> > > > > So we would have to backport quite a few patches into the stable branches.
> > > > > I don't know if it's worth it...
> > > > >
> > > > > I still think it is better to disable packed in the stable branches,
> > > > > otherwise I have to make a list of all the patches we need.
> > > > >
> > > > > Any other ideas?
> > > >
> > > > AFAIK, except for vp_vdpa, pds seems to be the first parent that
> > > > supports packed virtqueue. Users should not notice anything wrong if
> > > > they don't use packed virtqueue. And the problem of vp_vdpa + packed
> > > > virtqueue came since the day0 of vp_vdpa. It seems fine to do nothing
> > > > I guess.
> > > >
> > > > Thanks
> > >
> > >
> > > I have a question though, what if down the road there
> > > is a new feature that needs more changes? It will be
> > > broken too just like PACKED no?
> > > Shouldn't vdpa have an allowlist of features it knows how
> > > to support?
> >
> > It looks like we had it, but we took it out (by the way, we were
> > enabling packed even though we didn't support it):
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b
> >
> > The only problem I see is that for each new feature we have to modify
> > the kernel.
> > Could we have new features that don't require handling by vhost-vdpa?
> >
> > Thanks,
> > Stefano
>
> Jason what do you say to reverting this?

I may miss something but I don't see any problem with vDPA core.

It's the duty of the parents to advertise the features it has. For example,

1) If some kernel version that is packed is not supported via
set_vq_state, parents should not advertise PACKED features in this
case.
2) If the kernel has support packed set_vq_state(), but it's emulated
cvq doesn't support, parents should not advertise PACKED as well

If a parent violates the above 2, it looks like a bug of the parents.

Thanks

>
> --
> MST
>

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Michael S. Tsirkin]

On Thu, Jun 08, 2023 at 08:42:15AM +0800, Jason Wang wrote:
> On Wed, Jun 7, 2023 at 5:43 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Jun 07, 2023 at 10:39:15AM +0200, Stefano Garzarella wrote:
> > > On Tue, Jun 6, 2023 at 2:58 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Tue, Jun 06, 2023 at 09:29:22AM +0800, Jason Wang wrote:
> > > > > On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > > > > >
> > > > > > On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote:
> > > > > > >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote:
> > > > > > >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote:
> > > > > > >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote:
> > > > > > >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote:
> > > > > > >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
> > > > > > >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
> > > > > > >> > > > > don't support packed virtqueue well yet, so let's filter the
> > > > > > >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
> > > > > > >> > > > >
> > > > > > >> > > > > This way, even if the device supports it, we don't risk it being
> > > > > > >> > > > > negotiated, then the VMM is unable to set the vring state properly.
> > > > > > >> > > > >
> > > > > > >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> > > > > > >> > > > > Cc: stable@vger.kernel.org
> > > > > > >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > >> > > > > ---
> > > > > > >> > > > >
> > > > > > >> > > > > Notes:
> > > > > > >> > > > >     This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
> > > > > > >> > > > >     better PACKED support" series [1] and backported in stable branches.
> > > > > > >> > > > >
> > > > > > >> > > > >     We can revert it when we are sure that everything is working with
> > > > > > >> > > > >     packed virtqueues.
> > > > > > >> > > > >
> > > > > > >> > > > >     Thanks,
> > > > > > >> > > > >     Stefano
> > > > > > >> > > > >
> > > > > > >> > > > >     [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
> > > > > > >> > > >
> > > > > > >> > > > I'm a bit lost here. So why am I merging "better PACKED support" then?
> > > > > > >> > >
> > > > > > >> > > To really support packed virtqueue with vhost-vdpa, at that point we would
> > > > > > >> > > also have to revert this patch.
> > > > > > >> > >
> > > > > > >> > > I wasn't sure if you wanted to queue the series for this merge window.
> > > > > > >> > > In that case do you think it is better to send this patch only for stable
> > > > > > >> > > branches?
> > > > > > >> > > > Does this patch make them a NOP?
> > > > > > >> > >
> > > > > > >> > > Yep, after applying the "better PACKED support" series and being
> > > > > > >> > > sure that
> > > > > > >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this
> > > > > > >> > > patch.
> > > > > > >> > >
> > > > > > >> > > Let me know if you prefer a different approach.
> > > > > > >> > >
> > > > > > >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel
> > > > > > >> > > interprets them the right way, when it does not.
> > > > > > >> > >
> > > > > > >> > > Thanks,
> > > > > > >> > > Stefano
> > > > > > >> > >
> > > > > > >> >
> > > > > > >> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok
> > > > > > >> > to merge in this window. Probably easier than the elaborate
> > > > > > >> > mask/unmask dance.
> > > > > > >>
> > > > > > >> CCing Shannon (the original author of the "better PACKED support"
> > > > > > >> series).
> > > > > > >>
> > > > > > >> IIUC Shannon is going to send a v3 of that series to fix the
> > > > > > >> documentation, so Shannon can you also add the Fixes tags?
> > > > > > >>
> > > > > > >> Thanks,
> > > > > > >> Stefano
> > > > > > >
> > > > > > >Well this is in my tree already. Just reply with
> > > > > > >Fixes: <>
> > > > > > >to each and I will add these tags.
> > > > > >
> > > > > > I tried, but it is not easy since we added the support for packed
> > > > > > virtqueue in vdpa and vhost incrementally.
> > > > > >
> > > > > > Initially I was thinking of adding the same tag used here:
> > > > > >
> > > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> > > > > >
> > > > > > Then I discovered that vq_state wasn't there, so I was thinking of
> > > > > >
> > > > > > Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()")
> > > > > >
> > > > > > So we would have to backport quite a few patches into the stable branches.
> > > > > > I don't know if it's worth it...
> > > > > >
> > > > > > I still think it is better to disable packed in the stable branches,
> > > > > > otherwise I have to make a list of all the patches we need.
> > > > > >
> > > > > > Any other ideas?
> > > > >
> > > > > AFAIK, except for vp_vdpa, pds seems to be the first parent that
> > > > > supports packed virtqueue. Users should not notice anything wrong if
> > > > > they don't use packed virtqueue. And the problem of vp_vdpa + packed
> > > > > virtqueue came since the day0 of vp_vdpa. It seems fine to do nothing
> > > > > I guess.
> > > > >
> > > > > Thanks
> > > >
> > > >
> > > > I have a question though, what if down the road there
> > > > is a new feature that needs more changes? It will be
> > > > broken too just like PACKED no?
> > > > Shouldn't vdpa have an allowlist of features it knows how
> > > > to support?
> > >
> > > It looks like we had it, but we took it out (by the way, we were
> > > enabling packed even though we didn't support it):
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b
> > >
> > > The only problem I see is that for each new feature we have to modify
> > > the kernel.
> > > Could we have new features that don't require handling by vhost-vdpa?
> > >
> > > Thanks,
> > > Stefano
> >
> > Jason what do you say to reverting this?
> 
> I may miss something but I don't see any problem with vDPA core.
> 
> It's the duty of the parents to advertise the features it has. For example,
> 
> 1) If some kernel version that is packed is not supported via
> set_vq_state, parents should not advertise PACKED features in this
> case.
> 2) If the kernel has support packed set_vq_state(), but it's emulated
> cvq doesn't support, parents should not advertise PACKED as well
> 
> If a parent violates the above 2, it looks like a bug of the parents.
> 
> Thanks

Yes but what about vhost_vdpa? Talking about that not the core.
Should that not have a whitelist of features
since it interprets ioctls differently depending on this?

> >
> > --
> > MST
> >

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Jason Wang]

On Thu, Jun 8, 2023 at 2:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Jun 08, 2023 at 08:42:15AM +0800, Jason Wang wrote:
> > On Wed, Jun 7, 2023 at 5:43 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Wed, Jun 07, 2023 at 10:39:15AM +0200, Stefano Garzarella wrote:
> > > > On Tue, Jun 6, 2023 at 2:58 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Tue, Jun 06, 2023 at 09:29:22AM +0800, Jason Wang wrote:
> > > > > > On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > > > > > >
> > > > > > > On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote:
> > > > > > > >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote:
> > > > > > > >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote:
> > > > > > > >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote:
> > > > > > > >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote:
> > > > > > > >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
> > > > > > > >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
> > > > > > > >> > > > > don't support packed virtqueue well yet, so let's filter the
> > > > > > > >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
> > > > > > > >> > > > >
> > > > > > > >> > > > > This way, even if the device supports it, we don't risk it being
> > > > > > > >> > > > > negotiated, then the VMM is unable to set the vring state properly.
> > > > > > > >> > > > >
> > > > > > > >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> > > > > > > >> > > > > Cc: stable@vger.kernel.org
> > > > > > > >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > >> > > > > ---
> > > > > > > >> > > > >
> > > > > > > >> > > > > Notes:
> > > > > > > >> > > > >     This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
> > > > > > > >> > > > >     better PACKED support" series [1] and backported in stable branches.
> > > > > > > >> > > > >
> > > > > > > >> > > > >     We can revert it when we are sure that everything is working with
> > > > > > > >> > > > >     packed virtqueues.
> > > > > > > >> > > > >
> > > > > > > >> > > > >     Thanks,
> > > > > > > >> > > > >     Stefano
> > > > > > > >> > > > >
> > > > > > > >> > > > >     [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
> > > > > > > >> > > >
> > > > > > > >> > > > I'm a bit lost here. So why am I merging "better PACKED support" then?
> > > > > > > >> > >
> > > > > > > >> > > To really support packed virtqueue with vhost-vdpa, at that point we would
> > > > > > > >> > > also have to revert this patch.
> > > > > > > >> > >
> > > > > > > >> > > I wasn't sure if you wanted to queue the series for this merge window.
> > > > > > > >> > > In that case do you think it is better to send this patch only for stable
> > > > > > > >> > > branches?
> > > > > > > >> > > > Does this patch make them a NOP?
> > > > > > > >> > >
> > > > > > > >> > > Yep, after applying the "better PACKED support" series and being
> > > > > > > >> > > sure that
> > > > > > > >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this
> > > > > > > >> > > patch.
> > > > > > > >> > >
> > > > > > > >> > > Let me know if you prefer a different approach.
> > > > > > > >> > >
> > > > > > > >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel
> > > > > > > >> > > interprets them the right way, when it does not.
> > > > > > > >> > >
> > > > > > > >> > > Thanks,
> > > > > > > >> > > Stefano
> > > > > > > >> > >
> > > > > > > >> >
> > > > > > > >> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok
> > > > > > > >> > to merge in this window. Probably easier than the elaborate
> > > > > > > >> > mask/unmask dance.
> > > > > > > >>
> > > > > > > >> CCing Shannon (the original author of the "better PACKED support"
> > > > > > > >> series).
> > > > > > > >>
> > > > > > > >> IIUC Shannon is going to send a v3 of that series to fix the
> > > > > > > >> documentation, so Shannon can you also add the Fixes tags?
> > > > > > > >>
> > > > > > > >> Thanks,
> > > > > > > >> Stefano
> > > > > > > >
> > > > > > > >Well this is in my tree already. Just reply with
> > > > > > > >Fixes: <>
> > > > > > > >to each and I will add these tags.
> > > > > > >
> > > > > > > I tried, but it is not easy since we added the support for packed
> > > > > > > virtqueue in vdpa and vhost incrementally.
> > > > > > >
> > > > > > > Initially I was thinking of adding the same tag used here:
> > > > > > >
> > > > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> > > > > > >
> > > > > > > Then I discovered that vq_state wasn't there, so I was thinking of
> > > > > > >
> > > > > > > Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()")
> > > > > > >
> > > > > > > So we would have to backport quite a few patches into the stable branches.
> > > > > > > I don't know if it's worth it...
> > > > > > >
> > > > > > > I still think it is better to disable packed in the stable branches,
> > > > > > > otherwise I have to make a list of all the patches we need.
> > > > > > >
> > > > > > > Any other ideas?
> > > > > >
> > > > > > AFAIK, except for vp_vdpa, pds seems to be the first parent that
> > > > > > supports packed virtqueue. Users should not notice anything wrong if
> > > > > > they don't use packed virtqueue. And the problem of vp_vdpa + packed
> > > > > > virtqueue came since the day0 of vp_vdpa. It seems fine to do nothing
> > > > > > I guess.
> > > > > >
> > > > > > Thanks
> > > > >
> > > > >
> > > > > I have a question though, what if down the road there
> > > > > is a new feature that needs more changes? It will be
> > > > > broken too just like PACKED no?
> > > > > Shouldn't vdpa have an allowlist of features it knows how
> > > > > to support?
> > > >
> > > > It looks like we had it, but we took it out (by the way, we were
> > > > enabling packed even though we didn't support it):
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b
> > > >
> > > > The only problem I see is that for each new feature we have to modify
> > > > the kernel.
> > > > Could we have new features that don't require handling by vhost-vdpa?
> > > >
> > > > Thanks,
> > > > Stefano
> > >
> > > Jason what do you say to reverting this?
> >
> > I may miss something but I don't see any problem with vDPA core.
> >
> > It's the duty of the parents to advertise the features it has. For example,
> >
> > 1) If some kernel version that is packed is not supported via
> > set_vq_state, parents should not advertise PACKED features in this
> > case.
> > 2) If the kernel has support packed set_vq_state(), but it's emulated
> > cvq doesn't support, parents should not advertise PACKED as well
> >
> > If a parent violates the above 2, it looks like a bug of the parents.
> >
> > Thanks
>
> Yes but what about vhost_vdpa? Talking about that not the core.

Not sure it's a good idea to workaround parent bugs via vhost-vDPA.

> Should that not have a whitelist of features
> since it interprets ioctls differently depending on this?

If there's a bug, it might only matter the following setup:

SET_VRING_BASE/GET_VRING_BASE + VDUSE.

This seems to be broken since VDUSE was introduced. If we really want
to backport something, it could be a fix to filter out PACKED in
VDUSE?

Thanks

>
> > >
> > > --
> > > MST
> > >
>

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Stefano Garzarella]

On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote:

[...]

>> > > > > I have a question though, what if down the road there
>> > > > > is a new feature that needs more changes? It will be
>> > > > > broken too just like PACKED no?
>> > > > > Shouldn't vdpa have an allowlist of features it knows how
>> > > > > to support?
>> > > >
>> > > > It looks like we had it, but we took it out (by the way, we were
>> > > > enabling packed even though we didn't support it):
>> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b
>> > > >
>> > > > The only problem I see is that for each new feature we have to modify
>> > > > the kernel.
>> > > > Could we have new features that don't require handling by vhost-vdpa?
>> > > >
>> > > > Thanks,
>> > > > Stefano
>> > >
>> > > Jason what do you say to reverting this?
>> >
>> > I may miss something but I don't see any problem with vDPA core.
>> >
>> > It's the duty of the parents to advertise the features it has. For example,
>> >
>> > 1) If some kernel version that is packed is not supported via
>> > set_vq_state, parents should not advertise PACKED features in this
>> > case.
>> > 2) If the kernel has support packed set_vq_state(), but it's emulated
>> > cvq doesn't support, parents should not advertise PACKED as well
>> >
>> > If a parent violates the above 2, it looks like a bug of the parents.
>> >
>> > Thanks
>>
>> Yes but what about vhost_vdpa? Talking about that not the core.
>
>Not sure it's a good idea to workaround parent bugs via vhost-vDPA.

Sorry, I'm getting lost...
We were talking about the fact that vhost-vdpa doesn't handle
SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before
that series [1], no?

The parents seem okay, but maybe I missed a few things.

[1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/

>
>> Should that not have a whitelist of features
>> since it interprets ioctls differently depending on this?
>
>If there's a bug, it might only matter the following setup:
>
>SET_VRING_BASE/GET_VRING_BASE + VDUSE.
>
>This seems to be broken since VDUSE was introduced. If we really want
>to backport something, it could be a fix to filter out PACKED in
>VDUSE?

mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa.
I think VDUSE works fine with packed virtqueue using virtio-vdpa
(I haven't tried), so why should we filter PACKED in VDUSE?

Thanks,
Stefano

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Jason Wang]

On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote:
>
> [...]
>
> >> > > > > I have a question though, what if down the road there
> >> > > > > is a new feature that needs more changes? It will be
> >> > > > > broken too just like PACKED no?
> >> > > > > Shouldn't vdpa have an allowlist of features it knows how
> >> > > > > to support?
> >> > > >
> >> > > > It looks like we had it, but we took it out (by the way, we were
> >> > > > enabling packed even though we didn't support it):
> >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b
> >> > > >
> >> > > > The only problem I see is that for each new feature we have to modify
> >> > > > the kernel.
> >> > > > Could we have new features that don't require handling by vhost-vdpa?
> >> > > >
> >> > > > Thanks,
> >> > > > Stefano
> >> > >
> >> > > Jason what do you say to reverting this?
> >> >
> >> > I may miss something but I don't see any problem with vDPA core.
> >> >
> >> > It's the duty of the parents to advertise the features it has. For example,
> >> >
> >> > 1) If some kernel version that is packed is not supported via
> >> > set_vq_state, parents should not advertise PACKED features in this
> >> > case.
> >> > 2) If the kernel has support packed set_vq_state(), but it's emulated
> >> > cvq doesn't support, parents should not advertise PACKED as well
> >> >
> >> > If a parent violates the above 2, it looks like a bug of the parents.
> >> >
> >> > Thanks
> >>
> >> Yes but what about vhost_vdpa? Talking about that not the core.
> >
> >Not sure it's a good idea to workaround parent bugs via vhost-vDPA.
>
> Sorry, I'm getting lost...
> We were talking about the fact that vhost-vdpa doesn't handle
> SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before
> that series [1], no?
>
> The parents seem okay, but maybe I missed a few things.
>
> [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/

Yes, more below.

>
> >
> >> Should that not have a whitelist of features
> >> since it interprets ioctls differently depending on this?
> >
> >If there's a bug, it might only matter the following setup:
> >
> >SET_VRING_BASE/GET_VRING_BASE + VDUSE.
> >
> >This seems to be broken since VDUSE was introduced. If we really want
> >to backport something, it could be a fix to filter out PACKED in
> >VDUSE?
>
> mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa.
> I think VDUSE works fine with packed virtqueue using virtio-vdpa
> (I haven't tried), so why should we filter PACKED in VDUSE?

I don't think we need any filtering since:

PACKED features has been advertised to userspace via uAPI since
6234f80574d7569444d8718355fa2838e92b158b. Once we relax in uAPI, it
would be very hard to restrict it again. For the userspace that tries
to negotiate PACKED:

1) if it doesn't use SET_VRING_BASE/GET_VRING_BASE, everything works well
2) if it uses SET_VRING_BASE/GET_VRING_BASE. it might fail or break silently

If we backport the fixes to -stable, we may break the application at
least in the case 1).

Thanks

>
> Thanks,
> Stefano
>

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Stefano Garzarella]

On Thu, Jun 08, 2023 at 05:00:00PM +0800, Jason Wang wrote:
>On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote:
>>
>> [...]
>>
>> >> > > > > I have a question though, what if down the road there
>> >> > > > > is a new feature that needs more changes? It will be
>> >> > > > > broken too just like PACKED no?
>> >> > > > > Shouldn't vdpa have an allowlist of features it knows how
>> >> > > > > to support?
>> >> > > >
>> >> > > > It looks like we had it, but we took it out (by the way, we were
>> >> > > > enabling packed even though we didn't support it):
>> >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b
>> >> > > >
>> >> > > > The only problem I see is that for each new feature we have to modify
>> >> > > > the kernel.
>> >> > > > Could we have new features that don't require handling by vhost-vdpa?
>> >> > > >
>> >> > > > Thanks,
>> >> > > > Stefano
>> >> > >
>> >> > > Jason what do you say to reverting this?
>> >> >
>> >> > I may miss something but I don't see any problem with vDPA core.
>> >> >
>> >> > It's the duty of the parents to advertise the features it has. For example,
>> >> >
>> >> > 1) If some kernel version that is packed is not supported via
>> >> > set_vq_state, parents should not advertise PACKED features in this
>> >> > case.
>> >> > 2) If the kernel has support packed set_vq_state(), but it's emulated
>> >> > cvq doesn't support, parents should not advertise PACKED as well
>> >> >
>> >> > If a parent violates the above 2, it looks like a bug of the parents.
>> >> >
>> >> > Thanks
>> >>
>> >> Yes but what about vhost_vdpa? Talking about that not the core.
>> >
>> >Not sure it's a good idea to workaround parent bugs via vhost-vDPA.
>>
>> Sorry, I'm getting lost...
>> We were talking about the fact that vhost-vdpa doesn't handle
>> SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before
>> that series [1], no?
>>
>> The parents seem okay, but maybe I missed a few things.
>>
>> [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
>
>Yes, more below.
>
>>
>> >
>> >> Should that not have a whitelist of features
>> >> since it interprets ioctls differently depending on this?
>> >
>> >If there's a bug, it might only matter the following setup:
>> >
>> >SET_VRING_BASE/GET_VRING_BASE + VDUSE.
>> >
>> >This seems to be broken since VDUSE was introduced. If we really want
>> >to backport something, it could be a fix to filter out PACKED in
>> >VDUSE?
>>
>> mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa.
>> I think VDUSE works fine with packed virtqueue using virtio-vdpa
>> (I haven't tried), so why should we filter PACKED in VDUSE?
>
>I don't think we need any filtering since:
>
>PACKED features has been advertised to userspace via uAPI since
>6234f80574d7569444d8718355fa2838e92b158b. Once we relax in uAPI, it
>would be very hard to restrict it again. For the userspace that tries
>to negotiate PACKED:
>
>1) if it doesn't use SET_VRING_BASE/GET_VRING_BASE, everything works well
>2) if it uses SET_VRING_BASE/GET_VRING_BASE. it might fail or break silently
>
>If we backport the fixes to -stable, we may break the application at
>least in the case 1).

Okay, I see now, thanks for the details!

Maybe instead of "break silently", we can return an explicit error for
SET_VRING_BASE/GET_VRING_BASE in stable branches.
But if there are not many cases, we can leave it like that.

I was just concerned about how does the user space understand that it
can use SET_VRING_BASE/GET_VRING_BASE for PACKED virtqueues in a given
kernel or not.

Thanks,
Stefano

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Jason Wang]

On Thu, Jun 8, 2023 at 5:21 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Thu, Jun 08, 2023 at 05:00:00PM +0800, Jason Wang wrote:
> >On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >>
> >> On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote:
> >>
> >> [...]
> >>
> >> >> > > > > I have a question though, what if down the road there
> >> >> > > > > is a new feature that needs more changes? It will be
> >> >> > > > > broken too just like PACKED no?
> >> >> > > > > Shouldn't vdpa have an allowlist of features it knows how
> >> >> > > > > to support?
> >> >> > > >
> >> >> > > > It looks like we had it, but we took it out (by the way, we were
> >> >> > > > enabling packed even though we didn't support it):
> >> >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b
> >> >> > > >
> >> >> > > > The only problem I see is that for each new feature we have to modify
> >> >> > > > the kernel.
> >> >> > > > Could we have new features that don't require handling by vhost-vdpa?
> >> >> > > >
> >> >> > > > Thanks,
> >> >> > > > Stefano
> >> >> > >
> >> >> > > Jason what do you say to reverting this?
> >> >> >
> >> >> > I may miss something but I don't see any problem with vDPA core.
> >> >> >
> >> >> > It's the duty of the parents to advertise the features it has. For example,
> >> >> >
> >> >> > 1) If some kernel version that is packed is not supported via
> >> >> > set_vq_state, parents should not advertise PACKED features in this
> >> >> > case.
> >> >> > 2) If the kernel has support packed set_vq_state(), but it's emulated
> >> >> > cvq doesn't support, parents should not advertise PACKED as well
> >> >> >
> >> >> > If a parent violates the above 2, it looks like a bug of the parents.
> >> >> >
> >> >> > Thanks
> >> >>
> >> >> Yes but what about vhost_vdpa? Talking about that not the core.
> >> >
> >> >Not sure it's a good idea to workaround parent bugs via vhost-vDPA.
> >>
> >> Sorry, I'm getting lost...
> >> We were talking about the fact that vhost-vdpa doesn't handle
> >> SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before
> >> that series [1], no?
> >>
> >> The parents seem okay, but maybe I missed a few things.
> >>
> >> [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
> >
> >Yes, more below.
> >
> >>
> >> >
> >> >> Should that not have a whitelist of features
> >> >> since it interprets ioctls differently depending on this?
> >> >
> >> >If there's a bug, it might only matter the following setup:
> >> >
> >> >SET_VRING_BASE/GET_VRING_BASE + VDUSE.
> >> >
> >> >This seems to be broken since VDUSE was introduced. If we really want
> >> >to backport something, it could be a fix to filter out PACKED in
> >> >VDUSE?
> >>
> >> mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa.
> >> I think VDUSE works fine with packed virtqueue using virtio-vdpa
> >> (I haven't tried), so why should we filter PACKED in VDUSE?
> >
> >I don't think we need any filtering since:
> >
> >PACKED features has been advertised to userspace via uAPI since
> >6234f80574d7569444d8718355fa2838e92b158b. Once we relax in uAPI, it
> >would be very hard to restrict it again. For the userspace that tries
> >to negotiate PACKED:
> >
> >1) if it doesn't use SET_VRING_BASE/GET_VRING_BASE, everything works well
> >2) if it uses SET_VRING_BASE/GET_VRING_BASE. it might fail or break silently
> >
> >If we backport the fixes to -stable, we may break the application at
> >least in the case 1).
>
> Okay, I see now, thanks for the details!
>
> Maybe instead of "break silently", we can return an explicit error for
> SET_VRING_BASE/GET_VRING_BASE in stable branches.
> But if there are not many cases, we can leave it like that.

A second thought, if we need to do something for stable. is it better
if we just backport Shannon's series to stable?

>
> I was just concerned about how does the user space understand that it
> can use SET_VRING_BASE/GET_VRING_BASE for PACKED virtqueues in a given
> kernel or not.

My understanding is that if packed is advertised, the application
should assume SET/GET_VRING_BASE work.

Thanks

>
> Thanks,
> Stefano
>

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Stefano Garzarella]

On Thu, Jun 08, 2023 at 05:29:58PM +0800, Jason Wang wrote:
>On Thu, Jun 8, 2023 at 5:21 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Thu, Jun 08, 2023 at 05:00:00PM +0800, Jason Wang wrote:
>> >On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >>
>> >> On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote:
>> >>
>> >> [...]
>> >>
>> >> >> > > > > I have a question though, what if down the road there
>> >> >> > > > > is a new feature that needs more changes? It will be
>> >> >> > > > > broken too just like PACKED no?
>> >> >> > > > > Shouldn't vdpa have an allowlist of features it knows how
>> >> >> > > > > to support?
>> >> >> > > >
>> >> >> > > > It looks like we had it, but we took it out (by the way, we were
>> >> >> > > > enabling packed even though we didn't support it):
>> >> >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b
>> >> >> > > >
>> >> >> > > > The only problem I see is that for each new feature we have to modify
>> >> >> > > > the kernel.
>> >> >> > > > Could we have new features that don't require handling by vhost-vdpa?
>> >> >> > > >
>> >> >> > > > Thanks,
>> >> >> > > > Stefano
>> >> >> > >
>> >> >> > > Jason what do you say to reverting this?
>> >> >> >
>> >> >> > I may miss something but I don't see any problem with vDPA core.
>> >> >> >
>> >> >> > It's the duty of the parents to advertise the features it has. For example,
>> >> >> >
>> >> >> > 1) If some kernel version that is packed is not supported via
>> >> >> > set_vq_state, parents should not advertise PACKED features in this
>> >> >> > case.
>> >> >> > 2) If the kernel has support packed set_vq_state(), but it's emulated
>> >> >> > cvq doesn't support, parents should not advertise PACKED as well
>> >> >> >
>> >> >> > If a parent violates the above 2, it looks like a bug of the parents.
>> >> >> >
>> >> >> > Thanks
>> >> >>
>> >> >> Yes but what about vhost_vdpa? Talking about that not the core.
>> >> >
>> >> >Not sure it's a good idea to workaround parent bugs via vhost-vDPA.
>> >>
>> >> Sorry, I'm getting lost...
>> >> We were talking about the fact that vhost-vdpa doesn't handle
>> >> SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before
>> >> that series [1], no?
>> >>
>> >> The parents seem okay, but maybe I missed a few things.
>> >>
>> >> [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
>> >
>> >Yes, more below.
>> >
>> >>
>> >> >
>> >> >> Should that not have a whitelist of features
>> >> >> since it interprets ioctls differently depending on this?
>> >> >
>> >> >If there's a bug, it might only matter the following setup:
>> >> >
>> >> >SET_VRING_BASE/GET_VRING_BASE + VDUSE.
>> >> >
>> >> >This seems to be broken since VDUSE was introduced. If we really want
>> >> >to backport something, it could be a fix to filter out PACKED in
>> >> >VDUSE?
>> >>
>> >> mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa.
>> >> I think VDUSE works fine with packed virtqueue using virtio-vdpa
>> >> (I haven't tried), so why should we filter PACKED in VDUSE?
>> >
>> >I don't think we need any filtering since:
>> >
>> >PACKED features has been advertised to userspace via uAPI since
>> >6234f80574d7569444d8718355fa2838e92b158b. Once we relax in uAPI, it
>> >would be very hard to restrict it again. For the userspace that tries
>> >to negotiate PACKED:
>> >
>> >1) if it doesn't use SET_VRING_BASE/GET_VRING_BASE, everything works well
>> >2) if it uses SET_VRING_BASE/GET_VRING_BASE. it might fail or break silently
>> >
>> >If we backport the fixes to -stable, we may break the application at
>> >least in the case 1).
>>
>> Okay, I see now, thanks for the details!
>>
>> Maybe instead of "break silently", we can return an explicit error for
>> SET_VRING_BASE/GET_VRING_BASE in stable branches.
>> But if there are not many cases, we can leave it like that.
>
>A second thought, if we need to do something for stable. is it better
>if we just backport Shannon's series to stable?

I tried to look at it, but it looks like we have to backport quite a few
patches, I wrote a few things here:

https://lore.kernel.org/virtualization/32ejjuvhvcicv7wjuetkv34qtlpa657n4zlow4eq3fsi2twozk@iqnd2t5tw2an/

But if you think it's the best way, though, we can take a better look
at how many patches are to backport and whether it's risky or not.

>
>>
>> I was just concerned about how does the user space understand that it
>> can use SET_VRING_BASE/GET_VRING_BASE for PACKED virtqueues in a given
>> kernel or not.
>
>My understanding is that if packed is advertised, the application
>should assume SET/GET_VRING_BASE work.
>

Same here. So as an alternative to backporting a large set of patches,
I proposed to completely disable packed for stable branches where
vhost-vdpa IOCTLs doesn't support them very well.

Thanks,
Stefano

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Michael S. Tsirkin]

On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote:
> On Thu, Jun 8, 2023 at 2:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Jun 08, 2023 at 08:42:15AM +0800, Jason Wang wrote:
> > > On Wed, Jun 7, 2023 at 5:43 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Wed, Jun 07, 2023 at 10:39:15AM +0200, Stefano Garzarella wrote:
> > > > > On Tue, Jun 6, 2023 at 2:58 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > >
> > > > > > On Tue, Jun 06, 2023 at 09:29:22AM +0800, Jason Wang wrote:
> > > > > > > On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > > > > > > >
> > > > > > > > On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote:
> > > > > > > > >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote:
> > > > > > > > >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote:
> > > > > > > > >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote:
> > > > > > > > >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote:
> > > > > > > > >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
> > > > > > > > >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
> > > > > > > > >> > > > > don't support packed virtqueue well yet, so let's filter the
> > > > > > > > >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
> > > > > > > > >> > > > >
> > > > > > > > >> > > > > This way, even if the device supports it, we don't risk it being
> > > > > > > > >> > > > > negotiated, then the VMM is unable to set the vring state properly.
> > > > > > > > >> > > > >
> > > > > > > > >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> > > > > > > > >> > > > > Cc: stable@vger.kernel.org
> > > > > > > > >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > > >> > > > > ---
> > > > > > > > >> > > > >
> > > > > > > > >> > > > > Notes:
> > > > > > > > >> > > > >     This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
> > > > > > > > >> > > > >     better PACKED support" series [1] and backported in stable branches.
> > > > > > > > >> > > > >
> > > > > > > > >> > > > >     We can revert it when we are sure that everything is working with
> > > > > > > > >> > > > >     packed virtqueues.
> > > > > > > > >> > > > >
> > > > > > > > >> > > > >     Thanks,
> > > > > > > > >> > > > >     Stefano
> > > > > > > > >> > > > >
> > > > > > > > >> > > > >     [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
> > > > > > > > >> > > >
> > > > > > > > >> > > > I'm a bit lost here. So why am I merging "better PACKED support" then?
> > > > > > > > >> > >
> > > > > > > > >> > > To really support packed virtqueue with vhost-vdpa, at that point we would
> > > > > > > > >> > > also have to revert this patch.
> > > > > > > > >> > >
> > > > > > > > >> > > I wasn't sure if you wanted to queue the series for this merge window.
> > > > > > > > >> > > In that case do you think it is better to send this patch only for stable
> > > > > > > > >> > > branches?
> > > > > > > > >> > > > Does this patch make them a NOP?
> > > > > > > > >> > >
> > > > > > > > >> > > Yep, after applying the "better PACKED support" series and being
> > > > > > > > >> > > sure that
> > > > > > > > >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this
> > > > > > > > >> > > patch.
> > > > > > > > >> > >
> > > > > > > > >> > > Let me know if you prefer a different approach.
> > > > > > > > >> > >
> > > > > > > > >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel
> > > > > > > > >> > > interprets them the right way, when it does not.
> > > > > > > > >> > >
> > > > > > > > >> > > Thanks,
> > > > > > > > >> > > Stefano
> > > > > > > > >> > >
> > > > > > > > >> >
> > > > > > > > >> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok
> > > > > > > > >> > to merge in this window. Probably easier than the elaborate
> > > > > > > > >> > mask/unmask dance.
> > > > > > > > >>
> > > > > > > > >> CCing Shannon (the original author of the "better PACKED support"
> > > > > > > > >> series).
> > > > > > > > >>
> > > > > > > > >> IIUC Shannon is going to send a v3 of that series to fix the
> > > > > > > > >> documentation, so Shannon can you also add the Fixes tags?
> > > > > > > > >>
> > > > > > > > >> Thanks,
> > > > > > > > >> Stefano
> > > > > > > > >
> > > > > > > > >Well this is in my tree already. Just reply with
> > > > > > > > >Fixes: <>
> > > > > > > > >to each and I will add these tags.
> > > > > > > >
> > > > > > > > I tried, but it is not easy since we added the support for packed
> > > > > > > > virtqueue in vdpa and vhost incrementally.
> > > > > > > >
> > > > > > > > Initially I was thinking of adding the same tag used here:
> > > > > > > >
> > > > > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> > > > > > > >
> > > > > > > > Then I discovered that vq_state wasn't there, so I was thinking of
> > > > > > > >
> > > > > > > > Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()")
> > > > > > > >
> > > > > > > > So we would have to backport quite a few patches into the stable branches.
> > > > > > > > I don't know if it's worth it...
> > > > > > > >
> > > > > > > > I still think it is better to disable packed in the stable branches,
> > > > > > > > otherwise I have to make a list of all the patches we need.
> > > > > > > >
> > > > > > > > Any other ideas?
> > > > > > >
> > > > > > > AFAIK, except for vp_vdpa, pds seems to be the first parent that
> > > > > > > supports packed virtqueue. Users should not notice anything wrong if
> > > > > > > they don't use packed virtqueue. And the problem of vp_vdpa + packed
> > > > > > > virtqueue came since the day0 of vp_vdpa. It seems fine to do nothing
> > > > > > > I guess.
> > > > > > >
> > > > > > > Thanks
> > > > > >
> > > > > >
> > > > > > I have a question though, what if down the road there
> > > > > > is a new feature that needs more changes? It will be
> > > > > > broken too just like PACKED no?
> > > > > > Shouldn't vdpa have an allowlist of features it knows how
> > > > > > to support?
> > > > >
> > > > > It looks like we had it, but we took it out (by the way, we were
> > > > > enabling packed even though we didn't support it):
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b
> > > > >
> > > > > The only problem I see is that for each new feature we have to modify
> > > > > the kernel.
> > > > > Could we have new features that don't require handling by vhost-vdpa?
> > > > >
> > > > > Thanks,
> > > > > Stefano
> > > >
> > > > Jason what do you say to reverting this?
> > >
> > > I may miss something but I don't see any problem with vDPA core.
> > >
> > > It's the duty of the parents to advertise the features it has. For example,
> > >
> > > 1) If some kernel version that is packed is not supported via
> > > set_vq_state, parents should not advertise PACKED features in this
> > > case.
> > > 2) If the kernel has support packed set_vq_state(), but it's emulated
> > > cvq doesn't support, parents should not advertise PACKED as well
> > >
> > > If a parent violates the above 2, it looks like a bug of the parents.
> > >
> > > Thanks
> >
> > Yes but what about vhost_vdpa? Talking about that not the core.
> 
> Not sure it's a good idea to workaround parent bugs via vhost-vDPA.

these are not parent bugs. vhost-vdpa did not pass ioctl data
correctly to parent, right?

> > Should that not have a whitelist of features
> > since it interprets ioctls differently depending on this?
> 
> If there's a bug, it might only matter the following setup:
> 
> SET_VRING_BASE/GET_VRING_BASE + VDUSE.

Why does it not apply to any parent?

> This seems to be broken since VDUSE was introduced. If we really want
> to backport something, it could be a fix to filter out PACKED in
> VDUSE?
> 
> Thanks

what prevents VDUSE from supporting packed?

> >
> > > >
> > > > --
> > > > MST
> > > >
> >

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Michael S. Tsirkin]

On Thu, Jun 08, 2023 at 05:00:00PM +0800, Jason Wang wrote:
> On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >
> > On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote:
> >
> > [...]
> >
> > >> > > > > I have a question though, what if down the road there
> > >> > > > > is a new feature that needs more changes? It will be
> > >> > > > > broken too just like PACKED no?
> > >> > > > > Shouldn't vdpa have an allowlist of features it knows how
> > >> > > > > to support?
> > >> > > >
> > >> > > > It looks like we had it, but we took it out (by the way, we were
> > >> > > > enabling packed even though we didn't support it):
> > >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b
> > >> > > >
> > >> > > > The only problem I see is that for each new feature we have to modify
> > >> > > > the kernel.
> > >> > > > Could we have new features that don't require handling by vhost-vdpa?
> > >> > > >
> > >> > > > Thanks,
> > >> > > > Stefano
> > >> > >
> > >> > > Jason what do you say to reverting this?
> > >> >
> > >> > I may miss something but I don't see any problem with vDPA core.
> > >> >
> > >> > It's the duty of the parents to advertise the features it has. For example,
> > >> >
> > >> > 1) If some kernel version that is packed is not supported via
> > >> > set_vq_state, parents should not advertise PACKED features in this
> > >> > case.
> > >> > 2) If the kernel has support packed set_vq_state(), but it's emulated
> > >> > cvq doesn't support, parents should not advertise PACKED as well
> > >> >
> > >> > If a parent violates the above 2, it looks like a bug of the parents.
> > >> >
> > >> > Thanks
> > >>
> > >> Yes but what about vhost_vdpa? Talking about that not the core.
> > >
> > >Not sure it's a good idea to workaround parent bugs via vhost-vDPA.
> >
> > Sorry, I'm getting lost...
> > We were talking about the fact that vhost-vdpa doesn't handle
> > SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before
> > that series [1], no?
> >
> > The parents seem okay, but maybe I missed a few things.
> >
> > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
> 
> Yes, more below.
> 
> >
> > >
> > >> Should that not have a whitelist of features
> > >> since it interprets ioctls differently depending on this?
> > >
> > >If there's a bug, it might only matter the following setup:
> > >
> > >SET_VRING_BASE/GET_VRING_BASE + VDUSE.
> > >
> > >This seems to be broken since VDUSE was introduced. If we really want
> > >to backport something, it could be a fix to filter out PACKED in
> > >VDUSE?
> >
> > mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa.
> > I think VDUSE works fine with packed virtqueue using virtio-vdpa
> > (I haven't tried), so why should we filter PACKED in VDUSE?
> 
> I don't think we need any filtering since:
> 
> PACKED features has been advertised to userspace via uAPI since
> 6234f80574d7569444d8718355fa2838e92b158b. Once we relax in uAPI, it
> would be very hard to restrict it again. For the userspace that tries
> to negotiate PACKED:
> 
> 1) if it doesn't use SET_VRING_BASE/GET_VRING_BASE, everything works well
> 2) if it uses SET_VRING_BASE/GET_VRING_BASE. it might fail or break silently
> 
> If we backport the fixes to -stable, we may break the application at
> least in the case 1).
> 
> Thanks


I am less concerned about stable, and much more concerned about the
future. Assume we add a new ring format. It will be silently passed
to vhost-vdpa and things break again.
This is why I think we need an allowlist in vhost-vdpa.


> >
> > Thanks,
> > Stefano
> >

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Michael S. Tsirkin]

On Thu, Jun 08, 2023 at 05:29:58PM +0800, Jason Wang wrote:
> On Thu, Jun 8, 2023 at 5:21 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >
> > On Thu, Jun 08, 2023 at 05:00:00PM +0800, Jason Wang wrote:
> > >On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > >>
> > >> On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote:
> > >>
> > >> [...]
> > >>
> > >> >> > > > > I have a question though, what if down the road there
> > >> >> > > > > is a new feature that needs more changes? It will be
> > >> >> > > > > broken too just like PACKED no?
> > >> >> > > > > Shouldn't vdpa have an allowlist of features it knows how
> > >> >> > > > > to support?
> > >> >> > > >
> > >> >> > > > It looks like we had it, but we took it out (by the way, we were
> > >> >> > > > enabling packed even though we didn't support it):
> > >> >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b
> > >> >> > > >
> > >> >> > > > The only problem I see is that for each new feature we have to modify
> > >> >> > > > the kernel.
> > >> >> > > > Could we have new features that don't require handling by vhost-vdpa?
> > >> >> > > >
> > >> >> > > > Thanks,
> > >> >> > > > Stefano
> > >> >> > >
> > >> >> > > Jason what do you say to reverting this?
> > >> >> >
> > >> >> > I may miss something but I don't see any problem with vDPA core.
> > >> >> >
> > >> >> > It's the duty of the parents to advertise the features it has. For example,
> > >> >> >
> > >> >> > 1) If some kernel version that is packed is not supported via
> > >> >> > set_vq_state, parents should not advertise PACKED features in this
> > >> >> > case.
> > >> >> > 2) If the kernel has support packed set_vq_state(), but it's emulated
> > >> >> > cvq doesn't support, parents should not advertise PACKED as well
> > >> >> >
> > >> >> > If a parent violates the above 2, it looks like a bug of the parents.
> > >> >> >
> > >> >> > Thanks
> > >> >>
> > >> >> Yes but what about vhost_vdpa? Talking about that not the core.
> > >> >
> > >> >Not sure it's a good idea to workaround parent bugs via vhost-vDPA.
> > >>
> > >> Sorry, I'm getting lost...
> > >> We were talking about the fact that vhost-vdpa doesn't handle
> > >> SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before
> > >> that series [1], no?
> > >>
> > >> The parents seem okay, but maybe I missed a few things.
> > >>
> > >> [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
> > >
> > >Yes, more below.
> > >
> > >>
> > >> >
> > >> >> Should that not have a whitelist of features
> > >> >> since it interprets ioctls differently depending on this?
> > >> >
> > >> >If there's a bug, it might only matter the following setup:
> > >> >
> > >> >SET_VRING_BASE/GET_VRING_BASE + VDUSE.
> > >> >
> > >> >This seems to be broken since VDUSE was introduced. If we really want
> > >> >to backport something, it could be a fix to filter out PACKED in
> > >> >VDUSE?
> > >>
> > >> mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa.
> > >> I think VDUSE works fine with packed virtqueue using virtio-vdpa
> > >> (I haven't tried), so why should we filter PACKED in VDUSE?
> > >
> > >I don't think we need any filtering since:
> > >
> > >PACKED features has been advertised to userspace via uAPI since
> > >6234f80574d7569444d8718355fa2838e92b158b. Once we relax in uAPI, it
> > >would be very hard to restrict it again. For the userspace that tries
> > >to negotiate PACKED:
> > >
> > >1) if it doesn't use SET_VRING_BASE/GET_VRING_BASE, everything works well
> > >2) if it uses SET_VRING_BASE/GET_VRING_BASE. it might fail or break silently
> > >
> > >If we backport the fixes to -stable, we may break the application at
> > >least in the case 1).
> >
> > Okay, I see now, thanks for the details!
> >
> > Maybe instead of "break silently", we can return an explicit error for
> > SET_VRING_BASE/GET_VRING_BASE in stable branches.
> > But if there are not many cases, we can leave it like that.
> 
> A second thought, if we need to do something for stable. is it better
> if we just backport Shannon's series to stable?
> 
> >
> > I was just concerned about how does the user space understand that it
> > can use SET_VRING_BASE/GET_VRING_BASE for PACKED virtqueues in a given
> > kernel or not.
> 
> My understanding is that if packed is advertised, the application
> should assume SET/GET_VRING_BASE work.
> 
> Thanks


Let me ask you this. This is a bugfix yes? What is the appropriate Fixes
tag?

> >
> > Thanks,
> > Stefano
> >

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Jason Wang]

On Thu, Jun 8, 2023 at 10:23 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Jun 08, 2023 at 05:29:58PM +0800, Jason Wang wrote:
> > On Thu, Jun 8, 2023 at 5:21 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > >
> > > On Thu, Jun 08, 2023 at 05:00:00PM +0800, Jason Wang wrote:
> > > >On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > > >>
> > > >> On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote:
> > > >>
> > > >> [...]
> > > >>
> > > >> >> > > > > I have a question though, what if down the road there
> > > >> >> > > > > is a new feature that needs more changes? It will be
> > > >> >> > > > > broken too just like PACKED no?
> > > >> >> > > > > Shouldn't vdpa have an allowlist of features it knows how
> > > >> >> > > > > to support?
> > > >> >> > > >
> > > >> >> > > > It looks like we had it, but we took it out (by the way, we were
> > > >> >> > > > enabling packed even though we didn't support it):
> > > >> >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b
> > > >> >> > > >
> > > >> >> > > > The only problem I see is that for each new feature we have to modify
> > > >> >> > > > the kernel.
> > > >> >> > > > Could we have new features that don't require handling by vhost-vdpa?
> > > >> >> > > >
> > > >> >> > > > Thanks,
> > > >> >> > > > Stefano
> > > >> >> > >
> > > >> >> > > Jason what do you say to reverting this?
> > > >> >> >
> > > >> >> > I may miss something but I don't see any problem with vDPA core.
> > > >> >> >
> > > >> >> > It's the duty of the parents to advertise the features it has. For example,
> > > >> >> >
> > > >> >> > 1) If some kernel version that is packed is not supported via
> > > >> >> > set_vq_state, parents should not advertise PACKED features in this
> > > >> >> > case.
> > > >> >> > 2) If the kernel has support packed set_vq_state(), but it's emulated
> > > >> >> > cvq doesn't support, parents should not advertise PACKED as well
> > > >> >> >
> > > >> >> > If a parent violates the above 2, it looks like a bug of the parents.
> > > >> >> >
> > > >> >> > Thanks
> > > >> >>
> > > >> >> Yes but what about vhost_vdpa? Talking about that not the core.
> > > >> >
> > > >> >Not sure it's a good idea to workaround parent bugs via vhost-vDPA.
> > > >>
> > > >> Sorry, I'm getting lost...
> > > >> We were talking about the fact that vhost-vdpa doesn't handle
> > > >> SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before
> > > >> that series [1], no?
> > > >>
> > > >> The parents seem okay, but maybe I missed a few things.
> > > >>
> > > >> [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
> > > >
> > > >Yes, more below.
> > > >
> > > >>
> > > >> >
> > > >> >> Should that not have a whitelist of features
> > > >> >> since it interprets ioctls differently depending on this?
> > > >> >
> > > >> >If there's a bug, it might only matter the following setup:
> > > >> >
> > > >> >SET_VRING_BASE/GET_VRING_BASE + VDUSE.
> > > >> >
> > > >> >This seems to be broken since VDUSE was introduced. If we really want
> > > >> >to backport something, it could be a fix to filter out PACKED in
> > > >> >VDUSE?
> > > >>
> > > >> mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa.
> > > >> I think VDUSE works fine with packed virtqueue using virtio-vdpa
> > > >> (I haven't tried), so why should we filter PACKED in VDUSE?
> > > >
> > > >I don't think we need any filtering since:
> > > >
> > > >PACKED features has been advertised to userspace via uAPI since
> > > >6234f80574d7569444d8718355fa2838e92b158b. Once we relax in uAPI, it
> > > >would be very hard to restrict it again. For the userspace that tries
> > > >to negotiate PACKED:
> > > >
> > > >1) if it doesn't use SET_VRING_BASE/GET_VRING_BASE, everything works well
> > > >2) if it uses SET_VRING_BASE/GET_VRING_BASE. it might fail or break silently
> > > >
> > > >If we backport the fixes to -stable, we may break the application at
> > > >least in the case 1).
> > >
> > > Okay, I see now, thanks for the details!
> > >
> > > Maybe instead of "break silently", we can return an explicit error for
> > > SET_VRING_BASE/GET_VRING_BASE in stable branches.
> > > But if there are not many cases, we can leave it like that.
> >
> > A second thought, if we need to do something for stable. is it better
> > if we just backport Shannon's series to stable?
> >
> > >
> > > I was just concerned about how does the user space understand that it
> > > can use SET_VRING_BASE/GET_VRING_BASE for PACKED virtqueues in a given
> > > kernel or not.
> >
> > My understanding is that if packed is advertised, the application
> > should assume SET/GET_VRING_BASE work.
> >
> > Thanks
>
>
> Let me ask you this. This is a bugfix yes?

Not sure since it may break existing user space applications which
make it hard to be backported to -stable.

Before the fix, PACKED might work if SET/GET_VRING_BASE is not used.
After the fix, PACKED won't work at all.

Thanks

What is the appropriate Fixes
> tag?
>
> > >
> > > Thanks,
> > > Stefano
> > >
>

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Michael S. Tsirkin]

On Fri, Jun 09, 2023 at 10:16:50AM +0800, Jason Wang wrote:
> On Thu, Jun 8, 2023 at 10:23 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Jun 08, 2023 at 05:29:58PM +0800, Jason Wang wrote:
> > > On Thu, Jun 8, 2023 at 5:21 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > > >
> > > > On Thu, Jun 08, 2023 at 05:00:00PM +0800, Jason Wang wrote:
> > > > >On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > > > >>
> > > > >> On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote:
> > > > >>
> > > > >> [...]
> > > > >>
> > > > >> >> > > > > I have a question though, what if down the road there
> > > > >> >> > > > > is a new feature that needs more changes? It will be
> > > > >> >> > > > > broken too just like PACKED no?
> > > > >> >> > > > > Shouldn't vdpa have an allowlist of features it knows how
> > > > >> >> > > > > to support?
> > > > >> >> > > >
> > > > >> >> > > > It looks like we had it, but we took it out (by the way, we were
> > > > >> >> > > > enabling packed even though we didn't support it):
> > > > >> >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b
> > > > >> >> > > >
> > > > >> >> > > > The only problem I see is that for each new feature we have to modify
> > > > >> >> > > > the kernel.
> > > > >> >> > > > Could we have new features that don't require handling by vhost-vdpa?
> > > > >> >> > > >
> > > > >> >> > > > Thanks,
> > > > >> >> > > > Stefano
> > > > >> >> > >
> > > > >> >> > > Jason what do you say to reverting this?
> > > > >> >> >
> > > > >> >> > I may miss something but I don't see any problem with vDPA core.
> > > > >> >> >
> > > > >> >> > It's the duty of the parents to advertise the features it has. For example,
> > > > >> >> >
> > > > >> >> > 1) If some kernel version that is packed is not supported via
> > > > >> >> > set_vq_state, parents should not advertise PACKED features in this
> > > > >> >> > case.
> > > > >> >> > 2) If the kernel has support packed set_vq_state(), but it's emulated
> > > > >> >> > cvq doesn't support, parents should not advertise PACKED as well
> > > > >> >> >
> > > > >> >> > If a parent violates the above 2, it looks like a bug of the parents.
> > > > >> >> >
> > > > >> >> > Thanks
> > > > >> >>
> > > > >> >> Yes but what about vhost_vdpa? Talking about that not the core.
> > > > >> >
> > > > >> >Not sure it's a good idea to workaround parent bugs via vhost-vDPA.
> > > > >>
> > > > >> Sorry, I'm getting lost...
> > > > >> We were talking about the fact that vhost-vdpa doesn't handle
> > > > >> SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before
> > > > >> that series [1], no?
> > > > >>
> > > > >> The parents seem okay, but maybe I missed a few things.
> > > > >>
> > > > >> [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
> > > > >
> > > > >Yes, more below.
> > > > >
> > > > >>
> > > > >> >
> > > > >> >> Should that not have a whitelist of features
> > > > >> >> since it interprets ioctls differently depending on this?
> > > > >> >
> > > > >> >If there's a bug, it might only matter the following setup:
> > > > >> >
> > > > >> >SET_VRING_BASE/GET_VRING_BASE + VDUSE.
> > > > >> >
> > > > >> >This seems to be broken since VDUSE was introduced. If we really want
> > > > >> >to backport something, it could be a fix to filter out PACKED in
> > > > >> >VDUSE?
> > > > >>
> > > > >> mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa.
> > > > >> I think VDUSE works fine with packed virtqueue using virtio-vdpa
> > > > >> (I haven't tried), so why should we filter PACKED in VDUSE?
> > > > >
> > > > >I don't think we need any filtering since:
> > > > >
> > > > >PACKED features has been advertised to userspace via uAPI since
> > > > >6234f80574d7569444d8718355fa2838e92b158b. Once we relax in uAPI, it
> > > > >would be very hard to restrict it again. For the userspace that tries
> > > > >to negotiate PACKED:
> > > > >
> > > > >1) if it doesn't use SET_VRING_BASE/GET_VRING_BASE, everything works well
> > > > >2) if it uses SET_VRING_BASE/GET_VRING_BASE. it might fail or break silently
> > > > >
> > > > >If we backport the fixes to -stable, we may break the application at
> > > > >least in the case 1).
> > > >
> > > > Okay, I see now, thanks for the details!
> > > >
> > > > Maybe instead of "break silently", we can return an explicit error for
> > > > SET_VRING_BASE/GET_VRING_BASE in stable branches.
> > > > But if there are not many cases, we can leave it like that.
> > >
> > > A second thought, if we need to do something for stable. is it better
> > > if we just backport Shannon's series to stable?
> > >
> > > >
> > > > I was just concerned about how does the user space understand that it
> > > > can use SET_VRING_BASE/GET_VRING_BASE for PACKED virtqueues in a given
> > > > kernel or not.
> > >
> > > My understanding is that if packed is advertised, the application
> > > should assume SET/GET_VRING_BASE work.
> > >
> > > Thanks
> >
> >
> > Let me ask you this. This is a bugfix yes?
> 
> Not sure since it may break existing user space applications which
> make it hard to be backported to -stable.

Sorry, I was actually referring to 
    vhost_vdpa: support PACKED when setting-getting vring_base
and friends.

These are bugfixes yes?  What is the appropriate Fixes tag?


> Before the fix, PACKED might work if SET/GET_VRING_BASE is not used.
> After the fix, PACKED won't work at all.
> 
> Thanks
> 
> What is the appropriate Fixes
> > tag?
> >
> > > >
> > > > Thanks,
> > > > Stefano
> > > >
> >

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Stefano Garzarella]

On Thu, Jun 08, 2023 at 10:23:21AM -0400, Michael S. Tsirkin wrote:
>On Thu, Jun 08, 2023 at 05:29:58PM +0800, Jason Wang wrote:
>> On Thu, Jun 8, 2023 at 5:21 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >
>> > On Thu, Jun 08, 2023 at 05:00:00PM +0800, Jason Wang wrote:
>> > >On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>> > >>
>> > >> On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote:
>> > >>
>> > >> [...]
>> > >>
>> > >> >> > > > > I have a question though, what if down the road there
>> > >> >> > > > > is a new feature that needs more changes? It will be
>> > >> >> > > > > broken too just like PACKED no?
>> > >> >> > > > > Shouldn't vdpa have an allowlist of features it knows how
>> > >> >> > > > > to support?
>> > >> >> > > >
>> > >> >> > > > It looks like we had it, but we took it out (by the way, we were
>> > >> >> > > > enabling packed even though we didn't support it):
>> > >> >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b
>> > >> >> > > >
>> > >> >> > > > The only problem I see is that for each new feature we have to modify
>> > >> >> > > > the kernel.
>> > >> >> > > > Could we have new features that don't require handling by vhost-vdpa?
>> > >> >> > > >
>> > >> >> > > > Thanks,
>> > >> >> > > > Stefano
>> > >> >> > >
>> > >> >> > > Jason what do you say to reverting this?
>> > >> >> >
>> > >> >> > I may miss something but I don't see any problem with vDPA core.
>> > >> >> >
>> > >> >> > It's the duty of the parents to advertise the features it has. For example,
>> > >> >> >
>> > >> >> > 1) If some kernel version that is packed is not supported via
>> > >> >> > set_vq_state, parents should not advertise PACKED features in this
>> > >> >> > case.
>> > >> >> > 2) If the kernel has support packed set_vq_state(), but it's emulated
>> > >> >> > cvq doesn't support, parents should not advertise PACKED as well
>> > >> >> >
>> > >> >> > If a parent violates the above 2, it looks like a bug of the parents.
>> > >> >> >
>> > >> >> > Thanks
>> > >> >>
>> > >> >> Yes but what about vhost_vdpa? Talking about that not the core.
>> > >> >
>> > >> >Not sure it's a good idea to workaround parent bugs via vhost-vDPA.
>> > >>
>> > >> Sorry, I'm getting lost...
>> > >> We were talking about the fact that vhost-vdpa doesn't handle
>> > >> SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before
>> > >> that series [1], no?
>> > >>
>> > >> The parents seem okay, but maybe I missed a few things.
>> > >>
>> > >> [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
>> > >
>> > >Yes, more below.
>> > >
>> > >>
>> > >> >
>> > >> >> Should that not have a whitelist of features
>> > >> >> since it interprets ioctls differently depending on this?
>> > >> >
>> > >> >If there's a bug, it might only matter the following setup:
>> > >> >
>> > >> >SET_VRING_BASE/GET_VRING_BASE + VDUSE.
>> > >> >
>> > >> >This seems to be broken since VDUSE was introduced. If we really want
>> > >> >to backport something, it could be a fix to filter out PACKED in
>> > >> >VDUSE?
>> > >>
>> > >> mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa.
>> > >> I think VDUSE works fine with packed virtqueue using virtio-vdpa
>> > >> (I haven't tried), so why should we filter PACKED in VDUSE?
>> > >
>> > >I don't think we need any filtering since:
>> > >
>> > >PACKED features has been advertised to userspace via uAPI since
>> > >6234f80574d7569444d8718355fa2838e92b158b. Once we relax in uAPI, it
>> > >would be very hard to restrict it again. For the userspace that tries
>> > >to negotiate PACKED:
>> > >
>> > >1) if it doesn't use SET_VRING_BASE/GET_VRING_BASE, everything works well
>> > >2) if it uses SET_VRING_BASE/GET_VRING_BASE. it might fail or break silently
>> > >
>> > >If we backport the fixes to -stable, we may break the application at
>> > >least in the case 1).
>> >
>> > Okay, I see now, thanks for the details!
>> >
>> > Maybe instead of "break silently", we can return an explicit error for
>> > SET_VRING_BASE/GET_VRING_BASE in stable branches.
>> > But if there are not many cases, we can leave it like that.
>>
>> A second thought, if we need to do something for stable. is it better
>> if we just backport Shannon's series to stable?
>>
>> >
>> > I was just concerned about how does the user space understand that it
>> > can use SET_VRING_BASE/GET_VRING_BASE for PACKED virtqueues in a given
>> > kernel or not.
>>
>> My understanding is that if packed is advertised, the application
>> should assume SET/GET_VRING_BASE work.
>>
>> Thanks
>
>
>Let me ask you this. This is a bugfix yes? What is the appropriate Fixes
>tag?

I would say:

Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")

because we had an allow list and enabled PACKED explicitly.

I'm not sure if the parents at that time supported PACKED, but
maybe it doesn't matter since we are talking about the code in
vhost-vdpa.

Thanks,
Stefano

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Michael S. Tsirkin]

On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
> vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
> don't support packed virtqueue well yet, so let's filter the
> VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
> 
> This way, even if the device supports it, we don't risk it being
> negotiated, then the VMM is unable to set the vring state properly.
> 
> Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> Cc: stable@vger.kernel.org
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

OK so for now I dropped this, we have a better fix upstream.

> ---
> 
> Notes:
>     This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa:
>     better PACKED support" series [1] and backported in stable branches.
>     
>     We can revert it when we are sure that everything is working with
>     packed virtqueues.
>     
>     Thanks,
>     Stefano
>     
>     [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/
> 
>  drivers/vhost/vdpa.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> 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;
>  
> 
> base-commit: 9561de3a55bed6bdd44a12820ba81ec416e705a7
> -- 
> 2.40.1

Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature

[Stefano Garzarella]

On Thu, Jun 22, 2023 at 07:37:08AM -0400, Michael S. Tsirkin wrote:
>On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote:
>> vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE)
>> don't support packed virtqueue well yet, so let's filter the
>> VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features().
>>
>> This way, even if the device supports it, we don't risk it being
>> negotiated, then the VMM is unable to set the vring state properly.
>>
>> Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>
>OK so for now I dropped this, we have a better fix upstream.
>

Yep, I agree.

Maybe we can reuse this patch in the stable branches where the backport
is not easy. Although as Jason said, maybe we don't need it.

Thanks,
Stefano