kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Eric Farman <farman@linux.ibm.com>
To: Cornelia Huck <cohuck@redhat.com>
Cc: Jared Rossi <jrossi@linux.ibm.com>,
	Halil Pasic <pasic@linux.ibm.com>,
	linux-s390@vger.kernel.org, kvm@vger.kernel.org,
	Eric Farman <farman@linux.ibm.com>
Subject: [RFC PATCH v2 0/4] vfio-ccw: Fix interrupt handling for HALT/CLEAR
Date: Wed, 13 May 2020 16:29:30 +0200	[thread overview]
Message-ID: <20200513142934.28788-1-farman@linux.ibm.com> (raw)

Hi Conny,

Back in January, I suggested a small patch [1] to try to clean up
the handling of HSCH/CSCH interrupts, especially as it relates to
concurrent SSCH interrupts. Here is a new attempt to address this.

There was some suggestion earlier about locking the FSM, but I'm not
seeing any problems with that. Rather, what I'm noticing is that the
flow between a synchronous START and asynchronous HALT/CLEAR have
different impacts on the FSM state. Consider:

    CPU 1                           CPU 2

    SSCH (set state=CP_PENDING)
    INTERRUPT (set state=IDLE)
    CSCH (no change in state)
                                    SSCH (set state=CP_PENDING)
    INTERRUPT (set state=IDLE)
                                    INTERRUPT (set state=IDLE)

The second interrupt on CPU 1 will call cp_free() for the START
created by CPU 2, and our results will be, um, messy. This
suggests that three things must be true:

 A) cp_free() must be called for either a final interrupt,
or a failure issuing a SSCH
 B) The FSM state must only be set to IDLE once cp_free()
has been called
 C) A SSCH cannot be issued while an interrupt is outstanding

It's not great that items B and C are separated in the interrupt
path by a mutex boundary where the copy into io_region occurs.
We could (and perhaps should) move them together, which would
improve things somewhat, but still doesn't address the scenario
laid out above. Even putting that work within the mutex window
during interrupt processing doesn't address things totally.

So, the patches here do two things. One to handle unsolicited
interrupts [2], which I think is pretty straightforward. Though,
it does make me want to do a more drastic rearranging of the
affected function. :)

The other warrants some discussion, since it's sort of weird.
Basically, recognize what commands we've issued, expect interrupts
for each, and prevent new SSCH's while asynchronous commands are
pending. It doesn't address interrupts from the HSCH/CSCH that
could be generated by the Channel Path Handling code [3] for an
offline CHPID yet, and it needs some TLC to make checkpatch happy.
But it's the best possibility I've seen thus far.

I used private->scsw for this because it's barely used for anything
else; at one point I had been considering removing it outright because
we have entirely too many SCSW's in this code. :) I could implement
this as three small flags in private, to simplify the code and avoid
confusion with the different fctl/actl flags in private vs schib.

It does make me wonder whether the CP_PENDING check before cp_free()
is still necessary, but that's a query for a later date.

Also, perhaps this could be accomplished with two new FSM states,
one for a HALT and one for a CLEAR. I put it aside because the
interrupt handler got a little hairy with that, but I'm still looking
at it in parallel to this discussion.

I look forward to hearing your thoughts...

[1] https://lore.kernel.org/kvm/20200124145455.51181-1-farman@linux.ibm.com/
[2] https://lore.kernel.org/kvm/9635c45f-4652-c837-d256-46f426737a5e@linux.ibm.com/
[3] https://lore.kernel.org/kvm/20200505122745.53208-1-farman@linux.ibm.com/

Eric Farman (4):
  vfio-ccw: Do not reset FSM state for unsolicited interrupts
  vfio-ccw: Utilize scsw actl to serialize start operations
  vfio-ccw: Expand SCSW usage to HALT and CLEAR
  vfio-ccw: Clean up how to react to a failed START

 drivers/s390/cio/vfio_ccw_drv.c | 10 +++++++++-
 drivers/s390/cio/vfio_ccw_fsm.c | 26 ++++++++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_ops.c |  3 +--
 3 files changed, 36 insertions(+), 3 deletions(-)


             reply	other threads:[~2020-05-13 14:29 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-13 14:29 Eric Farman [this message]
2020-05-13 14:29 ` [RFC PATCH v2 1/4] vfio-ccw: Do not reset FSM state for unsolicited interrupts Eric Farman
2020-05-13 14:29 ` [RFC PATCH v2 2/4] vfio-ccw: Utilize scsw actl to serialize start operations Eric Farman
2020-05-13 14:29 ` [RFC PATCH v2 3/4] vfio-ccw: Expand SCSW usage to HALT and CLEAR Eric Farman
2020-05-13 14:29 ` [RFC PATCH v2 4/4] vfio-ccw: Clean up how to react to a failed START Eric Farman
2020-05-14 13:46 ` [RFC PATCH v2 0/4] vfio-ccw: Fix interrupt handling for HALT/CLEAR Halil Pasic
2020-05-15 13:09   ` Eric Farman
2020-05-15 14:55     ` Halil Pasic
2020-05-15 15:58       ` Cornelia Huck
2020-05-15 17:41         ` Halil Pasic
2020-05-15 18:19           ` Eric Farman
2020-05-15 18:12       ` Eric Farman
2020-05-15 18:37         ` Halil Pasic
2020-05-18 22:01           ` Eric Farman
2020-05-15 19:35         ` Halil Pasic
2020-05-18 16:09 ` Cornelia Huck
2020-05-18 21:57   ` Eric Farman
2020-05-19 11:23     ` Cornelia Huck
2020-05-18 22:09   ` Halil Pasic
2020-05-19 11:36     ` Cornelia Huck
2020-05-19 12:10       ` Halil Pasic
2020-05-26  9:55 ` Cornelia Huck
2020-05-26 11:08   ` Eric Farman

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:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200513142934.28788-1-farman@linux.ibm.com \
    --to=farman@linux.ibm.com \
    --cc=cohuck@redhat.com \
    --cc=jrossi@linux.ibm.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=pasic@linux.ibm.com \


* 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).