Re: [PULL] virtio/vhost: cross endian support

2015-07-07 Thread Thomas Huth
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PULL] virtio/vhost: cross endian support

2015-07-07 Thread Michael S. Tsirkin
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PULL] virtio/vhost: cross endian support

2015-07-03 Thread Linus Torvalds
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PULL] virtio/vhost: cross endian support

2015-07-03 Thread Michael S. Tsirkin
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PULL] virtio/vhost: cross endian support

2015-07-02 Thread Michael S. Tsirkin
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PULL] virtio/vhost: cross endian support

2015-07-02 Thread Michael S. Tsirkin
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PULL] virtio/vhost: cross endian support

2015-07-02 Thread Greg Kurz
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,
 

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PULL] virtio/vhost: cross endian support

2015-07-02 Thread Michael S. Tsirkin
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,
  
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PULL] virtio/vhost: cross endian support

2015-07-01 Thread Linus Torvalds
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PULL] virtio/vhost: cross endian support

2015-07-01 Thread Linus Torvalds
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization