On Wed, 28 Nov 2018 10:00:59 -0500
Farhan Ali <alifm@xxxxxxxxxxxxx> wrote:
On 11/28/2018 09:52 AM, Cornelia Huck wrote:
On Wed, 28 Nov 2018 09:31:51 -0500
Farhan Ali <alifm@xxxxxxxxxxxxx> wrote:
On 11/28/2018 04:02 AM, Cornelia Huck wrote:
On Tue, 27 Nov 2018 14:09:49 -0500
Farhan Ali <alifm@xxxxxxxxxxxxx> wrote:
On 11/22/2018 11:54 AM, Cornelia Huck wrote:
Add a region to the vfio-ccw device that can be used to submit
asynchronous I/O instructions. ssch continues to be handled by the
existing I/O region; the new region handles hsch and csch.
Interrupt status continues to be reported through the same channels
as for ssch.
Signed-off-by: Cornelia Huck <cohuck@xxxxxxxxxx>
---
drivers/s390/cio/Makefile | 3 +-
drivers/s390/cio/vfio_ccw_async.c | 88 ++++++++++++++++
drivers/s390/cio/vfio_ccw_drv.c | 48 ++++++---
drivers/s390/cio/vfio_ccw_fsm.c | 158 +++++++++++++++++++++++++++-
drivers/s390/cio/vfio_ccw_ops.c | 13 ++-
drivers/s390/cio/vfio_ccw_private.h | 6 ++
include/uapi/linux/vfio.h | 4 +
include/uapi/linux/vfio_ccw.h | 12 +++
8 files changed, 313 insertions(+), 19 deletions(-)
create mode 100644 drivers/s390/cio/vfio_ccw_async.c
@@ -76,7 +79,8 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
private = container_of(work, struct vfio_ccw_private, io_work);
irb = &private->irb;
- if (scsw_is_solicited(&irb->scsw)) {
+ if (scsw_is_solicited(&irb->scsw) &&
+ (scsw_fctl(&irb->scsw) & SCSW_FCTL_START_FUNC)) {
cp_update_scsw(&private->cp, &irb->scsw);
cp_free(&private->cp);
}
I am a little confused about this. Why do we need to update the scsw.cpa
if we have the start function function control bit set? Is it an
optimization?
No, it's not an optimization. This is the work function that is
scheduled if we get an interrupt for the device. Previously, we only
got an interrupt if either the device presented us an unsolicited
status or if we got an interrupt as a response to the channel program
we submitted. Now, we can get an interrupt for halt/clear subchannel as
well, and in that case, we don't necessarily have a cp.
[Thinking some more about it, we need to verify if the start function
actually remains set if we try to terminate a running channel program
with halt/clear. A clear might scrub too much. If that's the case, we
also need to free the cp if the start function is not set.]
According to PoPs (Chapter 16: I/O interruptions, under function control):
The start-function indication is also cleared at
the subchannel during the execution of CLEAR SUB-
CHANNEL.
So maybe we do need to free the cp.
Hm... so we need to make sure that cp_update_scsw() and cp_free() only
do something when there's actually a valid cp around and call them
unconditionally.
Yes, I agree.
Maybe add a ->valid flag to struct channel_program?
We could do that. So we would set the flag once we have copied the
channel program to kernel memory? since that's when we should care about
freeing it.
I hacked up the following (still untested):
From e771c8dc5abbfbd19688b452096bab9d032e0df5 Mon Sep 17 00:00:00 2001
From: Cornelia Huck <cohuck@xxxxxxxxxx>
Date: Wed, 28 Nov 2018 16:30:51 +0100
Subject: [PATCH] vfio-ccw: make it safe to access channel programs
When we get a solicited interrupt, the start function may have
been cleared by a csch, but we still have a channel program
structure allocated. Make it safe to call the cp accessors in
any case, so we can call them unconditionally.
Signed-off-by: Cornelia Huck <cohuck@xxxxxxxxxx>
---
drivers/s390/cio/vfio_ccw_cp.c | 9 ++++++++-
drivers/s390/cio/vfio_ccw_cp.h | 2 ++
drivers/s390/cio/vfio_ccw_drv.c | 3 +--
3 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 70a006ba4d05..35f87514276b 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -335,6 +335,7 @@ static void cp_unpin_free(struct channel_program *cp)
struct ccwchain *chain, *temp;
int i;
+ cp->initialized = false;
list_for_each_entry_safe(chain, temp, &cp->ccwchain_list, next) {
for (i = 0; i < chain->ch_len; i++) {
pfn_array_table_unpin_free(chain->ch_pat + i,
@@ -701,6 +702,8 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
*/
cp->orb.cmd.c64 = 1;
+ cp->initialized = true;
+
return ret;
}
@@ -715,7 +718,8 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
*/
void cp_free(struct channel_program *cp)
{
- cp_unpin_free(cp);
+ if (cp->initialized)
+ cp_unpin_free(cp);
}
/**
@@ -831,6 +835,9 @@ void cp_update_scsw(struct channel_program *cp, union scsw *scsw)
u32 cpa = scsw->cmd.cpa;
u32 ccw_head, ccw_tail;
+ if (!cp->initialized)
+ return;
+
/*
* LATER:
* For now, only update the cmd.cpa part. We may need to deal with
diff --git a/drivers/s390/cio/vfio_ccw_cp.h b/drivers/s390/cio/vfio_ccw_cp.h
index a4b74fb1aa57..3c20cd208da5 100644
--- a/drivers/s390/cio/vfio_ccw_cp.h
+++ b/drivers/s390/cio/vfio_ccw_cp.h
@@ -21,6 +21,7 @@
* @ccwchain_list: list head of ccwchains
* @orb: orb for the currently processed ssch request
* @mdev: the mediated device to perform page pinning/unpinning
+ * @initialized: whether this instance is actually initialized
*
* @ccwchain_list is the head of a ccwchain list, that contents the
* translated result of the guest channel program that pointed out by
@@ -30,6 +31,7 @@ struct channel_program {
struct list_head ccwchain_list;
union orb orb;
struct device *mdev;
+ bool initialized;
};
extern int cp_init(struct channel_program *cp, struct device *mdev,
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 890c588a3a61..83d6f43792b6 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -79,8 +79,7 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
private = container_of(work, struct vfio_ccw_private, io_work);
irb = &private->irb;
- if (scsw_is_solicited(&irb->scsw) &&
- (scsw_fctl(&irb->scsw) & SCSW_FCTL_START_FUNC)) {
+ if (scsw_is_solicited(&irb->scsw)) {
cp_update_scsw(&private->cp, &irb->scsw);
cp_free(&private->cp);
}
