[01/15] s390x: fix virtio feature bitmap

Submitted by Alexander Graf on April 4, 2011, 2:32 p.m.

Details

Message ID 1301927544-32767-2-git-send-email-agraf@suse.de
State New
Headers show

Commit Message

Alexander Graf April 4, 2011, 2:32 p.m.
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(-)

Comments

Aurelien Jarno April 10, 2011, 7:25 p.m.
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
> 
> 
>
Alexander Graf April 10, 2011, 7:26 p.m.
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
Aurelien Jarno April 10, 2011, 8:06 p.m.
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...
Alexander Graf April 10, 2011, 8:11 p.m.
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
Aurelien Jarno April 10, 2011, 8:26 p.m.
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.

Patch hide | download patch | download mbox

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);
     }