Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream

[Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>]

* 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.

-- 
Dong Jia Shi