On Thu, 11 Jan 2018 04:04:19 +0100
Dong Jia Shi <bjsdjshi@xxxxxxxxxxxxxxxxxx> wrote:
> This introduces a new region for vfio-ccw to provide subchannel
> information for user space.
>
> Signed-off-by: Dong Jia Shi <bjsdjshi@xxxxxxxxxxxxxxxxxx>
> ---
> drivers/s390/cio/vfio_ccw_fsm.c | 21 ++++++++++
> drivers/s390/cio/vfio_ccw_ops.c | 79 +++++++++++++++++++++++++++----------
> drivers/s390/cio/vfio_ccw_private.h | 3 ++
> include/uapi/linux/vfio.h | 1 +
> include/uapi/linux/vfio_ccw.h | 6 +++
> 5 files changed, 90 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index c30420c517b1..be081ccabea3 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -172,6 +172,22 @@ static void fsm_irq(struct vfio_ccw_private *private,
> complete(private->completion);
> }
>
> +static void fsm_update_subch(struct vfio_ccw_private *private,
> + enum vfio_ccw_event event)
> +{
> + struct subchannel *sch;
> +
> + sch = private->sch;
> + if (cio_update_schib(sch)) {
This implies device gone. Do we also want to trigger some event, or
just wait until a machine check comes around and we're notified in the
normal way? (Probably the latter.)
> + private->schib_region.cc = 3;
> + return;
> + }
> +
> + private->schib_region.cc = 0;
> + memcpy(private->schib_region.schib_area, &sch->schib,
> + sizeof(sch->schib));
We might want to add documentation that schib_area contains the schib
from the last successful invocation of stsch (if any). That makes sense
as the schib remains unchanged for cc=3 after stsch anyway, but it
can't hurt to spell it out.
> +}
> +
> /*
> * Device statemachine
> */
> @@ -180,25 +196,30 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
> [VFIO_CCW_EVENT_NOT_OPER] = fsm_nop,
> [VFIO_CCW_EVENT_IO_REQ] = fsm_io_error,
> [VFIO_CCW_EVENT_INTERRUPT] = fsm_disabled_irq,
> + [VFIO_CCW_EVENT_UPDATE_SUBCH] = fsm_update_subch,
> },
> [VFIO_CCW_STATE_STANDBY] = {
> [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
> [VFIO_CCW_EVENT_IO_REQ] = fsm_io_error,
> [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq,
> + [VFIO_CCW_EVENT_UPDATE_SUBCH] = fsm_update_subch,
> },
> [VFIO_CCW_STATE_IDLE] = {
> [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
> [VFIO_CCW_EVENT_IO_REQ] = fsm_io_request,
> [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq,
> + [VFIO_CCW_EVENT_UPDATE_SUBCH] = fsm_update_subch,
> },
> [VFIO_CCW_STATE_BOXED] = {
> [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
> [VFIO_CCW_EVENT_IO_REQ] = fsm_io_busy,
> [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq,
> + [VFIO_CCW_EVENT_UPDATE_SUBCH] = fsm_update_subch,
> },
> [VFIO_CCW_STATE_BUSY] = {
> [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
> [VFIO_CCW_EVENT_IO_REQ] = fsm_io_busy,
> [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq,
> + [VFIO_CCW_EVENT_UPDATE_SUBCH] = fsm_update_subch,
Does it makes to trigger this through the state machine if we always do
the same action and never change state?
> },
> };
Else, looks good.
