From c6c82e0cd8125d30f2f1b29205c7e1a2f1a6785b Mon Sep 17 00:00:00 2001 From: Eric Farman Date: Tue, 11 May 2021 21:56:29 +0200 Subject: vfio-ccw: Check initialized flag in cp_init() We have a really nice flag in the channel_program struct that indicates if it had been initialized by cp_init(), and use it as a guard in the other cp accessor routines, but not for a duplicate call into cp_init(). The possibility of this occurring is low, because that flow is protected by the private->io_mutex and FSM CP_PROCESSING state. But then why bother checking it in (for example) cp_prefetch() then? Let's just be consistent and check for that in cp_init() too. Fixes: 71189f263f8a3 ("vfio-ccw: make it safe to access channel programs") Signed-off-by: Eric Farman Reviewed-by: Cornelia Huck Acked-by: Matthew Rosato Message-Id: <20210511195631.3995081-2-farman@linux.ibm.com> Signed-off-by: Cornelia Huck --- drivers/s390/cio/vfio_ccw_cp.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'drivers/s390') diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c index b9febc581b1f..8d1b2771c1aa 100644 --- a/drivers/s390/cio/vfio_ccw_cp.c +++ b/drivers/s390/cio/vfio_ccw_cp.c @@ -638,6 +638,10 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) static DEFINE_RATELIMIT_STATE(ratelimit_state, 5 * HZ, 1); int ret; + /* this is an error in the caller */ + if (cp->initialized) + return -EBUSY; + /* * We only support prefetching the channel program. We assume all channel * programs executed by supported guests likewise support prefetching. -- cgit v1.2.3 From 6c02ac4c9211edabe17bda437ac97e578756f31b Mon Sep 17 00:00:00 2001 From: Eric Farman Date: Tue, 11 May 2021 21:56:30 +0200 Subject: vfio-ccw: Reset FSM state to IDLE inside FSM When an I/O request is made, the fsm_io_request() routine moves the FSM state from IDLE to CP_PROCESSING, and then fsm_io_helper() moves it to CP_PENDING if the START SUBCHANNEL received a cc0. Yet, the error case to go from CP_PROCESSING back to IDLE is done after the FSM call returns. Let's move this up into the FSM proper, to provide some better symmetry when unwinding in this case. Signed-off-by: Eric Farman Reviewed-by: Cornelia Huck Acked-by: Matthew Rosato Message-Id: <20210511195631.3995081-3-farman@linux.ibm.com> Signed-off-by: Cornelia Huck --- drivers/s390/cio/vfio_ccw_fsm.c | 1 + drivers/s390/cio/vfio_ccw_ops.c | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) (limited to 'drivers/s390') diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c index 23e61aa638e4..e435a9cd92da 100644 --- a/drivers/s390/cio/vfio_ccw_fsm.c +++ b/drivers/s390/cio/vfio_ccw_fsm.c @@ -318,6 +318,7 @@ static void fsm_io_request(struct vfio_ccw_private *private, } err_out: + private->state = VFIO_CCW_STATE_IDLE; trace_vfio_ccw_fsm_io_request(scsw->cmd.fctl, schid, io_region->ret_code, errstr); } diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c index 491a64c61fff..c57d2a7f0919 100644 --- a/drivers/s390/cio/vfio_ccw_ops.c +++ b/drivers/s390/cio/vfio_ccw_ops.c @@ -279,8 +279,6 @@ static ssize_t vfio_ccw_mdev_write_io_region(struct vfio_ccw_private *private, } vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_IO_REQ); - if (region->ret_code != 0) - private->state = VFIO_CCW_STATE_IDLE; ret = (region->ret_code != 0) ? region->ret_code : count; out_unlock: -- cgit v1.2.3 From 2af7a834a435460d546f0cf0a8b8e4d259f1d910 Mon Sep 17 00:00:00 2001 From: Eric Farman Date: Tue, 11 May 2021 21:56:31 +0200 Subject: vfio-ccw: Serialize FSM IDLE state with I/O completion Today, the stacked call to vfio_ccw_sch_io_todo() does three things: 1) Update a solicited IRB with CP information, and release the CP if the interrupt was the end of a START operation. 2) Copy the IRB data into the io_region, under the protection of the io_mutex 3) Reset the vfio-ccw FSM state to IDLE to acknowledge that vfio-ccw can accept more work. The trouble is that step 3 is (A) invoked for both solicited and unsolicited interrupts, and (B) sitting after the mutex for step 2. This second piece becomes a problem if it processes an interrupt for a CLEAR SUBCHANNEL while another thread initiates a START, thus allowing the CP and FSM states to get out of sync. That is: CPU 1 CPU 2 fsm_do_clear() fsm_irq() fsm_io_request() vfio_ccw_sch_io_todo() fsm_io_helper() Since the FSM state and CP should be kept in sync, let's make a note when the CP is released, and rely on that as an indication that the FSM should also be reset at the end of this routine and open up the device for more work. Signed-off-by: Eric Farman Acked-by: Matthew Rosato Reviewed-by: Cornelia Huck Message-Id: <20210511195631.3995081-4-farman@linux.ibm.com> Signed-off-by: Cornelia Huck --- drivers/s390/cio/vfio_ccw_drv.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'drivers/s390') diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c index 8c625b530035..9b61e9b131ad 100644 --- a/drivers/s390/cio/vfio_ccw_drv.c +++ b/drivers/s390/cio/vfio_ccw_drv.c @@ -86,6 +86,7 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work) struct vfio_ccw_private *private; struct irb *irb; bool is_final; + bool cp_is_finished = false; private = container_of(work, struct vfio_ccw_private, io_work); irb = &private->irb; @@ -94,14 +95,21 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work) (SCSW_ACTL_DEVACT | SCSW_ACTL_SCHACT)); if (scsw_is_solicited(&irb->scsw)) { cp_update_scsw(&private->cp, &irb->scsw); - if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING) + if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING) { cp_free(&private->cp); + cp_is_finished = true; + } } mutex_lock(&private->io_mutex); memcpy(private->io_region->irb_area, irb, sizeof(*irb)); mutex_unlock(&private->io_mutex); - if (private->mdev && is_final) + /* + * Reset to IDLE only if processing of a channel program + * has finished. Do not overwrite a possible processing + * state if the final interrupt was for HSCH or CSCH. + */ + if (private->mdev && cp_is_finished) private->state = VFIO_CCW_STATE_IDLE; if (private->io_trigger) -- cgit v1.2.3 From c0c8a8397fa8a74d04915f4d3d28cb4a5d401427 Mon Sep 17 00:00:00 2001 From: Stefan Haberland Date: Tue, 25 May 2021 14:50:06 +0200 Subject: s390/dasd: add missing discipline function Fix crash with illegal operation exception in dasd_device_tasklet. Commit b72949328869 ("s390/dasd: Prepare for additional path event handling") renamed the verify_path function for ECKD but not for FBA and DIAG. This leads to a panic when the path verification function is called for a FBA or DIAG device. Fix by defining a wrapper function for dasd_generic_verify_path(). Fixes: b72949328869 ("s390/dasd: Prepare for additional path event handling") Cc: #5.11 Reviewed-by: Jan Hoeppner Signed-off-by: Stefan Haberland Reviewed-by: Cornelia Huck Link: https://lore.kernel.org/r/20210525125006.157531-2-sth@linux.ibm.com Signed-off-by: Jens Axboe --- drivers/s390/block/dasd_diag.c | 8 +++++++- drivers/s390/block/dasd_fba.c | 8 +++++++- drivers/s390/block/dasd_int.h | 1 - 3 files changed, 14 insertions(+), 3 deletions(-) (limited to 'drivers/s390') diff --git a/drivers/s390/block/dasd_diag.c b/drivers/s390/block/dasd_diag.c index 1b9e1442e6a5..fd42a5fffaed 100644 --- a/drivers/s390/block/dasd_diag.c +++ b/drivers/s390/block/dasd_diag.c @@ -642,12 +642,18 @@ static void dasd_diag_setup_blk_queue(struct dasd_block *block) blk_queue_segment_boundary(q, PAGE_SIZE - 1); } +static int dasd_diag_pe_handler(struct dasd_device *device, + __u8 tbvpm, __u8 fcsecpm) +{ + return dasd_generic_verify_path(device, tbvpm); +} + static struct dasd_discipline dasd_diag_discipline = { .owner = THIS_MODULE, .name = "DIAG", .ebcname = "DIAG", .check_device = dasd_diag_check_device, - .verify_path = dasd_generic_verify_path, + .pe_handler = dasd_diag_pe_handler, .fill_geometry = dasd_diag_fill_geometry, .setup_blk_queue = dasd_diag_setup_blk_queue, .start_IO = dasd_start_diag, diff --git a/drivers/s390/block/dasd_fba.c b/drivers/s390/block/dasd_fba.c index 4789410885e4..3ad319aee51e 100644 --- a/drivers/s390/block/dasd_fba.c +++ b/drivers/s390/block/dasd_fba.c @@ -794,13 +794,19 @@ static void dasd_fba_setup_blk_queue(struct dasd_block *block) blk_queue_flag_set(QUEUE_FLAG_DISCARD, q); } +static int dasd_fba_pe_handler(struct dasd_device *device, + __u8 tbvpm, __u8 fcsecpm) +{ + return dasd_generic_verify_path(device, tbvpm); +} + static struct dasd_discipline dasd_fba_discipline = { .owner = THIS_MODULE, .name = "FBA ", .ebcname = "FBA ", .check_device = dasd_fba_check_characteristics, .do_analysis = dasd_fba_do_analysis, - .verify_path = dasd_generic_verify_path, + .pe_handler = dasd_fba_pe_handler, .setup_blk_queue = dasd_fba_setup_blk_queue, .fill_geometry = dasd_fba_fill_geometry, .start_IO = dasd_start_IO, diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h index 1c59b0e86a9f..155428bfed8a 100644 --- a/drivers/s390/block/dasd_int.h +++ b/drivers/s390/block/dasd_int.h @@ -297,7 +297,6 @@ struct dasd_discipline { * e.g. verify that new path is compatible with the current * configuration. */ - int (*verify_path)(struct dasd_device *, __u8); int (*pe_handler)(struct dasd_device *, __u8, __u8); /* -- cgit v1.2.3