[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: |
Halil Pasic |
Subject: |
Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream |
Date: |
Tue, 19 Sep 2017 15:30:29 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
On 09/19/2017 11:06 AM, Cornelia Huck wrote:
> On Tue, 19 Sep 2017 11:37:30 +0800
> Dong Jia Shi <address@hidden> wrote:
>
>> * 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?
>
> 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
- 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