[Qemu-devel] [PATCH RFC 1/2] vfio-ccw: forward halt/clear to device if supported

Discussion:

Pierre Morel

2018-05-11 09:53:52 UTC

The initial version of vfio-ccw did not support forwarding of the
halt or clear functions to the device, and we had to emulate them
instead.
For versions of the vfio-ccw kernel implementation that indeed do
support halt/clear (by indicating them in the fctl of the scsw in
the io_region), we can simply start making use of it. If the kernel
does not support handling halt/clear, fall back to emulation.
---
hw/s390x/css.c | 32 ++++++++++++++++++++++++++++----
hw/vfio/ccw.c | 11 +++++++++--
include/hw/s390x/css.h | 10 +++++++---
3 files changed, 44 insertions(+), 9 deletions(-)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 301bf1772f..b6727d0607 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1180,6 +1180,16 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
}
+static IOInstEnding sch_handle_clear_func_passthrough(SubchDev *sch)
+{
+ return s390_ccw_cmd_request(sch);
+}
+
+static IOInstEnding sch_handle_halt_func_passthrough(SubchDev *sch)
+{
+ return s390_ccw_cmd_request(sch);
+}
+
static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
{
@@ -1233,13 +1243,27 @@ IOInstEnding do_subchannel_work_virtual(SubchDev *sch)
IOInstEnding do_subchannel_work_passthrough(SubchDev *sch)
{
SCSW *s = &sch->curr_status.scsw;
+ static bool no_halt_clear;
+ /* if the kernel does not support halt/clear, fall back to emulation */
if (s->ctrl & SCSW_FCTL_CLEAR_FUNC) {
- /* TODO: Clear handling */
- sch_handle_clear_func(sch);
+ if (no_halt_clear) {
+ sch_handle_clear_func(sch);
+ } else {
+ if (sch_handle_clear_func_passthrough(sch) == IOINST_OPNOTSUPP) {
+ no_halt_clear = true;
+ sch_handle_halt_func(sch);
+ }
+ }
} else if (s->ctrl & SCSW_FCTL_HALT_FUNC) {
- /* TODO: Halt handling */
- sch_handle_halt_func(sch);
+ if (no_halt_clear) {
+ sch_handle_halt_func(sch);
+ } else {
+ if (sch_handle_halt_func_passthrough(sch) == IOINST_OPNOTSUPP) {
+ no_halt_clear = true;
+ sch_handle_halt_func(sch);
+ }
+ }
} else if (s->ctrl & SCSW_FCTL_START_FUNC) {
return sch_handle_start_func_passthrough(sch);
}
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index e67392c5f9..247901ae41 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -60,6 +60,7 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
memset(region, 0, sizeof(*region));
+ /* orb is only valid for ssch, but does not hurt for other functions */
memcpy(region->orb_area, &sch->orb, sizeof(ORB));
memcpy(region->scsw_area, &sch->curr_status.scsw, sizeof(SCSW));
if (errno == EAGAIN) {
goto again;
}
- error_report("vfio-ccw: wirte I/O region failed with errno=%d", errno);
- ret = -errno;
+ /* handle not supported operations like halt/clear on older kernels */
+ if (ret != -EOPNOTSUPP) {
+ error_report("vfio-ccw: write I/O region failed with errno=%d",
+ errno);
+ ret = -errno;
+ }
} else {
ret = region->ret_code;
}
return IOINST_CC_NOT_OPERATIONAL;
+ return IOINST_OPNOTSUPP;
sch_gen_unit_exception(sch);
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 35facb47d2..e33f26882b 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -100,9 +100,11 @@ typedef struct CcwDataStream {
} CcwDataStream;
/*
- * IO instructions conclude according to this. Currently we have only
- * cc codes. Valid values are 0, 1, 2, 3 and the generic semantic for
+ * IO instructions conclude according to this. One class of values are
+ * cc codes: Valid values are 0, 1, 2, 3 and the generic semantic for
* IO instructions is described briefly. For more details consult the PoP.
+ * Additionally, other endings may occur due to internal processing errors
+ * like lack of support for an operation.
*/
typedef enum IOInstEnding {
/* produced expected result */
@@ -112,7 +114,9 @@ typedef enum IOInstEnding {
/* inst. ineffective because busy with previously initiated function */
IOINST_CC_BUSY = 2,
/* inst. ineffective because not operational */
- IOINST_CC_NOT_OPERATIONAL = 3
+ IOINST_CC_NOT_OPERATIONAL = 3,
+ /* internal: operation not supported */
+ IOINST_OPNOTSUPP = 4
} IOInstEnding;
typedef struct SubchDev SubchDev;

Couldn't we introduce ABI versioning ?

--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Cornelia Huck

2018-05-15 16:01:04 UTC

Permalink

On Fri, 11 May 2018 11:53:52 +0200

Post by Pierre Morel

Couldn't we introduce ABI versioning ?

Can you elaborate what you're referring to?

If you mean checking capabilities of the kernel or so: If we can avoid
that and just try (and stop if it does not work), I'd prefer that (no
dependencies).

The IOINST_OPNOTSUPP is a bit ugly, but I did not see a more elegant
way to pass 'not supported' up to the caller.

Pierre Morel

2018-05-16 13:53:48 UTC

Permalink

Post by Cornelia Huck
On Fri, 11 May 2018 11:53:52 +0200

Post by Pierre Morel

Couldn't we introduce ABI versioning ?

Can you elaborate what you're referring to?
If you mean checking capabilities of the kernel or so: If we can avoid
that and just try (and stop if it does not work), I'd prefer that (no
dependencies).

VFIO_CHECK_EXTENSION is already used in different drivers
for this kind of interface extension.
We could use it to setup appropriate callbacks for scsh/csch/xsch/hsch
depending on the extension argument.

Post by Cornelia Huck
The IOINST_OPNOTSUPP is a bit ugly, but I did not see a more elegant
way to pass 'not supported' up to the caller.

--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Cornelia Huck

2018-05-16 15:52:43 UTC

Permalink

On Wed, 16 May 2018 15:53:48 +0200

Post by Pierre Morel

Post by Cornelia Huck
On Fri, 11 May 2018 11:53:52 +0200

Post by Pierre Morel
Couldn't we introduce ABI versioning ?

Can you elaborate what you're referring to?
If you mean checking capabilities of the kernel or so: If we can avoid
that and just try (and stop if it does not work), I'd prefer that (no
dependencies).

Hm, this might be useful for things like xsch that aren't really well
served by the current interface. Or we could try using the
device-specific capability interface.

Let me think and play with this a bit.