[PATCH 0/1] vfio-ccw: Enable transparent CCW IPL from DASD

[PATCH 0/1] vfio-ccw: Enable transparent CCW IPL from DASD

[Jared Rossi]

Remove the explicit prefetch check when using vfio-ccw devices.
This check is not needed as all Linux channel programs are intended
to use prefetch and will be executed in the same way regardless.

Jared Rossi (1):
  vfio-ccw: Enable transparent CCW IPL from DASD

 drivers/s390/cio/vfio_ccw_cp.c  | 16 ++++------------
 drivers/s390/cio/vfio_ccw_cp.h  |  2 +-
 drivers/s390/cio/vfio_ccw_fsm.c |  6 +++---
 3 files changed, 8 insertions(+), 16 deletions(-)

-- 
2.17.0

[PATCH 1/1] vfio-ccw: Enable transparent CCW IPL from DASD

[Jared Rossi]

Remove the explicit prefetch check when using vfio-ccw devices.
This check is not needed as all Linux channel programs are intended
to use prefetch and will be executed in the same way regardless.

Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c  | 16 ++++------------
 drivers/s390/cio/vfio_ccw_cp.h  |  2 +-
 drivers/s390/cio/vfio_ccw_fsm.c |  6 +++---
 3 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 3645d1720c4b..5b47ecbb4baa 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -625,9 +625,8 @@ static int ccwchain_fetch_one(struct ccwchain *chain,
  * the target channel program from @orb->cmd.iova to the new ccwchain(s).
  *
  * Limitations:
- * 1. Supports only prefetch enabled mode.
- * 2. Supports idal(c64) ccw chaining.
- * 3. Supports 4k idaw.
+ * 1. Supports idal(c64) ccw chaining.
+ * 2. Supports 4k idaw.
  *
  * Returns:
  *   %0 on success and a negative error value on failure.
@@ -636,13 +635,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
 {
 	int ret;
 
-	/*
-	 * XXX:
-	 * Only support prefetch enable mode now.
-	 */
-	if (!orb->cmd.pfch)
-		return -EOPNOTSUPP;
-
 	INIT_LIST_HEAD(&cp->ccwchain_list);
 	memcpy(&cp->orb, orb, sizeof(*orb));
 	cp->mdev = mdev;
@@ -690,7 +682,7 @@ void cp_free(struct channel_program *cp)
 }
 
 /**
- * cp_prefetch() - translate a guest physical address channel program to
+ * cp_fetch() - translate a guest physical address channel program to
  *                 a real-device runnable channel program.
  * @cp: channel_program on which to perform the operation
  *
@@ -726,7 +718,7 @@ void cp_free(struct channel_program *cp)
  * Returns:
  *   %0 on success and a negative error value on failure.
  */
-int cp_prefetch(struct channel_program *cp)
+int cp_fetch(struct channel_program *cp)
 {
 	struct ccwchain *chain;
 	int len, idx, ret;
diff --git a/drivers/s390/cio/vfio_ccw_cp.h b/drivers/s390/cio/vfio_ccw_cp.h
index ba31240ce965..a226f6e99d04 100644
--- a/drivers/s390/cio/vfio_ccw_cp.h
+++ b/drivers/s390/cio/vfio_ccw_cp.h
@@ -45,7 +45,7 @@ struct channel_program {
 extern int cp_init(struct channel_program *cp, struct device *mdev,
 		   union orb *orb);
 extern void cp_free(struct channel_program *cp);
-extern int cp_prefetch(struct channel_program *cp);
+extern int cp_fetch(struct channel_program *cp);
 extern union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm);
 extern void cp_update_scsw(struct channel_program *cp, union scsw *scsw);
 extern bool cp_iova_pinned(struct channel_program *cp, u64 iova);
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 23e61aa638e4..446f9186d070 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -274,14 +274,14 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 			goto err_out;
 		}
 
-		io_region->ret_code = cp_prefetch(&private->cp);
+		io_region->ret_code = cp_fetch(&private->cp);
 		if (io_region->ret_code) {
 			VFIO_CCW_MSG_EVENT(2,
-					   "%pUl (%x.%x.%04x): cp_prefetch=%d\n",
+					   "%pUl (%x.%x.%04x): cp_fetch=%d\n",
 					   mdev_uuid(mdev), schid.cssid,
 					   schid.ssid, schid.sch_no,
 					   io_region->ret_code);
-			errstr = "cp prefetch";
+			errstr = "cp fetch";
 			cp_free(&private->cp);
 			goto err_out;
 		}
-- 
2.17.0

Re: [PATCH 1/1] vfio-ccw: Enable transparent CCW IPL from DASD

[Cornelia Huck]

On Fri, 17 Apr 2020 14:29:39 -0400
Jared Rossi <jrossi@linux.ibm.com> wrote:

> Remove the explicit prefetch check when using vfio-ccw devices.
> This check is not needed as all Linux channel programs are intended
> to use prefetch and will be executed in the same way regardless.

Hm... my understanding is that we have the reasonable expectation that
our guests will always issue channel programs that work fine with
prefetch even if the bit is not set in the orb (including CCW IPL, in
the way it is implemented in the s390-ccw QEMU bios), and therefore
this patch is just making things less complicated.

If a future guest does issue a channel program where that does not hold
true, it will run into trouble, and I'm not sure that this would be
easy to debug. Can we log this somewhere?

Also, it might make sense to add some note of our
behaviour/expectations to Documentation/s390/vfio-ccw.rst.

> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c  | 16 ++++------------
>  drivers/s390/cio/vfio_ccw_cp.h  |  2 +-
>  drivers/s390/cio/vfio_ccw_fsm.c |  6 +++---
>  3 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 3645d1720c4b..5b47ecbb4baa 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
(...)
> @@ -690,7 +682,7 @@ void cp_free(struct channel_program *cp)
>  }
>  
>  /**
> - * cp_prefetch() - translate a guest physical address channel program to
> + * cp_fetch() - translate a guest physical address channel program to

I would not rename this -- we are still doing what is called
'prefetch', even though we also do it if the guest did not instruct us
to do so.

(also below)

>   *                 a real-device runnable channel program.
>   * @cp: channel_program on which to perform the operation
>   *

Re: [PATCH 1/1] vfio-ccw: Enable transparent CCW IPL from DASD

[Halil Pasic]

On Mon, 20 Apr 2020 14:13:17 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> > Remove the explicit prefetch check when using vfio-ccw devices.
> > This check is not needed as all Linux channel programs are intended
> > to use prefetch and will be executed in the same way regardless.  
> 
> Hm... my understanding is that we have the reasonable expectation that
> our guests will always issue channel programs that work fine with
> prefetch even if the bit is not set in the orb (including CCW IPL, in
> the way it is implemented in the s390-ccw QEMU bios), and therefore
> this patch is just making things less complicated.

AFAIR the problem is not s390-ccw QEMU bios. We could easily fix that.
The practical problem is some channel programs generated by zipl that
simply fail to set the bit (although they could). That is a perfectly
legit thing to do, because the prefetch bit was originally about
performance.

Sorry I missed your mail, so complained about the same
issues.

Regards,
Halil

Re: [PATCH 1/1] vfio-ccw: Enable transparent CCW IPL from DASD

[Halil Pasic]

On Fri, 17 Apr 2020 14:29:39 -0400
Jared Rossi <jrossi@linux.ibm.com> wrote:

> Remove the explicit prefetch check when using vfio-ccw devices.
> This check is not needed as all Linux channel programs are intended
> to use prefetch and will be executed in the same way regardless.

Hm. This is a guest thing or? So you basically say, it is OK to do
this, because you know that the guest is gonna be Linux and that it
the channel program is intended to use prefetch -- but the ORB supplied
by the guest that designates the channel program happens to state the
opposite.

Or am I missing something?

Regards,
Halil 

Re: [PATCH 1/1] vfio-ccw: Enable transparent CCW IPL from DASD

[Cornelia Huck]

On Thu, 23 Apr 2020 15:56:20 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Fri, 17 Apr 2020 14:29:39 -0400
> Jared Rossi <jrossi@linux.ibm.com> wrote:
> 
> > Remove the explicit prefetch check when using vfio-ccw devices.
> > This check is not needed as all Linux channel programs are intended
> > to use prefetch and will be executed in the same way regardless.  
> 
> Hm. This is a guest thing or? So you basically say, it is OK to do
> this, because you know that the guest is gonna be Linux and that it
> the channel program is intended to use prefetch -- but the ORB supplied
> by the guest that designates the channel program happens to state the
> opposite.
> 
> Or am I missing something?

I see this as a kind of architecture compliance/ease of administration
tradeoff, as we none of the guests we currently support uses something
that breaks with prefetching outside of IPL (which has a different
workaround).

One thing that still concerns me a bit is debuggability if a future
guest indeed does want to dynamically rewrite a channel program: the
guest thinks it instructed the device to not prefetch, and then
suddenly things do not work as expected. We can log when a guest
submits an orb without prefetch set, but we can't find out if the guest
actually does something that relies on non-prefetch.

The only correct way to handle this would be to actually implement
non-prefetch processing, where I would not really know where to even
start -- and then we'd only have synthetic test cases, for now. None of
the options are pleasant :(

Re: [PATCH 1/1] vfio-ccw: Enable transparent CCW IPL from DASD

[Eric Farman]

On 4/23/20 11:11 AM, Cornelia Huck wrote:
> On Thu, 23 Apr 2020 15:56:20 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
>> On Fri, 17 Apr 2020 14:29:39 -0400
>> Jared Rossi <jrossi@linux.ibm.com> wrote:
>>
>>> Remove the explicit prefetch check when using vfio-ccw devices.
>>> This check is not needed as all Linux channel programs are intended
>>> to use prefetch and will be executed in the same way regardless.  
>>
>> Hm. This is a guest thing or? So you basically say, it is OK to do
>> this, because you know that the guest is gonna be Linux and that it
>> the channel program is intended to use prefetch -- but the ORB supplied
>> by the guest that designates the channel program happens to state the
>> opposite.
>>
>> Or am I missing something?
> 
> I see this as a kind of architecture compliance/ease of administration
> tradeoff, as we none of the guests we currently support uses something
> that breaks with prefetching outside of IPL (which has a different
> workaround).>
> One thing that still concerns me a bit is debuggability if a future
> guest indeed does want to dynamically rewrite a channel program: the

+1 for some debuggability, just in general

> guest thinks it instructed the device to not prefetch, and then
> suddenly things do not work as expected. We can log when a guest
> submits an orb without prefetch set, but we can't find out if the guest
> actually does something that relies on non-prefetch.

Without going too far down a non-prefetch rabbit-hole, can we use the
cpa_within_range logic to see if the address of the CCW being fetched
exists as the CDA of an earlier (non-TIC) CCW in the chain we're
processing, and tracing/logging/messaging something about a possible
conflict?

(Jared, you did some level of this tracing with our real/synthetic tests
some time ago.  Any chance something of it could be polished and made
useful, without being overly heavy on the mainline path?)

> 
> The only correct way to handle this would be to actually implement
> non-prefetch processing, where I would not really know where to even
> start -- and then we'd only have synthetic test cases, for now. None of
> the options are pleasant :(
> 

And even if we knew where to start, it's quite a bit of effort for the
hypothetical.  From conversations I've had with long-time I/O folks,
non-prefetch seems to be the significant minority these days, dating
back to older CKD devices (and associated connectivity) in practice.

Re: [PATCH 1/1] vfio-ccw: Enable transparent CCW IPL from DASD

[Halil Pasic]

On Thu, 23 Apr 2020 16:25:39 -0400
Eric Farman <farman@linux.ibm.com> wrote:

> 
> 
> On 4/23/20 11:11 AM, Cornelia Huck wrote:
> > On Thu, 23 Apr 2020 15:56:20 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> > 
> >> On Fri, 17 Apr 2020 14:29:39 -0400
> >> Jared Rossi <jrossi@linux.ibm.com> wrote:
> >>
> >>> Remove the explicit prefetch check when using vfio-ccw devices.
> >>> This check is not needed as all Linux channel programs are intended
> >>> to use prefetch and will be executed in the same way regardless.  
> >>
> >> Hm. This is a guest thing or? So you basically say, it is OK to do
> >> this, because you know that the guest is gonna be Linux and that it
> >> the channel program is intended to use prefetch -- but the ORB supplied
> >> by the guest that designates the channel program happens to state the
> >> opposite.
> >>
> >> Or am I missing something?
> > 
> > I see this as a kind of architecture compliance/ease of administration
> > tradeoff, as we none of the guests we currently support uses something
> > that breaks with prefetching outside of IPL (which has a different
> > workaround).>

And that workaround AFAIR makes sure that we don't issue a CP that is
self-modifying or otherwise reliant on non-prefetch. So any time we see
a self-modifying program we know, we have an incompatible setup.

In any case I believe the commit message is inadequate, as it does not
reflect about the risks.

> > One thing that still concerns me a bit is debuggability if a future
> > guest indeed does want to dynamically rewrite a channel program: the
> 
> +1 for some debuggability, just in general
> 
> > guest thinks it instructed the device to not prefetch, and then
> > suddenly things do not work as expected. We can log when a guest
> > submits an orb without prefetch set, but we can't find out if the guest
> > actually does something that relies on non-prefetch.
> 
> Without going too far down a non-prefetch rabbit-hole, can we use the
> cpa_within_range logic to see if the address of the CCW being fetched
> exists as the CDA of an earlier (non-TIC) CCW in the chain we're
> processing, and tracing/logging/messaging something about a possible
> conflict?
> 
> (Jared, you did some level of this tracing with our real/synthetic tests
> some time ago.  Any chance something of it could be polished and made
> useful, without being overly heavy on the mainline path?)
> 

Back then I believe I made a proposal on how this logic could look like.
I think all we need is checking for self rewrites (ccw reads to the
addresses that comprise the  complete original channel program), and for
status-modifier 'skips'. The latter could be easily done by putting some
sort of poison at the end of the detected channel program segments.

> > 
> > The only correct way to handle this would be to actually implement
> > non-prefetch processing, where I would not really know where to even
> > start -- and then we'd only have synthetic test cases, for now. None of
> > the options are pleasant :(
> > 
> 

I don't think implementing non-prefetch processing is possible with
vfio-ccw. 

Regards,
Halil

Re: [PATCH 1/1] vfio-ccw: Enable transparent CCW IPL from DASD

[Jared Rossi]

On 2020-04-24 08:50, Halil Pasic wrote:
> On Thu, 23 Apr 2020 16:25:39 -0400
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> 
>> 
>> On 4/23/20 11:11 AM, Cornelia Huck wrote:
>> > On Thu, 23 Apr 2020 15:56:20 +0200
>> > Halil Pasic <pasic@linux.ibm.com> wrote:
>> >
>> >> On Fri, 17 Apr 2020 14:29:39 -0400
>> >> Jared Rossi <jrossi@linux.ibm.com> wrote:
>> >>
>> >>> Remove the explicit prefetch check when using vfio-ccw devices.
>> >>> This check is not needed as all Linux channel programs are intended
>> >>> to use prefetch and will be executed in the same way regardless.
>> >>
>> >> Hm. This is a guest thing or? So you basically say, it is OK to do
>> >> this, because you know that the guest is gonna be Linux and that it
>> >> the channel program is intended to use prefetch -- but the ORB supplied
>> >> by the guest that designates the channel program happens to state the
>> >> opposite.
>> >>
>> >> Or am I missing something?
>> >
>> > I see this as a kind of architecture compliance/ease of administration
>> > tradeoff, as we none of the guests we currently support uses something
>> > that breaks with prefetching outside of IPL (which has a different
>> > workaround).>
> 
> And that workaround AFAIR makes sure that we don't issue a CP that is
> self-modifying or otherwise reliant on non-prefetch. So any time we see
> a self-modifying program we know, we have an incompatible setup.
> 
> In any case I believe the commit message is inadequate, as it does not
> reflect about the risks.
> 
>> > One thing that still concerns me a bit is debuggability if a future
>> > guest indeed does want to dynamically rewrite a channel program: the
>> 
>> +1 for some debuggability, just in general
>> 
>> > guest thinks it instructed the device to not prefetch, and then
>> > suddenly things do not work as expected. We can log when a guest
>> > submits an orb without prefetch set, but we can't find out if the guest
>> > actually does something that relies on non-prefetch.
>> 
>> Without going too far down a non-prefetch rabbit-hole, can we use the
>> cpa_within_range logic to see if the address of the CCW being fetched
>> exists as the CDA of an earlier (non-TIC) CCW in the chain we're
>> processing, and tracing/logging/messaging something about a possible
>> conflict?
>> 
>> (Jared, you did some level of this tracing with our real/synthetic 
>> tests
>> some time ago.  Any chance something of it could be polished and made
>> useful, without being overly heavy on the mainline path?)
>> 
> 
> Back then I believe I made a proposal on how this logic could look 
> like.
> I think all we need is checking for self rewrites (ccw reads to the
> addresses that comprise the  complete original channel program), and 
> for
> status-modifier 'skips'. The latter could be easily done by putting 
> some
> sort of poison at the end of the detected channel program segments.
> 

 From what I previously did with the tracing, I don't think that there is 
a
practical way to determine if a cp is actually doing something that 
relies
on non-prefetch.  It seems we would need to examine the CCWs to find 
reads
and also validate the addresses those CCWs access to check if there is a
conflict.  Probably this is too much overhead considering that we expect
it to be a rare occurrence?

Is it too simplistic to print a kernel warning stating that an ORB did 
not
have the p-bit set, but it is being prefetched anyway?

Regards,
Jared Rossi