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

[Cornelia Huck]

On Thu, 24 May 2018 19:58:27 +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 not possible to implement support for the P bit not set without
> transitioning to lower level protocols for vfio-ccw.  So let's give the
> user the opportunity to force setting the P bit, if the user knows this
> is safe.  For self modifying channel programs forcing the P bit is not
> safe.  If the P bit is forced for a self modifying channel program things
> are expected to break in strange ways.
> 
> Let's also avoid warning multiple about P bit not set in the ORB in case
> P bit is not told to be forced, and designate the affected vfio-ccw
> device.
> 
> 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>

> +static inline void warn_once(bool *warned, const char *fmt, ...)
> +{
> +    va_list ap;
> +
> +    if (!warned || *warned) {
> +        return;
> +    }
> +    *warned = true;
> +    va_start(ap, fmt);
> +    warn_vreport(fmt, ap);
> +    va_end(ap);
> +}
> +
> +static inline void warn_once_pfch(VFIOCCWDevice *vcdev, SubchDev *sch,
> +                                  const char *msg)
> +{
> +    warn_once(&vcdev->warned_orb_pfch, "vfio-ccw (devno %x.%x.%04x): %s",
> +              sch->cssid, sch->ssid, sch->devno, msg);
> +}
> +

While I still think we want warn_once() in common error handling code,
this looks reasonable enough. We can still move it later.

>  static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)
>  {
>      vdev->needs_reset = false;
> @@ -54,6 +76,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_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");
> +        }
> +    }
> +

Looks good to me. I plan to queue this (and the other patch) to
s390-next, but (as always) further tags are still welcome :)