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: [email protected]
Signed-off-by: Stefano Garzarella <[email protected]>
---
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/[email protected]/
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
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: [email protected]
>> Signed-off-by: Stefano Garzarella <[email protected]>
>> ---
>>
>> 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/[email protected]/
>
>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
>
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: [email protected]
> Signed-off-by: Stefano Garzarella <[email protected]>
> ---
>
> 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/[email protected]/
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
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: [email protected]
> > > Signed-off-by: Stefano Garzarella <[email protected]>
> > > ---
> > >
> > > 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/[email protected]/
> >
> > 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
> >
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: [email protected]
>> > > Signed-off-by: Stefano Garzarella <[email protected]>
>> > > ---
>> > >
>> > > 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/[email protected]/
>> >
>> > 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
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: [email protected]
> > > > > Signed-off-by: Stefano Garzarella <[email protected]>
> > > > > ---
> > > > >
> > > > > 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/[email protected]/
> > > >
> > > > 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
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: [email protected]
>> > > > > Signed-off-by: Stefano Garzarella <[email protected]>
>> > > > > ---
>> > > > >
>> > > > > 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/[email protected]/
>> > > >
>> > > > 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
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: [email protected]
> > > > > > > Signed-off-by: Stefano Garzarella <[email protected]>
> > > > > > > ---
> > > > > > >
> > > > > > > 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/[email protected]/
> > > > > >
> > > > > > 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
On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <[email protected]> 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: [email protected]
> >> > > > > Signed-off-by: Stefano Garzarella <[email protected]>
> >> > > > > ---
> >> > > > >
> >> > > > > 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/[email protected]/
> >> > > >
> >> > > > 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
>
>
On Tue, Jun 06, 2023 at 09:29:22AM +0800, Jason Wang wrote:
>On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <[email protected]> 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: [email protected]
>> >> > > > > Signed-off-by: Stefano Garzarella <[email protected]>
>> >> > > > > ---
>> >> > > > >
>> >> > > > > 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/[email protected]/
>> >> > > >
>> >> > > > 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
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: [email protected]
>> > > > > > > Signed-off-by: Stefano Garzarella <[email protected]>
>> > > > > > > ---
>> > > > > > >
>> > > > > > > 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/[email protected]/
>> > > > > >
>> > > > > > 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
On Tue, Jun 06, 2023 at 09:29:22AM +0800, Jason Wang wrote:
> On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <[email protected]> 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: [email protected]
> > >> > > > > Signed-off-by: Stefano Garzarella <[email protected]>
> > >> > > > > ---
> > >> > > > >
> > >> > > > > 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/[email protected]/
> > >> > > >
> > >> > > > 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
> >
> >
On Tue, Jun 6, 2023 at 2:58 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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: [email protected]
> > > >> > > > > Signed-off-by: Stefano Garzarella <[email protected]>
> > > >> > > > > ---
> > > >> > > > >
> > > >> > > > > 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/[email protected]/
> > > >> > > >
> > > >> > > > 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
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 <[email protected]> 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 <[email protected]> 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: [email protected]
> > > > >> > > > > Signed-off-by: Stefano Garzarella <[email protected]>
> > > > >> > > > > ---
> > > > >> > > > >
> > > > >> > > > > 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/[email protected]/
> > > > >> > > >
> > > > >> > > > 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
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: [email protected]
> > > > > > > > > Signed-off-by: Stefano Garzarella <[email protected]>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > 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/[email protected]/
> > > > > > > >
> > > > > > > > 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
On Wed, Jun 7, 2023 at 5:43 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]> 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: [email protected]
> > > > > >> > > > > Signed-off-by: Stefano Garzarella <[email protected]>
> > > > > >> > > > > ---
> > > > > >> > > > >
> > > > > >> > > > > 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/[email protected]/
> > > > > >> > > >
> > > > > >> > > > 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
>
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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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: [email protected]
> > > > > > >> > > > > Signed-off-by: Stefano Garzarella <[email protected]>
> > > > > > >> > > > > ---
> > > > > > >> > > > >
> > > > > > >> > > > > 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/[email protected]/
> > > > > > >> > > >
> > > > > > >> > > > 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
> >
On Thu, Jun 8, 2023 at 2:03 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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: [email protected]
> > > > > > > >> > > > > Signed-off-by: Stefano Garzarella <[email protected]>
> > > > > > > >> > > > > ---
> > > > > > > >> > > > >
> > > > > > > >> > > > > 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/[email protected]/
> > > > > > > >> > > >
> > > > > > > >> > > > 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
> > >
>
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/[email protected]/
>
>> 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
On Thu, Jun 08, 2023 at 05:00:00PM +0800, Jason Wang wrote:
>On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <[email protected]> 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/[email protected]/
>
>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
On Thu, Jun 8, 2023 at 5:21 PM Stefano Garzarella <[email protected]> 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 <[email protected]> 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/[email protected]/
> >
> >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
>
On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <[email protected]> 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/[email protected]/
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
>
On Thu, Jun 08, 2023 at 05:29:58PM +0800, Jason Wang wrote:
>On Thu, Jun 8, 2023 at 5:21 PM Stefano Garzarella <[email protected]> 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 <[email protected]> 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/[email protected]/
>> >
>> >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
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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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: [email protected]
> > > > > > > > >> > > > > Signed-off-by: Stefano Garzarella <[email protected]>
> > > > > > > > >> > > > > ---
> > > > > > > > >> > > > >
> > > > > > > > >> > > > > 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/[email protected]/
> > > > > > > > >> > > >
> > > > > > > > >> > > > 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
> > > >
> >
On Thu, Jun 08, 2023 at 05:00:00PM +0800, Jason Wang wrote:
> On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <[email protected]> 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/[email protected]/
>
> 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
> >
On Thu, Jun 08, 2023 at 05:29:58PM +0800, Jason Wang wrote:
> On Thu, Jun 8, 2023 at 5:21 PM Stefano Garzarella <[email protected]> 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 <[email protected]> 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/[email protected]/
> > >
> > >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
> >
On Thu, Jun 8, 2023 at 10:23 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]> 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/[email protected]/
> > > >
> > > >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
> > >
>
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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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/[email protected]/
> > > > >
> > > > >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
> > > >
> >
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 <[email protected]> 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 <[email protected]> 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/[email protected]/
>> > >
>> > >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
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: [email protected]
> Signed-off-by: Stefano Garzarella <[email protected]>
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/[email protected]/
>
> 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
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: [email protected]
>> Signed-off-by: Stefano Garzarella <[email protected]>
>
>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