On 09/06/2017 02:42 PM, Cornelia Huck wrote:
> On Tue, 5 Sep 2017 13:16:43 +0200
> Halil Pasic <pa...@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 <pa...@linux.vnet.ibm.com>
>>
>> ---
>>
>> Error handling: At the moment we ignore errors reported
>> by stream ops to keep the change minimal. It might make sense
>> to change this.
>
> Do that as an add-on patch?
That was the idea.
>
>> ---
>> hw/s390x/virtio-ccw.c | 158
>> +++++++++++++++-----------------------------------
>> 1 file changed, 48 insertions(+), 110 deletions(-)
>>
>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>> index b1976fdd19..72dd129a15 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);
>
> That's a very nice contraction :)
Thanks.
>
>> 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;
>
> (...)
>
>> @@ -488,7 +446,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>> } else {
>> virtio_bus_get_vdev_config(&dev->bus, vdev->config);
>> /* XXX config space endianness */
>
> Unrelated: That should be fine, I guess?
>
>> - cpu_physical_memory_write(ccw.cda, vdev->config, len);
>> + /* TODO we may have made -EINVAL out of -EFAULT */
>
> Eek.
>
Actually I wanted to send a patch which gets rid of the -EFAULT case
in sch_handle_start_func_virtual.
IMHO a program check is more appropriate here. We did the EFAULT if
cpu_physical_memory_map failed, but we don't have that any more.
>> + ccw_dstream_write_buf(&sch->cds, vdev->config, len);
>> sch->curr_status.scsw.count = ccw.count - len;
>> ret = 0;
>> }
>
> Looks fine in general.
>
Thanks!
