From: Pierre Morel <pmorel@linux.vnet.ibm.com>
To: Halil Pasic <pasic@linux.vnet.ibm.com>,
Cornelia Huck <cohuck@redhat.com>
Cc: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v3 3/5] virtio-ccw: use ccw data stream
Date: Thu, 21 Sep 2017 11:44:15 +0200 [thread overview]
Message-ID: <de7b0939-df06-333f-28ce-cd9d3fe58eb2@linux.vnet.ibm.com> (raw)
In-Reply-To: <20170919182745.90280-4-pasic@linux.vnet.ibm.com>
On 19/09/2017 20:27, Halil Pasic 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 <pasic@linux.vnet.ibm.com>
> Reviewed-by: Pierre Morel<pmorel@linux.vnet.ibm.com>
> ---
> hw/s390x/virtio-ccw.c | 157 +++++++++++++++-----------------------------------
> 1 file changed, 46 insertions(+), 111 deletions(-)
>
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index b1976fdd19..d024f8b2d3 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);
Here again, I oversaw this.
Sorry.
> + ccw_dstream_read(&sch->cds, linfo);
Same as for patch 2/5:
Shouldn't you test the return value here?
> + be64_to_cpus(&linfo.queue);
> + be32_to_cpus(&linfo.align);
> + be16_to_cpus(&linfo.index);
> + be16_to_cpus(&linfo.num);
> 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);
and here
> + 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;
> @@ -342,15 +312,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> VirtioRevInfo revinfo;
> uint8_t status;
> VirtioFeatDesc features;
> - void *config;
> hwaddr indicators;
> VqConfigBlock vq_config;
> VirtioCcwDevice *dev = sch->driver_data;
> VirtIODevice *vdev = virtio_ccw_get_vdev(sch);
> bool check_len;
> int len;
> - hwaddr hw_len;
> - VirtioThinintInfo *thinint;
> + VirtioThinintInfo thinint;
>
> if (!dev) {
> return -EINVAL;
> @@ -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));
> + ccw_dstream_read(&sch->cds, features.index);
here again
> 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);
> ret = 0;
> }
> @@ -438,15 +403,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> if (!ccw.cda) {
> ret = -EFAULT;
> } else {
> - features.index = address_space_ldub(&address_space_memory,
> - ccw.cda
> - + sizeof(features.features),
> - MEMTXATTRS_UNSPECIFIED,
> - NULL);
> - features.features = address_space_ldl_le(&address_space_memory,
> - ccw.cda,
> - MEMTXATTRS_UNSPECIFIED,
> - NULL);
> + ccw_dstream_read(&sch->cds, features);
... and everywhere where you use the IDA stream...
Pierre
> + le32_to_cpus(&features.features);
> if (features.index == 0) {
> virtio_set_features(vdev,
> (vdev->guest_features & 0xffffffff00000000ULL) |
> @@ -487,8 +445,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> ret = -EFAULT;
> } else {
> virtio_bus_get_vdev_config(&dev->bus, vdev->config);
> - /* XXX config space endianness */
> - cpu_physical_memory_write(ccw.cda, vdev->config, len);
> + ccw_dstream_write_buf(&sch->cds, vdev->config, len);
> sch->curr_status.scsw.count = ccw.count - len;
> ret = 0;
> }
> @@ -501,21 +458,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) {
> virtio_bus_set_vdev_config(&dev->bus, vdev->config);
> sch->curr_status.scsw.count = ccw.count - len;
> - ret = 0;
> }
> }
> break;
> @@ -553,8 +502,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> if (!ccw.cda) {
> ret = -EFAULT;
> } else {
> - status = address_space_ldub(&address_space_memory, ccw.cda,
> - MEMTXATTRS_UNSPECIFIED, NULL);
> + ccw_dstream_read(&sch->cds, status);
> if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> virtio_ccw_stop_ioeventfd(dev);
> }
> @@ -597,8 +545,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> if (!ccw.cda) {
> ret = -EFAULT;
> } else {
> - indicators = address_space_ldq_be(&address_space_memory, ccw.cda,
> - MEMTXATTRS_UNSPECIFIED, NULL);
> + ccw_dstream_read(&sch->cds, indicators);
> + be64_to_cpus(&indicators);
> dev->indicators = get_indicator(indicators, sizeof(uint64_t));
> sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
> ret = 0;
> @@ -618,8 +566,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> if (!ccw.cda) {
> ret = -EFAULT;
> } else {
> - indicators = address_space_ldq_be(&address_space_memory, ccw.cda,
> - MEMTXATTRS_UNSPECIFIED, NULL);
> + ccw_dstream_read(&sch->cds, indicators);
> + be64_to_cpus(&indicators);
> dev->indicators2 = get_indicator(indicators, sizeof(uint64_t));
> sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
> ret = 0;
> @@ -639,67 +587,58 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> if (!ccw.cda) {
> ret = -EFAULT;
> } else {
> - vq_config.index = address_space_lduw_be(&address_space_memory,
> - ccw.cda,
> - MEMTXATTRS_UNSPECIFIED,
> - NULL);
> + ccw_dstream_read(&sch->cds, vq_config.index);
> + be16_to_cpus(&vq_config.index);
> if (vq_config.index >= VIRTIO_QUEUE_MAX) {
> ret = -EINVAL;
> break;
> }
> vq_config.num_max = virtio_queue_get_num(vdev,
> vq_config.index);
> - address_space_stw_be(&address_space_memory,
> - ccw.cda + sizeof(vq_config.index),
> - vq_config.num_max,
> - MEMTXATTRS_UNSPECIFIED,
> - NULL);
> + cpu_to_be16s(&vq_config.num_max);
> + ccw_dstream_write(&sch->cds, vq_config.num_max);
> sch->curr_status.scsw.count = ccw.count - sizeof(vq_config);
> ret = 0;
> }
> break;
> case CCW_CMD_SET_IND_ADAPTER:
> if (check_len) {
> - if (ccw.count != sizeof(*thinint)) {
> + if (ccw.count != sizeof(thinint)) {
> ret = -EINVAL;
> break;
> }
> - } else if (ccw.count < sizeof(*thinint)) {
> + } else if (ccw.count < sizeof(thinint)) {
> /* Can't execute command. */
> ret = -EINVAL;
> break;
> }
> - len = sizeof(*thinint);
> - hw_len = len;
> if (!ccw.cda) {
> ret = -EFAULT;
> } else if (dev->indicators && !sch->thinint_active) {
> /* Trigger a command reject. */
> ret = -ENOSYS;
> } else {
> - thinint = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
> - if (!thinint) {
> + if (ccw_dstream_read(&sch->cds, thinint)) {
> ret = -EFAULT;
> } else {
> - uint64_t ind_bit = ldq_be_p(&thinint->ind_bit);
> + be64_to_cpus(&thinint.ind_bit);
> + be64_to_cpus(&thinint.summary_indicator);
> + be64_to_cpus(&thinint.device_indicator);
>
> - len = hw_len;
> dev->summary_indicator =
> - get_indicator(ldq_be_p(&thinint->summary_indicator),
> - sizeof(uint8_t));
> + get_indicator(thinint.summary_indicator, sizeof(uint8_t));
> dev->indicators =
> - get_indicator(ldq_be_p(&thinint->device_indicator),
> - ind_bit / 8 + 1);
> - dev->thinint_isc = thinint->isc;
> - dev->routes.adapter.ind_offset = ind_bit;
> + get_indicator(thinint.device_indicator,
> + thinint.ind_bit / 8 + 1);
> + dev->thinint_isc = thinint.isc;
> + dev->routes.adapter.ind_offset = thinint.ind_bit;
> dev->routes.adapter.summary_offset = 7;
> - cpu_physical_memory_unmap(thinint, hw_len, 0, hw_len);
> dev->routes.adapter.adapter_id = css_get_adapter_id(
> CSS_IO_ADAPTER_VIRTIO,
> dev->thinint_isc);
> sch->thinint_active = ((dev->indicators != NULL) &&
> (dev->summary_indicator != NULL));
> - sch->curr_status.scsw.count = ccw.count - len;
> + sch->curr_status.scsw.count = ccw.count - sizeof(thinint);
> ret = 0;
> }
> }
> @@ -714,13 +653,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);
> + 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;
>
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
next prev parent reply other threads:[~2017-09-21 9:44 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-19 18:27 [Qemu-devel] [PATCH v3 0/5] add CCW indirect data access support Halil Pasic
2017-09-19 18:27 ` [Qemu-devel] [PATCH v3 1/5] s390x/css: introduce css data stream Halil Pasic
2017-09-20 6:44 ` Dong Jia Shi
2017-09-19 18:27 ` [Qemu-devel] [PATCH v3 2/5] s390x/css: use ccw " Halil Pasic
2017-09-21 9:40 ` Pierre Morel
2017-09-19 18:27 ` [Qemu-devel] [PATCH v3 3/5] virtio-ccw: " Halil Pasic
2017-09-20 6:47 ` Dong Jia Shi
2017-09-20 7:58 ` Cornelia Huck
2017-09-20 10:56 ` Halil Pasic
2017-09-20 10:57 ` Cornelia Huck
2017-09-21 9:44 ` Pierre Morel [this message]
2017-09-21 17:01 ` Halil Pasic
2017-09-19 18:27 ` [Qemu-devel] [PATCH v3 4/5] 390x/css: introduce maximum data address checking Halil Pasic
2017-09-20 7:47 ` Dong Jia Shi
2017-09-20 8:25 ` Cornelia Huck
2017-09-20 11:02 ` Halil Pasic
2017-09-21 0:39 ` Dong Jia Shi
2017-09-20 8:06 ` Cornelia Huck
2017-09-20 11:34 ` Halil Pasic
2017-09-20 11:43 ` Cornelia Huck
2017-09-19 18:27 ` [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA Halil Pasic
2017-09-20 7:42 ` Dong Jia Shi
2017-09-20 8:33 ` Cornelia Huck
2017-09-20 11:13 ` Halil Pasic
2017-09-20 11:18 ` Cornelia Huck
2017-09-20 16:46 ` Halil Pasic
2017-09-21 0:50 ` Dong Jia Shi
2017-09-21 7:31 ` Cornelia Huck
2017-09-21 1:10 ` Dong Jia Shi
2017-09-20 8:11 ` Cornelia Huck
2017-09-20 11:01 ` Halil Pasic
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=de7b0939-df06-333f-28ce-cd9d3fe58eb2@linux.vnet.ibm.com \
--to=pmorel@linux.vnet.ibm.com \
--cc=bjsdjshi@linux.vnet.ibm.com \
--cc=cohuck@redhat.com \
--cc=pasic@linux.vnet.ibm.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).