* [PATCH 0/1] vfio-ccw: Enable transparent CCW IPL from DASD @ 2020-04-17 18:29 Jared Rossi 2020-04-17 18:29 ` [PATCH 1/1] " Jared Rossi 0 siblings, 1 reply; 9+ messages in thread From: Jared Rossi @ 2020-04-17 18:29 UTC (permalink / raw) To: Eric Farman, Cornelia Huck; +Cc: linux-s390, kvm, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/1] vfio-ccw: Enable transparent CCW IPL from DASD 2020-04-17 18:29 [PATCH 0/1] vfio-ccw: Enable transparent CCW IPL from DASD Jared Rossi @ 2020-04-17 18:29 ` Jared Rossi 2020-04-20 12:13 ` Cornelia Huck 2020-04-23 13:56 ` Halil Pasic 0 siblings, 2 replies; 9+ messages in thread From: Jared Rossi @ 2020-04-17 18:29 UTC (permalink / raw) To: Eric Farman, Cornelia Huck; +Cc: linux-s390, kvm, linux-kernel 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] vfio-ccw: Enable transparent CCW IPL from DASD 2020-04-17 18:29 ` [PATCH 1/1] " Jared Rossi @ 2020-04-20 12:13 ` Cornelia Huck 2020-04-24 13:02 ` Halil Pasic 2020-04-23 13:56 ` Halil Pasic 1 sibling, 1 reply; 9+ messages in thread From: Cornelia Huck @ 2020-04-20 12:13 UTC (permalink / raw) To: Jared Rossi; +Cc: Eric Farman, linux-s390, kvm, linux-kernel 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 > * ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] vfio-ccw: Enable transparent CCW IPL from DASD 2020-04-20 12:13 ` Cornelia Huck @ 2020-04-24 13:02 ` Halil Pasic 0 siblings, 0 replies; 9+ messages in thread From: Halil Pasic @ 2020-04-24 13:02 UTC (permalink / raw) To: Cornelia Huck; +Cc: Jared Rossi, Eric Farman, linux-s390, kvm, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] vfio-ccw: Enable transparent CCW IPL from DASD 2020-04-17 18:29 ` [PATCH 1/1] " Jared Rossi 2020-04-20 12:13 ` Cornelia Huck @ 2020-04-23 13:56 ` Halil Pasic 2020-04-23 15:11 ` Cornelia Huck 1 sibling, 1 reply; 9+ messages in thread From: Halil Pasic @ 2020-04-23 13:56 UTC (permalink / raw) To: Jared Rossi; +Cc: Eric Farman, Cornelia Huck, linux-s390, kvm, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] vfio-ccw: Enable transparent CCW IPL from DASD 2020-04-23 13:56 ` Halil Pasic @ 2020-04-23 15:11 ` Cornelia Huck 2020-04-23 20:25 ` Eric Farman 0 siblings, 1 reply; 9+ messages in thread From: Cornelia Huck @ 2020-04-23 15:11 UTC (permalink / raw) To: Halil Pasic; +Cc: Jared Rossi, Eric Farman, linux-s390, kvm, linux-kernel 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 :( ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] vfio-ccw: Enable transparent CCW IPL from DASD 2020-04-23 15:11 ` Cornelia Huck @ 2020-04-23 20:25 ` Eric Farman 2020-04-24 12:50 ` Halil Pasic 0 siblings, 1 reply; 9+ messages in thread From: Eric Farman @ 2020-04-23 20:25 UTC (permalink / raw) To: Cornelia Huck, Halil Pasic; +Cc: Jared Rossi, linux-s390, kvm, linux-kernel 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] vfio-ccw: Enable transparent CCW IPL from DASD 2020-04-23 20:25 ` Eric Farman @ 2020-04-24 12:50 ` Halil Pasic 2020-04-29 0:38 ` Jared Rossi 0 siblings, 1 reply; 9+ messages in thread From: Halil Pasic @ 2020-04-24 12:50 UTC (permalink / raw) To: Eric Farman; +Cc: Cornelia Huck, Jared Rossi, linux-s390, kvm, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] vfio-ccw: Enable transparent CCW IPL from DASD 2020-04-24 12:50 ` Halil Pasic @ 2020-04-29 0:38 ` Jared Rossi 0 siblings, 0 replies; 9+ messages in thread From: Jared Rossi @ 2020-04-29 0:38 UTC (permalink / raw) To: Halil Pasic; +Cc: Eric Farman, Cornelia Huck, linux-s390, kvm, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-04-29 0:38 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-17 18:29 [PATCH 0/1] vfio-ccw: Enable transparent CCW IPL from DASD Jared Rossi 2020-04-17 18:29 ` [PATCH 1/1] " Jared Rossi 2020-04-20 12:13 ` Cornelia Huck 2020-04-24 13:02 ` Halil Pasic 2020-04-23 13:56 ` Halil Pasic 2020-04-23 15:11 ` Cornelia Huck 2020-04-23 20:25 ` Eric Farman 2020-04-24 12:50 ` Halil Pasic 2020-04-29 0:38 ` Jared Rossi
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).