From: Will Deacon <will@kernel.org> To: Stefano Garzarella <sgarzare@redhat.com> Cc: mst@redhat.com, stefanha@redhat.com, jasowang@redhat.com, torvalds@linux-foundation.org, ascull@google.com, maz@kernel.org, keirf@google.com, jiyong@google.com, kernel-team@android.com, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, kvm@vger.kernel.org Subject: Re: IOTLB support for vhost/vsock breaks crosvm on Android Date: Sat, 6 Aug 2022 10:42:40 +0100 [thread overview] Message-ID: <20220806094239.GA30268@willie-the-truck> (raw) In-Reply-To: <20220806074828.zwzgn5gj47gjx5og@sgarzare-redhat> Hi Stefano, On Sat, Aug 06, 2022 at 09:48:28AM +0200, Stefano Garzarella wrote: > On Fri, Aug 05, 2022 at 07:11:06PM +0100, Will Deacon wrote: > > The fundamental issue is, I think, that VIRTIO_F_ACCESS_PLATFORM is > > being used for two very different things within the same device; for the > > guest it basically means "use the DMA API, it knows what to do" but for > > vhost it very specifically means "enable IOTLB". We've recently had > > other problems with this flag [3] but in this case it used to work > > reliably and now it doesn't anymore. > > > > So how should we fix this? One possibility is for us to hack crosvm to > > clear the VIRTIO_F_ACCESS_PLATFORM flag when setting the vhost > > Why do you consider this a hack? I think it's a hack for two reasons: (1) We're changing userspace to avoid a breaking change in kernel behaviour (2) I think that crosvm's approach is actually pretty reasonable To elaborate on (2), crosvm has a set of device features that it has negotiated with the guest. It then takes the intersection of these features with those advertised by VHOST_GET_FEATURES and calls VHOST_SET_FEATURES with the result. If there was a common interpretation of what these features do, then this would work and would mean we wouldn't have to opt-in on a per-flag basis for vhost. Since VIRTIO_F_ACCESS_PLATFORM is being overloaded to mean two completely different things, then it breaks and I think masking out that specific flag is a hack because it's basically crosvm saying "yeah, I may have negotiated this with the driver but vhost _actually_ means 'IOTLB' when it says it supports this flag so I'll mask it out because I know better". > If the VMM implements the translation feature, it is right in my opinion > that it does not enable the feature for the vhost device. Otherwise, if it > wants the vhost device to do the translation, enable the feature and send > the IOTLB messages to set the translation. > > QEMU for example masks features when not required or supported. > crosvm should negotiate only the features it supports. > > @Michael and @Jason can correct me, but if a vhost device negotiates > VIRTIO_F_ACCESS_PLATFORM, then it expects the VMM to send IOTLB messages to > set the translation. As above, the issue is that vhost now unconditionally advertises this in VHOST_GET_FEATURES and so a VMM with no knowledge of IOTLB can end up enabling it by accident. Will
WARNING: multiple messages have this Message-ID (diff)
From: Will Deacon <will@kernel.org> To: Stefano Garzarella <sgarzare@redhat.com> Cc: jiyong@google.com, kvm@vger.kernel.org, mst@redhat.com, maz@kernel.org, keirf@google.com, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, ascull@google.com, stefanha@redhat.com, kernel-team@android.com, torvalds@linux-foundation.org Subject: Re: IOTLB support for vhost/vsock breaks crosvm on Android Date: Sat, 6 Aug 2022 10:42:40 +0100 [thread overview] Message-ID: <20220806094239.GA30268@willie-the-truck> (raw) In-Reply-To: <20220806074828.zwzgn5gj47gjx5og@sgarzare-redhat> Hi Stefano, On Sat, Aug 06, 2022 at 09:48:28AM +0200, Stefano Garzarella wrote: > On Fri, Aug 05, 2022 at 07:11:06PM +0100, Will Deacon wrote: > > The fundamental issue is, I think, that VIRTIO_F_ACCESS_PLATFORM is > > being used for two very different things within the same device; for the > > guest it basically means "use the DMA API, it knows what to do" but for > > vhost it very specifically means "enable IOTLB". We've recently had > > other problems with this flag [3] but in this case it used to work > > reliably and now it doesn't anymore. > > > > So how should we fix this? One possibility is for us to hack crosvm to > > clear the VIRTIO_F_ACCESS_PLATFORM flag when setting the vhost > > Why do you consider this a hack? I think it's a hack for two reasons: (1) We're changing userspace to avoid a breaking change in kernel behaviour (2) I think that crosvm's approach is actually pretty reasonable To elaborate on (2), crosvm has a set of device features that it has negotiated with the guest. It then takes the intersection of these features with those advertised by VHOST_GET_FEATURES and calls VHOST_SET_FEATURES with the result. If there was a common interpretation of what these features do, then this would work and would mean we wouldn't have to opt-in on a per-flag basis for vhost. Since VIRTIO_F_ACCESS_PLATFORM is being overloaded to mean two completely different things, then it breaks and I think masking out that specific flag is a hack because it's basically crosvm saying "yeah, I may have negotiated this with the driver but vhost _actually_ means 'IOTLB' when it says it supports this flag so I'll mask it out because I know better". > If the VMM implements the translation feature, it is right in my opinion > that it does not enable the feature for the vhost device. Otherwise, if it > wants the vhost device to do the translation, enable the feature and send > the IOTLB messages to set the translation. > > QEMU for example masks features when not required or supported. > crosvm should negotiate only the features it supports. > > @Michael and @Jason can correct me, but if a vhost device negotiates > VIRTIO_F_ACCESS_PLATFORM, then it expects the VMM to send IOTLB messages to > set the translation. As above, the issue is that vhost now unconditionally advertises this in VHOST_GET_FEATURES and so a VMM with no knowledge of IOTLB can end up enabling it by accident. Will _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2022-08-06 9:42 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-08-05 18:11 IOTLB support for vhost/vsock breaks crosvm on Android Will Deacon 2022-08-05 18:11 ` Will Deacon 2022-08-05 22:57 ` Linus Torvalds 2022-08-05 22:57 ` Linus Torvalds 2022-08-06 8:17 ` Stefano Garzarella 2022-08-06 8:17 ` Stefano Garzarella 2022-08-06 9:42 ` Will Deacon 2022-08-06 9:42 ` Will Deacon 2022-08-07 6:52 ` Christoph Hellwig 2022-08-07 6:52 ` Christoph Hellwig 2022-08-07 13:35 ` Michael S. Tsirkin 2022-08-07 13:35 ` Michael S. Tsirkin 2022-08-07 13:27 ` Michael S. Tsirkin 2022-08-07 13:27 ` Michael S. Tsirkin 2022-08-06 7:48 ` Stefano Garzarella 2022-08-06 7:48 ` Stefano Garzarella 2022-08-06 9:42 ` Will Deacon [this message] 2022-08-06 9:42 ` Will Deacon 2022-08-06 10:52 ` Stefan Hajnoczi 2022-08-06 10:52 ` Stefan Hajnoczi 2022-08-06 14:34 ` Will Deacon 2022-08-06 14:34 ` Will Deacon 2022-08-06 21:47 ` Stefan Hajnoczi 2022-08-06 10:52 ` Stefano Garzarella 2022-08-06 10:52 ` Stefano Garzarella 2022-08-07 13:14 ` Michael S. Tsirkin 2022-08-07 13:14 ` Michael S. Tsirkin 2022-08-08 10:18 ` Will Deacon 2022-08-08 10:18 ` Will Deacon 2022-08-08 12:45 ` Michael S. Tsirkin 2022-08-08 12:45 ` Michael S. Tsirkin 2022-08-08 14:33 ` Stefan Hajnoczi 2022-08-09 3:16 ` Jason Wang 2022-08-09 3:16 ` Jason Wang 2022-08-17 13:48 ` Will Deacon 2022-08-17 13:48 ` Will Deacon 2022-08-17 17:05 ` Michael S. Tsirkin 2022-08-17 17:05 ` Michael S. Tsirkin 2022-08-09 3:12 ` Jason Wang 2022-08-09 3:12 ` Jason Wang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220806094239.GA30268@willie-the-truck \ --to=will@kernel.org \ --cc=ascull@google.com \ --cc=jasowang@redhat.com \ --cc=jiyong@google.com \ --cc=keirf@google.com \ --cc=kernel-team@android.com \ --cc=kvm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=maz@kernel.org \ --cc=mst@redhat.com \ --cc=sgarzare@redhat.com \ --cc=stefanha@redhat.com \ --cc=torvalds@linux-foundation.org \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.