From: Cornelia Huck <cohuck@redhat.com>
To: Pierre Morel <pmorel@linux.ibm.com>
Cc: linux-s390@vger.kernel.org, Eric Farman <farman@linux.ibm.com>,
Alex Williamson <alex.williamson@redhat.com>,
kvm@vger.kernel.org, Farhan Ali <alifm@linux.ibm.com>,
qemu-devel@nongnu.org, Halil Pasic <pasic@linux.ibm.com>,
qemu-s390x@nongnu.org
Subject: Re: [PATCH 3/3] vfio-ccw: add handling for asnyc channel instructions
Date: Mon, 26 Nov 2018 10:47:49 +0100 [thread overview]
Message-ID: <20181126104749.1bbff657.cohuck@redhat.com> (raw)
In-Reply-To: <91b54c7e-a7b1-b721-89be-12a5706a0e21@linux.ibm.com>
On Fri, 23 Nov 2018 14:08:03 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:
> On 22/11/2018 17:54, Cornelia Huck wrote:
> > 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 <cohuck@redhat.com>
> > ---
> > drivers/s390/cio/Makefile | 3 +-
> > drivers/s390/cio/vfio_ccw_async.c | 88 ++++++++++++++++
> > drivers/s390/cio/vfio_ccw_drv.c | 48 ++++++---
> > drivers/s390/cio/vfio_ccw_fsm.c | 158 +++++++++++++++++++++++++++-
> > drivers/s390/cio/vfio_ccw_ops.c | 13 ++-
> > drivers/s390/cio/vfio_ccw_private.h | 6 ++
> > include/uapi/linux/vfio.h | 4 +
> > include/uapi/linux/vfio_ccw.h | 12 +++
> > 8 files changed, 313 insertions(+), 19 deletions(-)
> > create mode 100644 drivers/s390/cio/vfio_ccw_async.c
(...)
> > +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);
> > + private->state = VFIO_CCW_STATE_BUSY;
> > +
> > + /* 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;
> > + }
>
> Shouldn't you set the state back here?
This is handled as for ssch, i.e. the state is restored by the caller.
>
> > + 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);
> > + private->state = VFIO_CCW_STATE_BUSY;
> > +
> > + /* 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;
Same here, btw.
> > +}
> > +
> > static void fsm_notoper(struct vfio_ccw_private *private,
> > enum vfio_ccw_event event)
> > {
> > @@ -102,6 +179,20 @@ static void fsm_io_busy(struct vfio_ccw_private *private,
> > private->io_region->ret_code = -EBUSY;
> > }
> >
> > +static void fsm_async_error(struct vfio_ccw_private *private,
> > + enum vfio_ccw_event event)
> > +{
> > + pr_err("vfio-ccw: FSM: halt/clear request from state:%d\n",
> > + private->state);
> > + private->cmd_region->ret_code = -EIO;
> > +}
> > +
> > +static void fsm_async_busy(struct vfio_ccw_private *private,
> > + enum vfio_ccw_event event)
> > +{
> > + private->cmd_region->ret_code = -EBUSY;
> > +}
> > +
> > static void fsm_disabled_irq(struct vfio_ccw_private *private,
> > enum vfio_ccw_event event)
> > {
> > @@ -166,11 +257,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;
>
> What about filtering inside the vfio_ccw_mdev_write_io_region() before
> the call to the FSM?
We can do that as well, maybe as a patch on top. What I like about
doing it here is that all poking into the I/O region is done in one
place. On the other hand, doing it beforehand saves us some churn.
>
>
> > }
> > @@ -181,6 +272,59 @@ static void fsm_io_request(struct vfio_ccw_private *private,
> > io_region->ret_code, errstr);
> > }
> >
> > +/*
> > + * Deal with a halt request from userspace.
> > + */
> > +static void fsm_halt_request(struct vfio_ccw_private *private,
> > + enum vfio_ccw_event event)
> > +{
> > + struct ccw_cmd_region *cmd_region = private->cmd_region;
> > + int state = private->state;
> > +
> > + private->state = VFIO_CCW_STATE_BOXED;
> > +
> > + if (cmd_region->command != VFIO_CCW_ASYNC_CMD_HSCH) {
> > + /* should not happen? */
>
> I think we should make sure it does not happen before we get here.
> Like serializing HALT and CLEAR before the FSM.
Given that there's only one generator of that event, that really should
not happen :) It would mean that we have messed up our code later on.
Maybe complain loudly here?
>
> > + cmd_region->ret_code = -EINVAL;
> > + goto err_out;
> > + }
> > +
> > + cmd_region->ret_code = fsm_do_halt(private);
>
> fsm_do_halt() set the state to BUSY.
> Do we need a state change here and in fsm_do_halt ?
>
> Why not only the BUSY state?
I basically took the ssch implementation and adapted it for halt/clear
handling. We can certainly think about doing state transitions in
different places, but I'd like to do that for all channel instructions
at the same time.
[Also note that this is still based on a version that still contains
the BOXED state.]
>
> > + if (cmd_region->ret_code)
> > + goto err_out;
> > +
> > + return;
> > +
> > +err_out:
> > + private->state = state;
> > +}
> > +
>
> ...snip...
>
> Regards,
> Pierre
>
next prev parent reply other threads:[~2018-11-26 9:47 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-22 16:54 [PATCH 0/3] vfio-ccw: support hsch/csch (kernel part) Cornelia Huck
2018-11-22 16:54 ` [PATCH 1/3] vfio-ccw: add capabilities chain Cornelia Huck
2018-11-23 12:28 ` Pierre Morel
2018-11-23 12:45 ` Cornelia Huck
2018-11-23 13:26 ` Pierre Morel
2018-11-27 19:04 ` Farhan Ali
2018-11-28 9:05 ` Cornelia Huck
2018-12-17 21:53 ` Eric Farman
2018-12-18 17:24 ` Cornelia Huck
2018-12-18 17:56 ` Eric Farman
2018-12-19 16:28 ` Alex Williamson
2018-12-21 11:12 ` Cornelia Huck
2018-11-22 16:54 ` [PATCH 2/3] s390/cio: export hsch to modules Cornelia Huck
2018-11-23 12:30 ` Pierre Morel
2018-11-22 16:54 ` [PATCH 3/3] vfio-ccw: add handling for asnyc channel instructions Cornelia Huck
2018-11-23 13:08 ` Pierre Morel
2018-11-26 9:47 ` Cornelia Huck [this message]
2018-11-27 19:09 ` Farhan Ali
2018-11-28 9:02 ` Cornelia Huck
2018-11-28 14:31 ` Farhan Ali
2018-11-28 14:52 ` Cornelia Huck
2018-11-28 15:00 ` Farhan Ali
2018-11-28 15:35 ` Cornelia Huck
2018-11-28 15:55 ` Farhan Ali
2019-01-18 13:53 ` Cornelia Huck
2018-11-27 19:57 ` Farhan Ali
2018-11-28 8:41 ` Cornelia Huck
2018-11-28 16:36 ` [qemu-s390x] " Halil Pasic
2018-11-29 16:52 ` Cornelia Huck
2018-11-29 17:24 ` Halil Pasic
2018-12-17 21:54 ` Eric Farman
2018-12-18 16:45 ` Cornelia Huck
2018-11-24 21:07 ` [qemu-s390x] [PATCH 0/3] vfio-ccw: support hsch/csch (kernel part) Halil Pasic
2018-11-26 9:26 ` Cornelia Huck
2018-11-26 18:57 ` Farhan Ali
2018-11-26 19:00 ` Cornelia Huck
2018-12-04 12:38 ` Halil Pasic
2018-12-04 13:11 ` Cornelia Huck
2018-12-04 15:02 ` Halil Pasic
2018-12-05 12:54 ` Cornelia Huck
2018-12-05 18:34 ` Farhan Ali
2018-12-06 14:39 ` Cornelia Huck
2018-12-06 15:26 ` Farhan Ali
2018-12-06 16:21 ` Cornelia Huck
2018-12-06 17:50 ` Farhan Ali
2018-12-07 9:34 ` Cornelia Huck
2018-12-06 18:47 ` Halil Pasic
2018-12-07 10:05 ` Cornelia Huck
2018-12-07 15:49 ` Halil Pasic
2018-12-07 16:54 ` Halil Pasic
2018-12-19 11:54 ` Cornelia Huck
2018-12-19 14:17 ` Halil Pasic
2018-12-21 11:23 ` Cornelia Huck
2018-12-21 12:42 ` Halil Pasic
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181126104749.1bbff657.cohuck@redhat.com \
--to=cohuck@redhat.com \
--cc=alex.williamson@redhat.com \
--cc=alifm@linux.ibm.com \
--cc=farman@linux.ibm.com \
--cc=kvm@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=pasic@linux.ibm.com \
--cc=pmorel@linux.ibm.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-s390x@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).