Michael S. Tsirkin
2023-Jun-08 14:23 UTC
[PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature
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 at 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 at 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 at 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. > > ThanksLet 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 at 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 at 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 at 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 at 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 > > > >
Stefano Garzarella
2023-Jun-09 07:37 UTC
[PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature
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 at 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 at 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 at 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