On Fri, 15 May 2020 07:06:29 -0400
Eric Farman <far...@linux.ibm.com> wrote:
> On 5/14/20 2:39 PM, Jared Rossi wrote:
> > On 2020-05-14 11:20, Cornelia Huck wrote:
> >> On Tue, 12 May 2020 14:15:35 -0400
> >> Jared Rossi <jro...@linux.ibm.com> wrote:
> >>
> >>> Remove the explicit prefetch check when using vfio-ccw devices.
> >>> This check does not trigger in practice as all Linux channel programs
> >>> are intended to use prefetch.
> >>>
> >>> It is no longer required to force the PFCH flag when using vfio-ccw
> >>> devices.
> >>
> >> That's not quite true: Only kernels that include the currently-queued
> >> patch do not require it. Maybe
> >>
> >> "Newer Linux kernel versions do not require to force the PFCH flag with
> >> vfio-ccw devices anymore."
>
> I like it.
>
> >>
> >> ?
> >>
> >
> > This is a good point and your proposed message is reasonable.
I'll use it, then :)
> >
> >>>
> >>> Signed-off-by: Jared Rossi <jro...@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");
> >>> }
> >>>
> >>> QEMU_BUILD_BUG_ON(sizeof(region->orb_area) != sizeof(ORB));
> >>
> >> Let me spell out what happens:
> >> - PFCH bit set -> no change
> >> - PFCH bit not set, but force_orb_pfch set -> no change
> >> - neither PFCH bit nor force_orb_pfch set:
> >> - older kernels: QEMU makes the request, the kernel rejects it, guest
> >> gets a unit exception (same result for the guest as before, only a
> >> different code flow)
> >> - newer kernels: QEMU makes the request, the kernel forwards the
> >> request (logging a rate-limited warning); the result depends on
> >> whether the guest actually tries to rewrite the channel program or
> >> not
> >>
> >
> > This is correct, but I think it is worth noting that while the exception
> > is the same in the case of new QEMU + old kernel, the logging is different.
> > The old kernel code did not issue any warning if a non-prefetch ORB was
> > rejected, it simply raised the exception. In reality, the old kernel code
> > path was not accessible because QEMU would always reject ORBs before then
> > with the "requires PFCH flag set" message. The new QEMU code does not
> > issue a warning in this case.
> >
> > I considered keeping a warning for the non-prefetch path, but it seemed
> > excessive to me, since it causes a redundant warning when used with the
> > new kernel code (which I expect to be the case normally). Do you think
> > some sort of warning should still be issued by QEMU in this case, even
> > if it is redundant with the kernel warning?
>
> Hrm... Keeping the warning out of QEMU might be beneficial. Sure, when
> running with new kernels the message will be redundant, but if running
> with an old kernel the result will just be a silent error.
I don't think we need to care about that situation that much; I'd hope
that any distribution will pick both patches (or at least not the QEMU
patch without the kernel patch).
>
> >
> >> I think that is what we want, and I think I'll queue this patch with
> >> the tweaked commit message, but I'd like a second opinion.
>
> I don't have a strong opinion of the messaging, but think everything
> else looks fine. If you'd like to queue this patch with the tweaked
> commit message:
Ok, then I'll just go ahead and queue it.
>
> Reviewed-by: Eric Farman <far...@linux.ibm.com>
>
> >>
> >> (We should also deprecate force_orb_pfch in the future.)
>
> +1
>
