Re: [Qemu-devel] [PATCH RFC v6 10/20] s390x/virtio-ccw: add virtio set-revision call

[Cornelia Huck]

On Wed, 21 Jan 2015 12:51:41 +0100
Thomas Huth <address@hidden> wrote:

> On Wed, 21 Jan 2015 12:23:18 +0100
> Cornelia Huck <address@hidden> wrote:
> 
> > On Tue, 20 Jan 2015 11:08:24 +0000
> > Stefan Hajnoczi <address@hidden> wrote:
> > 
> > > On Thu, Dec 11, 2014 at 02:25:12PM +0100, Cornelia Huck wrote:
> > > > @@ -608,6 +631,25 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> > > >              }
> > > >          }
> > > >          break;
> > > > +    case CCW_CMD_SET_VIRTIO_REV:
> > > > +        len = sizeof(revinfo);
> > > > +        if (ccw.count < len || (check_len && ccw.count > len)) {
> > > > +            ret = -EINVAL;
> > > > +            break;
> > > > +        }
> > > > +        if (!ccw.cda) {
> > > > +            ret = -EFAULT;
> > > > +            break;
> > > > +        }
> > > > +        cpu_physical_memory_read(ccw.cda, &revinfo, len);
> > > > +        if (dev->revision >= 0 ||
> > > > +            revinfo.revision > virtio_ccw_rev_max(dev)) {
> > > 
> > > In the next patch virtio_ccw_handle_set_vq() uses big-endian memory
> > > access functions to load a struct from guest memory.
> > > 
> > > Here you just copy the struct in without byteswaps.
> > > 
> > > Are the byteswaps missing here?  (I guess this normally runs big-endian
> > > guests on big-endian hosts so it's not noticable.)
> > 
> > Indeed, these are supposed to be big-endian. I'll double check the
> > other payloads.
> 
> Right. Cornelia, can you take care of this or shall I rework the patch?

Currently already working on a patch :)

> NB: Actually, there are a couple of "XXX config space endianness"
> comments in that virtio_ccw_cb() function, so there are likely a bunch
> of problems when this code should be run on little endian hosts one day.
> So far, this code only runs with big-endian guests on big-endian hosts
> since the virtio-ccw machine is currently KVM-only as far as I know,
> that's likely why nobody complained about this yet.

The transport can't take care of the config space endianness, this
needs to be done by the individual devices. Probably best to simply
ditch the comments.