| Message ID | 20170913115029.47626-4-pasic@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | add CCW indirect data access support | expand |
On Wed, 13 Sep 2017 13:50:28 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > Replace direct access which implicitly assumes no IDA > or MIDA with the new ccw data stream interface which should > cope with these transparently in the future. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > @@ -488,7 +446,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > } else { > virtio_bus_get_vdev_config(&dev->bus, vdev->config); > /* XXX config space endianness */ > - cpu_physical_memory_write(ccw.cda, vdev->config, len); > + ccw_dstream_write_buf(&sch->cds, vdev->config, len); > sch->curr_status.scsw.count = ccw.count - len; > ret = 0; > } > @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > } > } > len = MIN(ccw.count, vdev->config_len); > - hw_len = len; > if (!ccw.cda) { > ret = -EFAULT; > } else { > - config = cpu_physical_memory_map(ccw.cda, &hw_len, 0); > - if (!config) { > - ret = -EFAULT; > - } else { > - len = hw_len; > - /* XXX config space endianness */ > - memcpy(vdev->config, config, len); > - cpu_physical_memory_unmap(config, hw_len, 0, hw_len); > + ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len); > + if (!ret) { > virtio_bus_set_vdev_config(&dev->bus, vdev->config); > sch->curr_status.scsw.count = ccw.count - len; > - ret = 0; > } > } > break; It feels a bit odd that you remove the stale XXX comment only for one direction. Care to post a patch that removes them separately? Else I can do that as well. [I'd queue that patch in front of this patchset, but no need to resend for that.]
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-13 13:50:28 +0200]: > Replace direct access which implicitly assumes no IDA > or MIDA with the new ccw data stream interface which should > cope with these transparently in the future. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > > --- > > v1 --> v2: > Removed todo comments on possible errno change as with > https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02441.html > applied it does not matter any more. > > Error handling: At the moment we ignore errors reported > by stream ops to keep the change minimal. An add-on > patch improving on this is to be expected later. Add a TODO somewhere around the code as a reminder? > --- > hw/s390x/virtio-ccw.c | 156 +++++++++++++++----------------------------------- > 1 file changed, 46 insertions(+), 110 deletions(-) > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index b1976fdd19..a9baf6f7ab 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c [...] > @@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > } else { > VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); > > - features.index = address_space_ldub(&address_space_memory, > - ccw.cda > - + sizeof(features.features), > - MEMTXATTRS_UNSPECIFIED, > - NULL); > + ccw_dstream_advance(&sch->cds, sizeof(features.features)); How about: ccw_dstream_advance(&sch->cds, offsetof(VirtioFeatDesc, index)); > + ccw_dstream_read(&sch->cds, features.index); > if (features.index == 0) { > if (dev->revision >= 1) { > /* Don't offer legacy features for modern devices. */ > @@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > /* Return zeroes if the guest supports more feature bits. */ > features.features = 0; > } > - address_space_stl_le(&address_space_memory, ccw.cda, > - features.features, MEMTXATTRS_UNSPECIFIED, > - NULL); > + ccw_dstream_rewind(&sch->cds); > + cpu_to_le32s(&features.features); > + ccw_dstream_write(&sch->cds, features.features); > sch->curr_status.scsw.count = ccw.count - sizeof(features); How about: sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds); This also applies to other places. > ret = 0; > } [...] > @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > } > } > len = MIN(ccw.count, vdev->config_len); > - hw_len = len; > if (!ccw.cda) { > ret = -EFAULT; > } else { > - config = cpu_physical_memory_map(ccw.cda, &hw_len, 0); > - if (!config) { > - ret = -EFAULT; > - } else { > - len = hw_len; > - /* XXX config space endianness */ > - memcpy(vdev->config, config, len); > - cpu_physical_memory_unmap(config, hw_len, 0, hw_len); > + ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len); > + if (!ret) { Add a TODO here, and: if (ccw_dstream_read_buf(&sch->cds, vdev->config, len)) { ret = -EFAULT; } else { .... } > virtio_bus_set_vdev_config(&dev->bus, vdev->config); > sch->curr_status.scsw.count = ccw.count - len; > - ret = 0; > } > } > break; [...] > @@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > ret = -EFAULT; > break; > } > - revinfo.revision = > - address_space_lduw_be(&address_space_memory, ccw.cda, > - MEMTXATTRS_UNSPECIFIED, NULL); > - revinfo.length = > - address_space_lduw_be(&address_space_memory, > - ccw.cda + sizeof(revinfo.revision), > - MEMTXATTRS_UNSPECIFIED, NULL); > + ccw_dstream_read_buf(&sch->cds, &revinfo, 4); ^ A magic number? O:) > + be16_to_cpus(&revinfo.revision); > + be16_to_cpus(&revinfo.length); > if (ccw.count < len + revinfo.length || > (check_len && ccw.count > len + revinfo.length)) { > ret = -EINVAL; > -- > 2.13.5 > Otherwise, looks good.
On Tue, 19 Sep 2017 11:37:30 +0800 Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-13 13:50:28 +0200]: > > > Replace direct access which implicitly assumes no IDA > > or MIDA with the new ccw data stream interface which should > > cope with these transparently in the future. > > > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > > > > --- > > > > v1 --> v2: > > Removed todo comments on possible errno change as with > > https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02441.html > > applied it does not matter any more. > > > > Error handling: At the moment we ignore errors reported > > by stream ops to keep the change minimal. An add-on > > patch improving on this is to be expected later. > Add a TODO somewhere around the code as a reminder? I expect Halil to have it on his TODO list and send a patch later ;) > > > --- > > hw/s390x/virtio-ccw.c | 156 +++++++++++++++----------------------------------- > > 1 file changed, 46 insertions(+), 110 deletions(-) > > > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > > index b1976fdd19..a9baf6f7ab 100644 > > --- a/hw/s390x/virtio-ccw.c > > +++ b/hw/s390x/virtio-ccw.c > [...] > > > @@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > > } else { > > VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); > > > > - features.index = address_space_ldub(&address_space_memory, > > - ccw.cda > > - + sizeof(features.features), > > - MEMTXATTRS_UNSPECIFIED, > > - NULL); > > + ccw_dstream_advance(&sch->cds, sizeof(features.features)); > How about: > ccw_dstream_advance(&sch->cds, offsetof(VirtioFeatDesc, index)); I think keeping sizeof(features.features) is better: It matches the old code, and we really do jump over the features flag and revisit it later... > > > + ccw_dstream_read(&sch->cds, features.index); > > if (features.index == 0) { > > if (dev->revision >= 1) { > > /* Don't offer legacy features for modern devices. */ > > @@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > > /* Return zeroes if the guest supports more feature bits. */ > > features.features = 0; > > } > > - address_space_stl_le(&address_space_memory, ccw.cda, > > - features.features, MEMTXATTRS_UNSPECIFIED, > > - NULL); > > + ccw_dstream_rewind(&sch->cds); > > + cpu_to_le32s(&features.features); > > + ccw_dstream_write(&sch->cds, features.features); > > sch->curr_status.scsw.count = ccw.count - sizeof(features); > How about: > sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds); Hm, I thought I had commented on this, but I seem to have missed these... I'd prefer to do it as a follow-up patch. > > This also applies to other places. > > > ret = 0; > > } > [...] > > > @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > > } > > } > > len = MIN(ccw.count, vdev->config_len); > > - hw_len = len; > > if (!ccw.cda) { > > ret = -EFAULT; > > } else { > > - config = cpu_physical_memory_map(ccw.cda, &hw_len, 0); > > - if (!config) { > > - ret = -EFAULT; > > - } else { > > - len = hw_len; > > - /* XXX config space endianness */ > > - memcpy(vdev->config, config, len); > > - cpu_physical_memory_unmap(config, hw_len, 0, hw_len); > > + ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len); > > + if (!ret) { > Add a TODO here, and: > > if (ccw_dstream_read_buf(&sch->cds, vdev->config, len)) { > ret = -EFAULT; > } else { > .... > } I don't think that would be correct: The function will (at least currently) return 0 or -EINVAL, and you are now mapping any error to -EFAULT? (Not that it has an effect in the end: We map both to a channel program check as of "s390x/css: drop data-check in interpretation".) > > > virtio_bus_set_vdev_config(&dev->bus, vdev->config); > > sch->curr_status.scsw.count = ccw.count - len; > > - ret = 0; > > } > > } > > break; > [...] > > > @@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > > ret = -EFAULT; > > break; > > } > > - revinfo.revision = > > - address_space_lduw_be(&address_space_memory, ccw.cda, > > - MEMTXATTRS_UNSPECIFIED, NULL); > > - revinfo.length = > > - address_space_lduw_be(&address_space_memory, > > - ccw.cda + sizeof(revinfo.revision), > > - MEMTXATTRS_UNSPECIFIED, NULL); > > + ccw_dstream_read_buf(&sch->cds, &revinfo, 4); > ^ > A magic number? O:) Hah :) We can't use sizeof(revinfo), and sizeof(revinfo.revision) + sizeof(revinfo.length) is a bit unwieldy. Let's keep some magic in our code :) > > > + be16_to_cpus(&revinfo.revision); > > + be16_to_cpus(&revinfo.length); > > if (ccw.count < len + revinfo.length || > > (check_len && ccw.count > len + revinfo.length)) { > > ret = -EINVAL; > > -- > > 2.13.5 > > > > Otherwise, looks good. > Can I get an ack?
On 09/19/2017 11:06 AM, Cornelia Huck wrote: > On Tue, 19 Sep 2017 11:37:30 +0800 > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > >> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-13 13:50:28 +0200]: >> >>> Replace direct access which implicitly assumes no IDA >>> or MIDA with the new ccw data stream interface which should >>> cope with these transparently in the future. >>> >>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>> >>> --- >>> >>> v1 --> v2: >>> Removed todo comments on possible errno change as with >>> https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02441.html >>> applied it does not matter any more. >>> >>> Error handling: At the moment we ignore errors reported >>> by stream ops to keep the change minimal. An add-on >>> patch improving on this is to be expected later. >> Add a TODO somewhere around the code as a reminder? > > I expect Halil to have it on his TODO list and send a patch later ;) > >> >>> --- >>> hw/s390x/virtio-ccw.c | 156 +++++++++++++++----------------------------------- >>> 1 file changed, 46 insertions(+), 110 deletions(-) >>> >>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c >>> index b1976fdd19..a9baf6f7ab 100644 >>> --- a/hw/s390x/virtio-ccw.c >>> +++ b/hw/s390x/virtio-ccw.c >> [...] >> >>> @@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) >>> } else { >>> VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); >>> >>> - features.index = address_space_ldub(&address_space_memory, >>> - ccw.cda >>> - + sizeof(features.features), >>> - MEMTXATTRS_UNSPECIFIED, >>> - NULL); >>> + ccw_dstream_advance(&sch->cds, sizeof(features.features)); >> How about: >> ccw_dstream_advance(&sch->cds, offsetof(VirtioFeatDesc, index)); > > I think keeping sizeof(features.features) is better: It matches the old > code, and we really do jump over the features flag and revisit it > later... > >> >>> + ccw_dstream_read(&sch->cds, features.index); >>> if (features.index == 0) { >>> if (dev->revision >= 1) { >>> /* Don't offer legacy features for modern devices. */ >>> @@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) >>> /* Return zeroes if the guest supports more feature bits. */ >>> features.features = 0; >>> } >>> - address_space_stl_le(&address_space_memory, ccw.cda, >>> - features.features, MEMTXATTRS_UNSPECIFIED, >>> - NULL); >>> + ccw_dstream_rewind(&sch->cds); >>> + cpu_to_le32s(&features.features); >>> + ccw_dstream_write(&sch->cds, features.features); >>> sch->curr_status.scsw.count = ccw.count - sizeof(features); >> How about: >> sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds); > > Hm, I thought I had commented on this, but I seem to have missed > these... > > I'd prefer to do it as a follow-up patch. > >> >> This also applies to other places. >> >>> ret = 0; >>> } >> [...] >> >>> @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) >>> } >>> } >>> len = MIN(ccw.count, vdev->config_len); >>> - hw_len = len; >>> if (!ccw.cda) { >>> ret = -EFAULT; >>> } else { >>> - config = cpu_physical_memory_map(ccw.cda, &hw_len, 0); >>> - if (!config) { >>> - ret = -EFAULT; >>> - } else { >>> - len = hw_len; >>> - /* XXX config space endianness */ >>> - memcpy(vdev->config, config, len); >>> - cpu_physical_memory_unmap(config, hw_len, 0, hw_len); >>> + ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len); >>> + if (!ret) { >> Add a TODO here, and: >> >> if (ccw_dstream_read_buf(&sch->cds, vdev->config, len)) { >> ret = -EFAULT; >> } else { >> .... >> } > > I don't think that would be correct: The function will (at least > currently) return 0 or -EINVAL, and you are now mapping any error to > -EFAULT? (Not that it has an effect in the end: We map both to a > channel program check as of "s390x/css: drop data-check in > interpretation".) > >> >>> virtio_bus_set_vdev_config(&dev->bus, vdev->config); >>> sch->curr_status.scsw.count = ccw.count - len; >>> - ret = 0; >>> } >>> } >>> break; >> [...] >> >>> @@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) >>> ret = -EFAULT; >>> break; >>> } >>> - revinfo.revision = >>> - address_space_lduw_be(&address_space_memory, ccw.cda, >>> - MEMTXATTRS_UNSPECIFIED, NULL); >>> - revinfo.length = >>> - address_space_lduw_be(&address_space_memory, >>> - ccw.cda + sizeof(revinfo.revision), >>> - MEMTXATTRS_UNSPECIFIED, NULL); >>> + ccw_dstream_read_buf(&sch->cds, &revinfo, 4); >> ^ >> A magic number? O:) > > Hah :) > > We can't use sizeof(revinfo), and sizeof(revinfo.revision) + > sizeof(revinfo.length) is a bit unwieldy. Let's keep some magic in our > code :) > >> >>> + be16_to_cpus(&revinfo.revision); >>> + be16_to_cpus(&revinfo.length); >>> if (ccw.count < len + revinfo.length || >>> (check_len && ccw.count > len + revinfo.length)) { >>> ret = -EINVAL; >>> -- >>> 2.13.5 >>> >> >> Otherwise, looks good. >> > > Can I get an ack? > I agree that further cleanups are possible (e.g. using ccw_dstream_residual_count() and tightening error handling) but I also prefer to see these as on-top, and prefer sticking to change as little as possible in the transformation patch and stay as close as possible to what we had before in terms of behavior). Regards, Halil
On 13/09/2017 13:50, Halil Pasic wrote: > Replace direct access which implicitly assumes no IDA > or MIDA with the new ccw data stream interface which should > cope with these transparently in the future. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > > --- > > v1 --> v2: > Removed todo comments on possible errno change as with > https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02441.html > applied it does not matter any more. > > Error handling: At the moment we ignore errors reported > by stream ops to keep the change minimal. An add-on > patch improving on this is to be expected later. > --- > hw/s390x/virtio-ccw.c | 156 +++++++++++++++----------------------------------- > 1 file changed, 46 insertions(+), 110 deletions(-) :) > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index b1976fdd19..a9baf6f7ab 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -287,49 +287,19 @@ static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len, > return -EFAULT; > } > if (is_legacy) { > - linfo.queue = address_space_ldq_be(&address_space_memory, ccw.cda, > - MEMTXATTRS_UNSPECIFIED, NULL); > - linfo.align = address_space_ldl_be(&address_space_memory, > - ccw.cda + sizeof(linfo.queue), > - MEMTXATTRS_UNSPECIFIED, > - NULL); > - linfo.index = address_space_lduw_be(&address_space_memory, > - ccw.cda + sizeof(linfo.queue) > - + sizeof(linfo.align), > - MEMTXATTRS_UNSPECIFIED, > - NULL); > - linfo.num = address_space_lduw_be(&address_space_memory, > - ccw.cda + sizeof(linfo.queue) > - + sizeof(linfo.align) > - + sizeof(linfo.index), > - MEMTXATTRS_UNSPECIFIED, > - NULL); > + ccw_dstream_read(&sch->cds, linfo); > + be64_to_cpus(&linfo.queue); > + be32_to_cpus(&linfo.align); > + be16_to_cpus(&linfo.index); > + be16_to_cpus(&linfo.num); > ret = virtio_ccw_set_vqs(sch, NULL, &linfo); > } else { > - info.desc = address_space_ldq_be(&address_space_memory, ccw.cda, > - MEMTXATTRS_UNSPECIFIED, NULL); > - info.index = address_space_lduw_be(&address_space_memory, > - ccw.cda + sizeof(info.desc) > - + sizeof(info.res0), > - MEMTXATTRS_UNSPECIFIED, NULL); > - info.num = address_space_lduw_be(&address_space_memory, > - ccw.cda + sizeof(info.desc) > - + sizeof(info.res0) > - + sizeof(info.index), > - MEMTXATTRS_UNSPECIFIED, NULL); > - info.avail = address_space_ldq_be(&address_space_memory, > - ccw.cda + sizeof(info.desc) > - + sizeof(info.res0) > - + sizeof(info.index) > - + sizeof(info.num), > - MEMTXATTRS_UNSPECIFIED, NULL); > - info.used = address_space_ldq_be(&address_space_memory, > - ccw.cda + sizeof(info.desc) > - + sizeof(info.res0) > - + sizeof(info.index) > - + sizeof(info.num) > - + sizeof(info.avail), > - MEMTXATTRS_UNSPECIFIED, NULL); > + ccw_dstream_read(&sch->cds, info); > + be64_to_cpus(&info.desc); > + be16_to_cpus(&info.index); > + be16_to_cpus(&info.num); > + be64_to_cpus(&info.avail); > + be64_to_cpus(&info.used); > ret = virtio_ccw_set_vqs(sch, &info, NULL); > } > sch->curr_status.scsw.count = 0; > @@ -342,15 +312,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > VirtioRevInfo revinfo; > uint8_t status; > VirtioFeatDesc features; > - void *config; > hwaddr indicators; > VqConfigBlock vq_config; > VirtioCcwDevice *dev = sch->driver_data; > VirtIODevice *vdev = virtio_ccw_get_vdev(sch); > bool check_len; > int len; > - hwaddr hw_len; > - VirtioThinintInfo *thinint; > + VirtioThinintInfo thinint; > > if (!dev) { > return -EINVAL; > @@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > } else { > VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); > > - features.index = address_space_ldub(&address_space_memory, > - ccw.cda > - + sizeof(features.features), > - MEMTXATTRS_UNSPECIFIED, > - NULL); > + ccw_dstream_advance(&sch->cds, sizeof(features.features)); > + ccw_dstream_read(&sch->cds, features.index); > if (features.index == 0) { > if (dev->revision >= 1) { > /* Don't offer legacy features for modern devices. */ > @@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > /* Return zeroes if the guest supports more feature bits. */ > features.features = 0; > } > - address_space_stl_le(&address_space_memory, ccw.cda, > - features.features, MEMTXATTRS_UNSPECIFIED, > - NULL); > + ccw_dstream_rewind(&sch->cds); > + cpu_to_le32s(&features.features); > + ccw_dstream_write(&sch->cds, features.features); > sch->curr_status.scsw.count = ccw.count - sizeof(features); > ret = 0; > } > @@ -438,15 +403,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > if (!ccw.cda) { > ret = -EFAULT; > } else { > - features.index = address_space_ldub(&address_space_memory, > - ccw.cda > - + sizeof(features.features), > - MEMTXATTRS_UNSPECIFIED, > - NULL); > - features.features = address_space_ldl_le(&address_space_memory, > - ccw.cda, > - MEMTXATTRS_UNSPECIFIED, > - NULL); > + ccw_dstream_read(&sch->cds, features); > + le32_to_cpus(&features.features); > if (features.index == 0) { > virtio_set_features(vdev, > (vdev->guest_features & 0xffffffff00000000ULL) | > @@ -488,7 +446,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > } else { > virtio_bus_get_vdev_config(&dev->bus, vdev->config); > /* XXX config space endianness */ > - cpu_physical_memory_write(ccw.cda, vdev->config, len); > + ccw_dstream_write_buf(&sch->cds, vdev->config, len); > sch->curr_status.scsw.count = ccw.count - len; > ret = 0; > } > @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > } > } > len = MIN(ccw.count, vdev->config_len); > - hw_len = len; > if (!ccw.cda) { > ret = -EFAULT; > } else { > - config = cpu_physical_memory_map(ccw.cda, &hw_len, 0); > - if (!config) { > - ret = -EFAULT; > - } else { > - len = hw_len; > - /* XXX config space endianness */ > - memcpy(vdev->config, config, len); > - cpu_physical_memory_unmap(config, hw_len, 0, hw_len); > + ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len); > + if (!ret) { > virtio_bus_set_vdev_config(&dev->bus, vdev->config); > sch->curr_status.scsw.count = ccw.count - len; > - ret = 0; > } > } > break; > @@ -553,8 +503,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > if (!ccw.cda) { > ret = -EFAULT; > } else { > - status = address_space_ldub(&address_space_memory, ccw.cda, > - MEMTXATTRS_UNSPECIFIED, NULL); > + ccw_dstream_read(&sch->cds, status); > if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) { > virtio_ccw_stop_ioeventfd(dev); > } > @@ -597,8 +546,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > if (!ccw.cda) { > ret = -EFAULT; > } else { > - indicators = address_space_ldq_be(&address_space_memory, ccw.cda, > - MEMTXATTRS_UNSPECIFIED, NULL); > + ccw_dstream_read(&sch->cds, indicators); > + be64_to_cpus(&indicators); > dev->indicators = get_indicator(indicators, sizeof(uint64_t)); > sch->curr_status.scsw.count = ccw.count - sizeof(indicators); > ret = 0; > @@ -618,8 +567,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > if (!ccw.cda) { > ret = -EFAULT; > } else { > - indicators = address_space_ldq_be(&address_space_memory, ccw.cda, > - MEMTXATTRS_UNSPECIFIED, NULL); > + ccw_dstream_read(&sch->cds, indicators); > + be64_to_cpus(&indicators); > dev->indicators2 = get_indicator(indicators, sizeof(uint64_t)); > sch->curr_status.scsw.count = ccw.count - sizeof(indicators); > ret = 0; > @@ -639,67 +588,58 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > if (!ccw.cda) { > ret = -EFAULT; > } else { > - vq_config.index = address_space_lduw_be(&address_space_memory, > - ccw.cda, > - MEMTXATTRS_UNSPECIFIED, > - NULL); > + ccw_dstream_read(&sch->cds, vq_config.index); > + be16_to_cpus(&vq_config.index); > if (vq_config.index >= VIRTIO_QUEUE_MAX) { > ret = -EINVAL; > break; > } > vq_config.num_max = virtio_queue_get_num(vdev, > vq_config.index); > - address_space_stw_be(&address_space_memory, > - ccw.cda + sizeof(vq_config.index), > - vq_config.num_max, > - MEMTXATTRS_UNSPECIFIED, > - NULL); > + cpu_to_be16s(&vq_config.num_max); > + ccw_dstream_write(&sch->cds, vq_config.num_max); > sch->curr_status.scsw.count = ccw.count - sizeof(vq_config); > ret = 0; > } > break; > case CCW_CMD_SET_IND_ADAPTER: > if (check_len) { > - if (ccw.count != sizeof(*thinint)) { > + if (ccw.count != sizeof(thinint)) { > ret = -EINVAL; > break; > } > - } else if (ccw.count < sizeof(*thinint)) { > + } else if (ccw.count < sizeof(thinint)) { > /* Can't execute command. */ > ret = -EINVAL; > break; > } > - len = sizeof(*thinint); > - hw_len = len; > if (!ccw.cda) { > ret = -EFAULT; > } else if (dev->indicators && !sch->thinint_active) { > /* Trigger a command reject. */ > ret = -ENOSYS; > } else { > - thinint = cpu_physical_memory_map(ccw.cda, &hw_len, 0); > - if (!thinint) { > + if (ccw_dstream_read(&sch->cds, thinint)) { > ret = -EFAULT; > } else { > - uint64_t ind_bit = ldq_be_p(&thinint->ind_bit); > + be64_to_cpus(&thinint.ind_bit); > + be64_to_cpus(&thinint.summary_indicator); > + be64_to_cpus(&thinint.device_indicator); > > - len = hw_len; > dev->summary_indicator = > - get_indicator(ldq_be_p(&thinint->summary_indicator), > - sizeof(uint8_t)); > + get_indicator(thinint.summary_indicator, sizeof(uint8_t)); > dev->indicators = > - get_indicator(ldq_be_p(&thinint->device_indicator), > - ind_bit / 8 + 1); > - dev->thinint_isc = thinint->isc; > - dev->routes.adapter.ind_offset = ind_bit; > + get_indicator(thinint.device_indicator, > + thinint.ind_bit / 8 + 1); > + dev->thinint_isc = thinint.isc; > + dev->routes.adapter.ind_offset = thinint.ind_bit; > dev->routes.adapter.summary_offset = 7; > - cpu_physical_memory_unmap(thinint, hw_len, 0, hw_len); > dev->routes.adapter.adapter_id = css_get_adapter_id( > CSS_IO_ADAPTER_VIRTIO, > dev->thinint_isc); > sch->thinint_active = ((dev->indicators != NULL) && > (dev->summary_indicator != NULL)); > - sch->curr_status.scsw.count = ccw.count - len; > + sch->curr_status.scsw.count = ccw.count - sizeof(thinint); > ret = 0; > } > } > @@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > ret = -EFAULT; > break; > } > - revinfo.revision = > - address_space_lduw_be(&address_space_memory, ccw.cda, > - MEMTXATTRS_UNSPECIFIED, NULL); > - revinfo.length = > - address_space_lduw_be(&address_space_memory, > - ccw.cda + sizeof(revinfo.revision), > - MEMTXATTRS_UNSPECIFIED, NULL); > + ccw_dstream_read_buf(&sch->cds, &revinfo, 4); > + be16_to_cpus(&revinfo.revision); > + be16_to_cpus(&revinfo.length); > if (ccw.count < len + revinfo.length || > (check_len && ccw.count > len + revinfo.length)) { > ret = -EINVAL; > Reviewed-by: Pierre Morel<pmorel@linux.vnet.ibm.com>
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 15:30:29 +0200]: > > > On 09/19/2017 11:06 AM, Cornelia Huck wrote: > > On Tue, 19 Sep 2017 11:37:30 +0800 > > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > > > >> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-13 13:50:28 +0200]: > >> > >>> Replace direct access which implicitly assumes no IDA > >>> or MIDA with the new ccw data stream interface which should > >>> cope with these transparently in the future. > >>> > >>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > >>> > >>> --- > >>> > >>> v1 --> v2: > >>> Removed todo comments on possible errno change as with > >>> https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02441.html > >>> applied it does not matter any more. > >>> > >>> Error handling: At the moment we ignore errors reported > >>> by stream ops to keep the change minimal. An add-on > >>> patch improving on this is to be expected later. > >> Add a TODO somewhere around the code as a reminder? > > > > I expect Halil to have it on his TODO list and send a patch later ;) No problem with me. > > > >> > >>> --- > >>> hw/s390x/virtio-ccw.c | 156 +++++++++++++++----------------------------------- > >>> 1 file changed, 46 insertions(+), 110 deletions(-) > >>> > >>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > >>> index b1976fdd19..a9baf6f7ab 100644 > >>> --- a/hw/s390x/virtio-ccw.c > >>> +++ b/hw/s390x/virtio-ccw.c > >> [...] > >> > >>> @@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > >>> } else { > >>> VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); > >>> > >>> - features.index = address_space_ldub(&address_space_memory, > >>> - ccw.cda > >>> - + sizeof(features.features), > >>> - MEMTXATTRS_UNSPECIFIED, > >>> - NULL); > >>> + ccw_dstream_advance(&sch->cds, sizeof(features.features)); > >> How about: > >> ccw_dstream_advance(&sch->cds, offsetof(VirtioFeatDesc, index)); > > > > I think keeping sizeof(features.features) is better: It matches the old > > code, and we really do jump over the features flag and revisit it > > later... I was thinking 'advance' to the offset of features.index, then 'read' the value for features.index seems more natural, and you do not need to care too much for what exaclty the elements are that you need to 'advance'. Not big deal for me. I could also accept the original version. > > > >> > >>> + ccw_dstream_read(&sch->cds, features.index); > >>> if (features.index == 0) { > >>> if (dev->revision >= 1) { > >>> /* Don't offer legacy features for modern devices. */ > >>> @@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > >>> /* Return zeroes if the guest supports more feature bits. */ > >>> features.features = 0; > >>> } > >>> - address_space_stl_le(&address_space_memory, ccw.cda, > >>> - features.features, MEMTXATTRS_UNSPECIFIED, > >>> - NULL); > >>> + ccw_dstream_rewind(&sch->cds); > >>> + cpu_to_le32s(&features.features); > >>> + ccw_dstream_write(&sch->cds, features.features); > >>> sch->curr_status.scsw.count = ccw.count - sizeof(features); > >> How about: > >> sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds); > > > > Hm, I thought I had commented on this, but I seem to have missed > > these... > > > > I'd prefer to do it as a follow-up patch. No problem. > > > >> > >> This also applies to other places. > >> > >>> ret = 0; > >>> } > >> [...] > >> > >>> @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > >>> } > >>> } > >>> len = MIN(ccw.count, vdev->config_len); > >>> - hw_len = len; > >>> if (!ccw.cda) { > >>> ret = -EFAULT; > >>> } else { > >>> - config = cpu_physical_memory_map(ccw.cda, &hw_len, 0); > >>> - if (!config) { > >>> - ret = -EFAULT; > >>> - } else { > >>> - len = hw_len; > >>> - /* XXX config space endianness */ > >>> - memcpy(vdev->config, config, len); > >>> - cpu_physical_memory_unmap(config, hw_len, 0, hw_len); > >>> + ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len); > >>> + if (!ret) { > >> Add a TODO here, and: > >> > >> if (ccw_dstream_read_buf(&sch->cds, vdev->config, len)) { > >> ret = -EFAULT; > >> } else { > >> .... > >> } > > > > I don't think that would be correct: The function will (at least > > currently) return 0 or -EINVAL, and you are now mapping any error to > > -EFAULT? (Not that it has an effect in the end: We map both to a > > channel program check as of "s390x/css: drop data-check in > > interpretation".) Ah, I missed this. Now no problem from me. > > > >> > >>> virtio_bus_set_vdev_config(&dev->bus, vdev->config); > >>> sch->curr_status.scsw.count = ccw.count - len; > >>> - ret = 0; > >>> } > >>> } > >>> break; > >> [...] > >> > >>> @@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > >>> ret = -EFAULT; > >>> break; > >>> } > >>> - revinfo.revision = > >>> - address_space_lduw_be(&address_space_memory, ccw.cda, > >>> - MEMTXATTRS_UNSPECIFIED, NULL); > >>> - revinfo.length = > >>> - address_space_lduw_be(&address_space_memory, > >>> - ccw.cda + sizeof(revinfo.revision), > >>> - MEMTXATTRS_UNSPECIFIED, NULL); > >>> + ccw_dstream_read_buf(&sch->cds, &revinfo, 4); > >> ^ > >> A magic number? O:) > > > > Hah :) > > > > We can't use sizeof(revinfo), and sizeof(revinfo.revision) + > > sizeof(revinfo.length) is a bit unwieldy. Let's keep some magic in our > > code :) No problem. This seems the best option. > > > >> > >>> + be16_to_cpus(&revinfo.revision); > >>> + be16_to_cpus(&revinfo.length); > >>> if (ccw.count < len + revinfo.length || > >>> (check_len && ccw.count > len + revinfo.length)) { > >>> ret = -EINVAL; > >>> -- > >>> 2.13.5 > >>> > >> > >> Otherwise, looks good. > >> > > > > Can I get an ack? Sure: Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> > > > > I agree that further cleanups are possible (e.g. using > ccw_dstream_residual_count() and tightening error handling) but > I also prefer to see these as on-top, and prefer sticking to > change as little as possible in the transformation patch and stay > as close as possible to what we had before in terms of behavior). Understand and agree. > > Regards, > Halil >
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index b1976fdd19..a9baf6f7ab 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -287,49 +287,19 @@ static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len, return -EFAULT; } if (is_legacy) { - linfo.queue = address_space_ldq_be(&address_space_memory, ccw.cda, - MEMTXATTRS_UNSPECIFIED, NULL); - linfo.align = address_space_ldl_be(&address_space_memory, - ccw.cda + sizeof(linfo.queue), - MEMTXATTRS_UNSPECIFIED, - NULL); - linfo.index = address_space_lduw_be(&address_space_memory, - ccw.cda + sizeof(linfo.queue) - + sizeof(linfo.align), - MEMTXATTRS_UNSPECIFIED, - NULL); - linfo.num = address_space_lduw_be(&address_space_memory, - ccw.cda + sizeof(linfo.queue) - + sizeof(linfo.align) - + sizeof(linfo.index), - MEMTXATTRS_UNSPECIFIED, - NULL); + ccw_dstream_read(&sch->cds, linfo); + be64_to_cpus(&linfo.queue); + be32_to_cpus(&linfo.align); + be16_to_cpus(&linfo.index); + be16_to_cpus(&linfo.num); ret = virtio_ccw_set_vqs(sch, NULL, &linfo); } else { - info.desc = address_space_ldq_be(&address_space_memory, ccw.cda, - MEMTXATTRS_UNSPECIFIED, NULL); - info.index = address_space_lduw_be(&address_space_memory, - ccw.cda + sizeof(info.desc) - + sizeof(info.res0), - MEMTXATTRS_UNSPECIFIED, NULL); - info.num = address_space_lduw_be(&address_space_memory, - ccw.cda + sizeof(info.desc) - + sizeof(info.res0) - + sizeof(info.index), - MEMTXATTRS_UNSPECIFIED, NULL); - info.avail = address_space_ldq_be(&address_space_memory, - ccw.cda + sizeof(info.desc) - + sizeof(info.res0) - + sizeof(info.index) - + sizeof(info.num), - MEMTXATTRS_UNSPECIFIED, NULL); - info.used = address_space_ldq_be(&address_space_memory, - ccw.cda + sizeof(info.desc) - + sizeof(info.res0) - + sizeof(info.index) - + sizeof(info.num) - + sizeof(info.avail), - MEMTXATTRS_UNSPECIFIED, NULL); + ccw_dstream_read(&sch->cds, info); + be64_to_cpus(&info.desc); + be16_to_cpus(&info.index); + be16_to_cpus(&info.num); + be64_to_cpus(&info.avail); + be64_to_cpus(&info.used); ret = virtio_ccw_set_vqs(sch, &info, NULL); } sch->curr_status.scsw.count = 0; @@ -342,15 +312,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) VirtioRevInfo revinfo; uint8_t status; VirtioFeatDesc features; - void *config; hwaddr indicators; VqConfigBlock vq_config; VirtioCcwDevice *dev = sch->driver_data; VirtIODevice *vdev = virtio_ccw_get_vdev(sch); bool check_len; int len; - hwaddr hw_len; - VirtioThinintInfo *thinint; + VirtioThinintInfo thinint; if (!dev) { return -EINVAL; @@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) } else { VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); - features.index = address_space_ldub(&address_space_memory, - ccw.cda - + sizeof(features.features), - MEMTXATTRS_UNSPECIFIED, - NULL); + ccw_dstream_advance(&sch->cds, sizeof(features.features)); + ccw_dstream_read(&sch->cds, features.index); if (features.index == 0) { if (dev->revision >= 1) { /* Don't offer legacy features for modern devices. */ @@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) /* Return zeroes if the guest supports more feature bits. */ features.features = 0; } - address_space_stl_le(&address_space_memory, ccw.cda, - features.features, MEMTXATTRS_UNSPECIFIED, - NULL); + ccw_dstream_rewind(&sch->cds); + cpu_to_le32s(&features.features); + ccw_dstream_write(&sch->cds, features.features); sch->curr_status.scsw.count = ccw.count - sizeof(features); ret = 0; } @@ -438,15 +403,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) if (!ccw.cda) { ret = -EFAULT; } else { - features.index = address_space_ldub(&address_space_memory, - ccw.cda - + sizeof(features.features), - MEMTXATTRS_UNSPECIFIED, - NULL); - features.features = address_space_ldl_le(&address_space_memory, - ccw.cda, - MEMTXATTRS_UNSPECIFIED, - NULL); + ccw_dstream_read(&sch->cds, features); + le32_to_cpus(&features.features); if (features.index == 0) { virtio_set_features(vdev, (vdev->guest_features & 0xffffffff00000000ULL) | @@ -488,7 +446,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) } else { virtio_bus_get_vdev_config(&dev->bus, vdev->config); /* XXX config space endianness */ - cpu_physical_memory_write(ccw.cda, vdev->config, len); + ccw_dstream_write_buf(&sch->cds, vdev->config, len); sch->curr_status.scsw.count = ccw.count - len; ret = 0; } @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) } } len = MIN(ccw.count, vdev->config_len); - hw_len = len; if (!ccw.cda) { ret = -EFAULT; } else { - config = cpu_physical_memory_map(ccw.cda, &hw_len, 0); - if (!config) { - ret = -EFAULT; - } else { - len = hw_len; - /* XXX config space endianness */ - memcpy(vdev->config, config, len); - cpu_physical_memory_unmap(config, hw_len, 0, hw_len); + ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len); + if (!ret) { virtio_bus_set_vdev_config(&dev->bus, vdev->config); sch->curr_status.scsw.count = ccw.count - len; - ret = 0; } } break; @@ -553,8 +503,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) if (!ccw.cda) { ret = -EFAULT; } else { - status = address_space_ldub(&address_space_memory, ccw.cda, - MEMTXATTRS_UNSPECIFIED, NULL); + ccw_dstream_read(&sch->cds, status); if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) { virtio_ccw_stop_ioeventfd(dev); } @@ -597,8 +546,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) if (!ccw.cda) { ret = -EFAULT; } else { - indicators = address_space_ldq_be(&address_space_memory, ccw.cda, - MEMTXATTRS_UNSPECIFIED, NULL); + ccw_dstream_read(&sch->cds, indicators); + be64_to_cpus(&indicators); dev->indicators = get_indicator(indicators, sizeof(uint64_t)); sch->curr_status.scsw.count = ccw.count - sizeof(indicators); ret = 0; @@ -618,8 +567,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) if (!ccw.cda) { ret = -EFAULT; } else { - indicators = address_space_ldq_be(&address_space_memory, ccw.cda, - MEMTXATTRS_UNSPECIFIED, NULL); + ccw_dstream_read(&sch->cds, indicators); + be64_to_cpus(&indicators); dev->indicators2 = get_indicator(indicators, sizeof(uint64_t)); sch->curr_status.scsw.count = ccw.count - sizeof(indicators); ret = 0; @@ -639,67 +588,58 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) if (!ccw.cda) { ret = -EFAULT; } else { - vq_config.index = address_space_lduw_be(&address_space_memory, - ccw.cda, - MEMTXATTRS_UNSPECIFIED, - NULL); + ccw_dstream_read(&sch->cds, vq_config.index); + be16_to_cpus(&vq_config.index); if (vq_config.index >= VIRTIO_QUEUE_MAX) { ret = -EINVAL; break; } vq_config.num_max = virtio_queue_get_num(vdev, vq_config.index); - address_space_stw_be(&address_space_memory, - ccw.cda + sizeof(vq_config.index), - vq_config.num_max, - MEMTXATTRS_UNSPECIFIED, - NULL); + cpu_to_be16s(&vq_config.num_max); + ccw_dstream_write(&sch->cds, vq_config.num_max); sch->curr_status.scsw.count = ccw.count - sizeof(vq_config); ret = 0; } break; case CCW_CMD_SET_IND_ADAPTER: if (check_len) { - if (ccw.count != sizeof(*thinint)) { + if (ccw.count != sizeof(thinint)) { ret = -EINVAL; break; } - } else if (ccw.count < sizeof(*thinint)) { + } else if (ccw.count < sizeof(thinint)) { /* Can't execute command. */ ret = -EINVAL; break; } - len = sizeof(*thinint); - hw_len = len; if (!ccw.cda) { ret = -EFAULT; } else if (dev->indicators && !sch->thinint_active) { /* Trigger a command reject. */ ret = -ENOSYS; } else { - thinint = cpu_physical_memory_map(ccw.cda, &hw_len, 0); - if (!thinint) { + if (ccw_dstream_read(&sch->cds, thinint)) { ret = -EFAULT; } else { - uint64_t ind_bit = ldq_be_p(&thinint->ind_bit); + be64_to_cpus(&thinint.ind_bit); + be64_to_cpus(&thinint.summary_indicator); + be64_to_cpus(&thinint.device_indicator); - len = hw_len; dev->summary_indicator = - get_indicator(ldq_be_p(&thinint->summary_indicator), - sizeof(uint8_t)); + get_indicator(thinint.summary_indicator, sizeof(uint8_t)); dev->indicators = - get_indicator(ldq_be_p(&thinint->device_indicator), - ind_bit / 8 + 1); - dev->thinint_isc = thinint->isc; - dev->routes.adapter.ind_offset = ind_bit; + get_indicator(thinint.device_indicator, + thinint.ind_bit / 8 + 1); + dev->thinint_isc = thinint.isc; + dev->routes.adapter.ind_offset = thinint.ind_bit; dev->routes.adapter.summary_offset = 7; - cpu_physical_memory_unmap(thinint, hw_len, 0, hw_len); dev->routes.adapter.adapter_id = css_get_adapter_id( CSS_IO_ADAPTER_VIRTIO, dev->thinint_isc); sch->thinint_active = ((dev->indicators != NULL) && (dev->summary_indicator != NULL)); - sch->curr_status.scsw.count = ccw.count - len; + sch->curr_status.scsw.count = ccw.count - sizeof(thinint); ret = 0; } } @@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) ret = -EFAULT; break; } - revinfo.revision = - address_space_lduw_be(&address_space_memory, ccw.cda, - MEMTXATTRS_UNSPECIFIED, NULL); - revinfo.length = - address_space_lduw_be(&address_space_memory, - ccw.cda + sizeof(revinfo.revision), - MEMTXATTRS_UNSPECIFIED, NULL); + ccw_dstream_read_buf(&sch->cds, &revinfo, 4); + be16_to_cpus(&revinfo.revision); + be16_to_cpus(&revinfo.length); if (ccw.count < len + revinfo.length || (check_len && ccw.count > len + revinfo.length)) { ret = -EINVAL;
Replace direct access which implicitly assumes no IDA or MIDA with the new ccw data stream interface which should cope with these transparently in the future. Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> --- v1 --> v2: Removed todo comments on possible errno change as with https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02441.html applied it does not matter any more. Error handling: At the moment we ignore errors reported by stream ops to keep the change minimal. An add-on patch improving on this is to be expected later. --- hw/s390x/virtio-ccw.c | 156 +++++++++++++++----------------------------------- 1 file changed, 46 insertions(+), 110 deletions(-)