Patchwork [01/15] s390x: fix virtio feature bitmap

login
register
mail settings
Submitter Alexander Graf
Date April 4, 2011, 2:32 p.m.
Message ID <1301927544-32767-2-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/89631/
State New
Headers show

Comments

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(-)
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

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