| Message ID | 1301927544-32767-2-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
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. > dev->feat_offs = cur_offs + dev->feat_len; > cur_offs += dev->feat_len * 2; > @@ -246,7 +246,7 @@ void s390_virtio_device_update_status(VirtIOS390Device *dev) > > /* Update guest supported feature bitmap */ > > - features = ldl_phys(dev->feat_offs); > + features = bswap32(ldl_phys(dev->feat_offs)); > if (vdev->set_features) { > vdev->set_features(vdev, features); > } > -- > 1.6.0.2 > > >
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. Alex
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...
On 10.04.2011, at 22:06, Aurelien Jarno wrote: > 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... Yeah. In fact, it mixes big and little endian pieces. But it really is too late to change the (unwritten) spec, unfortunately. Alex
On Sun, Apr 10, 2011 at 10:11:15PM +0200, Alexander Graf wrote: > > On 10.04.2011, at 22:06, Aurelien Jarno wrote: > > > 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... > > Yeah. In fact, it mixes big and little endian pieces. But it really is too late to change the (unwritten) spec, unfortunately. > Ok, thanks for the explanations, I have applied it.
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)); dev->feat_offs = cur_offs + dev->feat_len; cur_offs += dev->feat_len * 2; @@ -246,7 +246,7 @@ void s390_virtio_device_update_status(VirtIOS390Device *dev) /* Update guest supported feature bitmap */ - features = ldl_phys(dev->feat_offs); + features = bswap32(ldl_phys(dev->feat_offs)); if (vdev->set_features) { vdev->set_features(vdev, features); }
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(-)