| Message ID | 20200417183838.11796-2-jrossi@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio-ccw: Enable transparent CCW IPL from DASD | expand |
On Fri, 17 Apr 2020 14:38:38 -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. As already commented on the Linux patch: Can we log something, so this is debuggable if this statement does not hold true in the future? > > Signed-off-by: Jared Rossi <jrossi@linux.ibm.com> > --- > hw/vfio/ccw.c | 13 +++---------- > 1 file changed, 3 insertions(+), 10 deletions(-) > > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index 50cc2ec75c..e649377b68 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -74,16 +74,9 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch) > struct ccw_io_region *region = vcdev->io_region; > int ret; > > - if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) { > - if (!(vcdev->force_orb_pfch)) { > - warn_once_pfch(vcdev, sch, "requires PFCH flag set"); > - sch_gen_unit_exception(sch); > - css_inject_io_interrupt(sch); > - return IOINST_CC_EXPECTED; > - } else { > - sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH; > - warn_once_pfch(vcdev, sch, "PFCH flag forced"); > - } > + if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH) && vcdev->force_orb_pfch) { > + sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH; > + warn_once_pfch(vcdev, sch, "PFCH flag forced"); > } What happens when you run it with an old kernel? I guess the I/O is only rejected later (after a trip into the kernel), but has that path ever been tested? > > QEMU_BUILD_BUG_ON(sizeof(region->orb_area) != sizeof(ORB));
On Mon, 20 Apr 2020 14:26:17 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Fri, 17 Apr 2020 14:38:38 -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. > > As already commented on the Linux patch: Can we log something, so this > is debuggable if this statement does not hold true in the future? > > > > > Signed-off-by: Jared Rossi <jrossi@linux.ibm.com> > > --- > > hw/vfio/ccw.c | 13 +++---------- > > 1 file changed, 3 insertions(+), 10 deletions(-) > > > > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > > index 50cc2ec75c..e649377b68 100644 > > --- a/hw/vfio/ccw.c > > +++ b/hw/vfio/ccw.c > > @@ -74,16 +74,9 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch) > > struct ccw_io_region *region = vcdev->io_region; > > int ret; > > > > - if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) { > > - if (!(vcdev->force_orb_pfch)) { > > - warn_once_pfch(vcdev, sch, "requires PFCH flag set"); > > - sch_gen_unit_exception(sch); > > - css_inject_io_interrupt(sch); > > - return IOINST_CC_EXPECTED; > > - } else { > > - sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH; > > - warn_once_pfch(vcdev, sch, "PFCH flag forced"); > > - } > > + if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH) && vcdev->force_orb_pfch) { > > + sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH; > > + warn_once_pfch(vcdev, sch, "PFCH flag forced"); > > } > > What happens when you run it with an old kernel? I guess the I/O is > only rejected later (after a trip into the kernel), but has that path > ever been tested? > > > > > QEMU_BUILD_BUG_ON(sizeof(region->orb_area) != sizeof(ORB)); > Oh, and do we want to deprecate the force prefetch interface in the future? We probably need to wait a bit, until the kernel changes have become widely available.
On 2020-04-20 08:29, Cornelia Huck wrote: > On Mon, 20 Apr 2020 14:26:17 +0200 > Cornelia Huck <cohuck@redhat.com> wrote: > >> On Fri, 17 Apr 2020 14:38:38 -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. >> >> As already commented on the Linux patch: Can we log something, so this >> is debuggable if this statement does not hold true in the future? >> Agreed. I will work on debugging improvements so that any future issues related to unintended prefetching are more clearly logged. >> > >> > Signed-off-by: Jared Rossi <jrossi@linux.ibm.com> >> > --- >> > hw/vfio/ccw.c | 13 +++---------- >> > 1 file changed, 3 insertions(+), 10 deletions(-) >> > >> > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c >> > index 50cc2ec75c..e649377b68 100644 >> > --- a/hw/vfio/ccw.c >> > +++ b/hw/vfio/ccw.c >> > @@ -74,16 +74,9 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch) >> > struct ccw_io_region *region = vcdev->io_region; >> > int ret; >> > >> > - if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) { >> > - if (!(vcdev->force_orb_pfch)) { >> > - warn_once_pfch(vcdev, sch, "requires PFCH flag set"); >> > - sch_gen_unit_exception(sch); >> > - css_inject_io_interrupt(sch); >> > - return IOINST_CC_EXPECTED; >> > - } else { >> > - sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH; >> > - warn_once_pfch(vcdev, sch, "PFCH flag forced"); >> > - } >> > + if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH) && vcdev->force_orb_pfch) { >> > + sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH; >> > + warn_once_pfch(vcdev, sch, "PFCH flag forced"); >> > } >> >> What happens when you run it with an old kernel? I guess the I/O is >> only rejected later (after a trip into the kernel), but has that path >> ever been tested? >> Yes, this was tested and you are correct that the kernel will reject the I/O unless the corresponding patch is also applied there. I will revisit this path while I'm updating the logging to ensure that any potential interactions are appropriately considered. >> > >> > QEMU_BUILD_BUG_ON(sizeof(region->orb_area) != sizeof(ORB)); >> > > Oh, and do we want to deprecate the force prefetch interface in the > future? We probably need to wait a bit, until the kernel changes have > become widely available. Yes, I think we will want to deprecate it at an appropriate time in the future.
On Mon, 20 Apr 2020 18:35:58 -0400 Jared Rossi <jrossi@linux.ibm.com> wrote: > On 2020-04-20 08:29, Cornelia Huck wrote: > > On Mon, 20 Apr 2020 14:26:17 +0200 > > Cornelia Huck <cohuck@redhat.com> wrote: > > > >> On Fri, 17 Apr 2020 14:38:38 -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. > >> > >> As already commented on the Linux patch: Can we log something, so this > >> is debuggable if this statement does not hold true in the future? > >> > > Agreed. I will work on debugging improvements so that any future issues > related to unintended prefetching are more clearly logged. Great. > > >> > > >> > Signed-off-by: Jared Rossi <jrossi@linux.ibm.com> > >> > --- > >> > hw/vfio/ccw.c | 13 +++---------- > >> > 1 file changed, 3 insertions(+), 10 deletions(-) > >> > > >> > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > >> > index 50cc2ec75c..e649377b68 100644 > >> > --- a/hw/vfio/ccw.c > >> > +++ b/hw/vfio/ccw.c > >> > @@ -74,16 +74,9 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch) > >> > struct ccw_io_region *region = vcdev->io_region; > >> > int ret; > >> > > >> > - if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) { > >> > - if (!(vcdev->force_orb_pfch)) { > >> > - warn_once_pfch(vcdev, sch, "requires PFCH flag set"); > >> > - sch_gen_unit_exception(sch); > >> > - css_inject_io_interrupt(sch); > >> > - return IOINST_CC_EXPECTED; > >> > - } else { > >> > - sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH; > >> > - warn_once_pfch(vcdev, sch, "PFCH flag forced"); > >> > - } > >> > + if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH) && vcdev->force_orb_pfch) { > >> > + sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH; > >> > + warn_once_pfch(vcdev, sch, "PFCH flag forced"); > >> > } > >> > >> What happens when you run it with an old kernel? I guess the I/O is > >> only rejected later (after a trip into the kernel), but has that path > >> ever been tested? > >> > > Yes, this was tested and you are correct that the kernel will reject the > I/O unless > the corresponding patch is also applied there. I will revisit this path > while I'm > updating the logging to ensure that any potential interactions are > appropriately > considered. I've looked at the code again and it seems that the kernel will end up signaling -EOPNOTSUPP to us on that case, which causes the same unit exception as without this patch, so we should be all good. > > >> > > >> > QEMU_BUILD_BUG_ON(sizeof(region->orb_area) != sizeof(ORB)); > >> > > > > Oh, and do we want to deprecate the force prefetch interface in the > > future? We probably need to wait a bit, until the kernel changes have > > become widely available. > > Yes, I think we will want to deprecate it at an appropriate time in the > future. >
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 50cc2ec75c..e649377b68 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -74,16 +74,9 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch) struct ccw_io_region *region = vcdev->io_region; int ret; - if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) { - if (!(vcdev->force_orb_pfch)) { - warn_once_pfch(vcdev, sch, "requires PFCH flag set"); - sch_gen_unit_exception(sch); - css_inject_io_interrupt(sch); - return IOINST_CC_EXPECTED; - } else { - sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH; - warn_once_pfch(vcdev, sch, "PFCH flag forced"); - } + if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH) && vcdev->force_orb_pfch) { + sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH; + warn_once_pfch(vcdev, sch, "PFCH flag forced"); } QEMU_BUILD_BUG_ON(sizeof(region->orb_area) != sizeof(ORB));
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> --- hw/vfio/ccw.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)