Re: [Qemu-devel] [PATCH 01/15] s390x: fix virtio feature bitmap

[Aurelien Jarno <aurelien@aurel32.net>]

On Sun, Apr 10, 2011 at 09:26:26PM +0200, Alexander Graf wrote:
> 
> On 10.04.2011, at 21:25, Aurelien Jarno wrote:
> 
> > On Mon, Apr 04, 2011 at 04:32:10PM +0200, Alexander Graf wrote:
> >> The feature bitmap in the s390 virtio machine is little endian. To
> >> address for that, we need to bswap the values after reading them out.
> >> 
> >> Signed-off-by: Alexander Graf <agraf@suse.de>
> >> ---
> >> hw/s390-virtio-bus.c |    4 ++--
> >> 1 files changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
> >> index 58af164..60e0135 100644
> >> --- a/hw/s390-virtio-bus.c
> >> +++ b/hw/s390-virtio-bus.c
> >> @@ -223,7 +223,7 @@ void s390_virtio_device_sync(VirtIOS390Device *dev)
> >>     cur_offs += num_vq * VIRTIO_VQCONFIG_LEN;
> >> 
> >>     /* Sync feature bitmap */
> >> -    stl_phys(cur_offs, dev->host_features);
> >> +    stl_phys(cur_offs, bswap32(dev->host_features));
> > 
> > Is bswap32 correct here for both big and little endian guests? I don't
> > really understand the reason why a bswap is needed here, especially
> > given that AFAIK this code was already used when using KVM.
> 
> This is target specific code. The s390-virtio-bus is s390 specific. And yes, the code was also broken with KVM. That's how I first found it actually.
> 

So in short, s390-virtio-bus is specific to big endian machines, but is
using little endian? That looks a bit weired. I guess it's probably to
late to change the specification anyway...

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net