Re: [PULL] virtio/vhost: cross endian support
On Tue, Jul 07, 2015 at 06:36:53PM +0200, Thomas Huth wrote: On Thu, 2 Jul 2015 11:32:52 +0200 Michael S. Tsirkin m...@redhat.com wrote: On Thu, Jul 02, 2015 at 11:12:56AM +0200, Greg Kurz wrote: On Thu, 2 Jul 2015 08:01:28 +0200 Michael S. Tsirkin m...@redhat.com wrote: ... Yea, well - support for legacy BE guests on the new LE hosts is exactly the motivation for this. I dislike it too, but there are two redeeming properties that made me merge this: 1. It's a trivial amount of code: since we wrap host/guest accesses anyway, almost all of it is well hidden from drivers. 2. Sane platforms would never set flags like VHOST_CROSS_ENDIAN_LEGACY - and when it's clear, there's zero overhead (as some point it was tested by compiling with and without the patches, got the same stripped binary). Maybe we could create a Kconfig symbol to enforce point (2): prevent people from enabling it e.g. on x86. I will look into this - but it can be done by a patch on top, so I think this can be merged as is. This cross-endian *oddity* is targeting PowerPC book3s_64 processors... I am not aware of any other users. Maybe create a symbol that would be only selected by PPC_BOOK3S_64 ? I think some ARM systems are trying to support cross-endian configurations as well. Besides that, yes, this is more or less what I had in mind. Would something simple like this already do the job: diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig --- a/drivers/vhost/Kconfig +++ b/drivers/vhost/Kconfig @@ -35,6 +35,7 @@ config VHOST config VHOST_CROSS_ENDIAN_LEGACY bool Cross-endian support for vhost + depends on KVM_BOOK3S_64 || KVM_ARM_HOST default n ---help--- This option allows vhost to support guests with a different byte ? Do all ARM hosts support this dynamic endian-ness? If that looks acceptable, I can submit a proper patch if you like. Thomas I think I prefer some kind of symbol defined by these arches, so I don't get to maintain an arch list in vhost. -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] virtio/vhost: cross endian support
On Thu, 2 Jul 2015 11:32:52 +0200 Michael S. Tsirkin m...@redhat.com wrote: On Thu, Jul 02, 2015 at 11:12:56AM +0200, Greg Kurz wrote: On Thu, 2 Jul 2015 08:01:28 +0200 Michael S. Tsirkin m...@redhat.com wrote: ... Yea, well - support for legacy BE guests on the new LE hosts is exactly the motivation for this. I dislike it too, but there are two redeeming properties that made me merge this: 1. It's a trivial amount of code: since we wrap host/guest accesses anyway, almost all of it is well hidden from drivers. 2. Sane platforms would never set flags like VHOST_CROSS_ENDIAN_LEGACY - and when it's clear, there's zero overhead (as some point it was tested by compiling with and without the patches, got the same stripped binary). Maybe we could create a Kconfig symbol to enforce point (2): prevent people from enabling it e.g. on x86. I will look into this - but it can be done by a patch on top, so I think this can be merged as is. This cross-endian *oddity* is targeting PowerPC book3s_64 processors... I am not aware of any other users. Maybe create a symbol that would be only selected by PPC_BOOK3S_64 ? I think some ARM systems are trying to support cross-endian configurations as well. Besides that, yes, this is more or less what I had in mind. Would something simple like this already do the job: diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig --- a/drivers/vhost/Kconfig +++ b/drivers/vhost/Kconfig @@ -35,6 +35,7 @@ config VHOST config VHOST_CROSS_ENDIAN_LEGACY bool Cross-endian support for vhost + depends on KVM_BOOK3S_64 || KVM_ARM_HOST default n ---help--- This option allows vhost to support guests with a different byte ? If that looks acceptable, I can submit a proper patch if you like. Thomas -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] virtio/vhost: cross endian support
On Fri, Jul 3, 2015 at 12:59 AM, Michael S. Tsirkin m...@redhat.com wrote: Linus, could you please clarify whether making the feature depend on the cross-endian guest support would address your comment, and whether you think this can be merged as is, and the dependency added after -rc1? I'll take it. I still think dynamic byte order is a fundamental (and much too common) mistake, but whatever. Linus -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] virtio/vhost: cross endian support
On Thu, Jul 02, 2015 at 08:01:28AM +0200, Michael S. Tsirkin wrote: On Wed, Jul 01, 2015 at 12:02:50PM -0700, Linus Torvalds wrote: On Wed, Jul 1, 2015 at 2:31 AM, Michael S. Tsirkin m...@redhat.com wrote: virtio/vhost: cross endian support Ugh. Does this really have to be dynamic? Can't virtio do the sane thing, and just use a _fixed_ endianness? Doing a unconditional byte swap is faster and simpler than the crazy conditionals. That's true regardless of endianness, but gets to be even more so if the fixed endianness is little-endian, since BE is not-so-slowly fading from the world. Linus Yea, well - support for legacy BE guests on the new LE hosts is exactly the motivation for this. I dislike it too, but there are two redeeming properties that made me merge this: 1. It's a trivial amount of code: since we wrap host/guest accesses anyway, almost all of it is well hidden from drivers. 2. Sane platforms would never set flags like VHOST_CROSS_ENDIAN_LEGACY - and when it's clear, there's zero overhead (as some point it was tested by compiling with and without the patches, got the same stripped binary). Maybe we could create a Kconfig symbol to enforce point (2): prevent people from enabling it e.g. on x86. I will look into this - but it can be done by a patch on top, so I think this can be merged as is. Linus, could you please clarify whether making the feature depend on the cross-endian guest support would address your comment, and whether you think this can be merged as is, and the dependency added after -rc1? Or do you know of someone using kernel with all config options enabled undiscriminately? Thanks, -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] virtio/vhost: cross endian support
On Wed, Jul 01, 2015 at 12:02:50PM -0700, Linus Torvalds wrote: On Wed, Jul 1, 2015 at 2:31 AM, Michael S. Tsirkin m...@redhat.com wrote: virtio/vhost: cross endian support Ugh. Does this really have to be dynamic? Can't virtio do the sane thing, and just use a _fixed_ endianness? Doing a unconditional byte swap is faster and simpler than the crazy conditionals. That's true regardless of endianness, but gets to be even more so if the fixed endianness is little-endian, since BE is not-so-slowly fading from the world. Linus Yea, well - support for legacy BE guests on the new LE hosts is exactly the motivation for this. I dislike it too, but there are two redeeming properties that made me merge this: 1. It's a trivial amount of code: since we wrap host/guest accesses anyway, almost all of it is well hidden from drivers. 2. Sane platforms would never set flags like VHOST_CROSS_ENDIAN_LEGACY - and when it's clear, there's zero overhead (as some point it was tested by compiling with and without the patches, got the same stripped binary). Maybe we could create a Kconfig symbol to enforce point (2): prevent people from enabling it e.g. on x86. I will look into this - but it can be done by a patch on top, so I think this can be merged as is. Or do you know of someone using kernel with all config options enabled undiscriminately? Thanks, -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] virtio/vhost: cross endian support
On Wed, Jul 01, 2015 at 12:03:59PM -0700, Linus Torvalds wrote: On Wed, Jul 1, 2015 at 12:02 PM, Linus Torvalds torva...@linux-foundation.org wrote: Doing a unconditional byte swap is faster and simpler than the crazy conditionals. Unconditional endianness not only makes for simpler and faster code, it also ends up being easier to debug and add things like type annotations for sparse. Linus At least this last one is well covered by these patches: this uses separate sparse types so all accesses are statically verified by sparse to use the correct accessor. -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] virtio/vhost: cross endian support
On Thu, 2 Jul 2015 08:01:28 +0200 Michael S. Tsirkin m...@redhat.com wrote: On Wed, Jul 01, 2015 at 12:02:50PM -0700, Linus Torvalds wrote: On Wed, Jul 1, 2015 at 2:31 AM, Michael S. Tsirkin m...@redhat.com wrote: virtio/vhost: cross endian support Ugh. Does this really have to be dynamic? Can't virtio do the sane thing, and just use a _fixed_ endianness? Doing a unconditional byte swap is faster and simpler than the crazy conditionals. That's true regardless of endianness, but gets to be even more so if the fixed endianness is little-endian, since BE is not-so-slowly fading from the world. Linus Yea, well - support for legacy BE guests on the new LE hosts is exactly the motivation for this. I dislike it too, but there are two redeeming properties that made me merge this: 1. It's a trivial amount of code: since we wrap host/guest accesses anyway, almost all of it is well hidden from drivers. 2. Sane platforms would never set flags like VHOST_CROSS_ENDIAN_LEGACY - and when it's clear, there's zero overhead (as some point it was tested by compiling with and without the patches, got the same stripped binary). Maybe we could create a Kconfig symbol to enforce point (2): prevent people from enabling it e.g. on x86. I will look into this - but it can be done by a patch on top, so I think this can be merged as is. This cross-endian *oddity* is targeting PowerPC book3s_64 processors... I am not aware of any other users. Maybe create a symbol that would be only selected by PPC_BOOK3S_64 ? Or do you know of someone using kernel with all config options enabled undiscriminately? Thanks, -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] virtio/vhost: cross endian support
On Thu, Jul 02, 2015 at 11:12:56AM +0200, Greg Kurz wrote: On Thu, 2 Jul 2015 08:01:28 +0200 Michael S. Tsirkin m...@redhat.com wrote: On Wed, Jul 01, 2015 at 12:02:50PM -0700, Linus Torvalds wrote: On Wed, Jul 1, 2015 at 2:31 AM, Michael S. Tsirkin m...@redhat.com wrote: virtio/vhost: cross endian support Ugh. Does this really have to be dynamic? Can't virtio do the sane thing, and just use a _fixed_ endianness? Doing a unconditional byte swap is faster and simpler than the crazy conditionals. That's true regardless of endianness, but gets to be even more so if the fixed endianness is little-endian, since BE is not-so-slowly fading from the world. Linus Yea, well - support for legacy BE guests on the new LE hosts is exactly the motivation for this. I dislike it too, but there are two redeeming properties that made me merge this: 1. It's a trivial amount of code: since we wrap host/guest accesses anyway, almost all of it is well hidden from drivers. 2. Sane platforms would never set flags like VHOST_CROSS_ENDIAN_LEGACY - and when it's clear, there's zero overhead (as some point it was tested by compiling with and without the patches, got the same stripped binary). Maybe we could create a Kconfig symbol to enforce point (2): prevent people from enabling it e.g. on x86. I will look into this - but it can be done by a patch on top, so I think this can be merged as is. This cross-endian *oddity* is targeting PowerPC book3s_64 processors... I am not aware of any other users. Maybe create a symbol that would be only selected by PPC_BOOK3S_64 ? I think some ARM systems are trying to support cross-endian configurations as well. Besides that, yes, this is more or less what I had in mind. Or do you know of someone using kernel with all config options enabled undiscriminately? Thanks, -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] virtio/vhost: cross endian support
The following changes since commit 8a7b19d8b542b87bccc3eaaf81dcc90a5ca48aea: include/uapi/linux/virtio_balloon.h: include linux/virtio_types.h (2015-06-01 15:46:54 +0200) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus for you to fetch changes up to 59a5b0f7bf74f88da6670bcbf924d8cc1e75b1ee: virtio-pci: alloc only resources actually used. (2015-06-24 08:15:09 +0200) virtio/vhost: cross endian support I have just queued some more bugfix patches today but none fix regressions and none are related to these ones, so it looks like a good time for a merge for -rc1. Signed-off-by: Michael S. Tsirkin m...@redhat.com Gerd Hoffmann (1): virtio-pci: alloc only resources actually used. Greg Kurz (8): virtio: introduce virtio_is_little_endian() helper tun: add tun_is_little_endian() helper macvtap: introduce macvtap_is_little_endian() helper vringh: introduce vringh_is_little_endian() helper vhost: introduce vhost_is_little_endian() helper virtio: add explicit big-endian support to memory accessors vhost: cross-endian support for legacy devices macvtap/tun: cross-endian support for little-endian hosts drivers/vhost/vhost.h | 25 --- drivers/virtio/virtio_pci_common.h | 2 + include/linux/virtio_byteorder.h | 24 ++- include/linux/virtio_config.h | 18 +--- include/linux/vringh.h | 18 +--- include/uapi/linux/if_tun.h| 6 +++ include/uapi/linux/vhost.h | 14 +++ drivers/net/macvtap.c | 65 - drivers/net/tun.c | 67 +- drivers/vhost/vhost.c | 85 +- drivers/virtio/virtio_pci_common.c | 7 drivers/virtio/virtio_pci_legacy.c | 13 +- drivers/virtio/virtio_pci_modern.c | 24 --- drivers/net/Kconfig| 14 +++ drivers/vhost/Kconfig | 15 +++ 15 files changed, 350 insertions(+), 47 deletions(-) -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] virtio/vhost: cross endian support
On Wed, Jul 1, 2015 at 2:31 AM, Michael S. Tsirkin m...@redhat.com wrote: virtio/vhost: cross endian support Ugh. Does this really have to be dynamic? Can't virtio do the sane thing, and just use a _fixed_ endianness? Doing a unconditional byte swap is faster and simpler than the crazy conditionals. That's true regardless of endianness, but gets to be even more so if the fixed endianness is little-endian, since BE is not-so-slowly fading from the world. Linus -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] virtio/vhost: cross endian support
On Wed, Jul 1, 2015 at 12:02 PM, Linus Torvalds torva...@linux-foundation.org wrote: Doing a unconditional byte swap is faster and simpler than the crazy conditionals. Unconditional endianness not only makes for simpler and faster code, it also ends up being easier to debug and add things like type annotations for sparse. Linus -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
