[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream
From: |
Dong Jia Shi |
Subject: |
Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream |
Date: |
Tue, 19 Sep 2017 11:37:30 +0800 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
* Halil Pasic <address@hidden> [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 <address@hidden>
>
> ---
>
> 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
- Re: [Qemu-devel] [PATCH v2 1/4] s390x/css: introduce css data stream, (continued)
[Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream, Halil Pasic, 2017/09/13
Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream, Pierre Morel, 2017/09/19
Re: [Qemu-devel] [PATCH v2 0/4] add CCW indirect data access support, Cornelia Huck, 2017/09/14
Re: [Qemu-devel] [PATCH v2 0/4] add CCW indirect data access support, Cornelia Huck, 2017/09/14