From: Linus Torvalds <torvalds@linux-foundation.org> To: Will Deacon <will@kernel.org> Cc: mst@redhat.com, stefanha@redhat.com, jasowang@redhat.com, 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, Stefano Garzarella <sgarzare@redhat.com> Subject: Re: IOTLB support for vhost/vsock breaks crosvm on Android Date: Fri, 5 Aug 2022 15:57:08 -0700 [thread overview] Message-ID: <CAHk-=wip-Lju3ZdNwknS6ouyw+nKXeRSnhqVyNo8WSEdk-BfGw@mail.gmail.com> (raw) In-Reply-To: <20220805181105.GA29848@willie-the-truck> On Fri, Aug 5, 2022 at 11:11 AM Will Deacon <will@kernel.org> wrote: > > [tl;dr a change from ~18 months ago breaks Android userspace and I don't > know what to do about it] Augh. I had hoped that android being "closer" to upstream would have meant that somebody actually tests android with upstream kernels. People occasionally talk about it, but apparently it's not actually done. Or maybe it's done onl;y with a very limited android user space. The whole "we notice that something that happened 18 months ago broke our environment" is kind of broken. > After some digging, we narrowed this change in behaviour down to > e13a6915a03f ("vhost/vsock: add IOTLB API support") and further digging > reveals that the infamous VIRTIO_F_ACCESS_PLATFORM feature flag is to > blame. Indeed, our tests once again pass if we revert that patch (there's > a trivial conflict with the later addition of VIRTIO_VSOCK_F_SEQPACKET > but otherwise it reverts cleanly). I have to say, this smells for *so* many reasons. Why is "IOMMU support" called "VIRTIO_F_ACCESS_PLATFORM"? That seems insane, but seems fundamental in that commit e13a6915a03f ("vhost/vsock: add IOTLB API support") This code if ((features & (1ULL << VIRTIO_F_ACCESS_PLATFORM))) { if (vhost_init_device_iotlb(&vsock->dev, true)) goto err; } just makes me go "What?" It makes no sense. Why isn't that feature called something-something-IOTLB? Can we please just split that flag into two, and have that odd "platform access" be one bit, and the "enable iommu" be an entirely different bit? Now, since clearly nobody runs Android on newer kernels, I do think that the actual bit number choice should probably be one that makes the non-android use case binaries continue to work. And then the android system binaries that use this could maybe be compiled to know about *both* bits,. and work regardless? I'm also hoping that maybe Google android people could actually do some *testing*? I know, that sounds like a lot to ask, but humor me. Even if the product team runs stuff that is 18 months old, how about the dev team have a machine or two that actually tests current kernels, so that it's not a "oh, a few years have passed, and now we notice that a change doesn't work for us" situation any more. Is that really too much to ask for a big company like google? And hey, it's possible that the bit encoding is *so* incestuous that it's really hard to split it into two. But it really sounds to me like somebody mindlessly re-used a feature bit for a *completely* different thing. Why? Why have feature bits at all, when you then re-use the same bit for two different features? It kind of seems to defeat the whole purpose. Linus
WARNING: multiple messages have this Message-ID (diff)
From: Linus Torvalds <torvalds@linux-foundation.org> To: Will Deacon <will@kernel.org> 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 Subject: Re: IOTLB support for vhost/vsock breaks crosvm on Android Date: Fri, 5 Aug 2022 15:57:08 -0700 [thread overview] Message-ID: <CAHk-=wip-Lju3ZdNwknS6ouyw+nKXeRSnhqVyNo8WSEdk-BfGw@mail.gmail.com> (raw) In-Reply-To: <20220805181105.GA29848@willie-the-truck> On Fri, Aug 5, 2022 at 11:11 AM Will Deacon <will@kernel.org> wrote: > > [tl;dr a change from ~18 months ago breaks Android userspace and I don't > know what to do about it] Augh. I had hoped that android being "closer" to upstream would have meant that somebody actually tests android with upstream kernels. People occasionally talk about it, but apparently it's not actually done. Or maybe it's done onl;y with a very limited android user space. The whole "we notice that something that happened 18 months ago broke our environment" is kind of broken. > After some digging, we narrowed this change in behaviour down to > e13a6915a03f ("vhost/vsock: add IOTLB API support") and further digging > reveals that the infamous VIRTIO_F_ACCESS_PLATFORM feature flag is to > blame. Indeed, our tests once again pass if we revert that patch (there's > a trivial conflict with the later addition of VIRTIO_VSOCK_F_SEQPACKET > but otherwise it reverts cleanly). I have to say, this smells for *so* many reasons. Why is "IOMMU support" called "VIRTIO_F_ACCESS_PLATFORM"? That seems insane, but seems fundamental in that commit e13a6915a03f ("vhost/vsock: add IOTLB API support") This code if ((features & (1ULL << VIRTIO_F_ACCESS_PLATFORM))) { if (vhost_init_device_iotlb(&vsock->dev, true)) goto err; } just makes me go "What?" It makes no sense. Why isn't that feature called something-something-IOTLB? Can we please just split that flag into two, and have that odd "platform access" be one bit, and the "enable iommu" be an entirely different bit? Now, since clearly nobody runs Android on newer kernels, I do think that the actual bit number choice should probably be one that makes the non-android use case binaries continue to work. And then the android system binaries that use this could maybe be compiled to know about *both* bits,. and work regardless? I'm also hoping that maybe Google android people could actually do some *testing*? I know, that sounds like a lot to ask, but humor me. Even if the product team runs stuff that is 18 months old, how about the dev team have a machine or two that actually tests current kernels, so that it's not a "oh, a few years have passed, and now we notice that a change doesn't work for us" situation any more. Is that really too much to ask for a big company like google? And hey, it's possible that the bit encoding is *so* incestuous that it's really hard to split it into two. But it really sounds to me like somebody mindlessly re-used a feature bit for a *completely* different thing. Why? Why have feature bits at all, when you then re-use the same bit for two different features? It kind of seems to defeat the whole purpose. Linus _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2022-08-05 22:57 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 [this message] 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 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='CAHk-=wip-Lju3ZdNwknS6ouyw+nKXeRSnhqVyNo8WSEdk-BfGw@mail.gmail.com' \ --to=torvalds@linux-foundation.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=virtualization@lists.linux-foundation.org \ --cc=will@kernel.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.