Re: [qemu-s390x] [Qemu-devel] [PATCH 1/2] vfio-ccw: add force unlimited prefetch property

[Cornelia Huck]

On Mon, 14 May 2018 14:40:13 +0200
Halil Pasic <address@hidden> wrote:

> On 05/14/2018 02:18 PM, Cornelia Huck wrote:
> > On Thu, 10 May 2018 02:07:11 +0200
> > Halil Pasic <address@hidden> wrote:
> >   
> >> There is at least one control program (guest) that although it does not  
> > 
> > I'd drop 'control program' here as well, as it probably confuses more
> > than helps.
> >   
> 
> Will do (everywhere).
> 
> >> rely on the guarantees provided by ORB 1 word 9 bit (aka unlimited
> >> prefetch, aka P bit) not being set, fails to tell this to the machine.
> >>
> >> Usually this ain't a big deal, as the story is usually about performance
> >> optimizations only. But vfio-ccw can not provide the guarantees required
> >> if the bit is not set.  
> > 
> > Isn't that also about channel program rewriting? Or am I mixing things
> > up?
> >   
> 
> I don't understand the question. Can you rephrase it (maybe with more
> details)?

If the caller doesn't allow prefetching, it may manipulate parts of the
channel program that have not yet been fetched. For example, setting a
suspend flag and manipulating ccws that come after that. (I think the
ctc and lcs drivers do something like that, or at least did in the
past.)

> 
> >>
> >> Since it is impossible to implement support for P bit not set (at
> >> impossible least without transitioning to lower level protocols) in
> >> vfio-ccw let's provide a manual override.  
> > 
> > Hm... so the basic idea seems to be "we don't support !PFCH, but we
> > know that the guest will not rely on the guarantees, so we provide the
> > host admin with a way to override the setting"?
> >   
> 
> That is the idea, although I'm not sure what 'the setting' is.

Lack of coffee :) I meant 'handling'.

> 
> >>
> >> Signed-off-by: Halil Pasic <address@hidden>
> >> Suggested-by: Dong Jia Shi <address@hidden>
> >> Acked-by: Jason J. Herne <address@hidden>
> >> Tested-by: Jason J. Herne <address@hidden>
> >> ---
> >>   hw/s390x/css.c |  3 +--
> >>   hw/vfio/ccw.c  | 13 +++++++++++++
> >>   2 files changed, 14 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index 301bf1772f..32f1b2820d 100644
> >> --- a/hw/s390x/css.c
> >> +++ b/hw/s390x/css.c
> >> @@ -1196,8 +1196,7 @@ static IOInstEnding 
> >> sch_handle_start_func_passthrough(SubchDev *sch)
> >>        * Only support prefetch enable mode.
> >>        * Only support 64bit addressing idal.
> >>        */
> >> -    if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
> >> -        !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
> >> +    if (!(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
> >>           warn_report("vfio-ccw requires PFCH and C64 flags set");  
> > 
> > Adapt this warning?
> >   
> >>           sch_gen_unit_exception(sch);
> >>           css_inject_io_interrupt(sch);
> >> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> >> index e67392c5f9..32cf606a71 100644
> >> --- a/hw/vfio/ccw.c
> >> +++ b/hw/vfio/ccw.c
> >> @@ -32,6 +32,8 @@ typedef struct VFIOCCWDevice {
> >>       uint64_t io_region_offset;
> >>       struct ccw_io_region *io_region;
> >>       EventNotifier io_notifier;
> >> +    /* force unlimited prefetch */
> >> +    bool f_upfch;  
> > 
> > force_unlimited_prefetch? You only use it that often :)
> >   
> 
> I would have expected complaints for the property name in the
> first place. I think we should first find a good name for the
> property and then consider the rest.

What about 'force_pfch' (at least matches the name of the bit in the
code)?

> 
> >>   } VFIOCCWDevice;
> >>   
> >>   static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)
> >> @@ -52,8 +54,18 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev 
> >> *sch)
> >>       S390CCWDevice *cdev = sch->driver_data;
> >>       VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
> >>       struct ccw_io_region *region = vcdev->io_region;
> >> +    bool upfch = sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH;  
> > 
> > Frankly, I'd drop that variable...
> >   
> >>       int ret;
> >>   
> >> +    if (!upfch && !vcdev->f_upfch) {
> >> +        warn_report("vfio-ccw requires PFCH flag set");
> >> +        sch_gen_unit_exception(sch);
> >> +        css_inject_io_interrupt(sch);
> >> +        return IOINST_CC_EXPECTED;
> >> +    } else if (!upfch) {
> >> +        sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
> >> +    }  
> > 
> > and do
> > 
> > if (!(sch->orb.ctrl0 & ORB_CTR0_MASK_PFCH)) {
> >    if (!vcdev->f_upfch) {
> >      ...error...
> >    } else {
> >      ...set bit...
> >    }
> > }
> > 
> > Avoids discussions around variable naming, as well :)
> >   
> 
> Seems like more indentation and more lies of code to me, but
> no strong feelings. It may be easier to read.

Yes, I think this makes it easier to see which branch is taken.

> 
> >> +
> >>       QEMU_BUILD_BUG_ON(sizeof(region->orb_area) != sizeof(ORB));
> >>       QEMU_BUILD_BUG_ON(sizeof(region->scsw_area) != sizeof(SCSW));
> >>       QEMU_BUILD_BUG_ON(sizeof(region->irb_area) != sizeof(IRB));
> >> @@ -429,6 +441,7 @@ static void vfio_ccw_unrealize(DeviceState *dev, Error 
> >> **errp)
> >>   
> >>   static Property vfio_ccw_properties[] = {
> >>       DEFINE_PROP_STRING("sysfsdev", VFIOCCWDevice, vdev.sysfsdev),
> >> +    DEFINE_PROP_BOOL("f-upfch", VFIOCCWDevice, f_upfch, false),  
> > 
> > Any particular reason you want to control this on a device-by-device
> > level?
> >   
> 
> It seemed natural for me. What are our options here? I don't like
> machine property, as it is not a machine thing.

On the one hand, we want to accommodate certain guests; on the other
hand, the guest is free to address different devices in different ways
(although I would expect the difference to be more by different device
types).

In the end, it seems that a per-device property is the easiest approach
after all. (The admin can probably set this globally, if desired.)

Another thought: Should there be a warning logged somewhere if we
actually force pfch (i.e., not just set the property)?