| Message ID | 20180111030421.31418-2-bjsdjshi@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | vfio: ccw: basic channel path event handling | expand |
On Thu, 11 Jan 2018 04:04:19 +0100 Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > This introduces a new region for vfio-ccw to provide subchannel > information for user space. > > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> > --- > 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.
* Cornelia Huck <cohuck@redhat.com> [2018-01-11 15:16:59 +0100]: Hi Conny, > On Thu, 11 Jan 2018 04:04:19 +0100 > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > > > This introduces a new region for vfio-ccw to provide subchannel > > information for user space. > > > > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> > > --- > > 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.) > We'd need to handle machine checks better anyway, and we can trigger event there. I think we can choose the latter one. > > + 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. > PoP doesn't say anything about the content of SCHIB when cc=3. So it's fine to remain the last content I guess. I can add comments here and document in vfio-ccw.txt. Ok? > > +} > > + > > /* > > * 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? Yes. Ah, are you implying that we can call update_subch directly without state machine involved? If so, I agree. There seems no benifit to add a new VFIO_CCW_EVENT_UPDATE_SUBCH event to the FSM. > > > }, > > }; > > Else, looks good. > Thanks for the comments. :)
On 11/01/2018 04:04, Dong Jia Shi wrote: > This introduces a new region for vfio-ccw to provide subchannel > information for user space. > > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> > --- > 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)) { > + private->schib_region.cc = 3; > + return; > + } > + > + private->schib_region.cc = 0; > + memcpy(private->schib_region.schib_area, &sch->schib, > + sizeof(sch->schib)); > +} > + > /* > * 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, > }, > }; > diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c > index 41eeb57d68a3..9528fce2e7d9 100644 > --- a/drivers/s390/cio/vfio_ccw_ops.c > +++ b/drivers/s390/cio/vfio_ccw_ops.c > @@ -13,6 +13,11 @@ > > #include "vfio_ccw_private.h" > > +#define VFIO_CCW_OFFSET_SHIFT 40 > +#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) > + > static int vfio_ccw_mdev_reset(struct mdev_device *mdev) > { > struct vfio_ccw_private *private; > @@ -168,14 +173,31 @@ static ssize_t vfio_ccw_mdev_read(struct mdev_device *mdev, > loff_t *ppos) > { > struct vfio_ccw_private *private; > - struct ccw_io_region *region; > + void *region; > + u32 index; > + loff_t pos; > + > + index = VFIO_CCW_OFFSET_TO_INDEX(*ppos); > + pos = (*ppos) & VFIO_CCW_OFFSET_MASK; > + private = dev_get_drvdata(mdev_parent_dev(mdev)); > > - if (*ppos + count > sizeof(*region)) > + switch (index) { > + case VFIO_CCW_CONFIG_REGION_INDEX: > + if (pos + count > sizeof(struct ccw_io_region)) > + return -EINVAL; > + region = &private->io_region; > + break; > + case VFIO_CCW_SCHIB_REGION_INDEX: > + if (pos + count > sizeof(struct ccw_schib_region)) > + return -EINVAL; > + region = &private->schib_region; > + vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_UPDATE_SUBCH); > + break; > + default: > return -EINVAL; > + } > > - private = dev_get_drvdata(mdev_parent_dev(mdev)); > - region = &private->io_region; > - if (copy_to_user(buf, (void *)region + *ppos, count)) > + if (copy_to_user(buf, region + pos, count)) > return -EFAULT; > > return count; > @@ -187,23 +209,35 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev, > loff_t *ppos) > { > struct vfio_ccw_private *private; > - struct ccw_io_region *region; > - > - if (*ppos + count > sizeof(*region)) > - return -EINVAL; > + u32 index; > + loff_t pos; > > + index = VFIO_CCW_OFFSET_TO_INDEX(*ppos); > + pos = (*ppos) & VFIO_CCW_OFFSET_MASK; > 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)) > - return -EFAULT; > - > - vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_IO_REQ); > - if (region->ret_code != 0) { > - private->state = VFIO_CCW_STATE_IDLE; > - return region->ret_code; > + switch (index) { > + case VFIO_CCW_CONFIG_REGION_INDEX: > + { > + struct ccw_io_region *region; > + if (pos + count > sizeof(*region)) > + return -EINVAL; > + if (private->state != VFIO_CCW_STATE_IDLE) > + return -EACCES; > + region = &private->io_region; > + if (copy_from_user((void *)region + *ppos, buf, count)) > + return -EFAULT; > + vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_IO_REQ); > + if (region->ret_code != 0) { > + private->state = VFIO_CCW_STATE_IDLE; > + return region->ret_code; > + } > + break; > + } > + case VFIO_CCW_SCHIB_REGION_INDEX: > + return -EOPNOTSUPP; > + default: > + return -EINVAL; > } > > return count; > @@ -224,11 +258,16 @@ static int vfio_ccw_mdev_get_region_info(struct vfio_region_info *info, > { > switch (info->index) { > case VFIO_CCW_CONFIG_REGION_INDEX: > - info->offset = 0; > + info->offset = VFIO_CCW_INDEX_TO_OFFSET(info->index); > info->size = sizeof(struct ccw_io_region); > info->flags = VFIO_REGION_INFO_FLAG_READ > | VFIO_REGION_INFO_FLAG_WRITE; > return 0; > + case VFIO_CCW_SCHIB_REGION_INDEX: > + info->offset = VFIO_CCW_INDEX_TO_OFFSET(info->index); > + info->size = sizeof(struct ccw_schib_region); > + info->flags = VFIO_REGION_INFO_FLAG_READ; > + return 0; > default: > return -EINVAL; > } > diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h > index 78a66d96756b..460c8b90d834 100644 > --- a/drivers/s390/cio/vfio_ccw_private.h > +++ b/drivers/s390/cio/vfio_ccw_private.h > @@ -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 > + * @schib_region: MMIO region of subchannel information > * @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 ccw_schib_region schib_region; > > struct channel_program cp; > struct irb irb; Hi, I have a problem with these patches: you have 3 definitions for the subchannel status word: 1: direct in the scsw entry of the vfio_ccw_private structure 2: indirect in the IRB structure irb 3: now in the scsw of the schib_region How do you keep them all in sync? The direct scsw in io region seems to only serve as a trigger used by userland, while the IRB in the io region and the SCHIB in the schib region are updated asynchronously, from hardware, one by polling (scsw in schib region), one by IRQ (scsw in irb in io region). I find this at least a source for obfuscation. Regards, Pierre > @@ -76,6 +78,7 @@ enum vfio_ccw_event { > VFIO_CCW_EVENT_NOT_OPER, > VFIO_CCW_EVENT_IO_REQ, > VFIO_CCW_EVENT_INTERRUPT, > + VFIO_CCW_EVENT_UPDATE_SUBCH, > /* last element! */ > NR_VFIO_CCW_EVENTS > }; > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > index e3301dbd27d4..c60244debf71 100644 > --- a/include/uapi/linux/vfio.h > +++ b/include/uapi/linux/vfio.h > @@ -457,6 +457,7 @@ enum { > > enum { > VFIO_CCW_CONFIG_REGION_INDEX, > + VFIO_CCW_SCHIB_REGION_INDEX, > VFIO_CCW_NUM_REGIONS > }; > > diff --git a/include/uapi/linux/vfio_ccw.h b/include/uapi/linux/vfio_ccw.h > index 2ec5f367ff78..12508ef6e6fc 100644 > --- a/include/uapi/linux/vfio_ccw.h > +++ b/include/uapi/linux/vfio_ccw.h > @@ -22,4 +22,10 @@ struct ccw_io_region { > __u32 ret_code; > } __packed; > > +struct ccw_schib_region { > + __u8 cc; > +#define SCHIB_AREA_SIZE 52 > + __u8 schib_area[SCHIB_AREA_SIZE]; > +} __packed; > + > #endif
On Mon, 15 Jan 2018 14:43:09 +0800 Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > * Cornelia Huck <cohuck@redhat.com> [2018-01-11 15:16:59 +0100]: > > Hi Conny, > > > On Thu, 11 Jan 2018 04:04:19 +0100 > > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > > > > > This introduces a new region for vfio-ccw to provide subchannel > > > information for user space. > > > > > > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> > > > --- > > > 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.) > > > We'd need to handle machine checks better anyway, and we can trigger > event there. I think we can choose the latter one. Agreed. We can tackle the whole machine check complex later, especially as it also has implications for interacting with user space. > > > > + 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. > > > PoP doesn't say anything about the content of SCHIB when cc=3. So it's > fine to remain the last content I guess. I can add comments here and > document in vfio-ccw.txt. Ok? The PoP says "Condition code 3 is set, and no other action is taken" - I'd interpret this as "no content is changed, but you probably should not look at that storage area". I'd hope that the caller does not look at the contents for cc 3, but it's a good idea to document this. > > > > +} > > > + > > > /* > > > * 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? > Yes. Ah, are you implying that we can call update_subch directly without > state machine involved? If so, I agree. There seems no benifit to add > a new VFIO_CCW_EVENT_UPDATE_SUBCH event to the FSM. Yes, that's what I meant. Whatever makes the code easy to understand.
On Mon, 15 Jan 2018 10:50:00 +0100 Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: > On 11/01/2018 04:04, Dong Jia Shi wrote: > > This introduces a new region for vfio-ccw to provide subchannel > > information for user space. > > > > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> > > --- > > 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_private.h b/drivers/s390/cio/vfio_ccw_private.h > > index 78a66d96756b..460c8b90d834 100644 > > --- a/drivers/s390/cio/vfio_ccw_private.h > > +++ b/drivers/s390/cio/vfio_ccw_private.h > > @@ -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 > > + * @schib_region: MMIO region of subchannel information > > * @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 ccw_schib_region schib_region; > > > > struct channel_program cp; > > struct irb irb; > > Hi, > > I have a problem with these patches: you have 3 definitions for the > subchannel status word: > 1: direct in the scsw entry of the vfio_ccw_private structure > 2: indirect in the IRB structure irb > 3: now in the scsw of the schib_region > > How do you keep them all in sync? > > The direct scsw in io region seems to only serve as a trigger used by > userland, while > the IRB in the io region and the SCHIB in the schib region are updated > asynchronously, > from hardware, one by polling (scsw in schib region), one by IRQ (scsw > in irb in io region). > > I find this at least a source for obfuscation. I agree that this wants some more documentation. However, some of this structure duplication is a consequence of the hardware design, because the scsw is present in both the schib (updated by stsch) and the irb (updated by tsch). There are cases where the irb is simply not enough (it does not contain a pmcw, and you can only do tsch on an enabled subchannel). So I think that we really need two structures for those two distinct operations (and everything interfacing with this needs to keep synced on the scsw, as current users of stsch/tsch already need to do now). The direct scsw entry is used for initiating the different functions (only start right now), I don't really see a good alternative for that (and I also don't really see any problem with needed syncing.)
* Cornelia Huck <cohuck@redhat.com> [2018-01-15 13:24:22 +0100]: > On Mon, 15 Jan 2018 10:50:00 +0100 > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: > > > On 11/01/2018 04:04, Dong Jia Shi wrote: > > > This introduces a new region for vfio-ccw to provide subchannel > > > information for user space. > > > > > > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> > > > --- > > > 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_private.h b/drivers/s390/cio/vfio_ccw_private.h > > > index 78a66d96756b..460c8b90d834 100644 > > > --- a/drivers/s390/cio/vfio_ccw_private.h > > > +++ b/drivers/s390/cio/vfio_ccw_private.h > > > @@ -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 > > > + * @schib_region: MMIO region of subchannel information > > > * @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 ccw_schib_region schib_region; > > > > > > struct channel_program cp; > > > struct irb irb; > > > > Hi, > > > > I have a problem with these patches: you have 3 definitions for the > > subchannel status word: > > 1: direct in the scsw entry of the vfio_ccw_private structure > > 2: indirect in the IRB structure irb > > 3: now in the scsw of the schib_region > > > > How do you keep them all in sync? > > For the first two cases, we might need to keep them synced in the kernel if upper level application requires. Otherwise, we can let upper level application do the sync. The 3rd one is used only as an input parameter to indicate function types. To be more specific, we currently only has interests for its Function Control field. So, sync of this one is not applicable. > > The direct scsw in io region seems to only serve as a trigger used by > > userland, while > > the IRB in the io region and the SCHIB in the schib region are updated > > asynchronously, > > from hardware, one by polling (scsw in schib region), one by IRQ (scsw > > in irb in io region). > > > > I find this at least a source for obfuscation. > > I agree that this wants some more documentation. Ok. > > However, some of this structure duplication is a consequence of the > hardware design, because the scsw is present in both the schib (updated > by stsch) and the irb (updated by tsch). There are cases where the irb > is simply not enough (it does not contain a pmcw, and you can only do > tsch on an enabled subchannel). So I think that we really need two > structures for those two distinct operations (and everything > interfacing with this needs to keep synced on the scsw, as current > users of stsch/tsch already need to do now). > Nod. > The direct scsw entry is used for initiating the different functions > (only start right now), I don't really see a good alternative for that > (and I also don't really see any problem with needed syncing.) > +1
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)) { + private->schib_region.cc = 3; + return; + } + + private->schib_region.cc = 0; + memcpy(private->schib_region.schib_area, &sch->schib, + sizeof(sch->schib)); +} + /* * 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, }, }; diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c index 41eeb57d68a3..9528fce2e7d9 100644 --- a/drivers/s390/cio/vfio_ccw_ops.c +++ b/drivers/s390/cio/vfio_ccw_ops.c @@ -13,6 +13,11 @@ #include "vfio_ccw_private.h" +#define VFIO_CCW_OFFSET_SHIFT 40 +#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) + static int vfio_ccw_mdev_reset(struct mdev_device *mdev) { struct vfio_ccw_private *private; @@ -168,14 +173,31 @@ static ssize_t vfio_ccw_mdev_read(struct mdev_device *mdev, loff_t *ppos) { struct vfio_ccw_private *private; - struct ccw_io_region *region; + void *region; + u32 index; + loff_t pos; + + index = VFIO_CCW_OFFSET_TO_INDEX(*ppos); + pos = (*ppos) & VFIO_CCW_OFFSET_MASK; + private = dev_get_drvdata(mdev_parent_dev(mdev)); - if (*ppos + count > sizeof(*region)) + switch (index) { + case VFIO_CCW_CONFIG_REGION_INDEX: + if (pos + count > sizeof(struct ccw_io_region)) + return -EINVAL; + region = &private->io_region; + break; + case VFIO_CCW_SCHIB_REGION_INDEX: + if (pos + count > sizeof(struct ccw_schib_region)) + return -EINVAL; + region = &private->schib_region; + vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_UPDATE_SUBCH); + break; + default: return -EINVAL; + } - private = dev_get_drvdata(mdev_parent_dev(mdev)); - region = &private->io_region; - if (copy_to_user(buf, (void *)region + *ppos, count)) + if (copy_to_user(buf, region + pos, count)) return -EFAULT; return count; @@ -187,23 +209,35 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev, loff_t *ppos) { struct vfio_ccw_private *private; - struct ccw_io_region *region; - - if (*ppos + count > sizeof(*region)) - return -EINVAL; + u32 index; + loff_t pos; + index = VFIO_CCW_OFFSET_TO_INDEX(*ppos); + pos = (*ppos) & VFIO_CCW_OFFSET_MASK; 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)) - return -EFAULT; - - vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_IO_REQ); - if (region->ret_code != 0) { - private->state = VFIO_CCW_STATE_IDLE; - return region->ret_code; + switch (index) { + case VFIO_CCW_CONFIG_REGION_INDEX: + { + struct ccw_io_region *region; + if (pos + count > sizeof(*region)) + return -EINVAL; + if (private->state != VFIO_CCW_STATE_IDLE) + return -EACCES; + region = &private->io_region; + if (copy_from_user((void *)region + *ppos, buf, count)) + return -EFAULT; + vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_IO_REQ); + if (region->ret_code != 0) { + private->state = VFIO_CCW_STATE_IDLE; + return region->ret_code; + } + break; + } + case VFIO_CCW_SCHIB_REGION_INDEX: + return -EOPNOTSUPP; + default: + return -EINVAL; } return count; @@ -224,11 +258,16 @@ static int vfio_ccw_mdev_get_region_info(struct vfio_region_info *info, { switch (info->index) { case VFIO_CCW_CONFIG_REGION_INDEX: - info->offset = 0; + info->offset = VFIO_CCW_INDEX_TO_OFFSET(info->index); info->size = sizeof(struct ccw_io_region); info->flags = VFIO_REGION_INFO_FLAG_READ | VFIO_REGION_INFO_FLAG_WRITE; return 0; + case VFIO_CCW_SCHIB_REGION_INDEX: + info->offset = VFIO_CCW_INDEX_TO_OFFSET(info->index); + info->size = sizeof(struct ccw_schib_region); + info->flags = VFIO_REGION_INFO_FLAG_READ; + return 0; default: return -EINVAL; } diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h index 78a66d96756b..460c8b90d834 100644 --- a/drivers/s390/cio/vfio_ccw_private.h +++ b/drivers/s390/cio/vfio_ccw_private.h @@ -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 + * @schib_region: MMIO region of subchannel information * @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 ccw_schib_region schib_region; struct channel_program cp; struct irb irb; @@ -76,6 +78,7 @@ enum vfio_ccw_event { VFIO_CCW_EVENT_NOT_OPER, VFIO_CCW_EVENT_IO_REQ, VFIO_CCW_EVENT_INTERRUPT, + VFIO_CCW_EVENT_UPDATE_SUBCH, /* last element! */ NR_VFIO_CCW_EVENTS }; diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index e3301dbd27d4..c60244debf71 100644 --- a/include/uapi/linux/vfio.h +++ b/include/uapi/linux/vfio.h @@ -457,6 +457,7 @@ enum { enum { VFIO_CCW_CONFIG_REGION_INDEX, + VFIO_CCW_SCHIB_REGION_INDEX, VFIO_CCW_NUM_REGIONS }; diff --git a/include/uapi/linux/vfio_ccw.h b/include/uapi/linux/vfio_ccw.h index 2ec5f367ff78..12508ef6e6fc 100644 --- a/include/uapi/linux/vfio_ccw.h +++ b/include/uapi/linux/vfio_ccw.h @@ -22,4 +22,10 @@ struct ccw_io_region { __u32 ret_code; } __packed; +struct ccw_schib_region { + __u8 cc; +#define SCHIB_AREA_SIZE 52 + __u8 schib_area[SCHIB_AREA_SIZE]; +} __packed; + #endif
This introduces a new region for vfio-ccw to provide subchannel information for user space. Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> --- 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(-)