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

[Halil Pasic]

On 05/23/2018 11:37 AM, Cornelia Huck wrote:
> On Wed, 23 May 2018 00:16:54 +0200
> Halil Pasic <address@hidden> wrote:
>
> > There is at least one guest (OS) such that although it does not rely on
> > the guarantees provided by ORB 1 word 9 bit (aka unlimited prefetch, aka
> > P bit) not being set, it fails to tell this to the machine.
> >
> > Usually this ain't a big deal, as the original purpose of the P bit is to
> > allow for performance optimizations. vfio-ccw however can not provide the
> > guarantees required if the bit is not set.
> >
> > It is impossible to implement support for P bit not set (at impossible
> > least without transitioning to lower level protocols) for vfio-ccw.
>
> "It is not possible to implement support for the P bit not set without
> transitioning to lower level protocols for vfio-ccw."
> > ?

Sounds much better. My sentence is ungrammatical.

> > So
> > let's give the user the opportunity to force the P bit to set, if the
>
> s/to set/to be set/

Why do we need the 'be'?

> > user knows this is safe.  For self modifying channel programs forcing the
> > P bit is not safe. If P bit is forced for a self modifying channel
>
> s/P bit/the P bit/

Right.

> > program things are expected to break in strange ways.
> >
> > 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>
> >
> > ---
> > v1 -> v2:
> > * reworded commit message
> > * re-factored the code (Connie)
> > * added warning when the P bit is forced the first time (Connie)
> >
> > ---
> >   hw/s390x/css.c |  3 +--
> >   hw/vfio/ccw.c  | 25 +++++++++++++++++++++++++
> >   2 files changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> > index 56c3fa8c89..39ae5bbf7e 100644
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -1204,8 +1204,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");
> >           sch_gen_unit_exception(sch);
> >           css_inject_io_interrupt(sch);
> > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> > index e67392c5f9..62de4c9710 100644
> > --- a/hw/vfio/ccw.c
> > +++ b/hw/vfio/ccw.c
> > @@ -32,8 +32,20 @@ typedef struct VFIOCCWDevice {
> >       uint64_t io_region_offset;
> >       struct ccw_io_region *io_region;
> >       EventNotifier io_notifier;
> > +    bool force_orb_pfch;
> > +    bool warned_force_orb_pfch;
> >   } VFIOCCWDevice;
> >   
> > +#define WARN_ONCE(warned, fmt...) \
> > +({\
> > +if (!(warned)) {\
> > +    warn_report((fmt));\
> > +} \
> > +warned = true;\
> > +})
>
> I think introducing a macro for the single user is overkill here.
>
> We might contemplate a generic "print this error once, controlled by
> this flag" functionality, if there are more users.

I would prefer keeping the macro. If this generic functionality comes
along it will be easier to spot the home-brewn counterpart. Also it's
easier to read IMHO. BTW the macro could be an inline function like:

 static inline void warn_once(bool *warned, const char *fmt, ...)
{
    va_list ap;


    if (!warned || *warned) {
        return;
    }
    *warned= true;
    va_start(ap, fmt);
    vreport(REPORT_TYPE_WARNING, fmt, ap);
    va_end(ap);
}

if that's better.

> > +
> > +
> > +
> >   static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)
> >   {
> >       vdev->needs_reset = false;
> > @@ -54,6 +66,18 @@ 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_report("vfio-ccw 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(vcdev->warned_force_orb_pfch, "PFCH flag forced");
>
> This message should probably mention vfio-ccw as well as the subchannel
> id?

I was thinking about this. I think all it would make sense to have a common
prefix for all reports coming form vfio-ccw (QEMU). But then I was like, that
is a separate patch.

Maybe something like:
vfio-ccw (xx.xx.xxxx): specific message

OTOH we don't seem to do that elsewhere (git grep -e 
'warn\|error_report\|error_setg' -- hw/s390x/).
AFAIR the error_setg captures context (like, src, line, func) but does not
necessarily report it. Another question is if this should be extended to
hw/s390x/s390-ccw.c

What do you think?


> > +        }
> > +    }
> > +
> >       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 +453,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("force-orb-pfch", VFIOCCWDevice, force_orb_pfch, false),
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> >   
>
> Otherwise, the changes look good.

I guess there will be a v3 then.

Thanks!

Halil