* [Qemu-devel] [PATCH v3 1/5] s390x/css: introduce css data stream
2017-09-19 18:27 [Qemu-devel] [PATCH v3 0/5] add CCW indirect data access support Halil Pasic
@ 2017-09-19 18:27 ` 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
` (3 subsequent siblings)
4 siblings, 1 reply; 31+ messages in thread
From: Halil Pasic @ 2017-09-19 18:27 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel, Halil Pasic
This is a preparation for introducing handling for indirect data
addressing and modified indirect data addressing (CCW). Here we introduce
an interface which should make the addressing scheme transparent for the
client code. Here we implement only the basic scheme (no IDA or MIDA).
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
Reviewed-by: Pierre Morel<pmorel@linux.vnet.ibm.com>
---
hw/s390x/css.c | 55 +++++++++++++++++++++++++++++++++++++++++
include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 122 insertions(+)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 901dc6a0f3..e8d2016563 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -783,6 +783,61 @@ static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1)
}
return ret;
}
+/**
+ * If out of bounds marks the stream broken. If broken returns -EINVAL,
+ * otherwise the requested length (may be zero)
+ */
+static inline int cds_check_len(CcwDataStream *cds, int len)
+{
+ if (cds->at_byte + len > cds->count) {
+ cds->flags |= CDS_F_STREAM_BROKEN;
+ }
+ return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
+}
+
+static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
+ CcwDataStreamOp op)
+{
+ int ret;
+
+ ret = cds_check_len(cds, len);
+ if (ret <= 0) {
+ return ret;
+ }
+ if (op == CDS_OP_A) {
+ goto incr;
+ }
+ ret = address_space_rw(&address_space_memory, cds->cda,
+ MEMTXATTRS_UNSPECIFIED, buff, len, op);
+ if (ret != MEMTX_OK) {
+ cds->flags |= CDS_F_STREAM_BROKEN;
+ return -EINVAL;
+ }
+incr:
+ cds->at_byte += len;
+ cds->cda += len;
+ return 0;
+}
+
+void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
+{
+ /*
+ * We don't support MIDA (an optional facility) yet and we
+ * catch this earlier. Just for expressing the precondition.
+ */
+ g_assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));
+ cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) |
+ (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) |
+ (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0);
+ cds->count = ccw->count;
+ cds->cda_orig = ccw->cda;
+ ccw_dstream_rewind(cds);
+ if (!(cds->flags & CDS_F_IDA)) {
+ cds->op_handler = ccw_dstream_rw_noflags;
+ } else {
+ assert(false);
+ }
+}
static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
bool suspend_allowed)
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 0653d3c9be..078356e94c 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -75,6 +75,29 @@ typedef struct CMBE {
uint32_t reserved[7];
} QEMU_PACKED CMBE;
+typedef enum CcwDataStreamOp {
+ CDS_OP_R = 0, /* read, false when used as is_write */
+ CDS_OP_W = 1, /* write, true when used as is_write */
+ CDS_OP_A = 2 /* advance, should not be used as is_write */
+} CcwDataStreamOp;
+
+/* normal usage is via SuchchDev.cds instead of instantiating */
+typedef struct CcwDataStream {
+#define CDS_F_IDA 0x01
+#define CDS_F_MIDA 0x02
+#define CDS_F_I2K 0x04
+#define CDS_F_C64 0x08
+#define CDS_F_STREAM_BROKEN 0x80
+ uint8_t flags;
+ uint8_t at_idaw;
+ uint16_t at_byte;
+ uint16_t count;
+ uint32_t cda_orig;
+ int (*op_handler)(struct CcwDataStream *cds, void *buff, int len,
+ CcwDataStreamOp op);
+ hwaddr cda;
+} CcwDataStream;
+
typedef struct SubchDev SubchDev;
struct SubchDev {
/* channel-subsystem related things: */
@@ -92,6 +115,7 @@ struct SubchDev {
uint8_t ccw_no_data_cnt;
uint16_t migrated_schid; /* used for missmatch detection */
ORB orb;
+ CcwDataStream cds;
/* transport-provided data: */
int (*ccw_cb) (SubchDev *, CCW1);
void (*disable_cb)(SubchDev *);
@@ -240,4 +264,47 @@ SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss,
/** Turn on css migration */
void css_register_vmstate(void);
+
+void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb);
+
+static inline void ccw_dstream_rewind(CcwDataStream *cds)
+{
+ cds->at_byte = 0;
+ cds->at_idaw = 0;
+ cds->cda = cds->cda_orig;
+}
+
+static inline bool ccw_dstream_good(CcwDataStream *cds)
+{
+ return !(cds->flags & CDS_F_STREAM_BROKEN);
+}
+
+static inline uint16_t ccw_dstream_residual_count(CcwDataStream *cds)
+{
+ return cds->count - cds->at_byte;
+}
+
+static inline uint16_t ccw_dstream_avail(CcwDataStream *cds)
+{
+ return ccw_dstream_good(cds) ? ccw_dstream_residual_count(cds) : 0;
+}
+
+static inline int ccw_dstream_advance(CcwDataStream *cds, int len)
+{
+ return cds->op_handler(cds, NULL, len, CDS_OP_A);
+}
+
+static inline int ccw_dstream_write_buf(CcwDataStream *cds, void *buff, int len)
+{
+ return cds->op_handler(cds, buff, len, CDS_OP_W);
+}
+
+static inline int ccw_dstream_read_buf(CcwDataStream *cds, void *buff, int len)
+{
+ return cds->op_handler(cds, buff, len, CDS_OP_R);
+}
+
+#define ccw_dstream_read(cds, v) ccw_dstream_read_buf((cds), &(v), sizeof(v))
+#define ccw_dstream_write(cds, v) ccw_dstream_write_buf((cds), &(v), sizeof(v))
+
#endif
--
2.13.5
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 1/5] s390x/css: introduce css data stream
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
0 siblings, 0 replies; 31+ messages in thread
From: Dong Jia Shi @ 2017-09-20 6:44 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, Dong Jia Shi, Pierre Morel, qemu-devel
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:41 +0200]:
[...]
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 0653d3c9be..078356e94c 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -75,6 +75,29 @@ typedef struct CMBE {
> uint32_t reserved[7];
> } QEMU_PACKED CMBE;
>
> +typedef enum CcwDataStreamOp {
> + CDS_OP_R = 0, /* read, false when used as is_write */
> + CDS_OP_W = 1, /* write, true when used as is_write */
> + CDS_OP_A = 2 /* advance, should not be used as is_write */
Looks good to me.
> +} CcwDataStreamOp;
> +
>
[...]
--
Dong Jia Shi
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v3 2/5] s390x/css: use ccw data stream
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-19 18:27 ` Halil Pasic
2017-09-21 9:40 ` Pierre Morel
2017-09-19 18:27 ` [Qemu-devel] [PATCH v3 3/5] virtio-ccw: " Halil Pasic
` (2 subsequent siblings)
4 siblings, 1 reply; 31+ messages in thread
From: Halil Pasic @ 2017-09-19 18:27 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel, Halil Pasic
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: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
Reviewed-by: Pierre Morel<pmorel@linux.vnet.ibm.com>
---
hw/s390x/css.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index e8d2016563..6b0cd8861b 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -890,6 +890,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
}
/* Look at the command. */
+ ccw_dstream_init(&sch->cds, &ccw, &(sch->orb));
switch (ccw.cmd_code) {
case CCW_CMD_NOOP:
/* Nothing to do. */
@@ -903,8 +904,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
}
}
len = MIN(ccw.count, sizeof(sch->sense_data));
- cpu_physical_memory_write(ccw.cda, sch->sense_data, len);
- sch->curr_status.scsw.count = ccw.count - len;
+ ccw_dstream_write_buf(&sch->cds, sch->sense_data, len);
+ sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
memset(sch->sense_data, 0, sizeof(sch->sense_data));
ret = 0;
break;
@@ -930,8 +931,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
} else {
sense_id.reserved = 0;
}
- cpu_physical_memory_write(ccw.cda, &sense_id, len);
- sch->curr_status.scsw.count = ccw.count - len;
+ ccw_dstream_write_buf(&sch->cds, &sense_id, len);
+ sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
ret = 0;
break;
}
--
2.13.5
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/5] s390x/css: use ccw data stream
2017-09-19 18:27 ` [Qemu-devel] [PATCH v3 2/5] s390x/css: use ccw " Halil Pasic
@ 2017-09-21 9:40 ` Pierre Morel
0 siblings, 0 replies; 31+ messages in thread
From: Pierre Morel @ 2017-09-21 9:40 UTC (permalink / raw)
To: Halil Pasic, Cornelia Huck; +Cc: Dong Jia Shi, qemu-devel
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: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> Reviewed-by: Pierre Morel<pmorel@linux.vnet.ibm.com>
> ---
> hw/s390x/css.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index e8d2016563..6b0cd8861b 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -890,6 +890,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
> }
>
> /* Look at the command. */
> + ccw_dstream_init(&sch->cds, &ccw, &(sch->orb));
> switch (ccw.cmd_code) {
> case CCW_CMD_NOOP:
> /* Nothing to do. */
> @@ -903,8 +904,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
> }
> }
> len = MIN(ccw.count, sizeof(sch->sense_data));
> - cpu_physical_memory_write(ccw.cda, sch->sense_data, len);
> - sch->curr_status.scsw.count = ccw.count - len;
> + ccw_dstream_write_buf(&sch->cds, sch->sense_data, len);
Well, I oversaw this when I reviewed:
I think you should test the return value of ccw_dstream_write_buf here.
> + sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
> memset(sch->sense_data, 0, sizeof(sch->sense_data));
> ret = 0;
> break;
> @@ -930,8 +931,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
> } else {
> sense_id.reserved = 0;
> }
> - cpu_physical_memory_write(ccw.cda, &sense_id, len);
> - sch->curr_status.scsw.count = ccw.count - len;
> + ccw_dstream_write_buf(&sch->cds, &sense_id, len);
and here
> + sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
> ret = 0;
> break;
> }
>
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v3 3/5] virtio-ccw: use ccw data stream
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-19 18:27 ` [Qemu-devel] [PATCH v3 2/5] s390x/css: use ccw " Halil Pasic
@ 2017-09-19 18:27 ` Halil Pasic
2017-09-20 6:47 ` Dong Jia Shi
` (2 more replies)
2017-09-19 18:27 ` [Qemu-devel] [PATCH v3 4/5] 390x/css: introduce maximum data address checking Halil Pasic
2017-09-19 18:27 ` [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA Halil Pasic
4 siblings, 3 replies; 31+ messages in thread
From: Halil Pasic @ 2017-09-19 18:27 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel, Halil Pasic
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);
+ 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);
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;
@@ -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);
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);
+ 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;
--
2.13.5
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 3/5] virtio-ccw: use ccw data stream
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-21 9:44 ` Pierre Morel
2 siblings, 0 replies; 31+ messages in thread
From: Dong Jia Shi @ 2017-09-20 6:47 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, Dong Jia Shi, Pierre Morel, qemu-devel
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:43 +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 <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(-)
>
Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
[...]
--
Dong Jia Shi
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/5] virtio-ccw: use ccw data stream
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-21 9:44 ` Pierre Morel
2 siblings, 1 reply; 31+ messages in thread
From: Cornelia Huck @ 2017-09-20 7:58 UTC (permalink / raw)
To: Halil Pasic; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel
On Tue, 19 Sep 2017 20:27:43 +0200
Halil Pasic <pasic@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 <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(-)
>
> @@ -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);
Hm, didn't you want to convert these as well? Or do you plan to do it
as an add-on patch?
> ret = 0;
> }
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/5] virtio-ccw: use ccw data stream
2017-09-20 7:58 ` Cornelia Huck
@ 2017-09-20 10:56 ` Halil Pasic
2017-09-20 10:57 ` Cornelia Huck
0 siblings, 1 reply; 31+ messages in thread
From: Halil Pasic @ 2017-09-20 10:56 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel
On 09/20/2017 09:58 AM, Cornelia Huck wrote:
> On Tue, 19 Sep 2017 20:27:43 +0200
> Halil Pasic <pasic@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 <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(-)
>>
>
>> @@ -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);
>
> Hm, didn't you want to convert these as well? Or do you plan to do it
> as an add-on patch?
>
I plan to do it as an add on patch.
>> ret = 0;
>> }
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/5] virtio-ccw: use ccw data stream
2017-09-20 10:56 ` Halil Pasic
@ 2017-09-20 10:57 ` Cornelia Huck
0 siblings, 0 replies; 31+ messages in thread
From: Cornelia Huck @ 2017-09-20 10:57 UTC (permalink / raw)
To: Halil Pasic; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel
On Wed, 20 Sep 2017 12:56:16 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> On 09/20/2017 09:58 AM, Cornelia Huck wrote:
> > On Tue, 19 Sep 2017 20:27:43 +0200
> > Halil Pasic <pasic@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 <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(-)
> >>
> >
> >> @@ -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);
> >
> > Hm, didn't you want to convert these as well? Or do you plan to do it
> > as an add-on patch?
> >
>
> I plan to do it as an add on patch.
OK, fine with me as well.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/5] virtio-ccw: use ccw data stream
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-21 9:44 ` Pierre Morel
2017-09-21 17:01 ` Halil Pasic
2 siblings, 1 reply; 31+ messages in thread
From: Pierre Morel @ 2017-09-21 9:44 UTC (permalink / raw)
To: Halil Pasic, Cornelia Huck; +Cc: Dong Jia Shi, qemu-devel
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
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 3/5] virtio-ccw: use ccw data stream
2017-09-21 9:44 ` Pierre Morel
@ 2017-09-21 17:01 ` Halil Pasic
0 siblings, 0 replies; 31+ messages in thread
From: Halil Pasic @ 2017-09-21 17:01 UTC (permalink / raw)
To: Pierre Morel, Cornelia Huck; +Cc: Dong Jia Shi, qemu-devel
On 09/21/2017 11:44 AM, Pierre Morel wrote:
> 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?
>
It's keep the change minimal policy. Later we can discuss
tightening the checks. For now I don't want to introduce
any new ones.
Halil
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v3 4/5] 390x/css: introduce maximum data address checking
2017-09-19 18:27 [Qemu-devel] [PATCH v3 0/5] add CCW indirect data access support Halil Pasic
` (2 preceding siblings ...)
2017-09-19 18:27 ` [Qemu-devel] [PATCH v3 3/5] virtio-ccw: " Halil Pasic
@ 2017-09-19 18:27 ` Halil Pasic
2017-09-20 7:47 ` Dong Jia Shi
2017-09-20 8:06 ` Cornelia Huck
2017-09-19 18:27 ` [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA Halil Pasic
4 siblings, 2 replies; 31+ messages in thread
From: Halil Pasic @ 2017-09-19 18:27 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel, Halil Pasic
The architecture mandates the addresses to be accessed on the first
indirection level (that is, the data addresses without IDA, and the
(M)IDAW addresses with (M)IDA) to be checked against an CCW format
dependent limit maximum address. If a violation is detected, the storage
access is not to be performed and a channel program check needs to be
generated. As of today, we fail to do this check.
Let us stick even closer to the architecture specification.
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
hw/s390x/css.c | 10 ++++++++++
include/hw/s390x/css.h | 1 +
2 files changed, 11 insertions(+)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 6b0cd8861b..2d37a9ddde 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -795,6 +795,11 @@ static inline int cds_check_len(CcwDataStream *cds, int len)
return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
}
+static inline bool cds_ccw_addrs_ok(hwaddr addr, int len, bool ccw_fmt1)
+{
+ return (addr + len) < (ccw_fmt1 ? (1UL << 31) : (1UL << 24));
+}
+
static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
CcwDataStreamOp op)
{
@@ -804,6 +809,9 @@ static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
if (ret <= 0) {
return ret;
}
+ if (!cds_ccw_addrs_ok(cds->cda, len, cds->flags & CDS_F_FMT)) {
+ return -EINVAL; /* channel program check */
+ }
if (op == CDS_OP_A) {
goto incr;
}
@@ -828,7 +836,9 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
g_assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));
cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) |
(orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) |
+ (orb->ctrl0 & ORB_CTRL0_MASK_FMT ? CDS_F_FMT : 0) |
(ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0);
+
cds->count = ccw->count;
cds->cda_orig = ccw->cda;
ccw_dstream_rewind(cds);
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 078356e94c..69b374730e 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -87,6 +87,7 @@ typedef struct CcwDataStream {
#define CDS_F_MIDA 0x02
#define CDS_F_I2K 0x04
#define CDS_F_C64 0x08
+#define CDS_F_FMT 0x10 /* CCW format-1 */
#define CDS_F_STREAM_BROKEN 0x80
uint8_t flags;
uint8_t at_idaw;
--
2.13.5
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 4/5] 390x/css: introduce maximum data address checking
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 8:06 ` Cornelia Huck
1 sibling, 1 reply; 31+ messages in thread
From: Dong Jia Shi @ 2017-09-20 7:47 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, Dong Jia Shi, Pierre Morel, qemu-devel
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:44 +0200]:
> The architecture mandates the addresses to be accessed on the first
> indirection level (that is, the data addresses without IDA, and the
> (M)IDAW addresses with (M)IDA) to be checked against an CCW format
> dependent limit maximum address. If a violation is detected, the storage
> access is not to be performed and a channel program check needs to be
> generated. As of today, we fail to do this check.
>
> Let us stick even closer to the architecture specification.
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> hw/s390x/css.c | 10 ++++++++++
> include/hw/s390x/css.h | 1 +
> 2 files changed, 11 insertions(+)
>
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 6b0cd8861b..2d37a9ddde 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -795,6 +795,11 @@ static inline int cds_check_len(CcwDataStream *cds, int len)
> return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
> }
>
> +static inline bool cds_ccw_addrs_ok(hwaddr addr, int len, bool ccw_fmt1)
> +{
> + return (addr + len) < (ccw_fmt1 ? (1UL << 31) : (1UL << 24));
> +}
> +
> static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
> CcwDataStreamOp op)
> {
> @@ -804,6 +809,9 @@ static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
> if (ret <= 0) {
> return ret;
> }
> + if (!cds_ccw_addrs_ok(cds->cda, len, cds->flags & CDS_F_FMT)) {
> + return -EINVAL; /* channel program check */
> + }
> if (op == CDS_OP_A) {
> goto incr;
> }
> @@ -828,7 +836,9 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
> g_assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));
> cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) |
> (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) |
> + (orb->ctrl0 & ORB_CTRL0_MASK_FMT ? CDS_F_FMT : 0) |
This reminds me one more question:
Calling ccw_dsteram_init() after copy_ccw_from_guest() may lead to a
fmt-1 @ccw with an @orb that designates fmt-0 ccw. This sounds insane.
> (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0);
> +
> cds->count = ccw->count;
> cds->cda_orig = ccw->cda;
> ccw_dstream_rewind(cds);
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 078356e94c..69b374730e 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -87,6 +87,7 @@ typedef struct CcwDataStream {
> #define CDS_F_MIDA 0x02
> #define CDS_F_I2K 0x04
> #define CDS_F_C64 0x08
> +#define CDS_F_FMT 0x10 /* CCW format-1 */
> #define CDS_F_STREAM_BROKEN 0x80
> uint8_t flags;
> uint8_t at_idaw;
> --
> 2.13.5
>
--
Dong Jia Shi
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 4/5] 390x/css: introduce maximum data address checking
2017-09-20 7:47 ` Dong Jia Shi
@ 2017-09-20 8:25 ` Cornelia Huck
2017-09-20 11:02 ` Halil Pasic
0 siblings, 1 reply; 31+ messages in thread
From: Cornelia Huck @ 2017-09-20 8:25 UTC (permalink / raw)
To: Dong Jia Shi; +Cc: Halil Pasic, Pierre Morel, qemu-devel
On Wed, 20 Sep 2017 15:47:51 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:44 +0200]:
> > @@ -828,7 +836,9 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
> > g_assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));
> > cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) |
> > (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) |
> > + (orb->ctrl0 & ORB_CTRL0_MASK_FMT ? CDS_F_FMT : 0) |
> This reminds me one more question:
> Calling ccw_dsteram_init() after copy_ccw_from_guest() may lead to a
> fmt-1 @ccw with an @orb that designates fmt-0 ccw. This sounds insane.
That's just a consequence of us translating everything to format-1
ccws. A bit confusing, but no problem if we pay attention to the format
bit everywhere it makes a difference.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/5] 390x/css: introduce maximum data address checking
2017-09-20 8:25 ` Cornelia Huck
@ 2017-09-20 11:02 ` Halil Pasic
2017-09-21 0:39 ` Dong Jia Shi
0 siblings, 1 reply; 31+ messages in thread
From: Halil Pasic @ 2017-09-20 11:02 UTC (permalink / raw)
To: Cornelia Huck, Dong Jia Shi; +Cc: Pierre Morel, qemu-devel
On 09/20/2017 10:25 AM, Cornelia Huck wrote:
> On Wed, 20 Sep 2017 15:47:51 +0800
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
>
>> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:44 +0200]:
>
>>> @@ -828,7 +836,9 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>>> g_assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));
>>> cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) |
>>> (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) |
>>> + (orb->ctrl0 & ORB_CTRL0_MASK_FMT ? CDS_F_FMT : 0) |
>> This reminds me one more question:
>> Calling ccw_dsteram_init() after copy_ccw_from_guest() may lead to a
>> fmt-1 @ccw with an @orb that designates fmt-0 ccw. This sounds insane.
>
> That's just a consequence of us translating everything to format-1
> ccws. A bit confusing, but no problem if we pay attention to the format
> bit everywhere it makes a difference.
>
Agree.
Halil
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/5] 390x/css: introduce maximum data address checking
2017-09-20 11:02 ` Halil Pasic
@ 2017-09-21 0:39 ` Dong Jia Shi
0 siblings, 0 replies; 31+ messages in thread
From: Dong Jia Shi @ 2017-09-21 0:39 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, Dong Jia Shi, Pierre Morel, qemu-devel
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-20 13:02:59 +0200]:
>
>
> On 09/20/2017 10:25 AM, Cornelia Huck wrote:
> > On Wed, 20 Sep 2017 15:47:51 +0800
> > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> >
> >> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:44 +0200]:
> >
> >>> @@ -828,7 +836,9 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
> >>> g_assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));
> >>> cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) |
> >>> (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) |
> >>> + (orb->ctrl0 & ORB_CTRL0_MASK_FMT ? CDS_F_FMT : 0) |
> >> This reminds me one more question:
> >> Calling ccw_dsteram_init() after copy_ccw_from_guest() may lead to a
> >> fmt-1 @ccw with an @orb that designates fmt-0 ccw. This sounds insane.
> >
> > That's just a consequence of us translating everything to format-1
> > ccws. A bit confusing, but no problem if we pay attention to the format
> > bit everywhere it makes a difference.
> >
>
> Agree.
Ok. I'm fine with this.
>
> Halil
--
Dong Jia Shi
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/5] 390x/css: introduce maximum data address checking
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:06 ` Cornelia Huck
2017-09-20 11:34 ` Halil Pasic
1 sibling, 1 reply; 31+ messages in thread
From: Cornelia Huck @ 2017-09-20 8:06 UTC (permalink / raw)
To: Halil Pasic; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel
On Tue, 19 Sep 2017 20:27:44 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> The architecture mandates the addresses to be accessed on the first
> indirection level (that is, the data addresses without IDA, and the
> (M)IDAW addresses with (M)IDA) to be checked against an CCW format
> dependent limit maximum address. If a violation is detected, the storage
> access is not to be performed and a channel program check needs to be
> generated. As of today, we fail to do this check.
>
> Let us stick even closer to the architecture specification.
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> hw/s390x/css.c | 10 ++++++++++
> include/hw/s390x/css.h | 1 +
> 2 files changed, 11 insertions(+)
>
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 6b0cd8861b..2d37a9ddde 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -795,6 +795,11 @@ static inline int cds_check_len(CcwDataStream *cds, int len)
> return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
> }
>
> +static inline bool cds_ccw_addrs_ok(hwaddr addr, int len, bool ccw_fmt1)
cds_cda_limit_ok?
> +{
> + return (addr + len) < (ccw_fmt1 ? (1UL << 31) : (1UL << 24));
> +}
> +
> static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
> CcwDataStreamOp op)
> {
Looks good.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 4/5] 390x/css: introduce maximum data address checking
2017-09-20 8:06 ` Cornelia Huck
@ 2017-09-20 11:34 ` Halil Pasic
2017-09-20 11:43 ` Cornelia Huck
0 siblings, 1 reply; 31+ messages in thread
From: Halil Pasic @ 2017-09-20 11:34 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel
On 09/20/2017 10:06 AM, Cornelia Huck wrote:
> On Tue, 19 Sep 2017 20:27:44 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>
>> The architecture mandates the addresses to be accessed on the first
>> indirection level (that is, the data addresses without IDA, and the
>> (M)IDAW addresses with (M)IDA) to be checked against an CCW format
>> dependent limit maximum address. If a violation is detected, the storage
>> access is not to be performed and a channel program check needs to be
>> generated. As of today, we fail to do this check.
>>
>> Let us stick even closer to the architecture specification.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>> hw/s390x/css.c | 10 ++++++++++
>> include/hw/s390x/css.h | 1 +
>> 2 files changed, 11 insertions(+)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 6b0cd8861b..2d37a9ddde 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -795,6 +795,11 @@ static inline int cds_check_len(CcwDataStream *cds, int len)
>> return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
>> }
>>
>> +static inline bool cds_ccw_addrs_ok(hwaddr addr, int len, bool ccw_fmt1)
>
> cds_cda_limit_ok?
>
I use cda to point to the 2 level in case of IDA. This is about
level 1 (addressed by the ccw directly). That's why I used ccw_addrs
but if you think cds_cda_limit_ok is better I can live with that.
We could also think about renaming cds->cda. Btw what does cda stand
for (channel data address is my guess)?
Regards,
Halil
>> +{
>> + return (addr + len) < (ccw_fmt1 ? (1UL << 31) : (1UL << 24));
>> +}
>> +
>> static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
>> CcwDataStreamOp op)
>> {
>
> Looks good.
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 4/5] 390x/css: introduce maximum data address checking
2017-09-20 11:34 ` Halil Pasic
@ 2017-09-20 11:43 ` Cornelia Huck
0 siblings, 0 replies; 31+ messages in thread
From: Cornelia Huck @ 2017-09-20 11:43 UTC (permalink / raw)
To: Halil Pasic; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel
On Wed, 20 Sep 2017 13:34:21 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> On 09/20/2017 10:06 AM, Cornelia Huck wrote:
> > On Tue, 19 Sep 2017 20:27:44 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >
> >> The architecture mandates the addresses to be accessed on the first
> >> indirection level (that is, the data addresses without IDA, and the
> >> (M)IDAW addresses with (M)IDA) to be checked against an CCW format
> >> dependent limit maximum address. If a violation is detected, the storage
> >> access is not to be performed and a channel program check needs to be
> >> generated. As of today, we fail to do this check.
> >>
> >> Let us stick even closer to the architecture specification.
> >>
> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >> ---
> >> hw/s390x/css.c | 10 ++++++++++
> >> include/hw/s390x/css.h | 1 +
> >> 2 files changed, 11 insertions(+)
> >>
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index 6b0cd8861b..2d37a9ddde 100644
> >> --- a/hw/s390x/css.c
> >> +++ b/hw/s390x/css.c
> >> @@ -795,6 +795,11 @@ static inline int cds_check_len(CcwDataStream *cds, int len)
> >> return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
> >> }
> >>
> >> +static inline bool cds_ccw_addrs_ok(hwaddr addr, int len, bool ccw_fmt1)
> >
> > cds_cda_limit_ok?
> >
>
> I use cda to point to the 2 level in case of IDA. This is about
> level 1 (addressed by the ccw directly). That's why I used ccw_addrs
> but if you think cds_cda_limit_ok is better I can live with that.
I don't care that much, tbh.
>
> We could also think about renaming cds->cda. Btw what does cda stand
> for (channel data address is my guess)?
Yes, cda should stand for 'channel data address'. Its usage in cds->cda
is probably the source of this minor confusion.
But, as said, I don't really care that much; so unless one of the other
folks has a strong opinion, feel free to leave as-is.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA
2017-09-19 18:27 [Qemu-devel] [PATCH v3 0/5] add CCW indirect data access support Halil Pasic
` (3 preceding siblings ...)
2017-09-19 18:27 ` [Qemu-devel] [PATCH v3 4/5] 390x/css: introduce maximum data address checking Halil Pasic
@ 2017-09-19 18:27 ` Halil Pasic
2017-09-20 7:42 ` Dong Jia Shi
2017-09-20 8:11 ` Cornelia Huck
4 siblings, 2 replies; 31+ messages in thread
From: Halil Pasic @ 2017-09-19 18:27 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel, Halil Pasic
Let's add indirect data addressing support for our virtual channel
subsystem. This implementation does not bother with any kind of
prefetching. We simply step through the IDAL on demand.
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 116 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 2d37a9ddde..a3ce6d89b6 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -827,6 +827,121 @@ incr:
return 0;
}
+/* returns values between 1 and bsz, where bsz is a power of 2 */
+static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
+{
+ return bsz - (cda & (bsz - 1));
+}
+
+static inline uint64_t ccw_ida_block_size(uint8_t flags)
+{
+ if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
+ return 1ULL << 12;
+ }
+ return 1ULL << 11;
+}
+
+static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1,
+ bool idaw_fmt_2)
+{
+ union {uint64_t fmt2; uint32_t fmt1; } idaw;
+ int ret;
+ hwaddr idaw_addr;
+
+ if (idaw_fmt_2) {
+ idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
+ if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
+ return -EINVAL; /* channel program check */
+ }
+ ret = address_space_rw(&address_space_memory, idaw_addr,
+ MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
+ sizeof(idaw.fmt2), false);
+ cds->cda = be64_to_cpu(idaw.fmt2);
+ } else {
+ idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
+ if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
+ return -EINVAL; /* channel program check */
+ }
+ ret = address_space_rw(&address_space_memory, idaw_addr,
+ MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
+ sizeof(idaw.fmt1), false);
+ cds->cda = be64_to_cpu(idaw.fmt1);
+ }
+ ++(cds->at_idaw);
+ if (ret != MEMTX_OK) {
+ /* assume inaccessible address */
+ return -EINVAL; /* channel program check */
+
+ }
+ return 0;
+}
+
+static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
+ CcwDataStreamOp op)
+{
+ uint64_t bsz = ccw_ida_block_size(cds->flags);
+ int ret = 0;
+ uint16_t cont_left, iter_len;
+ const bool idaw_fmt2 = cds->flags & CDS_F_C64;
+ bool ccw_fmt1 = cds->flags & CDS_F_FMT;
+
+ ret = cds_check_len(cds, len);
+ if (ret <= 0) {
+ return ret;
+ }
+ if (!cds->at_idaw) {
+ /* read first idaw */
+ ret = ida_read_next_idaw(cds, ccw_fmt1, idaw_fmt2);
+ if (ret) {
+ goto err;
+ }
+ cont_left = ida_continuous_left(cds->cda, bsz);
+ } else {
+ cont_left = ida_continuous_left(cds->cda, bsz);
+ if (cont_left == bsz) {
+ ret = ida_read_next_idaw(cds, ccw_fmt1, idaw_fmt2);
+ if (ret) {
+ goto err;
+ }
+ if (cds->cda & (bsz - 1)) {
+ ret = -EINVAL; /* channel program check */
+ goto err;
+ }
+ }
+ }
+ do {
+ iter_len = MIN(len, cont_left);
+ if (!idaw_fmt2 && (cds->cda + iter_len) >= (1ULL << 31)) {
+ ret = -EINVAL; /* channel program check */
+ goto err;
+ }
+ if (op != CDS_OP_A) {
+ ret = address_space_rw(&address_space_memory, cds->cda,
+ MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);
+ if (ret != MEMTX_OK) {
+ /* assume inaccessible address */
+ ret = -EINVAL; /* channel program check */
+ goto err;
+ }
+ }
+ cds->at_byte += iter_len;
+ cds->cda += iter_len;
+ len -= iter_len;
+ if (!len) {
+ break;
+ }
+ ret = ida_read_next_idaw(cds, ccw_fmt1, idaw_fmt2);
+ if (ret) {
+ goto err;
+ }
+ cont_left = bsz;
+ } while (true);
+ return ret;
+err:
+ cds->flags |= CDS_F_STREAM_BROKEN;
+ return ret;
+}
+
void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
{
/*
@@ -845,7 +960,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
if (!(cds->flags & CDS_F_IDA)) {
cds->op_handler = ccw_dstream_rw_noflags;
} else {
- assert(false);
+ cds->op_handler = ccw_dstream_rw_ida;
}
}
--
2.13.5
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA
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 8:11 ` Cornelia Huck
1 sibling, 1 reply; 31+ messages in thread
From: Dong Jia Shi @ 2017-09-20 7:42 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, Dong Jia Shi, Pierre Morel, qemu-devel
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:45 +0200]:
> Let's add indirect data addressing support for our virtual channel
> subsystem. This implementation does not bother with any kind of
> prefetching. We simply step through the IDAL on demand.
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 116 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 2d37a9ddde..a3ce6d89b6 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -827,6 +827,121 @@ incr:
> return 0;
> }
>
> +/* returns values between 1 and bsz, where bsz is a power of 2 */
> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
> +{
> + return bsz - (cda & (bsz - 1));
> +}
> +
> +static inline uint64_t ccw_ida_block_size(uint8_t flags)
> +{
> + if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
> + return 1ULL << 12;
> + }
> + return 1ULL << 11;
> +}
> +
> +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1,
> + bool idaw_fmt_2)
> +{
> + union {uint64_t fmt2; uint32_t fmt1; } idaw;
> + int ret;
> + hwaddr idaw_addr;
> +
> + if (idaw_fmt_2) {
> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
> + if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> + return -EINVAL; /* channel program check */
> + }
> + ret = address_space_rw(&address_space_memory, idaw_addr,
Ahh, just got one question here:
Do we need to considerate endianess for idaw_addr?
> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> + sizeof(idaw.fmt2), false);
> + cds->cda = be64_to_cpu(idaw.fmt2);
> + } else {
> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> + return -EINVAL; /* channel program check */
> + }
> + ret = address_space_rw(&address_space_memory, idaw_addr,
> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> + sizeof(idaw.fmt1), false);
> + cds->cda = be64_to_cpu(idaw.fmt1);
Still need to check bit 0x80000000 here I think.
> + }
> + ++(cds->at_idaw);
> + if (ret != MEMTX_OK) {
> + /* assume inaccessible address */
> + return -EINVAL; /* channel program check */
> +
Extra line.
> + }
> + return 0;
> +}
> +
> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
> + CcwDataStreamOp op)
> +{
> + uint64_t bsz = ccw_ida_block_size(cds->flags);
> + int ret = 0;
> + uint16_t cont_left, iter_len;
> + const bool idaw_fmt2 = cds->flags & CDS_F_C64;
> + bool ccw_fmt1 = cds->flags & CDS_F_FMT;
Use 'const bool' either? Although I doubt the value of using const here.
;)
> +
> + ret = cds_check_len(cds, len);
> + if (ret <= 0) {
> + return ret;
> + }
[...]
--
Dong Jia Shi
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA
2017-09-20 7:42 ` Dong Jia Shi
@ 2017-09-20 8:33 ` Cornelia Huck
2017-09-20 11:13 ` Halil Pasic
0 siblings, 1 reply; 31+ messages in thread
From: Cornelia Huck @ 2017-09-20 8:33 UTC (permalink / raw)
To: Dong Jia Shi; +Cc: Halil Pasic, Pierre Morel, qemu-devel
On Wed, 20 Sep 2017 15:42:38 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:45 +0200]:
>
> > Let's add indirect data addressing support for our virtual channel
> > subsystem. This implementation does not bother with any kind of
> > prefetching. We simply step through the IDAL on demand.
> >
> > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> > hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 116 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> > index 2d37a9ddde..a3ce6d89b6 100644
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -827,6 +827,121 @@ incr:
> > return 0;
> > }
> >
> > +/* returns values between 1 and bsz, where bsz is a power of 2 */
> > +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
> > +{
> > + return bsz - (cda & (bsz - 1));
> > +}
> > +
> > +static inline uint64_t ccw_ida_block_size(uint8_t flags)
> > +{
> > + if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
> > + return 1ULL << 12;
> > + }
> > + return 1ULL << 11;
> > +}
> > +
> > +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1,
> > + bool idaw_fmt_2)
> > +{
> > + union {uint64_t fmt2; uint32_t fmt1; } idaw;
> > + int ret;
> > + hwaddr idaw_addr;
> > +
> > + if (idaw_fmt_2) {
> > + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
> > + if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> > + return -EINVAL; /* channel program check */
> > + }
> > + ret = address_space_rw(&address_space_memory, idaw_addr,
> Ahh, just got one question here:
> Do we need to considerate endianess for idaw_addr?
That is taken care of below.
And the previous version worked on my laptop via tcg ;)
>
> > + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> > + sizeof(idaw.fmt2), false);
> > + cds->cda = be64_to_cpu(idaw.fmt2);
> > + } else {
> > + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> > + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> > + return -EINVAL; /* channel program check */
> > + }
> > + ret = address_space_rw(&address_space_memory, idaw_addr,
> > + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> > + sizeof(idaw.fmt1), false);
> > + cds->cda = be64_to_cpu(idaw.fmt1);
> Still need to check bit 0x80000000 here I think.
Yes, I think this is 'must be zero' for format-1 idaws, and not covered
by the ccw-format specific checks above. (Although the PoP can be a bit
confusing with many similar terms...)
>
> > + }
> > + ++(cds->at_idaw);
> > + if (ret != MEMTX_OK) {
> > + /* assume inaccessible address */
> > + return -EINVAL; /* channel program check */
> > +
> Extra line.
>
> > + }
> > + return 0;
> > +}
> > +
> > +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
> > + CcwDataStreamOp op)
> > +{
> > + uint64_t bsz = ccw_ida_block_size(cds->flags);
> > + int ret = 0;
> > + uint16_t cont_left, iter_len;
> > + const bool idaw_fmt2 = cds->flags & CDS_F_C64;
> > + bool ccw_fmt1 = cds->flags & CDS_F_FMT;
> Use 'const bool' either? Although I doubt the value of using const here.
> ;)
Both being the same is still a good idea.
>
> > +
> > + ret = cds_check_len(cds, len);
> > + if (ret <= 0) {
> > + return ret;
> > + }
>
> [...]
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA
2017-09-20 8:33 ` Cornelia Huck
@ 2017-09-20 11:13 ` Halil Pasic
2017-09-20 11:18 ` Cornelia Huck
2017-09-21 1:10 ` Dong Jia Shi
0 siblings, 2 replies; 31+ messages in thread
From: Halil Pasic @ 2017-09-20 11:13 UTC (permalink / raw)
To: Cornelia Huck, Dong Jia Shi; +Cc: Pierre Morel, qemu-devel
On 09/20/2017 10:33 AM, Cornelia Huck wrote:
> On Wed, 20 Sep 2017 15:42:38 +0800
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
>
>> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:45 +0200]:
>>
>>> Let's add indirect data addressing support for our virtual channel
>>> subsystem. This implementation does not bother with any kind of
>>> prefetching. We simply step through the IDAL on demand.
>>>
>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>> hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 116 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>> index 2d37a9ddde..a3ce6d89b6 100644
>>> --- a/hw/s390x/css.c
>>> +++ b/hw/s390x/css.c
>>> @@ -827,6 +827,121 @@ incr:
>>> return 0;
>>> }
>>>
>>> +/* returns values between 1 and bsz, where bsz is a power of 2 */
>>> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
>>> +{
>>> + return bsz - (cda & (bsz - 1));
>>> +}
>>> +
>>> +static inline uint64_t ccw_ida_block_size(uint8_t flags)
>>> +{
>>> + if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
>>> + return 1ULL << 12;
>>> + }
>>> + return 1ULL << 11;
>>> +}
>>> +
>>> +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1,
>>> + bool idaw_fmt_2)
>>> +{
>>> + union {uint64_t fmt2; uint32_t fmt1; } idaw;
>>> + int ret;
>>> + hwaddr idaw_addr;
>>> +
>>> + if (idaw_fmt_2) {
>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
>>> + if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
>>> + return -EINVAL; /* channel program check */
>>> + }
>>> + ret = address_space_rw(&address_space_memory, idaw_addr,
>> Ahh, just got one question here:
>> Do we need to considerate endianess for idaw_addr?
>
> That is taken care of below.
>
> And the previous version worked on my laptop via tcg ;)
Nod.
>
>>
>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
>>> + sizeof(idaw.fmt2), false);
>>> + cds->cda = be64_to_cpu(idaw.fmt2);
>>> + } else {
>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
>>> + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
>>> + return -EINVAL; /* channel program check */
>>> + }
>>> + ret = address_space_rw(&address_space_memory, idaw_addr,
>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
>>> + sizeof(idaw.fmt1), false);
>>> + cds->cda = be64_to_cpu(idaw.fmt1);
>> Still need to check bit 0x80000000 here I think.
>
> Yes, I think this is 'must be zero' for format-1 idaws, and not covered
> by the ccw-format specific checks above. (Although the PoP can be a bit
> confusing with many similar terms...)
>
It's taken care of in ccw_dstream_rw_ida before the actual
access happens. Code looks like this:
+ if (!idaw_fmt2 && (cds->cda + iter_len) >= (1ULL << 31)) {
+ ret = -EINVAL; /* channel program check */
+ goto err;
+ }
The idea was to have it similar to the non-indirect case.
>>
>>> + }
>>> + ++(cds->at_idaw);
>>> + if (ret != MEMTX_OK) {
>>> + /* assume inaccessible address */
>>> + return -EINVAL; /* channel program check */
>>> +
>> Extra line.
>>
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
>>> + CcwDataStreamOp op)
>>> +{
>>> + uint64_t bsz = ccw_ida_block_size(cds->flags);
>>> + int ret = 0;
>>> + uint16_t cont_left, iter_len;
>>> + const bool idaw_fmt2 = cds->flags & CDS_F_C64;
>>> + bool ccw_fmt1 = cds->flags & CDS_F_FMT;
>> Use 'const bool' either? Although I doubt the value of using const here.
>> ;)
>
> Both being the same is still a good idea.
>
Yeah. For which one should I go (with const or without)?
>>
>>> +
>>> + ret = cds_check_len(cds, len);
>>> + if (ret <= 0) {
>>> + return ret;
>>> + }
>>
>> [...]
>>
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA
2017-09-20 11:13 ` Halil Pasic
@ 2017-09-20 11:18 ` Cornelia Huck
2017-09-20 16:46 ` Halil Pasic
2017-09-21 1:10 ` Dong Jia Shi
1 sibling, 1 reply; 31+ messages in thread
From: Cornelia Huck @ 2017-09-20 11:18 UTC (permalink / raw)
To: Halil Pasic; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel
On Wed, 20 Sep 2017 13:13:01 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> On 09/20/2017 10:33 AM, Cornelia Huck wrote:
> > On Wed, 20 Sep 2017 15:42:38 +0800
> > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> >
> >> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:45 +0200]:
> >>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> >>> + sizeof(idaw.fmt2), false);
> >>> + cds->cda = be64_to_cpu(idaw.fmt2);
> >>> + } else {
> >>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> >>> + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> >>> + return -EINVAL; /* channel program check */
> >>> + }
> >>> + ret = address_space_rw(&address_space_memory, idaw_addr,
> >>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> >>> + sizeof(idaw.fmt1), false);
> >>> + cds->cda = be64_to_cpu(idaw.fmt1);
> >> Still need to check bit 0x80000000 here I think.
> >
> > Yes, I think this is 'must be zero' for format-1 idaws, and not covered
> > by the ccw-format specific checks above. (Although the PoP can be a bit
> > confusing with many similar terms...)
> >
>
> It's taken care of in ccw_dstream_rw_ida before the actual
> access happens. Code looks like this:
> + if (!idaw_fmt2 && (cds->cda + iter_len) >= (1ULL << 31)) {
> + ret = -EINVAL; /* channel program check */
> + goto err;
> + }
>
> The idea was to have it similar to the non-indirect case.
<looks at patch again>
Ah, I was simply looking for the wrong pattern. Looks correct.
> >>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
> >>> + CcwDataStreamOp op)
> >>> +{
> >>> + uint64_t bsz = ccw_ida_block_size(cds->flags);
> >>> + int ret = 0;
> >>> + uint16_t cont_left, iter_len;
> >>> + const bool idaw_fmt2 = cds->flags & CDS_F_C64;
> >>> + bool ccw_fmt1 = cds->flags & CDS_F_FMT;
> >> Use 'const bool' either? Although I doubt the value of using const here.
> >> ;)
> >
> > Both being the same is still a good idea.
> >
>
> Yeah. For which one should I go (with const or without)?
For the one you prefer :) (I'm not sure if the const adds value here.)
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA
2017-09-20 11:18 ` Cornelia Huck
@ 2017-09-20 16:46 ` Halil Pasic
2017-09-21 0:50 ` Dong Jia Shi
0 siblings, 1 reply; 31+ messages in thread
From: Halil Pasic @ 2017-09-20 16:46 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel
On 09/20/2017 01:18 PM, Cornelia Huck wrote:
> On Wed, 20 Sep 2017 13:13:01 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>
>> On 09/20/2017 10:33 AM, Cornelia Huck wrote:
>>> On Wed, 20 Sep 2017 15:42:38 +0800
>>> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
>>>
>>>> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:45 +0200]:
>
>>>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
>>>>> + sizeof(idaw.fmt2), false);
>>>>> + cds->cda = be64_to_cpu(idaw.fmt2);
>>>>> + } else {
>>>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
>>>>> + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
>>>>> + return -EINVAL; /* channel program check */
>>>>> + }
>>>>> + ret = address_space_rw(&address_space_memory, idaw_addr,
>>>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
>>>>> + sizeof(idaw.fmt1), false);
>>>>> + cds->cda = be64_to_cpu(idaw.fmt1);
>>>> Still need to check bit 0x80000000 here I think.
>>>
>>> Yes, I think this is 'must be zero' for format-1 idaws, and not covered
>>> by the ccw-format specific checks above. (Although the PoP can be a bit
>>> confusing with many similar terms...)
>>>
>>
>> It's taken care of in ccw_dstream_rw_ida before the actual
>> access happens. Code looks like this:
>> + if (!idaw_fmt2 && (cds->cda + iter_len) >= (1ULL << 31)) {
>> + ret = -EINVAL; /* channel program check */
>> + goto err;
>> + }
>>
>> The idea was to have it similar to the non-indirect case.
>
> <looks at patch again>
>
> Ah, I was simply looking for the wrong pattern. Looks correct.
>
>
Thinking about this some more. Since in case of IDA we are guaranteed
to never cross a block boundary with a single IDAW we won't ever cross
block boundary. So we can do the check in ida_read_next_idaw by checking
bit 0x80000000 on the ccw->cda. So we could keep idaw_fmt2 and ccw_fmt1
local to ida_read_next_idaw and save one goto err. I think that would
look a bit nicer than what I have here in v3. Agree?
>>>>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
>>>>> + CcwDataStreamOp op)
>>>>> +{
>>>>> + uint64_t bsz = ccw_ida_block_size(cds->flags);
>>>>> + int ret = 0;
>>>>> + uint16_t cont_left, iter_len;
>>>>> + const bool idaw_fmt2 = cds->flags & CDS_F_C64;
>>>>> + bool ccw_fmt1 = cds->flags & CDS_F_FMT;
>>>> Use 'const bool' either? Although I doubt the value of using const here.
>>>> ;)
>>>
>>> Both being the same is still a good idea.
>>>
>>
>> Yeah. For which one should I go (with const or without)?
>
> For the one you prefer :) (I'm not sure if the const adds value here.)
>
I think we generally don't care about const-ness in such situations,
so I think I won't use consts.
I intend to fix the issues we have found and do a v4 tomorrow, unless
somebody screams -- could do it today but I would like to give Dong
Jia an opportunity to react. On the other hand waiting more that that
will IMHO do us no favor either (I think of our storage/memory hierarchy).
Regards,
Halil
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA
2017-09-20 16:46 ` Halil Pasic
@ 2017-09-21 0:50 ` Dong Jia Shi
2017-09-21 7:31 ` Cornelia Huck
0 siblings, 1 reply; 31+ messages in thread
From: Dong Jia Shi @ 2017-09-21 0:50 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, Dong Jia Shi, Pierre Morel, qemu-devel
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-20 18:46:57 +0200]:
>
>
> On 09/20/2017 01:18 PM, Cornelia Huck wrote:
> > On Wed, 20 Sep 2017 13:13:01 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >
> >> On 09/20/2017 10:33 AM, Cornelia Huck wrote:
> >>> On Wed, 20 Sep 2017 15:42:38 +0800
> >>> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> >>>
> >>>> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:45 +0200]:
> >
> >>>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> >>>>> + sizeof(idaw.fmt2), false);
> >>>>> + cds->cda = be64_to_cpu(idaw.fmt2);
> >>>>> + } else {
> >>>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> >>>>> + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> >>>>> + return -EINVAL; /* channel program check */
> >>>>> + }
> >>>>> + ret = address_space_rw(&address_space_memory, idaw_addr,
> >>>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> >>>>> + sizeof(idaw.fmt1), false);
> >>>>> + cds->cda = be64_to_cpu(idaw.fmt1);
> >>>> Still need to check bit 0x80000000 here I think.
> >>>
> >>> Yes, I think this is 'must be zero' for format-1 idaws, and not covered
> >>> by the ccw-format specific checks above. (Although the PoP can be a bit
> >>> confusing with many similar terms...)
> >>>
> >>
> >> It's taken care of in ccw_dstream_rw_ida before the actual
> >> access happens. Code looks like this:
> >> + if (!idaw_fmt2 && (cds->cda + iter_len) >= (1ULL << 31)) {
> >> + ret = -EINVAL; /* channel program check */
> >> + goto err;
> >> + }
> >>
> >> The idea was to have it similar to the non-indirect case.
> >
> > <looks at patch again>
> >
> > Ah, I was simply looking for the wrong pattern. Looks correct.
> >
> >
>
> Thinking about this some more. Since in case of IDA we are guaranteed
> to never cross a block boundary with a single IDAW we won't ever cross
> block boundary. So we can do the check in ida_read_next_idaw by checking
> bit 0x80000000 on the ccw->cda. So we could keep idaw_fmt2 and ccw_fmt1
> local to ida_read_next_idaw and save one goto err. I think that would
> look a bit nicer than what I have here in v3. Agree?
Agree. That would also do the check in the first place. Sounds better.
>
> >>>>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
> >>>>> + CcwDataStreamOp op)
> >>>>> +{
> >>>>> + uint64_t bsz = ccw_ida_block_size(cds->flags);
> >>>>> + int ret = 0;
> >>>>> + uint16_t cont_left, iter_len;
> >>>>> + const bool idaw_fmt2 = cds->flags & CDS_F_C64;
> >>>>> + bool ccw_fmt1 = cds->flags & CDS_F_FMT;
> >>>> Use 'const bool' either? Although I doubt the value of using const here.
> >>>> ;)
> >>>
> >>> Both being the same is still a good idea.
> >>>
> >>
> >> Yeah. For which one should I go (with const or without)?
> >
> > For the one you prefer :) (I'm not sure if the const adds value here.)
> >
>
> I think we generally don't care about const-ness in such situations,
> so I think I won't use consts.
>
> I intend to fix the issues we have found and do a v4 tomorrow, unless
> somebody screams -- could do it today but I would like to give Dong
> Jia an opportunity to react.
Thanks. I'm coming. :)
> On the other hand waiting more that that will IMHO do us no favor
> either (I think of our storage/memory hierarchy).
>
> Regards,
> Halil
--
Dong Jia Shi
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA
2017-09-21 0:50 ` Dong Jia Shi
@ 2017-09-21 7:31 ` Cornelia Huck
0 siblings, 0 replies; 31+ messages in thread
From: Cornelia Huck @ 2017-09-21 7:31 UTC (permalink / raw)
To: Dong Jia Shi; +Cc: Halil Pasic, Pierre Morel, qemu-devel
On Thu, 21 Sep 2017 08:50:36 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-20 18:46:57 +0200]:
> > Thinking about this some more. Since in case of IDA we are guaranteed
> > to never cross a block boundary with a single IDAW we won't ever cross
> > block boundary. So we can do the check in ida_read_next_idaw by checking
> > bit 0x80000000 on the ccw->cda. So we could keep idaw_fmt2 and ccw_fmt1
> > local to ida_read_next_idaw and save one goto err. I think that would
> > look a bit nicer than what I have here in v3. Agree?
> Agree. That would also do the check in the first place. Sounds better.
Can't argue with nicer code, either :) Looking forward to the next
version.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA
2017-09-20 11:13 ` Halil Pasic
2017-09-20 11:18 ` Cornelia Huck
@ 2017-09-21 1:10 ` Dong Jia Shi
1 sibling, 0 replies; 31+ messages in thread
From: Dong Jia Shi @ 2017-09-21 1:10 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, Dong Jia Shi, Pierre Morel, qemu-devel
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-20 13:13:01 +0200]:
>
>
> On 09/20/2017 10:33 AM, Cornelia Huck wrote:
> > On Wed, 20 Sep 2017 15:42:38 +0800
> > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> >
> >> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:45 +0200]:
> >>
> >>> Let's add indirect data addressing support for our virtual channel
> >>> subsystem. This implementation does not bother with any kind of
> >>> prefetching. We simply step through the IDAL on demand.
> >>>
> >>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> >>> ---
> >>> hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>> 1 file changed, 116 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >>> index 2d37a9ddde..a3ce6d89b6 100644
> >>> --- a/hw/s390x/css.c
> >>> +++ b/hw/s390x/css.c
> >>> @@ -827,6 +827,121 @@ incr:
> >>> return 0;
> >>> }
> >>>
> >>> +/* returns values between 1 and bsz, where bsz is a power of 2 */
> >>> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
> >>> +{
> >>> + return bsz - (cda & (bsz - 1));
> >>> +}
> >>> +
> >>> +static inline uint64_t ccw_ida_block_size(uint8_t flags)
> >>> +{
> >>> + if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
> >>> + return 1ULL << 12;
> >>> + }
> >>> + return 1ULL << 11;
> >>> +}
> >>> +
> >>> +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1,
> >>> + bool idaw_fmt_2)
> >>> +{
> >>> + union {uint64_t fmt2; uint32_t fmt1; } idaw;
> >>> + int ret;
> >>> + hwaddr idaw_addr;
> >>> +
> >>> + if (idaw_fmt_2) {
> >>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
> >>> + if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> >>> + return -EINVAL; /* channel program check */
> >>> + }
> >>> + ret = address_space_rw(&address_space_memory, idaw_addr,
> >> Ahh, just got one question here:
> >> Do we need to considerate endianess for idaw_addr?
> >
> > That is taken care of below.
> >
> > And the previous version worked on my laptop via tcg ;)
>
> Nod.
My fault!
I was thinking of the idaw_addr itself, not the content of it. Now I
realized that, since we already converted (cds->cda_orig) in
copy_ccw_from_guest(), there is no need to convert (idaw_addr +
idaw_size * idaw_index) anymore.
Please ingnore my noise. ;P
>
> >
> >>
> >>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> >>> + sizeof(idaw.fmt2), false);
> >>> + cds->cda = be64_to_cpu(idaw.fmt2);
> >>> + } else {
> >>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> >>> + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> >>> + return -EINVAL; /* channel program check */
> >>> + }
> >>> + ret = address_space_rw(&address_space_memory, idaw_addr,
> >>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> >>> + sizeof(idaw.fmt1), false);
> >>> + cds->cda = be64_to_cpu(idaw.fmt1);
[...]
--
Dong Jia Shi
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA
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:11 ` Cornelia Huck
2017-09-20 11:01 ` Halil Pasic
1 sibling, 1 reply; 31+ messages in thread
From: Cornelia Huck @ 2017-09-20 8:11 UTC (permalink / raw)
To: Halil Pasic; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel
On Tue, 19 Sep 2017 20:27:45 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> Let's add indirect data addressing support for our virtual channel
> subsystem. This implementation does not bother with any kind of
> prefetching. We simply step through the IDAL on demand.
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
My s-o-b is a bit preliminary ;)
> ---
> hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 116 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 2d37a9ddde..a3ce6d89b6 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -827,6 +827,121 @@ incr:
> return 0;
> }
>
> +/* returns values between 1 and bsz, where bsz is a power of 2 */
> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
> +{
> + return bsz - (cda & (bsz - 1));
> +}
> +
> +static inline uint64_t ccw_ida_block_size(uint8_t flags)
> +{
> + if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
> + return 1ULL << 12;
> + }
> + return 1ULL << 11;
> +}
> +
> +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1,
> + bool idaw_fmt_2)
> +{
> + union {uint64_t fmt2; uint32_t fmt1; } idaw;
> + int ret;
> + hwaddr idaw_addr;
> +
> + if (idaw_fmt_2) {
> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
> + if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
So, what is supposed to happen if the idaw_addr is not aligned
correctly _and_ is violating the limits?
> + return -EINVAL; /* channel program check */
> + }
> + ret = address_space_rw(&address_space_memory, idaw_addr,
> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> + sizeof(idaw.fmt2), false);
> + cds->cda = be64_to_cpu(idaw.fmt2);
> + } else {
> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> + return -EINVAL; /* channel program check */
> + }
> + ret = address_space_rw(&address_space_memory, idaw_addr,
> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> + sizeof(idaw.fmt1), false);
> + cds->cda = be64_to_cpu(idaw.fmt1);
> + }
> + ++(cds->at_idaw);
> + if (ret != MEMTX_OK) {
> + /* assume inaccessible address */
> + return -EINVAL; /* channel program check */
> +
> + }
> + return 0;
> +}
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA
2017-09-20 8:11 ` Cornelia Huck
@ 2017-09-20 11:01 ` Halil Pasic
0 siblings, 0 replies; 31+ messages in thread
From: Halil Pasic @ 2017-09-20 11:01 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Dong Jia Shi, Pierre Morel, qemu-devel
On 09/20/2017 10:11 AM, Cornelia Huck wrote:
> On Tue, 19 Sep 2017 20:27:45 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>
>> Let's add indirect data addressing support for our virtual channel
>> subsystem. This implementation does not bother with any kind of
>> prefetching. We simply step through the IDAL on demand.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>
> My s-o-b is a bit preliminary ;)
>
>> ---
>> hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 116 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 2d37a9ddde..a3ce6d89b6 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -827,6 +827,121 @@ incr:
>> return 0;
>> }
>>
>> +/* returns values between 1 and bsz, where bsz is a power of 2 */
>> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
>> +{
>> + return bsz - (cda & (bsz - 1));
>> +}
>> +
>> +static inline uint64_t ccw_ida_block_size(uint8_t flags)
>> +{
>> + if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
>> + return 1ULL << 12;
>> + }
>> + return 1ULL << 11;
>> +}
>> +
>> +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1,
>> + bool idaw_fmt_2)
>> +{
>> + union {uint64_t fmt2; uint32_t fmt1; } idaw;
>> + int ret;
>> + hwaddr idaw_addr;
>> +
>> + if (idaw_fmt_2) {
>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
>> + if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
>
> So, what is supposed to happen if the idaw_addr is not aligned
> correctly _and_ is violating the limits?
You are right this is obviously wrong.
Should have been
if (idaw_addr & 0x07 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
>
>> + return -EINVAL; /* channel program check */
>> + }
>> + ret = address_space_rw(&address_space_memory, idaw_addr,
>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
>> + sizeof(idaw.fmt2), false);
>> + cds->cda = be64_to_cpu(idaw.fmt2);
>> + } else {
>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
>> + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
Same here.
>> + return -EINVAL; /* channel program check */
>> + }
>> + ret = address_space_rw(&address_space_memory, idaw_addr,
>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
>> + sizeof(idaw.fmt1), false);
>> + cds->cda = be64_to_cpu(idaw.fmt1);
>> + }
>> + ++(cds->at_idaw);
>> + if (ret != MEMTX_OK) {
>> + /* assume inaccessible address */
>> + return -EINVAL; /* channel program check */
>> +
>> + }
>> + return 0;
>> +}
>
^ permalink raw reply [flat|nested] 31+ messages in thread