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

[Cornelia Huck]

On Mon, 20 Apr 2020 18:35:58 -0400
Jared Rossi <address@hidden> wrote:

> On 2020-04-20 08:29, Cornelia Huck wrote:
> > On Mon, 20 Apr 2020 14:26:17 +0200
> > Cornelia Huck <address@hidden> wrote:
> >   
> >> On Fri, 17 Apr 2020 14:38:38 -0400
> >> Jared Rossi <address@hidden> 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 <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");
> >> >      }  
> >> 
> >> 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.
>