qemu-devel
[Top][All Lists]
Advanced

[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 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]