Re: [Qemu-devel] [RFC PATCH 1/3] vfio: ccw: introduce schib region

[Cornelia Huck]

On Thu, 11 Jan 2018 04:04:19 +0100
Dong Jia Shi <address@hidden> wrote:

> This introduces a new region for vfio-ccw to provide subchannel
> information for user space.
> 
> Signed-off-by: Dong Jia Shi <address@hidden>
> ---
>  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.