On Thu, 24 May 2018 19:58:27 +0200 Halil Pasic <pa...@linux.ibm.com> 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 <pa...@linux.ibm.com> > Suggested-by: Dong Jia Shi <bjsdj...@linux.ibm.com> > Acked-by: Jason J. Herne <jjhe...@linux.ibm.com> > Tested-by: Jason J. Herne <jjhe...@linux.ibm.com> > +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 :)