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

[Cornelia Huck]

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?