| Message ID | 20230605110644.151211-1-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | vhost-vdpa: filter VIRTIO_F_RING_PACKED feature | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) > don't support packed virtqueue well yet, so let's filter the > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). > > This way, even if the device supports it, we don't risk it being > negotiated, then the VMM is unable to set the vring state properly. > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > Cc: stable@vger.kernel.org > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > > Notes: > This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: > better PACKED support" series [1] and backported in stable branches. > > We can revert it when we are sure that everything is working with > packed virtqueues. > > Thanks, > Stefano > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ I'm a bit lost here. So why am I merging "better PACKED support" then? Does this patch make them a NOP? > drivers/vhost/vdpa.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c > index 8c1aefc865f0..ac2152135b23 100644 > --- a/drivers/vhost/vdpa.c > +++ b/drivers/vhost/vdpa.c > @@ -397,6 +397,12 @@ static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep) > > features = ops->get_device_features(vdpa); > > + /* > + * IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) don't support > + * packed virtqueue well yet, so let's filter the feature for now. > + */ > + features &= ~BIT_ULL(VIRTIO_F_RING_PACKED); > + > if (copy_to_user(featurep, &features, sizeof(features))) > return -EFAULT; > > > base-commit: 9561de3a55bed6bdd44a12820ba81ec416e705a7 > -- > 2.40.1
On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote: >On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: >> vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) >> don't support packed virtqueue well yet, so let's filter the >> VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). >> >> This way, even if the device supports it, we don't risk it being >> negotiated, then the VMM is unable to set the vring state properly. >> >> Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") >> Cc: stable@vger.kernel.org >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> --- >> >> Notes: >> This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: >> better PACKED support" series [1] and backported in stable branches. >> >> We can revert it when we are sure that everything is working with >> packed virtqueues. >> >> Thanks, >> Stefano >> >> [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > >I'm a bit lost here. So why am I merging "better PACKED support" then? To really support packed virtqueue with vhost-vdpa, at that point we would also have to revert this patch. I wasn't sure if you wanted to queue the series for this merge window. In that case do you think it is better to send this patch only for stable branches? >Does this patch make them a NOP? Yep, after applying the "better PACKED support" series and being sure that the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this patch. Let me know if you prefer a different approach. I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel interprets them the right way, when it does not. Thanks, Stefano > >> drivers/vhost/vdpa.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c >> index 8c1aefc865f0..ac2152135b23 100644 >> --- a/drivers/vhost/vdpa.c >> +++ b/drivers/vhost/vdpa.c >> @@ -397,6 +397,12 @@ static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep) >> >> features = ops->get_device_features(vdpa); >> >> + /* >> + * IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) don't support >> + * packed virtqueue well yet, so let's filter the feature for now. >> + */ >> + features &= ~BIT_ULL(VIRTIO_F_RING_PACKED); >> + >> if (copy_to_user(featurep, &features, sizeof(features))) >> return -EFAULT; >> >> >> base-commit: 9561de3a55bed6bdd44a12820ba81ec416e705a7 >> -- >> 2.40.1 >
On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote: > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote: > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) > > > don't support packed virtqueue well yet, so let's filter the > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). > > > > > > This way, even if the device supports it, we don't risk it being > > > negotiated, then the VMM is unable to set the vring state properly. > > > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > --- > > > > > > Notes: > > > This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: > > > better PACKED support" series [1] and backported in stable branches. > > > > > > We can revert it when we are sure that everything is working with > > > packed virtqueues. > > > > > > Thanks, > > > Stefano > > > > > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > > > > I'm a bit lost here. So why am I merging "better PACKED support" then? > > To really support packed virtqueue with vhost-vdpa, at that point we would > also have to revert this patch. > > I wasn't sure if you wanted to queue the series for this merge window. > In that case do you think it is better to send this patch only for stable > branches? > > Does this patch make them a NOP? > > Yep, after applying the "better PACKED support" series and being sure that > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this > patch. > > Let me know if you prefer a different approach. > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel > interprets them the right way, when it does not. > > Thanks, > Stefano > If this fixes a bug can you add Fixes tags to each of them? Then it's ok to merge in this window. Probably easier than the elaborate mask/unmask dance. > > > > > drivers/vhost/vdpa.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c > > > index 8c1aefc865f0..ac2152135b23 100644 > > > --- a/drivers/vhost/vdpa.c > > > +++ b/drivers/vhost/vdpa.c > > > @@ -397,6 +397,12 @@ static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep) > > > > > > features = ops->get_device_features(vdpa); > > > > > > + /* > > > + * IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) don't support > > > + * packed virtqueue well yet, so let's filter the feature for now. > > > + */ > > > + features &= ~BIT_ULL(VIRTIO_F_RING_PACKED); > > > + > > > if (copy_to_user(featurep, &features, sizeof(features))) > > > return -EFAULT; > > > > > > > > > base-commit: 9561de3a55bed6bdd44a12820ba81ec416e705a7 > > > -- > > > 2.40.1 > >
On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote: >On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote: >> On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote: >> > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: >> > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) >> > > don't support packed virtqueue well yet, so let's filter the >> > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). >> > > >> > > This way, even if the device supports it, we don't risk it being >> > > negotiated, then the VMM is unable to set the vring state properly. >> > > >> > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") >> > > Cc: stable@vger.kernel.org >> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> > > --- >> > > >> > > Notes: >> > > This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: >> > > better PACKED support" series [1] and backported in stable branches. >> > > >> > > We can revert it when we are sure that everything is working with >> > > packed virtqueues. >> > > >> > > Thanks, >> > > Stefano >> > > >> > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ >> > >> > I'm a bit lost here. So why am I merging "better PACKED support" then? >> >> To really support packed virtqueue with vhost-vdpa, at that point we would >> also have to revert this patch. >> >> I wasn't sure if you wanted to queue the series for this merge window. >> In that case do you think it is better to send this patch only for stable >> branches? >> > Does this patch make them a NOP? >> >> Yep, after applying the "better PACKED support" series and being sure >> that >> the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this >> patch. >> >> Let me know if you prefer a different approach. >> >> I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel >> interprets them the right way, when it does not. >> >> Thanks, >> Stefano >> > >If this fixes a bug can you add Fixes tags to each of them? Then it's ok >to merge in this window. Probably easier than the elaborate >mask/unmask dance. CCing Shannon (the original author of the "better PACKED support" series). IIUC Shannon is going to send a v3 of that series to fix the documentation, so Shannon can you also add the Fixes tags? Thanks, Stefano
On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote: > On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote: > > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote: > > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote: > > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: > > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) > > > > > don't support packed virtqueue well yet, so let's filter the > > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). > > > > > > > > > > This way, even if the device supports it, we don't risk it being > > > > > negotiated, then the VMM is unable to set the vring state properly. > > > > > > > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > > > > Cc: stable@vger.kernel.org > > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > > > --- > > > > > > > > > > Notes: > > > > > This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: > > > > > better PACKED support" series [1] and backported in stable branches. > > > > > > > > > > We can revert it when we are sure that everything is working with > > > > > packed virtqueues. > > > > > > > > > > Thanks, > > > > > Stefano > > > > > > > > > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > > > > > > > > I'm a bit lost here. So why am I merging "better PACKED support" then? > > > > > > To really support packed virtqueue with vhost-vdpa, at that point we would > > > also have to revert this patch. > > > > > > I wasn't sure if you wanted to queue the series for this merge window. > > > In that case do you think it is better to send this patch only for stable > > > branches? > > > > Does this patch make them a NOP? > > > > > > Yep, after applying the "better PACKED support" series and being > > > sure that > > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this > > > patch. > > > > > > Let me know if you prefer a different approach. > > > > > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel > > > interprets them the right way, when it does not. > > > > > > Thanks, > > > Stefano > > > > > > > If this fixes a bug can you add Fixes tags to each of them? Then it's ok > > to merge in this window. Probably easier than the elaborate > > mask/unmask dance. > > CCing Shannon (the original author of the "better PACKED support" > series). > > IIUC Shannon is going to send a v3 of that series to fix the > documentation, so Shannon can you also add the Fixes tags? > > Thanks, > Stefano Well this is in my tree already. Just reply with Fixes: <> to each and I will add these tags. If I start dropping and rebasing this won't make it in this window.
On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote: >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote: >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote: >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote: >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote: >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) >> > > > > don't support packed virtqueue well yet, so let's filter the >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). >> > > > > >> > > > > This way, even if the device supports it, we don't risk it being >> > > > > negotiated, then the VMM is unable to set the vring state properly. >> > > > > >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") >> > > > > Cc: stable@vger.kernel.org >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> > > > > --- >> > > > > >> > > > > Notes: >> > > > > This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: >> > > > > better PACKED support" series [1] and backported in stable branches. >> > > > > >> > > > > We can revert it when we are sure that everything is working with >> > > > > packed virtqueues. >> > > > > >> > > > > Thanks, >> > > > > Stefano >> > > > > >> > > > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ >> > > > >> > > > I'm a bit lost here. So why am I merging "better PACKED support" then? >> > > >> > > To really support packed virtqueue with vhost-vdpa, at that point we would >> > > also have to revert this patch. >> > > >> > > I wasn't sure if you wanted to queue the series for this merge window. >> > > In that case do you think it is better to send this patch only for stable >> > > branches? >> > > > Does this patch make them a NOP? >> > > >> > > Yep, after applying the "better PACKED support" series and being >> > > sure that >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this >> > > patch. >> > > >> > > Let me know if you prefer a different approach. >> > > >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel >> > > interprets them the right way, when it does not. >> > > >> > > Thanks, >> > > Stefano >> > > >> > >> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok >> > to merge in this window. Probably easier than the elaborate >> > mask/unmask dance. >> >> CCing Shannon (the original author of the "better PACKED support" >> series). >> >> IIUC Shannon is going to send a v3 of that series to fix the >> documentation, so Shannon can you also add the Fixes tags? >> >> Thanks, >> Stefano > >Well this is in my tree already. Just reply with >Fixes: <> >to each and I will add these tags. I tried, but it is not easy since we added the support for packed virtqueue in vdpa and vhost incrementally. Initially I was thinking of adding the same tag used here: Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") Then I discovered that vq_state wasn't there, so I was thinking of Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()") So we would have to backport quite a few patches into the stable branches. I don't know if it's worth it... I still think it is better to disable packed in the stable branches, otherwise I have to make a list of all the patches we need. Any other ideas? Thanks, Stefano
On Mon, Jun 05, 2023 at 04:56:37PM +0200, Stefano Garzarella wrote: > On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote: > > On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote: > > > On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote: > > > > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote: > > > > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote: > > > > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: > > > > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) > > > > > > > don't support packed virtqueue well yet, so let's filter the > > > > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). > > > > > > > > > > > > > > This way, even if the device supports it, we don't risk it being > > > > > > > negotiated, then the VMM is unable to set the vring state properly. > > > > > > > > > > > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > > > > > > Cc: stable@vger.kernel.org > > > > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > > > > > --- > > > > > > > > > > > > > > Notes: > > > > > > > This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: > > > > > > > better PACKED support" series [1] and backported in stable branches. > > > > > > > > > > > > > > We can revert it when we are sure that everything is working with > > > > > > > packed virtqueues. > > > > > > > > > > > > > > Thanks, > > > > > > > Stefano > > > > > > > > > > > > > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > > > > > > > > > > > > I'm a bit lost here. So why am I merging "better PACKED support" then? > > > > > > > > > > To really support packed virtqueue with vhost-vdpa, at that point we would > > > > > also have to revert this patch. > > > > > > > > > > I wasn't sure if you wanted to queue the series for this merge window. > > > > > In that case do you think it is better to send this patch only for stable > > > > > branches? > > > > > > Does this patch make them a NOP? > > > > > > > > > > Yep, after applying the "better PACKED support" series and being > > > > > sure that > > > > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this > > > > > patch. > > > > > > > > > > Let me know if you prefer a different approach. > > > > > > > > > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel > > > > > interprets them the right way, when it does not. > > > > > > > > > > Thanks, > > > > > Stefano > > > > > > > > > > > > > If this fixes a bug can you add Fixes tags to each of them? Then it's ok > > > > to merge in this window. Probably easier than the elaborate > > > > mask/unmask dance. > > > > > > CCing Shannon (the original author of the "better PACKED support" > > > series). > > > > > > IIUC Shannon is going to send a v3 of that series to fix the > > > documentation, so Shannon can you also add the Fixes tags? > > > > > > Thanks, > > > Stefano > > > > Well this is in my tree already. Just reply with > > Fixes: <> > > to each and I will add these tags. > > I tried, but it is not easy since we added the support for packed virtqueue > in vdpa and vhost incrementally. > > Initially I was thinking of adding the same tag used here: > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > Then I discovered that vq_state wasn't there, so I was thinking of > > Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()") > > So we would have to backport quite a few patches into the stable branches. > I don't know if it's worth it... > > I still think it is better to disable packed in the stable branches, > otherwise I have to make a list of all the patches we need. > > Any other ideas? > > Thanks, > Stefano OK so. You want me to apply this one now, and fixes in the next kernel?
On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote: > >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote: > >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote: > >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote: > >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote: > >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: > >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) > >> > > > > don't support packed virtqueue well yet, so let's filter the > >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). > >> > > > > > >> > > > > This way, even if the device supports it, we don't risk it being > >> > > > > negotiated, then the VMM is unable to set the vring state properly. > >> > > > > > >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > >> > > > > Cc: stable@vger.kernel.org > >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > >> > > > > --- > >> > > > > > >> > > > > Notes: > >> > > > > This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: > >> > > > > better PACKED support" series [1] and backported in stable branches. > >> > > > > > >> > > > > We can revert it when we are sure that everything is working with > >> > > > > packed virtqueues. > >> > > > > > >> > > > > Thanks, > >> > > > > Stefano > >> > > > > > >> > > > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > >> > > > > >> > > > I'm a bit lost here. So why am I merging "better PACKED support" then? > >> > > > >> > > To really support packed virtqueue with vhost-vdpa, at that point we would > >> > > also have to revert this patch. > >> > > > >> > > I wasn't sure if you wanted to queue the series for this merge window. > >> > > In that case do you think it is better to send this patch only for stable > >> > > branches? > >> > > > Does this patch make them a NOP? > >> > > > >> > > Yep, after applying the "better PACKED support" series and being > >> > > sure that > >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this > >> > > patch. > >> > > > >> > > Let me know if you prefer a different approach. > >> > > > >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel > >> > > interprets them the right way, when it does not. > >> > > > >> > > Thanks, > >> > > Stefano > >> > > > >> > > >> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok > >> > to merge in this window. Probably easier than the elaborate > >> > mask/unmask dance. > >> > >> CCing Shannon (the original author of the "better PACKED support" > >> series). > >> > >> IIUC Shannon is going to send a v3 of that series to fix the > >> documentation, so Shannon can you also add the Fixes tags? > >> > >> Thanks, > >> Stefano > > > >Well this is in my tree already. Just reply with > >Fixes: <> > >to each and I will add these tags. > > I tried, but it is not easy since we added the support for packed > virtqueue in vdpa and vhost incrementally. > > Initially I was thinking of adding the same tag used here: > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > Then I discovered that vq_state wasn't there, so I was thinking of > > Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()") > > So we would have to backport quite a few patches into the stable branches. > I don't know if it's worth it... > > I still think it is better to disable packed in the stable branches, > otherwise I have to make a list of all the patches we need. > > Any other ideas? AFAIK, except for vp_vdpa, pds seems to be the first parent that supports packed virtqueue. Users should not notice anything wrong if they don't use packed virtqueue. And the problem of vp_vdpa + packed virtqueue came since the day0 of vp_vdpa. It seems fine to do nothing I guess. Thanks > > Thanks, > Stefano > >
On Mon, Jun 05, 2023 at 05:44:50PM -0400, Michael S. Tsirkin wrote: >On Mon, Jun 05, 2023 at 04:56:37PM +0200, Stefano Garzarella wrote: >> On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote: >> > On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote: >> > > On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote: >> > > > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote: >> > > > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote: >> > > > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: >> > > > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) >> > > > > > > don't support packed virtqueue well yet, so let's filter the >> > > > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). >> > > > > > > >> > > > > > > This way, even if the device supports it, we don't risk it being >> > > > > > > negotiated, then the VMM is unable to set the vring state properly. >> > > > > > > >> > > > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") >> > > > > > > Cc: stable@vger.kernel.org >> > > > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> > > > > > > --- >> > > > > > > >> > > > > > > Notes: >> > > > > > > This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: >> > > > > > > better PACKED support" series [1] and backported in stable branches. >> > > > > > > >> > > > > > > We can revert it when we are sure that everything is working with >> > > > > > > packed virtqueues. >> > > > > > > >> > > > > > > Thanks, >> > > > > > > Stefano >> > > > > > > >> > > > > > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ >> > > > > > >> > > > > > I'm a bit lost here. So why am I merging "better PACKED support" then? >> > > > > >> > > > > To really support packed virtqueue with vhost-vdpa, at that point we would >> > > > > also have to revert this patch. >> > > > > >> > > > > I wasn't sure if you wanted to queue the series for this merge window. >> > > > > In that case do you think it is better to send this patch only for stable >> > > > > branches? >> > > > > > Does this patch make them a NOP? >> > > > > >> > > > > Yep, after applying the "better PACKED support" series and being >> > > > > sure that >> > > > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this >> > > > > patch. >> > > > > >> > > > > Let me know if you prefer a different approach. >> > > > > >> > > > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel >> > > > > interprets them the right way, when it does not. >> > > > > >> > > > > Thanks, >> > > > > Stefano >> > > > > >> > > > >> > > > If this fixes a bug can you add Fixes tags to each of them? Then it's ok >> > > > to merge in this window. Probably easier than the elaborate >> > > > mask/unmask dance. >> > > >> > > CCing Shannon (the original author of the "better PACKED support" >> > > series). >> > > >> > > IIUC Shannon is going to send a v3 of that series to fix the >> > > documentation, so Shannon can you also add the Fixes tags? >> > > >> > > Thanks, >> > > Stefano >> > >> > Well this is in my tree already. Just reply with >> > Fixes: <> >> > to each and I will add these tags. >> >> I tried, but it is not easy since we added the support for packed virtqueue >> in vdpa and vhost incrementally. >> >> Initially I was thinking of adding the same tag used here: >> >> Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") >> >> Then I discovered that vq_state wasn't there, so I was thinking of >> >> Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()") >> >> So we would have to backport quite a few patches into the stable branches. >> I don't know if it's worth it... >> >> I still think it is better to disable packed in the stable branches, >> otherwise I have to make a list of all the patches we need. >> >> Any other ideas? >> >> Thanks, >> Stefano > >OK so. You want me to apply this one now, and fixes in the next >kernel? Yep, it seems to me the least risky approach. Thanks, Stefano
On Tue, Jun 06, 2023 at 09:29:22AM +0800, Jason Wang wrote: >On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote: >> >> On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote: >> >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote: >> >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote: >> >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote: >> >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote: >> >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: >> >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) >> >> > > > > don't support packed virtqueue well yet, so let's filter the >> >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). >> >> > > > > >> >> > > > > This way, even if the device supports it, we don't risk it being >> >> > > > > negotiated, then the VMM is unable to set the vring state properly. >> >> > > > > >> >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") >> >> > > > > Cc: stable@vger.kernel.org >> >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> >> > > > > --- >> >> > > > > >> >> > > > > Notes: >> >> > > > > This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: >> >> > > > > better PACKED support" series [1] and backported in stable branches. >> >> > > > > >> >> > > > > We can revert it when we are sure that everything is working with >> >> > > > > packed virtqueues. >> >> > > > > >> >> > > > > Thanks, >> >> > > > > Stefano >> >> > > > > >> >> > > > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ >> >> > > > >> >> > > > I'm a bit lost here. So why am I merging "better PACKED support" then? >> >> > > >> >> > > To really support packed virtqueue with vhost-vdpa, at that point we would >> >> > > also have to revert this patch. >> >> > > >> >> > > I wasn't sure if you wanted to queue the series for this merge window. >> >> > > In that case do you think it is better to send this patch only for stable >> >> > > branches? >> >> > > > Does this patch make them a NOP? >> >> > > >> >> > > Yep, after applying the "better PACKED support" series and being >> >> > > sure that >> >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this >> >> > > patch. >> >> > > >> >> > > Let me know if you prefer a different approach. >> >> > > >> >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel >> >> > > interprets them the right way, when it does not. >> >> > > >> >> > > Thanks, >> >> > > Stefano >> >> > > >> >> > >> >> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok >> >> > to merge in this window. Probably easier than the elaborate >> >> > mask/unmask dance. >> >> >> >> CCing Shannon (the original author of the "better PACKED support" >> >> series). >> >> >> >> IIUC Shannon is going to send a v3 of that series to fix the >> >> documentation, so Shannon can you also add the Fixes tags? >> >> >> >> Thanks, >> >> Stefano >> > >> >Well this is in my tree already. Just reply with >> >Fixes: <> >> >to each and I will add these tags. >> >> I tried, but it is not easy since we added the support for packed >> virtqueue in vdpa and vhost incrementally. >> >> Initially I was thinking of adding the same tag used here: >> >> Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") >> >> Then I discovered that vq_state wasn't there, so I was thinking of >> >> Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()") >> >> So we would have to backport quite a few patches into the stable branches. >> I don't know if it's worth it... >> >> I still think it is better to disable packed in the stable branches, >> otherwise I have to make a list of all the patches we need. >> >> Any other ideas? > >AFAIK, except for vp_vdpa, pds seems to be the first parent that IIUC also vduse and snet supports packed virtqueue. >supports packed virtqueue. Users should not notice anything wrong if >they don't use packed virtqueue. And the problem of vp_vdpa + packed >virtqueue came since the day0 of vp_vdpa. It seems fine to do nothing >I guess. Okay, maybe I'm overthinking it, not having a specific problem I don't object, it was just a concern about uAPI. Thanks, Stefano
On Tue, Jun 06, 2023 at 09:29:22AM +0800, Jason Wang wrote: > On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote: > > >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote: > > >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote: > > >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote: > > >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote: > > >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: > > >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) > > >> > > > > don't support packed virtqueue well yet, so let's filter the > > >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). > > >> > > > > > > >> > > > > This way, even if the device supports it, we don't risk it being > > >> > > > > negotiated, then the VMM is unable to set the vring state properly. > > >> > > > > > > >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > >> > > > > Cc: stable@vger.kernel.org > > >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > >> > > > > --- > > >> > > > > > > >> > > > > Notes: > > >> > > > > This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: > > >> > > > > better PACKED support" series [1] and backported in stable branches. > > >> > > > > > > >> > > > > We can revert it when we are sure that everything is working with > > >> > > > > packed virtqueues. > > >> > > > > > > >> > > > > Thanks, > > >> > > > > Stefano > > >> > > > > > > >> > > > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > > >> > > > > > >> > > > I'm a bit lost here. So why am I merging "better PACKED support" then? > > >> > > > > >> > > To really support packed virtqueue with vhost-vdpa, at that point we would > > >> > > also have to revert this patch. > > >> > > > > >> > > I wasn't sure if you wanted to queue the series for this merge window. > > >> > > In that case do you think it is better to send this patch only for stable > > >> > > branches? > > >> > > > Does this patch make them a NOP? > > >> > > > > >> > > Yep, after applying the "better PACKED support" series and being > > >> > > sure that > > >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this > > >> > > patch. > > >> > > > > >> > > Let me know if you prefer a different approach. > > >> > > > > >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel > > >> > > interprets them the right way, when it does not. > > >> > > > > >> > > Thanks, > > >> > > Stefano > > >> > > > > >> > > > >> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok > > >> > to merge in this window. Probably easier than the elaborate > > >> > mask/unmask dance. > > >> > > >> CCing Shannon (the original author of the "better PACKED support" > > >> series). > > >> > > >> IIUC Shannon is going to send a v3 of that series to fix the > > >> documentation, so Shannon can you also add the Fixes tags? > > >> > > >> Thanks, > > >> Stefano > > > > > >Well this is in my tree already. Just reply with > > >Fixes: <> > > >to each and I will add these tags. > > > > I tried, but it is not easy since we added the support for packed > > virtqueue in vdpa and vhost incrementally. > > > > Initially I was thinking of adding the same tag used here: > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > > > Then I discovered that vq_state wasn't there, so I was thinking of > > > > Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()") > > > > So we would have to backport quite a few patches into the stable branches. > > I don't know if it's worth it... > > > > I still think it is better to disable packed in the stable branches, > > otherwise I have to make a list of all the patches we need. > > > > Any other ideas? > > AFAIK, except for vp_vdpa, pds seems to be the first parent that > supports packed virtqueue. Users should not notice anything wrong if > they don't use packed virtqueue. And the problem of vp_vdpa + packed > virtqueue came since the day0 of vp_vdpa. It seems fine to do nothing > I guess. > > Thanks I have a question though, what if down the road there is a new feature that needs more changes? It will be broken too just like PACKED no? Shouldn't vdpa have an allowlist of features it knows how to support? > > > > Thanks, > > Stefano > > > >
On Tue, Jun 6, 2023 at 2:58 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Jun 06, 2023 at 09:29:22AM +0800, Jason Wang wrote: > > On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > > On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote: > > > >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote: > > > >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote: > > > >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote: > > > >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote: > > > >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: > > > >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) > > > >> > > > > don't support packed virtqueue well yet, so let's filter the > > > >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). > > > >> > > > > > > > >> > > > > This way, even if the device supports it, we don't risk it being > > > >> > > > > negotiated, then the VMM is unable to set the vring state properly. > > > >> > > > > > > > >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > > >> > > > > Cc: stable@vger.kernel.org > > > >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > >> > > > > --- > > > >> > > > > > > > >> > > > > Notes: > > > >> > > > > This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: > > > >> > > > > better PACKED support" series [1] and backported in stable branches. > > > >> > > > > > > > >> > > > > We can revert it when we are sure that everything is working with > > > >> > > > > packed virtqueues. > > > >> > > > > > > > >> > > > > Thanks, > > > >> > > > > Stefano > > > >> > > > > > > > >> > > > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > > > >> > > > > > > >> > > > I'm a bit lost here. So why am I merging "better PACKED support" then? > > > >> > > > > > >> > > To really support packed virtqueue with vhost-vdpa, at that point we would > > > >> > > also have to revert this patch. > > > >> > > > > > >> > > I wasn't sure if you wanted to queue the series for this merge window. > > > >> > > In that case do you think it is better to send this patch only for stable > > > >> > > branches? > > > >> > > > Does this patch make them a NOP? > > > >> > > > > > >> > > Yep, after applying the "better PACKED support" series and being > > > >> > > sure that > > > >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this > > > >> > > patch. > > > >> > > > > > >> > > Let me know if you prefer a different approach. > > > >> > > > > > >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel > > > >> > > interprets them the right way, when it does not. > > > >> > > > > > >> > > Thanks, > > > >> > > Stefano > > > >> > > > > > >> > > > > >> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok > > > >> > to merge in this window. Probably easier than the elaborate > > > >> > mask/unmask dance. > > > >> > > > >> CCing Shannon (the original author of the "better PACKED support" > > > >> series). > > > >> > > > >> IIUC Shannon is going to send a v3 of that series to fix the > > > >> documentation, so Shannon can you also add the Fixes tags? > > > >> > > > >> Thanks, > > > >> Stefano > > > > > > > >Well this is in my tree already. Just reply with > > > >Fixes: <> > > > >to each and I will add these tags. > > > > > > I tried, but it is not easy since we added the support for packed > > > virtqueue in vdpa and vhost incrementally. > > > > > > Initially I was thinking of adding the same tag used here: > > > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > > > > > Then I discovered that vq_state wasn't there, so I was thinking of > > > > > > Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()") > > > > > > So we would have to backport quite a few patches into the stable branches. > > > I don't know if it's worth it... > > > > > > I still think it is better to disable packed in the stable branches, > > > otherwise I have to make a list of all the patches we need. > > > > > > Any other ideas? > > > > AFAIK, except for vp_vdpa, pds seems to be the first parent that > > supports packed virtqueue. Users should not notice anything wrong if > > they don't use packed virtqueue. And the problem of vp_vdpa + packed > > virtqueue came since the day0 of vp_vdpa. It seems fine to do nothing > > I guess. > > > > Thanks > > > I have a question though, what if down the road there > is a new feature that needs more changes? It will be > broken too just like PACKED no? > Shouldn't vdpa have an allowlist of features it knows how > to support? It looks like we had it, but we took it out (by the way, we were enabling packed even though we didn't support it): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b The only problem I see is that for each new feature we have to modify the kernel. Could we have new features that don't require handling by vhost-vdpa? Thanks, Stefano
On Wed, Jun 07, 2023 at 10:39:15AM +0200, Stefano Garzarella wrote: > On Tue, Jun 6, 2023 at 2:58 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Tue, Jun 06, 2023 at 09:29:22AM +0800, Jason Wang wrote: > > > On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > > > > On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote: > > > > >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote: > > > > >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote: > > > > >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote: > > > > >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote: > > > > >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: > > > > >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) > > > > >> > > > > don't support packed virtqueue well yet, so let's filter the > > > > >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). > > > > >> > > > > > > > > >> > > > > This way, even if the device supports it, we don't risk it being > > > > >> > > > > negotiated, then the VMM is unable to set the vring state properly. > > > > >> > > > > > > > > >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > > > >> > > > > Cc: stable@vger.kernel.org > > > > >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > > >> > > > > --- > > > > >> > > > > > > > > >> > > > > Notes: > > > > >> > > > > This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: > > > > >> > > > > better PACKED support" series [1] and backported in stable branches. > > > > >> > > > > > > > > >> > > > > We can revert it when we are sure that everything is working with > > > > >> > > > > packed virtqueues. > > > > >> > > > > > > > > >> > > > > Thanks, > > > > >> > > > > Stefano > > > > >> > > > > > > > > >> > > > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > > > > >> > > > > > > > >> > > > I'm a bit lost here. So why am I merging "better PACKED support" then? > > > > >> > > > > > > >> > > To really support packed virtqueue with vhost-vdpa, at that point we would > > > > >> > > also have to revert this patch. > > > > >> > > > > > > >> > > I wasn't sure if you wanted to queue the series for this merge window. > > > > >> > > In that case do you think it is better to send this patch only for stable > > > > >> > > branches? > > > > >> > > > Does this patch make them a NOP? > > > > >> > > > > > > >> > > Yep, after applying the "better PACKED support" series and being > > > > >> > > sure that > > > > >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this > > > > >> > > patch. > > > > >> > > > > > > >> > > Let me know if you prefer a different approach. > > > > >> > > > > > > >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel > > > > >> > > interprets them the right way, when it does not. > > > > >> > > > > > > >> > > Thanks, > > > > >> > > Stefano > > > > >> > > > > > > >> > > > > > >> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok > > > > >> > to merge in this window. Probably easier than the elaborate > > > > >> > mask/unmask dance. > > > > >> > > > > >> CCing Shannon (the original author of the "better PACKED support" > > > > >> series). > > > > >> > > > > >> IIUC Shannon is going to send a v3 of that series to fix the > > > > >> documentation, so Shannon can you also add the Fixes tags? > > > > >> > > > > >> Thanks, > > > > >> Stefano > > > > > > > > > >Well this is in my tree already. Just reply with > > > > >Fixes: <> > > > > >to each and I will add these tags. > > > > > > > > I tried, but it is not easy since we added the support for packed > > > > virtqueue in vdpa and vhost incrementally. > > > > > > > > Initially I was thinking of adding the same tag used here: > > > > > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > > > > > > > Then I discovered that vq_state wasn't there, so I was thinking of > > > > > > > > Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()") > > > > > > > > So we would have to backport quite a few patches into the stable branches. > > > > I don't know if it's worth it... > > > > > > > > I still think it is better to disable packed in the stable branches, > > > > otherwise I have to make a list of all the patches we need. > > > > > > > > Any other ideas? > > > > > > AFAIK, except for vp_vdpa, pds seems to be the first parent that > > > supports packed virtqueue. Users should not notice anything wrong if > > > they don't use packed virtqueue. And the problem of vp_vdpa + packed > > > virtqueue came since the day0 of vp_vdpa. It seems fine to do nothing > > > I guess. > > > > > > Thanks > > > > > > I have a question though, what if down the road there > > is a new feature that needs more changes? It will be > > broken too just like PACKED no? > > Shouldn't vdpa have an allowlist of features it knows how > > to support? > > It looks like we had it, but we took it out (by the way, we were > enabling packed even though we didn't support it): > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b > > The only problem I see is that for each new feature we have to modify > the kernel. > Could we have new features that don't require handling by vhost-vdpa? > > Thanks, > Stefano Jason what do you say to reverting this?
On Tue, Jun 06, 2023 at 12:09:11PM +0200, Stefano Garzarella wrote: > On Mon, Jun 05, 2023 at 05:44:50PM -0400, Michael S. Tsirkin wrote: > > On Mon, Jun 05, 2023 at 04:56:37PM +0200, Stefano Garzarella wrote: > > > On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote: > > > > On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote: > > > > > On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote: > > > > > > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote: > > > > > > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote: > > > > > > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: > > > > > > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) > > > > > > > > > don't support packed virtqueue well yet, so let's filter the > > > > > > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). > > > > > > > > > > > > > > > > > > This way, even if the device supports it, we don't risk it being > > > > > > > > > negotiated, then the VMM is unable to set the vring state properly. > > > > > > > > > > > > > > > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > > > > > > > > Cc: stable@vger.kernel.org > > > > > > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > > > > > > > --- > > > > > > > > > > > > > > > > > > Notes: > > > > > > > > > This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: > > > > > > > > > better PACKED support" series [1] and backported in stable branches. > > > > > > > > > > > > > > > > > > We can revert it when we are sure that everything is working with > > > > > > > > > packed virtqueues. > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > Stefano > > > > > > > > > > > > > > > > > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > > > > > > > > > > > > > > > > I'm a bit lost here. So why am I merging "better PACKED support" then? > > > > > > > > > > > > > > To really support packed virtqueue with vhost-vdpa, at that point we would > > > > > > > also have to revert this patch. > > > > > > > > > > > > > > I wasn't sure if you wanted to queue the series for this merge window. > > > > > > > In that case do you think it is better to send this patch only for stable > > > > > > > branches? > > > > > > > > Does this patch make them a NOP? > > > > > > > > > > > > > > Yep, after applying the "better PACKED support" series and being > > > > > > > sure that > > > > > > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this > > > > > > > patch. > > > > > > > > > > > > > > Let me know if you prefer a different approach. > > > > > > > > > > > > > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel > > > > > > > interprets them the right way, when it does not. > > > > > > > > > > > > > > Thanks, > > > > > > > Stefano > > > > > > > > > > > > > > > > > > > If this fixes a bug can you add Fixes tags to each of them? Then it's ok > > > > > > to merge in this window. Probably easier than the elaborate > > > > > > mask/unmask dance. > > > > > > > > > > CCing Shannon (the original author of the "better PACKED support" > > > > > series). > > > > > > > > > > IIUC Shannon is going to send a v3 of that series to fix the > > > > > documentation, so Shannon can you also add the Fixes tags? > > > > > > > > > > Thanks, > > > > > Stefano > > > > > > > > Well this is in my tree already. Just reply with > > > > Fixes: <> > > > > to each and I will add these tags. > > > > > > I tried, but it is not easy since we added the support for packed virtqueue > > > in vdpa and vhost incrementally. > > > > > > Initially I was thinking of adding the same tag used here: > > > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > > > > > Then I discovered that vq_state wasn't there, so I was thinking of > > > > > > Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()") > > > > > > So we would have to backport quite a few patches into the stable branches. > > > I don't know if it's worth it... > > > > > > I still think it is better to disable packed in the stable branches, > > > otherwise I have to make a list of all the patches we need. > > > > > > Any other ideas? > > > > > > Thanks, > > > Stefano > > > > OK so. You want me to apply this one now, and fixes in the next > > kernel? > > Yep, it seems to me the least risky approach. > > Thanks, > Stefano I'd like something more general though. Bring back the allowlist? Jason, your thoughts?
On Wed, Jun 7, 2023 at 5:43 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Jun 07, 2023 at 10:39:15AM +0200, Stefano Garzarella wrote: > > On Tue, Jun 6, 2023 at 2:58 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Tue, Jun 06, 2023 at 09:29:22AM +0800, Jason Wang wrote: > > > > On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > > > > > > On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote: > > > > > >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote: > > > > > >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote: > > > > > >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote: > > > > > >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote: > > > > > >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: > > > > > >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) > > > > > >> > > > > don't support packed virtqueue well yet, so let's filter the > > > > > >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). > > > > > >> > > > > > > > > > >> > > > > This way, even if the device supports it, we don't risk it being > > > > > >> > > > > negotiated, then the VMM is unable to set the vring state properly. > > > > > >> > > > > > > > > > >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > > > > >> > > > > Cc: stable@vger.kernel.org > > > > > >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > > > >> > > > > --- > > > > > >> > > > > > > > > > >> > > > > Notes: > > > > > >> > > > > This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: > > > > > >> > > > > better PACKED support" series [1] and backported in stable branches. > > > > > >> > > > > > > > > > >> > > > > We can revert it when we are sure that everything is working with > > > > > >> > > > > packed virtqueues. > > > > > >> > > > > > > > > > >> > > > > Thanks, > > > > > >> > > > > Stefano > > > > > >> > > > > > > > > > >> > > > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > > > > > >> > > > > > > > > >> > > > I'm a bit lost here. So why am I merging "better PACKED support" then? > > > > > >> > > > > > > > >> > > To really support packed virtqueue with vhost-vdpa, at that point we would > > > > > >> > > also have to revert this patch. > > > > > >> > > > > > > > >> > > I wasn't sure if you wanted to queue the series for this merge window. > > > > > >> > > In that case do you think it is better to send this patch only for stable > > > > > >> > > branches? > > > > > >> > > > Does this patch make them a NOP? > > > > > >> > > > > > > > >> > > Yep, after applying the "better PACKED support" series and being > > > > > >> > > sure that > > > > > >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this > > > > > >> > > patch. > > > > > >> > > > > > > > >> > > Let me know if you prefer a different approach. > > > > > >> > > > > > > > >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel > > > > > >> > > interprets them the right way, when it does not. > > > > > >> > > > > > > > >> > > Thanks, > > > > > >> > > Stefano > > > > > >> > > > > > > > >> > > > > > > >> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok > > > > > >> > to merge in this window. Probably easier than the elaborate > > > > > >> > mask/unmask dance. > > > > > >> > > > > > >> CCing Shannon (the original author of the "better PACKED support" > > > > > >> series). > > > > > >> > > > > > >> IIUC Shannon is going to send a v3 of that series to fix the > > > > > >> documentation, so Shannon can you also add the Fixes tags? > > > > > >> > > > > > >> Thanks, > > > > > >> Stefano > > > > > > > > > > > >Well this is in my tree already. Just reply with > > > > > >Fixes: <> > > > > > >to each and I will add these tags. > > > > > > > > > > I tried, but it is not easy since we added the support for packed > > > > > virtqueue in vdpa and vhost incrementally. > > > > > > > > > > Initially I was thinking of adding the same tag used here: > > > > > > > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > > > > > > > > > Then I discovered that vq_state wasn't there, so I was thinking of > > > > > > > > > > Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()") > > > > > > > > > > So we would have to backport quite a few patches into the stable branches. > > > > > I don't know if it's worth it... > > > > > > > > > > I still think it is better to disable packed in the stable branches, > > > > > otherwise I have to make a list of all the patches we need. > > > > > > > > > > Any other ideas? > > > > > > > > AFAIK, except for vp_vdpa, pds seems to be the first parent that > > > > supports packed virtqueue. Users should not notice anything wrong if > > > > they don't use packed virtqueue. And the problem of vp_vdpa + packed > > > > virtqueue came since the day0 of vp_vdpa. It seems fine to do nothing > > > > I guess. > > > > > > > > Thanks > > > > > > > > > I have a question though, what if down the road there > > > is a new feature that needs more changes? It will be > > > broken too just like PACKED no? > > > Shouldn't vdpa have an allowlist of features it knows how > > > to support? > > > > It looks like we had it, but we took it out (by the way, we were > > enabling packed even though we didn't support it): > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b > > > > The only problem I see is that for each new feature we have to modify > > the kernel. > > Could we have new features that don't require handling by vhost-vdpa? > > > > Thanks, > > Stefano > > Jason what do you say to reverting this? I may miss something but I don't see any problem with vDPA core. It's the duty of the parents to advertise the features it has. For example, 1) If some kernel version that is packed is not supported via set_vq_state, parents should not advertise PACKED features in this case. 2) If the kernel has support packed set_vq_state(), but it's emulated cvq doesn't support, parents should not advertise PACKED as well If a parent violates the above 2, it looks like a bug of the parents. Thanks > > -- > MST >
On Thu, Jun 08, 2023 at 08:42:15AM +0800, Jason Wang wrote: > On Wed, Jun 7, 2023 at 5:43 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Wed, Jun 07, 2023 at 10:39:15AM +0200, Stefano Garzarella wrote: > > > On Tue, Jun 6, 2023 at 2:58 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Tue, Jun 06, 2023 at 09:29:22AM +0800, Jason Wang wrote: > > > > > On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > > > > > > > > On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote: > > > > > > >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote: > > > > > > >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote: > > > > > > >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote: > > > > > > >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote: > > > > > > >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: > > > > > > >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) > > > > > > >> > > > > don't support packed virtqueue well yet, so let's filter the > > > > > > >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). > > > > > > >> > > > > > > > > > > >> > > > > This way, even if the device supports it, we don't risk it being > > > > > > >> > > > > negotiated, then the VMM is unable to set the vring state properly. > > > > > > >> > > > > > > > > > > >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > > > > > >> > > > > Cc: stable@vger.kernel.org > > > > > > >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > > > > >> > > > > --- > > > > > > >> > > > > > > > > > > >> > > > > Notes: > > > > > > >> > > > > This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: > > > > > > >> > > > > better PACKED support" series [1] and backported in stable branches. > > > > > > >> > > > > > > > > > > >> > > > > We can revert it when we are sure that everything is working with > > > > > > >> > > > > packed virtqueues. > > > > > > >> > > > > > > > > > > >> > > > > Thanks, > > > > > > >> > > > > Stefano > > > > > > >> > > > > > > > > > > >> > > > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > > > > > > >> > > > > > > > > > >> > > > I'm a bit lost here. So why am I merging "better PACKED support" then? > > > > > > >> > > > > > > > > >> > > To really support packed virtqueue with vhost-vdpa, at that point we would > > > > > > >> > > also have to revert this patch. > > > > > > >> > > > > > > > > >> > > I wasn't sure if you wanted to queue the series for this merge window. > > > > > > >> > > In that case do you think it is better to send this patch only for stable > > > > > > >> > > branches? > > > > > > >> > > > Does this patch make them a NOP? > > > > > > >> > > > > > > > > >> > > Yep, after applying the "better PACKED support" series and being > > > > > > >> > > sure that > > > > > > >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this > > > > > > >> > > patch. > > > > > > >> > > > > > > > > >> > > Let me know if you prefer a different approach. > > > > > > >> > > > > > > > > >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel > > > > > > >> > > interprets them the right way, when it does not. > > > > > > >> > > > > > > > > >> > > Thanks, > > > > > > >> > > Stefano > > > > > > >> > > > > > > > > >> > > > > > > > >> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok > > > > > > >> > to merge in this window. Probably easier than the elaborate > > > > > > >> > mask/unmask dance. > > > > > > >> > > > > > > >> CCing Shannon (the original author of the "better PACKED support" > > > > > > >> series). > > > > > > >> > > > > > > >> IIUC Shannon is going to send a v3 of that series to fix the > > > > > > >> documentation, so Shannon can you also add the Fixes tags? > > > > > > >> > > > > > > >> Thanks, > > > > > > >> Stefano > > > > > > > > > > > > > >Well this is in my tree already. Just reply with > > > > > > >Fixes: <> > > > > > > >to each and I will add these tags. > > > > > > > > > > > > I tried, but it is not easy since we added the support for packed > > > > > > virtqueue in vdpa and vhost incrementally. > > > > > > > > > > > > Initially I was thinking of adding the same tag used here: > > > > > > > > > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > > > > > > > > > > > Then I discovered that vq_state wasn't there, so I was thinking of > > > > > > > > > > > > Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()") > > > > > > > > > > > > So we would have to backport quite a few patches into the stable branches. > > > > > > I don't know if it's worth it... > > > > > > > > > > > > I still think it is better to disable packed in the stable branches, > > > > > > otherwise I have to make a list of all the patches we need. > > > > > > > > > > > > Any other ideas? > > > > > > > > > > AFAIK, except for vp_vdpa, pds seems to be the first parent that > > > > > supports packed virtqueue. Users should not notice anything wrong if > > > > > they don't use packed virtqueue. And the problem of vp_vdpa + packed > > > > > virtqueue came since the day0 of vp_vdpa. It seems fine to do nothing > > > > > I guess. > > > > > > > > > > Thanks > > > > > > > > > > > > I have a question though, what if down the road there > > > > is a new feature that needs more changes? It will be > > > > broken too just like PACKED no? > > > > Shouldn't vdpa have an allowlist of features it knows how > > > > to support? > > > > > > It looks like we had it, but we took it out (by the way, we were > > > enabling packed even though we didn't support it): > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b > > > > > > The only problem I see is that for each new feature we have to modify > > > the kernel. > > > Could we have new features that don't require handling by vhost-vdpa? > > > > > > Thanks, > > > Stefano > > > > Jason what do you say to reverting this? > > I may miss something but I don't see any problem with vDPA core. > > It's the duty of the parents to advertise the features it has. For example, > > 1) If some kernel version that is packed is not supported via > set_vq_state, parents should not advertise PACKED features in this > case. > 2) If the kernel has support packed set_vq_state(), but it's emulated > cvq doesn't support, parents should not advertise PACKED as well > > If a parent violates the above 2, it looks like a bug of the parents. > > Thanks Yes but what about vhost_vdpa? Talking about that not the core. Should that not have a whitelist of features since it interprets ioctls differently depending on this? > > > > -- > > MST > >
On Thu, Jun 8, 2023 at 2:03 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Jun 08, 2023 at 08:42:15AM +0800, Jason Wang wrote: > > On Wed, Jun 7, 2023 at 5:43 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Wed, Jun 07, 2023 at 10:39:15AM +0200, Stefano Garzarella wrote: > > > > On Tue, Jun 6, 2023 at 2:58 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > On Tue, Jun 06, 2023 at 09:29:22AM +0800, Jason Wang wrote: > > > > > > On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > > > > > > > > > > On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote: > > > > > > > >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote: > > > > > > > >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote: > > > > > > > >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote: > > > > > > > >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote: > > > > > > > >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: > > > > > > > >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) > > > > > > > >> > > > > don't support packed virtqueue well yet, so let's filter the > > > > > > > >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). > > > > > > > >> > > > > > > > > > > > >> > > > > This way, even if the device supports it, we don't risk it being > > > > > > > >> > > > > negotiated, then the VMM is unable to set the vring state properly. > > > > > > > >> > > > > > > > > > > > >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > > > > > > >> > > > > Cc: stable@vger.kernel.org > > > > > > > >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > > > > > >> > > > > --- > > > > > > > >> > > > > > > > > > > > >> > > > > Notes: > > > > > > > >> > > > > This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: > > > > > > > >> > > > > better PACKED support" series [1] and backported in stable branches. > > > > > > > >> > > > > > > > > > > > >> > > > > We can revert it when we are sure that everything is working with > > > > > > > >> > > > > packed virtqueues. > > > > > > > >> > > > > > > > > > > > >> > > > > Thanks, > > > > > > > >> > > > > Stefano > > > > > > > >> > > > > > > > > > > > >> > > > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > > > > > > > >> > > > > > > > > > > >> > > > I'm a bit lost here. So why am I merging "better PACKED support" then? > > > > > > > >> > > > > > > > > > >> > > To really support packed virtqueue with vhost-vdpa, at that point we would > > > > > > > >> > > also have to revert this patch. > > > > > > > >> > > > > > > > > > >> > > I wasn't sure if you wanted to queue the series for this merge window. > > > > > > > >> > > In that case do you think it is better to send this patch only for stable > > > > > > > >> > > branches? > > > > > > > >> > > > Does this patch make them a NOP? > > > > > > > >> > > > > > > > > > >> > > Yep, after applying the "better PACKED support" series and being > > > > > > > >> > > sure that > > > > > > > >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this > > > > > > > >> > > patch. > > > > > > > >> > > > > > > > > > >> > > Let me know if you prefer a different approach. > > > > > > > >> > > > > > > > > > >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel > > > > > > > >> > > interprets them the right way, when it does not. > > > > > > > >> > > > > > > > > > >> > > Thanks, > > > > > > > >> > > Stefano > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok > > > > > > > >> > to merge in this window. Probably easier than the elaborate > > > > > > > >> > mask/unmask dance. > > > > > > > >> > > > > > > > >> CCing Shannon (the original author of the "better PACKED support" > > > > > > > >> series). > > > > > > > >> > > > > > > > >> IIUC Shannon is going to send a v3 of that series to fix the > > > > > > > >> documentation, so Shannon can you also add the Fixes tags? > > > > > > > >> > > > > > > > >> Thanks, > > > > > > > >> Stefano > > > > > > > > > > > > > > > >Well this is in my tree already. Just reply with > > > > > > > >Fixes: <> > > > > > > > >to each and I will add these tags. > > > > > > > > > > > > > > I tried, but it is not easy since we added the support for packed > > > > > > > virtqueue in vdpa and vhost incrementally. > > > > > > > > > > > > > > Initially I was thinking of adding the same tag used here: > > > > > > > > > > > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > > > > > > > > > > > > > Then I discovered that vq_state wasn't there, so I was thinking of > > > > > > > > > > > > > > Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()") > > > > > > > > > > > > > > So we would have to backport quite a few patches into the stable branches. > > > > > > > I don't know if it's worth it... > > > > > > > > > > > > > > I still think it is better to disable packed in the stable branches, > > > > > > > otherwise I have to make a list of all the patches we need. > > > > > > > > > > > > > > Any other ideas? > > > > > > > > > > > > AFAIK, except for vp_vdpa, pds seems to be the first parent that > > > > > > supports packed virtqueue. Users should not notice anything wrong if > > > > > > they don't use packed virtqueue. And the problem of vp_vdpa + packed > > > > > > virtqueue came since the day0 of vp_vdpa. It seems fine to do nothing > > > > > > I guess. > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > I have a question though, what if down the road there > > > > > is a new feature that needs more changes? It will be > > > > > broken too just like PACKED no? > > > > > Shouldn't vdpa have an allowlist of features it knows how > > > > > to support? > > > > > > > > It looks like we had it, but we took it out (by the way, we were > > > > enabling packed even though we didn't support it): > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b > > > > > > > > The only problem I see is that for each new feature we have to modify > > > > the kernel. > > > > Could we have new features that don't require handling by vhost-vdpa? > > > > > > > > Thanks, > > > > Stefano > > > > > > Jason what do you say to reverting this? > > > > I may miss something but I don't see any problem with vDPA core. > > > > It's the duty of the parents to advertise the features it has. For example, > > > > 1) If some kernel version that is packed is not supported via > > set_vq_state, parents should not advertise PACKED features in this > > case. > > 2) If the kernel has support packed set_vq_state(), but it's emulated > > cvq doesn't support, parents should not advertise PACKED as well > > > > If a parent violates the above 2, it looks like a bug of the parents. > > > > Thanks > > Yes but what about vhost_vdpa? Talking about that not the core. Not sure it's a good idea to workaround parent bugs via vhost-vDPA. > Should that not have a whitelist of features > since it interprets ioctls differently depending on this? If there's a bug, it might only matter the following setup: SET_VRING_BASE/GET_VRING_BASE + VDUSE. This seems to be broken since VDUSE was introduced. If we really want to backport something, it could be a fix to filter out PACKED in VDUSE? Thanks > > > > > > > -- > > > MST > > > >
On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote: [...] >> > > > > I have a question though, what if down the road there >> > > > > is a new feature that needs more changes? It will be >> > > > > broken too just like PACKED no? >> > > > > Shouldn't vdpa have an allowlist of features it knows how >> > > > > to support? >> > > > >> > > > It looks like we had it, but we took it out (by the way, we were >> > > > enabling packed even though we didn't support it): >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b >> > > > >> > > > The only problem I see is that for each new feature we have to modify >> > > > the kernel. >> > > > Could we have new features that don't require handling by vhost-vdpa? >> > > > >> > > > Thanks, >> > > > Stefano >> > > >> > > Jason what do you say to reverting this? >> > >> > I may miss something but I don't see any problem with vDPA core. >> > >> > It's the duty of the parents to advertise the features it has. For example, >> > >> > 1) If some kernel version that is packed is not supported via >> > set_vq_state, parents should not advertise PACKED features in this >> > case. >> > 2) If the kernel has support packed set_vq_state(), but it's emulated >> > cvq doesn't support, parents should not advertise PACKED as well >> > >> > If a parent violates the above 2, it looks like a bug of the parents. >> > >> > Thanks >> >> Yes but what about vhost_vdpa? Talking about that not the core. > >Not sure it's a good idea to workaround parent bugs via vhost-vDPA. Sorry, I'm getting lost... We were talking about the fact that vhost-vdpa doesn't handle SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before that series [1], no? The parents seem okay, but maybe I missed a few things. [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > >> Should that not have a whitelist of features >> since it interprets ioctls differently depending on this? > >If there's a bug, it might only matter the following setup: > >SET_VRING_BASE/GET_VRING_BASE + VDUSE. > >This seems to be broken since VDUSE was introduced. If we really want >to backport something, it could be a fix to filter out PACKED in >VDUSE? mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa. I think VDUSE works fine with packed virtqueue using virtio-vdpa (I haven't tried), so why should we filter PACKED in VDUSE? Thanks, Stefano
On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote: > > [...] > > >> > > > > I have a question though, what if down the road there > >> > > > > is a new feature that needs more changes? It will be > >> > > > > broken too just like PACKED no? > >> > > > > Shouldn't vdpa have an allowlist of features it knows how > >> > > > > to support? > >> > > > > >> > > > It looks like we had it, but we took it out (by the way, we were > >> > > > enabling packed even though we didn't support it): > >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b > >> > > > > >> > > > The only problem I see is that for each new feature we have to modify > >> > > > the kernel. > >> > > > Could we have new features that don't require handling by vhost-vdpa? > >> > > > > >> > > > Thanks, > >> > > > Stefano > >> > > > >> > > Jason what do you say to reverting this? > >> > > >> > I may miss something but I don't see any problem with vDPA core. > >> > > >> > It's the duty of the parents to advertise the features it has. For example, > >> > > >> > 1) If some kernel version that is packed is not supported via > >> > set_vq_state, parents should not advertise PACKED features in this > >> > case. > >> > 2) If the kernel has support packed set_vq_state(), but it's emulated > >> > cvq doesn't support, parents should not advertise PACKED as well > >> > > >> > If a parent violates the above 2, it looks like a bug of the parents. > >> > > >> > Thanks > >> > >> Yes but what about vhost_vdpa? Talking about that not the core. > > > >Not sure it's a good idea to workaround parent bugs via vhost-vDPA. > > Sorry, I'm getting lost... > We were talking about the fact that vhost-vdpa doesn't handle > SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before > that series [1], no? > > The parents seem okay, but maybe I missed a few things. > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ Yes, more below. > > > > >> Should that not have a whitelist of features > >> since it interprets ioctls differently depending on this? > > > >If there's a bug, it might only matter the following setup: > > > >SET_VRING_BASE/GET_VRING_BASE + VDUSE. > > > >This seems to be broken since VDUSE was introduced. If we really want > >to backport something, it could be a fix to filter out PACKED in > >VDUSE? > > mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa. > I think VDUSE works fine with packed virtqueue using virtio-vdpa > (I haven't tried), so why should we filter PACKED in VDUSE? I don't think we need any filtering since: PACKED features has been advertised to userspace via uAPI since 6234f80574d7569444d8718355fa2838e92b158b. Once we relax in uAPI, it would be very hard to restrict it again. For the userspace that tries to negotiate PACKED: 1) if it doesn't use SET_VRING_BASE/GET_VRING_BASE, everything works well 2) if it uses SET_VRING_BASE/GET_VRING_BASE. it might fail or break silently If we backport the fixes to -stable, we may break the application at least in the case 1). Thanks > > Thanks, > Stefano >
On Thu, Jun 08, 2023 at 05:00:00PM +0800, Jason Wang wrote: >On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote: >> >> On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote: >> >> [...] >> >> >> > > > > I have a question though, what if down the road there >> >> > > > > is a new feature that needs more changes? It will be >> >> > > > > broken too just like PACKED no? >> >> > > > > Shouldn't vdpa have an allowlist of features it knows how >> >> > > > > to support? >> >> > > > >> >> > > > It looks like we had it, but we took it out (by the way, we were >> >> > > > enabling packed even though we didn't support it): >> >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b >> >> > > > >> >> > > > The only problem I see is that for each new feature we have to modify >> >> > > > the kernel. >> >> > > > Could we have new features that don't require handling by vhost-vdpa? >> >> > > > >> >> > > > Thanks, >> >> > > > Stefano >> >> > > >> >> > > Jason what do you say to reverting this? >> >> > >> >> > I may miss something but I don't see any problem with vDPA core. >> >> > >> >> > It's the duty of the parents to advertise the features it has. For example, >> >> > >> >> > 1) If some kernel version that is packed is not supported via >> >> > set_vq_state, parents should not advertise PACKED features in this >> >> > case. >> >> > 2) If the kernel has support packed set_vq_state(), but it's emulated >> >> > cvq doesn't support, parents should not advertise PACKED as well >> >> > >> >> > If a parent violates the above 2, it looks like a bug of the parents. >> >> > >> >> > Thanks >> >> >> >> Yes but what about vhost_vdpa? Talking about that not the core. >> > >> >Not sure it's a good idea to workaround parent bugs via vhost-vDPA. >> >> Sorry, I'm getting lost... >> We were talking about the fact that vhost-vdpa doesn't handle >> SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before >> that series [1], no? >> >> The parents seem okay, but maybe I missed a few things. >> >> [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > >Yes, more below. > >> >> > >> >> Should that not have a whitelist of features >> >> since it interprets ioctls differently depending on this? >> > >> >If there's a bug, it might only matter the following setup: >> > >> >SET_VRING_BASE/GET_VRING_BASE + VDUSE. >> > >> >This seems to be broken since VDUSE was introduced. If we really want >> >to backport something, it could be a fix to filter out PACKED in >> >VDUSE? >> >> mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa. >> I think VDUSE works fine with packed virtqueue using virtio-vdpa >> (I haven't tried), so why should we filter PACKED in VDUSE? > >I don't think we need any filtering since: > >PACKED features has been advertised to userspace via uAPI since >6234f80574d7569444d8718355fa2838e92b158b. Once we relax in uAPI, it >would be very hard to restrict it again. For the userspace that tries >to negotiate PACKED: > >1) if it doesn't use SET_VRING_BASE/GET_VRING_BASE, everything works well >2) if it uses SET_VRING_BASE/GET_VRING_BASE. it might fail or break silently > >If we backport the fixes to -stable, we may break the application at >least in the case 1). Okay, I see now, thanks for the details! Maybe instead of "break silently", we can return an explicit error for SET_VRING_BASE/GET_VRING_BASE in stable branches. But if there are not many cases, we can leave it like that. I was just concerned about how does the user space understand that it can use SET_VRING_BASE/GET_VRING_BASE for PACKED virtqueues in a given kernel or not. Thanks, Stefano
On Thu, Jun 8, 2023 at 5:21 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > On Thu, Jun 08, 2023 at 05:00:00PM +0800, Jason Wang wrote: > >On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > >> > >> On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote: > >> > >> [...] > >> > >> >> > > > > I have a question though, what if down the road there > >> >> > > > > is a new feature that needs more changes? It will be > >> >> > > > > broken too just like PACKED no? > >> >> > > > > Shouldn't vdpa have an allowlist of features it knows how > >> >> > > > > to support? > >> >> > > > > >> >> > > > It looks like we had it, but we took it out (by the way, we were > >> >> > > > enabling packed even though we didn't support it): > >> >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b > >> >> > > > > >> >> > > > The only problem I see is that for each new feature we have to modify > >> >> > > > the kernel. > >> >> > > > Could we have new features that don't require handling by vhost-vdpa? > >> >> > > > > >> >> > > > Thanks, > >> >> > > > Stefano > >> >> > > > >> >> > > Jason what do you say to reverting this? > >> >> > > >> >> > I may miss something but I don't see any problem with vDPA core. > >> >> > > >> >> > It's the duty of the parents to advertise the features it has. For example, > >> >> > > >> >> > 1) If some kernel version that is packed is not supported via > >> >> > set_vq_state, parents should not advertise PACKED features in this > >> >> > case. > >> >> > 2) If the kernel has support packed set_vq_state(), but it's emulated > >> >> > cvq doesn't support, parents should not advertise PACKED as well > >> >> > > >> >> > If a parent violates the above 2, it looks like a bug of the parents. > >> >> > > >> >> > Thanks > >> >> > >> >> Yes but what about vhost_vdpa? Talking about that not the core. > >> > > >> >Not sure it's a good idea to workaround parent bugs via vhost-vDPA. > >> > >> Sorry, I'm getting lost... > >> We were talking about the fact that vhost-vdpa doesn't handle > >> SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before > >> that series [1], no? > >> > >> The parents seem okay, but maybe I missed a few things. > >> > >> [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > > > >Yes, more below. > > > >> > >> > > >> >> Should that not have a whitelist of features > >> >> since it interprets ioctls differently depending on this? > >> > > >> >If there's a bug, it might only matter the following setup: > >> > > >> >SET_VRING_BASE/GET_VRING_BASE + VDUSE. > >> > > >> >This seems to be broken since VDUSE was introduced. If we really want > >> >to backport something, it could be a fix to filter out PACKED in > >> >VDUSE? > >> > >> mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa. > >> I think VDUSE works fine with packed virtqueue using virtio-vdpa > >> (I haven't tried), so why should we filter PACKED in VDUSE? > > > >I don't think we need any filtering since: > > > >PACKED features has been advertised to userspace via uAPI since > >6234f80574d7569444d8718355fa2838e92b158b. Once we relax in uAPI, it > >would be very hard to restrict it again. For the userspace that tries > >to negotiate PACKED: > > > >1) if it doesn't use SET_VRING_BASE/GET_VRING_BASE, everything works well > >2) if it uses SET_VRING_BASE/GET_VRING_BASE. it might fail or break silently > > > >If we backport the fixes to -stable, we may break the application at > >least in the case 1). > > Okay, I see now, thanks for the details! > > Maybe instead of "break silently", we can return an explicit error for > SET_VRING_BASE/GET_VRING_BASE in stable branches. > But if there are not many cases, we can leave it like that. A second thought, if we need to do something for stable. is it better if we just backport Shannon's series to stable? > > I was just concerned about how does the user space understand that it > can use SET_VRING_BASE/GET_VRING_BASE for PACKED virtqueues in a given > kernel or not. My understanding is that if packed is advertised, the application should assume SET/GET_VRING_BASE work. Thanks > > Thanks, > Stefano >
On Thu, Jun 08, 2023 at 05:29:58PM +0800, Jason Wang wrote: >On Thu, Jun 8, 2023 at 5:21 PM Stefano Garzarella <sgarzare@redhat.com> wrote: >> >> On Thu, Jun 08, 2023 at 05:00:00PM +0800, Jason Wang wrote: >> >On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote: >> >> >> >> On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote: >> >> >> >> [...] >> >> >> >> >> > > > > I have a question though, what if down the road there >> >> >> > > > > is a new feature that needs more changes? It will be >> >> >> > > > > broken too just like PACKED no? >> >> >> > > > > Shouldn't vdpa have an allowlist of features it knows how >> >> >> > > > > to support? >> >> >> > > > >> >> >> > > > It looks like we had it, but we took it out (by the way, we were >> >> >> > > > enabling packed even though we didn't support it): >> >> >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b >> >> >> > > > >> >> >> > > > The only problem I see is that for each new feature we have to modify >> >> >> > > > the kernel. >> >> >> > > > Could we have new features that don't require handling by vhost-vdpa? >> >> >> > > > >> >> >> > > > Thanks, >> >> >> > > > Stefano >> >> >> > > >> >> >> > > Jason what do you say to reverting this? >> >> >> > >> >> >> > I may miss something but I don't see any problem with vDPA core. >> >> >> > >> >> >> > It's the duty of the parents to advertise the features it has. For example, >> >> >> > >> >> >> > 1) If some kernel version that is packed is not supported via >> >> >> > set_vq_state, parents should not advertise PACKED features in this >> >> >> > case. >> >> >> > 2) If the kernel has support packed set_vq_state(), but it's emulated >> >> >> > cvq doesn't support, parents should not advertise PACKED as well >> >> >> > >> >> >> > If a parent violates the above 2, it looks like a bug of the parents. >> >> >> > >> >> >> > Thanks >> >> >> >> >> >> Yes but what about vhost_vdpa? Talking about that not the core. >> >> > >> >> >Not sure it's a good idea to workaround parent bugs via vhost-vDPA. >> >> >> >> Sorry, I'm getting lost... >> >> We were talking about the fact that vhost-vdpa doesn't handle >> >> SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before >> >> that series [1], no? >> >> >> >> The parents seem okay, but maybe I missed a few things. >> >> >> >> [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ >> > >> >Yes, more below. >> > >> >> >> >> > >> >> >> Should that not have a whitelist of features >> >> >> since it interprets ioctls differently depending on this? >> >> > >> >> >If there's a bug, it might only matter the following setup: >> >> > >> >> >SET_VRING_BASE/GET_VRING_BASE + VDUSE. >> >> > >> >> >This seems to be broken since VDUSE was introduced. If we really want >> >> >to backport something, it could be a fix to filter out PACKED in >> >> >VDUSE? >> >> >> >> mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa. >> >> I think VDUSE works fine with packed virtqueue using virtio-vdpa >> >> (I haven't tried), so why should we filter PACKED in VDUSE? >> > >> >I don't think we need any filtering since: >> > >> >PACKED features has been advertised to userspace via uAPI since >> >6234f80574d7569444d8718355fa2838e92b158b. Once we relax in uAPI, it >> >would be very hard to restrict it again. For the userspace that tries >> >to negotiate PACKED: >> > >> >1) if it doesn't use SET_VRING_BASE/GET_VRING_BASE, everything works well >> >2) if it uses SET_VRING_BASE/GET_VRING_BASE. it might fail or break silently >> > >> >If we backport the fixes to -stable, we may break the application at >> >least in the case 1). >> >> Okay, I see now, thanks for the details! >> >> Maybe instead of "break silently", we can return an explicit error for >> SET_VRING_BASE/GET_VRING_BASE in stable branches. >> But if there are not many cases, we can leave it like that. > >A second thought, if we need to do something for stable. is it better >if we just backport Shannon's series to stable? I tried to look at it, but it looks like we have to backport quite a few patches, I wrote a few things here: https://lore.kernel.org/virtualization/32ejjuvhvcicv7wjuetkv34qtlpa657n4zlow4eq3fsi2twozk@iqnd2t5tw2an/ But if you think it's the best way, though, we can take a better look at how many patches are to backport and whether it's risky or not. > >> >> I was just concerned about how does the user space understand that it >> can use SET_VRING_BASE/GET_VRING_BASE for PACKED virtqueues in a given >> kernel or not. > >My understanding is that if packed is advertised, the application >should assume SET/GET_VRING_BASE work. > Same here. So as an alternative to backporting a large set of patches, I proposed to completely disable packed for stable branches where vhost-vdpa IOCTLs doesn't support them very well. Thanks, Stefano
On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote: > On Thu, Jun 8, 2023 at 2:03 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Thu, Jun 08, 2023 at 08:42:15AM +0800, Jason Wang wrote: > > > On Wed, Jun 7, 2023 at 5:43 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Wed, Jun 07, 2023 at 10:39:15AM +0200, Stefano Garzarella wrote: > > > > > On Tue, Jun 6, 2023 at 2:58 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > On Tue, Jun 06, 2023 at 09:29:22AM +0800, Jason Wang wrote: > > > > > > > On Mon, Jun 5, 2023 at 10:58 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > > > > > > > > > > > > On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrote: > > > > > > > > >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wrote: > > > > > > > > >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin wrote: > > > > > > > > >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella wrote: > > > > > > > > >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirkin wrote: > > > > > > > > >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: > > > > > > > > >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) > > > > > > > > >> > > > > don't support packed virtqueue well yet, so let's filter the > > > > > > > > >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). > > > > > > > > >> > > > > > > > > > > > > >> > > > > This way, even if the device supports it, we don't risk it being > > > > > > > > >> > > > > negotiated, then the VMM is unable to set the vring state properly. > > > > > > > > >> > > > > > > > > > > > > >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > > > > > > > >> > > > > Cc: stable@vger.kernel.org > > > > > > > > >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > > > > > > >> > > > > --- > > > > > > > > >> > > > > > > > > > > > > >> > > > > Notes: > > > > > > > > >> > > > > This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: > > > > > > > > >> > > > > better PACKED support" series [1] and backported in stable branches. > > > > > > > > >> > > > > > > > > > > > > >> > > > > We can revert it when we are sure that everything is working with > > > > > > > > >> > > > > packed virtqueues. > > > > > > > > >> > > > > > > > > > > > > >> > > > > Thanks, > > > > > > > > >> > > > > Stefano > > > > > > > > >> > > > > > > > > > > > > >> > > > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > > > > > > > > >> > > > > > > > > > > > >> > > > I'm a bit lost here. So why am I merging "better PACKED support" then? > > > > > > > > >> > > > > > > > > > > >> > > To really support packed virtqueue with vhost-vdpa, at that point we would > > > > > > > > >> > > also have to revert this patch. > > > > > > > > >> > > > > > > > > > > >> > > I wasn't sure if you wanted to queue the series for this merge window. > > > > > > > > >> > > In that case do you think it is better to send this patch only for stable > > > > > > > > >> > > branches? > > > > > > > > >> > > > Does this patch make them a NOP? > > > > > > > > >> > > > > > > > > > > >> > > Yep, after applying the "better PACKED support" series and being > > > > > > > > >> > > sure that > > > > > > > > >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we should revert this > > > > > > > > >> > > patch. > > > > > > > > >> > > > > > > > > > > >> > > Let me know if you prefer a different approach. > > > > > > > > >> > > > > > > > > > > >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking that the kernel > > > > > > > > >> > > interprets them the right way, when it does not. > > > > > > > > >> > > > > > > > > > > >> > > Thanks, > > > > > > > > >> > > Stefano > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > If this fixes a bug can you add Fixes tags to each of them? Then it's ok > > > > > > > > >> > to merge in this window. Probably easier than the elaborate > > > > > > > > >> > mask/unmask dance. > > > > > > > > >> > > > > > > > > >> CCing Shannon (the original author of the "better PACKED support" > > > > > > > > >> series). > > > > > > > > >> > > > > > > > > >> IIUC Shannon is going to send a v3 of that series to fix the > > > > > > > > >> documentation, so Shannon can you also add the Fixes tags? > > > > > > > > >> > > > > > > > > >> Thanks, > > > > > > > > >> Stefano > > > > > > > > > > > > > > > > > >Well this is in my tree already. Just reply with > > > > > > > > >Fixes: <> > > > > > > > > >to each and I will add these tags. > > > > > > > > > > > > > > > > I tried, but it is not easy since we added the support for packed > > > > > > > > virtqueue in vdpa and vhost incrementally. > > > > > > > > > > > > > > > > Initially I was thinking of adding the same tag used here: > > > > > > > > > > > > > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > > > > > > > > > > > > > > > > Then I discovered that vq_state wasn't there, so I was thinking of > > > > > > > > > > > > > > > > Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_vq_state()") > > > > > > > > > > > > > > > > So we would have to backport quite a few patches into the stable branches. > > > > > > > > I don't know if it's worth it... > > > > > > > > > > > > > > > > I still think it is better to disable packed in the stable branches, > > > > > > > > otherwise I have to make a list of all the patches we need. > > > > > > > > > > > > > > > > Any other ideas? > > > > > > > > > > > > > > AFAIK, except for vp_vdpa, pds seems to be the first parent that > > > > > > > supports packed virtqueue. Users should not notice anything wrong if > > > > > > > they don't use packed virtqueue. And the problem of vp_vdpa + packed > > > > > > > virtqueue came since the day0 of vp_vdpa. It seems fine to do nothing > > > > > > > I guess. > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > I have a question though, what if down the road there > > > > > > is a new feature that needs more changes? It will be > > > > > > broken too just like PACKED no? > > > > > > Shouldn't vdpa have an allowlist of features it knows how > > > > > > to support? > > > > > > > > > > It looks like we had it, but we took it out (by the way, we were > > > > > enabling packed even though we didn't support it): > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b > > > > > > > > > > The only problem I see is that for each new feature we have to modify > > > > > the kernel. > > > > > Could we have new features that don't require handling by vhost-vdpa? > > > > > > > > > > Thanks, > > > > > Stefano > > > > > > > > Jason what do you say to reverting this? > > > > > > I may miss something but I don't see any problem with vDPA core. > > > > > > It's the duty of the parents to advertise the features it has. For example, > > > > > > 1) If some kernel version that is packed is not supported via > > > set_vq_state, parents should not advertise PACKED features in this > > > case. > > > 2) If the kernel has support packed set_vq_state(), but it's emulated > > > cvq doesn't support, parents should not advertise PACKED as well > > > > > > If a parent violates the above 2, it looks like a bug of the parents. > > > > > > Thanks > > > > Yes but what about vhost_vdpa? Talking about that not the core. > > Not sure it's a good idea to workaround parent bugs via vhost-vDPA. these are not parent bugs. vhost-vdpa did not pass ioctl data correctly to parent, right? > > Should that not have a whitelist of features > > since it interprets ioctls differently depending on this? > > If there's a bug, it might only matter the following setup: > > SET_VRING_BASE/GET_VRING_BASE + VDUSE. Why does it not apply to any parent? > This seems to be broken since VDUSE was introduced. If we really want > to backport something, it could be a fix to filter out PACKED in > VDUSE? > > Thanks what prevents VDUSE from supporting packed? > > > > > > > > > > -- > > > > MST > > > > > >
On Thu, Jun 08, 2023 at 05:00:00PM +0800, Jason Wang wrote: > On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote: > > > > [...] > > > > >> > > > > I have a question though, what if down the road there > > >> > > > > is a new feature that needs more changes? It will be > > >> > > > > broken too just like PACKED no? > > >> > > > > Shouldn't vdpa have an allowlist of features it knows how > > >> > > > > to support? > > >> > > > > > >> > > > It looks like we had it, but we took it out (by the way, we were > > >> > > > enabling packed even though we didn't support it): > > >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b > > >> > > > > > >> > > > The only problem I see is that for each new feature we have to modify > > >> > > > the kernel. > > >> > > > Could we have new features that don't require handling by vhost-vdpa? > > >> > > > > > >> > > > Thanks, > > >> > > > Stefano > > >> > > > > >> > > Jason what do you say to reverting this? > > >> > > > >> > I may miss something but I don't see any problem with vDPA core. > > >> > > > >> > It's the duty of the parents to advertise the features it has. For example, > > >> > > > >> > 1) If some kernel version that is packed is not supported via > > >> > set_vq_state, parents should not advertise PACKED features in this > > >> > case. > > >> > 2) If the kernel has support packed set_vq_state(), but it's emulated > > >> > cvq doesn't support, parents should not advertise PACKED as well > > >> > > > >> > If a parent violates the above 2, it looks like a bug of the parents. > > >> > > > >> > Thanks > > >> > > >> Yes but what about vhost_vdpa? Talking about that not the core. > > > > > >Not sure it's a good idea to workaround parent bugs via vhost-vDPA. > > > > Sorry, I'm getting lost... > > We were talking about the fact that vhost-vdpa doesn't handle > > SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before > > that series [1], no? > > > > The parents seem okay, but maybe I missed a few things. > > > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > > Yes, more below. > > > > > > > > >> Should that not have a whitelist of features > > >> since it interprets ioctls differently depending on this? > > > > > >If there's a bug, it might only matter the following setup: > > > > > >SET_VRING_BASE/GET_VRING_BASE + VDUSE. > > > > > >This seems to be broken since VDUSE was introduced. If we really want > > >to backport something, it could be a fix to filter out PACKED in > > >VDUSE? > > > > mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa. > > I think VDUSE works fine with packed virtqueue using virtio-vdpa > > (I haven't tried), so why should we filter PACKED in VDUSE? > > I don't think we need any filtering since: > > PACKED features has been advertised to userspace via uAPI since > 6234f80574d7569444d8718355fa2838e92b158b. Once we relax in uAPI, it > would be very hard to restrict it again. For the userspace that tries > to negotiate PACKED: > > 1) if it doesn't use SET_VRING_BASE/GET_VRING_BASE, everything works well > 2) if it uses SET_VRING_BASE/GET_VRING_BASE. it might fail or break silently > > If we backport the fixes to -stable, we may break the application at > least in the case 1). > > Thanks I am less concerned about stable, and much more concerned about the future. Assume we add a new ring format. It will be silently passed to vhost-vdpa and things break again. This is why I think we need an allowlist in vhost-vdpa. > > > > Thanks, > > Stefano > >
On Thu, Jun 08, 2023 at 05:29:58PM +0800, Jason Wang wrote: > On Thu, Jun 8, 2023 at 5:21 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > On Thu, Jun 08, 2023 at 05:00:00PM +0800, Jason Wang wrote: > > >On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > >> > > >> On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote: > > >> > > >> [...] > > >> > > >> >> > > > > I have a question though, what if down the road there > > >> >> > > > > is a new feature that needs more changes? It will be > > >> >> > > > > broken too just like PACKED no? > > >> >> > > > > Shouldn't vdpa have an allowlist of features it knows how > > >> >> > > > > to support? > > >> >> > > > > > >> >> > > > It looks like we had it, but we took it out (by the way, we were > > >> >> > > > enabling packed even though we didn't support it): > > >> >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b > > >> >> > > > > > >> >> > > > The only problem I see is that for each new feature we have to modify > > >> >> > > > the kernel. > > >> >> > > > Could we have new features that don't require handling by vhost-vdpa? > > >> >> > > > > > >> >> > > > Thanks, > > >> >> > > > Stefano > > >> >> > > > > >> >> > > Jason what do you say to reverting this? > > >> >> > > > >> >> > I may miss something but I don't see any problem with vDPA core. > > >> >> > > > >> >> > It's the duty of the parents to advertise the features it has. For example, > > >> >> > > > >> >> > 1) If some kernel version that is packed is not supported via > > >> >> > set_vq_state, parents should not advertise PACKED features in this > > >> >> > case. > > >> >> > 2) If the kernel has support packed set_vq_state(), but it's emulated > > >> >> > cvq doesn't support, parents should not advertise PACKED as well > > >> >> > > > >> >> > If a parent violates the above 2, it looks like a bug of the parents. > > >> >> > > > >> >> > Thanks > > >> >> > > >> >> Yes but what about vhost_vdpa? Talking about that not the core. > > >> > > > >> >Not sure it's a good idea to workaround parent bugs via vhost-vDPA. > > >> > > >> Sorry, I'm getting lost... > > >> We were talking about the fact that vhost-vdpa doesn't handle > > >> SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before > > >> that series [1], no? > > >> > > >> The parents seem okay, but maybe I missed a few things. > > >> > > >> [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > > > > > >Yes, more below. > > > > > >> > > >> > > > >> >> Should that not have a whitelist of features > > >> >> since it interprets ioctls differently depending on this? > > >> > > > >> >If there's a bug, it might only matter the following setup: > > >> > > > >> >SET_VRING_BASE/GET_VRING_BASE + VDUSE. > > >> > > > >> >This seems to be broken since VDUSE was introduced. If we really want > > >> >to backport something, it could be a fix to filter out PACKED in > > >> >VDUSE? > > >> > > >> mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa. > > >> I think VDUSE works fine with packed virtqueue using virtio-vdpa > > >> (I haven't tried), so why should we filter PACKED in VDUSE? > > > > > >I don't think we need any filtering since: > > > > > >PACKED features has been advertised to userspace via uAPI since > > >6234f80574d7569444d8718355fa2838e92b158b. Once we relax in uAPI, it > > >would be very hard to restrict it again. For the userspace that tries > > >to negotiate PACKED: > > > > > >1) if it doesn't use SET_VRING_BASE/GET_VRING_BASE, everything works well > > >2) if it uses SET_VRING_BASE/GET_VRING_BASE. it might fail or break silently > > > > > >If we backport the fixes to -stable, we may break the application at > > >least in the case 1). > > > > Okay, I see now, thanks for the details! > > > > Maybe instead of "break silently", we can return an explicit error for > > SET_VRING_BASE/GET_VRING_BASE in stable branches. > > But if there are not many cases, we can leave it like that. > > A second thought, if we need to do something for stable. is it better > if we just backport Shannon's series to stable? > > > > > I was just concerned about how does the user space understand that it > > can use SET_VRING_BASE/GET_VRING_BASE for PACKED virtqueues in a given > > kernel or not. > > My understanding is that if packed is advertised, the application > should assume SET/GET_VRING_BASE work. > > Thanks Let me ask you this. This is a bugfix yes? What is the appropriate Fixes tag? > > > > Thanks, > > Stefano > >
On Thu, Jun 8, 2023 at 10:23 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Jun 08, 2023 at 05:29:58PM +0800, Jason Wang wrote: > > On Thu, Jun 8, 2023 at 5:21 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > > On Thu, Jun 08, 2023 at 05:00:00PM +0800, Jason Wang wrote: > > > >On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > >> > > > >> On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote: > > > >> > > > >> [...] > > > >> > > > >> >> > > > > I have a question though, what if down the road there > > > >> >> > > > > is a new feature that needs more changes? It will be > > > >> >> > > > > broken too just like PACKED no? > > > >> >> > > > > Shouldn't vdpa have an allowlist of features it knows how > > > >> >> > > > > to support? > > > >> >> > > > > > > >> >> > > > It looks like we had it, but we took it out (by the way, we were > > > >> >> > > > enabling packed even though we didn't support it): > > > >> >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b > > > >> >> > > > > > > >> >> > > > The only problem I see is that for each new feature we have to modify > > > >> >> > > > the kernel. > > > >> >> > > > Could we have new features that don't require handling by vhost-vdpa? > > > >> >> > > > > > > >> >> > > > Thanks, > > > >> >> > > > Stefano > > > >> >> > > > > > >> >> > > Jason what do you say to reverting this? > > > >> >> > > > > >> >> > I may miss something but I don't see any problem with vDPA core. > > > >> >> > > > > >> >> > It's the duty of the parents to advertise the features it has. For example, > > > >> >> > > > > >> >> > 1) If some kernel version that is packed is not supported via > > > >> >> > set_vq_state, parents should not advertise PACKED features in this > > > >> >> > case. > > > >> >> > 2) If the kernel has support packed set_vq_state(), but it's emulated > > > >> >> > cvq doesn't support, parents should not advertise PACKED as well > > > >> >> > > > > >> >> > If a parent violates the above 2, it looks like a bug of the parents. > > > >> >> > > > > >> >> > Thanks > > > >> >> > > > >> >> Yes but what about vhost_vdpa? Talking about that not the core. > > > >> > > > > >> >Not sure it's a good idea to workaround parent bugs via vhost-vDPA. > > > >> > > > >> Sorry, I'm getting lost... > > > >> We were talking about the fact that vhost-vdpa doesn't handle > > > >> SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before > > > >> that series [1], no? > > > >> > > > >> The parents seem okay, but maybe I missed a few things. > > > >> > > > >> [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > > > > > > > >Yes, more below. > > > > > > > >> > > > >> > > > > >> >> Should that not have a whitelist of features > > > >> >> since it interprets ioctls differently depending on this? > > > >> > > > > >> >If there's a bug, it might only matter the following setup: > > > >> > > > > >> >SET_VRING_BASE/GET_VRING_BASE + VDUSE. > > > >> > > > > >> >This seems to be broken since VDUSE was introduced. If we really want > > > >> >to backport something, it could be a fix to filter out PACKED in > > > >> >VDUSE? > > > >> > > > >> mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa. > > > >> I think VDUSE works fine with packed virtqueue using virtio-vdpa > > > >> (I haven't tried), so why should we filter PACKED in VDUSE? > > > > > > > >I don't think we need any filtering since: > > > > > > > >PACKED features has been advertised to userspace via uAPI since > > > >6234f80574d7569444d8718355fa2838e92b158b. Once we relax in uAPI, it > > > >would be very hard to restrict it again. For the userspace that tries > > > >to negotiate PACKED: > > > > > > > >1) if it doesn't use SET_VRING_BASE/GET_VRING_BASE, everything works well > > > >2) if it uses SET_VRING_BASE/GET_VRING_BASE. it might fail or break silently > > > > > > > >If we backport the fixes to -stable, we may break the application at > > > >least in the case 1). > > > > > > Okay, I see now, thanks for the details! > > > > > > Maybe instead of "break silently", we can return an explicit error for > > > SET_VRING_BASE/GET_VRING_BASE in stable branches. > > > But if there are not many cases, we can leave it like that. > > > > A second thought, if we need to do something for stable. is it better > > if we just backport Shannon's series to stable? > > > > > > > > I was just concerned about how does the user space understand that it > > > can use SET_VRING_BASE/GET_VRING_BASE for PACKED virtqueues in a given > > > kernel or not. > > > > My understanding is that if packed is advertised, the application > > should assume SET/GET_VRING_BASE work. > > > > Thanks > > > Let me ask you this. This is a bugfix yes? Not sure since it may break existing user space applications which make it hard to be backported to -stable. Before the fix, PACKED might work if SET/GET_VRING_BASE is not used. After the fix, PACKED won't work at all. Thanks What is the appropriate Fixes > tag? > > > > > > > Thanks, > > > Stefano > > > >
On Fri, Jun 09, 2023 at 10:16:50AM +0800, Jason Wang wrote: > On Thu, Jun 8, 2023 at 10:23 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Thu, Jun 08, 2023 at 05:29:58PM +0800, Jason Wang wrote: > > > On Thu, Jun 8, 2023 at 5:21 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > > > > On Thu, Jun 08, 2023 at 05:00:00PM +0800, Jason Wang wrote: > > > > >On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > >> > > > > >> On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote: > > > > >> > > > > >> [...] > > > > >> > > > > >> >> > > > > I have a question though, what if down the road there > > > > >> >> > > > > is a new feature that needs more changes? It will be > > > > >> >> > > > > broken too just like PACKED no? > > > > >> >> > > > > Shouldn't vdpa have an allowlist of features it knows how > > > > >> >> > > > > to support? > > > > >> >> > > > > > > > >> >> > > > It looks like we had it, but we took it out (by the way, we were > > > > >> >> > > > enabling packed even though we didn't support it): > > > > >> >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b > > > > >> >> > > > > > > > >> >> > > > The only problem I see is that for each new feature we have to modify > > > > >> >> > > > the kernel. > > > > >> >> > > > Could we have new features that don't require handling by vhost-vdpa? > > > > >> >> > > > > > > > >> >> > > > Thanks, > > > > >> >> > > > Stefano > > > > >> >> > > > > > > >> >> > > Jason what do you say to reverting this? > > > > >> >> > > > > > >> >> > I may miss something but I don't see any problem with vDPA core. > > > > >> >> > > > > > >> >> > It's the duty of the parents to advertise the features it has. For example, > > > > >> >> > > > > > >> >> > 1) If some kernel version that is packed is not supported via > > > > >> >> > set_vq_state, parents should not advertise PACKED features in this > > > > >> >> > case. > > > > >> >> > 2) If the kernel has support packed set_vq_state(), but it's emulated > > > > >> >> > cvq doesn't support, parents should not advertise PACKED as well > > > > >> >> > > > > > >> >> > If a parent violates the above 2, it looks like a bug of the parents. > > > > >> >> > > > > > >> >> > Thanks > > > > >> >> > > > > >> >> Yes but what about vhost_vdpa? Talking about that not the core. > > > > >> > > > > > >> >Not sure it's a good idea to workaround parent bugs via vhost-vDPA. > > > > >> > > > > >> Sorry, I'm getting lost... > > > > >> We were talking about the fact that vhost-vdpa doesn't handle > > > > >> SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before > > > > >> that series [1], no? > > > > >> > > > > >> The parents seem okay, but maybe I missed a few things. > > > > >> > > > > >> [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > > > > > > > > > >Yes, more below. > > > > > > > > > >> > > > > >> > > > > > >> >> Should that not have a whitelist of features > > > > >> >> since it interprets ioctls differently depending on this? > > > > >> > > > > > >> >If there's a bug, it might only matter the following setup: > > > > >> > > > > > >> >SET_VRING_BASE/GET_VRING_BASE + VDUSE. > > > > >> > > > > > >> >This seems to be broken since VDUSE was introduced. If we really want > > > > >> >to backport something, it could be a fix to filter out PACKED in > > > > >> >VDUSE? > > > > >> > > > > >> mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa. > > > > >> I think VDUSE works fine with packed virtqueue using virtio-vdpa > > > > >> (I haven't tried), so why should we filter PACKED in VDUSE? > > > > > > > > > >I don't think we need any filtering since: > > > > > > > > > >PACKED features has been advertised to userspace via uAPI since > > > > >6234f80574d7569444d8718355fa2838e92b158b. Once we relax in uAPI, it > > > > >would be very hard to restrict it again. For the userspace that tries > > > > >to negotiate PACKED: > > > > > > > > > >1) if it doesn't use SET_VRING_BASE/GET_VRING_BASE, everything works well > > > > >2) if it uses SET_VRING_BASE/GET_VRING_BASE. it might fail or break silently > > > > > > > > > >If we backport the fixes to -stable, we may break the application at > > > > >least in the case 1). > > > > > > > > Okay, I see now, thanks for the details! > > > > > > > > Maybe instead of "break silently", we can return an explicit error for > > > > SET_VRING_BASE/GET_VRING_BASE in stable branches. > > > > But if there are not many cases, we can leave it like that. > > > > > > A second thought, if we need to do something for stable. is it better > > > if we just backport Shannon's series to stable? > > > > > > > > > > > I was just concerned about how does the user space understand that it > > > > can use SET_VRING_BASE/GET_VRING_BASE for PACKED virtqueues in a given > > > > kernel or not. > > > > > > My understanding is that if packed is advertised, the application > > > should assume SET/GET_VRING_BASE work. > > > > > > Thanks > > > > > > Let me ask you this. This is a bugfix yes? > > Not sure since it may break existing user space applications which > make it hard to be backported to -stable. Sorry, I was actually referring to vhost_vdpa: support PACKED when setting-getting vring_base and friends. These are bugfixes yes? What is the appropriate Fixes tag? > Before the fix, PACKED might work if SET/GET_VRING_BASE is not used. > After the fix, PACKED won't work at all. > > Thanks > > What is the appropriate Fixes > > tag? > > > > > > > > > > Thanks, > > > > Stefano > > > > > >
On Thu, Jun 08, 2023 at 10:23:21AM -0400, Michael S. Tsirkin wrote: >On Thu, Jun 08, 2023 at 05:29:58PM +0800, Jason Wang wrote: >> On Thu, Jun 8, 2023 at 5:21 PM Stefano Garzarella <sgarzare@redhat.com> wrote: >> > >> > On Thu, Jun 08, 2023 at 05:00:00PM +0800, Jason Wang wrote: >> > >On Thu, Jun 8, 2023 at 4:00 PM Stefano Garzarella <sgarzare@redhat.com> wrote: >> > >> >> > >> On Thu, Jun 08, 2023 at 03:46:00PM +0800, Jason Wang wrote: >> > >> >> > >> [...] >> > >> >> > >> >> > > > > I have a question though, what if down the road there >> > >> >> > > > > is a new feature that needs more changes? It will be >> > >> >> > > > > broken too just like PACKED no? >> > >> >> > > > > Shouldn't vdpa have an allowlist of features it knows how >> > >> >> > > > > to support? >> > >> >> > > > >> > >> >> > > > It looks like we had it, but we took it out (by the way, we were >> > >> >> > > > enabling packed even though we didn't support it): >> > >> >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6234f80574d7569444d8718355fa2838e92b158b >> > >> >> > > > >> > >> >> > > > The only problem I see is that for each new feature we have to modify >> > >> >> > > > the kernel. >> > >> >> > > > Could we have new features that don't require handling by vhost-vdpa? >> > >> >> > > > >> > >> >> > > > Thanks, >> > >> >> > > > Stefano >> > >> >> > > >> > >> >> > > Jason what do you say to reverting this? >> > >> >> > >> > >> >> > I may miss something but I don't see any problem with vDPA core. >> > >> >> > >> > >> >> > It's the duty of the parents to advertise the features it has. For example, >> > >> >> > >> > >> >> > 1) If some kernel version that is packed is not supported via >> > >> >> > set_vq_state, parents should not advertise PACKED features in this >> > >> >> > case. >> > >> >> > 2) If the kernel has support packed set_vq_state(), but it's emulated >> > >> >> > cvq doesn't support, parents should not advertise PACKED as well >> > >> >> > >> > >> >> > If a parent violates the above 2, it looks like a bug of the parents. >> > >> >> > >> > >> >> > Thanks >> > >> >> >> > >> >> Yes but what about vhost_vdpa? Talking about that not the core. >> > >> > >> > >> >Not sure it's a good idea to workaround parent bugs via vhost-vDPA. >> > >> >> > >> Sorry, I'm getting lost... >> > >> We were talking about the fact that vhost-vdpa doesn't handle >> > >> SET_VRING_BASE/GET_VRING_BASE ioctls well for packed virtqueue before >> > >> that series [1], no? >> > >> >> > >> The parents seem okay, but maybe I missed a few things. >> > >> >> > >> [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ >> > > >> > >Yes, more below. >> > > >> > >> >> > >> > >> > >> >> Should that not have a whitelist of features >> > >> >> since it interprets ioctls differently depending on this? >> > >> > >> > >> >If there's a bug, it might only matter the following setup: >> > >> > >> > >> >SET_VRING_BASE/GET_VRING_BASE + VDUSE. >> > >> > >> > >> >This seems to be broken since VDUSE was introduced. If we really want >> > >> >to backport something, it could be a fix to filter out PACKED in >> > >> >VDUSE? >> > >> >> > >> mmm it doesn't seem to be a problem in VDUSE, but in vhost-vdpa. >> > >> I think VDUSE works fine with packed virtqueue using virtio-vdpa >> > >> (I haven't tried), so why should we filter PACKED in VDUSE? >> > > >> > >I don't think we need any filtering since: >> > > >> > >PACKED features has been advertised to userspace via uAPI since >> > >6234f80574d7569444d8718355fa2838e92b158b. Once we relax in uAPI, it >> > >would be very hard to restrict it again. For the userspace that tries >> > >to negotiate PACKED: >> > > >> > >1) if it doesn't use SET_VRING_BASE/GET_VRING_BASE, everything works well >> > >2) if it uses SET_VRING_BASE/GET_VRING_BASE. it might fail or break silently >> > > >> > >If we backport the fixes to -stable, we may break the application at >> > >least in the case 1). >> > >> > Okay, I see now, thanks for the details! >> > >> > Maybe instead of "break silently", we can return an explicit error for >> > SET_VRING_BASE/GET_VRING_BASE in stable branches. >> > But if there are not many cases, we can leave it like that. >> >> A second thought, if we need to do something for stable. is it better >> if we just backport Shannon's series to stable? >> >> > >> > I was just concerned about how does the user space understand that it >> > can use SET_VRING_BASE/GET_VRING_BASE for PACKED virtqueues in a given >> > kernel or not. >> >> My understanding is that if packed is advertised, the application >> should assume SET/GET_VRING_BASE work. >> >> Thanks > > >Let me ask you this. This is a bugfix yes? What is the appropriate Fixes >tag? I would say: Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") because we had an allow list and enabled PACKED explicitly. I'm not sure if the parents at that time supported PACKED, but maybe it doesn't matter since we are talking about the code in vhost-vdpa. Thanks, Stefano
On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) > don't support packed virtqueue well yet, so let's filter the > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). > > This way, even if the device supports it, we don't risk it being > negotiated, then the VMM is unable to set the vring state properly. > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") > Cc: stable@vger.kernel.org > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> OK so for now I dropped this, we have a better fix upstream. > --- > > Notes: > This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: > better PACKED support" series [1] and backported in stable branches. > > We can revert it when we are sure that everything is working with > packed virtqueues. > > Thanks, > Stefano > > [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ > > drivers/vhost/vdpa.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c > index 8c1aefc865f0..ac2152135b23 100644 > --- a/drivers/vhost/vdpa.c > +++ b/drivers/vhost/vdpa.c > @@ -397,6 +397,12 @@ static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep) > > features = ops->get_device_features(vdpa); > > + /* > + * IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) don't support > + * packed virtqueue well yet, so let's filter the feature for now. > + */ > + features &= ~BIT_ULL(VIRTIO_F_RING_PACKED); > + > if (copy_to_user(featurep, &features, sizeof(features))) > return -EFAULT; > > > base-commit: 9561de3a55bed6bdd44a12820ba81ec416e705a7 > -- > 2.40.1
On Thu, Jun 22, 2023 at 07:37:08AM -0400, Michael S. Tsirkin wrote: >On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzarella wrote: >> vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) >> don't support packed virtqueue well yet, so let's filter the >> VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). >> >> This way, even if the device supports it, we don't risk it being >> negotiated, then the VMM is unable to set the vring state properly. >> >> Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") >> Cc: stable@vger.kernel.org >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > >OK so for now I dropped this, we have a better fix upstream. > Yep, I agree. Maybe we can reuse this patch in the stable branches where the backport is not easy. Although as Jason said, maybe we don't need it. Thanks, Stefano
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;
vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET_VRING_BASE) don't support packed virtqueue well yet, so let's filter the VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get_features(). This way, even if the device supports it, we don't risk it being negotiated, then the VMM is unable to set the vring state properly. Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend") Cc: stable@vger.kernel.org Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- Notes: This patch should be applied before the "[PATCH v2 0/3] vhost_vdpa: better PACKED support" series [1] and backported in stable branches. We can revert it when we are sure that everything is working with packed virtqueues. Thanks, Stefano [1] https://lore.kernel.org/virtualization/20230424225031.18947-1-shannon.nelson@amd.com/ drivers/vhost/vdpa.c | 6 ++++++ 1 file changed, 6 insertions(+) base-commit: 9561de3a55bed6bdd44a12820ba81ec416e705a7