vfio-ccw: support hsch/csch (kernel part)
Subsystems (5)
v4 cover vfio-ccw: support hsch/csch (kernel part)
[This is the Linux kernel part, git tree is available at
https://git.kernel.org/pub/scm/linux/kernel/git/kvms390/vfio-ccw.git vfio-ccw-eagain-caps-v3
The companion QEMU patches are available at
https://github.com/cohuck/qemu vfio-ccw-caps]
Currently, vfio-ccw only relays START SUBCHANNEL requests to the real device. This tends to work well for the most common 'good path' scenarios; however, as we emulate {HALT,CLEAR} SUBCHANNEL in QEMU, things like clearing pending requests at the device is currently not supported. This may be a problem for e.g. error recovery.
This patch series introduces capabilities (similar to what vfio-pci uses) and exposes a new async region for handling hsch/csch.
Lightly tested (I can interact with a dasd as before, and reserve/release seems to work well.) Not sure if there is a better way to test this, ideas welcome.
Changes v3->v4:
- shrunk the capability offset, we don't need to accommodate as much as pci
- make checks in patch 1 more consistent and less buggy
- rebased on top of my current vfio-ccw branch
Changes v2->v3:
- Unb0rked patch 1, improved scope
- Split out the new mutex from patch 2 into new patch 3; added missing locking and hopefully improved description
- Patch 2 now reworks the state handling by splitting the BUSY state into CP_PROCESSING and CP_PENDING
- Patches 3 and 5 adapted on top of the reworked patches; hsch/csch are allowed in CP_PENDING, but not in CP_PROCESSING (did not add any R-b due to that)
- Added missing free in patch 5
- Probably some small changes I forgot to note down
Changes v1->v2:
- New patch 1: make it safe to use the cp accessors at any time; this should avoid problems with unsolicited interrupt handling
- New patch 2: handle concurrent accesses to the io region; the idea is to return -EAGAIN to userspace more often (so it can simply retry)
- also handle concurrent accesses to the async io region
- change VFIO_REGION_TYPE_CCW
- merge events for halt and clear to a single async event; this turned out to make the code quite a bit simpler
- probably some small changes I forgot to note down
Cornelia Huck (6):
vfio-ccw: make it safe to access channel programs
vfio-ccw: rework ssch state handling
vfio-ccw: protect the I/O region
vfio-ccw: add capabilities chain
s390/cio: export hsch to modules
vfio-ccw: add handling for async channel instructions
drivers/s390/cio/Makefile | 3 +- drivers/s390/cio/ioasm.c | 1 + drivers/s390/cio/vfio_ccw_async.c | 88 ++++++++++++ drivers/s390/cio/vfio_ccw_cp.c | 21 ++- drivers/s390/cio/vfio_ccw_cp.h | 2 + drivers/s390/cio/vfio_ccw_drv.c | 57 ++++++-- drivers/s390/cio/vfio_ccw_fsm.c | 143 +++++++++++++++++- drivers/s390/cio/vfio_ccw_ops.c | 215 +++++++++++++++++++++++----- drivers/s390/cio/vfio_ccw_private.h | 48 ++++++- include/uapi/linux/vfio.h | 4 + include/uapi/linux/vfio_ccw.h | 12 ++ 11 files changed, 537 insertions(+), 57 deletions(-) create mode 100644 drivers/s390/cio/vfio_ccw_async.c
Signature · 1 line
2.17.2
PATCH v4 1/6 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.
While at it, also make sure that functions called from other parts of the code return gracefully if the channel program structure has not been initialized (even though that is a bug in the caller).
--- drivers/s390/cio/vfio_ccw_cp.c | 21 ++++++++++++++++++++- drivers/s390/cio/vfio_ccw_cp.h | 2 ++ drivers/s390/cio/vfio_ccw_fsm.c | 5 +++++ 3 files changed, 27 insertions(+), 1 deletion(-)
drivers/s390/cio/vfio_ccw_cp.c +20 -1
@@ -362,6 +362,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,
@@ -732,6 +733,9 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)*/ cp->orb.cmd.c64 =1 ; +if (!ret) + cp->initialized = true; +return ret; }
@@ -746,7 +750,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); }/**
@@ -791,6 +796,10 @@ int cp_prefetch(struct channel_program *cp)struct ccwchain *chain;int len, idx, ret; +/* this is an error in the caller */ +if (!cp->initialized) +return -EINVAL ; +list_for_each_entry (chain, &cp->ccwchain_list, next) { len = chain->ch_len;for (idx =0 ; idx < len; idx++) {
@@ -826,6 +835,10 @@ union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm)struct ccwchain *chain;struct ccw1 *cpa; +/* this is an error in the caller */ +if (!cp->initialized) +return NULL ; + orb = &cp->orb; orb->cmd.intparm = intparm;
@@ -862,6 +875,9 @@ void cp_update_scsw(struct channel_program *cp, union scsw *scsw)u32 cpa = scsw->cmd.cpa;u32 ccw_head; +if (!cp->initialized) +return ; +/* * LATER: * For now, only update the cmd.cpa part. We may need to deal with
@@ -898,6 +914,9 @@ bool cp_iova_pinned(struct channel_program *cp, u64 iova)struct ccwchain *chain;int i; +if (!cp->initialized) +return false; +list_for_each_entry (chain, &cp->ccwchain_list, next) {for (i =0 ; i < chain->ch_len; i++) if (pfn_array_table_iova_pinned(chain->ch_pat + i,
drivers/s390/cio/vfio_ccw_cp.h +2 -0
@@ -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,
drivers/s390/cio/vfio_ccw_fsm.c +5 -0
@@ -31,6 +31,10 @@ static int fsm_io_helper(struct vfio_ccw_private *private) private->state =VFIO_CCW_STATE_BUSY ; orb =cp_get_orb (&private->cp, (u32 )(addr_t )sch, sch->lpm); +if (!orb) { + ret = -EIO ; + goto out; + }/* Issue "Start Subchannel" */ ccode =ssch (sch->schid, orb);
@@ -64,6 +68,7 @@ static int fsm_io_helper(struct vfio_ccw_private *private)default : ret = ccode; } +out:spin_unlock_irqrestore (sch->lock, flags);return ret; }
Signature · 1 line
2.17.2
Re: [PATCH v4 1/6] vfio-ccw: make it safe to access channel programs
Quoted 5 lines · Cornelia Huck
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.
Quoted 7 lines
While at it, also make sure that functions called from other parts of the code return gracefully if the channel program structure has not been initialized (even though that is a bug in the caller).
Reviewed-by Eric Farman<[email protected]>Signed-off-by Cornelia Huck<[email protected]>---
Hi Connie,
My series of fixes for vfio-ccw depends on this patch as I would like to call cp_free unconditionally :) (I had developed my code on top of your patches).
Could we pick this patch up as well when/if you pick up my patch series? I am in the process of sending out a v2.
Regarding this patch we could merge it as a stand alone patch, separate from this series. And also the patch LGTM
Thanks
Farhan
Re: [PATCH v4 1/6] vfio-ccw: make it safe to access channel programs
Quoted 20 lines · On Mon, 8 Apr 2019 13:02:12 -0400 Farhan Ali <[email protected]> wrote:
Quoted 5 lines · Cornelia Huck
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.
Quoted 7 lines
While at it, also make sure that functions called from other parts of the code return gracefully if the channel program structure has not been initialized (even though that is a bug in the caller).
Reviewed-by Eric Farman<[email protected]>Signed-off-by Cornelia Huck<[email protected]>---
Hi Connie,
My series of fixes for vfio-ccw depends on this patch as I would like to call
cp_freeunconditionally :) (I had developed my code on top of your patches).
Quoted 7 lines
Could we pick this patch up as well when/if you pick up my patch series? I am in the process of sending out a v2.
Regarding this patch we could merge it as a stand alone patch, separate from this series. And also the patch LGTM
Reviewed-by Farhan Ali <[email protected]>
Actually, I wanted to ask how people felt about merging this whole series for the next release :) It would be one thing less on my plate...
Re: [PATCH v4 1/6] vfio-ccw: make it safe to access channel programs
Quoted 26 lines · Cornelia Huck
Quoted 23 lines · On Mon, 8 Apr 2019 13:02:12 -0400 Farhan Ali <[email protected]> wrote:
Quoted 5 lines · Cornelia Huck
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.
Quoted 7 lines
While at it, also make sure that functions called from other parts of the code return gracefully if the channel program structure has not been initialized (even though that is a bug in the caller).
Reviewed-by Eric Farman<[email protected]>Signed-off-by Cornelia Huck<[email protected]>---
Hi Connie,
My series of fixes for vfio-ccw depends on this patch as I would like to call
cp_freeunconditionally :) (I had developed my code on top of your patches).Could we pick this patch up as well when/if you pick up my patch series? I am in the process of sending out a v2.
Quoted 9 lines
Quoted 4 lines
Regarding this patch we could merge it as a stand alone patch, separate from this series. And also the patch LGTM
Reviewed-by Farhan Ali <[email protected]>Actually, I wanted to ask how people felt about merging this whole series for the next release :) It would be one thing less on my plate...
I have been testing with your patches for a while now and I haven't hit any problems due to the patches.
IMHO I think we can merge the patches for the next release.
Thanks
Farhan
Eric Farman
maintainerRe: [PATCH v4 1/6] vfio-ccw: make it safe to access channel programs
In reply to Cornelia HuckQuoted 26 lines · Cornelia Huck
Quoted 23 lines · On Mon, 8 Apr 2019 13:02:12 -0400 Farhan Ali <[email protected]> wrote:
Quoted 5 lines · Cornelia Huck
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.
Quoted 7 lines
While at it, also make sure that functions called from other parts of the code return gracefully if the channel program structure has not been initialized (even though that is a bug in the caller).
Reviewed-by Eric Farman<[email protected]>Signed-off-by Cornelia Huck<[email protected]>---
Hi Connie,
My series of fixes for vfio-ccw depends on this patch as I would like to call
cp_freeunconditionally :) (I had developed my code on top of your patches).Could we pick this patch up as well when/if you pick up my patch series? I am in the process of sending out a v2.
Quoted 8 lines
Quoted 4 lines
Regarding this patch we could merge it as a stand alone patch, separate from this series. And also the patch LGTM
Reviewed-by Farhan Ali <[email protected]>Actually, I wanted to ask how people felt about merging this whole series for the next release :) It would be one thing less on my plate...
I'm not opposed to it. Every time I try to review patches 4 and 6 I get an asynchronous interrupt of my own. :) I'll at least get you an ack in the next day or two.
Halil Pasic
reviewerRe: [Qemu-devel] [PATCH v4 1/6] vfio-ccw: make it safe to access channel programs
In reply to Cornelia HuckQuoted 26 lines · On Mon, 8 Apr 2019 19:07:47 +0200 Cornelia Huck <[email protected]> wrote:
Quoted 23 lines · On Mon, 8 Apr 2019 13:02:12 -0400 Farhan Ali <[email protected]> wrote:
Quoted 5 lines · Cornelia Huck
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.
Quoted 7 lines
While at it, also make sure that functions called from other parts of the code return gracefully if the channel program structure has not been initialized (even though that is a bug in the caller).
Reviewed-by Eric Farman<[email protected]>Signed-off-by Cornelia Huck<[email protected]>---
Hi Connie,
My series of fixes for vfio-ccw depends on this patch as I would like to call
cp_freeunconditionally :) (I had developed my code on top of your patches).Could we pick this patch up as well when/if you pick up my patch series? I am in the process of sending out a v2.
Quoted 8 lines
Quoted 4 lines
Regarding this patch we could merge it as a stand alone patch, separate from this series. And also the patch LGTM
Reviewed-by Farhan Ali <[email protected]>Actually, I wanted to ask how people felt about merging this whole series for the next release :) It would be one thing less on my plate...
Sorry I was not able to spend any significant amount of time on this lately.
Gave the combined set (this + Farhans fio-ccw fixes for kernel stacktraces v2) it a bit of smoke testing after some minor adjustments to make it compile:
drivers/s390/cio/vfio_ccw_ops.c +1 -0
@@ -13,6 +13,7 @@#include <linux/vfio.h> #include <linux/mdev.h> #include <linux/nospec.h> +#include <linux/slab.h> #include "vfio_ccw_private.h"
I'm just running fio on a pass-through DASD and on some virto-blk disks in parallel. My QEMU is today's vfio-ccw-caps from your repo.
I see stuff like this:
qemu-git: vfio-ccw: wirte I/O region failed with errno=16[1811/7332/0 iops] [eta 26m:34s] [Thread 0x3ff75890910 (LWP 43803) exited]/7932KB/0KB /s] [1930/7932/0 iops] [eta 26m:33s] [Thread 0x3ff6b7b7910 (LWP 43800) exited]/8030KB/0KB /s] [2031/8030/0 iops] [eta 26m:32s] dasd-eckd 0.0.1234: An error occurred in the DASD device driver, reason=14 00000000caa27abe
INFO: task kworker/u6:1:26 blocked for more than 122 seconds.ps] [eta 23m:26s]eta 23m:25s]
Not tainted 5.1.0-rc3-00217-g6ab18dc #598
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
kworker/u6:1 D 0 26 2 0x00000000
Workqueue: writeback wb_workfn (flush-94:0)
Call Trace:
([<0000000000ae23f2>] __schedule+0x4fa/0xc98)
[<0000000000ae2bda>] schedule+0x4a/0xb0
[<00000000001b30ec>] io_schedule+0x2c/0x50
[<000000000071cc9c>] blk_mq_get_tag+0x1bc/0x310
[<000000000071571c>] blk_mq_get_request+0x1a4/0x4a8
[<0000000000719d38>] blk_mq_make_request+0x100/0x728
[<000000000070aa0a>] generic_make_request+0x26a/0x478
[<000000000070ac76>] submit_bio+0x5e/0x178
[<00000000004cfa2c>] ext4_io_submit+0x74/0x88
[<00000000004cfd32>] ext4_bio_write_page+0x2d2/0x4c8
[<00000000004aa5b4>] mpage_submit_page+0x74/0xa8
[<00000000004aa676>] mpage_process_page_bufs+0x8e/0x1b8
[<00000000004aa9bc>] mpage_prepare_extent_to_map+0x21c/0x390
[<00000000004b063c>] ext4_writepages+0x4bc/0x11a0
[<000000000032ef7a>] do_writepages+0x3a/0xf0
[<0000000000416226>] __writeback_single_inode+0x86/0x7a0
[<0000000000417154>] writeback_sb_inodes+0x2cc/0x550
[<0000000000417476>] __writeback_inodes_wb+0x9e/0xe8
[<00000000004179e0>] wb_writeback+0x468/0x598
[<0000000000418780>] wb_workfn+0x3b8/0x710
[<0000000000199322>] process_one_work+0x25a/0x668
[<000000000019977a>] worker_thread+0x4a/0x428
[<00000000001a1ae8>] kthread+0x150/0x170
[<0000000000aeadda>] kernel_thread_starter+0x6/0xc
[<0000000000aeadd4>] kernel_thread_starter+0x0/0xc
4 locks held by kworker/u6:1/26:
#0: 00000000792cf224 ((wq_completion)writeback){+.+.}, at: process_one_work+0x19c/0x668
#1: 000000009888c0e5 ((work_completion)(&(&wb->dwork)->work)){+.+.}, at: process_one_work+0x19c/0x668
#2: 000000002bfb76f0 (&type->s_umount_key#29){++++}, at: trylock_super+0x2e/0xa8
#3: 00000000ff47fe1d (&sbi->s_journal_flag_rwsem){.+.+}, at: do_writepages+0x3a/0xf0Since I haven't had the time to keep up lately, I will just trust Eric and Farhan on whether this should be merged or not. From a quick look at the code, and a quick stroll through my remaining memories, I think, there are a couple of things, that I myself would try to solve differently. But that is not a valid reason to hold this up.
I would like to spare the hustle of revisiting my old comments for everyone. From the stability and utility perspective I'm pretty convinced we are better off than without the patches in question.
TLDR:
If it is good enough for Eric and Farhan, I have no objections against merging.
Regards,
Halil
Eric Farman
maintainerRe: [Qemu-devel] [PATCH v4 1/6] vfio-ccw: make it safe to access channel programs
Quoted 49 lines · Halil Pasic
Quoted 26 lines · On Mon, 8 Apr 2019 19:07:47 +0200 Cornelia Huck <[email protected]> wrote:
Quoted 23 lines · On Mon, 8 Apr 2019 13:02:12 -0400 Farhan Ali <[email protected]> wrote:
Quoted 5 lines · Cornelia Huck
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.
Quoted 7 lines
While at it, also make sure that functions called from other parts of the code return gracefully if the channel program structure has not been initialized (even though that is a bug in the caller).
Reviewed-by Eric Farman<[email protected]>Signed-off-by Cornelia Huck<[email protected]>---
Hi Connie,
My series of fixes for vfio-ccw depends on this patch as I would like to call
cp_freeunconditionally :) (I had developed my code on top of your patches).Could we pick this patch up as well when/if you pick up my patch series? I am in the process of sending out a v2.
Quoted 8 lines
Quoted 4 lines
Regarding this patch we could merge it as a stand alone patch, separate from this series. And also the patch LGTM
Reviewed-by Farhan Ali <[email protected]>Actually, I wanted to ask how people felt about merging this whole series for the next release :) It would be one thing less on my plate...
Sorry I was not able to spend any significant amount of time on this lately.
Gave the combined set (this + Farhans fio-ccw fixes for kernel stacktraces v2) it a bit of smoke testing after some minor adjustments to make it compile:
drivers/s390/cio/vfio_ccw_ops.c line 19
Quoted 7 lines
drivers/s390/cio/vfio_ccw_ops.c +1
-0@@ -13,6 +13,7 @@#include <linux/vfio.h> #include <linux/mdev.h> #include <linux/nospec.h> +#include <linux/slab.h> #include "vfio_ccw_private.h"
Hrm... Taking today's master, and the two series you mention (slight adjustment to apply patch 3 of Connie's series, because part of it was split out a few weeks ago), and I don't encounter this. Tried switching between SLUB/SLAB, but still compiles fine.
Quoted 5 lines
I'm just running fio on a pass-through DASD and on some virto-blk disks in parallel. My QEMU is today's vfio-ccw-caps from your repo.
I see stuff like this:
qemu-git: vfio-ccw: wirte I/O region failed with errno=16[1811/7332/0 iops] [eta 26m:34s]
Without knowing what the I/O was that failed, this is a guessing game. But I encountered something similar just now running fio.
qemu:
2019-04-11T02:06:09.524838Z qemu-system-s390x: vfio-ccw: wirte I/O region failed with errno=16
guest:
[ 422.931458] dasd-eckd 0.0.ca8d: An error occurred in the DASD device driver, reason=14 00000000730bbe9a
[ 553.741554] dasd-eckd 0.0.ca8e: An error occurred in the DASD device driver, reason=14 00000000e59b81da
[ 554.761552] dasd-eckd 0.0.ca8d: An error occurred in the DASD device driver, reason=14 00000000cdf4fb4e
[ 554.921518] dasd-eckd 0.0.ca8b: An error occurred in the DASD device driver, reason=14 0000000068775082
[ 555.271556] dasd-eckd 0.0.ca8d: ERP 00000000cdf4fb4e has run out of retries and failed
[ 555.271786] dasd(eckd): I/O status report for device 0.0.ca8d:
dasd(eckd): in req: 00000000cdf4fb4e CC:00 FC:00 AC:00
SC:00 DS:00 CS:00 RC:-16
dasd(eckd): device 0.0.ca8d: Failing CCW: (null)
dasd(eckd): SORRY - NO VALID SENSE AVAILABLE
[ 555.272214] dasd(eckd): Related CP in req: 00000000cdf4fb4e
dasd(eckd): CCW 000000006434c30f: 03400000 00000000 DAT:
dasd(eckd): CCW 000000007a65f7e0: 08000000 70E5B700 DAT:
[ 555.272508] dasd(eckd):......From the associated I/O, I think this is fixed by a series I am nearly ready to send for review. I'll try again with those fixes on top of the two series here, and report back.
Quoted 47 lines
[Thread
0x3ff75890910(LWP 43803) exited]/7932KB/0KB /s] [1930/7932/0 iops] [eta 26m:33s] [Thread0x3ff6b7b7910(LWP 43800) exited]/8030KB/0KB /s] [2031/8030/0 iops] [eta 26m:32s] dasd-eckd 0.0.1234: An error occurred in the DASD device driver, reason=1400000000caa27abeINFO: task kworker/u6:1:26 blocked for more than 122 seconds.ps] [eta 23m:26s]eta 23m:25s] Not tainted 5.1.0-rc3-00217-g6ab18dc #598 "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. kworker/u6:1 D 0 26 2 0x00000000 Workqueue: writeback wb_workfn (flush-94:0) Call Trace: ([<0000000000ae23f2>] __schedule+0x4fa/0xc98) [<0000000000ae2bda>] schedule+0x4a/0xb0 [<00000000001b30ec>] io_schedule+0x2c/0x50 [<000000000071cc9c>] blk_mq_get_tag+0x1bc/0x310 [<000000000071571c>] blk_mq_get_request+0x1a4/0x4a8 [<0000000000719d38>] blk_mq_make_request+0x100/0x728 [<000000000070aa0a>] generic_make_request+0x26a/0x478 [<000000000070ac76>] submit_bio+0x5e/0x178 [<00000000004cfa2c>] ext4_io_submit+0x74/0x88 [<00000000004cfd32>] ext4_bio_write_page+0x2d2/0x4c8 [<00000000004aa5b4>] mpage_submit_page+0x74/0xa8 [<00000000004aa676>] mpage_process_page_bufs+0x8e/0x1b8 [<00000000004aa9bc>] mpage_prepare_extent_to_map+0x21c/0x390 [<00000000004b063c>] ext4_writepages+0x4bc/0x11a0 [<000000000032ef7a>] do_writepages+0x3a/0xf0 [<0000000000416226>] __writeback_single_inode+0x86/0x7a0 [<0000000000417154>] writeback_sb_inodes+0x2cc/0x550 [<0000000000417476>] __writeback_inodes_wb+0x9e/0xe8 [<00000000004179e0>] wb_writeback+0x468/0x598 [<0000000000418780>] wb_workfn+0x3b8/0x710 [<0000000000199322>] process_one_work+0x25a/0x668 [<000000000019977a>] worker_thread+0x4a/0x428 [<00000000001a1ae8>] kthread+0x150/0x170 [<0000000000aeadda>] kernel_thread_starter+0x6/0xc [<0000000000aeadd4>] kernel_thread_starter+0x0/0xc 4 locks held by kworker/u6:1/26: #0: 00000000792cf224 ((wq_completion)writeback){+.+.}, at: process_one_work+0x19c/0x668 #1: 000000009888c0e5 ((work_completion)(&(&wb->dwork)->work)){+.+.}, at: process_one_work+0x19c/0x668 #2: 000000002bfb76f0 (&type->s_umount_key#29){++++}, at: trylock_super+0x2e/0xa8 #3: 00000000ff47fe1d (&sbi->s_journal_flag_rwsem){.+.+}, at: do_writepages+0x3a/0xf0Since I haven't had the time to keep up lately, I will just trust Eric and Farhan on whether this should be merged or not. From a quick look at the code, and a quick stroll through my remaining memories, I think, there are a couple of things, that I myself would try to solve differently. But that is not a valid reason to hold this up.
Quoted 3 lines
I would like to spare the hustle of revisiting my old comments for everyone.
From the stability and utility perspective I'm pretty convinced we are
better off than without the patches in question.
I agree, both series are an improvement. I'll focus on both tomorrow.
- Eric
Quoted 7 lines
TLDR:
If it is good enough for Eric and Farhan, I have no objections against merging.Regards,
Halil
Halil Pasic
reviewerRe: [qemu-s390x] [Qemu-devel] [PATCH v4 1/6] vfio-ccw: make it safe to access channel programs
Quoted 18 lines · On Wed, 10 Apr 2019 22:59:41 -0400 Eric Farman <[email protected]> wrote:
r the next release :) It would be one thing less on my plate...
Quoted 17 lines
Quoted 1 line
Sorry I was not able to spend any significant amount of time on this lately.
Gave the combined set (this + Farhans fio-ccw fixes for kernel stacktraces v2) it a bit of smoke testing after some minor adjustments to make it compile:
drivers/s390/cio/vfio_ccw_ops.c +1
-0@@ -13,6 +13,7 @@#include <linux/vfio.h> #include <linux/mdev.h> #include <linux/nospec.h> +#include <linux/slab.h>
Quoted 8 lines
drivers/s390/cio/vfio_ccw_ops.c line 19
Quoted 3 lines
drivers/s390/cio/vfio_ccw_ops.c +0
-0#include "vfio_ccw_private.h" Hrm... Taking today's master, and the two series you mention (slight adjustment to apply patch 3 of Connie's series, because part of it was split out a few weeks ago), and I don't encounter this. Tried switching between SLUB/SLAB, but still compiles fine.
Let me guess: you have CONFIG_PCI in our .config, right?
In arch/s390/include/asm/pci_io.h we have
#ifndef _ASM_S390_PCI_IO_H
#define _ASM_S390_PCI_IO_H
#ifdef CONFIG_PCI
#include <linux/kernel.h>
#include <linux/slab.h>
and pci_io.h comes in via
include/linux/vfio.hinclude/linux/iommu.hinclude/linux/scatterlist.harch/s390/include/asm/io.harch/s390/include/asm/pci_io.h
Figured it out with help from Farhan. Took me quite some time.
Regards,
Halil
Eric Farman
maintainerRe: [qemu-s390x] [Qemu-devel] [PATCH v4 1/6] vfio-ccw: make it safe to access channel programs
Quoted 44 lines · Halil Pasic
Quoted 18 lines · On Wed, 10 Apr 2019 22:59:41 -0400 Eric Farman <[email protected]> wrote:
r the next release :) It would be one thing less on my plate...
Quoted 17 lines
Quoted 1 line
Sorry I was not able to spend any significant amount of time on this lately.
Gave the combined set (this + Farhans fio-ccw fixes for kernel stacktraces v2) it a bit of smoke testing after some minor adjustments to make it compile:
drivers/s390/cio/vfio_ccw_ops.c +1
-0@@ -13,6 +13,7 @@#include <linux/vfio.h> #include <linux/mdev.h> #include <linux/nospec.h> +#include <linux/slab.h> Quoted 8 lines
drivers/s390/cio/vfio_ccw_ops.c line 19
Quoted 3 lines
drivers/s390/cio/vfio_ccw_ops.c +0
-0#include "vfio_ccw_private.h" Hrm... Taking today's master, and the two series you mention (slight adjustment to apply patch 3 of Connie's series, because part of it was split out a few weeks ago), and I don't encounter this. Tried switching between SLUB/SLAB, but still compiles fine.
Let me guess: you have
CONFIG_PCIin our .config, right?In
arch/s390/include/asm/pci_io.hwe have#ifndef
_ASM_S390_PCI_IO_H
#define_ASM_S390_PCI_IO_H#ifdef
CONFIG_PCI#include <
linux/kernel.h>
#include <linux/slab.h>and
pci_io.hcomes in via
Quoted 8 lines
include/linux/vfio.hinclude/linux/iommu.hinclude/linux/scatterlist.harch/s390/include/asm/io.harch/s390/include/asm/pci_io.hFigured it out with help from Farhan. Took me quite some time.
That would have been useful information up front.
Quoted 5 lines
Regards,
Halil
Re: [qemu-s390x] [Qemu-devel] [PATCH v4 1/6] vfio-ccw: make it safe to access channel programs
Quoted 52 lines · On Thu, 11 Apr 2019 12:25:38 -0400 Eric Farman <[email protected]> wrote:
Quoted 51 lines · Halil Pasic
Quoted 18 lines · On Wed, 10 Apr 2019 22:59:41 -0400 Eric Farman <[email protected]> wrote:
r the next release :) It would be one thing less on my plate...
Quoted 17 lines
Quoted 1 line
Sorry I was not able to spend any significant amount of time on this lately.
Gave the combined set (this + Farhans fio-ccw fixes for kernel stacktraces v2) it a bit of smoke testing after some minor adjustments to make it compile:
drivers/s390/cio/vfio_ccw_ops.c +1
-0@@ -13,6 +13,7 @@#include <linux/vfio.h> #include <linux/mdev.h> #include <linux/nospec.h> +#include <linux/slab.h> Quoted 8 lines
drivers/s390/cio/vfio_ccw_ops.c line 19
Quoted 3 lines
drivers/s390/cio/vfio_ccw_ops.c +0
-0#include "vfio_ccw_private.h" Hrm... Taking today's master, and the two series you mention (slight adjustment to apply patch 3 of Connie's series, because part of it was split out a few weeks ago), and I don't encounter this. Tried switching between SLUB/SLAB, but still compiles fine.
Let me guess: you have
CONFIG_PCIin our .config, right?In
arch/s390/include/asm/pci_io.hwe have#ifndef
_ASM_S390_PCI_IO_H
#define_ASM_S390_PCI_IO_H#ifdef
CONFIG_PCI#include <
linux/kernel.h>
#include <linux/slab.h>and
pci_io.hcomes in via
include/linux/vfio.hinclude/linux/iommu.hinclude/linux/scatterlist.harch/s390/include/asm/io.harch/s390/include/asm/pci_io.h
Quoted 3 lines
Quoted 1 line
Figured it out with help from Farhan. Took me quite some time.
That would have been useful information up front.
Indeed. It's trivial to fold that change in, though :) (Should be in patch 4, if I see it correctly.)
Halil Pasic
reviewerRe: [qemu-s390x] [Qemu-devel] [PATCH v4 1/6] vfio-ccw: make it safe to access channel programs
Quoted 55 lines · On Thu, 11 Apr 2019 18:36:48 +0200 Cornelia Huck <[email protected]> wrote:
Quoted 52 lines · On Thu, 11 Apr 2019 12:25:38 -0400 Eric Farman <[email protected]> wrote:
Quoted 51 lines · Halil Pasic
Quoted 18 lines · On Wed, 10 Apr 2019 22:59:41 -0400 Eric Farman <[email protected]> wrote:
r the next release :) It would be one thing less on my plate...
Quoted 17 lines
Quoted 1 line
Sorry I was not able to spend any significant amount of time on this lately.
Gave the combined set (this + Farhans fio-ccw fixes for kernel stacktraces v2) it a bit of smoke testing after some minor adjustments to make it compile:
drivers/s390/cio/vfio_ccw_ops.c +1
-0@@ -13,6 +13,7 @@#include <linux/vfio.h> #include <linux/mdev.h> #include <linux/nospec.h> +#include <linux/slab.h> Quoted 8 lines
drivers/s390/cio/vfio_ccw_ops.c line 19
Quoted 3 lines
drivers/s390/cio/vfio_ccw_ops.c +0
-0#include "vfio_ccw_private.h" Hrm... Taking today's master, and the two series you mention (slight adjustment to apply patch 3 of Connie's series, because part of it was split out a few weeks ago), and I don't encounter this. Tried switching between SLUB/SLAB, but still compiles fine.
Let me guess: you have
CONFIG_PCIin our .config, right?In
arch/s390/include/asm/pci_io.hwe have#ifndef
_ASM_S390_PCI_IO_H
#define_ASM_S390_PCI_IO_H#ifdef
CONFIG_PCI#include <
linux/kernel.h>
#include <linux/slab.h>and
pci_io.hcomes in via
include/linux/vfio.hinclude/linux/iommu.hinclude/linux/scatterlist.harch/s390/include/asm/io.harch/s390/include/asm/pci_io.h
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Figured it out with help from Farhan. Took me quite some time.
That would have been useful information up front.
Indeed. It's trivial to fold that change in, though :) (Should be in patch 4, if I see it correctly.)
#4 vfio-ccw: add capabilities chain it is!
Cheers,
Halil
Eric Farman
maintainerRe: [Qemu-devel] [PATCH v4 1/6] vfio-ccw: make it safe to access channel programs
In reply to Eric FarmanQuoted 4 lines · Eric Farman
Quoted 1 line · Halil Pasic
On Mon, 8 Apr 2019 19:07:47 +0200
...snip...
Quoted 10 lines
Quoted 5 lines
I'm just running fio on a pass-through DASD and on some virto-blk disks in parallel. My QEMU is today's vfio-ccw-caps from your repo.
I see stuff like this:
qemu-git: vfio-ccw: wirte I/O region failed with errno=16[1811/7332/0 iops] [eta 26m:34s]Without knowing what the I/O was that failed, this is a guessing game. But I encountered something similar just now running fio.
qemu:
2019-04-11T02:06:09.524838Z qemu-system-s390x: vfio-ccw: wirte I/O region failed with errno=16
...snip...
Quoted 2 lines
From the associated I/O, I think this is fixed by a series I am nearly
ready to send for review. I'll try again with those fixes on top of the two series here, and report back.
So, I've run enough combinations to feel comfortable saying that the error (EBUSY) I saw last night (and presumably the one Halil saw) exists in today's code and is not introduced by this series. It also appears to be addressed by one of the patches in a series I'm working on, but which that series still has some further problems. Sigh, there are too many branches and too many interrupts.
- Eric
Re: [Qemu-devel] [PATCH v4 1/6] vfio-ccw: make it safe to access channel programs
Quoted 28 lines · On Thu, 11 Apr 2019 17:27:42 -0400 Eric Farman <[email protected]> wrote:
Quoted 4 lines · Eric Farman
Quoted 1 line · Halil Pasic
On Mon, 8 Apr 2019 19:07:47 +0200
...snip...
Quoted 13 lines
Quoted 6 lines
I'm just running fio on a pass-through DASD and on some virto-blk disks in parallel. My QEMU is today's vfio-ccw-caps from your repo.
I see stuff like this:
qemu-git: vfio-ccw: wirte I/O region failed with errno=16[1811/7332/0 iops] [eta 26m:34s]Without knowing what the I/O was that failed, this is a guessing game. But I encountered something similar just now running fio.
qemu:
2019-04-11T02:06:09.524838Z qemu-system-s390x: vfio-ccw: wirte I/Oregion failed with errno=16
...snip...
Quoted 3 lines
From the associated I/O, I think this is fixed by a series I am nearly
ready to send for review. I'll try again with those fixes on top of the two series here, and report back.
Quoted 6 lines
So, I've run enough combinations to feel comfortable saying that the error (EBUSY) I saw last night (and presumably the one Halil saw) exists in today's code and is not introduced by this series. It also appears to be addressed by one of the patches in a series I'm working on, but which that series still has some further problems. Sigh, there are too many branches and too many interrupts.
Great, thanks for checking! I know that feeling of being tangled in too many branches...
PATCH v4 2/6 vfio-ccw: rework ssch state handling
In reply to Cornelia HuckThe flow for processing ssch requests can be improved by splitting the BUSY state:
- CP_PROCESSING: We reject any user space requests while we are in the process of translating a channel program and submitting it to the hardware. Use -EAGAIN to signal user space that it should retry the request.
- CP_PENDING: We have successfully submitted a request with ssch and are now expecting an interrupt. As we can't handle more than one channel program being processed, reject any further requests with -EBUSY. A final interrupt will move us out of this state; this also fixes a latent bug where a non-final interrupt might have freed up a channel program that still was in progress.
By making this a separate state, we make it possible to issue a halt or a clear while we're still waiting for the final interrupt for the ssch (in a follow-on patch).
It also makes a lot of sense not to preemptively filter out writes to the io_region if we're in an incorrect state: the state machine will handle this correctly.
--- drivers/s390/cio/vfio_ccw_drv.c | 8 ++++++-- drivers/s390/cio/vfio_ccw_fsm.c | 19 ++++++++++++++----- drivers/s390/cio/vfio_ccw_ops.c | 2 -- drivers/s390/cio/vfio_ccw_private.h | 3 ++- 4 files changed, 22 insertions(+), 10 deletions(-)
drivers/s390/cio/vfio_ccw_drv.c +6 -2
@@ -72,20 +72,24 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work) {struct vfio_ccw_private *private;struct irb *irb; +bool is_final; private = container_of(work, struct vfio_ccw_private, io_work); irb = &private->irb; + is_final = !(scsw_actl (&irb->scsw) & + (SCSW_ACTL_DEVACT |SCSW_ACTL_SCHACT ));if (scsw_is_solicited (&irb->scsw)) {cp_update_scsw (&private->cp, &irb->scsw); -cp_free (&private->cp); +if (is_final) +cp_free (&private->cp); }memcpy (private->io_region->irb_area, irb,sizeof (*irb));if (private->io_trigger)eventfd_signal (private->io_trigger,1 ); -if (private->mdev) +if (private->mdev && is_final) private->state =VFIO_CCW_STATE_IDLE ; }
drivers/s390/cio/vfio_ccw_fsm.c +14 -5
@@ -28,7 +28,6 @@ static int fsm_io_helper(struct vfio_ccw_private *private) sch = private->sch;spin_lock_irqsave (sch->lock, flags); - private->state =VFIO_CCW_STATE_BUSY ; orb =cp_get_orb (&private->cp, (u32 )(addr_t )sch, sch->lpm);if (!orb) {
@@ -46,6 +45,7 @@ static int fsm_io_helper(struct vfio_ccw_private *private)*/ sch->schib.scsw.cmd.actl |=SCSW_ACTL_START_PEND ; ret =0 ; + private->state =VFIO_CCW_STATE_CP_PENDING ;break ;case 1 :/* Status pending */ case 2 :/* Busy */
@@ -107,6 +107,12 @@ static void fsm_io_busy(struct vfio_ccw_private *private, private->io_region->ret_code = -EBUSY ; } +static void fsm_io_retry (struct vfio_ccw_private *private, +enum vfio_ccw_event event) +{ + private->io_region->ret_code = -EAGAIN ; +} +static void fsm_disabled_irq (struct vfio_ccw_private *private,enum vfio_ccw_event event) {
@@ -135,8 +141,7 @@ static void fsm_io_request(struct vfio_ccw_private *private,struct mdev_device *mdev = private->mdev;char *errstr ="request" ; - private->state =VFIO_CCW_STATE_BUSY ; - + private->state =VFIO_CCW_STATE_CP_PROCESSING ;memcpy (scsw, io_region->scsw_area,sizeof (*scsw));if (scsw->cmd.fctl &SCSW_FCTL_START_FUNC ) {
@@ -181,7 +186,6 @@ static void fsm_io_request(struct vfio_ccw_private *private, } err_out: - private->state =VFIO_CCW_STATE_IDLE ;trace_vfio_ccw_io_fctl (scsw->cmd.fctl,get_schid (private), io_region->ret_code, errstr); }
@@ -221,7 +225,12 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = { [VFIO_CCW_EVENT_IO_REQ] = fsm_io_request, [VFIO_CCW_EVENT_INTERRUPT ] = fsm_irq, }, - [VFIO_CCW_STATE_BUSY] = { + [VFIO_CCW_STATE_CP_PROCESSING] = { + [VFIO_CCW_EVENT_NOT_OPER ] = fsm_notoper, + [VFIO_CCW_EVENT_IO_REQ ] = fsm_io_retry, + [VFIO_CCW_EVENT_INTERRUPT ] = fsm_irq, + }, + [VFIO_CCW_STATE_CP_PENDING ] = { [VFIO_CCW_EVENT_NOT_OPER ] = fsm_notoper, [VFIO_CCW_EVENT_IO_REQ ] = fsm_io_busy, [VFIO_CCW_EVENT_INTERRUPT ] = fsm_irq,
drivers/s390/cio/vfio_ccw_ops.c +0 -2
@@ -193,8 +193,6 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,return -EINVAL ; private =dev_get_drvdata (mdev_parent_dev (mdev)); -if (private->state !=VFIO_CCW_STATE_IDLE ) -return -EACCES ; region = private->io_region;if (copy_from_user ((void *)region + *ppos, buf, count))
drivers/s390/cio/vfio_ccw_private.h +2 -1
@@ -63,7 +63,8 @@ enum vfio_ccw_state {VFIO_CCW_STATE_NOT_OPER ,VFIO_CCW_STATE_STANDBY ,VFIO_CCW_STATE_IDLE , -VFIO_CCW_STATE_BUSY , +VFIO_CCW_STATE_CP_PROCESSING , +VFIO_CCW_STATE_CP_PENDING ,/* last element! */ NR_VFIO_CCW_STATES };
Signature · 1 line
2.17.2
Eric Farman
maintainerRe: [PATCH v4 2/6] vfio-ccw: rework ssch state handling
Quoted 51 lines · Cornelia Huck
The flow for processing ssch requests can be improved by splitting the BUSY state:
-
CP_PROCESSING: We reject any user space requests while we are in the process of translating a channel program and submitting it to the hardware. Use-EAGAINto signal user space that it should retry the request.
-CP_PENDING: We have successfully submitted a request with ssch and are now expecting an interrupt. As we can't handle more than one channel program being processed, reject any further requests with-EBUSY. A final interrupt will move us out of this state; this also fixes a latent bug where a non-final interrupt might have freed up a channel program that still was in progress.
By making this a separate state, we make it possible to issue a halt or a clear while we're still waiting for the final interrupt for the ssch (in a follow-on patch).It also makes a lot of sense not to preemptively filter out writes to the
io_regionif we're in an incorrect state: the state machine will handle this correctly.Reviewed-by Eric Farman <[email protected]>Signed-off-by Cornelia Huck <[email protected]>--- drivers/s390/cio/vfio_ccw_drv.c | 8 ++++++-- drivers/s390/cio/vfio_ccw_fsm.c | 19 ++++++++++++++----- drivers/s390/cio/vfio_ccw_ops.c | 2 -- drivers/s390/cio/vfio_ccw_private.h | 3 ++- 4 files changed, 22 insertions(+), 10 deletions(-)drivers/s390/cio/vfio_ccw_drv.c +5
-1@@ -72,20 +72,24 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work) {struct vfio_ccw_private *private;struct irb *irb; +bool is_final; private = container_of(work, struct vfio_ccw_private, io_work); irb = &private->irb; + is_final = !(scsw_actl (&irb->scsw) & + (SCSW_ACTL_DEVACT |SCSW_ACTL_SCHACT ));if (scsw_is_solicited (&irb->scsw)) {cp_update_scsw (&private->cp, &irb->scsw); -cp_free (&private->cp); +if (is_final) +cp_free (&private->cp); }
drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 95
Quoted 10 lines
drivers/s390/cio/vfio_ccw_drv.c +1
-1memcpy (private->io_region->irb_area, irb,sizeof (*irb));if (private->io_trigger)eventfd_signal (private->io_trigger,1 ); -if (private->mdev) +if (private->mdev && is_final) private->state =VFIO_CCW_STATE_IDLE ; }
Coincidentally, I did something AWESOME last night that the chunks listed above actually fix. I have a large channel program, and when it runs my host crashes which isn't nice. First, the callback:
[ 547.821235] Call Trace: [ 547.821236] ([<0000000000000000>] (null)) [ 547.821244] [<000003ff808d8b4a>] cp_prefetch+0x422/0x750 [vfio_ccw] [ 547.821247] [<000003ff808d9a90>] fsm_io_request+0x1a0/0x2f0 [vfio_ccw] [ 547.821250] [<000003ff808d90c4>] vfio_ccw_mdev_write+0xc4/0x1d8 [vfio_ccw] [ 547.821255] [<0000000000358d8c>] __vfs_write+0x34/0x1a8 [ 547.821256] [<00000000003590d0>] vfs_write+0xa0/0x1d8 [ 547.821259] [<0000000000359572>] ksys_pwrite64+0x8a/0xa8 [ 547.821264] [<0000000000866cf0>] system_call+0x270/0x290 [ 547.821264] Last Breaking-Event-Address: [ 547.821267] [<00000000003325b2>] __kmalloc+0x1c2/0x288
The channel program in question looks like this:
x01 cmd=0b flags=44 count=0006
x02 cmd=02 flags=64 count=07bf
x03 cmd=47 flags=44 count=0010
x04 cmd=49 flags=64 count=049b
x05 cmd=08 flags=00 count=0000 TIC to x04
x06 cmd=0b flags=64 count=0007
x07 cmd=23 flags=44 count=0001
x08 cmd=e4 flags=44 count=0018
x09 cmd=07 flags=44 count=0006
x0a cmd=e4 flags=44 count=0018
x0b cmd=47 flags=64 count=001b
x0c cmd=8e flags=64 count=013a
x0d cmd=9a flags=64 count=0009
x0e cmd=31 flags=4c count=0005
x0f cmd=08 flags=00 count=0000 TIC to x0e
x10 cmd=0d flags=64 count=061b
x11 cmd=07 flags=64 count=000b
x12 cmd=96 flags=64 count=0144
x13 cmd=a9 flags=64 count=0025
x14 cmd=08 flags=00 count=0000 TIC to x13
x15 cmd=05 flags=64 count=0387
x16 cmd=a4 flags=64 count=003e
x17 cmd=e4 flags=44 count=0018
x18 cmd=0b flags=64 count=000a
x19 cmd=96 flags=64 count=0497
x1a cmd=8e flags=64 count=02c3
x1b cmd=29 flags=64 count=01bf
x1c cmd=08 flags=00 count=0000 TIC to x1b
x1d cmd=1b flags=24 count=000a
Debugging it today, I found that we get an intermediate interrupt on CCW 0x0e, and a final interrupt (well, unit check) on CCW 0x11. But because of the intermediate interrupt, rewinding in cp_prefetch() at label out_err fails and we crash. Whoops!
Recalling the above changes, I applied JUST the above pieces (not the remainder of this patch), and the above channel program works fine. Now to figure out why I get a unit check. :)
- Eric
Re: [PATCH v4 2/6] vfio-ccw: rework ssch state handling
Quoted 57 lines · On Fri, 8 Mar 2019 17:18:22 -0500 Eric Farman <[email protected]> wrote:
Quoted 56 lines · Cornelia Huck
The flow for processing ssch requests can be improved by splitting the BUSY state:
-
CP_PROCESSING: We reject any user space requests while we are in the process of translating a channel program and submitting it to the hardware. Use-EAGAINto signal user space that it should retry the request.
-CP_PENDING: We have successfully submitted a request with ssch and are now expecting an interrupt. As we can't handle more than one channel program being processed, reject any further requests with-EBUSY. A final interrupt will move us out of this state; this also fixes a latent bug where a non-final interrupt might have freed up a channel program that still was in progress.
By making this a separate state, we make it possible to issue a halt or a clear while we're still waiting for the final interrupt for the ssch (in a follow-on patch).It also makes a lot of sense not to preemptively filter out writes to the
io_regionif we're in an incorrect state: the state machine will handle this correctly.Reviewed-by Eric Farman <[email protected]>Signed-off-by Cornelia Huck <[email protected]>--- drivers/s390/cio/vfio_ccw_drv.c | 8 ++++++-- drivers/s390/cio/vfio_ccw_fsm.c | 19 ++++++++++++++----- drivers/s390/cio/vfio_ccw_ops.c | 2 -- drivers/s390/cio/vfio_ccw_private.h | 3 ++- 4 files changed, 22 insertions(+), 10 deletions(-)drivers/s390/cio/vfio_ccw_drv.c +5
-1@@ -72,20 +72,24 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work) {struct vfio_ccw_private *private;struct irb *irb; +bool is_final; private = container_of(work, struct vfio_ccw_private, io_work); irb = &private->irb; + is_final = !(scsw_actl (&irb->scsw) & + (SCSW_ACTL_DEVACT |SCSW_ACTL_SCHACT ));if (scsw_is_solicited (&irb->scsw)) {cp_update_scsw (&private->cp, &irb->scsw); -cp_free (&private->cp); +if (is_final) +cp_free (&private->cp); }memcpy (private->io_region->irb_area, irb,sizeof (*irb));if (private->io_trigger)eventfd_signal (private->io_trigger,1 );
Quoted 9 lines
drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 95
Quoted 5 lines
drivers/s390/cio/vfio_ccw_drv.c +1
-1-if (private->mdev) +if (private->mdev && is_final) private->state =VFIO_CCW_STATE_IDLE ; }Coincidentally, I did something AWESOME last night that the chunks listed above actually fix. I have a large channel program, and when it runs my host crashes which isn't nice.
Ouch.
(...)
Quoted 2 lines
Recalling the above changes, I applied JUST the above pieces (not the remainder of this patch), and the above channel program works fine.
Thanks for pointing that out... I'll extract a patch with only the changes above and post it with cc:stable. A channel program submitted by the guest being able to crash the host is... not good.
PATCH v4 3/6 vfio-ccw: protect the I/O region
In reply to Cornelia HuckIntroduce a mutex to disallow concurrent reads or writes to the I/O region. This makes sure that the data the kernel or user space see is always consistent.
The same mutex will be used to protect the async region as well.
--- drivers/s390/cio/vfio_ccw_drv.c | 3 +++ drivers/s390/cio/vfio_ccw_ops.c | 28 +++++++++++++++++++--------- drivers/s390/cio/vfio_ccw_private.h | 2 ++ 3 files changed, 24 insertions(+), 9 deletions(-)
drivers/s390/cio/vfio_ccw_drv.c +3 -0
@@ -84,7 +84,9 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)if (is_final)cp_free (&private->cp); } +mutex_lock (&private->io_mutex);memcpy (private->io_region->irb_area, irb,sizeof (*irb)); +mutex_unlock (&private->io_mutex);if (private->io_trigger)eventfd_signal (private->io_trigger,1 );
@@ -129,6 +131,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch) private->sch = sch;dev_set_drvdata (&sch->dev, private); +mutex_init (&private->io_mutex);spin_lock_irq (sch->lock); private->state =VFIO_CCW_STATE_NOT_OPER ;
drivers/s390/cio/vfio_ccw_ops.c +19 -9
@@ -169,16 +169,20 @@ static ssize_t vfio_ccw_mdev_read(struct mdev_device *mdev, {struct vfio_ccw_private *private;struct ccw_io_region *region; +int ret;if (*ppos + count >sizeof (*region))return -EINVAL ; private =dev_get_drvdata (mdev_parent_dev (mdev)); +mutex_lock (&private->io_mutex); region = private->io_region;if (copy_to_user (buf, (void *)region + *ppos, count)) -return -EFAULT ; - -return count; + ret = -EFAULT ; +else + ret = count; +mutex_unlock (&private->io_mutex); +return ret; } static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
@@ -188,23 +192,29 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev, {struct vfio_ccw_private *private;struct ccw_io_region *region; +int ret;if (*ppos + count >sizeof (*region))return -EINVAL ; private =dev_get_drvdata (mdev_parent_dev (mdev)); +if (!mutex_trylock (&private->io_mutex)) +return -EAGAIN ; region = private->io_region; -if (copy_from_user ((void *)region + *ppos, buf, count)) -return -EFAULT ; +if (copy_from_user ((void *)region + *ppos, buf, count)) { + ret = -EFAULT ; + goto out_unlock; + }vfio_ccw_fsm_event (private,VFIO_CCW_EVENT_IO_REQ ); -if (region->ret_code !=0 ) { +if (region->ret_code !=0 ) private->state =VFIO_CCW_STATE_IDLE ; -return region->ret_code; - } + ret = (region->ret_code !=0 ) ? region->ret_code : count; -return count; +out_unlock: +mutex_unlock (&private->io_mutex); +return ret; }static int vfio_ccw_mdev_get_device_info (struct vfio_device_info *info)
drivers/s390/cio/vfio_ccw_private.h +2 -0
@@ -28,6 +28,7 @@* @mdev: pointer to the mediated device * @nb: notifier for vfio events * @io_region: MMIO region to input/output I/O arguments/results +* @io_mutex: protect against concurrent update of I/O regions * @cp: channel program for the current I/O operation * @irb: irb info received from interrupt * @scsw: scsw info
@@ -42,6 +43,7 @@ struct vfio_ccw_private {struct mdev_device *mdev;struct notifier_block nb;struct ccw_io_region *io_region; +struct mutex io_mutex;struct channel_program cp;struct irb irb;
Signature · 1 line
2.17.2
PATCH v4 4/6 vfio-ccw: add capabilities chain
In reply to Cornelia HuckAllow to extend the regions used by vfio-ccw. The first user will be handling of halt and clear subchannel.
--- drivers/s390/cio/vfio_ccw_ops.c | 186 ++++++++++++++++++++++++---- drivers/s390/cio/vfio_ccw_private.h | 38 ++++++ include/uapi/linux/vfio.h | 2 + 3 files changed, 200 insertions(+), 26 deletions(-)
drivers/s390/cio/vfio_ccw_ops.c +160 -26
@@ -3,13 +3,16 @@* Physical device callbacks for vfio_ccw * * Copyright IBM Corp. 2017 +* Copyright Red Hat, Inc. 2019 * * Author(s): Dong Jia Shi <[email protected]> * Xiao Feng Ren <[email protected]> +* Cornelia Huck <[email protected]> */ #include <linux/vfio.h> #include <linux/mdev.h> +#include <linux/nospec.h> #include "vfio_ccw_private.h"
@@ -157,27 +160,33 @@ static void vfio_ccw_mdev_release(struct mdev_device *mdev) {struct vfio_ccw_private *private =dev_get_drvdata (mdev_parent_dev (mdev)); +int i;vfio_unregister_notifier (mdev_dev (mdev),VFIO_IOMMU_NOTIFY , &private->nb); + +for (i =0 ; i < private->num_regions; i++) + private->region[i].ops->release (private, &private->region[i]); + + private->num_regions =0 ; +kfree (private->region); + private->region =NULL ; } -static ssize_t vfio_ccw_mdev_read (struct mdev_device *mdev, -char __user *buf, -size_t count, -loff_t *ppos) +static ssize_t vfio_ccw_mdev_read_io_region (struct vfio_ccw_private *private, +char __user *buf,size_t count, +loff_t *ppos) { -struct vfio_ccw_private *private; +loff_t pos = *ppos &VFIO_CCW_OFFSET_MASK ;struct ccw_io_region *region;int ret; -if (*ppos + count >sizeof (*region)) +if (pos + count >sizeof (*region))return -EINVAL ; - private =dev_get_drvdata (mdev_parent_dev (mdev));mutex_lock (&private->io_mutex); region = private->io_region; -if (copy_to_user (buf, (void *)region + *ppos, count)) +if (copy_to_user (buf, (void *)region + pos, count)) ret = -EFAULT ;else ret = count;
@@ -185,24 +194,47 @@ static ssize_t vfio_ccw_mdev_read(struct mdev_device *mdev,return ret; } -static ssize_t vfio_ccw_mdev_write (struct mdev_device *mdev, -const char __user *buf, -size_t count, -loff_t *ppos) +static ssize_t vfio_ccw_mdev_read (struct mdev_device *mdev, +char __user *buf, +size_t count, +loff_t *ppos) { +unsigned int index =VFIO_CCW_OFFSET_TO_INDEX (*ppos);struct vfio_ccw_private *private; + + private =dev_get_drvdata (mdev_parent_dev (mdev)); + +if (index >=VFIO_CCW_NUM_REGIONS + private->num_regions) +return -EINVAL ; + +switch (index) { +case VFIO_CCW_CONFIG_REGION_INDEX : +return vfio_ccw_mdev_read_io_region (private, buf, count, ppos); +default : + index -=VFIO_CCW_NUM_REGIONS ; +return private->region[index].ops->read (private, buf, count, + ppos); + } + +return -EINVAL ; +} + +static ssize_t vfio_ccw_mdev_write_io_region (struct vfio_ccw_private *private, +const char __user *buf, +size_t count,loff_t *ppos) +{ +loff_t pos = *ppos &VFIO_CCW_OFFSET_MASK ;struct ccw_io_region *region;int ret; -if (*ppos + count >sizeof (*region)) +if (pos + count >sizeof (*region))return -EINVAL ; - private =dev_get_drvdata (mdev_parent_dev (mdev));if (!mutex_trylock (&private->io_mutex))return -EAGAIN ; region = private->io_region; -if (copy_from_user ((void *)region + *ppos, buf, count)) { +if (copy_from_user ((void *)region + pos, buf, count)) { ret = -EFAULT ; goto out_unlock; }
@@ -217,19 +249,52 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,return ret; } -static int vfio_ccw_mdev_get_device_info (struct vfio_device_info *info) +static ssize_t vfio_ccw_mdev_write (struct mdev_device *mdev, +const char __user *buf, +size_t count, +loff_t *ppos) { +unsigned int index =VFIO_CCW_OFFSET_TO_INDEX (*ppos); +struct vfio_ccw_private *private; + + private =dev_get_drvdata (mdev_parent_dev (mdev)); + +if (index >=VFIO_CCW_NUM_REGIONS + private->num_regions) +return -EINVAL ; + +switch (index) { +case VFIO_CCW_CONFIG_REGION_INDEX : +return vfio_ccw_mdev_write_io_region (private, buf, count, ppos); +default : + index -=VFIO_CCW_NUM_REGIONS ; +return private->region[index].ops->write (private, buf, count, + ppos); + } + +return -EINVAL ; +} + +static int vfio_ccw_mdev_get_device_info (struct vfio_device_info *info, +struct mdev_device *mdev) +{ +struct vfio_ccw_private *private; + + private =dev_get_drvdata (mdev_parent_dev (mdev)); info->flags =VFIO_DEVICE_FLAGS_CCW |VFIO_DEVICE_FLAGS_RESET ; - info->num_regions =VFIO_CCW_NUM_REGIONS ; + info->num_regions =VFIO_CCW_NUM_REGIONS + private->num_regions; info->num_irqs =VFIO_CCW_NUM_IRQS ;return 0 ; }static int vfio_ccw_mdev_get_region_info (struct vfio_region_info *info, -u16 *cap_type_id, -void **cap_type) +struct mdev_device *mdev, +unsigned long arg) { +struct vfio_ccw_private *private; +int i; + + private =dev_get_drvdata (mdev_parent_dev (mdev));switch (info->index) {case VFIO_CCW_CONFIG_REGION_INDEX : info->offset =0 ;
@@ -237,9 +302,55 @@ static int vfio_ccw_mdev_get_region_info(struct vfio_region_info *info, info->flags =VFIO_REGION_INFO_FLAG_READ |VFIO_REGION_INFO_FLAG_WRITE ;return 0 ; -default : -return -EINVAL ; +default :/* all other regions are handled via capability chain */ + { +struct vfio_info_cap caps = { .buf =NULL , .size =0 }; +struct vfio_region_info_cap_type cap_type = { + .header.id =VFIO_REGION_INFO_CAP_TYPE , + .header.version =1 }; +int ret; + +if (info->index >= +VFIO_CCW_NUM_REGIONS + private->num_regions) +return -EINVAL ; + + info->index =array_index_nospec (info->index, +VFIO_CCW_NUM_REGIONS + + private->num_regions); + + i = info->index -VFIO_CCW_NUM_REGIONS ; + + info->offset =VFIO_CCW_INDEX_TO_OFFSET (info->index); + info->size = private->region[i].size; + info->flags = private->region[i].flags; + + cap_type.type = private->region[i].type; + cap_type.subtype = private->region[i].subtype; + + ret =vfio_info_add_capability (&caps, &cap_type.header, +sizeof (cap_type)); +if (ret) +return ret; + + info->flags |=VFIO_REGION_INFO_FLAG_CAPS ; +if (info->argsz <sizeof (*info) + caps.size) { + info->argsz =sizeof (*info) + caps.size; + info->cap_offset =0 ; + }else { +vfio_info_cap_shift (&caps,sizeof (*info)); +if (copy_to_user ((void __user *)arg +sizeof (*info), + caps.buf, caps.size)) { +kfree (caps.buf); +return -EFAULT ; + } + info->cap_offset =sizeof (*info); + } + +kfree (caps.buf); + + } } +return 0 ; }static int vfio_ccw_mdev_get_irq_info (struct vfio_irq_info *info)
@@ -316,6 +427,32 @@ static int vfio_ccw_mdev_set_irqs(struct mdev_device *mdev, } } +int vfio_ccw_register_dev_region (struct vfio_ccw_private *private, +unsigned int subtype, +const struct vfio_ccw_regops *ops, +size_t size,u32 flags,void *data) +{ +struct vfio_ccw_region *region; + + region =krealloc (private->region, + (private->num_regions +1 ) *sizeof (*region), +GFP_KERNEL ); +if (!region) +return -ENOMEM ; + + private->region = region; + private->region[private->num_regions].type =VFIO_REGION_TYPE_CCW ; + private->region[private->num_regions].subtype = subtype; + private->region[private->num_regions].ops = ops; + private->region[private->num_regions].size = size; + private->region[private->num_regions].flags = flags; + private->region[private->num_regions].data = data; + + private->num_regions++; + +return 0 ; +} +static ssize_t vfio_ccw_mdev_ioctl (struct mdev_device *mdev,unsigned int cmd,unsigned long arg)
@@ -336,7 +473,7 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,if (info.argsz < minsz)return -EINVAL ; - ret =vfio_ccw_mdev_get_device_info (&info); + ret =vfio_ccw_mdev_get_device_info (&info, mdev);if (ret)return ret;
@@ -345,8 +482,6 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,case VFIO_DEVICE_GET_REGION_INFO : {struct vfio_region_info info; -u16 cap_type_id =0 ; -void *cap_type =NULL ; minsz = offsetofend(struct vfio_region_info, offset);
@@ -356,8 +491,7 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,if (info.argsz < minsz)return -EINVAL ; - ret =vfio_ccw_mdev_get_region_info (&info, &cap_type_id, - &cap_type); + ret =vfio_ccw_mdev_get_region_info (&info, mdev, arg);if (ret)return ret;
drivers/s390/cio/vfio_ccw_private.h +38 -0
@@ -3,9 +3,11 @@* Private stuff for vfio_ccw driver * * Copyright IBM Corp. 2017 +* Copyright Red Hat, Inc. 2019 * * Author(s): Dong Jia Shi <[email protected]> * Xiao Feng Ren <[email protected]> +* Cornelia Huck <[email protected]> */ #ifndef _VFIO_CCW_PRIVATE_H_
@@ -19,6 +21,38 @@#include "css.h" #include "vfio_ccw_cp.h" +#define VFIO_CCW_OFFSET_SHIFT 10 +#define VFIO_CCW_OFFSET_TO_INDEX (off) (off >> VFIO_CCW_OFFSET_SHIFT) +#define VFIO_CCW_INDEX_TO_OFFSET (index) ((u64)(index) << VFIO_CCW_OFFSET_SHIFT) +#define VFIO_CCW_OFFSET_MASK (((u64)(1) << VFIO_CCW_OFFSET_SHIFT) - 1) + +/* capability chain handling similar to vfio-pci */ +struct vfio_ccw_private ; +struct vfio_ccw_region ; + +struct vfio_ccw_regops { +ssize_t (*read)(struct vfio_ccw_private *private,char __user *buf, +size_t count,loff_t *ppos); +ssize_t (*write)(struct vfio_ccw_private *private, +const char __user *buf,size_t count,loff_t *ppos); +void (*release)(struct vfio_ccw_private *private, +struct vfio_ccw_region *region); +}; + +struct vfio_ccw_region { +u32 type; +u32 subtype; +const struct vfio_ccw_regops *ops; +void *data; +size_t size; +u32 flags; +}; + +int vfio_ccw_register_dev_region (struct vfio_ccw_private *private, +unsigned int subtype, +const struct vfio_ccw_regops *ops, +size_t size,u32 flags,void *data); +/** * struct vfio_ccw_private * @sch: pointer to the subchannel
@@ -29,6 +63,8 @@* @nb: notifier for vfio events * @io_region: MMIO region to input/output I/O arguments/results * @io_mutex: protect against concurrent update of I/O regions +* @region: additional regions for other subchannel operations +* @num_regions: number of additional regions * @cp: channel program for the current I/O operation * @irb: irb info received from interrupt * @scsw: scsw info
@@ -44,6 +80,8 @@ struct vfio_ccw_private {struct notifier_block nb;struct ccw_io_region *io_region;struct mutex io_mutex; +struct vfio_ccw_region *region; +int num_regions;struct channel_program cp;struct irb irb;
include/uapi/linux/vfio.h +2 -0
@@ -353,6 +353,8 @@ struct vfio_region_gfx_edid {#define VFIO_DEVICE_GFX_LINK_STATE_DOWN 2 }; +#define VFIO_REGION_TYPE_CCW (2) +/* * 10de vendor sub-type *
Signature · 1 line
2.17.2
Re: [PATCH v4 4/6] vfio-ccw: add capabilities chain
Quoted 4 lines · Cornelia Huck
Allow to extend the regions used by vfio-ccw. The first user will be handling of halt and clear subchannel.
Signed-off-by Cornelia Huck <[email protected]>
Quoted 387 lines
--- drivers/s390/cio/vfio_ccw_ops.c | 186 ++++++++++++++++++++++++---- drivers/s390/cio/vfio_ccw_private.h | 38 ++++++ include/uapi/linux/vfio.h | 2 + 3 files changed, 200 insertions(+), 26 deletions(-)drivers/s390/cio/vfio_ccw_ops.c +160
-26@@ -3,13 +3,16 @@* Physical device callbacks for vfio_ccw * * Copyright IBM Corp. 2017 +* Copyright Red Hat, Inc. 2019 * * Author(s): Dong Jia Shi <[email protected]> * Xiao Feng Ren <[email protected]> +* Cornelia Huck <[email protected]> */ #include <linux/vfio.h> #include <linux/mdev.h> +#include <linux/nospec.h> #include "vfio_ccw_private.h" @@ -157,27 +160,33 @@ static void vfio_ccw_mdev_release(struct mdev_device *mdev) {struct vfio_ccw_private *private =dev_get_drvdata (mdev_parent_dev (mdev)); +int i;vfio_unregister_notifier (mdev_dev (mdev),VFIO_IOMMU_NOTIFY , &private->nb); + +for (i =0 ; i < private->num_regions; i++) + private->region[i].ops->release (private, &private->region[i]); + + private->num_regions =0 ; +kfree (private->region); + private->region =NULL ; } -static ssize_t vfio_ccw_mdev_read (struct mdev_device *mdev, -char __user *buf, -size_t count, -loff_t *ppos) +static ssize_t vfio_ccw_mdev_read_io_region (struct vfio_ccw_private *private, +char __user *buf,size_t count, +loff_t *ppos) { -struct vfio_ccw_private *private; +loff_t pos = *ppos &VFIO_CCW_OFFSET_MASK ;struct ccw_io_region *region;int ret; -if (*ppos + count >sizeof (*region)) +if (pos + count >sizeof (*region))return -EINVAL ; - private =dev_get_drvdata (mdev_parent_dev (mdev));mutex_lock (&private->io_mutex); region = private->io_region; -if (copy_to_user (buf, (void *)region + *ppos, count)) +if (copy_to_user (buf, (void *)region + pos, count)) ret = -EFAULT ;else ret = count;@@ -185,24 +194,47 @@ static ssize_t vfio_ccw_mdev_read(struct mdev_device *mdev,return ret; } -static ssize_t vfio_ccw_mdev_write (struct mdev_device *mdev, -const char __user *buf, -size_t count, -loff_t *ppos) +static ssize_t vfio_ccw_mdev_read (struct mdev_device *mdev, +char __user *buf, +size_t count, +loff_t *ppos) { +unsigned int index =VFIO_CCW_OFFSET_TO_INDEX (*ppos);struct vfio_ccw_private *private; + + private =dev_get_drvdata (mdev_parent_dev (mdev)); + +if (index >=VFIO_CCW_NUM_REGIONS + private->num_regions) +return -EINVAL ; + +switch (index) { +case VFIO_CCW_CONFIG_REGION_INDEX : +return vfio_ccw_mdev_read_io_region (private, buf, count, ppos); +default : + index -=VFIO_CCW_NUM_REGIONS ; +return private->region[index].ops->read (private, buf, count, + ppos); + } + +return -EINVAL ; +} + +static ssize_t vfio_ccw_mdev_write_io_region (struct vfio_ccw_private *private, +const char __user *buf, +size_t count,loff_t *ppos) +{ +loff_t pos = *ppos &VFIO_CCW_OFFSET_MASK ;struct ccw_io_region *region;int ret; -if (*ppos + count >sizeof (*region)) +if (pos + count >sizeof (*region))return -EINVAL ; - private =dev_get_drvdata (mdev_parent_dev (mdev));if (!mutex_trylock (&private->io_mutex))return -EAGAIN ; region = private->io_region; -if (copy_from_user ((void *)region + *ppos, buf, count)) { +if (copy_from_user ((void *)region + pos, buf, count)) { ret = -EFAULT ; goto out_unlock; }@@ -217,19 +249,52 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,return ret; } -static int vfio_ccw_mdev_get_device_info (struct vfio_device_info *info) +static ssize_t vfio_ccw_mdev_write (struct mdev_device *mdev, +const char __user *buf, +size_t count, +loff_t *ppos) { +unsigned int index =VFIO_CCW_OFFSET_TO_INDEX (*ppos); +struct vfio_ccw_private *private; + + private =dev_get_drvdata (mdev_parent_dev (mdev)); + +if (index >=VFIO_CCW_NUM_REGIONS + private->num_regions) +return -EINVAL ; + +switch (index) { +case VFIO_CCW_CONFIG_REGION_INDEX : +return vfio_ccw_mdev_write_io_region (private, buf, count, ppos); +default : + index -=VFIO_CCW_NUM_REGIONS ; +return private->region[index].ops->write (private, buf, count, + ppos); + } + +return -EINVAL ; +} + +static int vfio_ccw_mdev_get_device_info (struct vfio_device_info *info, +struct mdev_device *mdev) +{ +struct vfio_ccw_private *private; + + private =dev_get_drvdata (mdev_parent_dev (mdev)); info->flags =VFIO_DEVICE_FLAGS_CCW |VFIO_DEVICE_FLAGS_RESET ; - info->num_regions =VFIO_CCW_NUM_REGIONS ; + info->num_regions =VFIO_CCW_NUM_REGIONS + private->num_regions; info->num_irqs =VFIO_CCW_NUM_IRQS ;return 0 ; }static int vfio_ccw_mdev_get_region_info (struct vfio_region_info *info, -u16 *cap_type_id, -void **cap_type) +struct mdev_device *mdev, +unsigned long arg) { +struct vfio_ccw_private *private; +int i; + + private =dev_get_drvdata (mdev_parent_dev (mdev));switch (info->index) {case VFIO_CCW_CONFIG_REGION_INDEX : info->offset =0 ;@@ -237,9 +302,55 @@ static int vfio_ccw_mdev_get_region_info(struct vfio_region_info *info, info->flags =VFIO_REGION_INFO_FLAG_READ |VFIO_REGION_INFO_FLAG_WRITE ;return 0 ; -default : -return -EINVAL ; +default :/* all other regions are handled via capability chain */ + { +struct vfio_info_cap caps = { .buf =NULL , .size =0 }; +struct vfio_region_info_cap_type cap_type = { + .header.id =VFIO_REGION_INFO_CAP_TYPE , + .header.version =1 }; +int ret; + +if (info->index >= +VFIO_CCW_NUM_REGIONS + private->num_regions) +return -EINVAL ; + + info->index =array_index_nospec (info->index, +VFIO_CCW_NUM_REGIONS + + private->num_regions); + + i = info->index -VFIO_CCW_NUM_REGIONS ; + + info->offset =VFIO_CCW_INDEX_TO_OFFSET (info->index); + info->size = private->region[i].size; + info->flags = private->region[i].flags; + + cap_type.type = private->region[i].type; + cap_type.subtype = private->region[i].subtype; + + ret =vfio_info_add_capability (&caps, &cap_type.header, +sizeof (cap_type)); +if (ret) +return ret; + + info->flags |=VFIO_REGION_INFO_FLAG_CAPS ; +if (info->argsz <sizeof (*info) + caps.size) { + info->argsz =sizeof (*info) + caps.size; + info->cap_offset =0 ; + }else { +vfio_info_cap_shift (&caps,sizeof (*info)); +if (copy_to_user ((void __user *)arg +sizeof (*info), + caps.buf, caps.size)) { +kfree (caps.buf); +return -EFAULT ; + } + info->cap_offset =sizeof (*info); + } + +kfree (caps.buf); + + } } +return 0 ; }static int vfio_ccw_mdev_get_irq_info (struct vfio_irq_info *info)@@ -316,6 +427,32 @@ static int vfio_ccw_mdev_set_irqs(struct mdev_device *mdev, } } +int vfio_ccw_register_dev_region (struct vfio_ccw_private *private, +unsigned int subtype, +const struct vfio_ccw_regops *ops, +size_t size,u32 flags,void *data) +{ +struct vfio_ccw_region *region; + + region =krealloc (private->region, + (private->num_regions +1 ) *sizeof (*region), +GFP_KERNEL ); +if (!region) +return -ENOMEM ; + + private->region = region; + private->region[private->num_regions].type =VFIO_REGION_TYPE_CCW ; + private->region[private->num_regions].subtype = subtype; + private->region[private->num_regions].ops = ops; + private->region[private->num_regions].size = size; + private->region[private->num_regions].flags = flags; + private->region[private->num_regions].data = data; + + private->num_regions++; + +return 0 ; +} +static ssize_t vfio_ccw_mdev_ioctl (struct mdev_device *mdev,unsigned int cmd,unsigned long arg)@@ -336,7 +473,7 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,if (info.argsz < minsz)return -EINVAL ; - ret =vfio_ccw_mdev_get_device_info (&info); + ret =vfio_ccw_mdev_get_device_info (&info, mdev);if (ret)return ret;@@ -345,8 +482,6 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,case VFIO_DEVICE_GET_REGION_INFO : {struct vfio_region_info info; -u16 cap_type_id =0 ; -void *cap_type =NULL ; minsz = offsetofend(struct vfio_region_info, offset);@@ -356,8 +491,7 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,if (info.argsz < minsz)return -EINVAL ; - ret =vfio_ccw_mdev_get_region_info (&info, &cap_type_id, - &cap_type); + ret =vfio_ccw_mdev_get_region_info (&info, mdev, arg);if (ret)return ret;drivers/s390/cio/vfio_ccw_private.h +38
-0@@ -3,9 +3,11 @@* Private stuff for vfio_ccw driver * * Copyright IBM Corp. 2017 +* Copyright Red Hat, Inc. 2019 * * Author(s): Dong Jia Shi <[email protected]> * Xiao Feng Ren <[email protected]> +* Cornelia Huck <[email protected]> */ #ifndef _VFIO_CCW_PRIVATE_H_@@ -19,6 +21,38 @@#include "css.h" #include "vfio_ccw_cp.h" +#define VFIO_CCW_OFFSET_SHIFT 10 +#define VFIO_CCW_OFFSET_TO_INDEX (off) (off >> VFIO_CCW_OFFSET_SHIFT) +#define VFIO_CCW_INDEX_TO_OFFSET (index) ((u64)(index) << VFIO_CCW_OFFSET_SHIFT) +#define VFIO_CCW_OFFSET_MASK (((u64)(1) << VFIO_CCW_OFFSET_SHIFT) - 1) + +/* capability chain handling similar to vfio-pci */ +struct vfio_ccw_private ; +struct vfio_ccw_region ; + +struct vfio_ccw_regops { +ssize_t (*read)(struct vfio_ccw_private *private,char __user *buf, +size_t count,loff_t *ppos); +ssize_t (*write)(struct vfio_ccw_private *private, +const char __user *buf,size_t count,loff_t *ppos); +void (*release)(struct vfio_ccw_private *private, +struct vfio_ccw_region *region); +}; + +struct vfio_ccw_region { +u32 type; +u32 subtype; +const struct vfio_ccw_regops *ops; +void *data; +size_t size; +u32 flags; +}; + +int vfio_ccw_register_dev_region (struct vfio_ccw_private *private, +unsigned int subtype, +const struct vfio_ccw_regops *ops, +size_t size,u32 flags,void *data); +/** * struct vfio_ccw_private * @sch: pointer to the subchannel @@ -29,6 +63,8 @@* @nb: notifier for vfio events * @io_region: MMIO region to input/output I/O arguments/results * @io_mutex: protect against concurrent update of I/O regions +* @region: additional regions for other subchannel operations +* @num_regions: number of additional regions * @cp: channel program for the current I/O operation * @irb: irb info received from interrupt * @scsw: scsw info @@ -44,6 +80,8 @@ struct vfio_ccw_private {struct notifier_block nb;struct ccw_io_region *io_region;struct mutex io_mutex; +struct vfio_ccw_region *region; +int num_regions;struct channel_program cp;struct irb irb;include/uapi/linux/vfio.h +2
-0@@ -353,6 +353,8 @@ struct vfio_region_gfx_edid {#define VFIO_DEVICE_GFX_LINK_STATE_DOWN 2 }; +#define VFIO_REGION_TYPE_CCW (2) +/* * 10de vendor sub-type *
Re: [PATCH v4 4/6] vfio-ccw: add capabilities chain
In reply to Cornelia HuckQuoted 9 lines · Cornelia Huck
Allow to extend the regions used by vfio-ccw. The first user will be handling of halt and clear subchannel.
Signed-off-by Cornelia Huck<[email protected]>--- drivers/s390/cio/vfio_ccw_ops.c | 186 ++++++++++++++++++++++++---- drivers/s390/cio/vfio_ccw_private.h | 38 ++++++ include/uapi/linux/vfio.h | 2 + 3 files changed, 200 insertions(+), 26 deletions(-)
PATCH v4 5/6 s390/cio: export hsch to modules
In reply to Cornelia HuckThe vfio-ccw code will need this, and it matches treatment of ssch and csch.
--- drivers/s390/cio/ioasm.c | 1 + 1 file changed, 1 insertion(+)
drivers/s390/cio/ioasm.c +1 -0
@@ -233,6 +233,7 @@ int hsch(struct subchannel_id schid)return ccode; } +EXPORT_SYMBOL (hsch);static inline int __xsch (struct subchannel_id schid) {
Signature · 1 line
2.17.2
PATCH v4 6/6 vfio-ccw: add handling for async channel instructions
In reply to Cornelia HuckAdd 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.
--- drivers/s390/cio/Makefile | 3 +- drivers/s390/cio/vfio_ccw_async.c | 88 ++++++++++++++++++++ drivers/s390/cio/vfio_ccw_drv.c | 46 ++++++++--- drivers/s390/cio/vfio_ccw_fsm.c | 119 +++++++++++++++++++++++++++- drivers/s390/cio/vfio_ccw_ops.c | 13 ++- drivers/s390/cio/vfio_ccw_private.h | 5 ++ include/uapi/linux/vfio.h | 2 + include/uapi/linux/vfio_ccw.h | 12 +++ 8 files changed, 270 insertions(+), 18 deletions(-) create mode 100644 drivers/s390/cio/vfio_ccw_async.c
drivers/s390/cio/Makefile +2 -1
@@ -20,5 +20,6 @@ obj-$(CONFIG_CCWGROUP) += ccwgroup.o qdio-objs := qdio_main.o qdio_thinint.o qdio_debug.o qdio_setup.o obj-$(CONFIG_QDIO) += qdio.o -vfio_ccw-objs += vfio_ccw_drv.o vfio_ccw_cp.o vfio_ccw_ops.o vfio_ccw_fsm.o +vfio_ccw-objs += vfio_ccw_drv.o vfio_ccw_cp.o vfio_ccw_ops.o vfio_ccw_fsm.o \ + vfio_ccw_async.o obj-$(CONFIG_VFIO_CCW) += vfio_ccw.o
drivers/s390/cio/vfio_ccw_async.c new file +88 -0
88 lines not on this page · raw message
drivers/s390/cio/vfio_ccw_drv.c +33 -13
105 lines not on this page · raw message
drivers/s390/cio/vfio_ccw_fsm.c +117 -2
180 lines not on this page · raw message
drivers/s390/cio/vfio_ccw_ops.c +11 -2
@@ -149,11 +149,20 @@ static int vfio_ccw_mdev_open(struct mdev_device *mdev)struct vfio_ccw_private *private =dev_get_drvdata (mdev_parent_dev (mdev));unsigned long events =VFIO_IOMMU_NOTIFY_DMA_UNMAP ; +int ret; private->nb.notifier_call = vfio_ccw_mdev_notifier; -return vfio_register_notifier (mdev_dev (mdev),VFIO_IOMMU_NOTIFY , - &events, &private->nb); + ret =vfio_register_notifier (mdev_dev (mdev),VFIO_IOMMU_NOTIFY , + &events, &private->nb); +if (ret) +return ret; + + ret =vfio_ccw_register_async_dev_regions (private); +if (ret) +vfio_unregister_notifier (mdev_dev (mdev),VFIO_IOMMU_NOTIFY , + &private->nb); +return ret; }static void vfio_ccw_mdev_release (struct mdev_device *mdev)
drivers/s390/cio/vfio_ccw_private.h +5 -0
@@ -53,6 +53,8 @@ int vfio_ccw_register_dev_region(struct vfio_ccw_private *private,const struct vfio_ccw_regops *ops, size_t size, u32 flags, void *data); +int vfio_ccw_register_async_dev_regions (struct vfio_ccw_private *private); +/** * struct vfio_ccw_private * @sch: pointer to the subchannel
@@ -64,6 +66,7 @@ int vfio_ccw_register_dev_region(struct vfio_ccw_private *private,* @io_region: MMIO region to input/output I/O arguments/results * @io_mutex: protect against concurrent update of I/O regions * @region: additional regions for other subchannel operations +* @cmd_region: MMIO region for asynchronous I/O commands other than START * @num_regions: number of additional regions * @cp: channel program for the current I/O operation * @irb: irb info received from interrupt
@@ -81,6 +84,7 @@ struct vfio_ccw_private {struct ccw_io_region *io_region;struct mutex io_mutex;struct vfio_ccw_region *region; +struct ccw_cmd_region *cmd_region;int num_regions;struct channel_program cp;
@@ -116,6 +120,7 @@ enum vfio_ccw_event {VFIO_CCW_EVENT_NOT_OPER ,VFIO_CCW_EVENT_IO_REQ ,VFIO_CCW_EVENT_INTERRUPT , +VFIO_CCW_EVENT_ASYNC_REQ ,/* last element! */ NR_VFIO_CCW_EVENTS };
include/uapi/linux/vfio.h +2 -0
@@ -354,6 +354,8 @@ struct vfio_region_gfx_edid { };#define VFIO_REGION_TYPE_CCW (2) +/* ccw sub-types */ +#define VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD (1)/* * 10de vendor sub-type
include/uapi/linux/vfio_ccw.h +12 -0
@@ -12,6 +12,7 @@#include <linux/types.h> +/* used for START SUBCHANNEL, always present */ struct ccw_io_region {#define ORB_AREA_SIZE 12__u8 orb_area[ORB_AREA_SIZE ];
@@ -22,4 +23,15 @@ struct ccw_io_region { __u32 ret_code; } __packed; +/* +* used for processing commands that trigger asynchronous actions +* Note: this is controlled by a capability +*/ +#define VFIO_CCW_ASYNC_CMD_HSCH (1 << 0) +#define VFIO_CCW_ASYNC_CMD_CSCH (1 << 1) +struct ccw_cmd_region { +__u32 command; +__u32 ret_code; +} __packed; +#endif
Signature · 1 line
2.17.2
Re: [PATCH v4 6/6] vfio-ccw: add handling for async channel instructions
Quoted 8 lines · Cornelia Huck
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 <[email protected]>
This all looks pretty sensible to me. Sorry my interminable delays!
Quoted 531 lines
--- drivers/s390/cio/Makefile | 3 +- drivers/s390/cio/vfio_ccw_async.c | 88 ++++++++++++++++++++ drivers/s390/cio/vfio_ccw_drv.c | 46 ++++++++--- drivers/s390/cio/vfio_ccw_fsm.c | 119 +++++++++++++++++++++++++++- drivers/s390/cio/vfio_ccw_ops.c | 13 ++- drivers/s390/cio/vfio_ccw_private.h | 5 ++ include/uapi/linux/vfio.h | 2 + include/uapi/linux/vfio_ccw.h | 12 +++ 8 files changed, 270 insertions(+), 18 deletions(-)create mode 100644
drivers/s390/cio/vfio_ccw_async.cdrivers/s390/cio/Makefile +2
-1@@ -20,5 +20,6 @@ obj-$(CONFIG_CCWGROUP) += ccwgroup.o qdio-objs := qdio_main.o qdio_thinint.o qdio_debug.o qdio_setup.o obj-$(CONFIG_QDIO) += qdio.o -vfio_ccw-objs += vfio_ccw_drv.o vfio_ccw_cp.o vfio_ccw_ops.o vfio_ccw_fsm.o +vfio_ccw-objs += vfio_ccw_drv.o vfio_ccw_cp.o vfio_ccw_ops.o vfio_ccw_fsm.o \ + vfio_ccw_async.o obj-$(CONFIG_VFIO_CCW) += vfio_ccw.odrivers/s390/cio/vfio_ccw_async.c new file +88
-0@@ -0,0 +1,88 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Async I/O region for vfio_ccw +* +* Copyright Red Hat, Inc. 2019 +* +* Author(s): Cornelia Huck <[email protected]> +*/ + +#include <linux/vfio.h> +#include <linux/mdev.h> + +#include "vfio_ccw_private.h" + +static ssize_t vfio_ccw_async_region_read (struct vfio_ccw_private *private, +char __user *buf,size_t count, +loff_t *ppos) +{ +unsigned int i =VFIO_CCW_OFFSET_TO_INDEX (*ppos) -VFIO_CCW_NUM_REGIONS ; +loff_t pos = *ppos &VFIO_CCW_OFFSET_MASK ; +struct ccw_cmd_region *region; +int ret; + +if (pos + count >sizeof (*region)) +return -EINVAL ; + +mutex_lock (&private->io_mutex); + region = private->region[i].data; +if (copy_to_user (buf, (void *)region + pos, count)) + ret = -EFAULT ; +else + ret = count; +mutex_unlock (&private->io_mutex); +return ret; +} + +static ssize_t vfio_ccw_async_region_write (struct vfio_ccw_private *private, +const char __user *buf,size_t count, +loff_t *ppos) +{ +unsigned int i =VFIO_CCW_OFFSET_TO_INDEX (*ppos) -VFIO_CCW_NUM_REGIONS ; +loff_t pos = *ppos &VFIO_CCW_OFFSET_MASK ; +struct ccw_cmd_region *region; +int ret; + +if (pos + count >sizeof (*region)) +return -EINVAL ; + +if (!mutex_trylock (&private->io_mutex)) +return -EAGAIN ; + + region = private->region[i].data; +if (copy_from_user ((void *)region + pos, buf, count)) { + ret = -EFAULT ; + goto out_unlock; + } + +vfio_ccw_fsm_event (private,VFIO_CCW_EVENT_ASYNC_REQ ); + + ret = region->ret_code ? region->ret_code : count; + +out_unlock: +mutex_unlock (&private->io_mutex); +return ret; +} + +static void vfio_ccw_async_region_release (struct vfio_ccw_private *private, +struct vfio_ccw_region *region) +{ + +} + +const struct vfio_ccw_regops vfio_ccw_async_region_ops = { + .read = vfio_ccw_async_region_read, + .write = vfio_ccw_async_region_write, + .release = vfio_ccw_async_region_release, +}; + +int vfio_ccw_register_async_dev_regions (struct vfio_ccw_private *private) +{ +return vfio_ccw_register_dev_region (private, +VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD , + &vfio_ccw_async_region_ops, +sizeof (struct ccw_cmd_region ), +VFIO_REGION_INFO_FLAG_READ | +VFIO_REGION_INFO_FLAG_WRITE , + private->cmd_region); +}drivers/s390/cio/vfio_ccw_drv.c +33
-13@@ -3,9 +3,11 @@* VFIO based Physical Subchannel device driver * * Copyright IBM Corp. 2017 +* Copyright Red Hat, Inc. 2019 * * Author(s): Dong Jia Shi <[email protected]> * Xiao Feng Ren <[email protected]> +* Cornelia Huck <[email protected]> */ #include <linux/module.h> @@ -23,6 +25,7 @@struct workqueue_struct *vfio_ccw_work_q;static struct kmem_cache *vfio_ccw_io_region; +static struct kmem_cache *vfio_ccw_cmd_region;/* * Helpers @@ -110,7 +113,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch) {struct pmcw *pmcw = &sch->schib.pmcw;struct vfio_ccw_private *private; -int ret; +int ret = -ENOMEM ;if (pmcw->qf) { dev_warn(&sch->dev, "vfio: ccw: does not support QDIO: %s\n",@@ -124,10 +127,13 @@ static int vfio_ccw_sch_probe(struct subchannel *sch) private->io_region =kmem_cache_zalloc (vfio_ccw_io_region,GFP_KERNEL |GFP_DMA ); -if (!private->io_region) { -kfree (private); -return -ENOMEM ; - } +if (!private->io_region) + goto out_free; + + private->cmd_region =kmem_cache_zalloc (vfio_ccw_cmd_region, +GFP_KERNEL |GFP_DMA ); +if (!private->cmd_region) + goto out_free; private->sch = sch;dev_set_drvdata (&sch->dev, private);@@ -155,7 +161,10 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)cio_disable_subchannel (sch); out_free:dev_set_drvdata (&sch->dev,NULL ); -kmem_cache_free (vfio_ccw_io_region, private->io_region); +if (private->cmd_region) +kmem_cache_free (vfio_ccw_cmd_region, private->cmd_region); +if (private->io_region) +kmem_cache_free (vfio_ccw_io_region, private->io_region);kfree (private);return ret; }@@ -170,6 +179,7 @@ static int vfio_ccw_sch_remove(struct subchannel *sch)dev_set_drvdata (&sch->dev,NULL ); +kmem_cache_free (vfio_ccw_cmd_region, private->cmd_region);kmem_cache_free (vfio_ccw_io_region, private->io_region);kfree (private);@@ -244,7 +254,7 @@ static struct css_driver vfio_ccw_sch_driver = {static int __init vfio_ccw_sch_init (void ) { -int ret; +int ret = -ENOMEM ; vfio_ccw_work_q =create_singlethread_workqueue ("vfio-ccw" );if (!vfio_ccw_work_q)@@ -254,20 +264,30 @@ static int __init vfio_ccw_sch_init(void)sizeof (struct ccw_io_region ),0 ,SLAB_ACCOUNT ,0 , sizeof(struct ccw_io_region), NULL); -if (!vfio_ccw_io_region) { -destroy_workqueue (vfio_ccw_work_q); -return -ENOMEM ; - } +if (!vfio_ccw_io_region) + goto out_err; + + vfio_ccw_cmd_region =kmem_cache_create_usercopy ("vfio_ccw_cmd_region" , +sizeof (struct ccw_cmd_region ),0 , +SLAB_ACCOUNT ,0 , +sizeof (struct ccw_cmd_region ),NULL ); +if (!vfio_ccw_cmd_region) + goto out_err;isc_register (VFIO_CCW_ISC ); ret =css_driver_register (&vfio_ccw_sch_driver);if (ret) {isc_unregister (VFIO_CCW_ISC ); -kmem_cache_destroy (vfio_ccw_io_region); -destroy_workqueue (vfio_ccw_work_q); + goto out_err; }return ret; + +out_err: +kmem_cache_destroy (vfio_ccw_cmd_region); +kmem_cache_destroy (vfio_ccw_io_region); +destroy_workqueue (vfio_ccw_work_q); +return ret; } static void __exit vfio_ccw_sch_exit(void)drivers/s390/cio/vfio_ccw_fsm.c +117
-2@@ -3,8 +3,10 @@* Finite state machine for vfio-ccw device handling * * Copyright IBM Corp. 2017 +* Copyright Red Hat, Inc. 2019 * * Author(s): Dong Jia Shi <[email protected]> +* Cornelia Huck <[email protected]> */ #include <linux/vfio.h> @@ -73,6 +75,75 @@ static int fsm_io_helper(struct vfio_ccw_private *private)return ret; } +static int fsm_do_halt (struct vfio_ccw_private *private) +{ +struct subchannel *sch; +unsigned long flags; +int ccode; +int ret; + + sch = private->sch; + +spin_lock_irqsave (sch->lock, flags); + +/* Issue "Halt Subchannel" */ + ccode =hsch (sch->schid); + +switch (ccode) { +case 0 : +/* +* Initialize device status information +*/ + sch->schib.scsw.cmd.actl |=SCSW_ACTL_HALT_PEND ; + ret =0 ; +break ; +case 1 :/* Status pending */ +case 2 :/* Busy */ + ret = -EBUSY ; +break ; +case 3 :/* Device not operational */ + ret = -ENODEV ; +break ; +default : + ret = ccode; + } +spin_unlock_irqrestore (sch->lock, flags); +return ret; +} + +static int fsm_do_clear (struct vfio_ccw_private *private) +{ +struct subchannel *sch; +unsigned long flags; +int ccode; +int ret; + + sch = private->sch; + +spin_lock_irqsave (sch->lock, flags); + +/* Issue "Clear Subchannel" */ + ccode =csch (sch->schid); + +switch (ccode) { +case 0 : +/* +* Initialize device status information +*/ + sch->schib.scsw.cmd.actl =SCSW_ACTL_CLEAR_PEND ; +/* TODO: check what else we might need to clear */ + ret =0 ; +break ; +case 3 :/* Device not operational */ + ret = -ENODEV ; +break ; +default : + ret = ccode; + } +spin_unlock_irqrestore (sch->lock, flags); +return ret; +} +static void fsm_notoper (struct vfio_ccw_private *private,enum vfio_ccw_event event) {@@ -113,6 +184,24 @@ static void fsm_io_retry(struct vfio_ccw_private *private, private->io_region->ret_code = -EAGAIN ; } +static void fsm_async_error (struct vfio_ccw_private *private, +enum vfio_ccw_event event) +{ +struct ccw_cmd_region *cmd_region = private->cmd_region; + +pr_err ("vfio-ccw: FSM: %s request from state:%d\n" , + cmd_region->command ==VFIO_CCW_ASYNC_CMD_HSCH ?"halt" : + cmd_region->command ==VFIO_CCW_ASYNC_CMD_CSCH ?"clear" : +"<unknown>" , private->state); + cmd_region->ret_code = -EIO ; +} + +static void fsm_async_retry (struct vfio_ccw_private *private, +enum vfio_ccw_event event) +{ + private->cmd_region->ret_code = -EAGAIN ; +} +static void fsm_disabled_irq (struct vfio_ccw_private *private,enum vfio_ccw_event event) {@@ -176,11 +265,11 @@ static void fsm_io_request(struct vfio_ccw_private *private, }return ; }else if (scsw->cmd.fctl &SCSW_FCTL_HALT_FUNC ) { -/* XXX: Handle halt. */ +/* halt is handled via the async cmd region */ io_region->ret_code = -EOPNOTSUPP ; goto err_out; }else if (scsw->cmd.fctl &SCSW_FCTL_CLEAR_FUNC ) { -/* XXX: Handle clear. */ +/* clear is handled via the async cmd region */ io_region->ret_code = -EOPNOTSUPP ; goto err_out; }@@ -190,6 +279,27 @@ static void fsm_io_request(struct vfio_ccw_private *private, io_region->ret_code, errstr); } +/* +* Deal with an async request from userspace. +*/ +static void fsm_async_request (struct vfio_ccw_private *private, +enum vfio_ccw_event event) +{ +struct ccw_cmd_region *cmd_region = private->cmd_region; + +switch (cmd_region->command) { +case VFIO_CCW_ASYNC_CMD_HSCH : + cmd_region->ret_code =fsm_do_halt (private); +break ; +case VFIO_CCW_ASYNC_CMD_CSCH : + cmd_region->ret_code =fsm_do_clear (private); +break ; +default : +/* should not happen? */ + cmd_region->ret_code = -EINVAL ; + } +} +/* * Got an interrupt for a normal io (state busy). */ @@ -213,26 +323,31 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = { [VFIO_CCW_STATE_NOT_OPER ] = { [VFIO_CCW_EVENT_NOT_OPER ] = fsm_nop, [VFIO_CCW_EVENT_IO_REQ ] = fsm_io_error, + [VFIO_CCW_EVENT_ASYNC_REQ ] = fsm_async_error, [VFIO_CCW_EVENT_INTERRUPT ] = fsm_disabled_irq, }, [VFIO_CCW_STATE_STANDBY ] = { [VFIO_CCW_EVENT_NOT_OPER ] = fsm_notoper, [VFIO_CCW_EVENT_IO_REQ ] = fsm_io_error, + [VFIO_CCW_EVENT_ASYNC_REQ ] = fsm_async_error, [VFIO_CCW_EVENT_INTERRUPT ] = fsm_irq, }, [VFIO_CCW_STATE_IDLE ] = { [VFIO_CCW_EVENT_NOT_OPER ] = fsm_notoper, [VFIO_CCW_EVENT_IO_REQ ] = fsm_io_request, + [VFIO_CCW_EVENT_ASYNC_REQ ] = fsm_async_request, [VFIO_CCW_EVENT_INTERRUPT ] = fsm_irq, }, [VFIO_CCW_STATE_CP_PROCESSING ] = { [VFIO_CCW_EVENT_NOT_OPER ] = fsm_notoper, [VFIO_CCW_EVENT_IO_REQ ] = fsm_io_retry, + [VFIO_CCW_EVENT_ASYNC_REQ ] = fsm_async_retry, [VFIO_CCW_EVENT_INTERRUPT ] = fsm_irq, }, [VFIO_CCW_STATE_CP_PENDING ] = { [VFIO_CCW_EVENT_NOT_OPER ] = fsm_notoper, [VFIO_CCW_EVENT_IO_REQ ] = fsm_io_busy, + [VFIO_CCW_EVENT_ASYNC_REQ ] = fsm_async_request, [VFIO_CCW_EVENT_INTERRUPT ] = fsm_irq, }, };drivers/s390/cio/vfio_ccw_ops.c +11
-2@@ -149,11 +149,20 @@ static int vfio_ccw_mdev_open(struct mdev_device *mdev)struct vfio_ccw_private *private =dev_get_drvdata (mdev_parent_dev (mdev));unsigned long events =VFIO_IOMMU_NOTIFY_DMA_UNMAP ; +int ret; private->nb.notifier_call = vfio_ccw_mdev_notifier; -return vfio_register_notifier (mdev_dev (mdev),VFIO_IOMMU_NOTIFY , - &events, &private->nb); + ret =vfio_register_notifier (mdev_dev (mdev),VFIO_IOMMU_NOTIFY , + &events, &private->nb); +if (ret) +return ret; + + ret =vfio_ccw_register_async_dev_regions (private); +if (ret) +vfio_unregister_notifier (mdev_dev (mdev),VFIO_IOMMU_NOTIFY , + &private->nb); +return ret; }static void vfio_ccw_mdev_release (struct mdev_device *mdev)drivers/s390/cio/vfio_ccw_private.h +5
-0@@ -53,6 +53,8 @@ int vfio_ccw_register_dev_region(struct vfio_ccw_private *private,const struct vfio_ccw_regops *ops, size_t size, u32 flags, void *data); +int vfio_ccw_register_async_dev_regions (struct vfio_ccw_private *private); +/** * struct vfio_ccw_private * @sch: pointer to the subchannel @@ -64,6 +66,7 @@ int vfio_ccw_register_dev_region(struct vfio_ccw_private *private,* @io_region: MMIO region to input/output I/O arguments/results * @io_mutex: protect against concurrent update of I/O regions * @region: additional regions for other subchannel operations +* @cmd_region: MMIO region for asynchronous I/O commands other than START * @num_regions: number of additional regions * @cp: channel program for the current I/O operation * @irb: irb info received from interrupt @@ -81,6 +84,7 @@ struct vfio_ccw_private {struct ccw_io_region *io_region;struct mutex io_mutex;struct vfio_ccw_region *region; +struct ccw_cmd_region *cmd_region;int num_regions;struct channel_program cp;@@ -116,6 +120,7 @@ enum vfio_ccw_event {VFIO_CCW_EVENT_NOT_OPER ,VFIO_CCW_EVENT_IO_REQ ,VFIO_CCW_EVENT_INTERRUPT , +VFIO_CCW_EVENT_ASYNC_REQ ,/* last element! */ NR_VFIO_CCW_EVENTS };include/uapi/linux/vfio.h +2
-0@@ -354,6 +354,8 @@ struct vfio_region_gfx_edid { };#define VFIO_REGION_TYPE_CCW (2) +/* ccw sub-types */ +#define VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD (1)/* * 10de vendor sub-type include/uapi/linux/vfio_ccw.h +12
-0@@ -12,6 +12,7 @@#include <linux/types.h> +/* used for START SUBCHANNEL, always present */ struct ccw_io_region {#define ORB_AREA_SIZE 12__u8 orb_area[ORB_AREA_SIZE ];@@ -22,4 +23,15 @@ struct ccw_io_region { __u32 ret_code; } __packed; +/* +* used for processing commands that trigger asynchronous actions +* Note: this is controlled by a capability +*/ +#define VFIO_CCW_ASYNC_CMD_HSCH (1 << 0) +#define VFIO_CCW_ASYNC_CMD_CSCH (1 << 1) +struct ccw_cmd_region { +__u32 command; +__u32 ret_code; +} __packed; +#endif
Re: [PATCH v4 6/6] vfio-ccw: add handling for async channel instructions
In reply to Cornelia HuckQuoted 8 lines · Cornelia Huck
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<[email protected]>
Eric Farman
maintainerQuoted 12 lines · Cornelia Huck
[This is the Linux kernel part, git tree is available at
https://git.kernel.org/pub/scm/linux/kernel/git/kvms390/vfio-ccw.git vfio-ccw-eagain-caps-v3The companion QEMU patches are available at
https://github.com/cohuck/qemu vfio-ccw-caps]Currently, vfio-ccw only relays START SUBCHANNEL requests to the real device. This tends to work well for the most common 'good path' scenarios; however, as we emulate {HALT,CLEAR} SUBCHANNEL in QEMU, things like clearing pending requests at the device is currently not supported. This may be a problem for e.g. error recovery.
Quoted 6 lines
This patch series introduces capabilities (similar to what vfio-pci uses) and exposes a new async region for handling hsch/csch.
Lightly tested (I can interact with a dasd as before, and reserve/release seems to work well.) Not sure if there is a better way to test this, ideas welcome.
Hi Connie,
Thanks for the updated branches! I promise that I'll get back to reviewing them "soon," though I've been on some other tangents as of late.
In terms of testing, Farhan and I have been playing around with some exercisers/data pushers both with and without your new code, and things seem to be okay. Some of the problems we've reproduced without this code (and would fall into the area Pierre's FSM code would address), and some seem wholly unrelated. We're looking into them, but didn't want a whole week to go by without you hearing anything on this series.
If I have nothing meaningful tomorrow, enjoy your weekend!
- Eric
Quoted 51 lines
Changes
v3->v4:
- shrunk the capability offset, we don't need to accommodate as much as pci
- make checks in patch 1 more consistent and less buggy
- rebased on top of my current vfio-ccw branchChanges
v2->v3:
- Unb0rked patch 1, improved scope
- Split out the new mutex from patch 2 into new patch 3; added missing locking and hopefully improved description
- Patch 2 now reworks the state handling by splitting the BUSY state intoCP_PROCESSINGandCP_PENDING
- Patches 3 and 5 adapted on top of the reworked patches; hsch/csch are allowed inCP_PENDING, but not inCP_PROCESSING(did not add any R-b due to that)
- Added missing free in patch 5
- Probably some small changes I forgot to note downChanges
v1->v2:
- New patch 1: make it safe to use the cp accessors at any time; this should avoid problems with unsolicited interrupt handling
- New patch 2: handle concurrent accesses to the io region; the idea is to return-EAGAINto userspace more often (so it can simply retry)
- also handle concurrent accesses to the async io region
- changeVFIO_REGION_TYPE_CCW
- merge events for halt and clear to a single async event; this turned out to make the code quite a bit simpler
- probably some small changes I forgot to note downCornelia Huck (6):
vfio-ccw: make it safe to access channel programs
vfio-ccw: rework ssch state handling
vfio-ccw: protect the I/O region
vfio-ccw: add capabilities chain
s390/cio: export hsch to modules
vfio-ccw: add handling for async channel instructionsdrivers/s390/cio/Makefile | 3 +- drivers/s390/cio/ioasm.c | 1 + drivers/s390/cio/vfio_ccw_async.c | 88 ++++++++++++ drivers/s390/cio/vfio_ccw_cp.c | 21 ++- drivers/s390/cio/vfio_ccw_cp.h | 2 + drivers/s390/cio/vfio_ccw_drv.c | 57 ++++++-- drivers/s390/cio/vfio_ccw_fsm.c | 143 +++++++++++++++++- drivers/s390/cio/vfio_ccw_ops.c | 215 +++++++++++++++++++++++----- drivers/s390/cio/vfio_ccw_private.h | 48 ++++++- include/uapi/linux/vfio.h | 4 + include/uapi/linux/vfio_ccw.h | 12 ++ 11 files changed, 537 insertions(+), 57 deletions(-)create mode 100644
drivers/s390/cio/vfio_ccw_async.c
Quoted 2 lines · On Fri, 1 Mar 2019 10:38:56 +0100 Cornelia Huck <[email protected]> wrote:
[This is the Linux kernel part, git tree is available at
https://git.kernel.org/pub/scm/linux/kernel/git/kvms390/vfio-ccw.git vfio-ccw-eagain-caps-v3
Obviously, that should have been -v4 /o\
In the meanwhile, I have rebased this on top of my vfio-ccw branch with the factored-out non-final interrupt handling and pushed that out to vfio-ccw-eagain-v4.5. The only changes are in patch 2 (removed non-final interrupt handling from the patch and its description); Eric:
I've kept your r-b.
Quoted 67 lines
The companion QEMU patches are available at
https://github.com/cohuck/qemu vfio-ccw-caps]Currently, vfio-ccw only relays START SUBCHANNEL requests to the real device. This tends to work well for the most common 'good path' scenarios; however, as we emulate {HALT,CLEAR} SUBCHANNEL in QEMU, things like clearing pending requests at the device is currently not supported. This may be a problem for e.g. error recovery.
This patch series introduces capabilities (similar to what vfio-pci uses) and exposes a new async region for handling hsch/csch.
Lightly tested (I can interact with a dasd as before, and reserve/release seems to work well.) Not sure if there is a better way to test this, ideas welcome.
Changes
v3->v4:
- shrunk the capability offset, we don't need to accommodate as much as pci
- make checks in patch 1 more consistent and less buggy
- rebased on top of my current vfio-ccw branchChanges
v2->v3:
- Unb0rked patch 1, improved scope
- Split out the new mutex from patch 2 into new patch 3; added missing locking and hopefully improved description
- Patch 2 now reworks the state handling by splitting the BUSY state intoCP_PROCESSINGandCP_PENDING
- Patches 3 and 5 adapted on top of the reworked patches; hsch/csch are allowed inCP_PENDING, but not inCP_PROCESSING(did not add any R-b due to that)
- Added missing free in patch 5
- Probably some small changes I forgot to note downChanges
v1->v2:
- New patch 1: make it safe to use the cp accessors at any time; this should avoid problems with unsolicited interrupt handling
- New patch 2: handle concurrent accesses to the io region; the idea is to return-EAGAINto userspace more often (so it can simply retry)
- also handle concurrent accesses to the async io region
- changeVFIO_REGION_TYPE_CCW
- merge events for halt and clear to a single async event; this turned out to make the code quite a bit simpler
- probably some small changes I forgot to note downCornelia Huck (6):
vfio-ccw: make it safe to access channel programs
vfio-ccw: rework ssch state handling
vfio-ccw: protect the I/O region
vfio-ccw: add capabilities chain
s390/cio: export hsch to modules
vfio-ccw: add handling for async channel instructionsdrivers/s390/cio/Makefile | 3 +- drivers/s390/cio/ioasm.c | 1 + drivers/s390/cio/vfio_ccw_async.c | 88 ++++++++++++ drivers/s390/cio/vfio_ccw_cp.c | 21 ++- drivers/s390/cio/vfio_ccw_cp.h | 2 + drivers/s390/cio/vfio_ccw_drv.c | 57 ++++++-- drivers/s390/cio/vfio_ccw_fsm.c | 143 +++++++++++++++++- drivers/s390/cio/vfio_ccw_ops.c | 215 +++++++++++++++++++++++----- drivers/s390/cio/vfio_ccw_private.h | 48 ++++++- include/uapi/linux/vfio.h | 4 + include/uapi/linux/vfio_ccw.h | 12 ++ 11 files changed, 537 insertions(+), 57 deletions(-) create mode 100644 drivers/s390/cio/vfio_ccw_async.c
Quoted 6 lines · On Fri, 1 Mar 2019 10:38:56 +0100 Cornelia Huck <[email protected]> wrote:
[This is the Linux kernel part, git tree is available at
https://git.kernel.org/pub/scm/linux/kernel/git/kvms390/vfio-ccw.git vfio-ccw-eagain-caps-v3The companion QEMU patches are available at
https://github.com/cohuck/qemu vfio-ccw-caps]
Quoted 8 lines
Currently, vfio-ccw only relays START SUBCHANNEL requests to the real device. This tends to work well for the most common 'good path' scenarios; however, as we emulate {HALT,CLEAR} SUBCHANNEL in QEMU, things like clearing pending requests at the device is currently not supported. This may be a problem for e.g. error recovery.
This patch series introduces capabilities (similar to what vfio-pci uses) and exposes a new async region for handling hsch/csch.
One more gentle ping for an ack/r-b for patches 4 and 6.
I could go ahead and merge patches 1-3 and then patches 1+3 of Farhan's series, but I'd really like to merge patches 4-6 as well so I can later merge the QEMU part after a headers update to 5.2-rc...
Cornelia Huck
AppliedQuoted 55 lines · On Fri, 1 Mar 2019 10:38:56 +0100 Cornelia Huck <[email protected]> wrote:
[This is the Linux kernel part, git tree is available at
https://git.kernel.org/pub/scm/linux/kernel/git/kvms390/vfio-ccw.git vfio-ccw-eagain-caps-v3The companion QEMU patches are available at
https://github.com/cohuck/qemu vfio-ccw-caps]Currently, vfio-ccw only relays START SUBCHANNEL requests to the real device. This tends to work well for the most common 'good path' scenarios; however, as we emulate {HALT,CLEAR} SUBCHANNEL in QEMU, things like clearing pending requests at the device is currently not supported. This may be a problem for e.g. error recovery.
This patch series introduces capabilities (similar to what vfio-pci uses) and exposes a new async region for handling hsch/csch.
Lightly tested (I can interact with a dasd as before, and reserve/release seems to work well.) Not sure if there is a better way to test this, ideas welcome.
Changes
v3->v4:
- shrunk the capability offset, we don't need to accommodate as much as pci
- make checks in patch 1 more consistent and less buggy
- rebased on top of my current vfio-ccw branchChanges
v2->v3:
- Unb0rked patch 1, improved scope
- Split out the new mutex from patch 2 into new patch 3; added missing locking and hopefully improved description
- Patch 2 now reworks the state handling by splitting the BUSY state intoCP_PROCESSINGandCP_PENDING
- Patches 3 and 5 adapted on top of the reworked patches; hsch/csch are allowed inCP_PENDING, but not inCP_PROCESSING(did not add any R-b due to that)
- Added missing free in patch 5
- Probably some small changes I forgot to note downChanges
v1->v2:
- New patch 1: make it safe to use the cp accessors at any time; this should avoid problems with unsolicited interrupt handling
- New patch 2: handle concurrent accesses to the io region; the idea is to return-EAGAINto userspace more often (so it can simply retry)
- also handle concurrent accesses to the async io region
- changeVFIO_REGION_TYPE_CCW
- merge events for halt and clear to a single async event; this turned out to make the code quite a bit simpler
- probably some small changes I forgot to note downCornelia Huck (6):
vfio-ccw: make it safe to access channel programs
vfio-ccw: rework ssch state handling
vfio-ccw: protect the I/O region
vfio-ccw: add capabilities chain
s390/cio: export hsch to modules
vfio-ccw: add handling for async channel instructions
Quoted 14 lines
drivers/s390/cio/Makefile | 3 +- drivers/s390/cio/ioasm.c | 1 + drivers/s390/cio/vfio_ccw_async.c | 88 ++++++++++++ drivers/s390/cio/vfio_ccw_cp.c | 21 ++- drivers/s390/cio/vfio_ccw_cp.h | 2 + drivers/s390/cio/vfio_ccw_drv.c | 57 ++++++-- drivers/s390/cio/vfio_ccw_fsm.c | 143 +++++++++++++++++- drivers/s390/cio/vfio_ccw_ops.c | 215 +++++++++++++++++++++++----- drivers/s390/cio/vfio_ccw_private.h | 48 ++++++- include/uapi/linux/vfio.h | 4 + include/uapi/linux/vfio_ccw.h | 12 ++ 11 files changed, 537 insertions(+), 57 deletions(-) create mode 100644 drivers/s390/cio/vfio_ccw_async.c
Queued (with the touchups discussed in the thread) to the vfio-ccw branch on kernel.org.