Re: [PATCH v2 1/1] vfio-ccw: allow non-prefetch ORBs

[Cornelia Huck]

On Fri, 15 May 2020 07:06:29 -0400
Eric Farman <address@hidden> 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 <address@hidden> 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 <address@hidden>
> >>> ---
> >>>  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 <address@hidden>
> 
> >>
> >> (We should also deprecate force_orb_pfch in the future.)  
> 
> +1
>